fleetd #267: warn once per profile when the model check can't run
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.
This commit is contained in:
@@ -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<String> 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<String> 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<String> 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
|
||||
|
||||
@@ -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<String> 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<ILoggingEvent> 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<MemberSession> 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.
|
||||
*
|
||||
* <p>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<String> 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<ILoggingEvent> 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<MemberSession> 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<String> 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));
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user