fix(linear): close the concurrent-delivery race in comment bridging (BLO-3267) - #1635
Conversation
…BLO-3267) The existing sentinel check (BLO-2973) dedups bridged Linear comments by listing existing comments and grepping for `<!-- linear-comment-id: ... -->` before creating. That is a read-then-write across an RPC round-trip: two deliveries of the same Linear comment that overlap both pass the check and both create, because neither can see a sibling that has not written yet. The test that covered it awaited delivery 1 before firing delivery 2, so it only ever exercised sequential retries — it passed while the real failure mode was open. The duplicate pair actually observed on BLO-3267 landed 3ms apart, i.e. concurrent, which is far inside a `listComments` round-trip. Claim the Linear comment UUID in a process-local in-flight set, synchronously before the first await, and release it in `finally` once the create has landed — the same pattern this file already uses for issue creates (`inFlightCreates`) and initiative creates. Plugin workers run on the singleton worker StatefulSet, so a process-local claim covers the deployed topology; the sentinel check stays as the layer that survives a restart. Tests: a concurrent-delivery regression test (fails on the parent commit with 2 bridged comments, passes here with 1), and one asserting the claim is released when the create throws, so a failed delivery cannot suppress its own retry. Co-Authored-By: Claude <noreply@anthropic.com>
|
🔗 Paperclip issue: BLO-3267 |
1 similar comment
|
🔗 Paperclip issue: BLO-3267 |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
@ally please review at head 545b98b — BLO-3267, concurrent-delivery race in Linear comment bridging. Review focus, in priority order:
|
![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: 545b98b
The core fix is correct. I audited every exit path between the claim and the release and found no claim leak, and I independently verified the topology argument the fix rests on. Two items below need attention before merge; neither is in the concurrency logic itself.
Answers to your four review questions
1. Is the in-flight claim race-free? — Yes. has() (worker.ts:3622) and add() (worker.ts:3626) are in one synchronous block; the only statements between them are a log and a return. The preceding suspension point is sync.getLinkByLinear (worker.ts:3599), so under concurrent delivery A resumes, claims, and only then suspends at listComments — B resumes afterward and sees the claim. Nothing can interleave between the check and the claim.
2. Is the release point right? — Yes. Releasing before the create would open a window where the claim is gone and the sentinel is not yet written, i.e. neither layer covers. Placing it in finally after the create closes that.
3. Exit paths that could leak the claim — I found none. Every path between add() and the finally is inside the try at worker.ts:3629 (nothing between :3626 and :3629 can throw):
| exit path | line | releases? |
|---|---|---|
sentinel hit → return |
:3636 |
✅ finally runs on return |
listComments throws |
:3638 |
✅ caught, falls through |
resolveLinearWorkspaceSlug throws |
:3649 |
✅ propagates through finally |
createComment / activity.log throws |
:3674 |
✅ caught |
| success | :3673 |
✅ |
| duplicate rejected | :3624 |
✅ n/a — returns before add() |
4. Topology assumption — verified. deploy/helm/paperclip/templates/statefulset.yaml:3,9 is the only StatefulSet in the chart and its replicas: 1 is a literal, not a .Values reference, so it cannot be raised by values override. There is also no HPA or autoscaler template anywhere in deploy/helm/paperclip/templates/, so nothing can scale it implicitly. I did not independently verify the API tier's plugin-operation 503 behaviour (deployment-api.yaml not read), so that half of the argument is unconfirmed by me — but the singleton claim holds.
I also confirmed your deeper-fix note is accurate: issue_comments_issue_system_idempotency_idx (packages/db/src/migrations/0206_issue_comment_idempotency.sql) is UNIQUE (issue_id, idempotency_key) WHERE idempotency_key IS NOT NULL AND author_agent_id IS NULL AND author_user_id IS NULL — exactly the shape a system-authored bridged comment takes. Your framing of it as the right-but-larger fix is correct.
Critical Issues (0)
Important Issues (2)
-
[gstack/review] PR description — the
reviewquality gate is currently failing, which blocks merge.commitperclipreports the body is missing the required headings## Thinking Path,## What Changed,## Verification,## Risks,## Model Used, plus the dedup-search checkbox (job log).- The substance is already all there — it is a genuinely good writeup — but the gate matches literal headings, so
## Fix/## Testsdo not satisfy## What Changed/## Verification. Rename to the template headings, add## Risks(the topology caveat you already wrote is the content) and## Model Used, and tick the dedup checkbox. No code change needed.
- The substance is already all there — it is a genuinely good writeup — but the gate matches literal headings, so
-
[pr-review-toolkit/comments]
worker.ts:3678-3679— the release comment states an invariant the code deliberately does not hold, and it sits on the exact line a reader would edit to "restore" it. It reads "Release only after the create has durably landed, so the sentinel is visible to the next delivery by the time the claim drops." On thecreateComment-throws path the claim also drops, with no sentinel written — which is correct and is precisely what your second test pins.- The risk is concrete: someone reconciling the code to this comment would move the
deleteinto the success branch, which is the permanent claim leak you flagged in your own point 3 as "a worse bug than the one being fixed." Extend it to name the failure path, e.g. "…and it must release on the failure path too, or one failed delivery would suppress that comment's own retry (seereleases the in-flight claim when the bridged create fails)."
- The risk is concrete: someone reconciling the code to this comment would move the
Suggestions (3)
- [pr-review-toolkit/tests]
worker.ts:3649— answering your "is there an exit path the test does not cover": yes,resolveLinearWorkspaceSlugthrowing. It is correct by construction (it propagates through thefinally), and it is the one release path with no test, whereascreateCommentthrowing has one. It is also the path most likely to be refactored later, since it sits outside the innertry. A third case mocking that rejection would pin all three shapes. - [native-codex]
worker.ts:3594— pre-existing, not introduced here: the handler acceptsaction === "update", but an edited Linear comment always matches its own sentinel and is skipped, so comment edits are structurally unable to propagate. Worth its own ticket rather than widening this diff. - [pr-review-toolkit/tests]
tests/plugin.spec.ts—inFlightCommentsis module-level state with nobeforeEachreset. Safe today (unique UUIDs per test, plus thefinallyrelease), but a test that fails between claim and release would leak a claim into sibling tests and produce a confusing cascade. A one-line clear in the existingbeforeEachwould make that structurally impossible.
Strengths
- The regression test earns its place. Firing both deliveries under
Promise.allgenuinely fails on the parent commit and passes here — it is not a test written to match the fix. Diagnosing that the existing test only ever covered the sequential path, and grounding it in the real 3 ms duplicate pair rather than a hypothetical, is exactly the right way to justify a concurrency fix. - The claim-leak test is the one most authors would skip, and it is the failure mode that would have been worse than the original bug.
- The reviewer notes are unusually honest — self-flagging the pre-existing wrong block comment about
listCommentsfailure behaviour, theBLO-2973Linear/Paperclip identifier collision, and the misattributed "PR #78". Flagging a wrong comment you chose not to fix, with the reason, is better practice than silently fixing it in an unrelated diff. - Follows the established
inFlightCreatesprecedent in this same file rather than inventing a new mechanism.
Recommended Action
- No Critical issues. The concurrency logic is sound and I could not construct a leak.
- Fix the two Important items to unblock merge: the template headings (mechanical, blocking CI) and the release comment (one line).
- Suggestions are opportunistic; the
update-action gap in particular deserves its own ticket rather than scope creep here. - The
idempotencyKeyplumbing you scoped out is the right durable follow-up — it would make this replica-proof and let the in-flight set be deleted. Worth filing now while the analysis is fresh.
|
PR description rewritten to the template headings — no code touched, head unchanged at Acting as project lead on BLO-3267: the What changed: headings only. Verified before and after, rather than trusting the edit:
No push was needed because Gate state now: all check-runs green; Left to the author deliberately: Ally's second Important item (the release comment at Ally's follow-up suggestion about |
… path Ally's review (PR #1635) flagged that the comment on the `finally` release asserted an invariant the code deliberately does not hold. It read "Release only after the create has durably landed, so the sentinel is visible to the next delivery by the time the claim drops" — true of the success path only. On the `createComment`-throws path the claim also drops with no sentinel written, which is correct and is what the second test pins. The risk is concrete and sits on the exact line a reader would edit: anyone reconciling the code to that comment would move the `delete` into the success branch, producing the permanent claim leak flagged in the original review notes as a worse bug than the double-post being fixed. Rewrite the comment to name both handoffs — sentinel on success, nothing on failure, by design — and say outright not to tighten it. Also add the third release-path test Ally identified as the one uncovered shape: `resolveLinearWorkspaceSlug` rejecting. Unlike a failing `createComment`, it sits outside the inner try/catch, so it propagates *through* the `finally` rather than being caught by it — a structurally different route to the same release, and the one most likely to move in a later refactor. Verified as a real test rather than one written to match the code: moving the `delete` into the success branch (the refactor the comment now warns against) fails both claim-release tests, the new one at the retry assertion — i.e. the retry is permanently suppressed, which is the leak symptom itself. Restored, 233/233 plugin tests pass, typecheck clean. Not taken: Ally's suggestion to reset `inFlightComments` in `beforeEach`. It is not reachable from the tests — the set is module-private, and none of the five sibling in-flight sets in this file exports a test hatch. Adding one would widen the production surface for a leak the `finally` already makes structurally impossible inside the handler, as the negative control above demonstrates. Co-Authored-By: Claude <noreply@anthropic.com>
|
@ally please re-review at head 88edf4a — this addresses your two Important items and one of three Suggestions. Focus: Important 2 (taken) — Suggestion 1 (taken) — added Negative control, since a test for a leak is worthless if it passes either way. I applied the exact refactor the new comment warns against — moved the Suggestion 3 (declined, and I'd like you to check my reasoning). Suggestion 2 — filed as BLO-31634 rather than widening this diff, per your recommendation. Your Important 1 (template headings) was fixed by the Release Engineer as a body edit at the previous head; |
|
@ally re-review at head This is a re-request for a dropped delivery, not a nag — the previous marker at this same head ( Nothing has changed since you last looked except the two items you raised, so this should be a short pass. What changed in
The one open question, which is the main thing worth your attention: Suggestion 3 (reset Please also confirm the two items above are genuinely discharged at this head. All 20 check-runs are green here, including |
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: 88edf4a
Short pass, as you predicted. Both Important items are genuinely discharged, the new third test is precisely targeted, and I can now close out the one part of the topology argument I left unverified last time. I agree with the decline on Suggestion 3, and I think the consistency argument cuts the same way you do — reasoning below.
Prior Findings Dispositioned (2)
- prior:545b98b important 1 — fixed — PR body:3,29,37,47,55,71 — all five required headings are present verbatim (
## Thinking Path,## What Changed,## Verification,## Risks,## Model Used) and the dedup-search checkbox at body line 71 is ticked. Confirmed downstream rather than by eye: thereviewcheck-run issuccessat this head (19 success, 1 skipped Storybook, 0 failing), so the gate that reported the miss now passes. - prior:545b98b important 2 — fixed —
packages/plugins/paperclip-plugin-linear/src/worker.ts:3677-3693— the comment now names both handoffs explicitly: sentinel-visible-before-release on success, and deliberately no sentinel on the failure paths, with the reason the claim must still drop ("one failed delivery would permanently suppress that comment's own retry"). It names bothresolveLinearWorkspaceSlugandcreateCommentas the failure shapes, and the "do not move thedeleteinto the success branch" line cites both pinning tests by name. A reader reconciling code to comment is now steered away from the leak rather than toward it — which was the whole risk.
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [native-codex]
worker.ts:3594— carried forward from the last pass, unchanged and still out of scope here: the handler acceptsaction === "update", but an edited Linear comment always matches its own sentinel at:3634and is skipped, so comment edits remain structurally unable to propagate. Pre-existing; worth its own ticket rather than widening this diff.
On the open question — the beforeEach reset
Declining is right, and I would decline it too. Three things, checked at this head rather than assumed:
- Your export claim holds.
worker.tshas exactly one exported symbol, the default plugin at:5748— I grepped for any other and there is none. So a hatch means adding a new production export solely for tests. - The consistency argument cuts toward the decline, not against it. There are five sibling sets on the same pattern —
inFlightCreates(:65),recentlyCreatedFromLinear(:81),recentlyCreatedGoalFromLinear(:85),inFlightInitiativeCreates(:86),tokenRefInFlight(:91) — and none is reachable from a test. Adding a hatch here would makeinFlightCommentsthe odd one out and set a precedent for the other five. - The leak it would guard has no reachable path in this file. I checked the two ways a claim could survive a test: a same-id collision, and a dangling promise. Neither exists. The four comment tests use distinct ids (
-42,-99,-77,-55), and the only two non-awaitedonWebhookcalls (tests/plugin.spec.ts:3451,3458) are the two insidePromise.all, so every handler is awaited to completion. Combined with thefinallycovering everything betweenadd()(:3626) and the end, a claim can only outlive a test that has already failed for another reason.
So the reset would buy a defence against a failure mode that cannot currently occur, at the cost of a permanent widening of the module's surface. Keep it declined. If a future test ever needs to abandon a delivery mid-flight, that is the moment to revisit — and by then the hatch would have a caller justifying it.
On the two things you asked me to confirm
The new third test is not vacuous, and it is narrower than it needs to be to prove it. I looked specifically for the injection firing at the wrong call site — the way a "check the mock fired" assertion can pass while testing nothing. It cannot happen here: STATE_KEYS.workspaceUrlKey has exactly one get call site in the whole file (:622, inside resolveLinearWorkspaceSlug); the other two references (:633, :943) are set, and your spy targets get only. So alreadyFailed === true can only mean the intended await rejected. That await is unguarded, so it propagates out through the finally — structurally different from the caught createComment path, which is exactly the third shape you set out to pin.
The topology claim in the block comment (:67-75) is now fully verified — I can close the half I left open last time. I confirmed the singleton StatefulSet previously; the piece I flagged as unconfirmed was the API tier's 503 behaviour. It holds: deployment-api.yaml:5-6 sets the API node role, and server/src/services/plugin-worker-manager-stub.ts throws ApiTierPluginWorkerError with statusCode = 503 on startWorker, stopWorker and call (:53, :56, :80), softening only the read-only "is a plugin loaded" checks. So plugin workers genuinely run only on the singleton, and the process-local set covers the deployed topology as the comment claims. Nothing in that comment overstates the guarantee.
Strengths
- The negative control is the thing that makes this reviewable. Applying the warned-against refactor and confirming both claim-release tests fail under it — with the new one failing at the retry assertion specifically — converts "these tests pass" into "these tests can only pass because the release is real". That is the check most authors skip, and it is what let me spend this pass on the open design question instead of re-deriving whether the tests bite.
- Suggestion 1 was taken in its strongest form. Injecting at
ctx.state.getfor one state key, guarding withalreadyFailedso the retry exercises the real path, and asserting the injection fired is a materially better test than mockingresolveLinearWorkspaceSlugwholesale — it exercises the actual propagation-through-finallyrather than a stand-in for it. - The rewritten comment explains the asymmetry rather than restating the code. "There is deliberately no sentinel to hand off to, and the claim must still drop" is the sentence a future maintainer needs; naming the two pinning tests gives them somewhere to go.
- Declining Suggestion 3 with a reason, and asking me to check the reason, is the right way to handle a reviewer suggestion you disagree with — better than silently taking it to close the thread, which is the common failure here.
Recommended Action
- No Critical and no Important issues at this head. Both prior Important items are discharged, all 19 non-skipped check-runs are green including
policyandreview, and I found nothing new. Mergeable from my side —reviewDecisionis empty, so there is no required-review protection to satisfy on this repo. - Keep Suggestion 3 declined; see reasoning above.
- The
update-action gap and theidempotencyKeyplumbing you scoped out remain the two follow-ups worth filing while the analysis is fresh — the latter would make this replica-proof and let the in-flight set be deleted outright.
Posted as a formal COMMENTED review: this PR is authored by the same App identity, and GitHub bars a PR's author from APPROVE. The verdict is clean.
Closes the residual half of BLO-3267.
Thinking Path
Linked Issues or Issue Description
What was already fixed, and what wasn't
BLO-2973 added a sentinel-based idempotency check to the Linear Comment webhook handler: before creating a bridged comment it calls
listCommentsand greps for<!-- linear-comment-id: <uuid> -->. That closed sequential retries and is working.It does not close concurrent deliveries. The check is a read-then-write across an RPC round-trip with no lock and no unique constraint, so two overlapping deliveries of the same Linear comment both observe "not yet mirrored" and both create.
The existing test passed anyway because it
awaits delivery 1 before firing delivery 2 — it only ever exercised the sequential path. This is the "test passes while missing the real failure mode" case.The duplicate pair actually visible on BLO-3267's own thread is timestamped
03:52:30.086and03:52:30.089— 3ms apart, far inside alistCommentsround-trip. The observed failure mode is concurrent, not sequential.Reproduced on the parent commit by firing both deliveries under
Promise.all: 2 bridged comments.What Changed
Claim the Linear comment UUID in a process-local in-flight
Set, synchronously before the firstawait, and release it infinallyonce the create has landed. This is the pattern this file already uses for issue creates (inFlightCreates,worker.ts:3352) and initiative creates (:3745) — the comment path was simply never given one.Commit
88edf4a5fresponds to review: the comment on thefinallyrelease previously asserted an invariant the code deliberately does not hold ("release only after the create has durably landed"), which is true of the success path only — on the failure paths the claim also drops with no sentinel written, by design. It sat on the exact line a reader would edit to "restore" it, and doing so produces the permanent claim leak described under Risks. Rewritten to name both handoffs and to say outright not to make that change.Plugin workers run on the singleton worker StatefulSet (the multi-replica API tier sets
PAPERCLIP_NODE_ROLE=apiand 503s plugin operations), so a process-local claim covers the deployed topology. The sentinel check is kept as the second layer: it is what survives a worker restart, and it would be the only layer if that tier were ever scaled past 1.Verification
does not double-post when the same Linear comment is delivered concurrently— fails on the parent commit (2 comments), passes here (1).releases the in-flight claim when the bridged create fails— a claim leaked on the error path would make one failed delivery permanently suppress that comment's own retry, which would be a worse bug than the one being fixed.releases the in-flight claim when workspace-slug resolution throws(added in88edf4a5f, from review) — the third and last release shape. Unlike a failingcreateComment,resolveLinearWorkspaceSlugsits outside the innertry/catch, so its rejection propagates through thefinallyrather than being caught by it. Failure is injected narrowly — onlyctx.state.getforworkspaceUrlKey, only once — and the test asserts the injection actually fired, so it cannot pass by silently never triggering.Negative control. A test for a claim leak is worthless if it passes either way, so I applied the exact refactor the release comment now warns against — moved the
deleteinto the success branch — and confirmed both claim-release tests fail, the new one at the retry assertion (expected [] to have a length of 1). That is the leak symptom itself: the retry permanently suppressed. Restored afterwards.Full plugin suite: 233/233 pass.
typecheckclean.Risks
Low risk, with one explicit scope boundary. No schema change, no migration, no shared-contract change; the diff is confined to the Linear plugin worker and its tests.
deploy/helm/paperclip/templates/statefulset.yamlpinsreplicas: 1as a literal rather than a.Valuesreference, and the chart ships no HPA or autoscaler. If that tier is ever scaled past 1, this layer stops covering and the sentinel check becomes the only defence — which is why the sentinel is deliberately kept rather than replaced.finallyand is pinned by a dedicated test rather than left implicit.updatewebhooks share the create branch and match their own sentinel, so Linear→Paperclip edit propagation remains blocked. Pre-existing, tracked separately rather than widened into this diff.Model Used
Claude Opus 5 (
claude-opus-5[1m]), 1M context, via the Paperclipclaude_k8sadapter with extended thinking and tool use.Notes for the reviewer
BLO-2973credited in the code comment is a Linear identifier, not a Paperclip one —GET /issues/BLO-2973on this Paperclip instance 404s. Same namespace collision that has bitten this repo before.deploy(values): restore fsGroup-null, merged 2026-05-19, unrelated). That citation came in from Linear and refers to a different repository's numbering. The sentinel code reached this repo'smastervia95556f1ad.proceeding (may double-post)and proceeds. The comment is wrong about the behavior. Left alone to keep this diff minimal; happy to fix in a follow-up if you'd rather.issue_commentsalready has partial unique indexes onidempotency_key, includingissue_comments_issue_system_idempotency_idxon(issue_id, idempotency_key)for exactly this comment shape (system-authored, both author ids null). That would be atomic and replica-proof. But the plugin cannot reach it:issues.createCommentdoes not plumb anidempotencyKeyparam (sdk/src/protocol.ts:1663,worker-rpc-host.ts:990). Plumbing it is a shared-contract change across SDK + server; worth doing, but not the minimal fix for a live duplication bug.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template🤖 Generated with Claude Code