diff --git a/fleetd/src/main/java/dev/ltms/fleet/member/ClaudeCodeLauncher.java b/fleetd/src/main/java/dev/ltms/fleet/member/ClaudeCodeLauncher.java index 01855ae..4a5ec8a 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/ClaudeCodeLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/ClaudeCodeLauncher.java @@ -19,6 +19,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.StandardCopyOption; import java.nio.file.attribute.PosixFileAttributeView; +import java.util.Arrays; import java.util.EnumSet; import java.util.List; import java.util.Map; @@ -519,6 +520,26 @@ public final class ClaudeCodeLauncher extends HerdrPeerLauncher { * the first. Both exist because of a real incident: see {@link HerdrPeerLauncher#isProvisionedWorktree}'s javadoc * and {@link #writeAtomically}'s javadoc. * + *

Compare-and-swap against a writer the lock cannot reach (fleetd #247). + * {@code TRUST_JSON_LOCK} only serialises calls this launcher itself makes inside this one JVM. + * It does nothing about a writer outside it — and on a host where the profile's + * {@code configDir} is the operator's own {@code CLAUDE_CONFIG_DIR}, the file this method writes + * IS the operator's own live Claude Code session's config file, being read and written by that + * session while it runs. Measured 2026-09-03: its mtime moved minutes after a spawn while that + * session was active. A plain read-modify-write there is a routine lost update, not a rare one: + * fleetd reads v1, the operator's session reads v1 and writes v2 with their own change, fleetd's + * {@code ATOMIC_MOVE} then lands v3 built from v1 — atomic, but v2's change is gone. So before + * the move this method re-reads {@code target}'s exact bytes and compares them with the bytes it + * built its update from; a mismatch means someone else wrote in between, and it discards its + * work and rebuilds from the fresh bytes, up to {@link #MAX_TRUST_JSON_CAS_ATTEMPTS} times. + * Exhausting the retries writes nothing — see the WARN at the end of the loop for why + * that, not a last write-anyway, is the safe failure: the member shows the trust dialog and + * fails to reach an injectable state, which is visible, logged and recoverable; overwriting the + * operator's live config with a stale copy is neither. This narrows the lost-update window, it + * does not close it — a write landing between the final re-read and the {@code ATOMIC_MOVE} + * itself is still lost, because there is no OS-level compare-and-swap on a plain file, only this + * cooperative narrowing of the gap. + * * @param configDir the profile's {@code CLAUDE_CONFIG_DIR} ({@code cfg.configDir()}), or * {@code null}/blank to target the default {@code ~/.claude.json} * @param cwd the spawn's resolved working directory — the exact key Claude Code will look @@ -528,55 +549,118 @@ public final class ClaudeCodeLauncher extends HerdrPeerLauncher { if (!isProvisionedWorktree(cwd)) { return; } - Path target = (configDir == null || configDir.isBlank()) + boolean unsetConfigDir = configDir == null || configDir.isBlank(); + Path target = unsetConfigDir ? Path.of(System.getProperty("user.home"), ".claude.json") : Path.of(configDir, ".claude.json"); + if (unsetConfigDir) { + // fleetd #247: configDir unset is the ONLY path that targets ~/.claude.json — the + // operator's own home file, not a per-profile one — and it is the default, so a + // profile that simply forgot to set configDir gets no signal at all short of the + // operator noticing their own file changing. Say so loudly, every time it is about to + // happen, rather than only once ever: each occurrence is a live write to a real + // person's home config and deserves its own log line. + log.warn("seedTrustDialog: profile has no configDir set, so the workspace-trust seed " + + "for cwd '{}' is about to write the operator's own default '{}' — set " + + "configDir on this profile to target a per-member config file instead", + cwd, target); + } synchronized (TRUST_JSON_LOCK) { try { if (target.getParent() != null) { Files.createDirectories(target.getParent()); } - ObjectNode root = null; - if (Files.isRegularFile(target)) { - JsonNode existing = TRUST_JSON.readTree(target.toFile()); - if (existing instanceof ObjectNode existingObject) { - root = existingObject; + for (int attempt = 1; attempt <= MAX_TRUST_JSON_CAS_ATTEMPTS; attempt++) { + byte[] before = Files.isRegularFile(target) ? Files.readAllBytes(target) : null; + ObjectNode root = parseTrustJsonOrEmpty(before); + JsonNode projectsNode = root.get("projects"); + ObjectNode projects = projectsNode instanceof ObjectNode projectsObject + ? projectsObject : TRUST_JSON.createObjectNode(); + if (!(projectsNode instanceof ObjectNode)) { + root.set("projects", projects); } + JsonNode projectNode = projects.get(cwd); + ObjectNode project = projectNode instanceof ObjectNode projectObject + ? projectObject : TRUST_JSON.createObjectNode(); + if (!(projectNode instanceof ObjectNode)) { + projects.set(cwd, project); + } + // fleetd #247: ONLY hasTrustDialogAccepted. We used to write + // hasCompletedProjectOnboarding beside it; do not put it back. Measured on + // 2026-09-03, minutes after a live spawn seeded this file: 28 of 28 project + // entries carried hasTrustDialogAccepted and 0 of 28 carried the onboarding key + // — including the 27 entries Claude Code wrote for itself. Claude Code + // normalises the whole file when it saves and drops that key every time, so + // writing it achieved nothing except making the next reader think it mattered. + // The member reached idle with the trust flag alone, which is the only outcome + // this seed exists for. If a future Claude Code needs the second flag the + // symptom returns as the trust dialog fleetd #149 describes — re-measure then, + // do not restore it on a guess. + project.put("hasTrustDialogAccepted", true); + String newContent = TRUST_JSON.writerWithDefaultPrettyPrinter().writeValueAsString(root); + + trustJsonCasTestHook.run(); + + // fleetd #247 CAS: re-read immediately before the move and compare with what + // this attempt built its update from. A mismatch means another writer (most + // plausibly the operator's own live Claude Code — see this method's javadoc) + // landed a change in between; discard this attempt's work and rebuild from the + // fresh bytes rather than blindly overwriting it. + byte[] atMove = Files.isRegularFile(target) ? Files.readAllBytes(target) : null; + if (!Arrays.equals(before, atMove)) { + continue; + } + writeAtomically(target, newContent); + return; } - if (root == null) { - root = TRUST_JSON.createObjectNode(); - } - JsonNode projectsNode = root.get("projects"); - ObjectNode projects = projectsNode instanceof ObjectNode projectsObject - ? projectsObject : TRUST_JSON.createObjectNode(); - if (!(projectsNode instanceof ObjectNode)) { - root.set("projects", projects); - } - JsonNode projectNode = projects.get(cwd); - ObjectNode project = projectNode instanceof ObjectNode projectObject - ? projectObject : TRUST_JSON.createObjectNode(); - if (!(projectNode instanceof ObjectNode)) { - projects.set(cwd, project); - } - // fleetd #247: ONLY hasTrustDialogAccepted. We used to write - // hasCompletedProjectOnboarding beside it; do not put it back. Measured on - // 2026-09-03, minutes after a live spawn seeded this file: 28 of 28 project - // entries carried hasTrustDialogAccepted and 0 of 28 carried the onboarding key - // — including the 27 entries Claude Code wrote for itself. Claude Code - // normalises the whole file when it saves and drops that key every time, so - // writing it achieved nothing except making the next reader think it mattered. - // The member reached idle with the trust flag alone, which is the only outcome - // this seed exists for. If a future Claude Code needs the second flag the - // symptom returns as the trust dialog fleetd #149 describes — re-measure then, - // do not restore it on a guess. - project.put("hasTrustDialogAccepted", true); - writeAtomically(target, TRUST_JSON.writerWithDefaultPrettyPrinter().writeValueAsString(root)); + // fleetd #247: deliberately do NOT write here. A member that starts without the + // seed still starts — it may hit the trust dialog fleetd #149 describes and fail to + // reach an injectable state, but that failure is visible (herdr reports it, the + // spawn-readiness gate times out) and recoverable (retry the spawn). Writing our + // stale copy over whatever the other writer left would be silent and, if that other + // writer is the operator's own live session, could destroy real configuration — + // fail toward the recoverable outcome, not the silent one. + log.warn("seedTrustDialog: gave up seeding workspace-trust for cwd '{}' into '{}' " + + "after {} attempts — another writer (most plausibly the operator's own " + + "live Claude Code sharing this file) kept changing it faster than we could " + + "re-read it, so nothing was written; the member may show the trust dialog " + + "instead", cwd, target, MAX_TRUST_JSON_CAS_ATTEMPTS); } catch (Exception e) { log.debug("cannot seed workspace-trust entry for cwd '{}' into '{}'", cwd, target, e); } } } + /** Bound on {@link #seedTrustDialog}'s fleetd #247 compare-and-swap retry loop. */ + private static final int MAX_TRUST_JSON_CAS_ATTEMPTS = 5; + + /** + * Test-only seam for fleetd #247: invoked once per CAS attempt inside {@link #seedTrustDialog}'s + * retry loop, after that attempt has read the target's bytes and built its replacement content, + * but immediately before the final re-read/compare that decides whether to write. A no-op in + * production. Package-visible (not {@code private}) so {@code ClaudeCodeLauncherTest} can install + * a hook here that writes to the target file, deterministically simulating a writer racing + * fleetd's own read-modify-write at the exact instant the CAS is meant to catch — the same race + * an external process (most plausibly the operator's own live Claude Code) creates, without + * depending on real thread scheduling to land the interleaving. A test that sets this MUST + * restore it to the no-op default in a {@code finally} block — it is shared, static state. + */ + static Runnable trustJsonCasTestHook = () -> {}; + + /** + * Parse {@code bytes} as a {@code .claude.json} tree, or hand back a fresh empty object when + * {@code bytes} is {@code null} (no file yet) or does not parse to a JSON object — the same + * missing-or-unreadable-is-empty fallback {@link #seedTrustDialog} always used, factored out so + * the fleetd #247 CAS loop can call it once per attempt. + */ + private static ObjectNode parseTrustJsonOrEmpty(byte[] bytes) throws IOException { + if (bytes == null) { + return TRUST_JSON.createObjectNode(); + } + JsonNode existing = TRUST_JSON.readTree(bytes); + return existing instanceof ObjectNode existingObject ? existingObject : TRUST_JSON.createObjectNode(); + } + /** * Write {@code content} to {@code target} atomically: serialise to a sibling temp file in the * same directory as {@code target} (an atomic move is only guaranteed within diff --git a/fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java b/fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java index f93ae1d..c332f23 100644 --- a/fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java +++ b/fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java @@ -26,6 +26,7 @@ import org.junit.jupiter.api.io.TempDir; import org.slf4j.LoggerFactory; import java.io.IOException; +import java.io.UncheckedIOException; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.attribute.PosixFileAttributeView; @@ -39,6 +40,7 @@ import java.util.UUID; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Function; import java.util.function.Supplier; @@ -2594,4 +2596,150 @@ class ClaudeCodeLauncherTest { + tornRead.get()); assertEquals(newContent, Files.readString(target), "the final content must be the new content"); } + + // --- fleetd #247: CAS against a writer TRUST_JSON_LOCK cannot reach -------------------------- + // + // fleetd #149's lock only serialises seedTrustDialog calls THIS launcher makes inside this one + // JVM. It does nothing about the one writer that actually shares this file on a real host: the + // operator's own live Claude Code, whose CLAUDE_CONFIG_DIR is routinely the very configDir this + // profile is given. A plain read-modify-write there is a routine lost update — fleetd reads v1, + // the operator's session writes v2, fleetd's ATOMIC_MOVE lands v3 built from v1, and v2 is gone, + // atomically. The fix re-reads the file's exact bytes immediately before the move and compares + // them with what the update was built from, retrying from fresh bytes on a mismatch. + // + // Both tests below drive the race through the real seedTrustDialog/spawn() path (not a + // hand-rolled call to some extracted primitive) using trustJsonCasTestHook — a seam fired once + // per CAS attempt, at the exact point between the read and the final compare, so the race is + // deterministic instead of depending on real thread timing. + + @Test + void seedTrustDialogRetriesAndPreservesAConcurrentExternalWritersChange( + @TempDir Path configDir, @TempDir Path worktree) throws Exception { + markAsProvisionedWorktree(worktree); + Path claudeJson = configDir.resolve(".claude.json"); + Files.writeString(claudeJson, "{\"projects\":{}}"); + + // Fires exactly once, on the first CAS attempt — simulating the operator's own live Claude + // Code landing its own write to this SAME file in the gap between fleetd's read and write. + AtomicBoolean fired = new AtomicBoolean(false); + ClaudeCodeLauncher.trustJsonCasTestHook = () -> { + if (fired.compareAndSet(false, true)) { + try { + Files.writeString(claudeJson, + "{\"projects\":{\"/operator/own/project\":" + + "{\"hasTrustDialogAccepted\":true}}}"); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + } + }; + try { + FakeHerdr herdr = new FakeHerdr(); + FleetConfig.Profile cfg = trustProfile(configDir.toString(), worktree.toString()); + new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr), + new SubscriptionGuard(Set.of("gx00.gw")), Map.of(cfg.profile(), cfg), cfg.profile(), _ -> null) + .spawn(); + } finally { + ClaudeCodeLauncher.trustJsonCasTestHook = () -> {}; + } + + assertTrue(fired.get(), "the race hook must actually have fired during the spawn"); + JsonNode root = new ObjectMapper().readTree(claudeJson.toFile()); + assertTrue(root.path("projects").path("/operator/own/project") + .path("hasTrustDialogAccepted").asBoolean(false), + "the external writer's change, landed between fleetd's read and write, must SURVIVE " + + "— this is the whole point of the CAS: without it, fleetd's stale-built " + + "ATOMIC_MOVE would have silently discarded it. Final file: " + + Files.readString(claudeJson)); + assertTrue(root.path("projects").path(worktree.toString()) + .path("hasTrustDialogAccepted").asBoolean(false), + "fleetd's own retry must still land its own trust entry, built from the fresh bytes"); + } + + /** + * Retry-exhaustion path: an external writer that changes the file on EVERY attempt (not just + * once) exhausts all {@code MAX_TRUST_JSON_CAS_ATTEMPTS} retries. fleetd must then write nothing + * at all — not even a partial/best-effort write — and log a WARN naming the file it gave up on. + */ + @Test + void seedTrustDialogWritesNothingAndWarnsWhenCasRetriesAreExhausted( + @TempDir Path configDir, @TempDir Path worktree) throws Exception { + markAsProvisionedWorktree(worktree); + Path claudeJson = configDir.resolve(".claude.json"); + Files.writeString(claudeJson, "{\"marker\":\"start\"}"); + + AtomicInteger hookCalls = new AtomicInteger(); + ClaudeCodeLauncher.trustJsonCasTestHook = () -> { + try { + Files.writeString(claudeJson, + "{\"marker\":\"race-" + hookCalls.incrementAndGet() + "\"}"); + } catch (IOException e) { + throw new UncheckedIOException(e); + } + }; + + Logger logger = (Logger) LoggerFactory.getLogger(ClaudeCodeLauncher.class); + ListAppender appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + try { + FakeHerdr herdr = new FakeHerdr(); + FleetConfig.Profile cfg = trustProfile(configDir.toString(), worktree.toString()); + new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr), + new SubscriptionGuard(Set.of("gx00.gw")), Map.of(cfg.profile(), cfg), cfg.profile(), _ -> null) + .spawn(); + } finally { + logger.detachAppender(appender); + ClaudeCodeLauncher.trustJsonCasTestHook = () -> {}; + } + + assertEquals(5, hookCalls.get(), "the race hook must fire exactly once per CAS attempt"); + String finalContent = Files.readString(claudeJson); + assertEquals("{\"marker\":\"race-" + hookCalls.get() + "\"}", finalContent, + "the file must be left exactly as the external writer last left it — fleetd must not " + + "have written at all once retries are exhausted"); + assertFalse(finalContent.contains("hasTrustDialogAccepted"), + "the trust entry must never appear — its presence would mean the CAS gave up and " + + "wrote anyway instead of skipping the seed"); + + assertTrue(appender.list.stream().anyMatch(e -> e.getLevel() == Level.WARN + && e.getFormattedMessage().contains("gave up seeding workspace-trust") + && e.getFormattedMessage().contains(worktree.toString()) + && e.getFormattedMessage().contains(claudeJson.toString())), + "a WARN naming both the cwd and the file it gave up on must be logged: " + + appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList()); + } + + /** + * The loud-default WARN (fleetd #247): a profile with no {@code configDir} targets the + * operator's real {@code ~/.claude.json} (redirected here to a {@code @TempDir}), and every time + * that happens must be logged, not just detected. + */ + @Test + void seedTrustDialogWarnsEveryTimeItTargetsTheDefaultClaudeJson( + @TempDir Path fakeHome, @TempDir Path worktree) throws Exception { + markAsProvisionedWorktree(worktree); + String originalHome = System.getProperty("user.home"); + System.setProperty("user.home", fakeHome.toString()); + Logger logger = (Logger) LoggerFactory.getLogger(ClaudeCodeLauncher.class); + ListAppender appender = new ListAppender<>(); + appender.start(); + logger.addAppender(appender); + try { + FakeHerdr herdr = new FakeHerdr(); + FleetConfig.Profile cfg = trustProfile(null, worktree.toString()); + new ClaudeCodeLauncher(new AgentControl(herdr), new WorkspaceControl(herdr), + new SubscriptionGuard(Set.of("gx00.gw")), Map.of(cfg.profile(), cfg), cfg.profile(), _ -> null) + .spawn(); + } finally { + logger.detachAppender(appender); + System.setProperty("user.home", originalHome); + } + + assertTrue(appender.list.stream().anyMatch(e -> e.getLevel() == Level.WARN + && e.getFormattedMessage().contains("no configDir set") + && e.getFormattedMessage().contains(worktree.toString())), + "a WARN naming the cwd must fire when the profile sets no configDir: " + + appender.list.stream().map(ILoggingEvent::getFormattedMessage).toList()); + } }