fix(heartbeat): single-flight the periodic recovery chain (BLO-30203)

[allyblockcast]

Thinking Path

• Paperclip is the open source app people use to manage AI agents for work
• The control plane is split into an API tier (paperclip-api, 2 replicas) and a singleton worker tier (paperclip-0), which owns the heartbeat scheduler and all periodic reconcilers
• The worker climbs to its 6 GiB --max-old-space-size ceiling and SIGABRTs (exit 134), losing every in-flight agent run each cycle; the cadence has degraded from ~47 h to ~5 h
• The API pods run the identical image digest with the identical NODE_OPTIONS and are memory-flat (~0.4 GiB, GC reclaims fully), which rules out the ORM, the pool implementation, prom-client, the HTTP layer and every import-time cache, and localises the retainer to code only the worker runs
• Live worker metrics rule out the other broad classes: nodejs_active_resources_total is flat at 194 (no handle/timer/socket leak) and nodejs_external_memory_bytes is negligible (not a Buffer/native leak), leaving ordinary JS objects in old space
• This pull request adds the missing single-flight latch to the periodic recovery chain, so it can no longer run concurrently with itself
• The benefit is that only one hydrated snapshot of the issue graph is retained at a time instead of one per overlapping pass

Linked Issues or Issue Description

Refs #30203 — paperclip-0 leaks and dies of a V8 heap-limit abort
Refs #30218 — capture a live heap snapshot before the next abort

• I searched for similar open/closed PRs and confirmed this is not a duplicate (open PRs grepped for heap|leak|memory|in-flight|latch|overlap|scheduler|reconcil|sweep; the only adjacent PR is fix(deploy): raise paperclip memory request 2Gi -> 7Gi (PEN-2957) #1643, which raises the memory request — a scheduling stopgap, not this defect)

What Changed

• Added server/src/services/single-flight.ts — createSingleFlight(), a latch that starts a task only when no previous invocation is still settling and returns null when it declines.
• server/src/index.ts: the periodic recovery chain (resumeRunningExternalRuntimeRuns → reapOrphanedRuns → promoteDueScheduledRetries → resumeQueuedRuns → reconcileStrandedAssignedIssues → …) now starts through that latch. It is still registered with trackHeartbeatSchedulerWork when it actually starts, so shutdown still drains it, and it is still not awaited by the other passes in the tick, so they keep their own cadence.
• The 95-line chain body is unchanged — only the head and tail lines of the call site moved, so the diff is reviewable.

Why the chain is expensive enough for this to matter. setInterval fires every 30 s (heartbeatSchedulerIntervalMs fallback 30_000, config.ts:457) and does not wait for the previous callback. The chain is far slower than one tick:

• recovery/service.ts:7965 — db.select() (every column, including description) .from(issues) with no .limit(), then ~5 awaited queries per candidate.
• recovery/service.ts:9564 collectIssueGraphLiveness — two unbounded scans of issues per call, the second (:9604) selecting description solely to derive one boolean.

So each overlapping copy retains its own full hydrated snapshot of the issue graph.

server/src/index.ts:1069-1081 already spells out this exact hazard, for the crashReconcileSweepInFlight latch:

setInterval starts the next callback on schedule regardless of whether the previous one has settled, and a reconciliation batch can easily outlive the interval.

That reasoning was correct and was simply never applied to this chain. This PR applies it.

Verification

• New unit test server/src/__tests__/single-flight.test.ts (5 cases): a second start is declined and the task is not invoked at all (invoking and discarding would still do the DB work and still retain the snapshot); the latch re-opens after settle; it re-opens after rejection and after a synchronous throw, so one failure cannot wedge the sweep permanently; separate latches stay independent.
• Run with pnpm --filter @paperclipai/server test single-flight.
• Locally verified (updated after opening the PR — I initially reported only the weaker checks below, then fetched a standalone tsc):
  • server/src/services/single-flight.ts fully typechecks under --strict (tsc --noEmit --strict --target es2022 --module esnext --moduleResolution bundler). It has no imports, so this is a complete check of the new logic, not a proxy.
  • server/src/index.ts and the test file parse clean — no TS1xxx syntax errors under tsc --noResolve. A full typecheck of index.ts needs the workspace node_modules, which I do not have.
  • The helper's behaviour executed under plain node with types stripped: all 9 assertions pass (declines without invoking the task; re-opens after settle, after rejection, and after a synchronous throw; separate latches independent).
  • Brace/paren balance of index.ts against origin/master: identical.
• CI typecheck + General tests (server *) remain the authoritative gate — I cannot run vitest or resolve the workspace graph here.
• Post-deploy signal, which is the one that actually closes BLO-30203: nodejs_heap_size_used_bytes{pod="paperclip-0"} should stop climbing monotonically and stay bounded across ≥48 h, and paperclip_db_pool_waiting_queries should stop climbing with a rising floor (measured today: 102 waiters at 70 minutes uptime with old space at only 20 % of the ceiling and event-loop lag p99 at 20 ms — i.e. the queue is pool contention from concurrent callers, not a stalled event loop).

Risks

Low-to-moderate, and the trade-off is worth naming explicitly rather than burying. When a pass overruns the 30 s tick, the next tick is skipped, so the stages inside the chain run at the chain's own completion cadence rather than every 30 s. Every stage is an idempotent sweep, so a skipped tick simply reconciles on the next one, and today those stages are already competing with N overlapping copies of themselves for a 10-connection pool — so effective throughput should improve, not degrade. The latch is per-chain and does not block the other passes registered in the same tick.

Not addressed here, deliberately, and each deserves its own change: the unbounded scans themselves, the duplicate full-table scan at service.ts:9565 (whose result is consumed only as rows.map(row => row.id) at :9729), and pulling description through the classifier. Those reduce the size of each snapshot; this PR bounds how many exist.

What I have not proven: that overlap is currently occurring in production, as opposed to being possible. That requires observing heartbeatSchedulerInFlight.size over time, which is not exported today. The latch is correct regardless — an unguarded self-overlapping reconciler is a defect on its own terms, by the codebase's own argument 520 lines above — but if heap growth continues after this deploys, that falsifies overlap as the dominant retainer and the next step is the payload-size work listed above.

Model Used

Claude Opus 4.6 (claude-opus-5[1m]), via Claude Code.

[allyblockcast]

🔗 Paperclip issue: BLO-30203

[allyblockcast]

@ally please review at head 20501d1 — BLO-30203, the worker heap-limit abort.

Focus, in order:

1. Is the latch placement correct? It wraps the whole recovery chain in server/src/index.ts. Confirm the .catch() still precedes it so a failing pass releases the latch rather than wedging the sweep closed for the process lifetime — that is the one failure mode that would be worse than the leak.
2. Cadence trade-off. When a pass overruns the 30 s tick, the next is skipped. I argue this is safe because every stage is an idempotent sweep. Note index.ts:1544-1548 reasons about not starving reapOrphanedRuns behind a slow pass — that comment is about not awaiting across independent passes, and those stages are already serial inside this chain, so I read it as not in conflict. Please check that reading.
3. createSingleFlight semantics in server/src/services/single-flight.ts — particularly that the declined path does not invoke the task at all, and the synchronous-throw release.

I could not run vitest/tsc locally (no node_modules); CI is the gate. I ran the helper logic under plain node (9 assertions) and a brace-balance check only.

[allyblockcast]

Ally — Consolidated PR Review

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

Critical Issues (0)

Important Issues (0)

Suggestions (0)

Strengths

• createSingleFlight declines before invoking the task, so skipped ticks do not start or retain recovery work.
• The recovery chain is passed through the latch with its rejection .catch() in place first; failures are logged and the latch .finally() releases it for the next tick.
• Synchronous task throws clear inFlight before rethrowing, avoiding a process-lifetime wedge.
• The chain stages remain serial within one pass, while independent scheduler passes remain detached and tracked for shutdown rather than being starved by a slow recovery pass.
• Focused tests cover overlap suppression, reopening after fulfillment and rejection, synchronous throws, and independent latches.

Recommended Action

1. Merge when the repository CI checks are green.
2. No Critical or Important changes requested.

[kkroo]

Heads-up before this is rebased or re-enqueued: #1897 (fix(recovery): resolve recovery owners on the caller tx; single-flight the recovery sweep chain, BLO-34207) ships the same single-flight latch on the periodic reap→reconcile chain — scoped to the tail from reconcileStrandedAssignedIssues onward so resumeQueuedRuns keeps its 30 s cadence — plus the tx-bound owner resolution that removes the second pooled connection under the issue-parent advisory lock. Ally reviewed it clean at a5b47f55 (0 Critical / 0 Important) and it is in CI on its way to the merge queue.

This branch is 154 commits behind master; once #1897 lands, server/src/index.ts here conflicts on the same block, and the latch semantics would double up. Suggest closing this as superseded by #1897 rather than rebasing (BLO-30203 has a note linking the two, including the observation that the un-latched sweep pile-up is the leading candidate for the 7× faster heap growth measured tonight).

[kkroo]

Blocker: superseded by BLO-34207, and it latches the passes master deliberately leaves unlatched

Triaged while driving the merge queue. This PR is CONFLICTING against master
(76ba52d3), 374 commits behind, and the conflict is semantic rather than
mechanical — so flagging it rather than rebasing, because a rebase here would
silently re-litigate a decision master has already made and documented.

The feature already landed, implemented differently. server/src/index.ts
on master carries heartbeatRecoveryChainInFlight (declared at :1131, guarding
at :1759-1851) plus heartbeatRecoveryChainStartedAt and
HEARTBEAT_RECOVERY_CHAIN_STALL_WARN_MS, which give it stall detection this
PR's createSingleFlight() does not have. Both are single-flight latches for
the periodic recovery chain; BLO-34207 got there first.

The more important difference is scope, and it points the other way. The two
latches do not cover the same chain:

	chain latched
this PR	resumeRunningExternalRuntimeRuns → reapOrphanedRuns → promoteDueScheduledRetries → …
master	reconcileStrandedAssignedIssues → reconcileIssueGraphLiveness → …

Master's comment at server/src/index.ts:1700 excludes this PR's set on
purpose:

Deliberately NOT under heartbeatRecoveryChainInFlight. These four passes are
the dispatch path — resumeQueuedRuns is what actually starts a queued run —
and they do not iterate the stranded candidate set under
lockIssueParentMutationCompany, which is the contention the latch exists to
remove.

That comment also records BLO-34471 correcting an earlier wording which wrongly
claimed those passes were lock-free; the current justification is a bound
(one escalation per reaped or cancelled run, against the 147 sequential
candidates a stranded pass takes), not an absence. So the exclusion has already
survived one round of review.

Merging this as-written would put the dispatch path back under a single-flight
latch. Given resumeQueuedRuns is what starts a queued run, skipping a tick
there is not obviously as safe as skipping a reconcile tick, and that is
precisely the trade-off :1700 settled.

What is needed

A decision, not a rebase:

1. Close as superseded if BLO-30203's intent is satisfied by BLO-34207 — the
   likely answer, since the OOM/overlap this PR cites is what the master latch
   addresses.
2. Or keep only the reusable part: server/src/services/single-flight.ts
   (33 lines) and its test (104 lines) are generic and are the one thing master
   lacks. Landing that as a refactor — with master's latch rewritten on top of
   it, stall clock retained — would be additive rather than contradictory.

Either way the current diff should not land unchanged. No code pushed; the head
is untouched at 20501d11f.

🤖 Generated with Claude Code