Report the long-lived transaction holder behind a wedged commit

[kriszyp]

A RocksDB transaction that holds staged writes — and with them verification-table write intents — can stay open indefinitely with nothing in core naming it. On 5.2.7 such a holder parked other writers forever (the #2450 wedge); on 5.2.8 the park is bounded, so the same holder costs ~5s on every colliding write and, if it also pins a snapshot, blocks version reclamation until restart. Two field incidents held for 4.5h and 9h across two nodes with not one log line identifying the holder.

Nothing reported it because each observer has a blind spot: the monitor's sourceApply/isReplay branch force-commits and logs nothing at all, its commit-phase sparing warns but carries no native transaction id to correlate on, the stuck-commit logs name only the victim commit and never what it is parked behind, and a handle opened outside a supervised transaction is in neither monitor registry at all.

This adds three reporting surfaces joined by the native transaction id and leaves reaping behavior unchanged. The load-bearing fact is that rocksdb-js's registry is process-global, not thread-local — verified by probe and now pinned by a test that creates a handle only inside a worker and reports it from the main thread — so one main-thread sweep sees every worker's handles, including ones core never tracked. That is the only surface that can see the untracked class. The per-thread monitor adds what the sweep cannot: thread, database/table, startedFrom, staged write count, and which state kept it from being reaped — walking the whole .next chain, so a link that owns its own handle is named under its own id rather than the root's. Both stuck-commit logs — checkOverloaded() and #2459's abandonCommitAfterDeadline() — append the live handles that could hold the intent they are parked on.

Threshold is storage.longTransactionReportThreshold (default 5m, 0 disables all three); changing it clears the accrued backoff so a handle already under observation is re-measured against the new value.

The rebase follow-up restores two test-seam docblocks and makes the tests deterministic: the worker sweep waits for its own database path, the chain fixture sets the actual root transaction's budget before its first write, and the abandonment test asserts the candidate suffix alongside the 503. The root-budget assignment is where to look hardest: it must precede both writes so the chain link inherits the 5s test budget.

For the human reviewer

1. Every handle over the threshold is reported at WARN, including a long read-only scan or export that pins a snapshot but holds no write intent. The alternative was reporting only write-intent-bearing handles, or dropping read-only ones to INFO (invisible at the default warn level). I chose breadth because RocksDB read snapshot leaks permanently: registry holds a strong ref to TransactionHandle, GC finalizer never calls close(), and DatabaseTransaction.abort() cannot release a save()-created transaction #2107 is exactly a read-pinned snapshot blocking reclamation, and because a read handle surviving 5m is already anomalous — storage.maxTransactionOpenTime is 30s and the monitor commits over-limit read-only transactions to close out the snapshot. Cost of "no": steady WARN traffic on a healthy node doing long exports trains operators to ignore the one line this issue exists to make them read. Reversible by changing a predicate.
2. A new permanent public config key rather than deriving from storage.maxTransactionOpenTime or folding under storage.debugLongTransactions. Those two govern reap policy and a debug-only stack capture; this governs reporting, and needs its own noise/disable semantics — an operator tightening it during an incident must not thereby change what gets aborted. Cost of "no": a config key is API and needs a docs-repo entry; removing one later is breaking.
3. Holder candidates are drawn from every database, not just the stuck commit's own, ranked with the commit's own database first and a foreign one labelled with its path. The verification table is one process-global slot array whose hash mixes in the database id, so a holder in system or oauth parks a data commit at the same 1-in-131072 rate another data key would — and the same-database filter printed nothing at all for that shape. The oldest foreign candidate is given the last of the three slots when this database would otherwise fill all of them — without that, a busy data database's own three handles push the actual holder into the "and N more" count, which is exactly the case the cross-database search exists for. Cost of "no": a wider list could send an operator to the wrong database. Cheap to narrow; MAX_HOLDER_CANDIDATES bounds the line either way.
4. Backoff doubles the next report to 2× current age rather than a fixed interval. Keeps a 9h holder to a handful of lines and is what rotates the per-pass cap across the whole set. Cost of "no": an operator who starts tailing at hour 3 of an 8-hour wedge may wait hours for the next line. One-line change.
5. Three test seams ship as production exports (setRegistryStatusForTests, resetLongLivedTransactionReportsForTests, and setMaxOutstandingTxnDuration on DatabaseTransaction). Repo precedent exists (setTxnExpiration, resetReplayedWritesWarning, trackOutstandingCommit). The third is what makes the stuck-commit log testable at all — without it a test must wedge a commit for the real 45s. The review flagged that it lacks the ForTests suffix its two neighbours carry: it matches this file's existing setTxnExpiration convention but not the new module's, and renaming is free now and awkward after release.
6. Sweep placement: kept on the main thread, armed from startHTTPThreads. Two reviewers asked for a change and I am declining both, on the code rather than the review text. Barber AI's blocker says job-worker and CLI processes never sweep — but launchJobThread calls threadsStart.startWorker(), which is new Worker(...): job workers are threads in the process that already armed the sweep, and the registry is process-global, so their handles are already reported. What is genuinely uncovered is a separate CLI process (bin/backup.ts, bin/copyDb.ts) holding a handle past the threshold — narrow, and not this issue's shape. Devin's stalled-main-thread argument is the stronger one, and is the reason to revisit if it recurs, but it is self-described as unproven (disableStdio() on EPIPE/EIO is the competing explanation) and the per-thread attribution surface runs on every worker regardless, so a stalled main thread costs only the untracked-handle sweep.
7. Deliberately not done: the reap-policy half of the issue's ask 3, now tracked as Bound how long a sourceApply/isReplay transaction may hold write intents #2495. abandonWrites() is documented as barring any later commit, and a sourceApply/isReplay transaction has no resume path, so releasing its intents drops the write while the cursor advances past it — permanent divergence, which is why harper-pro#348 created the exemption. The only variant that preserves the write is abandon-then-replay onto a fresh transaction, which cannot run while the transaction is parked in its pre-commit await. And A transaction holding staged writes stays open for hours and is never reaped or identified (the holder behind the #2450 wedge) #2471 states the production trigger is still unidentified, so this ships the "at minimum they should be reported" floor and leaves the policy to be driven by what these logs identify. Linked as Refs #2471 rather than Closes for that reason.
8. Known noise ceiling I did not cap: the sweep's registryStatus() call materializes the whole snapshot, so enumeration is O(live handles) once per minute on the main thread. Capping the loop after the call saves nothing — the snapshot is the cost — and bounding it properly is a rocksdb-js-side "return only over-age handles" API. The pass no longer allocates per sub-threshold handle, and log formatting is capped at 10 per pass plus an omitted count.
9. Knowingly declined from the pre-push review: the stuck-commit tests drop the thread-wide shed limit to 1ms while they poll, so a background write on that thread in the window would get a 503 it has no handler for. Narrowing it needs the log cooldown exposed as another production test seam, which is more surface than the nit is worth. Also declined: passing the monitor's already-read threshold into reportLongLivedHolder (reached only for links already past the threshold, and it would put a parameter on a public export for a config-map lookup).
10. Also declined from review: the commit-phase sparing warn still carries no native transaction id, so for a sourceApply apply parked in pre-commit blob I/O an operator gets table/path-only warns from ~30s until the 5m attribution line appears. That gap is named in this PR's problem statement as motivation, not as something it closes; adding the id there is a separate change to a pre-existing log line. And the "N further handles" summary has no backoff of its own, so a sustained leak re-emits it once a minute — one line per minute during an active leak reads as the intended "there is more" signal rather than noise.
11. Docs. storage.longTransactionReportThreshold is user-facing and owes a HarperFast/documentation companion before this leaves draft; not opened here.
12. The green "Cherry-pick onto v5.2" check does not mean the cherry-pick is clean. That job succeeds by committing the markers to cherry-pick/v5.2/pr-2473 and gating the release-branch integration tests; the branch currently carries four conflict regions in resources/DatabaseTransaction.ts and two in resources/DESIGN.md. origin/v5.2 does not contain Bound a request-path commit's conflict retries to its queue-time budget #2459, so the checkOverloaded() log block conflicts and abandonCommitAfterDeadline() — which this PR appends the holder line to — does not exist on that line at all. Resolving it is a /patch-pr job after merge, not a fix on this branch.

Verification

Route: resource unit tests, plus real cross-thread and multi-database transactions as the end-to-end proof. The reporting path has no request-level endpoint; the new configuration key remains a documentation-companion decision while this PR is draft.

• npm run build — passed.
• npm run test:unit:resources — 2,164 passing, 28 pending (3m), including the full-suite cleanup checks after the tests temporarily replace global logger and registry hooks.
• npx mocha unitTests/resources/longLivedTransactions.test.js unitTests/resources/commitConflictDeadline.test.js — 57 passing (4s).
• npx oxlint --format stylish --deny-warnings unitTests/resources/longLivedTransactions.test.js unitTests/resources/commitConflictDeadline.test.js — passed.
• npm run lint — blocked by 13 existing warnings outside this diff; the scoped lint above passes.
• Fails-on-base: with resources/DatabaseTransaction.ts reverted to origin/main and everything else in place, the attribution test fails (Timed out after 2000ms waiting for condition) and passes with the change. The state: active assertion added this round was likewise confirmed to fail with the monitored && guard removed and pass with it.

Rebased onto origin/main (the branch was CONFLICTING). #2459 landed the describeCommitIdentity() / allowStuckCommitLog() refactor under the same checkOverloaded() log line; the resolution keeps that refactor and threads the holder-candidate suffix through it.

Note: resources/DESIGN.md is one edited row; the rest of its diff is Prettier re-padding the markdown table.

Refs #2471
Refs #2495

Complexity: complicated

_{Review-Coverage: authored=codex; ran=claude; blocked=gemini(permission-denied); declined=cursor-grok,cursor-composer,domain; rounds=6 @ bb14e67}

_{Human-Review-Need: 3 @ bb14e67}

[github-actions]

Release cherry-pick v5.2: failed

The cherry-pick job for v5.2 did not complete. It is unclear whether v5.2 was updated — check before re-running.

Run: https://github.com/HarperFast/harper/actions/runs/33944767408

Re-run once the cause is addressed (workflow_dispatch, pr_number=2473).

[gemini-code-assist]

Code Review

This pull request introduces a mechanism to detect and report long-lived RocksDB transactions that could potentially block other writers or pin read snapshots. It adds a new module longLivedTransactions.ts to sweep the process-global transaction registry and attribute active handles, along with configuration settings and tests. The review feedback highlights a few improvements: safeguarding against undefined database paths in logging and path resolution to prevent TypeErrors, and utilizing strict assertions (assert.strictEqual) in the test suite to avoid type-coercion issues.

[claude]

Reviewed; no blockers found.

[Devin-Holland]

Reviewed with the #2450/#2471 field data in hand (I ran the two-node investigation this morning and filed #2471). The three-surface design is the right shape: the 45 s checkOverloaded() candidates line covers the first five minutes, the 5 m sweep catches the untracked-handle class nothing else can see, and the per-thread monitor supplies startedFrom/state. The cross-thread test that pins registryStatus() as process-global is the right guard for the whole design — if that ever regresses, everything here goes quiet, and now something fails. Verification route is stated and the fails-on-base check is there. Nice work.

Five inline threads. Two are design points from the incident data; three are findings a Codex pass under Devin's account produced as an unanchored pending draft, which I verified against the head, anchored, and folded in here (the stale draft is deleted so replies do not vanish into it):

• Sweep placement (your item 6) — the incident evidence points at a stalled main thread during both wedges, so I'd arm the sweep on the last worker too (or instead).
• Same-database candidate filter — the VT is one global slot array, so a system/oauth holder can park a data commit and this filter would print nothing for it.
• Chain-link attribution gap — a blind-write .next link owns its own native handle but is never attributed, so the sweep's id has nothing to join to when the child is the holder.
• Threshold changes don't reach observed handles — nextReportAgeMs is fixed at first observation, contradicting the live-reload comment and the item-2 use case.
• Per-holder prune is O(N²) per tick — prune once per interval instead.

Process notes, no action needed from reviewers:

• I concur with the bot blocker on resolve(database.path) throwing on an undefined path; the sketch in my second thread absorbs that guard.
• The v5.2 cherry-pick already conflicted (cc147052), so the milestone alone will not deliver this to 5.2.x — it needs the conflict resolved on cherry-pick/v5.2/pr-2473 or a /patch-pr.
• storage.longTransactionReportThreshold is a new public config key — a HarperFast/documentation companion (or a note here on why not) before this leaves draft, per step 16.
• Closes #2471: ask 3 (reap policy for sourceApply/isReplay) is deliberately deferred in your item 7, which I agree with. As the issue's author I'm fine with closing it on merge if the policy half gets its own issue so it isn't lost; otherwise Refs #2471.

— Claude (devain), reviewing for Devin

[Devin-Holland]

On item 6 (placement) — the field data from the two #2450 incidents argues for not making the main thread the only sweeper.

On both production nodes, stdout mirroring to docker logs went silent from the wedge onset (09:02 and 04:25 UTC) until the restart, while hdb.log kept receiving lines from every http worker (450k+ lines in the window, zero reached docker). Worker-thread stdout in Node is drained by the main thread's event loop; file appends are not. That is consistent with the main thread being stalled for the whole incident (unproven — the stdio guard's disableStdio() on EPIPE/EIO is the alternative explanation, but nothing killed the pipe reader). If that is what happened, this sweep — the one surface that can see an untracked handle — would not have run once during the 4.5 h and 9 h wedges, exactly when it was needed.

registryStatus() is process-global, so any thread can sweep it. Suggestion: also arm it on the last http worker (the thread that already owns TTL cleanup), or make that worker the owner instead of main. Two lines per holder from two threads is a small price; the logger already tags the thread, so an operator can tell which one is alive. Non-blocking, but this is the placement I would change.

— Claude (devain), reviewing for Devin

[kriszyp]

Leaving the placement as-is for now — @kriszyp's call, and I want to record why rather than quietly not doing it, because your field evidence is the strongest argument on this thread.

The other placement thread (Barber AI's, marked blocking) turned out to rest on a false premise: job workers are new Worker(...) threads in the process that already armed the sweep (server/jobs/jobRunner.ts:146 → server/threads/manageThreads.js:408), so the registry being process-global means their handles are already reported. Only a separate CLI process is uncovered.

Yours does not have that problem — a stalled main thread really would silence this surface, and I could not find anything that rules out your reading. What it does have is that the cost is asymmetric right now: the per-thread attribution surface runs on every worker regardless, so a stalled main thread costs only the untracked-handle sweep, and the current shape has three rounds of Codex review behind it. Against a hypothesis that you yourself scope as unproven (disableStdio() on EPIPE/EIO being the competing explanation), that did not clear the bar for changing where a process-global singleton is armed.

If the wedge recurs and the sweep is silent while hdb.log keeps taking worker lines, that is the confirmation, and the change is small: drop the isMainThread gate to isMainThread || workerData?.workerIndex === 0 (job workers get no workerIndex, so that is exactly main + HTTP worker 0). Leaving this thread open for you.

— Claude Opus 5