From 9d62717837c3e7dcb4b18df044f70a1800e908da Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Fri, 4 Sep 2026 13:35:18 +0700 Subject: [PATCH] Features: #306, #307, #308, #309 --- 11-Features.md | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 96 insertions(+) diff --git a/11-Features.md b/11-Features.md index e4fa326..709634c 100644 --- a/11-Features.md +++ b/11-Features.md @@ -3739,3 +3739,99 @@ remove, which shrinks the window without closing it and leaves the code looking the worker happens to have just become busy. A skipped reap logs one debug line naming the pane; without it the race is unobservable by construction, and a reaper that quietly stops reaping is very hard to diagnose. + +## A wedged post-turn phase releases itself, like the turn phase already did + +**What.** After a delegated turn finishes, the injector runs a housekeeping phase — it sends the +adapter's `/clear` and waits for the pane to pick it up. Four latches gate delivery to a member: +`awaitingCompletion`, `postTurnPending`, `awaitingPostTurnPickup` and `postTurnObserved`. Only the +first had a way out of a long run of unknown pane states. The other two now get the same escape: a +sustained unknown streak drops them and the member becomes deliverable again. + +**On.** Always on, but only reachable when the post-turn `/clear` is configured +(`lifecycle.clearAfterTurn`). It is **not** set in the live `fleetd.yaml` today, so this is a latent +fix here rather than one that was costing us turns. + +**Why it exists.** A pane whose state cannot be read stays unknown forever, and a latch with no +timer waits forever with it. The member never returns to `idle`, so every later `fleet_send` to it +waits and then fails without ever reaching the pane. The turn phase was given an escape when this +happened to it; the housekeeping phase was left with none, which is the same gate closed in one +direction only. + +**One thing to know for maintenance.** The new escape uses its own counter, `unknownSincePostTurn`, +and deliberately does **not** set `turnFailed`. The delegated turn already completed and its waiter +already resolved — what is outstanding is adapter housekeeping, so failing the turn would report a +false failure for work that succeeded. When you fix a latch here, check its siblings in the same +method: this fix had to cover two latches, not the one that was reported, or the wedge simply moved +one notch further along. + +## A worker's real reply after a lapsed `fleet_ask` completes its ticket + +**What.** A worker that calls `fleet_ask` and gets no answer within the ~55s window resumes on its +own, finishes, and ends the turn with `fleet_reply`. That reply used to land in the session inbox +with nothing tying it to the delegation: `fleet_poll{ticket}` stayed `PENDING`, and `fleet_stop` +later forced the ticket `FAILED` with the reason "session released before it replied". The reply now +completes its own ticket. + +**On.** Always on. + +**Why it exists.** The lead was told the opposite of what happened. The report was never destroyed — +it reached the inbox — but the ticket said the worker never replied, and a lead that believes that +re-does the work. An unanswered ask is the ordinary case on this fleet, not an edge, which is why +the charter says never to brief a worker to "ask me". The sibling timeout in `answer()` already had +this recovery; `ask()`'s did not. + +**One thing to know for maintenance.** **Do not "fix" this by passing `forgetTurn=false` on the ask +timeout.** It looks like the one-line version of the same fix and it wedges the member: the stamped +`turnId` keeps `hasAsyncQuestion` reporting the target BUSY, so every later `fleet_send` to it is +refused. The fix instead sets a separate `Task.askTimedOut` flag *before* the forgetting, and never +re-adds the task to `asyncTasksByTurn`. One real consequence: the ambiguity branch in `reply()` used +to be unreachable and is now reachable, because a lapsed ask frees its target for a fresh +delegation that can lapse in turn. Two open tasks on one target fall back to the inbox rather than +guess — completing the wrong ticket would hand the lead a plausible answer to work nobody did. + +## Shutdown refuses new spawns and sweeps up stragglers + +**What.** `fleet_spawn` is now refused once the daemon's shutdown drain has started — a named error +over MCP, HTTP 503 over REST — and the drain re-reads the registry after its main pass to tear down +anything that raced in anyway. + +**On.** Always on. + +**Why it exists.** The drain worked from a one-shot registry snapshot, and the MCP server stayed up +for a long time after it started: on the live config `lifecycle.drainTimeoutSeconds` is 120, so a +drain waiting on a busy worker could hold `fleet_spawn` open for two minutes. A session accepted in +that window was invisible to the drain — its pane kept running, its worktree was never preserved, +and the in-memory registry died with the process, so nothing else could ever reclaim either. The +caller got a normal `sessionId` and no way to know. + +**One thing to know for maintenance.** The guard and the sweep are a pair and neither is redundant: a +guard alone still loses to a caller already inside `launcher.spawn()`, and a sweep alone hands the +caller a session that is then destroyed. The sweep shares the drain's one deadline rather than +taking a second budget — a per-session grace could push the daemon past launchd's exit window and +get it `SIGKILL`ed mid-teardown. The `draining` flag is never reset, which is correct only because +`drainAll` is reachable from the shutdown hook alone; if a `fleet_drain` tool is ever added for a +live daemon, that flag becomes a permanent spawn outage. + +## A failed `git worktree add` cleans up what it half-created + +**What.** The `git worktree add` command now runs inside the same cleanup scope as the provisioning +steps that follow it. If the command is killed — the 30-second timeout, or an interrupt — after Git +has begun writing worktree state, that state is removed instead of leaking. + +**On.** Always on. + +**Why it exists.** The cleanup added for #274 covered every step *after* the add and assumed the add +itself was atomic on failure. It is, for an error Git reports; it is not when fleetd calls +`destroyForcibly` on it. `SessionManager.acquireWithWorktree` never receives a path in that case, so +its own `if (path != null)` cleanup never fires either, and nothing at any layer reclaims the +directory or the branch. This is a resource leak, not data loss — no worker was ever started, so the +half-made worktree holds nobody's work. + +**One thing to know for maintenance.** Widening the cleanup scope creates a data-loss risk that the +guard exists to stop. `cleanupAfterAddFailure` deletes the branch with `-D`, so running it after an +ordinary "a branch named X already exists" refusal would delete the operator's existing branch. The +`Files.exists(worktreePath)` check prevents that: measured in a throwaway repo, Git creates no +directory on that refusal, so no directory means nothing was created and the branch is not ours to +touch. Removing that check makes `addFailureBeforeCreatingAWorktreeIsQuiet` fail with the branch +actually deleted. Do not remove it.