fleetd #612 B3 correction: distinguish lead vs member herdr in FleetdLeadRolloverAssemblyTest

Ticket comment 17553 on fleetd #612 found that the test's single shared
FakeHerdr made router.leadAgents() and router.memberAgents() collapse to
the identical client (FleetdAssembly.java:140-142's no-distinct-socket
fallback), so a mutation swapping leadAgents() for memberAgents() at the
FleetdAssembly.java:408 call site was invisible to this test even though
the two are genuinely different daemons in production.

Configure two distinct herdr sockets and two distinct FakeHerdr instances
(the same TwoHerdrResourcePorts shape B2's FleetdAssemblyConnectionIdentityTest
uses) and assert the roll's /clear + bootstrap sends land on the LEAD fake
and never on the MEMBER one.

Proven red against the router.memberAgents() mutation, reverted, touched,
and re-run green — both outputs recorded in the PR.
This commit is contained in:
Dai Ha
2026-09-22 12:31:39 +07:00
parent bc49d87cb8
commit 6edeb70bc4
@@ -15,6 +15,7 @@ import org.junit.jupiter.api.io.TempDir;
import java.nio.file.Files; import java.nio.file.Files;
import java.nio.file.Path; import java.nio.file.Path;
import java.util.LinkedHashMap;
import java.util.List; import java.util.List;
import java.util.Map; import java.util.Map;
import java.util.concurrent.Executors; import java.util.concurrent.Executors;
@@ -38,12 +39,31 @@ import static org.junit.jupiter.api.Assertions.fail;
* #absentLeadRolloverConfigMeansNoRolloverIsBuilt} — a claim this ticket found was NOT actually * #absentLeadRolloverConfigMeansNoRolloverIsBuilt} — a claim this ticket found was NOT actually
* covered behaviourally anywhere else: {@code LeadRolloverTest}'s only related assertion is * covered behaviourally anywhere else: {@code LeadRolloverTest}'s only related assertion is
* vacuous, {@code assertNull(null)}, and never calls the real factory). * vacuous, {@code assertNull(null)}, and never calls the real factory).
*
* <p><strong>fleetd #612 B3 correction (ticket comment 17553):</strong> the first version of this
* test configured a single shared {@link FakeHerdr} for both the lead and member herdr sockets.
* {@code FleetdAssembly.java:140-142} falls back to {@code memberHerdr = herdr} whenever no
* distinct {@code memberHerdrSocket} is configured, so with one fake, {@code
* router.leadAgents()} and {@code router.memberAgents()} wrapped the identical client — a
* mutation swapping {@code Fleetd.leadRollover(cfg, router.leadAgents(), config, leads)} for
* {@code ..., router.memberAgents(), ...} at {@code FleetdAssembly.java:408} was therefore
* invisible to this test, even though the two are genuinely different daemons in production. This
* version configures two distinct sockets and two distinct {@link FakeHerdr} instances (the same
* pattern {@code FleetdAssemblyConnectionIdentityTest}, fleetd #612 B2, already uses to separate
* lead from member) and asserts the roll's {@code /clear}/bootstrap sends land on the LEAD fake
* and never on the MEMBER one.
*/ */
class FleetdLeadRolloverAssemblyTest { class FleetdLeadRolloverAssemblyTest {
private static final Path LEAD_SOCKET = Path.of("/fake/lead-herdr.sock");
private static final Path MEMBER_SOCKET = Path.of("/fake/member-herdr.sock");
/** Keys {@code connectHerdr} by socket path so the lead and member daemons can be two
* DIFFERENT {@link FakeHerdr}s — same shape as B2's {@code FleetdAssemblyConnectionIdentityTest
* .TwoHerdrResourcePorts}. */
private static final class RecordingResourcePorts implements ResourcePorts { private static final class RecordingResourcePorts implements ResourcePorts {
final FakeHerdr herdr = new FakeHerdr(); final Map<Path, HerdrClient> herdrsBySocket = new LinkedHashMap<>();
final SentinelReplyInbox replyInbox = new SentinelReplyInbox(); final SentinelReplyInbox replyInbox = new SentinelReplyInbox();
@Override @Override
@@ -53,7 +73,11 @@ class FleetdLeadRolloverAssemblyTest {
@Override @Override
public HerdrClient connectHerdr(Path socketPath) { public HerdrClient connectHerdr(Path socketPath) {
return herdr; HerdrClient client = herdrsBySocket.get(socketPath);
if (client == null) {
throw new IllegalStateException("no fake herdr registered for socket " + socketPath);
}
return client;
} }
@Override @Override
@@ -127,6 +151,8 @@ class FleetdLeadRolloverAssemblyTest {
bind: bind:
host: 127.0.0.1 host: 127.0.0.1
port: 8765 port: 8765
herdrSocket: "%s"
memberHerdrSocket: "%s"
idleSleepGuard: idleSleepGuard:
enabled: false enabled: false
broker: broker:
@@ -139,7 +165,7 @@ class FleetdLeadRolloverAssemblyTest {
leadRollover: leadRollover:
handoverPath: handover.md handoverPath: handover.md
requireOperatorConfirm: false requireOperatorConfirm: false
""".formatted(leadCwd.toString())); """.formatted(LEAD_SOCKET, MEMBER_SOCKET, leadCwd.toString()));
return FleetConfig.load(f); return FleetConfig.load(f);
} }
@@ -154,7 +180,13 @@ class FleetdLeadRolloverAssemblyTest {
ConfigRef config = new ConfigRef(dir.resolve("fleetd.yaml"), cfg); ConfigRef config = new ConfigRef(dir.resolve("fleetd.yaml"), cfg);
SubscriptionGuard guard = new SubscriptionGuard(cfg.guard().hostSet()); SubscriptionGuard guard = new SubscriptionGuard(cfg.guard().hostSet());
RecordingResourcePorts ports = new RecordingResourcePorts(); RecordingResourcePorts ports = new RecordingResourcePorts();
ports.herdr.withTab("w2", "w2:t7", "lead: opus"); // Two DISTINCT fakes — one per configured socket — so leadAgents()/memberAgents() wrap
// genuinely different clients, exactly like production when memberHerdrSocket is set.
FakeHerdr lead = new FakeHerdr();
lead.withTab("w2", "w2:t7", "lead: opus");
FakeHerdr member = new FakeHerdr();
ports.herdrsBySocket.put(LEAD_SOCKET, lead);
ports.herdrsBySocket.put(MEMBER_SOCKET, member);
FleetdRuntime runtime = FleetdAssembly.assembleAndStart(new AssemblyInputs(cfg, config, guard), ports); FleetdRuntime runtime = FleetdAssembly.assembleAndStart(new AssemblyInputs(cfg, config, guard), ports);
@@ -188,19 +220,30 @@ class FleetdLeadRolloverAssemblyTest {
+ "the turn-boundary wait settles immediately and the post-/clear wait " + "the turn-boundary wait settles immediately and the post-/clear wait "
+ "releases via its pickup-grace path — detail: " + status.detail()); + "releases via its pickup-grace path — detail: " + status.detail());
// Prove the real herdr router actually sent BOTH messages, in order, to the real pane — // Prove the real herdr router actually sent BOTH messages, in order, to the real LEAD
// this is the one thing a source-text pin on the call site could never show. // pane — this is the one thing a source-text pin on the call site could never show.
List<FakeHerdr.Call> prompts = ports.herdr.calls.stream() List<FakeHerdr.Call> prompts = lead.calls.stream()
.filter(c -> c.method().equals("agent.prompt")) .filter(c -> c.method().equals("agent.prompt"))
.toList(); .toList();
assertTrue(prompts.size() >= 2, "expected at least a /clear send and a bootstrapText send, " assertTrue(prompts.size() >= 2, "expected at least a /clear send and a bootstrapText send "
+ "got " + prompts.size() + " agent.prompt calls: " + prompts); + "on the LEAD daemon, got " + prompts.size() + " agent.prompt calls: " + prompts);
assertEquals("/clear", ((Map<String, Object>) prompts.get(0).params()).get("text"), assertEquals("/clear", ((Map<String, Object>) prompts.get(0).params()).get("text"),
"the first send must be the literal /clear housekeeping command"); "the first send must be the literal /clear housekeeping command");
Object secondText = ((Map<String, Object>) prompts.get(1).params()).get("text"); Object secondText = ((Map<String, Object>) prompts.get(1).params()).get("text");
assertTrue(secondText instanceof String && ((String) secondText).contains(expectedHandoverPath), assertTrue(secondText instanceof String && ((String) secondText).contains(expectedHandoverPath),
"the second send must be the default bootstrapText naming the resolved handover " "the second send must be the default bootstrapText naming the resolved handover "
+ "path, got: " + secondText); + "path, got: " + secondText);
// fleetd #612 B3 correction: prove the roll never touches the MEMBER daemon. A mutation
// swapping router.leadAgents() for router.memberAgents() at the real call site would move
// both sends above onto `member` instead, which this assertion catches — the thing the
// single-fake version of this test could never see, because both wrapped the same client.
List<FakeHerdr.Call> memberPrompts = member.calls.stream()
.filter(c -> c.method().equals("agent.prompt"))
.toList();
assertTrue(memberPrompts.isEmpty(), "the roll must be wired to the LEAD daemon only — got "
+ memberPrompts.size() + " agent.prompt call(s) on the MEMBER daemon instead: "
+ memberPrompts);
} }
private static LeadRollover.RollStatus pollUntilTerminal(LeadRollover rollover, String token) private static LeadRollover.RollStatus pollUntilTerminal(LeadRollover rollover, String token)