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

#316: re-check worktree dirtiness after the pane stops, before removing it #322

Closed
agent wants to merge 0 commits from worker/fix-316b-bd0860-11 into main
pull from: worker/fix-316b-bd0860-11
merge into: fleet:main
Conversation 0 Commits - Files Changed -
agent commented 2026-09-04 09:13:54 +02:00
Member
Copy Link
Copy Source

Fix

SessionManager.releaseRemoved read worktrees.hasUncommitted(...) once, while the worker
could still write to its worktree, and then used that one boolean — after launcher.stop(paneId)
had already closed the pane — to decide both whether to git worktree remove --force the tree and
whether to run trySnapshot. A worker that wrote new work between the read and the stop lost that
work with no preserve and no snapshot.

The fix adds a second, best-effort hasUncommitted read immediately before the removal, taken with
the pane already stopped. It runs only on the path that is actually about to delete something —
never when the release already decided to preserve (the ordinary dirty path, or SHUTDOWN, which
preserves unconditionally regardless of git state). If the tree is now dirty:

  • the removal is skipped (preserveWorktree = true), and
  • a fresh trySnapshot attempt runs, because the original pre-stop snapshot never fired (the
    pre-stop read said clean).

A failing re-check preserves too, matching the existing CB-581 rule: once the state can't be
determined, fail toward keeping the worktree.

Design question 1 — where should the snapshot go?

Kept the original pre-stop trySnapshot where it is, and added a second, conditional snapshot
attempt on the late-recheck path only.

Reasoning: releaseRemoved's finally block calls notifyReleased(...) — which carries
snapshotRef in ReleaseDetail — before launcher.stop(paneId) runs. That ordering is
deliberate and already documented (CB-516/CB-581): a caller with a fleet_ask rendezvous blocked
on this worker must be told the terminal is gone before the pane teardown RPC, or it sits waiting
on a call that can never resolve. If I moved the whole snapshot step to always run after the stop,
notifyReleased would either have to move too (defeating the fast-fail it exists for) or fire with
a snapshotRef that hasn't been computed yet (always null, even for the ordinary dirty-at-start
case) — a regression for the common case to fix the rare one.

So: the pre-stop snapshot stays where it is and keeps feeding the pre-stop notification exactly as
before. The late, post-stop snapshot is new and additional — it only fires on the rare
flip-to-dirty path, so it costs nothing on every other release. Its ref does not reach the
already-fired ReleaseDetail notification; it is logged at WARN (worktree, branch, pane, terminal,
and the commit sha when the snapshot succeeds) so an operator can still find it. I did not add a
second notification round for this — that would change the public onRelease contract (possibly
firing twice for one release) for a narrow race, which is bigger than this ticket's scope.

Design question 2 — is a re-check enough, or is the ordering itself wrong?

A re-check right before the removal is enough; I did not reorder to "stop first, then do the
whole decide-snapshot-preserve sequence once."

The bigger reorder is the more obvious fix and I considered it first, but it collides with the
notifyReleased-before-stop ordering above: to keep the fast-fail notification meaningful (a
correct worktreePath/branch/snapshotRef for a caller to re-dispatch onto), the decide step has
to already be done by the time stop runs — i.e. before the pane closes, which is exactly the
ordering the issue calls unsafe. Moving the whole sequence after stop instead would force
notifyReleased to either wait until after stop (reintroducing the "blocked rendezvous caller
sits until the herdr RPC finishes" cost CB-581 was written to avoid) or fire with wrong/stale detail.

The re-check is also strictly cheaper and lower-risk: it costs nothing on a release that already
decided to preserve (rule 4), it is one extra git status only on the path about to force-delete
something, and it does not touch the notifyReleased-before-stop invariant at all. It closes
exactly the race the issue reports: the window from hasUncommitted returning to remove starting
now ends right before remove, not right after the pre-stop read.

The herdr factual question

I could not establish it. HerdrPeerLauncher.stop (fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java:938-986)
calls agents.close(paneId), which is AgentControl.close (fleetd/src/main/java/dev/ltms/fleet/herdr/AgentControl.java):

public void close(String paneId) {
    herdr.call("pane.close", Map.of("pane_id", paneId));
}

This is one synchronous RPC over HerdrClient to the external herdr daemon (a unix-socket
process, not part of this repo) and HerdrPeerLauncher.stop waits for that call to return before
doing tab cleanup. But whether pane.close returning means the agent's child process has actually
been killed and reaped, or only that herdr has scheduled/started its teardown, is decided inside
herdr itself. herdr's own source is not checked out in this worktree — there is no herdr code
in this repo, only the client-side protocol layer (HerdrClient, AgentControl, HerdrPeerLauncher)
and its own protocol docs are external to this checkout. I found nothing else in this repo (no test
double, no doc under docs/*.md) that asserts one way or the other. So: I read the call path and
confirmed it is a single blocking RPC, but I did not — and could not, from this checkout — confirm
the death/reap guarantee itself.

Practical consequence for the fix: the post-stop re-check narrows the race to whatever residual gap
exists between pane.close returning and the worker process actually being gone (if any), plus the
few milliseconds between the re-check and worktrees.remove. It does not claim to close that gap to
zero, because I can't prove from this repo how large it is.

Mutation proof

Reverted the production fix (SessionManager.java back to HEAD, kept the new tests), reran
SessionManagerTest, restored the fix:

[ERROR] Tests run: 63, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 0.426 s <<< FAILURE! -- in dev.ltms.fleet.session.SessionManagerTest
[ERROR] dev.ltms.fleet.session.SessionManagerTest.releaseSnapshotsWorkFoundOnlyByTheLateRecheck -- Time elapsed: 0.006 s <<< FAILURE!
org.opentest4j.AssertionFailedError: the newly-dirty worktree is snapshotted even though the pre-stop check saw it clean ==> expected: <[/wt/worker_cb-316b-8cdcd1-1]> but was: <[]>
[ERROR] dev.ltms.fleet.session.SessionManagerTest.releaseDoesNotRemoveAWorktreeThatBecameDirtyBetweenTheFirstCheckAndRemoval -- Time elapsed: 0.001 s <<< FAILURE!
org.opentest4j.AssertionFailedError: a worktree that turned dirty between the pre-stop read and removal must be preserved ==> expected: <true> but was: <false>
[INFO] Tests run: 63, Failures: 2, Errors: 0, Skipped: 0
[INFO] BUILD FAILURE

Fix restored; full mvn clean install afterward is green (see below).

Build

cd fleetd && mvn clean install
...
[INFO] Tests run: 1329, Failures: 0, Errors: 0, Skipped: 0
[INFO] BUILD SUCCESS

Tests added (SessionManagerTest.java)

  • releaseDoesNotRemoveAWorktreeThatBecameDirtyBetweenTheFirstCheckAndRemoval — the exact The dirty-worktree check runs before the worker is stopped, so work written during teardown is deleted with no preserve and no snapshot (#316)
    race: hasUncommitted answers false then true; the worktree must not be removed, and the
    fake proves the second read actually happened (hasUncommittedCallCount() == 2).
  • releaseSnapshotsWorkFoundOnlyByTheLateRecheck — the work the late recheck finds is snapshotted,
    not just preserved on disk.
  • releaseStillRemovesAWorktreeThatStaysCleanOnTheLateRecheck — ordinary clean case is unaffected.
  • releaseNeverReChecksAWorktreeAlreadyPreservedByTheFirstDirtyCheck — invariant 4: a release that
    already decided to preserve on the first read never pays for a second hasUncommitted call.

Also extended RecordingWorktrees (the existing CB-581 test double in SessionManagerTest, not a
new class) with a dirtySequence(boolean...) — successive per-call answers, last one sticky — and a
hasUncommittedCallCount() counter. I checked first and there was no existing double whose answer
changes between calls; FakeWorktrees (session/FakeWorktrees.java) only has a single fixed
dirty flag, same as RecordingWorktrees did before this change.

Shape check (SessionManager.java only — not fixed, reported only)

  • acquireWithWorktree's failure-cleanup branch (around fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java:572-593)
    reads repoRoot/path once near the top of the method and reuses them later, on the exception
    path, to authorise worktrees.remove(repoRoot, path) and worktrees.deleteBranch(repoRoot, branch) — same "read once, use later for a destructive git op" shape as #316, though repoRoot
    is a structural fact (the repo's location) rather than working-tree state, so nothing plausible
    changes it mid-call, and no worker has been live long enough at that point to have written
    anything — the practical risk looks much lower than #316's, but I did not verify that beyond
    reading the code.

That is the only instance of the pattern I found confined to SessionManager.java; I did not widen
the search to other files.

Files changed

  • fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java
  • fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java

Caveats for review

  • ReleaseDetail.snapshotRef on the onRelease notification can lag: in the flip-to-dirty case the
    notification already fired (before stop) with snapshotRef = null; the late snapshot's ref only
    reaches the log, not that notification. Documented above under design question 1 — flagging for
    reviewer visibility since it's a real (if narrow) gap, not because I think it should block this PR.
  • Did not read fleetd/fleetd.yaml (gitignored, absent from this worktree) and make no claims about
    it.
  • Did not touch anything in HerdrPeerLauncher/AgentControl — the herdr-guarantee question is
    answered as "could not establish", not worked around with a new mechanism.
## Fix `SessionManager.releaseRemoved` read `worktrees.hasUncommitted(...)` once, while the worker could still write to its worktree, and then used that one boolean — after `launcher.stop(paneId)` had already closed the pane — to decide both whether to `git worktree remove --force` the tree and whether to run `trySnapshot`. A worker that wrote new work between the read and the stop lost that work with no preserve and no snapshot. The fix adds a second, best-effort `hasUncommitted` read immediately before the removal, taken with the pane already stopped. It runs only on the path that is actually about to delete something — never when the release already decided to preserve (the ordinary dirty path, or `SHUTDOWN`, which preserves unconditionally regardless of git state). If the tree is now dirty: - the removal is skipped (`preserveWorktree = true`), and - a fresh `trySnapshot` attempt runs, because the original pre-stop snapshot never fired (the pre-stop read said clean). A failing re-check preserves too, matching the existing CB-581 rule: once the state can't be determined, fail toward keeping the worktree. ## Design question 1 — where should the snapshot go? **Kept the original pre-stop `trySnapshot` where it is, and added a second, conditional snapshot attempt on the late-recheck path only.** Reasoning: `releaseRemoved`'s `finally` block calls `notifyReleased(...)` — which carries `snapshotRef` in `ReleaseDetail` — *before* `launcher.stop(paneId)` runs. That ordering is deliberate and already documented (CB-516/CB-581): a caller with a `fleet_ask` rendezvous blocked on this worker must be told the terminal is gone *before* the pane teardown RPC, or it sits waiting on a call that can never resolve. If I moved the whole snapshot step to always run after the stop, `notifyReleased` would either have to move too (defeating the fast-fail it exists for) or fire with a `snapshotRef` that hasn't been computed yet (always `null`, even for the ordinary dirty-at-start case) — a regression for the common case to fix the rare one. So: the pre-stop snapshot stays where it is and keeps feeding the pre-stop notification exactly as before. The late, post-stop snapshot is new and additional — it only fires on the rare flip-to-dirty path, so it costs nothing on every other release. Its ref does **not** reach the already-fired `ReleaseDetail` notification; it is logged at `WARN` (worktree, branch, pane, terminal, and the commit sha when the snapshot succeeds) so an operator can still find it. I did not add a second notification round for this — that would change the public `onRelease` contract (possibly firing twice for one release) for a narrow race, which is bigger than this ticket's scope. ## Design question 2 — is a re-check enough, or is the ordering itself wrong? **A re-check right before the removal is enough; I did not reorder to "stop first, then do the whole decide-snapshot-preserve sequence once."** The bigger reorder is the more obvious fix and I considered it first, but it collides with the `notifyReleased`-before-`stop` ordering above: to keep the fast-fail notification meaningful (a correct `worktreePath`/`branch`/`snapshotRef` for a caller to re-dispatch onto), the decide step has to already be done by the time `stop` runs — i.e. before the pane closes, which is exactly the ordering the issue calls unsafe. Moving the whole sequence after `stop` instead would force `notifyReleased` to either wait until after `stop` (reintroducing the "blocked rendezvous caller sits until the herdr RPC finishes" cost CB-581 was written to avoid) or fire with wrong/stale detail. The re-check is also strictly cheaper and lower-risk: it costs nothing on a release that already decided to preserve (rule 4), it is one extra `git status` only on the path about to force-delete something, and it does not touch the `notifyReleased`-before-`stop` invariant at all. It closes exactly the race the issue reports: the window from `hasUncommitted` returning to `remove` starting now ends right before `remove`, not right after the pre-stop read. ## The herdr factual question **I could not establish it.** `HerdrPeerLauncher.stop` (`fleetd/src/main/java/dev/ltms/fleet/member/HerdrPeerLauncher.java:938-986`) calls `agents.close(paneId)`, which is `AgentControl.close` (`fleetd/src/main/java/dev/ltms/fleet/herdr/AgentControl.java`): ```java public void close(String paneId) { herdr.call("pane.close", Map.of("pane_id", paneId)); } ``` This is one synchronous RPC over `HerdrClient` to the external `herdr` daemon (a unix-socket process, not part of this repo) and `HerdrPeerLauncher.stop` waits for that call to return before doing tab cleanup. But whether `pane.close` returning means the agent's child process has actually been killed and reaped, or only that herdr has scheduled/started its teardown, is decided inside `herdr` itself. `herdr`'s own source is not checked out in this worktree — there is no `herdr` code in this repo, only the client-side protocol layer (`HerdrClient`, `AgentControl`, `HerdrPeerLauncher`) and its own protocol docs are external to this checkout. I found nothing else in this repo (no test double, no doc under `docs/*.md`) that asserts one way or the other. So: I read the call path and confirmed it is a single blocking RPC, but I did not — and could not, from this checkout — confirm the death/reap guarantee itself. Practical consequence for the fix: the post-stop re-check narrows the race to whatever residual gap exists between `pane.close` returning and the worker process actually being gone (if any), plus the few milliseconds between the re-check and `worktrees.remove`. It does not claim to close that gap to zero, because I can't prove from this repo how large it is. ## Mutation proof Reverted the production fix (`SessionManager.java` back to HEAD, kept the new tests), reran `SessionManagerTest`, restored the fix: ``` [ERROR] Tests run: 63, Failures: 2, Errors: 0, Skipped: 0, Time elapsed: 0.426 s <<< FAILURE! -- in dev.ltms.fleet.session.SessionManagerTest [ERROR] dev.ltms.fleet.session.SessionManagerTest.releaseSnapshotsWorkFoundOnlyByTheLateRecheck -- Time elapsed: 0.006 s <<< FAILURE! org.opentest4j.AssertionFailedError: the newly-dirty worktree is snapshotted even though the pre-stop check saw it clean ==> expected: <[/wt/worker_cb-316b-8cdcd1-1]> but was: <[]> [ERROR] dev.ltms.fleet.session.SessionManagerTest.releaseDoesNotRemoveAWorktreeThatBecameDirtyBetweenTheFirstCheckAndRemoval -- Time elapsed: 0.001 s <<< FAILURE! org.opentest4j.AssertionFailedError: a worktree that turned dirty between the pre-stop read and removal must be preserved ==> expected: <true> but was: <false> [INFO] Tests run: 63, Failures: 2, Errors: 0, Skipped: 0 [INFO] BUILD FAILURE ``` Fix restored; full `mvn clean install` afterward is green (see below). ## Build ``` cd fleetd && mvn clean install ... [INFO] Tests run: 1329, Failures: 0, Errors: 0, Skipped: 0 [INFO] BUILD SUCCESS ``` ## Tests added (`SessionManagerTest.java`) - `releaseDoesNotRemoveAWorktreeThatBecameDirtyBetweenTheFirstCheckAndRemoval` — the exact #316 race: `hasUncommitted` answers `false` then `true`; the worktree must not be removed, and the fake proves the second read actually happened (`hasUncommittedCallCount() == 2`). - `releaseSnapshotsWorkFoundOnlyByTheLateRecheck` — the work the late recheck finds is snapshotted, not just preserved on disk. - `releaseStillRemovesAWorktreeThatStaysCleanOnTheLateRecheck` — ordinary clean case is unaffected. - `releaseNeverReChecksAWorktreeAlreadyPreservedByTheFirstDirtyCheck` — invariant 4: a release that already decided to preserve on the first read never pays for a second `hasUncommitted` call. Also extended `RecordingWorktrees` (the existing CB-581 test double in `SessionManagerTest`, not a new class) with a `dirtySequence(boolean...)` — successive per-call answers, last one sticky — and a `hasUncommittedCallCount()` counter. I checked first and there was no existing double whose answer changes between calls; `FakeWorktrees` (`session/FakeWorktrees.java`) only has a single fixed `dirty` flag, same as `RecordingWorktrees` did before this change. ## Shape check (`SessionManager.java` only — not fixed, reported only) - `acquireWithWorktree`'s failure-cleanup branch (around `fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java:572-593`) reads `repoRoot`/`path` once near the top of the method and reuses them later, on the exception path, to authorise `worktrees.remove(repoRoot, path)` and `worktrees.deleteBranch(repoRoot, branch)` — same "read once, use later for a destructive git op" shape as #316, though `repoRoot` is a structural fact (the repo's location) rather than working-tree state, so nothing plausible changes it mid-call, and no worker has been live long enough at that point to have written anything — the practical risk looks much lower than #316's, but I did not verify that beyond reading the code. That is the only instance of the pattern I found confined to `SessionManager.java`; I did not widen the search to other files. ## Files changed - `fleetd/src/main/java/dev/ltms/fleet/session/SessionManager.java` - `fleetd/src/test/java/dev/ltms/fleet/session/SessionManagerTest.java` ## Caveats for review - `ReleaseDetail.snapshotRef` on the `onRelease` notification can lag: in the flip-to-dirty case the notification already fired (before `stop`) with `snapshotRef = null`; the late snapshot's ref only reaches the log, not that notification. Documented above under design question 1 — flagging for reviewer visibility since it's a real (if narrow) gap, not because I think it should block this PR. - Did not read `fleetd/fleetd.yaml` (gitignored, absent from this worktree) and make no claims about it. - Did not touch anything in `HerdrPeerLauncher`/`AgentControl` — the herdr-guarantee question is answered as "could not establish", not worked around with a new mechanism.
agent added 1 commit 2026-09-04 09:13:54 +02:00
#316: re-check worktree dirtiness after the pane stops, before removing it
CI / contract (pull_request) Successful in 39s
Details
CI / build (pull_request) Successful in 1m46s
Details
667254df47 
SessionManager.releaseRemoved read hasUncommitted() once, while the worker
could still write, then used that stale boolean after launcher.stop() to
authorise `git worktree remove --force`. The same stale read also gated
trySnapshot, so a worker that wrote between the read and the stop lost its
work with neither a preserve nor a snapshot.

Add a second, best-effort hasUncommitted read immediately before the
removal, taken only on the path that is actually about to delete something
(never on a release that already decided to preserve, and never for
SHUTDOWN, which preserves unconditionally). If the tree is now dirty,
preserve it and attempt a fresh snapshot, since the original snapshot never
ran when the pre-stop read said clean. A failing re-check also preserves,
matching the existing CB-581 fail-safe rule.
ltms closed this pull request 2026-09-04 09:24:11 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 39s
Details
CI / build (pull_request) Successful in 1m46s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
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#322
Delete Branch "worker/fix-316b-bd0860-11"

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?