feat(rt): add os/rename and os/delete-tree - #698
Open
mparrett wants to merge 2 commits into
Open
Conversation
mparrett
force-pushed
the
wt/os-fs-primitives
branch
from
August 10, 2026 04:24
2a5c28f to
a7a20ba
Compare
Collaborator
|
Both guards are the interesting part. On the |
mparrett
force-pushed
the
wt/os-fs-primitives
branch
2 times, most recently
from
August 11, 2026 05:26
87bd0a1 to
6a3f1cc
Compare
mparrett
force-pushed
the
wt/os-fs-primitives
branch
from
August 11, 2026 17:45
6a3f1cc to
c9c83bb
Compare
mparrett
force-pushed
the
wt/os-fs-primitives
branch
2 times, most recently
from
August 14, 2026 23:10
1b73601 to
d6e5a0b
Compare
Grenadine's host contract needs an atomic move and a recursive delete, and neither had a home. `delete-file` removes a single entry and fails on a non-empty directory, so callers open-code a walk; nothing at all wrapped rename(2), so publishing a file meant spit and a window where a reader could see it half-written. os/rename returns the destination and fails across filesystems rather than falling back to copy-then-delete. That fallback is the thing callers reach for this instead of, so substituting it silently would drop the only property separating it from spit. os/delete-tree removes a path and everything under it, and succeeds when the path is already absent — the post-state asked for is the one that holds. That diverges from delete-file, which throws, so it is pinned by a test in both suites. An empty path is refused: os.RemoveAll treats "" as a silent no-op, which hides the unset variable that produced it. Both land in the os namespace rather than core, following os/unzip (#688). Core is the always-loaded surface and there is active work to shrink it; these are host effects and belong beside the other ones. The TinyGo os namespace is a deliberate three-function subset and gains neither, matching how os/unzip was added. Requested in #688; abogoyavlensky confirmed they were not working on these. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three real findings from review. syscall/rm-rf already exists on every platform and is the same RemoveAll call, so the claim that delete-file was the only removal primitive was wrong. Say so in the doc comment and give the actual reason for a second spelling: syscall is the container-setup namespace, sitting beside clone, pivot-root and seccomp, which is not where a caller doing ordinary filesystem work looks. The empty-path guard protected the least damaging case while permitting the worst. (str nil) is "" in lg, so a caller building "$root/$name" with root unset gets "/name" — one level below the root, straight past the guard. Refuse the filesystem root too, via Dir(p) == p, which holds exactly at a volume root on both unix and Windows. TestOsDeleteTreeLeavesSiblingsAlone asserted a property no implementation could violate: two unrelated directories, one deleted, the other checked. Replace it with the containment property that can actually break — a symlink inside the tree pointing out of it must be unlinked, not followed — plus its surprising corollary, that a path which is itself a symlink to a directory loses only the link. Both are now documented on the native, since "removes path and everything beneath it" does not suggest the second. Also: match the file's error-message grammar, drop a test comment claiming to observe a torn intermediate state the test cannot see, and cover the zero-arg case for rename. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
mparrett
force-pushed
the
wt/os-fs-primitives
branch
from
August 16, 2026 22:40
d6e5a0b to
92bd03f
Compare
This was referenced Aug 16, 2026
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
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.
Two filesystem primitives that had no equivalent in the runtime.
os/renamewrapsrename(2). Nothing did, so publishing a file meantspitand a window in which a reader could observe it half-written. Staging to a temporary name and renaming into place closes that window, and rename is the only call that gives it.os/delete-treeis the recursive form ofdelete-file, which removes a single entry and fails on a non-empty directory.test/os_unzip_test.lgin this repo unwinds its fixture with fivedelete-filecalls in dependency order, which is the shape this replaces.A correction to what I wrote on #688: a recursive delete does already exist, as
syscall/rm-rf, and I missed it. It is the sameRemoveAllcall. The argument for a second spelling is placement rather than capability —syscallis the container-setup namespace, sitting besideclone,pivot-root,chrootandseccomp, which is not where someone doing ordinary filesystem work looks, andosis now whererenameandunziplive. If you'd rather have one, the alternative is to leavesyscall/rm-rfas the only spelling and drop this half of the PR;os/renamehas no equivalent anywhere and stands either way.Both came out of surveying let-go against Grenadine's host contract (#688). Of the slots still unfilled, these were the two that were plain missing functions rather than design questions; @abogoyavlensky confirmed they weren't working on them.
Behavior worth agreeing on
os/renamefails across filesystems rather than falling back to copy-then-delete. The fallback is the thing a caller reaches for this instead of, so substituting it silently would remove the only property separating the call fromspit. AnEXDEVerror is the honest answer.os/delete-treesucceeds when the path is already absent. The post-state the caller asked for is the one that holds. This diverges fromdelete-file, which throws, so it's pinned by a test in both suites rather than left to be rediscovered.Symlinks are unlinked, never followed. A link inside the tree pointing outside it does not take the target down. By the same rule a path that is itself a symlink to a directory loses only the link, and the directory keeps its contents — worth documenting because "removes path and everything beneath it" does not suggest it. Both are now tested.
An empty path and the filesystem root are refused.
os.RemoveAll("")is a silent no-op in Go, which hides the unset variable that produced it. The root is the same mistake with a worse outcome:(str nil)is""in lg, so a caller building"$root/$name"withrootunset gets"/name", one level below the root and past an empty-string check. The root test isDir(p) == pafterClean, which holds exactly at a volume root on unix and Windows alike.Placement
Both go in the
osnamespace rather than core, followingos/unzipfrom #688. Core is the always-loaded surface and there's active work to shrink it; these are host effects and belong beside the other host effects.The TinyGo
osnamespace is a deliberate three-function subset (exit,getenv,args) and gains neither, matching howos/unzipwas added.Verification
pkg/rt/os_fs_test.go— 13 tests covering the effect on disk, replacing an existing destination, moving a populated directory, symlink containment in both directions, absent-source, empty-path and root errors, and the argument guards.test/os_fs_test.lg— the same surface from lg, checking what each call returns and that siblings survive.go test -short ./...— 21 packages, no failures.linux/amd64,js/wasm,wasip1/wasm,plan9/amd64, anddarwin/arm64.windows/amd64fails inpkg/rt/term.goonunix.SIGWINCH, which reproduces onmainataeac42d4and is untouched by this change.No generated artifacts change: the
osnamespace is registered from Go, so there's nogenerated.sumsregen the way #690 needed one.