Compare commits

...

8 Commits

Author SHA1 Message Date
Dai Ha d703ce1313 #337: extend ConfigRefTopLevelReportingCoverageTest to DEFERRED_KEYS
CI / contract (pull_request) Successful in 1m23s
CI / build (pull_request) Successful in 1m27s
ConfigRefTopLevelReportingCoverageTest (added by #333) proved every COLD_KEYS
and SPLIT_KEYS member has a real comparison behind it, but left
DEFERRED_TOP_LEVEL_KEYS unexercised. Re-measured by mutation (drop each
key's branch from changedDeferredKeys, run the suite, restore): 6 of the 11
deferred keys had no behavioural test naming them — guard, leadHeartbeat,
worktreeRoot, spawnReadyTimeoutMs, spawnReadyPollMs, quarantineCooldownSeconds
— which corrects the issue's own guessed list in two ways: lifecycle is
actually covered (ConfigRefTest.aDeferredChangeIsAppliedAndReported), and
worktreeRoot was missing from the issue's list entirely.

Promoted the test-side DEFERRED_TOP_LEVEL_KEYS copy into ConfigRef.DEFERRED_KEYS
(package-private, alongside COLD_KEYS/SPLIT_KEYS) so the reflective test reads
the same set changedDeferredKeys is compared against, and made
changedDeferredKeys package-private so the test can call it directly. Every
DEFERRED_KEYS component turned out to be a scalar or a simple record, so no
exclusion set was needed.

Mutation proof: dropping guard's branch from changedDeferredKeys leaves the
whole suite green except the new
everyDeferredKeyIsActuallyReportedByChangedDeferredKeys test, which fails
naming guard exactly.
2026-09-04 16:06:09 +07:00
Dai Ha eee4d576a2 #333: the Cold doc bullet listed four keys, COLD_KEYS has five
CI / build (push) Successful in 1m52s
CI / contract (push) Successful in 2m8s
memberHerdrSocket was missing from the prose. The #333 worker spotted it and
correctly left it alone as outside its scope.

Fixed by pointing the bullet at COLD_KEYS instead of re-listing its contents,
so the prose and the set cannot drift apart a second time.
2026-09-04 15:43:39 +07:00
Dai Ha b4f9d7f53a Merge #333: fleet: is a split key, and split membership now proves a reporting branch exists 2026-09-04 15:39:02 +07:00
Dai Ha 3aca53b967 fleetd#333: fleet.leaders is split too, and split membership now proves reporting exists
CI / contract (pull_request) Successful in 1m12s
CI / build (pull_request) Failing after 1m59s
F1: fleet: was sitting in ConfigRefTopLevelCoverageTest's HOT_EXCLUDED_TOP_LEVEL_KEYS
escape hatch, even though fleet.leaders is read only at startup (LeadTabScanner's
identity map, LeadLauncher.ensureLeads) while the rest of fleet: (role pools,
charters, tabLabel) is live. A reload changing only fleet.leaders reported a bare
"config reloaded" -- the operator edits a lead's tab: label, sees the reload
succeed, and the pane keeps resolving as a worker. Moved fleet into
ConfigRef.SPLIT_KEYS; changedSplitKeys now compares fleet.leaders specifically
(not the whole Fleet record, which would over-claim "restart" for a tabLabel-only
change) and names both halves in the message.

F2: membership in SPLIT_KEYS/COLD_KEYS never proved a matching branch existed in
changedSplitKeys/changedColdKeys -- measured by dropping the coordinator branch
while leaving "coordinator" in SPLIT_KEYS: both ConfigRefTopLevelCoverageTest and
the in-method "kept in step" assert stayed green. Added
ConfigRefTopLevelReportingCoverageTest, the ConfigRefProfileCoverageTest mechanism
one level up: reflection-built FleetConfig pairs that differ in exactly one
top-level component, calling the real (now package-private) changedColdKeys/
changedSplitKeys to prove each COLD_KEYS/SPLIT_KEYS member is actually reported.
Scoped to split+cold, not deferred -- see the new test's javadoc for why and what
that leaves open.

Both findings carry a behavioural test in ConfigRefTest plus a mutation proof
(revert -> real failure -> restore) recorded in the PR description.
2026-09-04 15:36:40 +07:00
Dai Ha 4aa1fae296 #329: a null task in answer() is not only "never an async ticket"
CI / contract (push) Successful in 59s
CI / build (push) Successful in 2m26s
The comment that landed with #329 said a null task means the turn was never
an async ticket. That is wrong, and it makes the guard read as complete.

A genuine async ticket also reaches answer() with task == null. ask() runs
clearAsyncQuestion(turnId, true) in its catch block, which drops the
asyncTasksByTurn entry, while rendezvous.closeAsk(turnId) runs later, in its
finally. Between the two the ask is still answerable and the map entry is
already gone, so answer()'s lookup returns null and the ticket is stranded.

Measured with a throwaway probe firing only that first half: answer() reported
REPLIED while the ticket stayed PENDING with a null reply. The probe used
forgetTurnForTest, so it omits markAskTimedOut; that cannot change the outcome,
because askTimedOut is read only by askAnsweredAsyncTasks, which reply() never
reaches while answer()'s own waiter is live.

Open as fleetd #334. The comment now says so.
2026-09-04 15:33:57 +07:00
Dai Ha 0c865032f9 Merge #329: log an exception thrown after a ticket resolves, complete the ticket from the task answer() already holds, and read orphan.turnId once 2026-09-04 15:26:45 +07:00
Dai Ha ea41bbf6b9 fleetd#329: fix silent async-ticket bugs in MessageService (F1/F2/F3)
CI / contract (pull_request) Successful in 50s
CI / build (pull_request) Failing after 1m28s
F2 (sendAsync executor catch): log when completeExceptionally returns
false, so an exception thrown after finishAsyncTask already completed
the ticket's future is no longer silently lost.

F1 (answer()'s stranded async ticket): reuse the Task reference answer()
already looked up before rendezvous.answerAsk(), instead of a second
asyncTasksByTurn lookup by turnId in finishAsyncTask. The second lookup
raced ask()'s unlocked timeout cleanup, which could forget turnId first
and leave the ticket stuck PENDING even though answer() itself returned
REPLIED. The #282 chained-ask guard is unaffected: it is still keyed on
result.outcome() == QUESTION, not on this lookup. Removed the now-unused
finishAsyncTask(String, Reply) overload.

F3 (reply()'s orphan recovery path): read orphan.turnId once instead of
twice, closing the same double-read shape fleetd #324 fixed in
finishAsyncTask.

Each fix has its own test plus a test-only race hook (mirroring #324's
finishAsyncTaskRaceHook) to force the exact interleaving deterministically.
Mutation-tested each fix by reverting it, confirming the real failure
(swallowed exception / PENDING ticket / NullPointerException), then
restoring it.

mvn clean install: Tests run: 1345, Failures: 0, Errors: 0, Skipped: 0,
BUILD SUCCESS.
2026-09-04 15:19:38 +07:00
Dai Ha 7b918c51ff Merge #330: a fourth reload class for split keys, and a top-level coverage checker
CI / build (push) Successful in 1m46s
CI / contract (push) Successful in 1m48s
2026-09-04 15:19:14 +07:00
6 changed files with 819 additions and 82 deletions
@@ -27,14 +27,14 @@ import java.util.function.Supplier;
*
* <ul>
* <li><strong>Hot</strong> — re-read per use, so a reload takes effect on the next spawn:
* {@code fleet:} (every role pool, {@code charters}, and {@code tabLabel}),
* {@code placement:}, and an existing profile's {@code weight} / {@code maxLoad}. Those
* three are read through a supplier on {@code CompositePeerLauncher}, which is what makes
* them hot — not the fact that they are config. <strong>This does NOT include
* {@code fleet.leaders}</strong>: {@code Fleetd.main} reads {@code cfg.fleet().leaders()}
* once at startup to build the {@code LeadTabScanner} and the {@code LeadLauncher}, and
* neither is reconstructed on reload — so a lead added, removed, or re-{@code tab}'d under
* {@code fleet.leaders} needs a restart, the same as any deferred key below.</li>
* {@code placement:}, and an existing profile's {@code weight} / {@code maxLoad}. Both are
* read through a supplier on {@code CompositePeerLauncher}, which is what makes them hot —
* not the fact that they are config. Most of {@code fleet:} — every role pool
* ({@code architects}/{@code developers}/{@code reviewers}), {@code charters}, and
* {@code tabLabel} — is read the same live way, through the same supplier
* ({@code () -> config.get().fleet()}). <strong>But {@code fleet:} as a whole is NOT in this
* class</strong>: {@code fleet.leaders} inside the same key is frozen, which is exactly what
* makes {@code fleet:} split rather than hot — see below.</li>
* <li><strong>Deferred</strong> — accepted into the new snapshot, but the wiring built at startup
* keeps the old value until a restart: {@code lifecycle:}, {@code leadHeartbeat:},
* {@code spawnReadyTimeoutMs} / {@code spawnReadyPollMs}, {@code quarantineCooldownSeconds}
@@ -71,12 +71,12 @@ import java.util.function.Supplier;
* {@code HerdrPeerLauncher} takes {@code Map.copyOf(profiles)} at construction and resolves
* each spawn out of that copy, so those never reach a launch until the daemon restarts. A
* reload logs these rather than pretending they applied.</li>
* <li><strong>Split</strong> (fleetd #330) — read <em>both</em> ways at different sites, so the
* key does not fit any class above as a whole: {@code health:} and {@code coordinator:}.
* Each is read off the startup snapshot to build a long-lived object, and read live off
* {@link #get()} at a different, unrelated site — so half of a reload's effect already
* applies while the other half waits for a restart, and a bare "config reloaded" would
* under-claim by exactly that half.
* <li><strong>Split</strong> (fleetd #330; extended to a third key by fleetd #333) — read
* <em>both</em> ways at different sites, so the key does not fit any class above as a whole:
* {@code health:}, {@code coordinator:} and {@code fleet:}. Each is read off the startup
* snapshot to build a long-lived object, and read live off {@link #get()} at a different,
* unrelated site — so half of a reload's effect already applies while the other half waits
* for a restart, and a bare "config reloaded" would under-claim by exactly that half.
* <ul>
* <li>{@code health:} — the monitor itself ({@code enabled}, {@code intervalSeconds},
* {@code workingSuspectAfterSeconds}) is built once at {@code Fleetd.java:556-563}
@@ -92,34 +92,75 @@ import java.util.function.Supplier;
* URI env-var <em>name</em> that {@code MemberEnvAllowList} keeps out of a member's
* environment is read live off {@link #get()} on every spawn
* ({@code HerdrPeerLauncher.java:1530}), so it already applies.</li>
* <li>{@code fleet:} (fleetd #333) — {@code fleet.leaders} is the frozen half:
* {@code Fleetd.java:281} reads {@code cfg.fleet().leaders()} off the startup snapshot
* for two long-lived objects built right after it and never rebuilt — the
* {@code LeadTabScanner}'s {@code tab label → lead name} map ({@code Fleetd.java:301},
* wired into {@code CallerResolver.withLeadsAndMembers} at {@code Fleetd.java:620/624},
* which is how a caller's pane is recognised as a lead at all) and, when herdr answered
* at startup, {@code LeadLauncher(...).ensureLeads()} ({@code Fleetd.java:315}), which
* auto-launches each declared lead up to its {@code instances} count. So a lead added,
* removed, or given a new {@code tab:} label under {@code fleet.leaders} needs a
* restart — until then it is invisible to identity resolution, and this is exactly the
* scenario fleetd #333 named: an operator edits a lead's {@code tab:} to match a
* renamed pane, sees "config reloaded", and the pane keeps resolving as a worker,
* because {@code CallerResolver} is still matching against the old label. The live
* half is the rest of {@code fleet:} — {@code architects}/{@code developers}/
* {@code reviewers}, {@code charters}, {@code tabLabel} — read live through the same
* {@code CompositePeerLauncher} supplier the Hot bullet above names, so a reload that
* only touches those already applies with nothing to report. Because the hot and frozen
* halves of {@code fleet:} are disjoint sub-fields rather than the same fields read two
* ways (contrast {@code coordinator.uriEnv} above), {@link #changedSplitKeys} compares
* {@code fleet.leaders} alone, not the whole {@code Fleet} record — comparing the whole
* record would report "split" for a {@code tabLabel}-only change that is actually fully
* hot, over-claiming in exactly the direction this class exists to avoid under-claiming
* in.</li>
* </ul>
* A split change is still accepted — {@link Outcome#applied()} stays {@code true}, the same
* as a deferred change — because the live half genuinely took effect; refusing the whole
* reload would leave the operator worse off than today. {@link Outcome#split()} names the
* key and says which half is which each time, rather than trying to score "how changed" a
* mixed key is or handle "both halves changed in one reload" as a special case.</li>
* <li><strong>Cold</strong> — cannot change at all under a running daemon: {@code bind:},
* {@code herdrSocket:}, {@code broker:} and {@code auth:}. The socket is bound, the broker
* <li><strong>Cold</strong> — cannot change at all under a running daemon. All five of
* {@link #COLD_KEYS}: {@code bind:}, {@code herdrSocket:}, {@code memberHerdrSocket:},
* {@code broker:} and {@code auth:}. The sockets are already connected, the broker
* connection is open, and the auth mode decides who may reach the port that is already
* listening.</li>
* listening. This bullet omitted {@code memberHerdrSocket:} until fleetd #333 — say "all
* five of COLD_KEYS" rather than re-listing them, so prose and set cannot drift again.</li>
* </ul>
*
* <p><strong>The denominator, measured on 2026-09-04 (fleetd #330).</strong> {@code FleetConfig} has
* 22 top-level record components. Two of them are named nowhere in this file, and the reason is the
* same for both: {@code memberCredentials} and {@code memberLoginShell} are <strong>hot</strong> and
* correctly absent — both are read live off {@code config.get()} at spawn time
* ({@code Fleetd.java:198, 205, 729} and {@code HerdrPeerLauncher#configuredMemberLoginShell}), so a
* reload takes effect on the next spawn with no entry needed here.
* <p><strong>The denominator, measured on 2026-09-04 (fleetd #330; recounted for fleetd #333).</strong>
* {@code FleetConfig} has 22 top-level record components: 5 cold, 11 deferred, 3 split, 3
* hot-excluded. Three of them are named nowhere in this file, and the reason is the same for all
* three: {@code placement}, {@code memberCredentials} and {@code memberLoginShell} are
* <strong>hot</strong> and correctly absent — all three are read live off {@code config.get()}
* (placement through the {@code CompositePeerLauncher} supplier the Hot bullet names;
* {@code memberCredentials}/{@code memberLoginShell} at spawn time, {@code Fleetd.java:198, 205, 729}
* and {@code HerdrPeerLauncher#configuredMemberLoginShell}), so a reload takes effect on the next
* spawn with no entry needed here.
* {@code health} and {@code coordinator} used to be a third kind — <strong>undecided</strong>, not
* hot — until fleetd #330 added the <strong>split</strong> class above and gave them a home. A
* reload touching either used to report a bare "config reloaded", which under-claimed; now it names
* the key and says which half is which.
* the key and says which half is which. {@code fleet} was the same story in reverse: fleetd #330's
* own fact-find named {@code health}/{@code coordinator} as "the complete set of split-shaped keys"
* and filed {@code fleet.leaders}'s restart requirement as a documented caveat sitting in the
* <strong>hot-excluded</strong> escape hatch instead — correctly documented, but in the one bucket
* this file's own coverage test cannot check the truth of (see that test's javadoc). fleetd #333
* moved it into <strong>split</strong>, where {@link #changedSplitKeys} actually reports it.
* <p>The point of writing the count down: "not mentioned in this file" looks identical for a key
* that is correctly hot and for a key nobody triaged. Twice now — {@code worktreeGroup} (#323) and
* {@code primary}/{@code configReload} (#326) — the second kind hid among the first. A top-level
* coverage checker in the {@link ConfigRefProfileCoverageTest} shape (one level up, over
* {@code FleetConfig} itself rather than {@code FleetConfig.Profile}) proves this file's four
* classes exhaust the record's components — see {@code ConfigRefTopLevelCoverageTest}.
* that is correctly hot and for a key nobody triaged. Three times now — {@code worktreeGroup} (#323),
* {@code primary}/{@code configReload} (#326), and {@code fleet.leaders} sitting in the escape hatch
* (#333) — the second kind hid among the first. A top-level coverage checker in the
* {@link ConfigRefProfileCoverageTest} shape (one level up, over {@code FleetConfig} itself rather
* than {@code FleetConfig.Profile}) proves this file's four classes exhaust the record's components
* — see {@code ConfigRefTopLevelCoverageTest}. That test proves the record's <em>shape</em> is fully
* triaged; it does NOT prove a {@code SPLIT_KEYS}/{@code COLD_KEYS}/{@code DEFERRED_KEYS} member has
* any reporting code behind it at all — {@code ConfigRefTopLevelReportingCoverageTest} is what
* fleetd #333 added for that, after measuring that a {@code SPLIT_KEYS} entry with its reporting
* branch deleted passes both this file's own "kept in step" assert and
* {@code ConfigRefTopLevelCoverageTest} unchanged. fleetd #337 extended it to {@code DEFERRED_KEYS}
* after measuring the same one-way gap there directly: dropping {@code guard}'s branch out of
* {@link #changedDeferredKeys} while {@code "guard"} stayed in the set left the whole suite green.
*
* <p><strong>A cold change refuses the whole reload.</strong> Not the hot half applied and the cold
* half warned about: that would leave the running daemon in a state matching no file on disk, which
@@ -153,7 +194,26 @@ public final class ConfigRef implements Supplier<FleetConfig> {
* {@code true}) and reported by name, with a message naming which half is live and which needs
* a restart.
*/
static final Set<String> SPLIT_KEYS = Set.of("health", "coordinator");
static final Set<String> SPLIT_KEYS = Set.of("health", "coordinator", "fleet");
/**
* Top-level keys {@link #changedDeferredKeys} compares — see the class doc's Deferred bullet.
* Promoted here from a test-side copy in {@code ConfigRefTopLevelCoverageTest} by fleetd #337,
* the same reason {@link #COLD_KEYS} and {@link #SPLIT_KEYS} live here rather than in a test: a
* second, hand-maintained copy of this set is exactly the kind of thing that silently drifts
* from the method it is supposed to describe. {@code spawnReadyTimeoutMs} and
* {@code spawnReadyPollMs} are compared together in one branch and reported under the combined
* label {@code "spawnReady*"}; {@code profiles} is compared twice over (added/removed names,
* then an existing profile's launch settings) — see {@link #changedDeferredKeys}.
*
* <p>Package-private (not {@code private}) so {@code ConfigRefTopLevelCoverageTest} and
* {@code ConfigRefTopLevelReportingCoverageTest} can both read it, the same way they already
* read {@link #COLD_KEYS} and {@link #SPLIT_KEYS}.
*/
static final Set<String> DEFERRED_KEYS = Set.of(
"guard", "worktreeRoot", "worktreeGroup", "primary", "configReload",
"leadHeartbeat", "lifecycle", "spawnReadyTimeoutMs", "spawnReadyPollMs",
"quarantineCooldownSeconds", "profiles");
private final Path path;
private final AtomicReference<FleetConfig> current;
@@ -283,8 +343,14 @@ public final class ConfigRef implements Supplier<FleetConfig> {
return out;
}
/** Cold keys whose value differs between the running config and the candidate. */
private static List<String> changedColdKeys(FleetConfig old, FleetConfig fresh) {
/**
* Cold keys whose value differs between the running config and the candidate.
*
* <p>Package-private (not {@code private}) so {@code ConfigRefTopLevelReportingCoverageTest}
* can call it directly with a reflection-built {@code FleetConfig} pair, the same reason
* {@link #sameLaunchSettings} is package-private — see that test's class doc (fleetd #333).
*/
static List<String> changedColdKeys(FleetConfig old, FleetConfig fresh) {
List<String> changed = new ArrayList<>();
if (!Objects.equals(old.bind(), fresh.bind())) {
changed.add("bind");
@@ -306,8 +372,16 @@ public final class ConfigRef implements Supplier<FleetConfig> {
return changed;
}
/** Changed keys that were accepted but whose effect waits for a restart. */
private static List<String> changedDeferredKeys(FleetConfig old, FleetConfig fresh) {
/**
* Changed keys that were accepted but whose effect waits for a restart.
*
* <p>Package-private (not {@code private}) so {@code ConfigRefTopLevelReportingCoverageTest}
* can call it directly with a reflection-built {@code FleetConfig} pair, the same reason
* {@link #changedColdKeys} and {@link #changedSplitKeys} already are (fleetd #333, extended to
* this method by fleetd #337 — membership in {@link #DEFERRED_KEYS} proved nothing about this
* method on its own until then; see that test's class doc).
*/
static List<String> changedDeferredKeys(FleetConfig old, FleetConfig fresh) {
List<String> changed = new ArrayList<>();
if (!Objects.equals(old.lifecycle(), fresh.lifecycle())) {
changed.add("lifecycle");
@@ -389,12 +463,24 @@ public final class ConfigRef implements Supplier<FleetConfig> {
/**
* Split keys whose value differs between the running config and the candidate — see the class
* doc's Split bullet (fleetd #330). Unlike {@link #changedDeferredKeys}, this does not try to
* tell which sub-field moved: any change to {@code health:} or {@code coordinator:} gets the
* same fixed message, because the message already names both halves every time, so there is no
* "which half changed" question left for the caller to answer.
* doc's Split bullet (fleetd #330, extended for {@code fleet:} by fleetd #333). Unlike
* {@link #changedDeferredKeys}, this does not try to tell which sub-field moved for {@code
* health:} or {@code coordinator:}: any change to either gets the same fixed message, because
* the message already names both halves every time, so there is no "which half changed"
* question left for the caller to answer. {@code fleet:} is different on purpose — see below.
*
* <p>Package-private (not {@code private}) so {@code ConfigRefTopLevelReportingCoverageTest}
* can call it directly with a reflection-built {@code FleetConfig} pair, the same reason
* {@link #sameLaunchSettings} is package-private — see that test's class doc (fleetd #333). That
* test exists because membership in {@link #SPLIT_KEYS} proves nothing about this method on its
* own: fleetd #333 measured that dropping the {@code coordinator} branch out of this method
* while leaving {@code "coordinator"} in {@code SPLIT_KEYS} left the whole suite green except a
* hand-written {@code ConfigRefTest} case — neither {@code ConfigRefTopLevelCoverageTest} (it
* only reads the set) nor the "kept in step" assert below (it only checks the reported keys are
* a SUBSET of {@code SPLIT_KEYS}, never that every {@code SPLIT_KEYS} member has a branch here)
* would have caught it.
*/
private static List<String> changedSplitKeys(FleetConfig old, FleetConfig fresh) {
static List<String> changedSplitKeys(FleetConfig old, FleetConfig fresh) {
List<String> changed = new ArrayList<>();
if (!Objects.equals(old.health(), fresh.health())) {
changed.add("health: the monitor itself (enabled, interval, workingSuspectAfter) is "
@@ -406,13 +492,42 @@ public final class ConfigRef implements Supplier<FleetConfig> {
+ "opened once and needs a restart; the broker URI env-var name kept out of a "
+ "member's environment is read live on every spawn and already applied");
}
// fleetd #333: unlike health/coordinator above, most of `fleet:` (architects, developers,
// reviewers, charters, tabLabel) is genuinely hot — ConfigRefTest.aHotChangeIsAppliedAndRead-
// ThroughGet and aCharterChangeIsHotAndReachesTheLiveConfig prove it reaches the live config
// with no restart note. Only fleet.leaders is frozen (Fleetd.java:281 reads
// cfg.fleet().leaders() off the startup snapshot to build both the LeadTabScanner's
// tab-label-to-name map, wired into CallerResolver.withLeadsAndMembers at Fleetd.java:620/624,
// and — when herdr answered — LeadLauncher(...).ensureLeads() at Fleetd.java:315, which
// auto-launches each lead up to its `instances` count; neither is rebuilt on reload). So this
// compares fleet.leaders alone, not the whole Fleet record: comparing the whole record would
// report "split" for a tabLabel-only or charters-only change that is actually fully hot,
// which is the over-claim mirror of the under-claim bug this class exists to prevent.
if (!Objects.equals(leadersOf(old), leadersOf(fresh))) {
changed.add("fleet: fleet.leaders (each lead's tab, workspace, cwd, profile and "
+ "instances count) is read once at startup to build the LeadTabScanner's "
+ "identity map and to auto-launch leads, and neither is rebuilt on reload, so a "
+ "lead added, removed, or given a new tab: label needs a restart — until then it "
+ "stays unrecognised, and a caller from its new tab resolves as a worker, not a "
+ "lead; the rest of fleet: (architects, developers, reviewers, charters, "
+ "tabLabel) is read live through the supplier on CompositePeerLauncher and "
+ "already applied");
}
// Kept in step with SPLIT_KEYS the same way changedColdKeys is kept in step with COLD_KEYS —
// every message here must be traceable to one of the two split keys the class doc documents.
// every message here must be traceable to one of the split keys the class doc documents.
// NOTE what this does NOT prove, per the javadoc above: it does not catch a SPLIT_KEYS
// member with no branch above at all, only a branch whose message is mis-worded relative to
// the set. ConfigRefTopLevelReportingCoverageTest is what proves the former.
assert changed.stream().allMatch(m -> SPLIT_KEYS.stream().anyMatch(k -> m.startsWith(k + ":")))
: "a split entry was reported that does not start with a SPLIT_KEYS name: " + changed;
return changed;
}
/** {@code cfg.fleet().leaders()}, defensively, in case a caller hands in a non-defaulted config. */
private static Map<String, FleetConfig.Leader> leadersOf(FleetConfig cfg) {
return cfg.fleet() == null ? Map.of() : cfg.fleet().leaders();
}
/**
* {@link FleetConfig.Profile} record components deliberately left out of
* {@link #sameLaunchSettings} because they are read <em>live</em>, not baked in at spawn — see
@@ -466,8 +466,18 @@ public final class MessageService {
if (candidates.size() == 1) {
Task orphan = candidates.get(0);
if (orphan.future.complete(new Reply(Outcome.REPLIED, content))) {
if (orphan.turnId != null) {
asyncTasksByTurn.remove(orphan.turnId, orphan);
// fleetd #329 (F3): read orphan.turnId once. It used to be read twice, under no
// lock — once for this null check, once as the removal key — the same double-read
// shape fleetd #324 fixed in finishAsyncTask. clearAsyncQuestion's unlocked
// forgetTurn=true path (ask()'s timeout cleanup) can null the field between the two
// reads; capturing it once removes the torn read here too.
String turnId = orphan.turnId;
if (turnId != null) {
if (replyOrphanTurnIdRaceHookForTest != null) {
// Test-only (fleetd #329, F3): see the field's own javadoc.
replyOrphanTurnIdRaceHookForTest.run();
}
asyncTasksByTurn.remove(turnId, orphan);
}
count(FleetMetrics.REPLIES, "path", "async-recovered");
return true; // the ticket itself took it — no inbox stranding at all
@@ -1002,13 +1012,49 @@ public final class MessageService {
// Measured when #282 was merged: this guard is DEFENCE IN DEPTH, not the thing
// that makes the chained ask work. ask() calls markAsyncQuestion (:860) before
// resolveQuestion (:861), so by the time this thread wakes, the task has already
// moved to the new turnId and finishAsyncTask(oldTurnId, ...) finds nothing. Removing
// this guard alone leaves the test green. Keep it anyway: it mirrors sendAsync's
// sibling guard, and that sibling's own comment (:1017) warns the two orderings are
// not something to rely on. Do NOT delete it as dead code without re-checking that
// ordering, and do not treat it as the sole protection either.
if (result.outcome() != Outcome.QUESTION) {
finishAsyncTask(turnId, result);
// moved to the new turnId — this outcome check, not a Task lookup, is what tells the
// two cases apart (see fleetd #329 below). Removing this guard alone leaves the test
// green. Keep it anyway: it mirrors sendAsync's sibling guard, and that sibling's own
// comment (:1017) warns the two orderings are not something to rely on. Do NOT delete
// it as dead code without re-checking that ordering, and do not treat it as the sole
// protection either.
//
// fleetd #329 (F1): complete the SAME Task object this method already looked up at
// :991, rather than re-resolving it from turnId a second time. The old
// finishAsyncTask(turnId, result) did its own asyncTasksByTurn.get(turnId) here, and
// that second lookup races ask()'s own timeout path: ask()'s ticket.answer().get(...)
// can time out (or lose that exact race) at essentially the same instant this method's
// rendezvous.answerAsk(turnId, ...) above already succeeded, running
// markAskTimedOut + clearAsyncQuestion(turnId, true) with no lock at all — which
// forgets turnId (removes it from asyncTasksByTurn, nulls Task.turnId) before this
// thread ever gets here. The worker's real reply then arrived, this method's own wait
// woke up with it, and the by-turnId lookup found nothing: the async ticket's future
// was never completed, so fleet_poll{ticket} stayed PENDING forever even though
// answer() itself correctly returned REPLIED. Reusing the reference captured at :991
// — before that race window opens — sidesteps the second lookup entirely: it is the
// identical Task whatever asyncTasksByTurn or Task.turnId say by the time we reach
// this point, so finishAsyncTask(Task, Reply) can still complete its future and detach
// it using whatever turnId it now reads. The #282 chained-ask case is unaffected
// because it is still gated purely by result.outcome() == QUESTION above, which does
// not depend on this lookup — widening what "task" means here cannot complete a ticket
// the chained ask deliberately left open.
//
// A null task is NOT only "this was never an async ticket". That reading was in this
// comment when #329 merged and it is wrong. A genuine async ticket also lands here
// with task == null, because ask()'s timeout path runs clearAsyncQuestion(turnId,
// true) — which drops the asyncTasksByTurn entry — in its catch block, while
// rendezvous.closeAsk(turnId) runs later, in its finally. Between those two the ask
// is still answerable but the map entry is already gone, so the lookup at :991
// returns null and this ticket is never completed. Measured on 2026-09-04: a probe
// firing only that first half before answer() runs printed
// "answer=REPLIED phase=PENDING reply=null" — the same stranded ticket #329 set out
// to fix, one step earlier in the same race. The probe used forgetTurnForTest, which
// omits ask()'s markAskTimedOut; that cannot change the outcome, because askTimedOut
// is read only by askAnsweredAsyncTasks, and reply() never reaches it while this
// method's own waiter is live. So #329 narrows this window rather than closing it.
// Open as fleetd #334 — do not read this guard as complete.
if (result.outcome() != Outcome.QUESTION && task != null) {
finishAsyncTask(task, result);
}
return result;
} catch (TimeoutException e) {
@@ -1059,7 +1105,7 @@ public final class MessageService {
// this fires exactly once, from whichever path completes it: finishAsyncTask(task, result)
// below on any non-QUESTION outcome of send() — a worker's fleet_reply, the CB-106
// completion fallback, a CB-109 wedge, TIMED_OUT, BUSY, or BACKEND_EXHAUSTED — the same
// finishAsyncTask reached via answer()'s finishAsyncTask(turnId, result) once a QUESTION
// finishAsyncTask reached via answer()'s own finishAsyncTask(task, result) once a QUESTION
// is resolved, completeExceptionally(t) just below when send() itself throws, or a CB-516
// abandon() on teardown. Without this, MessageService.reply's rendezvous fast path (the
// one an async ticket always takes) never told the push loop anything happened — see the
@@ -1079,7 +1125,22 @@ public final class MessageService {
finishAsyncTask(task, result);
}
} catch (Throwable t) {
task.future.completeExceptionally(t);
// fleetd #329 (F2): finishAsyncTask above already completes task.future — on its very
// first line — before doing anything else, so anything that throws afterward (inside
// finishAsyncTask's own cleanup, or from a future addition to this try block) lands
// here with the future already resolved. completeExceptionally on an already-completed
// future is a silent no-op: it returns false and does nothing, so without the check
// below the exception simply vanished — no log, no metric, nothing. Measured (see the
// ticket): temporarily reintroducing the fleetd #324 NPE reproduced 19 real exceptions
// on the ordinary path, with 74/74 tests staying green and not one log line produced.
// Do not stop completing the future first — finishAsyncTask completing before it
// cleans up is what makes a late failure harmless to the ticket's own result — only
// add the missing visibility for the case where that step, or whatever ran after it,
// has already lost the race to report through the future.
if (!task.future.completeExceptionally(t)) {
log.error("async send {} -> {} threw after its ticket was already resolved",
ticket, target, t);
}
}
});
pruneTerminalTickets();
@@ -1247,6 +1308,10 @@ public final class MessageService {
*/
private void finishAsyncTask(Task task, Reply result) {
task.future.complete(result);
if (afterFinishAsyncTaskCompleteHookForTest != null) {
// Test-only (fleetd #329, F2): see the field's own javadoc.
afterFinishAsyncTaskCompleteHookForTest.run();
}
String turnId = task.turnId;
if (turnId != null) {
if (finishAsyncTaskRaceHook != null) {
@@ -1276,6 +1341,49 @@ public final class MessageService {
this.finishAsyncTaskRaceHook = hook;
}
/**
* Null in production; test seam for fleetd #329 (F2) — invoked from {@link #finishAsyncTask(Task,
* Reply)} unconditionally, immediately after {@code task.future.complete(result)} runs (before
* {@link Task#turnId} is even read, so it fires regardless of whether this task was ever asked).
* A test installs this to force an exception into exactly the shape fleetd #329 identified:
* something throws inside {@link #sendAsync}'s executor task after the async ticket's future is
* already resolved, so the surrounding {@code catch (Throwable t)} can only report failure through
* {@code completeExceptionally} — a silent no-op on an already-completed future. Engineering a
* real exception to land in that exact post-completion window is what the ticket itself had to do
* by temporarily deleting a production guard (fleetd #324's {@code turnId != null} check); this
* hook drives the identical shape deterministically instead.
*/
private volatile Runnable afterFinishAsyncTaskCompleteHookForTest;
/**
* Test-only (fleetd #329, F2): install {@link #afterFinishAsyncTaskCompleteHookForTest}.
* Package-private so the test, in the same package, can reach it without widening any production
* API.
*/
void setAfterFinishAsyncTaskCompleteHookForTest(Runnable hook) {
this.afterFinishAsyncTaskCompleteHookForTest = hook;
}
/**
* Null in production; test seam for fleetd #329 (F3) — invoked from {@link #reply} right after
* the single local read of {@code orphan.turnId} passes its null-check and before that (now-local)
* value is used as the {@code asyncTasksByTurn} removal key. Mirrors {@link
* #finishAsyncTaskRaceHook} exactly, for the structurally identical double-read fleetd #324 fixed
* in {@link #finishAsyncTask}: a test installs this to force, deterministically, {@code
* clearAsyncQuestion}'s unlocked {@code forgetTurn=true} path nulling {@link Task#turnId} in that
* exact window, and to confirm the single-read fix tolerates it (the captured local is used
* unconditionally, so a hook that nulls the field afterward cannot affect this call).
*/
private volatile Runnable replyOrphanTurnIdRaceHookForTest;
/**
* Test-only (fleetd #329, F3): install {@link #replyOrphanTurnIdRaceHookForTest}. Package-private
* so the test, in the same package, can reach it without widening any production API.
*/
void setReplyOrphanTurnIdRaceHookForTest(Runnable hook) {
this.replyOrphanTurnIdRaceHookForTest = hook;
}
/**
* Test-only (fleetd #324): run the exact production cleanup {@link #ask}'s own timeout path runs
* unlocked — {@link #clearAsyncQuestion(String, boolean)} with {@code forgetTurn=true} — so a test
@@ -1285,14 +1393,6 @@ public final class MessageService {
clearAsyncQuestion(turnId, true);
}
/** Complete the async ticket correlated to a specific answered turn. */
private void finishAsyncTask(String turnId, Reply result) {
Task task = asyncTasksByTurn.get(turnId);
if (task != null) {
finishAsyncTask(task, result);
}
}
/** A new send must not open a waiter while an async ticket owns this worker's paused turn. */
private boolean hasAsyncQuestion(String target) {
return asyncTasksByTurn.values().stream().anyMatch(task -> target.equals(task.target));
@@ -638,6 +638,79 @@ class ConfigRefTest {
assertEquals("mac-b", ref.get().coordinator().selfId());
}
/**
* fleetd #333: {@code fleet:} is split too — {@code fleet.leaders} is read only at
* {@code Fleetd.java:281} to build the {@code LeadTabScanner}'s identity map (fed into
* {@code CallerResolver}) and to auto-launch leads via {@code LeadLauncher.ensureLeads()}, and
* neither is rebuilt on reload, while the rest of {@code fleet:} (role pools, charters,
* tabLabel) is read live through the {@code CompositePeerLauncher} supplier. Before this fix
* {@code fleet} sat in the coverage checker's hot-excluded escape hatch, so a reload that
* changed only {@code fleet.leaders} reported a bare "config reloaded" — exactly the
* under-claim the split class exists to prevent for {@code health}/{@code coordinator}.
*/
@Test
void changingFleetLeadersIsReportedAsSplit(@TempDir Path dir) throws Exception {
Path f = dir.resolve("fleetd.yaml");
Files.writeString(f, yaml("""
fleet:
leaders:
opus:
tab: "lead: opus-a"
profile: sonnet
"""));
ConfigRef ref = refFor(f);
Files.writeString(f, yaml("""
fleet:
leaders:
opus:
tab: "lead: opus-b"
profile: sonnet
"""));
ConfigRef.Outcome out = ref.reload();
assertTrue(out.applied());
assertTrue(out.deferred().isEmpty(), out.deferred().toString());
assertEquals(1, out.split().size(), out.split().toString());
assertTrue(out.split().getFirst().startsWith("fleet:"), out.split().toString());
assertTrue(out.split().getFirst().contains("restart"), out.split().toString());
assertTrue(out.split().getFirst().contains("live"), out.split().toString());
assertTrue(out.summary().contains("partially live"), out.summary());
// The snapshot still carries the new value — the LeadTabScanner's identity map and
// LeadLauncher's auto-launch are what wait for a restart; a reload rebuilds neither.
assertEquals("lead: opus-b", ref.get().fleet().leaders().get("opus").tab());
}
/**
* fleetd #333: {@code fleet.leaders} is the ONLY frozen part of {@code fleet:}. A reload that
* changes {@code tabLabel} (or charters, or a role pool) without touching {@code fleet.leaders}
* must stay fully hot with nothing reported — proving {@link ConfigRef#changedSplitKeys}
* compares {@code fleet.leaders} specifically rather than the whole {@code Fleet} record, which
* would over-claim "needs a restart" for a change that is genuinely all live (the mirror
* mistake of the under-claim this class exists to prevent).
*/
@Test
void changingFleetTabLabelWithoutLeadersStaysFullyHot(@TempDir Path dir) throws Exception {
Path f = dir.resolve("fleetd.yaml");
Files.writeString(f, yaml("""
fleet:
tabLabel: "{role}: {profile} #{n}"
"""));
ConfigRef ref = refFor(f);
Files.writeString(f, yaml("""
fleet:
tabLabel: "[{profile}] {role}"
"""));
ConfigRef.Outcome out = ref.reload();
assertTrue(out.applied());
assertTrue(out.deferred().isEmpty(), out.deferred().toString());
assertTrue(out.split().isEmpty(), out.split().toString());
assertEquals("config reloaded", out.summary());
assertEquals("[{profile}] {role}", ref.get().fleet().tabLabel());
}
/**
* A split change must not refuse the reload (invariant 2 of fleetd #330) and
* {@code Outcome.applied()} must stay {@code true} (invariant 3) — unlike a cold change, the
@@ -47,21 +47,21 @@ class ConfigRefTopLevelCoverageTest {
private static final RecordComponent[] COMPONENTS = FleetConfig.class.getRecordComponents();
/**
* Top-level components whose change {@code ConfigRef.changedDeferredKeys} reads and reports on
* — verified by reading that method as of fleetd #330, not derived from this test.
* {@code spawnReadyTimeoutMs}/{@code spawnReadyPollMs} are compared together and reported under
* one combined label ({@code "spawnReady*"}); {@code profiles} is compared twice over — once for
* added/removed profile names, once for an existing profile's launch settings — and that second
* comparison excludes {@code weight}/{@code maxLoad}/{@code credentialId} as hot sub-fields,
* which is what {@link ConfigRefProfileCoverageTest} exists to keep honest at the sub-field
* level. {@code profiles} itself still belongs here, not in the hot-exclusion set below: most of
* a profile's fields are NOT read live, so citing "read live off the config supplier" for the
* whole top-level key would be false.
* Top-level components whose change {@code ConfigRef.changedDeferredKeys} reads and reports on.
* Fleetd #337 promoted this out of a hand-maintained copy here into {@link ConfigRef#DEFERRED_KEYS}
* itself, the same reason {@link ConfigRef#COLD_KEYS} and {@link ConfigRef#SPLIT_KEYS} are
* production constants rather than test-side copies: two lists that are supposed to describe the
* same method are exactly the shape that silently drifts apart. {@code spawnReadyTimeoutMs}/
* {@code spawnReadyPollMs} are compared together and reported under one combined label
* ({@code "spawnReady*"}); {@code profiles} is compared twice over — once for added/removed
* profile names, once for an existing profile's launch settings — and that second comparison
* excludes {@code weight}/{@code maxLoad}/{@code credentialId} as hot sub-fields, which is what
* {@link ConfigRefProfileCoverageTest} exists to keep honest at the sub-field level. {@code
* profiles} itself still belongs here, not in the hot-exclusion set below: most of a profile's
* fields are NOT read live, so citing "read live off the config supplier" for the whole
* top-level key would be false.
*/
private static final Set<String> DEFERRED_TOP_LEVEL_KEYS = Set.of(
"guard", "worktreeRoot", "worktreeGroup", "primary", "configReload",
"leadHeartbeat", "lifecycle", "spawnReadyTimeoutMs", "spawnReadyPollMs",
"quarantineCooldownSeconds", "profiles");
private static final Set<String> DEFERRED_TOP_LEVEL_KEYS = ConfigRef.DEFERRED_KEYS;
/**
* The escape hatch: top-level components with no reload bookkeeping at all, because every read
@@ -76,20 +76,24 @@ class ConfigRefTopLevelCoverageTest {
* <li>{@code placement} — read live by the placement policy on every spawn (class doc, Hot
* bullet; {@code ConfigRefTest.aConsumerHoldingTheRefSeesTheNewValue} proves it
* behaviourally).</li>
* <li>{@code fleet} — role pools, {@code charters} and {@code tabLabel} are read live through
* the supplier on {@code CompositePeerLauncher} (class doc, Hot bullet). The one
* documented exception, {@code fleet.leaders}, is read only at startup and genuinely needs
* a restart — a real gap in {@code changedDeferredKeys}, but one the class doc already
* carries and that fleetd #330 explicitly did not re-open (its "seven readers" fact-find
* named {@code health}/{@code coordinator} as the complete set of split-shaped keys, not
* {@code fleet}). Reported as a caveat, not fixed here.</li>
* <li>{@code memberCredentials} — read live at {@code Fleetd.java:198, 205, 729}.</li>
* <li>{@code memberLoginShell} — read live at
* {@code HerdrPeerLauncher#configuredMemberLoginShell}.</li>
* </ul>
*
* <p>{@code fleet} used to sit here too, on the strength of most of it (role pools, charters,
* tabLabel) being read the same live way — but {@code fleet.leaders} inside the same key is
* read only at startup and genuinely needs a restart, which is a real gap this escape hatch
* cannot represent: it excuses a whole top-level key from reload bookkeeping, and {@code fleet}
* needed exactly half of it excused. fleetd #333 moved it into {@link ConfigRef#SPLIT_KEYS}
* instead, where {@code changedSplitKeys} reports the frozen half by name. That is the
* cautionary tale this test's own javadoc already told: this checker proves the record's shape
* is triaged, never that a bucket a key sits in is the right one — a person has to read the
* source, which is exactly how fleetd #333 found {@code fleet} sitting in the wrong bucket
* while this test stayed green throughout.</p>
*/
private static final Set<String> HOT_EXCLUDED_TOP_LEVEL_KEYS =
Set.of("placement", "fleet", "memberCredentials", "memberLoginShell");
Set.of("placement", "memberCredentials", "memberLoginShell");
@Test
void everyTopLevelComponentIsAccountedForInExactlyOneClass() {
@@ -116,7 +120,7 @@ class ConfigRefTopLevelCoverageTest {
// The escape hatch is pinned. Growing it requires editing this line — a visible, deliberate
// diff, not a quiet one. See the field javadoc above for what "belongs here" actually means.
assertEquals(Set.of("placement", "fleet", "memberCredentials", "memberLoginShell"), hot,
assertEquals(Set.of("placement", "memberCredentials", "memberLoginShell"), hot,
"HOT_EXCLUDED_TOP_LEVEL_KEYS changed. A component belongs here ONLY if it is read "
+ "live off the config supplier, never because adding it makes this test "
+ "pass. If you are adding one to silence this test, that is fleetd #323 "
@@ -0,0 +1,284 @@
package dev.ltms.fleet.config;
import org.junit.jupiter.api.Test;
import java.lang.reflect.Constructor;
import java.lang.reflect.RecordComponent;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.TreeSet;
import static org.junit.jupiter.api.Assertions.assertEquals;
/**
* fleetd #333, finding F2: {@link ConfigRefTopLevelCoverageTest} proves every {@link FleetConfig}
* top-level component sits in exactly one of {@link ConfigRef#COLD_KEYS}, {@link
* ConfigRef#DEFERRED_KEYS}, {@link ConfigRef#SPLIT_KEYS} or the hot-excluded set. It does
* <strong>not</strong> prove that a key's membership in one of the first three sets corresponds to
* any actual comparison in {@link ConfigRef}: a key can sit in a set with no branch in {@code
* changedColdKeys}/{@code changedSplitKeys}/{@code changedDeferredKeys} checking it, and both
* {@link ConfigRefTopLevelCoverageTest} and the "kept in step" {@code assert} inside the first two
* of those methods stay green, because neither one reads the method body — the coverage test only
* reads set membership, and the assert only checks that reported entries are a SUBSET of the set,
* never that every set member produced a reported entry. {@code changedDeferredKeys} does not even
* have a "kept in step" assert of its own.
*
* <p>Measured directly, live, while fixing fleetd #333: dropping the {@code coordinator} branch out
* of {@code ConfigRef.changedSplitKeys} while leaving {@code "coordinator"} in
* {@link ConfigRef#SPLIT_KEYS} left {@link ConfigRefTopLevelCoverageTest} and the in-method assert
* both green — only a hand-written behavioural case in {@link ConfigRefTest} caught it, because it
* happened to name that exact key. This is the {@link ConfigRefProfileCoverageTest} mechanism one
* level up, generalised over every {@code COLD_KEYS}/{@code SPLIT_KEYS}/{@code DEFERRED_KEYS}
* member rather than one hand-picked field: enumerate {@link FleetConfig}'s own record components by
* reflection, build "a config where only {@code <key>} differs" for each key, and call the real
* {@link ConfigRef#changedColdKeys}/{@link ConfigRef#changedSplitKeys}/
* {@link ConfigRef#changedDeferredKeys} methods (all package-private for exactly this, the same
* reason {@link ConfigRef#sameLaunchSettings} already is) to prove each one is actually reported —
* not assumed from a set literal.
*
* <h2>fleetd #337 — DEFERRED_KEYS was the gap left open here</h2>
* This class originally covered {@code COLD_KEYS} and {@code SPLIT_KEYS} only — {@code
* DEFERRED_TOP_LEVEL_KEYS} (now {@link ConfigRef#DEFERRED_KEYS}) carried the identical one-way risk
* in principle, unexercised. fleetd #337 measured the real consequence rather than assuming it from
* the shape of the gap: dropping {@code guard}'s comparison out of {@code changedDeferredKeys} while
* {@code "guard"} stayed in the set left all 1355 tests green — the same failure mode {@code
* coordinator} demonstrated for {@code SPLIT_KEYS} in fleetd #333, now confirmed for {@code
* DEFERRED_KEYS} too. Re-deriving the full list by mutation (drop each key's branch in turn, run the
* suite, restore) found six of the eleven {@code DEFERRED_KEYS} members with no behavioural test in
* {@link ConfigRefTest} naming them: {@code guard}, {@code leadHeartbeat}, {@code worktreeRoot},
* {@code spawnReadyTimeoutMs}, {@code spawnReadyPollMs} and {@code quarantineCooldownSeconds}. That
* list corrects fleetd #333's own guess at it in two ways the mutation proved and a reading did not:
* {@code lifecycle} is NOT on it — {@code ConfigRefTest.aDeferredChangeIsAppliedAndReported} already
* names it, and dropping its branch fails that test — and {@code worktreeRoot} IS on it, which #333
* never named at all. The other five {@code DEFERRED_KEYS} members ({@code lifecycle}, {@code
* worktreeGroup}, {@code primary}, {@code configReload}, {@code profiles}) already had a hand-written
* case each. {@link #everyDeferredKeyIsActuallyReportedByChangedDeferredKeys} below now covers all
* eleven the reflective way, so the six with no hand-written test are no longer silently unpinned —
* every {@code DEFERRED_KEYS} component turned out to be a scalar or a simple record, so, unlike
* {@code fleet.leaders} in fleetd #333, none needed an exclusion set: {@link #BASE}/{@link #ALT} give
* every top-level component (not only {@code COLD_KEYS}/{@code SPLIT_KEYS}) a real, distinct value.
*/
class ConfigRefTopLevelReportingCoverageTest {
private static final RecordComponent[] COMPONENTS = FleetConfig.class.getRecordComponents();
/**
* One valid value per top-level {@link FleetConfig} component — "the a value". fleetd #337 gave
* every {@code DEFERRED_KEYS} component a real value here too (previously left {@code null} on
* both sides, which meant {@code mutate(key)} produced no actual difference for any of them);
* only {@code placement}, {@code memberCredentials} and {@code memberLoginShell} — the
* hot-excluded set, never compared by any {@code changed*Keys} method — stay {@code null}.
* {@link FleetConfig}'s compact constructor only normalizes {@code profiles}, so every other
* field accepts whatever is put here unmutated.
*/
private static final Map<String, Object> BASE = baseValues();
/** The same shape, each value distinct from {@link #BASE} — "the b value". */
private static final Map<String, Object> ALT = altValues();
private static Map<String, Object> baseValues() {
Map<String, Object> v = new LinkedHashMap<>();
v.put("bind", new FleetConfig.Bind("127.0.0.1", 8765));
v.put("herdrSocket", "~/.config/herdr/a.sock");
v.put("memberHerdrSocket", "~/.config/herdr/member-a.sock");
v.put("profiles", Map.of());
v.put("guard", new FleetConfig.Guard(List.of("host-a")));
v.put("worktreeRoot", "/wt/a");
v.put("lifecycle", new FleetConfig.Lifecycle(300, 5, 30, false));
v.put("spawnReadyTimeoutMs", 5000);
v.put("spawnReadyPollMs", 100);
v.put("broker", new FleetConfig.Broker("amqp://a", null, 1));
v.put("primary", new FleetConfig.Primary("term-a", 1, 1000));
v.put("fleet", new FleetConfig.Fleet(
Map.of("opus", new FleetConfig.Leader("sonnet", "lead: opus-a", 1, null, 10,
"claude", null, null, null)),
Map.of(), Map.of(), Map.of(), Map.of(), "{role}: {profile} #{n}"));
v.put("leadHeartbeat", new FleetConfig.LeadHeartbeat(300, 60_000L, 3));
v.put("health", new FleetConfig.Health(true, 30, 600, null, null));
v.put("placement", null);
v.put("auth", new FleetConfig.Auth("loopback-trust", null));
v.put("configReload", new FleetConfig.ConfigReload(true, 10));
v.put("quarantineCooldownSeconds", 1800);
v.put("memberCredentials", null);
v.put("coordinator", new FleetConfig.Coordinator("amqp://coord-a", null, "self-a", 1));
v.put("worktreeGroup", "group-a");
v.put("memberLoginShell", null);
assertNamesMatchComponents(v);
return v;
}
private static Map<String, Object> altValues() {
Map<String, Object> v = new LinkedHashMap<>();
v.put("bind", new FleetConfig.Bind("127.0.0.2", 8766));
v.put("herdrSocket", "~/.config/herdr/b.sock");
v.put("memberHerdrSocket", "~/.config/herdr/member-b.sock");
// A single added profile — enough to trip the "added/removed" comparison in
// ConfigRef.changedDeferredKeys, which is all this mechanism needs to prove "profiles" has
// a branch behind it; the launch-settings comparison already has its own hand-written cases
// in ConfigRefTest (changingAProfilesLaunchSettingsIsReportedAsDeferred and siblings).
v.put("profiles", Map.of("sonnet", minimalProfile("sonnet")));
v.put("guard", new FleetConfig.Guard(List.of("host-b")));
v.put("worktreeRoot", "/wt/b");
v.put("lifecycle", new FleetConfig.Lifecycle(600, 10, 60, true));
v.put("spawnReadyTimeoutMs", 10_000);
v.put("spawnReadyPollMs", 200);
v.put("broker", new FleetConfig.Broker("amqp://b", null, 2));
v.put("primary", new FleetConfig.Primary("term-b", 2, 2000));
// Differs from BASE.fleet only in fleet.leaders (a different tab for "opus") — the frozen
// sub-field ConfigRef.changedSplitKeys actually compares. A Fleet that instead differed only
// in tabLabel would correctly NOT be reported (see
// ConfigRefTest.changingFleetTabLabelWithoutLeadersStaysFullyHot) and would wrongly fail this
// test — that is by design, not a gap: this map exists to prove fleet.leaders is covered.
v.put("fleet", new FleetConfig.Fleet(
Map.of("opus", new FleetConfig.Leader("sonnet", "lead: opus-b", 1, null, 10,
"claude", null, null, null)),
Map.of(), Map.of(), Map.of(), Map.of(), "{role}: {profile} #{n}"));
v.put("leadHeartbeat", new FleetConfig.LeadHeartbeat(600, 120_000L, 5));
v.put("health", new FleetConfig.Health(false, 90, 900, null, null));
v.put("placement", null);
v.put("auth", new FleetConfig.Auth("token", "TOKEN_ENV"));
v.put("configReload", new FleetConfig.ConfigReload(false, 20));
v.put("quarantineCooldownSeconds", 3600);
v.put("memberCredentials", null);
v.put("coordinator", new FleetConfig.Coordinator("amqp://coord-b", null, "self-b", 2));
v.put("worktreeGroup", "group-b");
v.put("memberLoginShell", null);
assertNamesMatchComponents(v);
return v;
}
/** A minimal, otherwise-null {@link FleetConfig.Profile} — just enough to name one in a map. */
private static FleetConfig.Profile minimalProfile(String name) {
return new FleetConfig.Profile(name, null, null, null, null, null, null, null, null, null,
null, null, null, null, null, null, null, null, null, null, null, null, null, null,
null, null);
}
private static void assertNamesMatchComponents(Map<String, Object> values) {
Set<String> names = new TreeSet<>();
for (RecordComponent rc : COMPONENTS) {
names.add(rc.getName());
}
assertEquals(names, new TreeSet<>(values.keySet()),
"this test's value map has drifted from FleetConfig's actual top-level components — "
+ "update BASE/ALT alongside the record");
}
private static FleetConfig configOf(Map<String, Object> values) throws ReflectiveOperationException {
Class<?>[] types = Arrays.stream(COMPONENTS).map(RecordComponent::getType).toArray(Class<?>[]::new);
Object[] args = Arrays.stream(COMPONENTS).map(rc -> values.get(rc.getName())).toArray();
Constructor<FleetConfig> ctor = FleetConfig.class.getDeclaredConstructor(types);
return ctor.newInstance(args);
}
/** {@code BASE} with exactly one named top-level component swapped for its {@code ALT} value. */
private static FleetConfig mutate(String componentName) throws ReflectiveOperationException {
Map<String, Object> values = new LinkedHashMap<>(BASE);
values.put(componentName, ALT.get(componentName));
return configOf(values);
}
/**
* The mechanism fleetd #333 F2 asked for, applied to {@link ConfigRef#COLD_KEYS}: mutate each
* cold key in isolation and prove {@code changedColdKeys} actually names it, not just that
* {@code COLD_KEYS} claims it does.
*/
@Test
void everyColdKeyIsActuallyReportedByChangedColdKeys() throws ReflectiveOperationException {
FleetConfig base = configOf(BASE);
List<String> uncovered = new ArrayList<>();
for (String key : new TreeSet<>(ConfigRef.COLD_KEYS)) {
FleetConfig mutated = mutate(key);
if (!ConfigRef.changedColdKeys(base, mutated).contains(key)) {
uncovered.add(key);
}
}
System.out.printf(
"ConfigRef.changedColdKeys reporting coverage — %d COLD_KEYS, %d verified%n",
ConfigRef.COLD_KEYS.size(), ConfigRef.COLD_KEYS.size() - uncovered.size());
assertEquals(List.of(), uncovered,
"these keys are in ConfigRef.COLD_KEYS but mutating them alone produces no matching "
+ "entry from changedColdKeys — a set entry with no comparison behind it: "
+ uncovered);
}
/**
* The same mechanism applied to {@link ConfigRef#SPLIT_KEYS}: mutate each split key in
* isolation and prove {@code changedSplitKeys} actually names it (message starting with
* {@code "<key>:"}), not just that {@code SPLIT_KEYS} claims it does. This is the exact check
* that would have failed fleetd #333's own reproduction — dropping the {@code coordinator}
* branch from {@code changedSplitKeys} while {@code "coordinator"} stayed in {@code SPLIT_KEYS}
* — see the mutation proof in the fleetd #333 PR description; the earlier checkers here (the
* shape test and the in-method assert) do not.
*/
@Test
void everySplitKeyIsActuallyReportedByChangedSplitKeys() throws ReflectiveOperationException {
FleetConfig base = configOf(BASE);
List<String> uncovered = new ArrayList<>();
for (String key : new TreeSet<>(ConfigRef.SPLIT_KEYS)) {
FleetConfig mutated = mutate(key);
List<String> split = ConfigRef.changedSplitKeys(base, mutated);
if (split.stream().noneMatch(s -> s.startsWith(key + ":"))) {
uncovered.add(key);
}
}
System.out.printf(
"ConfigRef.changedSplitKeys reporting coverage — %d SPLIT_KEYS, %d verified%n",
ConfigRef.SPLIT_KEYS.size(), ConfigRef.SPLIT_KEYS.size() - uncovered.size());
assertEquals(List.of(), uncovered,
"these keys are in ConfigRef.SPLIT_KEYS but mutating them alone produces no matching "
+ "entry from changedSplitKeys — a set entry with no comparison behind it, "
+ "exactly the fleetd #333 F2 shape: " + uncovered);
}
/**
* For most {@link ConfigRef#DEFERRED_KEYS} members, {@code changedDeferredKeys} reports the key
* name verbatim — the default this map assumes. Two entries don't: {@code spawnReadyTimeoutMs}
* and {@code spawnReadyPollMs} are compared together in one branch and reported under the
* combined label {@code "spawnReady*"} (see {@link ConfigRef#changedDeferredKeys}). {@code
* profiles} keeps the default: mutating it here only exercises the added/removed comparison
* (see {@link #altValues}), which reports {@code "profiles (added/removed: …)"} — starts with
* {@code "profiles"}, same as the default would expect.
*/
private static final Map<String, String> DEFERRED_REPORT_PREFIX = Map.of(
"spawnReadyTimeoutMs", "spawnReady*",
"spawnReadyPollMs", "spawnReady*");
/**
* fleetd #337: the same mechanism applied to {@link ConfigRef#DEFERRED_KEYS}, closing the gap
* this class's own javadoc left open since fleetd #333. Mutate each deferred key in isolation
* and prove {@code changedDeferredKeys} actually names it (message starting with the key's
* expected report prefix — see {@link #DEFERRED_REPORT_PREFIX}), not just that {@code
* DEFERRED_KEYS} claims it does. This is the exact check that fails for {@code guard} the way
* {@code coordinator} failed {@link #everySplitKeyIsActuallyReportedByChangedSplitKeys} in
* fleetd #333 — verified live: dropping {@code guard}'s branch from {@code changedDeferredKeys}
* while {@code "guard"} stayed in {@code DEFERRED_KEYS} left the whole 1355-test suite green,
* and this test is what now catches it (it fails naming {@code guard} with that mutation in
* place).
*/
@Test
void everyDeferredKeyIsActuallyReportedByChangedDeferredKeys() throws ReflectiveOperationException {
FleetConfig base = configOf(BASE);
List<String> uncovered = new ArrayList<>();
for (String key : new TreeSet<>(ConfigRef.DEFERRED_KEYS)) {
FleetConfig mutated = mutate(key);
List<String> deferred = ConfigRef.changedDeferredKeys(base, mutated);
String prefix = DEFERRED_REPORT_PREFIX.getOrDefault(key, key);
if (deferred.stream().noneMatch(s -> s.startsWith(prefix))) {
uncovered.add(key);
}
}
System.out.printf(
"ConfigRef.changedDeferredKeys reporting coverage — %d DEFERRED_KEYS, %d verified%n",
ConfigRef.DEFERRED_KEYS.size(), ConfigRef.DEFERRED_KEYS.size() - uncovered.size());
assertEquals(List.of(), uncovered,
"these keys are in ConfigRef.DEFERRED_KEYS but mutating them alone produces no "
+ "matching entry from changedDeferredKeys — a set entry with no comparison "
+ "behind it, exactly the fleetd #333 F2 shape, confirmed here for "
+ "DEFERRED_KEYS by fleetd #337: " + uncovered);
}
}
@@ -1,5 +1,9 @@
package dev.ltms.fleet.msg;
import ch.qos.logback.classic.Level;
import ch.qos.logback.classic.Logger;
import ch.qos.logback.classic.spi.ILoggingEvent;
import ch.qos.logback.core.read.ListAppender;
import dev.ltms.fleet.herdr.AgentControl;
import dev.ltms.fleet.herdr.AgentStatus;
import dev.ltms.fleet.herdr.FakeHerdr;
@@ -11,6 +15,7 @@ import dev.ltms.fleet.mcp.PrimaryRegistry;
import dev.ltms.fleet.inject.Injector;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.slf4j.LoggerFactory;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.TimeUnit;
@@ -1041,6 +1046,105 @@ class MessageServiceTest {
"the reply completed its own ticket directly and never touched the inbox");
}
/**
* fleetd #329 (F1). {@code answer()} completes the async ticket by looking {@code turnId} up in
* {@code asyncTasksByTurn} a SECOND time (the first is at :991, purely to re-register the
* {@code asyncTasksByWaiter} entry for #282's chained-ask case). That second lookup races
* {@code ask()}'s own unlocked timeout cleanup ({@code clearAsyncQuestion(turnId, true)}, run from
* {@code markAskTimedOut} + forgetting): {@code ask()}'s {@code ticket.answer().get(timeoutMillis)}
* can time out at essentially the same instant {@code answer()}'s {@code rendezvous.answerAsk}
* call above already succeeded and unblocked the worker. fleetd #324's single-read fix does not
* help here — that fixed a torn read of one already-held {@link MessageService} internal
* {@code Task}; this is a second, independent map lookup by a caller that no longer holds the
* {@code Task} it already found once.
*
* <p>This test does not wait for that real race to land on its own schedule — it drives the exact
* sequence the ticket describes (worker asks, primary answers, worker's real reply arrives) and
* fires the identical production cleanup {@code ask()}'s timeout path runs
* ({@code clearAsyncQuestion(turnId, true)}, via the {@code forgetTurnForTest} seam fleetd #324
* already merged) at the point between "the worker's own {@code ask()} call has unblocked" and
* "the worker's real {@code fleet_reply} arrives" — the exact window fleetd #329 names.
*
* <p>What this proves: given that exact interleaving, the async ticket must still resolve to the
* worker's real reply, not stay stuck {@link MessageService.Phase#PENDING} forever. What it does
* not prove: that the interleaving itself is reachable in production on its own timing — that is
* established by reading the code (see the ticket's "path in"), not by this test, for the same
* reason fleetd #324's own race test says so.
*/
@Test
void aReplyRacingAsksTimeoutCleanupStillCompletesTheAsyncTicket() throws Exception {
String ticket = messages.sendAsync(T, "task that asks");
awaitWaiting();
injectDelivery();
CompletableFuture<MessageService.AskResult> ask =
CompletableFuture.supplyAsync(() -> messages.ask(T, "which config?", 5000));
MessageService.TaskView asking = awaitTicketPhase(ticket, MessageService.Phase.ASKING);
String turnId = asking.turnId();
CompletableFuture<MessageService.Reply> answer =
CompletableFuture.supplyAsync(() -> messages.answer(turnId, "config.yaml", 5000));
assertEquals("config.yaml", ask.get(5, TimeUnit.SECONDS).answer(),
"the worker's own ask() call must have already unblocked with the primary's answer "
+ "before we force the race below");
// ask()'s own unlocked timeout cleanup can forget this exact turnId at essentially the same
// instant answer() has already unblocked the worker and is now waiting on the resumed turn's
// real reply — reproduce that interleaving directly instead of trying to win a real race.
messages.forgetTurnForTest(turnId);
assertTrue(messages.reply(T, "PR opened: https://example/pulls/42"));
assertEquals(MessageService.Outcome.REPLIED, answer.get(5, TimeUnit.SECONDS).outcome(),
"the primary's own answer() call must still see the worker's real reply");
MessageService.TaskView done = awaitTicketPhase(ticket, MessageService.Phase.DONE);
assertEquals("PR opened: https://example/pulls/42", done.reply(),
"fleet_poll{ticket} must return the worker's real reply, not stay PENDING forever "
+ "just because ask()'s timeout cleanup forgot this turnId first");
}
/**
* fleetd #329 (F3). {@link MessageService#reply} reads the same orphan {@code Task}'s {@code
* turnId} twice on this path (once to check it is non-null, once as the {@code
* asyncTasksByTurn.remove} key) — the exact double-read shape fleetd #324 fixed in {@code
* finishAsyncTask}. This test drives the same real sequence as {@code
* aReplyAfterAnswerTimesOutStillCompletesTheAsyncTicket} (worker asks, primary answers, the
* primary's own bounded wait for the resumed turn expires) to reach a task with {@code turnId}
* genuinely stamped and no live rendezvous waiter open — the state {@code askAnsweredAsyncTasks}
* matches here — then fires the identical production cleanup {@code ask()}'s own timeout path
* runs ({@code clearAsyncQuestion(turnId, true)}, via the {@code forgetTurnForTest} seam fleetd
* #324 merged) at the point between the check and the removal use, via a dedicated test hook
* mirroring {@code finishAsyncTaskRaceHook}.
*/
@Test
void replyToAnOrphanedTaskSurvivesTurnIdGoingNullBetweenItsTwoReads() throws Exception {
String ticket = messages.sendAsync(T, "task that asks");
awaitWaiting();
injectDelivery();
CompletableFuture<MessageService.AskResult> ask =
CompletableFuture.supplyAsync(() -> messages.ask(T, "which config?", 5000));
MessageService.TaskView asking = awaitTicketPhase(ticket, MessageService.Phase.ASKING);
String turnId = asking.turnId();
MessageService.Reply answerReply = messages.answer(turnId, "config.yaml", 150);
assertEquals("config.yaml", ask.get(5, TimeUnit.SECONDS).answer());
assertEquals(MessageService.Outcome.TIMED_OUT_WORKING, answerReply.outcome(),
"the primary's own bounded wait must give up first, leaving turnId stamped with no "
+ "live waiter — the state reply()'s F3 code path matches");
messages.setReplyOrphanTurnIdRaceHookForTest(() -> messages.forgetTurnForTest(turnId));
try {
assertTrue(messages.reply(T, "PR opened: https://example/pulls/42"));
MessageService.TaskView done = awaitTicketPhase(ticket, MessageService.Phase.DONE);
assertEquals("PR opened: https://example/pulls/42", done.reply(),
"the ticket must still resolve to the worker's real reply despite the forced race");
} finally {
messages.setReplyOrphanTurnIdRaceHookForTest(null);
}
}
/**
* fleetd #307's ambiguity guard: an ask timeout frees its target ({@code hasAsyncQuestion}
* becomes false the instant it lapses — proven above), so a second, independent delegation can
@@ -1151,6 +1255,63 @@ class MessageServiceTest {
assertEquals("done", done.reply());
}
// --- fleetd #329 (F2): an exception after the ticket's future completes must reach a log -----
//
// sendAsync's own executor task ends with catch (Throwable t) { task.future.completeExceptionally(t); }
// — but finishAsyncTask completes that same future on its first line, so anything that throws
// afterward hits an already-completed future: completeExceptionally returns false and does
// nothing, and (before this fix) nothing logged it either. Measured on the ticket: temporarily
// reintroducing the fleetd #324 NPE reproduced 19 real exceptions on the ordinary path with
// 74/74 tests staying green and zero log lines. There is no reachable production call site where
// finishAsyncTask throws after completing the future (fleetd #324 already closed the one that
// used to), so this test drives the shape directly via a dedicated test-only hook
// (afterFinishAsyncTaskCompleteHookForTest) rather than trying to engineer a real exception into
// that narrow window — the same technique fleetd #324's own race test and this ticket's F1/F3
// tests use for their own races.
private static ListAppender<ILoggingEvent> attachMessageServiceLog() {
Logger logger = (Logger) LoggerFactory.getLogger(MessageService.class);
ListAppender<ILoggingEvent> appender = new ListAppender<>();
appender.start();
logger.addAppender(appender);
return appender;
}
private static void detachMessageServiceLog(ListAppender<ILoggingEvent> appender) {
((Logger) LoggerFactory.getLogger(MessageService.class)).detachAppender(appender);
}
@Test
void anExceptionAfterTheTicketFutureCompletesStillReachesTheLog() throws Exception {
ListAppender<ILoggingEvent> appender = attachMessageServiceLog();
try {
messages.setAfterFinishAsyncTaskCompleteHookForTest(() -> {
throw new RuntimeException("PROBE-329-F2");
});
String ticket = messages.sendAsync(T, "do the task");
awaitWaiting();
assertTrue(rendezvous.resolve(T, "done"));
// finishAsyncTask's own first line already completed the future with the real result
// BEFORE the hook threw — F2 is a visibility gap, not a correctness gap for the ticket
// itself, so the ticket's own outcome must be unaffected by the swallowed exception.
MessageService.TaskView done = awaitTicketPhase(ticket, MessageService.Phase.DONE);
assertEquals("done", done.reply());
assertTrue(appender.list.stream().anyMatch(e ->
e.getLevel() == Level.ERROR
&& e.getFormattedMessage().contains(ticket)
&& e.getThrowableProxy() != null
&& "PROBE-329-F2".equals(e.getThrowableProxy().getMessage())),
"an exception thrown after the ticket's future already completed must still "
+ "reach the log, not vanish silently");
} finally {
messages.setAfterFinishAsyncTaskCompleteHookForTest(null);
detachMessageServiceLog(appender);
}
}
// --- CB-582: fleet_status pendingAsk() ------------------------------------------------------
@Test