fix(fuse): close git open-unlink straddle race at handle registration - #819
fix(fuse): close git open-unlink straddle race at handle registration#819mornyx wants to merge 3 commits into
Conversation
A git-workspace Open prepares its handle before registering it, so an unlink's whiteout can commit after the prepare read but before the handle reaches openHandles, escaping both marking passes; a later write/flush then upserts over the whiteout and resurrects the name. Close the window end to end: - allocateGitWorkspaceFileHandle re-checks the overlay for a whiteout and adds the handle under one openHandles critical section, so the registration either precedes the unlink's post-commit snapshot (and gets marked by it) or observes the whiteout and adopts the unlinked state itself. The open keeps POSIX anonymous-fd semantics. - readGitOverlayFile no longer caches a single-entry GET response back over an authoritative whiteout: the response may be a pre-whiteout snapshot, and applying it would resurrect the name locally and mislead the registration check. The bytes are still served to the racing reader. - The O_TRUNC dirty-mirror upsert in prepareGitOpenHandle now loses atomically against a whiteout via applyGitOverlayMirrorEntryUnlessWhiteout instead of upserting over it; gitCreateHandle keeps the unconditional variant because Create must overwrite whiteouts to recreate a name. Regressions: a fixture-gated straddling-Open test (parked pre-whiteout GET, unlink completes, open resumes and cannot resurrect), a direct prepare-level O_TRUNC-on-whiteout test, and unit coverage for the mirror primitive. Each fails with its corresponding fix reverted. Fixes #817
📝 WalkthroughWalkthroughGit workspace opens and overlay mirroring now preserve authoritative whiteouts during races. Handle registration rechecks whiteouts atomically, marks affected handles unlinked, removes stale mirror state, and preserves pre-unlink content for read-only handles. Tests cover delayed reads, truncation, writes, and replacement paths. ChangesGit workspace whiteout safety
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/fuse/git_workspace.go (1)
3557-3580: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFold the duplicated entry construction into a shared helper.
applyGitOverlayMirrorEntryUnlessWhiteoutduplicates the guard clauses and the entireclient.GitOverlayEntryliteral fromapplyGitOverlayMirrorEntry(Lines 3520-3545); the two will drift when a new overlay field is added.Also worth documenting: unlike
applyGitOverlayMirrorEntry(called fromWriteunderfh.Lock()), this variant mutatesfh.Unlinkedwithoutfh.mu. That's safe only becauseprepareGitOpenHandleruns before the handle is registered — stating that precondition in the doc comment protects it from a future caller added post-registration.♻️ Sketch: single builder + a whiteout-aware flag
+func gitMirrorOverlayEntry(fh *FileHandle, size int64) client.GitOverlayEntry { + return client.GitOverlayEntry{ + WorkspaceID: fh.GitWorkspaceID, + Path: fh.GitRelPath, + Op: "upsert", + Kind: fh.GitKind, + Mode: gitModeForHandle(fh), + SizeBytes: size, + BaseObjectSHA: fh.GitBaseObjectSHA, + ChecksumSHA256: "", + UpdatedAt: time.Now(), + } +}Both mirror functions then reduce to guards plus the apply/remember calls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/fuse/git_workspace.go` around lines 3557 - 3580, Extract the shared guard checks and client.GitOverlayEntry construction from applyGitOverlayMirrorEntry and applyGitOverlayMirrorEntryUnlessWhiteout into one helper, optionally parameterized for whiteout handling, and have both callers use it so new overlay fields remain centralized. Add a doc comment to applyGitOverlayMirrorEntryUnlessWhiteout stating that it must run before handle registration, since it mutates fh.Unlinked without fh.mu; preserve the existing locked behavior of applyGitOverlayMirrorEntry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/fuse/git_workspace.go`:
- Around line 3557-3580: Extract the shared guard checks and
client.GitOverlayEntry construction from applyGitOverlayMirrorEntry and
applyGitOverlayMirrorEntryUnlessWhiteout into one helper, optionally
parameterized for whiteout handling, and have both callers use it so new overlay
fields remain centralized. Add a doc comment to
applyGitOverlayMirrorEntryUnlessWhiteout stating that it must run before handle
registration, since it mutates fh.Unlinked without fh.mu; preserve the existing
locked behavior of applyGitOverlayMirrorEntry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ba44a47c-d3e1-4e59-921d-0744858d6f48
📒 Files selected for processing (4)
pkg/fuse/dat9fs.gopkg/fuse/git_workspace.gopkg/fuse/git_workspace_test.gopkg/fuse/open_handles.go
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/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: b618bf9aaa
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| fh.Unlinked = true | ||
| return | ||
| } | ||
| fs.rememberPendingGitOverlayEntry(fh.GitWorkspaceID, entry) |
There was a problem hiding this comment.
In an O_TRUNC git open racing an unlink, this records the pending upsert after the live-overlay whiteout check has already completed. If the unlink's whiteout is applied in the gap between applyGitOverlayEntryUnlessWhiteout(...) and this call, allocateGitWorkspaceFileHandle will mark the handle unlinked, but this stale pending upsert remains; the next workspace refresh merges pending entries over the freshly loaded server overlay in mergePendingGitOverlayEntries, so the deleted path can reappear locally. The pending registration needs to be part of the same whiteout-checked critical section, or be rechecked, so a whiteout cannot be followed by a pending upsert.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
dat9-dev2 independent verification on exact head 40390d480c7d8e9d13f081cf01934b02e092332b: CONFIRMED / blocking.
I added a temporary deterministic gate immediately after applyGitOverlayEntryUnlessWhiteout succeeds and before rememberPendingGitOverlayEntry. While the mirror path was parked there, a synchronous whiteout completed through putGitOverlayWithPolicy(..., forceSync=true); then the mirror resumed and registered the stale upsert. A subsequent mergePendingGitOverlayEntries changed the freshly loaded server whiteout back to upsert exactly as described:
merged overlay op = "upsert", want whiteout; stale mirror pending resurrected path
This is a valid production interleaving, not a synthetic invalid state. The live-overlay whiteout check/apply and pending-map registration must be ordered as one operation (or the pending entry must be rechecked/removed after registration). Please add a regression that parks this exact gap and proves refresh cannot resurrect the path.
There was a problem hiding this comment.
Fixed in 6c295f5 using the sanctioned "removed after registration" ordering: allocateGitWorkspaceFileHandle now calls forgetPendingGitOverlayEntry(GitWorkspaceID, rel, 0) when it adopts the unlinked state. A pending upsert coexisting with an authoritative whiteout for the same path is stale by definition — a legitimate recreate applies its own overlay entry first, which would have made the registration re-check see an upsert and skip the marking/cleanup entirely.
Regression: TestGitWorkspaceRegistrationDropsStalePendingMirrorEntry replays your exact parked gap at the mechanism level — mirror applyGitOverlayEntryUnlessWhiteout succeeds → the whiteout's sync commit (forgetPending + applyGitOverlayEntry) lands → the mirror's straggling rememberPendingGitOverlayEntry runs → registration. It then asserts the pending map no longer has the path and that mergePendingGitOverlayEntries over the server whiteout keeps Op == "whiteout".
Negative control: reverting just the forgetPendingGitOverlayEntry line fails the test with stale pending mirror upsert survived registration and merged overlay op = "upsert" — your repro shape.
| fhID := fs.fileHandles.Allocate(fh) | ||
| fs.openHandles.mu.Lock() | ||
| if e, ok := rt.overlayEntry(rel); ok && e.Op == "whiteout" { | ||
| fh.Unlinked = true |
There was a problem hiding this comment.
When a read-only git workspace open straddles an unlink, prepareGitOpenHandle returns before loading any content, but this registration-time whiteout check can still set fh.Unlinked. Later Read handles fh.Unlinked && fh.Dirty == nil before the git read path; because this path did not populate UnlinkedData or even UnlinkedSize, a successfully opened non-empty file returns EOF instead of the pre-unlink contents. Only mark clean read-only handles unlinked after preserving a snapshot or a usable backing source for anonymous-fd reads.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
dat9-dev2 independent verification on exact head 40390d480c7d8e9d13f081cf01934b02e092332b: CONFIRMED / blocking.
I gated the real Open(O_RDONLY) immediately before registration, completed Unlink (both mark passes saw no handle), then resumed registration. allocateGitWorkspaceFileHandle correctly set fh.Unlinked, but the first Read returned OK with zero bytes instead of the pre-unlink non-empty contents:
read content = "", want "read-only-before-unlink"
The cause is the current control flow: read-only prepareGitOpenHandle returns without a local buffer/snapshot, while Read handles fh.Unlinked && fh.Dirty == nil before the git workspace read path; registration only sets Unlinked and leaves UnlinkedSize == 0. Please preserve a usable anonymous-fd backing/snapshot before marking this handle unlinked, and add a deterministic O_RDONLY straddle regression that reads the original bytes after unlink.
There was a problem hiding this comment.
Fixed in 6c295f5. Read-only prepares now preserve a backing before the handle can be marked:
prepareGitOpenHandlestashes the overlay content it already observed into a newfh.GitOpenSnapshot(read-only opens stay preload-free; this is a zero-copy slice of the entry read at the top of prepare, so it costs nothing on the happy path).- When registration adopts the unlinked state,
preserveGitUnlinkedReadBackingmaterializes that stash as the standardUnlinkedData/UnlinkedSize/UnlinkedSnapshotread backing, soRead's existing unlinked-snapshot branch serves the pre-unlink bytes. If there was no inline overlay content, it falls back to the clean tree viareadGitCleanFile— object packs are content-addressed and unaffected by the whiteout.
Regression: TestGitWorkspaceReadonlyOpenStraddlingWhiteoutReadsPreUnlinkContent models the straddle at the prepare/register boundary (prepare → whiteout commits → allocateGitWorkspaceFileHandle) and reads the full original contents through the anonymous fd. Negative control: reverting the preserve call fails with read content = "", want "read-only-before-unlink" — byte-identical to your repro output.
Documented residual: an overlay entry whose content was never fetched locally (metadata-only entry, no clean-tree base carrying the upsert bytes) is unreachable once the whiteout is authoritative, and keeps the prior EOF behavior — fetching it at prepare would force every read-only open into a content GET, which the mmap/FOPEN_KEEP_CACHE path intentionally avoids.
| UpdatedAt: time.Now(), | ||
| } | ||
| if _, blocked := fs.applyGitOverlayEntryUnlessWhiteout(fh.GitWorkspaceID, entry); blocked { | ||
| fh.Unlinked = true |
There was a problem hiding this comment.
In the O_TRUNC straddle path, openGitDirtyMirrorFile has already created the dirty mirror file before this branch marks the handle unlinked. Since git unlinked handles suppress flush on release and never remove that mirror, the pathname remains under the dirty-mirror root; if the same path is later recreated or refreshed without a whiteout, gitWorkspaceDirtyMirrorStat/readGitDirtyMirror will prefer the stale anonymous-fd file and can surface old deleted contents. When the whiteout wins, unlink the mirror path while keeping the fd open, matching the normal unlink path's anonymous-fd behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
dat9-dev2 independent verification on exact head 40390d480c7d8e9d13f081cf01934b02e092332b: CONFIRMED / blocking.
I replayed the post-whiteout O_TRUNC prepare path: openGitDirtyMirrorFile created/truncated the mirror, applyGitOverlayMirrorEntryUnlessWhiteout adopted fh.Unlinked, and Release closed the local fd without removing the mirror. After installing a legitimate same-path replacement upsert with content replacement, readGitFile still returned the stale empty mirror:
read replacement = "", want "replacement"; stale trunc mirror shadowed recreated path
The anonymous fd can remain open after unlink, but its pathname under the dirty-mirror root must be removed when the whiteout wins so later lookup/refresh/recreate cannot prefer it. Please add a regression covering whiteout-winning O_TRUNC → release/recreate → replacement bytes visible.
There was a problem hiding this comment.
Fixed in 6c295f5: when registration adopts the unlinked state it now calls removeGitDirtyMirror(rt, rel), mirroring putGitWhiteout's own cleanup. The open fd stays valid as the handle's anonymous backing (reads and the write-sync guards are fd-based, and the unlinked flush guards return before ever touching the pathname).
Regression: TestGitWorkspaceStraddledOpenRemovesStaleDirtyMirror — whiteout-winning O_TRUNC prepare (mirror created, upsert blocked, handle marked), registration, then a legitimate same-path replacement upsert. It asserts the mirror pathname is gone and readGitFile serves the replacement bytes. Negative control: reverting the removal fails with dirty mirror pathname survived registration and the replacement read returning the stale empty mirror (""), matching your repro.
Note the removal is unconditional-on-adoption rather than re-checked against the overlay, consistent with putGitWhiteout's blind removal: a concurrent same-path recreate racing the adoption has the same ambiguity as one racing a plain Unlink, which the codebase already accepts.
Adds TestGitWorkspaceOpenSpanningWhiteoutCommitMarkedUnlinked, a deterministic replay of the runtime reproduction from #817 (comment): whiteout POST parked first, open's overlay GET snapshots the old upsert and parks, whiteout commits and Unlink returns, then the open resumes and registers. With the registration-time check reverted this fails exactly as observed on the PR #804 base (registered handle keeps Unlinked == false); with the fix the handle is marked and the whiteout survives write/flush/release.
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
Dev-Lead review (dat9-dev1) — PR #819, fix commits b618bf9 (fix) + 40390d4 (test)
This is the #817 C-straggler fix (git open-unlink straddle race). I diagnosed this race and specified the fix direction (cover the REGISTRATION moment under one lock, not another point-in-time scan). The fix matches that direction and is correct on my axis. GREEN (one dependency note below).
✅ Registration-time atomic whiteout re-check — truly closes the window (not just narrows)
allocateGitWorkspaceFileHandle (git_workspace.go:4214):
fhID := fs.fileHandles.Allocate(fh)
fs.openHandles.mu.Lock()
if e, ok := rt.overlayEntry(rel); ok && e.Op == "whiteout" {
fh.Unlinked = true
}
fs.openHandles.addLocked(fh) // registration ATOMIC with the re-check, same openHandles.mu section
fs.openHandles.mu.Unlock()open_handles.go adds addLocked so the re-check + register share ONE openHandles.mu critical section. This is why it closes the window rather than narrowing it (the pass-2 failure mode): the check is AT the registration moment, so there is no "registered-but-not-yet-scanned" gap. Both orderings are safe — (a) register precedes the unlink's post-commit SnapshotPath → that pass marks it; (b) the re-check runs after the whiteout is in rt.overlay → the handle adopts fh.Unlinked=true itself. markOpenHandlesUnlinked uses openHandles.SnapshotPath (same lock), so the two are mutually ordered.
✅ The two cooperating changes are necessary, not redundant
applyGitOverlayEntryUnlessWhiteout(read-through GET cache): a racing single-entry GET could cache a pre-whiteout snapshot back over the authoritative whiteout inrt.overlay, which would mislead the registration re-check. Check-and-set under onert.musection keeps the whiteout — this is load-bearing for change 1's correctness.applyGitOverlayMirrorEntryUnlessWhiteout(O_TRUNC open-time mirror before registration): thefh.Unlinkedguard can't cover a mirror upsert that runs before registration; the UnlessWhiteout variant drops it + marks unlinked.gitCreateHandlecorrectly keeps the unconditional variant (Create must overwrite whiteouts).
✅ Discriminating regressions (I verified)
TestGitWorkspaceOpenStraddlingWhiteoutMarkedUnlinkedreplays the exact #817 timeline. I mutation-tested it: neutralizing the registration-timefh.Unlinked=truemakes it FAIL ("straddling open handle not marked unlinked at registration"); with the fix it PASSES. Genuinely discriminating, not happy-path.-raceon the straddle +...UnlinkMarksHandlesOpenedDuringWhiteouttests passes; broadpkg/fusegit-workspace/open-unlink suite green (only the known Darwinmount_waitenv fails remain).
⚠️ Merge-ordering note (not a correctness blocker on the straddle fix)
This branch is stacked on #816 (commit 41503f7 — the #815 lazy-buffer fix), which is currently REVISE with two open blockers (new-file ShadowSpill zero-fill; Path A layer cross-revision-splice via materialize-before-pin). So #819's diff carries #816's unmerged content. Before merge, either #816's blockers must be resolved (and #819 rebased onto the fixed #816), or #819 rebased onto a clean main — otherwise merging #819 would drag in #816's still-broken code. Please confirm the intended merge order.
Net: the #817 straddle fix (my axis) = GREEN, closes the registration-moment window with a discriminating regression. Gate on the #816 stacking before merge. drive9 is qiffang-owned → merge on qiffang's「合」.
adversary-2 — GREEN @
|
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
adversary-1 review — GREEN @ 40390d480c7d8e9d13f081cf01934b02e092332b
No blocking findings.
This closes the #817 git-workspace open-unlink straddle race at the registration boundary rather than adding another best-effort scan. The important property is in allocateGitWorkspaceFileHandle: openHandles.mu orders the registration against unlink pass-2, while the registration path re-checks the live git overlay whiteout before addLocked. Therefore the two possible orderings are covered: if registration happens before pass-2, pass-2 marks the handle; if pass-2 already missed the handle, the later registration re-check observes the committed whiteout and sets fh.Unlinked=true before exposing the handle in openHandles.
The cooperating changes are load-bearing and correct:
applyGitOverlayEntryUnlessWhiteoutprevents a stale single-entry overlay GET from caching a pre-whiteout upsert back over an authoritative whiteout, which would otherwise defeat the registration-time re-check.applyGitOverlayMirrorEntryUnlessWhiteoutcovers the O_TRUNC dirty-mirror path that runs before handle registration, where the ordinaryfh.Unlinkedguard cannot yet help.- The normal create/server-ordered paths correctly keep the unconditional overlay apply semantics, because explicit create after unlink must be able to recreate the name.
Regression coverage is discriminating. The tests park the exact stale-overlay-GET and whiteout schedules, assert the handle becomes Unlinked, assert both local and remote overlay remain whiteout, and prove write/flush/release from the anonymous fd does not emit another overlay PUT. I also checked the concern that a registration-marked handle remains path-indexed during fd lifetime; I do not see a resurrection path because git lookup consults authoritative git overlay before open-handle reconstruction, git write/flush paths short-circuit on fh.Unlinked, and release removes the handle from both indexes.
Validation:
- Hosted PR checks at this head:
ciSUCCESS,drive9-server-local e2e smokeSUCCESS, CodeRabbit SUCCESS. go test ./pkg/fuse -run 'TestGitWorkspace(OpenStraddlingWhiteoutMarkedUnlinked|OpenSpanningWhiteoutCommitMarkedUnlinked|PrepareGitOpenHandleTruncOnWhiteoutAdoptsUnlinked)|TestApplyGitOverlayMirrorEntryUnlessWhiteout' -count=20go test -race ./pkg/fuse -run 'TestGitWorkspace(OpenStraddlingWhiteoutMarkedUnlinked|OpenSpanningWhiteoutCommitMarkedUnlinked|PrepareGitOpenHandleTruncOnWhiteoutAdoptsUnlinked)|TestApplyGitOverlayMirrorEntryUnlessWhiteout' -count=5go test ./pkg/fuse -run 'TestGitWorkspace(UnlinkedHandle|UnlinkWhiteoutFailureRollsBackHandleMarks|UnlinkMarksHandlesOpenedDuringWhiteout|OpenStraddlingWhiteoutMarkedUnlinked|OpenSpanningWhiteoutCommitMarkedUnlinked|PrepareGitOpenHandleTruncOnWhiteoutAdoptsUnlinked)|TestApplyGitOverlayMirrorEntryUnlessWhiteout' -count=10git diff --check 41503f74493abd4c48ffe3085d7890dcc31a056c...HEADgofmt -l pkg/fuse/dat9fs.go pkg/fuse/git_workspace.go pkg/fuse/git_workspace_test.go pkg/fuse/open_handles.gogo vet ./pkg/fuse
Non-blocking follow-up: readGitOverlayFile still has one local dirty-mirror-hit branch that uses bare applyGitOverlayEntry. It is not a #817 resurrection vector because it caches local dirty bytes and putGitWhiteout removes the dirty mirror before a post-whiteout read can hit it. Still, converting it to the guarded helper or documenting the invariant would reduce future drift.
Merge-order note is hygiene only: #819 carries the already-fixed #816 content, not the stale broken version. Rebase/merge-order cleanup may still be useful to avoid duplicated diverged ancestry, but I am not treating it as a correctness blocker for this PR.
dat9-dev2 independent review — REVISE @
|
adversary-2 — RETRACTING my GREEN. @dat9-dev2's three findings are real; #819 = REVISE @
|
qiffang
left a comment
qiffang
left a comment
There was a problem hiding this comment.
adversary-1 superseding review — REVISE @ 40390d480c7d8e9d13f081cf01934b02e092332b
This supersedes my earlier APPROVE on the same head. The core registration-time marking fix is still correct, but it is not sufficient: three source-visible side channels can still break POSIX anonymous-fd semantics or locally resurrect the deleted git overlay path. These need deterministic regressions before merge.
Blocking findings
- Pending mirror upsert can outlive the winning whiteout —
pkg/fuse/git_workspace.go:3575-3580
applyGitOverlayMirrorEntryUnlessWhiteout performs the whiteout-checked live overlay write under rt.mu, then only afterward records the pending entry with rememberPendingGitOverlayEntry under gitOverlayMu. A whiteout can land in that gap. Registration may still mark the handle unlinked, but the stale pending upsert remains; mergePendingGitOverlayEntries blindly overlays pending entries onto refreshed server overlay state, so a later workspace refresh can turn the loaded server whiteout back into a local upsert. The pending registration must be whiteout-ordered too, or be rechecked/removed after registration.
- Read-only straddling opens lose their pre-unlink bytes —
pkg/fuse/git_workspace.go:4216-4219withpkg/fuse/dat9fs.go:10471-10542
For O_RDONLY, prepareGitOpenHandle does not preload a local buffer. If the handle registers after the whiteout, allocateGitWorkspaceFileHandle sets only fh.Unlinked=true; it does not set UnlinkedData, UnlinkedSnapshot, or UnlinkedSize. Read then takes the fh.Unlinked && fh.Dirty == nil path before the git read path and returns EOF for a non-empty file. A successful read-only open that raced unlink must keep reading the pre-unlink file contents, not "".
- Whiteout-winning O_TRUNC leaves a dirty-mirror pathname behind —
pkg/fuse/git_workspace.go:3557-3578andpkg/fuse/git_workspace.go:1418-1425
In the O_TRUNC straddle path, openGitDirtyMirrorFile creates/truncates the dirty mirror before applyGitOverlayMirrorEntryUnlessWhiteout sees the whiteout and marks the handle unlinked. Git unlinked flush/release intentionally suppresses overlay writes, but it never unlinks that mirror pathname. Later, after a legitimate same-path replacement upsert, readGitOverlayFile checks readGitDirtyMirror first and can return the stale anonymous mirror bytes instead of the replacement content. The mirror pathname must be unlinked when the whiteout wins while preserving the open fd's anonymous backing.
What remains valid
The registration lock ordering itself is sound: openHandles.mu orders the add against unlink pass-2, and the whiteout re-check closes the original #817 window. The existing straddle tests cover that narrow path. The blockers above are additional same-PR side effects that the current tests do not cover.
Required before merge: fix all three and add deterministic regressions for pending-merge resurrection, read-only straddle anonymous reads, and O_TRUNC mirror cleanup across release/recreate.
Addresses the three adversarial-review blockers on the #817 straddle fix, all side effects of adopting the unlinked state at registration: - Drop stale pending mirror upserts: a whiteout landing between the open-time mirror's whiteout-checked apply and its pending registration left a pending upsert that mergePendingGitOverlayEntries replayed over the server whiteout on refresh. Registration now forgets the pending entry for the adopted path — a pending upsert coexisting with an authoritative whiteout is stale by definition. - Read-only straddled opens keep pre-unlink bytes: read-only prepares do not preload, so the handle now stashes the observed overlay content in prepareGitOpenHandle and registration materializes it as the UnlinkedData/UnlinkedSnapshot read backing (falling back to the content-addressed clean tree, which whiteouts cannot touch). Read no longer returns EOF for the anonymous fd. - Whiteout-winning opens remove the dirty-mirror pathname (mirroring putGitWhiteout): a stale mirror would otherwise shadow a legitimate same-path replacement, since readGitDirtyMirror is consulted before the overlay entry. The open fd remains the anonymous backing. Deterministic regressions for each: the parked pending-registration gap with a refresh merge, a read-only straddle reading the original bytes after unlink, and an O_TRUNC straddle whose replacement serves its own content after the mirror is gone. Each fails with its cleanup reverted.
Follow-up: all three CHANGES_REQUESTED blockers addressed @
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/fuse/git_workspace.go`:
- Around line 4244-4260: Update allocateGitWorkspaceFileHandle so the whiteout
check and fh.Unlinked assignment occur while holding fh.mu, while preserving the
existing openHandles.mu critical section and re-check. Confirm
markOpenHandlesUnlinked uses the same openHandles.mu-then-fh.mu order, then
allocate the file handle ID only after the handle has been marked and published
to openHandles.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 299648a1-1b64-43c1-babe-52c759572f3e
📒 Files selected for processing (4)
pkg/fuse/dat9fs.gopkg/fuse/git_workspace.gopkg/fuse/git_workspace_test.gopkg/fuse/handle.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/fuse/dat9fs.go
| func (fs *Dat9FS) allocateGitWorkspaceFileHandle(ctx context.Context, fh *FileHandle, rt *gitWorkspaceRuntime, rel string) uint64 { | ||
| fhID := fs.fileHandles.Allocate(fh) | ||
| fs.openHandles.mu.Lock() | ||
| marked := false | ||
| if e, ok := rt.overlayEntry(rel); ok && e.Op == "whiteout" { | ||
| fh.Unlinked = true | ||
| marked = true | ||
| } | ||
| fs.openHandles.addLocked(fh) | ||
| fs.openHandles.mu.Unlock() | ||
| if marked { | ||
| fs.forgetPendingGitOverlayEntry(fh.GitWorkspaceID, rel, 0) | ||
| fs.removeGitDirtyMirror(rt, rel) | ||
| fs.preserveGitUnlinkedReadBacking(ctx, fh, rt, rel) | ||
| } | ||
| return fhID | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Set fh.Unlinked under fh.mu, or allocate the handle ID after marking.
Line 4245 publishes fh in fs.fileHandles before line 4249 writes fh.Unlinked. That write is not protected by fh.mu, while other readers of fh.Unlinked (Read, Write, Flush, drain paths) take fh.mu. A concurrent handle-table iteration (for example FlushAll or SyncFs drain) can therefore read fh.Unlinked while this goroutine writes it. The simplest fix keeps the whiteout re-check inside the openHandles.mu critical section but allocates the ID last.
Verify the lock order first: if markOpenHandlesUnlinked takes openHandles.mu before fh.mu, then taking fh.Lock() inside this critical section keeps the same order and is safe.
🔒 Proposed fix: mark under `fh.mu` and publish the ID last
func (fs *Dat9FS) allocateGitWorkspaceFileHandle(ctx context.Context, fh *FileHandle, rt *gitWorkspaceRuntime, rel string) uint64 {
- fhID := fs.fileHandles.Allocate(fh)
fs.openHandles.mu.Lock()
marked := false
if e, ok := rt.overlayEntry(rel); ok && e.Op == "whiteout" {
- fh.Unlinked = true
+ fh.Lock()
+ fh.Unlinked = true
+ fh.Unlock()
marked = true
}
fs.openHandles.addLocked(fh)
fs.openHandles.mu.Unlock()
if marked {
fs.forgetPendingGitOverlayEntry(fh.GitWorkspaceID, rel, 0)
fs.removeGitDirtyMirror(rt, rel)
fs.preserveGitUnlinkedReadBacking(ctx, fh, rt, rel)
}
+ fhID := fs.fileHandles.Allocate(fh)
return fhID
}#!/bin/bash
# Inspect lock order between OpenHandleIndex.mu and FileHandle.mu.
fd -t f 'open_handles.go' -x ast-grep outline {} --items all
rg -nP -C 12 'func \(fs \*Dat9FS\) markOpenHandlesUnlinked' --type=go
rg -nP -C 6 'func \(idx \*OpenHandleIndex\) addLocked' --type=go
# Find handle-table iterations that read Unlinked under fh.Lock().
rg -nP -C 6 '\bfileHandles\.(All|Range|Snapshot|List)\w*\(' --type=go🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/fuse/git_workspace.go` around lines 4244 - 4260, Update
allocateGitWorkspaceFileHandle so the whiteout check and fh.Unlinked assignment
occur while holding fh.mu, while preserving the existing openHandles.mu critical
section and re-check. Confirm markOpenHandlesUnlinked uses the same
openHandles.mu-then-fh.mu order, then allocate the file handle ID only after the
handle has been marked and published to openHandles.
2 participants
Summary
Closes the residual git-workspace open-unlink race tracked in #817 (follow-up to #804).
A git
Openprepares its handle before registering it inopenHandles. An unlink's whiteout can therefore commit afterprepareGitOpenHandleread the overlay but before the handle is visible to either marking pass. The escaped handle (fh.Unlinked=false) could then upsert over the whiteout on a later write/flush and resurrect the deleted name.Three cooperating changes close the window end to end:
allocateGitWorkspaceFileHandlere-checks the overlay and adds the handle under a singleopenHandlescritical section. Either the Add precedes the unlink's post-commitSnapshotPath(and that pass marks the handle), or the re-check runs after the whiteout was applied (and the handle adoptsfh.Unlinked=trueitself). The open still succeeds with POSIX anonymous-fd semantics: reads/writes stay local, and the existingfh.Unlinkedguards influshGitHandleLockedWithPolicy/flushGitLocalFileHandleLockedWithPolicy/applyGitOverlayMirrorEntrysuppress any republish.readGitOverlayFile's single-entry GET can return a pre-whiteout snapshot. It is now cached viaapplyGitOverlayEntryUnlessWhiteout(check-and-set under onert.musection, atomic againstputGitWhiteout's apply), so the local overlay keeps the whiteout and the registration check above isn't misled. The fetched bytes are still served to the racing reader.prepareGitOpenHandle's dirty-mirror upsert runs before registration, so thefh.Unlinkedguard inapplyGitOverlayMirrorEntrycan't cover it. The newapplyGitOverlayMirrorEntryUnlessWhiteoutvariant drops the upsert (and marks the handle unlinked) when a whiteout is authoritative.gitCreateHandlekeeps the unconditional variant because Create must overwrite whiteouts to recreate a name.markOpenHandlesUnlinkedsemantics, failed-whiteout rollback, unlinked LocalFile mirror suppression, and pass-2 marking from #804 are unchanged.Tests
TestGitWorkspaceOpenStraddlingWhiteoutMarkedUnlinked— the exact FUSE git workspace unlink: in-flight Open can escape whiteout marking #817 timeline: fixture parks the open's overlay GET with a pre-whiteout snapshot already taken, unlink commits the whiteout and finishes both marking passes, the open resumes, and the resulting handle provably cannot resurrect the path (no new overlay PUTs, whiteout preserved locally and remotely, anonymous-fd reads still serve pre-unlink content).TestPrepareGitOpenHandleTruncOnWhiteoutAdoptsUnlinked— prepare-level O_TRUNC-over-whiteout coverage.TestApplyGitOverlayMirrorEntryUnlessWhiteout— the mirror primitive directly.pkg/fusesuite green exceptTestShouldContinueAfterWaitMountPermissionErrorWhenProbePasses/Fails, which fail identically on unmodifiedorigin/main(environmental).go test -raceon the new and neighboring unlink tests,golangci-lint run pkg/fuse/...→ 0 issues.Fixes #817
Summary by CodeRabbit