diff --git a/11-Features.md b/11-Features.md index 1ff51e6..fbbab28 100644 --- a/11-Features.md +++ b/11-Features.md @@ -3974,3 +3974,58 @@ deliberate edit. A component belongs there only if it is read live off the confi because adding it makes the build pass. The general rule: when a ticket asks for a checker rather than a fix, mutate the checker too, and ask what the cheapest way to pass it without doing the work would be. + +--- + +## An answered `fleet_ask` no longer fails the lead's own call + +**What it does.** When the lead answers a worker's `fleet_ask` at the same moment that ask's ~55 +second window lapses, `fleet_send{turnId, content}` used to be able to throw +`NullPointerException` back at the lead — even though the answer had already been delivered. It +does not any more. + +**On.** Always on. There is no knob; it is a correctness fix (fleetd #324, merged `02e6aef`). + +**Why it exists.** `answer()` does its work holding the target's `sessionLocks` entry. `ask()`'s own +timeout path holds no lock at all, and it nulls `Task.turnId`. `finishAsyncTask` read that field +twice — once to check it was not null, once as the key for `asyncTasksByTurn.remove`. `volatile` +makes each read fresh, but it does not make a *pair* of reads atomic. When the unlocked null-out +landed between them, the second read saw `null` and `ConcurrentHashMap.remove(null, task)` threw. The +lead was told its answer failed. It had not: `task.future.complete(result)` ran on the line above. +A lead that reacts by re-sending is acting on a false failure. + +**One thing to know for maintenance.** The fix reads the field once into a local. That closes the +crash and **not** the asymmetry behind it. One side of this invariant is still locked and the other +is not, and two consequences of that are open in fleetd #329: a worker's real reply can leave its +async ticket `PENDING` for good, and `reply()` still contains the same double read. There is also a +production test seam here — a `volatile Runnable` hook plus two package-private setters, null and +unused outside tests. It exists because no public path reaches this interleaving without a real race, +so a deterministic test needs somewhere to stand. + +--- + +## A reload now says a restart is needed for `primary:` and `configReload:` + +**What it does.** `ConfigRef` reports a changed `primary:` or `configReload:` block as *deferred* — +accepted into the new snapshot, but not in effect until the daemon restarts. Before this, changing +either one reported a bare `config reloaded` and the daemon quietly kept the old value. + +**On.** Always on (fleetd #326, merged `823976c`). Visible in the reload summary and in +`ConfigRef.Outcome.deferred()`. + +**Why it exists.** Both keys are read once, at startup. `primary:` feeds `PrimaryRegistry`'s pinned +terminal and sizes `ReplyPushLoop`'s reminder cap and backoff; neither is rebuilt. `configReload:` +decides whether a `ConfigWatcher` is built at all and with what interval — so the component that +would apply a later change is itself built once. Turning reload off through a reload reported +success and changed nothing. This is the same drift fleetd #323 fixed one level down, in the +per-profile list. + +**One thing to know for maintenance.** Write down the denominator, because "not mentioned in +`ConfigRef`" looks identical for a key that is correctly hot and for a key nobody triaged. +`FleetConfig` has 22 top-level components; four are named nowhere in that file. +`memberCredentials` and `memberLoginShell` are hot and correctly absent — both are read live off +`config.get()` at spawn. `health` and `coordinator` are **undecided**: each is read both off the +startup snapshot and live, at different sites, so no single class fits either. A reload touching +them still under-claims. That count and both verdicts are now in `ConfigRef`'s class doc so the next +person does not measure it again. Twice now — `worktreeGroup`, then `primary`/`configReload` — the +untriaged kind hid among the correct kind.