Skip to content

fix(storage): bound commit settlement — recover lost native completions and retry-backoff timers (#1785) - #1830

Closed
kriszyp wants to merge 3 commits into
mainfrom
kris/commit-settle-watchdog-1785
Closed

fix(storage): bound commit settlement — recover lost native completions and retry-backoff timers (#1785)#1830
kriszyp wants to merge 3 commits into
mainfrom
kris/commit-settle-watchdog-1785

Conversation

@kriszyp

@kriszyp kriszyp commented Jul 16, 2026

Copy link
Copy Markdown
Member

Fixes the silent-hang manifestation of #1785 (ingest permanently hangs under sustained sequential bulk upsert).

Summary

Root-cause investigation (instrumented native binding + gdb on live frozen processes, reproduced under cgroup CPU throttling per @ldt1996's repro) found the reported hang is a severed retry-backoff continuation: under CPU throttle, the ERR_BUSY/ERR_TRY_AGAIN retry chain's one-shot setTimeout(...).then(recommit) can be scheduled and never run — observed definitively (an 81ms backoff dead for 21+ minutes on a live process whose other timers kept firing and whose other retry chains completed their full 40-retry ladders and threw correctly). The orphaned commit promise then hangs forever with no error. When every link fires, the existing retry path already errors correctly at MAX_RETRIES.

  • robustBackoff replaces the bare backoff delay(...).then(recommit): each backoff registers in a small Set; the existing long-transaction monitor interval (startMonitoringTxns) fires any entry overdue past a 2s grace window, with a Set.delete once-guard so a late real timer no-ops. No second watchdog timer — a transaction has already left trackedTxns when its commit chain runs (so the existing sweep can't see a stuck commit), but its interval can host this recovery. Worst-case recovery latency ≈ one monitor tick (30s default) vs. infinite hang today.
  • Each recovery logs an error — production telemetry for how often the underlying primitive loss actually occurs.
  • Adjacent fix: the coordinated RETRY_NOW retry cap now aborts the native transaction before throwing (handle leak; mirrors the ERR_BUSY cap path).

Evidence correction (why the earlier settle-watch was removed)

An earlier revision also wrapped every native commit/abort resolution with a settlement deadline ("lost native completion" recovery). Re-examination showed that observed instance was a capture artifact — the completion arrived late (>90s under 50× throttle) and did resolve — and a deadline-reject would misreport legitimately-slow commits whose outcome is merely unknown. Removed; DESIGN.md documents the retraction and the remaining primitive suspects (AsyncLocalStorage promise-hook machinery around the timer boundary; server/throttle.ts as a separate candidate for request-level hangs). The severed primitive itself is still unidentified — this PR bounds the damage and instruments the incidence while that hunt continues (targeted ALS isolation repro + rocksdb-js#694 A/B soak in progress).

Where to look

  • The once-guard/adoption semantics in robustBackoff (resolve(resume()) adopts the recursive commit() promise; sync throws reject).
  • The sweep runs inside the startMonitoringTxns interval — cheap (Array.from of a normally-empty Set), but it now executes every monitor tick.
  • Recovery latency is bounded by the monitor interval (STORAGE_MAXTRANSACTIONOPENTIME, 30s default) — judged fine vs. a permanent hang; flag if you want a faster dedicated cadence instead.

No docs PR: no user-facing API/config change (new exports are internal test hooks following the setTxnExpiration precedent).

Generated by an LLM (Claude Fable 5) working with Kris; full investigation trail on #1785.

🤖 Generated with Claude Code

…ns and retry-backoff timers (#1785)

Under CPU throttling (cgroup CFS quotas in CPU-limited containers), two async-delivery
links in the commit path were observed being silently severed: the native commit's
completion callback (commit durable, JS promise never settles) and the ERR_BUSY/
ERR_TRY_AGAIN retry-backoff timer. Either loss orphans the caller's commit promise
forever — no error, ingest wedged indefinitely (14+ hours reported).

Enforce settlement externally rather than chasing the unidentified primitive:
- watchCommitSettlement wraps every native commit/abort resolution; a lazy unref'd
  sweeper settles entries past 2× STORAGE_MAXTRANSACTIONQUEUETIME: lost aborts resolve,
  a feature-detected rocksdb-js terminal-outcome getter replays committed/RETRY_NOW
  exactly (Phase 2, rides 2.5.x), otherwise reject with an explicit outcome-unknown 503.
  sourceApply entries are never rejected (never-drop invariant) — rate-limited error
  logs, keep waiting.
- robustBackoff replaces the bare backoff delay: the sweeper fires overdue timers with
  a once-guard; each recovery logs an error (production telemetry for the loss rate).
- Recovered entries null their native-txn/write-batch refs so the lost promise's
  reaction closures cannot retain them forever.
- Adjacent fix: the coordinated RETRY_NOW retry cap now aborts the native transaction
  before throwing, mirroring the ERR_BUSY cap path (handle leak).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@kriszyp
kriszyp requested review from cb1kenobi and ldt1996 July 16, 2026 05:54
gemini-code-assist[bot]

This comment was marked as resolved.

@claude

This comment has been minimized.

…ode:assert/strict in tests

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@kriszyp
kriszyp marked this pull request as ready for review July 16, 2026 06:19
@kriszyp
kriszyp marked this pull request as draft July 16, 2026 12:35
@kriszyp

kriszyp commented Jul 16, 2026

Copy link
Copy Markdown
Member Author

Back to draft: re-examination of the capture evidence shows the 'lost native commit completion' failure mode was a capture artifact (that commit's completion arrived late — >90s under 50× CPU throttle — and did resolve; the snapshot just cut it off). The confirmed failure mode is the severed retry-backoff continuation only. Reworking the PR to match the corrected evidence before review. — Claude (Fable), for Kris

…ff only

Evidence re-examination showed the 'lost native commit completion' was a capture
artifact (the completion arrived late — >90s under 50x CPU throttle — and did
resolve), so the settle-watch wrapper is removed: its deadline reject would
misreport legitimately-slow commits whose outcome is merely unknown. The
confirmed severed link is the retry-backoff continuation (81ms setTimeout dead
21+ min on a live process whose other timers and retry ladders stayed healthy).

robustBackoff remains, its recovery sweep now piggybacking on the existing
long-transaction monitor interval (startMonitoringTxns) instead of a second
timer — transactions leave trackedTxns before the commit chain runs, so the
existing sweep cannot see a stuck commit, but its interval can host the backoff
recovery. DESIGN.md documents the corrected evidence, the retraction, and the
remaining primitive suspects (AsyncLocalStorage promise-hook path; throttle.ts
for request-level hangs).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@kriszyp
kriszyp marked this pull request as ready for review July 16, 2026 12:47
@kriszyp
kriszyp marked this pull request as draft July 16, 2026 12:58
@kriszyp

kriszyp commented Jul 21, 2026

Copy link
Copy Markdown
Member Author

Closing after re-auditing the root-cause evidence. The remaining robustBackoff premise is not supported by the retained artifacts, and the post-5.1.22 capture was produced by a watchdog that ran 781,206 ms late and did not record eventual settlement. A controlled Harper 5.1.22 commit/suspend/resume test produced the same apparent-stall signature, then returned HTTP 200 with the row committed; a separate 100M-row sequential soak completed cleanly. The original ERR_TRY_AGAIN retry defect remains addressed by #1696. No additional Harper watchdog is justified without a corrected, continuously observed reproduction.

— KrAIs (OpenAI Codex)

@kriszyp kriszyp closed this Jul 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant