From 743377d6cd9a5448fe794e912f11d25cc20ba40d Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 3 Sep 2026 11:38:10 +0700 Subject: [PATCH] fleetd #149 review round 2: make the trust-dialog seed atomic and lock-protected Files.writeString truncates the target in place before writing, so there was a window where .claude.json could be observed empty or half-written - exactly the shape of the incident this ticket already hit once, but reachable in production too: a crash/kill mid-write, or two concurrent claude-code spawns (normal here - several run in parallel routinely) racing a naive read-modify-write and silently discarding one spawn's entry. Two independent fixes, each with its own dedicated test proving it (not the other): - ClaudeCodeLauncher.writeAtomically: serialise to a sibling temp file in the same directory, then Files.move with ATOMIC_MOVE + REPLACE_EXISTING, preserving the target's existing POSIX permissions (.claude.json ships 0600). A reader now only ever observes the fully-old or fully-new file, never a torn one. Package-visible so a test can drive it directly. - TRUST_JSON_LOCK: a process-wide lock around seedTrustDialog's whole read-modify-write, so two concurrent spawns for different cwds both keep their entry instead of the second write discarding the first. Sufficient because every spawn on this daemon runs in one JVM; it does NOT protect against a second daemon process or the operator's own live Claude Code writing at the same instant - writeAtomically covers that case instead. Both fail soft, same as before: any I/O failure here must never block a spawn. Four new tests: a large (30-project) existing file survives without collapsing (asserted on the restored key set, not just that the result parses); two concurrent spawns for different cwds both keep their entry (CountDownLatch-synchronised, not a sleep); existing 0600 permissions survive the write; and a direct test of writeAtomically with a busy-poll reader thread proving a concurrent reader never observes a torn file. See PR body for the full mutation-testing table, including an honest note on which of these tests the atomicity mutation actually caught (not the one implied by the numbering in review) and why. --- .../ltms/fleet/member/ClaudeCodeLauncher.java | 140 ++++++++--- .../fleet/member/ClaudeCodeLauncherTest.java | 222 ++++++++++++++++++ 2 files changed, 335 insertions(+), 27 deletions(-) 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 otherProjectPaths = new HashSet<>(); + ObjectNode projects = root.putObject("projects"); + for (int i = 0; i < 30; i++) { + String path = "/Users/operator/code/project-" + i; + otherProjectPaths.add(path); + ObjectNode project = projects.putObject(path); + project.put("hasTrustDialogAccepted", true); + project.putObject("mcpServers").putObject("server-" + i).put("command", "server-" + i + "-mcp"); + } + String before = mapper.writerWithDefaultPrettyPrinter().writeValueAsString(root); + Files.writeString(claudeJson, before); + long sizeBefore = Files.size(claudeJson); + assertTrue(sizeBefore > 4096, + "fixture must actually be large-ish to be a meaningful proof: " + sizeBefore + " bytes"); + + 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(); + + long sizeAfter = Files.size(claudeJson); + assertTrue(sizeAfter >= sizeBefore, + "the file must never collapse below its pre-seed size — before=" + sizeBefore + + " after=" + sizeAfter + " bytes (the incident shrank ~72 KB to 178 bytes)"); + + JsonNode after = mapper.readTree(claudeJson.toFile()); + assertEquals(4200, after.path("numStartups").asInt(), "unrelated top-level key survives"); + assertEquals("operator@example.com", after.path("oauthAccount").path("emailAddress").asText()); + JsonNode afterProjects = after.path("projects"); + for (String path : otherProjectPaths) { + assertTrue(afterProjects.path(path).path("hasTrustDialogAccepted").asBoolean(false), + "pre-existing project entry " + path + " must survive"); + } + assertEquals(otherProjectPaths.size() + 1, afterProjects.size(), + "exactly one NEW project entry (this worktree's) must be added, none dropped"); + assertTrue(afterProjects.path(worktree.toString()).path("hasTrustDialogAccepted").asBoolean(false)); + } + + /** + * Requested test 2: two concurrent spawns for DIFFERENT cwd values sharing one configDir must + * both end up present in the final file — a naive concurrent read-modify-write would let the + * second writer's read (taken before the first writer's write lands) silently discard the + * first. A {@link CountDownLatch} — not a sleep — lines both threads up at the starting line so + * this does not depend on scheduling luck to be meaningful. + * + *

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 failureA = new AtomicReference<>(); + AtomicReference failureB = new AtomicReference<>(); + + Thread ta = new Thread(spawnTask(configDir, worktreeA, ready, go, failureA), "spawn-a"); + Thread tb = new Thread(spawnTask(configDir, worktreeB, ready, go, failureB), "spawn-b"); + ta.start(); + tb.start(); + + assertTrue(ready.await(5, TimeUnit.SECONDS), "both threads must reach the starting line"); + go.countDown(); + ta.join(5000); + tb.join(5000); + assertFalse(ta.isAlive(), "spawn A must finish within the timeout"); + assertFalse(tb.isAlive(), "spawn B must finish within the timeout"); + assertNull(failureA.get(), "spawn A must not throw: " + failureA.get()); + assertNull(failureB.get(), "spawn B must not throw: " + failureB.get()); + + JsonNode root = new ObjectMapper().readTree(configDir.resolve(".claude.json").toFile()); + assertTrue(root.path("projects").path(worktreeA.toString()) + .path("hasTrustDialogAccepted").asBoolean(false), + "worktree A's entry must survive the race"); + assertTrue(root.path("projects").path(worktreeB.toString()) + .path("hasTrustDialogAccepted").asBoolean(false), + "worktree B's entry must survive the race"); + } + + private Runnable spawnTask(Path configDir, Path worktree, CountDownLatch ready, CountDownLatch go, + AtomicReference failure) { + return () -> { + try { + FakeHerdr herdr = new FakeHerdr(); + FleetConfig.Profile cfg = trustProfile(configDir.toString(), worktree.toString()); + ClaudeCodeLauncher launcher = new ClaudeCodeLauncher(new AgentControl(herdr), + new WorkspaceControl(herdr), new SubscriptionGuard(Set.of("gx00.gw")), + Map.of(cfg.profile(), cfg), cfg.profile(), _ -> null); + ready.countDown(); + go.await(); + launcher.spawn(); + } catch (Exception e) { + failure.set(e); + } + }; + } + + /** + * Requested test 3: the atomic write must preserve {@code .claude.json}'s existing {@code 0600} + * permissions, not silently widen them via a fresh temp file's own defaults landing on top of a + * file that had different (e.g. group-readable) permissions. Skips rather than fails on a + * filesystem with no POSIX permissions (e.g. Windows) — the same {@code assumeTrue} pattern this + * file already uses for {@code memberHerdrSocketWithWorktreeRootAndGroupPutsCharterUnderWorktreeRootAndSharesIt}. + */ + @Test + void seedTrustDialogPreservesExisting0600Permissions( + @TempDir Path configDir, @TempDir Path worktree) throws Exception { + markAsProvisionedWorktree(worktree); + Path claudeJson = configDir.resolve(".claude.json"); + Files.writeString(claudeJson, "{}"); + assumeTrue(Files.getFileAttributeView(claudeJson, PosixFileAttributeView.class) != null, + "no POSIX permissions on this filesystem — skipping rather than failing"); + Files.setPosixFilePermissions(claudeJson, PosixFilePermissions.fromString("rw-------")); + + 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(); + + assertEquals("rw-------", PosixFilePermissions.toString(Files.getPosixFilePermissions(claudeJson)), + "the atomic write must preserve .claude.json's existing 0600 permissions, not widen " + + "them via a fresh temp file's own defaults"); + } + + /** + * Direct proof that {@link ClaudeCodeLauncher#writeAtomically} — not {@code TRUST_JSON_LOCK} — + * is what keeps a concurrent reader of {@code target} from ever observing a truncated or + * partially-written file. Calls {@code writeAtomically} directly (package-visible for exactly + * this test) rather than going through {@code seedTrustDialog}/{@code spawn()}, because {@link + * #concurrentSeedsForDifferentCwdsBothSurvive} above is protected by the lock and would pass + * even without atomicity — it proves the lock, not the atomic move. This test proves the atomic + * move specifically: a reader racing a writer OUTSIDE that lock (a second daemon process, or the + * operator's own live Claude Code — exactly what the lock cannot reach) must still never see a + * torn file. + * + *

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 tornRead = new AtomicReference<>(); + AtomicBoolean stop = new AtomicBoolean(false); + Thread reader = new Thread(() -> { + while (!stop.get()) { + try { + String seen = Files.readString(target); + if (!seen.equals(oldContent) && !seen.equals(newContent)) { + tornRead.compareAndSet(null, "torn read of length " + seen.length() + ": " + + seen.substring(0, Math.min(seen.length(), 80))); + stop.set(true); + } + } catch (IOException ignored) { + // ATOMIC_MOVE guarantees the path always resolves to old-or-new content once + // readable at all — a transient "briefly missing during the rename" is fine to + // ignore and keep sampling. + } + } + }); + reader.start(); + try { + ClaudeCodeLauncher.writeAtomically(target, newContent); + } finally { + stop.set(true); + reader.join(5000); + } + + assertNull(tornRead.get(), "a concurrent reader must never observe a partially-written file: " + + tornRead.get()); + assertEquals(newContent, Files.readString(target), "the final content must be the new content"); + } }