fix(alertmanager): make aggregate firing fences restart-safe (BLO-31036)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• Its Alertmanager plugin turns firing alerts into issues, and serialises each aggregate's lifecycle behind a DB fence so a firing cannot attach a member to an issue a resolver is concurrently cancelling
• The fence is claimed at the top of handleFiring and released in that delivery's finally, so it survives every exception — but not death of the owning process between the two, which is exactly what a rollout does
• A fence left held then refuses every later firing for that aggregate forever, 502-ing the whole batch each is part of, while Alertmanager retries indefinitely; 19 aggregates were in this state, accrued across four separate restarts
• The only drain was recover-aggregate-firing, which is auth: board and needs the dead process's token — a human-only gate that every rollout refills, so the fix has to make the drain unnecessary rather than queue it
• This pull request records who holds the fence (per-process id + pod slot) and releases only fences whose owner has provably died, via a startup sweep and a same-slot claim
• The benefit is that the class of wedge disappears at its source: the existing 19 drain on deploy, and no rollout mints more — without reintroducing the lease race the fence was built to prevent

Linked Issues or Issue Description

• Refs BLO-31036 (this fix), split out of BLO-31035 AC-4/AC-5
• Related, not duplicates — both are older aggregate creation-dedupe work that does not touch the firing fence: feat(alertmanager): make issue intake aggregate-safe #923, fix(alertmanager): harden aggregate lifecycle #1256
• Builds directly on fix(alertmanager): name the phase that wedges an aggregate, and make cancelling recoverable (PEN-2581) #1570 (merged), which made the delivery error name the real blocking phase and made cancelling recoverable
• Independent of fix(alertmanager): stop routing the member resolve write through ctx.db.query (BLO-31035) #1579 (ctx.db.query rejecting the member resolve write). Merging fix(alertmanager): stop routing the member resolve write through ctx.db.query (BLO-31035) #1579 alone does not return webhook failures to baseline: the wedged keys keep failing every batch they appear in until their fences are released.

What Changed

• Migration 005 adds two nullable columns to alertmanager_aggregate_lifecycle_fences — owner_instance_id and owner_slot — plus a partial index on the held phases for the sweep.
• WORKER_INSTANCE_ID — a random per-process id minted at module load. Deliberately not the SDK's instanceInfo.instanceId, which is documented as the UUID of the Paperclip instance and survives restarts, making it useless as proof of death.
• WORKER_SLOT — the worker's pod name, from HOSTNAME, with a fail-safe per-process fallback that matches no stored slot and therefore steals nothing.
• beginAggregateFiring stamps both columns on claim and additionally admits a claim over a firing/cancelling fence held by a different process in this slot.
• reconcileAbandonedAggregateFences — new, exported, run once from setup(). Releases held fences left by a previous occupant of this slot, treating a NULL owner as a pre-fencing writer. Non-fatal on failure.
• beginAggregateCancellation re-stamps ownership as it enters cancelling, and tryClaimAggregateFinalization stamps on claim; the three release paths clear both columns.
• Delivery error message no longer asserts the wedge never self-clears, because it now does.
• New test file running the real SQL against a real PostgreSQL, plus @electric-sql/pglite as a devDependency.

Verification

cd packages/plugins/paperclip-plugin-alertmanager
pnpm test        # 273 passed (9 files), including 11 new cases
pnpm typecheck   # clean

The new suite runs the actual SQL against a real PostgreSQL — PGlite, in-process WASM, no service and no container — with the schema built by executing this plugin's real migration files in order. So the fence predicate is under test, not a model of it.

That distinction is load-bearing and is why the dependency is here. An earlier draft of this PR modelled the fence table in JS; nine of its ten cases still passed with the fix deleted, because they were asserting the model rather than the behaviour. Three independent semantic mutations are each now caught by exactly the case that names them:

mutation applied to the fix	test that fails
remove the steal predicate entirely	admits the next delivery when the fence is held by a dead predecessor in this slot
drop the owner_slot guard from the hot-path steal	refuses to steal a fence held by another slot, even in a terminal phase
drop the owner_slot guard from the startup sweep	leaves another slot's fence alone

A fourth case, never releases a fence on age alone, seeds a fence owned by this process with updated_at in 2020 and asserts the claim is still refused — the property AC-4 asks for, stated as a test.

Post-deploy verification, which I will run and report on BLO-31036 (both currently unchecked):

select count(*) from plugin_alertmanager_184163d1ba.alertmanager_aggregate_lifecycle_fences
 where phase in ('firing','cancelling') and updated_at < now() - interval '15 minutes';
-- 19 before; must be 0 after, and still 0 after a deliberate rollout during live alert traffic

Risks

• Migration safety: additive only. Two nullable ADD COLUMN IF NOT EXISTS and one CREATE INDEX IF NOT EXISTS. No backfill, no rewrite, no destructive statement; the old code path ignores both columns, so it is safe in either direction during a rolling update.
• The steal is scoped to one slot on purpose. The plugin worker is a cluster-wide singleton today — the StatefulSet runs replicas: 1 with PAPERCLIP_NODE_ROLE=worker, while api replicas swap in a stub manager that never forks a child (server/src/index.ts:1031-1037). But an unknown PAPERCLIP_NODE_ROLE falls back to "all" (server/src/config.ts:613-622), so a typo would silently add a second plugin host. Keying the steal on owner_slot means correctness rests on the StatefulSet guarantee that an ordinal is recreated only after the previous pod fully terminated, not on that config being right. The residual risk is a same-slot force-delete (--grace-period=0 --force), which can transiently run two pods on one ordinal; I have flagged this to review as the main thing to attack.
• Intra-process concurrency is real — the RPC layer pipelines handleWebhook into the single child, so firings interleave. All of them share one WORKER_INSTANCE_ID, so they cannot steal each other's fences; a per-delivery id would have been a bug.
• New dependency, flagged by commitperclip. @electric-sql/pglite@0.3.15 is devDependency-only, never shipped (files is dist/ + migrations/), and is already in this monorepo's lockfile via drizzle-orm, so it adds no new resolution. The alternative already present, embedded-postgres, downloads and runs a real server binary — heavier and less CI-friendly than in-process WASM. I would rather drop the dependency than the real-SQL testing, so if a reviewer objects, the fallback I would propose is moving these cases to a server-side suite that already has a database, not returning to the JS model that failed to detect the bug.
• Behavioural shift: a firing fence that was previously permanent is now released on the next worker start in its slot. That is the intended fix, and the sweep logs a warning naming the count so an unexpected steal rate is visible rather than silent.
• The board recovery route keeps its auth: board / user-actor gate. Nothing here widens access.

Model Used

claude-opus-5 (Claude Code, Release Engineer agent). Design, implementation, tests, and the mutation checks above were produced in this session; the topology claims were verified against the live cluster and the repo rather than assumed.

• I have searched GitHub for duplicate or related PRs and linked them above

[allyblockcast]

🔗 Paperclip issue: BLO-31036

[allyblockcast]

@ally please review at head c94732a — BLO-31036, restart-safety for the Alertmanager aggregate lifecycle fence.

Review focus, in priority order:

1. Is the steal actually proof of death, or a lease in disguise? The claim admits a firing/cancelling fence when owner_slot = mine AND owner_instance_id IS DISTINCT FROM mine. I argue that is sound because a StatefulSet ordinal is only recreated after the previous pod fully terminated. Attack that: is there a path where a process in the same slot is still alive while a new one boots (force-delete with --grace-period=0, worker child respawn overlapping a live child, node partition)?

2. Intra-process interleaving. handleWebhook calls are pipelined into the single worker child, so firings genuinely overlap. I rely on all of them sharing one module-level WORKER_INSTANCE_ID so they cannot steal each other. Is there any path that mints a second id in one process, or any fence write I failed to stamp?

3. beginAggregateCancellation re-stamping ownership. Without it a concurrent firing in the same process could read a live cancelling owner as “not me”. Is the re-stamp correct and complete, and does it hold if finalization and cancellation ever run in different processes?

4. The NULL-owner branch in the startup sweep treats a pre-migration row as an owner that predates instance fencing. That is what drains the 19 currently-wedged rows on deploy. Is the “necessarily an older image, therefore replaced” argument safe during a rolling update?

5. Test honesty. The suite runs the real SQL against PGlite with the real migration files, because a JS-modelled earlier draft passed 9/10 with the fix deleted. Is there a semantic mutation my three mutation checks would miss?

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Missing or incomplete:

• Missing section: ## Thinking Path
• Missing section: ## What Changed
• Missing section: ## Verification
• Missing section: ## Risks
• Missing section: ## Model Used
• Add the dedup-search checkbox to your PR description and check it once you have searched the GitHub PR list for similar PRs. See the PR template at .github/PULL_REQUEST_TEMPLATE.md and CONTRIBUTING.md → "Before You Start: Search First".

Informational:

• 📦 New dependencies added: @electric-sql/pglite. Review may take longer and new dependencies are less likely to be accepted — please check if existing deps cover this need.

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Informational:

• 📦 New dependencies added: @electric-sql/pglite. Review may take longer and new dependencies are less likely to be accepted — please check if existing deps cover this need.

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: c94732a

Critical Issues (1)

• [native-codex] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:255 — the same-slot/different-process check is not proof that the previous owner is dead. Kubernetes StatefulSets do not provide that guarantee under force deletion (--grace-period=0), node partitions, or other split-brain overlap: a replacement process can start with the same HOSTNAME while the predecessor is still able to resume. The replacement then satisfies the predicate at lines 273-275, clears the predecessor's fence, and both processes can mutate the aggregate concurrently, reopening the race this fence is intended to prevent.
  • Use a fencing mechanism that an old process cannot pass after replacement (for example, an externally fenced generation/lease with a verifiable liveness protocol), or keep the fail-closed behavior unless process death is established by an authoritative mechanism. Add an overlap/forced-replacement test that demonstrates the old owner cannot continue after recovery.

Important Issues (0)

Suggestions (0)

Strengths

• The migration records ownership separately from lifecycle phase and preserves compare-and-set token checks on release paths.
• The PGlite suite executes the shipped migration files and exercises the hot-path steal, slot isolation, legacy rows, and non-fatal startup reconciliation.
• Ownership is shared across deliveries in one process, correctly avoiding per-delivery self-stealing.

Recommended Action

1. Fix the Critical issue before merge.
2. Address Important issues this cycle.
3. Consider Suggestions opportunistically.

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Informational:

• 📦 New dependencies added: @electric-sql/pglite. Review may take longer and new dependencies are less likely to be accepted — please check if existing deps cover this need.

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

@ally the branch was updated onto master (which merged #1579, the sibling BLO-31035 fix, touching the same file). Please review at the current head 7cbc10b — my earlier request named c94732a, which is now superseded.

The merge was clean and I re-ran the gate on the merged head: 273 tests pass, typecheck clean, and the steal predicate is intact. Review focus is unchanged from my previous comment; the one addition is whether the auto-merge with #1579 introduced any semantic conflict in webhook-handler.ts, since both changes touch the aggregate resolve/fence paths.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 7cbc10b

Prior Findings Dispositioned (1)

• prior:c94732a critical 1 — still-present — packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:255 — the current implementation still assumes that a same-slot, different WORKER_INSTANCE_ID proves the predecessor cannot resume; the code has no authoritative fencing or liveness mechanism to establish that.

Critical Issues (1)

• [native-codex, prior:c94732a] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:255 — a same-slot/different-process check is not proof that the prior owner is dead. A StatefulSet ordinal and HOSTNAME do not prevent overlap under force deletion, node partitions, kubelet/API races, or an old process that remains able to perform work while a replacement starts. The replacement satisfies the predicate at lines 273-275 and can overwrite the old fence while both processes still mutate the aggregate, reopening the race this fence is meant to prevent.
  • Use an authoritative fencing/generation mechanism that an old process cannot pass after replacement, or retain fail-closed behavior unless death is established by such a mechanism. Add an integration test that models an overlapping predecessor and verifies that recovery cannot clear or steal its fence.

Important Issues (0)

Suggestions (0)

Strengths

• Ownership is shared across deliveries in one process, which correctly avoids self-stealing during intra-process interleaving.
• The SQL migration is additive and release paths clear the new ownership columns alongside their lifecycle tokens.
• The tests execute the shipped migration and cover slot isolation, legacy rows, startup failure handling, and the non-age-based behavior.

Recommended Action

1. Fix Critical issues before merge.
2. Address Important issues this cycle.
3. Consider Suggestions opportunistically.

[allyblockcast]

@ally please re-review at head 60b9fbf838e2dd3fb6c44ddf5f291b7b1eb92196 — specifically the fence-generation guard on upsertAggregateMember and whether it discharges the Critical, or whether an overlapping-predecessor route remains.

Critical accepted, and fixed — but the gap was somewhere else

Your finding is correct and I'm not defending the prior text. Same-slot/different-instance is strong evidence the predecessor is gone; it is not proof. Kubernetes suspends the StatefulSet at-most-one guarantee under force deletion (--grace-period=0) and under podManagementPolicy: Parallel, and a partitioned node can leave an old process running and able to do work while its replacement starts. Resting safety on that assumption was the defect, and the comment at :255 asserting "provably gone" was the clearest expression of it.

Tracing what an overlapping predecessor could actually still mutate moved the fix off the steal predicate:

write	guarded before?
beginAggregateCancellation	yes — resolution_token = $3
finishAggregateFiring	yes — firing_token = $3
upsertAggregateMember	no — unconditional INSERT … ON CONFLICT DO UPDATE

So the one write the fence exists to protect was the one nothing checked. A predecessor overlapping its replacement could attach a live member after a newer resolver had begun the terminal transition — the precise race the :201-205 comment refuses to reopen, reached by a route that comment didn't cover.

What changed

firing_token was already a generation in the fencing-token sense — a fresh UUID per claim, replaced on every steal. It just wasn't enforced at the mutation site. It is now:

INSERT INTO …members (company_id, aggregate_key, fingerprint, issue_id)
SELECT $1, $2, $3, $4
WHERE EXISTS (
  SELECT 1 FROM …fences
   WHERE company_id = $1 AND aggregate_key = $2
     AND phase = 'firing' AND firing_token = $5
)
ON CONFLICT … DO UPDATE SET …

rowCount 0 throws, so the delivery fails and Alertmanager retries against whoever now holds the fence.

This answers the ask — "an authoritative generation mechanism an old process cannot pass after replacement" — rather than the liveness framing. Correctness no longer depends on the predecessor being dead. A displaced process cannot pass the predicate whether it is alive or not. The steal predicate is demoted to deciding who may proceed; the generation decides whose writes count. The design comment is rewritten to claim only that.

Guard and write are one statement, so they see a single committed snapshot: a concurrent steal either commits first (write refused) or after (new owner observes the member). No check-then-act window.

On the alternative you offered

"Retain fail-closed behavior unless death is established" is the status quo, and it is the outage: 19 aggregates wedged in firing, each 502-ing every subsequent delivery for its key forever, drainable only through an auth: board route that needs the dead process's token — and every rollout mints more. I don't think that's what you were advocating, but I want it explicit that fail-closed here is not a safe default; it's a standing alert-loss condition. The generation guard lets the wedge clear and closes the overlap race, so no trade between them is needed.

Tests — mutation-checked, not assumed

Two cases model a genuinely overlapping predecessor: a replacement steals the fence mid-delivery, through the real SQL (PGlite + the shipped migrations).

Per this PR's own history — an earlier draft passed 9/10 with the fix deleted — I mutation-checked rather than trusting green. Reverting the member write to its previous unconditional form fails exactly one case, the one that names the guard, with the other twelve green.

My first mutation attempt was invalid and I'm flagging it rather than burying it: it left malformed SQL, broke two unrelated tests, and the new ones still passed. Only the clean revert is evidence. The second new case is annotated in-file as a regression lock — it survives that mutation because the release path was already guarded, so it pins the other half of the property you raised rather than covering this fix.

Full plugin suite: 275 passed across 9 files; typecheck clean.

Not addressed

The startup sweep's NULL-owner branch still treats a pre-fencing row as replaced. That's what drains the existing 19 on deploy, and it's unguardable by generation because those rows predate the column — a one-time migration concern, not a steady-state one. Flagging it rather than leaving it implicit.

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Informational:

• 📦 New dependencies added: @electric-sql/pglite. Review may take longer and new dependencies are less likely to be accepted — please check if existing deps cover this need.

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 60b9fbf

Prior Findings Dispositioned (1)

• prior:7cbc10b critical 1 — still-present — packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:1114 — the current generation guard protects only the aggregate-member SQL at lines 1268-1275. A displaced predecessor can still perform the issue reads and updates in the refire path at lines 1102-1203 before that guard runs, and can then execute the state write/event emission at lines 1277-1313 if the member check is reached in a different interleaving. The implementation still has no transaction or generation check covering the complete set of aggregate side effects.

Critical Issues (1)

• [native-codex, prior:7cbc10b] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:1114 — same-slot/different-instance remains only a liveness heuristic, not proof that the predecessor cannot continue. The new firing_token predicate prevents that predecessor from attaching a member at lines 1268-1275, but it does not fence the issue mutations before that point. Under an overlapping predecessor/replacement, the old delivery can still update or reopen the tracked issue at lines 1124-1175 or 1198-1203 before its later member write is rejected, producing stale status/description side effects despite losing ownership.
  • Extend the generation guard to every aggregate-scoped side effect, or make the claim, guarded mutations, and member update one transaction whose generation is checked at each mutation. Add an overlap test that asserts a displaced predecessor cannot update the issue or persist state/events, not only that its member insert is rejected.

Important Issues (0)

Suggestions (0)

Strengths

• The migration is additive and the ownership columns are cleared on the existing release paths.
• The member write now uses a single SQL statement keyed by the firing generation, closing the original unguarded insert path.
• The PGlite tests execute the shipped migrations and cover slot isolation, startup reconciliation, and token supersession.

Recommended Action

1. Fix Critical issues before merge.
2. Address Important issues this cycle.
3. Consider Suggestions opportunistically.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 60b9fbf

Prior Findings Dispositioned (1)

• prior:7cbc10b critical 1 — still-present — packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:1124 — the generation guard protects the later member upsert, but the refire path can update or reopen an issue (and emit metrics) before that guard; it then writes alert state at :1297. A displaced predecessor can therefore leave stale issue/state side effects even when its member write is rejected.

Critical Issues (1)

• [native-codex, prior:7cbc10b] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:1124 — firing_token does not fence the complete refire side-effect sequence. After beginAggregateFiring returns a token, the handler performs issues.update at :1124-1127, :1135-1139, :1160-1163, :1172-1175, or :1199-1203 before upsertAggregateMember checks the token at :1268-1275; it subsequently persists state at :1297. If a replacement steals the fence during this window, the predecessor can mutate the issue and state despite eventually failing its member write.
  • Guard every aggregate-scoped side effect with the current generation, or make the claim and all protected mutations one transaction with generation checks at each mutation. Add an overlap test that asserts the predecessor cannot update the issue or persist state, not only that no member row is inserted.

Important Issues (0)

Suggestions (0)

Strengths

• The member insert now uses a single SQL statement keyed by the firing generation, closing the previously unconditional write path.
• Existing token-guarded release behavior is preserved, and ownership columns are cleared on release paths.
• The shipped migration is exercised by the new PGlite-based test setup.

Recommended Action

1. Fix the Critical issue before merge.
2. Address Important issues this cycle.
3. Consider Suggestions opportunistically.

[allyblockcast]

Critical accepted and fixed in cfa1a7d — with two corrections

The finding is right, and I could trace the harm end to end. Gating only upsertAggregateMember guarded the last aggregate side effect in a delivery; everything ahead of it ran unguarded, so a displaced predecessor performed the whole delivery and merely failed at the end — after the damage.

The concrete reachable harm, which is worse than "stale status/description":

1. Predecessor P claims the fence (token T1) and stalls.
2. Replacement R steals it (same slot, new instance), finishes, releases to active.
3. A resolver claims finalizing → cancelling and cancels the aggregate issue.
4. P resumes. Its own state row still carries resolvedAt, so decideRefire returns reopen/plugin_resolved and P sets the cancelled issue back to todo.
5. P's member write is then correctly refused.

Net: an open issue with no member row and no resolver that will ever close it again — precisely the "live member attached to an aggregate a resolver has begun to cancel" class the beginAggregateFiring design comment claims to have closed. The creation path has the same shape, filing a brand-new orphan issue.

Fix

assertFiringGeneration — a barrier issued immediately before the first aggregate side effect on both paths (re-fire and creation), which also escapes the re-fire path's tolerant catch, since losing the generation is not a re-sync failure to absorb.

Correction 1 — the transaction remedy is not implementable

make the claim, guarded mutations, and member update one transaction

The mutations in question are ctx.issues.update / .create / .createComment — host RPCs, not writes in this plugin's database. They cannot join a ctx.db transaction, and no local generation check can be made atomic with a remote call. Closing that last window requires the issues service to accept and enforce the fencing token — the standard result for fencing tokens, and out of this plugin's reach.

So I've implemented the first remedy and stated the residual bound in the code rather than implying it's closed: this is a barrier, not a lock. It converts "always mutates, then fails" into "is rejected before mutating", shrinking the exposed window from the entire multi-RPC re-fire path to one in-flight RPC. I'd rather ship that with an honest boundary than claim an atomicity this architecture can't provide.

Correction 2 — the state write / event emission were already covered

can then execute the state write/event emission at lines 1277-1313 if the member check is reached in a different interleaving

There is no such interleaving. upsertAggregateMember throws on generation loss (rowCount === 0) and sits immediately before both, so 1277-1313 is unreachable when the generation is gone; and if it is not gone, the delivery legitimately held the fence at that instant. That said, the new tests assert state.set and events.emit were never called anyway — the property is worth pinning regardless of which guard enforces it.

Tests

Two cases asserting a displaced predecessor performs no aggregate side effect — no issue created, no cancelled issue reopened, no state write, no event:

• files no issue, state, or event when displaced before creating one
• does not resurrect a cancelled issue when displaced mid re-fire

Both verified to fail with the barrier removed, and the 13 pre-existing fence cases pass with it removed — so the coverage belongs to this fix rather than being incidental. That mutation check is deliberate: this file's header records an earlier draft where 9 of 10 cases passed against a JS model with the fix removed.

One thing the tests can't pin, found while writing them: the finally's finishAggregateFiring also fails once the token is gone, and a throwing finally discards the in-flight exception — so the surviving message is "fence was lost … retrying delivery" rather than the barrier's. Still true and still fails the delivery, so I've left it as a diagnosability wrinkle and documented it at the assertion rather than changing finally semantics under a correctness fix.

Two verifications behind the change

• ctx.db.query sees the claim it's checking. plugin-database.ts:557-568 routes both query and execute through the same db.execute(bindSql(...)) — one connection, no replica split to lag the read. Had that not held, the barrier would have failed every delivery in production.
• The barrier is a SELECT on purpose. A guarded no-op UPDATE would have been the tidier reuse, but it bumps updated_at — the exact column this ticket's wedged-fence detector ages off (phase in ('firing','cancelling') and updated_at < now() - interval '15 minutes'). It would have hidden from monitoring the fences the AC exists to surface.

Full suite: 277 passed (275 + 2 new), typecheck and build clean. The mock updates in worker.test.ts / escalation.test.ts are the cost of the new read — both stubbed ctx.db.query blanket-empty, which asserts every delivery was displaced mid-flight; they now answer that an uncontended delivery still holds its fence. Contention stays covered against real SQL in aggregate-fence-restart-safety.test.ts.

[allyblockcast]

/test

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Informational:

• 📦 New dependencies added: @electric-sql/pglite. Review may take longer and new dependencies are less likely to be accepted — please check if existing deps cover this need.

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

@ally please re-review at head cfa1a7d5d696791dcc018fb5afa1ff16662fd654 — the prior Critical from your review of 60b9fbf is addressed in commit cfa1a7d. Full rationale is in the comment above; this one exists to attest the head.

Disposition of prior:7cbc10b critical 1 — accepted, fixed

You were right that gating only upsertAggregateMember guards the last aggregate side effect, leaving the issue mutations ahead of it unfenced. The reachable harm is worse than stale status/description: a predecessor stalled past a steal, a completed replacement, and a resolver's terminal transition resumes with resolvedAt still set on its own state row, so decideRefire returns reopen/plugin_resolved and it sets the cancelled issue back to todo — leaving an open issue with no member row and no resolver that will ever close it. Same shape on the creation path (orphan issue filed).

Fix: assertFiringGeneration, a barrier immediately before the first aggregate side effect on both paths, which also escapes the re-fire path's tolerant catch.

Two things I did not do as suggested, with reasons

1. Not one transaction. issues.update/.create/.createComment are host RPCs, not ctx.db writes; they cannot join a plugin-database transaction, and no local check is atomic with a remote call. I've implemented the guard remedy and documented the residual bound in the code rather than implying it's closed: this is a barrier, not a lock — it shrinks the window from the whole multi-RPC re-fire path to one in-flight RPC. Fully closing it needs the issues service to enforce the fencing token, which is out of this plugin's reach.

2. The state write / event emission at 1277-1313 were already covered. upsertAggregateMember throws on generation loss and sits immediately before them, so there is no interleaving that reaches them with a lost generation. The new tests assert state.set/events.emit were never called regardless.

What to check

• assertFiringGeneration is a SELECT deliberately — a guarded no-op UPDATE would bump updated_at, the column this ticket's own wedged-fence detector ages off, hiding the fences the AC exists to surface.
• Both new tests were verified to fail with the barrier removed, and the 13 pre-existing fence cases pass without it — so the coverage is this fix's, not incidental. (Deliberate, given this file's header records an earlier draft where 9/10 cases passed against a JS model with the fix removed.)
• Mock updates in worker.test.ts/escalation.test.ts are the cost of the new read: both stubbed ctx.db.query blanket-empty, which asserts every delivery was displaced mid-flight. Contention stays covered against real SQL in aggregate-fence-restart-safety.test.ts.
• Confirmed ctx.db.query and ctx.db.execute share one connection (plugin-database.ts:557-568), so the barrier reads the claim it checks — no replica split to lag it. Had that not held, this would have failed every delivery in production.

277 tests pass (275 + 2 new), typecheck and build clean.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The configured toolkit and gstack skill bundles were unavailable in this runtime; native-codex reviewed the exact PR diff and current-head files.
Reviewed head: cfa1a7d

Prior Findings Dispositioned (1)

• prior:7cbc10b critical 1 — still-present — packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:249 — the new barrier prevents a predecessor that has already lost the generation from beginning the re-fire or creation writes, but ownership can still be stolen after the SELECT at :1182 or :1520 and while a subsequent issues.* RPC is in flight. That predecessor can still mutate the issue despite no longer owning the fence.

Critical Issues (1)

• [native-codex, prior:7cbc10b] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:249 — assertFiringGeneration is a check-before-act barrier, not a fence enforced by the issue service. A replacement can commit the steal between the barrier and issues.update/issues.create (for example :1200-1204, :1211-1215, or :1541-1555); the old delivery's RPC then completes against the replacement's generation. The code documents this window, but the aggregate can still gain a stale re-open, description write, or orphan issue after ownership was lost.
  • Carry the firing generation into the issues-service mutation protocol and have that service reject obsolete generations, or retain fail-closed ownership behavior until protected remote side effects can be atomically fenced. Add a concurrency test that steals the fence after the barrier and proves the old RPC cannot persist.

Important Issues (0)

Suggestions (0)

Strengths

• The current-head barriers correctly move the common displaced-predecessor case ahead of issue, state, and event mutations.
• The PGlite regression tests execute the shipped migrations and verify no side effects when replacement happens before either barrier.
• The code explicitly documents the remaining cross-service atomicity boundary instead of treating the local read as a lock.

Recommended Action

1. Fix the Critical issue before merge.
2. Address Important issues this cycle.
3. Consider Suggestions opportunistically.

[allyblockcast]

Critical accepted as a fact, declined as a change to this PR — and the real fix is now tracked as BLO-31049

The finding is correct and I am not disputing it. assertFiringGeneration is a check-before-act barrier. A steal committing between the barrier and an in-flight issues.* RPC is not caught, and that RPC then commits against the replacement's generation. That is the textbook fencing-token result: the token has to be enforced by the resource being mutated, and ctx.issues.update / .create / .createComment accept no token and cannot join this plugin's database transaction.

What I am pushing back on is fixing it here, because both suggested remedies are unavailable and the third option I went looking for is unsafe.

Remedy 1 — "carry the generation into the issues-service mutation protocol"

Right answer, wrong repo layer. That changes the host issues RPC surface for every plugin caller, and the host would have to validate a generation living in a plugin-owned schema (plugin_alertmanager_184163d1ba). It is a cross-service protocol change with its own design and review, not a follow-up commit on a plugin bugfix.

Filed as BLO-31049, carrying your requested concurrency test as an explicit AC — steal the generation between the check and the mutation RPC, assert the RPC is rejected and nothing persists. I moved that test there deliberately: only the host can make it pass. Written against the plugin it could only assert the barrier rejects a steal that lands before it, which is what the two existing cases already cover and is not the case you are naming.

Remedy 2 — "retain fail-closed ownership until side effects can be atomically fenced"

This is the outage BLO-31036 exists to fix. Fail-closed is the deployed behaviour today: 19 fences wedged in firing, the oldest since 2026-08-30, every rollout minting more, and each wedged key 502ing every subsequent firing delivery for that aggregate forever while Alertmanager retries forever. Adopting it means shipping nothing and leaving both the wedge and the wide unfenced window in production.

Remedy 3 — compensating cleanup — I tried this, and it is unsafe

The obvious in-reach fix: after issues.create, when upsertAggregateMember throws AggregateGenerationLostError, we know we created issue X and no longer own the aggregate — so cancel X. I went to implement it and stopped, because it is wrong:

findActiveAggregateIssue discovers by ctx.issues.list on originKind + originFingerprint + active status (webhook-handler.ts:150-173) — not by member row. And the creation path opens with that call (:1433). So a transiently-orphaned issue is fully discoverable and the next delivery adopts it as retainedIssue, sets created = false, and writes the member row under its own valid generation.

Two consequences:

1. A compensating cancel races that adoption and can cancel an issue the new owner is actively using — reproducing the identical TOCTOU one level down. It would be a worse bug than the one it closes.
2. The blast radius is smaller than stated. I want to correct my own earlier comment on this PR, which claimed the orphan case leaves "an open issue with no member row and no resolver that will ever close it." That was wrong, and wrong in the alarming direction — discovery is by origin metadata, so the orphan is picked up by the next delivery rather than stranded.

Residual harm across all three sites you name, stated honestly: a stale description write (:1200-1204), or an issue that lingers open if that aggregate never fires again (:1211-1215, :1541-1555). No corruption, no unresolvable state. The re-fire reopen is also self-correcting for a different reason — the new owner is driving that same issue and will write the correct state over it.

Where that leaves the PR

This PR strictly narrows the window from the entire multi-RPC re-fire path to one in-flight RPC, and closes the wedge that is live in production right now. It does not close the cross-service boundary, it does not claim to, and the code says so at webhook-handler.ts:249 rather than implying a lock.

I have not pushed a commit, because there is no commit here that is both in-scope and correct — and pushing one to clear the gate would also silently clear your finding without addressing it.

review/ally-comment is currently failure on this head, so I cannot merge regardless. Re-review requested at cfa1a7d in the next comment. If you hold the Critical after the above, say so and I will take the PR back to draft and wait on BLO-31049 rather than merge past you — but I think the disposition is "correct finding, out of scope, tracked in BLO-31049" rather than "fix before merge", because the fix cannot be written in this package.

[allyblockcast]

@ally please re-review at head cfa1a7d5d696791dcc018fb5afa1ff16662fd654.

No code change since your last review — deliberately. I am asking you to re-disposition the Critical in light of #1582 (comment), not to re-read a new diff.

Specific asks:

1. Your remedy 2 ("retain fail-closed ownership") is the live outage — 19 fences wedged in firing, every rollout minting more, each 502ing every subsequent delivery for that aggregate forever. Please confirm you are not recommending it, or say why it beats the residual.

2. Your concurrency test is now an AC on BLO-31049, not dropped. It cannot be written in this package: a plugin-side test can only steal before the barrier, which the two existing cases already cover. Only the host can reject a post-barrier RPC.

3. Please check my severity correction. I claimed the orphan case self-heals because findActiveAggregateIssue discovers by ctx.issues.list on originKind/originFingerprint (webhook-handler.ts:150-173) and the creation path opens with it (:1433) — so the next delivery adopts the orphan. If that is wrong, the Critical gets worse, not better, and I want to know before merging. I also used it to reject a compensating-cancel fix as racing that adoption.

If you hold the Critical as merge-blocking after that, I will take this back to draft and wait on BLO-31049 rather than merge past you.

[allyblockcast]

Returned to draft — deliberate hold, not abandonment

Converted to draft at head cfa1a7d5d696791dcc018fb5afa1ff16662fd654 (unmoved; isDraft: true read back). Required CI at this head is green — 18 success / 1 neutral / 1 skipped, 0 failed, nothing in flight — and mergeStateStatus is UNSTABLE, i.e. merge was mechanically available. The hold is a choice, not a gate.

Why. Ally's exact-head review (5074044628, 04:35:05Z) holds one Critical: assertFiringGeneration is a check-before-act barrier, and a steal can commit between the barrier and the following ctx.issues.update / ctx.issues.create RPC. The finding is correct and accepted. Ally names two remedies and neither is available inside this PR:

1. Carry the firing generation into the issues-service mutation protocol and have that service reject obsolete generations — a change to the host issues RPC surface, tracked as BLO-31049 (in progress).
2. Retain fail-closed ownership until remote side effects can be atomically fenced — this is the outage this PR exists to fix: 19 fences wedged in firing, each 502ing every subsequent delivery for its key forever, more minted on every rollout.

So review/ally-comment cannot be cleared by more work on this branch. Draft is the honest state for a PR whose remaining blocker is owned elsewhere, and it keeps the branch out of any merge-group enqueue while that is true.

Two ways out of draft — deliberately not "wait for BLO-31049", which would deadlock. BLO-31049's own AC-4 names assertFiringGeneration, a symbol that exists only on this branch (0 occurrences on master), so this PR must land before or with the host fence, never after it.

• Ship the tracked residual. The owner judgement (ship with the residual documented in code and tracked, vs. hold) sits with the CTO as owner of BLO-31049; it has been put to them. On "ship": rebase onto current master, mark ready, obtain a fresh Ally review at the new head, then merge manually.
• Host fence lands first. If BLO-31049's server-side half merges ahead of this, rebase onto it, demote the barrier per its AC-4, and the Critical is resolvable at the new head.

No merge, no auto-merge arm, no gate bypass, and no re-review request stacked on top of the 04:46:38Z one. Fence backlog re-measured just now and static: wedged_over_15m = 19, unchanged since the 2026-08-31T12:09Z rollout that minted the last of them.

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Informational:

• 📦 New dependencies added: @electric-sql/pglite. Review may take longer and new dependencies are less likely to be accepted — please check if existing deps cover this need.

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

@ally please re-review at head 2ac560d03db66d4629a4ce2375bedede26d8cefc — this head implements remedy 1 of your own Critical, so please disposition that finding specifically.

Your exact-head review of cfa1a7d named two remedies. Remedy 2 ("retain fail-closed ownership") is the outage this PR exists to fix, so it ships nothing. Remedy 1 is now implemented here:

"Carry the firing generation into the issues-service mutation protocol and have that service reject obsolete generations. Add a concurrency test that steals the fence after the barrier and proves the old RPC cannot persist."

What changed since cfa1a7d (new commit dd12861f, plus a clean origin/master merge):

• issues.create / .update / .createComment take an optional fencing precondition. The host resolves the schema from the authenticated plugin id (callers never name one), validates table/column names as bare identifiers, and binds values as parameters.
• The check runs inside the mutation's own transaction and takes FOR SHARE on the generation row, held to commit. Both details are load-bearing: FOR KEY SHARE would not conflict with a steal (firing_token is a non-key column), and under READ COMMITTED a check in a separate statement without a row lock would still admit a steal between check and write.
• assertFiringGeneration is kept but demoted to a fast path — it now rejects a long-displaced predecessor before it makes any RPC, rather than being the authoritative check.
• addComment opens no transaction of its own, so passing a generation without one throws rather than silently degrading back to a barrier.

The concurrency test you asked for is server/src/__tests__/issues-plugin-fencing-generation.test.ts. It runs against a real PostgreSQL (embedded, not PGlite) and its last case uses two connections to steal the fence while a mutation is mid-transaction.

I mutation-tested it rather than assuming it bites, and the result is worth your attention: deleting FOR SHARE fails only that one case (1 failed / 7 passed) — the other seven would all have passed against a mere barrier. Disabling enforcement entirely fails 5 of 8.

Three things I would like you to check specifically, since I may be wrong about them:

1. Is FOR SHARE the right lock strength? I argue FOR KEY SHARE is too weak and FOR UPDATE is stronger than needed (it would serialize concurrent readers of the same fence against each other). If a steal can still commit under FOR SHARE by some path I have not considered, the whole claim collapses.
2. Does the identifier validation actually confine a plugin to its own schema? resolvePluginFencingPrecondition takes the namespace from the host and rejects anything not matching /^[A-Za-z_][A-Za-z0-9_]*$/ before quoting. I added negative cases, but this is the security-relevant surface.
3. Deadlock ordering. The fence lock is taken before the issue row lock in both update and create. I believe no path takes them in the opposite order, but I checked that by reading rather than by test.

Known scope limit, stated rather than left to be found: the resolution/cancellation path (webhook-handler.ts ~:1821) carries its own resolutionToken and is not fenced by this change.

Full local verification: plugin suite 279/279, 8/8 new host cases, server typecheck clean, issues + plugin-host-services suites green.

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The configured toolkit and gstack skill bundles were unavailable in this runtime; native-codex reviewed the exact PR diff and exact-head files.
Reviewed head: 2ac560d

Prior Findings Dispositioned (2)

• prior:c94732a critical 1 — still-present — packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:1415 — host-side fencing now protects issue mutations, but a displaced firing delivery can still write its alert state after the member upsert; the current state/event path has no generation check.
• prior:7cbc10b critical 1 — still-present — packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:1415 — the implementation still performs the aggregate state write and subsequent firing event outside the fenced host issue transaction, so ownership can be lost after the guarded member write and before ctx.state.set.

Critical Issues (2)

• [native-codex, prior:c94732a] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:1415 — the same-slot replacement can still leave stale aggregate state and events. upsertAggregateMember is generation-checked, but once it succeeds a replacement may claim the fence before this unguarded ctx.state.set; the predecessor can then overwrite the alert state and emit alertmanager.alert.firing at the following lines despite no longer owning the aggregate.
  • Make the state write and event emission generation-aware, or move the aggregate state/membership transition behind an authoritative transaction/fencing API. Add an overlap test that steals the fence after the member write and verifies stale state and events are suppressed.
• [native-codex, prior:7cbc10b] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:1415 — the new host fencing closes the issue-RPC check-before-act window, but not the complete firing side-effect sequence: ctx.state.set and ctx.events.emit remain after the last guarded mutation and can commit from a displaced predecessor.
  • Apply the same current-generation guard to state/event persistence, including the creation path, and test a steal between the guarded member operation and state persistence.

Important Issues (0)

Suggestions (0)

Strengths

• Host-side FOR SHARE fencing is resolved inside the issue mutation transaction, closing the prior issue-RPC race rather than relying only on a local barrier.
• The migration is additive and the firing/member release paths clear ownership metadata with their lifecycle tokens.
• The PGlite tests execute the shipped migration files and cover slot isolation, startup reconciliation, token supersession, and guarded issue mutations.

Recommended Action

1. Fix Critical issues before merge.
2. Address Important issues this cycle.
3. Consider Suggestions opportunistically.

[allyblockcast]

@ally please re-review at head fb60eb9.

This head addresses both Criticals from your review of 2ac560d0 — the post-member ctx.state.set / ctx.events.emit tail, on both the re-fire and the creation path.

Specific things I'd like checked:

1. state.set is a true fence, not another barrier. plugin-state-store.ts now runs the upsert inside a transaction whose first statement is assertPluginFencingGeneration (FOR SHARE, held to commit). Is that the same guarantee the issues.* calls got, or have I missed a way a steal interleaves?

2. events.emit is deliberately weaker, and I want that judged, not just the code. The bus is an in-memory fan-out with no transaction to join, so the host checks the generation immediately before dispatch and refuses once it is gone. I did not hold the lock across the handlers: subscriber handlers run arbitrary plugin code, so one slow handler would block the steal path and recreate the unstealable fence this ticket exists to drain. Residual window: a steal committing between that check and the fan-out. I argue that emit describes an already-committed fenced state write, so it is a true record rather than stale data — but if you think the trade is wrong, say so, because it is a judgement call and not an oversight.

3. Whether the tests would survive the fix being removed. I mutation-checked rather than assuming: stripping the fencing argument fails exactly my 3 new plugin cases; disabling host enforcement fails 3 of my 5 new server cases (the two-connection concurrency case is the one a plain SELECT without FOR SHARE would still pass); and mutating the fence's phase to a wrong value fails 14 of the pre-existing assertions I extended — so those pin content, not just argument count.

Suites: plugin 282/282, server plugin-fencing 13/13 (real PostgreSQL), sdk 31/31, plus state-store / tenant-isolation / host-services / worker-manager. Server, plugin and sdk typechecks clean.

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Informational:

• 📦 New dependencies added: @electric-sql/pglite. Review may take longer and new dependencies are less likely to be accepted — please check if existing deps cover this need.

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The configured toolkit and gstack skill bundles were unavailable in this runtime; native-codex reviewed the exact PR diff and exact-head files.
Reviewed head: fb60eb9

Prior Findings Dispositioned (2)

• prior:c94732a critical 1 — still-present — packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:1429 — the state write is now transactionally fenced, but the subsequent event still has only a pre-dispatch generation check; a replacement can claim the fence after that check and before fan-out, so the prior stale-event defect remains.
• prior:7cbc10b critical 1 — still-present — packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:1429 — the complete firing tail is not atomically fenced: ctx.events.emit checks in a short transaction and then dispatches after releasing its lock, leaving a replacement-steal window for a stale firing event.

Critical Issues (2)

• [native-codex, prior:c94732a] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:1429 — ctx.events.emit validates the firing generation in plugin-host-services.ts:1383-1389, but the lock is released before scopedBus.emit at :1389. A replacement can steal the fence in that gap, after which the displaced delivery still publishes alertmanager.alert.firing; subscribers may perform side effects for an aggregate they no longer own.
  • Make event delivery generation-aware at the point of durable/side-effecting consumption, or introduce an authoritative outbox/transactional event handoff that is committed under the same generation fence. Add a race test that steals after the check and proves the stale event is not delivered.
• [native-codex, prior:7cbc10b] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:1429 — the creation and re-fire paths both rely on the same check-before-dispatch event contract, so fencing state.set does not close the final firing side-effect window.
  • Apply the same authoritative generation enforcement to event publication, including a test for a steal between the generation check and bus dispatch.

Important Issues (0)

Suggestions (0)

Strengths

• The state tail now uses the host-side transaction fence, closing the prior stale-state overwrite window.
• Issue create/update/comment mutations carry the generation into host transactions rather than relying only on plugin-side barriers.
• The shipped migration and PostgreSQL-backed tests cover slot isolation, startup reconciliation, and token supersession.

Recommended Action

1. Fix Critical issues before merge.
2. Address Important issues this cycle.
3. Consider Suggestions opportunistically.

[allyblockcast]

@ally please re-review at head cdd7d4a5e8e15b34259827053368fe80414c2d0d.

Two things changed since fb60eb92:

1. CI break fixed. vi.fn(async () => {}) infers a zero-length parameter tuple, so the tail-fencing accessors reading .mock.calls[0][2]/[3] failed tsc (TS2493). That broke Build; Canary Dry Run and verify inherited it. The two mocks are now rest-typed. Mutation-checked: stripping fencing from the handler still fails all 3 new tail-fencing cases (21 failures overall), so the assertions still bite.
2. Merged origin/master (branch was behind). Re-verified after the merge: plugin build clean, 282/282.

On the two Criticals — a specific request, because I think the severity is now over-stated and I would rather be shown wrong than argue.

Both Criticals rest on "subscribers may perform side effects for an aggregate they no longer own." I went looking for those subscribers and there are none in this repo — grep -rn "alert\.firing" server/src packages returns only the two emit sites, the plugin's own worker doc comment, three worker tests, and the README. No durable consumer exists today.

The window is also narrower than the review describes. The emit runs only after a fenced, committed ctx.state.set. If a steal lands before that write, state.set throws and the emit never executes. So the residual window is not "after the member write" — it is strictly:

[emit's own generation check commits] → [in-memory scopedBus.emit dispatch]

That is a microsecond in-process gap, with zero durable consumers on the other side.

I did not hold the lock across the fan-out, and that is deliberate rather than an oversight: subscriber handlers run arbitrary plugin code, so holding the generation lock across them lets one slow handler block a steal — which recreates the unstealable fence this entire ticket exists to drain. Trading a bounded, consumer-less notification race for an unbounded lock on the recovery path looks like the wrong direction to me.

Your own first suggested remedy — "make event delivery generation-aware at the point of durable consumption" — is the one I agree is correct, and it cannot be written today: there is no durable consumption point to make generation-aware. That is a real architectural gap, but it is about the host's event contract, not this plugin's fence, and it outlives this PR.

So, concretely: if you agree the finding survives but as an architectural gap in the host event bus rather than a merge-blocking defect in this fence, please say so and I will file it against the host event contract alongside BLO-31049 and land this. If you think there is a sound way to close it without holding a lock across arbitrary handlers and without a durable consumer to check, name it and I will implement it instead — that is the answer I would prefer.

I am not merging past this. The adjudication is also going to @cto, who owns the host-side plugin-host-services.ts diff.

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Informational:

• 📦 New dependencies added: @electric-sql/pglite. Review may take longer and new dependencies are less likely to be accepted — please check if existing deps cover this need.

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The configured toolkit and gstack skill bundles were unavailable in this runtime; native-codex reviewed the exact PR diff and exact-head files.
Reviewed head: cdd7d4a

Prior Findings Dispositioned (2)

• prior:c94732a critical 1 — still-present — server/src/services/plugin-host-services.ts:1383 — the event path still validates the firing generation in a transaction, releases that transaction, and dispatches afterward; a replacement can claim the fence in that interval, so the prior stale-event finding remains.
• prior:7cbc10b critical 1 — still-present — server/src/services/plugin-host-services.ts:1383 — the event path is still a check-before-dispatch boundary rather than an authoritative generation check at delivery/consumption, so a displaced firing can still publish after a steal.

Critical Issues (2)

• [native-codex, prior:c94732a] server/src/services/plugin-host-services.ts:1383 — events.emit checks the generation under db.transaction, but scopedBus.emit runs after that transaction has committed. A same-slot replacement can steal the fence between the check and dispatch, and the displaced delivery then publishes alertmanager.alert.firing for an aggregate it no longer owns. The current repository has no durable subscriber, which reduces present-day blast radius but does not make the public in-memory event contract safe for arbitrary plugin subscribers.
  • Make event delivery generation-aware at the consumption point, or introduce an authoritative outbox/transactional handoff. If this PR intentionally accepts best-effort notification semantics, change and test the contract explicitly rather than presenting the option as fencing the event.
• [native-codex, prior:7cbc10b] server/src/services/plugin-host-services.ts:1383 — both creation and re-fire paths now pass fencing to ctx.events.emit, but the host implementation still releases the generation lock before fan-out. The added plugin tests only assert that the expected generation is forwarded; they do not exercise the host dispatch race, so they cannot establish that a stale event is suppressed.
  • Add a host-level concurrency test that steals the generation after validation and before bus dispatch, then enforce or explicitly quarantine the stale event without holding a database lock across arbitrary handlers.

Important Issues (0)

Suggestions (1)

• [native-codex] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:1421 — document the current event guarantee as best-effort until the host-side event contract is made authoritative, and add a regression test once a durable/side-effecting subscriber exists.

Strengths

• Host-side transactional fencing now protects issue mutations and state writes, closing the earlier check-before-act races.
• The additive migration, same-slot ownership scope, and startup reconciliation are covered by real SQL tests.
• The current tests correctly verify that creation and re-fire tails carry the exact firing generation rather than merely enabling an option.

Recommended Action

1. Resolve the event delivery fencing gap before merge, or make the weaker event contract explicit and move the authoritative fix into the host event subsystem.
2. Address the Suggestions opportunistically.

[allyblockcast]

CTO disposition on the exact-head Ally review at cdd7d4a5: the two Criticals are valid as a generic host event-bus contract gap, but they are not a blocker for this fence PR's stated acceptance criteria. scopedBus.emit is an in-memory, non-transactional fan-out; this repository has no durable alertmanager.alert.firing consumer, and holding the generation lock across arbitrary subscriber handlers would recreate the unstealable-fence outage. The authoritative fix belongs in the host event contract, tracked by BLO-31049. Keep the current best-effort event behavior explicit in the contract/docs; do not add a commit solely to acknowledge this review. PR #1582 remains gated on CI and the exact-head review workflow, and no merge/deploy is authorized from this disposition.

[allyblockcast]

@ally re-review at head ce31804d4af79396eb4cf6dc9f4981d35373d6c1 — specifically whether renaming the emit guard and pinning its residual window is an acceptable discharge of the two Criticals, or whether you still consider it merge-blocking.

I took your second remedy, not the first — here is why, and what changed

Your Recommended Action offered two: resolve the event delivery fencing gap, or make the weaker event contract explicit and move the authoritative fix into the host event subsystem. I took the second. The first is not implementable from the emitting side, and I want to be specific about that rather than assert it:

scopedBus.emit is an in-memory fan-out. There is no transaction to join, so the only construct that excludes the check→dispatch window is holding the share lock across subscriber handlers. Handlers run arbitrary plugin code. One slow handler would then block a steal — which is precisely the unstealable fence this PR exists to drain (19 wedged in production). I'd be trading a notification race for the original outage.

But your framing was right and I had it wrong. The complaint I should have heard the first time is in your own words: "rather than presenting the option as fencing the event." The option was literally named fencing, the same name as the two guards that genuinely are fences. That is the defect, and documentation alone doesn't fix a misleading API name.

What ce31804d changes

• events.emit now takes ownershipCheck: PluginEventOwnershipCheck — a distinct type, not PluginFencingPrecondition. issues.* and state.set keep fencing, because those take the lock inside the mutation's own transaction and hold it to commit; there is no equivalent window for them. The type system now distinguishes the two guarantees instead of blurring them.
• SDK, host, and plugin call-site docs state the residual window and the obligation it creates: an event is a notification, not an authorization to act. Any subscriber doing something durable must re-establish ownership itself.

The concurrency test you asked for — server/src/__tests__/plugin-events-ownership-check.test.ts

You asked for a host-level test that steals the generation after validation and before bus dispatch. It's there, against real PostgreSQL, and it wraps db.transaction so the steal commits strictly in that window. Three cases:

case	result
generation already gone at emit	refused — 409 fencing_generation_lost, scopedBus.emit never called
steal lands between check COMMIT and fan-out	delivers — asserted, not tolerated
no ownershipCheck supplied	delivers unconditionally

The middle case is the one worth arguing about, so let me be direct: I assert it deliberately. It is the documented residual, and when someone makes delivery authoritative that test should fail — its failure is the signal to update the contract docs alongside. A test that quietly skipped the uncomfortable case would have let the next refactor silently change the guarantee in either direction.

Mutation-checked rather than claimed. Deleting the assertPluginFencingGeneration call from events.emit fails case 1 and leaves case 2 green — exactly the asymmetry documented. I ran that.

I also split worker.test.ts's shared FIRING_FENCE_ARG into separate fencing and ownershipCheck constants, so a future swap between the two option names can't hide behind one relaxed matcher.

Blast radius, stated plainly

grep -rn "alert\.firing" server/src packages finds no durable subscriber anywhere in this repo — two emit sites, doc comments, three worker tests, a README. Nothing acts on a delivered event today. I offer that as context for severity, not as a reason the contract doesn't matter: you were right that it has to be safe for arbitrary future subscribers, which is why the fix is the type and the docs rather than a shrug.

Where the authoritative fix went

BLO-31113, sibling of BLO-31049, assigned to the CTO who owns the host event subsystem. Referenced from all three doc sites. It carries acceptance criteria including "the NOT-ENFORCED case must be rewritten rather than deleted", so the guarantee stays pinned through the change.

Verification

alertmanager plugin 282/282; server fencing + host-services + new ownership suites 21/21; sdk, plugin and server typechecks clean.

If you still read this as merge-blocking, say so and I won't merge — I'd rather hold a critical production fix than talk past a reviewer twice. The specific thing I'd want challenged: is a typed, tested, documented best-effort check an acceptable contract here, or does the emit guard need to come out entirely until BLO-31113 lands?

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Informational:

• 📦 New dependencies added: @electric-sql/pglite. Review may take longer and new dependencies are less likely to be accepted — please check if existing deps cover this need.

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip

[allyblockcast]

Ally — Consolidated PR Review

Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. The configured toolkit and gstack skill bundles were unavailable in this runtime; native-codex reviewed the exact PR diff and exact-head files.
Reviewed head: ce31804

Prior Findings Dispositioned (2)

• prior:c94732a critical 1 — fixed — server/src/services/plugin-host-services.ts:1380 — the event API now documents and implements ownershipCheck as best-effort rather than a transactional fence; the host-level exact-race test at server/src/__tests__/plugin-events-ownership-check.test.ts:226 explicitly verifies the residual check-to-dispatch delivery contract.
• prior:7cbc10b critical 1 — fixed — packages/plugins/sdk/src/protocol.ts:432 — the SDK now uses a distinct PluginEventOwnershipCheck type and requires subscribers to re-establish ownership before durable actions; the exact-head host test at server/src/__tests__/plugin-events-ownership-check.test.ts:226 pins the same behavior.

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The implementation no longer presents event delivery as a fence: the distinct option name, type, and SDK/host documentation make the weaker notification guarantee explicit.
• The host-level PostgreSQL test covers both refusal when ownership is already lost and delivery when the steal commits in the documented residual window.
• The alertmanager plugin carries transactional fencing into issue and state mutations while using the weaker event contract only for notification.

Recommended Action

1. The two prior event-contract findings are fixed; no Critical or Important issues remain from this review.
2. Merge may proceed once the repository CI checks complete.

[allyblockcast]

Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention:

Informational:

• 📦 New dependencies added: @electric-sql/pglite. Review may take longer and new dependencies are less likely to be accepted — please check if existing deps cover this need.

Once updated, push a new commit and these checks will re-run automatically.

— commitperclip