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 5c6a562..652b28d 100644 --- a/fleetd/src/main/java/dev/ltms/fleet/member/ClaudeCodeLauncher.java +++ b/fleetd/src/main/java/dev/ltms/fleet/member/ClaudeCodeLauncher.java @@ -17,6 +17,8 @@ import java.io.IOException; import java.io.UncheckedIOException; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.StandardCopyOption; +import java.nio.file.attribute.PosixFileAttributeView; import java.util.EnumSet; import java.util.List; import java.util.Map; @@ -53,6 +55,18 @@ public final class ClaudeCodeLauncher extends HerdrPeerLauncher { /** JSON codec for the additive workspace-trust seed (fleetd #149) — Jackson's default settings. */ private static final ObjectMapper TRUST_JSON = new ObjectMapper(); + /** + * Serialises every {@link #seedTrustDialog} read-modify-write for the whole daemon process. + * Two claude-code spawns starting at once are normal (fleetd runs several members in parallel + * routinely) and both would otherwise read the same {@code .claude.json}, add their own entry + * to their own in-memory copy, and write — the second write wins and the first spawn's trust + * entry silently disappears. A single process-wide lock is enough because every spawn on this + * daemon runs in this one JVM; it does not protect against a second daemon process or the + * operator's own Claude Code process writing at the same instant, which {@link #writeAtomically} + * covers instead (each writer only ever sees a fully-old or fully-new file, never a torn one). + */ + private static final Object TRUST_JSON_LOCK = new Object(); + private final SubscriptionGuard guard; /** @@ -496,6 +510,15 @@ public final class ClaudeCodeLauncher extends HerdrPeerLauncher { * never run against a real checkout or an un-configured fallback cwd, only the exact * always-fresh-directory population fleetd #149 describes. * + *
Atomic and lock-protected. {@code .claude.json} is a live file — Claude Code itself + * rewrites it while running, and this daemon routinely spawns several members at once, each + * calling this method for its own cwd. Every write goes through {@link #writeAtomically} (a + * sibling-temp-file + {@code ATOMIC_MOVE}, never a truncate-in-place) so a crash mid-write or a + * concurrent reader never observes a half-written file, and through {@link #TRUST_JSON_LOCK} so + * two concurrent spawns' entries both survive instead of the second write silently discarding + * the first. Both exist because of a real incident: see {@link #isProvisionedWorktree}'s javadoc + * and {@link #writeAtomically}'s javadoc. + * * @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 @@ -508,37 +531,100 @@ public final class ClaudeCodeLauncher extends HerdrPeerLauncher { Path target = (configDir == null || configDir.isBlank()) ? Path.of(System.getProperty("user.home"), ".claude.json") : Path.of(configDir, ".claude.json"); - try { - ObjectNode root = null; - if (Files.isRegularFile(target)) { - JsonNode existing = TRUST_JSON.readTree(target.toFile()); - if (existing instanceof ObjectNode existingObject) { - root = existingObject; + 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; + } + } + 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); + } + project.put("hasTrustDialogAccepted", true); + project.put("hasCompletedProjectOnboarding", true); + writeAtomically(target, TRUST_JSON.writerWithDefaultPrettyPrinter().writeValueAsString(root)); + } catch (Exception e) { + log.debug("cannot seed workspace-trust entry for cwd '{}' into '{}'", cwd, target, e); } - if (root == null) { - root = 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 + * one filesystem — a different directory could mean a different filesystem), then + * {@link StandardCopyOption#ATOMIC_MOVE} it into place. A reader — Claude Code itself, or + * another {@code seedTrustDialog} call — only ever observes the fully-old file or the + * fully-new one, never a truncated or half-written one. + * + *
fleetd #149 incident. The original implementation used + * {@code Files.writeString(target, content)} directly, which truncates {@code target} in place + * before writing the replacement bytes. Combined with an ungated {@code cwd} (see + * {@link #isProvisionedWorktree}'s javadoc), a mutation-testing run hit that truncation window + * against the operator's real {@code ~/.claude.json} and left it at 178 bytes. The gate closes + * which file this can ever target; this closes how the target is written, so + * that even a legitimate write against a real, live, concurrently-read {@code .claude.json} + * cannot leave it observably empty or partial. + * + *
Preserves {@code target}'s existing POSIX permissions (Claude Code ships {@code + * .claude.json} as {@code 0600}) when the filesystem reports them; a freshly created temp file + * already defaults to owner-only permissions on a POSIX filesystem, so a first-ever write (no + * existing {@code target}) is no less private without this. On a non-POSIX filesystem (e.g. + * Windows) the permission copy is a silent no-op rather than a failure. + * + *
Package-visible (not {@code private}) so a test can drive it directly with a concurrent
+ * reader thread and prove the torn-file property this method exists for — the LOCK in
+ * {@link #seedTrustDialog} already fully serialises every call this launcher itself makes, so a
+ * test that only ever goes through {@code seedTrustDialog}/{@code spawn()} could never observe
+ * a torn file regardless of whether this method is atomic; it would be proving the lock, not
+ * this method. Atomicity's actual job is protecting against a writer the lock cannot reach at
+ * all — a second daemon process, or the operator's own live Claude Code — so the test for it
+ * has to reach this method on its own.
+ */
+ static void writeAtomically(Path target, String content) throws IOException {
+ Path parent = target.getParent();
+ Path tmp = Files.createTempFile(parent, target.getFileName() + ".", ".tmp");
+ try {
+ Files.writeString(tmp, content);
+ copyPosixPermissionsIfPresent(target, tmp);
+ Files.move(tmp, target, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING);
+ } catch (IOException e) {
+ Files.deleteIfExists(tmp);
+ throw e;
+ }
+ }
+
+ /** Copy {@code target}'s POSIX permissions onto {@code tmp}, or no-op where either is unsupported. */
+ private static void copyPosixPermissionsIfPresent(Path target, Path tmp) {
+ try {
+ if (!Files.isRegularFile(target)) {
+ return; // nothing to inherit from — first-ever write, temp file's own default stands
}
- JsonNode projectsNode = root.get("projects");
- ObjectNode projects = projectsNode instanceof ObjectNode projectsObject
- ? projectsObject : TRUST_JSON.createObjectNode();
- if (!(projectsNode instanceof ObjectNode)) {
- root.set("projects", projects);
+ PosixFileAttributeView view = Files.getFileAttributeView(target, PosixFileAttributeView.class);
+ if (view == null) {
+ return; // non-POSIX filesystem — nothing this JVM can read/set here
}
- JsonNode projectNode = projects.get(cwd);
- ObjectNode project = projectNode instanceof ObjectNode projectObject
- ? projectObject : TRUST_JSON.createObjectNode();
- if (!(projectNode instanceof ObjectNode)) {
- projects.set(cwd, project);
- }
- project.put("hasTrustDialogAccepted", true);
- project.put("hasCompletedProjectOnboarding", true);
- if (target.getParent() != null) {
- Files.createDirectories(target.getParent());
- }
- Files.writeString(target, TRUST_JSON.writerWithDefaultPrettyPrinter().writeValueAsString(root));
- } catch (Exception e) {
- log.debug("cannot seed workspace-trust entry for cwd '{}' into '{}'", cwd, target, e);
+ Files.setPosixFilePermissions(tmp, Files.getPosixFilePermissions(target));
+ } catch (IOException e) {
+ log.debug("cannot preserve permissions of '{}' onto its replacement", target, e);
}
}
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 8361948..ea5f991 100644
--- a/fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java
+++ b/fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java
@@ -6,6 +6,7 @@ import ch.qos.logback.classic.spi.ILoggingEvent;
import ch.qos.logback.core.read.ListAppender;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.node.ObjectNode;
import dev.ltms.fleet.config.FleetConfig;
import dev.ltms.fleet.guard.GuardException;
import dev.ltms.fleet.guard.SubscriptionGuard;
@@ -25,11 +26,16 @@ import org.slf4j.LoggerFactory;
import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
+import java.nio.file.attribute.PosixFileAttributeView;
import java.nio.file.attribute.PosixFilePermissions;
+import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
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.AtomicReference;
import java.util.function.Function;
import java.util.function.Supplier;
@@ -2313,4 +2319,220 @@ class ClaudeCodeLauncherTest {
System.setProperty("user.home", originalHome);
}
}
+
+ // --- fleetd #149 review round 2: the write must be atomic and lock-protected -----------------
+ //
+ // The first round fixed WHICH cwd this can ever target. This round fixes HOW the target is
+ // written: .claude.json is large (tens of KB, dozens of projects on a real host), Claude Code
+ // itself rewrites it while running, and this daemon spawns several members in parallel as a
+ // matter of routine. The pre-fix Files.writeString(target, content) truncates target in place
+ // before writing the replacement bytes — a crash mid-write, or another writer's read landing in
+ // that window, loses data. Two more failure modes follow directly: (a) a crash/kill mid-write
+ // leaves target truncated, and (b) two concurrent spawns racing a naive read-modify-write let
+ // the second writer's write silently discard the first spawn's entry. The fix is
+ // ClaudeCodeLauncher.writeAtomically (sibling temp file + ATOMIC_MOVE, package-visible for the
+ // test below that proves it directly) plus TRUST_JSON_LOCK (a process-wide lock serialising
+ // every seedTrustDialog call this launcher itself makes).
+
+ /**
+ * Requested test 1: an existing, large-ish (not just a two-key fixture) .claude.json must never
+ * collapse. Asserts on the restored KEY SET — not merely that the result still parses as JSON,
+ * which the incident's 178-byte file also did.
+ */
+ @Test
+ void seedTrustDialogPreservesALargeExistingFileWithoutCollapsing(
+ @TempDir Path configDir, @TempDir Path worktree) throws Exception {
+ markAsProvisionedWorktree(worktree);
+ Path claudeJson = configDir.resolve(".claude.json");
+
+ ObjectMapper mapper = new ObjectMapper();
+ ObjectNode root = mapper.createObjectNode();
+ root.put("numStartups", 4200);
+ root.put("firstStartTime", "2025-01-01T00:00:00.000Z");
+ root.putObject("oauthAccount").put("emailAddress", "operator@example.com");
+ Set This is a test of {@code TRUST_JSON_LOCK}, not of {@code writeAtomically}: the lock fully
+ * serialises every {@code seedTrustDialog} call this launcher itself makes, so this test would
+ * pass even without atomicity. See {@link #writeAtomicallyNeverExposesATornFileToAConcurrentReader}
+ * for the test that exercises atomicity specifically.
+ */
+ @Test
+ void concurrentSeedsForDifferentCwdsBothSurvive(
+ @TempDir Path configDir, @TempDir Path worktreeA, @TempDir Path worktreeB) throws Exception {
+ markAsProvisionedWorktree(worktreeA);
+ markAsProvisionedWorktree(worktreeB);
+
+ CountDownLatch ready = new CountDownLatch(2);
+ CountDownLatch go = new CountDownLatch(1);
+ AtomicReference The new content is made large (tens of MB) so a naive truncate-then-write has a real,
+ * non-instantaneous window for the busy-poll reader thread to land in — this is inherently a
+ * race, not a guaranteed-deterministic assertion, but it uses no {@code Thread.sleep} and
+ * reliably reproduced the torn read when run against the pre-fix
+ * {@code Files.writeString(target, content)} implementation (see the PR's mutation table).
+ */
+ @Test
+ void writeAtomicallyNeverExposesATornFileToAConcurrentReader(@TempDir Path dir) throws Exception {
+ Path target = dir.resolve(".claude.json");
+ String oldContent = "{\"marker\":\"OLD\"}";
+ Files.writeString(target, oldContent);
+ String newContent = "{\"marker\":\"NEW\",\"pad\":\"" + "x".repeat(20_000_000) + "\"}";
+
+ AtomicReference