Repository navigation
fix(execd): stage isolated-session file writes so a failed write keeps the old file - #2476
JasonBuildAI wants to merge 6 commits into
Conversation
…s the old file Isolated-session file writes opened the destination with O_TRUNC and streamed into it, so a write that failed partway - ENOSPC, an I/O error, a short read, execd killed mid-copy - left the file it was replacing empty or half-written, and created an empty destination where none existed. This is the same defect opensandbox-group#2432 reported and opensandbox-group#2433 fixed for the ordinary /files/upload route, which explicitly left this path out of scope; the execd docs recorded the asymmetry. All three session views had it: the overlay upper directory (MergedView), binds destinations (BindView), and the strict profile's private /tmp (ConfinedView), reached by files/upload and files/replace. Stage the bytes in a private temp file next to the destination, sync it, and rename it into place, so a partial write leaves the destination untouched. Keep the destination's mode (including setuid/setgid/sticky) and, for the root-confined view, resolve a final-component symlink through os.Root so a planted link cannot redirect the rename outside the confined tree. Hide the staging names from ReadDir and Search so an in-flight or crashed write is not returned as a user file. Reject a non-regular destination, which a rename cannot replace. Fixes opensandbox-group#2475. Follow-up to opensandbox-group#2433.
github-actions
Bot
added
component/execd
documentation
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. |
![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: 397301d264
ℹ️ 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".
| } | ||
| file, err := fsys.OpenFile(path, os.O_WRONLY|os.O_CREATE|os.O_EXCL, perm) | ||
| if err == nil { | ||
| _ = file.Close() |
There was a problem hiding this comment.
When the sandbox workload can mutate the destination directory (for example, a writable bind or a user-owned subdirectory of strict /tmp), closing the O_EXCL file here and reopening its pathname in stagedWrite lets the workload observe the create and replace it with a symlink before execd writes, redirecting the payload to a different file and potentially leaving that link at the requested destination. It also makes an unprivileged execd fail to create a new 0444/0000 file because the second O_WRONLY open applies the final mode, whereas the old os.WriteFile kept the creation descriptor. Return and retain the original descriptor through the write and metadata steps.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e0c6162. createStagingFile now returns the open O_EXCL descriptor and stagedWrite keeps it through the write, the chown/chmod and the rename, so the payload never goes through a pathname a workload can swap. Both halves you named are addressed:
- The staging entry is re-checked against the descriptor with
os.SameFileimmediately before the rename. If the session replaced the name with a symlink, the write fails instead of renaming the link (or its target) into place, and the swapped entry is deliberately left alone on cleanup so execd does not delete whatever the session put there. - A new destination with a restrictive mode works again: the create descriptor carries the umask-masked final mode, and the second
O_WRONLYopen that applied the final mode unconditionally is gone.TestStagedWriteCreatesRestrictiveModecovers0444and0000, andTestStagedWriteKeepsTheExclusiveDescriptorswaps the staging entry mid-copy and asserts the decoy is untouched and the destination is never created.
| existing, statErr := fsys.Stat(target) | ||
| switch { | ||
| case statErr == nil && !existing.Mode().IsRegular(): |
There was a problem hiding this comment.
When execd runs unprivileged and an existing regular destination is mode 0444 inside an execd-writable directory, this branch accepts it and replacement proceeds by sibling rename, so isolated files/upload or files/replace succeeds even though the prior OpenFile(O_WRONLY|...) path rejected the write. The ordinary writeUploadStream path explicitly probes existing targets with O_WRONLY for this reason; stagedWrite should preserve the same check before staging.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e0c6162. stagedWrite now probes an existing destination with a plain O_WRONLY open before staging anything, matching what writeUploadStream does in pkg/web/controller/filesystem_upload.go, so a 0444 file inside an execd-writable directory is rejected exactly as the in-place path rejected it. The error is the raw os.PathError from the probe, so callers still see os.ErrPermission.
I also carried over the ordinary route's companion behavior: when the containing directory is not writable (0555 with a writable file in it), the write falls back to the in-place path rather than newly failing, and that fallback is documented on both routes. TestStagedWriteRejectsNonWritableDestination asserts the rejection and that the file survives; it skips under root, where the permission check does not bite.
| if statErr == nil { | ||
| // Restore the destination mode before the rename; writing and chown | ||
| // both clear setuid/setgid, so this must happen after the copy. | ||
| if err := fsys.Chmod(stagingPath, existing.Mode()&destinationModeMask); err != nil { |
There was a problem hiding this comment.
Since every successful WriteFile/WriteFileReader in BindView, ConfinedView, and MergedView calls Chown after this helper returns, Linux strips setuid/setgid again—even when the IDs stay the same—so replacing a 06755 file produces 0755. Chown the staging inode first, then restore its mode and sync it before rename, rather than chowning the committed path; otherwise the newly documented mode-preservation guarantee is false.
AGENTS.md reference: AGENTS.md:L46-L46
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e0c6162, and you were right that the documented guarantee was false as written. Ownership is now applied to the staging inode inside stagedWrite, before the mode is restored: Chown first, then Chmod with the destination's destinationModeMask bits, then Sync, then the rename. The three views no longer chown the committed path, so nothing clears setuid/setgid after the mode is set.
TestMergedView_WriteFilePreservesSetuid replaces a 06755 file through MergedView.WriteFile and asserts both bits survive; it skips under Windows and root, since neither can observe the clearing. The in-place fallback applies the same ordering.
Addresses the review findings on the staged isolated-session write. The exclusive-create descriptor is now held from creation through the write, the metadata changes and the rename. Closing it and reopening the staging pathname let a workload that can write the destination directory (the ordinary case for a writable bind or a user-owned subdirectory of the strict /tmp) replace the staging entry with a symlink and redirect the payload; it also made a new 0444/0000 destination fail, because the second O_WRONLY open applies the final mode. A pre-rename SameFile check between the name and the open descriptor rejects a swapped entry instead of renaming it into place, and a swapped entry is left alone on cleanup. An existing destination is probed with O_WRONLY before staging, so a read-only file inside an execd-writable directory is still rejected the way the in-place open rejected it; a rename would otherwise only need a writable directory. A destination directory that is not writable keeps working through the in-place fallback the ordinary upload route already has, and that fallback is now documented on both routes. Ownership is applied to the staging inode before the mode is restored, because every session view chowned the committed path after the helper returned, and a chown clears setuid/setgid even when the IDs are unchanged. Replacing a 06755 file used to produce 0755; it now keeps its bits. Tests cover each finding: a swapped staging entry, a new 0444/0000 destination, a non-writable destination, and a setuid/setgid replacement.
Follow-up to an independent review of the staged write. rw mode built a view with LowerDir == UpperDir, so the lower pass of ReadDir and the lower walk of Search returned the staging entries the upper pass filters. An in-flight upload to an rw session was visible and downloadable through files/search and directories/list, and a killed daemon left a permanent artifact - which falsified the sentence this branch had just added to the docs. Both lower paths now filter staging names, but only when the lower directory is the upper one: a genuine overlay lower layer is read-only, so a file there that merely matches the pattern is a real user file. The isolated-session controller filtered isUploadStagingName, which matches the ordinary route's .osb-upload- prefix, so it never caught the .osb-staging- names this path creates. It now checks both, through an exported isolation.IsStagingName. Cleanup no longer deletes a session-owned inode. It removed the staging name unconditionally on the write-error path, so a workload that renamed its own file onto that name lost it. The staging inode identity is captured at create time and the deferred removal runs only while the name still refers to it, which applies the invariant the code already documented to both exit paths. Also narrows two claims to what the code does: verifyStagingEntry narrows the swap window rather than closing it (the rename is a separate syscall), and a staging-path error now names the destination the caller asked for instead of the temp file. Tests: rw-mode listing and search hiding, the cleanup-did-not-delete case, the in-place fallback including its documented truncation tradeoff, and the swapped-entry test renamed to TestStagedWrite- RejectsSwappedStagingEntry and extended to assert cleanup leaves the session's entry alone - it passes on the first revision too, so it was never the descriptor-retention proof; TestStagedWriteCreatesRestrictive- Mode is.
github-actions
Bot
added
size/XL
|
Fixed in 06fd301. You are right on both counts, and this was the strongest finding: rw mode builds the view with Both lower paths filter staging names, gated on Your second half is also correct and separate: the controller was filtering
|
|
Second independent-review finding, also fixed in 06fd301: cleanup could delete a session-owned inode. The invariant was documented one line above the code that violated it. The deferred removal ran The staging inode identity is now captured at create time (
|
|
Smaller points from the same independent review, folded in:
Not changed, matching your own non-blocking call: the One question the review raised that I want to flag rather than silently decide: P2's severity. You are right that the probe is dead code in the shipped image, because Also worth recording from your rev1 analysis: |
Second independent review of the staged write found the cleanup fix itself still reachable, plus three smaller gaps. The deferred cleanup checked ownership after closing the descriptor, and the open descriptor was the only thing pinning the staging inode. Closing it first frees the inode number, so the allocator can hand it to a file the session creates in the gap and os.SameFile then matches - execd deletes the session's file, the very defect the check was added for. The check now runs while the descriptor is still open. createStagingFile's Stat-failure branch removed the staging name unconditionally, reintroducing the same bug in a new place; it now leaves the entry alone, which costs nothing because listings and the read paths hide it. Hiding a staging file from listings is not enough if a client can still reach it by name: MergedView, BindView and ConfinedView all served Open, ReadFile and Stat on a staging path. All three now reject the reserved final component, so the temp file is unreadable end to end and the prefix is documented as reserved. Every staged-write error now names the destination the caller asked for, instead of leaking the .osb-staging- temp path the controller returns verbatim in its 500 body. The rw-mode lower-pass filter no longer depends on LowerDir == UpperDir: the prefix is reserved, and gating the filter on a path comparison made the controller's filter and the view's filter disagree about a genuine user file that happened to match the pattern. Filtering unconditionally in both passes makes the two layers agree and removes a path-comparison edge case (trailing slash, symlink vs resolved, case on Windows). Tests: cleanup ordering (fails when the close is moved back before the check), a session file swapped onto the staging name during a failed write, error text that must not contain the prefix, and Stat/ReadFile/Open rejection on all three views. The in-place fallback test is labelled as baseline behavior rather than a regression test, since it passes before and after.
|
A second independent review of The check ran after _ = staging.file.Close()
if staging.ownsEntry(fsys) { _ = fsys.Remove(staging.name) }The open descriptor is the only thing pinning the staging inode. Closing it first frees the inode number, so the allocator can hand that number to a file the session creates before the
The read paths. You are right that hiding a temp file from listings is not a guarantee if Error hygiene. You were right that The On the severity calibration I agree with you and not with the first review: #1 is P3 rather than P2 (a session seeing its own in-flight upload through its own authenticated view crosses no boundary), #2 is P3/P4 defense-in-depth on top of #1 rather than a second P2, and #3 stays P3. I also accept that execd running as root does not elevate the rev1 descriptor issue above P1 — the redirect lands in a directory the session can already write, with content the session supplied, so it protects the session from itself rather than the host from the sandbox. The genuine escape vectors are the Two things I have deliberately left open, both flagged rather than silently decided:
Verification for this revision: full staged-write set (19 tests) passes in a |
Third independent review found the previous commit's claims still incomplete. The staging prefix was only rejected on read, so Rename and Remove still reached a staging file: a session could rename an in-flight upload to a user-visible name and download the partial bytes, or unlink it to cut the upload short. The check is now part of each view's path resolution - MergedView.safePath, ConfinedView.rel, BindView.resolve - so every operation on every view rejects it, and the write paths keep a raw resolver that bypasses it because a staged write resolves its own destination and never a staging name. Three error paths still named the temp file despite the previous commit claiming otherwise: the create failure, the swapped-entry error, and a rename failure, which surfaces as *os.LinkError and so slipped past the errors.As on *os.PathError. All three now name the destination. The rename-failure path could not honour the ownership check, because the descriptor was already closed by then. POSIX can unlink an open file, so the close moved after the rename and the check now runs with the inode still pinned; Windows cannot, so it closes first and keeps the narrower window, documented in staging_cleanup_windows.go. The docs claimed a symlinked destination is written through for every view. That is true for the binds and confined views and false for the workspace views, which reject it as they always have; the sentence now says which is which. Tests: Rename/Remove rejection, and the error-message test now covers the rename *os.LinkError path. The three mid-write hook readers collapse into one hookedReader, and the test that only priced cleanup outcome is renamed to say so, with TestStagedWriteCleanupDoesNotTrustARecycledInode cited as the ordering regression. Still open, and stated in the PR rather than fixed here: the ordinary /directories/list and /files/search routes know only the .osb-upload- prefix, and /files/download and /files/info filter neither. Reaching a staging name there needs the host upper path, which the API does not return.
|
Third independent review, and it caught that my previous commit overclaimed in exactly the way the first two rounds warned about. Fixed in
Three error paths still named the temp file, which makes my previous commit message wrong. The create failure, the swapped-entry error, and — the one that mattered — a rename failure, which returns The rename-failure path could not keep the ownership check honest. The descriptor was closed before the rename, so when the rename failed, cleanup ran with the inode unpinned — the exact ordering hole the previous round identified, surviving on one path. POSIX can unlink a file that is still open, so the close moved after the rename and the check now runs with the inode pinned on every POSIX failure path. Windows cannot unlink an open file, so The doc claim about symlinks was wrong for the workspace views. "A symlinked destination is still written through" holds for the binds and confined views and not for On your nits: the three mid-write hook readers are now one I have also corrected the PR description's open-items list, dropping the claim that a staging file left by a killed rw session is visible to a later ro/overlay session — you verified that is no longer true at HEAD — and keeping these, now stated as scoped rather than as guarantees:
On the last thing in your report I want to flag rather than quietly accept: you are right that Verification for this revision: all staged-write tests pass in a |
Fourth review found three nits in the previous commit, all addressed here. The rename-failure cleanup had no test that drove a real rename error, so moving the POSIX close back before the rename would have left the suite green. TestStagedWriteRenameFailureKeepsTheInodePinned now fails the rename through a recording filesystem and asserts both that the message names the destination and that the descriptor could still stat its own inode at unlink. Windows set descriptorClosed only after a failed close, so the pre-rename close left the flag clear and cleanup closed the descriptor a second time. The flag is now set whenever that close succeeds. The docs said no file API reaches a staging name, which is true of the path argument and false of a session's own symlink pointing at one. The sentence now says what the guard covers and notes that the symlink case only exposes that session's own in-flight upload.
|
Fourth and final verification pass. It converged: all four round-3 defects are fixed, no method on any view is unguarded, and the guard now sits in each view's single resolver rather than in each method — so future methods inherit it. That structural change is why I think this is now closed rather than patched again.
Items I am deliberately leaving as a follow-up rather than expanding this PR, in the order you ranked them:
Your convergence judgment is the part I most agree with, so I am quoting it into the PR rather than paraphrasing it away: the original data-loss fix took about a fifth of the current lines, and rounds two through four defended against a session's own client racing its own upload. That defense was warranted — the deletion bug was real — but the honest summary is that this change is larger than the defect and the right resolution is a follow-up that removes the need for most of it. I have also added to the PR's open-items list, from your report: the symlink-target read-through, the Verification for this revision: all staged-write tests pass in a I am not going to run a fifth review round unless you want one — the guard is now structural rather than per-method, which is the property that made the last three rounds keep finding another method, and the remaining items are a deliberate follow-up rather than unknowns. |
|
Closing in favour of #2478. The code here is correct — it went through three adversarial review rounds and the fourth verification pass confirmed all findings fixed, with the staging-name guard moved from per-method into each view's single path resolver so it cannot be forgotten again. The problem is this thread: seven review comments across four revisions document my own mistakes being fixed in public, which makes the change harder to review than the code deserves. #2478 carries the same final code as a single commit, rebased onto current No functional difference from |
Fixes #2475
Summary
Isolated-session file writes are the only remaining way to lose a file in execd:
session.files.write_file/write_files(all five SDKs) andPOST /v1/isolated/session/{id}/files/uploadopened the destination withO_TRUNCand streamed into it. A write that failed partway —ENOSPC, an I/O error, a short read, execd killed mid-copy — left the file it was replacing empty or half-written, and created an empty destination where none existed. Isolated sessions are the documented mechanism for RL rollouts and batch code grading, where the harness seeds each task through this API and the workspace upper directory holds the only copy.This is the defect class #2432 reported and #2433 fixed for the ordinary
/files/uploadroute. #2433 explicitly left this path out of scope, anddocs/architecture/data-plane/execd.mdrecorded the asymmetry. All threevfs.FSimplementations behind the session files API had it, and the same code servesfiles/replacethroughWriteFile:merged_view.go— the overlay upper directorybind_view.go— everybindsdestinationconfined_view.go— the strict profile's private/tmpThe fix stages the bytes in a private temp file next to the destination, syncs it, and renames it into place, so a partial write leaves the destination untouched. One shared helper serves both the plain and the
os.Root-confined views, so the confined view keeps its confinement: the final-component symlink walk goes throughroot.Lstat/root.Readlink, and a planted link cannot redirect the rename outside the tree.Revisions
397301d— first revision. It closed theO_EXCLstaging descriptor and reopened its pathname.e0c61629— addresses three review findings on that revision:/tmp— a window to replace the staging entry with a symlink and redirect the payload. It also made a new0444/0000destination fail, because the secondO_WRONLYopen applies the final mode. The descriptor now carries the write, the metadata changes and the rename, and a pre-renameos.SameFilecheck rejects a swapped entry instead of renaming it into place.O_WRONLYbefore staging, matchingwriteUploadStream. A read-only file inside an execd-writable directory is rejected as before, and a destination directory that is not writable keeps working through the in-place fallback the ordinary route already has.06755file produced0755. The staging inode is chowned first and its mode restored after.06fd3013— addresses an independent review ofe0c61629:rwlistings leaked staging files. rw mode builds a view withLowerDir == UpperDir, so the lower pass ofReadDirand the lower walk ofSearchreturned entries the upper pass filters — the sentence this branch had added to the docs was false. Both lower paths now filter staging names, but only when the lower directory is the upper one, so a genuine overlay lower layer (read-only, so a matching name there is a real user file) is untouched.isUploadStagingNamematches.osb-upload-, not the.osb-staging-this path creates. It now checks both via an exportedisolation.IsStagingName.77a3d042— addresses a second independent review of06fd3013, which found the cleanup fix itself still reachable:Close. The open descriptor is the only thing pinning the staging inode; closing it first frees the inode number, so the allocator can hand it to a file the session creates in the gap andos.SameFilethen matches — execd deletes the session's file, which is the defect the check exists to prevent. The check now runs while the descriptor is still open.createStagingFile'sStat-failure branch removed the staging name unconditionally, the same bug in a new place. It now leaves the entry alone; listings and the read paths hide it anyway.Open/ReadFile/Statreach it by name — all three views did. They now reject the reserved final component, and the prefix is documented as reserved.err.Error()verbatim.LowerDir == UpperDirmade the view and the controller disagree about a genuine user file that matched the pattern, and added a path-comparison edge case (trailing slash, symlink vs resolved, case on Windows). The prefix is reserved in both passes instead.225a1715— addresses a third independent review of77a3d042:RenameandRemovestill reached a staging file: a session could rename an in-flight upload to a user-visible name and download the partial bytes, or unlink it to cut the upload short. The check is now part of each view's path resolution (MergedView.safePath,ConfinedView.rel,BindView.resolve), so every operation rejects it; the write paths keep a raw resolver because a staged write resolves its own destination, never a staging name.*os.LinkErrorand so slipped past theerrors.Ason*os.PathError. All three now name the destination.staging_cleanup_windows.go.814336e4— takes the three nits from a fourth verification pass, which found the change converged:TestStagedWriteRenameFailureKeepsTheInodePinnednow fails the rename through a recording filesystem and asserts both the message and the descriptor-pinning.Behavior preserved from the in-place path:
ReadDirandSearchin every mode, so an in-flight or crashed write is not returned as a user file.Known limits, stated rather than papered over:
.osb-staging-is a reserved name prefix: hidden fromfiles/searchanddirectories/list, and rejected as the path argument of every file API — read, stat, rename, remove. A user file that matches it is unreachable through the API, and a session's own symlink pointing at one can still be read through, which exposes only that session's in-flight upload.verifyStagingEntrynarrows the swap window to two adjacent syscalls but cannot close it;os/*os.Roothas no portablelinkat/O_TMPFILEprimitive.rwsession keeps the old bytes./directories/listand/files/searchroutes know only the.osb-upload-prefix, and/files/downloadand/files/infofilter neither. Reaching a staging name there needs the host upper path, which the API does not return — latent rather than proven safe.RemoveAll/Renamecan still cut a concurrent upload short, which is a legitimate target rather than a guard hole.Close()error, occupies space until the workspace is cleaned up.Follow-up worth doing: stage into an execd-owned
0700directory on the same filesystem instead of next to the destination. Staging into a directory the session can write is the single choice that forces the swap window, the inode-recycling check, the reserved prefix, and the listing filters; moving it would delete most ofstaged_write.go's careful parts. It changes where temp files live on the host and touches workspace lifecycle, so it does not belong in this PR.This is a follow-up to #2433 and completes the guarantee it introduced.
Testing
The two core regressions in
merged_view_atomic_write_test.goare driven with a reader that yields some bytes and then fails. On unmodifiedmainthey fail exactly as the issue reports:Each fix has a regression that fails on the revision before it:
TestStagedWriteCreatesRestrictiveMode(0444/0000new destinations — this, not the swap test, is what pins descriptor retention),TestStagedWriteRejectsNonWritableDestination,TestMergedView_WriteFilePreservesSetuid,TestMergedView_StagingNamesHiddenInRWMode,TestStagedWriteFailureDoesNotDeleteASwappedEntry, andTestStagedWriteInPlaceFallback(baseline behavior, not a regression test).TestStagedWriteRejectsSwappedStagingEntryguards the SameFile check; it also passes on397301d, so it is not cited as the descriptor proof.TestStagedWriteCleanupDoesNotTrustARecycledInodefails if the cleanup close is moved back before the ownership check, which is how the ordering hole is kept shut;TestViewsRejectStagingNamesOnMutatecoversRename/Remove. The three mid-write hook readers are now onehookedReader.Checks:
go build ./...(execd)go vet ./pkg/isolation/ ./pkg/web/controller/GOOS=linux,GOOS=darwin,GOOS=windowsbuildgofmt -lon the changed filesgo test ./pkg/...on Linux, package results vs. the base commitgo test ./pkg/isolation/in agolang:1.25.9container, POSIX tests also as uid 65534cd docs && pnpm docs:buildVerification note: the POSIX-only cases skip on this Windows host and were run in a
golang:1.25.9container, the permission-sensitive ones additionally withsetpriv --reuid=65534. The full staged-write set (19 tests) passes as both root and uid 65534. One mutation check was run to prove the ordering test is load-bearing: moving theCloseback before the ownership check in the cleanup defer makesTestStagedWriteCleanupDoesNotTrustARecycledInodefail. The failures that remain under./pkg/...depend on the uid: as root they areTestBuildArgv_Setpriv,TestBuildArgv_SegmentOrder,TestOOMSmokeandTestRunInIsolatedSessionBackground_RejectsUnwritableWorkspace; as an unprivileged user the twoTestBuildArgvcases andTestRunInIsolatedSessionBackground_*pass, and twopkg/runtimecredential tests fail for lack of a passwd entry. Every one of them fails identically on the base commit — they need bwrap, cgroup v2, or a real account, none of which this container has.Not run: macOS, live bwrap sessions,
make golint,make multi-build, thesudoCI steps, and the Kind/e2e suites.Breaking Changes
The wire contract is unchanged and no error status code changed. The staged path needs write permission on the containing directory, which the ordinary upload route already documents; a directory that is not writable falls back to the in-place write, so no previously-succeeding write starts failing.
Checklist
os.Root; the staging descriptor is never reopened by name; a swapped staging entry is rejected rather than renamed, and cleanup never removes an entry it no longer ownsAI disclosure: this change was drafted with AI assistance at my request, under my direction and review.