[lukemaj]

What: a server secret being created is never visible until its bytes are complete, so concurrent creators always agree on the stored secret.
Why: create opened the final secret file with wx and then wrote it; a creator that lost the race read the winner's still-empty file and returned 0 bytes. The flaky test was catching a real race.
So what: after merge the race test is deterministic and the store no longer hands out empty secrets; nothing else is needed.

Closes #249

Elon record

• Requirements and who asked: test(server): ServerSecretStore race test is flaky #249 (the user's standing rule to file problems found during other work): the race test must pass reliably. Its non-goal allows a store change because the flake revealed a real race.
• Deleted: any test-only fix (retries, longer timeouts); the defect is in create.
• Bottleneck: reproducing the failure on demand. Solved with a probe and then a deterministic test.
• Checked myself: a disposable probe racing two real creators 300 times per run hit one mismatch where the loser returned a 0-byte secret (lengths [ 0, 32 ]); the new test fails on the baseline and passes with the fix.

Fix

create now writes the complete value to <name>.bin.<uuid>.tmp (wx, mode 0600, fsync, chmod), then links it to <name>.bin. The hard link is atomic and fails with EEXIST (mapped to AlreadyExists) when another creator won, so every existing caller (getOrCreateRandom, persistEnvironmentKeyPair, CloudLink, SecretRequests) keeps its contract. The temporary file is always removed.

The changed upstream files are carried edits: listed in scripts/fork-upstream-edits.txt and owned by the secret-store-atomic-create entry in docs/fork-features.md (scripts/fork-check.sh: OK).

Proof

• New test "never exposes a secret before its bytes are written": pauses writeAll inside create, reads the secret mid-write, and asserts it is absent. Baseline: AssertionError: expected false to be true. With the fix: passes.
• vp test run src/auth/ServerSecretStore.test.ts 50 times in a loop: 50 passed, 0 failed.
• vp test run src/auth src/cloud src/secrets: 30 files, 321 tests passed.
• tsc --noEmit in apps/server: no diagnostics in changed files; vp lint and vp fmt --check clean.

Not checked: Windows. fs.link creates NTFS hard links, but this PR ran on Linux only.

Upstream: pingdotgg/t3code main has the same create; no Issue or PR there covers this race.

Model and harness: Claude Opus 5.5 in Claude Code.

🤖 Generated with Claude Code

[lukemaj]

Independent review (Sol, gpt-6.1-sol) of 6d4e7c7

What: no blocking findings at exact head 6d4e7c785b03c6fc659b4cbada04470e832cf927.
Why: the baseline exposes the final filename before writing, while this change publishes complete bytes with an exclusive hard link.
So what: independent review passes; merge remains the maintainer’s decision.

Findings

• Non-blocking, filesystem compatibility: apps/server/src/auth/ServerSecretStore.ts:252. Creation now requires hard-link support in the secrets directory. A home on a filesystem without it fails with SecretStorePersistError, including initial getOrCreateRandom calls. This is a real new filesystem constraint, not a general Windows incompatibility: Windows NTFS supports hard links, but FAT/exFAT do not. Keep this limitation explicit; Windows execution was not verified here. Microsoft’s CreateHardLinkW contract documents the NTFS requirement. Source inspection does not establish support on every network filesystem.
• Non-blocking, cleanup claim: apps/server/src/auth/ServerSecretStore.ts:253. Cleanup is attempted on success, typed failure, and interruption, but removal failures are ignored and process termination cannot run the finalizer. A crash after writing or linking can therefore retain a mode-0600 secret-bearing .tmp file, including after the canonical secret is later removed. There is no stale-temp scavenging. This does not invalidate atomic publication, but the PR body’s “temporary file is always removed” should be qualified as best-effort cleanup on effect exit.

Verified mechanism and callers

• The final file is absent until writeAll, sync, scoped file close, and chmod(0600) finish. The same-directory link publishes the already-complete inode without overwriting another winner; removing the temporary name does not remove the final link. Directory mode remains 0700 and both names share the secured file’s permissions. Closing before linking also avoids retaining a writable descriptor across publication.
• Installed @effect/platform-node-shared@4.0.1 calls Node fs.link(existingPath, newPath) and maps EEXIST to PlatformError.reason._tag === "AlreadyExists". SecretStorePersistError retains that cause, so isSecretAlreadyExistsError still recognizes collisions.
• Reviewed all production callers: getOrCreateRandom and cloud/environmentKeys.ts reread the winner; cloud/CloudLink.ts treats the collision as replay; secrets/SecretRequests.ts preserves an earlier saved value. Other failures remain typed errors, including UUID-generation failure through the existing SecretStoreTemporaryPathError variant. No caller contract or transport/schema change is required.
• The regression pauses the actual file’s writeAll using Deferred handshakes, observes absence through the real store, then releases and joins creation and checks persisted bytes. It contains no sleep, polling, or timeout-based synchronization.

Independent proof

Candidate worktree HEAD and live PR head both matched the exact SHA above; base was 7184184a5919f1f86296febca690e4b9fa389059. The worktree was clean before and after review.

• Candidate: from apps/server, npx -y pnpm@11.10.0 exec vp test run src/auth/ServerSecretStore.test.ts: 8 passed, exit 0.
• Baseline comparison: ran the unchanged new test with a disposable external Vite config whose pre-load hook supplied the complete baseline ServerSecretStore.ts from git show. No checkout or repository file edits. npx -y pnpm@11.10.0 exec vp test run src/auth/ServerSecretStore.test.ts --config /tmp/pr261-sol-review-iitpxtua/config.mts -t 'never exposes a secret before its bytes are written': 1 failed, 7 skipped, exit 1, specifically AssertionError: expected false to be true at ServerSecretStore.test.ts:226. The baseline exposes an empty final file while the writer is paused, confirming the diagnosis and intended regression signal.
• Complete diff reviewed: only the store’s existing create implementation and one deterministic regression test changed. No repo-wide checks, server startup, browser use, or live T3 data access. The author’s 50-run and broader-suite claims were not independently repeated. Windows and unsupported-filesystem behavior were inspected, not executed.

Reviewer: Sol, gpt-6.1-sol through the Codex harness.

[github-actions]

Thread transfer impact

✅ Thread transfer remains within every enforced ceiling.

Provider	Metric	Main baseline	This PR	Impact	PR ceiling
Codex	Total thread wire	5.0 KiB	5.0 KiB	0 B (0.0%)	6.8 KiB	✅
Codex	Thread snapshot wire	3.8 KiB	3.8 KiB	0 B (0.0%)	4.9 KiB	✅
Codex	Live turn WebSocket wire	1.2 KiB	1.2 KiB	0 B (0.0%)	2.0 KiB	✅
Codex	Live turn WebSocket decoded	20.9 KiB	20.9 KiB	0 B (0.0%)	29.3 KiB	✅
Codex	Live turn messages	2	2	0 (0.0%)	8	✅
Claude	Total thread wire	5.0 KiB	5.0 KiB	0 B (0.0%)	6.8 KiB	✅
Claude	Thread snapshot wire	3.8 KiB	3.8 KiB	0 B (0.0%)	4.9 KiB	✅
Claude	Live turn WebSocket wire	1.2 KiB	1.2 KiB	0 B (0.0%)	2.0 KiB	✅
Claude	Live turn WebSocket decoded	21.2 KiB	21.2 KiB	0 B (0.0%)	29.3 KiB	✅
Claude	Live turn messages	2	2	0 (0.0%)	8	✅

Baseline: 4ac498f · PR result: 5dd590e · Source CI: success

Scenario and decoded snapshot size

10 historical turns, 5 command tools per turn, 878.9 KiB retained MCP result per historical turn, and a 1.05 MiB retained result in the measured turn.

• Codex decoded thread snapshot: 108.5 KiB
• Claude decoded thread snapshot: 108.8 KiB

Updated in place by a trusted workflow. PR artifacts are strictly validated and never executed.

[lukemaj]

Independent review (Grok, grok-4.7) of 6d4e7c7

Verdict: no blocking findings, and no non-blocking findings. review/independent-grok is success on this SHA only.

During the review the PR head moved to 045489ace61ae4565c87f70455ca30a7f7c4a724 (chore(fork): carry the atomic secret create), which only allowlists the two upstream files and adds a feature-map entry. That commit is outside this verdict.

Diagnosis

The flake is a real race. On the parent of 6d4e7c7, create opened <name>.bin with wx and then wrote it (ServerSecretStore.ts in that parent). wx publishes the directory entry before writeAll returns. get (apps/server/src/auth/ServerSecretStore.ts:174-185) returns Option.some for every successful read, including a 0-byte file, and only NotFound becomes none. getOrCreateRandom (:266-305) returns that value and does not regenerate. A loser that observes AlreadyExists and reads during the winner's write therefore returns empty bytes while the winner returns the generated secret.

A local open(path, "wx") on Linux was readable at length 0 before the write, and length 3 after it. That is the window the new test closes.

Create

create (ServerSecretStore.ts:229-263) writes <name>.bin.<uuid>.tmp with wx and mode 0600, writeAll, fsync, closes the descriptor (the Effect.scoped release), chmod 0600, then fileSystem.link(temp, final). The same-directory link is the publication. A same-filesystem probe of that sequence left mode 600, kept bytes [1, 2, 3] after fs.rm of the temp name (nlink 2 to 1), and a second link onto the existing final path failed with EEXIST / syscall: link.

@effect/platform-node-shared@4.0.1 link goes through handleErrnoException, which maps EEXIST to AlreadyExists (internal/utils.ts:22-23). mapError (ServerSecretStore.ts:254-260) stores that PlatformError as SecretStorePersistError.cause. isSecretAlreadyExistsError (:135-136) reads cause.reason._tag === "AlreadyExists", the same shape open(wx) already produced. Effect.ensuring runs the temp remove on success, failure, and interruption; Effect.ignore keeps a cleanup failure from replacing the EEXIST failure (effect@4.0.1 ensuring is onExit). The concurrent test forces both creators through create (the first two reads are NotFound) and both results must equal the persisted bytes, so a swallowed AlreadyExists would fail that test.

Cleanup covers the paths that leave a temp name: open / writeAll / sync / chmod / link failure, and the success path after the link. A crash can leave *.bin.<uuid>.tmp (mode at most 0600 inside the 0700 secrets directory). get never reads that name. Replay pruning keeps only names that endsWith(".bin") (replayMarkers.ts:38-41), and the secret-request sweep only matches /^(secret-request-[0-9a-f]{32})\.bin$/ (SecretRequests.ts:42). set already uses this temp shape (replayMarkers.ts:29-31).

Windows was not executed. Node fs.link is CreateHardLink and works on NTFS; libuv reports an existing destination as EEXIST, which this mapper already turns into AlreadyExists. A volume that cannot hard-link fails link before the final name exists, so create returns SecretStorePersistError and does not publish a partial file.

Callers

Each production caller still treats AlreadyExists as "the winner's complete file is the result":

• getOrCreateRandom re-reads (ServerSecretStore.ts:283-298). After a losing link, the final name exists and the bytes are the winner's completed write.
• persistEnvironmentKeyPair re-reads (environmentKeys.ts:62-75).
• consumeCloudReplayGuards turns AlreadyExists into false (CloudLink.ts:344-351). Existence still means a full marker.
• SecretRequests.answer ignores AlreadyExists and does not replace the stored value (SecretRequests.ts:133-142).
• verifyRequestDpopProof maps AlreadyExists to a replay rejection (dpop.ts:43-54, call at :100-113). A crash mid-write no longer leaves an empty canonical marker.

replayMarkers.test.ts calls create only as a fixture.

Test and scope

never exposes a secret before its bytes are written (ServerSecretStore.test.ts:189-238) pauses writeAll on a Deferred pair and reads before releasing it. No sleep and no poll. On the baseline, wx has already created the final file, get returns Some([]), and assert.isTrue(Option.isNone(midWrite)) fails (expected false to be true). I did not re-run the old test file; the parent source plus the wx probe is the evidence. With the fix, that read happens before link, so the final name is absent.

cd apps/server && npx -y pnpm@11.10.0 exec vp test run src/auth/ServerSecretStore.test.ts on this SHA (vitest start 08:54:17, before 045489a at 08:54:42): 8 tests passed, 32ms, including the new test and the concurrent-creator test.

6d4e7c7 changes only ServerSecretStore.ts and ServerSecretStore.test.ts. Issue #249 allows a store change when the flake is a real race. No version bump.

[lukemaj]

Independent review round 2 (Sol, gpt-6.1-sol) of 5dd590e

What: independent review passes at exact head 5dd590edbf13a02a95448f6bd3b17324d07f340d, with no blocking findings.
Why: the rebase preserved the reviewed code and neighbouring carry entries; the new carry owner is valid and focused checks pass.
So what: review/independent-sol will be success on this SHA; merge remains the maintainer's decision.

Findings

• Blocking: none.
• Non-blocking, carried forward from round 1: apps/server/src/auth/ServerSecretStore.ts:252 now requires hard-link support in the secrets directory. On a filesystem that rejects hard links, creation fails with SecretStorePersistError; there is no fallback. Filesystem compatibility beyond the current Linux test environment was not executed.
• Non-blocking, carried forward from round 1: apps/server/src/auth/ServerSecretStore.ts:253 removes the temporary name on effect exit, but ignores removal errors and cannot clean up after process termination. A secret-bearing mode-0600 temporary file can remain. The PR body's claim that the temporary file is "always removed" is still too strong; describe cleanup as best effort on effect exit.

Exact-head and rebase verification

Live PR and clean author worktree both matched the exact head above before and after proof; base is 4ac498f0d03b040f8841411db962b94552594409.

• Read the entire four-file PR diff and surrounding create/get/collision paths and consumers (getOrCreateRandom, environment key persistence, CloudLink replay guards, SecretRequests, and DPoP replay handling).
• Both complete secret-store files, and the two-file code patch relative to their respective bases, are byte-for-byte identical to round 1's 6d4e7c7. The final filename remains absent until the temporary write, sync, scoped close and chmod finish; exclusive link publication preserves collision handling.
• Parsed every feature JSON block. All existing feature objects are unchanged from the new base, including macos-temp-realpath-tests; the only new object is secret-store-atomic-create.
• The allowlist preserves its base prefix and appends exactly the two secret-store paths, each once. Those paths each have exactly one primary owner, secret-store-atomic-create. The two neighbouring macOS test paths each remain listed once with sole primary owner macos-temp-realpath-tests.
• The new entry links Issue test(server): ServerSecretStore race test is flaky #249 and PR fix(server): a racing secret creator never reads a half-written secret #261, accurately names the atomic-create mechanism and its upstream removal condition, and introduces no unrelated ownership changes.

Independent proof on this candidate

From /home/luke/dev/toolboxmd/chromeria-worktrees/249-secret-race:

• node scripts/fork-features.mjs check: exit 0, fork-features: OK (32 features).
• npx -y pnpm@11.10.0 exec vp test run scripts/fork-maintenance.test.ts: exit 0, 15 passed. Printed failure messages are expected negative-fixture assertions, not failed tests.
• From apps/server, npx -y pnpm@11.10.0 exec vp test run src/auth/ServerSecretStore.test.ts: exit 0, 8 passed, including the paused-write regression and concurrent-creator test.
• git diff --check 4ac498f0d03b040f8841411db962b94552594409 HEAD: exit 0.

No repository edits, checkout, config/remote changes, server startup, browser use, live T3 data access, or repo-wide checks. Round 1's baseline-failure proof and the author's repeat-loop/broader-suite reports were not rerun in this round. Windows was not executed. This verdict is an independent code review with focused proof, not a claim that the full CI merge gate passed.

Reviewer: Sol, gpt-6.1-sol through the Codex harness.

[lukemaj]

Independent review round 2 (Grok, grok-4.7) of 5dd590e

Verdict: no blocking findings at exact head 5dd590edbf13a02a95448f6bd3b17324d07f340d.

Rebase and carry

The code change matches round 1. apps/server/src/auth/ServerSecretStore.ts and apps/server/src/auth/ServerSecretStore.test.ts have the same blobs as 6d4e7c7 (8f20f22b7d6cb9754161d32e4ef9f1099211e211 and af7bc0774d08f028f96a89517aaaf7c040069ecb). Those files are unchanged between the old base 7184184 and 4ac498f. Commit 5dd590e touches only docs/fork-features.md and scripts/fork-upstream-edits.txt. History is linear: 4ac498f → 7ef8713 → 5dd590e.

The new secret-store-atomic-create entry parses. node scripts/fork-features.mjs check printed fork-features: OK (32 features). It is the only upstreamFiles owner of both secret-store paths, and nothing lists them in sharedFiles. macos-temp-realpath-tests is intact and remains the only owner of apps/server/src/cli/invocation.test.ts and apps/server/src/environment/ServerEnvironment.test.ts. The feature-map diff against 4ac498f appends after that entry's closing fence. The allowlist diff appends the two secret paths after that entry's two test paths, each once.

Proof

Worktree /home/luke/dev/toolboxmd/chromeria-worktrees/249-secret-race was clean at this SHA before and after these runs.

• node scripts/fork-features.mjs check: OK, 32 features, exit 0.
• npx -y pnpm@11.10.0 exec vp test run scripts/fork-maintenance.test.ts: 15 passed, exit 0.
• From apps/server, npx -y pnpm@11.10.0 exec vp test run src/auth/ServerSecretStore.test.ts: 8 passed, exit 0.

Findings

None.

Round 1's non-blocking notes still apply. The code diff is unchanged, so they are not new defects and they do not block.

• Non-blocking, filesystem compatibility: apps/server/src/auth/ServerSecretStore.ts:252. Publication uses fileSystem.link in the secrets directory. A filesystem without hard links fails create with SecretStorePersistError and does not publish a partial file. Windows was not run in this round.
• Non-blocking, cleanup claim: apps/server/src/auth/ServerSecretStore.ts:253. Removal is Effect.ensuring plus Effect.ignore, so it runs on success, typed failure, and interruption, and a removal failure does not replace the original error. A crash after the temp write can leave a mode-0600 *.bin.<uuid>.tmp. The PR body's "temporary file is always removed" overstates that. get does not read that name.