Repository navigation
Conversation
f72a423 to
5dd9492
Compare
|
@devsuitup ready for review: an internal review loop converged with no findings left (the window edges, the closed DB at quit and the cost on folders that never verify were each checked by running probes). Since this PR comes from a fork, its workflows (Test, Build & Release) are waiting for your approval before they can run. |
devsuitup
left a comment
There was a problem hiding this comment.
Review at 5dd9492 (the head was rewritten while I was reviewing f72a423; this review replaces that one). Thanks for the report: the cause is clear, the tail check is a sound direction, the exact tail-window boundary case is fixed, and its regression test passes. The branch is level with main and 161 tests in the 12 related files pass locally on Node 24. Four points block; the reviewer reproduced them with the shipped code and I re-read the scan.
Blocking
- The scan still has no aggregate budget (
derive-project-path.js,extractVerifiedCwdFromJsonland its caller). It runs synchronously, and 100 unresolved transcripts that start at the repository root can cost over 50 MiB of reads on the main thread, on every refresh of the folder. Bound it: a total byte or file budget per folder, and remember a negative result per file (path, size, mtime). - Removing the old transcript directory still deletes the live rows (
session-cache.js~148,main.js~3080). When the CLI moves the file, the old folder disappears and the refresh drops its rows without going through retention. The row of a session whose PTY is live must survive until it has a new folder. - Touches made before the move resolve against the later worktree cwd (
session-touched-files.js~313). A relative path touched while the cwd was still the repository root can open the wrong file. Resolve each touch against the cwd in force at that point, or leave it unresolved when that is unknown. - Regression from the rewrite: hashed long-path folder names (
derive-project-path.js,isWorktreeFolderOf). It requires the folder name to start withencodeProjectPath(projectPath)followed by--worktrees-; for a long path the encoded name is hashed, so the check fails and a later valid worktree cwd is never found. Compare through the same encoder for the worktree path (or compare resolved paths), and add a test with a long path. The Windows alias and junction cases are plausible but not reproduced.
Also needed before merge
CHANGELOG.mdhas no entry under## Unreleasedending(#485).- The history has no test-only commit: per the repo rule, push the test alone first so that CI goes red, then the fix.
Non-blocking
- Rationale comments remain in
derive-project-path.jsandtest/worktree-moved-transcript.test.js; they belong in.ai/contexts/session-cache.md, at most a one-line pointer in the code. - The exit wiring in
main.js(~2423) has no test through the real handler.
Not run: Linux Node 20/22 with c8, lint, fresh CI. CI on this head is not approved yet; I will approve it once the blocking points are fixed.
Reproduces devsuitup#485 before the fix: a transcript the CLI moved from the repository's folder into a worktree folder carries the root as cwd in its first 256 KB, so the worktree folder resolves to no project and the running session's row is dropped. Covers a hashed long-path folder name, the read budget and memo, a vanished old folder, Touched paths written before the move, and the PTY exit handler. Claude-Session: https://claude.ai/code/session_01FDnFeSaiQWifMYd39ojyyt
When a session started at a repository's root enters a worktree, the CLI moves its transcript into the worktree's project folder. The head of that transcript still carries the root as cwd, which does not encode to the new folder, so deriveProjectPath rejected the folder, refreshFolder dropped it, and the running session vanished from the sidebar. When the first rejected cwd is the repository the folder may be a worktree of (compared through the encoder, so hashed long names match), the scan reads the rest of the head and then the last 256 KB; the trust rule is unchanged. Tail reads are budgeted per derivation and unresolved transcripts are memoised by path, size and mtime. Touched keeps the head-only cwd, so paths written before the move stay unresolved. A running session's cache row is no longer deleted by refreshes that follow from the disk, a vanished folder included, and is re-checked when its PTY exits. Claude-Session: https://claude.ai/code/session_01FDnFeSaiQWifMYd39ojyyt
5dd9492 to
d1db126
Compare
|
@devsuitup thanks for the review. Each point is addressed at 2fcf8c5. History: d1db126 holds only the tests (17 failures on its own), and 2fcf8c5 holds the fix. Blocking
Also needed
Non-blocking
|
devsuitup
left a comment
There was a problem hiding this comment.
Review at 2fcf8c5. Thanks for the rework: the four earlier blockers are fixed (the scan budget with the negative cache, the live rows kept when the old directory goes, touches resolved against the right cwd, hashed long-path folder names), and so are the changelog entry, the comments and the real exit-handler tests. The test-only commit d1db126 gives 17 failures before the fix, which is the red-first evidence. 218 tests in 19 related files pass locally on Node 24, and I found no conflict with current main (#374 included) or with #482. One point still blocks.
Blocking
- A temporary read failure is cached as a final answer (
derive-project-path.js,extractVerifiedCwdFromJsonlandtrustedCwdOf). Thecatch {}around the open and read ends inreturn { cwd: null, rejected: firstRejected, complete: true }, so anEBUSYor similar error while the CLI still holds the file is stored byrememberUnresolvedas "complete, rejected". As long as the file keeps the same size and mtime, it is never opened again. With one injectedEBUSYon the first read, the base code retries and resolves the unchanged transcript, and this head never does: the moved session stays out of the sidebar, which is the bug this PR fixes. Fix: returncomplete: falsewhen the open or a read throws, and do not memoise it. Test: inject oneEBUSY, then check that the next refresh resolves the transcript.
Non-blocking
- A valid transcript can be starved behind about 4,100 unresolved candidates: the memo evicts its oldest entry at 4,096, and each refresh then consumes the four-window tail budget before reaching it. A rotating start index per folder, or a larger memo for the folders that matter, would avoid it.
Not run: pre-fix replay of the whole suite, Linux Node 20/22 with c8, lint, fresh CI. CI on this head is not approved yet; I will approve it once the blocking point is fixed.
…ved transcript A transcript whose open fails once (EBUSY while the CLI holds it) must be read again on the next derivation and its folder left to be derived again, and the tail budget must reach a transcript listed after more unresolved ones than one derivation can read, even when the memo cannot keep them. Claude-Session: https://claude.ai/code/session_01FDnFeSaiQWifMYd39ojyyt
…budget An open or read error in the cwd scan was returned as a complete "unresolved" result and memoised, so a transcript held busy by the CLI on the first read was never opened again while its size and mtime held. It is now reported incomplete: not memoised, and its folder is derived again. The next derivation of a folder starts its walk at the first transcript the tail budget skipped, so a valid one listed after more unresolved ones than a budget covers is reached even when the memo cannot hold them all. Claude-Session: https://claude.ai/code/session_01FDnFeSaiQWifMYd39ojyyt
|
@devsuitup both points are addressed: 1a55715 holds only the tests (3 failures on 2fcf8c5), and 3533dea holds the fix. Blocking: a failed read was memoised. The
Non-blocking: starvation. The next derivation of a folder starts its walk at the first transcript the budget skipped (
|
devsuitup
left a comment
There was a problem hiding this comment.
Review at 3533dea. Thanks: the open, head, tail and fstat failures are now retried and not memoised, the scan advances through 4,101 candidates over successive refreshes with bounded memo sizes, and the new tests in 1a55715 fail on 2fcf8c5 and pass on this head (red first, as asked). 270 tests in 19 related files pass locally on Node 24, and the changelog and comment checks pass. One gap remains in the same class as the previous blocker.
Blocking
- A failing
statSyncor directory read still ends as "complete" (derive-project-path.js,unresolvedCwdOf~104 and the enclosingcatch {}of the listing loop ~198-200).unresolvedCwdOfreturnsnullwhenstatSyncthrows, andtrustedCwdOfthen returnsnullwithout settingincomplete. A directory read that throws inside the loop falls into the surroundingcatch {}the same way. In both casesonIncompleteis not called, the refresh records a positive index mtime for the folder, and after the file or directory recovers the reconciliation skips the unchanged transcript. Fix: setincompletein those two catches (and propagate it from the listing failure), and do not memoise. Test: makestatSyncfail once for the moved transcript, then check that the next refresh resolves it.
Non-blocking
- A persistent I/O failure on one candidate can pin the resume cursor (
derive-project-path.js~155) and starve a later valid transcript while the candidates before it keep changing. Skipping a candidate that failed twice in a row, with a retry later, would avoid it.
Not run: Linux Node 20/22 with c8, lint, fresh CI. CI on this head is not approved yet; I will approve it once the blocking point is fixed.
…nscript A transcript whose stat fails once, or a folder or session subdirectory whose listing fails once, must leave the folder to be derived again, and a transcript that keeps failing to open must not hold back the walk past it. Claude-Session: https://claude.ai/code/session_01FDnFeSaiQWifMYd39ojyyt
A transcript whose stat threw, or a folder or session subdirectory whose listing threw, ended the derivation as complete, so the folder was marked indexed and an unchanged transcript was never read again once the error cleared. Those failures now report the derivation incomplete. Only a transcript skipped for budget becomes the next walk's starting point, so one that keeps failing to open cannot hold the walk back. Claude-Session: https://claude.ai/code/session_01FDnFeSaiQWifMYd39ojyyt
|
@devsuitup both points are addressed: 833c933 holds only the tests (4 failures on 3533dea), and 90cbf66 holds the fix. Blocking: a failed stat or listing counted as complete. These now report the derivation incomplete, so
Tests: one Non-blocking: a failing file pinned the cursor. Only a transcript skipped for budget ( Other I/O errors treated as final. I went through the remaining catches on this path:
|
devsuitup
left a comment
There was a problem hiding this comment.
Review at 90cbf66. The blocker of 3533dea is fixed: a failing statSync now marks the folder incomplete, and a failing directory read (both the folder listing and a session subdirectory) calls onIncomplete, so the unchanged transcript is retried after recovery. I read the delta and the listing path, and the four new tests fail on 3533dea's derive-project-path.js by reasoning and pass here. The branch is level with main. 205 tests in 19 related files pass locally on Node 24; no conflict with #374 or #482.
Non-blocking
- The resume cursor only resumes direct files (
derive-project-path.js~158): a persistent tail-read failure plus three changing unresolved files can use up every pass's budget and starve a valid nested transcript. It existed at 3533dea and is a corner of a corner; a follow-up is fine.
Not run: Linux Node 20/22 with c8, lint, fresh CI. The CI runs on this head need to be approved; I am doing that now, and the merge waits for them to be green.
Problem
A session started at a repository's root that then enters a worktree (
EnterWorktree) vanishes from the sidebar while its PTY keeps running. Leaving the terminal panel then leaves no way back to the session.Witnessed on v0.0.89, 2026-10-07. The CLI moves the transcript from
enc(P)intoenc(P/.claude/worktrees/x), and the old folder can disappear with it. The transcript's first ~100 lines keepcwd = P; the first worktree cwd sat at byte 483 010, past the 256 KB head scan.deriveProjectPathrejected the folder (main log:[session-cache] no transcript of folder …--claude-worktrees-… has a cwd that encodes to it; first rejected cwd: "…/runner").refreshFolderthen dropped the folder's rows, and the row inenc(P)went too.Fix
Finding the worktree cwd (
derive-project-path.js,extractVerifiedCwdFromJsonl):mayBeWorktreeFolderOf). Only then does it search the rest of the head, then the last 256 KB.encodedFolderMayExtendinencode-project-path.js), so hashed names over 200 characters match. It only decides whether to read more; the trust rule ((sandbox): refuse extra binds under .claude/.git, and tighten the schedule registry seed #385) is unchanged.Cost:
indexMtimeMs = 0, so reconcile derives the folder again; each pass reads only what is not yet memoised.Callers:
deriveProjectPath(local) andresolveSessionRealCwduse the new scan.Safety net (
session-cache.js):dropFolderRowsdeletes a folder's rows except those of a session with a live non-plain PTY.refreshFolderand the watcher'sflushChanges), a folder with no project, and a cold-scan folder with no verifiable transcript. A transcript missing from its folder gets the same rule.main.jscallsreleaseLiveSessioninside atry, since the DB may already be closed at quit. It re-checks the folder.Docs:
.ai/contexts/session-cache.md→ "Bounded cwd scan", "A transcript moved into a worktree folder", "A running session keeps its row". CHANGELOG: entry under Unreleased / Fixed.Tests
The first commit holds only the tests: alone it has 17 failures in
test/worktree-moved-transcript.test.jsandtest/derive-project-path.test.js. The second commit makes them pass.What
test/worktree-moved-transcript.test.jscovers:resolveSessionRealCwdfinds the worktree cwd, and a complete line starting exactly at the tail window is read.indexMtimeMs = 0, then indexed once the scan completes.dropFolderRowspath, and a folder with no project. They are dropped after exit.ptyProcess.onExitblock frommain.js, run in a VM. It releases both ids after removing them, notifies when a row was released, and survives a release that throws "The database connection is not open".Measured on 50 rejected 660 KB transcripts: 7.9 ms per flush with the former head-only scan, 0.64 ms averaged over repeated flushes with the memo.
task check: 0 errors (the warnings were already there); 4 399 + 119 tests pass. Three internal review rounds verified everything by running probes; the last one found no blocking finding.https://claude.ai/code/session_01FDnFeSaiQWifMYd39ojyyt