fix: resolve worktree-lifecycle review findings (dead-child wedge, classification degrades, Held-lock wait, gate probes, sweep ENOSYS) - #111
Merged
Conversation
The gated source-root-missing check (HTTP require_worktree_ready plus the MCP consult_worktree_gate / worktree_gate_block_reason twins) used Path::exists(), which returns false on ANY stat error. A transient EACCES/ELOOP/automount hiccup on the linked worktree's source root was therefore answered as a permanent, non-retryable 410 SOURCE_ROOT_MISSING (and the MCP twin told the agent to start a new serve), so a conforming client permanently stopped polling for a condition that clears seconds later — even though the check is per-request and would self-heal. All three sites now share loomweave_mcp::source_root_confirmed_missing, which uses Path::try_exists and treats only a confirmed Ok(false) (NotFound-style resolution) as gone. On Err the request proceeds: a genuinely-broken root fails downstream with a retryable error instead. Tests: source_root_gate.rs covers present/absent roots and simulates EACCES via a 0o000 parent dir (skipped when running privileged). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d resolve cannot complete Two verified review findings in WorktreeContext's probe, both breaking the invariant that a checkout git has proven to be a linked worktree must never silently fall back to a worktree-local (decoy) store: - Moved worktree: the probe keyed the checkout's own root on its `git worktree list` entry, which goes stale when a linked worktree is moved without `git worktree repair` — the stale entry made the probe degrade to Standalone, letting `analyze` build a 20-30 minute decoy index at <worktree>/.weft/loomweave/. The own checkout root now comes from `git rev-parse --show-toplevel` at the source, which stays correct across an unrepaired move, so the checkout classifies Linked with its unchanged stable ID. The residual can't-complete guards after the linked proof now return a typed LinkedWorktreeUnresolvable error naming `git worktree repair` instead of degrading. - Branch-only nested project: a project directory that exists only on the worktree's branch resolved primary_root to a nonexistent path in the primary checkout, sending callers to an unsatisfiable "run `loomweave install`" hint. The probe now detects the missing primary-side directory and returns a truthful, actionable NestedProjectMissingFromPrimary error naming the missing path. Tests: moved_linked_worktree_without_repair_still_classifies_linked, branch_only_nested_project_errors_instead_of_pointing_at_a_nonexistent_primary. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Moving the pinned-root open ahead of candidate enumeration made every
sweep on a Linux kernel older than 5.6 abort as StoreDirUnreadable:
WorktreesRoot::open is itself an openat2 call, so ENOSYS killed even
pure report-only runs that delete nothing — a regression from the
pre-pin-first behavior, and a contradiction of confine.rs's documented
posture (old kernels get full report visibility with deletions refused
as UnsupportedPlatform).
Restore graceful degradation without weakening the deletion path:
- confine.rs gains error_signals_missing_openat2 (the ENOSYS-equivalent
classifier, unit-testable since a real ENOSYS cannot be provoked on a
modern kernel) and unpinned_candidate_names (plain read_dir + the
wt-[0-9a-f]{64} grammar filter; report-only, never deletion input).
- sweep.rs routes the pin result through sweep_root_after_pin into a
SweepRoot seam: Pinned keeps both report visibility and deletion
authority off the same pinned inode; UnpinnedReportOnly (ENOSYS only)
enumerates unpinned for the report side and refuses every deletion as
UnsupportedPlatform. Every other pin failure still aborts as
StoreDirUnreadable.
- Module/StoreDirUnreadable/open docs updated to state the old-kernel
posture truthfully (Linux is the supported target; this is about
pre-5.6 kernels, not other OSes).
Tests: enosys_pin_failure_selects_the_unpinned_report_only_root and
other_pin_failures_still_abort_the_sweep (in-file), plus
missing_openat2_degrades_to_report_only_enumeration
(tests/worktree_sweep.rs).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…fatal Two review findings on serve's linked-worktree bootstrap wait: 1. Generation-unaware probe: when another analyze held the lock, the wait only checked that the store db had a 'runs' table. If the holder was mid-ensure_isolated_store delete-and-rebuild, that probe passed instantly against the OLD, about-to-be-unlinked db, and a lazily opened ReaderPool connection could pin the deleted generation for the whole session (its stale completed run row reading as Ready). The wait now also requires the store's metadata.json to match this context (new store.rs helper isolated_store_metadata_is_current, sharing the exact reuse-vs-rebuild predicate ensure_isolated_store gates on): a rebuilding holder deletes metadata before create_fresh writes the new file, so a matching metadata + runs-table pair can never belong to a doomed generation. 2. 5-second hard bail: the Held arm killed the MCP session if no runs table appeared in 5s — punishing both a holder that crashed between lock acquisition and initialise_db (fs2 frees the lock; serve should take it and self-recover) and a slow-but-healthy holder. The wait is now a lock-retry loop that never errors: it re-tries acquisition each iteration (becoming the initializer if the lock frees), and after a generous 60s cap falls back to serving a schema-bearing store behind the existing index-building gate (version-skewed holder) instead of dying. The per-iteration policy is a pure function (held_wait_step) with the full truth table pinned in unit tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…path contract once in core Two review findings on the worktree bootstrap path: 1. Dead-child wedge (high): a bootstrap analyze child that died uncleanly (OOM-kill, kill -9, reboot) AFTER publishing its BeginRun running row wedged the worktree in retryable index-building forever — the row suppressed record_early_bootstrap_failure's synthetic-failure INSERT (and its 0-rows arm misread the dead child's row as live progress), should_spawn_bootstrap_analyze refused to respawn once any row existed, and the heartbeat sweep only runs on the next manual analyze. Readiness now proves builder liveness via the per-worktree analyze lock: a live analyze holds it exclusively from before BeginRun until its final transaction lands, so read_worktree_readiness_with_liveness_repair — while HOLDING the lock itself, no probe-then-write window — converts abandoned running rows to failed (new storage helper mark_abandoned_running_runs_failed), and the gate reports the non-retryable index-build-failed envelope with the explicit recovery command. record_early_bootstrap_failure now holds the same lock and repairs the dead child's own row too. A builder that is alive keeps the lock and keeps gating reads as Building; a failed row still requires the explicit recovery command — never an automatic respawn. 2. Lock-path duplication (cleanup): the <repository-store>/worktrees/<stable-id>.lock contract was encoded twice — analyze_lock.rs's lock_path_for_context (hardcoding "worktrees") and worktree_bootstrap.rs's db-path surgery. It now lives once in loomweave-core (worktree::paths: WORKTREES_DIR_NAME moved from the CLI plus linked_worktree_analyze_lock_path and its _for_store inverse); both call sites route through it, and the CLI re-exports the constant so sweep/doctor/install imports are unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ecycle-review # Conflicts: # crates/loomweave-mcp/src/lib.rs
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
This PR hardens linked-worktree lifecycle behavior across MCP, CLI, core, and storage to prevent wedged “index-building” states, eliminate misclassification after worktree moves/nested-branch-only projects, and improve robustness of locking/gating and sweep behavior (including old-kernel ENOSYS handling).
Changes:
- Add dead-builder liveness repair: when a
runs.status='running'row exists but the per-worktree analyze lock is unowned, repair the run tofailedso gates surfaceindex-build-failed(non-retryable) instead ofindex-buildingforever. - Replace
Path::exists()source-root gating withtry_exists()-based “confirmed missing” logic shared across MCP + CLI to avoid treating transient stat errors as permanent removal. - Centralize linked-worktree lock-path contract in
loomweave-core::worktree::paths, and make CLI’s Held-lock wait generation-aware; sweep degrades to report-only enumeration onopenat2ENOSYS.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/loomweave-storage/src/runs.rs | Adds mark_abandoned_running_runs_failed and unit test for repairing abandoned running rows. |
| crates/loomweave-storage/src/lib.rs | Re-exports the new abandoned-run repair helper. |
| crates/loomweave-mcp/tests/worktree_bootstrap.rs | Adds integration tests for dead-builder repair vs live-lock holder gating. |
| crates/loomweave-mcp/tests/source_root_gate.rs | Adds unit tests for confirmed-missing source root logic (try_exists). |
| crates/loomweave-mcp/src/worktree_bootstrap.rs | Implements readiness liveness repair under the analyze lock; tightens lock probing semantics. |
| crates/loomweave-mcp/src/tools/status.rs | Ensures gated sessions consult readiness with liveness repair before displaying latest run. |
| crates/loomweave-mcp/src/lib.rs | Introduces source_root_confirmed_missing and wires it into worktree gate checks. |
| crates/loomweave-core/tests/worktree_context.rs | Adds regression tests for moved linked worktree classification and branch-only nested project erroring. |
| crates/loomweave-core/src/worktree/paths.rs | Defines WORKTREES_DIR_NAME and the linked-worktree analyze lock path + inverse derivation. |
| crates/loomweave-core/src/worktree/mod.rs | Re-exports new worktree path helpers/constants. |
| crates/loomweave-core/src/worktree/context.rs | Makes linked-worktree resolution move-invariant via git rev-parse --show-toplevel; adds typed errors to avoid silent standalone fallback. |
| crates/loomweave-cli/tests/worktree_sweep.rs | Adds tests pinning ENOSYS degradation classification + fallback enumeration behavior. |
| crates/loomweave-cli/src/worktree/sweep.rs | Adds SweepRoot to degrade to report-only enumeration on ENOSYS while refusing deletion as unsupported. |
| crates/loomweave-cli/src/worktree/store.rs | Extracts metadata_matches and adds isolated_store_metadata_is_current for generation-aware Held-lock waiting. |
| crates/loomweave-cli/src/worktree/confine.rs | Adds error_signals_missing_openat2 and unpinned_candidate_names to support report-only fallback on old kernels. |
| crates/loomweave-cli/src/serve.rs | Reworks Held-lock wait to be generation-aware, non-fatal, and cap-based degraded serving. |
| crates/loomweave-cli/src/http_read.rs | Uses confirmed-missing source root predicate instead of Path::exists(). |
| crates/loomweave-cli/src/analyze_lock.rs | Routes linked-worktree analyze lock path derivation through core’s single contract. |
Suppressed comments (1)
crates/loomweave-mcp/src/worktree_bootstrap.rs:372
Connection::open(db_path)will create an empty SQLite DB if the file is missing. In the early-failure path that can leave behind an unmigrated DB file even though this function ultimately can't insert intoruns(no schema), which risks confusing later readers. Open the DB read-write without CREATE and just log+return if it doesn't exist.
let Ok(conn) = Connection::open(db_path) else {
tracing::warn!(db = %db_path.display(), reason, "could not record early bootstrap failure");
return;
};
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+194
to
+200
| let Ok(write_conn) = Connection::open(db_path) else { | ||
| tracing::warn!( | ||
| db = %db_path.display(), | ||
| "worktree bootstrap: dead builder detected but the repair connection failed to open" | ||
| ); | ||
| return read; | ||
| }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the 8 findings from the adversarial code review of
main...release/1.5.0(the worktree-hardening arc). Five independently-developed fixes, each TDD'd in an isolated worktree and merged here.Correctness
worktree_bootstrap.rs): a bootstrap analyze child dying uncleanly after itsBeginRunrow left the worktree in retryableindex-buildingforever. Readiness now does liveness repair via the per-worktree analyze lock: while holding the lock (not probe-and-release, eliminating the probe-then-write race), anyrunningrow is provably abandoned and is converted tofailed, so gates emit non-retryableindex-build-failedwith the recovery command. Live builders still gate as Building; failed rows still require the explicit recovery command.context.rs): an unrepairedgit worktree mv/rename degraded a proven-linked worktree to Standalone and silently built a decoy store. Own-checkout-root now derives fromgit rev-parse --show-toplevel(move-invariant), so moved worktrees classify Linked with their unchanged stable ID; residual failures after linked proof fail loud with a typed error naminggit worktree repair.context.rs): a subdirectory project existing only on the worktree's branch resolved to a nonexistent primary store and an unsatisfiableloomweave installhint. Now a typedNestedProjectMissingFromPrimaryerror with truthful remediation.serve.rs): the wait probed bareruns-table existence and could validate the old db mid delete-and-rebuild, pinning the deleted generation in the reader pool. The wait is now a lock-retry loop keyed on a sharedmetadata_matchespredicate (metadata probed before db, so a pass can't straddle the delete).serve.rs): serve no longer dies on a crashed or slow lock holder — it retries the freed lock (self-heals) or, after a 60s cap on a version-skewed store, serves degraded behind the existing index-building gate.Path::exists()on the source-root gates (http_read.rs+ two MCP twins): transient stat errors (EACCES/automount) read as permanent 410SOURCE_ROOT_MISSINGretryable:false. All three sites now sharesource_root_confirmed_missing(try_exists; only a confirmedOk(false)counts as gone).Regression / cleanup
sweep.rs/confine.rs): openat2 ENOSYS aborted even report-only sweeps. Exactly ENOSYS now degrades to an unpinned report-only enumeration; deletion stays hard-gated on the pinned inode-stable root (UnsupportedPlatform), per confine.rs's documented posture.<repository-store>/worktrees/<stable-id>.lockderivation existed independently in loomweave-cli and loomweave-mcp. Hoisted toloomweave-core::worktree::paths(forward + validated inverse); both crates now route through it.Verification
Full CI floor green on the merged result: fmt, clippy
-D warnings, workspace build, nextest 2339/2339 (3 skipped), rustdoc-D warnings, cargo-deny. Each constituent fix landed with failing-test-first coverage (18 new tests across cli/core/mcp/storage).🤖 Generated with Claude Code