This commit is contained in:
Dai Ha
@@ -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.
|
||||
*
|
||||
* <p><b>Compare-and-swap against a writer the lock cannot reach (fleetd #247).</b>
|
||||
* {@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.
|
||||
* <b>Exhausting the retries writes nothing</b> — 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
|
||||
* <strong>same directory</strong> as {@code target} (an atomic move is only guaranteed within
|
||||
|
||||
@@ -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<ILoggingEvent> 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<ILoggingEvent> 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());
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user