fix(alertmanager): stop resolve auto-cancel from evicting a live execution lock (BLO-29908)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• The execution lock (checkoutRunId / executionRunId / executionAgentNameKey / executionLockedAt) is the only guardrail stopping two runs from doing the same work; checkout is mandatory precisely because that lock is enforced atomically
• updateIssue clears all four lock columns on any transition out of in_progress (server/src/services/issues.ts:9900). That is correct for the holding run closing its own work, but it means a third-party actor that writes a status silently evicts whoever held the row
• paperclip-plugin-alertmanager was exactly such an actor: on status=resolved it cancelled the tracked issue after checking only that the status was not already terminal — never that a run held it
• Observed live on 2026-08-23: a run checked out BLO-25023 at 15:22:08, wrote its findings document at 15:24:24, and the bridge cancelled the row at 15:24:54, nulling all four columns. The run got no signal; a later PATCH would have written to a row it no longer held, and another run could have checked the same row out concurrently
• Because LLMProxy* rules carry keepFiringFor: 0 and the underlying fault cycles in ~16-minute blocks, this ran about twice an hour per alert — four rows were evicted together at 15:24Z
• This pull request makes the resolve-driven cancel state its precondition instead of assuming it: both lock columns are pinned to null, so a held row fails the write and is annotated rather than cancelled
• The benefit is that an investigation in progress is no longer deleted from the board by its own alert clearing — and a sustained flapping fault stops erasing the evidence that it is sustained

Linked Issues or Issue Description

Paperclip issue: BLO-29908

Related, not fixed here:

• BLO-29905 — the route flap that drives the cancel cycle
• BLO-29393 — keep_firing_for on the LLMProxy rules; reduces the frequency but not the defect
• BLO-24234 — the adjacent, already-fixed re-fire edge (re-fire onto a terminal row). This PR is the entry into that terminal state; re-opening on the next fire does not restore a dropped checkout

Distinct from #923, which is intake-side (aggregate-safe issue creation). This is resolve-side.

What Changed

• packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts — the resolve cancel now passes expectedCurrentCheckoutRunId: null and expectedCurrentExecutionRunId: null. On a precondition failure the plugin posts a one-per-holding-run annotation naming the run, logs, and emits alertmanager.resolved.cancel_withheld. Non-precondition errors still propagate so Alertmanager retries.
  • Both columns are pinned deliberately: an issue can be held via checkoutRunId with executionRunId still null (BLO-19749), which a one-column guard would miss.
  • Precondition failures are matched on the shared "before the update could be applied" suffix — these are the only preconditions this call sets, so any of the three variants means exactly one thing.
• packages/plugins/paperclip-plugin-alertmanager/src/types.ts — AlertStateRecord gains diagnostic-only cancelWithheldForRunId / cancelWithheldAt, so "open even though its alert cleared" is answerable from the state row.
• packages/plugins/sdk/src/types.ts — the two expectation fields are now part of the typed issues.update surface, documented as what any plugin transitioning an issue it does not hold should pass.
• server/src/services/plugin-host-services.ts — lifts the preconditions out of patch (so they are not logged as attempted column writes) and forwards them explicitly rather than by implicit spread, with normalizeExpectedRunId rejecting malformed values instead of degrading them to undefined.

Deliberately not changed: updateIssue's lock-clearing at issues.ts:9900. Making that conditional would alter semantics for every writer in the product, including the holding run's own legitimate release and the recovery paths that clear wedged locks. The defect is a third party writing a status it has no claim on, so the guard belongs at that call site.

Verification

# Plugin suite (includes 4 new tests under "resolve does not evict a live execution lock")
cd packages/plugins/paperclip-plugin-alertmanager && npx vitest run
#   Test Files  7 passed (7)   Tests  254 passed (254)

npx tsc --noEmit -p tsconfig.json    # clean

The new tests fail on master. Reverting only webhook-handler.ts and re-running:

× pins BOTH lock columns on the cancel, not just executionRunId
× leaves the row untouched and annotates it when the lock is held
× annotates once per holding run, not once per flap cycle
✓ rethrows a non-precondition failure so Alertmanager retries   (regression guard; passes either way)

The tests are the observed trace: a resolved webhook delivered against a fingerprint whose issue is in_progress with both lock columns naming run 0657f242-….

Server typecheck was run (npx tsc --noEmit -p server/tsconfig.typecheck.json) — 834 pre-existing errors repo-wide under that config, none in any file this PR touches.

Gap a reviewer should close: I could not run the server test suite (no Postgres in this environment), so the host→service forwarding of the two preconditions is verified by code reading plus the explicit forwarding above, not by execution. The end-to-end assertion worth adding in CI is BLO-29908's own: check out an alertmanager-origin issue as run R, deliver a resolved webhook for that fingerprint, and assert the three lock columns still name R and status is not cancelled.

Risks

Low-to-moderate, and biased toward safety.

• Behavioral shift: a resolved alert whose issue is held no longer auto-closes. That is the point, but it means such rows stay open until the holder disposes of them. This is a stated choice (BLO-29908 acceptance criterion b) — the holder is the correct owner of that decision, and a later re-fire refreshes the row in place per BLO-24234.
• Stale locks: the guard pins on the lock columns, not on run liveness, so a row whose holder died without releasing also stops auto-cancelling. Fail-closed was chosen over reading run liveness in the plugin because the only liveness source available to it (getOrchestration().runs) derives run→issue from heartbeatRuns.contextSnapshot ->> 'issueId' and caps at 100 rows — a holder whose snapshot lacks issueId would read as dead and be evicted anyway, which is the bug. The host's own isTerminalOrMissingHeartbeatRun is the authoritative predicate; exposing it as a precondition (expectedNoLiveExecutionLock) is the precise follow-up and would let this guard narrow to live holders only.
• No migration. The new AlertStateRecord fields are optional; existing state rows read as undefined.
• Sibling instance left in place: escalation.ts:278 cancels a cover issue through the same unguarded path. Not fixed here to keep the diff scoped to the filed defect; filed as follow-up.

Model Used

Claude (Anthropic), claude-opus-4-6, extended thinking, with tool use / code execution — running as the PlatformSREEngineer agent in Paperclip.

Checklist

• I have included a thinking path that traces from project context to this change
• I have specified the model used (with version and capability details)
• I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work
• I have searched GitHub for duplicate or related PRs and linked them above
• I have either (a) linked existing issues with Fixes: # / Closes # / Refs # OR (b) described the issue in-PR following the relevant issue template
• I have run tests locally and they pass
• I have added or updated tests where applicable
• If this change affects the UI, I have included before/after screenshots — n/a, no UI surface
• I have updated relevant documentation to reflect my changes — SDK contract documented inline at the new fields; no behavior doc covers this path
• I have considered and documented any risks above
• All Paperclip CI gates are green — not yet run at open time
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — not yet reviewed

[allyblockcast]

🔗 Paperclip issue: BLO-29908
🔗 Paperclip issue: BLO-29393
🔗 Paperclip issue: BLO-25023
🔗 Paperclip issue: BLO-29905
🔗 Paperclip issue: BLO-24234
🔗 Paperclip issue: BLO-19749

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (1)

• [native-codex] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:848-878 — the holding-run diagnostic can become permanently keyed to unknown. If the initial issues.get sees no lock (or a lock that is released) and another run acquires the issue before the guarded update executes, the CAS correctly fails, but holderRunId is still null and the code stores cancelWithheldForRunId = "unknown". The next resolve sees the unknown marker and suppresses its notification even if a different run now holds the issue, contradicting the stated per-holding-run behavior and hiding the actual owner. Re-read the issue after the CAS conflict to obtain the current lock owner, or avoid persisting/marker-deduping an unknown owner when the owner cannot be established.

Suggestions (1)

• [pr-review-toolkit] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:791-804 — the list-then-create marker check is not atomic, so concurrent duplicate resolved deliveries can still post two annotations for the same run. Consider an idempotent server-side comment marker or a state-store/CAS-based notification claim if exactly-once annotation is required.

Strengths

• The cancel uses transaction-level preconditions on both lock columns, covering checkout-only ownership as well as execution ownership.
• Non-precondition failures are rethrown, preserving Alertmanager retry semantics.
• The new tests cover both lock columns, withheld cancellation, repeated resolution, and transient failure handling.

Recommended Action

1. Fix the Important issue before merge.
2. Consider making the notification marker claim atomic.

[allyblockcast]

@allyblockcast Please re-review commit 32b6caec7 for the resolved-alert lock-holder diagnostic race.

The CAS conflict path now re-reads the issue and uses the current checkout/execution holder when available; it no longer persists unknown as a notification marker. Added regression coverage for a holder acquired between the initial read and the guarded update.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:ad7429a important 1 — fixed — packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:882-887 — after the guarded update conflicts, the handler re-reads the issue and derives the diagnostic run from the current checkout/execution holder instead of persisting an unknown marker.

Critical Issues (0)

Important Issues (1)

• [native-codex] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:882-887 — the fallback still reuses the stale pre-CAS holder when the post-conflict read finds no holder. If the initially observed run releases before the guarded update reaches the transaction, the CAS can fail due to the intervening lock, then currentIssue can be unheld; the ?? holderRunId fallback records the released run as the current holder and creates a marker that can suppress notification for a later, different holder.
  • Only persist and deduplicate a run ID confirmed by the post-conflict read. If no holder is present after the conflict, avoid writing a holder-specific marker/comment (or use a separate non-holder diagnostic that cannot suppress a future holder).

Suggestions (1)

• [pr-review-toolkit] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:790-804 — the list-then-create marker check is not atomic, so concurrent duplicate resolved deliveries can still post two annotations for the same run. Consider an idempotent server-side marker or state-store compare-and-set if exactly-once annotation is required.

Strengths

• The cancel uses transaction-level preconditions on both lock columns, covering checkout-only ownership as well as execution ownership.
• Non-precondition failures are rethrown, preserving Alertmanager retry semantics.
• The regression coverage exercises both lock columns, withheld cancellation, repeated resolution, transient failures, and the post-conflict holder re-read.

Recommended Action

1. Fix the Important issue before merge.
2. Consider making the notification marker claim atomic.

[allyblockcast]

Addressed the important stale-holder fallback in commit d8e9de1.

• The post-conflict read is now the sole source of cancelWithheldForRunId.
• If that read finds no holder, no holder-specific comment or marker is written.
• Added a regression test for the initial holder releasing before the CAS conflict is observed.
• The list-then-create marker race remains a non-blocking suggestion; exact-once annotation is not required for the acceptance criteria and is outside this targeted fix.

Verification: Test Files 1 passed (1), Tests 128 passed (128) for the alertmanager worker suite; plugin typecheck passes.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:32b6cae important 1 — fixed — packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:881-895 — after a guarded cancel conflicts, the handler re-reads the issue and only uses a holder confirmed by that post-conflict read; an unheld row produces no holder-specific marker, so a released run cannot suppress notification for a later holder.

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [pr-review-toolkit] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:790-804 — the list-then-create marker check is not atomic, so concurrent duplicate resolved deliveries can still post duplicate annotations for one holding run. Consider an idempotent server-side marker or state-store compare-and-set if exactly-once annotation is required.

Strengths

• The cancel uses transaction-level preconditions on both lock columns, covering checkout-only ownership as well as execution ownership.
• The post-conflict re-read and regression tests close the prior stale and unknown-holder marker cases.
• Malformed preconditions are rejected, and non-precondition failures still propagate so Alertmanager retries.

Recommended Action

1. Consider making the notification marker claim atomic.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:32b6cae important 1 — fixed — packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:881-895 — after a guarded cancel conflicts, the handler re-reads the issue and only uses a holder confirmed by that post-conflict read; an unheld row produces no holder-specific marker, so a released run cannot suppress notification for a later holder.

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [pr-review-toolkit] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:790-804 — the list-then-create marker check is not atomic, so concurrent duplicate resolved deliveries can still post duplicate annotations for one holding run. Consider an idempotent server-side marker or state-store compare-and-set if exactly-once annotation is required.

Strengths

• The cancel uses transaction-level preconditions on both lock columns, covering checkout-only ownership as well as execution ownership.
• The post-conflict re-read and regression tests close the prior stale and unknown-holder marker cases.
• Malformed preconditions are rejected, and non-precondition failures still propagate so Alertmanager retries.

Recommended Action

1. Consider making the notification marker claim atomic.

[allyblockcast]

Ally — Consolidated PR Review

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

Prior Findings Dispositioned (1)

• prior:32b6cae important 1 — fixed — packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:881-895 — after a guarded cancel conflicts, the handler re-reads the issue and only uses a holder confirmed by that post-conflict read; an unheld row produces no holder-specific marker, so a released run cannot suppress notification for a later holder.

Critical Issues (0)

Important Issues (0)

Suggestions (1)

• [pr-review-toolkit] packages/plugins/paperclip-plugin-alertmanager/src/webhook-handler.ts:790-804 — the list-then-create marker check is not atomic, so concurrent duplicate resolved deliveries can still post duplicate annotations for one holding run. Consider an idempotent server-side marker or state-store compare-and-set if exactly-once annotation is required.

Strengths

• The cancel uses transaction-level preconditions on both lock columns, covering checkout-only ownership as well as execution ownership.
• The post-conflict re-read and regression tests close the prior stale and unknown-holder marker cases.
• Malformed preconditions are rejected, and non-precondition failures still propagate so Alertmanager retries.

Recommended Action

1. Consider making the notification marker claim atomic.

[allyblockcast]

Thanks for the review. I agree the list-then-create check is not atomic under concurrent resolved deliveries. I am leaving it unchanged in this PR because the marker is an operational notification and the acceptance criterion is observable notification of the holding run, not exactly-once annotation; duplicate comments are harmless and do not affect lock safety or state. Making this atomic would require extending the plugin SDK/host comment contract with an idempotency key or adding a state-store CAS, which is a separate cross-layer API change. The guarded cancel and post-conflict holder re-read remain the correctness boundary for BLO-29908.