diff --git a/11-Features.md b/11-Features.md index 69a6b6c..4cd87a2 100644 --- a/11-Features.md +++ b/11-Features.md @@ -3122,6 +3122,25 @@ file, and it is the default, so a profile that simply forgot the setting used to final re-read and the move itself is still lost. There is no operating-system compare-and-swap on a plain file, only this cooperative narrowing. Do not read the CAS as making the race gone. +**The third gotcha: under `memberHerdrSocket` the seed used to write the wrong home (fleetd #285).** +When `memberHerdrSocket:` is set, the member pane runs as a *different OS user with its own `$HOME`*. +The seeding code could not see that at all — it was a `static` method, so it had no access to the +launcher's own `memberHerdrSocketConfigured()` check. With `configDir` unset it therefore wrote +**fleetd's own `~/.claude.json`** — the operator's real file — while believing it was seeding the +member's. The member never got a seed, and the operator's file was edited for nothing. + +The sibling method one line below, `writeCharterFile`, had already solved this: it refuses the spawn +when it cannot place a file where a different-uid member can read it. The trust seed now does the +same. Under `memberHerdrSocket` it requires **both** `configDir` (so there is a member-readable +target at all) **and** `worktreeGroup` (so the `0600` file it writes can be shared), and refuses the +spawn — naming which key is missing — before touching any file. When both are set, the written file +is chgrp'd and chmod'd to `rw-r-----` for that group, so the member's OS user can actually open it. + +The refusal is deliberate rather than a degradation. A member with no readable trust seed is not +"slightly worse": it sits on the interactive dialog forever and never calls `fleet_reply`, which is +the exact failure this whole feature exists to prevent. **With `memberHerdrSocket` absent — every +live fleet today — nothing about this changed.** + ## A worktree tells you which of its config files are stubs **What.** A provisioned worktree neutralizes `.mcp.json`, `opencode.json` and `.autoenv`: the copy in @@ -3410,3 +3429,90 @@ repo's recorded pointer is deliberately never updated (committing it is on the n `git submodule update --init` in a worker's worktree checks out a **months-old** snapshot. During this very backfill a worker reported that an entry "does not exist anywhere in the repo" when it had been on the page for hours. Paste the relevant text into the brief instead of pointing at the file. + +## A dead member's seat comes back + +**What.** A member whose backend errored (`BACKEND_ERROR`) or whose turn failed (`FAILED`) no longer +counts against its profile's `maxLoad`. Its row stays in `fleet_list` so you can still see what +happened, but a fresh `fleet_spawn` on that profile is granted. + +**On.** Always on; no configuration. + +**Why it exists.** Nothing ever removed these sessions, and the live counter had no state filter at +all — it counted every roster entry. That counter is the one the real spawn gate reads +(`CompositePeerLauncher.enforceMaxLoad`), so one backend error took a seat and never gave it back. +Only an explicit `fleet_stop` on that exact pane, or a daemon restart, freed it. + +The harm ran in the direction that hurts. On a `maxLoad: 1` profile — `opus` and `sol` on this host — +a single backend error put the profile out of service for good, and `fleet_spawn` answered *"at +maxLoad: 1 live >= 1 cap; refusing spawn — no fallback to another profile"*. The lead saw a capacity +refusal with no reason to suspect a dead seat. It also outlived the cooldown that was supposed to be +the remedy: quarantine and cool-off both expire on their own, the dead seat did not, so the profile +was still refusing long after the credential recovered. + +**The gotcha: `free` and `reclaimable` count different things, and a freed seat belongs in only one +of them.** `reclaimable` means "this member holds a seat and has no open bridge work — stop it and +you get the seat back". A terminal session holds no seat any more, so its seat is already in `free`. +Counting it as `reclaimable` too would report the same seat twice, and `free + reclaimable` would +read as more capacity than `maxLoad` allows. The ticket asked for exactly that, and that half of the +ticket was wrong. The dead session is still visible without it: its roster row carries +`state: "backend_error"` or `"failed"`, which is what tells the lead to stop it. + +Both places that report `reclaimable` — the per-member flag and the per-profile count, which travel +in the same `fleet_list` response — now call one shared predicate, so they cannot drift apart. A test +runs it over every `MemberSession.State` value, so a state added later cannot slip through +unconsidered. + +**What this does not do.** Terminal sessions are still never reaped. They accumulate in the roster +until stopped or until the daemon restarts. That is now cosmetic rather than a capacity loss, and it +is deliberate: the roster entry is the only record of what went wrong. + +## A member's teardown no longer leaks a worktree or a branch + +**What.** Two cleanup paths in `SessionManager` were one-sided, and both are closed. Stopping a +member whose worktree cannot be removed now completes the stop and logs a WARN instead of throwing. +A spawn that fails after its worktree was created now deletes the orphaned branch as well as the +worktree. + +**On.** Always on; no configuration. + +**Why it exists.** Every other cleanup step in `release()` was wrapped in a try/catch — the last one, +removing the worktree, was not. By the time it ran, the session was already out of the registry and +the pane already stopped, so a throw there escaped `release()` with the teardown in fact complete. +There was no retry path: a second stop on that pane is a no-op. The caller saw a failed stop for a +session that was gone, and the directory leaked with nothing left to point at it. `git worktree +remove --force` can throw for ordinary reasons — a stale index lock, a slow filesystem, its own 30 +second timeout — so this was not exotic. + +The second leak is the same shape as the one fixed one layer down earlier: `GitWorktrees` already +deleted both the worktree **and** the branch when provisioning failed *inside* `add()`. The catch +that covers failures *after* `add()` returns — the parity overlay, the group share, the spawn itself +— removed only the worktree. A spawn failure there is routine (a quarantined credential, a backend +refusal), so every occurrence left an orphan `worker/-` branch behind. + +**The gotcha: only the failed-provisioning path deletes a branch.** A **normal** release deliberately +keeps the branch, so a worker's committed work can still be recovered after its pane is gone. Getting +these two backwards would destroy real work, so the tests pin the separation from both sides: the +normal-release test asserts the branch delete is *never* called, and the failed-spawn test asserts +the deleted branch is the exact one `add()` created. + +## A chained second question no longer kills its own ticket + +**What.** A member that calls `fleet_ask` twice in one turn — asks, gets an answer, then asks again — +keeps its ticket. Before, the second question silently ended the ticket, and the lead's `fleet_poll` +returned nothing for a member that was still working. + +**On.** Always on; no configuration. + +**Why it exists.** Each `fleet_ask` moves the async task to a new `turnId`. The thread answering the +*first* question then finished its work against the *old* `turnId`. Whether that killed the ticket +came down to which of two lines ran first — a race, not a decision. Answering now registers its +waiter before it resolves, and drops the stale key explicitly rather than relying on the order. + +**The gotcha: the guard that looks load-bearing is not.** The fix also added a state guard on the +answering path, and the pull request claimed the ticket would still die without it. It would not: a +mutation removing only that guard left the test green, because `ask()` already moves the task to the +new `turnId` before the answering thread wakes. The guard is kept as defence in depth — it mirrors a +sibling guard whose own comment warns that this ordering is not something to rely on — and the +measurement is written into the code next to it, so nobody deletes it as dead code without +re-checking the ordering, and nobody trusts it as the only protection either.