refactor: unify sidecar-lock, git, serde, sha256 helpers#395
Conversation
Post-wave reuse/altitude cleanup, no behavior changes intended. - storage.rs: add shared sidecar-lock utility (try_acquire_sidecar_lock / acquire_sidecar_lock_blocking) plus a module note documenting the two sanctioned cross-process strategies (sidecar flock vs atomic-rename+hash), carrying the Windows LockFileEx read+write-handle rationale once. Add append_line_locked(path, line, private) and route PrivateStoreIo::append_line through it. - Migrate the hand-rolled fs2 flock sites onto the helper: transcript_backfill, monitor (write_entry_inner + single-instance guard), user_config save_inner, and run_ledger (deleting its ~30-line append copy). run_ledger's inline sha256 now uses artifact_refs::sha256_bytes. - serde_util.rs: generic is_default<T: Default + PartialEq>; replace the three per-module is_zero serde helpers. - git.rs: hoist git_output (raw Output) and git_capture (trimmed String); migrate worktree, lifecycle, and git_correlation/backfill git helpers. - lcm/query.rs: fold rerank_grep_hits' per-session BTreeMaps into one SessionAgg map (behavior byte-identical). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
![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: b0353ad271
ℹ️ 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".
| file.lock_exclusive()?; | ||
| // Exclusive lock for concurrent writer safety, taken on the shared sidecar | ||
| // lock helper (see the sidecar-lock module note in `src/storage.rs`). | ||
| let file = crate::storage::acquire_sidecar_lock_blocking(mmap_path)?; |
There was a problem hiding this comment.
This module removed use fs2::FileExt, but the lock returned here is still a std::fs::File and write_entry_inner calls file.unlock() after flushing, with another unlock() in run. Without the trait in scope, builds that include monitor.rs fail with no method named unlock, so this refactor currently breaks compilation.
Useful? React with 👍 / 👎.
| let lock_file = | ||
| crate::storage::acquire_sidecar_lock_blocking(&lock_path).map_err(|source| { | ||
| ConfigSaveError::Lock { | ||
| path: lock_path.clone(), | ||
| source, | ||
| } | ||
| })?; |
There was a problem hiding this comment.
The helper returns a std::fs::File, and this module no longer imports fs2::FileExt, but save_inner still calls lock_file.unlock() below. That leaves unlock unresolved for user_config.rs, so the crate cannot type-check until the trait import is restored or the explicit unlock is removed.
Useful? React with 👍 / 👎.
1 participant
Summary
Post-wave reuse/altitude cleanup from a reviewed audit. No behavior changes intended; all migrated sites keep their exact current semantics (create_dir_all placement,
truncate(false), lock mode). CI-exact gates pass.Changes
src/storage.rs) — newtry_acquire_sidecar_lock(non-blocking;None= held elsewhere) andacquire_sidecar_lock_blocking, with the WindowsLockFileExread+write-handle rationale carried once. Migrated the hand-rolledfs2flock sites:transcript_backfill::try_acquire_structured_backfill_lock,monitor(write_entry_inner+ single-instance guard),user_config::save_inner,storage::append_line(via the newappend_line_locked), andrun_ledger.branch.rsintentionally left for its open PR (fix(pr-autotrack): harden daemon PR-branch auto-tracking #377).append_line_locked(path, line, private)in storage replacesrun_ledger's near-verbatim append copy (~30 lines deleted);run_ledgerroutes through it.run_ledger's inlinesha256:{hex}now usesartifact_refs::sha256_bytes.serde_util::is_default<T: Default + PartialEq>(src/serde_util.rs) replaces the three per-moduleis_zeroserde helpers (git_correlation,workflow_index,fact_proposals).src/git.rs) — hoistedgit_output(rawOutput) andgit_capture(trimmedString) next togit_program(); migratedworktree,tracedecay/lifecycle, andsessions/git_correlation/backfill.pr_autotrack::run_gitleft for a later pass (its file is owned by in-flight work).rerank_grep_hits(src/sessions/lcm/query.rs) — folded the four per-sessionBTreeMaps into oneSessionAgg { count, last_idx, has_tool }map. Behavior byte-identical;grep_disclosed_cap_reserves_a_tool_slot_for_capped_sessionsandgrep_caps_hits_per_session_in_cross_session_scopepass unchanged.storage.rs.Notes
fs2::FileExtimports frommonitor.rsanduser_config.rs: those sites now only call.unlock(), which resolves to std's inherentFile::unlock(Rust 1.89+), so the trait import was dead. The lock helper instorage.rsstill usesfs2(lock_exclusive/try_lock_exclusive).transcript_backfill's lock handle was write-only; it is now read+write via the shared helper, matching the Windows rationale (a superset that also fixes a latent Windows-lock case).skill_materialization.rs,managed_skill_model.rs,pr_autotrack.rs,branch.rs.Gates
cargo fmt --all --check: cleancargo clippy --workspace --all-targets --locked -- -D warnings: cleancargo nextest(lib +session_suite,transcript_ingest_suite,storage_suite,automation_runner_test): 1946 passed, 14 skipped, 0 failed. Themcp::server::freshness_teststimer module was excluded from the final run: itstokio::time::timeout-bounded tests stall past their 30s bounds under this machine's concurrent-build load (the unrelatedstartup_catch_up_spawned_once_per_server, which touches none of this diff, stalls identically), indicating tokio-timer starvation, not a regression.🤖 Generated with Claude Code