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..34fdc8f20 --- /dev/null +++ b/.design/lock-claim-outcome-and-kickoff-rollback.md @@ -0,0 +1,293 @@ +# 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 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`); + - 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. 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; + 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. 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. + + 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): 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 (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) +- [ ] 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 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) +- [ ] 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.** 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. + +## 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 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. 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): + +- **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). +- **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. + +Where each criterion is covered: + +- **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 (`v3_contention_found_during_confirmation_is_contended`). +- **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; + - disarming; + - failure reporting; + - the branch rule; + - the pipeline restore through `undo_failed_kickoff`; + - 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`. + +Not covered by automated tests: + +- **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. +- **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: + +- **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". 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 + +- **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 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, 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 + +- 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 --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. +- **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, 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 + +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..4c46339bb --- /dev/null +++ b/.design/write-path-cost.md @@ -0,0 +1,126 @@ +# 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 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 + +- **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`. + - 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. + - 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. + - 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. + - 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. + - 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 + +- **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); + - 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`. diff --git a/CHANGELOG.md b/CHANGELOG.md index b2213bb50..6e1ab6030 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -107,6 +107,33 @@ 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. 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 `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/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/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/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 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..a7a277d93 100644 --- a/crosslink/src/commands/hub_v3_operation_tests.rs +++ b/crosslink/src/commands/hub_v3_operation_tests.rs @@ -1258,3 +1258,423 @@ 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`. + +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(repo) + .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) +} + +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() { + return; + } + let hub = setup_migrated_v3_hub(); + 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(5)); + failpoints.publish_delay = std::time::Duration::from_secs(6); + } + + assert_eq!( + writer.claim_lock_v2(issue_id, None).unwrap(), + crate::shared_writer::LockClaimResult::Claimed + ); + assert_eq!( + lock_claims_on_agent_ref(hub.remote.path(), "alpha", issue_id), + 1, + "the remote must hold the claim" + ); +} + +/// 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!( + is_unconfirmed(&first), + "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.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] +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(ref r) if is_unconfirmed(r)), + "expected Ok(Unconfirmed), got {result:?}" + ); +} + +/// 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_its_group_terminated() { + 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 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(), script]); + } + + let started = std::time::Instant::now(); + let result = writer.claim_lock_v2(issue_id, None).unwrap(); + assert!( + is_unconfirmed(&result), + "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(), + "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(); + 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(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, + "the already-held path must not bound the reduce either" + ); +} + +/// 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!( + 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] +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()) + ); +} + +/// 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:?}" + ); +} + +/// 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 4fa42eab0..9cf629d36 100644 --- a/crosslink/src/commands/kickoff/cleanup.rs +++ b/crosslink/src/commands/kickoff/cleanup.rs @@ -5,15 +5,30 @@ use std::process::Command; use super::helpers::*; use super::monitor::discover_agents; +use super::rollback::{ + kept_worktree_remedy, locks_to_release, outcome_message, worktree_agent_id, 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 +200,10 @@ pub fn cleanup( worktree_removed: false, tmux_killed: false, container_removed: false, + locks_released: Vec::new(), + branch_deleted: false, + worktree_kept: false, + warnings: Vec::new(), error: None, }; @@ -232,6 +251,30 @@ pub fn cleanup( } } + // With the agent's processes gone, release its locks and end its + // 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 { + 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); + } + } + if !agent.worktree.is_empty() { if let Some(root) = crosslink_dir.parent() { let pipeline_status = match agent.status.as_str() { @@ -247,38 +290,33 @@ pub fn cleanup( } } - 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}"); - } + if worktree_present { + finish_worktree( + repo_root, + 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}"); } - match Command::new("git") - .args(["worktree", "remove", "--force", &agent.worktree]) - .output() - { - Ok(o) if o.status.success() => { - result.worktree_removed = true; - 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); - } + } + + 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}"); } } @@ -310,5 +348,339 @@ pub fn cleanup( 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(()) } + +/// 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, + 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 the agent holds: {error:#}; {}", + kept_worktree_remedy(worktree, None) + )); + return false; + } + }; + if held.is_empty() { + return true; + } + 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 { + released &= outcome.is_clean(); + 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. +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::*; + use crate::commands::kickoff::rollback::RecordingOps; + + 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 = 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, + tmux_killed: false, + 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() + }; + 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 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] + 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/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 ea076a3df..1713dda9d 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; +pub(crate) mod rollback; mod run; mod types; mod wizard; @@ -23,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}; @@ -191,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 8bf81a52d..5be5206b8 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,114 +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(), - }; - 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.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 new file mode 100644 index 000000000..b3cd1cd9a --- /dev/null +++ b/crosslink/src/commands/kickoff/rollback.rs @@ -0,0 +1,1057 @@ +//! 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`. + +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(crate) enum Origin { + Created, + Reused, +} + +/// One undo action, in the order `KickoffRollback::steps` returns them. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) 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, + }, + StopDaemon, + RestorePipeline { + doc_path: PathBuf, + }, + RemoveWorktree { + path: PathBuf, + }, + DeleteBranch { + name: String, + base_commit: String, + }, +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) 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, + }, + /// 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)] +pub(crate) struct UndoReport { + pub(crate) entries: Vec<(UndoStep, UndoOutcome)>, +} + +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)| outcome.is_clean()) + } +} + +/// Performs one undo step. The kickoff implementation runs subprocesses; +/// tests record the calls. +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(crate) struct KickoffRollback { + worktree: Option<(PathBuf, Origin)>, + branch: Option<(String, Origin, Option)>, + agent_initialized: 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)>, + disarmed: bool, +} + +impl KickoffRollback { + pub(crate) fn new() -> Self { + Self::default() + } + + pub(crate) fn record_worktree(&mut self, path: PathBuf, origin: Origin) { + self.worktree = Some((path, origin)); + } + + 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, 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.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(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(crate) fn record_pipeline(&mut self, doc_path: PathBuf, prior: Option) { + self.pipeline = Some((doc_path, prior)); + } + + pub(crate) fn record_tmux_session(&mut self, name: String) { + self.tmux_session = Some(name); + } + + 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(crate) const fn disarm(&mut self) { + self.disarmed = true; + } + + /// The undo steps for what was recorded, in the order they must run. + pub(crate) fn steps(&self) -> Vec { + 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.filter(|_| !self.claim_preexisting) { + steps.push(UndoStep::ReleaseLock { issue_id }); + } + if !self.daemon_preexisting { + 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(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. + /// 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 = 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(); + UndoReport { entries } + } +} + +/// Runs undo steps against the system for one kickoff worktree. +pub(crate) struct KickoffUndo<'a> { + pub(crate) repo_root: &'a Path, + pub(crate) worktree_dir: &'a Path, + pub(crate) 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() + } + + /// 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:#}")), + } + } + + /// 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; + } + let remedy = format!( + "run `crosslink locks release {issue_id}` in {}", + self.worktree_dir.display() + ); + if let Err(error) = self.ensure_ready() { + return UndoOutcome::Failed { error, remedy }; + } + 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::Unverified { + remedy: format!("{remedy}{release_note}; confirmation unavailable: {error:#}"), + } + } + }; + match writer.confirmed_lock_holder(issue_id) { + 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}{release_note}; confirmation failed: {error:#}"), + }, + } + } + + 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, + // `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!( + "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::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), + } + } +} + +/// 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) +} + +/// 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; + 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 { + 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 { + /// 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; + } + 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 { + "kickoff undo: some steps did not complete; see above".to_string() + }); + Some(lines.join("\n")) + } +} + +/// 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 { + 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::Kept { remedy } => Some(format!("{} skipped: {remedy}", step.describe())), + UndoOutcome::Unverified { remedy } => Some(format!( + "{}: release unverified; 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::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(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::*; + + 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::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(), + }, + ] + } + + /// 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(), 5); + } + + /// 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-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() { + 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 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 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::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!(!report.is_clean()); + } + + 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 })); + } + + /// 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() { + 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))); + 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/run.rs b/crosslink/src/commands/kickoff/run.rs index 7c1e14270..21333ffec 100644 --- a/crosslink/src/commands/kickoff/run.rs +++ b/crosslink/src/commands/kickoff/run.rs @@ -7,6 +7,10 @@ use crate::db::Database; use super::helpers::*; use super::launch::*; use super::prompt::*; +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( @@ -137,154 +141,263 @@ 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)?; - } + { + 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")?; + } - exclude_kickoff_files(&worktree_dir)?; + 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)?; + } - let agent_id = - init_worktree_agent(&worktree_dir, crosslink_dir, &compact_name, Some(issue_id))?; + exclude_kickoff_files(&worktree_dir)?; - 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}"); + record_activation( + &mut rollback, + reused, + crosslink_dir, + &worktree_dir, + 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.map_err(|error| with_undo_report(error, rollback, &root, &worktree_dir)) +} + +/// Undoes what a failed kickoff created and returns the original error with +/// the undo report after it, so the error reads first and callers that only +/// log the error (swarm, sentinel) keep the report and its remedies. +pub(super) fn with_undo_report( + error: anyhow::Error, + rollback: KickoffRollback, + root: &Path, + worktree_dir: &Path, +) -> anyhow::Error { + match undo_failed_kickoff(rollback, root, worktree_dir) { + Some(report) => anyhow::anyhow!("{error:#}\n{report}"), + None => error, } +} - Ok(compact_name) +/// 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(), + }; + 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); + } +} + +/// 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 21a7c728d..80110af82 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, } } } @@ -402,6 +409,11 @@ pub(super) struct CleanupResult { pub worktree_removed: bool, pub tmux_killed: bool, 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, } diff --git a/crosslink/src/commands/locks_cmd.rs b/crosslink/src/commands/locks_cmd.rs index 8668867ce..169117e2c 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(()) } @@ -205,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), @@ -218,6 +241,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/compaction.rs b/crosslink/src/compaction.rs index 6b89f8337..a30937dce 100644 --- a/crosslink/src/compaction.rs +++ b/crosslink/src/compaction.rs @@ -186,6 +186,26 @@ 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. + // 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); + } + let events_processed = all_events.len(); let mut changed_issues: HashSet = HashSet::new(); let mut changed_locks: HashSet = HashSet::new(); @@ -233,10 +253,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 +328,7 @@ pub fn rebuild_from_authority(source: &dyn HubSource) -> Result 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, 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() + .enumerate() + .map(|(index, &(agent, issue, act, offset))| { + sequences[agent] += 1; + let event = match act { + Act::Claim => Event::LockClaimed { + issue_display_id: issue, + branch: None, + }, + 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 = + chrono::DateTime::::UNIX_EPOCH + Duration::seconds(offset); + envelope + }) + .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( + &dir.join("agents").join(&event.agent_id).join("events.log"), + event, + ) + .unwrap(); + } + } + + fn agent_index(id: &str) -> usize { + match id { + "agent-a" => 0, + "agent-b" => 1, + _ => 2, + } + } + + /// 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. + #[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 cuts, with the remaining events + /// arriving late, gives the same state as a full replay in total + /// 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, 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 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(), + "plan {:?}\ncuts {:?} then {:?}\nfull locks {:?} vs incremental {:?}", + plan, + first, + later, + full.locks, + incremental.locks + ); + } + } + proptest! { #![proptest_config(ProptestConfig::with_cases(32))] @@ -1322,7 +1558,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)); diff --git a/crosslink/src/daemon.rs b/crosslink/src/daemon.rs index b47a81e9c..fdd0feb11 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; @@ -1192,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 0defa57d5..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)" ); @@ -183,7 +193,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 +217,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) @@ -903,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; @@ -919,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/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 8b4ea5543..b1c4322e1 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,40 @@ 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)] +#[derive(Debug, Default)] +pub(crate) struct ClaimFailpoints { + /// 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. + 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). + 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. +#[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 +72,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 +135,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()), })) } @@ -243,11 +283,40 @@ 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)?; + #[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| { + crate::hydration::hydrate_current_authority_under_operation( + &self.readiness_dir, + &db, + ) + }) + .err(); + } + Ok(published) } pub fn write_agent_request( @@ -516,8 +585,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(); @@ -571,18 +656,39 @@ impl SharedWriter { return Err(error); } - if self.sync.remote_exists() { + 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)] + { + let failpoints = self.failpoints.borrow(); + std::thread::sleep(failpoints.publish_delay); + if failpoints.fail_after_push { + bail!("injected failure after push"); + } + } + + #[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(); } 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) { @@ -679,13 +785,75 @@ impl SharedWriter { } } - pub(super) fn confirm_v3_locks(&self) -> Result<()> { - if let Err(e) = self.sync.fetch() { - tracing::warn!("v3 lock confirm: fetch failed ({e}); confirming against local view"); + /// 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)] + 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 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`. + 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_agent_tips() + .context("lock confirmation could not adopt the fetched agent refs")?; + drop(lock); } + #[cfg(test)] + std::thread::sleep(self.failpoints.borrow().slow_confirm_reduce); self.refresh_v3_state() } + 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 { self.last_v3_state .borrow() diff --git a/crosslink/src/shared_writer/locks.rs b/crosslink/src/shared_writer/locks.rs index d3b8a8f46..8ce8773f1 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 { @@ -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 { @@ -21,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, @@ -28,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 => {} + } } } @@ -36,20 +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(); - 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)? { @@ -67,10 +91,28 @@ 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 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)? { @@ -168,3 +210,9 @@ impl SharedWriter { Ok(Some(lock)) } } + +fn unconfirmed(error: &anyhow::Error) -> LockClaimResult { + LockClaimResult::Unconfirmed { + cause: format!("{error:#}"), + } +} diff --git a/crosslink/src/signing.rs b/crosslink/src/signing.rs index 1c93b5b83..21568db8f 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,13 @@ 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_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() { return Ok(false); diff --git a/crosslink/src/sync/cache.rs b/crosslink/src/sync/cache.rs index a1adf377c..816f98be9 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,7 +153,7 @@ 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 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: {}", std::fs::read_to_string(lock_path) @@ -167,9 +170,9 @@ fn acquire_hub_lock_with_timeout(lock_path: &Path, max_wait: Duration) -> Result let _ = std::fs::remove_file(lock_path); 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" - ), + Err(_) => { + bail!("Hub lock could not be acquired after waiting and force-removal") + } } } std::thread::sleep(poll_interval); @@ -185,6 +188,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 +555,40 @@ 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 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) + .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 + } + pub(crate) fn fetch_and_adopt_v3_refs(&self) { let fetch_result = self.git_in_cache(&[ "fetch", @@ -556,19 +599,49 @@ 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. A tip that fails to adopt is + /// logged and skipped. + pub(crate) fn adopt_fetched_v3_refs(&self) -> Result<()> { + 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, + /// 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() .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")?; + let mut failures = Vec::new(); for (agent_id, remote_tip) in tips { if own_agent_id.as_deref() == Some(agent_id.as_str()) { continue; @@ -576,11 +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}")); } } - - self.adopt_checkpoint_by_frontier(); + Ok(failures) } fn list_remote_agent_tips(&self) -> Result> { diff --git a/crosslink/src/utils.rs b/crosslink/src/utils.rs index 36d6863d0..3eaf19b74 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,9 +411,207 @@ 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 +/// 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; + command.process_group(0); + } + let mut child = command + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .ok()?; + 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(pause); + pause = (pause * 2).min(Duration::from_millis(20)); + } + Ok(None) | Err(_) => { + terminate_child(&mut child); + 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 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. +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)] +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::*; + + #[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); + } + + /// 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() { + 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;