Features: #324 (answered ask no longer fails the lead's call) and #326 (primary/configReload are deferred)

Dai Ha
2026-09-04 15:01:04 +07:00
parent 92bc455b1b
commit 0aefbae904
+55
@@ -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.