From 6eddcc20d775ea2977a90c30f2d88fe71a648580 Mon Sep 17 00:00:00 2001 From: magnificentlycursed <190320250+magnificentlycursed@users.noreply.github.com> Date: Thu, 8 Oct 2026 20:16:17 -0700 Subject: [PATCH 01/14] docs(design): write-path umbrella and the lock-claim outcome and kickoff rollback increment The umbrella states the write-path problem, the measured and estimated evidence, five increments named by feature in build order, and the cross-increment decisions (lock-clearing wire format reusing the generation protocol gate; git binary resolution on macOS; scaffold-first builds; a work-in-progress limit). The first increment's document specifies claim outcomes decided by a fallible, network-bounded confirmation, kickoff undo for run and plan, and kickoff cleanup that releases the agent's lock and session. Both were reviewed in two rounds by one Solution Architect reviewer. --- ...lock-claim-outcome-and-kickoff-rollback.md | 210 ++++++++++++++++++ .design/write-path-cost.md | 121 ++++++++++ 2 files changed, 331 insertions(+) create mode 100644 .design/lock-claim-outcome-and-kickoff-rollback.md create mode 100644 .design/write-path-cost.md diff --git a/.design/lock-claim-outcome-and-kickoff-rollback.md b/.design/lock-claim-outcome-and-kickoff-rollback.md new file mode 100644 index 000000000..ec22a08f9 --- /dev/null +++ b/.design/lock-claim-outcome-and-kickoff-rollback.md @@ -0,0 +1,210 @@ +# Feature: Lock-claim outcome and kickoff rollback + +Increment design document, written 2026-10-08 against `origin/develop` at 3440c0d05, for the first increment of the umbrella `.design/write-path-cost.md`. Status: reviewed. It was revised after review round 1 (16 findings) and round 2 (verdict: ready to build after fixes), and round 2's fixes are applied. It amends `.design/hub-v3-per-agent-refs.md`: the confirmation timeout now bounds confirmation only, and its 30 s value and REQ-6(b) stand. It is to be amended as the code lands. A bare `#N` is a tracker item; GitHub records are written in full. Handles are listed in the key at the end. + +## Summary + +Today a lock claim is published, then reported as failed if the whole write took more than 30 s. The lock stays on the hub, and the caller records nothing. On v3, confirmation cannot fail at all: a failed fetch is logged and the claim is confirmed against the stale local view. Kickoff then exits, or later fails to launch, without undoing what it created, and `kickoff cleanup` never releases locks. + +This increment does three things: +- makes a claim's reported outcome match what was confirmed on the remote; +- makes kickoff undo what it created when it fails before the agent starts; +- makes cleanup release the agent's lock and session. + +## User-visible behavior + +- `crosslink session work ` succeeds only once the claim is confirmed against a fresh fetch of the remote. Otherwise it exits non-zero with "claim published but not confirmed: ; run `crosslink session work ` again to confirm". The rerun publishes no second claim. +- A kickoff (`run` or `plan`) that fails before its agent starts undoes everything it created in this invocation: + - it releases the issue lock and ends the worktree session; + - it stops the worktree daemon; + - it removes any container or tmux session it started; + - it restores the pipeline state; + - it removes the worktree and the branch, if this invocation created them. + + Each step that fails, or that finds nothing to undo when it expected something, is named along with the command that clears it. +- `crosslink kickoff cleanup` brings the worktree up enough to release the agent's locks and end its session. It then removes the worktree, and deletes the agent's branch only when the branch has no commits beyond the recorded base, unless `--keep-branch` is given. + +## Requirements + +Claim outcome: + +- REQ-1: A claim's outcome comes from confirmation, never from the elapsed time of publication. Confirmation is a fetch of every agent ref and the checkpoint, followed by a reduce. The elapsed-time `bail!` in `claim_lock_v2_inner` is removed. +- REQ-2: Confirmation is fallible. The result is `Unconfirmed { cause }` in three cases: + - the confirmation fetch fails; + - the hub-lock wait plus the fetch does not finish within 30 s (`LOCK_CONFIRM_TIMEOUT_SECS`); + - any step errors after the claim's push has succeeded: adopt, reduce, checkpoint or hydration. + + The bound covers only the network part, the hub-lock wait and the fetch. The local reduce of the fetched snapshot runs to completion: freshness comes from the fetch, and a slow reduce must not make a confirmed claim fail. No caller treats `Unconfirmed` as ownership, which keeps hub-v3 REQ-6(b): no state lets two agents both verify ownership. +- REQ-3: When the local view already shows this agent as holder, the claim runs the same confirmation before returning `AlreadyHeld`. It publishes nothing. If confirmation shows another holder, the result is `Contended`; if confirmation does not complete, the result is `Unconfirmed`. The contention-cleanup `LockReleased` stays on the new-claim path only. +- REQ-4: `session work` records the active issue and the `.active-issue` sentinel only for `Claimed` and `AlreadyHeld`. For `Unconfirmed` it prints the rerun message and exits non-zero, leaving the published claim for the rerun to confirm. +- REQ-5: If `session work` fails after a confirmed claim, it releases the lock. (The base revision already does this for `set_session_issue`; this increment keeps it.) + +Kickoff undo: + +- REQ-6: Kickoff `run` and `plan` record each resource as this invocation creates it, and on any error before the agent starts, undo them in reverse order: + 1. the container or tmux session; + 2. the issue lock and the session, by running `crosslink locks release ` and `crosslink session end` inside the worktree while its daemon is still up, so the worktree agent's own identity signs them; + 3. the worktree daemon; + 4. the pipeline state, restored to what it was before `mark_running` (`run`) or `mark_planning` (`plan`); + 5. the worktree, only if this invocation created it; + 6. the branch, only if this invocation created it and it has no commits beyond the recorded base. + + A worktree or branch that kickoff reused (`--branch` with an existing directory, a swarm-stored branch) is never removed. +- REQ-7: The agent has started once tmux `send-keys` succeeds (`launch_local`) or `launch_container` returns `Ok`, which happens after its early-exit check. From then on, undo is disarmed, and the lock, worktree and branch belong to the agent. +- REQ-8: `kickoff cleanup` works in this order: + 1. it removes the tmux session or container, so no agent process is still running; + 2. it ensures the worktree daemon is ready (`daemon::ensure`, which waits up to 120 s per agent, plus a readiness bootstrap); + 3. it runs `crosslink locks release ` and `crosslink session end` inside the worktree; + 4. it stops the daemon and removes the worktree; + 5. it deletes the branch under the rule in REQ-6 step 6. + + `--keep-branch` skips the branch step. A worktree whose metadata has no recorded base keeps its branch. +- REQ-9: Undo and cleanup verify each lock release with the same bounded, fallible confirmation as REQ-2, run in the worktree after the release: + - if confirmation shows the worktree agent still holding the lock, the lock is reported as stranded; + - if confirmation does not complete, the release is reported as unverified. + + Each report includes its remedy. A failed step does not stop the remaining steps. Each failure is reported with the resource left behind and the command that clears it. The process exits non-zero, with the original error first. +- REQ-10: Kickoff metadata gains an optional `base_commit`, the commit the worktree's branch was created from, written by `create_worktree`. It is absent in metadata written before this increment. +- REQ-11: The worktree agent's identity, its host-side key file and the host's trust approval are deliberately left in place by undo and cleanup. Revoking the approval would leave the agent's already-published events signed by a revoked key. Their removal is tracker #819. + +## Acceptance criteria + +Tests marked "fails first" must fail on 3440c0d05 and pass after the change. They are the regression tests for Corvidae-Coding-Projects/crosslink#110. + +- [ ] AC-1 (fails first): publication is delayed past 30 s by a test hook, and confirmation succeeds. `session work` succeeds, records the active issue, and the hub shows the lock held by the agent. (REQ-1, REQ-4) +- [ ] AC-2 (fails first): the remote becomes unreachable after the claim's push. The result is `Unconfirmed`, `session work` exits non-zero with the rerun message, and no active issue is recorded. With the remote back, a rerun records the active issue, and the agent ref holds exactly one `LockClaimed` for the issue. (REQ-2, REQ-3, REQ-4) +- [ ] AC-3: an error is injected into the reduce after the push. The result is `Unconfirmed`, not a plain error. (REQ-2) +- [ ] AC-4: a test sets the confirmation deadline to 2 s, and a fake git holds the fetch open. The result is `Unconfirmed` within 4 s. No process from the fetch's process group remains, and a normal fetch afterwards succeeds, so no lock files were left. On Windows, only the direct child is checked. (REQ-2) +- [ ] AC-4a: the fetch is fast, and a test hook makes the reduce take longer than the deadline. The result is `Claimed` (and `AlreadyHeld` on a rerun), not `Unconfirmed`. (REQ-2, REQ-3) +- [ ] AC-5: the local view shows this agent as holder, and the remote is unreachable. `session work` returns `Unconfirmed`, not `AlreadyHeld`. (REQ-3) +- [ ] AC-6: another agent's earlier-ordered claim arrives during confirmation. The result is `Contended`, and no active issue is recorded. (REQ-3) +- [ ] AC-7 (guard, passes on base): `set_session_issue` fails after a confirmed claim. A `LockReleased` follows, and the hub shows no lock. (REQ-5) +- [ ] AC-8 (fails first for activation): this is a fake-runtime test for each failure point: activation, missing preflight, tmux start, container start, and early container exit. Afterwards the issue is unlocked, the worktree directory and branch are gone, the worktree daemon is not running, no container or tmux session remains, and the pipeline state equals its prior value. This holds for both `run` and `plan`. (REQ-6) +- [ ] AC-9: in a reuse case (`--branch` with an existing worktree), activation fails. The lock is released, but the worktree and branch remain. (REQ-6) +- [ ] AC-10: a launch succeeds. Afterwards the lock is held by the worktree agent, and the worktree, branch and daemon remain. (REQ-7) +- [ ] AC-11: `kickoff cleanup --force` runs on a stopped agent whose daemon is not running. Afterwards the issue is unlocked, the session ended, the daemon stopped, and the worktree removed. A branch with no commits beyond `base_commit` is deleted. A branch with a commit, any branch under `--keep-branch`, and any branch whose metadata has no `base_commit` remain. (REQ-8, REQ-10) +- [ ] AC-12: the lock release does not reach the remote (the remote is rejecting pushes), and confirmation still shows the worktree agent holding the lock. The output names the stranded lock, and the remaining steps run. With the remote unreachable, the output says "release unverified". (REQ-9) +- [ ] AC-13: the agent identity and trust approval remain after undo and after cleanup. (REQ-11) + +## Current architecture + +All references are to 3440c0d05. + +**Claim.** +- `claim_lock_v2_inner` (`crosslink/src/shared_writer/locks.rs`) times `emit_compact_push_inner` (`crosslink/src/shared_writer/core.rs`). That call covers `commit_v3` (append, push, fetch-and-adopt, reduce, checkpoint) and then hydration. The function bails when more than 30 s have passed, after the event is already on the remote. +- `confirm_v3_locks` (`core.rs:682-687`) logs a failed fetch and reduces the local view. The v3 `SyncManager::fetch` (`crosslink/src/sync/cache.rs:500-505`) always returns `Ok`, and `fetch_and_adopt_v3_refs` (`cache.rs:548-556`) returns silently when git fails. So confirmation never fails. +- `AlreadyHeld` is decided from the local view, with no fetch. +- The bail came from commit 4728766c. It cites §8 of the event-sourced coordination design: "if compaction hasn't run within 30s, agent compacts itself". That was a wait-then-act rule, implemented as a bail. +- GitHub PR Corvidae-Coding-Projects/crosslink#103 raised the bound to 120 s. It was closed unmerged because it worked around write cost instead of fixing the semantics. + +**session work.** `crosslink/src/commands/session.rs` runs `enforce_lock`, `try_claim_lock`, `set_session_issue`, then writes the sentinel. +- A claim error returns early. +- A stale lock held by another agent lets `enforce_lock` proceed, but the reducer then ignores the new claim (`crosslink/src/compaction.rs:1084-1087`), so the result is `Contended`. +- A stranded lock therefore stays until its holder releases it. + +**Kickoff.** +- `run.rs` and `plan.rs` call `create_worktree` (`crosslink/src/commands/kickoff/launch.rs`) from `HEAD`, with `base_branch: None`. `run` reuses an existing worktree when `--branch` names one. `create_worktree` may delete and recreate a merged branch. +- `init_worktree_agent` (`launch.rs`) runs these steps: + - `crosslink init`; + - `daemon::ensure`; + - agent creation, with the key stored on the host; + - a host-signed trust approval committed to the hub; + - `sync`, `session start` and `session work`, as subprocesses. +- Since b6dbf785 the daemon is stopped on a bootstrap failure. Nothing else is undone. +- `run` calls `mark_running`; `plan` calls `mark_planning` before activation. +- After tmux `send-keys` succeeds, or `launch_container` returns, no later step can fail: they are `let _ =` or warnings only. +- `KickoffMetadata` (`types.rs:157-171`) records no base. + +**Cleanup.** `cleanup.rs` removes tmux, the container, pipeline state, the daemon (stop only) and the worktree. It touches no locks, sessions or branches. + +**Release.** `locks release` takes the mutation permit, which requires a ready worktree daemon. It exits 0 and prints "was not locked" whenever the local view does not show this agent as holder (`crosslink/src/commands/locks_cmd.rs:170-174`). + +**Callers.** Swarm and sentinel call `kickoff::run` and treat an error as a failed launch. + +## Proposed design + +1. **The claim.** + - `LockClaimResult` becomes `Claimed`, `AlreadyHeld`, `Contended { winner_agent_id }` and `Unconfirmed { cause }`. + - `commit_v3` (`crosslink/src/shared_writer/core.rs`) returns a typed outcome distinguishing "not published" (an error before or during the push, where the ref is rolled back as today) from "published", which carries any later error from adopt, reduce or checkpoint. `emit_compact_push_inner` keeps that distinction through hydration. `claim_lock_v2_inner` maps "published with an error" to `Unconfirmed`. Other callers of `emit_compact_push_inner` keep their current behaviour, by converting "published with an error" back to an error. + - A new `confirm_v3_locks_bounded(deadline)` works in three steps: + 1. it takes the hub lock with the remaining time, through `acquire_hub_lock_with_timeout` (`crosslink/src/sync/cache.rs`), made `pub(crate)` with the duration taken out of its error text; + 2. it runs the v3 fetch against the deadline and propagates every error; + 3. it reduces the fetched snapshot without a deadline. + + The `AlreadyHeld` path calls the same confirmation. + - The fetch runs through `command_output_with_timeout` (`crosslink/src/commands/kickoff/helpers.rs`), promoted to a shared module. It is extended for Unix: it spawns the child in its own process group (as `detach_daemon_command` in `crosslink/src/daemon.rs` does), and on the deadline it sends SIGTERM to the group, so git removes its lock files, then waits 500 ms and sends SIGKILL. On Windows it kills the direct child, as it does today. No third timeout helper is written. +2. **`session work`.** `try_claim_lock` (`crosslink/src/lock_check.rs`) returns an error carrying the rerun message for `Unconfirmed`. Every exhaustive match on `LockClaimResult` is updated. +3. **Kickoff undo.** Add `KickoffRollback` in `crosslink/src/commands/kickoff/`. + - It records created resources as they are created, with a flag saying whether the worktree and branch were created or reused. + - `run` and `plan` wrap everything after `create_worktree` in one `match`. They call `unwind(&err)` on error and `disarm()` at the boundary in REQ-7. + - Each undo step returns a report entry. + - The lock and session steps run before the daemon stop, while the daemon `init_worktree_agent` ensured is still up. +4. **Cleanup.** It reuses the lock, session and branch steps. Before them it calls `daemon::ensure` on the worktree. If the worktree cannot become ready, those steps are reported as skipped, with `crosslink locks release ` (run from the worktree) as the remedy. +5. **Metadata.** `create_worktree` returns the base commit, and the caller writes it to `base_commit` in `KickoffMetadata` (`#[serde(default)]`). + +The build starts from a scaffold commit containing the new `LockClaimResult` variant, the bounded-confirmation and `KickoffRollback` signatures with stubbed bodies, the metadata field, and the failing tests for the acceptance criteria. + +## Decisions + +- **An unconfirmed claim is not ownership.** hub-v3 REQ-6(b) forbids a state in which two agents both verify ownership. Treating `Unconfirmed` as held, or confirming against a stale local view, could create one. Rejected: "held with a warning", which the first draft of this design proposed; it was withdrawn after the precedent check. +- **Undo is an explicit `unwind`, not `Drop`.** It runs subprocesses, can fail and must report. `Drop` can do none of that visibly, and it would also run during a panic. +- **Release under the worktree agent's identity.** Running the release inside the worktree keeps the signed log truthful. Rejected: signing from the host with the worktree agent's key. +- **The deadline bounds only the network.** The hub-lock wait and the fetch are bounded; the local reduce is not. A slow reduce of a fresh snapshot is still a correct confirmation, and bounding it would make claims fail on slow machines until the write-cost increment lands. Rejected: bounding the whole confirmation, which round 2 of the review showed would turn today's slow success into a permanent failure. +- **The 30 s value stays; what it measures changes.** Raising it, as Corvidae-Coding-Projects/crosslink#103 did, would treat write cost, and that is the write-cost increment's job. +- **Identity and trust approval are left in place.** Revoking them would make the agent's published events unverifiable. + +## Data and compatibility + +- **One additive field.** `base_commit` in kickoff metadata, optional, with a default. Metadata without it means "keep the branch". +- `LockClaimResult` and rollback state are internal, so no other persisted format changes. +- Older binaries on the same hub are unaffected. + +## Failure handling + +- **Confirmation does not complete.** The claim stays published, the command exits non-zero, and a rerun confirms it. If nobody reruns, kickoff undo releases it: the release comes from the holder, so the reducer accepts it. Otherwise the lock stays until its holder releases it. A stale timeout does not clear it on v3. Clearing another agent's lock arrives with the lock-clearing increment (#818). +- **An undo or cleanup step fails.** The failure is reported with its remedy, and the remaining steps run. +- **The worktree cannot become ready during cleanup.** The lock and session steps are reported as skipped, with the remedy. + +## Security considerations + +- Releases are signed by the agent that claimed, so no new authority is added. +- Branch deletion requires a recorded base and an equal tip, so no work is lost. +- Leaving trust approvals for dead kickoff agents keeps existing exposure as it is. Removing them is out of scope. + +## Verification + +- **Tests:** + - `cargo test --manifest-path crosslink/Cargo.toml --lib shared_writer::tests`, `lock_check::tests` and `commands::kickoff::tests`; + - `cargo test --manifest-path crosslink/Cargo.toml --test smoke coordination`. +- **Before every commit:** `cargo fmt --check` and `cargo clippy -- -D warnings -W clippy::unwrap_used -W clippy::expect_used`. +- **Full suite:** CI only. +- **Fake runtimes and fake git:** the kickoff tests reuse those from Corvidae-Coding-Projects/crosslink#129. +- **Status wording:** "CI passed" for the gates, "reviewed" for an independent review. Neither stands in for the other. +- **Manual check on this Mac:** run `crosslink session work ` with `/usr/bin/git` on `PATH`. It succeeds, or it prints the rerun message and a rerun succeeds. A run that never succeeds on this machine is a failure of this increment. Record the timings on #817. + +## Rollout and rollback + +- This increment merges when its tests and review pass. +- Reverting it restores the elapsed-time bail. +- `base_commit` values already written are ignored by older binaries. + +## Open questions + +None. + +## Out of scope + +- Write cost, heartbeats, the knowledge cache, and lock clearing on v3 hubs. These are later increments of the umbrella. +- Removing dead kickoff agents' identities, key files and trust approvals: tracker #819. +- Swarm- and sentinel-specific tests. Both call `run` and inherit its undo. + +## Key + +- Tracker (`crosslink issue show N`): + - #817 "Design: write-path cost and lock-confirmation semantics (upstream #118, #110, #120)", open. It holds the 2026-10-09 claim timings. + - #818 "v3 hubs cannot steal or force-release another agent's lock", open. + - #819 "Dead kickoff agents leave identities, host key files and trust approvals behind", open. +- Corvidae-Coding-Projects/crosslink#110 "kickoff: a failed launch keeps the issue lock and worktree, and kickoff cleanup leaves the lock and branch behind", open. +- Corvidae-Coding-Projects/crosslink#118 "Each write spawns hundreds of git processes (~615 on bootstrap, ~350 per write), which trips fixed timeouts where git spawns are slow", open. +- Corvidae-Coding-Projects/crosslink#103, the 120 s timeout increase, closed unmerged. +- Corvidae-Coding-Projects/crosslink#129 "Build-matched agent image, launch-time version check, and one gated, verifiable release flow", merged. diff --git a/.design/write-path-cost.md b/.design/write-path-cost.md new file mode 100644 index 000000000..e639c7747 --- /dev/null +++ b/.design/write-path-cost.md @@ -0,0 +1,121 @@ +# Feature: Write-path cost and lock semantics (umbrella) + +Umbrella design document, written 2026-10-08 against `origin/develop` at 3440c0d05. Status: reviewed. It was revised after review rounds 1 and 2, and is to be frozen when the first increment's scaffold commit lands. It states the shared problem, the evidence, the increments named by feature and their order, and the decisions that cross increments. Each increment gets its own design document, written when that increment is next to build. Only the first exists: `.design/lock-claim-outcome-and-kickoff-rollback.md`. The entries for later increments are provisional. This document follows `.design/crosslink-architecture-overhead-map.md` and amends `.design/hub-v3-per-agent-refs.md` where the increment documents say so. A bare `#N` is a tracker item, and GitHub records are written in full. Handles are listed in the key at the end. + +## Problem + +- **Writes are slow.** One hub write, such as `crosslink session work `, spawns hundreds of git processes. On macOS, `/usr/bin/git` is a shim that is slow to start, so a write takes tens of seconds. +- **Claims are misreported.** A 30 s guard reports a lock claim as failed after the claim has already been published. The lock is stranded, and the caller records nothing. +- **Kickoff leaves debris.** It does not undo what it created when it fails. `kickoff cleanup` never releases locks. +- **Stranded locks cannot be cleared.** On v3 hubs nobody but the holder can clear a lock: steal and force-release do nothing. +- **Heartbeats ride the slow path.** While a slow write runs, heartbeats wait behind it, so locks read stale. +- **The knowledge cache has no owner.** Since reads became side-effect-free, nothing creates or refreshes it. + +## Evidence + +Each figure is marked as measured, estimated or a target. + +- **Measured**, 2026-09-29, on macOS on develop, with a `PATH` shim counting spawns (Corvidae-Coding-Projects/crosslink#118): + - about 615 git spawns to bring a fresh repository to ready; + - about 350 git spawns per write on a four-agent hub; + - `/usr/bin/git` at about 35 ms per spawn, against about 9 ms for the Command Line Tools git; + - writes took 16–30 s. +- **Measured**, 2026-10-08, in a downstream project (comment on Corvidae-Coding-Projects/crosslink#118): lock claims took 58 s, 72 s and 86 s. +- **Measured**, 2026-10-09 UTC, in this checkout: two claims timed out at 51 s and 52 s, and both locks landed anyway (tracker #817). +- **Estimated** by reading the code, not measured. The write-cost increment's spawn-count test replaces these with a baseline. + - About 40 spawns per readiness validation. + - Four to five validations per write. + - Three to four full reduces per claim. + - The `cat-file -t` and `ls-remote` counts in the Corvidae-Coding-Projects/crosslink#118 measurement come from daemon reconciliation running alongside the write. It validates the same immutable objects three to four times per pass. +- **Found** by reading the code, 2026-10-08: + - The reducer ignores claim and release events from anyone but the holder. + - Steal and force-release edit local files that a v3 reduce never reads. + - The smoke test for steal passes whether or not the steal worked (tracker #818). + +## Increments, in build order + +1. **Lock-claim outcome and kickoff rollback** (Corvidae-Coding-Projects/crosslink#110, and the timeout half of Corvidae-Coding-Projects/crosslink#118). The design document is written: `.design/lock-claim-outcome-and-kickoff-rollback.md`. +2. **Knowledge cache ownership** (Corvidae-Coding-Projects/crosslink#120, tracker #807). Provisional. + - The main checkout's daemon creates the cache after its first ready activation and refreshes it on its sync tick when the remote tip moves. Creation is atomic. + - A failure is logged and never affects readiness or the reconcile budget. + - The hook and the knowledge MCP server name `crosslink knowledge sync` when the cache is missing. + - Constraints found: + - The step must run only in the ready normal loop, per the readiness contract's state gating (tracker #794). + - Running it outside the operation permit departs from Corvidae-Coding-Projects/crosslink#120's suggestion, and the increment document must say why. + - The overhead map asks that the cache never be made authoritative. + - It is second because it is small and independent. Downstream projects rely on it: pages attached to an issue reach agents at session start only if the cache exists. +3. **Write cost** (Corvidae-Coding-Projects/crosslink#118). Provisional. + - A spawn-count test lands first and records the baseline. + - Then readiness validated once per command, with `SyncManager` setup and `remote_exists` computed once. + - At most two reduces per write. + - One `for-each-ref` per reduce, and a shared history cache. + - Batched `cat-file --batch-check` validation, with a memo per reconciler. + - One `ls-remote` per publication step. + - The git binary resolved once (see the git-binary decision below). + - Budget: no more than a quarter of the baseline and no more than 100 spawns per `session work`. + - Constraints found: + - The readiness contract forbids "in-memory-only readiness" and requires every write to reject a non-ready repository. So a cached validation must still re-check record age and daemon liveness on every acquisition. + - Every new cache needs a property test against full recomputation, proved by removing its invalidation and watching the test fail. +4. **Heartbeats off the write path.** Provisional. + - A heartbeat writes only the agent's own ref, without a reduce or full validation. + - It still runs only while readiness grants writes; the readiness contract lists heartbeats among the operations that require ready. + - The daemon publishes heartbeats even while a CLI write holds the permit. Today the deferral skips the heartbeat counter. + - Constraints found: + - The hook heartbeat (`heartbeat.py`, upstream #275) and the daemon heartbeat share the agent ref and the hub write lock. + - On v3 a stale lock is never cleared by time. Staleness only lets the next claim proceed, and the reducer then ignores that claim, so a stranded lock stays until its holder releases it. Aging heartbeat-less locks from `claimed_at` changes only when a lock is reported stale, so decide it together with lock clearing. +5. **Lock clearing on v3 hubs** (tracker #818). Provisional. + - Steal and force-release produce an event the reducer applies deterministically and idempotently, naming the previous holder and that holder's claim. + - The steal is recorded for audit. + - Increment 1 does not depend on this increment. Until it lands, a lock that increment 1's undo fails to release stays until its holder releases it. + - Constraints found: + - The holder-only release rule is deliberate (event-sourced design §4.2 and §6.2). + - Steal worked on v2 only by editing cache files, and stopped working when v3 reads moved to refs (upstream #754). + - The hub-v3 design's motivation lists a "live-holder lock force-steal" defect, so the emitter must re-check the holder's heartbeat immediately before emitting, and the tests need a live-holder negative case. + - The reader-version mechanism must reuse an existing gate (see the steal decision below). + - A binary that skips an unknown event must never publish a checkpoint covering it. + - It is last because it changes the hub protocol and needs the upstream owners' agreement. + +Each increment is dispatched as one issue. The number of merges each takes is decided in its own document. + +## Cross-increment decisions + +- **Steal wire format** (2026-10-08, @magnificentlycursed). The direction was a tolerant decoder that skips unknown event types with a warning, plus a minimum-reader-version gate before any new lock-clearing event is emitted. Rejected: an optional field on `LockReleased`, which old binaries would reduce differently, and a staged rollout without a gate. The precedent check the same day found that a gate already exists: + - the generation descriptor's `protocol_version` on `crosslink/reconciliation/current`, checked by exact equality, which every readiness-era binary refuses to become ready on; + - the checkpoint schema's fail-closed watermark guard (#798); + - the design history's stated rule that "mixed-version operation is refused". + + The lock-clearing increment's document must use or extend that gate rather than add a new meta-ref field. If that changes the decision's substance, the document must take it back to the operator. This is a hub protocol change: the upstream owners must agree before it merges. +- **Git binary** (2026-10-08, @magnificentlycursed). `CROSSLINK_GIT` from the environment overrides the git binary. Otherwise, on macOS, when `PATH` gives `/usr/bin/git`, crosslink runs `xcrun --find git` once per process and falls back to `/usr/bin/git`. It never reads a repository's config, so a cloned repository cannot redirect it. Rejected: override only, which leaves every Mac slow by default; and plain `PATH`. No library (git2, gix) is added, consistent with the readiness work's no-new-dependency boundary. +- **Each increment builds from a scaffold commit** of types, stubs and failing tests. Bug fixes are failing-first: the regression test fails on the base revision and passes after the fix. +- **Work-in-progress limit.** While this umbrella's increments wait to be built, no other design cycle opens unless it blocks them. The hook design (Corvidae-Coding-Projects/crosslink#104, tracker #814, with Corvidae-Coding-Projects/crosslink#116 and tracker #816) waits until the first increment is built. + +## Out of scope + +- The work-check hook and commit gate, agent command policy, and the post-wake readiness block: these are the hook design. +- Integration drift in `init --update`: Corvidae-Coding-Projects/crosslink#105, #13, #15, #20 and #99. +- Replacing git subprocesses with a library. +- The global checkpoint watermark (tracker #798). +- Removing dead kickoff agents' identities, key files and trust approvals (tracker #819). +- Clearing the existing stale `dashboard-bootstrap` locks. That is the upstream maintainer's driver identity, so it is an operator action to coordinate with them once lock clearing lands. +- Knowledge features beyond creating and refreshing the cache, such as page status, supersede, a generated index and export. + +## Key + +Tracker records (`crosslink issue show N`), with title and state as of 2026-10-08: +- #794: the readiness contract (its plan comment holds the contract). +- #798 "Replace global checkpoint watermark with per-agent causal frontiers", open. +- #807 "Knowledge cache is never created or refreshed since the readiness model", open. +- #814 "Commit gate and work-check hook misfires (upstream #104 item 2 and related)", open. +- #816 "Readiness blocks all work for ~30 s plus a reconcile after every wake", open. +- #817 "Design: write-path cost and lock-confirmation semantics (upstream #118, #110, #120)", open. +- #818 "v3 hubs cannot steal or force-release another agent's lock", open. +- #819 "Dead kickoff agents leave identities, host key files and trust approvals behind", open. + +GitHub records, with state as of 2026-10-08: +- Corvidae-Coding-Projects/crosslink#104 "Provider hooks: deployed hooks can predate the readiness model unnoticed; work-check blocks on a 3 s timeout and mislabels out-of-root paths", open. +- Corvidae-Coding-Projects/crosslink#110 "kickoff: a failed launch keeps the issue lock and worktree, and kickoff cleanup leaves the lock and branch behind", open. +- Corvidae-Coding-Projects/crosslink#116 "Container agents with a GitHub login can push, open PRs and read the token; the agent hook policy prices colleague trust, not agent trust", open. +- Corvidae-Coding-Projects/crosslink#118 "Each write spawns hundreds of git processes (~615 on bootstrap, ~350 per write), which trips fixed timeouts where git spawns are slow", open. +- Corvidae-Coding-Projects/crosslink#120 "Knowledge cache is never created in a fresh checkout or refreshed since the readiness model; reads fail and the session hook and MCP server go silent", open. +- Corvidae-Coding-Projects/crosslink#105, #13, #15, #20 and #99: the `init --update` integration-drift issues, open. +- Upstream #275, #754 and the event-sourced coordination design are cited from git history. That design was last present at the parent of commit 8e054a3e, as `DESIGN-EVENT-SOURCED-COORDINATION.md`. From 2cac336d9e9ba51203a330d42394175328fcb99f Mon Sep 17 00:00:00 2001 From: magnificentlycursed <190320250+magnificentlycursed@users.noreply.github.com> Date: Thu, 8 Oct 2026 20:56:21 -0700 Subject: [PATCH 02/14] test(locks,kickoff): scaffold the lock-claim outcome and kickoff rollback increment Adds the types and stubs the increment builds on, and the failing tests that define it (design: .design/lock-claim-outcome-and-kickoff-rollback.md). - LockClaimResult::Unconfirmed: a published claim whose confirmation did not complete; session work refuses it with a rerun message, and the other callers report it. - The Published outcome type and confirm_v3_locks_bounded (stub). - ClaimFailpoints on SharedWriter (test builds only) to inject slow publication, failed or hung confirmation fetches, failures after the push, and a slow reduce. - KickoffRollback, UndoStep, UndoOps and branch_deletable (stubs), and an optional base_commit in kickoff metadata. Red until the implementation lands: five claim tests against real v3 hubs (slow publication, failed confirmation and rerun, failure after push, bounded confirmation fetch, already-held confirmation) and eight rollback unit tests. Guards that pass: the slow-reduce claim, the metadata round trip, and the existing claim and release tests. Session-level and kickoff integration criteria come with the implementation. AC-4 in the design is amended: its bound is checked against the hung fetch, not 4 s, because publication and the pre-confirmation read are outside the deadline. --- ...lock-claim-outcome-and-kickoff-rollback.md | 2 +- crosslink/src/commands/create.rs | 7 + .../src/commands/hub_v3_operation_tests.rs | 222 +++++++++++ crosslink/src/commands/kickoff/mod.rs | 1 + crosslink/src/commands/kickoff/plan.rs | 1 + crosslink/src/commands/kickoff/rollback.rs | 356 ++++++++++++++++++ crosslink/src/commands/kickoff/types.rs | 7 + crosslink/src/commands/locks_cmd.rs | 10 + crosslink/src/commands/session.rs | 6 + crosslink/src/lock_check.rs | 12 +- crosslink/src/shared_writer/core.rs | 64 +++- crosslink/src/shared_writer/locks.rs | 12 +- 12 files changed, 696 insertions(+), 4 deletions(-) create mode 100644 crosslink/src/commands/kickoff/rollback.rs diff --git a/.design/lock-claim-outcome-and-kickoff-rollback.md b/.design/lock-claim-outcome-and-kickoff-rollback.md index ec22a08f9..2aea709bc 100644 --- a/.design/lock-claim-outcome-and-kickoff-rollback.md +++ b/.design/lock-claim-outcome-and-kickoff-rollback.md @@ -74,7 +74,7 @@ Tests marked "fails first" must fail on 3440c0d05 and pass after the change. The - [ ] AC-1 (fails first): publication is delayed past 30 s by a test hook, and confirmation succeeds. `session work` succeeds, records the active issue, and the hub shows the lock held by the agent. (REQ-1, REQ-4) - [ ] AC-2 (fails first): the remote becomes unreachable after the claim's push. The result is `Unconfirmed`, `session work` exits non-zero with the rerun message, and no active issue is recorded. With the remote back, a rerun records the active issue, and the agent ref holds exactly one `LockClaimed` for the issue. (REQ-2, REQ-3, REQ-4) - [ ] AC-3: an error is injected into the reduce after the push. The result is `Unconfirmed`, not a plain error. (REQ-2) -- [ ] AC-4: a test sets the confirmation deadline to 2 s, and a fake git holds the fetch open. The result is `Unconfirmed` within 4 s. No process from the fetch's process group remains, and a normal fetch afterwards succeeds, so no lock files were left. On Windows, only the direct child is checked. (REQ-2) +- [ ] AC-4: a test sets the confirmation deadline to 2 s, and a fake git holds the fetch open for 30 s. On the already-held path, the result is `Unconfirmed` in well under 30 s (the test asserts under 20 s). Amended 2026-10-08 during the scaffold: "within 4 s" could not hold, because the claim's publication and the local read before confirmation are not part of the bound. No process from the fetch's process group remains, and a normal fetch afterwards succeeds, so no lock files were left. On Windows, only the direct child is checked. (REQ-2) - [ ] AC-4a: the fetch is fast, and a test hook makes the reduce take longer than the deadline. The result is `Claimed` (and `AlreadyHeld` on a rerun), not `Unconfirmed`. (REQ-2, REQ-3) - [ ] AC-5: the local view shows this agent as holder, and the remote is unreachable. `session work` returns `Unconfirmed`, not `AlreadyHeld`. (REQ-3) - [ ] AC-6: another agent's earlier-ordered claim arrives during confirmation. The result is `Contended`, and no active issue is recorded. (REQ-3) diff --git a/crosslink/src/commands/create.rs b/crosslink/src/commands/create.rs index 113141da4..afe2bf6ba 100644 --- a/crosslink/src/commands/create.rs +++ b/crosslink/src/commands/create.rs @@ -277,6 +277,13 @@ fn auto_claim_and_set_work( winner_agent_id ); } + Ok(Some(crate::shared_writer::LockClaimResult::Unconfirmed { cause })) => { + tracing::warn!( + "Lock claim on {} published but not confirmed: {}", + format_issue_id(id), + cause + ); + } Err(e) => tracing::warn!("Could not auto-claim lock: {}", e), } } diff --git a/crosslink/src/commands/hub_v3_operation_tests.rs b/crosslink/src/commands/hub_v3_operation_tests.rs index 56bbfb3e8..a5a56f1b8 100644 --- a/crosslink/src/commands/hub_v3_operation_tests.rs +++ b/crosslink/src/commands/hub_v3_operation_tests.rs @@ -1258,3 +1258,225 @@ fn v3_to_shared_promotes_sqlite_only_rows() { let state2 = crate::compaction::reduce(&source2).unwrap().state; assert_eq!(state2.issues.len(), before, "second to-shared is a no-op"); } + +// Lock-claim outcome: `.design/lock-claim-outcome-and-kickoff-rollback.md`. +// AC-6 (contention found during confirmation) is covered by +// `v3_lock_claim_confirm_winner_and_loser` above. + +fn lock_claims_on_agent_ref(cache_dir: &Path, agent_id: &str, issue_id: i64) -> usize { + let reference = agent_ref_name(agent_id).unwrap(); + let out = Command::new("git") + .current_dir(cache_dir) + .args(["show", &format!("{reference}:events.log")]) + .output() + .unwrap(); + assert!(out.status.success(), "agent ref has no events.log"); + crate::events::read_events_from_bytes(&out.stdout) + .unwrap() + .iter() + .filter(|envelope| { + matches!( + envelope.event, + crate::events::Event::LockClaimed { issue_display_id, .. } + if issue_display_id == issue_id + ) + }) + .count() +} + +fn writer_with_issue(hub: &V3Hub, title: &str) -> (SharedWriter, i64) { + let db = Database::open(&hub.crosslink_dir.join("issues.db")).unwrap(); + let writer = SharedWriter::new(&hub.crosslink_dir).unwrap().unwrap(); + let issue_id = writer + .create_issue(&db, title, None, "high", None, None) + .unwrap(); + (writer, issue_id) +} + +/// AC-1 (fails first): a slow publication does not turn a confirmed claim +/// into a failure. +#[test] +fn v3_slow_publication_still_confirms_the_claim() { + if !git_ok() { + return; + } + let hub = setup_migrated_v3_hub(); + let (writer, issue_id) = writer_with_issue(&hub, "Slow publication"); + writer.failpoints.borrow_mut().extra_publish_elapsed = std::time::Duration::from_secs(31); + + let result = writer.claim_lock_v2(issue_id, None); + assert_eq!( + result.unwrap(), + crate::shared_writer::LockClaimResult::Claimed + ); + let cold = SharedWriter::new(&hub.crosslink_dir).unwrap().unwrap(); + assert_eq!( + cold.read_lock_v2(issue_id) + .unwrap() + .map(|lock| lock.agent_id), + Some("alpha".to_string()) + ); +} + +/// AC-2 (fails first): a failed confirmation fetch is `Unconfirmed`, and the +/// rerun confirms without publishing a second claim. +#[test] +fn v3_failed_confirmation_is_unconfirmed_and_the_rerun_confirms() { + if !git_ok() { + return; + } + let hub = setup_migrated_v3_hub(); + let (writer, issue_id) = writer_with_issue(&hub, "Unreachable confirmation"); + writer.failpoints.borrow_mut().fail_confirm_fetch = true; + + let first = writer.claim_lock_v2(issue_id, None).unwrap(); + assert!( + matches!( + first, + crate::shared_writer::LockClaimResult::Unconfirmed { .. } + ), + "expected Unconfirmed, got {first:?}" + ); + + writer.failpoints.borrow_mut().fail_confirm_fetch = false; + assert_eq!( + writer.claim_lock_v2(issue_id, None).unwrap(), + crate::shared_writer::LockClaimResult::AlreadyHeld + ); + assert_eq!( + lock_claims_on_agent_ref(&hub.cache_dir, "alpha", issue_id), + 1 + ); +} + +/// AC-3 (fails first): a failure after the push lands is `Unconfirmed`, not +/// a plain error. +#[test] +fn v3_failure_after_push_is_unconfirmed() { + if !git_ok() { + return; + } + let hub = setup_migrated_v3_hub(); + let (writer, issue_id) = writer_with_issue(&hub, "Failure after push"); + writer.failpoints.borrow_mut().fail_after_push = true; + + let result = writer.claim_lock_v2(issue_id, None); + assert!( + matches!( + result, + Ok(crate::shared_writer::LockClaimResult::Unconfirmed { .. }) + ), + "expected Ok(Unconfirmed), got {result:?}" + ); +} + +/// AC-4 (fails first): the confirmation fetch is bounded and killed with its +/// process group, and leaves nothing that blocks a later fetch. The timed call +/// is the already-held path, so publication time is not measured; the bound +/// is checked against the hung fetch's 30 s, because the local read before +/// confirmation also reduces. +#[cfg(unix)] +#[test] +fn v3_confirmation_fetch_is_bounded_and_cleaned_up() { + if !git_ok() { + return; + } + let hub = setup_migrated_v3_hub(); + let (writer, issue_id) = writer_with_issue(&hub, "Hung confirmation"); + assert_eq!( + writer.claim_lock_v2(issue_id, None).unwrap(), + crate::shared_writer::LockClaimResult::Claimed + ); + let marker = format!("crosslink-confirm-deadline-{}", std::process::id()); + { + let mut failpoints = writer.failpoints.borrow_mut(); + failpoints.confirm_deadline = Some(std::time::Duration::from_secs(2)); + failpoints.confirm_fetch_command = Some(vec![ + "sh".to_string(), + "-c".to_string(), + format!("sleep 30; : {marker}"), + ]); + } + + let started = std::time::Instant::now(); + let result = writer.claim_lock_v2(issue_id, None).unwrap(); + assert!( + matches!( + result, + crate::shared_writer::LockClaimResult::Unconfirmed { .. } + ), + "expected Unconfirmed, got {result:?}" + ); + assert!( + started.elapsed() < std::time::Duration::from_secs(20), + "confirmation was not bounded: {:?}", + started.elapsed() + ); + let leftover = Command::new("pgrep") + .args(["-f", &marker]) + .output() + .unwrap(); + assert!( + !leftover.status.success(), + "fetch process group survived: {}", + String::from_utf8_lossy(&leftover.stdout) + ); + + *writer.failpoints.borrow_mut() = crate::shared_writer::core::ClaimFailpoints::default(); + SyncManager::new(&hub.crosslink_dir) + .unwrap() + .fetch() + .unwrap(); + assert_eq!( + writer.claim_lock_v2(issue_id, None).unwrap(), + crate::shared_writer::LockClaimResult::AlreadyHeld + ); +} + +/// AC-4a (guard): a reduce slower than the deadline does not make a claim +/// unconfirmed, because the deadline bounds only the network. +#[test] +fn v3_slow_reduce_after_the_fetch_still_confirms() { + if !git_ok() { + return; + } + let hub = setup_migrated_v3_hub(); + let (writer, issue_id) = writer_with_issue(&hub, "Slow reduce"); + { + let mut failpoints = writer.failpoints.borrow_mut(); + failpoints.confirm_deadline = Some(std::time::Duration::from_secs(1)); + failpoints.slow_confirm_reduce = std::time::Duration::from_secs(2); + } + assert_eq!( + writer.claim_lock_v2(issue_id, None).unwrap(), + crate::shared_writer::LockClaimResult::Claimed + ); + assert_eq!( + writer.claim_lock_v2(issue_id, None).unwrap(), + crate::shared_writer::LockClaimResult::AlreadyHeld + ); +} + +/// AC-5 (fails first): an already-held lock is confirmed against the remote, +/// not the local view. +#[test] +fn v3_already_held_is_confirmed_against_the_remote() { + if !git_ok() { + return; + } + let hub = setup_migrated_v3_hub(); + let (writer, issue_id) = writer_with_issue(&hub, "Held, remote gone"); + assert_eq!( + writer.claim_lock_v2(issue_id, None).unwrap(), + crate::shared_writer::LockClaimResult::Claimed + ); + writer.failpoints.borrow_mut().fail_confirm_fetch = true; + let result = writer.claim_lock_v2(issue_id, None).unwrap(); + assert!( + matches!( + result, + crate::shared_writer::LockClaimResult::Unconfirmed { .. } + ), + "expected Unconfirmed, got {result:?}" + ); +} diff --git a/crosslink/src/commands/kickoff/mod.rs b/crosslink/src/commands/kickoff/mod.rs index ea076a3df..917c0467f 100644 --- a/crosslink/src/commands/kickoff/mod.rs +++ b/crosslink/src/commands/kickoff/mod.rs @@ -6,6 +6,7 @@ mod monitor; pub(crate) mod pipeline; mod plan; mod prompt; +mod rollback; mod run; mod types; mod wizard; diff --git a/crosslink/src/commands/kickoff/plan.rs b/crosslink/src/commands/kickoff/plan.rs index 8bf81a52d..6b30da24a 100644 --- a/crosslink/src/commands/kickoff/plan.rs +++ b/crosslink/src/commands/kickoff/plan.rs @@ -225,6 +225,7 @@ pub fn plan(crosslink_dir: &Path, db: &impl QueryService, opts: &PlanOpts) -> Re model: validation_agent.resolve_model(Some(opts.model)), effort: opts.policy.effort.clone(), budget_usd: opts.policy.monetary_budget_usd.clone(), + base_commit: None, }; std::fs::write( worktree_dir.join(".kickoff-metadata.json"), diff --git a/crosslink/src/commands/kickoff/rollback.rs b/crosslink/src/commands/kickoff/rollback.rs new file mode 100644 index 000000000..41ed1b01d --- /dev/null +++ b/crosslink/src/commands/kickoff/rollback.rs @@ -0,0 +1,356 @@ +//! Undo for a kickoff that fails before its agent starts, and the lock, +//! session and branch steps that `kickoff cleanup` shares with it. +//! +//! Design: `.design/lock-claim-outcome-and-kickoff-rollback.md`. +// Scaffold: kickoff run, plan and cleanup call this in the implementation commit. +#![allow(dead_code)] + +use std::path::PathBuf; + +use super::pipeline::PipelineState; + +/// Whether this kickoff invocation created a resource or found it in place. +/// Only created resources are removed. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(super) enum Origin { + Created, + Reused, +} + +/// One undo action, in the order `KickoffRollback::steps` returns them. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(super) enum UndoStep { + RemoveContainer { + runtime: String, + name: String, + }, + KillTmuxSession { + name: String, + }, + /// Run inside the worktree, so the worktree agent signs the release. + ReleaseLock { + issue_id: i64, + }, + EndSession, + StopDaemon, + RestorePipeline { + doc_path: PathBuf, + }, + RemoveWorktree { + path: PathBuf, + }, + DeleteBranch { + name: String, + base_commit: String, + }, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(super) enum UndoOutcome { + Done, + NothingToUndo, + Failed { + error: String, + remedy: String, + }, + /// The release ran, but confirmation shows the worktree agent still + /// holding the lock. + Stranded { + remedy: String, + }, + /// The release ran, but confirmation could not complete. + Unverified { + remedy: String, + }, +} + +#[derive(Debug, Default)] +pub(super) struct UndoReport { + pub(super) entries: Vec<(UndoStep, UndoOutcome)>, +} + +impl UndoReport { + /// True when every step either completed or had nothing to undo. + pub(super) fn is_clean(&self) -> bool { + unimplemented!("scaffold: undo report classification") + } +} + +/// Performs one undo step. The kickoff implementation runs subprocesses; +/// tests record the calls. +pub(super) trait UndoOps { + fn run(&mut self, step: &UndoStep) -> UndoOutcome; +} + +/// What a kickoff invocation has created so far. Record each resource as it +/// is created; call `disarm` once the agent has started. +#[derive(Debug, Default)] +pub(super) struct KickoffRollback { + worktree: Option<(PathBuf, Origin)>, + branch: Option<(String, Origin, Option)>, + agent_initialized: bool, + claimed_issue: Option, + pipeline: Option<(PathBuf, Option)>, + tmux_session: Option, + container: Option<(String, String)>, + disarmed: bool, +} + +impl KickoffRollback { + pub(super) fn new() -> Self { + Self::default() + } + + pub(super) fn record_worktree(&mut self, path: PathBuf, origin: Origin) { + self.worktree = Some((path, origin)); + } + + pub(super) fn record_branch(&mut self, name: String, origin: Origin, base: Option) { + self.branch = Some((name, origin, base)); + } + + /// The worktree has an agent, a daemon and a session. + pub(super) const fn record_agent(&mut self) { + self.agent_initialized = true; + } + + /// Record before invoking `session work`: a claim that fails to confirm + /// may still have been published. Releasing a lock this agent does not + /// hold is ignored by the reducer. + pub(super) const fn record_claim(&mut self, issue_id: i64) { + self.claimed_issue = Some(issue_id); + } + + /// Record the pipeline state as it was before `mark_running` or + /// `mark_planning` changed it. + pub(super) fn record_pipeline(&mut self, doc_path: PathBuf, prior: Option) { + self.pipeline = Some((doc_path, prior)); + } + + pub(super) fn record_tmux_session(&mut self, name: String) { + self.tmux_session = Some(name); + } + + pub(super) fn record_container(&mut self, runtime: String, name: String) { + self.container = Some((runtime, name)); + } + + /// The agent has started: from here on, its resources are its own. + pub(super) const fn disarm(&mut self) { + self.disarmed = true; + } + + /// The undo steps for what was recorded, in the order they must run. + pub(super) fn steps(&self) -> Vec { + unimplemented!("scaffold: undo step plan") + } + + /// Runs every step, continuing past failures, and reports each outcome. + #[allow(clippy::needless_pass_by_ref_mut)] // scaffold: the implementation calls `ops.run` + pub(super) fn unwind(self, ops: &mut impl UndoOps) -> UndoReport { + let _ = ops; + unimplemented!("scaffold: undo execution") + } +} + +/// A branch may be deleted only when a base was recorded and the branch has +/// no commits beyond it. +pub(super) fn branch_deletable(tip: &str, base_commit: Option<&str>) -> bool { + let _ = (tip, base_commit); + unimplemented!("scaffold: branch deletion rule") +} + +#[cfg(test)] +mod tests { + use super::*; + + const BASE: &str = "1111111111111111111111111111111111111111"; + + fn created_worktree_with_agent_and_claim() -> KickoffRollback { + let mut rollback = KickoffRollback::new(); + rollback.record_worktree(PathBuf::from("/wt/feature-x"), Origin::Created); + rollback.record_branch( + "feature/feature-x".to_string(), + Origin::Created, + Some(BASE.to_string()), + ); + rollback.record_agent(); + rollback.record_claim(7); + rollback + } + + fn tail_steps() -> Vec { + vec![ + UndoStep::ReleaseLock { issue_id: 7 }, + UndoStep::EndSession, + UndoStep::StopDaemon, + ] + } + + fn removal_steps() -> Vec { + vec![ + UndoStep::RemoveWorktree { + path: PathBuf::from("/wt/feature-x"), + }, + UndoStep::DeleteBranch { + name: "feature/feature-x".to_string(), + base_commit: BASE.to_string(), + }, + ] + } + + #[derive(Default)] + struct RecordingOps { + calls: Vec, + outcomes: Vec<(UndoStep, UndoOutcome)>, + } + + impl UndoOps for RecordingOps { + fn run(&mut self, step: &UndoStep) -> UndoOutcome { + self.calls.push(step.clone()); + self.outcomes + .iter() + .find(|(s, _)| s == step) + .map_or(UndoOutcome::Done, |(_, outcome)| outcome.clone()) + } + } + + /// AC-8: activation fails after the claim. + #[test] + fn activation_failure_undoes_in_reverse_order() { + let rollback = created_worktree_with_agent_and_claim(); + let mut expected = tail_steps(); + expected.extend(removal_steps()); + assert_eq!(rollback.steps(), expected); + } + + /// AC-8: container start or early container exit, for `run` after + /// `mark_running`. + #[test] + fn container_failure_removes_the_container_first_and_restores_the_pipeline() { + let mut rollback = created_worktree_with_agent_and_claim(); + rollback.record_pipeline(PathBuf::from("/repo/.design/x.md"), None); + rollback.record_container("docker".to_string(), "crosslink-feature-x".to_string()); + let mut expected = vec![UndoStep::RemoveContainer { + runtime: "docker".to_string(), + name: "crosslink-feature-x".to_string(), + }]; + expected.extend(tail_steps()); + expected.push(UndoStep::RestorePipeline { + doc_path: PathBuf::from("/repo/.design/x.md"), + }); + expected.extend(removal_steps()); + assert_eq!(rollback.steps(), expected); + } + + /// AC-8: tmux start fails after the session was created. + #[test] + fn tmux_failure_kills_the_session_first() { + let mut rollback = created_worktree_with_agent_and_claim(); + rollback.record_tmux_session("feat-x".to_string()); + let steps = rollback.steps(); + assert_eq!( + steps.first(), + Some(&UndoStep::KillTmuxSession { + name: "feat-x".to_string() + }) + ); + assert_eq!(steps.len(), 6); + } + + /// AC-9: a reused worktree and branch are never removed. + #[test] + fn reused_worktree_and_branch_are_kept() { + let mut rollback = KickoffRollback::new(); + rollback.record_worktree(PathBuf::from("/wt/feature-x"), Origin::Reused); + rollback.record_branch("feature/feature-x".to_string(), Origin::Reused, None); + rollback.record_agent(); + rollback.record_claim(7); + assert_eq!(rollback.steps(), tail_steps()); + } + + /// AC-11 (unit level): without a recorded base, the branch is kept. + #[test] + fn branch_without_a_recorded_base_is_kept() { + let mut rollback = created_worktree_with_agent_and_claim(); + rollback.record_branch("feature/feature-x".to_string(), Origin::Created, None); + let steps = rollback.steps(); + assert!(!steps + .iter() + .any(|step| matches!(step, UndoStep::DeleteBranch { .. }))); + } + + /// AC-10: once the agent has started, nothing is undone. + #[test] + fn disarmed_rollback_does_nothing() { + let mut rollback = created_worktree_with_agent_and_claim(); + rollback.disarm(); + assert!(rollback.steps().is_empty()); + let mut ops = RecordingOps::default(); + let report = rollback.unwind(&mut ops); + assert!(ops.calls.is_empty()); + assert!(report.entries.is_empty()); + } + + /// AC-12: a stranded lock and a failed step are reported, and every + /// remaining step still runs. + #[test] + fn failures_are_reported_and_do_not_stop_later_steps() { + let rollback = created_worktree_with_agent_and_claim(); + let expected_calls = rollback.steps(); + let mut ops = RecordingOps { + outcomes: vec![ + ( + UndoStep::ReleaseLock { issue_id: 7 }, + UndoOutcome::Stranded { + remedy: "crosslink locks release 7".to_string(), + }, + ), + ( + UndoStep::StopDaemon, + UndoOutcome::Failed { + error: "daemon did not stop".to_string(), + remedy: "crosslink daemon stop".to_string(), + }, + ), + ], + ..RecordingOps::default() + }; + let report = rollback.unwind(&mut ops); + assert_eq!(ops.calls, expected_calls); + assert_eq!(report.entries.len(), expected_calls.len()); + assert!(!report.is_clean()); + assert!(report + .entries + .iter() + .any(|(_, outcome)| matches!(outcome, UndoOutcome::Stranded { .. }))); + } + + #[test] + fn branch_is_deletable_only_at_its_recorded_base() { + assert!(branch_deletable(BASE, Some(BASE))); + assert!(!branch_deletable( + "2222222222222222222222222222222222222222", + Some(BASE) + )); + assert!(!branch_deletable(BASE, None)); + } + + /// REQ-10 (guard): metadata written before `base_commit` existed still + /// parses, and a recorded base round-trips. + #[test] + fn metadata_base_commit_is_optional_and_round_trips() { + let old = r#"{"started_at":"2026-10-01T00:00:00Z","timeout_secs":3600}"#; + let parsed: super::super::types::KickoffMetadata = + serde_json::from_str(old).expect("old metadata parses"); + assert_eq!(parsed.base_commit, None); + + let mut with_base = parsed; + with_base.base_commit = Some(BASE.to_string()); + let json = serde_json::to_string(&with_base).expect("serializes"); + assert!(json.contains(BASE)); + let back: super::super::types::KickoffMetadata = + serde_json::from_str(&json).expect("round-trips"); + assert_eq!(back.base_commit.as_deref(), Some(BASE)); + } +} diff --git a/crosslink/src/commands/kickoff/types.rs b/crosslink/src/commands/kickoff/types.rs index 21a7c728d..036888b4e 100644 --- a/crosslink/src/commands/kickoff/types.rs +++ b/crosslink/src/commands/kickoff/types.rs @@ -170,6 +170,12 @@ pub struct KickoffMetadata { #[serde(default, skip_serializing_if = "Option::is_none")] pub budget_usd: Option, + + /// The commit the worktree's branch was created from. Absent when the + /// branch was reused, or in metadata written before it was recorded; + /// cleanup then keeps the branch. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub base_commit: Option, } impl KickoffMetadata { @@ -181,6 +187,7 @@ impl KickoffMetadata { model: Some(opts.model.to_string()), effort: opts.policy.effort.clone(), budget_usd: opts.policy.monetary_budget_usd.clone(), + base_commit: None, } } } diff --git a/crosslink/src/commands/locks_cmd.rs b/crosslink/src/commands/locks_cmd.rs index 8668867ce..efa36796b 100644 --- a/crosslink/src/commands/locks_cmd.rs +++ b/crosslink/src/commands/locks_cmd.rs @@ -153,6 +153,13 @@ pub fn claim( winner_agent_id ); } + LockClaimResult::Unconfirmed { cause } => { + anyhow::bail!( + "Lock claim on issue {} published but not confirmed: {cause}; \ + run `crosslink locks claim {issue_id}` again to confirm", + format_issue_id(issue_id) + ); + } } Ok(()) } @@ -218,6 +225,9 @@ pub fn steal(commands: &impl CommandService, crosslink_dir: &Path, issue_id: i64 LockClaimResult::Contended { winner_agent_id } => { anyhow::bail!("Lock contended — won by '{winner_agent_id}'"); } + LockClaimResult::Unconfirmed { cause } => { + anyhow::bail!("Lock claim published but not confirmed: {cause}"); + } } println!( "Claimed lock on issue {} (was not locked)", diff --git a/crosslink/src/commands/session.rs b/crosslink/src/commands/session.rs index 826e15c23..5485d377e 100644 --- a/crosslink/src/commands/session.rs +++ b/crosslink/src/commands/session.rs @@ -242,6 +242,12 @@ pub fn work( issue_id ); } + ClaimResult::Unconfirmed { cause } => { + bail!( + "claim published but not confirmed: {cause}; \ + run `crosslink session work {issue_id}` again to confirm" + ); + } }; if let Err(e) = service.set_session_issue(session.id, issue_id) { diff --git a/crosslink/src/lock_check.rs b/crosslink/src/lock_check.rs index 0defa57d5..9c46d6776 100644 --- a/crosslink/src/lock_check.rs +++ b/crosslink/src/lock_check.rs @@ -183,7 +183,14 @@ pub enum ClaimResult { AlreadyHeld, - Contended { winner_agent_id: String }, + Contended { + winner_agent_id: String, + }, + + /// Published but not confirmed; not ownership. + Unconfirmed { + cause: String, + }, NotConfigured, } @@ -200,6 +207,9 @@ pub fn try_claim_lock( crate::shared_writer::LockClaimResult::Contended { winner_agent_id } => { Ok(ClaimResult::Contended { winner_agent_id }) } + crate::shared_writer::LockClaimResult::Unconfirmed { cause } => { + Ok(ClaimResult::Unconfirmed { cause }) + } }, Err(error) if error.to_string().contains("locks require configured") => { Ok(ClaimResult::NotConfigured) diff --git a/crosslink/src/shared_writer/core.rs b/crosslink/src/shared_writer/core.rs index 8b4ea5543..835b15059 100644 --- a/crosslink/src/shared_writer/core.rs +++ b/crosslink/src/shared_writer/core.rs @@ -2,6 +2,7 @@ use anyhow::{bail, Context, Result}; use chrono::Utc; use std::cell::Cell; use std::path::{Path, PathBuf}; +use std::time::Duration; use uuid::Uuid; use crate::db::Database; @@ -28,6 +29,37 @@ pub enum PushOutcome { LocalOnly, } +/// Failure and delay injection for claim tests, in the style of the +/// reconciliation `FailureController`. Each field is consulted at one point. +#[cfg(test)] +#[allow(dead_code)] // scaffold: the deadline fields are read by the bounded confirmation +#[derive(Debug, Default)] +pub(crate) struct ClaimFailpoints { + /// Added to the measured publication time of a lock claim. + pub(crate) extra_publish_elapsed: Duration, + /// Makes the lock-confirmation fetch fail. + pub(crate) fail_confirm_fetch: bool, + /// Fails the first step after a successful push. + pub(crate) fail_after_push: bool, + /// Overrides the confirmation deadline. + pub(crate) confirm_deadline: Option, + /// Replaces the confirmation fetch command (program and arguments). + pub(crate) confirm_fetch_command: Option>, + /// Delays the reduce that follows the confirmation fetch. + pub(crate) slow_confirm_reduce: Duration, +} + +/// The result of committing events once publication was attempted. An `Err` +/// from the commit means nothing was published; this type means the events +/// reached the remote, possibly followed by a failure in a later step. +#[allow(dead_code)] // scaffold: constructed by the implementation commit +#[derive(Debug)] +pub(crate) struct Published { + pub(crate) outcome: PushOutcome, + /// An error from adopt, reduce, checkpoint or hydration after the push. + pub(crate) after_push_error: Option, +} + pub struct SharedWriter { pub(super) sync: SyncManager, pub(super) agent: AgentConfig, @@ -37,6 +69,9 @@ pub struct SharedWriter { pub(super) event_seq: Cell, pub(super) last_v3_state: std::cell::RefCell>, + + #[cfg(test)] + pub(crate) failpoints: std::cell::RefCell, } impl SharedWriter { @@ -97,6 +132,8 @@ impl SharedWriter { readiness_dir: crosslink_dir.to_path_buf(), event_seq, last_v3_state: std::cell::RefCell::new(None), + #[cfg(test)] + failpoints: std::cell::RefCell::new(ClaimFailpoints::default()), })) } @@ -571,6 +608,11 @@ impl SharedWriter { return Err(error); } + #[cfg(test)] + if self.failpoints.borrow().fail_after_push { + bail!("injected failure after push"); + } + if self.sync.remote_exists() { self.sync.fetch_and_adopt_v3_refs(); } @@ -680,12 +722,32 @@ impl SharedWriter { } pub(super) fn confirm_v3_locks(&self) -> Result<()> { - if let Err(e) = self.sync.fetch() { + #[cfg(test)] + let fetched = if self.failpoints.borrow().fail_confirm_fetch { + Err(anyhow::anyhow!("injected confirmation fetch failure")) + } else { + self.sync.fetch() + }; + #[cfg(not(test))] + let fetched = self.sync.fetch(); + if let Err(e) = fetched { tracing::warn!("v3 lock confirm: fetch failed ({e}); confirming against local view"); } + #[cfg(test)] + std::thread::sleep(self.failpoints.borrow().slow_confirm_reduce); self.refresh_v3_state() } + /// Confirms lock state against a fresh fetch. The hub-lock wait and the + /// fetch share `deadline`; the reduce of the fetched snapshot is not + /// bounded. Every failure is returned, never confirmed against the local + /// view. See `.design/lock-claim-outcome-and-kickoff-rollback.md`. + #[allow(dead_code)] // scaffold: called by the implementation commit + pub(super) fn confirm_v3_locks_bounded(&self, deadline: Duration) -> Result<()> { + let _ = deadline; + unimplemented!("scaffold: bounded lock confirmation") + } + pub(super) fn v3_assigned_display_id(&self, uuid: &Uuid) -> Option { self.last_v3_state .borrow() diff --git a/crosslink/src/shared_writer/locks.rs b/crosslink/src/shared_writer/locks.rs index d3b8a8f46..f5ef31460 100644 --- a/crosslink/src/shared_writer/locks.rs +++ b/crosslink/src/shared_writer/locks.rs @@ -8,7 +8,15 @@ pub enum LockClaimResult { AlreadyHeld, - Contended { winner_agent_id: String }, + Contended { + winner_agent_id: String, + }, + + /// The claim was published, but confirmation against a fresh fetch did not + /// complete. Not ownership: a rerun confirms it without publishing again. + Unconfirmed { + cause: String, + }, } impl SharedWriter { @@ -39,6 +47,8 @@ impl SharedWriter { let start = std::time::Instant::now(); self.emit_compact_push_inner(event, &format!("claim lock on #{issue_display_id}"))?; let elapsed = start.elapsed(); + #[cfg(test)] + let elapsed = elapsed + self.failpoints.borrow().extra_publish_elapsed; if elapsed > std::time::Duration::from_secs(LOCK_CONFIRM_TIMEOUT_SECS) { bail!( "Lock confirmation timed out after {}s (threshold {}s) -- \ From dcbe2bd928f1e3d33c6943bd0321f1f34c38037a Mon Sep 17 00:00:00 2001 From: magnificentlycursed <190320250+magnificentlycursed@users.noreply.github.com> Date: Thu, 8 Oct 2026 21:39:47 -0700 Subject: [PATCH 03/14] fix(locks,kickoff): decide claims by confirmation; undo failed kickoffs; cleanup releases locks A lock claim is no longer reported as failed because its publication took more than 30 s. The outcome comes from a confirmation against a fresh fetch: the hub-lock wait and the fetch share the 30 s bound, and the local reduce of the fetched snapshot runs to completion. A failed or timed-out confirmation, or any failure after the push lands, is Unconfirmed, which is not ownership; session work refuses it with a rerun message, and the rerun confirms without publishing a second claim. An already-held lock is confirmed against the remote, not the local view. - commit_v3 and the emit path report whether the events were published. - The confirmation fetch runs through utils::command_output_with_timeout, promoted from kickoff and extended to run in its own process group and terminate it with SIGTERM, then SIGKILL after 500 ms, on Unix. - The v3 fetch is split into a fetch command and adopt_fetched_v3_refs. Kickoff run and plan undo what they created when they fail before the agent starts: container or tmux session, the issue lock (released and verified under the worktree agent's identity), the session, the worktree daemon, the pipeline state, and the worktree and branch when this invocation created them. Kickoff metadata records the branch's base commit. kickoff cleanup brings the worktree daemon up to release the agent's locks and end its session, then removes the worktree and deletes the branch when it has no commits beyond its recorded base (--keep-branch keeps it). Steps that do not complete are reported with their remedy. Fixes Corvidae-Coding-Projects/crosslink#110 and the timeout half of Corvidae-Coding-Projects/crosslink#118. Design: .design/lock-claim-outcome-and-kickoff-rollback.md (amended during the build; the amendments and test coverage are recorded there). --- ...lock-claim-outcome-and-kickoff-rollback.md | 32 +- .../src/commands/hub_v3_operation_tests.rs | 10 +- crosslink/src/commands/kickoff/cleanup.rs | 212 ++++++++- crosslink/src/commands/kickoff/helpers.rs | 28 +- crosslink/src/commands/kickoff/mod.rs | 14 +- crosslink/src/commands/kickoff/plan.rs | 215 +++++---- crosslink/src/commands/kickoff/rollback.rs | 446 +++++++++++++++++- crosslink/src/commands/kickoff/run.rs | 302 +++++++----- crosslink/src/commands/kickoff/types.rs | 3 + crosslink/src/main.rs | 4 + crosslink/src/shared_writer/core.rs | 161 +++++-- crosslink/src/shared_writer/locks.rs | 82 +++- crosslink/src/sync/cache.rs | 50 +- crosslink/src/utils.rs | 66 ++- 14 files changed, 1295 insertions(+), 330 deletions(-) diff --git a/.design/lock-claim-outcome-and-kickoff-rollback.md b/.design/lock-claim-outcome-and-kickoff-rollback.md index 2aea709bc..def19bd5a 100644 --- a/.design/lock-claim-outcome-and-kickoff-rollback.md +++ b/.design/lock-claim-outcome-and-kickoff-rollback.md @@ -71,7 +71,7 @@ Kickoff undo: Tests marked "fails first" must fail on 3440c0d05 and pass after the change. They are the regression tests for Corvidae-Coding-Projects/crosslink#110. -- [ ] AC-1 (fails first): publication is delayed past 30 s by a test hook, and confirmation succeeds. `session work` succeeds, records the active issue, and the hub shows the lock held by the agent. (REQ-1, REQ-4) +- [ ] AC-1 (fails first): a test hook delays publication past the confirmation deadline (a 2 s delay against a 1 s deadline), and confirmation succeeds. Amended 2026-10-08 during the build, from "past 30 s": the deadline is what publication time must not count against. `session work` succeeds, records the active issue, and the hub shows the lock held by the agent. (REQ-1, REQ-4) - [ ] AC-2 (fails first): the remote becomes unreachable after the claim's push. The result is `Unconfirmed`, `session work` exits non-zero with the rerun message, and no active issue is recorded. With the remote back, a rerun records the active issue, and the agent ref holds exactly one `LockClaimed` for the issue. (REQ-2, REQ-3, REQ-4) - [ ] AC-3: an error is injected into the reduce after the push. The result is `Unconfirmed`, not a plain error. (REQ-2) - [ ] AC-4: a test sets the confirmation deadline to 2 s, and a fake git holds the fetch open for 30 s. On the already-held path, the result is `Unconfirmed` in well under 30 s (the test asserts under 20 s). Amended 2026-10-08 during the scaffold: "within 4 s" could not hold, because the claim's publication and the local read before confirmation are not part of the bound. No process from the fetch's process group remains, and a normal fetch afterwards succeeds, so no lock files were left. On Windows, only the direct child is checked. (REQ-2) @@ -153,6 +153,36 @@ The build starts from a scaffold commit containing the new `LockClaimResult` var - **The 30 s value stays; what it measures changes.** Raising it, as Corvidae-Coding-Projects/crosslink#103 did, would treat write cost, and that is the write-cost increment's job. - **Identity and trust approval are left in place.** Revoking them would make the agent's published events unverifiable. +## Amendments during the build + +Dated 2026-10-08, made while building on `feat/lock-claim-outcome`: + +- **AC-1** is relative to the confirmation deadline (a 2 s publication delay against a 1 s deadline), not "past 30 s". +- **AC-4** checks the bound against the hung fetch (under 20 s), not "within 4 s". Publication and the local read before confirmation are outside the bound. +- **Undo and cleanup release the lock in-process.** They use a `SharedWriter` built on the worktree's `.crosslink`, not a `crosslink locks release` subprocess. The signer is the same worktree agent. Running in-process lets the release be checked by `SharedWriter::confirmed_lock_holder`, the bounded confirmation, which a subprocess exit code cannot provide. +- **`session end` runs without notes.** With notes it would post a comment on the issue, an extra hub write for every failed kickoff. +- **Cleanup releases every lock the worktree agent holds** (`SharedWriter::locks_held_by_self`), because cleanup does not know the agent's issue. + +Where each criterion is covered: + +- **Claim (AC-1 to AC-6):** tests in `crosslink/src/commands/hub_v3_operation_tests.rs` against real v3 hubs with a bare remote. AC-6 is the existing `v3_lock_claim_confirm_winner_and_loser`. +- **Undo and cleanup:** unit tests in `crosslink/src/commands/kickoff/rollback.rs` and `cleanup.rs`, covering: + - the step order for each failure point; + - reuse; + - disarming; + - failure reporting; + - the branch rule; + - the pipeline restore; + - the real git operations on a temporary repository. + +Not covered by automated tests: + +- the `session work` message for an unconfirmed claim (AC-2's command-level half); +- AC-7; +- AC-8 and AC-11 end to end, which need the `crosslink` binary, tmux and a container runtime. + +These are checked by hand on this Mac before merge. + ## Data and compatibility - **One additive field.** `base_commit` in kickoff metadata, optional, with a default. Metadata without it means "keep the branch". diff --git a/crosslink/src/commands/hub_v3_operation_tests.rs b/crosslink/src/commands/hub_v3_operation_tests.rs index a5a56f1b8..d84973fd2 100644 --- a/crosslink/src/commands/hub_v3_operation_tests.rs +++ b/crosslink/src/commands/hub_v3_operation_tests.rs @@ -1293,8 +1293,8 @@ fn writer_with_issue(hub: &V3Hub, title: &str) -> (SharedWriter, i64) { (writer, issue_id) } -/// AC-1 (fails first): a slow publication does not turn a confirmed claim -/// into a failure. +/// AC-1 (fails first): publication time never counts against the +/// confirmation deadline, so a slow publication still confirms the claim. #[test] fn v3_slow_publication_still_confirms_the_claim() { if !git_ok() { @@ -1302,7 +1302,11 @@ fn v3_slow_publication_still_confirms_the_claim() { } let hub = setup_migrated_v3_hub(); let (writer, issue_id) = writer_with_issue(&hub, "Slow publication"); - writer.failpoints.borrow_mut().extra_publish_elapsed = std::time::Duration::from_secs(31); + { + let mut failpoints = writer.failpoints.borrow_mut(); + failpoints.confirm_deadline = Some(std::time::Duration::from_secs(1)); + failpoints.publish_delay = std::time::Duration::from_secs(2); + } let result = writer.claim_lock_v2(issue_id, None); assert_eq!( diff --git a/crosslink/src/commands/kickoff/cleanup.rs b/crosslink/src/commands/kickoff/cleanup.rs index 4fa42eab0..42102488e 100644 --- a/crosslink/src/commands/kickoff/cleanup.rs +++ b/crosslink/src/commands/kickoff/cleanup.rs @@ -5,15 +5,27 @@ use std::process::Command; use super::helpers::*; use super::monitor::discover_agents; +use super::rollback::{outcome_message, KickoffUndo, UndoOps, UndoOutcome, UndoStep}; use super::types::*; -pub fn cleanup( - crosslink_dir: &Path, - dry_run: bool, - force: bool, - keep: usize, - json_output: bool, -) -> Result<()> { +#[derive(Debug, Clone, Copy, Default)] +pub struct CleanupOptions { + pub dry_run: bool, + pub force: bool, + pub keep: usize, + /// Keep each agent's branch even when it has no commits beyond its base. + pub keep_branch: bool, + pub json_output: bool, +} + +pub fn cleanup(crosslink_dir: &Path, opts: &CleanupOptions) -> Result<()> { + let CleanupOptions { + dry_run, + force, + keep, + keep_branch, + json_output, + } = *opts; let agents = discover_agents(crosslink_dir)?; if !dry_run { for agent in &agents { @@ -185,6 +197,9 @@ pub fn cleanup( worktree_removed: false, tmux_killed: false, container_removed: false, + locks_released: Vec::new(), + branch_deleted: false, + warnings: Vec::new(), error: None, }; @@ -232,6 +247,19 @@ pub fn cleanup( } } + // With the agent's processes gone, release its locks and end its + // session under its own identity, before the worktree goes. + let worktree_path = Path::new(&agent.worktree); + let worktree_present = !agent.worktree.is_empty() && worktree_path.exists(); + let repo_root = crosslink_dir.parent().unwrap_or(crosslink_dir); + let mut branch_to_delete = None; + if worktree_present { + release_agent_work(repo_root, worktree_path, &mut result); + if !keep_branch { + branch_to_delete = branch_and_base(worktree_path); + } + } + if !agent.worktree.is_empty() { if let Some(root) = crosslink_dir.parent() { let pipeline_status = match agent.status.as_str() { @@ -260,6 +288,20 @@ pub fn cleanup( { Ok(o) if o.status.success() => { result.worktree_removed = true; + if let Some((name, base_commit)) = branch_to_delete.take() { + let step = UndoStep::DeleteBranch { name, base_commit }; + let mut ops = KickoffUndo { + repo_root, + worktree_dir: worktree_path, + prior_pipeline: None, + }; + let outcome = ops.run(&step); + if outcome == UndoOutcome::Done { + result.branch_deleted = true; + } else if let Some(message) = outcome_message(&step, &outcome) { + result.warnings.push(message); + } + } let wt_display = std::path::Path::new(&agent.worktree) .file_name() .and_then(|n| n.to_str()) @@ -282,6 +324,18 @@ pub fn cleanup( } } + if !json_output { + for issue in &result.locks_released { + println!(" Released lock on #{issue}"); + } + if result.branch_deleted { + println!(" Deleted branch with no commits beyond its base"); + } + for warning in &result.warnings { + eprintln!(" Warning: {warning}"); + } + } + results.push(result); } @@ -312,3 +366,147 @@ pub fn cleanup( Ok(()) } + +/// Brings the worktree's daemon up so the agent's locks can be released and +/// its session ended under its own identity. Anything left is a warning +/// with its remedy. +fn release_agent_work(repo_root: &Path, worktree: &Path, result: &mut CleanupResult) { + let worktree_crosslink = worktree.join(".crosslink"); + if !worktree_crosslink.join("agent.json").exists() { + return; + } + if crate::reconcile::readiness::requires_readiness(&worktree_crosslink) { + let ready = crate::daemon::ensure(&worktree_crosslink, true) + .map(|record| record.state.grants_mutations()); + if !matches!(ready, Ok(true)) { + result.warnings.push(format!( + "the worktree daemon did not become ready, so the agent's locks and session \ + were left; to clear them, run `crosslink locks list` and \ + `crosslink locks release ` in {}", + worktree.display() + )); + return; + } + } + let held = match crate::shared_writer::SharedWriter::new(&worktree_crosslink) { + Ok(Some(writer)) => writer.locks_held_by_self(), + Ok(None) => Ok(Vec::new()), + Err(error) => Err(error), + }; + let held = match held { + Ok(held) => held, + Err(error) => { + result.warnings.push(format!( + "could not read the agent's locks: {error:#}; to clear them, run \ + `crosslink locks release ` in {}", + worktree.display() + )); + Vec::new() + } + }; + let mut ops = KickoffUndo { + repo_root, + worktree_dir: worktree, + prior_pipeline: None, + }; + for issue_id in held { + let step = UndoStep::ReleaseLock { issue_id }; + let outcome = ops.run(&step); + if outcome == UndoOutcome::Done { + result.locks_released.push(issue_id); + } else if let Some(message) = outcome_message(&step, &outcome) { + result.warnings.push(message); + } + } + let step = UndoStep::EndSession; + let outcome = ops.run(&step); + if let Some(message) = outcome_message(&step, &outcome) { + result.warnings.push(message); + } +} + +/// The worktree's branch and its recorded base, when kickoff recorded one. +fn branch_and_base(worktree: &Path) -> Option<(String, String)> { + let metadata: KickoffMetadata = + serde_json::from_slice(&std::fs::read(worktree.join(".kickoff-metadata.json")).ok()?) + .ok()?; + let base_commit = metadata.base_commit?; + let branch = Command::new("git") + .current_dir(worktree) + .args(["branch", "--show-current"]) + .output() + .ok() + .filter(|output| output.status.success())?; + let branch = String::from_utf8_lossy(&branch.stdout).trim().to_string(); + (!branch.is_empty()).then_some((branch, base_commit)) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn git(dir: &Path, args: &[&str]) -> String { + let output = Command::new("git") + .current_dir(dir) + .args(args) + .output() + .expect("git runs"); + assert!(output.status.success(), "git {args:?}"); + String::from_utf8_lossy(&output.stdout).trim().to_string() + } + + fn write_metadata(worktree: &Path, base_commit: Option<&str>) { + let metadata = serde_json::json!({ + "started_at": "2026-10-08T00:00:00Z", + "timeout_secs": 3600, + "base_commit": base_commit, + }); + std::fs::write( + worktree.join(".kickoff-metadata.json"), + serde_json::to_vec(&metadata).expect("json"), + ) + .expect("metadata"); + } + + /// AC-11 (cleanup): the branch rule needs a recorded base; metadata + /// written before `base_commit` existed keeps the branch. + #[test] + fn cleanup_deletes_a_branch_only_with_a_recorded_base() { + let dir = tempfile::tempdir().expect("tempdir"); + let worktree = dir.path(); + git(worktree, &["init", "-q", "-b", "feature/x"]); + git(worktree, &["config", "user.email", "test@test.local"]); + git(worktree, &["config", "user.name", "Test"]); + git(worktree, &["config", "commit.gpgsign", "false"]); + git(worktree, &["commit", "-q", "--allow-empty", "-m", "init"]); + let head = git(worktree, &["rev-parse", "HEAD"]); + + write_metadata(worktree, Some(&head)); + assert_eq!( + branch_and_base(worktree), + Some(("feature/x".to_string(), head)) + ); + + write_metadata(worktree, None); + assert_eq!(branch_and_base(worktree), None); + } + + #[test] + fn cleanup_leaves_locks_alone_without_an_agent() { + let dir = tempfile::tempdir().expect("tempdir"); + let mut result = CleanupResult { + id: "agent".to_string(), + class: CleanupClass::Stale, + worktree_removed: false, + tmux_killed: false, + container_removed: false, + locks_released: Vec::new(), + branch_deleted: false, + warnings: Vec::new(), + error: None, + }; + release_agent_work(dir.path(), dir.path(), &mut result); + assert!(result.locks_released.is_empty()); + assert!(result.warnings.is_empty()); + } +} diff --git a/crosslink/src/commands/kickoff/helpers.rs b/crosslink/src/commands/kickoff/helpers.rs index 36bfad7bc..77822ab95 100644 --- a/crosslink/src/commands/kickoff/helpers.rs +++ b/crosslink/src/commands/kickoff/helpers.rs @@ -1,7 +1,6 @@ use std::collections::HashSet; use std::path::Path; -use std::process::{Command, Output, Stdio}; -use std::time::{Duration, Instant}; +use std::process::Command; use super::types::*; @@ -452,30 +451,7 @@ pub(crate) fn command_available(cmd: &str) -> bool { lookup.is_ok_and(|o| o.status.success()) } -pub(super) fn command_output_with_timeout( - command: &mut Command, - timeout: Duration, -) -> Option { - let mut child = command - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .spawn() - .ok()?; - let deadline = Instant::now() + timeout; - loop { - match child.try_wait() { - Ok(Some(_)) => return child.wait_with_output().ok(), - Ok(None) if Instant::now() < deadline => { - std::thread::sleep(Duration::from_millis(20)); - } - Ok(None) | Err(_) => { - let _ = child.kill(); - let _ = child.wait(); - return None; - } - } - } -} +pub(super) use crate::utils::command_output_with_timeout; pub(super) fn detect_platform() -> Platform { if cfg!(target_os = "macos") { diff --git a/crosslink/src/commands/kickoff/mod.rs b/crosslink/src/commands/kickoff/mod.rs index 917c0467f..93febc52f 100644 --- a/crosslink/src/commands/kickoff/mod.rs +++ b/crosslink/src/commands/kickoff/mod.rs @@ -24,7 +24,7 @@ pub use types::{parse_container_mode, parse_duration, parse_verify_level}; #[cfg(test)] pub use types::{resolve_agent_image_from, AGENT_IMAGE_TAG}; -pub use cleanup::cleanup; +pub use cleanup::{cleanup, CleanupOptions}; pub use graph::graph; pub use monitor::{list, logs, report, report_all, status, stop}; pub use plan::{plan, show_plan}; @@ -192,8 +192,18 @@ pub fn dispatch( dry_run, force, keep, + keep_branch, json: cleanup_json, - } => cleanup(crosslink_dir, dry_run, force, keep, cleanup_json), + } => cleanup( + crosslink_dir, + &CleanupOptions { + dry_run, + force, + keep, + keep_branch, + json_output: cleanup_json, + }, + ), KickoffCommands::Launch { doc, plan: do_plan, diff --git a/crosslink/src/commands/kickoff/plan.rs b/crosslink/src/commands/kickoff/plan.rs index 6b30da24a..4a393b78d 100644 --- a/crosslink/src/commands/kickoff/plan.rs +++ b/crosslink/src/commands/kickoff/plan.rs @@ -8,6 +8,7 @@ use crate::identity::AgentConfig; use super::helpers::*; use super::launch::*; use super::prompt::{interpolate_template, TemplateContext}; +use super::rollback::{KickoffRollback, Origin}; use super::types::*; pub(crate) fn build_allowed_tools_plan() -> String { @@ -211,115 +212,135 @@ pub fn plan(crosslink_dir: &Path, db: &impl QueryService, opts: &PlanOpts) -> Re let (worktree_dir, branch_name) = create_worktree(&root, &slug, None)?; - std::fs::write(worktree_dir.join(".kickoff-slug"), &slug) - .context("Failed to write .kickoff-slug sentinel")?; - - std::fs::write(worktree_dir.join("PLAN_KICKOFF.md"), &prompt) - .context("Failed to write PLAN_KICKOFF.md")?; - std::fs::create_dir_all(worktree_dir.join(".crosslink/runtime")) - .context("Failed to create provider runtime log directory")?; - let metadata = KickoffMetadata { - started_at: chrono::Utc::now().to_rfc3339(), - timeout_secs: opts.timeout.as_secs(), - provider: Some(validation_agent.provider.to_string()), - model: validation_agent.resolve_model(Some(opts.model)), - effort: opts.policy.effort.clone(), - budget_usd: opts.policy.monetary_budget_usd.clone(), - base_commit: None, - }; - std::fs::write( - worktree_dir.join(".kickoff-metadata.json"), - serde_json::to_vec_pretty(&metadata)?, - ) - .context("Failed to write plan provider metadata")?; - - exclude_kickoff_files(&worktree_dir)?; - - if let Some(doc_path) = opts.doc_path { - let _ = super::pipeline::mark_planning( - doc_path, - &format!("driver--{slug}"), - &worktree_dir.to_string_lossy(), - ); - } - - let agent_id = init_worktree_agent(&worktree_dir, crosslink_dir, &slug, issue_id)?; + // Everything below is undone if it fails before the agent starts. + let base_commit = super::run::worktree_head(&worktree_dir); + let mut rollback = KickoffRollback::new(); + rollback.record_worktree(worktree_dir.clone(), Origin::Created); + rollback.record_branch(branch_name.clone(), Origin::Created, base_commit.clone()); + + let launched = (|| -> Result<()> { + std::fs::write(worktree_dir.join(".kickoff-slug"), &slug) + .context("Failed to write .kickoff-slug sentinel")?; + + std::fs::write(worktree_dir.join("PLAN_KICKOFF.md"), &prompt) + .context("Failed to write PLAN_KICKOFF.md")?; + std::fs::create_dir_all(worktree_dir.join(".crosslink/runtime")) + .context("Failed to create provider runtime log directory")?; + let metadata = KickoffMetadata { + started_at: chrono::Utc::now().to_rfc3339(), + timeout_secs: opts.timeout.as_secs(), + provider: Some(validation_agent.provider.to_string()), + model: validation_agent.resolve_model(Some(opts.model)), + effort: opts.policy.effort.clone(), + budget_usd: opts.policy.monetary_budget_usd.clone(), + base_commit: base_commit.clone(), + }; + std::fs::write( + worktree_dir.join(".kickoff-metadata.json"), + serde_json::to_vec_pretty(&metadata)?, + ) + .context("Failed to write plan provider metadata")?; + + exclude_kickoff_files(&worktree_dir)?; + + if let Some(doc_path) = opts.doc_path { + rollback.record_pipeline( + doc_path.to_path_buf(), + super::pipeline::read_pipeline_state(doc_path), + ); + let _ = super::pipeline::mark_planning( + doc_path, + &format!("driver--{slug}"), + &worktree_dir.to_string_lossy(), + ); + } - let preflight = preflight.context("preflight check was skipped unexpectedly")?; + rollback.record_agent(); + if let Some(id) = issue_id { + rollback.record_claim(id); + } + let agent_id = init_worktree_agent(&worktree_dir, crosslink_dir, &slug, issue_id)?; - let allowed_tools = build_allowed_tools_plan(); - let mut session_name = tmux_session_name(&slug); - if tmux_session_exists(&session_name) { - let suffix = rand_suffix(); - session_name = format!("{}-{}", &session_name[..session_name.len().min(44)], suffix); - } + let preflight = preflight.context("preflight check was skipped unexpectedly")?; - let cmd = build_resolved_agent_command( - &preflight.agent, - preflight.timeout_cmd, - opts.model, - &allowed_tools, - "PLAN_KICKOFF.md", - preflight.sandbox_command.as_deref(), - &worktree_dir, - &opts.policy, - )?; + let allowed_tools = build_allowed_tools_plan(); + let mut session_name = tmux_session_name(&slug); + if tmux_session_exists(&session_name) { + let suffix = rand_suffix(); + session_name = format!("{}-{}", &session_name[..session_name.len().min(44)], suffix); + } - let output = Command::new("tmux") - .args([ - "new-session", - "-d", - "-s", - &session_name, - "-c", - &worktree_dir.to_string_lossy(), - ]) - .output() - .context("Failed to create tmux session")?; - - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - bail!("Failed to create tmux session: {}", stderr.trim()); - } + let cmd = build_resolved_agent_command( + &preflight.agent, + preflight.timeout_cmd, + opts.model, + &allowed_tools, + "PLAN_KICKOFF.md", + preflight.sandbox_command.as_deref(), + &worktree_dir, + &opts.policy, + )?; + + rollback.record_tmux_session(session_name.clone()); + let output = Command::new("tmux") + .args([ + "new-session", + "-d", + "-s", + &session_name, + "-c", + &worktree_dir.to_string_lossy(), + ]) + .output() + .context("Failed to create tmux session")?; + + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + bail!("Failed to create tmux session: {}", stderr.trim()); + } - let output = Command::new("tmux") - .args(["send-keys", "-t", &session_name, &cmd, "Enter"]) - .output() - .context("Failed to send command to tmux session")?; + let output = Command::new("tmux") + .args(["send-keys", "-t", &session_name, &cmd, "Enter"]) + .output() + .context("Failed to send command to tmux session")?; - if !output.status.success() { - let stderr = String::from_utf8_lossy(&output.stderr); - bail!("Failed to send keys to tmux: {}", stderr.trim()); - } + if !output.status.success() { + let stderr = String::from_utf8_lossy(&output.stderr); + bail!("Failed to send keys to tmux: {}", stderr.trim()); + } + rollback.disarm(); - let _ = std::fs::write(worktree_dir.join(".kickoff-session"), &session_name); + let _ = std::fs::write(worktree_dir.join(".kickoff-session"), &session_name); - let watchdog_cfg = read_watchdog_config(crosslink_dir); - if watchdog_cfg.enabled { - if let Err(e) = spawn_watchdog(&session_name, &worktree_dir, &watchdog_cfg) { - tracing::warn!("failed to spawn watchdog: {}", e); + let watchdog_cfg = read_watchdog_config(crosslink_dir); + if watchdog_cfg.enabled { + if let Err(e) = spawn_watchdog(&session_name, &worktree_dir, &watchdog_cfg) { + tracing::warn!("failed to spawn watchdog: {}", e); + } } - } - if opts.quiet { - println!("{session_name}"); - } else { - println!("Plan analysis agent launched (read-only mode)."); - println!(); - println!(" Worktree: {}", worktree_dir.display()); - println!(" Branch: {branch_name}"); - if let Some(id) = issue_id { - println!(" Issue: #{id}"); + if opts.quiet { + println!("{session_name}"); + } else { + println!("Plan analysis agent launched (read-only mode)."); + println!(); + println!(" Worktree: {}", worktree_dir.display()); + println!(" Branch: {branch_name}"); + if let Some(id) = issue_id { + println!(" Issue: #{id}"); + } + println!(" Agent: {agent_id}"); + println!(" Session: {session_name}"); + println!(); + println!(" Approve trust: tmux attach -t {session_name}"); + println!(" Check status: crosslink kickoff status {agent_id}"); + println!(" View report: crosslink kickoff show-plan {agent_id}"); } - println!(" Agent: {agent_id}"); - println!(" Session: {session_name}"); - println!(); - println!(" Approve trust: tmux attach -t {session_name}"); - println!(" Check status: crosslink kickoff status {agent_id}"); - println!(" View report: crosslink kickoff show-plan {agent_id}"); - } - Ok(()) + Ok(()) + })(); + + launched.inspect_err(|_| super::run::undo_failed_kickoff(rollback, &root, &worktree_dir)) } pub fn show_plan(crosslink_dir: &Path, agent: &str) -> Result<()> { diff --git a/crosslink/src/commands/kickoff/rollback.rs b/crosslink/src/commands/kickoff/rollback.rs index 41ed1b01d..61bd069e3 100644 --- a/crosslink/src/commands/kickoff/rollback.rs +++ b/crosslink/src/commands/kickoff/rollback.rs @@ -2,10 +2,9 @@ //! session and branch steps that `kickoff cleanup` shares with it. //! //! Design: `.design/lock-claim-outcome-and-kickoff-rollback.md`. -// Scaffold: kickoff run, plan and cleanup call this in the implementation commit. -#![allow(dead_code)] -use std::path::PathBuf; +use std::path::{Path, PathBuf}; +use std::process::Command; use super::pipeline::PipelineState; @@ -72,7 +71,9 @@ pub(super) struct UndoReport { impl UndoReport { /// True when every step either completed or had nothing to undo. pub(super) fn is_clean(&self) -> bool { - unimplemented!("scaffold: undo report classification") + self.entries + .iter() + .all(|(_, outcome)| matches!(outcome, UndoOutcome::Done | UndoOutcome::NothingToUndo)) } } @@ -142,22 +143,315 @@ impl KickoffRollback { /// The undo steps for what was recorded, in the order they must run. pub(super) fn steps(&self) -> Vec { - unimplemented!("scaffold: undo step plan") + if self.disarmed { + return Vec::new(); + } + let mut steps = Vec::new(); + if let Some((runtime, name)) = &self.container { + steps.push(UndoStep::RemoveContainer { + runtime: runtime.clone(), + name: name.clone(), + }); + } + if let Some(name) = &self.tmux_session { + steps.push(UndoStep::KillTmuxSession { name: name.clone() }); + } + if self.agent_initialized { + if let Some(issue_id) = self.claimed_issue { + steps.push(UndoStep::ReleaseLock { issue_id }); + } + steps.push(UndoStep::EndSession); + steps.push(UndoStep::StopDaemon); + } + if let Some((doc_path, _)) = &self.pipeline { + steps.push(UndoStep::RestorePipeline { + doc_path: doc_path.clone(), + }); + } + if let Some((path, Origin::Created)) = &self.worktree { + steps.push(UndoStep::RemoveWorktree { path: path.clone() }); + } + if let Some((name, Origin::Created, Some(base_commit))) = &self.branch { + steps.push(UndoStep::DeleteBranch { + name: name.clone(), + base_commit: base_commit.clone(), + }); + } + steps + } + + /// The pipeline state recorded before kickoff changed it, for the + /// `RestorePipeline` step. + pub(super) fn prior_pipeline(&self) -> Option<&PipelineState> { + self.pipeline.as_ref().and_then(|(_, prior)| prior.as_ref()) } /// Runs every step, continuing past failures, and reports each outcome. - #[allow(clippy::needless_pass_by_ref_mut)] // scaffold: the implementation calls `ops.run` pub(super) fn unwind(self, ops: &mut impl UndoOps) -> UndoReport { - let _ = ops; - unimplemented!("scaffold: undo execution") + let entries = self + .steps() + .into_iter() + .map(|step| { + let outcome = ops.run(&step); + (step, outcome) + }) + .collect(); + UndoReport { entries } + } +} + +/// Runs undo steps against the system for one kickoff worktree. +pub(super) struct KickoffUndo<'a> { + pub(super) repo_root: &'a Path, + pub(super) worktree_dir: &'a Path, + pub(super) prior_pipeline: Option, +} + +impl KickoffUndo<'_> { + fn worktree_crosslink(&self) -> PathBuf { + self.worktree_dir.join(".crosslink") + } + + fn has_agent(&self) -> bool { + self.worktree_crosslink().join("agent.json").exists() + } + + fn release_lock(&self, issue_id: i64) -> UndoOutcome { + if !self.has_agent() { + return UndoOutcome::NothingToUndo; + } + let remedy = format!( + "run `crosslink locks release {issue_id}` in {}", + self.worktree_dir.display() + ); + let writer = match crate::shared_writer::SharedWriter::new(&self.worktree_crosslink()) { + Ok(Some(writer)) => writer, + Ok(None) => return UndoOutcome::NothingToUndo, + Err(error) => { + return UndoOutcome::Failed { + error: format!("{error:#}"), + remedy, + } + } + }; + if let Err(error) = writer.release_lock_v2(issue_id) { + return UndoOutcome::Failed { + error: format!("{error:#}"), + remedy, + }; + } + match writer.confirmed_lock_holder(issue_id) { + Ok(Some(holder)) if holder == writer.agent_id() => UndoOutcome::Stranded { remedy }, + Ok(_) => UndoOutcome::Done, + Err(error) => UndoOutcome::Unverified { + remedy: format!("{remedy} (the release could not be confirmed: {error:#})"), + }, + } + } + + fn end_session(&self) -> UndoOutcome { + if !self.has_agent() { + return UndoOutcome::NothingToUndo; + } + command_outcome( + Command::new("crosslink") + .current_dir(self.worktree_dir) + .args(["session", "end"]), + &format!( + "run `crosslink session end` in {}", + self.worktree_dir.display() + ), + &["No active session"], + ) + } + + fn stop_daemon(&self) -> UndoOutcome { + let crosslink = self.worktree_crosslink(); + if !crate::reconcile::readiness::requires_readiness(&crosslink) { + return UndoOutcome::NothingToUndo; + } + match crate::daemon::stop(&crosslink) { + Ok(()) => UndoOutcome::Done, + Err(error) => UndoOutcome::Failed { + error: format!("{error:#}"), + remedy: format!( + "run `crosslink daemon stop` in {}", + self.worktree_dir.display() + ), + }, + } + } + + fn restore_pipeline(&self, doc_path: &Path) -> UndoOutcome { + let result = if let Some(prior) = &self.prior_pipeline { + super::pipeline::write_pipeline_state(doc_path, prior) + } else { + let path = super::pipeline::pipeline_path_for_doc(doc_path); + if !path.exists() { + return UndoOutcome::NothingToUndo; + } + std::fs::remove_file(&path).map_err(anyhow::Error::from) + }; + match result { + Ok(()) => UndoOutcome::Done, + Err(error) => UndoOutcome::Failed { + error: format!("{error:#}"), + remedy: format!( + "restore {} by hand", + super::pipeline::pipeline_path_for_doc(doc_path).display() + ), + }, + } + } + + fn remove_worktree(&self, path: &Path) -> UndoOutcome { + if !path.exists() { + return UndoOutcome::NothingToUndo; + } + let path_arg = path.to_string_lossy(); + command_outcome( + Command::new("git") + .current_dir(self.repo_root) + .args(["worktree", "remove", "--force", &path_arg]), + &format!("run `git worktree remove --force {path_arg}`"), + &[], + ) + } + + fn delete_branch(&self, name: &str, base_commit: &str) -> UndoOutcome { + let tip = Command::new("git") + .current_dir(self.repo_root) + .args([ + "rev-parse", + "--verify", + "--quiet", + &format!("refs/heads/{name}"), + ]) + .output(); + let Ok(tip) = tip else { + return UndoOutcome::NothingToUndo; + }; + if !tip.status.success() { + return UndoOutcome::NothingToUndo; + } + let tip = String::from_utf8_lossy(&tip.stdout).trim().to_string(); + if !branch_deletable(&tip, Some(base_commit)) { + return UndoOutcome::NothingToUndo; + } + command_outcome( + Command::new("git") + .current_dir(self.repo_root) + .args(["branch", "-D", name]), + &format!("run `git branch -D {name}`"), + &[], + ) + } +} + +impl UndoOps for KickoffUndo<'_> { + fn run(&mut self, step: &UndoStep) -> UndoOutcome { + match step { + UndoStep::RemoveContainer { runtime, name } => command_outcome( + Command::new(runtime).args(["rm", "-f", name]), + &format!("run `{runtime} rm -f {name}`"), + &["No such container", "no such container"], + ), + UndoStep::KillTmuxSession { name } => command_outcome( + Command::new("tmux").args(["kill-session", "-t", name]), + &format!("run `tmux kill-session -t {name}`"), + &["can't find session", "no server running"], + ), + UndoStep::ReleaseLock { issue_id } => self.release_lock(*issue_id), + UndoStep::EndSession => self.end_session(), + UndoStep::StopDaemon => self.stop_daemon(), + UndoStep::RestorePipeline { doc_path } => self.restore_pipeline(doc_path), + UndoStep::RemoveWorktree { path } => self.remove_worktree(path), + UndoStep::DeleteBranch { name, base_commit } => self.delete_branch(name, base_commit), + } + } +} + +/// Runs a command for an undo step. Output matching one of `absent` means the +/// resource was already gone. +fn command_outcome(command: &mut Command, remedy: &str, absent: &[&str]) -> UndoOutcome { + match command.output() { + Ok(output) if output.status.success() => UndoOutcome::Done, + Ok(output) => { + let stderr = String::from_utf8_lossy(&output.stderr); + if absent.iter().any(|needle| stderr.contains(needle)) { + UndoOutcome::NothingToUndo + } else { + UndoOutcome::Failed { + error: stderr.trim().to_string(), + remedy: remedy.to_string(), + } + } + } + Err(error) => UndoOutcome::Failed { + error: error.to_string(), + remedy: remedy.to_string(), + }, + } +} + +impl UndoReport { + /// Prints every step that did not complete cleanly, with its remedy, and + /// one summary line. + pub(super) fn print(&self) { + for (step, outcome) in &self.entries { + if let Some(message) = outcome_message(step, outcome) { + eprintln!("kickoff undo: {message}"); + } + } + if self.is_clean() { + eprintln!("kickoff undo: removed everything this kickoff created"); + } else { + eprintln!("kickoff undo: some steps did not complete; see above"); + } + } +} + +/// What to tell the user about a step that did not complete cleanly, or +/// `None` when it did. +pub(super) fn outcome_message(step: &UndoStep, outcome: &UndoOutcome) -> Option { + match outcome { + UndoOutcome::Done | UndoOutcome::NothingToUndo => None, + UndoOutcome::Failed { error, remedy } => Some(format!( + "{} failed: {error}; to clear it, {remedy}", + step.describe() + )), + UndoOutcome::Stranded { remedy } => Some(format!( + "{} left the lock held (stranded); to clear it, {remedy}", + step.describe() + )), + UndoOutcome::Unverified { remedy } => Some(format!( + "{} could not be verified; to check it, {remedy}", + step.describe() + )), + } +} + +impl UndoStep { + fn describe(&self) -> String { + match self { + Self::RemoveContainer { name, .. } => format!("removing container {name}"), + Self::KillTmuxSession { name } => format!("killing tmux session {name}"), + Self::ReleaseLock { issue_id } => format!("releasing the lock on #{issue_id}"), + Self::EndSession => "ending the worktree session".to_string(), + Self::StopDaemon => "stopping the worktree daemon".to_string(), + Self::RestorePipeline { doc_path } => { + format!("restoring the pipeline state of {}", doc_path.display()) + } + Self::RemoveWorktree { path } => format!("removing worktree {}", path.display()), + Self::DeleteBranch { name, .. } => format!("deleting branch {name}"), + } } } /// A branch may be deleted only when a base was recorded and the branch has /// no commits beyond it. pub(super) fn branch_deletable(tip: &str, base_commit: Option<&str>) -> bool { - let _ = (tip, base_commit); - unimplemented!("scaffold: branch deletion rule") + base_commit.is_some_and(|base| base == tip) } #[cfg(test)] @@ -326,6 +620,138 @@ mod tests { .any(|(_, outcome)| matches!(outcome, UndoOutcome::Stranded { .. }))); } + fn git(dir: &Path, args: &[&str]) -> String { + let output = Command::new("git") + .current_dir(dir) + .args(args) + .output() + .expect("git runs"); + assert!( + output.status.success(), + "git {args:?}: {}", + String::from_utf8_lossy(&output.stderr) + ); + String::from_utf8_lossy(&output.stdout).trim().to_string() + } + + /// A repository with one commit and a kickoff-style worktree on a fresh + /// branch. Returns the temp dir, repo root, worktree path and base commit. + fn repo_with_worktree() -> (tempfile::TempDir, PathBuf, PathBuf, String) { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path().join("repo"); + std::fs::create_dir_all(&root).expect("repo dir"); + git(&root, &["init", "-q", "-b", "main"]); + git(&root, &["config", "user.email", "test@test.local"]); + git(&root, &["config", "user.name", "Test"]); + git(&root, &["config", "commit.gpgsign", "false"]); + std::fs::write(root.join("README.md"), "# test\n").expect("readme"); + git(&root, &["add", "README.md"]); + git(&root, &["commit", "-q", "-m", "init"]); + let worktree = dir.path().join("wt"); + git( + &root, + &[ + "worktree", + "add", + "-q", + "-b", + "feature/x", + worktree.to_str().expect("utf-8 path"), + "HEAD", + ], + ); + let base = git(&worktree, &["rev-parse", "HEAD"]); + (dir, root, worktree, base) + } + + /// AC-8 (operations): a created worktree and its unchanged branch are + /// removed; steps with no agent have nothing to undo. + #[test] + fn undo_removes_a_created_worktree_and_its_unchanged_branch() { + let (_dir, root, worktree, base) = repo_with_worktree(); + let mut rollback = KickoffRollback::new(); + rollback.record_worktree(worktree.clone(), Origin::Created); + rollback.record_branch("feature/x".to_string(), Origin::Created, Some(base)); + rollback.record_agent(); + rollback.record_claim(7); + let mut ops = KickoffUndo { + repo_root: &root, + worktree_dir: &worktree, + prior_pipeline: None, + }; + let report = rollback.unwind(&mut ops); + assert!(report.is_clean(), "{:?}", report.entries); + assert!(!worktree.exists()); + let branches = git(&root, &["branch", "--list", "feature/x"]); + assert!(branches.is_empty(), "branch survived: {branches}"); + let nothing: Vec<_> = report + .entries + .iter() + .filter(|(_, outcome)| *outcome == UndoOutcome::NothingToUndo) + .map(|(step, _)| step.clone()) + .collect(); + assert!(nothing.contains(&UndoStep::ReleaseLock { issue_id: 7 })); + assert!(nothing.contains(&UndoStep::EndSession)); + } + + /// AC-11 (operations): a branch with its own commits is kept. + #[test] + fn undo_keeps_a_branch_with_its_own_commits() { + let (_dir, root, worktree, base) = repo_with_worktree(); + std::fs::write(worktree.join("work.txt"), "work\n").expect("work"); + git(&worktree, &["add", "work.txt"]); + git(&worktree, &["commit", "-q", "-m", "agent work"]); + let mut rollback = KickoffRollback::new(); + rollback.record_worktree(worktree.clone(), Origin::Created); + rollback.record_branch("feature/x".to_string(), Origin::Created, Some(base)); + let mut ops = KickoffUndo { + repo_root: &root, + worktree_dir: &worktree, + prior_pipeline: None, + }; + let report = rollback.unwind(&mut ops); + assert!(report.is_clean(), "{:?}", report.entries); + assert!(!worktree.exists()); + let branches = git(&root, &["branch", "--list", "feature/x"]); + assert!(!branches.is_empty(), "branch with commits was deleted"); + } + + /// AC-8 (pipeline): the prior state is written back, or a pipeline file + /// kickoff created is removed. + #[test] + fn undo_restores_or_removes_the_pipeline_state() { + let dir = tempfile::tempdir().expect("tempdir"); + let doc = dir.path().join("feature.md"); + std::fs::write(&doc, "# Feature\n").expect("doc"); + let prior = super::super::pipeline::create_initial_pipeline(&doc).expect("pipeline"); + let path = super::super::pipeline::pipeline_path_for_doc(&doc); + super::super::pipeline::mark_running(&doc, "agent", "/wt", Some(7)).expect("running"); + assert_ne!( + super::super::pipeline::read_pipeline_state(&doc) + .expect("state") + .stage, + prior.stage + ); + + let step = UndoStep::RestorePipeline { + doc_path: doc.clone(), + }; + let mut ops = KickoffUndo { + repo_root: dir.path(), + worktree_dir: dir.path(), + prior_pipeline: Some(prior.clone()), + }; + assert_eq!(ops.run(&step), UndoOutcome::Done); + let restored = super::super::pipeline::read_pipeline_state(&doc).expect("restored"); + assert_eq!(restored.stage, prior.stage); + assert!(restored.runs.is_empty()); + + ops.prior_pipeline = None; + assert_eq!(ops.run(&step), UndoOutcome::Done); + assert!(!path.exists()); + assert_eq!(ops.run(&step), UndoOutcome::NothingToUndo); + } + #[test] fn branch_is_deletable_only_at_its_recorded_base() { assert!(branch_deletable(BASE, Some(BASE))); diff --git a/crosslink/src/commands/kickoff/run.rs b/crosslink/src/commands/kickoff/run.rs index 7c1e14270..d13ac16c4 100644 --- a/crosslink/src/commands/kickoff/run.rs +++ b/crosslink/src/commands/kickoff/run.rs @@ -7,6 +7,7 @@ use crate::db::Database; use super::helpers::*; use super::launch::*; use super::prompt::*; +use super::rollback::{KickoffRollback, KickoffUndo, Origin}; use super::types::*; pub(crate) fn resolve_kickoff_issue( @@ -137,154 +138,211 @@ pub fn run(crosslink_dir: &Path, db: &Database, opts: &KickoffOpts) -> Result Result { + std::fs::write(worktree_dir.join(".kickoff-slug"), &compact_name) + .context("Failed to write .kickoff-slug sentinel")?; + + std::fs::write(worktree_dir.join("KICKOFF.md"), &prompt) + .context("Failed to write KICKOFF.md")?; + + if let Some(doc) = opts.design_doc { + if !doc.acceptance_criteria.is_empty() { + let source = opts.doc_path.unwrap_or("unknown"); + let criteria_file = extract_criteria(doc, source); + let json = serde_json::to_string_pretty(&criteria_file) + .context("Failed to serialize criteria")?; + std::fs::write(worktree_dir.join(".kickoff-criteria.json"), &json) + .context("Failed to write .kickoff-criteria.json")?; + } } - } - { - let mut metadata = KickoffMetadata::for_launch(opts, chrono::Utc::now().to_rfc3339()); - metadata.provider = Some(validation_agent.provider.to_string()); - metadata.model = validation_agent.resolve_model(Some(opts.model)); - let json = serde_json::to_string_pretty(&metadata) - .context("Failed to serialize kickoff metadata")?; - std::fs::write(worktree_dir.join(".kickoff-metadata.json"), &json) - .context("Failed to write .kickoff-metadata.json")?; - } - - let protected_doc_rel = resolve_worktree_relative_doc(opts.doc_path, &root); - if let Some(rel) = protected_doc_rel.as_deref() { - protect_design_doc(&worktree_dir, rel)?; - } - - exclude_kickoff_files(&worktree_dir)?; + { + let mut metadata = KickoffMetadata::for_launch(opts, chrono::Utc::now().to_rfc3339()); + metadata.base_commit.clone_from(&base_commit); + metadata.provider = Some(validation_agent.provider.to_string()); + metadata.model = validation_agent.resolve_model(Some(opts.model)); + let json = serde_json::to_string_pretty(&metadata) + .context("Failed to serialize kickoff metadata")?; + std::fs::write(worktree_dir.join(".kickoff-metadata.json"), &json) + .context("Failed to write .kickoff-metadata.json")?; + } - let agent_id = - init_worktree_agent(&worktree_dir, crosslink_dir, &compact_name, Some(issue_id))?; + let protected_doc_rel = resolve_worktree_relative_doc(opts.doc_path, &root); + if let Some(rel) = protected_doc_rel.as_deref() { + protect_design_doc(&worktree_dir, rel)?; + } - if let Some(doc_path_str) = opts.doc_path { - let doc_path = Path::new(doc_path_str); - if let Err(e) = super::pipeline::mark_running( - doc_path, - &agent_id, - &worktree_dir.to_string_lossy(), - Some(issue_id), - ) { - tracing::warn!("could not record pipeline run row for {doc_path_str}: {e}"); + exclude_kickoff_files(&worktree_dir)?; + + rollback.record_agent(); + rollback.record_claim(issue_id); + let agent_id = + init_worktree_agent(&worktree_dir, crosslink_dir, &compact_name, Some(issue_id))?; + + if let Some(doc_path_str) = opts.doc_path { + let doc_path = Path::new(doc_path_str); + rollback.record_pipeline( + doc_path.to_path_buf(), + super::pipeline::read_pipeline_state(doc_path), + ); + if let Err(e) = super::pipeline::mark_running( + doc_path, + &agent_id, + &worktree_dir.to_string_lossy(), + Some(issue_id), + ) { + tracing::warn!("could not record pipeline run row for {doc_path_str}: {e}"); + } } - } - let preflight = preflight.context("preflight check was skipped unexpectedly")?; + let preflight = preflight.context("preflight check was skipped unexpectedly")?; - let allowed_tools = build_allowed_tools(&conventions, &opts.verify); + let allowed_tools = build_allowed_tools(&conventions, &opts.verify); - match &opts.container { - ContainerMode::None => { - let mut session_name = tmux_session_name(&compact_name); - if tmux_session_exists(&session_name) { - let suffix: u32 = rand_suffix(); - session_name = - format!("{}-{}", &session_name[..session_name.len().min(58)], suffix); - } + match &opts.container { + ContainerMode::None => { + let mut session_name = tmux_session_name(&compact_name); + if tmux_session_exists(&session_name) { + let suffix: u32 = rand_suffix(); + session_name = + format!("{}-{}", &session_name[..session_name.len().min(58)], suffix); + } - launch_local( - &preflight.agent, - &worktree_dir, - &session_name, - opts.model, - &allowed_tools, - preflight.timeout_cmd, - preflight.sandbox_command.as_deref(), - crosslink_dir, - &opts.policy, - )?; - - let _ = std::fs::write(worktree_dir.join(".kickoff-session"), &session_name); - - if opts.quiet { - println!("{session_name}"); - } else { - println!("Feature agent launched."); - println!(); - println!(" Worktree: {}", worktree_dir.display()); - println!(" Branch: {branch_name}"); - println!(" Issue: #{issue_id}"); - println!(" Agent: {agent_id}"); - println!(" Session: {session_name}"); - println!(" Verify: {:?}", opts.verify); - println!(); - println!(" Approve trust: tmux attach -t {session_name}"); - println!(" Check status: crosslink kickoff status {agent_id}"); - if opts.verify == VerifyLevel::Ci || opts.verify == VerifyLevel::Thorough { + rollback.record_tmux_session(session_name.clone()); + launch_local( + &preflight.agent, + &worktree_dir, + &session_name, + opts.model, + &allowed_tools, + preflight.timeout_cmd, + preflight.sandbox_command.as_deref(), + crosslink_dir, + &opts.policy, + )?; + rollback.disarm(); + + let _ = std::fs::write(worktree_dir.join(".kickoff-session"), &session_name); + + if opts.quiet { + println!("{session_name}"); + } else { + println!("Feature agent launched."); + println!(); + println!(" Worktree: {}", worktree_dir.display()); + println!(" Branch: {branch_name}"); + println!(" Issue: #{issue_id}"); + println!(" Agent: {agent_id}"); + println!(" Session: {session_name}"); + println!(" Verify: {:?}", opts.verify); println!(); - println!(" CI verification is enabled. The agent will push and open a draft PR after local tests pass."); + println!(" Approve trust: tmux attach -t {session_name}"); + println!(" Check status: crosslink kickoff status {agent_id}"); + if opts.verify == VerifyLevel::Ci || opts.verify == VerifyLevel::Thorough { + println!(); + println!(" CI verification is enabled. The agent will push and open a draft PR after local tests pass."); + } } } - } - mode @ (ContainerMode::Docker | ContainerMode::Podman) => { - let container_id = launch_container( - mode, - &preflight.agent, - &worktree_dir, - &root, - opts.image, - &agent_id, - opts.model, - &allowed_tools, - opts.timeout, - protected_doc_rel.as_deref(), - &opts.policy, - )?; - - if opts.quiet { - println!("{container_id}"); - } else { + mode @ (ContainerMode::Docker | ContainerMode::Podman) => { let runtime = if *mode == ContainerMode::Docker { "docker" } else { "podman" }; - println!("Feature agent launched in container."); - println!(); - println!(" Worktree: {}", worktree_dir.display()); - println!(" Branch: {branch_name}"); - println!(" Issue: #{issue_id}"); - println!(" Agent: {agent_id}"); - println!( - " Container: {}", - &container_id[..12.min(container_id.len())] - ); - println!(" Verify: {:?}", opts.verify); - println!(); - println!( - " View logs: {} logs -f {}", - runtime, - &container_id[..12.min(container_id.len())] - ); - println!(" Check status: crosslink kickoff status {agent_id}"); + rollback + .record_container(runtime.to_string(), format!("crosslink-agent-{agent_id}")); + let container_id = launch_container( + mode, + &preflight.agent, + &worktree_dir, + &root, + opts.image, + &agent_id, + opts.model, + &allowed_tools, + opts.timeout, + protected_doc_rel.as_deref(), + &opts.policy, + )?; + rollback.disarm(); + + if opts.quiet { + println!("{container_id}"); + } else { + println!("Feature agent launched in container."); + println!(); + println!(" Worktree: {}", worktree_dir.display()); + println!(" Branch: {branch_name}"); + println!(" Issue: #{issue_id}"); + println!(" Agent: {agent_id}"); + println!( + " Container: {}", + &container_id[..12.min(container_id.len())] + ); + println!(" Verify: {:?}", opts.verify); + println!(); + println!( + " View logs: {} logs -f {}", + runtime, + &container_id[..12.min(container_id.len())] + ); + println!(" Check status: crosslink kickoff status {agent_id}"); + } } } + + Ok(compact_name.clone()) + })(); + + launched.inspect_err(|_| undo_failed_kickoff(rollback, &root, &worktree_dir)) +} + +/// Undoes what a failed kickoff created and reports any step that did not +/// complete. +pub(super) fn undo_failed_kickoff(rollback: KickoffRollback, root: &Path, worktree_dir: &Path) { + let mut ops = KickoffUndo { + repo_root: root, + worktree_dir, + prior_pipeline: rollback.prior_pipeline().cloned(), + }; + let report = rollback.unwind(&mut ops); + if !report.entries.is_empty() { + report.print(); } +} - Ok(compact_name) +/// The commit a freshly created worktree's branch points at. +pub(super) fn worktree_head(worktree_dir: &Path) -> Option { + std::process::Command::new("git") + .current_dir(worktree_dir) + .args(["rev-parse", "HEAD"]) + .output() + .ok() + .filter(|output| output.status.success()) + .map(|output| String::from_utf8_lossy(&output.stdout).trim().to_string()) } fn resolve_worktree_relative_doc(doc_path: Option<&str>, repo_root: &Path) -> Option { diff --git a/crosslink/src/commands/kickoff/types.rs b/crosslink/src/commands/kickoff/types.rs index 036888b4e..908bc15e2 100644 --- a/crosslink/src/commands/kickoff/types.rs +++ b/crosslink/src/commands/kickoff/types.rs @@ -409,6 +409,9 @@ pub(super) struct CleanupResult { pub worktree_removed: bool, pub tmux_killed: bool, pub container_removed: bool, + pub locks_released: Vec, + pub branch_deleted: bool, + pub warnings: Vec, pub error: Option, } diff --git a/crosslink/src/main.rs b/crosslink/src/main.rs index a65c5437f..e2c9e1fbb 100644 --- a/crosslink/src/main.rs +++ b/crosslink/src/main.rs @@ -1581,6 +1581,10 @@ enum KickoffCommands { #[arg(long, default_value = "0")] keep: usize, + /// Keep each agent's branch even when it has no commits beyond its base. + #[arg(long = "keep-branch")] + keep_branch: bool, + #[arg(long)] json: bool, }, diff --git a/crosslink/src/shared_writer/core.rs b/crosslink/src/shared_writer/core.rs index 835b15059..6eb540954 100644 --- a/crosslink/src/shared_writer/core.rs +++ b/crosslink/src/shared_writer/core.rs @@ -32,11 +32,10 @@ pub enum PushOutcome { /// Failure and delay injection for claim tests, in the style of the /// reconciliation `FailureController`. Each field is consulted at one point. #[cfg(test)] -#[allow(dead_code)] // scaffold: the deadline fields are read by the bounded confirmation #[derive(Debug, Default)] pub(crate) struct ClaimFailpoints { - /// Added to the measured publication time of a lock claim. - pub(crate) extra_publish_elapsed: Duration, + /// Sleeps after a successful push, before the steps that follow it. + pub(crate) publish_delay: Duration, /// Makes the lock-confirmation fetch fail. pub(crate) fail_confirm_fetch: bool, /// Fails the first step after a successful push. @@ -52,7 +51,6 @@ pub(crate) struct ClaimFailpoints { /// The result of committing events once publication was attempted. An `Err` /// from the commit means nothing was published; this type means the events /// reached the remote, possibly followed by a failure in a later step. -#[allow(dead_code)] // scaffold: constructed by the implementation commit #[derive(Debug)] pub(crate) struct Published { pub(crate) outcome: PushOutcome, @@ -280,11 +278,36 @@ impl SharedWriter { bail!(V2_WRITE_REFUSAL); } + let published = self.emit_compact_push_tracked(event)?; + match published.after_push_error { + Some(error) => Err(error), + None => Ok(published.outcome), + } + } + + /// `emit_compact_push_inner`, keeping apart "not published" (`Err`) and + /// "published, then a later step failed" (`after_push_error`). + pub(super) fn emit_compact_push_tracked( + &self, + event: crate::events::Event, + ) -> Result { + if !self.is_v3() { + bail!(V2_WRITE_REFUSAL); + } + let lock_guard = self.sync.acquire_lock()?; - let outcome = self.commit_v3(vec![event], &lock_guard)?; - let db = Database::open(&self.readiness_dir.join("issues.db"))?; - crate::hydration::hydrate_current_authority_under_operation(&self.readiness_dir, &db)?; - Ok(outcome) + let mut published = self.commit_v3_tracked(vec![event], &lock_guard)?; + if published.after_push_error.is_none() { + published.after_push_error = Database::open(&self.readiness_dir.join("issues.db")) + .and_then(|db| { + crate::hydration::hydrate_current_authority_under_operation( + &self.readiness_dir, + &db, + ) + }) + .err(); + } + Ok(published) } pub fn write_agent_request( @@ -553,8 +576,24 @@ impl SharedWriter { fn commit_v3( &self, events: Vec, - _lock: &crate::sync::HubWriteLock, + lock: &crate::sync::HubWriteLock, ) -> Result { + let published = self.commit_v3_tracked(events, lock)?; + match published.after_push_error { + Some(error) => Err(error), + None => Ok(published.outcome), + } + } + + /// Commits and publishes `events`. An `Err` means nothing was published + /// (the agent ref is rolled back). Once the push has succeeded, a failure + /// in a later step is returned inside `Published`, so the caller knows + /// the events are on the remote. + pub(super) fn commit_v3_tracked( + &self, + events: Vec, + _lock: &crate::sync::HubWriteLock, + ) -> Result { let agent_id = self.agent.agent_id.clone(); let remote = self.sync.remote(); let requires_publication = self.sync.remote_exists(); @@ -608,9 +647,26 @@ impl SharedWriter { return Err(error); } + let outcome = if requires_publication { + PushOutcome::Pushed + } else { + PushOutcome::LocalOnly + }; + let after_push_error = self.after_publication().err(); + Ok(Published { + outcome, + after_push_error, + }) + } + + fn after_publication(&self) -> Result<()> { #[cfg(test)] - if self.failpoints.borrow().fail_after_push { - bail!("injected failure after push"); + { + let failpoints = self.failpoints.borrow(); + std::thread::sleep(failpoints.publish_delay); + if failpoints.fail_after_push { + bail!("injected failure after push"); + } } if self.sync.remote_exists() { @@ -619,12 +675,7 @@ impl SharedWriter { self.refresh_v3_state()?; self.write_and_push_v3_checkpoint(); - - Ok(if requires_publication { - PushOutcome::Pushed - } else { - PushOutcome::LocalOnly - }) + Ok(()) } fn write_and_push_v3_checkpoint(&self) { @@ -721,31 +772,71 @@ impl SharedWriter { } } - pub(super) fn confirm_v3_locks(&self) -> Result<()> { + /// The bound on a lock confirmation's hub-lock wait and fetch. + #[cfg_attr(not(test), allow(clippy::unused_self, clippy::missing_const_for_fn))] // test builds read the failpoints + pub(super) fn lock_confirm_deadline(&self) -> Duration { #[cfg(test)] - let fetched = if self.failpoints.borrow().fail_confirm_fetch { - Err(anyhow::anyhow!("injected confirmation fetch failure")) - } else { - self.sync.fetch() - }; - #[cfg(not(test))] - let fetched = self.sync.fetch(); - if let Err(e) = fetched { - tracing::warn!("v3 lock confirm: fetch failed ({e}); confirming against local view"); + if let Some(deadline) = self.failpoints.borrow().confirm_deadline { + return deadline; + } + Duration::from_secs(LOCK_CONFIRM_TIMEOUT_SECS) + } + + /// Confirms lock state against a fresh fetch. The hub-lock wait and the + /// fetch share `deadline`; adopting and reducing the fetched snapshot are + /// local and not bounded. Every failure is returned, never confirmed + /// against the local view. With no remote, the local authority is the + /// authority, and confirmation is a reduce. + /// See `.design/lock-claim-outcome-and-kickoff-rollback.md`. + pub(super) fn confirm_v3_locks_bounded(&self, deadline: Duration) -> Result<()> { + if self.sync.remote_exists() { + let started = std::time::Instant::now(); + let lock = self + .sync + .acquire_lock_within(deadline) + .context("lock confirmation could not take the hub lock")?; + self.run_confirmation_fetch(deadline.saturating_sub(started.elapsed()))?; + self.sync + .adopt_fetched_v3_refs() + .context("lock confirmation could not adopt the fetched refs")?; + drop(lock); } #[cfg(test)] std::thread::sleep(self.failpoints.borrow().slow_confirm_reduce); self.refresh_v3_state() } - /// Confirms lock state against a fresh fetch. The hub-lock wait and the - /// fetch share `deadline`; the reduce of the fetched snapshot is not - /// bounded. Every failure is returned, never confirmed against the local - /// view. See `.design/lock-claim-outcome-and-kickoff-rollback.md`. - #[allow(dead_code)] // scaffold: called by the implementation commit - pub(super) fn confirm_v3_locks_bounded(&self, deadline: Duration) -> Result<()> { - let _ = deadline; - unimplemented!("scaffold: bounded lock confirmation") + fn run_confirmation_fetch(&self, remaining: Duration) -> Result<()> { + let mut command = self.sync.v3_fetch_command(); + #[cfg(test)] + { + let failpoints = self.failpoints.borrow(); + if failpoints.fail_confirm_fetch { + bail!("injected confirmation fetch failure"); + } + if let Some((program, args)) = failpoints + .confirm_fetch_command + .as_ref() + .and_then(|argv| argv.split_first()) + { + command = std::process::Command::new(program); + command.args(args); + } + } + let output = crate::utils::command_output_with_timeout(&mut command, remaining) + .ok_or_else(|| { + anyhow::anyhow!( + "the confirmation fetch did not finish within {}s", + remaining.as_secs() + ) + })?; + if !output.status.success() { + bail!( + "the confirmation fetch failed: {}", + String::from_utf8_lossy(&output.stderr).trim() + ); + } + Ok(()) } pub(super) fn v3_assigned_display_id(&self, uuid: &Uuid) -> Option { diff --git a/crosslink/src/shared_writer/locks.rs b/crosslink/src/shared_writer/locks.rs index f5ef31460..cdc7e091e 100644 --- a/crosslink/src/shared_writer/locks.rs +++ b/crosslink/src/shared_writer/locks.rs @@ -1,6 +1,6 @@ -use anyhow::{bail, Context, Result}; +use anyhow::{Context, Result}; -use super::core::{SharedWriter, LOCK_CONFIRM_TIMEOUT_SECS}; +use super::core::SharedWriter; #[derive(Debug, Clone, PartialEq, Eq)] pub enum LockClaimResult { @@ -29,6 +29,11 @@ impl SharedWriter { self.claim_lock_v2_inner(issue_display_id, branch) } + /// Claims the lock and reports the outcome confirmed against a fresh + /// fetch. Publication time does not count: only the confirmation's + /// hub-lock wait and fetch are bounded. A claim that was published but + /// could not be confirmed is `Unconfirmed`, which is not ownership; a + /// rerun confirms it without publishing a second claim. fn claim_lock_v2_inner( &self, issue_display_id: i64, @@ -36,7 +41,23 @@ impl SharedWriter { ) -> Result { if let Some(lock) = self.read_lock_v2(issue_display_id)? { if lock.agent_id == self.agent.agent_id { - return Ok(LockClaimResult::AlreadyHeld); + if !self.is_v3() { + return Ok(LockClaimResult::AlreadyHeld); + } + if let Err(error) = self.confirm_v3_locks_bounded(self.lock_confirm_deadline()) { + return Ok(unconfirmed(&error)); + } + match self.read_lock_v2(issue_display_id)? { + Some(lock) if lock.agent_id == self.agent.agent_id => { + return Ok(LockClaimResult::AlreadyHeld); + } + Some(lock) => { + return Ok(LockClaimResult::Contended { + winner_agent_id: lock.agent_id, + }); + } + None => {} + } } } @@ -44,22 +65,15 @@ impl SharedWriter { issue_display_id, branch: branch.map(std::string::ToString::to_string), }; - let start = std::time::Instant::now(); - self.emit_compact_push_inner(event, &format!("claim lock on #{issue_display_id}"))?; - let elapsed = start.elapsed(); - #[cfg(test)] - let elapsed = elapsed + self.failpoints.borrow().extra_publish_elapsed; - if elapsed > std::time::Duration::from_secs(LOCK_CONFIRM_TIMEOUT_SECS) { - bail!( - "Lock confirmation timed out after {}s (threshold {}s) -- \ - compaction result may be stale, not treating as authoritative", - elapsed.as_secs(), - LOCK_CONFIRM_TIMEOUT_SECS - ); + let published = self.emit_compact_push_tracked(event)?; + if let Some(error) = published.after_push_error { + return Ok(unconfirmed(&error)); } if self.is_v3() { - self.confirm_v3_locks()?; + if let Err(error) = self.confirm_v3_locks_bounded(self.lock_confirm_deadline()) { + return Ok(unconfirmed(&error)); + } } match self.read_lock_v2(issue_display_id)? { @@ -81,6 +95,36 @@ impl SharedWriter { } } + /// The issues whose locks this agent holds in the local view. + pub fn locks_held_by_self(&self) -> Result> { + if !self.is_v3() { + return Ok(Vec::new()); + } + self.refresh_v3_state()?; + let state = self.last_v3_state.borrow(); + Ok(state.as_ref().map_or_else(Vec::new, |state| { + state + .locks + .iter() + .filter(|(_, entry)| entry.agent_id == self.agent.agent_id) + .map(|(issue, _)| *issue) + .collect() + })) + } + + /// The lock's holder, confirmed against a fresh fetch under the same + /// bound as a claim's confirmation. An `Err` means it could not be + /// confirmed; the local view is never returned in its place. + pub fn confirmed_lock_holder(&self, issue_display_id: i64) -> Result> { + let _permit = self.acquire_mutation_operation_permit()?; + if self.is_v3() { + self.confirm_v3_locks_bounded(self.lock_confirm_deadline())?; + } + Ok(self + .read_lock_v2(issue_display_id)? + .map(|lock| lock.agent_id)) + } + pub fn release_lock_v2(&self, issue_display_id: i64) -> Result { let _permit = self.acquire_mutation_operation_permit()?; match self.read_lock_v2(issue_display_id)? { @@ -178,3 +222,9 @@ impl SharedWriter { Ok(Some(lock)) } } + +fn unconfirmed(error: &anyhow::Error) -> LockClaimResult { + LockClaimResult::Unconfirmed { + cause: format!("{error:#}"), + } +} diff --git a/crosslink/src/sync/cache.rs b/crosslink/src/sync/cache.rs index a1adf377c..2e0e83476 100644 --- a/crosslink/src/sync/cache.rs +++ b/crosslink/src/sync/cache.rs @@ -127,7 +127,10 @@ pub fn acquire_hub_lock(lock_path: &Path) -> Result { acquire_hub_lock_with_timeout(lock_path, Duration::from_secs(30)) } -fn acquire_hub_lock_with_timeout(lock_path: &Path, max_wait: Duration) -> Result { +pub(crate) fn acquire_hub_lock_with_timeout( + lock_path: &Path, + max_wait: Duration, +) -> Result { let poll_interval = Duration::from_millis(100); let start = std::time::Instant::now(); @@ -150,9 +153,10 @@ fn acquire_hub_lock_with_timeout(lock_path: &Path, max_wait: Duration) -> Result if start.elapsed() > max_wait { if holder_alive { bail!( - "hub write lock held by live process for >30s ({}); \ + "hub write lock held by live process for more than {}s ({}); \ waiting aborted to avoid concurrent worktree mutation — \ retry, or remove the lock file if the process is hung: {}", + max_wait.as_secs(), std::fs::read_to_string(lock_path) .ok() .and_then(|c| c.trim().parse::().ok()) @@ -168,7 +172,8 @@ fn acquire_hub_lock_with_timeout(lock_path: &Path, max_wait: Duration) -> Result match try_create_lock(lock_path) { Ok(guard) => return Ok(guard), Err(_) => bail!( - "Hub lock held for >30s and could not be acquired after force-removal" + "Hub lock held for more than {}s and could not be acquired after force-removal", + max_wait.as_secs() ), } } @@ -185,6 +190,12 @@ impl SyncManager { acquire_hub_lock(&lock_path) } + /// Like `acquire_lock`, but gives up after `max_wait`. + pub(crate) fn acquire_lock_within(&self, max_wait: Duration) -> Result { + let lock_path = self.cache_dir.join(".hub-write-lock"); + acquire_hub_lock_with_timeout(&lock_path, max_wait) + } + pub fn init_cache(&self) -> Result<()> { self.migrate_from_locks_branch()?; @@ -546,6 +557,20 @@ impl SyncManager { self.refresh_local_checkpoint(); } + /// The fetch of every agent ref and the checkpoint into the remote-tracking + /// namespace, as a command the caller runs (for example under a deadline). + pub(crate) fn v3_fetch_command(&self) -> std::process::Command { + let mut command = std::process::Command::new("git"); + command.current_dir(&self.cache_dir).args([ + "fetch", + "--quiet", + &self.remote, + "+refs/heads/crosslink/checkpoint:refs/crosslink-remote/checkpoint", + "refs/heads/crosslink/agents/*:refs/crosslink-remote/agents/*", + ]); + command + } + pub(crate) fn fetch_and_adopt_v3_refs(&self) { let fetch_result = self.git_in_cache(&[ "fetch", @@ -556,19 +581,23 @@ impl SyncManager { if fetch_result.is_err() { return; } + if let Err(error) = self.adopt_fetched_v3_refs() { + tracing::warn!("v3 fetch: {error:#}"); + } + } + /// Adopts what a v3 fetch brought into the remote-tracking namespace: + /// every other agent's ref tip and, by frontier, the checkpoint. Local + /// only; never touches this agent's own ref. + pub(crate) fn adopt_fetched_v3_refs(&self) -> Result<()> { let own_agent_id = crate::identity::AgentConfig::load(&self.crosslink_dir) .ok() .flatten() .map(|a| a.agent_id); - let tips = match self.list_remote_agent_tips() { - Ok(t) => t, - Err(e) => { - tracing::warn!("v3 fetch: could not list remote agent tips: {e}"); - return; - } - }; + let tips = self + .list_remote_agent_tips() + .context("could not list remote agent tips")?; for (agent_id, remote_tip) in tips { if own_agent_id.as_deref() == Some(agent_id.as_str()) { continue; @@ -581,6 +610,7 @@ impl SyncManager { } self.adopt_checkpoint_by_frontier(); + Ok(()) } fn list_remote_agent_tips(&self) -> Result> { diff --git a/crosslink/src/utils.rs b/crosslink/src/utils.rs index 36d6863d0..a824b092c 100644 --- a/crosslink/src/utils.rs +++ b/crosslink/src/utils.rs @@ -1,6 +1,7 @@ use chrono::{DateTime, NaiveDate, NaiveDateTime, NaiveTime, Utc}; use std::path::{Path, PathBuf}; -use std::process::Command; +use std::process::{Child, Command, Output, Stdio}; +use std::time::{Duration, Instant}; #[deprecated(note = "use agents::resolve_agent")] pub fn read_agent_binary(crosslink_dir: &Path) -> String { @@ -410,6 +411,69 @@ pub fn parse_due_date(s: &str) -> Result, String> { }) } +/// Runs `command` to completion or until `timeout`, capturing its output. +/// Returns `None` when the command cannot be spawned or runs past `timeout`. +/// +/// On Unix the command runs in its own process group. On timeout the group +/// gets SIGTERM, so git can remove its lock files, then SIGKILL after a short +/// grace period, so no helper (a remote helper, ssh, index-pack) outlives the +/// call. Elsewhere only the direct child is killed. +pub fn command_output_with_timeout(command: &mut Command, timeout: Duration) -> Option { + #[cfg(unix)] + { + use std::os::unix::process::CommandExt; + command.process_group(0); + } + let mut child = command + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .ok()?; + let deadline = Instant::now() + timeout; + loop { + match child.try_wait() { + Ok(Some(_)) => return child.wait_with_output().ok(), + Ok(None) if Instant::now() < deadline => { + std::thread::sleep(Duration::from_millis(20)); + } + Ok(None) | Err(_) => { + terminate_child(&mut child); + return None; + } + } + } +} + +#[cfg(unix)] +fn terminate_child(child: &mut Child) { + const GRACE: Duration = Duration::from_millis(500); + let group = format!("-{}", child.id()); + let _ = Command::new("kill") + .args(["-TERM", "--", &group]) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status(); + let grace_ends = Instant::now() + GRACE; + while Instant::now() < grace_ends { + if matches!(child.try_wait(), Ok(Some(_))) { + break; + } + std::thread::sleep(Duration::from_millis(20)); + } + let _ = Command::new("kill") + .args(["-KILL", "--", &group]) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status(); + let _ = child.wait(); +} + +#[cfg(not(unix))] +fn terminate_child(child: &mut Child) { + let _ = child.kill(); + let _ = child.wait(); +} + #[cfg(test)] mod tests { use super::*; From f60712ed60b0e33f034a0f94d3d72b5b05ba2822 Mon Sep 17 00:00:00 2001 From: magnificentlycursed <190320250+magnificentlycursed@users.noreply.github.com> Date: Thu, 8 Oct 2026 23:26:30 -0700 Subject: [PATCH 04/14] perf(locks): confirm a claim against agent refs without adopting the remote checkpoint Lock confirmation adopted the fetched checkpoint, and verifying a remote checkpoint rebuilds state from every event with a per-event ssh-keygen signature check. On the shared hub that made the confirmation of an already-held lock take 56 s and a new claim 302 s on macOS. The lock table comes from reducing the agent refs; the checkpoint only shortens the replay. Confirmation now adopts other agents' fetched ref tips (adopt_fetched_agent_tips) and reduces them, and leaves checkpoint adoption to the ordinary fetch. A test checks that the confirmation alone finds a claim the checkout never fetched. --- .../src/commands/hub_v3_operation_tests.rs | 24 +++++++++++++++++++ crosslink/src/shared_writer/core.rs | 10 ++++---- crosslink/src/sync/cache.rs | 12 ++++++++-- 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/crosslink/src/commands/hub_v3_operation_tests.rs b/crosslink/src/commands/hub_v3_operation_tests.rs index d84973fd2..19eebb1a8 100644 --- a/crosslink/src/commands/hub_v3_operation_tests.rs +++ b/crosslink/src/commands/hub_v3_operation_tests.rs @@ -1484,3 +1484,27 @@ fn v3_already_held_is_confirmed_against_the_remote() { "expected Unconfirmed, got {result:?}" ); } + +/// The confirmation adopts other agents' ref tips (not the remote +/// checkpoint) and still sees a claim this checkout never fetched. +#[test] +fn v3_confirmation_sees_an_unfetched_claim_without_the_checkpoint() { + if !git_ok() { + return; + } + let hub = setup_migrated_v3_hub(); + let remote = hub.remote.path(); + let (writer_a, issue_id) = writer_with_issue(&hub, "Claimed elsewhere"); + + let (_wb, beta_dir, _bc) = clone_for_agent(remote, "beta"); + let writer_b = SharedWriter::new(&beta_dir).unwrap().unwrap(); + + assert_eq!( + writer_a.claim_lock_v2(issue_id, None).unwrap(), + crate::shared_writer::LockClaimResult::Claimed + ); + assert_eq!( + writer_b.confirmed_lock_holder(issue_id).unwrap(), + Some("alpha".to_string()) + ); +} diff --git a/crosslink/src/shared_writer/core.rs b/crosslink/src/shared_writer/core.rs index 6eb540954..7b970b814 100644 --- a/crosslink/src/shared_writer/core.rs +++ b/crosslink/src/shared_writer/core.rs @@ -783,8 +783,10 @@ impl SharedWriter { } /// Confirms lock state against a fresh fetch. The hub-lock wait and the - /// fetch share `deadline`; adopting and reducing the fetched snapshot are - /// local and not bounded. Every failure is returned, never confirmed + /// fetch share `deadline`; adopting the fetched agent refs and reducing + /// them are local and not bounded. The fetched checkpoint is not adopted: + /// the lock table comes from the agent refs, and verifying a remote + /// checkpoint rebuilds state from every event. Every failure is returned, never confirmed /// against the local view. With no remote, the local authority is the /// authority, and confirmation is a reduce. /// See `.design/lock-claim-outcome-and-kickoff-rollback.md`. @@ -797,8 +799,8 @@ impl SharedWriter { .context("lock confirmation could not take the hub lock")?; self.run_confirmation_fetch(deadline.saturating_sub(started.elapsed()))?; self.sync - .adopt_fetched_v3_refs() - .context("lock confirmation could not adopt the fetched refs")?; + .adopt_fetched_agent_tips() + .context("lock confirmation could not adopt the fetched agent refs")?; drop(lock); } #[cfg(test)] diff --git a/crosslink/src/sync/cache.rs b/crosslink/src/sync/cache.rs index 2e0e83476..c9d4dcbb7 100644 --- a/crosslink/src/sync/cache.rs +++ b/crosslink/src/sync/cache.rs @@ -590,6 +590,16 @@ impl SyncManager { /// every other agent's ref tip and, by frontier, the checkpoint. Local /// only; never touches this agent's own ref. pub(crate) fn adopt_fetched_v3_refs(&self) -> Result<()> { + self.adopt_fetched_agent_tips()?; + self.adopt_checkpoint_by_frontier(); + Ok(()) + } + + /// Adopts every other agent's fetched ref tip, without the checkpoint. + /// A reduce over the adopted tips is complete without it: the checkpoint + /// only shortens the replay, and adopting a remote one means verifying it + /// by rebuilding state from every event. + pub(crate) fn adopt_fetched_agent_tips(&self) -> Result<()> { let own_agent_id = crate::identity::AgentConfig::load(&self.crosslink_dir) .ok() .flatten() @@ -608,8 +618,6 @@ impl SyncManager { tracing::warn!("v3 fetch: failed to adopt ref '{local_ref}': {e}"); } } - - self.adopt_checkpoint_by_frontier(); Ok(()) } From 444f28bf0f45274f5522e750d37835e18e725594 Mon Sep 17 00:00:00 2001 From: magnificentlycursed <190320250+magnificentlycursed@users.noreply.github.com> Date: Fri, 9 Oct 2026 10:56:03 -0700 Subject: [PATCH 05/14] fix(kickoff): undo brings the worktree daemon up before releasing; a stop that finished late is not a failure A manual check of kickoff undo after an activation failure across a sleep showed the lock release and session end failing on the worktree's stale readiness, and the daemon stop reported as failed although the process had exited. Undo now ensures the worktree daemon grants mutations before it releases the lock or ends the session, as cleanup already did, and treats a daemon stop error as done when the daemon is gone within a further 5 s. --- crosslink/src/commands/kickoff/rollback.rs | 49 ++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/crosslink/src/commands/kickoff/rollback.rs b/crosslink/src/commands/kickoff/rollback.rs index 61bd069e3..c50910067 100644 --- a/crosslink/src/commands/kickoff/rollback.rs +++ b/crosslink/src/commands/kickoff/rollback.rs @@ -216,6 +216,21 @@ impl KickoffUndo<'_> { self.worktree_crosslink().join("agent.json").exists() } + /// Brings the worktree's daemon to a state that grants mutations. A + /// failed kickoff can leave the worktree's readiness stale, for example + /// across a sleep, and releasing the lock or ending the session needs it. + fn ensure_ready(&self) -> Result<(), String> { + let crosslink = self.worktree_crosslink(); + if !crate::reconcile::readiness::requires_readiness(&crosslink) { + return Ok(()); + } + match crate::daemon::ensure(&crosslink, true) { + Ok(record) if record.state.grants_mutations() => Ok(()), + Ok(record) => Err(format!("the worktree daemon is {}", record.state.as_str())), + Err(error) => Err(format!("{error:#}")), + } + } + fn release_lock(&self, issue_id: i64) -> UndoOutcome { if !self.has_agent() { return UndoOutcome::NothingToUndo; @@ -224,6 +239,9 @@ impl KickoffUndo<'_> { "run `crosslink locks release {issue_id}` in {}", self.worktree_dir.display() ); + if let Err(error) = self.ensure_ready() { + return UndoOutcome::Failed { error, remedy }; + } let writer = match crate::shared_writer::SharedWriter::new(&self.worktree_crosslink()) { Ok(Some(writer)) => writer, Ok(None) => return UndoOutcome::NothingToUndo, @@ -253,6 +271,15 @@ impl KickoffUndo<'_> { if !self.has_agent() { return UndoOutcome::NothingToUndo; } + if let Err(error) = self.ensure_ready() { + return UndoOutcome::Failed { + error, + remedy: format!( + "run `crosslink session end` in {}", + self.worktree_dir.display() + ), + }; + } command_outcome( Command::new("crosslink") .current_dir(self.worktree_dir) @@ -272,6 +299,11 @@ impl KickoffUndo<'_> { } match crate::daemon::stop(&crosslink) { Ok(()) => UndoOutcome::Done, + // `daemon::stop` allows 2 s after SIGKILL; on a loaded or + // sleeping machine the process can take longer to go. + Err(_) if daemon_gone_within(&crosslink, std::time::Duration::from_secs(5)) => { + UndoOutcome::Done + } Err(error) => UndoOutcome::Failed { error: format!("{error:#}"), remedy: format!( @@ -371,6 +403,23 @@ impl UndoOps for KickoffUndo<'_> { } } +/// Whether the worktree's daemon is gone, or goes within `timeout`. +fn daemon_gone_within(crosslink: &Path, timeout: std::time::Duration) -> bool { + let deadline = std::time::Instant::now() + timeout; + loop { + match crate::reconcile::readiness::read_daemon_identity(crosslink) { + Ok(None) => return true, + Ok(Some(identity)) + if !crate::reconcile::readiness::daemon_identity_is_live(&identity) => + { + return true; + } + _ if std::time::Instant::now() >= deadline => return false, + _ => std::thread::sleep(std::time::Duration::from_millis(250)), + } + } +} + /// Runs a command for an undo step. Output matching one of `absent` means the /// resource was already gone. fn command_outcome(command: &mut Command, remedy: &str, absent: &[&str]) -> UndoOutcome { From 73ed70db196f3a616e6f742b9d2af7f6f4e46c4d Mon Sep 17 00:00:00 2001 From: magnificentlycursed <190320250+magnificentlycursed@users.noreply.github.com> Date: Fri, 9 Oct 2026 11:05:27 -0700 Subject: [PATCH 06/14] fix(daemon): reap a spawned daemon so stopping it from a running parent succeeds The process that spawns a daemon dropped its child handle without waiting on it. When that process is still running as the daemon exits, as kickoff is when its undo stops the worktree daemon, the daemon lingers as a zombie that kill -0 still reports alive, and daemon stop fails with "did not exit after forced shutdown". A background thread now waits on the child, so it is reaped when it exits; a short-lived parent exits first and the daemon is adopted as before. --- crosslink/src/daemon.rs | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/crosslink/src/daemon.rs b/crosslink/src/daemon.rs index b47a81e9c..e3f3adc47 100644 --- a/crosslink/src/daemon.rs +++ b/crosslink/src/daemon.rs @@ -387,9 +387,25 @@ fn ensure_process(crosslink_dir: &Path) -> Result { process_start: readiness::process_start_token_for(child.id())?, }; readiness::write_daemon_identity(crosslink_dir, &identity)?; + reap_when_exited(child); Ok(identity) } +/// Waits for the daemon on a background thread, so that when it exits while +/// the process that started it is still running (kickoff stopping a worktree +/// daemon), it does not linger as a zombie that still looks alive to `stop`. +/// A short-lived parent exits first and the daemon is adopted as before. +fn reap_when_exited(mut child: std::process::Child) { + let spawned = thread::Builder::new() + .name("crosslink-daemon-reaper".to_string()) + .spawn(move || { + let _ = child.wait(); + }); + if let Err(error) = spawned { + tracing::debug!("could not start the daemon reaper thread: {error}"); + } +} + #[cfg(windows)] fn prevent_standard_handle_inheritance() -> Result<()> { use std::ffi::c_void; From 9b848e187c94eae1e7505886e98d3ce13b628ca3 Mon Sep 17 00:00:00 2001 From: magnificentlycursed <190320250+magnificentlycursed@users.noreply.github.com> Date: Fri, 9 Oct 2026 11:31:05 -0700 Subject: [PATCH 07/14] docs(design): record the manual checks of the lock-claim outcome and kickoff rollback Results on this Mac with installed builds of the branch: claims complete without stranding (85 s new, 34 s already held after the confirmation change); kickoff undo after an activation failure leaves nothing behind after the readiness and reaper fixes; cleanup of a stopped agent releases its lock, stops its daemon and removes its worktree and unchanged branch. Out-of-scope observations (the daemon exiting after writes over 90 s) are noted for the later increments. --- .design/lock-claim-outcome-and-kickoff-rollback.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/.design/lock-claim-outcome-and-kickoff-rollback.md b/.design/lock-claim-outcome-and-kickoff-rollback.md index def19bd5a..3cd82cbae 100644 --- a/.design/lock-claim-outcome-and-kickoff-rollback.md +++ b/.design/lock-claim-outcome-and-kickoff-rollback.md @@ -183,6 +183,17 @@ Not covered by automated tests: These are checked by hand on this Mac before merge. +Manual checks, 2026-10-09, macOS, installed builds of this branch: + +- **New claim.** With `dcbe2bd9`, `session work` on an unheld issue succeeded in 302 s, and `locks check` agreed. A stack sample showed the confirmation spending most of its time verifying the fetched remote checkpoint. `signing::verify_content` polls `ssh-keygen` with a 50 ms sleep per event. `f60712ed` confirms against agent refs without adopting the checkpoint. With it, a new claim took 85 s and an already-held lock 34 s. The remaining time is the publication's own cost (the write-cost increment). +- **Activation failure.** The kickoff aimed at an issue another agent held. The undo released, ended the session and removed the worktree and branch, and the other agent's lock was untouched. Two defects were found and fixed: + - the undo now brings stale worktree readiness up before releasing (`444f28bf`); + - the daemon stop reported a zombie as alive, because the spawning kickoff never reaped it; spawned daemons are now reaped (`73ed70db`). + + After the fixes, the undo reported "removed everything this kickoff created". +- **Cleanup of a stopped agent.** A local kickoff was stopped at the trust prompt, and `kickoff cleanup --force` ran on it. It stopped the daemon, released the agent's lock, removed the worktree and deleted the unchanged branch. The issue then read "not locked". +- **Observed, out of scope.** The host daemon exited, as `running: false`, after writes that held permits for more than 90 s: the 130 s cleanup, and the kickoff runs. It also parked once as `blocked_corrupt` after repeated record expiries. These are tracked on #816 and #817, for the heartbeat and write-cost increments. + ## Data and compatibility - **One additive field.** `base_commit` in kickoff metadata, optional, with a default. Metadata without it means "keep the branch". From ea5402a0ecfdb586bdb4a9124933213a5d2e77fe Mon Sep 17 00:00:00 2001 From: magnificentlycursed <190320250+magnificentlycursed@users.noreply.github.com> Date: Fri, 9 Oct 2026 14:03:21 -0700 Subject: [PATCH 08/14] fix(locks,kickoff): address phase 3 review round 1 Correctness: - Lock confirmation fails when any other agent's fetched ref fails to adopt, and on v3 a confirmed view with no lock is Unconfirmed, not ownership. - The confirmation fetch never prompts: no stdin, GIT_TERMINAL_PROMPT=0, and ssh BatchMode appended to the user's ssh command (GIT_SSH is left alone). - command_output_with_timeout drains output on reader threads while waiting and waits at most a second for it once the command has exited. - locks steal and lock auto-steal act on the claim result instead of reporting success; the auto-steal test now asserts the untaken steal that v3 returns until tracker #818 is fixed. - Undo releases the lock through the worktree's command service and classifies the outcome from a bounded confirmation (stranded, unverified or done) whatever the release reported. - The undo report follows the original error in the returned error, so swarm and sentinel keep it. - kickoff cleanup reads the agent's locks from the host's view and wakes the worktree daemon only when there is one to release, stops daemons through the shared undo step, and exits non-zero when it leaves work behind. - On a reused worktree, undo leaves a pre-existing agent, daemon and lock as found (operator decision; AC-9 amended). - Hub-lock errors no longer quote a wait duration. Tests: a grandchild in the fetch's process group and a SIGTERM-cleaned lock file, a real failing fetch, a hydration failure after push, contention found during confirmation (fails until the reducer change that follows), the real undo release (stranded and unverified), pre-existing reuse state, the host key file surviving undo, the pipeline restore through the undo wiring, and daemon reaping. Deadlines in timing tests are 5 s against 6 s delays. Docs: design amendments, coverage and limits restated; CHANGELOG and the --keep-branch guide lines. --- ...lock-claim-outcome-and-kickoff-rollback.md | 57 +++-- CHANGELOG.md | 19 ++ README.md | 2 +- .../agent/skills/crosslink-guide/SKILL.md | 2 +- .../src/commands/hub_v3_operation_tests.rs | 242 ++++++++++++++---- crosslink/src/commands/kickoff/cleanup.rs | 66 ++--- crosslink/src/commands/kickoff/mod.rs | 2 +- crosslink/src/commands/kickoff/plan.rs | 2 +- crosslink/src/commands/kickoff/rollback.rs | 241 +++++++++++++---- crosslink/src/commands/kickoff/run.rs | 75 +++++- crosslink/src/commands/locks_cmd.rs | 18 +- crosslink/src/daemon.rs | 29 +++ crosslink/src/lock_check.rs | 23 +- crosslink/src/shared_writer/core.rs | 15 +- crosslink/src/shared_writer/locks.rs | 22 +- crosslink/src/sync/cache.rs | 78 ++++-- crosslink/src/utils.rs | 39 ++- 17 files changed, 718 insertions(+), 214 deletions(-) diff --git a/.design/lock-claim-outcome-and-kickoff-rollback.md b/.design/lock-claim-outcome-and-kickoff-rollback.md index 3cd82cbae..38efbe602 100644 --- a/.design/lock-claim-outcome-and-kickoff-rollback.md +++ b/.design/lock-claim-outcome-and-kickoff-rollback.md @@ -28,7 +28,7 @@ This increment does three things: Claim outcome: -- REQ-1: A claim's outcome comes from confirmation, never from the elapsed time of publication. Confirmation is a fetch of every agent ref and the checkpoint, followed by a reduce. The elapsed-time `bail!` in `claim_lock_v2_inner` is removed. +- REQ-1: A claim's outcome comes from confirmation, never from the elapsed time of publication. Confirmation fetches every agent ref and the checkpoint, adopts every other agent's ref tip (not the remote checkpoint), and reduces over the local checkpoint. Any tip that fails to adopt fails the confirmation. On v3, a confirmed view that shows no lock is `Unconfirmed`, not ownership. The elapsed-time `bail!` in `claim_lock_v2_inner` is removed. (Amended 2026-10-09; see Amendments.) - REQ-2: Confirmation is fallible. The result is `Unconfirmed { cause }` in three cases: - the confirmation fetch fails; - the hub-lock wait plus the fetch does not finish within 30 s (`LOCK_CONFIRM_TIMEOUT_SECS`); @@ -80,7 +80,7 @@ Tests marked "fails first" must fail on 3440c0d05 and pass after the change. The - [ ] AC-6: another agent's earlier-ordered claim arrives during confirmation. The result is `Contended`, and no active issue is recorded. (REQ-3) - [ ] AC-7 (guard, passes on base): `set_session_issue` fails after a confirmed claim. A `LockReleased` follows, and the hub shows no lock. (REQ-5) - [ ] AC-8 (fails first for activation): this is a fake-runtime test for each failure point: activation, missing preflight, tmux start, container start, and early container exit. Afterwards the issue is unlocked, the worktree directory and branch are gone, the worktree daemon is not running, no container or tmux session remains, and the pipeline state equals its prior value. This holds for both `run` and `plan`. (REQ-6) -- [ ] AC-9: in a reuse case (`--branch` with an existing worktree), activation fails. The lock is released, but the worktree and branch remain. (REQ-6) +- [ ] AC-9: in a reuse case (`--branch` with an existing worktree), activation fails. The worktree and branch remain. A lock, session and daemon that existed before this invocation are left as found; a lock or daemon this invocation created is undone. (REQ-6; amended 2026-10-09 by operator decision) - [ ] AC-10: a launch succeeds. Afterwards the lock is held by the worktree agent, and the worktree, branch and daemon remain. (REQ-7) - [ ] AC-11: `kickoff cleanup --force` runs on a stopped agent whose daemon is not running. Afterwards the issue is unlocked, the session ended, the daemon stopped, and the worktree removed. A branch with no commits beyond `base_commit` is deleted. A branch with a commit, any branch under `--keep-branch`, and any branch whose metadata has no `base_commit` remain. (REQ-8, REQ-10) - [ ] AC-12: the lock release does not reach the remote (the remote is rejecting pushes), and confirmation still shows the worktree agent holding the lock. The output names the stranded lock, and the remaining steps run. With the remote unreachable, the output says "release unverified". (REQ-9) @@ -148,7 +148,7 @@ The build starts from a scaffold commit containing the new `LockClaimResult` var - **An unconfirmed claim is not ownership.** hub-v3 REQ-6(b) forbids a state in which two agents both verify ownership. Treating `Unconfirmed` as held, or confirming against a stale local view, could create one. Rejected: "held with a warning", which the first draft of this design proposed; it was withdrawn after the precedent check. - **Undo is an explicit `unwind`, not `Drop`.** It runs subprocesses, can fail and must report. `Drop` can do none of that visibly, and it would also run during a panic. -- **Release under the worktree agent's identity.** Running the release inside the worktree keeps the signed log truthful. Rejected: signing from the host with the worktree agent's key. +- **Release under the worktree agent's identity.** The release runs with the worktree's `.crosslink` as the crosslink directory, so the event is signed as the worktree agent and the signed log stays truthful. Built in-process, it is the same host process and key file a subprocess would use. Rejected: releasing as the host or driver identity, or through a writer built on the host's `.crosslink`. - **The deadline bounds only the network.** The hub-lock wait and the fetch are bounded; the local reduce is not. A slow reduce of a fresh snapshot is still a correct confirmation, and bounding it would make claims fail on slow machines until the write-cost increment lands. Rejected: bounding the whole confirmation, which round 2 of the review showed would turn today's slow success into a permanent failure. - **The 30 s value stays; what it measures changes.** Raising it, as Corvidae-Coding-Projects/crosslink#103 did, would treat write cost, and that is the write-cost increment's job. - **Identity and trust approval are left in place.** Revoking them would make the agent's published events unverifiable. @@ -161,27 +161,54 @@ Dated 2026-10-08, made while building on `feat/lock-claim-outcome`: - **AC-4** checks the bound against the hung fetch (under 20 s), not "within 4 s". Publication and the local read before confirmation are outside the bound. - **Undo and cleanup release the lock in-process.** They use a `SharedWriter` built on the worktree's `.crosslink`, not a `crosslink locks release` subprocess. The signer is the same worktree agent. Running in-process lets the release be checked by `SharedWriter::confirmed_lock_holder`, the bounded confirmation, which a subprocess exit code cannot provide. - **`session end` runs without notes.** With notes it would post a comment on the issue, an extra hub write for every failed kickoff. -- **Cleanup releases every lock the worktree agent holds** (`SharedWriter::locks_held_by_self`), because cleanup does not know the agent's issue. +- **Cleanup releases every lock the worktree agent holds**, because cleanup does not know the agent's issue. Amended 2026-10-09: it reads them from the host's view of the hub, and brings the worktree daemon up only when there is one to release. + +Dated 2026-10-09, after the manual checks and the phase 3 review (three reviewers, recorded on #820): + +- **Confirmation adopts agent tips, not the remote checkpoint** (`f60712ed`). Verifying a remote checkpoint rebuilds state from every event with an `ssh-keygen` check per event, and a claim took 302 s. Skipping it keeps the lock table correct: each agent's history is validated from sequence 1, so a missing or pruned event fails the reduce instead of hiding a claim, and a stale local checkpoint only lengthens the replay. REQ-1 is restated to match. +- **Undo brings the worktree daemon up before releasing** (`444f28bf`). Readiness can be stale by the time undo runs, for example across a sleep. +- **Spawned daemons are reaped** (`73ed70db`). A daemon stopped by the process that started it lingered as a zombie, and `daemon stop` reported it alive. +- **Review fixes:** + - a tip that fails to adopt fails the confirmation; + - a confirmed "no lock" is `Unconfirmed` on v3; + - the confirmation fetch never prompts (no stdin, `GIT_TERMINAL_PROMPT=0`, ssh `BatchMode=yes` appended to the user's ssh command); + - the timeout helper drains output while waiting and waits at most a second for it after exit; + - `locks steal` and auto-steal act on the claim result; + - the undo release goes through the worktree's command service and is classified from confirmation (`Stranded`, `Unverified` or done) whatever the release reported; + - the undo report follows the original error in the returned error, so swarm and sentinel keep it; + - cleanup stops daemons through the shared undo step and exits non-zero when it leaves work behind; + - on a reused worktree, undo leaves what pre-existed (AC-9). +- **Permit hold time.** A claim now holds its mutation permit through confirmation: up to 30 s more for the hub-lock wait and fetch, plus one reduce. Writes that hold permits past 90 s let the readiness record expire and the daemon exit (#816, #817), so this narrows that margin on slow machines until the write-cost and heartbeat increments land. Where each criterion is covered: -- **Claim (AC-1 to AC-6):** tests in `crosslink/src/commands/hub_v3_operation_tests.rs` against real v3 hubs with a bare remote. AC-6 is the existing `v3_lock_claim_confirm_winner_and_loser`. -- **Undo and cleanup:** unit tests in `crosslink/src/commands/kickoff/rollback.rs` and `cleanup.rs`, covering: +- **Claim:** tests in `crosslink/src/commands/hub_v3_operation_tests.rs`, against real v3 hubs with a bare remote. + - **AC-1:** deadline-relative. It does not catch a literal revert of the old bail against the fixed 30 s, which only review would. + - **AC-2 and AC-5:** a failpoint, plus a real failing fetch. + - **AC-3:** after-push and hydration failures. + - **AC-4:** the grandchild in the fetch's process group and the SIGTERM lock-file cleanup. + - **AC-4a.** + - **AC-6:** contention found during confirmation, on the already-held path. + - **AC-12:** the real release on a refused push (`Stranded`) and an unreachable remote (`Unverified`), Unix only. +- **Undo and cleanup:** unit tests in `crosslink/src/commands/kickoff/rollback.rs` and `cleanup.rs`: - the step order for each failure point; - - reuse; + - reuse, including pre-existing state; - disarming; - failure reporting; - the branch rule; - - the pipeline restore; - - the real git operations on a temporary repository. + - the pipeline restore through `undo_failed_kickoff`; + - the host key file surviving undo (AC-13); + - git operations on a temporary repository. +- **Daemon reaping:** `daemon::tests::a_spawned_child_is_reaped_when_it_exits`. Not covered by automated tests: -- the `session work` message for an unconfirmed claim (AC-2's command-level half); -- AC-7; -- AC-8 and AC-11 end to end, which need the `crosslink` binary, tmux and a container runtime. - -These are checked by hand on this Mac before merge. +- **The `session work` message for an unconfirmed claim** (AC-2's command-level half). Not checked by hand either. +- **AC-7.** The base revision already releases on that path; not checked by hand. +- **AC-8 end to end,** which needs the `crosslink` binary, tmux and a container runtime. Checked by hand for the activation failure point on `run` only. Kickoff's preflight runs the image first, so a missing image fails before any undo. +- **AC-10 and AC-11 end to end.** AC-11's cleanup flow was checked by hand. `--keep-branch` is untested. +- **The undo's readiness wake-up.** It would start a daemon, so no unit test runs it. +- **Windows:** the AC-4 and AC-12 tests are Unix only, and the Windows CI shards do not run the v3 operation suite. Manual checks, 2026-10-09, macOS, installed builds of this branch: @@ -215,7 +242,7 @@ Manual checks, 2026-10-09, macOS, installed builds of this branch: ## Verification - **Tests:** - - `cargo test --manifest-path crosslink/Cargo.toml --lib shared_writer::tests`, `lock_check::tests` and `commands::kickoff::tests`; + - `cargo test --manifest-path crosslink/Cargo.toml --bin crosslink -- hub_v3_operation_tests kickoff::rollback kickoff::cleanup daemon::tests::a_spawned_child lock_check sync:: utils::` (the new tests live in these modules); - `cargo test --manifest-path crosslink/Cargo.toml --test smoke coordination`. - **Before every commit:** `cargo fmt --check` and `cargo clippy -- -D warnings -W clippy::unwrap_used -W clippy::expect_used`. - **Full suite:** CI only. diff --git a/CHANGELOG.md b/CHANGELOG.md index b2213bb50..71dabaff5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -107,6 +107,25 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Fixed +- A lock claim is no longer reported as failed because its write took more + than 30 s, which left the lock on the hub while `session work` recorded + nothing (gh#110, gh#118). The outcome now comes from a confirmation against + a fresh fetch, bounded at 30 s for the hub-lock wait and the fetch. A claim + that was published but could not be confirmed makes `session work` and + `locks claim` exit non-zero with "claim published but not confirmed"; the + rerun confirms it without publishing a second claim. An already-held lock is + confirmed against the remote, not the local view. +- `kickoff run` and `kickoff plan` undo what they created when they fail + before the agent starts: the container or tmux session, the issue lock, the + worktree session and daemon, the pipeline state, and the worktree and branch + they created (gh#110). Anything left is reported after the error, with the + command that clears it. +- `kickoff cleanup` releases the agent's locks, then removes the worktree and + deletes its branch when the branch has no commits beyond the base recorded + at kickoff; `--keep-branch` keeps it. Cleanup exits non-zero when it leaves + work behind. +- `locks steal` and lock auto-steal no longer report success when the steal + did not take. - `crosslink container build` no longer fails with a cryptic `COPY crosslink-: not found` (gh#75). It staged the binary as `crosslink` while the Dockerfile expects `crosslink-${TARGETARCH}`; it now diff --git a/README.md b/README.md index 9b29f089d..90596bf88 100644 --- a/README.md +++ b/README.md @@ -78,7 +78,7 @@ Launch multiple agents and let them coordinate automatically. - `kickoff plan` — Read-only gap analysis against the codebase before committing to a build - `kickoff report` — Spec validation reports from completed agents - `kickoff graph` — Visual branch topology of active kickoff branches - - `kickoff list` / `kickoff cleanup` — Monitor and manage running agents + - `kickoff list` / `kickoff cleanup` — Monitor and manage running agents; cleanup releases the agent's locks and deletes branches with no new commits (`--keep-branch` keeps them) - **`crosslink swarm`** — Multi-agent phased builds from design documents - `swarm init --doc` — Decompose a design document into phases and work units - `swarm launch` / `swarm gate` / `swarm checkpoint` — Execute, gate, and record phase progress diff --git a/crosslink/resources/agent/skills/crosslink-guide/SKILL.md b/crosslink/resources/agent/skills/crosslink-guide/SKILL.md index ccc695939..7b8de64e1 100644 --- a/crosslink/resources/agent/skills/crosslink-guide/SKILL.md +++ b/crosslink/resources/agent/skills/crosslink-guide/SKILL.md @@ -86,7 +86,7 @@ crosslink kickoff status crosslink kickoff logs crosslink kickoff report crosslink kickoff stop -crosslink kickoff cleanup +crosslink kickoff cleanup # releases locks; --keep-branch keeps unchanged branches crosslink swarm init crosslink swarm status crosslink swarm launch diff --git a/crosslink/src/commands/hub_v3_operation_tests.rs b/crosslink/src/commands/hub_v3_operation_tests.rs index 19eebb1a8..e17a7f3ab 100644 --- a/crosslink/src/commands/hub_v3_operation_tests.rs +++ b/crosslink/src/commands/hub_v3_operation_tests.rs @@ -1260,13 +1260,11 @@ fn v3_to_shared_promotes_sqlite_only_rows() { } // Lock-claim outcome: `.design/lock-claim-outcome-and-kickoff-rollback.md`. -// AC-6 (contention found during confirmation) is covered by -// `v3_lock_claim_confirm_winner_and_loser` above. -fn lock_claims_on_agent_ref(cache_dir: &Path, agent_id: &str, issue_id: i64) -> usize { +fn lock_claims_on_agent_ref(repo: &Path, agent_id: &str, issue_id: i64) -> usize { let reference = agent_ref_name(agent_id).unwrap(); let out = Command::new("git") - .current_dir(cache_dir) + .current_dir(repo) .args(["show", &format!("{reference}:events.log")]) .output() .unwrap(); @@ -1293,8 +1291,17 @@ fn writer_with_issue(hub: &V3Hub, title: &str) -> (SharedWriter, i64) { (writer, issue_id) } -/// AC-1 (fails first): publication time never counts against the -/// confirmation deadline, so a slow publication still confirms the claim. +fn is_unconfirmed(result: &crate::shared_writer::LockClaimResult) -> bool { + matches!( + result, + crate::shared_writer::LockClaimResult::Unconfirmed { .. } + ) +} + +/// AC-1: publication time never counts against the confirmation deadline, +/// so a publication slower than the deadline still confirms the claim. It +/// does not catch a literal revert of the old bail against the fixed 30 s, +/// which publication here does not reach. #[test] fn v3_slow_publication_still_confirms_the_claim() { if !git_ok() { @@ -1304,21 +1311,18 @@ fn v3_slow_publication_still_confirms_the_claim() { let (writer, issue_id) = writer_with_issue(&hub, "Slow publication"); { let mut failpoints = writer.failpoints.borrow_mut(); - failpoints.confirm_deadline = Some(std::time::Duration::from_secs(1)); - failpoints.publish_delay = std::time::Duration::from_secs(2); + failpoints.confirm_deadline = Some(std::time::Duration::from_secs(5)); + failpoints.publish_delay = std::time::Duration::from_secs(6); } - let result = writer.claim_lock_v2(issue_id, None); assert_eq!( - result.unwrap(), + writer.claim_lock_v2(issue_id, None).unwrap(), crate::shared_writer::LockClaimResult::Claimed ); - let cold = SharedWriter::new(&hub.crosslink_dir).unwrap().unwrap(); assert_eq!( - cold.read_lock_v2(issue_id) - .unwrap() - .map(|lock| lock.agent_id), - Some("alpha".to_string()) + lock_claims_on_agent_ref(hub.remote.path(), "alpha", issue_id), + 1, + "the remote must hold the claim" ); } @@ -1335,10 +1339,7 @@ fn v3_failed_confirmation_is_unconfirmed_and_the_rerun_confirms() { let first = writer.claim_lock_v2(issue_id, None).unwrap(); assert!( - matches!( - first, - crate::shared_writer::LockClaimResult::Unconfirmed { .. } - ), + is_unconfirmed(&first), "expected Unconfirmed, got {first:?}" ); @@ -1348,11 +1349,34 @@ fn v3_failed_confirmation_is_unconfirmed_and_the_rerun_confirms() { crate::shared_writer::LockClaimResult::AlreadyHeld ); assert_eq!( - lock_claims_on_agent_ref(&hub.cache_dir, "alpha", issue_id), - 1 + lock_claims_on_agent_ref(hub.remote.path(), "alpha", issue_id), + 1, + "the rerun must not publish a second claim" ); } +/// AC-2 and AC-5 through a real failing fetch, not a failpoint: a fetch that +/// exits non-zero is `Unconfirmed`, for a new claim and for an already-held +/// lock. +#[test] +fn v3_a_failing_confirmation_fetch_is_unconfirmed() { + if !git_ok() { + return; + } + let hub = setup_migrated_v3_hub(); + let (writer, issue_id) = writer_with_issue(&hub, "Failing fetch"); + writer.failpoints.borrow_mut().confirm_fetch_command = Some(vec![ + "git".to_string(), + "fetch".to_string(), + "--quiet".to_string(), + "/nonexistent-crosslink-remote".to_string(), + ]); + let first = writer.claim_lock_v2(issue_id, None).unwrap(); + assert!(is_unconfirmed(&first), "new claim: got {first:?}"); + let again = writer.claim_lock_v2(issue_id, None).unwrap(); + assert!(is_unconfirmed(&again), "already held: got {again:?}"); +} + /// AC-3 (fails first): a failure after the push lands is `Unconfirmed`, not /// a plain error. #[test] @@ -1366,22 +1390,37 @@ fn v3_failure_after_push_is_unconfirmed() { let result = writer.claim_lock_v2(issue_id, None); assert!( - matches!( - result, - Ok(crate::shared_writer::LockClaimResult::Unconfirmed { .. }) - ), + matches!(result, Ok(ref r) if is_unconfirmed(r)), "expected Ok(Unconfirmed), got {result:?}" ); } -/// AC-4 (fails first): the confirmation fetch is bounded and killed with its -/// process group, and leaves nothing that blocks a later fetch. The timed call +/// AC-3: a hydration failure after the push is `Unconfirmed` too. +#[test] +fn v3_hydration_failure_after_push_is_unconfirmed() { + if !git_ok() { + return; + } + let hub = setup_migrated_v3_hub(); + let (writer, issue_id) = writer_with_issue(&hub, "Hydration failure"); + writer.failpoints.borrow_mut().fail_hydration = true; + + let result = writer.claim_lock_v2(issue_id, None); + assert!( + matches!(result, Ok(ref r) if is_unconfirmed(r)), + "expected Ok(Unconfirmed), got {result:?}" + ); +} + +/// AC-4 (fails first): the confirmation fetch is bounded, and the whole +/// process group is terminated: the marker is on a grandchild, and the +/// fake fetch holds a lock file it removes only on SIGTERM. The timed call /// is the already-held path, so publication time is not measured; the bound /// is checked against the hung fetch's 30 s, because the local read before /// confirmation also reduces. #[cfg(unix)] #[test] -fn v3_confirmation_fetch_is_bounded_and_cleaned_up() { +fn v3_confirmation_fetch_is_bounded_and_its_group_terminated() { if !git_ok() { return; } @@ -1392,23 +1431,22 @@ fn v3_confirmation_fetch_is_bounded_and_cleaned_up() { crate::shared_writer::LockClaimResult::Claimed ); let marker = format!("crosslink-confirm-deadline-{}", std::process::id()); + let lock_file = hub.work.path().join("fake-fetch.lock"); + let script = format!( + "touch '{lock}'; trap 'rm -f \"{lock}\"; exit 143' TERM; \ + sh -c 'sleep 30; : {marker}' & wait", + lock = lock_file.display() + ); { let mut failpoints = writer.failpoints.borrow_mut(); failpoints.confirm_deadline = Some(std::time::Duration::from_secs(2)); - failpoints.confirm_fetch_command = Some(vec![ - "sh".to_string(), - "-c".to_string(), - format!("sleep 30; : {marker}"), - ]); + failpoints.confirm_fetch_command = Some(vec!["sh".to_string(), "-c".to_string(), script]); } let started = std::time::Instant::now(); let result = writer.claim_lock_v2(issue_id, None).unwrap(); assert!( - matches!( - result, - crate::shared_writer::LockClaimResult::Unconfirmed { .. } - ), + is_unconfirmed(&result), "expected Unconfirmed, got {result:?}" ); assert!( @@ -1422,15 +1460,15 @@ fn v3_confirmation_fetch_is_bounded_and_cleaned_up() { .unwrap(); assert!( !leftover.status.success(), - "fetch process group survived: {}", + "a grandchild in the fetch's process group survived: {}", String::from_utf8_lossy(&leftover.stdout) ); + assert!( + !lock_file.exists(), + "the fetch was not given SIGTERM to clean up its lock file" + ); *writer.failpoints.borrow_mut() = crate::shared_writer::core::ClaimFailpoints::default(); - SyncManager::new(&hub.crosslink_dir) - .unwrap() - .fetch() - .unwrap(); assert_eq!( writer.claim_lock_v2(issue_id, None).unwrap(), crate::shared_writer::LockClaimResult::AlreadyHeld @@ -1448,17 +1486,13 @@ fn v3_slow_reduce_after_the_fetch_still_confirms() { let (writer, issue_id) = writer_with_issue(&hub, "Slow reduce"); { let mut failpoints = writer.failpoints.borrow_mut(); - failpoints.confirm_deadline = Some(std::time::Duration::from_secs(1)); - failpoints.slow_confirm_reduce = std::time::Duration::from_secs(2); + failpoints.confirm_deadline = Some(std::time::Duration::from_secs(5)); + failpoints.slow_confirm_reduce = std::time::Duration::from_secs(6); } assert_eq!( writer.claim_lock_v2(issue_id, None).unwrap(), crate::shared_writer::LockClaimResult::Claimed ); - assert_eq!( - writer.claim_lock_v2(issue_id, None).unwrap(), - crate::shared_writer::LockClaimResult::AlreadyHeld - ); } /// AC-5 (fails first): an already-held lock is confirmed against the remote, @@ -1477,14 +1511,51 @@ fn v3_already_held_is_confirmed_against_the_remote() { writer.failpoints.borrow_mut().fail_confirm_fetch = true; let result = writer.claim_lock_v2(issue_id, None).unwrap(); assert!( - matches!( - result, - crate::shared_writer::LockClaimResult::Unconfirmed { .. } - ), + is_unconfirmed(&result), "expected Unconfirmed, got {result:?}" ); } +/// AC-6: the local view shows this agent holding the lock, but another +/// agent's earlier-ordered claim is on the remote. Confirmation finds it and +/// the result is `Contended`, never a second owner. +#[test] +fn v3_contention_found_during_confirmation_is_contended() { + if !git_ok() { + return; + } + let hub = setup_migrated_v3_hub(); + let remote = hub.remote.path(); + let (writer_a, issue_id) = writer_with_issue(&hub, "Claimed first elsewhere"); + + let (_wb, beta_dir, _bc) = clone_for_agent(remote, "beta"); + SyncManager::new(&beta_dir).unwrap().fetch().unwrap(); + let writer_b = SharedWriter::new(&beta_dir).unwrap().unwrap(); + assert_eq!( + writer_b.claim_lock_v2(issue_id, None).unwrap(), + crate::shared_writer::LockClaimResult::Claimed + ); + + { + let mut failpoints = writer_a.failpoints.borrow_mut(); + failpoints.skip_adopt_after_push = true; + failpoints.fail_confirm_fetch = true; + } + let first = writer_a.claim_lock_v2(issue_id, None).unwrap(); + assert!( + is_unconfirmed(&first), + "expected Unconfirmed, got {first:?}" + ); + + writer_a.failpoints.borrow_mut().fail_confirm_fetch = false; + match writer_a.claim_lock_v2(issue_id, None).unwrap() { + crate::shared_writer::LockClaimResult::Contended { winner_agent_id } => { + assert_eq!(winner_agent_id, "beta"); + } + other => panic!("expected Contended by beta, got {other:?}"), + } +} + /// The confirmation adopts other agents' ref tips (not the remote /// checkpoint) and still sees a claim this checkout never fetched. #[test] @@ -1508,3 +1579,70 @@ fn v3_confirmation_sees_an_unfetched_claim_without_the_checkpoint() { Some("alpha".to_string()) ); } + +/// AC-12: undo's release through the real worktree service, classified from +/// confirmation. A push the remote refuses leaves the lock stranded; an +/// unreachable remote leaves the release unverified. +#[cfg(unix)] +#[test] +fn v3_undo_release_reports_stranded_and_unverified() { + use crate::commands::kickoff::rollback::{KickoffUndo, UndoOps, UndoOutcome, UndoStep}; + + if !git_ok() { + return; + } + let is_root = Command::new("id") + .arg("-u") + .output() + .is_ok_and(|o| String::from_utf8_lossy(&o.stdout).trim() == "0"); + if is_root { + // Root ignores directory permissions, so a refused push cannot be staged. + return; + } + let hub = setup_migrated_v3_hub(); + let (writer, issue_id) = writer_with_issue(&hub, "Undo release"); + assert_eq!( + writer.claim_lock_v2(issue_id, None).unwrap(), + crate::shared_writer::LockClaimResult::Claimed + ); + drop(writer); + let worktree_dir = hub.work.path().to_path_buf(); + let step = UndoStep::ReleaseLock { issue_id }; + + let objects = hub.remote.path().join("objects"); + let refs = hub.remote.path().join("refs"); + let set_mode = |mode: u32| { + for dir in [&objects, &refs] { + Command::new("chmod") + .args(["-R", &format!("{mode:o}"), dir.to_str().unwrap()]) + .status() + .unwrap(); + } + }; + set_mode(0o555); + let stranded = KickoffUndo { + repo_root: &worktree_dir, + worktree_dir: &worktree_dir, + prior_pipeline: None, + } + .run(&step); + set_mode(0o755); + assert!( + matches!(stranded, UndoOutcome::Stranded { .. }), + "refused push: got {stranded:?}" + ); + + let parked = hub.work.path().join("remote-parked"); + std::fs::rename(hub.remote.path(), &parked).unwrap(); + let unverified = KickoffUndo { + repo_root: &worktree_dir, + worktree_dir: &worktree_dir, + prior_pipeline: None, + } + .run(&step); + std::fs::rename(&parked, hub.remote.path()).unwrap(); + assert!( + matches!(unverified, UndoOutcome::Unverified { .. }), + "unreachable remote: got {unverified:?}" + ); +} diff --git a/crosslink/src/commands/kickoff/cleanup.rs b/crosslink/src/commands/kickoff/cleanup.rs index 42102488e..b01fe509f 100644 --- a/crosslink/src/commands/kickoff/cleanup.rs +++ b/crosslink/src/commands/kickoff/cleanup.rs @@ -5,7 +5,10 @@ use std::process::Command; use super::helpers::*; use super::monitor::discover_agents; -use super::rollback::{outcome_message, KickoffUndo, UndoOps, UndoOutcome, UndoStep}; +use super::rollback::{ + host_locks_held_by, outcome_message, worktree_agent_id, KickoffUndo, UndoOps, UndoOutcome, + UndoStep, +}; use super::types::*; #[derive(Debug, Clone, Copy, Default)] @@ -276,11 +279,15 @@ pub fn cleanup(crosslink_dir: &Path, opts: &CleanupOptions) -> Result<()> { } if !agent.worktree.is_empty() && std::path::Path::new(&agent.worktree).exists() { - let worktree_crosslink = std::path::Path::new(&agent.worktree).join(".crosslink"); - if crate::reconcile::readiness::requires_readiness(&worktree_crosslink) { - if let Err(error) = crate::daemon::stop(&worktree_crosslink) { - tracing::warn!("could not stop the worktree daemon before removal: {error}"); - } + let step = UndoStep::StopDaemon; + let outcome = KickoffUndo { + repo_root, + worktree_dir: worktree_path, + prior_pipeline: None, + } + .run(&step); + if let Some(message) = outcome_message(&step, &outcome) { + result.warnings.push(message); } match Command::new("git") .args(["worktree", "remove", "--force", &agent.worktree]) @@ -364,46 +371,41 @@ pub fn cleanup(crosslink_dir: &Path, opts: &CleanupOptions) -> Result<()> { println!("."); } + let left_behind = results + .iter() + .filter(|r| r.error.is_some() || !r.warnings.is_empty()) + .count(); + if left_behind > 0 { + anyhow::bail!( + "cleanup left work behind for {left_behind} agent(s); see the warnings and errors above" + ); + } Ok(()) } -/// Brings the worktree's daemon up so the agent's locks can be released and -/// its session ended under its own identity. Anything left is a warning -/// with its remedy. +/// Releases the agent's locks and ends its session under its own identity. +/// The host's view says which locks the agent holds; the worktree's daemon is +/// brought up only when there is one to release (its session is local to the +/// worktree and goes with it). Anything left is a warning with its remedy. fn release_agent_work(repo_root: &Path, worktree: &Path, result: &mut CleanupResult) { let worktree_crosslink = worktree.join(".crosslink"); - if !worktree_crosslink.join("agent.json").exists() { + let Some(agent_id) = worktree_agent_id(&worktree_crosslink) else { return; - } - if crate::reconcile::readiness::requires_readiness(&worktree_crosslink) { - let ready = crate::daemon::ensure(&worktree_crosslink, true) - .map(|record| record.state.grants_mutations()); - if !matches!(ready, Ok(true)) { - result.warnings.push(format!( - "the worktree daemon did not become ready, so the agent's locks and session \ - were left; to clear them, run `crosslink locks list` and \ - `crosslink locks release ` in {}", - worktree.display() - )); - return; - } - } - let held = match crate::shared_writer::SharedWriter::new(&worktree_crosslink) { - Ok(Some(writer)) => writer.locks_held_by_self(), - Ok(None) => Ok(Vec::new()), - Err(error) => Err(error), }; - let held = match held { + let held = match host_locks_held_by(&repo_root.join(".crosslink"), &agent_id) { Ok(held) => held, Err(error) => { result.warnings.push(format!( - "could not read the agent's locks: {error:#}; to clear them, run \ - `crosslink locks release ` in {}", + "could not read which locks '{agent_id}' holds: {error:#}; to clear them, run \ + `crosslink locks list` and `crosslink locks release ` in {}", worktree.display() )); - Vec::new() + return; } }; + if held.is_empty() { + return; + } let mut ops = KickoffUndo { repo_root, worktree_dir: worktree, diff --git a/crosslink/src/commands/kickoff/mod.rs b/crosslink/src/commands/kickoff/mod.rs index 93febc52f..1713dda9d 100644 --- a/crosslink/src/commands/kickoff/mod.rs +++ b/crosslink/src/commands/kickoff/mod.rs @@ -6,7 +6,7 @@ mod monitor; pub(crate) mod pipeline; mod plan; mod prompt; -mod rollback; +pub(crate) mod rollback; mod run; mod types; mod wizard; diff --git a/crosslink/src/commands/kickoff/plan.rs b/crosslink/src/commands/kickoff/plan.rs index 4a393b78d..5be5206b8 100644 --- a/crosslink/src/commands/kickoff/plan.rs +++ b/crosslink/src/commands/kickoff/plan.rs @@ -340,7 +340,7 @@ pub fn plan(crosslink_dir: &Path, db: &impl QueryService, opts: &PlanOpts) -> Re Ok(()) })(); - launched.inspect_err(|_| super::run::undo_failed_kickoff(rollback, &root, &worktree_dir)) + launched.map_err(|error| super::run::with_undo_report(error, rollback, &root, &worktree_dir)) } pub fn show_plan(crosslink_dir: &Path, agent: &str) -> Result<()> { diff --git a/crosslink/src/commands/kickoff/rollback.rs b/crosslink/src/commands/kickoff/rollback.rs index c50910067..c90c6ab7f 100644 --- a/crosslink/src/commands/kickoff/rollback.rs +++ b/crosslink/src/commands/kickoff/rollback.rs @@ -6,19 +6,21 @@ use std::path::{Path, PathBuf}; use std::process::Command; +use anyhow::Result; + use super::pipeline::PipelineState; /// Whether this kickoff invocation created a resource or found it in place. /// Only created resources are removed. #[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub(super) enum Origin { +pub(crate) enum Origin { Created, Reused, } /// One undo action, in the order `KickoffRollback::steps` returns them. #[derive(Debug, Clone, PartialEq, Eq)] -pub(super) enum UndoStep { +pub(crate) enum UndoStep { RemoveContainer { runtime: String, name: String, @@ -45,7 +47,7 @@ pub(super) enum UndoStep { } #[derive(Debug, Clone, PartialEq, Eq)] -pub(super) enum UndoOutcome { +pub(crate) enum UndoOutcome { Done, NothingToUndo, Failed { @@ -64,13 +66,13 @@ pub(super) enum UndoOutcome { } #[derive(Debug, Default)] -pub(super) struct UndoReport { - pub(super) entries: Vec<(UndoStep, UndoOutcome)>, +pub(crate) struct UndoReport { + pub(crate) entries: Vec<(UndoStep, UndoOutcome)>, } impl UndoReport { /// True when every step either completed or had nothing to undo. - pub(super) fn is_clean(&self) -> bool { + pub(crate) fn is_clean(&self) -> bool { self.entries .iter() .all(|(_, outcome)| matches!(outcome, UndoOutcome::Done | UndoOutcome::NothingToUndo)) @@ -79,18 +81,25 @@ impl UndoReport { /// Performs one undo step. The kickoff implementation runs subprocesses; /// tests record the calls. -pub(super) trait UndoOps { +pub(crate) trait UndoOps { fn run(&mut self, step: &UndoStep) -> UndoOutcome; } /// What a kickoff invocation has created so far. Record each resource as it /// is created; call `disarm` once the agent has started. #[derive(Debug, Default)] -pub(super) struct KickoffRollback { +pub(crate) struct KickoffRollback { worktree: Option<(PathBuf, Origin)>, branch: Option<(String, Origin, Option)>, agent_initialized: bool, + /// The worktree already had an agent (a reused worktree): its session is + /// not this invocation's to end. + agent_preexisting: bool, + /// The worktree's daemon was already running before this invocation. + daemon_preexisting: bool, claimed_issue: Option, + /// The worktree agent already held the issue before this invocation. + claim_preexisting: bool, pipeline: Option<(PathBuf, Option)>, tmux_session: Option, container: Option<(String, String)>, @@ -98,51 +107,67 @@ pub(super) struct KickoffRollback { } impl KickoffRollback { - pub(super) fn new() -> Self { + pub(crate) fn new() -> Self { Self::default() } - pub(super) fn record_worktree(&mut self, path: PathBuf, origin: Origin) { + pub(crate) fn record_worktree(&mut self, path: PathBuf, origin: Origin) { self.worktree = Some((path, origin)); } - pub(super) fn record_branch(&mut self, name: String, origin: Origin, base: Option) { + pub(crate) fn record_branch(&mut self, name: String, origin: Origin, base: Option) { self.branch = Some((name, origin, base)); } - /// The worktree has an agent, a daemon and a session. - pub(super) const fn record_agent(&mut self) { + /// The worktree has an agent, a daemon and a session, all created by + /// this invocation. + pub(crate) const fn record_agent(&mut self) { + self.agent_initialized = true; + } + + /// A reused worktree already had an agent; `daemon_was_live` says whether + /// its daemon was already running. Undo leaves what pre-existed as found. + pub(crate) const fn record_existing_agent(&mut self, daemon_was_live: bool) { self.agent_initialized = true; + self.agent_preexisting = true; + self.daemon_preexisting = daemon_was_live; } /// Record before invoking `session work`: a claim that fails to confirm /// may still have been published. Releasing a lock this agent does not /// hold is ignored by the reducer. - pub(super) const fn record_claim(&mut self, issue_id: i64) { + pub(crate) const fn record_claim(&mut self, issue_id: i64) { self.claimed_issue = Some(issue_id); } + /// The worktree agent already held the issue before this invocation, so + /// undo does not release it. + pub(crate) const fn record_existing_claim(&mut self, issue_id: i64) { + self.claimed_issue = Some(issue_id); + self.claim_preexisting = true; + } + /// Record the pipeline state as it was before `mark_running` or /// `mark_planning` changed it. - pub(super) fn record_pipeline(&mut self, doc_path: PathBuf, prior: Option) { + pub(crate) fn record_pipeline(&mut self, doc_path: PathBuf, prior: Option) { self.pipeline = Some((doc_path, prior)); } - pub(super) fn record_tmux_session(&mut self, name: String) { + pub(crate) fn record_tmux_session(&mut self, name: String) { self.tmux_session = Some(name); } - pub(super) fn record_container(&mut self, runtime: String, name: String) { + pub(crate) fn record_container(&mut self, runtime: String, name: String) { self.container = Some((runtime, name)); } /// The agent has started: from here on, its resources are its own. - pub(super) const fn disarm(&mut self) { + pub(crate) const fn disarm(&mut self) { self.disarmed = true; } /// The undo steps for what was recorded, in the order they must run. - pub(super) fn steps(&self) -> Vec { + pub(crate) fn steps(&self) -> Vec { if self.disarmed { return Vec::new(); } @@ -157,11 +182,15 @@ impl KickoffRollback { steps.push(UndoStep::KillTmuxSession { name: name.clone() }); } if self.agent_initialized { - if let Some(issue_id) = self.claimed_issue { + if let Some(issue_id) = self.claimed_issue.filter(|_| !self.claim_preexisting) { steps.push(UndoStep::ReleaseLock { issue_id }); } - steps.push(UndoStep::EndSession); - steps.push(UndoStep::StopDaemon); + if !self.agent_preexisting { + steps.push(UndoStep::EndSession); + } + if !self.daemon_preexisting { + steps.push(UndoStep::StopDaemon); + } } if let Some((doc_path, _)) = &self.pipeline { steps.push(UndoStep::RestorePipeline { @@ -182,12 +211,12 @@ impl KickoffRollback { /// The pipeline state recorded before kickoff changed it, for the /// `RestorePipeline` step. - pub(super) fn prior_pipeline(&self) -> Option<&PipelineState> { + pub(crate) fn prior_pipeline(&self) -> Option<&PipelineState> { self.pipeline.as_ref().and_then(|(_, prior)| prior.as_ref()) } /// Runs every step, continuing past failures, and reports each outcome. - pub(super) fn unwind(self, ops: &mut impl UndoOps) -> UndoReport { + pub(crate) fn unwind(self, ops: &mut impl UndoOps) -> UndoReport { let entries = self .steps() .into_iter() @@ -201,10 +230,10 @@ impl KickoffRollback { } /// Runs undo steps against the system for one kickoff worktree. -pub(super) struct KickoffUndo<'a> { - pub(super) repo_root: &'a Path, - pub(super) worktree_dir: &'a Path, - pub(super) prior_pipeline: Option, +pub(crate) struct KickoffUndo<'a> { + pub(crate) repo_root: &'a Path, + pub(crate) worktree_dir: &'a Path, + pub(crate) prior_pipeline: Option, } impl KickoffUndo<'_> { @@ -231,6 +260,10 @@ impl KickoffUndo<'_> { } } + /// Releases the lock through the worktree's command service, under the + /// worktree agent's identity, then classifies the outcome from a bounded + /// confirmation: a release can land although its command failed + /// afterwards, or fail although nothing reported it. fn release_lock(&self, issue_id: i64) -> UndoOutcome { if !self.has_agent() { return UndoOutcome::NothingToUndo; @@ -242,27 +275,30 @@ impl KickoffUndo<'_> { if let Err(error) = self.ensure_ready() { return UndoOutcome::Failed { error, remedy }; } - let writer = match crate::shared_writer::SharedWriter::new(&self.worktree_crosslink()) { + let crosslink = self.worktree_crosslink(); + let release = crate::db::Database::open(&crosslink.join("issues.db")).and_then(|db| { + let service = crate::application::RepositoryService::new(&db, &crosslink)?; + crate::lock_check::try_release_lock(&service, issue_id) + }); + let release_note = release.err().map_or_else(String::new, |error| { + format!(" (the release reported: {error:#})") + }); + let writer = match crate::shared_writer::SharedWriter::new(&crosslink) { Ok(Some(writer)) => writer, Ok(None) => return UndoOutcome::NothingToUndo, Err(error) => { - return UndoOutcome::Failed { - error: format!("{error:#}"), - remedy, + return UndoOutcome::Unverified { + remedy: format!("{remedy}{release_note}; confirmation unavailable: {error:#}"), } } }; - if let Err(error) = writer.release_lock_v2(issue_id) { - return UndoOutcome::Failed { - error: format!("{error:#}"), - remedy, - }; - } match writer.confirmed_lock_holder(issue_id) { - Ok(Some(holder)) if holder == writer.agent_id() => UndoOutcome::Stranded { remedy }, + Ok(Some(holder)) if holder == writer.agent_id() => UndoOutcome::Stranded { + remedy: format!("{remedy}{release_note}"), + }, Ok(_) => UndoOutcome::Done, Err(error) => UndoOutcome::Unverified { - remedy: format!("{remedy} (the release could not be confirmed: {error:#})"), + remedy: format!("{remedy}{release_note}; confirmation failed: {error:#}"), }, } } @@ -403,6 +439,36 @@ impl UndoOps for KickoffUndo<'_> { } } +/// Whether the worktree's daemon is running now. +pub(crate) fn worktree_daemon_is_live(worktree_crosslink: &Path) -> bool { + crate::reconcile::readiness::read_daemon_identity(worktree_crosslink) + .ok() + .flatten() + .is_some_and(|identity| crate::reconcile::readiness::daemon_identity_is_live(&identity)) +} + +/// The worktree agent's id, from its `agent.json`. +pub(crate) fn worktree_agent_id(worktree_crosslink: &Path) -> Option { + crate::identity::AgentConfig::load(worktree_crosslink) + .ok() + .flatten() + .map(|agent| agent.agent_id) +} + +/// The issues the host's view of the hub shows `agent_id` holding. A read: +/// it needs no worktree daemon. +pub(crate) fn host_locks_held_by(host_crosslink: &Path, agent_id: &str) -> Result> { + let locks = crate::sync::SyncManager::new(host_crosslink)?.read_locks_auto()?; + let mut held: Vec = locks + .locks + .iter() + .filter(|(_, lock)| lock.agent_id == agent_id) + .map(|(issue, _)| *issue) + .collect(); + held.sort_unstable(); + Ok(held) +} + /// Whether the worktree's daemon is gone, or goes within `timeout`. fn daemon_gone_within(crosslink: &Path, timeout: std::time::Duration) -> bool { let deadline = std::time::Instant::now() + timeout; @@ -444,25 +510,31 @@ fn command_outcome(command: &mut Command, remedy: &str, absent: &[&str]) -> Undo } impl UndoReport { - /// Prints every step that did not complete cleanly, with its remedy, and - /// one summary line. - pub(super) fn print(&self) { - for (step, outcome) in &self.entries { - if let Some(message) = outcome_message(step, outcome) { - eprintln!("kickoff undo: {message}"); - } + /// The report for the user: every step that did not complete cleanly, + /// with its remedy, and one summary line. `None` when there were no + /// steps. + pub(crate) fn render(&self) -> Option { + if self.entries.is_empty() { + return None; } - if self.is_clean() { - eprintln!("kickoff undo: removed everything this kickoff created"); + let mut lines: Vec = self + .entries + .iter() + .filter_map(|(step, outcome)| outcome_message(step, outcome)) + .map(|message| format!("kickoff undo: {message}")) + .collect(); + lines.push(if self.is_clean() { + "kickoff undo: removed everything this kickoff created".to_string() } else { - eprintln!("kickoff undo: some steps did not complete; see above"); - } + "kickoff undo: some steps did not complete; see above".to_string() + }); + Some(lines.join("\n")) } } /// What to tell the user about a step that did not complete cleanly, or /// `None` when it did. -pub(super) fn outcome_message(step: &UndoStep, outcome: &UndoOutcome) -> Option { +pub(crate) fn outcome_message(step: &UndoStep, outcome: &UndoOutcome) -> Option { match outcome { UndoOutcome::Done | UndoOutcome::NothingToUndo => None, UndoOutcome::Failed { error, remedy } => Some(format!( @@ -474,7 +546,7 @@ pub(super) fn outcome_message(step: &UndoStep, outcome: &UndoOutcome) -> Option< step.describe() )), UndoOutcome::Unverified { remedy } => Some(format!( - "{} could not be verified; to check it, {remedy}", + "{}: release unverified; to check it, {remedy}", step.describe() )), } @@ -499,7 +571,7 @@ impl UndoStep { /// A branch may be deleted only when a base was recorded and the branch has /// no commits beyond it. -pub(super) fn branch_deletable(tip: &str, base_commit: Option<&str>) -> bool { +pub(crate) fn branch_deletable(tip: &str, base_commit: Option<&str>) -> bool { base_commit.is_some_and(|base| base == tip) } @@ -612,6 +684,28 @@ mod tests { assert_eq!(rollback.steps(), tail_steps()); } + /// AC-9: on a reused worktree, what already existed (the agent and its + /// session, a running daemon, a lock the agent already held) is left as + /// found. + #[test] + fn reused_worktree_leaves_existing_agent_daemon_and_lock_alone() { + let mut rollback = KickoffRollback::new(); + rollback.record_worktree(PathBuf::from("/wt/feature-x"), Origin::Reused); + rollback.record_branch("feature/feature-x".to_string(), Origin::Reused, None); + rollback.record_existing_agent(true); + rollback.record_existing_claim(7); + assert!(rollback.steps().is_empty()); + + let mut started_daemon = KickoffRollback::new(); + started_daemon.record_worktree(PathBuf::from("/wt/feature-x"), Origin::Reused); + started_daemon.record_existing_agent(false); + started_daemon.record_claim(7); + assert_eq!( + started_daemon.steps(), + vec![UndoStep::ReleaseLock { issue_id: 7 }, UndoStep::StopDaemon] + ); + } + /// AC-11 (unit level): without a recorded base, the branch is kept. #[test] fn branch_without_a_recorded_base_is_kept() { @@ -743,6 +837,45 @@ mod tests { assert!(nothing.contains(&UndoStep::EndSession)); } + /// AC-13: undo never touches the host-side identity: the agent's key + /// file in the host `.crosslink/keys` survives. + #[test] + fn undo_leaves_the_host_key_file() { + let (_dir, root, worktree, base) = repo_with_worktree(); + let keys = root.join(".crosslink").join("keys"); + std::fs::create_dir_all(&keys).expect("keys dir"); + let key = keys.join("agent_ed25519"); + std::fs::write(&key, "key").expect("key"); + let mut rollback = KickoffRollback::new(); + rollback.record_worktree(worktree.clone(), Origin::Created); + rollback.record_branch("feature/x".to_string(), Origin::Created, Some(base)); + rollback.record_agent(); + rollback.record_claim(7); + let report = super::super::run::undo_failed_kickoff(rollback, &root, &worktree); + assert!(report.is_some()); + assert!(!worktree.exists()); + assert!(key.exists(), "undo removed the host key file"); + } + + /// The wiring carries the recorded prior pipeline state into the undo. + #[test] + fn undo_failed_kickoff_restores_the_prior_pipeline_state() { + let dir = tempfile::tempdir().expect("tempdir"); + let doc = dir.path().join("feature.md"); + std::fs::write(&doc, "# Feature\n").expect("doc"); + let prior = super::super::pipeline::create_initial_pipeline(&doc).expect("pipeline"); + let mut rollback = KickoffRollback::new(); + rollback.record_pipeline(doc.clone(), Some(prior.clone())); + super::super::pipeline::mark_running(&doc, "agent", "/wt", Some(7)).expect("running"); + + let report = super::super::run::undo_failed_kickoff(rollback, dir.path(), dir.path()) + .expect("report"); + assert!(report.contains("removed everything"), "{report}"); + let restored = super::super::pipeline::read_pipeline_state(&doc).expect("restored"); + assert_eq!(restored.stage, prior.stage); + assert!(restored.runs.is_empty()); + } + /// AC-11 (operations): a branch with its own commits is kept. #[test] fn undo_keeps_a_branch_with_its_own_commits() { diff --git a/crosslink/src/commands/kickoff/run.rs b/crosslink/src/commands/kickoff/run.rs index d13ac16c4..21333ffec 100644 --- a/crosslink/src/commands/kickoff/run.rs +++ b/crosslink/src/commands/kickoff/run.rs @@ -7,7 +7,10 @@ use crate::db::Database; use super::helpers::*; use super::launch::*; use super::prompt::*; -use super::rollback::{KickoffRollback, KickoffUndo, Origin}; +use super::rollback::{ + host_locks_held_by, worktree_agent_id, worktree_daemon_is_live, KickoffRollback, KickoffUndo, + Origin, +}; use super::types::*; pub(crate) fn resolve_kickoff_issue( @@ -196,8 +199,13 @@ pub fn run(crosslink_dir: &Path, db: &Database, opts: &KickoffOpts) -> Result Result anyhow::Error { + match undo_failed_kickoff(rollback, root, worktree_dir) { + Some(report) => anyhow::anyhow!("{error:#}\n{report}"), + None => error, + } +} + +/// Undoes what a failed kickoff created; returns the report, if any step ran. +pub(super) fn undo_failed_kickoff( + rollback: KickoffRollback, + root: &Path, + worktree_dir: &Path, +) -> Option { let mut ops = KickoffUndo { repo_root: root, worktree_dir, prior_pipeline: rollback.prior_pipeline().cloned(), }; - let report = rollback.unwind(&mut ops); - if !report.entries.is_empty() { - report.print(); + rollback.unwind(&mut ops).render() +} + +/// Records the agent, daemon and claim that activation is about to set up. +/// On a reused worktree, whatever already existed (the agent and its session, +/// a running daemon, a lock the agent already held) is recorded as such, so +/// undo leaves it as found. +pub(super) fn record_activation( + rollback: &mut KickoffRollback, + reused: bool, + host_crosslink: &Path, + worktree_dir: &Path, + issue_id: i64, +) { + let worktree_crosslink = worktree_dir.join(".crosslink"); + let existing_agent = if reused { + worktree_agent_id(&worktree_crosslink) + } else { + None + }; + let Some(agent_id) = existing_agent else { + rollback.record_agent(); + rollback.record_claim(issue_id); + return; + }; + rollback.record_existing_agent(worktree_daemon_is_live(&worktree_crosslink)); + let already_held = + host_locks_held_by(host_crosslink, &agent_id).is_ok_and(|held| held.contains(&issue_id)); + if already_held { + rollback.record_existing_claim(issue_id); + } else { + rollback.record_claim(issue_id); } } diff --git a/crosslink/src/commands/locks_cmd.rs b/crosslink/src/commands/locks_cmd.rs index efa36796b..169117e2c 100644 --- a/crosslink/src/commands/locks_cmd.rs +++ b/crosslink/src/commands/locks_cmd.rs @@ -212,7 +212,23 @@ pub fn steal(commands: &impl CommandService, crosslink_dir: &Path, issue_id: i64 ); } - commands.steal_lock(issue_id, &existing.agent_id, None)?; + use crate::shared_writer::LockClaimResult; + match commands.steal_lock(issue_id, &existing.agent_id, None)? { + LockClaimResult::Claimed | LockClaimResult::AlreadyHeld => {} + LockClaimResult::Contended { winner_agent_id } => { + anyhow::bail!( + "Could not steal the lock on issue {}: it is still held by '{winner_agent_id}'", + format_issue_id(issue_id) + ); + } + LockClaimResult::Unconfirmed { cause } => { + anyhow::bail!( + "Lock steal on issue {} published but not confirmed: {cause}; \ + run `crosslink locks check {issue_id}` to see who holds it", + format_issue_id(issue_id) + ); + } + } println!( "Stole lock on issue {} from '{}'", format_issue_id(issue_id), diff --git a/crosslink/src/daemon.rs b/crosslink/src/daemon.rs index e3f3adc47..fdd0feb11 100644 --- a/crosslink/src/daemon.rs +++ b/crosslink/src/daemon.rs @@ -1208,6 +1208,35 @@ fn kill_process_force(pid: u32) -> Result<()> { #[cfg(test)] mod tests { use super::*; + + /// A daemon spawned by a process that keeps running is reaped when it + /// exits, instead of lingering as a zombie that still looks alive. + #[cfg(unix)] + #[test] + fn a_spawned_child_is_reaped_when_it_exits() { + let child = Command::new("sh") + .args(["-c", "exit 0"]) + .spawn() + .expect("spawn"); + let pid = child.id(); + reap_when_exited(child); + let deadline = Instant::now() + Duration::from_secs(5); + loop { + let state = Command::new("ps") + .args(["-o", "stat=", "-p", &pid.to_string()]) + .output() + .expect("ps"); + let state = String::from_utf8_lossy(&state.stdout).trim().to_string(); + if state.is_empty() { + return; + } + assert!( + Instant::now() < deadline, + "child {pid} was not reaped (state {state})" + ); + thread::sleep(Duration::from_millis(50)); + } + } use crate::sync::SyncManager; use std::sync::{mpsc, Barrier}; diff --git a/crosslink/src/lock_check.rs b/crosslink/src/lock_check.rs index 9c46d6776..308a09f2d 100644 --- a/crosslink/src/lock_check.rs +++ b/crosslink/src/lock_check.rs @@ -103,7 +103,17 @@ fn auto_steal_if_configured( if sync.hub_mode().is_v3() { { - service.steal_lock(issue_id, stale_agent_id, None)?; + match service.steal_lock(issue_id, stale_agent_id, None)? { + crate::shared_writer::LockClaimResult::Claimed + | crate::shared_writer::LockClaimResult::AlreadyHeld => {} + other => { + tracing::warn!( + "auto-steal of the lock on {} from '{stale_agent_id}' did not take: {other:?}", + crate::utils::format_issue_id(issue_id) + ); + return Ok(false); + } + } let comment = format!( "[auto-steal] Lock auto-stolen from agent '{stale_agent_id}' (stale for {stale_minutes} min, threshold: {auto_steal_threshold} min)" ); @@ -913,8 +923,12 @@ mod tests { assert!(!result.unwrap()); } + /// Until tracker #818 lands, a v3 steal does not take: the reducer ignores + /// another agent's claim on a held lock, so the steal comes back + /// `Contended`. Auto-steal must report that, not success. When #818 is + /// fixed this test fails and should assert `Ok(true)` again. #[test] - fn test_auto_steal_stale_lock_is_stolen_via_v3() { + fn test_auto_steal_on_v3_reports_a_steal_that_did_not_take() { let Some((_dir, crosslink_dir)) = seed_v3_hub("agent-self", 55, Some("other-agent"), false) else { return; @@ -929,7 +943,10 @@ mod tests { let service = RepositoryService::new(&db, &crosslink_dir).unwrap(); let result = auto_steal_if_configured(&crosslink_dir, 55, "other-agent", &service); - assert!(matches!(result, Ok(true)), "expected steal, got {result:?}"); + assert!( + matches!(result, Ok(false)), + "expected the untaken steal to be reported (#818), got {result:?}" + ); } #[test] diff --git a/crosslink/src/shared_writer/core.rs b/crosslink/src/shared_writer/core.rs index 7b970b814..b1c4322e1 100644 --- a/crosslink/src/shared_writer/core.rs +++ b/crosslink/src/shared_writer/core.rs @@ -40,6 +40,11 @@ pub(crate) struct ClaimFailpoints { pub(crate) fail_confirm_fetch: bool, /// Fails the first step after a successful push. pub(crate) fail_after_push: bool, + /// Skips adopting other agents' refs after a successful push, so the + /// local view can miss a claim that only confirmation would find. + pub(crate) skip_adopt_after_push: bool, + /// Fails hydration after a successful push. + pub(crate) fail_hydration: bool, /// Overrides the confirmation deadline. pub(crate) confirm_deadline: Option, /// Replaces the confirmation fetch command (program and arguments). @@ -297,6 +302,10 @@ impl SharedWriter { let lock_guard = self.sync.acquire_lock()?; let mut published = self.commit_v3_tracked(vec![event], &lock_guard)?; + #[cfg(test)] + if published.after_push_error.is_none() && self.failpoints.borrow().fail_hydration { + published.after_push_error = Some(anyhow::anyhow!("injected hydration failure")); + } if published.after_push_error.is_none() { published.after_push_error = Database::open(&self.readiness_dir.join("issues.db")) .and_then(|db| { @@ -669,7 +678,11 @@ impl SharedWriter { } } - if self.sync.remote_exists() { + #[cfg(test)] + let skip_adopt = self.failpoints.borrow().skip_adopt_after_push; + #[cfg(not(test))] + let skip_adopt = false; + if self.sync.remote_exists() && !skip_adopt { self.sync.fetch_and_adopt_v3_refs(); } diff --git a/crosslink/src/shared_writer/locks.rs b/crosslink/src/shared_writer/locks.rs index cdc7e091e..8ce8773f1 100644 --- a/crosslink/src/shared_writer/locks.rs +++ b/crosslink/src/shared_writer/locks.rs @@ -91,27 +91,15 @@ impl SharedWriter { winner_agent_id: lock.agent_id, }) } + // On v3 the confirmed view must show the claim; "no lock" is not + // ownership (for example, the reducer dropped the event). + None if self.is_v3() => Ok(LockClaimResult::Unconfirmed { + cause: "the claim is not visible after confirmation".to_string(), + }), None => Ok(LockClaimResult::Claimed), } } - /// The issues whose locks this agent holds in the local view. - pub fn locks_held_by_self(&self) -> Result> { - if !self.is_v3() { - return Ok(Vec::new()); - } - self.refresh_v3_state()?; - let state = self.last_v3_state.borrow(); - Ok(state.as_ref().map_or_else(Vec::new, |state| { - state - .locks - .iter() - .filter(|(_, entry)| entry.agent_id == self.agent.agent_id) - .map(|(issue, _)| *issue) - .collect() - })) - } - /// The lock's holder, confirmed against a fresh fetch under the same /// bound as a claim's confirmation. An `Err` means it could not be /// confirmed; the local view is never returned in its place. diff --git a/crosslink/src/sync/cache.rs b/crosslink/src/sync/cache.rs index c9d4dcbb7..816f98be9 100644 --- a/crosslink/src/sync/cache.rs +++ b/crosslink/src/sync/cache.rs @@ -153,10 +153,9 @@ pub(crate) fn acquire_hub_lock_with_timeout( if start.elapsed() > max_wait { if holder_alive { bail!( - "hub write lock held by live process for more than {}s ({}); \ + "hub write lock still held by a live process after waiting ({}); \ waiting aborted to avoid concurrent worktree mutation — \ retry, or remove the lock file if the process is hung: {}", - max_wait.as_secs(), std::fs::read_to_string(lock_path) .ok() .and_then(|c| c.trim().parse::().ok()) @@ -171,10 +170,9 @@ pub(crate) fn acquire_hub_lock_with_timeout( let _ = std::fs::remove_file(lock_path); match try_create_lock(lock_path) { Ok(guard) => return Ok(guard), - Err(_) => bail!( - "Hub lock held for more than {}s and could not be acquired after force-removal", - max_wait.as_secs() - ), + Err(_) => { + bail!("Hub lock could not be acquired after waiting and force-removal") + } } } std::thread::sleep(poll_interval); @@ -558,16 +556,36 @@ impl SyncManager { } /// The fetch of every agent ref and the checkpoint into the remote-tracking - /// namespace, as a command the caller runs (for example under a deadline). + /// namespace, as a command the caller runs under a deadline. It never + /// prompts: it may run in its own process group, where a credential or + /// passphrase prompt would stop it until the deadline. Missing credentials + /// fail fast with git's own message instead. pub(crate) fn v3_fetch_command(&self) -> std::process::Command { let mut command = std::process::Command::new("git"); - command.current_dir(&self.cache_dir).args([ - "fetch", - "--quiet", - &self.remote, - "+refs/heads/crosslink/checkpoint:refs/crosslink-remote/checkpoint", - "refs/heads/crosslink/agents/*:refs/crosslink-remote/agents/*", - ]); + command + .current_dir(&self.cache_dir) + .stdin(std::process::Stdio::null()) + .env("GIT_TERMINAL_PROMPT", "0") + .args([ + "fetch", + "--quiet", + &self.remote, + "+refs/heads/crosslink/checkpoint:refs/crosslink-remote/checkpoint", + "refs/heads/crosslink/agents/*:refs/crosslink-remote/agents/*", + ]); + if std::env::var_os("GIT_SSH").is_none() { + let ssh = std::env::var("GIT_SSH_COMMAND") + .ok() + .filter(|value| !value.trim().is_empty()) + .or_else(|| { + self.git_in_cache(&["config", "--get", "core.sshCommand"]) + .ok() + .map(|output| String::from_utf8_lossy(&output.stdout).trim().to_string()) + .filter(|value| !value.is_empty()) + }) + .unwrap_or_else(|| "ssh".to_string()); + command.env("GIT_SSH_COMMAND", format!("{ssh} -o BatchMode=yes")); + } command } @@ -588,18 +606,33 @@ impl SyncManager { /// Adopts what a v3 fetch brought into the remote-tracking namespace: /// every other agent's ref tip and, by frontier, the checkpoint. Local - /// only; never touches this agent's own ref. + /// only; never touches this agent's own ref. A tip that fails to adopt is + /// logged and skipped. pub(crate) fn adopt_fetched_v3_refs(&self) -> Result<()> { - self.adopt_fetched_agent_tips()?; + for failure in self.adopt_tips()? { + tracing::warn!("v3 fetch: {failure}"); + } self.adopt_checkpoint_by_frontier(); Ok(()) } - /// Adopts every other agent's fetched ref tip, without the checkpoint. - /// A reduce over the adopted tips is complete without it: the checkpoint - /// only shortens the replay, and adopting a remote one means verifying it - /// by rebuilding state from every event. + /// Adopts every other agent's fetched ref tip, without the checkpoint, + /// and fails if any tip could not be adopted: a reduce over a stale tip + /// could miss that agent's earlier claim. A reduce over the adopted tips + /// is complete without the checkpoint, which only shortens the replay + /// (each agent's history is validated from sequence 1), and adopting a + /// remote checkpoint means verifying it by rebuilding state from every + /// event. pub(crate) fn adopt_fetched_agent_tips(&self) -> Result<()> { + let failures = self.adopt_tips()?; + if !failures.is_empty() { + bail!("{}", failures.join("; ")); + } + Ok(()) + } + + /// Adopts each other agent's fetched tip; returns the failures. + fn adopt_tips(&self) -> Result> { let own_agent_id = crate::identity::AgentConfig::load(&self.crosslink_dir) .ok() .flatten() @@ -608,6 +641,7 @@ impl SyncManager { let tips = self .list_remote_agent_tips() .context("could not list remote agent tips")?; + let mut failures = Vec::new(); for (agent_id, remote_tip) in tips { if own_agent_id.as_deref() == Some(agent_id.as_str()) { continue; @@ -615,10 +649,10 @@ impl SyncManager { let local_ref = format!("{}{agent_id}", crate::hub_v3::AGENT_REF_PREFIX); if let Err(e) = self.git_in_cache(&["update-ref", &local_ref, &remote_tip]) { - tracing::warn!("v3 fetch: failed to adopt ref '{local_ref}': {e}"); + failures.push(format!("failed to adopt ref '{local_ref}': {e}")); } } - Ok(()) + Ok(failures) } fn list_remote_agent_tips(&self) -> Result> { diff --git a/crosslink/src/utils.rs b/crosslink/src/utils.rs index a824b092c..b61e1fa42 100644 --- a/crosslink/src/utils.rs +++ b/crosslink/src/utils.rs @@ -414,6 +414,11 @@ pub fn parse_due_date(s: &str) -> Result, String> { /// Runs `command` to completion or until `timeout`, capturing its output. /// Returns `None` when the command cannot be spawned or runs past `timeout`. /// +/// Output is drained on reader threads while the command runs, so a command +/// that writes more than a pipe buffer does not stall. Once the command has +/// exited, its output is awaited for at most a second more: a process it +/// left behind holding a pipe open cannot extend the call. +/// /// On Unix the command runs in its own process group. On timeout the group /// gets SIGTERM, so git can remove its lock files, then SIGKILL after a short /// grace period, so no helper (a remote helper, ssh, index-pack) outlives the @@ -430,9 +435,11 @@ pub fn command_output_with_timeout(command: &mut Command, timeout: Duration) -> .spawn() .ok()?; let deadline = Instant::now() + timeout; - loop { + let stdout = drain(child.stdout.take()); + let stderr = drain(child.stderr.take()); + let status = loop { match child.try_wait() { - Ok(Some(_)) => return child.wait_with_output().ok(), + Ok(Some(status)) => break status, Ok(None) if Instant::now() < deadline => { std::thread::sleep(Duration::from_millis(20)); } @@ -441,7 +448,33 @@ pub fn command_output_with_timeout(command: &mut Command, timeout: Duration) -> return None; } } - } + }; + // The command has exited. A process it left behind (an ssh + // ControlPersist master, say) may still hold a pipe open; wait briefly for + // the output, then return without it rather than kill that process. + let wait = deadline + .saturating_duration_since(Instant::now()) + .min(Duration::from_secs(1)); + let stdout = stdout.recv_timeout(wait).unwrap_or_default(); + let stderr = stderr.recv_timeout(wait).unwrap_or_default(); + Some(Output { + status, + stdout, + stderr, + }) +} + +/// Reads a pipe to its end on a background thread. +fn drain(pipe: Option) -> std::sync::mpsc::Receiver> { + let (sender, receiver) = std::sync::mpsc::channel(); + std::thread::spawn(move || { + let mut buffer = Vec::new(); + if let Some(mut pipe) = pipe { + let _ = pipe.read_to_end(&mut buffer); + } + let _ = sender.send(buffer); + }); + receiver } #[cfg(unix)] From 9b6a59ed0a5611c99a7df2e4977b260ce5cb9c16 Mon Sep 17 00:00:00 2001 From: magnificentlycursed <190320250+magnificentlycursed@users.noreply.github.com> Date: Fri, 9 Oct 2026 14:49:39 -0700 Subject: [PATCH 09/14] fix(compaction): rebuild in total order when an earlier event arrives late A reduce applied unseen events on top of the checkpoint. When another agent's event ordered before an event the checkpoint already covered (it was written without it, for example after a transient fetch failure), it was applied out of order: a lock claim lost to a later claim already in the checkpoint, and display ids were allocated in a different order. Clients reducing from different checkpoints disagreed about who held a lock, and a checkpoint written from that state then failed verification against the total-order reconstruction for every reader. When the earliest unseen event orders before the latest covered event, reduce now replays every event after the authority baseline in total order (rebuild_from_authority, sharing the histories already validated). The cost is one full replay, only on a late arrival. A property test checks that a reduce from any per-agent checkpoint cut, with the remaining events arriving late, equals a full replay (three agents, contended locks, releases by non-holders, timestamp ties, inversions and skew; 256 cases), with a deterministic regression for the case found by the lock contention test. Both fail with the rule disabled. The phase-2 per-agent selection test now expects the replay on a late arrival. --- crosslink/src/compaction.rs | 151 +++++++++++++++++++++++++++++++++++- 1 file changed, 148 insertions(+), 3 deletions(-) diff --git a/crosslink/src/compaction.rs b/crosslink/src/compaction.rs index 6b89f8337..9877ac1fc 100644 --- a/crosslink/src/compaction.rs +++ b/crosslink/src/compaction.rs @@ -186,6 +186,17 @@ pub fn reduce(source: &dyn HubSource) -> Result { all_events.sort_by_cached_key(OrderingKey::from_envelope); + // Applying unseen events on top of the checkpoint equals a replay in + // total order only if none of them orders before an event the checkpoint + // already covers. A late arrival (another agent's earlier event that this + // checkpoint was written without) would otherwise be applied out of order: + // a lock claim would lose to a later claim already in the checkpoint, and + // display ids would be allocated in a different order, so clients reducing + // from different checkpoints would disagree. Rebuild in total order then. + if arrives_late(&histories, &state.frontier, all_events.first()) { + return rebuild_with_histories(source, &histories); + } + let events_processed = all_events.len(); let mut changed_issues: HashSet = HashSet::new(); let mut changed_locks: HashSet = HashSet::new(); @@ -233,10 +244,43 @@ pub(crate) fn reduce_legacy_compatible(source: &dyn HubSource) -> Result, + frontier: &CausalFrontier, + earliest_unseen: Option<&EventEnvelope>, +) -> bool { + let Some(earliest_unseen) = earliest_unseen else { + return false; + }; + let latest_covered = histories + .iter() + .filter_map(|(agent_id, history)| { + let covered = frontier.agents.get(agent_id)?.sequence; + history + .events + .iter() + .filter(|event| event.agent_seq <= covered) + .map(OrderingKey::from_envelope) + .max() + }) + .max(); + latest_covered.is_some_and(|latest| OrderingKey::from_envelope(earliest_unseen) < latest) +} + pub fn rebuild_from_authority(source: &dyn HubSource) -> Result { let histories = validated_histories(source)?; + rebuild_with_histories(source, &histories) +} + +/// Replays every event after the authority baseline in total order. +fn rebuild_with_histories( + source: &dyn HubSource, + histories: &BTreeMap, +) -> Result { let mut state = if let Some(baseline) = source.read_authority_baseline()? { - validate_frontier(source, &baseline.frontier, &histories)?; + validate_frontier(source, &baseline.frontier, histories)?; let mut state = baseline.state; state.activate_causal(baseline.frontier); state @@ -275,7 +319,7 @@ pub fn rebuild_from_authority(source: &dyn HubSource) -> Result Vec { + const AGENTS: [&str; 3] = ["agent-a", "agent-b", "agent-c"]; + let mut sequences = [0_u64; 3]; + plan.iter() + .map(|&(agent, issue, claim, offset)| { + sequences[agent] += 1; + let event = if claim { + Event::LockClaimed { + issue_display_id: issue, + branch: None, + } + } else { + Event::LockReleased { + issue_display_id: issue, + } + }; + let mut envelope = make_envelope(AGENTS[agent], sequences[agent], event); + envelope.timestamp = + chrono::DateTime::::UNIX_EPOCH + Duration::seconds(offset); + envelope + }) + .collect() + } + + fn append_all<'a>(dir: &Path, events: impl IntoIterator) { + for event in events { + append_event( + &dir.join("agents").join(&event.agent_id).join("events.log"), + event, + ) + .unwrap(); + } + } + + /// Reduces `events` from a checkpoint cut after the first `cut[agent]` + /// events of each agent, with the rest arriving afterwards. + fn reduce_from_cut(events: &[EventEnvelope], cut: &[usize]) -> CheckpointState { + let dir = tempfile::tempdir().unwrap(); + setup_cache(dir.path()); + let agent_index = |id: &str| match id { + "agent-a" => 0, + "agent-b" => 1, + _ => 2, + }; + let mut seen = [0_usize; 3]; + let (covered, late): (Vec<_>, Vec<_>) = events.iter().partition(|event| { + let index = agent_index(&event.agent_id); + seen[index] += 1; + seen[index] <= cut[index] + }); + if !covered.is_empty() { + append_all(dir.path(), covered); + compact_t(dir.path(), "agent-a", true).unwrap(); + } + append_all(dir.path(), late); + reduce(&WorktreeSource::new(dir.path())).unwrap().state + } + + /// The late arrival found by the #820 contention test: agent-b claims + /// first, agent-a's checkpoint is written with only its own later claim, + /// then agent-b's claim arrives. Every reader must name agent-b. + #[test] + fn a_late_earlier_claim_wins_over_a_checkpointed_later_claim() { + let events = lock_events(&[(1, 1, true, 10), (0, 1, true, 20)]); + let full = reduce_from_cut(&events, &[0, 0, 0]); + let incremental = reduce_from_cut(&events, &[1, 0, 0]); + assert_eq!(full.locks[&1].agent_id, "agent-b"); + assert_eq!(incremental.locks[&1].agent_id, "agent-b"); + assert!(checkpoint_semantics_equal(&full, &incremental).unwrap()); + } + + proptest! { + #![proptest_config(ProptestConfig::with_cases(256))] + + /// Reducing from any checkpoint cut, with the remaining events + /// arriving late, gives the same state as a full replay in total + /// order: contended locks, releases by non-holders, timestamp ties, + /// inversions and per-agent clock skew included. + #[test] + fn prop_reduce_from_any_checkpoint_equals_full_replay( + plan in prop::collection::vec((0_usize..3, 1_i64..3, any::(), 0_i64..4), 1..12), + cut in prop::collection::vec(0_usize..12, 3), + ) { + let events = lock_events(&plan); + let full = reduce_from_cut(&events, &[0, 0, 0]); + let incremental = reduce_from_cut(&events, &cut); + prop_assert!( + checkpoint_semantics_equal(&full, &incremental).unwrap(), + "full locks {:?} vs incremental {:?}", + full.locks, + incremental.locks + ); + } + } + proptest! { #![proptest_config(ProptestConfig::with_cases(32))] @@ -1322,7 +1463,11 @@ mod tests { + Duration::seconds(second_offset); append_event(&cache_dir.join("agents/agent-b/events.log"), &second).unwrap(); let result = compact_t(cache_dir, "agent-b", true).unwrap().unwrap(); - prop_assert_eq!(result.events_processed, 1); + // No event is lost whatever the clocks say. A second event that + // orders before the checkpointed one is a late arrival, and the + // reduce replays both in total order instead of applying it on top. + let late = second_offset < first_offset; + prop_assert_eq!(result.events_processed, if late { 2 } else { 1 }); let state = read_checkpoint(cache_dir).unwrap(); prop_assert!(state.locks.contains_key(&1)); prop_assert!(state.locks.contains_key(&2)); From 21ff0aada7a29d55c5d0cd520ac8cfbeb89bf0be Mon Sep 17 00:00:00 2001 From: magnificentlycursed <190320250+magnificentlycursed@users.noreply.github.com> Date: Fri, 9 Oct 2026 14:49:52 -0700 Subject: [PATCH 10/14] perf(signing): wait for ssh-keygen without a fixed 50 ms polling sleep verify_content spawned ssh-keygen -Y verify per event and polled it with a 50 ms sleep, so every signature check cost at least 50 ms. A reduce that replays the hub verifies every signed event, and on the shared hub that made a lock confirmation take minutes. The reducer's late-arrival replay will run whenever writers overlap, so this cost would recur. The signed content now goes to ssh-keygen on stdin from the file already written, and the wait uses utils::command_output_with_timeout (30 s bound, process-group termination, drained output). That helper now polls every 1 ms at first, backing off to 20 ms, so a short command returns promptly. Tests cover a prompt return, output larger than a pipe buffer, and the deadline, which verify_content turns into an error, never a success. --- crosslink/src/signing.rs | 42 ++++++++------------------------ crosslink/src/utils.rs | 52 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 61 insertions(+), 33 deletions(-) diff --git a/crosslink/src/signing.rs b/crosslink/src/signing.rs index 1c93b5b83..ed79b3667 100644 --- a/crosslink/src/signing.rs +++ b/crosslink/src/signing.rs @@ -596,7 +596,10 @@ pub fn verify_content( format!("-----BEGIN SSH SIGNATURE-----\n{signature_b64}\n-----END SSH SIGNATURE-----\n"); std::fs::write(&sig_path, pem_sig)?; - let mut child = Command::new("ssh-keygen") + // The signed content goes in on stdin from the file written above; the + // shared helper bounds the wait without a fixed polling sleep. + let mut command = Command::new("ssh-keygen"); + command .args([ "-Y", "verify", @@ -609,37 +612,12 @@ pub fn verify_content( "-s", ]) .arg(&sig_path) - .stdin(std::process::Stdio::piped()) - .stdout(std::process::Stdio::piped()) - .stderr(std::process::Stdio::piped()) - .spawn() - .context("Failed to run ssh-keygen -Y verify")?; - - if let Some(ref mut stdin) = child.stdin { - use std::io::Write; - - let _ = stdin.write_all(content); - } - - drop(child.stdin.take()); - - { - use std::time::{Duration, Instant}; - let start = Instant::now(); - let timeout = Duration::from_secs(30); - loop { - if child.try_wait()?.is_some() { - break; - } - if start.elapsed() > timeout { - let _ = child.kill(); - bail!("ssh-keygen verification timed out after 30 seconds"); - } - std::thread::sleep(Duration::from_millis(50)); - } - } - - let output = child.wait_with_output()?; + .stdin(std::fs::File::open(&content_path).context("opening signed content")?); + let Some(output) = + crate::utils::command_output_with_timeout(&mut command, std::time::Duration::from_secs(30)) + else { + bail!("ssh-keygen verification could not run or did not finish within 30 seconds"); + }; if !output.status.success() { return Ok(false); diff --git a/crosslink/src/utils.rs b/crosslink/src/utils.rs index b61e1fa42..36e253b25 100644 --- a/crosslink/src/utils.rs +++ b/crosslink/src/utils.rs @@ -437,11 +437,15 @@ pub fn command_output_with_timeout(command: &mut Command, timeout: Duration) -> let deadline = Instant::now() + timeout; let stdout = drain(child.stdout.take()); let stderr = drain(child.stderr.take()); + // Poll quickly at first, so a short command returns promptly, and back + // off to 20 ms for long ones. + let mut pause = Duration::from_millis(1); let status = loop { match child.try_wait() { Ok(Some(status)) => break status, Ok(None) if Instant::now() < deadline => { - std::thread::sleep(Duration::from_millis(20)); + std::thread::sleep(pause); + pause = (pause * 2).min(Duration::from_millis(20)); } Ok(None) | Err(_) => { terminate_child(&mut child); @@ -510,6 +514,52 @@ fn terminate_child(child: &mut Child) { #[cfg(test)] mod tests { use super::*; + + #[cfg(unix)] + #[test] + fn command_output_with_timeout_returns_a_short_command_promptly() { + let started = Instant::now(); + let output = command_output_with_timeout( + Command::new("sh").args(["-c", "printf ok"]), + Duration::from_secs(10), + ) + .expect("completes"); + assert_eq!(output.stdout, b"ok"); + assert!( + started.elapsed() < Duration::from_secs(2), + "took {:?}", + started.elapsed() + ); + } + + /// Output larger than a pipe buffer is drained while waiting, so the + /// command neither stalls nor times out. + #[cfg(unix)] + #[test] + fn command_output_with_timeout_drains_large_output() { + let output = command_output_with_timeout( + Command::new("sh").args(["-c", "head -c 300000 /dev/zero"]), + Duration::from_secs(10), + ) + .expect("completes"); + assert_eq!(output.stdout.len(), 300_000); + } + + #[cfg(unix)] + #[test] + fn command_output_with_timeout_gives_up_at_the_deadline() { + let started = Instant::now(); + let output = command_output_with_timeout( + Command::new("sh").args(["-c", "sleep 10"]), + Duration::from_millis(300), + ); + assert!(output.is_none()); + assert!( + started.elapsed() < Duration::from_secs(5), + "took {:?}", + started.elapsed() + ); + } use std::process::Command as StdCommand; use tempfile::tempdir; From 4a43adc96b42ed4920e421e032137e85581d7b02 Mon Sep 17 00:00:00 2001 From: magnificentlycursed <190320250+magnificentlycursed@users.noreply.github.com> Date: Fri, 9 Oct 2026 14:50:05 -0700 Subject: [PATCH 11/14] fix(kickoff): keep the worktree when a lock release does not complete kickoff cleanup released nothing when the worktree's daemon did not become ready, then removed the worktree anyway, so its remedy pointed at a directory it had just deleted and the agent identity that could release the lock was gone (Corvidae-Coding-Projects/crosslink#110, field run on this branch's build, 2026-10-09T20:43Z). Kickoff undo had the same order. When a lock release ends in anything but done (failed, stranded, unverified, or readiness not granted), undo and cleanup still stop the worktree daemon, so an orphan cannot hold the hub write lock, but keep the worktree and its branch and say so: run crosslink locks release there, then kickoff cleanup again, or git worktree remove --force to give up. Cleanup's release and removal sit behind the UndoOps seam, so the not-ready path is tested without starting a daemon; CleanupResult gains worktree_kept. --- crosslink/src/commands/kickoff/cleanup.rs | 302 +++++++++++++++------ crosslink/src/commands/kickoff/rollback.rs | 161 ++++++++--- crosslink/src/commands/kickoff/types.rs | 2 + 3 files changed, 342 insertions(+), 123 deletions(-) diff --git a/crosslink/src/commands/kickoff/cleanup.rs b/crosslink/src/commands/kickoff/cleanup.rs index b01fe509f..5a0d88f0e 100644 --- a/crosslink/src/commands/kickoff/cleanup.rs +++ b/crosslink/src/commands/kickoff/cleanup.rs @@ -6,8 +6,8 @@ use std::process::Command; use super::helpers::*; use super::monitor::discover_agents; use super::rollback::{ - host_locks_held_by, outcome_message, worktree_agent_id, KickoffUndo, UndoOps, UndoOutcome, - UndoStep, + host_locks_held_by, kept_worktree_remedy, outcome_message, worktree_agent_id, KickoffUndo, + UndoOps, UndoOutcome, UndoStep, }; use super::types::*; @@ -202,6 +202,7 @@ pub fn cleanup(crosslink_dir: &Path, opts: &CleanupOptions) -> Result<()> { container_removed: false, locks_released: Vec::new(), branch_deleted: false, + worktree_kept: false, warnings: Vec::new(), error: None, }; @@ -251,13 +252,23 @@ pub fn cleanup(crosslink_dir: &Path, opts: &CleanupOptions) -> Result<()> { } // With the agent's processes gone, release its locks and end its - // session under its own identity, before the worktree goes. + // session under its own identity, before the worktree goes. If a + // release does not complete, the worktree (and the identity in it) + // is kept so the lock can still be released. let worktree_path = Path::new(&agent.worktree); let worktree_present = !agent.worktree.is_empty() && worktree_path.exists(); let repo_root = crosslink_dir.parent().unwrap_or(crosslink_dir); + let mut ops = KickoffUndo { + repo_root, + worktree_dir: worktree_path, + prior_pipeline: None, + }; + let mut released = true; let mut branch_to_delete = None; if worktree_present { - release_agent_work(repo_root, worktree_path, &mut result); + let held = worktree_agent_id(&worktree_path.join(".crosslink")) + .map(|agent_id| host_locks_held_by(&repo_root.join(".crosslink"), &agent_id)); + released = release_agent_work(worktree_path, held, &mut ops, &mut result); if !keep_branch { branch_to_delete = branch_and_base(worktree_path); } @@ -278,56 +289,21 @@ pub fn cleanup(crosslink_dir: &Path, opts: &CleanupOptions) -> Result<()> { } } - if !agent.worktree.is_empty() && std::path::Path::new(&agent.worktree).exists() { - let step = UndoStep::StopDaemon; - let outcome = KickoffUndo { + if worktree_present { + finish_worktree( repo_root, - worktree_dir: worktree_path, - prior_pipeline: None, - } - .run(&step); - if let Some(message) = outcome_message(&step, &outcome) { - result.warnings.push(message); - } - match Command::new("git") - .args(["worktree", "remove", "--force", &agent.worktree]) - .output() - { - Ok(o) if o.status.success() => { - result.worktree_removed = true; - if let Some((name, base_commit)) = branch_to_delete.take() { - let step = UndoStep::DeleteBranch { name, base_commit }; - let mut ops = KickoffUndo { - repo_root, - worktree_dir: worktree_path, - prior_pipeline: None, - }; - let outcome = ops.run(&step); - if outcome == UndoOutcome::Done { - result.branch_deleted = true; - } else if let Some(message) = outcome_message(&step, &outcome) { - result.warnings.push(message); - } - } - let wt_display = std::path::Path::new(&agent.worktree) - .file_name() - .and_then(|n| n.to_str()) - .unwrap_or(&agent.worktree); - if !json_output { - println!(" Removed worktree: {wt_display}"); - } - } - Ok(o) => { - let stderr = String::from_utf8_lossy(&o.stderr); - let msg = format!("git worktree remove failed: {}", stderr.trim()); - tracing::warn!("{}", msg); - result.error = Some(msg); - } - Err(e) => { - let msg = format!("git worktree remove error: {e}"); - tracing::warn!("{}", msg); - result.error = Some(msg); - } + worktree_path, + released, + branch_to_delete, + &mut ops, + &mut result, + ); + if result.worktree_removed && !json_output { + let wt_display = worktree_path + .file_name() + .and_then(|n| n.to_str()) + .unwrap_or(&agent.worktree); + println!(" Removed worktree: {wt_display}"); } } @@ -384,40 +360,40 @@ pub fn cleanup(crosslink_dir: &Path, opts: &CleanupOptions) -> Result<()> { } /// Releases the agent's locks and ends its session under its own identity. -/// The host's view says which locks the agent holds; the worktree's daemon is -/// brought up only when there is one to release (its session is local to the -/// worktree and goes with it). Anything left is a warning with its remedy. -fn release_agent_work(repo_root: &Path, worktree: &Path, result: &mut CleanupResult) { - let worktree_crosslink = worktree.join(".crosslink"); - let Some(agent_id) = worktree_agent_id(&worktree_crosslink) else { - return; - }; - let held = match host_locks_held_by(&repo_root.join(".crosslink"), &agent_id) { - Ok(held) => held, - Err(error) => { +/// `held` is the host's view of the locks the worktree agent holds (`None` +/// when the worktree has no agent). Returns whether every lock was released; +/// anything left is a warning with its remedy. +fn release_agent_work( + worktree: &Path, + held: Option>>, + ops: &mut impl UndoOps, + result: &mut CleanupResult, +) -> bool { + let held = match held { + None => return true, + Some(Ok(held)) => held, + Some(Err(error)) => { result.warnings.push(format!( - "could not read which locks '{agent_id}' holds: {error:#}; to clear them, run \ - `crosslink locks list` and `crosslink locks release ` in {}", - worktree.display() + "could not read which locks the agent holds: {error:#}; {}", + kept_worktree_remedy(worktree, None) )); - return; + return false; } }; if held.is_empty() { - return; + return true; } - let mut ops = KickoffUndo { - repo_root, - worktree_dir: worktree, - prior_pipeline: None, - }; + let mut released = true; for issue_id in held { let step = UndoStep::ReleaseLock { issue_id }; let outcome = ops.run(&step); if outcome == UndoOutcome::Done { result.locks_released.push(issue_id); - } else if let Some(message) = outcome_message(&step, &outcome) { - result.warnings.push(message); + } else { + released &= outcome.is_clean(); + if let Some(message) = outcome_message(&step, &outcome) { + result.warnings.push(message); + } } } let step = UndoStep::EndSession; @@ -425,6 +401,62 @@ fn release_agent_work(repo_root: &Path, worktree: &Path, result: &mut CleanupRes if let Some(message) = outcome_message(&step, &outcome) { result.warnings.push(message); } + released +} + +/// Stops the worktree's daemon, then removes the worktree and deletes its +/// branch, unless a lock release did not complete: then both are kept, with +/// a remedy that names the kept path. +fn finish_worktree( + repo_root: &Path, + worktree: &Path, + released: bool, + branch_to_delete: Option<(String, String)>, + ops: &mut impl UndoOps, + result: &mut CleanupResult, +) { + let step = UndoStep::StopDaemon; + let outcome = ops.run(&step); + if let Some(message) = outcome_message(&step, &outcome) { + result.warnings.push(message); + } + if !released { + result.worktree_kept = true; + result.warnings.push(kept_worktree_remedy(worktree, None)); + return; + } + match Command::new("git") + .current_dir(repo_root) + .args(["worktree", "remove", "--force"]) + .arg(worktree) + .output() + { + Ok(o) if o.status.success() => { + result.worktree_removed = true; + if let Some((name, base_commit)) = branch_to_delete { + let step = UndoStep::DeleteBranch { name, base_commit }; + let outcome = ops.run(&step); + if outcome == UndoOutcome::Done { + result.branch_deleted = true; + } else if let Some(message) = outcome_message(&step, &outcome) { + result.warnings.push(message); + } + } + } + Ok(o) => { + let msg = format!( + "git worktree remove failed: {}", + String::from_utf8_lossy(&o.stderr).trim() + ); + tracing::warn!("{}", msg); + result.error = Some(msg); + } + Err(e) => { + let msg = format!("git worktree remove error: {e}"); + tracing::warn!("{}", msg); + result.error = Some(msg); + } + } } /// The worktree's branch and its recorded base, when kickoff recorded one. @@ -446,6 +478,7 @@ fn branch_and_base(worktree: &Path) -> Option<(String, String)> { #[cfg(test)] mod tests { use super::*; + use crate::commands::kickoff::rollback::RecordingOps; fn git(dir: &Path, args: &[&str]) -> String { let output = Command::new("git") @@ -496,7 +529,16 @@ mod tests { #[test] fn cleanup_leaves_locks_alone_without_an_agent() { let dir = tempfile::tempdir().expect("tempdir"); - let mut result = CleanupResult { + let mut result = empty_result(); + let mut ops = RecordingOps::default(); + assert!(release_agent_work(dir.path(), None, &mut ops, &mut result)); + assert!(ops.calls.is_empty()); + assert!(result.locks_released.is_empty()); + assert!(result.warnings.is_empty()); + } + + fn empty_result() -> CleanupResult { + CleanupResult { id: "agent".to_string(), class: CleanupClass::Stale, worktree_removed: false, @@ -504,11 +546,111 @@ mod tests { container_removed: false, locks_released: Vec::new(), branch_deleted: false, + worktree_kept: false, warnings: Vec::new(), error: None, + } + } + + /// A repository with one commit and a worktree on a fresh branch. + fn repo_with_worktree() -> ( + tempfile::TempDir, + std::path::PathBuf, + std::path::PathBuf, + String, + ) { + let dir = tempfile::tempdir().expect("tempdir"); + let root = dir.path().join("repo"); + std::fs::create_dir_all(&root).expect("repo dir"); + git(&root, &["init", "-q", "-b", "main"]); + git(&root, &["config", "user.email", "test@test.local"]); + git(&root, &["config", "user.name", "Test"]); + git(&root, &["config", "commit.gpgsign", "false"]); + git(&root, &["commit", "-q", "--allow-empty", "-m", "init"]); + let worktree = dir.path().join("wt"); + git( + &root, + &[ + "worktree", + "add", + "-q", + "-b", + "feature/x", + worktree.to_str().expect("utf-8"), + "HEAD", + ], + ); + let base = git(&worktree, &["rev-parse", "HEAD"]); + (dir, root, worktree, base) + } + + /// The field scenario on gh#110 (2026-10-09T20:43Z): the worktree's + /// daemon never becomes ready, so the release fails. Cleanup stops the + /// daemon but keeps the worktree and branch, and the remedy names a path + /// that still exists. + #[test] + fn cleanup_keeps_the_worktree_when_a_release_does_not_complete() { + let (_dir, root, worktree, base) = repo_with_worktree(); + let mut ops = RecordingOps { + outcomes: vec![( + UndoStep::ReleaseLock { issue_id: 7 }, + UndoOutcome::Failed { + error: "the worktree daemon did not become ready".to_string(), + remedy: "r".to_string(), + }, + )], + ..RecordingOps::default() }; - release_agent_work(dir.path(), dir.path(), &mut result); - assert!(result.locks_released.is_empty()); - assert!(result.warnings.is_empty()); + let mut result = empty_result(); + let released = release_agent_work(&worktree, Some(Ok(vec![7])), &mut ops, &mut result); + assert!(!released); + finish_worktree( + &root, + &worktree, + released, + Some(("feature/x".to_string(), base)), + &mut ops, + &mut result, + ); + assert!(ops.calls.contains(&UndoStep::StopDaemon)); + assert!(worktree.exists(), "the worktree was removed"); + assert!(!git(&root, &["branch", "--list", "feature/x"]).is_empty()); + assert!(result.worktree_kept); + assert!(!result.worktree_removed); + let shown = worktree.display().to_string(); + assert!( + result + .warnings + .iter() + .any(|w| w.contains(&shown) && w.contains("kept")), + "{:?}", + result.warnings + ); + } + + /// When every release completes, the worktree is removed and the + /// unchanged branch deleted. + #[test] + fn cleanup_removes_the_worktree_when_releases_complete() { + let (_dir, root, worktree, base) = repo_with_worktree(); + let mut ops = RecordingOps::default(); + let mut result = empty_result(); + let released = release_agent_work(&worktree, Some(Ok(vec![7])), &mut ops, &mut result); + assert!(released); + assert_eq!(result.locks_released, vec![7]); + finish_worktree( + &root, + &worktree, + released, + Some(("feature/x".to_string(), base.clone())), + &mut ops, + &mut result, + ); + assert!(!worktree.exists()); + assert!(result.worktree_removed); + assert!(ops.calls.contains(&UndoStep::DeleteBranch { + name: "feature/x".to_string(), + base_commit: base, + })); } } diff --git a/crosslink/src/commands/kickoff/rollback.rs b/crosslink/src/commands/kickoff/rollback.rs index c90c6ab7f..d6a0a49b4 100644 --- a/crosslink/src/commands/kickoff/rollback.rs +++ b/crosslink/src/commands/kickoff/rollback.rs @@ -63,6 +63,18 @@ pub(crate) enum UndoOutcome { Unverified { remedy: String, }, + /// Not done on purpose: the worktree and its branch are kept because a + /// lock release did not complete, and its identity is needed to finish it. + Kept { + remedy: String, + }, +} + +impl UndoOutcome { + /// Done, or there was nothing to undo. + pub(crate) const fn is_clean(&self) -> bool { + matches!(self, Self::Done | Self::NothingToUndo) + } } #[derive(Debug, Default)] @@ -73,9 +85,7 @@ pub(crate) struct UndoReport { impl UndoReport { /// True when every step either completed or had nothing to undo. pub(crate) fn is_clean(&self) -> bool { - self.entries - .iter() - .all(|(_, outcome)| matches!(outcome, UndoOutcome::Done | UndoOutcome::NothingToUndo)) + self.entries.iter().all(|(_, outcome)| outcome.is_clean()) } } @@ -216,12 +226,29 @@ impl KickoffRollback { } /// Runs every step, continuing past failures, and reports each outcome. + /// If the lock release does not complete, the worktree and branch are + /// kept (the daemon is still stopped): the worktree holds the agent's + /// identity, which is what can still release the lock. pub(crate) fn unwind(self, ops: &mut impl UndoOps) -> UndoReport { + let mut release_incomplete = None; let entries = self .steps() .into_iter() .map(|step| { - let outcome = ops.run(&step); + let outcome = match (&step, release_incomplete) { + (UndoStep::RemoveWorktree { path }, Some(issue_id)) => UndoOutcome::Kept { + remedy: kept_worktree_remedy(path, Some(issue_id)), + }, + (UndoStep::DeleteBranch { .. }, Some(_)) => UndoOutcome::Kept { + remedy: "it is checked out in the kept worktree".to_string(), + }, + _ => ops.run(&step), + }; + if let UndoStep::ReleaseLock { issue_id } = step { + if !outcome.is_clean() { + release_incomplete = Some(issue_id); + } + } (step, outcome) }) .collect(); @@ -532,6 +559,22 @@ impl UndoReport { } } +/// How to finish with a worktree kept because a lock release did not +/// complete. +pub(crate) fn kept_worktree_remedy(path: &Path, issue_id: Option) -> String { + let release = issue_id.map_or_else( + || "`crosslink locks list` and `crosslink locks release `".to_string(), + |id| format!("`crosslink locks release {id}`"), + ); + format!( + "kept {} so the agent can release its lock: run {release} there, then \ + `crosslink kickoff cleanup` again; to give up instead, run \ + `git worktree remove --force {}`", + path.display(), + path.display() + ) +} + /// What to tell the user about a step that did not complete cleanly, or /// `None` when it did. pub(crate) fn outcome_message(step: &UndoStep, outcome: &UndoOutcome) -> Option { @@ -545,6 +588,7 @@ pub(crate) fn outcome_message(step: &UndoStep, outcome: &UndoOutcome) -> Option< "{} left the lock held (stranded); to clear it, {remedy}", step.describe() )), + UndoOutcome::Kept { remedy } => Some(format!("{} skipped: {remedy}", step.describe())), UndoOutcome::Unverified { remedy } => Some(format!( "{}: release unverified; to check it, {remedy}", step.describe() @@ -575,6 +619,26 @@ pub(crate) fn branch_deletable(tip: &str, base_commit: Option<&str>) -> bool { base_commit.is_some_and(|base| base == tip) } +/// Records undo calls and returns canned outcomes, for tests of the undo +/// and cleanup decisions. +#[cfg(test)] +#[derive(Default)] +pub(crate) struct RecordingOps { + pub(crate) calls: Vec, + pub(crate) outcomes: Vec<(UndoStep, UndoOutcome)>, +} + +#[cfg(test)] +impl UndoOps for RecordingOps { + fn run(&mut self, step: &UndoStep) -> UndoOutcome { + self.calls.push(step.clone()); + self.outcomes + .iter() + .find(|(s, _)| s == step) + .map_or(UndoOutcome::Done, |(_, outcome)| outcome.clone()) + } +} + #[cfg(test)] mod tests { use super::*; @@ -614,22 +678,6 @@ mod tests { ] } - #[derive(Default)] - struct RecordingOps { - calls: Vec, - outcomes: Vec<(UndoStep, UndoOutcome)>, - } - - impl UndoOps for RecordingOps { - fn run(&mut self, step: &UndoStep) -> UndoOutcome { - self.calls.push(step.clone()); - self.outcomes - .iter() - .find(|(s, _)| s == step) - .map_or(UndoOutcome::Done, |(_, outcome)| outcome.clone()) - } - } - /// AC-8: activation fails after the claim. #[test] fn activation_failure_undoes_in_reverse_order() { @@ -729,38 +777,65 @@ mod tests { assert!(report.entries.is_empty()); } - /// AC-12: a stranded lock and a failed step are reported, and every - /// remaining step still runs. + /// AC-12 and the kept-worktree rule: when the release does not complete + /// (stranded, failed, unverified), every other step still runs, the + /// daemon is still stopped, and the worktree and branch are kept with a + /// remedy that names the kept path. #[test] - fn failures_are_reported_and_do_not_stop_later_steps() { + fn an_incomplete_release_keeps_the_worktree_and_branch() { + let incomplete = [ + UndoOutcome::Stranded { + remedy: "r".to_string(), + }, + UndoOutcome::Failed { + error: "e".to_string(), + remedy: "r".to_string(), + }, + UndoOutcome::Unverified { + remedy: "r".to_string(), + }, + ]; + for outcome in incomplete { + let rollback = created_worktree_with_agent_and_claim(); + let mut ops = RecordingOps { + outcomes: vec![(UndoStep::ReleaseLock { issue_id: 7 }, outcome.clone())], + ..RecordingOps::default() + }; + let report = rollback.unwind(&mut ops); + assert_eq!(ops.calls, tail_steps(), "for {outcome:?}"); + assert!(!report.is_clean()); + let kept: Vec<_> = report + .entries + .iter() + .filter(|(_, outcome)| matches!(outcome, UndoOutcome::Kept { .. })) + .map(|(step, _)| step.clone()) + .collect(); + assert_eq!(kept, removal_steps(), "for {outcome:?}"); + let rendered = report.render().expect("report"); + assert!(rendered.contains("/wt/feature-x"), "{rendered}"); + assert!(rendered.contains("crosslink locks release 7"), "{rendered}"); + } + } + + /// A completed release still removes the worktree and branch, and a + /// failed later step does not stop the rest. + #[test] + fn a_completed_release_still_removes_the_worktree() { let rollback = created_worktree_with_agent_and_claim(); let expected_calls = rollback.steps(); let mut ops = RecordingOps { - outcomes: vec![ - ( - UndoStep::ReleaseLock { issue_id: 7 }, - UndoOutcome::Stranded { - remedy: "crosslink locks release 7".to_string(), - }, - ), - ( - UndoStep::StopDaemon, - UndoOutcome::Failed { - error: "daemon did not stop".to_string(), - remedy: "crosslink daemon stop".to_string(), - }, - ), - ], + outcomes: vec![( + UndoStep::StopDaemon, + UndoOutcome::Failed { + error: "daemon did not stop".to_string(), + remedy: "crosslink daemon stop".to_string(), + }, + )], ..RecordingOps::default() }; let report = rollback.unwind(&mut ops); assert_eq!(ops.calls, expected_calls); - assert_eq!(report.entries.len(), expected_calls.len()); assert!(!report.is_clean()); - assert!(report - .entries - .iter() - .any(|(_, outcome)| matches!(outcome, UndoOutcome::Stranded { .. }))); } fn git(dir: &Path, args: &[&str]) -> String { diff --git a/crosslink/src/commands/kickoff/types.rs b/crosslink/src/commands/kickoff/types.rs index 908bc15e2..80110af82 100644 --- a/crosslink/src/commands/kickoff/types.rs +++ b/crosslink/src/commands/kickoff/types.rs @@ -411,6 +411,8 @@ pub(super) struct CleanupResult { pub container_removed: bool, pub locks_released: Vec, pub branch_deleted: bool, + /// Kept because a lock release did not complete. + pub worktree_kept: bool, pub warnings: Vec, pub error: Option, } From 7ec14546fa57f1c08b95b45917e67444a295ef6b Mon Sep 17 00:00:00 2001 From: magnificentlycursed <190320250+magnificentlycursed@users.noreply.github.com> Date: Fri, 9 Oct 2026 14:51:44 -0700 Subject: [PATCH 12/14] docs(design): record the reducer, signature-wait and kept-worktree changes; reorder the umbrella The increment's design records the late-arrival rule as what REQ-6(b) rests on, the signature-wait change, and the kept-worktree amendments to REQ-6 and REQ-8, with the field run on Corvidae-Coding-Projects/crosslink#110 as evidence against the earlier build and the tests that verify the fix. The umbrella builds write cost second and the knowledge cache third (operator decision, 2026-10-09). --- ...lock-claim-outcome-and-kickoff-rollback.md | 16 ++++++++++---- .design/write-path-cost.md | 21 ++++++++++--------- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/.design/lock-claim-outcome-and-kickoff-rollback.md b/.design/lock-claim-outcome-and-kickoff-rollback.md index 38efbe602..4ab066730 100644 --- a/.design/lock-claim-outcome-and-kickoff-rollback.md +++ b/.design/lock-claim-outcome-and-kickoff-rollback.md @@ -49,7 +49,7 @@ Kickoff undo: 5. the worktree, only if this invocation created it; 6. the branch, only if this invocation created it and it has no commits beyond the recorded base. - A worktree or branch that kickoff reused (`--branch` with an existing directory, a swarm-stored branch) is never removed. + A worktree or branch that kickoff reused (`--branch` with an existing directory, a swarm-stored branch) is never removed. Amended 2026-10-09: if the lock release in step 2 does not complete (failed, stranded, unverified, or readiness not granted), steps 5 and 6 are skipped and reported as kept, with a remedy naming the kept path. Step 3 still runs, so no orphaned daemon holds the hub write lock. - REQ-7: The agent has started once tmux `send-keys` succeeds (`launch_local`) or `launch_container` returns `Ok`, which happens after its early-exit check. From then on, undo is disarmed, and the lock, worktree and branch belong to the agent. - REQ-8: `kickoff cleanup` works in this order: 1. it removes the tmux session or container, so no agent process is still running; @@ -58,7 +58,7 @@ Kickoff undo: 4. it stops the daemon and removes the worktree; 5. it deletes the branch under the rule in REQ-6 step 6. - `--keep-branch` skips the branch step. A worktree whose metadata has no recorded base keeps its branch. + `--keep-branch` skips the branch step. A worktree whose metadata has no recorded base keeps its branch. Amended 2026-10-09: if a release in step 3 does not complete, step 4 still stops the daemon but keeps the worktree, and step 5 is skipped. The warning names the kept path and how to finish (`crosslink locks release` there, then `kickoff cleanup` again) or give up (`git worktree remove --force`). - REQ-9: Undo and cleanup verify each lock release with the same bounded, fallible confirmation as REQ-2, run in the worktree after the release: - if confirmation shows the worktree agent still holding the lock, the lock is reported as stranded; - if confirmation does not complete, the release is reported as unverified. @@ -178,6 +178,9 @@ Dated 2026-10-09, after the manual checks and the phase 3 review (three reviewer - the undo report follows the original error in the returned error, so swarm and sentinel keep it; - cleanup stops daemons through the shared undo step and exits non-zero when it leaves work behind; - on a reused worktree, undo leaves what pre-existed (AC-9). +- **Reducer: late arrivals are replayed in total order** (`9b6a59ed`). The contention test (AC-6) found that a reduce applying unseen events on top of the checkpoint diverges from a total-order replay when another agent's earlier event arrives late. Two clients could then each see themselves as holder, and a checkpoint written from that state failed verification for every reader. REQ-6(b) depends on the reduce matching a total-order replay. Reduce now rebuilds from the authority baseline when the earliest unseen event orders before the latest covered one. `f60712ed` had widened the window by dropping the checkpoint adoption that recomputed on concurrent checkpoints; the gap also existed before this branch when the earlier claim was in no checkpoint. This is not a protocol change: the event and checkpoint formats are unchanged, and checkpoint verification already replays in total order. Older binaries on a mixed-version hub can still compute the wrong holder locally until they upgrade; their checkpoints from that state already fail verification on every reader. +- **Signature checks without a polling floor** (`21ff0aad`). `verify_content` waited for `ssh-keygen` with a 50 ms sleep per event, and the late-arrival replay will run whenever writers overlap. It now uses the shared timeout helper, which polls from 1 ms. Strictly this belongs to the write-cost increment; it is here because the replay above would otherwise make claims slow again. +- **Keep the worktree when a release does not complete** (`4a43adc9`). See the amendments to REQ-6 and REQ-8. This also brings the build back in line with REQ-11: the worktree holds the agent's `agent.json`, half the identity REQ-11 keeps. - **Permit hold time.** A claim now holds its mutation permit through confirmation: up to 30 s more for the hub-lock wait and fetch, plus one reduce. Writes that hold permits past 90 s let the readiness record expire and the daemon exit (#816, #817), so this narrows that margin on slow machines until the write-cost and heartbeat increments land. Where each criterion is covered: @@ -188,7 +191,8 @@ Where each criterion is covered: - **AC-3:** after-push and hydration failures. - **AC-4:** the grandchild in the fetch's process group and the SIGTERM lock-file cleanup. - **AC-4a.** - - **AC-6:** contention found during confirmation, on the already-held path. + - **AC-6:** contention found during confirmation, on the already-held path (`v3_contention_found_during_confirmation_is_contended`). +- **Reducer:** `compaction::tests::prop_reduce_from_any_checkpoint_equals_full_replay` (256 cases) and `a_late_earlier_claim_wins_over_a_checkpointed_later_claim`; both fail with the rule disabled. - **AC-12:** the real release on a refused push (`Stranded`) and an unreachable remote (`Unverified`), Unix only. - **Undo and cleanup:** unit tests in `crosslink/src/commands/kickoff/rollback.rs` and `cleanup.rs`: - the step order for each failure point; @@ -198,6 +202,7 @@ Where each criterion is covered: - the branch rule; - the pipeline restore through `undo_failed_kickoff`; - the host key file surviving undo (AC-13); + - an incomplete release keeping the worktree and branch, in undo and in cleanup, including the field scenario of a daemon that never becomes ready; - git operations on a temporary repository. - **Daemon reaping:** `daemon::tests::a_spawned_child_is_reaped_when_it_exits`. @@ -209,6 +214,7 @@ Not covered by automated tests: - **AC-10 and AC-11 end to end.** AC-11's cleanup flow was checked by hand. `--keep-branch` is untested. - **The undo's readiness wake-up.** It would start a daemon, so no unit test runs it. - **Windows:** the AC-4 and AC-12 tests are Unix only, and the Windows CI shards do not run the v3 operation suite. +- **Known local failure, not this branch:** `reconcile::migration::tests::production_ready_observer_completes_fallback_pointer_window` fails on this Mac on develop (`3440c0d0`) as well; its 5 s wait for a migration's pointer push is shorter than a migration takes here. Manual checks, 2026-10-09, macOS, installed builds of this branch: @@ -218,7 +224,9 @@ Manual checks, 2026-10-09, macOS, installed builds of this branch: - the daemon stop reported a zombie as alive, because the spawning kickoff never reaped it; spawned daemons are now reaped (`73ed70db`). After the fixes, the undo reported "removed everything this kickoff created". -- **Cleanup of a stopped agent.** A local kickoff was stopped at the trust prompt, and `kickoff cleanup --force` ran on it. It stopped the daemon, released the agent's lock, removed the worktree and deleted the unchanged branch. The issue then read "not locked". +- **Cleanup of a stopped agent.** A local kickoff was stopped at the trust prompt, and `kickoff cleanup --force` ran on it. It stopped the daemon, released the agent's lock, removed the worktree and deleted the unchanged branch. The issue then read "not locked". This run took the path where the worktree daemon became ready; the not-ready path was not exercised by hand. + +Field observation, 2026-10-09T20:43Z, from another repository's session on an installed build of this branch before `ea5402a0` (Corvidae-Coding-Projects/crosslink#110): `kickoff cleanup --force` over three timed-out plan worktrees removed the worktrees and stopped their daemons, but the worktree daemons did not become ready, so no lock was released, and each warning's remedy named the directory just removed. The lock stayed stranded with no identity left to release it. This is evidence against AC-11 and REQ-8 as implemented at that build. Fixed by `4a43adc9`, verified by `cleanup_keeps_the_worktree_when_a_release_does_not_complete` and `an_incomplete_release_keeps_the_worktree_and_branch`. - **Observed, out of scope.** The host daemon exited, as `running: false`, after writes that held permits for more than 90 s: the 130 s cleanup, and the kickoff runs. It also parked once as `blocked_corrupt` after repeated record expiries. These are tracked on #816 and #817, for the heartbeat and write-cost increments. ## Data and compatibility diff --git a/.design/write-path-cost.md b/.design/write-path-cost.md index e639c7747..d23419339 100644 --- a/.design/write-path-cost.md +++ b/.design/write-path-cost.md @@ -35,16 +35,8 @@ Each figure is marked as measured, estimated or a target. ## Increments, in build order 1. **Lock-claim outcome and kickoff rollback** (Corvidae-Coding-Projects/crosslink#110, and the timeout half of Corvidae-Coding-Projects/crosslink#118). The design document is written: `.design/lock-claim-outcome-and-kickoff-rollback.md`. -2. **Knowledge cache ownership** (Corvidae-Coding-Projects/crosslink#120, tracker #807). Provisional. - - The main checkout's daemon creates the cache after its first ready activation and refreshes it on its sync tick when the remote tip moves. Creation is atomic. - - A failure is logged and never affects readiness or the reconcile budget. - - The hook and the knowledge MCP server name `crosslink knowledge sync` when the cache is missing. - - Constraints found: - - The step must run only in the ready normal loop, per the readiness contract's state gating (tracker #794). - - Running it outside the operation permit departs from Corvidae-Coding-Projects/crosslink#120's suggestion, and the increment document must say why. - - The overhead map asks that the cache never be made authoritative. - - It is second because it is small and independent. Downstream projects rely on it: pages attached to an issue reach agents at session start only if the cache exists. -3. **Write cost** (Corvidae-Coding-Projects/crosslink#118). Provisional. +2. **Write cost** (Corvidae-Coding-Projects/crosslink#118). Provisional. + - Moved to second on 2026-10-09 (operator decision, from the #820 review): bootstrap and write latency now break the first increment's undo and cleanup on large hubs. The reducer's late-arrival rule (#820) lands before it, and its caches must keep the property test against full replay. - A spawn-count test lands first and records the baseline. - Then readiness validated once per command, with `SyncManager` setup and `remote_exists` computed once. - At most two reduces per write. @@ -56,6 +48,15 @@ Each figure is marked as measured, estimated or a target. - Constraints found: - The readiness contract forbids "in-memory-only readiness" and requires every write to reject a non-ready repository. So a cached validation must still re-check record age and daemon liveness on every acquisition. - Every new cache needs a property test against full recomputation, proved by removing its invalidation and watching the test fail. +3. **Knowledge cache ownership** (Corvidae-Coding-Projects/crosslink#120, tracker #807). Provisional. + - The main checkout's daemon creates the cache after its first ready activation and refreshes it on its sync tick when the remote tip moves. Creation is atomic. + - A failure is logged and never affects readiness or the reconcile budget. + - The hook and the knowledge MCP server name `crosslink knowledge sync` when the cache is missing. + - Constraints found: + - The step must run only in the ready normal loop, per the readiness contract's state gating (tracker #794). + - Running it outside the operation permit departs from Corvidae-Coding-Projects/crosslink#120's suggestion, and the increment document must say why. + - The overhead map asks that the cache never be made authoritative. + - It was second because it is small and independent; moved to third on 2026-10-09 (operator decision) because bootstraps after a wake now exceed the 120 s `ensure` wait (Corvidae-Coding-Projects/crosslink#118), which breaks claims, kickoff undo and cleanup. Downstream projects rely on it: pages attached to an issue reach agents at session start only if the cache exists. 4. **Heartbeats off the write path.** Provisional. - A heartbeat writes only the agent's own ref, without a reduce or full validation. - It still runs only while readiness grants writes; the readiness contract lists heartbeats among the operations that require ready. From f83becacc5947e74c2946d5f893fb4d2f85ef762 Mon Sep 17 00:00:00 2001 From: magnificentlycursed <190320250+magnificentlycursed@users.noreply.github.com> Date: Fri, 9 Oct 2026 15:19:03 -0700 Subject: [PATCH 13/14] fix(kickoff,compaction,signing): address phase 3 review round 2 - Cleanup decides whether an agent may still hold a lock from the host's view plus the agent's own recent claims, read from its ref in the shared repository (the host's view may not have seen them), and keeps the worktree when that cannot be read. - Undo and cleanup no longer end the worktree session: it is local state discarded with the worktree, and a failed session end printed a remedy for a path that was then removed. - Signature checks use a strict variant of the timeout helper: output that did not arrive in full is an error, not a bad signature. Both pipes share one post-exit deadline. - The reducer logs a late-arrival replay at debug level. Tests: the reduce-equivalence property now generates issue creation (display id allocation) and two successive checkpoint cuts, and prints the plan and cuts on failure (with the proptest regression seed); cleanup keeps the worktree when the held locks cannot be read; the already-held rerun is back in AC-4a; the agent's own claims are read from its ref; a leftover pipe holder does not extend the helper's call. Docs: stale and overstated coverage corrected; older binaries, revert scope, the reducer's costs and display-id renumbering (#824) recorded; the reduce invariant added to the umbrella; CHANGELOG notes cleanup's exit status, the kept worktree, the reducer and the signature wait. --- ...lock-claim-outcome-and-kickoff-rollback.md | 31 ++-- .design/write-path-cost.md | 6 +- CHANGELOG.md | 12 +- crosslink/proptest-regressions/compaction.txt | 7 + .../src/commands/hub_v3_operation_tests.rs | 32 ++++ crosslink/src/commands/kickoff/cleanup.rs | 52 ++++-- crosslink/src/commands/kickoff/rollback.rs | 102 ++++++----- crosslink/src/compaction.rs | 165 ++++++++++++++---- crosslink/src/signing.rs | 9 +- crosslink/src/utils.rs | 72 ++++++-- 10 files changed, 371 insertions(+), 117 deletions(-) create mode 100644 crosslink/proptest-regressions/compaction.txt diff --git a/.design/lock-claim-outcome-and-kickoff-rollback.md b/.design/lock-claim-outcome-and-kickoff-rollback.md index 4ab066730..34fdc8f20 100644 --- a/.design/lock-claim-outcome-and-kickoff-rollback.md +++ b/.design/lock-claim-outcome-and-kickoff-rollback.md @@ -71,10 +71,10 @@ Kickoff undo: Tests marked "fails first" must fail on 3440c0d05 and pass after the change. They are the regression tests for Corvidae-Coding-Projects/crosslink#110. -- [ ] AC-1 (fails first): a test hook delays publication past the confirmation deadline (a 2 s delay against a 1 s deadline), and confirmation succeeds. Amended 2026-10-08 during the build, from "past 30 s": the deadline is what publication time must not count against. `session work` succeeds, records the active issue, and the hub shows the lock held by the agent. (REQ-1, REQ-4) +- [ ] AC-1 (fails first): a test hook delays publication past the confirmation deadline (a 6 s delay against a 5 s deadline), and confirmation succeeds. Amended 2026-10-08 during the build, from "past 30 s": the deadline is what publication time must not count against. `session work` succeeds, records the active issue, and the hub shows the lock held by the agent. (REQ-1, REQ-4) - [ ] AC-2 (fails first): the remote becomes unreachable after the claim's push. The result is `Unconfirmed`, `session work` exits non-zero with the rerun message, and no active issue is recorded. With the remote back, a rerun records the active issue, and the agent ref holds exactly one `LockClaimed` for the issue. (REQ-2, REQ-3, REQ-4) - [ ] AC-3: an error is injected into the reduce after the push. The result is `Unconfirmed`, not a plain error. (REQ-2) -- [ ] AC-4: a test sets the confirmation deadline to 2 s, and a fake git holds the fetch open for 30 s. On the already-held path, the result is `Unconfirmed` in well under 30 s (the test asserts under 20 s). Amended 2026-10-08 during the scaffold: "within 4 s" could not hold, because the claim's publication and the local read before confirmation are not part of the bound. No process from the fetch's process group remains, and a normal fetch afterwards succeeds, so no lock files were left. On Windows, only the direct child is checked. (REQ-2) +- [ ] AC-4: a test sets the confirmation deadline to 2 s, and a fake git holds the fetch open for 30 s. On the already-held path, the result is `Unconfirmed` in well under 30 s (the test asserts under 20 s). Amended 2026-10-08 during the scaffold: "within 4 s" could not hold, because the claim's publication and the local read before confirmation are not part of the bound. No process from the fetch's process group remains (the test's marker is on a grandchild), and a lock file the fake fetch removes only on SIGTERM is gone, so termination was graceful. On Windows, only the direct child is checked. (REQ-2) - [ ] AC-4a: the fetch is fast, and a test hook makes the reduce take longer than the deadline. The result is `Claimed` (and `AlreadyHeld` on a rerun), not `Unconfirmed`. (REQ-2, REQ-3) - [ ] AC-5: the local view shows this agent as holder, and the remote is unreachable. `session work` returns `Unconfirmed`, not `AlreadyHeld`. (REQ-3) - [ ] AC-6: another agent's earlier-ordered claim arrives during confirmation. The result is `Contended`, and no active issue is recorded. (REQ-3) @@ -157,10 +157,10 @@ The build starts from a scaffold commit containing the new `LockClaimResult` var Dated 2026-10-08, made while building on `feat/lock-claim-outcome`: -- **AC-1** is relative to the confirmation deadline (a 2 s publication delay against a 1 s deadline), not "past 30 s". +- **AC-1** is relative to the confirmation deadline (a 6 s publication delay against a 5 s deadline), not "past 30 s". - **AC-4** checks the bound against the hung fetch (under 20 s), not "within 4 s". Publication and the local read before confirmation are outside the bound. - **Undo and cleanup release the lock in-process.** They use a `SharedWriter` built on the worktree's `.crosslink`, not a `crosslink locks release` subprocess. The signer is the same worktree agent. Running in-process lets the release be checked by `SharedWriter::confirmed_lock_holder`, the bounded confirmation, which a subprocess exit code cannot provide. -- **`session end` runs without notes.** With notes it would post a comment on the issue, an extra hub write for every failed kickoff. +- **`session end` runs without notes.** With notes it would post a comment on the issue, an extra hub write for every failed kickoff. Superseded 2026-10-09 (round 2): undo and cleanup no longer end the worktree session at all. The session is local state in the worktree and is discarded with it, and a failed session end printed a remedy for a path that was then removed. REQ-6 step 2, REQ-8 step 3 and AC-11's "session ended" read accordingly. - **Cleanup releases every lock the worktree agent holds**, because cleanup does not know the agent's issue. Amended 2026-10-09: it reads them from the host's view of the hub, and brings the worktree daemon up only when there is one to release. Dated 2026-10-09, after the manual checks and the phase 3 review (three reviewers, recorded on #820): @@ -180,6 +180,12 @@ Dated 2026-10-09, after the manual checks and the phase 3 review (three reviewer - on a reused worktree, undo leaves what pre-existed (AC-9). - **Reducer: late arrivals are replayed in total order** (`9b6a59ed`). The contention test (AC-6) found that a reduce applying unseen events on top of the checkpoint diverges from a total-order replay when another agent's earlier event arrives late. Two clients could then each see themselves as holder, and a checkpoint written from that state failed verification for every reader. REQ-6(b) depends on the reduce matching a total-order replay. Reduce now rebuilds from the authority baseline when the earliest unseen event orders before the latest covered one. `f60712ed` had widened the window by dropping the checkpoint adoption that recomputed on concurrent checkpoints; the gap also existed before this branch when the earlier claim was in no checkpoint. This is not a protocol change: the event and checkpoint formats are unchanged, and checkpoint verification already replays in total order. Older binaries on a mixed-version hub can still compute the wrong holder locally until they upgrade; their checkpoints from that state already fail verification on every reader. - **Signature checks without a polling floor** (`21ff0aad`). `verify_content` waited for `ssh-keygen` with a 50 ms sleep per event, and the late-arrival replay will run whenever writers overlap. It now uses the shared timeout helper, which polls from 1 ms. Strictly this belongs to the write-cost increment; it is here because the replay above would otherwise make claims slow again. +- **Round 2 review fixes (2026-10-09):** + - cleanup decides what the agent may hold from the host's view plus the agent's own claims read from its ref, and keeps the worktree if that cannot be read; + - undo and cleanup no longer end the worktree session (see the superseded entry above); + - signature verification treats incomplete output as an error, not a bad signature, and a hung `ssh-keygen` now fails after 30 s instead of blocking; + - the late-arrival replay is logged at debug level. +- **Costs and visible effects of the reducer rule.** The rebuilt state is not persisted by the reduce itself. Until a write publishes a checkpoint covering the late event, each reduce that writes no checkpoint replays the history again: lock confirmation, side-effect-free reads, the daemon's hydration tick and dashboard polls. An agent with a lagging clock keeps producing late arrivals. This is a constraint on the write-cost increment. A late `IssueCreated` that orders earlier also renumbers display ids a client has already shown, and lock events are keyed by display id. That is the canonical total-order semantics, which diverging clients used to hide; whether hydration tolerates two ids swapping is tracked separately (#824). - **Keep the worktree when a release does not complete** (`4a43adc9`). See the amendments to REQ-6 and REQ-8. This also brings the build back in line with REQ-11: the worktree holds the agent's `agent.json`, half the identity REQ-11 keeps. - **Permit hold time.** A claim now holds its mutation permit through confirmation: up to 30 s more for the hub-lock wait and fetch, plus one reduce. Writes that hold permits past 90 s let the readiness record expire and the daemon exit (#816, #817), so this narrows that margin on slow machines until the write-cost and heartbeat increments land. @@ -192,8 +198,8 @@ Where each criterion is covered: - **AC-4:** the grandchild in the fetch's process group and the SIGTERM lock-file cleanup. - **AC-4a.** - **AC-6:** contention found during confirmation, on the already-held path (`v3_contention_found_during_confirmation_is_contended`). -- **Reducer:** `compaction::tests::prop_reduce_from_any_checkpoint_equals_full_replay` (256 cases) and `a_late_earlier_claim_wins_over_a_checkpointed_later_claim`; both fail with the rule disabled. - - **AC-12:** the real release on a refused push (`Stranded`) and an unreachable remote (`Unverified`), Unix only. +- **Reducer:** `compaction::tests::prop_reduce_from_any_checkpoint_equals_full_replay` (256 cases: lock claims and releases, issue creation with allocated display ids, two successive checkpoint cuts) and `a_late_earlier_claim_wins_over_a_checkpointed_later_claim`; both failed with the rule disabled (lock-only version). The oracle is `reduce` from empty, the total order checkpoint verification also uses. `prop_` tests run on Ubuntu CI only. The baseline branch of the rebuild is not exercised: the generated hubs have no authority baseline. +- **AC-12:** the real release on a refused push (`Stranded`) and an unreachable remote (`Unverified`), Unix only; it skips itself when run as root. - **Undo and cleanup:** unit tests in `crosslink/src/commands/kickoff/rollback.rs` and `cleanup.rs`: - the step order for each failure point; - reuse, including pre-existing state; @@ -201,8 +207,9 @@ Where each criterion is covered: - failure reporting; - the branch rule; - the pipeline restore through `undo_failed_kickoff`; - - the host key file surviving undo (AC-13); - - an incomplete release keeping the worktree and branch, in undo and in cleanup, including the field scenario of a daemon that never becomes ready; + - a host key file surviving undo (weak evidence for AC-13: undo never touches that directory, and the trust approval and the after-cleanup half are not tested); + - an incomplete release keeping the worktree and branch, in undo and in cleanup, with the field scenario of a daemon that never becomes ready simulated at the `UndoOps` boundary (a canned `Failed` release), and an unreadable lock view keeping it too; + - the agent's own claims read from its ref (`v3_agent_own_claims_are_read_from_its_ref`); - git operations on a temporary repository. - **Daemon reaping:** `daemon::tests::a_spawned_child_is_reaped_when_it_exits`. @@ -233,13 +240,13 @@ Field observation, 2026-10-09T20:43Z, from another repository's session on an in - **One additive field.** `base_commit` in kickoff metadata, optional, with a default. Metadata without it means "keep the branch". - `LockClaimResult` and rollback state are internal, so no other persisted format changes. -- Older binaries on the same hub are unaffected. +- Older binaries on the same hub are unaffected by the claim, undo and cleanup changes. They do not have the reducer's late-arrival rule: until they upgrade they can compute a different lock holder locally, and checkpoints they write from that state fail verification on every reader, as they already did. ## Failure handling - **Confirmation does not complete.** The claim stays published, the command exits non-zero, and a rerun confirms it. If nobody reruns, kickoff undo releases it: the release comes from the holder, so the reducer accepts it. Otherwise the lock stays until its holder releases it. A stale timeout does not clear it on v3. Clearing another agent's lock arrives with the lock-clearing increment (#818). - **An undo or cleanup step fails.** The failure is reported with its remedy, and the remaining steps run. -- **The worktree cannot become ready during cleanup.** The lock and session steps are reported as skipped, with the remedy. +- **The worktree cannot become ready during cleanup, or the agent's locks cannot be read.** The daemon is stopped and the worktree and branch are kept, with a remedy naming the kept path. ## Security considerations @@ -250,7 +257,7 @@ Field observation, 2026-10-09T20:43Z, from another repository's session on an in ## Verification - **Tests:** - - `cargo test --manifest-path crosslink/Cargo.toml --bin crosslink -- hub_v3_operation_tests kickoff::rollback kickoff::cleanup daemon::tests::a_spawned_child lock_check sync:: utils::` (the new tests live in these modules); + - `cargo test --manifest-path crosslink/Cargo.toml --bin crosslink -- hub_v3_operation_tests compaction:: kickoff::rollback kickoff::cleanup daemon::tests::a_spawned_child lock_check sync:: utils:: signing::` (the new tests live in these modules); - `cargo test --manifest-path crosslink/Cargo.toml --test smoke coordination`. - **Before every commit:** `cargo fmt --check` and `cargo clippy -- -D warnings -W clippy::unwrap_used -W clippy::expect_used`. - **Full suite:** CI only. @@ -261,7 +268,7 @@ Field observation, 2026-10-09T20:43Z, from another repository's session on an in ## Rollout and rollback - This increment merges when its tests and review pass. -- Reverting it restores the elapsed-time bail. +- Reverting it restores the elapsed-time bail, and also removes the reducer's late-arrival rule, reopening the two-holder gap REQ-6(b) depends on. - `base_commit` values already written are ignored by older binaries. ## Open questions diff --git a/.design/write-path-cost.md b/.design/write-path-cost.md index d23419339..4c46339bb 100644 --- a/.design/write-path-cost.md +++ b/.design/write-path-cost.md @@ -1,6 +1,6 @@ # Feature: Write-path cost and lock semantics (umbrella) -Umbrella design document, written 2026-10-08 against `origin/develop` at 3440c0d05. Status: reviewed. It was revised after review rounds 1 and 2, and is to be frozen when the first increment's scaffold commit lands. It states the shared problem, the evidence, the increments named by feature and their order, and the decisions that cross increments. Each increment gets its own design document, written when that increment is next to build. Only the first exists: `.design/lock-claim-outcome-and-kickoff-rollback.md`. The entries for later increments are provisional. This document follows `.design/crosslink-architecture-overhead-map.md` and amends `.design/hub-v3-per-agent-refs.md` where the increment documents say so. A bare `#N` is a tracker item, and GitHub records are written in full. Handles are listed in the key at the end. +Umbrella design document, written 2026-10-08 against `origin/develop` at 3440c0d05. Status: reviewed. It was revised after review rounds 1 and 2, and was to be frozen when the first increment's scaffold commit landed. It has since been amended, by dated operator decisions only (the 2026-10-09 reorder and the reduce invariant); later changes follow the same rule. It states the shared problem, the evidence, the increments named by feature and their order, and the decisions that cross increments. Each increment gets its own design document, written when that increment is next to build. Only the first exists: `.design/lock-claim-outcome-and-kickoff-rollback.md`. The entries for later increments are provisional. This document follows `.design/crosslink-architecture-overhead-map.md` and amends `.design/hub-v3-per-agent-refs.md` where the increment documents say so. A bare `#N` is a tracker item, and GitHub records are written in full. Handles are listed in the key at the end. ## Problem @@ -35,6 +35,7 @@ Each figure is marked as measured, estimated or a target. ## Increments, in build order 1. **Lock-claim outcome and kickoff rollback** (Corvidae-Coding-Projects/crosslink#110, and the timeout half of Corvidae-Coding-Projects/crosslink#118). The design document is written: `.design/lock-claim-outcome-and-kickoff-rollback.md`. + - It also carries the reducer rule that a reduce from any checkpoint equals a replay of every event in total order (late arrivals are replayed), which REQ-6(b) depends on. Write cost and lock clearing build on that invariant. Recorded 2026-10-09. 2. **Write cost** (Corvidae-Coding-Projects/crosslink#118). Provisional. - Moved to second on 2026-10-09 (operator decision, from the #820 review): bootstrap and write latency now break the first increment's undo and cleanup on large hubs. The reducer's late-arrival rule (#820) lands before it, and its caches must keep the property test against full replay. - A spawn-count test lands first and records the baseline. @@ -48,6 +49,7 @@ Each figure is marked as measured, estimated or a target. - Constraints found: - The readiness contract forbids "in-memory-only readiness" and requires every write to reject a non-ready repository. So a cached validation must still re-check record age and daemon liveness on every acquisition. - Every new cache needs a property test against full recomputation, proved by removing its invalidation and watching the test fail. + - The reducer's late-arrival replay is not persisted: until a write publishes a covering checkpoint, every read-only reduce replays again (confirmation, reads, the daemon tick, dashboard polls). Persist or memoise the rebuilt state, or rebuild from the newest checkpoint before the late event. 3. **Knowledge cache ownership** (Corvidae-Coding-Projects/crosslink#120, tracker #807). Provisional. - The main checkout's daemon creates the cache after its first ready activation and refreshes it on its sync tick when the remote tip moves. Creation is atomic. - A failure is logged and never affects readiness or the reconcile budget. @@ -80,6 +82,8 @@ Each increment is dispatched as one issue. The number of merges each takes is de ## Cross-increment decisions +- **Reduce equals a total-order replay** (2026-10-09, from the first increment's review). A reduce from any checkpoint must give the same state as replaying every event after the authority baseline in total order. Late arrivals trigger that replay, and `compaction::tests::prop_reduce_from_any_checkpoint_equals_full_replay` is its oracle. Every later increment that caches or shortcuts reduction keeps this property. + - **Steal wire format** (2026-10-08, @magnificentlycursed). The direction was a tolerant decoder that skips unknown event types with a warning, plus a minimum-reader-version gate before any new lock-clearing event is emitted. Rejected: an optional field on `LockReleased`, which old binaries would reduce differently, and a staged rollout without a gate. The precedent check the same day found that a gate already exists: - the generation descriptor's `protocol_version` on `crosslink/reconciliation/current`, checked by exact equality, which every readiness-era binary refuses to become ready on; - the checkpoint schema's fail-closed watermark guard (#798); diff --git a/CHANGELOG.md b/CHANGELOG.md index 71dabaff5..6e1ab6030 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -122,8 +122,16 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). command that clears it. - `kickoff cleanup` releases the agent's locks, then removes the worktree and deletes its branch when the branch has no commits beyond the base recorded - at kickoff; `--keep-branch` keeps it. Cleanup exits non-zero when it leaves - work behind. + at kickoff; `--keep-branch` keeps it. When a lock cannot be released (for + example, the worktree's daemon does not become ready), cleanup and kickoff's + undo keep the worktree and its branch, so the agent identity in it can still + release the lock, and say how to finish or give up. Cleanup now exits + non-zero when it leaves work behind, and its JSON gains `worktree_kept`. +- Clients reducing the hub from different checkpoints no longer disagree when + another agent's earlier event arrives late (for example after a transient + fetch failure): the reduce replays every event in total order, so every + client names the same lock holder and allocates the same display ids. +- Signature checks no longer wait a fixed 50 ms per event for `ssh-keygen`. - `locks steal` and lock auto-steal no longer report success when the steal did not take. - `crosslink container build` no longer fails with a cryptic diff --git a/crosslink/proptest-regressions/compaction.txt b/crosslink/proptest-regressions/compaction.txt new file mode 100644 index 000000000..f07b77a05 --- /dev/null +++ b/crosslink/proptest-regressions/compaction.txt @@ -0,0 +1,7 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc 0d4e4d63dfe5e1292e87cba1496e9b17b3f1c3f840acceeb8871f9a97f395b71 # shrinks to plan = [(0, 2, true, 0), (1, 2, true, 0)], cut = [0, 1, 0] diff --git a/crosslink/src/commands/hub_v3_operation_tests.rs b/crosslink/src/commands/hub_v3_operation_tests.rs index e17a7f3ab..a7a277d93 100644 --- a/crosslink/src/commands/hub_v3_operation_tests.rs +++ b/crosslink/src/commands/hub_v3_operation_tests.rs @@ -1493,6 +1493,11 @@ fn v3_slow_reduce_after_the_fetch_still_confirms() { writer.claim_lock_v2(issue_id, None).unwrap(), crate::shared_writer::LockClaimResult::Claimed ); + assert_eq!( + writer.claim_lock_v2(issue_id, None).unwrap(), + crate::shared_writer::LockClaimResult::AlreadyHeld, + "the already-held path must not bound the reduce either" + ); } /// AC-5 (fails first): an already-held lock is confirmed against the remote, @@ -1646,3 +1651,30 @@ fn v3_undo_release_reports_stranded_and_unverified() { "unreachable remote: got {unverified:?}" ); } + +/// Cleanup's view of what an agent may still hold includes its own recent +/// claims, read from its ref in the shared repository, even when the host's +/// lock view has not caught up. A released claim no longer counts. +#[test] +fn v3_agent_own_claims_are_read_from_its_ref() { + use crate::commands::kickoff::rollback::agent_own_claims; + + if !git_ok() { + return; + } + let hub = setup_migrated_v3_hub(); + let (writer, first) = writer_with_issue(&hub, "Kept claim"); + let db = Database::open(&hub.crosslink_dir.join("issues.db")).unwrap(); + let second = writer + .create_issue(&db, "Released claim", None, "high", None, None) + .unwrap(); + writer.claim_lock_v2(first, None).unwrap(); + writer.claim_lock_v2(second, None).unwrap(); + assert!(writer.release_lock_v2(second).unwrap()); + + let claims = agent_own_claims(hub.work.path(), "alpha").unwrap(); + assert_eq!(claims, vec![first]); + assert!(agent_own_claims(hub.work.path(), "nobody") + .unwrap() + .is_empty()); +} diff --git a/crosslink/src/commands/kickoff/cleanup.rs b/crosslink/src/commands/kickoff/cleanup.rs index 5a0d88f0e..9cf629d36 100644 --- a/crosslink/src/commands/kickoff/cleanup.rs +++ b/crosslink/src/commands/kickoff/cleanup.rs @@ -6,7 +6,7 @@ use std::process::Command; use super::helpers::*; use super::monitor::discover_agents; use super::rollback::{ - host_locks_held_by, kept_worktree_remedy, outcome_message, worktree_agent_id, KickoffUndo, + kept_worktree_remedy, locks_to_release, outcome_message, worktree_agent_id, KickoffUndo, UndoOps, UndoOutcome, UndoStep, }; use super::types::*; @@ -266,8 +266,9 @@ pub fn cleanup(crosslink_dir: &Path, opts: &CleanupOptions) -> Result<()> { let mut released = true; let mut branch_to_delete = None; if worktree_present { - let held = worktree_agent_id(&worktree_path.join(".crosslink")) - .map(|agent_id| host_locks_held_by(&repo_root.join(".crosslink"), &agent_id)); + let held = worktree_agent_id(&worktree_path.join(".crosslink")).map(|agent_id| { + locks_to_release(&repo_root.join(".crosslink"), repo_root, &agent_id) + }); released = release_agent_work(worktree_path, held, &mut ops, &mut result); if !keep_branch { branch_to_delete = branch_and_base(worktree_path); @@ -359,9 +360,10 @@ pub fn cleanup(crosslink_dir: &Path, opts: &CleanupOptions) -> Result<()> { Ok(()) } -/// Releases the agent's locks and ends its session under its own identity. -/// `held` is the host's view of the locks the worktree agent holds (`None` -/// when the worktree has no agent). Returns whether every lock was released; +/// Releases the agent's locks under its own identity. `held` is what it may +/// hold: the host's view plus its own recent claims (`None` when the worktree +/// has no agent). Its session is local state in the worktree and goes with +/// it, so it is not ended separately. Returns whether every lock was released; /// anything left is a warning with its remedy. fn release_agent_work( worktree: &Path, @@ -396,11 +398,6 @@ fn release_agent_work( } } } - let step = UndoStep::EndSession; - let outcome = ops.run(&step); - if let Some(message) = outcome_message(&step, &outcome) { - result.warnings.push(message); - } released } @@ -628,6 +625,39 @@ mod tests { ); } + /// When which locks the agent holds cannot be read (the host's view is + /// unavailable, the #822 situation), the worktree is kept: its identity + /// may still be needed. + #[test] + fn cleanup_keeps_the_worktree_when_the_held_locks_cannot_be_read() { + let (_dir, root, worktree, base) = repo_with_worktree(); + let mut ops = RecordingOps::default(); + let mut result = empty_result(); + let released = release_agent_work( + &worktree, + Some(Err(anyhow::anyhow!("host view unavailable"))), + &mut ops, + &mut result, + ); + assert!(!released); + finish_worktree( + &root, + &worktree, + released, + Some(("feature/x".to_string(), base)), + &mut ops, + &mut result, + ); + assert!(worktree.exists()); + assert!(result.worktree_kept); + let shown = worktree.display().to_string(); + assert!( + result.warnings.iter().any(|w| w.contains(&shown)), + "{:?}", + result.warnings + ); + } + /// When every release completes, the worktree is removed and the /// unchanged branch deleted. #[test] diff --git a/crosslink/src/commands/kickoff/rollback.rs b/crosslink/src/commands/kickoff/rollback.rs index d6a0a49b4..b3cd1cd9a 100644 --- a/crosslink/src/commands/kickoff/rollback.rs +++ b/crosslink/src/commands/kickoff/rollback.rs @@ -32,7 +32,6 @@ pub(crate) enum UndoStep { ReleaseLock { issue_id: i64, }, - EndSession, StopDaemon, RestorePipeline { doc_path: PathBuf, @@ -102,9 +101,6 @@ pub(crate) struct KickoffRollback { worktree: Option<(PathBuf, Origin)>, branch: Option<(String, Origin, Option)>, agent_initialized: bool, - /// The worktree already had an agent (a reused worktree): its session is - /// not this invocation's to end. - agent_preexisting: bool, /// The worktree's daemon was already running before this invocation. daemon_preexisting: bool, claimed_issue: Option, @@ -139,7 +135,6 @@ impl KickoffRollback { /// its daemon was already running. Undo leaves what pre-existed as found. pub(crate) const fn record_existing_agent(&mut self, daemon_was_live: bool) { self.agent_initialized = true; - self.agent_preexisting = true; self.daemon_preexisting = daemon_was_live; } @@ -195,9 +190,6 @@ impl KickoffRollback { if let Some(issue_id) = self.claimed_issue.filter(|_| !self.claim_preexisting) { steps.push(UndoStep::ReleaseLock { issue_id }); } - if !self.agent_preexisting { - steps.push(UndoStep::EndSession); - } if !self.daemon_preexisting { steps.push(UndoStep::StopDaemon); } @@ -330,31 +322,6 @@ impl KickoffUndo<'_> { } } - fn end_session(&self) -> UndoOutcome { - if !self.has_agent() { - return UndoOutcome::NothingToUndo; - } - if let Err(error) = self.ensure_ready() { - return UndoOutcome::Failed { - error, - remedy: format!( - "run `crosslink session end` in {}", - self.worktree_dir.display() - ), - }; - } - command_outcome( - Command::new("crosslink") - .current_dir(self.worktree_dir) - .args(["session", "end"]), - &format!( - "run `crosslink session end` in {}", - self.worktree_dir.display() - ), - &["No active session"], - ) - } - fn stop_daemon(&self) -> UndoOutcome { let crosslink = self.worktree_crosslink(); if !crate::reconcile::readiness::requires_readiness(&crosslink) { @@ -457,7 +424,6 @@ impl UndoOps for KickoffUndo<'_> { &["can't find session", "no server running"], ), UndoStep::ReleaseLock { issue_id } => self.release_lock(*issue_id), - UndoStep::EndSession => self.end_session(), UndoStep::StopDaemon => self.stop_daemon(), UndoStep::RestorePipeline { doc_path } => self.restore_pipeline(doc_path), UndoStep::RemoveWorktree { path } => self.remove_worktree(path), @@ -496,6 +462,64 @@ pub(crate) fn host_locks_held_by(host_crosslink: &Path, agent_id: &str) -> Resul Ok(held) } +/// The issues whose most recent lock event on `agent_id`'s own ref is a +/// claim, read from the repository the worktree shares with the host. No +/// fetch and no daemon: the agent's ref is local. A superset of what the agent +/// holds (a claim it lost to contention still counts), which is the safe side +/// for deciding whether its identity is still needed. +pub(crate) fn agent_own_claims(repo_root: &Path, agent_id: &str) -> Result> { + let reference = crate::hub_v3::agent_ref_name(agent_id)?; + let exists = Command::new("git") + .current_dir(repo_root) + .args(["rev-parse", "--verify", "--quiet", &reference]) + .output()?; + if !exists.status.success() { + return Ok(Vec::new()); + } + let log = Command::new("git") + .current_dir(repo_root) + .args(["show", &format!("{reference}:events.log")]) + .output()?; + anyhow::ensure!( + log.status.success(), + "could not read {reference}:events.log: {}", + String::from_utf8_lossy(&log.stderr).trim() + ); + let mut events = crate::events::read_events_from_bytes(&log.stdout)?; + events.sort_by_key(|event| event.agent_seq); + let mut claimed = std::collections::BTreeSet::new(); + for event in events { + match event.event { + crate::events::Event::LockClaimed { + issue_display_id, .. + } => { + claimed.insert(issue_display_id); + } + crate::events::Event::LockReleased { issue_display_id } => { + claimed.remove(&issue_display_id); + } + _ => {} + } + } + Ok(claimed.into_iter().collect()) +} + +/// The locks a cleanup must release before it may remove the worktree: what +/// the host's view shows the agent holding, plus the agent's own recent +/// claims (the host's view may not have seen them yet). An error means it +/// cannot be known, and the worktree must be kept. +pub(crate) fn locks_to_release( + host_crosslink: &Path, + repo_root: &Path, + agent_id: &str, +) -> Result> { + let mut held = host_locks_held_by(host_crosslink, agent_id)?; + held.extend(agent_own_claims(repo_root, agent_id)?); + held.sort_unstable(); + held.dedup(); + Ok(held) +} + /// Whether the worktree's daemon is gone, or goes within `timeout`. fn daemon_gone_within(crosslink: &Path, timeout: std::time::Duration) -> bool { let deadline = std::time::Instant::now() + timeout; @@ -602,7 +626,6 @@ impl UndoStep { Self::RemoveContainer { name, .. } => format!("removing container {name}"), Self::KillTmuxSession { name } => format!("killing tmux session {name}"), Self::ReleaseLock { issue_id } => format!("releasing the lock on #{issue_id}"), - Self::EndSession => "ending the worktree session".to_string(), Self::StopDaemon => "stopping the worktree daemon".to_string(), Self::RestorePipeline { doc_path } => { format!("restoring the pipeline state of {}", doc_path.display()) @@ -659,11 +682,7 @@ mod tests { } fn tail_steps() -> Vec { - vec![ - UndoStep::ReleaseLock { issue_id: 7 }, - UndoStep::EndSession, - UndoStep::StopDaemon, - ] + vec![UndoStep::ReleaseLock { issue_id: 7 }, UndoStep::StopDaemon] } fn removal_steps() -> Vec { @@ -718,7 +737,7 @@ mod tests { name: "feat-x".to_string() }) ); - assert_eq!(steps.len(), 6); + assert_eq!(steps.len(), 5); } /// AC-9: a reused worktree and branch are never removed. @@ -909,7 +928,6 @@ mod tests { .map(|(step, _)| step.clone()) .collect(); assert!(nothing.contains(&UndoStep::ReleaseLock { issue_id: 7 })); - assert!(nothing.contains(&UndoStep::EndSession)); } /// AC-13: undo never touches the host-side identity: the agent's key diff --git a/crosslink/src/compaction.rs b/crosslink/src/compaction.rs index 9877ac1fc..a30937dce 100644 --- a/crosslink/src/compaction.rs +++ b/crosslink/src/compaction.rs @@ -193,7 +193,16 @@ pub fn reduce(source: &dyn HubSource) -> Result { // a lock claim would lose to a later claim already in the checkpoint, and // display ids would be allocated in a different order, so clients reducing // from different checkpoints would disagree. Rebuild in total order then. + // The rebuilt state is not persisted here: until a write publishes a + // checkpoint covering the late event, each reduce replays again (a known + // cost, recorded for the write-cost increment), so make it visible. if arrives_late(&histories, &state.frontier, all_events.first()) { + tracing::debug!( + "late arrival from '{}' (seq {}): replaying {} unseen events in total order", + all_events[0].agent_id, + all_events[0].agent_seq, + all_events.len() + ); return rebuild_with_histories(source, &histories); } @@ -1336,23 +1345,52 @@ mod tests { compact(cache_dir, agent_id, force, &lock) } + /// What a generated event does: claim or release a lock, or create an + /// issue (whose display id is allocated in apply order). + #[derive(Debug, Clone, Copy)] + enum Act { + Claim, + Release, + Create, + } + + fn act(kind: u8) -> Act { + match kind % 3 { + 0 => Act::Claim, + 1 => Act::Release, + _ => Act::Create, + } + } + /// Writes a gap-free event sequence per agent from `plan` entries of - /// (agent index, issue, claim or release, timestamp offset). - fn lock_events(plan: &[(usize, i64, bool, i64)]) -> Vec { + /// (agent index, issue, act, timestamp offset). + fn plan_events(plan: &[(usize, i64, Act, i64)]) -> Vec { const AGENTS: [&str; 3] = ["agent-a", "agent-b", "agent-c"]; let mut sequences = [0_u64; 3]; plan.iter() - .map(|&(agent, issue, claim, offset)| { + .enumerate() + .map(|(index, &(agent, issue, act, offset))| { sequences[agent] += 1; - let event = if claim { - Event::LockClaimed { + let event = match act { + Act::Claim => Event::LockClaimed { issue_display_id: issue, branch: None, - } - } else { - Event::LockReleased { + }, + Act::Release => Event::LockReleased { issue_display_id: issue, - } + }, + Act::Create => Event::IssueCreated { + uuid: Uuid::from_u128(index as u128 + 1), + title: format!("issue {index}"), + description: None, + priority: "medium".to_string(), + labels: Vec::new(), + parent_uuid: None, + created_by: AGENTS[agent].to_string(), + display_id: None, + scheduled_at: None, + due_at: None, + }, }; let mut envelope = make_envelope(AGENTS[agent], sequences[agent], event); envelope.timestamp = @@ -1362,6 +1400,21 @@ mod tests { .collect() } + fn lock_events(plan: &[(usize, i64, bool, i64)]) -> Vec { + let plan: Vec<_> = plan + .iter() + .map(|&(agent, issue, claim, offset)| { + ( + agent, + issue, + if claim { Act::Claim } else { Act::Release }, + offset, + ) + }) + .collect(); + plan_events(&plan) + } + fn append_all<'a>(dir: &Path, events: impl IntoIterator) { for event in events { append_event( @@ -1372,30 +1425,56 @@ mod tests { } } - /// Reduces `events` from a checkpoint cut after the first `cut[agent]` - /// events of each agent, with the rest arriving afterwards. - fn reduce_from_cut(events: &[EventEnvelope], cut: &[usize]) -> CheckpointState { - let dir = tempfile::tempdir().unwrap(); - setup_cache(dir.path()); - let agent_index = |id: &str| match id { + fn agent_index(id: &str) -> usize { + match id { "agent-a" => 0, "agent-b" => 1, _ => 2, - }; - let mut seen = [0_usize; 3]; - let (covered, late): (Vec<_>, Vec<_>) = events.iter().partition(|event| { - let index = agent_index(&event.agent_id); - seen[index] += 1; - seen[index] <= cut[index] - }); - if !covered.is_empty() { - append_all(dir.path(), covered); - compact_t(dir.path(), "agent-a", true).unwrap(); } - append_all(dir.path(), late); + } + + /// Reduces `events` after checkpointing at each cut in turn (a cut is the + /// number of each agent's events written before compacting), with the + /// rest arriving afterwards. Cuts must not shrink. + fn reduce_from_cuts(events: &[EventEnvelope], cuts: &[[usize; 3]]) -> CheckpointState { + let dir = tempfile::tempdir().unwrap(); + setup_cache(dir.path()); + let mut written = [0_usize; 3]; + for cut in cuts { + let mut seen = [0_usize; 3]; + let batch: Vec<_> = events + .iter() + .filter(|event| { + let index = agent_index(&event.agent_id); + seen[index] += 1; + seen[index] > written[index] && seen[index] <= cut[index] + }) + .collect(); + for event in &batch { + written[agent_index(&event.agent_id)] += 1; + } + if !batch.is_empty() { + append_all(dir.path(), batch); + compact_t(dir.path(), "agent-a", true).unwrap(); + } + } + let mut seen = [0_usize; 3]; + let rest: Vec<_> = events + .iter() + .filter(|event| { + let index = agent_index(&event.agent_id); + seen[index] += 1; + seen[index] > written[index] + }) + .collect(); + append_all(dir.path(), rest); reduce(&WorktreeSource::new(dir.path())).unwrap().state } + fn reduce_from_cut(events: &[EventEnvelope], cut: &[usize]) -> CheckpointState { + reduce_from_cuts(events, &[[cut[0], cut[1], cut[2]]]) + } + /// The late arrival found by the #820 contention test: agent-b claims /// first, agent-a's checkpoint is written with only its own later claim, /// then agent-b's claim arrives. Every reader must name agent-b. @@ -1412,21 +1491,37 @@ mod tests { proptest! { #![proptest_config(ProptestConfig::with_cases(256))] - /// Reducing from any checkpoint cut, with the remaining events + /// Reducing from any checkpoint cuts, with the remaining events /// arriving late, gives the same state as a full replay in total - /// order: contended locks, releases by non-holders, timestamp ties, - /// inversions and per-agent clock skew included. + /// order: contended locks, releases by non-holders, issue creation + /// (display ids allocated in apply order), timestamp ties, inversions, + /// per-agent clock skew, and a second checkpoint written after a + /// first. The oracle is `reduce` replaying from empty, the same total + /// order checkpoint verification uses. #[test] fn prop_reduce_from_any_checkpoint_equals_full_replay( - plan in prop::collection::vec((0_usize..3, 1_i64..3, any::(), 0_i64..4), 1..12), - cut in prop::collection::vec(0_usize..12, 3), + plan in prop::collection::vec((0_usize..3, 1_i64..3, 0_u8..3, 0_i64..4), 1..12), + first in prop::array::uniform3(0_usize..12), + second in prop::array::uniform3(0_usize..12), ) { - let events = lock_events(&plan); - let full = reduce_from_cut(&events, &[0, 0, 0]); - let incremental = reduce_from_cut(&events, &cut); + let plan: Vec<_> = plan + .into_iter() + .map(|(agent, issue, kind, offset)| (agent, issue, act(kind), offset)) + .collect(); + let events = plan_events(&plan); + let later = [ + first[0].max(second[0]), + first[1].max(second[1]), + first[2].max(second[2]), + ]; + let full = reduce_from_cuts(&events, &[]); + let incremental = reduce_from_cuts(&events, &[first, later]); prop_assert!( checkpoint_semantics_equal(&full, &incremental).unwrap(), - "full locks {:?} vs incremental {:?}", + "plan {:?}\ncuts {:?} then {:?}\nfull locks {:?} vs incremental {:?}", + plan, + first, + later, full.locks, incremental.locks ); diff --git a/crosslink/src/signing.rs b/crosslink/src/signing.rs index ed79b3667..21568db8f 100644 --- a/crosslink/src/signing.rs +++ b/crosslink/src/signing.rs @@ -613,10 +613,11 @@ pub fn verify_content( ]) .arg(&sig_path) .stdin(std::fs::File::open(&content_path).context("opening signed content")?); - let Some(output) = - crate::utils::command_output_with_timeout(&mut command, std::time::Duration::from_secs(30)) - else { - bail!("ssh-keygen verification could not run or did not finish within 30 seconds"); + let Some(output) = crate::utils::command_complete_output_with_timeout( + &mut command, + std::time::Duration::from_secs(30), + ) else { + bail!("ssh-keygen verification could not run, did not finish within 30 seconds, or its output was incomplete"); }; if !output.status.success() { diff --git a/crosslink/src/utils.rs b/crosslink/src/utils.rs index 36e253b25..3eaf19b74 100644 --- a/crosslink/src/utils.rs +++ b/crosslink/src/utils.rs @@ -424,6 +424,24 @@ pub fn parse_due_date(s: &str) -> Result, String> { /// grace period, so no helper (a remote helper, ssh, index-pack) outlives the /// call. Elsewhere only the direct child is killed. pub fn command_output_with_timeout(command: &mut Command, timeout: Duration) -> Option { + run_with_timeout(command, timeout).map(|(output, _complete)| output) +} + +/// Like [`command_output_with_timeout`], but only with complete output. +/// +/// Returns `None` unless both streams arrived in full, for callers that decide +/// on the output's content: a signature check must not read missing output as +/// a bad signature. +pub fn command_complete_output_with_timeout( + command: &mut Command, + timeout: Duration, +) -> Option { + run_with_timeout(command, timeout).and_then(|(output, complete)| complete.then_some(output)) +} + +/// Runs the command; returns its output and whether both streams arrived in +/// full. +fn run_with_timeout(command: &mut Command, timeout: Duration) -> Option<(Output, bool)> { #[cfg(unix)] { use std::os::unix::process::CommandExt; @@ -456,16 +474,25 @@ pub fn command_output_with_timeout(command: &mut Command, timeout: Duration) -> // The command has exited. A process it left behind (an ssh // ControlPersist master, say) may still hold a pipe open; wait briefly for // the output, then return without it rather than kill that process. - let wait = deadline - .saturating_duration_since(Instant::now()) - .min(Duration::from_secs(1)); - let stdout = stdout.recv_timeout(wait).unwrap_or_default(); - let stderr = stderr.recv_timeout(wait).unwrap_or_default(); - Some(Output { - status, - stdout, - stderr, - }) + let output_deadline = Instant::now() + + deadline + .saturating_duration_since(Instant::now()) + .min(Duration::from_secs(1)); + let stdout = stdout + .recv_timeout(output_deadline.saturating_duration_since(Instant::now())) + .ok(); + let stderr = stderr + .recv_timeout(output_deadline.saturating_duration_since(Instant::now())) + .ok(); + let complete = stdout.is_some() && stderr.is_some(); + Some(( + Output { + status, + stdout: stdout.unwrap_or_default(), + stderr: stderr.unwrap_or_default(), + }, + complete, + )) } /// Reads a pipe to its end on a background thread. @@ -545,6 +572,31 @@ mod tests { assert_eq!(output.stdout.len(), 300_000); } + /// A process left behind holding a pipe open does not extend the call: + /// the lenient helper returns the exit status, the strict one reports the + /// output as incomplete. + #[cfg(unix)] + #[test] + fn a_leftover_pipe_holder_does_not_extend_the_call() { + let started = Instant::now(); + let output = command_output_with_timeout( + Command::new("sh").args(["-c", "sleep 5 & exit 0"]), + Duration::from_secs(10), + ) + .expect("returns once the command has exited"); + assert!(output.status.success()); + assert!( + started.elapsed() < Duration::from_secs(3), + "took {:?}", + started.elapsed() + ); + assert!(command_complete_output_with_timeout( + Command::new("sh").args(["-c", "sleep 5 & exit 0"]), + Duration::from_secs(10), + ) + .is_none()); + } + #[cfg(unix)] #[test] fn command_output_with_timeout_gives_up_at_the_deadline() { From c01888bcd31d823abde4ae2aaa071fc4ca6d9394 Mon Sep 17 00:00:00 2001 From: magnificentlycursed <190320250+magnificentlycursed@users.noreply.github.com> Date: Fri, 9 Oct 2026 15:58:02 -0700 Subject: [PATCH 14/14] chore(plugins): regenerate the Codex plugin assets for the guide's cleanup line The crosslink-guide skill gained a --keep-branch note; regenerate its Codex plugin copy and the generated-assets manifest (scripts/sync-codex-plugin.py). --- .../resources/plugins/crosslink-codex/generated-assets.json | 2 +- .../plugins/crosslink-codex/skills/crosslink-guide/SKILL.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crosslink/resources/plugins/crosslink-codex/generated-assets.json b/crosslink/resources/plugins/crosslink-codex/generated-assets.json index 4a3f8e07c..8d92c6b28 100644 --- a/crosslink/resources/plugins/crosslink-codex/generated-assets.json +++ b/crosslink/resources/plugins/crosslink-codex/generated-assets.json @@ -17,7 +17,7 @@ "skills/audit/SKILL.md": "1d39b8ad12ac78566cb80ef2573b1210c9ba20befe609c91e0d5a8021ecff93c", "skills/check/SKILL.md": "41ac06446aa1ab10d78938c240e3d27f0bd386d368d8b5b61da8698dc29300cb", "skills/commit/SKILL.md": "8f68838eb18436b4a9176e2a592a43697e0339e8e1051f6d08bc0268c7de2c70", - "skills/crosslink-guide/SKILL.md": "b84d9ae293be1d7c6b5bb1001e9e4c4f7b6214c220bea4e56f451e8224728c5b", + "skills/crosslink-guide/SKILL.md": "876658a5d1094606b1a23a1c4d2075f72b5225b0ae8b347b456bc50e59460c3f", "skills/design/SKILL.md": "9bdf425da96922fe5bcc9de244e84a7f2b6f4c752270651b3fa3c02a9efb36d8", "skills/dev-release/SKILL.md": "15dfa94a164c37fe04ceba52f9906b4541a544493dfdfaf407608a369a4b75d6", "skills/featree/SKILL.md": "0e59afa559f2542587610d3a956dddc65d5c2d808ea046010173bcd94369ae61", diff --git a/crosslink/resources/plugins/crosslink-codex/skills/crosslink-guide/SKILL.md b/crosslink/resources/plugins/crosslink-codex/skills/crosslink-guide/SKILL.md index ccc695939..7b8de64e1 100644 --- a/crosslink/resources/plugins/crosslink-codex/skills/crosslink-guide/SKILL.md +++ b/crosslink/resources/plugins/crosslink-codex/skills/crosslink-guide/SKILL.md @@ -86,7 +86,7 @@ crosslink kickoff status crosslink kickoff logs crosslink kickoff report crosslink kickoff stop -crosslink kickoff cleanup +crosslink kickoff cleanup # releases locks; --keep-branch keeps unchanged branches crosslink swarm init crosslink swarm status crosslink swarm launch