fleetd #234: key the opencode model check on the resolved session id, and make the spawn-time quarantine actually happen
Defect 1: OpenCodeSessionDiscovery.actualModelForDirectory queried WHERE directory = ?, the same heuristic sessionIdForDirectory uses. Since a default fleet_spawn (no worktree:) shares the lead's cwd with every other worker and every past session ever run there, the model read-back could silently compare against a DIFFERENT session's row. Renamed to actualModelForSessionId(sessionId), keyed on the primary key id instead, and made OpenCodeLauncher's SessionAwareHandle cache the resolved id once non-null (AtomicReference) so a later sibling row in the same directory can never flip which session's evidence is read. sessionIdForDirectory (#209) is left directory-based on purpose, with a comment explaining why the heuristic is unavoidable at that layer. Defect 2: the ERROR log claimed "quarantining this profile's credential" but Fleetd's ExhaustionSink lambda resolved target -> roster -> profile -> credential, while OpenCodeLauncher's model-mismatch check fires from agentSessionId() during SessionManager.acquire(), before the session is registered in the roster -- the lookup found nothing and silently no-opped. Added a default 3-arg ExhaustionSink.onExhausted(target, reason, profile) overload (defaults to the 2-arg method, so CompletionResolver's two call sites are unchanged); OpenCodeLauncher now passes its own already-known profile name; Fleetd's sink became an anonymous class that tries the roster first, falls back to the hint, and logs loudly at ERROR naming target/reason when neither resolves, instead of silently no-oping. Both fixes proven by mutation: reverting each independently makes its new test fail with a real assertion message, restoring makes it pass again. mvn clean install: Tests run: 1127, Failures: 0, Errors: 0, Skipped: 0, BUILD SUCCESS.
This commit is contained in:
@@ -345,17 +345,46 @@ public final class Fleetd {
|
||||
// model-mismatch check — it needs nothing profile-specific from the caller beyond `target`
|
||||
// (a herdr terminal id) and `reason`, so reusing it here is exactly "the existing
|
||||
// ExhaustionSink path", not a new mechanism.
|
||||
ExhaustionSink exhaustionSink = (target, reason) -> sessions.roster().stream()
|
||||
.filter(session -> target.equals(session.terminalId()))
|
||||
.findFirst()
|
||||
.map(MemberSession::profile)
|
||||
.map(profileName -> config.get().profiles().get(profileName))
|
||||
.ifPresent(profile -> {
|
||||
String credentialId = profile.effectiveCredentialId();
|
||||
quarantine.quarantine(credentialId);
|
||||
log.warn("credential '{}' quarantined for {}s (profile '{}'): {}", credentialId,
|
||||
cfg.quarantineCooldownSeconds(), profile.profile(), reason);
|
||||
});
|
||||
//
|
||||
// fleetd #234: that check fires from SessionAwareHandle.agentSessionId(), which runs during
|
||||
// SessionManager.acquire() BEFORE this session is registered in sessions.roster() — so the
|
||||
// roster-only lookup below used to find nothing, .ifPresent silently no-op'd, and the
|
||||
// ERROR the check had just logged ("quarantining this profile's credential") was a lie:
|
||||
// nothing was quarantined, and nothing said so. Two changes: (1) OpenCodeLauncher now
|
||||
// passes its OWN profile name via ExhaustionSink's 3-arg overload — it already has the
|
||||
// FleetConfig.Profile in hand and does not need the roster at all — used here as a
|
||||
// fallback whenever the roster lookup misses; (2) if a profile still cannot be resolved
|
||||
// (neither the roster nor the hint names a configured one), this logs loudly at ERROR
|
||||
// instead of silently doing nothing — a control that cannot act must say so.
|
||||
ExhaustionSink exhaustionSink = new ExhaustionSink() {
|
||||
@Override
|
||||
public void onExhausted(String target, String reason) {
|
||||
onExhausted(target, reason, null);
|
||||
}
|
||||
|
||||
@Override
|
||||
public void onExhausted(String target, String reason, String profileHint) {
|
||||
String profileName = sessions.roster().stream()
|
||||
.filter(session -> target.equals(session.terminalId()))
|
||||
.findFirst()
|
||||
.map(MemberSession::profile)
|
||||
.orElse(profileHint);
|
||||
FleetConfig.Profile profile = profileName == null ? null : config.get().profiles().get(profileName);
|
||||
if (profile == null) {
|
||||
log.error("quarantine requested for target '{}' ({}) but no profile could be "
|
||||
+ "resolved — the target is not (yet) in the roster, and {} — "
|
||||
+ "credential NOT quarantined (fleetd #234)",
|
||||
target, reason,
|
||||
profileHint == null ? "no profile hint was given"
|
||||
: "the hinted profile '" + profileHint + "' is not configured");
|
||||
return;
|
||||
}
|
||||
String credentialId = profile.effectiveCredentialId();
|
||||
quarantine.quarantine(credentialId);
|
||||
log.warn("credential '{}' quarantined for {}s (profile '{}'): {}", credentialId,
|
||||
cfg.quarantineCooldownSeconds(), profile.profile(), reason);
|
||||
}
|
||||
};
|
||||
// fleetd #175: point the forwarding sink handed to OpenCodeLauncher above at the real one,
|
||||
// now that `sessions` exists to resolve target -> session -> profile.
|
||||
exhaustionSinkRef.set(exhaustionSink);
|
||||
|
||||
@@ -20,6 +20,32 @@ public interface ExhaustionSink {
|
||||
*/
|
||||
void onExhausted(String target, String reason);
|
||||
|
||||
/**
|
||||
* Same notification, plus a profile name the CALLER already knows — for a caller whose
|
||||
* {@code target} is not yet resolvable through whatever roster the sink's implementation
|
||||
* consults (fleetd #234). {@link dev.ltms.fleet.member.OpenCodeLauncher}'s model-mismatch check
|
||||
* fires from {@code SessionAwareHandle.agentSessionId()}, which runs during {@code
|
||||
* SessionManager.acquire()} <em>before</em> that session is registered — a target -> session ->
|
||||
* profile lookup finds nothing at that point. That launcher already has its own {@code
|
||||
* FleetConfig.Profile} in hand and does not need the roster to know which profile to
|
||||
* quarantine, so it calls this overload instead of leaving the sink to guess.
|
||||
*
|
||||
* <p>Defaults to the two-arg overload, discarding {@code profile} — the correct behaviour for
|
||||
* every caller that has not been updated to supply one: {@link
|
||||
* dev.ltms.fleet.inject.CompletionResolver}'s two call sites always call {@code target} that
|
||||
* IS live in the roster at the time of the call, so they need no hint and keep working exactly
|
||||
* as before. A sink that wants to use the hint (see {@code Fleetd.main}'s wiring) overrides this
|
||||
* method directly rather than relying on the default.
|
||||
*
|
||||
* @param target as {@link #onExhausted(String, String)}
|
||||
* @param reason as {@link #onExhausted(String, String)}
|
||||
* @param profile the profile the caller already knows should be quarantined, or {@code null}
|
||||
* when the caller has no better answer than {@code target} alone
|
||||
*/
|
||||
default void onExhausted(String target, String reason, String profile) {
|
||||
onExhausted(target, reason);
|
||||
}
|
||||
|
||||
/**
|
||||
* Inert sink — nothing happens on exhaustion. The explicit stand-in a caller (or a test not
|
||||
* exercising this feature) passes instead of a defaulting overload, exactly like
|
||||
|
||||
@@ -22,6 +22,7 @@ import java.util.List;
|
||||
import java.util.Map;
|
||||
import java.util.Set;
|
||||
import java.util.concurrent.atomic.AtomicBoolean;
|
||||
import java.util.concurrent.atomic.AtomicReference;
|
||||
import java.util.function.BooleanSupplier;
|
||||
import java.util.function.Function;
|
||||
import java.util.function.LongSupplier;
|
||||
@@ -706,6 +707,17 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher {
|
||||
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();
|
||||
/**
|
||||
* The session id, once {@link OpenCodeSessionDiscovery#sessionIdForDirectory} first
|
||||
* resolves a non-null answer for this handle (fleetd #234). Sticky on purpose: {@code
|
||||
* directory} is a shared-cwd heuristic (see {@link OpenCodeSessionDiscovery}'s class
|
||||
* javadoc) that can start returning a DIFFERENT row once another session shares the same
|
||||
* directory and writes a newer one — re-deriving it on every call would let this handle's
|
||||
* identity silently drift to a sibling's session. Once resolved, this IS the answer, and
|
||||
* {@link #checkModelMatch} reads only the row this id names, never "whatever is newest in
|
||||
* the directory right now."
|
||||
*/
|
||||
private final AtomicReference<String> resolvedSessionId = new AtomicReference<>();
|
||||
|
||||
SessionAwareHandle(PeerHandle delegate, OpenCodeSessionDiscovery discovery, String cwd,
|
||||
FleetConfig.Profile cfg,
|
||||
@@ -759,17 +771,27 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher {
|
||||
}
|
||||
return null;
|
||||
}
|
||||
// fleetd #234: once resolved, stay resolved. Re-deriving from `directory` on every call
|
||||
// would let this handle's identity drift to a sibling session that later shares the
|
||||
// same cwd and writes a newer row — see resolvedSessionId's javadoc.
|
||||
String cached = resolvedSessionId.get();
|
||||
if (cached != null) {
|
||||
return cached;
|
||||
}
|
||||
// Lazy + retried, never a spawn-time blocker: opencode writes the session record only
|
||||
// when the session is first persisted, so null here is the correct interim answer and
|
||||
// the caller re-calls later (each call re-scans, picking up a record that has since
|
||||
// appeared).
|
||||
String id = discovery.sessionIdForDirectory(cwd);
|
||||
if (id != null) {
|
||||
resolvedSessionId.compareAndSet(null, id);
|
||||
}
|
||||
// fleetd #175: check on the SAME tick — while the caller (SessionManager's late-resolve
|
||||
// step) is still re-polling because the id is unknown, the row this id came from (once
|
||||
// it exists) is exactly the row that also carries the actual model. Once id resolves,
|
||||
// the caller stops calling agentSessionId() for this session, so this is naturally a
|
||||
// once-only check that happens right when the row first appears.
|
||||
checkModelMatch();
|
||||
checkModelMatch(id);
|
||||
return id;
|
||||
}
|
||||
|
||||
@@ -777,16 +799,25 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher {
|
||||
* Verify the live opencode session is running the model {@link #cfg} requested (fleetd
|
||||
* #175) and, on a real mismatch, log an ERROR and quarantine through {@link
|
||||
* #exhaustionSink}. A no-op when there is nothing to compare against — no model configured,
|
||||
* already reported once for this handle, or the actual model is still UNKNOWN (no row yet,
|
||||
* unreadable database, or unparseable evidence). UNKNOWN must never be treated as a
|
||||
* mismatch: that is the single most important safety rule here — a false positive would
|
||||
* already reported once for this handle, {@code sessionId} itself is not resolved yet
|
||||
* (fleetd #234: absent evidence, not a mismatch), or the actual model is still UNKNOWN (no
|
||||
* row yet, unreadable database, or unparseable evidence). UNKNOWN must never be treated as
|
||||
* a mismatch: that is the single most important safety rule here — a false positive would
|
||||
* quarantine a perfectly working profile's credential.
|
||||
*
|
||||
* @param sessionId the id {@link #agentSessionId()} just resolved (or had cached) for THIS
|
||||
* handle — the model is read back for this exact session (fleetd #234's
|
||||
* {@link OpenCodeSessionDiscovery#actualModelForSessionId}), never
|
||||
* re-derived from {@code directory}
|
||||
*/
|
||||
private void checkModelMatch() {
|
||||
private void checkModelMatch(String sessionId) {
|
||||
if (modelMismatchReported.get() || cfg.model() == null || cfg.model().isBlank()) {
|
||||
return;
|
||||
}
|
||||
OpenCodeSessionDiscovery.ActualModel actual = discovery.actualModelForDirectory(cwd);
|
||||
if (sessionId == null || sessionId.isBlank()) {
|
||||
return; // id not resolved yet — UNKNOWN, never a mismatch (fleetd #175's rule)
|
||||
}
|
||||
OpenCodeSessionDiscovery.ActualModel actual = discovery.actualModelForSessionId(sessionId);
|
||||
if (actual == null) {
|
||||
return; // UNKNOWN evidence — never a mismatch
|
||||
}
|
||||
@@ -817,10 +848,16 @@ public final class OpenCodeLauncher extends HerdrPeerLauncher {
|
||||
+ "falls back to a default model, which may be a PAID credential "
|
||||
+ "(fleetd #175); quarantining this profile's credential",
|
||||
cfg.profile(), cfg.model(), actualDisplay);
|
||||
// fleetd #234: pass our OWN profile name too. This check fires from agentSessionId(),
|
||||
// called during SessionManager.acquire() BEFORE this session is registered in
|
||||
// sessions.roster() — a roster-only sink (Fleetd's target -> session -> profile lookup)
|
||||
// finds nothing at this point and silently no-ops (defect 2). We already know exactly
|
||||
// which profile to quarantine without the roster; the sink is passed it explicitly.
|
||||
exhaustionSink.onExhausted(delegate.terminalId(),
|
||||
"opencode model mismatch: profile '" + cfg.profile() + "' requested '"
|
||||
+ cfg.model() + "' but the live session is running '" + actualDisplay
|
||||
+ "' (fleetd #175)");
|
||||
+ "' (fleetd #175)",
|
||||
cfg.profile());
|
||||
}
|
||||
|
||||
@Override
|
||||
|
||||
@@ -32,11 +32,19 @@ import java.util.concurrent.atomic.AtomicBoolean;
|
||||
* here, so a layout change, or a switch to the HTTP server, changes exactly one class and nothing
|
||||
* in {@link OpenCodeLauncher}.
|
||||
*
|
||||
* <p>The determinism that makes this useful is structural, not a guess: every fleetd worker runs
|
||||
* in its own unique git worktree, so the row's {@code directory} (its project root) equals the
|
||||
* worker's cwd identifies <em>its</em> session unambiguously. We match on {@code directory} rather
|
||||
* than diffing {@code opencode session list} before/after — that races under concurrent spawns, and
|
||||
* the CLI listing does not even show the directory.
|
||||
* <p><strong>The {@code directory} match is a heuristic, not an identity — fleetd #234.</strong> A
|
||||
* worktree is opt-in: {@code fleet_spawn} only provisions one when the caller passes {@code
|
||||
* worktree:}; the default spawn inherits the lead's own cwd, which every other worker spawned the
|
||||
* same way (and every past session ever run there) shares. {@code directory} therefore does
|
||||
* <em>not</em> identify a session unambiguously in general — only in the special case of a fresh,
|
||||
* unique worktree does "most recently updated row for this directory" reliably mean "this worker's
|
||||
* own row." {@link #sessionIdForDirectory} still has to use this heuristic (the id has to come from
|
||||
* somewhere, and nothing else is available at this layer — see that method's javadoc), but a caller
|
||||
* that already holds a resolved id must never re-derive evidence about that same session via
|
||||
* {@code directory} again; see {@link #actualModelForSessionId}, which looks up by {@code id}
|
||||
* instead for exactly this reason. We match on {@code directory} rather than diffing
|
||||
* {@code opencode session list} before/after — that races under concurrent spawns, and the CLI
|
||||
* listing does not even show the directory.
|
||||
*
|
||||
* <p>All reads are best-effort and never throw: a missing or unreadable database, a query that
|
||||
* fails, or a directory with no row yet all yield {@code null}, and the caller (the session
|
||||
@@ -89,8 +97,16 @@ final class OpenCodeSessionDiscovery {
|
||||
/**
|
||||
* The opencode session id whose row references {@code directory} (the worker's cwd), or
|
||||
* {@code null} when no row matches yet. When several rows share the directory — e.g. repeated
|
||||
* spawns into the same worktree — the row with the highest {@code time_updated} wins: it is
|
||||
* the session the pane most likely corresponds to.
|
||||
* spawns into the same worktree, OR several workers sharing one cwd because none of them was
|
||||
* given a worktree (fleetd #234) — the row with the highest {@code time_updated} wins: it is
|
||||
* the session the pane most likely corresponds to. That "most likely" is a real caveat, not a
|
||||
* formality: when the directory is shared, this can and does pick another session's row (see
|
||||
* the class javadoc). This is the one place fleetd resolves an opencode session id at all —
|
||||
* nothing else is available at this layer to disambiguate further (no {@code opencode session
|
||||
* list} entry names the directory, and diffing before/after races under concurrent spawns) — so
|
||||
* the heuristic stays here unchanged. What must never happen is a SECOND, independent piece of
|
||||
* evidence about the same session being re-derived via {@code directory} once an id has already
|
||||
* come out of this method; see {@link #actualModelForSessionId}.
|
||||
*
|
||||
* <p>Never throws: a missing {@code opencode.db}, a locked/unreadable database, a query
|
||||
* failure, or a directory that has not been persisted yet all resolve to {@code null} rather
|
||||
@@ -134,24 +150,36 @@ final class OpenCodeSessionDiscovery {
|
||||
}
|
||||
|
||||
/**
|
||||
* The model opencode actually ran the {@code directory}'s most-recent session on (fleetd
|
||||
* #175), read from the same row {@link #sessionIdForDirectory} matches — but via its own
|
||||
* query and its own connection, deliberately kept independent so a database whose schema
|
||||
* predates the {@code model} column (or any other read failure on this column alone) can
|
||||
* never take {@link #sessionIdForDirectory}'s id resolution down with it. That would be a
|
||||
* regression of the id-resolution feature #209 shipped; this method degrades on its own.
|
||||
* The model opencode actually ran the session {@code sessionId} on (fleetd #175/#234), read by
|
||||
* primary-key lookup — the ONE row that id names, and no other. Deliberately keyed on
|
||||
* {@code id} rather than {@code directory}: two independent {@code WHERE directory = ? ORDER BY
|
||||
* time_updated DESC LIMIT 1} queries (one for the id, one for the model) can each pick a
|
||||
* DIFFERENT row once more than one session shares a directory (fleetd #234 — the default
|
||||
* no-worktree spawn shares the lead's cwd with every other worker and every past session ever
|
||||
* run there), silently comparing a profile's requested model against a session that is not even
|
||||
* the one whose id was returned. Keying on {@code id} instead makes that impossible: the model
|
||||
* read back is always the SAME session {@link #sessionIdForDirectory} (or a cached copy of its
|
||||
* answer) already resolved.
|
||||
*
|
||||
* <p>Kept as its own query and its own connection, independent from {@link
|
||||
* #sessionIdForDirectory}: a database whose schema predates the {@code model} column (or any
|
||||
* other read failure on this column alone) can never take id resolution down with it. That
|
||||
* would be a regression of the id-resolution feature #209 shipped; this method degrades on its
|
||||
* own.
|
||||
*
|
||||
* <p>Never throws, and every failure mode — no matching row, a missing/unreadable database, a
|
||||
* missing {@code model} column, a null/blank {@code model} value, or JSON that does not parse
|
||||
* into {@code {"id": "...", "providerID": "..."}} with a non-blank {@code id} — resolves to
|
||||
* {@code null}. That is UNKNOWN evidence, not a mismatch signal: the caller must never
|
||||
* quarantine a profile on the strength of a {@code null} here.
|
||||
* quarantine a profile on the strength of a {@code null} here. A blank/null {@code sessionId}
|
||||
* (the id is not resolved yet) is UNKNOWN too, for the same reason — never call this with one.
|
||||
*
|
||||
* @param directory the worker's cwd, as resolved for this spawn
|
||||
* @param sessionId the session id already resolved by {@link #sessionIdForDirectory} for this
|
||||
* spawn — never re-derived from {@code directory} here
|
||||
* @return the actual model, or {@code null} when unknown
|
||||
*/
|
||||
ActualModel actualModelForDirectory(String directory) {
|
||||
if (directory == null || directory.isBlank()) {
|
||||
ActualModel actualModelForSessionId(String sessionId) {
|
||||
if (sessionId == null || sessionId.isBlank()) {
|
||||
return null;
|
||||
}
|
||||
if (!Files.isRegularFile(databasePath)) {
|
||||
@@ -159,10 +187,10 @@ final class OpenCodeSessionDiscovery {
|
||||
// exact condition — do not double-log it here.
|
||||
return null;
|
||||
}
|
||||
String sql = "SELECT model FROM session WHERE directory = ? ORDER BY time_updated DESC LIMIT 1";
|
||||
String sql = "SELECT model FROM session WHERE id = ?";
|
||||
try (Connection connection = openReadOnly();
|
||||
PreparedStatement statement = connection.prepareStatement(sql)) {
|
||||
statement.setString(1, directory);
|
||||
statement.setString(1, sessionId);
|
||||
try (ResultSet rows = statement.executeQuery()) {
|
||||
if (rows.next()) {
|
||||
return parseModel(rows.getString("model"));
|
||||
|
||||
@@ -17,6 +17,7 @@ import dev.ltms.fleet.peer.MemberRole;
|
||||
import dev.ltms.fleet.peer.PeerHandle;
|
||||
import dev.ltms.fleet.peer.PeerUnreachableException;
|
||||
import dev.ltms.fleet.peer.SpawnRequest;
|
||||
import dev.ltms.fleet.placement.BackendQuarantine;
|
||||
import dev.ltms.fleet.session.MemberSession;
|
||||
import dev.ltms.fleet.session.SessionManager;
|
||||
import org.junit.jupiter.api.Test;
|
||||
@@ -34,6 +35,7 @@ import java.util.Optional;
|
||||
import java.util.concurrent.ExecutorService;
|
||||
import java.util.concurrent.Executors;
|
||||
import java.util.concurrent.Future;
|
||||
import java.util.concurrent.TimeUnit;
|
||||
import java.util.function.Supplier;
|
||||
|
||||
import static org.junit.jupiter.api.Assertions.*;
|
||||
@@ -52,6 +54,19 @@ class OpenCodeLauncherTest {
|
||||
null, null, gitTokenEnv, null, FleetConfig.Profile.KIND_OPENCODE);
|
||||
}
|
||||
|
||||
/**
|
||||
* A profile carrying an explicit {@code credentialId} (fleetd #234 defect 2) — distinct from
|
||||
* the profile's own name, so a test can assert on the credential precisely rather than relying
|
||||
* on {@code effectiveCredentialId()}'s profile-name fallback.
|
||||
*/
|
||||
private static FleetConfig.Profile opencodeCfgWithCredential(String profileName, String model,
|
||||
String credentialId) {
|
||||
return new FleetConfig.Profile(profileName, null, model, null, "FLEETD_WORKER_TOKEN",
|
||||
List.of("opencode"), "tab", "fleetd-workers", "opencode: {model} #{n}", null,
|
||||
null, List.of(), null, null, FleetConfig.Profile.KIND_OPENCODE, Map.of(), 1.0f,
|
||||
null, false, null, credentialId, null);
|
||||
}
|
||||
|
||||
/** Gate-disabled launcher whose per-spawn config dirs land under an inspectable temp root. */
|
||||
private static OpenCodeLauncher service(FakeHerdr herdr, Path configRoot, FleetConfig.Profile cfg) {
|
||||
return new OpenCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr),
|
||||
@@ -1133,4 +1148,142 @@ class OpenCodeLauncherTest {
|
||||
assertEquals("ses_x", handle.agentSessionId());
|
||||
assertTrue(exhausted.isEmpty(), "no model configured → nothing to compare: " + exhausted);
|
||||
}
|
||||
|
||||
// --- fleetd #234 defect 1: key the model check on the RESOLVED id, not the shared directory --
|
||||
|
||||
/**
|
||||
* The exact shape fleetd #234 reported: {@code fleet_spawn} with no {@code worktree:} shares
|
||||
* the lead's cwd across every worker, so more than one session row can exist for the SAME
|
||||
* {@code directory}. Once THIS handle's own session id is resolved, a sibling member spawned
|
||||
* later into the same shared directory — writing a NEWER, unrelated row — must never make the
|
||||
* already-resolved session look mismatched. Today's code re-derives "the newest row in this
|
||||
* directory" on every call (both for the id AND, independently, for the model), so it would
|
||||
* pick up the sibling's row on the second call and flag a false mismatch AND flip the returned
|
||||
* id. The fix (fleetd #234) makes the id sticky once resolved and reads the model back for
|
||||
* exactly that id (see {@link OpenCodeSessionDiscovery#actualModelForSessionId}) — never
|
||||
* "whatever is newest in the directory right now."
|
||||
*/
|
||||
@Test
|
||||
void modelCheckReadsTheResolvedSessionsOwnRowNotWhateverIsNewestInTheSharedDirectory(
|
||||
@TempDir Path configRoot, @TempDir Path discRoot) throws Exception {
|
||||
List<String> exhausted = new ArrayList<>();
|
||||
ExhaustionSink sink = (target, reason) -> exhausted.add(reason);
|
||||
FleetConfig.Profile cfg = opencodeCfg("openai/gpt-5.6-terra", null, null);
|
||||
PeerHandle handle = serviceWithSink(new FakeHerdr(), configRoot, discRoot, cfg, sink)
|
||||
.spawn(new SpawnRequest(null, "/work/dir", null));
|
||||
|
||||
// Our own session's row, correctly matching the profile's requested model.
|
||||
OpenCodeSessionDiscoveryTest.writeRecord(discRoot, "ses_ours", "/work/dir", 1000L,
|
||||
"{\"id\":\"gpt-5.6-terra\",\"providerID\":\"openai\"}");
|
||||
assertEquals("ses_ours", handle.agentSessionId(), "resolves to our own session");
|
||||
assertTrue(exhausted.isEmpty(), "matching model → no mismatch on first resolve: " + exhausted);
|
||||
|
||||
// A sibling member, spawned later into the SAME shared directory (no worktree, fleetd
|
||||
// #234's default), writes a newer row running a totally different model.
|
||||
OpenCodeSessionDiscoveryTest.writeRecord(discRoot, "ses_sibling", "/work/dir", 9000L,
|
||||
"{\"id\":\"deepseek-v4-flash\",\"providerID\":\"gx\"}");
|
||||
|
||||
assertEquals("ses_ours", handle.agentSessionId(),
|
||||
"the session id, once resolved, must not flip to a sibling sharing the directory");
|
||||
assertTrue(exhausted.isEmpty(),
|
||||
"a sibling's later, unrelated row in the same shared directory must never be read "
|
||||
+ "as OUR session's model: " + exhausted);
|
||||
}
|
||||
|
||||
// --- fleetd #234 defect 2: the quarantine the ERROR announces must actually happen -----------
|
||||
|
||||
/**
|
||||
* fleetd #234: {@link OpenCodeLauncher.SessionAwareHandle#checkModelMatch} fires from {@code
|
||||
* agentSessionId()}, which {@code SessionManager.acquire()} calls to build the very first
|
||||
* {@code MemberSession} record — BEFORE that session is put into the registry {@code
|
||||
* sessions.roster()} reads. A sink that resolves {@code target -> profile} ONLY through the
|
||||
* roster (today's {@code Fleetd.java} code, before this fix) therefore finds nothing at this
|
||||
* exact moment and silently does not quarantine, even though it just logged an ERROR saying it
|
||||
* would. This test drives the REAL path — {@code SessionManager.acquire()} — not the sink
|
||||
* directly, because the bug is entirely about this ordering; a direct-sink test cannot see it
|
||||
* (and is exactly why #175's own test suite, which only ever called the sink directly or after
|
||||
* registration, never caught this).
|
||||
*
|
||||
* <p>The sink under test mirrors {@code Fleetd.main()}'s real wiring after the fix: resolve
|
||||
* via the roster first (unchanged for {@code CompletionResolver}'s two call sites), falling
|
||||
* back to the profile hint {@link OpenCodeLauncher} now supplies via {@link
|
||||
* ExhaustionSink#onExhausted(String, String, String)} when the roster lookup misses.
|
||||
*/
|
||||
@Test
|
||||
void aSpawnTimeModelMismatchActuallyQuarantinesTheCredentialThroughTheRealAcquirePath(
|
||||
@TempDir Path configRoot, @TempDir Path discRoot) throws Exception {
|
||||
FleetConfig.Profile cfg = opencodeCfgWithCredential(
|
||||
"terra", "opencode/nemotron-3-ultra-free", "openai-shared");
|
||||
Map<String, FleetConfig.Profile> profiles = Map.of(cfg.profile(), cfg);
|
||||
BackendQuarantine quarantine = new BackendQuarantine(() -> 0L, TimeUnit.SECONDS.toNanos(1800));
|
||||
|
||||
ExhaustionSink sink = new ExhaustionSink() {
|
||||
@Override
|
||||
public void onExhausted(String target, String reason) {
|
||||
onExhausted(target, reason, null); // no roster resolution modelled here — see below
|
||||
}
|
||||
|
||||
@Override
|
||||
public void onExhausted(String target, String reason, String profileHint) {
|
||||
FleetConfig.Profile profile = profileHint == null ? null : profiles.get(profileHint);
|
||||
if (profile != null) {
|
||||
quarantine.quarantine(profile.effectiveCredentialId());
|
||||
}
|
||||
}
|
||||
};
|
||||
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
OpenCodeLauncher launcher = serviceWithSink(herdr, configRoot, discRoot, cfg, sink);
|
||||
SessionManager sessions = new SessionManager(launcher);
|
||||
|
||||
// The mismatching row exists BEFORE the spawn — reproducing fleetd #234's exact timing:
|
||||
// opencode's session table already carries evidence by the moment acquire() first asks.
|
||||
OpenCodeSessionDiscoveryTest.writeRecord(discRoot, "ses_x", "/work/dir", 1000L,
|
||||
"{\"id\":\"gpt-5.6-sol\",\"providerID\":\"openai\"}");
|
||||
|
||||
assertFalse(quarantine.isQuarantined("openai-shared"), "nothing quarantined before the spawn");
|
||||
|
||||
// The real production entrypoint: acquire() builds the MemberSession by calling
|
||||
// handle.agentSessionId() BEFORE registry.put() runs.
|
||||
MemberSession acquired = sessions.acquire(cfg.profile(), "/work/dir", null, null);
|
||||
|
||||
assertEquals("ses_x", acquired.agentSessionId(), "the id itself still resolves correctly");
|
||||
assertTrue(quarantine.isQuarantined("openai-shared"),
|
||||
"the mismatch fires DURING acquire(), before roster registration, and must still "
|
||||
+ "reach the quarantine via the profile hint — not silently no-op");
|
||||
}
|
||||
|
||||
/**
|
||||
* The other half of the same proof: a sink that resolves {@code target -> profile} ONLY
|
||||
* through the roster (i.e. ignores the profile hint entirely, ~today's pre-fix {@code
|
||||
* Fleetd.java}) drops the SAME spawn-time mismatch silently — the credential is never
|
||||
* quarantined even though {@link OpenCodeLauncher} logged the mismatch ERROR. This is the
|
||||
* failure fleetd #234 reported, reproduced through the real {@code SessionManager.acquire()}
|
||||
* path rather than asserted by inspecting the fix.
|
||||
*/
|
||||
@Test
|
||||
void aRosterOnlySinkSilentlyDropsTheSpawnTimeQuarantine(@TempDir Path configRoot,
|
||||
@TempDir Path discRoot) throws Exception {
|
||||
FleetConfig.Profile cfg = opencodeCfgWithCredential(
|
||||
"terra", "opencode/nemotron-3-ultra-free", "openai-shared");
|
||||
BackendQuarantine quarantine = new BackendQuarantine(() -> 0L, TimeUnit.SECONDS.toNanos(1800));
|
||||
|
||||
// Deliberately ignores the profile hint — the pre-fix shape: only a roster lookup (modelled
|
||||
// here as always empty, since acquire() has not registered the session yet either way).
|
||||
ExhaustionSink rosterOnlySink = (target, reason) -> { };
|
||||
|
||||
FakeHerdr herdr = new FakeHerdr();
|
||||
OpenCodeLauncher launcher = serviceWithSink(herdr, configRoot, discRoot, cfg, rosterOnlySink);
|
||||
SessionManager sessions = new SessionManager(launcher);
|
||||
|
||||
OpenCodeSessionDiscoveryTest.writeRecord(discRoot, "ses_x", "/work/dir", 1000L,
|
||||
"{\"id\":\"gpt-5.6-sol\",\"providerID\":\"openai\"}");
|
||||
|
||||
MemberSession acquired = sessions.acquire(cfg.profile(), "/work/dir", null, null);
|
||||
|
||||
assertEquals("ses_x", acquired.agentSessionId(), "the id itself still resolves correctly");
|
||||
assertFalse(quarantine.isQuarantined("openai-shared"),
|
||||
"a roster-only sink cannot see this target yet — the quarantine silently never "
|
||||
+ "happens, which is exactly fleetd #234 defect 2");
|
||||
}
|
||||
}
|
||||
|
||||
@@ -146,14 +146,14 @@ class OpenCodeSessionDiscoveryTest {
|
||||
"an unreadable database resolves to null, not an exception");
|
||||
}
|
||||
|
||||
// --- fleetd #175: actualModelForDirectory / the model JSON column ---------------------------
|
||||
// --- fleetd #175/#234: actualModelForSessionId / the model JSON column -----------------------
|
||||
|
||||
@Test
|
||||
void parsesTheModelJsonIntoProviderAndId(@TempDir Path root) throws Exception {
|
||||
writeRecord(root, "ses_aaa", "/w/a", 1000L, "{\"id\":\"gpt-5.6-terra\",\"providerID\":\"openai\"}");
|
||||
|
||||
OpenCodeSessionDiscovery.ActualModel actual =
|
||||
new OpenCodeSessionDiscovery(root).actualModelForDirectory("/w/a");
|
||||
new OpenCodeSessionDiscovery(root).actualModelForSessionId("ses_aaa");
|
||||
|
||||
assertNotNull(actual, "a well-formed model JSON parses");
|
||||
assertEquals("gpt-5.6-terra", actual.id());
|
||||
@@ -161,28 +161,39 @@ class OpenCodeSessionDiscoveryTest {
|
||||
}
|
||||
|
||||
@Test
|
||||
void prefersTheModelOfTheMostRecentlyUpdatedRow(@TempDir Path root) throws Exception {
|
||||
writeRecord(root, "ses_old", "/w/a", 1000L, "{\"id\":\"old-model\",\"providerID\":\"openai\"}");
|
||||
writeRecord(root, "ses_new", "/w/a", 5000L, "{\"id\":\"new-model\",\"providerID\":\"openai\"}");
|
||||
void looksUpByIdEvenWhenAnotherRowInTheSameDirectoryIsNewer(@TempDir Path root) throws Exception {
|
||||
writeRecord(root, "ses_ours", "/work/dir", 1000L, "{\"id\":\"gpt-5.6-terra\",\"providerID\":\"openai\"}");
|
||||
writeRecord(root, "ses_sibling", "/work/dir", 9000L, "{\"id\":\"deepseek-v4-flash\",\"providerID\":\"gx\"}");
|
||||
|
||||
assertEquals("new-model",
|
||||
new OpenCodeSessionDiscovery(root).actualModelForDirectory("/w/a").id(),
|
||||
"the model of the row with the highest time_updated wins, same as the id");
|
||||
OpenCodeSessionDiscovery discovery = new OpenCodeSessionDiscovery(root);
|
||||
assertEquals("gpt-5.6-terra", discovery.actualModelForSessionId("ses_ours").id(),
|
||||
"querying by id reads OUR row, not the directory's newest row");
|
||||
assertEquals("deepseek-v4-flash", discovery.actualModelForSessionId("ses_sibling").id(),
|
||||
"each id resolves to its own row independently of time_updated ordering");
|
||||
}
|
||||
|
||||
@Test
|
||||
void aNonMatchingDirectoryYieldsUnknownModelRatherThanAMismatch(@TempDir Path root) throws Exception {
|
||||
void anUnknownSessionIdYieldsUnknownModelRatherThanAMismatch(@TempDir Path root) throws Exception {
|
||||
writeRecord(root, "ses_aaa", "/w/a", 1000L, "{\"id\":\"x\",\"providerID\":\"y\"}");
|
||||
|
||||
assertNull(new OpenCodeSessionDiscovery(root).actualModelForDirectory("/w/other"),
|
||||
"no row for this cwd yet → unknown, not a wrong model");
|
||||
assertNull(new OpenCodeSessionDiscovery(root).actualModelForSessionId("ses_no_such_row"),
|
||||
"no row for this id yet → unknown, not a wrong model");
|
||||
}
|
||||
|
||||
@Test
|
||||
void aBlankOrNullSessionIdYieldsUnknownModel(@TempDir Path root) throws Exception {
|
||||
writeRecord(root, "ses_aaa", "/w/a", 1000L, "{\"id\":\"x\",\"providerID\":\"y\"}");
|
||||
|
||||
OpenCodeSessionDiscovery discovery = new OpenCodeSessionDiscovery(root);
|
||||
assertNull(discovery.actualModelForSessionId(null));
|
||||
assertNull(discovery.actualModelForSessionId(" "));
|
||||
}
|
||||
|
||||
@Test
|
||||
void aNullModelColumnYieldsUnknownWithoutThrowing(@TempDir Path root) throws Exception {
|
||||
writeRecord(root, "ses_aaa", "/w/a", 1000L, null);
|
||||
|
||||
assertNull(new OpenCodeSessionDiscovery(root).actualModelForDirectory("/w/a"),
|
||||
assertNull(new OpenCodeSessionDiscovery(root).actualModelForSessionId("ses_aaa"),
|
||||
"a row with no model value yet is unknown, not a mismatch");
|
||||
}
|
||||
|
||||
@@ -190,7 +201,7 @@ class OpenCodeSessionDiscoveryTest {
|
||||
void unparseableModelJsonYieldsUnknownWithoutThrowing(@TempDir Path root) throws Exception {
|
||||
writeRecord(root, "ses_aaa", "/w/a", 1000L, "this is not json");
|
||||
|
||||
assertNull(new OpenCodeSessionDiscovery(root).actualModelForDirectory("/w/a"),
|
||||
assertNull(new OpenCodeSessionDiscovery(root).actualModelForSessionId("ses_aaa"),
|
||||
"JSON that fails to parse resolves to unknown, never an exception");
|
||||
}
|
||||
|
||||
@@ -198,13 +209,13 @@ class OpenCodeSessionDiscoveryTest {
|
||||
void modelJsonMissingIdYieldsUnknown(@TempDir Path root) throws Exception {
|
||||
writeRecord(root, "ses_aaa", "/w/a", 1000L, "{\"providerID\":\"openai\"}");
|
||||
|
||||
assertNull(new OpenCodeSessionDiscovery(root).actualModelForDirectory("/w/a"),
|
||||
assertNull(new OpenCodeSessionDiscovery(root).actualModelForSessionId("ses_aaa"),
|
||||
"no id in the JSON → unknown, since id is what a caller actually compares");
|
||||
}
|
||||
|
||||
@Test
|
||||
void aMissingDatabaseYieldsUnknownModelWithoutThrowing(@TempDir Path root) {
|
||||
assertNull(new OpenCodeSessionDiscovery(root).actualModelForDirectory("/w/a"));
|
||||
assertNull(new OpenCodeSessionDiscovery(root).actualModelForSessionId("ses_aaa"));
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -212,7 +223,7 @@ class OpenCodeSessionDiscoveryTest {
|
||||
* shape a real opencode upgrade/downgrade could produce. This must degrade to UNKNOWN for the
|
||||
* model, and — the property that actually matters — must NOT take id resolution down with it.
|
||||
* A combined single query for both columns would fail this test; that is why
|
||||
* {@link OpenCodeSessionDiscovery#actualModelForDirectory} runs its own independent query.
|
||||
* {@link OpenCodeSessionDiscovery#actualModelForSessionId} runs its own independent query.
|
||||
*/
|
||||
@Test
|
||||
void aMissingModelColumnYieldsUnknownButIdResolutionStillWorks(@TempDir Path root) throws Exception {
|
||||
@@ -234,7 +245,7 @@ class OpenCodeSessionDiscoveryTest {
|
||||
OpenCodeSessionDiscovery discovery = new OpenCodeSessionDiscovery(root);
|
||||
assertEquals("ses_aaa", discovery.sessionIdForDirectory("/w/a"),
|
||||
"id resolution must survive a database with no model column at all");
|
||||
assertNull(discovery.actualModelForDirectory("/w/a"),
|
||||
assertNull(discovery.actualModelForSessionId("ses_aaa"),
|
||||
"no model column → unknown, not a throw and not a mismatch");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user