fix(sessions): stop orphaning sessions at their writers and repair existing orphans (#6144) - #6640
5 commits merged into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…verage threads, recover orphans in unbound stores (#6144) Review follow-ups on #6640: - POST /v1/sessions writes only the document derived from the thread id. A thread opened with resume-thread is bound to the original saved session; updating that in place from the thread's projection would drop content the projection does not carry. Re-export and crash retry stay idempotent on the derived id. - resume-thread reuses an open thread only when its checkpoint covers the whole document. A document that grew after the bind (TUI autosave) gets a fresh thread holding all of it, instead of a thread missing the appended messages. - Reconcile applies R4 to unbound stores too: threads naming a missing document are unbound with a receipt, then recovered, instead of keeping the store forever. Tests: focused nextest (session_reconcile, resume/export/fork filters) 126 passed, 0 failed, including 3 new tests. cargo fmt --check clean; clippy -p codewhale-tui --all-targets --all-features with CI flags clean; blocking-calls, dead-code, command-crate-boundaries and module_graph gates pass. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a507c4e1c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| autosave. `PATCH`, `PUT` and `DELETE` therefore fail closed on it with | ||
| `409 Conflict` rather than writing something that would be silently reverted. | ||
| Change it in the terminal instead. The open process holds a lock on the | ||
| session (`sessions/.late-usage/<id>.live`), so this holds whether the request | ||
| reaches the API inside that process or a separate `codewhale serve`. |
There was a problem hiding this comment.
When the API runs in a separate codewhale serve process, patch_session still calls rename_session and set_session_archived, which consult only the process-local is_live_session registry rather than the new cross-process lease. Consequently, a PATCH /v1/sessions/{id} can race an interactive TUI autosave and be silently overwritten even though this documented contract promises a 409 Conflict; the PATCH path must check is_session_live_anywhere as PUT and DELETE do.
Useful? React with 👍 / 👎.
| if references.binds(&store.path) { | ||
| // R4 for a bound store nobody holds. | ||
| if self.options.dry_run { | ||
| return; | ||
| } | ||
| match held.unbind_threads_without_documents(exists) { |
There was a problem hiding this comment.
If repair stops after recovered_document saves the derived session but before bind_recovered_thread updates the thread, the next run sees that document's runtime_store binding and enters this branch. It then only unbinds references to missing documents, so the still-unbound thread never reaches recover_store and the retry logic at recovered_document is unreachable; the thread remains permanently detached and later turns are not reflected in the recovered document.
Useful? React with 👍 / 👎.
| if self.options.skip_session.as_deref() == Some(id.as_str()) | ||
| || SessionManager::load_session_metadata(&path).is_ok() | ||
| { | ||
| continue; |
There was a problem hiding this comment.
For a session file whose metadata object is intact but whose later messages or tool_log JSON is truncated or malformed, load_session_metadata succeeds because it intentionally parses only the metadata prefix. This early continue therefore bypasses classify_unlisted_document's full SavedSession deserialization, leaving the corrupt document listed and counted as a valid store reference while every actual resume fails; R1 never sets it aside.
Useful? React with 👍 / 👎.
| if self.options.dry_run { | ||
| return; |
There was a problem hiding this comment.
With --dry-run, a bound store containing a thread whose referenced document was deleted returns here without even counting the pending unbind, while a real run increments threads_unbound. The analogous skip in recover_store also leaves such threads marked bound, causing recoverable_threads to omit the sessions that the real run would recover, so the command advertised as a repair preview substantially under-reports its planned actions.
Useful? React with 👍 / 👎.
| let target_id = req | ||
| .session_id | ||
| .clone() | ||
| .unwrap_or_else(|| snapshot.session_id.clone()); |
There was a problem hiding this comment.
When PUT /v1/sessions omits session_id for a resumed thread, the new engine configuration makes snapshot.session_id equal the existing bound document ID, but target_id is used only for the lease check. The construction below still enters the req.session_id == None branch and creates a blank SavedSession over that ID instead of loading and updating it, which discards the existing title, archive/fork metadata, artifacts, work state, and runtime_store binding; the latter recreates the orphan this change is intended to prevent.
Useful? React with 👍 / 👎.
| let thread: ThreadRecord = match read_store_file(&path) | ||
| .with_context(|| format!("Failed to read {}", path.display())) | ||
| .and_then(|raw| { | ||
| serde_json::from_str(&raw) | ||
| .with_context(|| format!("Failed to parse {}", path.display())) | ||
| }) { | ||
| Ok(thread) => thread, | ||
| Err(error) => { | ||
| skipped.push(format!("{error:#}")); | ||
| continue; |
There was a problem hiding this comment.
A valid record written by a newer Codewhale can fail ThreadRecord deserialization when that schema changes an enum or field shape. Because this branch classifies every deserialization failure as a skippable corrupt record before inspecting schema_version, such a future record disappears from thread listings instead of triggering the promised newer-schema refusal, allowing an older binary to operate while silently hiding work it cannot understand.
Useful? React with 👍 / 👎.
| fs::remove_dir(session_dir)?; | ||
| } | ||
| } | ||
| self.retire_released_stores(id, bound_store); |
There was a problem hiding this comment.
This newly added call synchronously scans every saved document, canonicalizes paths, inspects automations, and may move a whole Runtime store. SessionManager::delete_session is called directly by the async Runtime API delete handler and by the TUI event path, so a large session store now blocks the Tokio/UI thread during deletion; schedule the retirement work through spawn_blocking or another dedicated worker.
AGENTS.md reference: AGENTS.md:L164-L170
Useful? React with 👍 / 👎.
| parts.push(format!("{} recovered", self.sessions_recovered)); | ||
| } | ||
| if self.threads_unbound > 0 { | ||
| parts.push(format!("{} threads re-linked", self.threads_unbound)); | ||
| } | ||
| let set_aside = | ||
| self.stores_set_aside + self.artifact_dirs_set_aside + self.documents_set_aside; | ||
| if set_aside > 0 { | ||
| parts.push(format!("{set_aside} unused items set aside")); | ||
| } | ||
| let mut line = format!("Sessions repaired: {}", parts.join(", ")); |
There was a problem hiding this comment.
The result of ReconcileSummary::notice is displayed directly as a TUI status toast, but all of its newly added user-visible fragments are hard-coded English. Non-English users therefore receive an untranslated launch notification; define message IDs with parameters for the counts/path and construct the toast through tr(locale, MessageId::...).
AGENTS.md reference: crates/tui/AGENTS.md:L25-L26
Useful? React with 👍 / 👎.
| let mut session = crate::session_manager::create_saved_session_with_id_and_mode( | ||
| id, | ||
| &recovered.messages, | ||
| &thread.model, | ||
| &thread.workspace, | ||
| 0, |
There was a problem hiding this comment.
Every R3 recovery passes a hard-coded zero token count and leaves the new session's cost snapshot at its default even though the recovered thread's durable turn records contain usage and route settlement data. A normal orphan with completed paid turns therefore reappears with zero tokens and zero historical cost, and resuming/autosaving that document makes the incorrect accounting durable; aggregate the recovered turns using the same usage projection used by Runtime API exports.
Useful? React with 👍 / 👎.
| let sessions_dir = default_sessions_dir().unwrap_or_else(|_| fallback_sessions_dir()); | ||
| // Repair the saved-session store once per server start (#6144); this | ||
| // server's own store is open by now, so it reads as in use. | ||
| crate::session_reconcile::spawn_background_reconcile(None); |
There was a problem hiding this comment.
If default_sessions_dir() fails, the server deliberately continues with fallback_sessions_dir() for all session endpoints, but spawn_background_reconcile independently calls SessionManager::default_location() and silently returns on that same failure. In exactly the environment that selects the fallback, codewhale serve therefore never repairs the store it is serving; pass the selected sessions_dir into the worker and surface initialization failures rather than skipping repair.
AGENTS.md reference: AGENTS.md:L79-L81
Useful? React with 👍 / 👎.
…isting orphans (#6144) The session document is the one authority for a conversation. The document-to-store link lives only in metadata.runtime_store, and the thread-to-document link lives only in ThreadRecord.session_id plus its checkpoint. Directory names are not ownership. Fixed at the writers: - P1a: a switch that rebinds a conversation to this host's store sets aside the empty store it left, unless another document binds it. - P1b: on exit, the host retires its own store if nothing binds it. - P2: remove_session follows the binding instead of sessions/<id>/runtime. It also no longer deletes runtime-recovered-* stores, which another document may bind. - P3: POST /v1/sessions is idempotent. It reuses thread.session_id, or an id derived from the thread, so a retry after a crash binds the document that was already written. PUT with another thread's session_id returns 409. - P4: checkpoints cover a prefix (messages_len), so appends to the document no longer strand the thread. Legacy checkpoints migrate on read in one O(n) pass. A thread unbinds, with a receipt, only when the document is missing or has diverged. - P4: a cross-process live lease (.late-usage/<id>.live). PUT and DELETE refuse a session that is open in any process. - P5: DELETE unbinds the threads that referenced the document. - P7: one corrupt thread record no longer fails list_threads. - P8: session_boot_owners.json read-modify-write is locked. - P9: Runtime-thread engines write under the thread's session id. They previously used a random id on each load. Repair on load (new session_reconcile.rs): runs at TUI launch after the host holds its store, at codewhale serve start, and from `codewhale doctor --repair-sessions [--dry-run]`. It is bounded, single-flight, and moves items aside with a manifest; it never deletes. Tests: 989 run, 989 passed (nextest, focused on runtime_threads, runtime_api session/thread, session_manager, session_reconcile, runtime_store_binding, snapshot, task_manager, doctor and delete filters). Clippy (CI flags) is clean on codewhale-tui. fmt is clean. The blocking-call budget was raised by 3 canonicalize sites in the synchronous session_reconcile module. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…verage threads, recover orphans in unbound stores (#6144) Review follow-ups on #6640: - POST /v1/sessions writes only the document derived from the thread id. A thread opened with resume-thread is bound to the original saved session; updating that in place from the thread's projection would drop content the projection does not carry. Re-export and crash retry stay idempotent on the derived id. - resume-thread reuses an open thread only when its checkpoint covers the whole document. A document that grew after the bind (TUI autosave) gets a fresh thread holding all of it, instead of a thread missing the appended messages. - Reconcile applies R4 to unbound stores too: threads naming a missing document are unbound with a receipt, then recovered, instead of keeping the store forever. Tests: focused nextest (session_reconcile, resume/export/fork filters) 126 passed, 0 failed, including 3 new tests. cargo fmt --check clean; clippy -p codewhale-tui --all-targets --all-features with CI flags clean; blocking-calls, dead-code, command-crate-boundaries and module_graph gates pass. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… set them aside (#6144) HeldRuntimeStore::move_to renamed the store directory while holding the open owner-lock file inside it. Windows refuses to rename a directory with an open handle beneath it, so every set-aside and retire failed there (five session_reconcile / runtime_store_binding tests red on Test (windows-latest)). Now each entry except the lock file is renamed into the destination while the lock is still held (renaming siblings of an open file is allowed on every platform), a partial failure moves entries back, and only after the lock drops are the lease file and the emptied directory removed. The session export retry test now addresses DELETE by the session id the server returned instead of the SHA-derived thread_session_id, which removes the taint source behind the two CodeQL cleartext-transmission alerts in runtime_api/tests.rs. Tests (macOS): cargo test -p codewhale-tui --lib -- session_reconcile:: runtime_store_binding:: session_export_retry_after_a_crash_binds_the_document_it_wrote -> 25 passed, 0 failed. cargo fmt --check clean; clippy -p codewhale-tui --all-targets --all-features with CI flags clean. Windows proof comes from the Windows CI job. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…e move (#6144) a4abe8a moved every entry except the owner-lock file, dropped the lock, then unlinked the lock file and removed the directory. On unix remove_file succeeds while another process holds an flock on the file, so an opener that locked in that gap held a lock on an unlinked inode; the directory was recreated and a second opener took a fresh lock: two owners of one store. HeldRuntimeStore::move_to now renames the lock file into the destination too, last, while it is still held; removes the emptied source directory while still holding it (a non-empty directory means an opener created a fresh store there, which is its own and stays); and drops the lock last. Nothing is unlinked. The Windows fix is kept: the directory is still not renamed, and renaming the open, locked file is allowed there because std opens files with FILE_SHARE_DELETE (library/std/src/sys/fs/windows.rs, OpenOptions::new share_mode) and CreateFileW documents that delete access "allows both delete and rename operations"; LockFile byte-range locks do not block a rename. Renaming the lock file leaves one narrower race: an opener that opened the old path before the move and locks after it holds the moved file. RuntimeProcessOwnerLock::acquire and try_acquire_file now check, after locking, that the locked file is still the one at the path (dev/inode, or volume serial/file index on Windows) and reopen the path when it is not. Tests: opener_in_the_set_aside_gap_is_the_only_owner takes the lock at the old path after the lock file moved and before the lock drops, and proves a second opener is refused and the moved store's lock is free. opener_that_locks_a_moved_lock_file_reopens_its_path runs a whole set-aside between an opener's open and lock; with the identity check stubbed out it fails (second opener also acquires), with it it passes. Verification (macOS): - scripts/dev-cargo.sh nextest run -p codewhale-tui --lib --locked -E 'test(adoption_refusal::) | test(session_reconcile::) | test(runtime_store_binding) | test(task_manager::) | test(owner_lock) | test(live_session) | test(picker_adopt) | test(process_owner) | test(session_export_retry) | test(runtime_threads::)': 337 run, 337 passed - cargo fmt --all -- --check: clean - clippy -p codewhale-tui --all-targets --all-features --locked with CI flags: clean - check-blocking-calls-budget.py, check-dead-code-budget.py, check-command-crate-boundaries.py: pass Windows: not run locally; the Windows CI job is the proof. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
a507c4e to
cf42682
Compare
…egacy_root main (#6641) removed Config's top-level api_key/base_url fields, so the session_reconcile fixture no longer compiled on CI (E0560). Checks: cargo test -p codewhale-tui --lib -- session_reconcile: 11 passed; 0 failed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Reconciles #6640 with #6645 (already merged here): - Engine identity: #6645's rule stands (every Runtime thread's engine runs under thread.id). #6640 had also set the field to the bound document or a derived UUID; git kept both assignments, so the duplicate is dropped. - thread_session_id(thread_id) now returns the thread id itself, so the export document (POST /v1/sessions), the engine's session directory and the thread name one conversation, and the export stays idempotent. The derived-UUID scheme never shipped. - save_current_session keeps #6640's guards (409 when naming another thread's document; live-session refusal) and #6645's target choice (the bound document, else the thread's own id). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Conflict in RuntimeProcessOwnerLock::acquire (runtime_threads.rs): #6640 wrapped the open+flock in a retry loop that re-opens when a maintenance move renamed the lock file (OWNER_LOCK_MOVED_RETRIES, owner_lock_is_at_path), while #6601 changed the contention-deadline exit from an untyped bail! to an io::Error of kind WouldBlock so session_secret_scrub can skip a busy Runtime store and report it instead of failing the whole scrub. Kept #6640's retry structure and applied #6601's typed error to both "held" exits (deadline and moved-retries exhausted) through a small held_error() helper, so the scrub's WouldBlock downcast sees every held case. scripts/check-blocking-calls-budget.json: both sides added a new file entry at the same spot (session_reconcile.rs from #6640, session_secret_scrub.rs from #6601); kept both, sorted. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Each fix below reconciles two PRs that merged textually but not semantically. - snapshot/repo.rs, receipts.rs: #6645/#6682 added SnapshotRepo::changed_paths_between(from, to) -> Vec<PathBuf> (undo, turn artifacts) and #6591 added a different changed_paths_between(from, to, limit) -> (Vec<SnapshotPathChange>, bool) (receipts). Duplicate definition; #6591's is renamed path_changes_between and its one caller (receipts) updated. - core/engine/turn_loop.rs: #6673's repl-fence approval match did not cover #6601's ApprovalResult::TimedOut. A timed-out card now refunds the tool-call budget slot and reports a timeout, like direct and code-mode calls; the audit line records "timeout" instead of "denied". - runtime_api/sessions.rs: #6640's session-owner 409 predates #6645's ApiError.code field; code: None. - tools/verifier.rs: #6671's env-scrub test called run_gate(gate) without the session_id argument run_gate takes on main (#6508). - skills/install.rs + integration harness: #6679 made install.rs read downloads through crate::utils::read_response_body_capped, but the integration harness #[path]-includes install.rs and has no utils module, so the integration test target did not compile (also on the #6679 branch). The capped reader moves to utils/response_body.rs (re-exported from utils, unchanged API) and the harness includes just that file as crate::utils. - Test files where an add-only conflict was auto-resolved by concatenating both sides lost the shared closing lines of the first test: commands/groups/debug/tests.rs (#6682 + #6591), tui/ui/tests.rs (#6635 + main), tools/shell/tests.rs (#6674 + #6679), and runtime_api/tests.rs (#6645 merge in round 1). Restored the missing `}` / `);` so each test is whole again; no assertions were dropped. - tools/shell/tests.rs: #6637's executor test expected `sort * | cat` to be admitted and then fail at run time; with #6675's rule ported into the #6637 lexer (see the #6675 merge), a word-leading unquoted `*` is refused before anything runs. The test now asserts that refusal and still checks the sentinel and option-named files are untouched. - core/engine/tests.rs -> tui/history/tests.rs: #6601's engine test asserted crate::tui::history on the trust warning, raising the runtime->UI test reference ratchet 40 -> 41 (check-command-crate- boundaries FAIL). That assertion moved to a tui::history test on workspace_trust_runtime_message, so the ratchet is back at 40. - scripts/check-blocking-calls-budget.json: runtime_api/git.rs 5 -> 6. #6648 justified this budget in its PR body (working-tree fingerprint reads in sync fns reached only from spawn_blocking); its own branch already has six such sites (the File::open used for hashing), so the recorded 5 was stale. subagent/worktree.rs tightened 7 -> 6. Checks: cargo check --workspace --tests clean (no warnings); cargo test -p codewhale-execpolicy 221+1+5+7+1 passed; cargo test -p codewhale-tui --test integration 187 passed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Closes #6144
Refs #6639 (follow-ups split out of this PR)
What orphans sessions, and where each one is fixed
The session document is the one authority for a conversation. Each of the two links has one home:
metadata.runtime_storeThreadRecord.session_idplus its checkpointA directory name does not say who owns it. On one real store, 0 of 82 documents were bound to the store under their own id. 79 were bound to a store under another id's directory, and 78 of the "document-less" store directories were live bindings. The host's boot anchor store holds every conversation that process saved. Any cleanup keyed on "
sessions/<id>/runtimewith no<id>.json" would have set aside live stores. Every rule here is keyed on the reference set instead: all document and checkpoint bindings, plus every thread'ssession_idand derived id./load,/resumeand switches rebind a document onto this host's store and leave the old store on disk with nothing pointing at itremove_sessionlooked only atsessions/<id>/runtime, never at the store the document is actually bound to. It also deletedruntime-recovered-*stores under that directory (viaremove_dir_all), even when another document was bound to themruntime-recovered-*directories are never deletedPOST /v1/sessionscreated a new document on every call, then bound the thread in a second writePUTwith asession_idthat another thread is bound to returns 409PUT) stranded the threadmessages_lenplus a hash of that prefix. The thread keeps its prefix plus its own turns. Legacy checkpoints migrate on read in one O(n) pass that serializes each message once, and the length is written back. The thread unbinds, with a receipt, only when the document is missing or has really diverged; it then loads from its own turns.resume-threadreuses an open thread only when its checkpoint covers the whole document; a document that grew since gets a fresh thread holding all of it.late-usage/<id>.live, separate from the per-write lock.PUTandDELETEreturn 409 if the session is open in any processDELETEunbinds the threads that referenced it. A missing document now also unbinds at read time, and repair unbinds threads in unheld storessession_boot_owners.jsonwas not lockedsession_id: None, so each engine load used a new random id. Compaction transfers (context-transfer-*.json, 411 of the stray artifact files measured), background-shell evidence and spills went to directories that no document names, and the thread lost them at its next loadthread.session_id, or the id derived from the thread, which is the same id export usesRepair on load (
session_reconcile.rs)It runs once the TUI host holds its own store, so that store and any store it resumed or recovered into count as in use and are never candidates. It also runs at
codewhale servestart and fromcodewhale doctor --repair-sessions [--dry-run]..reconcile.lock) and bounded (500 actions per run; the next run continues).sessions/.set-aside/<run>/, andMANIFEST.jsonlrecords each one's original path and reason, plus a sha256 for documents. Actions are logged insessions/.reconcile/receipts.jsonl, and the summary is written to.reconcile/last.json.What each rule does:
Recovered: …document per thread, bound to that store, and each thread is bound back to its document.Surfaces: a one-line toast when something changed;
codewhale doctor(human output, and asessions.last_repairkey in JSON);GET /v1/sessions/repair.Before/after on a copy of one real
~/.codewhale/sessionsThe copy was 447 MB, with bindings rebased onto the copy.
codewhale doctor --repair-sessions:After run 1 there were 85 documents, 83 of them bound. Every bound store is still in place except one binding that was already missing in the original store (it is not in the manifest). All 267 moves are recorded in the manifest.
Where this differs from the approved design, and why
sha256("codewhale-thread-session-v1:" + thread_id)as a UUIDv8, notuuid_v5, which avoids a newuuidfeature and dependency.session-unbind-receipts.jsonl), not insessions/.reconcile. The thread manager does not know the sessions directory, and writing to the default one leaked into a developer home during tests. The file is outside the store's work directories, so it never makes a store look busy.session_id-only link gets a real checkpoint when the projection matches. When it does not, the thread loads from turns, and it unbinds only if the stored record still carries that same link.list_sessionsstill leaves unreadable documents out of the list. R1 sets them aside at launch and reports them, instead of changing the return type for every caller.run_tuiafterTaskManager::start, not in the pre-TUI janitor. This closes the race between the janitor and a launch opening aruntime-recovered-*store.state.dbview or retirement, archiving pre-fix duplicate exports. Also filed there: thecheckpoints/*.offline_queue.lockleak found during measurement (about 3.8k files).Note for the founder: the #6094 release-plan text and
CURRENT_DECISIONS§18 still describe a report-only doctor. §18 is updated locally in codewhale-ops (see below). #6094 needs your edit, since agents do not comment.Verification (run locally)
CARGO_BUILD_JOBS=3 scripts/dev-cargo.sh nextest run -p codewhale-tui --locked -E 'test(runtime_threads::) | test(runtime_api::tests::session) | test(runtime_api::tests::thread) | test(snapshot) | test(session_control) | test(task_manager::) | test(session_picker) | test(doctor) | test(session_reconcile::) | test(runtime_store_binding) | test(picker_adopt) | test(session_manager::) | test(live_session) | test(delete)': 989 run, 989 passedcargo fmt --all -- --check: cleanscripts/dev-cargo.sh clippy -p codewhale-tui --all-targets --all-features --locked -- -D warnings -A clippy::uninlined_format_args -A clippy::too_many_arguments -A clippy::unnecessary_map_or: cleancheck-blocking-calls-budget.py: within budget after--update, which adds 3canonicalizesites insession_reconcile.rs. That module is synchronous by design: its callers run it onspawn_blocking, from the doctor CLI, or on the already-synchronous delete path.check-dead-code-budget.py,check-command-crate-boundaries.py,split/module_graph.py --check: passsync-changelog.sh,derive-changelog,derive-facts,derive-install,check-versions.sh --range-audit-advisory,check-contributor-credit.py v0.10.0, andvitest lib/public-copy.test.ts(6 passed)New tests cover:
DELETEreturning 409Moving a held store (set-aside and retire)
HeldRuntimeStore::move_toholds the store's owner lock for the whole move and unlinks nothing. The store directory is not renamed, because Windows refuses to rename a directory with an open handle inside it. Each entry is renamed into the destination instead, with the lock file last, while the lock is still held. std opens files withFILE_SHARE_DELETE, and CreateFileW documents that delete access "allows both delete and rename operations", so this rename works on Windows too. The emptied source directory is then removed, still under the lock. If the directory is no longer empty, an opener created a fresh store there after the lock file moved; that store belongs to the opener and stays. The lock is dropped last.An earlier revision unlinked the lock file after dropping it. On unix,
remove_filesucceeds even while another process holds an flock, so a gap opener could end up holding a lock on an unlinked inode while a second opener took a fresh lock. That gave two owners. One narrower race remains: an opener can open the old path before the move and lock after it.RuntimeProcessOwnerLock::acquire/try_acquire_filenow check after locking that the locked file is still the one at the path, and reopen it if not. Two tests cover this:opener_in_the_set_aside_gap_is_the_only_ownerandopener_that_locks_a_moved_lock_file_reopens_its_path. The second fails when the identity check is stubbed out.Focused run after this change: 337 run, 337 passed (
adoption_refusal,session_reconcile,runtime_store_binding,task_manager,runtime_threadsand related filters). fmt and CI-flag clippy are clean. Not verified locally: Windows. The Windows CI job is the proof.🤖 Generated with Claude Code