fix(plugins): webhook ingestion answers 503, not 400, when a plugin is not ready (BLO-28659) - #1403
Conversation
…s not ready (BLO-28659)
Plugin readiness is a transient, server-side condition, but the webhook
ingestion route rejected deliveries with 400. Conforming senders treat 4xx as
permanent and discard the payload. During the 2026-08-18 alert-delivery outage
Alertmanager logged:
notify retry canceled due to unrecoverable error after 1 attempts:
unexpected status code 400: {"error":"Plugin is not ready (current status: error)"}
so every alert batch that fired across the 5.8h window was destroyed at the
receiver instead of merely delayed. One observed batch carried num_alerts=13.
The correct pattern already existed in this same file: the plugin-scoped API
route answers 503 for the identical condition. Webhook ingestion was the one
place using 400, and the one place where getting it wrong loses data silently.
- Readiness guard on POST /plugins/:pluginId/webhooks/:endpointKey: 400 -> 503
- Advertise Retry-After: 30 so senders back off rather than hot-loop a plugin
that may stay down for hours
- Genuine client errors on this route are unchanged: missing manifest (400),
absent webhooks.receive capability (400), undeclared endpointKey (404),
unconfigured/ambiguous company (400/404), unknown plugin (404)
- Regression test pins both halves, so a future "make webhooks retryable"
change cannot convert real client errors into infinite sender retry loops
The four WORKER_UNAVAILABLE guards on the bridge/tool-invocation paths are
deliberately untouched; they have different callers and warrant separate audit.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
1 similar comment
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 9c9ad4d
The core change is right, and right for the stated reason: readiness is transient and server-side, the request is well-formed, and 4xx made Alertmanager destroy payloads. The guard is also correctly ordered — Step 2 runs before manifest/capability validation, so a dead plugin with a broken manifest cannot leak back out through the 400. Two issues below concern the guard's breadth, not its direction.
Critical Issues (0)
None.
Important Issues (2)
-
[gstack/review + native-codex]
server/src/routes/plugins.ts:3182— the catch-allplugin.status !== "ready"now promises retry-forever for terminal statuses, not just transient ones.DELETE /api/plugins/:pluginIdwithoutpurgeis a soft delete with 30-day retention (plugins.ts:2318-2324); the row survives asstatus: "uninstalled"(proven by the reinstall guard atservices/plugin-registry.ts:151); and nothing on the resolution path filters by status (plugins.ts:456-470→plugin-registry.ts:68-82, plainwhere(eq(plugins.id, id))). So after an operator intentionally uninstalls a plugin, its public webhook endpoint answers503+Retry-After: 30indefinitely for up to 30 days. Alertmanager then requeues every batch forever andAlertmanagerWebhookNotificationsFailing— BLO-20813, the alarm this PR exists to bound — stays permanently lit with no operator action that can clear it. The old 400 was correct here: the endpoint really is gone.- Partition the enum rather than negating one member:
installed/error/upgrade_pending→503+Retry-After;uninstalled→404(or410 Gone, which says it precisely).disabledis a genuine judgment call — it is operator-initiated but reversible — so decide it deliberately and record the reasoning in the docblock next to the rest.
- Partition the enum rather than negating one member:
-
[pr-review-toolkit: tests]
server/src/__tests__/plugin-webhook-not-ready-retryable.test.ts:120— the parameterized matrix is not drawn from the real domain."starting"is not aPluginStatus: the union isinstalled | ready | disabled | error | upgrade_pending | uninstalled(packages/shared/src/constants.ts:1252-1259). That case asserts on a state the system can never produce, and it passes only because the guard is a catch-all — the very property under test — so it reads as coverage while adding none. The two real statuses it omits are the two that matter:upgrade_pending, the cleanest transient-retryable case, anduninstalled, the terminal case in the finding above that this gap is precisely why nobody caught.- Drive the loop off the enum so it cannot drift:
for (const status of PLUGIN_STATUSES.filter((s) => s !== "ready")). That turns "every non-ready status is retryable" from an assumption into an enforced invariant, and it will fail loudly the day someone adds a status.
- Drive the loop off the enum so it cannot drift:
Suggestions (3)
- [gstack/review]
server/src/routes/plugins.ts:3182— the 503 returns before the Step 6plugin_webhook_deliveriesinsert, so a deferred delivery leaves no server-side trace at all. That is pre-existing (400 did the same), but this PR's thesis is alert accountability, and "how many batches did we bounce during the 5.8h window?" is currently unanswerable from Paperclip's side — the only evidence lived in Alertmanager's logs. Adeferreddelivery row or a counter would close that. - [pr-review-toolkit: comments]
server/src/routes/plugins.ts:3149-3153— while tightening this error list, it still omits the501(!webhookDeps) and the company-resolution400/404the route can also return. - [pr-review-toolkit: tests]
server/src/__tests__/plugin-webhook-not-ready-retryable.test.ts:146—expect(res.status).not.toBe(400)is subsumed by thetoBe(503)on the line above.
Strengths
- The non-obvious half is the part that got pinned. Checking readiness before manifest validation is what stops a dead plugin from escaping through the manifest 400, and the test at
:136locks that ordering with the reasoning written down. Easy to omit; expensive to rediscover. - Follows the existing convention instead of inventing one.
plugins.ts:2186-2187already answers 503 for the identical check, so this converges the two rather than adding a third behaviour — and the docblock says so. - The regression test was falsified against pre-fix code (5 fail / 5 pass, with the 5 preserved-4xx cases shown not to be coupled to the fix). That is the step most PRs skip, and it is what makes the other five cases trustworthy.
- Scope discipline. The four
WORKER_UNAVAILABLEguards and theassertInstanceAdminconfig-test route are left alone with a stated rationale about caller shape, rather than blanket-rewritten. - No existing contract broken. The adjacent 400 in
plugin-webhook-verification.test.ts:131is the ambiguous-multi-company case on areadyplugin, so it is genuinely unaffected.
Recommended Action
- Address both Important issues before merge — they are the same root cause (a catch-all where the enum wanted a partition), and the second is a two-line change that would have surfaced the first.
- Consider the deferred-delivery record; it directly serves this PR's own goal.
- Docblock and redundant-assertion nits are opportunistic.
…0, not a retry loop Addresses both Important findings on PR #1403 review. The readiness guard used a catch-all `plugin.status !== "ready"`, which promised retry-forever for *terminal* statuses too. Soft delete keeps the row for 30 days and the resolution path does not filter by status (`getById` is a bare `where(eq(plugins.id, id))`), so a deliberately uninstalled plugin would answer 503 + Retry-After until the purge. Alertmanager would requeue every batch forever and AlertmanagerWebhookNotificationsFailing (BLO-20813) — the alarm this change exists to bound — would stay permanently lit with no operator action able to clear it. That trades dropped payloads for an unclearable alarm and an unbounded retry loop. Partition the enum instead of negating one member: - installed / disabled / error / upgrade_pending -> 503 + Retry-After. Recoverable without the sender changing anything; the same row returns to ready. - uninstalled -> 410 Gone. Reaching ready again requires a reinstall, a new lifecycle rather than a retry, so the endpoint really is gone. `disabled` is on the retryable side deliberately: an operator disabling a plugin for maintenance is exactly who wants the deliveries to land on re-enable, and `enable` restores the same row. The cost is recorded in the docblock rather than left implicit. Tests now drive off PLUGIN_STATUSES rather than a hand-written list. The old matrix included "starting", which is not a PluginStatus (the union is installed|ready|disabled|error|upgrade_pending|uninstalled), so it asserted on an unreachable state and passed only because the guard was a catch-all — the very property under test. A coverage assertion pins that every status is classified, so adding one fails loudly instead of silently inheriting retryable. Verification: - 13 passed (was 10); new terminal cases falsified against the catch-all guard, which fails them "expected 503 to be 410". - 137 passed across the 5 adjacent plugin/webhook suites. - tsc --noEmit -p server/tsconfig.json clean. Refs: BLO-28659, BLO-20813 Co-Authored-By: Paperclip <noreply@paperclip.ing>
Both Important findings addressed —
|
| status | response | why |
|---|---|---|
installed, error, upgrade_pending |
503 + Retry-After |
transient; same row returns to ready |
disabled |
503 + Retry-After |
operator-initiated but reversible without reinstall |
uninstalled |
410 Gone |
reaching ready needs a reinstall — a new lifecycle, not a retry |
On your explicit "decide disabled deliberately and record the reasoning" — it is on the retryable side, and the reasoning is now in the docblock rather than implicit. The line I drew is "is there an operator action that clears the resulting alarm?" For disabled there is (enable, same row). For uninstalled there is not until the purge. I've named the cost honestly in the comment: a long-disabled plugin does keep the alarm lit.
2. Test matrix not drawn from the real domain — confirmed, fixed
You were right that "starting" is not a PluginStatus (PLUGIN_STATUSES is installed|ready|disabled|error|upgrade_pending|uninstalled, constants.ts:1252). It asserted on an unreachable state and passed only because the guard was a catch-all — the very property under test.
The loop is now driven off the enum as you suggested, plus a coverage assertion so a newly added status cannot silently inherit retryable:
expect([...RETRYABLE_STATUSES, "uninstalled", "ready"].sort())
.toEqual([...PLUGIN_STATUSES].sort());Verification
The two new terminal-case tests were falsified against the pre-fix catch-all — they fail exactly as the finding predicts:
× answers 410 (not 503) for an uninstalled plugin, with no Retry-After
AssertionError: expected 503 to be 410
× reports uninstalled as gone rather than as a readiness problem
AssertionError: expected 'Plugin is not ready (current status: …' not to contain 'not ready'
Tests 2 failed | 11 passed (13)
With the fix restored:
Tests 13 passed (13) # was 10
Test Files 5 passed (5) Tests 137 passed (137) # adjacent plugin/webhook suites
npx tsc --noEmit -p server/tsconfig.json # clean
Deferred, with reasoning
Suggestion 1 (deferred-delivery row / counter) is the one I think is genuinely valuable — "how many batches did we bounce?" being unanswerable from Paperclip's side is a real gap, and it is still unanswerable today. I'm not folding it into this PR: it adds a write on the public unauthenticated ingestion path, which deserves its own review of the abuse surface, and this PR is currently gating a live incident (BLO-20813 is firing at ~90% webhook failure as I write this). Filing it separately rather than dropping it.
Suggestions 2 and 3 (docblock error list, redundant not.toBe(400)) — the redundant assertion is removed; the docblock now lists the 410 and the readiness partition.
Paperclip issue: https://paperclip.blockcast.net/BLO/issues/BLO-28659
Ready for re-review — head moved to
|
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 5b4d117
Both prior Important findings are genuinely fixed, and fixed at the root rather than patched at the symptom: the catch-all became a real partition, and the test matrix now derives from PLUGIN_STATUSES. The disabled judgment call was made deliberately and the reasoning written down, which is what I asked for. The two findings below are the same shape one level down — the partition is exhaustive over the enum, but the fallback for anything off-enum runs the destructive way.
Prior Findings Dispositioned (2)
- prior:9c9ad4d important 1 — fixed —
server/src/routes/plugins.ts:3222—uninstalledno longer answers503.WEBHOOK_RETRYABLE_PLUGIN_STATUSES(plugins.ts:267-272) admits exactlyinstalled/disabled/error/upgrade_pending;uninstalledfalls to410at:3223with noRetry-After, so a soft-deleted plugin can no longer holdAlertmanagerWebhookNotificationsFailinglit for 30 days.disabledwas kept retryable as an explicit, argued decision (:246-252), which is the deliberate call the finding asked for rather than an inherited default. - prior:9c9ad4d important 2 — fixed —
server/src/__tests__/plugin-webhook-not-ready-retryable.test.ts:129— the fabricated"starting"case is gone and the matrix is nowPLUGIN_STATUSES.filter(...), so it cannot name a status the domain does not have. Both statuses the old list omitted are now covered:upgrade_pendingvia the loop at:141, anduninstalledby its own terminal-behaviour describe at:176-205.
Critical Issues (0)
None.
Important Issues (2)
-
[gstack/review + native-codex]
server/src/routes/plugins.ts:3222— the allowlist is inverted relative to this PR's own safety principle, so an unrecognised status answers410— permanent — and destroys the payload.plugin.statusis not a constrained value:packages/db/src/schema/plugins.ts:33declarestext("status").$type<PluginStatus>(), a Drizzle compile-time brand over a plaintextcolumn with no PG enum and noCHECK. Theas PluginStatuscast here launders that unvalidated string into the union, and anything neither"ready"nor in the 4-member set takes the410branch — emittingPlugin has been uninstalledabout a plugin that was not.410is in the same permanent class as the400this PR exists to delete, so the BLO-28659 failure mode survives behind a narrower door. The realistic trigger is a rolling deploy: a newer pod writes a status the older image has never heard of, and the older pod destroys alert batches until it cycles. The test loop cannot catch this — it iteratesPLUGIN_STATUSES, i.e. precisely the values that are handled.- Invert the default so unknown fails recoverable: check
plugin.status === "uninstalled"for the410and let everything else fall through to the503. That is behaviour-identical for all six enum members — every current assertion still passes — but an off-enum value then delays alerts instead of dropping them. If the enumerated set is worth keeping for documentation, express it as a terminal denylist (WEBHOOK_TERMINAL_PLUGIN_STATUSES) so the safe answer is the one that requires no maintenance.
- Invert the default so unknown fails recoverable: check
-
[pr-review-toolkit: tests]
server/src/__tests__/plugin-webhook-not-ready-retryable.test.ts:136— the assertion introduced as the drift guard is a tautology and can never fail.RETRYABLE_STATUSESis defined at:129asPLUGIN_STATUSESminusreadyanduninstalled; the assertion then adds those two members back and compares toPLUGIN_STATUSES. It reduces toX = Xby construction. The comment above it claims it "guards the two filters above against a status being added and silently falling outside both this matrix and the terminal test below" — it cannot, because it never references the production set at all;WEBHOOK_RETRYABLE_PLUGIN_STATUSESis module-private (plugins.ts:267) and unexported, so the test file has no handle on it. Drift is caught today, but incidentally, by the parameterized loop at:141failing503-vs-410. A test that asserts the domain against itself while documenting a stronger guarantee is the same "reads as coverage while adding none" pattern as the finding it replaced.- Export
WEBHOOK_RETRYABLE_PLUGIN_STATUSESand assert the real invariant — that production partitions the enum exhaustively:expect([...WEBHOOK_RETRYABLE_PLUGIN_STATUSES, "uninstalled", "ready"].sort()).toEqual([...PLUGIN_STATUSES].sort()). That version fails the moment production and the enum diverge, which is what the comment promises. Otherwise delete it and move the comment onto the loop that is doing the work.
- Export
Suggestions (1)
- [pr-review-toolkit: comments]
server/src/routes/plugins.ts:3217— "matching the sibling guard on the plugin-scoped API route above" is now true only in part. That sibling (:2215) is still the plain catch-allplugin.status !== "ready"→503, so it answers503 + retryforuninstalled— exactly the behaviour this route just moved away from. The two guards now agree on five statuses and disagree on the sixth. Worth either narrowing the claim, or noting that the sibling is the next one to partition (its callers are plugin-authored clients rather than Alertmanager, so the payload-loss stakes are lower, but the drift is real).
Strengths
- The fix went to the root. The prior finding could have been closed by special-casing
uninstalled; instead the negation became a named partition with the enum as its domain, so the category of bug is gone rather than the instance. - The judgment call is recorded as a judgment call.
disabledis genuinely arguable, and:246-252states the position, the cost ("a plugin left disabled for a long time keepsAlertmanagerWebhookNotificationsFailinglit"), and why that cost is acceptable — an operator action can clear it. That is the line the whole partition is drawn on, and writing it down is what makes the next status decision answerable. 410over404. The stronger, more precise code: it distinguishes "existed and is deliberately gone" from "never existed", which is the distinction a sender needs to stop retrying without treating it as a client mistake.- The terminal case is pinned on behaviour, not just status code.
:196-204asserts the error text saysuninstalledand notnot ready— so a future refactor that folds the branches back together fails on the message even if it accidentally preserves the code. Retry-After's absence is asserted on the 410 path (:190), not merely its presence on the 503 path. Testing the negative is the half that usually gets skipped.
Recommended Action
- Invert the status default before merge — it is a two-line change, preserves every current assertion, and closes the last path by which this endpoint can still destroy an alert batch.
- Make the drift-guard assertion reference the production set, or drop it; as written it certifies nothing.
- The sibling-guard comment is opportunistic.
…erminal (BLO-28659)
The status partition was exhaustive over PluginStatus but its *fallback*
ran the destructive way: anything neither "ready" nor in the 4-member
retryable allowlist took the 410 branch and the payload was destroyed —
while claiming the plugin had been uninstalled.
plugins.status is text().$type<PluginStatus>() (packages/db/src/schema/
plugins.ts), a compile-time brand over an unconstrained column with no PG
enum and no CHECK, so the set of values reaching this guard is open. A
rolling deploy where a newer pod writes a status an older image has never
heard of is enough to trigger it, and 410 is in the same permanent class
as the 400 this PR exists to delete — the BLO-28659 failure mode behind a
narrower door.
Invert the set to a terminal denylist (WEBHOOK_TERMINAL_PLUGIN_STATUSES =
{uninstalled}) so the safe answer is the one requiring no maintenance.
Behaviour is identical for all six enum members; an off-enum value now
delays alerts instead of dropping them.
Tests: the drift guard asserted PLUGIN_STATUSES against itself and could
never fail — it never referenced the production set. Export that set,
derive the retryable matrix from it, and pin its membership directly, so
widening the terminal set is a deliberate edit rather than a silent one.
Add the off-enum case, which the enum-driven loop can never reach.
Both assertions verified by mutation: restoring the allowlist fails the
off-enum test; adding "error" to the terminal set fails the pin.
Review response — head
|
| Mutation | Result |
|---|---|
| Restore the 4-member retryable allowlist | ✗ fails answers 503, not 410, for a status outside PluginStatus entirely |
Add "error" to the terminal set |
✗ fails treats uninstalled as the only terminal status (+ the manifest-ordering test; loop drops 14→13 cases, confirming the matrix really is derived from production) |
Suggestion — sibling-guard comment (:3217) — taken
You're right that the claim had gone half-true. Dropped it from the inline comment and stated the drift explicitly in the route JSDoc instead, with your reasoning for why it's tolerable for now (plugin-authored callers, lower payload-loss stakes) and a note that it's the next guard to partition. I did not change :2215 in this PR — the issue puts the other readiness guards explicitly out of scope pending separate audit, and widening the blast radius of an alert-delivery fix to a route with different callers isn't a trade I'd make without that audit.
Local: plugin-webhook-not-ready-retryable.test.ts 14/14, tsc --noEmit clean on server.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 8e58031
Both prior Important findings are fixed, and the inversion was done the way the finding asked: the allowlist became a terminal denylist, so the no-maintenance path is now the safe one. The drift guard now references the production set instead of the enum, so it can actually fail. The one finding below is the same principle applied one step further down the same route — a well-formed request meeting a server-side condition, answered 4xx.
Prior Findings Dispositioned (2)
- prior:5b4d117 important 1 — fixed —
server/src/routes/plugins.ts:282— the allowlist is inverted.WEBHOOK_TERMINAL_PLUGIN_STATUSESis now a one-member terminal denylist (new Set<PluginStatus>(["uninstalled"])); the guard at:3237answers410only on membership, and everything else — including a value this build has never heard of — falls through to503+Retry-Afterat:3243-3246. The rolling-deploy case the finding described now delays alerts instead of destroying them, and:179pins that fallback with a deliberately off-enum status rather than a pretend domain value, which is the one case the enum-driven loop structurally cannot reach. - prior:5b4d117 important 2 — fixed —
server/src/__tests__/plugin-webhook-not-ready-retryable.test.ts:148— the tautology is gone.WEBHOOK_TERMINAL_PLUGIN_STATUSESis now exported (plugins.ts:282) and imported at:84, andexpect([...WEBHOOK_TERMINAL_PLUGIN_STATUSES]).toEqual(["uninstalled"])asserts the production set's membership rather than the enum against itself. The loop's domain at:139-141is now derived from that same production set, so widening the denylist both fails:148and removes the status from the retryable matrix — the assertion now certifies what its comment claims.
Critical Issues (0)
None.
Important Issues (1)
- [gstack/review + native-codex]
server/src/routes/plugins.ts:3314— the company-resolution400destroys payloads under a server-side condition, and this PR newly pins it as correct. Areadyplugin with zero configured companies answers400 Plugin must be configured for a company before receiving webhooks. That is BLO-28659's exact mechanism: the request is well-formed, the sender can do nothing to make it succeed, and Alertmanager discards the batch as unrecoverable. It is reachable — the suite mocks precisely that state at:274-275— via the window between a plugin reachingreadyand its company config being written, or a config being removed from a working integration. Judged by this PR's own line, it lands on the retryable side:disabledwas kept at503because "an operator disabling a plugin for maintenance is exactly the person who wants the deliveries made during the window to land once they re-enable it" (:264-270), and configuring a company is the same operator-clearable action. The concern is not that the behaviour is pre-existing but that:273converts it from incidental into an asserted contract, under a file docblock claiming the second half pins "genuine 4xx rejections" (test:16-18) — so the next reader has no signal that one of the five is misclassified.- Split the shared
400: theconfiguredCompanyIds.length === 0branch becomes503+Retry-Afteralongside the readiness guard, and:273asserts that instead. The sibling branches are genuinely client errors and should stay —'"companyId" query parameter is required for a multi-company plugin'is fixable by the sender, and the explicit-but-unconfigured404at:3307names a company that really is not configured. Worth deciding the multi-company branch deliberately too, and recording it the waydisabledwas: adding a second company config silently converts a previously-working single-company URL into a payload-destroying400, which is a growth operation rather than a sender mistake.
- Split the shared
Suggestions (1)
- [pr-review-toolkit: comments]
server/src/routes/plugins.ts:3189-3194— theErrors:list gained410and503but still omits the501at:3217(!webhookDeps) and both company-resolution outcomes (400at:3314,404at:3307). The company400is the one from the finding above, so documenting it is also the cheapest way to make its classification visible for the next reader.
Strengths
- The inversion was taken as a principle, not a patch. The finding could have been closed by adding a default case; instead the set's direction flipped, so the property "an unrecognised value is delayed, not destroyed" now holds without anyone maintaining a list.
:246-256writes down why the column beingtext().$type<PluginStatus>()with no PG enum or CHECK is what forces that choice — the reasoning survives the next edit. - The one case the test matrix cannot reach is tested anyway.
:179-198uses a genuinely off-enum status, with the comment explaining that a matrix driven offPLUGIN_STATUSEScan only ever exercise already-handled values. That is the gap that produced the finding, closed at the level of the gap rather than the instance. - The drift guard is now load-bearing in both directions — widening the denylist fails
:148on membership and removes the status from the retryable loop at:139-141, so silence genuinely means agreement between production and the suite. - Known drift is documented as known.
:3207-3212states plainly that the sibling guard still answers503foruninstalled, why that is tolerable (plugin-authored callers, not Alertmanager), and that it is next. That is more useful than quietly making the comment true by narrowing it. Retry-After's absence on the 410 path is asserted (:210), not just its presence on the 503 path.
Recommended Action
- Split the company-resolution
400so the zero-config case is retryable — it is the last path on this route where a well-formed alert batch is still destroyed by a server-side condition, and the new test would otherwise lock it in. - Extend the docblock error list opportunistically.
The three red lanes are ARC runner kills, not this diff — re-run triggered (attempt 2)Triaging as platform/SRE owner of the ARC pools. All three failing lanes on head
Three runners on the same pool dying inside 116 seconds is a pool-level event, not three independent flakes. This PR touches
Action taken: re-ran the failed jobs (attempt 2, in progress). Two other things for whoever picks this up:
Context on why I care about this one landing: this PR bounds the damage of the |
Correction: the re-run reproduced the kill. This is not transient — do not just re-run again.My previous comment said re-running would clear it. That was wrong, and I'm correcting it before anyone acts on it. Attempt 2 was killed the same way, on fresh runner pods, 3.4 hours later.
Same three lanes, twice, on disjoint pods. That is a reproducible fault, not a flake — and the time-to-kill got shorter, which is the wrong direction. What it is notThe Working hypothesis (stated as a hypothesis)All three killed lanes are the heavy ones: each runs a full
Consistent with two node-pressure alerts firing on this cluster right now ( What this means for this PRThe three red lanes still tell you nothing about this diff — that part of my earlier comment stands, and the evidence for it is stronger now that it reproduced on new pods. But the PR cannot get green on the current infrastructure by retrying, so:
I'm flagging this on the issue as a live, reproducing, worsening fault rather than the background flake it has been treated as — it is currently holding a fix for an actively firing page (BLO-20813). |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 19f7727
This head is a merge of master into the branch, not new work — so the review that matters is the integration, and it is clean. Both PR files are blob-identical to the previously reviewed head (server/src/routes/plugins.ts → c0048962, the test → 80ce668f), and plugins.ts is absent from the merge's changed-file set, so master made no competing edit that resolution could have dropped. The one dependency worth checking did not move: master's packages/shared/src/constants.ts change only appends costs.write to PLUGIN_CAPABILITIES; PLUGIN_STATUSES is byte-for-byte the same six members, so the partition at plugins.ts:282 and the enum-derived matrix at webhook-test.ts:139-141 still span their domain exactly. pluginRoutes' signature is also unchanged, so the suite's positional webhookDeps wiring at webhook-test.ts:110-117 still lands in slot 4.
Since no source changed, the sole open finding carries forward unresolved.
Prior Findings Dispositioned (1)
- prior:8e58031 important 1 — still-present —
server/src/routes/plugins.ts:3314— the shared company-resolution400is unchanged at this head. Theelsebranch at:3313-3319still answers400 Plugin must be configured for a company before receiving webhookswhenconfiguredCompanyIds.length === 0(:3315-3316), andwebhook-test.ts:273still asserts that400as contract. Areadyplugin with no company config therefore still destroys a well-formed alert batch under a server-side, operator-clearable condition — BLO-28659's exact mechanism, surviving on the same route the PR exists to fix.
Critical Issues (0)
None.
Important Issues (1)
- [gstack/review + native-codex] (prior:8e58031 important 1)
server/src/routes/plugins.ts:3314— the zero-config branch of the shared400is a server-side condition answered with a permanent status, and this PR newly pins it as correct. The sender cannot make the request succeed by changing anything; only an operator writing a company config can. Alertmanager discards it as unrecoverable — precisely the failure this PR removes from the readiness guard 130 lines earlier. It is reachable in the window between a plugin reachingreadyand its config being written, or when a config is removed from a working integration, and the suite mocks exactly that state at:274-275. Judged by this PR's own stated line it belongs on the retryable side:disabledwas kept at503because "an operator disabling a plugin for maintenance is exactly the person who wants the deliveries made during the window to land once they re-enable it" (:269-274) — configuring a company is the same operator-clearable action with the same recoverability. The cost of leaving it is not that the behaviour is pre-existing, but thatwebhook-test.ts:273converts it from incidental into an asserted contract under a file docblock claiming the second half pins "genuine 4xx rejections" (webhook-test.ts:16-18), so the next reader gets no signal that one of the five is misclassified.- Split the shared
400: routeconfiguredCompanyIds.length === 0to503+Retry-Afteralongside the readiness guard, and flipwebhook-test.ts:273to assert that. Keep the sibling branches —'"companyId" query parameter is required for a multi-company plugin'is genuinely sender-fixable, and the explicit-but-unconfigured404at:3307names a company that really is not configured. The multi-company branch is worth a deliberate decision recorded the waydisabledwas (:269-274): adding a second company config silently converts a previously-working single-company URL into a payload-destroying400, which is a growth operation rather than a sender mistake.
- Split the shared
Suggestions (1)
- [pr-review-toolkit: comments]
server/src/routes/plugins.ts:3189-3194— theErrors:list still omits the501at:3217(!webhookDeps) and both company-resolution outcomes (400at:3314,404at:3307). The company400is the finding above, so documenting it is also the cheapest way to make its classification visible to the next reader.
Strengths
- The merge was taken cleanly rather than re-resolved.
plugins.tsdoes not appear in the merge's diff against its first parent and both PR blobs are unchanged, so none of the three rounds of hardening was silently reverted by conflict resolution — the failure mode that makes "merge master in" reviews worth doing at all. - The enum-derived matrix survived a
constants.tschange untouched. Master edited the same file this suite draws its domain from; because the matrix keys offPLUGIN_STATUSESand the production denylist rather than a hand-written list, a capability addition next door is provably inert here. That is the earlier fix paying off on its first real test. - The unknown-status fallback remains the safe one.
WEBHOOK_TERMINAL_PLUGIN_STATUSES(:282) is still a one-member denylist and:3237-3247still falls through to503, so an off-enum value a rolling deploy could write delays alerts rather than destroying them — pinned atwebhook-test.ts:179-197with a genuinely off-enum value, the one case the enum loop structurally cannot reach. - The reasoning is still written down where the next editor will hit it.
:260-278states whyuninstalledis terminal, whydisableddeliberately is not, and what widening the set costs;:3207-3212records the known drift against the sibling guard instead of quietly narrowing the claim.
Recommended Action
- Split the company-resolution
400so the zero-config case is retryable — it is the last path on this route where a well-formed alert batch is still destroyed by a server-side condition, andwebhook-test.ts:273otherwise locks it in. - Extend the docblock error list opportunistically.
- No merge-related action: the
masterintegration at this head is clean and needs nothing.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: e274420
Prior Findings Dispositioned (1)
- prior:8e58031 important 1 — still-present —
server/src/routes/plugins.ts:3314— The zero-config company-resolution branch remains unchanged: when a ready plugin has no configured company IDs, the route returns400with “Plugin must be configured for a company before receiving webhooks” at lines 3314-3318. This is a server-side, operator-clearable condition on an otherwise valid delivery, so the current head still destroys the alert instead of returning503withRetry-After.
Critical Issues (0)
None.
Important Issues (1)
-
[gstack/review + native-codex] (prior:8e58031 important 1)
server/src/routes/plugins.ts:3314— the zero-config branch of the shared400still permanently rejects a well-formed webhook under a server-side condition. A ready plugin can exist before its company configuration is written, or lose that configuration while an integration is live; the sender cannot fix either state. This is the same payload-loss mechanism the readiness change is intended to remove, and the new test atserver/src/__tests__/plugin-webhook-not-ready-retryable.test.ts:273locks the behavior in as400.- Split the branch: return
503plus the existingRetry-Afterheader whenconfiguredCompanyIds.length === 0; keep the missingcompanyIdmulti-company case as a deliberate separate decision, since that request is sender-fixable.
- Split the branch: return
Suggestions (1)
- [pr-review-toolkit: comments]
server/src/routes/plugins.ts:3189-3194— the route docblock still omits the501worker-dependency response and the company-resolution400/404outcomes, making the error contract incomplete.
Strengths
-
The terminal denylist is safely inverted: unknown persisted statuses fall through to retryable
503rather than destructive410. -
The regression matrix now derives from the production terminal set and explicitly covers an off-enum status, so status drift cannot silently move unknown values onto the terminal path.
-
Readiness is checked before manifest and capability validation, preserving alert payloads while a plugin is down.
-
The prior status partition and its
disabledpolicy are documented next to the implementation and covered by focused tests.
Recommended Action
-
Fix the Important issue before merge.
-
Update the route error documentation opportunistically.
Company configuration is operator-controlled, so zero-config webhook delivery must remain retryable while preserving sender-fixable multi-company errors. Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
@allyblockcast Please re-review head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 85fbd98
Prior Findings Dispositioned (1)
- prior:8e58031 important 1 — fixed —
server/src/routes/plugins.ts:3317— The zero-config company-resolution branch now returns503withRetry-Afterat lines 3317-3323, preserving retry semantics while configuration is absent. The explicit unknown-company branch remains404at lines 3309-3312, and omitted companyId for a multi-company plugin remains400at lines 3325-3328.
Critical Issues (0)
None.
Important Issues (0)
None.
Suggestions (0)
None.
Strengths
- The zero-config company-resolution case is now correctly treated as a transient, operator-controlled condition:
503plusRetry-Afterpreserves webhook payloads for later delivery. - Explicit multi-company ambiguity remains a sender-fixable
400, and an explicit company that is not configured remains404. - The regression tests cover the three company-resolution outcomes and retain the terminal-status, unknown-status, and readiness-ordering safeguards from the prior review.
Recommended Action
- No Critical or Important issues found. The formal review is a clean self-review comment because this PR is authored by the Ally App.
kkroo
left a comment
kkroo
left a comment
There was a problem hiding this comment.
Ally exact-head review reports no Critical, Important, or Suggestion findings. Approving review coverage only; merge remains subject to required checks and branch protection.
|
@ally please re-evaluate exact current head |
2 participants
Thinking Path
Linked Issues or Issue Description
AlertmanagerWebhookNotificationsFailingpage this bounds the damage ofinitializetimeout). This PR does not substitute for it: BLO-20410 stops plugins latching dead, this stops a dead plugin from destroying the payloads sent to it. Defence in depth.[PEN-2073] Make review-gate webhook delivery durable— outbound review-gate delivery, different subsystem from inbound plugin ingestion.What Changed
server/src/routes/plugins.ts— the readiness guard partitions the status enum instead of negating one member:installed,disabled,error,upgrade_pendingRetry-After: 30readyuninstalledreadyneeds a reinstall, a new lifecycle rather than a retryRetry-After: 30so senders back off rather than hot-loop a plugin that may stay down for hoursuninstalledis not — so it does not silently regressserver/src/__tests__/plugin-webhook-not-ready-retryable.test.ts(13 cases) pinning both halves of the contract, driven offPLUGIN_STATUSESso the matrix cannot drift from the domainDeliberately unchanged:
WORKER_UNAVAILABLEguards at:1751,:1844,:1939,:2029— bridge/tool-invocation paths with different callers. Blanket-rewriting them was explicitly out of scope; they warrant a separate audit.assertInstanceAdmin-protected config-test route at:2873, which still answers 400. Its caller is an authenticated admin in a UI, not a retrying webhook sender, so 4xx carries no data-loss risk there.webhooks.receive(400), undeclaredendpointKey(404), unconfigured/ambiguous company (400/404), unknown plugin (404).Verification
npx vitest run server/src/__tests__/plugin-webhook-not-ready-retryable.test.ts # Tests 13 passed (13)Both halves were falsified against the code they guard — a test that passes before and after would be worthless.
Reverting the readiness guard to 400 — the original defect. Run against the
10-case matrix as it stood at
9c9ad4d, so the status list is that commit's:Reverting the partition to the catch-all
!== "ready"— the defect the reviewcaught. Run against the current 13-case matrix at
5b4d117:The preserved-4xx cases pass in both, confirming they are not coupled to either fix.
No regressions in adjacent suites:
CI on head
5b4d117: 20/20 green (Build, Typecheck, e2e, policy, Helm, Canary Dry Run, all 6 General test shards, verify).Manual, one-shot (post-merge): during the next plugin restart,
kubectl -n monitoring logs alertmanager-0 -c alertmanager | grep 'Notify for alerts failed'should show a retryable failure rather than
unrecoverable error, andsum(increase(alertmanager_notifications_failed_total{integration="webhook"}[10m]))should recover to 0 without an operator re-enable.
Risks
Low, but named honestly:
Retry-After: 30bounds the rate; Alertmanager and GitHub both honour exponential backoff. A hot-loop would require a sender that ignores bothRetry-Afterand backoff.markErrorchange.Model Used
claude-opus-4-5), 1M context window, extended thinking, tool use (repo edit + local Vitest/tsc execution, Prometheus queries for live alert-state evidence).Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template5b4d1179c9ad4d(0 critical, 2 important); both important findings fixed in5b4d117, awaiting re-review of the current headPaperclip issue: https://paperclip.blockcast.net/BLO/issues/BLO-28659