fix(daemon): persist the pending-output counter across empty incremental takes (STA-4297) - #14496
Conversation
…tal takes (STA-4297) An empty incremental take advanced pendingOutputSeq without writing a log batch, so the in-memory counter ran permanently ahead of the log. The next warm reattach could not prove continuity and committed the live 1000-row window over a deep durable checkpoint. Advance the counter only for takes that get persisted: a snapshot take (stamped into the checkpoint) or one carrying records/overflow. This matches the layers below, which already treat an empty take as a no-op write.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthrough
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/daemon/session.ts (1)
497-501: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce the new multi-line comments.
Keep one concise non-obvious reason where needed. Remove rationale that the code, test name, or assertions already show.
src/main/daemon/session.ts#L497-L501: Reduce this explanation to one brief reason for preservingpendingOutputSeq.src/main/daemon/types.ts#L288-L292: Keep the publicseqcontract without the recovery-path explanation.src/main/daemon/session-pending-output.test.ts#L99-L101: Remove this comment because the test name states the behavior.src/main/daemon/daemon-restore-scrollback-depth.test.ts#L489-L491: Remove or reduce this comment because the test name and assertions state the outcome.As per coding guidelines, “Comments must be concise, non-obvious, and brief—prefer one line; do not explain obvious behavior or walk through code.”
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8d934a6a-99c2-4a9e-b183-cc932e79056e
📒 Files selected for processing (4)
src/main/daemon/daemon-restore-scrollback-depth.test.tssrc/main/daemon/session-pending-output.test.tssrc/main/daemon/session.tssrc/main/daemon/types.ts
…tal takes (STA-4297) (stablyai#14496) * fix(daemon): persist the pending-output counter across empty incremental takes (STA-4297) An empty incremental take advanced pendingOutputSeq without writing a log batch, so the in-memory counter ran permanently ahead of the log. The next warm reattach could not prove continuity and committed the live 1000-row window over a deep durable checkpoint. Advance the counter only for takes that get persisted: a snapshot take (stamped into the checkpoint) or one carrying records/overflow. This matches the layers below, which already treat an empty take as a no-op write. * test(daemon): keep empty-take coverage outcome-based (cherry picked from commit 6cd987e)
1 participant
ELI5
The daemon keeps a counter that says "this is batch number N of your terminal's history." Every time it drains pending output it bumped that counter — even when there was nothing to drain, in which case nothing gets written to disk. So the counter said "batch 5" while the disk only ever got up to batch 4.
Later, when the app reattaches to that terminal, it checks "does the disk agree with the counter?" before trusting the deep saved history. It didn't agree, so it threw away the deep history and kept only the last ~1000 visible rows. Now only an empty drain leaves the counter unchanged; snapshots, records, and overflow still advance it, so genuine lost writes remain detectable while empty ticks no longer manufacture a false gap.
What Changed
Session.takePendingOutputnow advancespendingOutputSeqonly for takes that carry persistence state:overflowed(a real log hole worth recording).An empty incremental take repeats the previous seq instead of advancing it.
TakePendingOutputResult.seq's doc comment now states the exact contract: snapshot, record, and overflow takes advance; empty incremental takes repeat the prior value.Scope is deliberately one condition in
session.ts. Nothing in checkpoint admission, the per-session queue,runWithDeadline, or final-checkpoint deadline behavior is touched (STA-4228 / STA-4229 territory, #14346 / #14385). #14193 and #13061 are unmodified.Why
An empty incremental take advanced the counter without ever writing a log batch, so the in-memory seq ran permanently ahead of the log:
session.tstakePendingOutputbumpedpendingOutputSequnconditionally, including on zero records.daemon-pty-adapter.tsreturns early ontake.records.length === 0, beforehistoryManager.appendIncrements— so that advanced seq is never persisted.restoreInfo.pendingOutputSeq === take.seq - 1.history-reader.tsstampspendingOutputSeqfrom the last log batch (or, when the log is header-only, from the checkpoint).Net: the first post-reattach compact fails the continuity proof and commits the live snapshot over the deep checkpoint. No corruption, but it destroys exactly the restore depth #14193 exists to provide.
Why this shape. The release owner named two options: (a) skip the continuity proof on empty takes, or (b) persist an empty-batch marker so the log tracks the counter. This implements (a)'s intent — an empty take does not participate in the continuity ledger — at the single place the divergence is created.
HistoryManager.appendIncrementsUntrackedreturnsokwithout writing whenrecords.length === 0, and the adapter returnsdoneon an empty take. Onlysession.tsdisagreed. Making the log record no-op batches would contradict that rule and, because idle sessions are the common case, would spend the 5MB log cap and force checkpoint rotations on sessions producing zero output.This also keeps the adapter's
take.seq - 1proof arithmetic unchanged, and incidentally repairspendingRecordsAreComplete: take.seq === 1— idle ticks before a session's first real output used to push the seq past 1 and defeat that check.Remote wire compatibility. No wire shape change;
seqis still a number on the same RPC. A mixed-version pairing is safe in both directions because an old adapter already early-returns on an empty take and never uses the seq it was handed, so a repeated value is never observed. Only what the host publishes for empty takes changes, and only in the direction that makes disk and memory agree.Linked Issue
STA-4297 — https://linear.app/stably/issue/STA-4297
This is item 2 of the dropped durable-history unit. It is intended for the 1.4.184 pick set, not today's 1.4.183 cut, so a later daily can cherry-pick the durable-history unit as a complete stack.
Fixes #
Visual Proof
N/A— no UI or interaction surface. The change is a daemon-side counter condition; the observable effect (restore depth after reattach) is asserted by the automated test below rather than being a visual diff.Testing
src/main/daemon/daemon-restore-scrollback-depth.test.ts— "preserves durable depth when an empty incremental take precedes a warm reattach". It asserts the outcome (recovered depth), not the counter: after the empty take plus a warm reattach it requires the restored history to still containLINE_00001andLINE_01000andscrollbackLinesto exceed the live window, so a fix that merely made the bookkeeping agree while still flattening the checkpoint would not pass.Repro path in the test follows production, not internals: 5000 lines of output → deep durable checkpoint →
adapter.write()marks the session dirty while the mock PTY echoes nothing back (a dirty mark with zero new records) →checkpointDirtySessions()runs the empty take → adapter crash + fresh adapter → warm reattach.RED verified on unmodified
origin/main(a6a64439a0), exact output:LINE_03979onward is the ~1000-row live window — the deep checkpoint was flattened, which is the defect.Control run (to prove the RED is caused by the empty take and not incidental setup): removing only the two trigger lines, with every other line of the test identical, passes on unmodified main. Adding them back fails. So the empty incremental take is the sole cause.
The depth test is the sole regression oracle; no test asserts the counter directly. No matching entry exists in
config/reliability-gates.jsonc, so that manifest coverage is an accepted gap for this focused patch; the existing[history] durable continuity unproven; using live snapshot:warning remains the diagnostic for genuine gaps. Live restore-depth reproduction on SSH, WSL, Linux, and Windows remains unverified; the changed arithmetic is provider- and platform-neutral, while CI covers cross-version compatibility and native smoke.Checks run locally on macOS:
pnpm vitest run --config config/vitest.config.ts src/main/daemon→ 1471 passed, 3 skipped (107 files). Includes the two sibling tests that must still lose depth (uses the live window when a crashed adapter left a pending-output gap,uses the live window when durable history disappears after a prior drain) — the fix does not weaken them.pnpm vitest run --config config/vitest.config.ts src/main→ 21423 passed, 90 skipped on the PR branch; 21422 passed, 90 skipped with all four scoped files restored exactly toorigin/main(a6a64439a0).src/main/ipc/pty.test.tsseparately passed 480/480 on both states. The previously reported 10 overlay-environment failures did not reproduce, so they are not claimed as pre-existing evidence.pnpm run typecheck:node→ clean.oxlint(default +oxlint-code-quality-native-plugins+--type-aware oxlint-code-quality-type-aware,--deny-warnings) → clean. Nomax-linesdisable added and nomobile/.oxlintrc.jsonbump;check:max-lines-ratchetreports no new bypasses.oxfmt --check→ clean (this repo formats with oxfmt, not prettier).Platforms: the changed code is platform-neutral (an integer counter in the daemon session, no paths, no shell, no keybindings) and runs identically for local, SSH, and WSL hosts and for both git-worktree and folder workspaces. Verified on macOS; no platform-conditional behavior added.
Review
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)Author