From e028a0ae54bbd068ff476720cd8b135a856dfeca Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Fri, 4 Sep 2026 08:36:12 +0700 Subject: [PATCH] fleetd #267: warn once per profile when the model check can't run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OpenCodeLauncher.SessionAwareHandle.agentSessionId() is the only caller of checkModelMatch (fleetd #175), and it sits behind the fleetd #249 worktree gate. A spawn with no worktree:true — the ordinary shape of most opencode spawns — never reached the check at all, and the gap was totally silent. The check cannot be decoupled from agentSessionId()'s resolved id: doing so would re-derive 'whatever is newest in the shared directory' and reintroduce the false-positive risk fleetd #234 fixed (a sibling's differently-configured model looking like a mismatch for a profile that never actually ran it). The #249 gate is correct and stays as-is. Instead, log once per profile at WARN, naming the profile, the same treatment discoveryUnavailable already gets a few lines above — a logged UNKNOWN beats a check that silently never runs. --- .../ltms/fleet/member/OpenCodeLauncher.java | 41 +++++- .../fleet/member/OpenCodeLauncherTest.java | 127 ++++++++++++++++++ 2 files changed, 165 insertions(+), 3 deletions(-) diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java b/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java index badcfc6..e7330dc 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/OpenCodeLauncher.java @@ -21,6 +21,7 @@ import java.util.EnumSet; import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import java.util.function.BooleanSupplier; @@ -669,6 +670,16 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { private final AtomicBoolean discoveryUnavailableWarned = new AtomicBoolean(); + /** + * fleetd #267: one WARN per PROFILE (not per launcher instance — several profiles can each hit + * this gap independently) for the model-mismatch check (fleetd #175) never getting to run + * because the spawn was not given a fleetd-provisioned worktree (fleetd #249). Profile names + * accumulate here for the life of this launcher instance and are never removed — the same + * one-shot treatment {@link #discoveryUnavailableWarned} already gets, just keyed per profile + * instead of globally. + */ + private final Set modelCheckSkippedWarned = ConcurrentHashMap.newKeySet(); + /** Add lazy on-disk session discovery to the base handle. */ @Override public PeerHandle spawn(SpawnRequest req) { @@ -695,7 +706,8 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { // be running. FleetConfig.Profile cfg = requireProfile(req.profileName()); return new SessionAwareHandle(inner, discovery, cwd, cfg, - this::memberHerdrSocketConfigured, discoveryUnavailableWarned, exhaustionSink); + this::memberHerdrSocketConfigured, discoveryUnavailableWarned, + modelCheckSkippedWarned, exhaustionSink); } /** @@ -720,6 +732,7 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { private final FleetConfig.Profile cfg; private final BooleanSupplier discoveryUnavailable; private final AtomicBoolean discoveryUnavailableWarned; + private final Set modelCheckSkippedWarned; private final ExhaustionSink exhaustionSink; /** CAS'd true the first (and only) time a model mismatch is reported for this handle. */ private final AtomicBoolean modelMismatchReported = new AtomicBoolean(); @@ -750,6 +763,7 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { FleetConfig.Profile cfg, BooleanSupplier discoveryUnavailable, AtomicBoolean discoveryUnavailableWarned, + Set modelCheckSkippedWarned, ExhaustionSink exhaustionSink) { this.delegate = delegate; this.discovery = discovery; @@ -757,6 +771,7 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { this.cfg = cfg; this.discoveryUnavailable = discoveryUnavailable; this.discoveryUnavailableWarned = discoveryUnavailableWarned; + this.modelCheckSkippedWarned = modelCheckSkippedWarned; this.exhaustionSink = exhaustionSink; this.worktreeProvisioned = isProvisionedWorktree(cwd); } @@ -807,9 +822,29 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher { // member's row apart from a sibling's in that case (measured: a three-day-old row from // a different profile). Refuse to guess — absent is the honest answer, and it is what // this codebase already returns elsewhere for absent evidence (fleetd #175's UNKNOWN). - // No WARN here: unlike discoveryUnavailable above, this is the ordinary, expected shape - // of the large majority of spawns (no worktree requested), not a configuration gap. + // This IS the ordinary, expected shape of the large majority of spawns (no worktree + // requested), not a configuration gap — but fleetd #267 found that same shape silently + // switches off the fleetd #175 model-mismatch check for those spawns too, since + // checkModelMatch's only call site is right below this gate. The check cannot be moved + // off agentSessionId()'s resolved id: the id is the only safe way to key + // actualModelForSessionId to THIS session's own row rather than "whatever is newest in + // the shared directory" (fleetd #234) — re-deriving a second, independent answer via + // `directory` here would reintroduce exactly the false-positive risk #234 fixed (a + // sibling's differently-configured model looking like THIS profile's mismatch). So the + // model genuinely is unknowable without a provisioned worktree, and unlike the silence + // this branch used to keep, that gap now gets the same one-time, per-profile WARN + // treatment discoveryUnavailable already gets above — but keyed by profile, since + // several profiles can each hit this independently. if (!worktreeProvisioned) { + if (cfg.model() != null && !cfg.model().isBlank() + && modelCheckSkippedWarned.add(cfg.profile())) { + log.warn("opencode model-mismatch check (fleetd #175) cannot run for profile " + + "'{}': it was spawned without a fleetd-provisioned worktree (fleetd " + + "#249), so its cwd may be shared with other sessions and the actual " + + "model it is running cannot be safely told apart from a sibling's — " + + "spawn with worktree:true to enable the check for this profile.", + cfg.profile()); + } return null; } // fleetd #234: once resolved, stay resolved. Re-deriving from `directory` on every call diff --git a/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java b/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java index d10e98f..f0d700a 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/OpenCodeLauncherTest.java @@ -1440,4 +1440,131 @@ class OpenCodeLauncherTest { "the profile hint must survive the Fleetd-style forwarding hop and reach the real " + "sink — a lambda forwarder drops it and this must go red"); } + + // --- fleetd #267: the #175 check never ran for the ordinary (no-worktree) spawn shape -------- + + /** + * fleetd #267 acceptance criterion 2, half 1 — a regression guard for the NEW code path only: + * a spawn WITH a fleetd-provisioned worktree must keep running the fleetd #175 model check + * exactly as before (already proven thoroughly above), and must now ALSO never emit the new + * fleetd #267 "cannot run" WARN, since the check is not skipped in this shape. Driven through + * the real {@code SessionManager.acquire()}/{@code get()} late-resolve path (fleetd #209), + * the same path the existing #175 tests already exercise. + */ + @Test + void aProvisionedWorktreeSpawnRunsTheModelCheckThroughSessionManagerAndNeverLogsTheSkipWarn( + @TempDir Path configRoot, @TempDir Path discRoot) throws Exception { + String workDir = provisionedWorkDir(configRoot); + FakeHerdr herdr = new FakeHerdr(); + FleetConfig.Profile cfg = opencodeCfg("opencode/nemotron-3-ultra-free", null, null); + List exhausted = new ArrayList<>(); + ExhaustionSink sink = (target, reason, profile) -> exhausted.add(reason); + OpenCodeLauncher launcher = serviceWithSink(herdr, configRoot, discRoot, cfg, sink); + SessionManager sessions = new SessionManager(launcher); + + Logger logger = (Logger) LoggerFactory.getLogger(OpenCodeLauncher.class); + ListAppender appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + try { + MemberSession acquired = sessions.acquire(cfg.profile(), workDir, null, null); + assertNull(acquired.agentSessionId(), "no opencode row yet"); + + OpenCodeSessionDiscoveryTest.writeRecord(discRoot, "ses_x", workDir, 1000L, + "{\"id\":\"gpt-5.6-sol\",\"providerID\":\"openai\"}"); + + Optional after = sessions.get(acquired.paneId()); + assertEquals("ses_x", after.get().agentSessionId()); + } finally { + logger.detachAppender(appender); + } + + assertEquals(1, exhausted.size(), + "the mismatch check still runs on the real path with a provisioned worktree: " + exhausted); + boolean cannotRunWarn = appender.list.stream() + .filter(e -> e.getLevel() == Level.WARN) + .anyMatch(e -> e.getFormattedMessage().contains("cannot run")); + assertFalse(cannotRunWarn, "a provisioned-worktree spawn must never log the fleetd #267 " + + "'cannot run' WARN — the check ran, it was not skipped: " + appender.list); + } + + /** + * fleetd #267's central defect, reproduced and fixed: {@code OpenCodeLauncher.SessionAwareHandle + * .agentSessionId()} is the ONLY caller of {@code checkModelMatch}, and it sits behind the + * fleetd #249 worktree gate — so a plain {@code fleet_spawn} with no {@code worktree:true} + * (the ticket's "ordinary, expected shape of the large majority of spawns") never reached + * {@code checkModelMatch} at all. A test that called {@code checkModelMatch} directly, or built + * a {@link OpenCodeLauncher.SessionAwareHandle}/{@link PeerHandle} in isolation, would have + * passed on every single day this gap existed — it never drives {@code agentSessionId()} + * through the worktree gate the way production does. This test instead drives the REAL + * late-resolve path: {@code SessionManager.acquire()} (which calls {@code handle + * .agentSessionId()} to build the very first {@code MemberSession}) and a re-poll via {@code + * SessionManager.get()} (fleetd #209's retained-handle mechanism) — the exact sequence a live + * pane goes through. + * + *

The fix chosen (see {@code OpenCodeLauncher}'s javadoc on the {@code !worktreeProvisioned} + * branch) is the WARN path, not a decoupled check: {@code actualModelForSessionId} can only be + * keyed safely by a RESOLVED session id (fleetd #234's fix for exactly this false-positive + * risk), and without a provisioned worktree no id can ever be safely resolved (fleetd #249) — + * re-deriving "whatever is newest in this shared directory" here would silently reintroduce the + * false-positive risk #234 fixed. This test proves both halves: a plausible-looking mismatch + * row for the shared, non-provisioned cwd never quarantines anything, AND the new one-time, + * per-profile WARN replaces the old total silence. + */ + @Test + void aSpawnWithoutAProvisionedWorktreeNeverRunsTheModelCheckButWarnsOncePerProfile( + @TempDir Path configRoot, @TempDir Path discRoot) throws Exception { + // Deliberately NOT provisionedWorkDir(...) / markAsProvisionedWorktree(...): a plain + // directory with no .git marker — the exact "fleet_spawn with no worktree:" shape fleetd + // #267 is about, and the ordinary shape the ticket says most spawns actually take. + Path workDir = Files.createDirectories(configRoot.resolve("shared-cwd")); + FakeHerdr herdr = new FakeHerdr(); + FleetConfig.Profile cfg = opencodeCfg("opencode/nemotron-3-ultra-free", null, null); + List exhausted = new ArrayList<>(); + ExhaustionSink sink = (target, reason, profile) -> exhausted.add(reason); + OpenCodeLauncher launcher = serviceWithSink(herdr, configRoot, discRoot, cfg, sink); + SessionManager sessions = new SessionManager(launcher); + + Logger logger = (Logger) LoggerFactory.getLogger(OpenCodeLauncher.class); + ListAppender appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + try { + // The real production entrypoint: acquire() calls handle.agentSessionId() itself to + // build the very first MemberSession, BEFORE any row exists. + MemberSession acquired = sessions.acquire(cfg.profile(), workDir.toString(), null, null); + assertNull(acquired.agentSessionId(), + "still refuses to guess an identity for a shared, non-provisioned cwd (fleetd #249)"); + + // A row for this exact (shared) directory appears, running a model that WOULD look like + // a mismatch against cfg.model() if fleetd trusted the shared-directory heuristic — + // exactly the false-positive shape fleetd #234 fixed for the id-resolved case. + OpenCodeSessionDiscoveryTest.writeRecord(discRoot, "ses_sibling", workDir.toString(), 1000L, + "{\"id\":\"gpt-5.6-sol\",\"providerID\":\"openai\"}"); + + // Re-drive the SAME real late-resolve path (fleetd #209) — repeatedly, to also prove + // the new WARN fires at most once per profile, not once per poll. + Optional resolved = sessions.get(acquired.paneId()); + assertNull(resolved.get().agentSessionId(), "still no identity — the gate never opens"); + sessions.get(acquired.paneId()); + } finally { + logger.detachAppender(appender); + } + + assertTrue(exhausted.isEmpty(), + "must never quarantine off a shared-directory row it cannot trust as this session's " + + "own — fleetd #234's exact concern, now also for the model check: " + exhausted); + + List skipWarnings = appender.list.stream() + .filter(e -> e.getLevel() == Level.WARN) + .map(ILoggingEvent::getFormattedMessage) + .filter(m -> m.contains("cannot run")) + .toList(); + assertEquals(1, skipWarnings.size(), + "exactly one 'cannot run' WARN across acquire() + two get() re-polls — the old code " + + "logged NOTHING here, which is the bug this ticket fixes; got: " + skipWarnings); + assertTrue(skipWarnings.get(0).contains(cfg.profile()), + "the WARN must name the profile, same treatment discoveryUnavailable already gets: " + + skipWarnings.get(0)); + } }