+96
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user