feat(windows): core I/O — file locking and path separators#171
feat(windows): core I/O — file locking and path separators#171JF10R wants to merge 6 commits intojustrach:mainfrom
Conversation
![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: 7a078963f7
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/watcher.zig
Outdated
| const rel = if (std.mem.startsWith(u8, path, root)) | ||
| std.mem.trimLeft(u8, path[root.len..], "/") | ||
| std.mem.trimLeft(u8, path[root.len..], "/\\") | ||
| else |
There was a problem hiding this comment.
drainNotifyFile now accepts Windows-style \ paths but forwards rel unchanged into indexFileContent. The downstream sensitive-file guard (shouldSkipFile) still parses basenames and sensitive directories using /-based checks (e.g. .ssh/), so notified paths like .ssh\id_rsa can bypass exclusion and get indexed/searched on Windows. This is a security regression for the new Windows notify flow and should be fixed by normalizing separators (or making shouldSkipFile separator-agnostic) before filtering/indexing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds core Windows I/O support needed for watcher/store functionality by addressing Windows path separator behavior, stack constraints, and cross-platform file operations/locking.
Changes:
- Introduces
compat.path_buf_sizeand updates multiple path buffers to use it on Windows. - Heap-allocates large data structures (
watcher.EventQueue, frequency-table working buffers) to avoid Windows stack overflow. - Improves Windows filesystem interoperability:
\-aware directory skipping,%TEMP%/%TMP%notify path,setEndPos()truncation, and Windows advisory locking viaNtLockFile/NtUnlockFile.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/watcher.zig | Heap-allocate event queue; add separator helper; make skip + notify draining Windows-aware; use setEndPos() |
| src/tests.zig | Update watcher queue tests to use EventQueue.init()/deinit() |
| src/store.zig | Add Windows data.log locking using NtLockFile/NtUnlockFile; use flock elsewhere |
| src/main.zig | Switch to noinline mainImpl; use compat.path_buf_size; pass queue by pointer after new init |
| src/index.zig | Move large frequency-table buffers to heap; adjust finishFrequencyTable to write into provided output |
| src/compat.zig | Add path_buf_size to cap Windows path buffers for stack safety |
| build.zig | Set Windows executable stack size to 8MB |
Comments suppressed due to low confidence (1)
src/main.zig:548
- resolveRoot now takes a *[compat.path_buf_size]u8 buffer, which caps resolvable absolute root path length to 1024 bytes on Windows. If the intent is only to avoid huge stack buffers in hot/replicated structs (like FsEvent/EventQueue), consider leaving resolveRoot on std.fs.max_path_bytes (or using a dynamically sized buffer) so long Windows roots remain supported.
fn resolveRoot(root: []const u8, buf: *[compat.path_buf_size]u8) ![]const u8 {
if (std.mem.eql(u8, root, ".")) {
return std.fs.cwd().realpath(".", buf) catch return error.ResolveFailed;
}
return std.fs.cwd().realpath(root, buf) catch return error.ResolveFailed;
src/store.zig
Outdated
| if (comptime @import("builtin").os.tag == .windows) { | ||
| const ntdll = std.os.windows.ntdll; | ||
| const max_off: i64 = std.math.maxInt(i64); | ||
| const zero_off: i64 = 0; | ||
| var iosb: std.os.windows.IO_STATUS_BLOCK = undefined; | ||
| _ = ntdll.NtLockFile(log.handle, null, null, null, &iosb, &zero_off, &max_off, null, 0, 1); | ||
| defer { | ||
| var iosb2: std.os.windows.IO_STATUS_BLOCK = undefined; | ||
| _ = ntdll.NtUnlockFile(log.handle, &iosb2, &zero_off, &max_off, null); | ||
| } | ||
| } else { | ||
| const fd = log.handle; | ||
| _ = std.posix.flock(fd, std.posix.LOCK.EX) catch {}; | ||
| defer _ = std.posix.flock(fd, std.posix.LOCK.UN) catch {}; | ||
| } |
There was a problem hiding this comment.
The Windows branch calls NtLockFile/NtUnlockFile but ignores the returned NTSTATUS and IO_STATUS_BLOCK.Status. If the lock fails (or completes asynchronously), the code will still proceed to write under the assumption that cross-process safety was achieved. Consider checking the status (and only unlocking when the lock succeeded), and decide whether to fail the write or fall back to an unlocked write with an explicit warning when locking is unavailable.
| } | ||
|
|
||
| var root_buf: [std.fs.max_path_bytes]u8 = undefined; | ||
| var root_buf: [compat.path_buf_size]u8 = undefined; |
There was a problem hiding this comment.
Using compat.path_buf_size (1024 on Windows) for the realpath buffer means roots with absolute paths longer than 1024 bytes will now fail to resolve on Windows, even though std.fs.max_path_bytes supports much longer paths there. Since this buffer isn’t multiplied into large stack allocations like FsEvent/EventQueue, consider keeping std.fs.max_path_bytes here (or using a heap allocation on Windows) to avoid unnecessarily limiting valid project roots.
| var root_buf: [compat.path_buf_size]u8 = undefined; | |
| var root_buf: [std.fs.max_path_bytes]u8 = undefined; |
JF10R
force-pushed
the
windows/core-io
branch
2 times, most recently
from
ffa3705 to
ef0ef34
Compare
Addressing reviewer feedback✅ Fixed:
|
|
Hey @JF10R — thanks for the Windows work! Before we can review and merge, could you please:
We'll get Codex to do a review pass once rebased. Thanks! |
justrach
left a comment
justrach
left a comment
There was a problem hiding this comment.
Requesting rebase onto current main and CONTRIBUTING.md compliance before review. See comment above.
Add Windows platform support foundations that all subsequent Windows PRs depend on. No behavioral change on Linux/macOS. - compat.zig: add path_buf_size (1024 on Windows vs max_path_bytes) - build.zig: set 8MB stack for Windows (matches Linux default) - main.zig: replace thread trampoline with noinline mainImpl to avoid LLVM frame merging (~128MB) that exceeds Windows guard page stride - main.zig: fix double scan_thread.join() (UB on all platforms) - watcher.zig: heap-allocate EventQueue (avoids ~4MB stack array) - watcher.zig: use compat.path_buf_size in FsEvent - index.zig: heap-allocate frequency table buffers (~640KB each) - tests.zig: update EventQueue construction to use init()/deinit()
Upstream issue-148 added stdin HUP detection via std.posix.poll() and pipe tests. These use POSIX APIs unavailable on Windows. Guard the poll block with comptime os check (Windows uses idle timeout only) and skip pipe tests on Windows.
Upstream's mmap trigram index (justrach#167) uses std.posix.mmap/munmap which are unavailable on Windows. Guard initFromDisk to return null on Windows (falling back to heap-based index) and skip mmap-dependent tests.
Platform-specific I/O changes for Windows file system operations. - store.zig: use NtLockFile/NtUnlockFile on Windows (POSIX flock on others) - watcher.zig: add isSep() to handle both / and \ path separators - watcher.zig: backslash-aware shouldSkip() for Windows paths - watcher.zig: use %TEMP%/%TMP% for codedb-notify path on Windows - watcher.zig: use setEndPos() instead of ftruncate (cross-platform) - watcher.zig: trim both / and \ when making paths relative to root
- store.zig: check NtLockFile return status; only unlock if lock succeeded; skip write-under-lock if lock fails (matches upstream's advisory lock pattern) - watcher.zig: normalize backslash paths to forward slashes in drainNotifyFile to avoid duplicate entries in the explorer (walker uses / convention, notify file may contain \ on Windows)
|
Rebased onto current
|
Closes #182
Depends on #170
Summary
Adds Windows-compatible file I/O: advisory locking via NT APIs, backslash-aware path handling, and cross-platform file truncation.
Changes
src/store.zigNtLockFile/NtUnlockFileadvisory locking on Windows (POSIXflockon others); checkNTSTATUSbefore unlockingsrc/watcher.zigisSep()helper for both/and\; backslash-awareshouldSkip();%TEMP%/%TMP%notify path on Windows; backslash-to-forward-slash normalization indrainNotifyFile; cross-platformsetEndPos()replacingftruncateTests
Same pass rate as foundation PR — no regressions.
Before/After
flock()calls fail on Windows (POSIX-only). Paths with\separators bypassshouldSkip(). Notify file path hardcodes/tmp/.NtLockFileon Windows. All path operations handle both separators. Notify file uses%TEMP%/%TMP%on Windows.Compliance
main(d7e3bfb), branched from feat(windows): foundation — build system, stack safety, heap allocation #170