fix(checkpoint): don't count a timed-out writer wait as a writer failure - #1938
Merged
Merged
Conversation
waitForWriter bounded the writer Deferred at 300s and mapped the expiry to
{status:"failure"}. That expiry does not cancel the writer, and the settle
watcher that owns the watermark advance awaits the same Deferred with no
bound — so a slow-but-successful writer still advances
last_checkpoint_message_id after the wait gave up.
The prune retry watcher therefore booked a working writer as broken:
writerFailures ticked and the session's crossed thresholds were cleared
("checkpoint writer failed — cleared thresholds for retry"). After
MAX_WRITER_FAILURES such waits it logs "gave up after max consecutive
failures" and stops checkpointing the session entirely — even though every
writer had actually succeeded. Losing checkpoint coverage also degrades
/rebuild, whose released span is what the checkpoint covers.
Observed on a live TUI run: writer spawned at 10:47:07, "failed" logged at
10:52:07 (exactly +300s), and the same writer then succeeded and advanced the
watermark. Reproduced twice in one session.
Report "timeout" instead. prune's existing `result !== "failure"` guard then
skips the counter and leaves thresholds alone, and /rebuild's `!== "success"`
check is unchanged. Retrying while the writer is still in flight was already
a no-op: isWriterRunning short-circuits new triggers.
wqymi
added a commit
that referenced
this pull request
Jul 29, 2026
…hold retry
The checkpoint writer is fail-acceptable and the code already implements
that redundancy:
- ensureCheckpointTemplate (checkpoint.ts:117-121) writes the template
only when the file is ABSENT, so a failed writer cannot clobber the
previous checkpoint.
- last_checkpoint_message_id advances only on a successful insert
(checkpoint.ts:918-934; the sole external resetThresholds caller is
prompt.ts:428, gated on `if (inserted)`).
So a failed write leaves file and watermark mutually consistent, both
pointing at the last good checkpoint, and a rebuild uses that
automatically. The cost of a failure is a STALER checkpoint, never a
missing one, and the next threshold crossing is a natural retry with
fresher context than an in-place one.
The accounting layered on top did not trust that, and its failure model
was wrong: a writer failing repeatedly means the provider is broken, in
which case the foreground turns are failing too and the user already
knows. The writer's LLM calls go through the same retry ladder as the
foreground (retry.ts), so any failure reaching prune is already
post-retry. Counting it again, three times, then falling silent, added
nothing.
Deleted:
- MAX_WRITER_WAIT_EXTENSIONS and the wait-extension loop
- writerFailures, MAX_WRITER_FAILURES, and the in-place-retry path
that cleared crossed/maxCrossed on failure
- cfg.checkpoint.max_writer_failures (schema, docs, generated SDK)
Kept: the bounded wait itself, forked purely as observation, so
waitForWriter's bound-expiry log stays reachable. That log is the only
evidence a slow writer leaves, and a line exactly like it is what
diagnosed #1938. Terminal outcomes are already logged by the settle
watcher, so the outcome is discarded here.
waitForWriter and its distinct "timeout" outcome are #1938's merged work
and are untouched.
Residual, accepted: with no in-place retry, a session whose LAST
threshold fails (mid-tier's last is 80%) gets no further fire, so its
checkpoint stays stale and an overflow rebuild uses the stale one. That
is the design's own premise, not a regression.
wqymi
added a commit
that referenced
this pull request
Jul 29, 2026
…very "The next threshold crossing is the retry" presumes a next threshold. The last one has none, and its only would-be successor is unreliable: the discard+rebuild that maxThresholdCrossed triggers re-arms the ladder via resetThresholds (prompt.ts:428), but rebuildFromCheckpoint bails when lastBoundary is unset (prompt.ts:413) — and the watermark is unset exactly when no writer has ever succeeded. So in the one case that needs recovery most, the session never checkpoints again until overflow. The principle: a failed checkpoint write earns a retry only when the failure's CLASS says a retry could succeed, and only where the token axis still has ROOM for one. Both are readings of the present state, so neither needs failure history. Mechanism (prune.ts): on a settled retryable failure of the FINAL threshold, arm a per-session gate one ladder STEP above the token count that fired, clamped to maxAllowed. Deterministic or unclassified failures arm nothing — they would recur identically. Non-final thresholds arm nothing — the ladder already is their retry. The gate is consumed when it fires and dropped by resetThresholds. Why this is not MAX_WRITER_FAILURES renamed: a counter accumulates attempts and permits N retries whether or not anything changed. This is a position, overwritten not accumulated, derived from the ladder and the window. It bounds the retry RATE to the ladder's own firing rate, bounds the retry COUNT to whatever the unused window affords (zero once firedAt + step is past maxAllowed), and a conversation that stops growing stops retrying by itself. Plumbing: waitForWriterSettlement carries the FailureInfo the outcome already had. #1938's flat waitForWriter contract is untouched — it is now projected off the same implementation, so the two cannot drift. A "timeout" never carries a classification, so a merely slow writer can never arm the gate. Also corrects a test comment that still justified itself by "keeping the failure cap reachable" so a broken slow writer "would never be counted" — the cap and the counting were deleted earlier in this branch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
waitForWriterbounds the writer'sDeferredat 300s and maps the expiry to{status:"failure"}(packages/opencode/src/session/checkpoint.ts:989-993on main).That expiry does not cancel the writer. The settle watcher that owns the watermark advance awaits the same
Deferredwith no bound (checkpoint.ts:911-921), so a slow-but-successful writer still advanceslast_checkpoint_message_idafter the bounded wait gave up.So the prune retry watcher (
prune.ts:312-337) books a working writer as broken:writerFailurestickscrossed/maxCrossedthresholds are cleared →checkpoint writer failed — cleared thresholds for retryMAX_WRITER_FAILURESsuch waits →checkpoint writer gave up after max consecutive failures, and the session stops being checkpointed even though every writer actually succeededLosing checkpoint coverage also degrades
/rebuild: the span it can release is exactly the span the checkpoint covers.Evidence (live TUI, dev build)
Real run on
mimo-v2.5, checkpoint writer on the parent's model:threshold=60000 currentTokens=112596→ writer spawned10:47:07checkpoint writer failed — cleared thresholds for retry(attempt=1)10:52:07(exactly +300s)msg1 → msg4Reproduced twice in a single session. The
+300salignment is exact — it is the wait bound expiring, not a writer error.Fix
Return a distinct
"timeout"outcome instead of"failure".prune.ts's existingif (result !== "failure") returnguard then skips the counter and leaves thresholds untouched — no prune logic change needed./rebuildis unaffected — and more strongly than an earlier revision of this description claimed. That revision cited a case-2if (writerOutcome !== "success")bail atprompt.ts:4073-4082. No such code exists:git grep writerOutcomehas zero hits repo-wide, andprompt.ts:4073-4082is actor-notification rendering. The real shared path isrebuildFromCheckpoint(prompt.ts:398-430), used by both the automatic overflow path and manual/rebuild, and it never consults the writer outcome at all — it gates onhasCheckpoint+lastBoundaryand deliberately does not block on an in-flight writer (see its own comment atprompt.ts:394-397). Changing the outcome value therefore cannot affect it.isWriterRunningshort-circuits new threshold triggers, so nothing is lost by not counting.Test
New
test/session/checkpoint-writer-wait-timeout.test.tsusesTestClockto drive past the 5-minute bound with the writer'sDeferredstill unresolved.Verified it is a real regression test — with the source fix reverted it fails with
Expected: "timeout"/Received: "failure".bun run typecheck→ exit 0test/session/→ 821 pass / 1 flaky fail that also passes on re-run and is unrelated to this changeNotes
An accidental benefit was removed along with the bug
Stating the trade honestly: on
main, the mistakentimeout → failuremapping clearedcrossedandmaxCrossed(prune.ts:325-326). That had a beneficial side effect nobody designed — after a slow writer finally settled, the threshold re-fired. This PR removes that re-fire together with the false failure.Mitigation: at the max threshold
maxCrossednow stays set, soprompt.ts:3308-3314still reachesrebuildFromCheckpoint, which callsprune.resetThresholdswhen it succeeds (prompt.ts:428) and re-arms the thresholds. Checkpointing is therefore not permanently stalled.Follow-up
Two gaps this PR left behind are fixed in #1945: hitting the bound became completely silent (no log from either layer), and a writer that genuinely fails — or succeeds — past 300s had its real outcome booked nowhere, since prune's watcher returned on
"timeout"and never re-awaited.Independent of #1752 — the defect is present on
mainand this PR is deliberately not bundled into it.Two related issues found during the same investigation are not addressed here (they need their own discussion):
computeBoundaryanchors on the last finished assistant and returnsmsgs[0]when there is none. A token blowup inside the first assistant turn pinscoveredUpToto message 1, so a full 5-minute writer run covers exactly one message.modelfield on thecheckpoint-writeragent) and needs ~13 sequential LLM round-trips (~20-25s each) to read prior checkpoint/memory and write its 3-4 files, which is what puts it near the 300s line in the first place.