Repository navigation
fix(claude): reconcile background commands after cold history reads - #2608
Conversation
 |
| Layer / File(s) | Summary |
|---|---|
History checkpoint scanning server/claude-command-history.ts, server/claude-command-history-worker.ts, server/claude-command-history.test.ts |
The worker reads bounded history chunks, validates file versions and checkpoint witnesses, and reports command endings with coverage status. Tests cover parsing, cache invalidation, and partial records. |
Bounded history worker lifecycle server/claude-command-history.ts, server/claude-command-history-worker.fake.ts, server/claude-command-history.test.ts, server/index.ts |
The manager schedules history work through one worker lane, tracks bounded session evidence, and provides reset and hold controls. Tests cover worker lifecycle, queue limits, and scheduling. Server startup and shutdown acquire and release a history hold. |
Subagent history reconciliation server/claude-subagents.ts, server/claude-subagents.test.ts, changelog.d/fix-claude-command-lifecycle.md |
Subagent status merges matching historical endings with transcript results and remains pending while evidence is incomplete. The status tracker watches for history changes. Tests cover cold reads, unresolved commands, and queue holds. |
Task scan history and cancellation server/file-tasks.ts, server/claude-task-scan.ts, server/claude-task-scan.test.ts |
Task scans can wait for history on large transcripts with running or lost shell tasks, then pass evidence to subagent analysis. Shared scans abort when their final waiter cancels. |
Estimated code review effort: 4 (Complex) | ~60 minutes
Severity of issue fixed: Medium
Suggested reviewers: devswha, kilhyeonjun
Merge Risk: 
0a3c9
-
When many sessions are tracked, a session denied admission can reconcile background commands slowly on large histories. Queued input can stay held longer than needed. The registration retry should be fixed before merge.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 29.17% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. Linked Issues check Directly linked issue #2601is active. Its coding objectives are implemented at head 0a3c933.
Implemented and evidenced:
- Coverage-aware state: server/claude-command-history.ts tracks offset, comple…Provide evidence of a test that exercises failed-chunk retry behavior and shows retries stay bounded. Locate or add it in server/claude-subagents.test.ts or server/claude-command-history.test.ts. Also confirm whether the sampled-witness lim… -
Check name Status Explanation Title check The title clearly and concisely describes the main change: reconciling background-command status after cold history reads. Out of Scope Changes check Every changed file ties to the #2601objectives or to their validation:
- server/claude-command-history.ts and server/claude-command-history-worker.ts: core history reconciliation, worker chunks and t…Description check The description follows the template's two sections, Change and Validation. The Change section explains the problem (cold reads of long Claude sessions miss terminal notices outside the 16 MiB tail, s… Full details: Out of Scope Changes check
-
Explanation
Every changed file ties to the
#2601objectives or to their validation:- server/claude-command-history.ts and server/claude-command-history-worker.ts: core history reconciliation, worker chunks and the physical slot.
- server/claude-subagents.ts: command terminal extraction and historyPending, which feed the task list and pane tracker.
- server/claude-task-scan.ts and its test: shared scan cancellation for the task-list route.
- server/file-tasks.ts: passes history evidence to the claude-tasks file route.
- server/index.ts: holds the history worker during server lifetime. This supports the retained-slot requirement.
- Tests and fake worker: validate the listed regressions. The fake worker is test-only.
- changelog.d entry: documentation of the fix.
No change is shown to be unrelated to the issue.
Full details: Description check
-
Explanation
The description follows the template's two sections, Change and Validation. The Change section explains the problem (cold reads of long Claude sessions miss terminal notices outside the 16 MiB tail, so completed commands appear running), the approach (shared resumable history evidence, cooperative slicing, validation against stored digests), and links issue
#2601. The Validation section lists concrete checks run on a specific head commit with results (354 targeted tests, 10,621 fast-check tests, 0 failed), notes which checks were not run (full integration/browser, SSH, native Windows locally), and reports one browser script timeout that passed on retry. The template's comment asks for the changelog entry in changelog.d/, and the raw summary shows changelog.d/fix-claude-command-lifecycle.md is present, so that requirement is met. The description is dense and uses some internal terms (for example 'independent busy-signal barriers', 'frozen-source') without definitions, and the validation section mixes many numbers that are hard to reproduce from the text alone, but the required information is present.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 29.17% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 24 functions across 10 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
Full details: Linked Issues check
-
Explanation
Directly linked issue
#2601is active. Its coding objectives are implemented at head 0a3c933.Implemented and evidenced:
- Coverage-aware state: server/claude-command-history.ts tracks offset, complete, uncertain and waiting. Unresolved work stays conservative. Age alone does not drop a launch.
- Version binding: HistoryVersion uses dev:ino, size and mtimeMs. sameHistory() rejects replacement and truncation. Worker checkpoints carry a witness hash and throw "rewritten" on prefix change.
- Incremental work: historyChunk reads bounded chunks. Each chunk requeues behind other sessions, which gives fairness.
- Shared view: claude-subagents, file-tasks and claude-task-scan consume the same HistoryEvidence.
- Notice semantics: the test "shares accepted notice and successful TaskStop semantics" covers accepted notices and removed or quoted records.
- Worker slot: the single physical thread stays charged until close. The test "keeps a blocked physical slot..." covers this. File-pool work proceeds meanwhile.
- Cancellation: a cancelled waiter does not discard the checkpoint.
- Bounds: MAX_SESSIONS 16, MAX_ENDS 5000, queue admission limit.
- Test coverage: cold/warm agreement, partial-record resume, oversized and malformed records, replacement and same-size rewrite, saturation, and a queue/fairness test.
Open points:
- Bounded retries. The issue requires regression coverage for bounded retries. Code bounds retry rate: a failed chunk sets retryAt to now + 2000 ms. The retry count is not capped. No retry-bound test appears in claude-command-history.test.ts. claude-subagents.test.ts was truncated at 16000 bytes, so its retry coverage is unknown.
- Interior rewrites. The PR states that sampled witness checks do not fully validate arbitrary interior rewrites combined with growth. The issue requires rejecting stale evidence after "relevant history changes". Append-only Claude histories are assumed. This limit is disclosed but not covered by the issue's own wording.
- Fairness. The queue test asserts only that the large session has not finished before the small one. It shows weak evidence of fair scheduling.
Resolution
Provide evidence of a test that exercises failed-chunk retry behavior and shows retries stay bounded. Locate or add it in server/claude-subagents.test.ts or server/claude-command-history.test.ts. Also confirm whether the sampled-witness limit on interior rewrites with growth meets the issue's stale-evidence requirement. A reviewer must decide whether the disclosed limit is acceptable for append-only histories.
-
Finishing Touches
-
Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
|
Independent agent review completed before publishing this PR and was repeated after the fixes. The review found four reproducible defects, all resolved before the final frozen-source checks:
The reviewer independently reran the original history, reset/reacquisition, and cancellation reproductions successfully, reviewed the six new regression cases, and reported no remaining blockers. The review also covered the dedicated lane's retained physical capacity, bounded queues, shared task-list/status evidence, conservative queue holds, accepted terminal carriers, and terminal-only parsing. Final focused tests: 146 passed, 0 failed. Final frozen-source The documented generation limit remains: sampled checkpoints do not prove arbitrary unseen interior rewrites combined with growth. The PR does not claim full content-addressed history validation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Thanks @duytbp, this fixes #2601: the cold task-list scan reports finished commands on this branch and |
… launch Replace the history Worker with a cooperative drain on the main thread (finishAsync slices of about 4 ms), so a crash or hang can no longer respawn or wedge a thread. It starts lazily and leaves nothing scheduled once the last server lets go. A failed read wakes waiting requests and retries with backoff while watched, giving up after five failures; a replaced transcript wakes its waiters, who follow the new one. - An end closes only a launch at or before it: a reused id is a new command, and each transcript keeps the last few ends of an id. - Every byte read is kept as per-span xxHash3 digests and checked again after a change, so a rewrite anywhere in it drops its ends. - A row whose sessionId names another session ends nothing. - History never holds a pane's queue or keeps it from resting; records too large or not JSON are passed over without a validating parse. - The session cap evicts the least recently used unwatched read; a refused watch is asked for again at the next poll. - Tests wait on events instead of fixed sleeps. Fixes devswha#2601
|
Pushed the maintainers' fix round as 8e1ecbe on top of your f2b6948 (fast-forward, your intent kept: a refused watch is asked for again at the next poll).
16 new tests fail on 0a3c933; the three test files pass 135/0 and |
|
Re-review of 8e1ecbe (Codex Terra xhigh; omo still running). Confirmed fixed: an end older than its launch never closes it, the digest catches a rewrite after a completed check, history rows from another session end nothing, history that cannot be fully read no longer holds a pane's queue (#2069), waiters follow a replaced transcript. Still open on this branch (a maintainer lane will push these here, no force push):
Behaviour that is the same on main, noted here and not part of this PR: a row from another session inside the recent 16 MiB window still feeds command reconciliation, and when the parent and a subagent both use an id, the parent's earlier command wins over the subagent's later one. |
|
One remaining P2 on In The synthetic reproduction below completes the initial read, changes an interior task ID from Could the changed-version evidence expose terminal authority only after current-version verification, or explicitly identify the pending snapshot so the main parser cannot treat unverified cached ends as current? I have made no code changes or pushes while your round is in progress. Synthetic reproduction — temporary files onlySave this as import {
appendFileSync, closeSync, mkdirSync, mkdtempSync, openSync,
rmSync, statSync, writeFileSync, writeSync,
} from "node:fs";
import { tmpdir } from "node:os";
import { join } from "node:path";
import {
claudeSubagentState, forgetSubagents, wholeClaudeSubagents,
} from "./server/claude-subagents.ts";
import {
commandHistory, waitCommandHistory,
} from "./server/claude-command-history.ts";
const root = mkdtempSync(join(tmpdir(), "ovis-history-authority-"));
const path = join(root, "synthetic.jsonl");
const dir = join(root, "synthetic", "subagents");
const now = Date.now();
const json = (value: unknown) => `${JSON.stringify(value)}\n`;
mkdirSync(dir, { recursive: true });
try {
const lead = json({ type: "progress", value: "x".repeat(4096) });
const notice = json({
type: "queue-operation", operation: "enqueue",
timestamp: new Date(now - 1000).toISOString(),
content: "<task-notification><task-id>shell</task-id><status>completed</status><summary>Background command finished</summary></task-notification>",
});
const filler = json({ type: "progress", value: "x".repeat(1024) });
writeFileSync(path, lead + notice + filler.repeat(18000));
const limit = statSync(path).size;
writeFileSync(join(dir, "agent-a.meta.json"), JSON.stringify({
agentType: "reviewer", description: "Synthetic task",
requestShape: "background",
}));
writeFileSync(join(dir, "agent-a.jsonl"), json({
type: "user", timestamp: new Date(now - 2000).toISOString(),
message: { content: [{
type: "tool_result", tool_use_id: "bash", content: "running",
}] },
toolUseResult: { backgroundTaskId: "shell" },
}));
wholeClaudeSubagents(path, true, now);
await waitCommandHistory(path, limit);
const status = () => claudeSubagentState(path, true, now)
.tasks.find((task) => task.id === "shell")?.status;
console.log("initial:", status());
const fd = openSync(path, "r+");
try {
writeSync(fd, Buffer.from("other"), 0, 5,
Buffer.byteLength(lead) + notice.indexOf("shell"));
} finally { closeSync(fd); }
appendFileSync(path, json({ type: "progress", value: "appended" }));
const immediate = claudeSubagentState(path, true, now);
const evidence = commandHistory(path, limit, false);
console.log("before digest:", {
status: immediate.tasks.find((task) => task.id === "shell")?.status,
historyPending: immediate.historyPending,
complete: evidence?.complete,
ends: evidence?.ends.map((end) => end.id),
});
await waitCommandHistory(path, statSync(path).size);
console.log("after digest:", status());
} finally {
forgetSubagents();
await new Promise<void>((resolve) => setImmediate(resolve));
rmSync(root, { recursive: true, force: true });
}Observed on Independent validation on this head: the three history/subagent/task-scan suites passed 135/0; |
|
Thanks for the maintainer fix round. Independent re-review confirms the pre-launch/foreign-session cases and awaited prefix-digest validation are corrected; 343 targeted history/status/pending tests passed, including the existing busy-signal guards. The remaining transient cached-authority case matches the synthetic reproduction I posted below. I will hold further code pushes on this branch while your lane addresses that and the equal-timestamp/shutdown findings, then review the updated head. |
…d to its launch - Each history end keeps the line it came from: its offsets and an xxHash3 of it with the newlines on both sides. Unless every byte read was checked at the transcript's current version, an end is told only if its own line reads the same now, re-read before and after a stat. A rewrite that also grew the file no longer shows a live command finished for up to 5 s, and a transcript that only grows keeps its ends without waiting for a check. Evidence handed to the task list's file thread is vouched for again the same way, and at most 4 MiB of lines are re-read at once (past that, unknown: running). - An end closes a launch only when it is strictly after it, or in the same millisecond when both name the same Bash call, and never when they name different calls: an older use's end leaves a reuse running. - A stopped tracker polls nothing, and a history watcher let go of before its queued callback runs is not called, so no read starts after the last server lets go.
|
Thanks again @duytbp for this fix and for bearing with the review rounds. The maintainers pushed one more fix round on top of 8e1ecbe (fast-forward, no force push) for the three points left open in the re-review:
Ten new tests use events, not sleeps. The nine that cover these fixes fail on 8e1ecbe, and mutation checks confirmed that undoing each guard fails a test. The three test files pass 145/0, and |
…ends it TaskStop and KillShell answers name no Bash call, so the tie guard left a command stopped in its launch's own millisecond running forever. In the same transcript the order of its rows decides a tie: an end written after the launch closes it. Across transcripts a tie still closes a launch only when both name the same call.
|
Independent review of I then reviewed the additional
I have made no code changes and am still respecting the maintainer push hold. On exact head |
|
Thanks @duytbp, this fixes #2601: in a long Claude session, background commands whose end notices sit before the parent transcript's last 16 MiB showed as running forever after a cold read (task list and pane count). The cold task-list scan reports them finished on this branch and running on main. How it works now (after the maintainers' rounds on this branch): the older history is read on the main thread in ~4 ms slices through the cooperative step runner (no Worker: total time matched the Worker in our measurements, 200 MB worst case ~1.3 s with no slice over ~5 ms, and there is no thread to crash or hang). Ends found there are matched to the launch they close (an end before its launch never closes it; a same-millisecond tie is decided by row order inside one transcript and needs the same Bash call id across transcripts), each end is trusted only while its own line is unchanged, rows naming another session end nothing, and history that cannot be fully read never holds a pane's queued input or keeps it working (#2069's behaviour is unchanged). Review (omo + Codex Terra xhigh on every round): every HIGH reproduced on this PR's own changes is fixed, with tests that fail on the earlier heads; the last round was checked by Terra (APPROVE) and the omo re-review approved. Known, pre-existing on main, not part of this PR: a row from another session inside the recent 16 MiB window still feeds command reconciliation, and when the parent and a subagent both use an id, the parent's earlier command wins over the subagent's later one. LOW, left as is: past 16 watched sessions a further session falls back to main's behaviour (#2601 unfixed for that pane); only the newest 5000 ends per transcript are kept; the 5-second follow-up check of an idle transcript waits for its next change; a same-size CodeRabbit's check shows skipping on these heads; the merge pass and both reviewers reviewed every head. |
Change
A cold read of a long Claude session found old background Bash launches in subagent files but missed terminal notices before the parent's 16 MiB tail. Completed commands then appeared running indefinitely in the task list and pane count.
Share resumable historical terminal evidence between the task-list reader and pane tracker. Reads and consumed-span validation run cooperatively on the main thread in roughly 4 ms slices, with bounded session/terminal metadata and fair scheduling. Existing file/command/task pool capacity is unchanged.
Match historical ends to the session, launch time and Bash call. Before using cached evidence at a changed transcript version, validate the needed terminal lines, including newline boundaries, against their stored digests. Same-transcript row order also permits a stop written after its launch in the same millisecond. Released watchers and stopped trackers cannot restart reads after shutdown.
Incomplete, malformed, oversized or capped history does not hold queued input or keep a pane working; existing independent busy-signal barriers retain that policy (#2069). History never replays prompts or turn state. Refused watches retry after admission, partial records resume after append, and reads retry with bounded backoff.
Fixes #2601.
Validation
ce789b9, independent history/subagent/task-scan suites passed 146 tests and core status/background-wait/pending suites passed 208: 354 passed, 0 failed. All three prior rewrite/timestamp/shutdown reproductions and five additional row-order controls pass.bun run check fastonce789b9with CI-pinned Bun 1.4.2 passed 10,621 tests, 0 failed, 41 skipped in 658 files, plus workflow syntax, generated files, typecheck and build. The independently reproduced same-millisecond repeated-stop deduplication edge remains open. Upstream CI succeeded, including integration/browser and native Windows. One browser script (device-security-demo-regression.ts:283) timed out once and passed on its automatic retry.