feat(canary): name the instance that fired a Slack alert (#1987) - #1997
Conversation
A Slack incoming webhook carries no sender identity, so two instances pointed at one channel emit indistinguishable alerts. `dev` and `eu2` now both post to the canary channel (the latter for the #1766 soak), and because a continuing-red invariant does not re-post, whatever the one-shot omits is not recoverable from a later message — an S-01 needs triaging as "the pilot" or "unrelated" from the notification itself. `services/instance_identity.py::get_instance_label()` resolves a short label: `TRINITY_INSTANCE_NAME` override -> first DNS label of `FRONTEND_URL`'s host (`https://eu2.example.com` -> `eu2`; an IP literal keeps its whole host, since the first label of `10.0.0.5` is `10`) -> `installation_id[:8]` -> `None`. `_build_slack_payload` renders it as a `[eu2]` prefix on both the Block Kit header and the `text` fallback — the fallback because that is what a mobile push actually shows. Tier 2 is the reason there is no new *required* var: managed instances already set `FRONTEND_URL` and both compose files already forward it, so attribution improves fleet-wide with no `.env` rollout. Every tier degrades to the next rather than raising — an unlabelled alert is today's behaviour, a lost alert is the failure the sink exists to prevent — and the resolver is a stdlib-only leaf, so the operator-queue and retention-guard alarms can reuse it verbatim if either grows a webhook. The label is sanitized at resolution *and* again at the render boundary, per `_mrkdwn_safe`'s own argument: `<!channel>` mass-pings the channel, non-ASCII alphanumerics let a homoglyph impersonate another instance in a message whose whole job is saying which instance sent it, and an over-long `header` is a 400 that drops the entire message while the transition still counts as alerted (the #1880 silently-lost-alert mode). The label is resolved on the send path, not inside the composer, so `_build_slack_payload` stays a pure function of its arguments and the render path never touches env or the DB. Webhook URL handling is untouched.
|
Resolve by running |
) /review caught that `_label_from_installation_id` calls `get_or_create_installation_id`, which mints and persists the UUID on a miss — so on an install that never completed operator intake, rendering the first canary alert is what creates the id. The behaviour is the right one (a read-only tier would return None on exactly the un-configured OSS install tier 3 exists to label), but it was undocumented, which is the actual defect. Also records the inherited race: `canary_service` has no leader lock, unlike monitoring (#1464) and operator-queue (#1632), so under `--workers 2` both workers can hit the accessor's read-then-write. Cost is bounded to a differing 8-char label across one cycle on a fresh install, and the race is pre-existing in the accessor. Comments and one learnings entry only — no behaviour change.
…ert-instance-label # Conflicts: # docs/memory/learnings.md
dolho
left a comment
dolho
left a comment
There was a problem hiding this comment.
Review — instance label on canary Slack alerts
Good shape overall. The three things that matter most are right: no new required var (tier 2 off FRONTEND_URL makes this work fleet-wide on day one), sanitization at the render boundary as well as at resolution, and the var wired into both composes rather than only .env.example. The _mrkdwn_safe-style "re-verify where the value crosses into the payload" argument is the correct one.
Four notes, none blocking on their own.
1. Tier 2 collides on the most common hostname shape
First-DNS-label works for eu2.abilityai.dev / dev.abilityai.dev because the ops slug happens to be the first label. It does not work for the shape most installs actually have: trinity.acme.com and trinity.acme.eu (or a prod/DR pair behind the same first label) both resolve to trinity.
That failure mode is worse than the one this PR fixes: an unlabelled alert is honestly ambiguous, a [trinity] label on both instances looks attributed and is silently wrong — in the one message whose entire job is saying which instance sent it. Given you have 32 chars of budget and the longest current header is 57, consider using more of the host (two labels, or the full host truncated) and reserving the single-label form for the override. At minimum, call the collision out in the docstring so an operator hitting it knows to set TRINITY_INSTANCE_NAME.
2. : in the allowed set + "emoji": True on the header
_EXTRA_ALLOWED_CHARS = "._-: " keeps colons, and the header block sets "emoji": True, so a label like :fire: renders as an emoji rather than as literal text. Not a security issue (plain_text won't take <!channel>, and the mrkdwn text fallback is already covered by the filter), but a label that silently becomes a picture is the wrong outcome for an identity string. Nothing in a hostname needs : except a port, which you explicitly don't want in a label — dropping it costs nothing.
3. Tier 3 takes a DB write dependency on the alert path
get_or_create_installation_id() mints and persists when absent, so the first canary alert on an un-intook install performs a write. You document this, including the --workers 2 race, which is the right instinct — but the mitigation is cheaper than the disclosure: a read-only accessor returning None keeps the sink pure. The install this tier is meant to rescue (no TRINITY_INSTANCE_NAME, no FRONTEND_URL) is a bare local dev box, which is exactly the install where an 8-hex label buys the least. Worth reconsidering; if you keep the write, keep the docstring.
4. Nit — prefix position differs between the two surfaces
text = f"{prefix}{emoji} canary {invariant_id} ..."
header= f"{emoji} {prefix}{invariant_id} {name} — {severity}"Fallback leads with the label (right for a lock screen), header leads with the emoji. Probably deliberate; say so in the docstring or align them, otherwise it reads as a slip.
Merge order
Touches canary_alerts.py, as do #1879 and #2022. Pick an order across the three and rebase rather than resolving three-way at merge.
2 participants
Closes #1987
What
Canary green→red Slack alerts now carry a short label naming the instance that fired them — a
[eu2]prefix on both the Block Kit header and thetextfallback.Why
A Slack incoming webhook carries no sender identity, so two instances pointed at one channel emit indistinguishable alerts.
devandeu2both post to the canary channel now (the latter for the #1766 pull/work-stealing soak). Because a continuing-red invariant does not re-post, each alert is a one-shot — whatever it omits is not recoverable from a later message. Triaging an S-01 as "eu2, the pilot" vs "dev, unrelated" has to be possible from the notification itself.The prefix goes on the
textfallback as well as the header because the fallback is what a mobile push notification actually renders — the lock-screen case is the one that motivated the ticket.How
New
src/backend/services/instance_identity.py—get_instance_label(), resolution order per AC:TRINITY_INSTANCE_NAME(new, optional, unset by default)eu2FRONTEND_URL's hosthttps://eu2.example.com→eu2installation_id[:8]a1b2c3d4None→ today's unlabelled payloadTier 2 is why there is no new required var: managed instances already set
FRONTEND_URLand both compose files already forward it, so attribution improves fleet-wide without an.envrollout. An IP literal keeps its whole host — the first label of10.0.0.5is10, which is worse than no label because it looks like a name.Every tier degrades to the next rather than raising (the tier-3 DB read is lazily imported and broadly wrapped). An unlabelled alert is the prior behaviour; a lost alert is the failure the sink exists to prevent. The resolver is a stdlib-only leaf, so the operator-queue and retention-guard alarms can reuse it verbatim if either grows a webhook — the ticket's "worth putting somewhere reusable" note.
The label resolves on the send path, not inside the composer, so
_build_slack_payloadstays a pure function of its arguments and the render path never touches env or the DB.Safety
Sanitization (ASCII-alnum + hostname punctuation
._-:, 32-char cap) runs at resolution and again at the render boundary — the same argument_mrkdwn_safealready makes for itself:<!channel>mass-pings everyone in the alert channel;<url|text>renders as a live link. Neither survives the filter.str.isalnum()is true for Cyrillic, so allowing it would let a homoglyph label impersonate another instance in a message whose entire job is saying which instance sent it.headerwith a 400 that drops the whole message while the transition is still recorded as alerted — the Canary: 5 Phase-4 invariants alert with no name, runbook, or detail — including the critical credential-leak check (G-04) #1880 silently-lost-alert mode. 32 chars puts the longest current header at ~92 of Slack's 150.A garbage
TRINITY_INSTANCE_NAMEsanitizes toNoneand falls through to the derived label rather than silently costing the alert its attribution.Webhook URL handling is untouched — still never logged or echoed (AC #4).
Test plan
tests/unit/test_1987_instance_label.py— every resolution branch incl. the all-absent fallback, override precedence, garbage-override fall-through, IP literals (v4/v6), scheme-less hosts, ports, sanitization (markup injection, homoglyphs, length cap), and end-to-end payload assertions that two instances produce visibly distinct alerts.Docs
docs/memory/requirements/infrastructure.md— CANARY-001 instance-attribution requirementdocs/memory/architecture.md— Canary Harness subsystem block + service catalog entry.env.example,docker-compose.yml,docker-compose.prod.yml—TRINITY_INSTANCE_NAMEwired in both composes (an env var set only in.env.exampleis inert in prod — the #1039 packaging-gap class)🤖 Generated with Claude Code