fix(node-sync): write the git identity through the merged view, not the overlay upper#546
Merged
Conversation
…he overlay upper #541 shipped git author identity and it did not work in production. A live prod session with a correctly resolved identity still committed as `Centaur AI <ai@centaur.local>`. The daemon wrote the identity into the overlay's `upper` before `mount_overlay`. That is fine for a cold create, and silently useless for a warm claim — which is the common case. A warm pod's overlay is mounted when the pod is created, minutes before the claim that gives it an identity, and modifying an upperdir behind a live mount is undefined: the file lands on disk (the upper listed it, 10s before the commit) while the merged view keeps serving its cached negative dentry, so the agent never sees it. It also leaves the path incoherent — a later mkdir through the mount failed with ESTALE. The lowerdir comment a few lines up in the same loop describes this exact hazard; the identity write walked into it anyway. Write through `mounted.merged` after the mount instead, which is the supported way to modify an overlay. `materialize_git_identity` now takes the `OverlayMountPlan` rather than a bare `&Path` and derives `merged` itself, so passing the upper is no longer expressible. That is the actual fix: the old signature made the wrong call site type-check, and every tempdir unit test passed while production was broken. Adds tests/git_identity_overlay_visibility.rs — a real overlay, mounted first, then the identity, asserting through the merged view. That shape is the only one that can catch this; a tempdir test passes either way. Ordering caveat, documented at the call site: a warm pod's ready marker predates the claim, so there is no write-before-ready guarantee here — the agent could commit in the seconds before the identity lands. That degrades to the baked identity rather than a wrong one. Closing it for real needs the claim path to carry the identity.
…lly gate cargo test runs as the unprivileged runner, so every test needing a real overlay mount self-skips there. That is exactly how the identity bug reached prod green: the write went to an upperdir behind a live mount, and no lane could see it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#541 shipped broken and prod proved it
A live prod session with a correctly resolved identity still committed as
Centaur AI <ai@centaur.local>:Not a timing problem — the file was there 10 seconds early. It was in the wrong place.
The daemon wrote into the overlay's upper before
mount_overlay. Fine for a cold create; silently useless for a warm claim, which is the common case — a warm pod's overlay is mounted at pod creation, minutes before the claim that gives it an identity. Modifying an upperdir behind a live mount is undefined: the file lands on disk while the merged view keeps serving its cached negative dentry. It also leaves the path incoherent — a latermkdirthrough the mount failed with ESTALE.The lowerdir comment a few lines above in the same loop describes this exact hazard ("the mount keeps the old dir inode pinned... re-materialized files never become visible"). The identity write walked straight into it.
The fix
Write through
mounted.mergedafter the mount — the supported way to modify an overlay.More importantly:
materialize_git_identitynow takes theOverlayMountPlanand derivesmergeditself, instead of accepting a bare&Path. That's the real fix. The old signature is why the wrong call site type-checked; deriving the path makes passing the upper unrepresentable rather than merely discouraged. It immediately caught its own call sites at compile time when I changed it.Why the tests missed it
Every unit test used a plain tempdir, where upper and merged are the same directory — so they passed either way and proved nothing about the thing that broke. Adds
tests/git_identity_overlay_visibility.rs: a real overlay, mounted first, then the identity — the warm-claim shape — asserting through the merged view, plus that a 204 clears it there too. Runs in the privileged Linux suite (skips silently without root).Known limitation (documented at the call site)
A warm pod's ready marker predates the claim, so there's no write-before-ready guarantee here — an agent could commit in the seconds before the identity lands. That degrades to the baked identity rather than a wrong one, which is why it's acceptable for now; closing it properly needs the claim path itself to carry the identity.
Verification
Linux clippy
-D warnings,cargo fmt, 257 lib + 16 contract + privileged overlay tests, all green in the Docker gate (host clippy is blind tolinux_daemon).