Conversation
📝 WalkthroughWalkthroughAdds macOS/Darwin support and platform splits across filesystem, openat2 detection, system info, and build targets; introduces Darwin-specific implementations and tests, and updates build/Makefile targets and logging levels. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)internal/ufs/fs_unix.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (9)
✏️ Tip: You can disable this entire section by setting 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.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@internal/ufs/fs_darwin.go`:
- Around line 40-58: The code in UnixFS.Chtimesat uses linux-style Stat_t fields
Atim/Mtim which don't exist on macOS; replace references to st.Atim and st.Mtim
with Darwin names st.Atimespec and st.Mtimespec and convert their timespecs to
time.Time (e.g., using st.Atimespec.Unix()/UnixNano() or an equivalent
conversion) before assigning atime/mtime so the function compiles and uses the
correct Darwin fields.
In `@internal/ufs/fs_unix.go`:
- Around line 653-660: The field useOpenat2 on UnixFS is written without
synchronization causing a data race; change the UnixFS struct to use an
atomic.Bool for useOpenat2, update all reads to use fs.useOpenat2.Load() and the
write in the open path to fs.useOpenat2.Store(false) (so the branch around
fs._openat2 / fs._openat remains correct), and add the "sync/atomic" import (or
the atomic package alias used) so the race detector is silenced.
In `@internal/ufs/walk_linux.go`:
- Around line 18-33: The defensive fallback in nameFromDirent uses
name[:len(de.Name)] which can panic when ml (derived from de.Reclen) is smaller
than len(de.Name); change the fallback to return the already-sliced name (the
variable name) directly so you never slice past its length—update the fallback
in function nameFromDirent to return name instead of name[:len(de.Name)].
In `@Makefile`:
- Line 24: The cross-build target currently depends on "clean build compress"
but omits "build-darwin" and references a non-existent "compress" target; update
the cross-build dependency list to include build-darwin (so cross-build produces
darwin artifacts) and either add a well-defined compress target (e.g.,
packaging/compression step) or remove "compress" from the dependency list; edit
the Makefile entry for the cross-build target and verify the related targets
"build", "build-darwin", and "clean" are present and correctly implemented.
🧹 Nitpick comments (8)
router/router_server_ws.go (1)
78-78: Log level downgrade for websocket close failure looks intentional and consistent.This aligns with the similar changes in
router/websocket/listeners.go. However, a failed close could occasionally indicate a real issue (e.g., resource leak). Consider usingWarnas a middle ground so these don't get completely buried in production, while still not alarming on expected closes.router/websocket/listeners.go (1)
100-100: Consider keepingWarnfor websocket send failures.Unlike close errors (which are expected during shutdown), a failure to send an event over the websocket may indicate a genuine issue — broken pipe, serialization error, or resource exhaustion. Logging this at
Infolevel risks burying actionable signals in production.Warnwould be a more appropriate middle ground.Proposed change
- h.Logger().WithField("event", evt).WithField("error", err2).Info("failed to send event over server websocket") + h.Logger().WithField("event", evt).WithField("error", err2).Warn("failed to send event over server websocket")environment/settings.go (1)
120-123: Platform guard forBlkioWeightis reasonable but may be overly conservative.
BlkioWeightis a Linux cgroup feature, so guarding it makes sense for native macOS execution. However, note that Docker Desktop on macOS runs a Linux VM, and the Docker API may acceptBlkioWeightthere. If users running Wings on macOS with Docker Desktop need IO weight support, this guard would silently skip it.Consider adding a comment explaining the rationale (e.g., Darwin doesn't support cgroup blkio) so future maintainers understand the intent.
Suggested comment
+ // BlkioWeight is only supported on Linux (cgroup blkio controller). + // On macOS, Docker Desktop uses a Linux VM but may not expose this setting. if runtime.GOOS == "linux" { resources.BlkioWeight = l.IoWeight }Makefile (1)
29-29: Addtestto.PHONYdeclaration.The static analysis tool correctly flags that
testis missing from the.PHONYlist. Sincetestis a valid filename, not declaring it as phony could cause issues if a file namedtestexists.Proposed fix
-.PHONY: all build build-darwin compress clean +.PHONY: all build build-darwin cross-build compress clean test debug rmdebugmacos.md (1)
40-47: Docker socket path may vary by Docker Desktop version.The path
~/.docker/run/docker.sockis the default for recent Docker Desktop versions, but some installations use/var/run/docker.socknatively or$HOME/.docker/desktop/docker.sock. Consider adding a brief note to verify the actual socket path (e.g.,docker context inspectorls ~/.docker/run/docker.sock) before creating the symlink.config/config.go (1)
505-515: Darwin user resolution looks correct.The approach mirrors the existing rootless-mode logic (lines 525–534). Since macOS lacks
useradd/adduser, using the current user is the right call.Note: the
user.Current()→MustInt(u.Uid)→MustInt(u.Gid)pattern is now repeated in three places (darwin, distroless, rootless). Consider extracting a small helper likesetCurrentUser()to reduce duplication, but this is not blocking.♻️ Optional: extract helper to reduce duplication
+// setCurrentUser sets the system user configuration from the current OS user. +func setCurrentUser() error { + u, err := user.Current() + if err != nil { + return err + } + _config.System.Username = u.Username + _config.System.User.Uid = system.MustInt(u.Uid) + _config.System.User.Gid = system.MustInt(u.Gid) + return nil +}Then in
EnsurePelicanUser, the darwin, distroless, and rootless blocks could callsetCurrentUser()(with the distroless block keeping its env-var override logic separately).config/config_openat_linux.go (1)
15-40: Benign race on concurrent first calls toUseOpenat2().Two goroutines can both observe
openat2Setasfalseand enter the detection path concurrently. Because the syscall result is deterministic for a given kernel, the final cached value will be correct in practice, but the detection work (including theOpenat2syscall +Close) may execute more than once.A
sync.Oncewould eliminate the redundant work and make the intent clearer. Not a correctness bug given the deterministic nature of the check, so feel free to defer.internal/ufs/fs_linux.go (1)
58-72: Nit:Secfield in theUTIME_OMITtimespec.On Line 62, both
SecandNsecare set tounix.UTIME_OMIT. Perutimensat(2), only thetv_nsecfield is inspected for theUTIME_OMITsentinel —tv_secis ignored. This works correctly but is slightly misleading. Consider settingSec: 0for clarity.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
Makefileconfig/config.goconfig/config_openat_darwin.goconfig/config_openat_linux.goconfig/config_openat_linux_test.goconfig/config_openat_test.goenvironment/settings.gointernal/ufs/file.gointernal/ufs/file_darwin.gointernal/ufs/file_linux.gointernal/ufs/fs_darwin.gointernal/ufs/fs_linux.gointernal/ufs/fs_platform_test.gointernal/ufs/fs_unix.gointernal/ufs/fs_unix_test.gointernal/ufs/walk_darwin.gointernal/ufs/walk_linux.gointernal/ufs/walk_unix.gomacos.mdrouter/router_server_ws.gorouter/websocket/listeners.goserver/filesystem/filesystem_test.goserver/filesystem/stat_darwin.goserver/filesystem/stat_test.gosystem/system.go
🧰 Additional context used
🧬 Code graph analysis (11)
server/filesystem/stat_darwin.go (1)
server/filesystem/stat.go (1)
Stat(14-17)
internal/ufs/file_linux.go (1)
internal/ufs/file_darwin.go (1)
O_LARGEFILE(4-4)
internal/ufs/fs_platform_test.go (1)
server/filesystem/stat.go (1)
Stat(14-17)
config/config_openat_darwin.go (1)
config/config_openat_linux.go (1)
UseOpenat2(15-41)
config/config.go (2)
system/system.go (1)
System(60-67)system/utils.go (1)
MustInt(35-41)
config/config_openat_linux.go (3)
config/config_openat_darwin.go (1)
UseOpenat2(5-7)config/config.go (1)
Get(439-447)system/system.go (1)
System(60-67)
config/config_openat_test.go (3)
config/config.go (3)
Set(408-420)Configuration(320-379)SystemConfiguration(138-250)config/config_openat_darwin.go (1)
UseOpenat2(5-7)config/config_openat_linux.go (1)
UseOpenat2(15-41)
server/filesystem/stat_test.go (3)
server/filesystem/filesystem_test.go (1)
NewFs(20-55)server/filesystem/stat.go (1)
Stat(14-17)server/filesystem/errors.go (1)
Error(25-37)
internal/ufs/fs_unix.go (1)
internal/ufs/file.go (2)
O_DIRECTORY(169-169)O_RDONLY(152-152)
internal/ufs/fs_linux.go (3)
internal/ufs/fs_unix.go (1)
UnixFS(23-30)internal/ufs/file.go (1)
O_CLOEXEC(172-172)internal/ufs/file_linux.go (1)
O_LARGEFILE(5-5)
internal/ufs/fs_darwin.go (1)
internal/ufs/fs_unix.go (1)
UnixFS(23-30)
🪛 checkmake (0.2.2)
Makefile
[warning] 29-29: Missing required phony target "test"
(minphony)
🪛 LanguageTool
macos.md
[style] ~160-~160: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... not exist on Darwin. - unix.Getdents does not exist on Darwin; `unix.Getdirentrie...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[grammar] ~174-~174: Ensure spelling is correct
Context: ...at_linux.go| OriginalCTime()usingunix.Stat_t.Ctim(was//go:build linux) | | stat_darw...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~175-~175: Ensure spelling is correct
Context: ...at_darwin.go|CTime()handling bothunix.Stat_t.Ctimandsyscall.Stat_t.Ctimespec| gola...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (go)
- GitHub Check: Build and Test (ubuntu-22.04, 1.24.7, linux, amd64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.25.1, linux, amd64)
🔇 Additional comments (31)
server/filesystem/filesystem_test.go (1)
34-37: Good fix for macOS symlink resolution in test setup.This correctly handles the
/var→/private/varsymlink on macOS, preventing spuriousErrBadPathResolutionfailures in tests.router/websocket/listeners.go (1)
30-33: Log level changes for websocket lifecycle events are reasonable.These log during normal connection teardown, so
Infois appropriate.internal/ufs/file_linux.go (1)
1-5: Clean platform-specific constant definition.The Linux/Darwin split for
O_LARGEFILEis correct: Darwin universally supports large files (hence0), while Linux needs the explicit flag.internal/ufs/file.go (1)
172-173: No action needed. AT_EMPTY_PATH is not referenced anywhere in the codebase and is not defined in any platform-specific files, confirming it was safely removed and is not required for the current implementation.system/system.go (2)
115-129: LGTM — Darwin-specific OS detection looks correct.Clean branching: Darwin skips
osrelease.Read()(which would fail on macOS) and returns a sensible hardcoded value, while the Linux path retains the existing fallback chain.
209-218: LGTM — Early return avoids reading a non-existent file on macOS.server/filesystem/stat_darwin.go (1)
10-22: The comment and TODO are accurate —Ctim/Ctimespecis status-change time, not creation time.On macOS, the actual file creation (birth) time is available via
Birthtimespeconsyscall.Stat_t. Since the TODO already flags this for removal and the Linux counterpart has the same semantics, this is consistent. Just noting for awareness that if the intent ever shifts to truly returning creation time on Darwin,Birthtimespecwould be the correct field.server/filesystem/stat_test.go (2)
9-29: Test looks good but may be fragile ifCTime()returns zero time.On line 23, the test asserts
CTime()is non-zero, but the Darwin implementation falls back totime.Time{}if neitherunix.Stat_tnorsyscall.Stat_tis the underlying type. If a future Go or platform change alters theSys()return type, this test would fail with a non-obvious error. Consider a more descriptive message or loggingst.Sys()type on failure for easier debugging.That said, this is minor — the current implementations should always match one of the two branches.
31-71: LGTM — Good coverage of JSON marshaling fields.The test validates the key fields (
created,modified,name) and timestamp format. Clean and readable.macos.md (1)
1-203: Well-written documentation covering both user setup and developer context.The separation between user-facing setup instructions and developer-focused code change documentation is clear and useful. The explanations for why specific settings are needed (Docker Desktop file sharing paths,
machine_id.enable: false, etc.) are particularly helpful.internal/ufs/file_darwin.go (1)
1-4: LGTM — Correct no-op constant for Darwin.internal/ufs/fs_unix_test.go (1)
37-40: LGTM — Necessary symlink resolution for macOS test compatibility.Consistent with the same pattern applied in
server/filesystem/filesystem_test.go'sNewFs().internal/ufs/walk_darwin.go (1)
9-18: LGTM — Clean Darwin-specific dirent handling.Using
Namlendirectly is the correct approach for Darwin and avoids the NUL-scanning complexity required on Linux. Theunsafe.Sliceusage is idiomatic for this pattern.config/config_openat_test.go (1)
8-28: LGTM — reasonable cross-platform smoke test.Clean setup and the OS-specific assertions are correct: Darwin's
UseOpenat2()always returnsfalse, and Linux's result is kernel-dependent.One minor note: on Linux, if
TestUseOpenat2ConfigOverride(inconfig_openat_linux_test.go) runs first, the atomic cache (openat2Set) will already be populated, so this test won't actually exercise the"auto"detection path — it'll return the cached value. Since the stated goal is just "doesn't panic," that's fine, but worth being aware of if you ever want stronger coverage of the auto path in isolation.config/config_openat_darwin.go (1)
1-7: LGTM!Clean platform stub. The comment accurately explains why
openat2is unavailable on Darwin.config/config.go (1)
804-807: LGTM — preventsosrelease.Read()failure on macOS.
/etc/os-releasedoesn't exist on macOS, so this early return is necessary to avoid a runtime error.config/config_openat_linux_test.go (1)
5-25: LGTM — clean config override test.Properly resets the atomic cache before each assertion, ensuring the config mode is re-evaluated. Both forced
"openat"and"openat2"paths are covered.internal/ufs/fs_unix.go (3)
36-49: LGTM — symlink resolution in base path is essential for macOS.On macOS,
/varis a symlink to/private/var, so without this resolution, theunsafeIsPathInsideOfBaseprefix check would fail for any fd whose resolved path goes through/private/var. Silently ignoringEvalSymlinkserrors is reasonable here — the path may not exist yet during initialization.
682-683: Good:fdPath(fd)replaces the Linux-specific/proc/self/fdapproach.This is the correct abstraction for cross-platform fd-to-path resolution (Linux uses
/proc/self/fd, Darwin usesfcntl(F_GETPATH)).
762-762: Correct:AT_FDCWDis the right choice for opening an absolute path.Using
AT_FDCWDwith the absolutefs.basePathis semantically correct and portable across Unix platforms.internal/ufs/fs_platform_test.go (4)
15-53: LGTM — clean stat validation test.Covers the essential fields (name, size, directory flag, mod time) through the
UnixFSabstraction.
55-88: Good sandbox escape test.This exercises the critical security invariant that symlinks pointing outside the sandbox root are blocked. Using
os.Symlinkdirectly (bypassingUnixFS) correctly simulates an external actor creating the escape link.
90-130: Test depends on timing — consider a more robust assertion.The 50ms sleep + 100ms tolerance window is reasonable for CI but could be flaky on heavily loaded machines where the filesystem timestamp granularity is coarse (e.g., some filesystems have 1-second or 2-second granularity like HFS+, which is relevant for macOS).
If the test target filesystem on macOS uses APFS (nanosecond granularity), this should be fine. But if run on HFS+ volumes,
origMtimeand the post-Chtimesmtime could both round to the same second, making the test pass vacuously (not actually verifying preservation). Worth noting but not blocking.
132-145: LGTM — validates platform-specific constant.Correctly asserts
O_LARGEFILEis non-zero on Linux (fromunix.O_LARGEFILE) and zero on Darwin (where large file support is default).internal/ufs/walk_unix.go (1)
187-187: LGTM — clean delegation to platform-specificgetdentswrapper.This change properly delegates OS-specific directory entry reading to the
walk_linux.goandwalk_darwin.goimplementations, with bothgetdentsandnameFromDirentcorrectly defined in each platform file.config/config_openat_linux.go (1)
10-13: LGTM on the atomic caching variables.The two package-level atomic booleans cleanly separate the "is set" flag from the "value" flag.
internal/ufs/fs_linux.go (2)
13-14: LGTM — standard/proc/self/fdapproach for Linux.
23-54: LGTM — solidopenat2wrapper with proper flag defaults andRESOLVE_BENEATH.The
EINTR/EAGAINpass-through withoutensurePathErrorwrapping is correct, as callers will retry the operation.internal/ufs/fs_darwin.go (2)
15-27: LGTM — correct use ofF_GETPATHwithruntime.KeepAliveto prevent premature GC of the buffer.
32-34: LGTM — clean stub returningENOSYS.internal/ufs/walk_linux.go (1)
10-13: LGTM — thin wrapper overGetdents.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| func (fs *UnixFS) Chtimesat(dirfd int, name string, atime, mtime time.Time) error { | ||
| if atime.IsZero() || mtime.IsZero() { | ||
| var st unix.Stat_t | ||
| if err := unix.Fstatat(dirfd, name, &st, 0); err != nil { | ||
| return ensurePathError(err, "chtimes", name) | ||
| } | ||
| if atime.IsZero() { | ||
| atime = time.Unix(st.Atim.Unix()) | ||
| } | ||
| if mtime.IsZero() { | ||
| mtime = time.Unix(st.Mtim.Unix()) | ||
| } | ||
| } | ||
|
|
||
| utimes := [2]unix.Timespec{ | ||
| unix.NsecToTimespec(atime.UnixNano()), | ||
| unix.NsecToTimespec(mtime.UnixNano()), | ||
| } | ||
| return ensurePathError(unix.UtimesNanoAt(dirfd, name, utimes[0:], 0), "chtimes", name) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check what version of golang.org/x/sys/unix is being used
fd -t f "go.mod" -o "go.sum" | head -5 | xargs grep -E "golang.org/x/sys"Repository: pelican-dev/wings
Length of output: 194
🌐 Web query:
golang.org/x/sys/unix Stat_t Darwin Atimespec Mtimespec field names
💡 Result:
On Darwin (GOOS=darwin), golang.org/x/sys/unix.Stat_t uses the BSD-style timestamp fields:
Atimespec(access time)Mtimespec(modification time)Ctimespec(inode change time)Birthtimespec(creation/birth time) (git.linux.ucla.edu)
Each of those fields is a unix.Timespec, whose field names are:
Sec(seconds)Nsec(nanoseconds) (docs.activestate.com)
So you access them like: st.Atimespec.Sec, st.Atimespec.Nsec, st.Mtimespec.Sec, etc. (docs.activestate.com)
Citations:
- 1: https://git.linux.ucla.edu/uwaru/matterbridge/src/commit/7a161463049b72e29b00ddf3a99d0308c48d2d80/vendor/golang.org/x/sys/unix/ztypes_darwin_amd64.go?display=source&utm_source=openai
- 2: https://docs.activestate.com/activego/1.8/pkg/golang.org/x/sys/unix/?utm_source=openai
- 3: https://docs.activestate.com/activego/1.8/pkg/golang.org/x/sys/unix/?utm_source=openai
🏁 Script executed:
# Check the actual code in fs_darwin.go
cat -n internal/ufs/fs_darwin.go | sed -n '40,58p'Repository: pelican-dev/wings
Length of output: 781
🏁 Script executed:
# Also check if there's a go.mod to understand dependency versions
cat go.mod | grep -A 5 "require"Repository: pelican-dev/wings
Length of output: 826
Fix field names for Darwin Stat_t struct — Atim/Mtim don't exist on macOS.
On Darwin, unix.Stat_t uses Atimespec and Mtimespec (not Atim/Mtim as on Linux). Lines 47 and 50 reference nonexistent fields and will fail to compile on macOS.
Fix
if atime.IsZero() {
- atime = time.Unix(st.Atim.Unix())
+ atime = time.Unix(st.Atimespec.Unix())
}
if mtime.IsZero() {
- mtime = time.Unix(st.Mtim.Unix())
+ mtime = time.Unix(st.Mtimespec.Unix())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (fs *UnixFS) Chtimesat(dirfd int, name string, atime, mtime time.Time) error { | |
| if atime.IsZero() || mtime.IsZero() { | |
| var st unix.Stat_t | |
| if err := unix.Fstatat(dirfd, name, &st, 0); err != nil { | |
| return ensurePathError(err, "chtimes", name) | |
| } | |
| if atime.IsZero() { | |
| atime = time.Unix(st.Atim.Unix()) | |
| } | |
| if mtime.IsZero() { | |
| mtime = time.Unix(st.Mtim.Unix()) | |
| } | |
| } | |
| utimes := [2]unix.Timespec{ | |
| unix.NsecToTimespec(atime.UnixNano()), | |
| unix.NsecToTimespec(mtime.UnixNano()), | |
| } | |
| return ensurePathError(unix.UtimesNanoAt(dirfd, name, utimes[0:], 0), "chtimes", name) | |
| func (fs *UnixFS) Chtimesat(dirfd int, name string, atime, mtime time.Time) error { | |
| if atime.IsZero() || mtime.IsZero() { | |
| var st unix.Stat_t | |
| if err := unix.Fstatat(dirfd, name, &st, 0); err != nil { | |
| return ensurePathError(err, "chtimes", name) | |
| } | |
| if atime.IsZero() { | |
| atime = time.Unix(st.Atimespec.Unix()) | |
| } | |
| if mtime.IsZero() { | |
| mtime = time.Unix(st.Mtimespec.Unix()) | |
| } | |
| } | |
| utimes := [2]unix.Timespec{ | |
| unix.NsecToTimespec(atime.UnixNano()), | |
| unix.NsecToTimespec(mtime.UnixNano()), | |
| } | |
| return ensurePathError(unix.UtimesNanoAt(dirfd, name, utimes[0:], 0), "chtimes", name) |
🤖 Prompt for AI Agents
In `@internal/ufs/fs_darwin.go` around lines 40 - 58, The code in UnixFS.Chtimesat
uses linux-style Stat_t fields Atim/Mtim which don't exist on macOS; replace
references to st.Atim and st.Mtim with Darwin names st.Atimespec and
st.Mtimespec and convert their timespecs to time.Time (e.g., using
st.Atimespec.Unix()/UnixNano() or an equivalent conversion) before assigning
atime/mtime so the function compiles and uses the correct Darwin fields.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests