fleetd #247: CAS seedTrustDialog against the operator's own live Claude Code #261

Closed
agent wants to merge 0 commits from worker/fleetd-247-342356-5 into main
Member

fleetd #247 — CAS seedTrustDialog against the operator's own live Claude Code

The bug (confirmed facts from the brief, not re-derived)

TRUST_JSON_LOCK (fleetd #149) only serialises seedTrustDialog calls made inside this one JVM.
It cannot reach a writer outside it — and on this host that writer is the operator's own live
Claude Code
: configDir is routinely set to the operator's own CLAUDE_CONFIG_DIR, so the file
fleetd writes on every claude-code spawn is the config of the very session doing the spawning. A
plain read-modify-write there is a routine lost update on every spawn, not a rare collision:
fleetd reads v1, the operator's session writes v2, fleetd's ATOMIC_MOVE lands v3 built from v1 —
atomic, but v2 is gone.

What changed

fleetd/src/main/java/dev/ltms/fleet/member/ClaudeCodeLauncher.java:

  • seedTrustDialog now runs a bounded (5 attempts, MAX_TRUST_JSON_CAS_ATTEMPTS)
    compare-and-swap loop inside TRUST_JSON_LOCK: read the target's bytes, build the update, then
    immediately before the ATOMIC_MOVE re-read the target's bytes and compare with what the
    update was built from. A mismatch discards the attempt and rebuilds from the fresh bytes.
  • Exhausting the retries writes nothing. A WARN is logged naming the cwd and the file it gave
    up on. This is deliberate (commented in the code): a member that starts unseeded shows the trust
    dialog and fails to reach an injectable state — visible, logged, recoverable. Writing a stale
    copy over the operator's live config would be silent and not recoverable. Fail toward the
    recoverable outcome.
  • A new WARN fires every time the seed is about to target the default ~/.claude.json
    (configDir null/blank) — the only path that hits the operator's home file, and the default.
  • Added trustJsonCasTestHook (package-visible, no-op in production): a seam fired once per CAS
    attempt, between the read and the final compare, so tests can deterministically simulate a
    racing external writer instead of depending on real thread timing (mirrors why writeAtomically
    is package-visible rather than private — see its javadoc).
  • The call-site javadoc states plainly: the window is narrowed, not closed — a write landing
    between the final re-read and the ATOMIC_MOVE itself is still lost, because there is no
    OS-level CAS on a plain file — and that the file at risk is the lead's own config file
    (<configDir>/.claude.json), not some unrelated process's, so this is a routine spawn-time race,
    not a rare one.

fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java — 3 new tests, all going
through the real seedTrustDialog/spawn() path via trustJsonCasTestHook:

  • seedTrustDialogRetriesAndPreservesAConcurrentExternalWritersChange — the acceptance-criterion
    test: an external writer changes the file once, between fleetd's read and write; asserts the
    external writer's change survives in the final file, alongside fleetd's own retried entry.
  • seedTrustDialogWritesNothingAndWarnsWhenCasRetriesAreExhausted — an external writer changes the
    file on every attempt; asserts fleetd writes nothing (file left exactly as the external
    writer last left it, no hasTrustDialogAccepted for our cwd) and a WARN naming the cwd/file is
    logged.
  • seedTrustDialogWarnsEveryTimeItTargetsTheDefaultClaudeJson — asserts the loud-default WARN
    fires when configDir is unset.

All new fixtures use @TempDir for both configDir/worktree and (where relevant) a redirected
user.home, per the project's .claude.json-safety rule; noFixtureSeededTheDefaultClaudeJson
(the existing guard from fleetd #258) still passes — untouched and not weakened.

Proof the CAS is a real guard (per the brief's required recipe)

  1. Got the new tests passing (see build output below).
  2. Temporarily removed the CAS check in seedTrustDialog (changed
    if (!Arrays.equals(before, atMove)) to if (false && !Arrays.equals(before, atMove)) — i.e.
    "just do the move as before"). Re-ran ClaudeCodeLauncherTest. Both new race tests failed,
    verbatim:
[ERROR] dev.ltms.fleet.member.ClaudeCodeLauncherTest.seedTrustDialogRetriesAndPreservesAConcurrentExternalWritersChange(Path, Path) -- Time elapsed: 0.004 s <<< FAILURE!
org.opentest4j.AssertionFailedError:
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: {
  "projects" : {
    "/var/folders/wf/ljcnfwxj2pd1nxtd63tv2df40000gq/T/junit-17400176206649590785" : {
      "hasTrustDialogAccepted" : true
    }
  }
} ==> expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
	at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:214)
	at dev.ltms.fleet.member.ClaudeCodeLauncherTest.seedTrustDialogRetriesAndPreservesAConcurrentExternalWritersChange(ClaudeCodeLauncherTest.java:2648)

[ERROR]   ClaudeCodeLauncherTest.seedTrustDialogWritesNothingAndWarnsWhenCasRetriesAreExhausted:2696 the race hook must fire exactly once per CAS attempt ==> expected: <5> but was: <1>

[ERROR] Tests run: 105, Failures: 2, Errors: 0, Skipped: 0
[INFO] BUILD FAILURE
  1. Restored ClaudeCodeLauncher.java from a cp copy taken before the experimental edit; confirmed
    with a plain diff against that backup that the restore was byte-for-byte exact (no leftover
    experiment changes made it into the commit).
  2. Re-ran: green — see build output below.

Build (all commands run unpiped, full output read)

  • mvn compile (after the CAS change) — BUILD SUCCESS.
  • mvn test-compile (after adding tests) — BUILD SUCCESS.
  • mvn test -Dtest=ClaudeCodeLauncherTest (before the removal experiment) —
    Tests run: 105, Failures: 0, Errors: 0, Skipped: 0 / BUILD SUCCESS.
  • mvn test -Dtest=ClaudeCodeLauncherTest (with the CAS check removed) —
    Tests run: 105, Failures: 2, Errors: 0, Skipped: 0 / BUILD FAILURE (verbatim failure text
    above).
  • After restoring: mvn test -Dtest=ClaudeCodeLauncherTest —
    Tests run: 105, Failures: 0, Errors: 0, Skipped: 0 / BUILD SUCCESS.
  • Full project build: mvn clean install —
    Tests run: 1260, Failures: 0, Errors: 0, Skipped: 0 / BUILD SUCCESS. (main was 1257 green;
    this PR adds exactly 3 tests.)

Safety

Every new fixture uses @TempDir for configDir/worktree, and the one default-path test
redirects user.home to a @TempDir in a try/finally. No test wrote to any real .claude.json.
The existing noFixtureSeededTheDefaultClaudeJson guard (fleetd #258) ran and passed, unmodified.

Scope note

Out of scope, not investigated: nothing else in ClaudeCodeLauncher.java was touched beyond
seedTrustDialog's javadoc/body and the two new supporting members (MAX_TRUST_JSON_CAS_ATTEMPTS,
parseTrustJsonOrEmpty, trustJsonCasTestHook).

Branch: worker/fleetd-247-342356-5
Worktree root: /Users/dai.ha/LTMS/.bridged-worktrees/63b767-5
Files changed: fleetd/src/main/java/dev/ltms/fleet/member/ClaudeCodeLauncher.java,
fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java

## fleetd #247 — CAS seedTrustDialog against the operator's own live Claude Code ### The bug (confirmed facts from the brief, not re-derived) `TRUST_JSON_LOCK` (fleetd #149) only serialises `seedTrustDialog` calls made inside this one JVM. It cannot reach a writer outside it — and on this host that writer is the **operator's own live Claude Code**: `configDir` is routinely set to the operator's own `CLAUDE_CONFIG_DIR`, so the file fleetd writes on every claude-code spawn is the config of the very session doing the spawning. A plain read-modify-write there is a routine lost update on every spawn, not a rare collision: fleetd reads v1, the operator's session writes v2, fleetd's `ATOMIC_MOVE` lands v3 built from v1 — atomic, but v2 is gone. ### What changed `fleetd/src/main/java/dev/ltms/fleet/member/ClaudeCodeLauncher.java`: - `seedTrustDialog` now runs a bounded (5 attempts, `MAX_TRUST_JSON_CAS_ATTEMPTS`) compare-and-swap loop inside `TRUST_JSON_LOCK`: read the target's bytes, build the update, then **immediately before the `ATOMIC_MOVE`** re-read the target's bytes and compare with what the update was built from. A mismatch discards the attempt and rebuilds from the fresh bytes. - **Exhausting the retries writes nothing.** A WARN is logged naming the cwd and the file it gave up on. This is deliberate (commented in the code): a member that starts unseeded shows the trust dialog and fails to reach an injectable state — visible, logged, recoverable. Writing a stale copy over the operator's live config would be silent and not recoverable. Fail toward the recoverable outcome. - A new WARN fires every time the seed is about to target the **default `~/.claude.json`** (`configDir` null/blank) — the only path that hits the operator's home file, and the default. - Added `trustJsonCasTestHook` (package-visible, no-op in production): a seam fired once per CAS attempt, between the read and the final compare, so tests can deterministically simulate a racing external writer instead of depending on real thread timing (mirrors why `writeAtomically` is package-visible rather than private — see its javadoc). - The call-site javadoc states plainly: **the window is narrowed, not closed** — a write landing between the final re-read and the `ATOMIC_MOVE` itself is still lost, because there is no OS-level CAS on a plain file — and that **the file at risk is the lead's own config file** (`<configDir>/.claude.json`), not some unrelated process's, so this is a routine spawn-time race, not a rare one. `fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java` — 3 new tests, all going through the real `seedTrustDialog`/`spawn()` path via `trustJsonCasTestHook`: - `seedTrustDialogRetriesAndPreservesAConcurrentExternalWritersChange` — the acceptance-criterion test: an external writer changes the file once, between fleetd's read and write; asserts the external writer's change **survives** in the final file, alongside fleetd's own retried entry. - `seedTrustDialogWritesNothingAndWarnsWhenCasRetriesAreExhausted` — an external writer changes the file on every attempt; asserts fleetd writes **nothing** (file left exactly as the external writer last left it, no `hasTrustDialogAccepted` for our cwd) and a WARN naming the cwd/file is logged. - `seedTrustDialogWarnsEveryTimeItTargetsTheDefaultClaudeJson` — asserts the loud-default WARN fires when `configDir` is unset. All new fixtures use `@TempDir` for both `configDir`/`worktree` and (where relevant) a redirected `user.home`, per the project's `.claude.json`-safety rule; `noFixtureSeededTheDefaultClaudeJson` (the existing guard from fleetd #258) still passes — untouched and not weakened. ### Proof the CAS is a real guard (per the brief's required recipe) 1. Got the new tests passing (see build output below). 2. Temporarily removed the CAS check in `seedTrustDialog` (changed `if (!Arrays.equals(before, atMove))` to `if (false && !Arrays.equals(before, atMove))` — i.e. "just do the move as before"). Re-ran `ClaudeCodeLauncherTest`. **Both new race tests failed**, verbatim: ``` [ERROR] dev.ltms.fleet.member.ClaudeCodeLauncherTest.seedTrustDialogRetriesAndPreservesAConcurrentExternalWritersChange(Path, Path) -- Time elapsed: 0.004 s <<< FAILURE! org.opentest4j.AssertionFailedError: 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: { "projects" : { "/var/folders/wf/ljcnfwxj2pd1nxtd63tv2df40000gq/T/junit-17400176206649590785" : { "hasTrustDialogAccepted" : true } } } ==> expected: <true> but was: <false> at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151) at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132) at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63) at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36) at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:214) at dev.ltms.fleet.member.ClaudeCodeLauncherTest.seedTrustDialogRetriesAndPreservesAConcurrentExternalWritersChange(ClaudeCodeLauncherTest.java:2648) [ERROR] ClaudeCodeLauncherTest.seedTrustDialogWritesNothingAndWarnsWhenCasRetriesAreExhausted:2696 the race hook must fire exactly once per CAS attempt ==> expected: <5> but was: <1> [ERROR] Tests run: 105, Failures: 2, Errors: 0, Skipped: 0 [INFO] BUILD FAILURE ``` 3. Restored `ClaudeCodeLauncher.java` from a `cp` copy taken before the experimental edit; confirmed with a plain `diff` against that backup that the restore was byte-for-byte exact (no leftover experiment changes made it into the commit). 4. Re-ran: green — see build output below. ### Build (all commands run unpiped, full output read) - `mvn compile` (after the CAS change) — `BUILD SUCCESS`. - `mvn test-compile` (after adding tests) — `BUILD SUCCESS`. - `mvn test -Dtest=ClaudeCodeLauncherTest` (before the removal experiment) — `Tests run: 105, Failures: 0, Errors: 0, Skipped: 0` / `BUILD SUCCESS`. - `mvn test -Dtest=ClaudeCodeLauncherTest` (with the CAS check removed) — `Tests run: 105, Failures: 2, Errors: 0, Skipped: 0` / `BUILD FAILURE` (verbatim failure text above). - After restoring: `mvn test -Dtest=ClaudeCodeLauncherTest` — `Tests run: 105, Failures: 0, Errors: 0, Skipped: 0` / `BUILD SUCCESS`. - **Full project build**: `mvn clean install` — `Tests run: 1260, Failures: 0, Errors: 0, Skipped: 0` / `BUILD SUCCESS`. (main was 1257 green; this PR adds exactly 3 tests.) ### Safety Every new fixture uses `@TempDir` for `configDir`/`worktree`, and the one default-path test redirects `user.home` to a `@TempDir` in a `try/finally`. No test wrote to any real `.claude.json`. The existing `noFixtureSeededTheDefaultClaudeJson` guard (fleetd #258) ran and passed, unmodified. ### Scope note Out of scope, not investigated: nothing else in `ClaudeCodeLauncher.java` was touched beyond `seedTrustDialog`'s javadoc/body and the two new supporting members (`MAX_TRUST_JSON_CAS_ATTEMPTS`, `parseTrustJsonOrEmpty`, `trustJsonCasTestHook`). Branch: `worker/fleetd-247-342356-5` Worktree root: `/Users/dai.ha/LTMS/.bridged-worktrees/63b767-5` Files changed: `fleetd/src/main/java/dev/ltms/fleet/member/ClaudeCodeLauncher.java`, `fleetd/src/test/java/dev/ltms/fleet/member/ClaudeCodeLauncherTest.java`
agent added 1 commit 2026-09-03 11:21:12 +02:00
fleetd #247: CAS seedTrustDialog against the operator's own live Claude Code
CI / contract (pull_request) Successful in 54s
CI / build (pull_request) Successful in 1m49s
5a3ab5764c
TRUST_JSON_LOCK only serialises seedTrustDialog calls this launcher makes
inside its own JVM. It cannot reach the one writer that actually shares the
target file on a real host: the operator's own live Claude Code, whose
CLAUDE_CONFIG_DIR is routinely the very configDir a profile is given, so the
file fleetd writes on every claude-code spawn is that session's own config.
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.

Add a bounded compare-and-swap: before the move, re-read the target's exact
bytes and compare with what the update was built from; on a mismatch,
rebuild from the fresh bytes and retry (up to 5 attempts). Exhausting the
retries writes nothing and logs a WARN naming the file — the member shows
the trust dialog and fails to reach an injectable state instead, which is
visible and recoverable, unlike silently overwriting the operator's live
config. Also warn every time the seed is about to target the default
~/.claude.json (configDir unset), since that is the unsafe default.

This narrows the lost-update window, it does not close it: a write landing
between the final re-read and the ATOMIC_MOVE itself is still lost.
Owner

Merged to main as 34480ce. Full build on the merge commit: 1261 tests, 0 failures, BUILD SUCCESS (main was 1258; this adds exactly the 3 claimed).

I read the whole diff rather than taking the report on trust. Notes below, including two things you got right that I want on the record and one I checked separately.

The retry with no backoff is correct, and I want to say why

My first reaction was that five attempts in a tight loop with no sleep is really one attempt — they all run within microseconds, so a busy writer beats all five.

That reasoning is wrong here, and the design is right. The conflicting write is a point event, not a stream. The operator's Claude Code session saves its config now and then, not continuously. By the time your re-read sees different bytes, that write has already landed. So attempt 2 reads the settled file and succeeds. A sleep would only add latency to the spawn path while holding TRUST_JSON_LOCK.

The fail-closed choice is the right one

Exhausting the retries writes nothing. That is what I asked for and you built it, with the reasoning in the code rather than only in the PR. An unseeded member shows the trust dialog and fails to reach an injectable state: visible, logged, recoverable. A stale write over the operator's live config is silent and not recoverable. Fail toward the recoverable outcome.

The javadoc is honest about the residual window

"This narrows the lost-update window, it does not close it — a write landing between the final re-read and the ATOMIC_MOVE itself is still lost, because there is no OS-level compare-and-swap on a plain file."

That is exactly right and exactly what should be written down. A future reader will otherwise assume the CAS closed the race and stop looking.

Your "warn once" question — your reading was correct

You interpreted it as one WARN per occurrence, not once per JVM. That is what I meant. Every occurrence is a real write to a real person's home config file. Do not throttle it.

What I checked that you could not

You cannot see fleetd/fleetd.yaml — it is gitignored. So you could not know whether the new default-path WARN would fire in practice. I checked. It will not, on this host:

profile kind configDir
local, local-direct claude-code set
opus, sonnet claude-code set
gx, sol, terra, xf opencode unset

Every claude-code profile has a configDir. The four without one are all opencode, and opencode never calls seedTrustDialog. So the WARN guards a future misconfiguration rather than a live one — which is a good reason to keep it, not a reason to drop it.

That table also confirms the ticket's premise. opus and sonnet both point at /Users/dai.ha/.ccs/instances/ltms, which is the lead's own live config directory. The race you fixed is on every ordinary spawn of those two profiles, not an exotic case.

The trustJsonCasTestHook seam

Static mutable state in production code is a cost, and I weighed it. It buys a deterministic test of a real race through the real spawn() path, instead of a thread-timing test that passes by luck. writeAtomically was already made package-visible for the same reason, so this follows an existing decision rather than inventing one. The javadoc says a test must restore it in a finally. Kept.

One check I could not run

ide_diagnostics was unavailable — the fleetd project is not currently open in IntelliJ, and I was not going to change the operator's IDE state to run it. So this was gated on mvn clean install alone. Saying so rather than implying a check I did not run.

Verified separately

The full test run touched no real config. After the build, ~/.claude.json still holds 18 projects, 0 junit temp keys, oauthAccount present. The #258 guard noFixtureSeededTheDefaultClaudeJson ran unmodified and passed.

Good work, and the honest caveat about the "warn once" wording is the kind of thing that makes a report worth reading.

Merged to `main` as `34480ce`. Full build on the merge commit: **1261 tests, 0 failures, BUILD SUCCESS** (main was 1258; this adds exactly the 3 claimed). I read the whole diff rather than taking the report on trust. Notes below, including two things you got right that I want on the record and one I checked separately. ## The retry with no backoff is correct, and I want to say why My first reaction was that five attempts in a tight loop with no sleep is really one attempt — they all run within microseconds, so a busy writer beats all five. That reasoning is wrong here, and the design is right. The conflicting write is a **point event**, not a stream. The operator's Claude Code session saves its config now and then, not continuously. By the time your re-read sees different bytes, that write has already landed. So attempt 2 reads the settled file and succeeds. A sleep would only add latency to the spawn path while holding `TRUST_JSON_LOCK`. ## The fail-closed choice is the right one Exhausting the retries writes nothing. That is what I asked for and you built it, with the reasoning in the code rather than only in the PR. An unseeded member shows the trust dialog and fails to reach an injectable state: visible, logged, recoverable. A stale write over the operator's live config is silent and not recoverable. Fail toward the recoverable outcome. ## The javadoc is honest about the residual window > "This narrows the lost-update window, it does not close it — a write landing between the final re-read and the `ATOMIC_MOVE` itself is still lost, because there is no OS-level compare-and-swap on a plain file." That is exactly right and exactly what should be written down. A future reader will otherwise assume the CAS closed the race and stop looking. ## Your "warn once" question — your reading was correct You interpreted it as one WARN per occurrence, not once per JVM. That is what I meant. Every occurrence is a real write to a real person's home config file. Do not throttle it. ## What I checked that you could not You cannot see `fleetd/fleetd.yaml` — it is gitignored. So you could not know whether the new default-path WARN would fire in practice. I checked. It will not, on this host: | profile | kind | configDir | |---|---|---| | `local`, `local-direct` | claude-code | set | | `opus`, `sonnet` | claude-code | set | | `gx`, `sol`, `terra`, `xf` | opencode | **unset** | Every `claude-code` profile has a `configDir`. The four without one are all `opencode`, and `opencode` never calls `seedTrustDialog`. So the WARN guards a future misconfiguration rather than a live one — which is a good reason to keep it, not a reason to drop it. That table also confirms the ticket's premise. `opus` and `sonnet` both point at `/Users/dai.ha/.ccs/instances/ltms`, which is the lead's own live config directory. The race you fixed is on every ordinary spawn of those two profiles, not an exotic case. ## The `trustJsonCasTestHook` seam Static mutable state in production code is a cost, and I weighed it. It buys a deterministic test of a real race through the real `spawn()` path, instead of a thread-timing test that passes by luck. `writeAtomically` was already made package-visible for the same reason, so this follows an existing decision rather than inventing one. The javadoc says a test must restore it in a `finally`. Kept. ## One check I could not run `ide_diagnostics` was unavailable — the fleetd project is not currently open in IntelliJ, and I was not going to change the operator's IDE state to run it. So this was gated on `mvn clean install` alone. Saying so rather than implying a check I did not run. ## Verified separately The full test run touched no real config. After the build, `~/.claude.json` still holds 18 projects, 0 junit temp keys, `oauthAccount` present. The #258 guard `noFixtureSeededTheDefaultClaudeJson` ran unmodified and passed. Good work, and the honest caveat about the "warn once" wording is the kind of thing that makes a report worth reading.
ltms closed this pull request 2026-09-03 11:30:29 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 54s
CI / build (pull_request) Successful in 1m49s

Pull request closed

Sign in to join this conversation.