test(recovery): bound the raced attempt count instead of pinning it (BLO-33410)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• When an agent run dies mid-execution, the heartbeat's stranded-issue recovery sweep reserves a single issue_recovery_actions row so the work gets exactly one recovery owner, not one per racing sweep
• heartbeat-process-recovery.test.ts guards that dedupe invariant by firing 8 concurrent reconcileStrandedAssignedIssues() calls and asserting one row survives
• Alongside that real invariant it also asserted an exact attemptCount — that precisely 5 of the 8 racing reserves land — while its own comment ten lines below documents the count as scheduling-dependent and records measurements of 4, 6 and 7
• Those two statements are mutually exclusive, so the test fails whenever the race falls anywhere but exactly 5, which is roughly 2 runs in 3 on unchanged master
• This pull request replaces that equality with the bound the code actually guarantees, and leaves every real invariant untouched
• The benefit is that a merge-gating suite stops going red on unrelated PRs, which is what trains reviewers to wave through red checks

Linked Issues or Issue Description

• Fixes: BLO-33410
• Refs BLO-33385 — the flake was found while verifying that change (fix(heartbeat): retry issue-scoped pre-adapter process_lost runs (BLO-33385) #1774); it is pre-existing on master and unrelated to it
• Refs BLO-33657 — this flake failed the General tests (server 3/4) shard on BLO-33657: publish the Ally comment gate as a check-run so "not evaluated" stops rendering green #1815 at 61e3bbec on 2026-09-13, which is what surfaced that this fix had been pushed but never opened as a PR

Searched for duplicates. No open or closed PR references BLO-33410. Five open PRs touch this same test file (#1811, #1799, #1794, #1789, #1774) — I checked each patch and none touches the attemptCount assertion, so there is no overlap and no expected conflict.

What Changed

• server/src/__tests__/heartbeat-process-recovery.test.ts: replace expect(actions[0]?.attemptCount).toBe(Math.min(8, defaultRecoveryActionMaxAttempts)) with a bound — toBeGreaterThanOrEqual(1) and toBeLessThanOrEqual(8).
• Same file: drop the now-false comment clause claiming the budget "is not over-spendable by the race".

Why a bound is the correct assertion, not a smaller number. attemptCount was never a gate. The reserve is a read-modify-write (existing.attemptCount + 1), so it counts sweeps that landed, while defaultRecoveryActionMaxAttempts bounds wakes rather than increments. 8 racing sweeps can therefore legitimately carry the count past the budget — escalated is sticky, so the terminal state stays correct either way. 8 is the real ceiling, one increment per concurrent caller, so this assertion still fails if a retry path ever double-increments.

Verification

npx vitest run server/src/__tests__/heartbeat-process-recovery.test.ts \
  -t "reuses the raced stranded recovery action"

Baseline on unmodified master, measured 2026-09-11: 1 of 3 green — expected 6 to be 5, expected 7 to be 5 on separate checkouts. Independently reproduced on 2026-09-13 in CI on an unrelated PR (#1815, head 61e3bbec, job 103682776748): AssertionError: expected 4 to be 5, after an automatic retry x1, as the single failure in a 940-test shard (1 failed | 939 passed).

The two assertions that carry the test's actual meaning are untouched:

• expect(actions).toHaveLength(1) — the dedupe invariant this test exists for.
• The if/else pairing below — retired iff the budget was reached, with retiringBound: "attempt_budget" stamped. Note this branch was previously unreachable: the equality on the line above required attemptCount === 5, so the else arm (attemptCount < budget → still active) could never execute. This change is what makes the author's own branch testable.

Risks

Low, and scoped to one test. No production code changes — the diff is a single test file, +10/−3.

The one thing given up is the exact-count assertion, deliberately: it could not hold, because the quantity it pinned is scheduling-dependent by the test's own documented measurements. Weakening a bound in order to go green is normally the wrong move, so it is worth being explicit that the invariants with teeth (dedupe; budget/retirement pairing) are both retained, and that the replacement bound of 8 still fails on a double-increment regression.

Not verified in this PR: I have not run the 10-consecutive-green sequence that BLO-33410's acceptance criteria ask for, nor the negative control it requires (break the dedupe path, confirm the test goes red). Both need a working checkout; this PR is opened from an already-pushed commit whose originating workspace failed git-worktree validation. Flagging rather than claiming it.

Model Used

Claude Opus 4.5 (claude-opus-4-5, 1M context), extended thinking, with tool use via the Paperclip agent runtime.

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 — not in this run. The commit predates this run and its originating workspace failed worktree validation; the measurements above are from the original authoring run plus CI.
• I have added or updated tests where applicable — this PR is the test change
• 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 — the in-test comment is the documentation and is corrected here
• I have considered and documented any risks above
• All Paperclip CI gates are green — unknown at open time
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — not yet run
• I will address all Greptile and reviewer comments before requesting merge

[allyblockcast]

🔗 Paperclip issue: BLO-33410
🔗 Paperclip issue: BLO-33385
🔗 Paperclip issue: BLO-33657

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The test now asserts the actual concurrency invariant: one recovery-action row, with a bounded persisted count across the eight racing callers.
• The existing retirement-bound pairing remains covered, including both budget-exhausted and still-active outcomes.
• The change is narrowly scoped to the flaky assertion and removes the contradictory exact-count assumption without changing production behavior.

Recommended Action

1. No Critical or Important issues found; the App-authored PR is ready for the repository's remaining CI checks.