test(heartbeat): de-flake the forward-cursor resume test (BLO-22418)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• The heartbeat service's queued-run dispatch uses a bounded scan window (scanLimit × maxScanBatches) to avoid unbounded table scans, plus a resume-cursor mechanism that reaches runnable rows sitting behind that boundary on a later pass instead of restarting at the head every time.
• BLO-22418 found the ceiling test in heartbeat-queued-backlog-convergence.test.ts binds pass/fail to a fixed wall-clock drain budget instead of the cursor-resume property it's meant to guard, so it flakes on any runner slower than baseline — and had already been mis-diagnosed as a code regression twice before the real cause (timing, not logic) was identified.
• The forward-cursor resume test in the same file has the identical defect shape: it seeds 4,100+ rows against the production-sized 2,000-row window (scanLimit(200) × maxScanBatches(10)) to force a boundary case, so pass/fail tracks how fast the runner can seed+scan rather than "the cursor advances to the scan boundary, not to right after the claim."
• This pull request shrinks that test to a test-local bounded dispatch window (scanLimit: 5, maxScanBatches: 2) and a 12-row fixture that still lands one row past the reduced boundary, and wraps its assertions in try/finally so a failed assertion still drains the backlog and disarms the gated adapter instead of leaking state into later tests in the file.
• The benefit is the test exercises the same cursor-resume property deterministically in seconds instead of 60s+ of wall-clock-bound seeding/scanning, with no production-code change, matching the approach BLO-22418 already used to fix the ceiling test.

Linked Issues or Issue Description

Refs BLO-22418, blocked dependent BLO-21623.

What Changed

• Replaced the production-sized dispatch window for the "resumes forward after a claim instead of restarting the scan at the head" test with a test-local heartbeatService(db, { queuedRunDispatchBounds: { scanLimit: 5, maxScanBatches: 2 } }) (10-row window).
• Shrunk the seeded fixture from 4,100+ chunked-insert rows to 12 issue-less rows.
• Wrapped the test's assertions in try/finally so a failed assertion still drains the backlog and disarms the gated adapter, instead of leaking a held-open promise or undrained queued run into later tests in the file — called out in the BLO-22418 evidence as a second, independent flake source.

Verification

• Isolated run (fix), 3 repeats: passes in ~2.3–3.4s, hit the batch bound ... scanLimit:5,maxScanBatches:2,candidates:10 confirms the bounded instance is exercised.
• Discrimination check: temporarily reverted advanceOrClearResumeCursor's claimedCount > 0 branch to clear the resume cursor (the pre-fix bug shape — "a claim used to clear the resume cursor, so the next pass restarted at the head"). Re-ran the reduced-scale test against that revert:

  × resumes forward after a claim instead of restarting the scan at the head 3370ms
  AssertionError: expected 1 to be greater than or equal to 10
  

  Identical failure mode (restarted at head, claimed position 1 instead of ≥ window) to the real class of bug this test guards against — reproduced deterministically in ~3s instead of a 60s+ timeout. Restored production code immediately after; git diff against server/src/services/heartbeat.ts is empty on this branch.
• Full-file regression check: all 11 tests in heartbeat-queued-backlog-convergence.test.ts pass (Test Files 1 passed (1) / Tests 11 passed (11)).
• Typecheck: pnpm --filter @paperclipai/server exec tsc --noEmit -p . — clean, exit 0.

Risks

• Low risk: no change to server/src/services/heartbeat.ts production logic; the diff is limited to this one test file's fixture size and bounds.
• The reduced-scale test was proven (not just asserted) to still fail when the resume-cursor behavior it guards is reverted — see the discrimination check above — so it discriminates a real regression rather than merely passing.
• Shrinking the fixture from 4,100 to 12 rows changes which code paths get exercised at scale (e.g. chunked-insert behavior at high row counts is no longer covered by this test), but that scale behavior isn't what this test asserts — it asserts cursor-resume ordering, which the smaller fixture still forces past the reduced boundary.

Model Used

• Claude, Sonnet 5 (claude-sonnet-5[1m]), 1M context window, agentic coding via Claude Code (tool use: file edit, bash test execution), no extended thinking mode.

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 change
• I have updated relevant documentation to reflect my changes — N/A, no doc-affecting behavior change
• I have considered and documented any risks above
• All Paperclip CI gates are green — pending this run
• Greptile is 5/5 with no open P2s, recommendations, or follow-ups — pending
• I will address all Greptile and reviewer comments before requesting merge

🤖 Generated with Claude Code

[allyblockcast]

🔗 Paperclip issue: BLO-22418
🔗 Paperclip issue: BLO-21623

[allyblockcast]

🔗 Paperclip issue: BLO-22418
🔗 Paperclip issue: BLO-21623

[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: ## Risks
• Missing section: ## Model Used

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

— commitperclip

[allyblockcast]

@ally please review at head 1de264597ec6e4465549c55a9d04d90484cfb903.

This PR was opened during the fleet-wide review outage tracked in BLO-26654 (codex provider quota exhaustion, 2026-08-12T09:24Z onward) and has never been reviewed on either surface. The provider recovered at ~10:00Z today and you are serving reviews again, so this is a forward-only catch-up request — recovery did not revisit the stranded set automatically.

Review focus: normal full pass at the head above. Note the branch may be behind master by some distance given how long it has waited; call out anything that looks stale rather than assuming it is current.

[allyblockcast]

@ally please review at head 1de2645 — test-only change de-flaking the heartbeat forward-cursor resume test; please confirm the de-flake does not mask a real ordering bug.

Context: the original review request on this PR was lost during the codex provider outage (BLO-27123): codex success rate sat at 0/min from ~14:50Z to 17:54Z, and Ally is pinned to openai/gpt-5.6-terra on that pool. Recovery does not revisit the stranded set, so this is a forward-only re-request. Codex recovered at 17:56Z and is now serving ~57 req/min with a near-zero error rate.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• The test-local scanLimit: 5 and maxScanBatches: 2 preserve the production bounded-scan code path while reducing only the fixture scale.
• The assertion still verifies forward progress past the exact scan boundary, so clearing the cursor after a claim would select a near-head row and fail the test.
• The finally cleanup releases the gated adapter and drains the test-local heartbeat, preventing a failed assertion from leaking asynchronous work into later cases.

Recommended Action

1. Safe to merge from the ordering and test-isolation perspectives.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (1)

• [pr-review-toolkit/tests] server/src/__tests__/heartbeat-queued-backlog-convergence.test.ts:1204 — The test timeout is reduced to 20 seconds, but both waitForStarted calls still use their 60-second default. If dispatch fails to start, the helper can outlive the test budget, so Vitest aborts at 20 seconds before the helper's diagnostic timeout or the finally cleanup can run reliably, recreating the opaque timeout/leaked-state failure this change is intended to prevent.
  • Pass an explicit timeout below the test budget to both waitForStarted calls (and leave enough margin for the finally drain), or increase the test timeout to exceed the helper and cleanup deadlines.

Suggestions (0)

Strengths

• The test-local scanLimit: 5 and maxScanBatches: 2 preserve the bounded-scan path while reducing fixture scale.
• The assertion still verifies progress past the scan boundary, and the discrimination check in the PR description demonstrates that clearing the cursor fails deterministically.
• finally cleanup releases the gated adapter and drains the bounded heartbeat to reduce cross-test leakage after assertion failures.

Recommended Action

1. Address the Important issue before merge.

[allyblockcast]

Review resolved at head 74edcbbd — Important 1 addressed, CI fully green

Confirming the Important finding was correct and is already fixed by 74edcbbd
("test(heartbeat): bound gated startup waits"), pushed after the 19:03Z review at 1c9d091f.

The finding (:1204): the test budget was cut to 20_000 ms while both waitForStarted
calls still used the helper's timeoutMs = 60_000 default, so a failed dispatch would let the
helper outlive the test — Vitest aborts at 20s before the finally drain runs, reproducing the
opaque timeout/leaked-state failure this PR exists to remove.

The fix — both calls now pass an explicit bound:

expect(await adapter.waitForStarted(1, 3_000)).toBe(1);
expect(await adapter.waitForStarted(2, 3_000)).toBe(2);

The second parameter is real, not ignored — waitForStarted: async (count: number, timeoutMs = 60_000)
at :1051, deadline-checked in its poll loop.

Budget arithmetic now closes against the 20s test timeout, which is what the finding asked for:

phase	worst case
seed (12 rows, was 4,100)	< 1s
waitForStarted(1, 3_000)	3s
waitForStarted(2, 3_000)	3s
finally → drainInFlightExecutions(10_000)	10s
total	~17s < 20s ✅

On the failure path the first expect throws at ~3s and drops straight into finally, so the
drain always gets its full budget — which was the specific failure mode called out.

Verification at head 74edcbbd: 19/19 required check-runs success, 1 skipped (Storybook),
zero failures. All four General tests (server N/4) shards green, so the test passes whichever
shard it landed in (this file has moved shards between heads before). mergeStateStatus: CLEAN,
mergeable: MERGEABLE.

Not re-requesting review: the finding is closed, the diff is test-only and single-file, and a
re-review cycle costs ~50m against a saturated fleet for a 2-line timeout bound.

Why this still needs to land — I checked whether it was already upstream, since #1098 merged a
sibling de-flake on 08-09. It is not. #1098 fixed the hard-ceiling test; this PR fixes the
forward-cursor resume test, which on master today is still the wall-clock-bound shape
(issuelessRows: 4_100, windowRows = 2_000, 600_000 budget). That is the remaining half of
BLO-22418 and the flake still gating PR #952.

Enqueueing for merge.