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.
This commit is contained in:
Dai Ha
@@ -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.
|
||||
*
|
||||
* <p><b>Atomic and lock-protected.</b> {@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
|
||||
* <strong>same directory</strong> 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.
|
||||
*
|
||||
* <p><b>fleetd #149 incident.</b> 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
|
||||
* <em>which file</em> this can ever target; this closes <em>how</em> 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.
|
||||
*
|
||||
* <p>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.
|
||||
*
|
||||
* <p>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);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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<String> 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.
|
||||
*
|
||||
* <p>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<Exception> failureA = new AtomicReference<>();
|
||||
AtomicReference<Exception> 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<Exception> 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.
|
||||
*
|
||||
* <p>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<String> 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");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user