fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

The dirty-worktree check runs before the worker is stopped, so work written during teardown is deleted with no preserve and no snapshot #316

New Issue
Closed
opened 2026-09-04 08:48:06 +02:00 by ltms · 1 comment
ltms commented 2026-09-04 08:48:06 +02:00
Owner
Copy Link
Copy Source

Found by a delegated hunter. I read the whole method and confirmed the structure, and I extended the consequence — the hunter reported half of it.

Not observed. Read the severity honestly

I have not seen this happen, and I cannot point at a lost file. What follows is a structural TOCTOU with a narrow window. It is worth fixing because the direction of harm is data loss and the fix is cheap, not because it is known to have cost us anything. Do not write it up as an incident.

The order

SessionManager.releaseRemoved decides, stops, then deletes:

boolean dirty = removed.worktree() != null && worktrees.hasUncommitted(removed.worktree());
...
} else if (dirty) {
    preserveWorktree = true;                       // CB-576
}
if (dirty) {
    snapshotRef = trySnapshot(removed, cause);     // CB-578 stage C
}
...
launcher.stop(paneId);                             // the writer stops HERE
if (removed != null && !preserveWorktree && removed.worktree() != null) {
    worktrees.remove(worktrees.repoRoot(removed.cwd()), removed.worktree());   // --force
}

dirty is computed while the worker is still running and is then used, unchanged, to authorise a git worktree remove --force that runs after the worker is stopped. Nothing re-reads it.

Why this is worse than "we lose the preserve"

The hunter reported the preserve. The same stale boolean also gates the snapshot:

if (dirty) {
    snapshotRef = trySnapshot(removed, cause);
}

So in the racing case the worktree is not preserved and not snapshotted into refs/wip/*. CB-578 stage C exists precisely so that preserving on disk is not the only copy. Here both defences are gated on the same stale read, so they fail together. There is no third layer: the pane is gone, the registry entry is gone, and the directory is force-deleted.

The path in

  1. A worker commits its work. Its tree is clean.
  2. fleet_stop{paneId} (or the reaper, or a failed turn) calls release.
  3. hasUncommitted shells out to git status and returns false. dirty = false, preserveWorktree = false, no snapshot.
  4. The worker — still alive; nothing has stopped it yet — writes a new file. A worker that just committed and moved on to the next edit is the ordinary case, not a contrived one.
  5. launcher.stop(paneId) closes the pane.
  6. worktrees.remove(...) runs git worktree remove --force. The new file is gone.

Direction of harm: data loss — the worst direction in this area, and the one CB-576 and CB-578 were both written to prevent.

Window: from hasUncommitted returning to remove starting. That covers memberLifecycle.released, the resolveAgentSessionId call, the release-listener notification, and the whole of launcher.stop (a herdr RPC). Tens to hundreds of milliseconds, not microseconds.

One thing I did not establish: whether agents.close(paneId) guarantees the agent process is dead before it returns. I read HerdrPeerLauncher.stop and it issues the close and moves on; whether the child is reaped synchronously is inside herdr. If it is not, the window extends past step 5 and a write can land after the pane is closed. Check this before you decide where the re-check goes — say what you found rather than assuming either way.

Why nothing else catches it

  • The catch (RuntimeException e) around the dirty check already encodes the right instinct — "fail toward the safe answer and preserve it — deleting on a guess can destroy work that has no other copy". That is the correct rule; it just is not applied to a result that has gone stale rather than thrown.
  • worktrees.remove is --force by design and does not consult the tree's state.
  • The refs/wip snapshot is gated on the same dirty, as above.

What I want

Goal: the decision to delete a worktree must be based on the tree's state at the moment of deletion, with the worker no longer able to write to it.

Invariants:

  1. Fail toward preserving. If the state cannot be determined, keep the worktree. That is already this method's rule for an exception and must stay the rule for anything you add.
  2. ReleaseCause.SHUTDOWN still preserves unconditionally. Do not make a shutdown depend on a git status that might now say clean.
  3. A slow or failing check must not stop the pane teardown or escape release. The pane must always stop — that is CB-581, and an orphaned pane is its own kind of harm. Whatever you add is best-effort in exactly the way the surrounding steps are.
  4. Do not add a second unconditional git status to every release. This runs on every teardown of every member. If you re-check, re-check only on the path that is actually about to delete something.

Candidate mechanism, as a candidate only: re-run hasUncommitted after launcher.stop and immediately before worktrees.remove, and skip the removal if it now says dirty (logging it the way the CB-576 branch does). Decide it yourself and justify it.

Two things to weigh, and I do not know the answers:

  • Where should the snapshot go? Today trySnapshot runs before the stop, which is a reasonable "capture it while we can". If the re-check finds new work after the stop, that work has no snapshot. Should the snapshot move after the stop, be repeated, or stay where it is? Argue it.
  • Is a re-check enough, or is the ordering itself wrong? An alternative is to stop the pane first and do the whole dirty-decide-snapshot-preserve sequence afterwards, so there is only ever one read and it is taken with the writer gone. That is a bigger change and it moves the snapshot after the pane dies — say whether that is better or worse and why.

A tested, reported deviation is a good outcome here.

Rules

  • Prove it with a test that fails without the fix: a worktree that reports clean on the first hasUncommitted call and dirty on the second must not be removed. A test double for Worktrees that changes its answer between calls is the obvious seam — check whether one already exists before writing a new one.
  • Mutation proof required: revert the fix, quote the real failure output, restore it.
  • Never run git worktree remove or git worktree prune against this repo or any worktree under /Users/dai.ha/LTMS/.bridged-worktrees/. Other workers are live in them right now and you would destroy their work. If you need to test real git behaviour, use a throwaway repo under /tmp and say in your report which parts of this repo's shape (submodule, --skip-worktree file, per-worktree config) your throwaway does not cover.
  • Never run git stash — the stash is shared across every worktree here.
  • Stage files explicitly; never git add -A. Never merge.
  • Run cd fleetd && mvn clean install unpiped, and quote the real Tests run: and BUILD lines. Never pipe maven through tail/head, and never read $? after a pipe — after a pipe it is the pipe's last command's status, not Maven's.

Shape check

When done, look in SessionManager.java only for the same shape: a value read once and then used to authorise a destructive or irreversible action later in the same method, after something else has had a chance to change it. One line each, do not fix any of it.

Found by a delegated hunter. I read the whole method and confirmed the structure, and I extended the consequence — the hunter reported half of it. ## Not observed. Read the severity honestly I have not seen this happen, and I cannot point at a lost file. What follows is a structural TOCTOU with a narrow window. It is worth fixing because the direction of harm is data loss and the fix is cheap, **not** because it is known to have cost us anything. Do not write it up as an incident. ## The order `SessionManager.releaseRemoved` decides, stops, then deletes: ```java boolean dirty = removed.worktree() != null && worktrees.hasUncommitted(removed.worktree()); ... } else if (dirty) { preserveWorktree = true; // CB-576 } if (dirty) { snapshotRef = trySnapshot(removed, cause); // CB-578 stage C } ... launcher.stop(paneId); // the writer stops HERE if (removed != null && !preserveWorktree && removed.worktree() != null) { worktrees.remove(worktrees.repoRoot(removed.cwd()), removed.worktree()); // --force } ``` `dirty` is computed while the worker is still running and is then used, unchanged, to authorise a `git worktree remove --force` that runs after the worker is stopped. Nothing re-reads it. ## Why this is worse than "we lose the preserve" The hunter reported the preserve. The same stale boolean also gates the snapshot: ```java if (dirty) { snapshotRef = trySnapshot(removed, cause); } ``` So in the racing case the worktree is **not** preserved *and* **not** snapshotted into `refs/wip/*`. CB-578 stage C exists precisely so that preserving on disk is not the only copy. Here both defences are gated on the same stale read, so they fail together. There is no third layer: the pane is gone, the registry entry is gone, and the directory is force-deleted. ## The path in 1. A worker commits its work. Its tree is clean. 2. `fleet_stop{paneId}` (or the reaper, or a failed turn) calls `release`. 3. `hasUncommitted` shells out to `git status` and returns false. `dirty = false`, `preserveWorktree = false`, no snapshot. 4. The worker — still alive; nothing has stopped it yet — writes a new file. A worker that just committed and moved on to the next edit is the ordinary case, not a contrived one. 5. `launcher.stop(paneId)` closes the pane. 6. `worktrees.remove(...)` runs `git worktree remove --force`. The new file is gone. **Direction of harm:** data loss — the worst direction in this area, and the one CB-576 and CB-578 were both written to prevent. **Window:** from `hasUncommitted` returning to `remove` starting. That covers `memberLifecycle.released`, the `resolveAgentSessionId` call, the release-listener notification, and the whole of `launcher.stop` (a herdr RPC). Tens to hundreds of milliseconds, not microseconds. **One thing I did not establish:** whether `agents.close(paneId)` guarantees the agent process is dead before it returns. I read `HerdrPeerLauncher.stop` and it issues the close and moves on; whether the child is reaped synchronously is inside herdr. If it is not, the window extends past step 5 and a write can land after the pane is closed. **Check this before you decide where the re-check goes** — say what you found rather than assuming either way. ## Why nothing else catches it - The `catch (RuntimeException e)` around the dirty check already encodes the right instinct — *"fail toward the safe answer and preserve it — deleting on a guess can destroy work that has no other copy"*. That is the correct rule; it just is not applied to a result that has gone stale rather than thrown. - `worktrees.remove` is `--force` by design and does not consult the tree's state. - The `refs/wip` snapshot is gated on the same `dirty`, as above. ## What I want **Goal:** the decision to delete a worktree must be based on the tree's state at the moment of deletion, with the worker no longer able to write to it. **Invariants:** 1. **Fail toward preserving.** If the state cannot be determined, keep the worktree. That is already this method's rule for an exception and must stay the rule for anything you add. 2. **`ReleaseCause.SHUTDOWN` still preserves unconditionally.** Do not make a shutdown depend on a `git status` that might now say clean. 3. **A slow or failing check must not stop the pane teardown or escape `release`.** The pane must always stop — that is CB-581, and an orphaned pane is its own kind of harm. Whatever you add is best-effort in exactly the way the surrounding steps are. 4. **Do not add a second unconditional `git status` to every release.** This runs on every teardown of every member. If you re-check, re-check only on the path that is actually about to delete something. **Candidate mechanism, as a candidate only:** re-run `hasUncommitted` after `launcher.stop` and immediately before `worktrees.remove`, and skip the removal if it now says dirty (logging it the way the CB-576 branch does). **Decide it yourself and justify it.** Two things to weigh, and I do not know the answers: - **Where should the snapshot go?** Today `trySnapshot` runs *before* the stop, which is a reasonable "capture it while we can". If the re-check finds new work after the stop, that work has no snapshot. Should the snapshot move after the stop, be repeated, or stay where it is? Argue it. - **Is a re-check enough, or is the ordering itself wrong?** An alternative is to stop the pane first and do the whole dirty-decide-snapshot-preserve sequence afterwards, so there is only ever one read and it is taken with the writer gone. That is a bigger change and it moves the snapshot after the pane dies — say whether that is better or worse and why. A tested, reported deviation is a good outcome here. ## Rules - Prove it with a test that fails without the fix: a worktree that reports clean on the first `hasUncommitted` call and dirty on the second must not be removed. A test double for `Worktrees` that changes its answer between calls is the obvious seam — check whether one already exists before writing a new one. - Mutation proof required: revert the fix, quote the real failure output, restore it. - **Never run `git worktree remove` or `git worktree prune` against this repo or any worktree under `/Users/dai.ha/LTMS/.bridged-worktrees/`.** Other workers are live in them right now and you would destroy their work. If you need to test real git behaviour, use a throwaway repo under `/tmp` and say in your report which parts of this repo's shape (submodule, `--skip-worktree` file, per-worktree config) your throwaway does not cover. - Never run `git stash` — the stash is shared across every worktree here. - Stage files explicitly; never `git add -A`. Never merge. - Run `cd fleetd && mvn clean install` **unpiped**, and quote the real `Tests run:` and `BUILD` lines. Never pipe maven through `tail`/`head`, and never read `$?` after a pipe — after a pipe it is the pipe's last command's status, not Maven's. ## Shape check When done, look in `SessionManager.java` only for the same shape: **a value read once and then used to authorise a destructive or irreversible action later in the same method, after something else has had a chance to change it.** One line each, do **not** fix any of it.
ltms commented 2026-09-04 09:23:26 +02:00
Author
Owner
Copy Link
Copy Source

Merged as 65f98ba, with one follow-up commit 8426c35. Pushed to main.

Build on main after the merge: Tests run: 1334, Failures: 0, Errors: 0, Skipped: 0 — BUILD SUCCESS. After my added test: Tests run: 1335, same result. Both unpiped.

The worker answered both design questions well, and taught me something

I asked whether the snapshot should move after the stop. The answer is no, and the reason is an invariant I did not know when I wrote the ticket:

notifyReleased (carrying snapshotRef) fires before launcher.stop on purpose (CB-516/CB-581 — so a blocked fleet_ask rendezvous fails fast rather than waiting on the herdr RPC).

So moving the whole decide-snapshot-preserve sequence after the stop — my second candidate — would have forced that notification to either move too, losing the fast-fail, or fire with a snapshotRef that was never computed. That would trade a regression on the common path for a fix on the rare one. Keeping the pre-stop snapshot and adding a second one only on the late-dirty path is the right call.

The same invariant answers question 2. I am satisfied the targeted re-check beats the reorder.

The herdr question was answered honestly and I want that on the record. The worker traced HerdrPeerLauncher.stop → agents.close(paneId) → AgentControl.close → one blocking pane.close RPC, and then said plainly that whether the child is dead and reaped when that call returns is decided inside herdr, whose source is not in this checkout. So the re-check narrows the race to whatever gap remains after pane.close returns, plus the few milliseconds to the removal. It does not prove that gap is zero. That is the correct thing to report and it is better than a confident guess either way.

My own mutations, and the gap one of them found

J — the late re-check still snapshots, but no longer preserves. Removed only preserveWorktree = true;:

SessionManagerTest.releaseDoesNotRemoveAWorktreeThatBecameDirtyBetweenTheFirstCheckAndRemoval:1251
  a worktree that turned dirty between the pre-stop read and removal must be preserved
  ==> expected: <true> but was: <false>

Good — the preserve half is pinned on its own.

K — the failed re-check falls toward deleting. Changed dirtyImmediatelyBeforeRemoval's catch from return true to return false:

Tests run: 87, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

It passed. Nothing pinned it. That mutation turns the guard into a cause of exactly the data loss it was added to prevent — a worktree whose git status fails is force-deleted — and the whole suite stayed green.

This was invariant 1 of this ticket, stated first and in bold: "Fail toward preserving. If the state cannot be determined, keep the worktree." The worker implemented it correctly. It just did not pin it, and I would not have known without running the mutation.

I fixed it myself in 8426c35 rather than sending it back, because it is one test. The existing failHasUncommittedWith seam throws on every call, which cannot express this case — the pre-stop read has to succeed and only the late read fail — so I added a call-indexed seam, failHasUncommittedOnCall(int call, RuntimeException e), and the test releasePreservesAWorktreeWhoseLateRecheckCannotBeRead. Re-running mutation K now gives:

SessionManagerTest.releasePreservesAWorktreeWhoseLateRecheckCannotBeRead:1290
  a worktree whose state cannot be read immediately before removal must be kept: preserving costs
  disk, deleting on a guess destroys work with no other copy
  ==> expected: <true> but was: <false>

The lesson for me, not for the worker. I wrote three invariants into this ticket and only demanded a test for the racing case. An invariant nobody can break in a test is not an invariant, it is a comment. When a ticket states a fail-safe rule, the ticket has to ask for the test that pins it — otherwise the next person to touch that catch block gets a green build.

Accepted caveat

ReleaseDetail.snapshotRef still lags in the flip-to-dirty case: the notification has already fired with snapshotRef = null, so the late snapshot reaches the log and not the listener. Fixing that means firing onRelease twice for one release. That is a bigger change than this ticket and the worker was right to document it rather than do it. Not filing it separately — the WARN line names the ref, and this path is rare by construction.

Shape check

The worker reports acquireWithWorktree's failure-cleanup branch (~572-593) reads repoRoot/path near the top and reuses them on the exception path to authorise worktrees.remove/deleteBranch. Same shape, lower risk: no worker has been started at that point, so there is nothing of anyone's to lose. Not filing.

Closing.

Merged as `65f98ba`, with one follow-up commit `8426c35`. Pushed to `main`. Build on `main` after the merge: `Tests run: 1334, Failures: 0, Errors: 0, Skipped: 0` — `BUILD SUCCESS`. After my added test: `Tests run: 1335`, same result. Both unpiped. ## The worker answered both design questions well, and taught me something I asked whether the snapshot should move after the stop. The answer is no, and the reason is an invariant I did not know when I wrote the ticket: > `notifyReleased` (carrying `snapshotRef`) fires **before** `launcher.stop` on purpose (CB-516/CB-581 — so a blocked `fleet_ask` rendezvous fails fast rather than waiting on the herdr RPC). So moving the whole decide-snapshot-preserve sequence after the stop — my second candidate — would have forced that notification to either move too, losing the fast-fail, or fire with a `snapshotRef` that was never computed. That would trade a regression on the common path for a fix on the rare one. Keeping the pre-stop snapshot and adding a second one only on the late-dirty path is the right call. The same invariant answers question 2. I am satisfied the targeted re-check beats the reorder. **The herdr question was answered honestly and I want that on the record.** The worker traced `HerdrPeerLauncher.stop` → `agents.close(paneId)` → `AgentControl.close` → one blocking `pane.close` RPC, and then said plainly that whether the child is dead and reaped when that call returns is decided inside herdr, whose source is not in this checkout. So the re-check narrows the race to whatever gap remains after `pane.close` returns, plus the few milliseconds to the removal. It does not prove that gap is zero. That is the correct thing to report and it is better than a confident guess either way. ## My own mutations, and the gap one of them found **J — the late re-check still snapshots, but no longer preserves.** Removed only `preserveWorktree = true;`: ``` SessionManagerTest.releaseDoesNotRemoveAWorktreeThatBecameDirtyBetweenTheFirstCheckAndRemoval:1251 a worktree that turned dirty between the pre-stop read and removal must be preserved ==> expected: <true> but was: <false> ``` Good — the preserve half is pinned on its own. **K — the failed re-check falls toward deleting.** Changed `dirtyImmediatelyBeforeRemoval`'s catch from `return true` to `return false`: ``` Tests run: 87, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` **It passed.** Nothing pinned it. That mutation turns the guard into a cause of exactly the data loss it was added to prevent — a worktree whose `git status` fails is force-deleted — and the whole suite stayed green. This was **invariant 1** of this ticket, stated first and in bold: *"Fail toward preserving. If the state cannot be determined, keep the worktree."* The worker implemented it correctly. It just did not pin it, and I would not have known without running the mutation. I fixed it myself in `8426c35` rather than sending it back, because it is one test. The existing `failHasUncommittedWith` seam throws on *every* call, which cannot express this case — the pre-stop read has to succeed and only the late read fail — so I added a call-indexed seam, `failHasUncommittedOnCall(int call, RuntimeException e)`, and the test `releasePreservesAWorktreeWhoseLateRecheckCannotBeRead`. Re-running mutation K now gives: ``` SessionManagerTest.releasePreservesAWorktreeWhoseLateRecheckCannotBeRead:1290 a worktree whose state cannot be read immediately before removal must be kept: preserving costs disk, deleting on a guess destroys work with no other copy ==> expected: <true> but was: <false> ``` **The lesson for me, not for the worker.** I wrote three invariants into this ticket and only demanded a test for the racing case. An invariant nobody can break in a test is not an invariant, it is a comment. When a ticket states a fail-safe rule, the ticket has to ask for the test that pins it — otherwise the next person to touch that `catch` block gets a green build. ## Accepted caveat `ReleaseDetail.snapshotRef` still lags in the flip-to-dirty case: the notification has already fired with `snapshotRef = null`, so the late snapshot reaches the log and not the listener. Fixing that means firing `onRelease` twice for one release. That is a bigger change than this ticket and the worker was right to document it rather than do it. Not filing it separately — the WARN line names the ref, and this path is rare by construction. ## Shape check The worker reports `acquireWithWorktree`'s failure-cleanup branch (~572-593) reads `repoRoot`/`path` near the top and reuses them on the exception path to authorise `worktrees.remove`/`deleteBranch`. Same shape, lower risk: no worker has been started at that point, so there is nothing of anyone's to lose. Not filing. Closing.
ltms closed this issue 2026-09-04 09:23:58 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#316
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?