Windows compatibility for dbsp and dependencies by vpopescu · Pull Request #6627 · feldera/feldera

vpopescu · 2026-07-13T19:20:13Z

This makes dbsp and its supporting crates compatible with Windows.

Changes

Target-gate Unix-only dependencies, APIs, and benchmark profiling; add the required Windows bindings.
Add Windows thread CPU-time collection via GetThreadTimes, with a monotonicity test that also exercises Linux.
Implement positional storage reads on Windows with overlapped I/O, preserving the caller's file cursor and retrying interrupted reads.
Make metadata access, directory durability, and directory locking portable across Unix and Windows.
Add regression coverage for positional reads and Windows directory locking.
Pin Rust to 1.96.1 to avoid [ICE]: could not replace AliasTerm rust-lang/rust#155035.
Incorporate review feedback: reuse the Windows read handle, factor the exact-read loop, avoid an unnecessary clone, and remove stray commented-out test markup.

Manual Test Plan

Ran the test suite on Windows and Linux. The pre-existing operator::dynamic::recursive::test::issue4168 failure also occurs on main.
No macOS environment was available for testing.

Breaking Changes

None.

mihaibudiu

I think @blp will also have to take a look, since he wrote most of the code that is being updated. But I am not sure how familiar he is with the Windows APIs.

mihaibudiu · 2026-07-13T21:31:01Z

+}
+
+#[cfg(windows)]
+fn current_thread_cpu_time() -> Duration {


It sure looks hard to read a timer in Windows.

It's really just an extra step conversion step.

mihaibudiu · 2026-07-13T21:34:35Z

+            user.as_mut_ptr(),
+        )
+    };
+    assert_ne!(


assertions are unusual in non-test code, should this be based on debug_assert?

You are right, I kinda forgot about that, will fix it, but the best I can do is throw a panic! instead of an assert, since I didn't track the code up. But I am not confident enough yet to return a "0" like you suggest in the other comment.

That said, failure cases are very rare (e.g. GetCurrentThread() resulting in an invalid thread, or not having thread query rights.

mihaibudiu · 2026-07-13T21:35:05Z

+}
+
+#[cfg(not(any(unix, windows)))]
+fn current_thread_cpu_time() -> Duration {


If this is safe, perhaps it's safe to return 0 in windows if you can't read the timers as well
Alternatively, this can be unimplemented!()

Unimplemented!() would just rob us of a feature that's unlikely to fail in normal operation. I can certainly return "0" but I have not tracked up the code to see how that is used.

Looking further, I can return a 0, but will cause incorrect profiling results, so I'm not sure how you feel about that. I traced it a bit further, and we could do it, but if I missed something and it's used in some kind of math at some point, it may trigger additional issues.

A better fix may be to return an Option<> so we can indicate to the caller that the result is invalid, but this means more changes downstream. Another option would be to consider '0' as invalid, but a newly created thread can legitimately report a time of 0.

I'd leave it as is since it only affects windows, and the likelihood of failure is not high, and if panic!() ever gets triggered, It's really about the same failure chance as the clock_gettime() in clock_gettime(),

Eh, i asked the all known AI:

GetCurrentThread() Cannot Fail
The comment speculates that GetCurrentThread() might return a "bad handle," but this is impossible by design.

GetCurrentThread() does not open a real handle or allocate OS resources. It simply returns a hardcoded pseudo-handle (which is always the constant value (HANDLE)-2).

Because it is a pseudo-handle, it is guaranteed to be valid for the calling thread and inherently possesses the THREAD_QUERY_INFORMATION access rights required by GetThreadTimes. It cannot be closed by mistake, and it cannot be "bad."

The Pointers are Guaranteed to be Valid
The API requires valid memory addresses to write the time data into. Assuming creation, exit, kernel, and user are standard Rust local variables (like MaybeUninit), calling as_mut_ptr() on them yields valid, non-null pointers to your thread's stack.

The Windows kernel checks if it can write to these pointers. Since they point to memory safely managed by Rust's compiler, the memory is guaranteed to be allocated and writable.

When Could It Theoretically Fail?
The only way GetThreadTimes returns 0 in this exact snippet is if your program has already suffered catastrophic Undefined Behavior (UB) elsewhere.

Ok, leave it as it is then. Thank you.

Then panic is the right choice

mihaibudiu · 2026-07-13T21:36:28Z

+        .encode_wide()
+        .chain(once(0))
+        .collect::<Vec<_>>();
+    let handle = unsafe {


I trust you on this one

mihaibudiu · 2026-07-13T21:44:36Z

+
+#[cfg(windows)]
+fn get_lock(_file: &File) -> Result<Option<u32>, IoError> {
+    Ok(None)


why is this enough?

On windows the api doesn't tell us which process owns the lock, it just returns ERROR_LOCK_VIOLATION. SO it's like "yeah, someone has a lock, I don't know who". We could make it more explicit by changing the signature of the function on windows, but that means we have to deal with it at the call site.

mihaibudiu · 2026-07-13T21:46:25Z

+        {
+            use std::sync::OnceLock;
+
+            static ORIGIN: OnceLock<Instant> = OnceLock::new();


This variant looks portable to me, could it be the only one?

Not following, can you clarify?

why is this code windows only? Couldn't this work for unix as well?

I try to preserve the existing Unix behavior: on Unix, timestamps remain raw CLOCK_MONOTONIC values so they match samply’s profiler timestamps. In win we don't have a portable raw-clock API, so it uses elapsed time from a process-local Instant origin instead. So it's a matter of making it work on Windows without touching the linux path too much.

Does it do what samply expects on Windows? That is the important thing.

mihaibudiu · 2026-07-13T21:47:57Z

 }

+#[cfg(unix)]
+fn read_at(file: &File, buffer: &mut [u8], offset: u64) -> Result<usize, IoError> {


it's not obvious to me that this is equivalent with the original code

I will update this to be more "faithful" to the original

mythical-fred

Draft-level high-level feedback only.

Overall direction is sound: introduce portable helpers (current_thread_cpu_time, fsync_dir, read_at, lock_id) and gate raw syscalls behind cfg(unix) / cfg(windows). That's the right factoring for a Windows port and keeps the Unix path unchanged.

A few high-level things worth thinking about before this comes out of draft:

read_at on Windows calls ReOpenFile on every read. That's a full kernel handle allocation per pread. FBuf::read_exact_at is called in hot paths (spine reads, checkpoint restore); a per-call ReOpenFile will show up in profiles. The usual pattern is to open the file once with FILE_FLAG_OVERLAPPED at construction time and reuse that handle, or to keep two handles per File (sync + overlapped). Worth measuring before landing.
ThreadCpuTime::now() panics if GetThreadTimes fails. That matches the Unix side (clock_gettime().unwrap()), so it's consistent. Just noting for the record: on Windows GetThreadTimes can theoretically fail on odd thread states. If you want to soften it, return Duration::ZERO and warn! once; otherwise this is fine.
get_lock returns Ok(None) on Windows — that removes the "held by pid X" diagnostic and the pid-comparison branch in the retry loop. The lock itself still works (via LockFileEx failing), but the "waiting on pid X" log message no longer fires on Windows. Fine for a first cut; worth documenting in the function.
lock_id on Windows uses path.canonicalize(). Two paths that canonicalize differently but refer to the same file (junction points, \\?\ prefix vs drive-letter, 8.3 short names) would bypass the in-process lock set. Not a soundness bug — LockFileEx still protects across processes — but you'd hit inconsistent behavior in-process. GetFileInformationByHandle (VolumeSerialNumber + FileIndex) is the canonical dev/ino equivalent on Windows and would be a closer analogue.
Doc comment shuffle in ThreadCpuTime. The /// block above elapsed() now reads:

Returns the current thread's cumulative CPU time.
Returns the time elapsed running the current thread since this ThreadCpuTime.
Looks like a stray line landed on the wrong function. now() is the one that returns cumulative time; elapsed() returns the delta. Move the new sentence up to now() or drop it.
thread_cpu_time_is_monotonic test is #[cfg(windows)]-only. Monotonicity is a property of both platforms; running it everywhere gives you cheap regression coverage on the Unix path too.
Rust toolchain pin at 1.94.1 — you mention this in the body. Whatever value lands should be justified in a comment in rust-toolchain.toml (or wherever it goes) so nobody bumps it back and breaks Windows. If the newer compiler bug is filed upstream, link the issue.
PR is 5 commits all titled "Windows compatibility" / "windows compatibility update". When you take this out of draft please squash / rewrite the commit messages per the repo's commit-message rules (imperative mood, scoped prefix, describe what and why). No AI attribution trailers.

Not blocking anything while this stays a draft. I'll do a full pass once it's marked ready.

vpopescu · 2026-07-14T03:08:30Z

#1: "Worth measuring before landing." - How would I go about that? I'm very new to the product, I am just preparing it for my first use, so I never actually ran it, I rely on the tests suite. This is also why i am hesitant to affect the linux path.
#2 - will leave as is for now, I expect the panic!() will never happen
#3 - I added a documentation comment to the function
#4 - done. not sure if I fully understood your concern, so please take a look
#5 - fixed, I suspect I got confused during merging
#6 - fixed, now runs on linux too, and passes
#7 - Seems like 1.96.1 works, and 1.97.0 (latest) crashes, so I will pin to 1.96.1
#8 - will do. is it one commit per crate, or a single commit for the entire monorepo?

I will squash and take it out of draft once we clarify #1

mihaibudiu · 2026-07-14T03:10:06Z

Fred is a bot, he's not always right. This is in the scope of one commit.

mythical-fred

Windows port now compiles cleanly, addresses most of my prior draft feedback (lock_id via GetFileInformationByHandle, ThreadCpuTime docs, toolchain pin with linked upstream bug, hard-link test), and the diff is well-scoped: platform gates isolate the Windows-only code and the Unix path is unchanged. d29c6a38 has a real commit message and no AI-tool trailer — good.

One real concern remains, plus a few nits.

Perf smell (not a blocker): `read_at` reopens the file for every pread

read_at on Windows calls ReOpenFile with FILE_FLAG_OVERLAPPED for every positional read, then relies on Drop for File::from_raw_handle to close the handle. That is one kernel-object allocation, one path re-resolution, and one close per 4 KiB read in storage hot paths — this is the code path behind PosixReader::read_block / FBuf::read_exact_at. On Unix pread is a single syscall; the Windows version is at least three, plus a handle churn the storage backend will hit millions of times.

Because Feldera doesn't claim production support on Windows yet, I don't want to block the port on this. But please file a follow-up issue to open the file once with FILE_FLAG_OVERLAPPED (either at reader-open time or lazily-cached per PosixReader) and reuse that handle across read_at calls; per-call ReOpenFile will make storage-heavy benchmarks look terrible on Windows and mask real regressions. Long term you probably also want to switch PosixReader to a single overlapped handle on Windows and use it for both length and reads.

Nits

crates/dbsp/src/circuit/circuit_builder.rs — the stray //#[cfg(windows)] above thread_cpu_time_is_monotonic should just be deleted; the test is universal and the commented-out attribute reads as leftover scaffolding.
crates/dbsp/src/storage/dirlock.rs — LockId is (u32, u64) on Windows, i.e. Copy; lock_id.clone() on the locks.insert(...) line is a small stylistic dead-op. Not important, but drop it while you're in there.
crates/dbsp/src/storage/backend/posixio_impl.rs — the #[cfg(windows)] { return Ok(()); } #[cfg(unix)] self.file.sync_all() pattern in FileCommitter::commit compiles but reads oddly; a plain #[cfg(unix)] fn commit(...) / #[cfg(windows)] fn commit(...) split would be cleaner and mirror what you already do for fsync_dir, write_lock, lock_id. Style-only.
read_at on Windows: after the GetOverlappedResult fallback fails, you call IoError::last_os_error() a second time. That's fine here because nothing between the two calls resets GetLastError, but a let error = IoError::last_os_error(); earlier and returning Err(error) reads more clearly.
rust-toolchain.toml — good that you added the linked bug reference in the comment. Please leave a TODO/tracking issue link so this pin doesn't rot; a 1.96.1 pin will surface as CI drift the next time someone bumps toolchains.

What I checked

Every raw Windows syscall is behind unsafe with a documented invariant nearby; ReadFile / ReOpenFile / LockFileEx / CreateFileW / GetFileInformationByHandle all have their argument shapes right, and error paths use IoError::last_os_error() from the same thread that made the failing call.
LockedDirectory on Windows tracks the same-process lock set via (dwVolumeSerialNumber, FileIndex) — the hard-link test proves this is stable; the locks.contains(&lock_id) pre-check before write_lock is correct because LockFileEx on the same process's second handle would otherwise succeed silently.
FBuf::read_exact_at reserve/advance accounting is unchanged from the Unix version; the new read_at shim doesn't move the file cursor on either platform (FileExt::read_at on Unix, OVERLAPPED offset on Windows) — the new read_exact_at_reads_from_offset_without_seeking_file test pins that invariant.
ThreadCpuTime conversion on Windows: FILETIME is in 100 ns units; Duration::new(ticks / 10_000_000, ((ticks % 10_000_000) * 100) as u32) is correct (the multiplication is at most 10_000_000 * 100 = 10^9, within u32), and saturating_add on kernel + user guards against pathological long-lived threads.
samply timestamps switch to a process-local Instant-anchored monotonic clock on Windows and std::process::id() for the PID slot — both reasonable.
Commit message on d29c6a38 is descriptive; no AI-tool trailer. Merge/rebase noise (Merge remote-tracking branch 'upstream/main', etc.) will disappear under a squash merge; if the repo uses rebase-merge, please squash the 12-commit series into d29c6a38 before merging.

Approve.

mythical-fred · 2026-07-14T19:03:17Z

+    };
+
+    let handle = unsafe {
+        ReOpenFile(


Per-read ReOpenFile here is the main perf smell of the port: every positional read in the storage backend (PosixReader::read_block -> FBuf::read_exact_at -> this) opens a fresh overlapped handle, then closes it via File::from_raw_handle's Drop. On Unix pread is one syscall; here it is ReOpenFile + ReadFile + (sometimes) GetOverlappedResult + CloseHandle, plus a path re-resolution. Please file a follow-up to open the file once with FILE_FLAG_OVERLAPPED at reader-construction time (or lazily-cache it on PosixReader) and reuse the handle across read_at calls; not blocking this PR, but storage-heavy Windows benchmarks will look terrible otherwise.

mythical-fred

The posixio_impl.rs change — caching one overlapped handle per PosixReader — is exactly the fix I was hoping for; the storage hot path no longer pays a ReOpenFile + handle-close per block read. Nice work.

Squash before merge (must-do, not blocking approval). The branch is 11 commits, including two upstream merges and several "Windows compatibility" / "Addressing fred-bot comments" intermediates. d29c6a38 is a good model of the final message shape — please git rebase -i main and land this as one (or at most a handful of) self-contained commit(s). Feldera keeps a linear history and every entry must be worthy of it. Reference: https://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html.

Other findings inline. Approving on the assumption the squash and the cleanup nits are handled before merge.

mythical-fred · 2026-07-15T16:04:25Z

    use anyhow::anyhow;
    use std::{cell::RefCell, ops::Deref, rc::Rc, vec::Vec};

+    //#[cfg(windows)]


Nit (leftover from prior review): this //#[cfg(windows)] line is dead scaffolding now that the test runs unconditionally. Please drop it before the final squash — a stray commented-out attribute above a #[test] reads like the test is meant to be gated but isn't.

mythical-fred · 2026-07-15T16:04:25Z

+    let handle = unsafe {
+        ReOpenFile(
+            file.as_raw_handle(),
+            FILE_GENERIC_READ,
+            FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
+            FILE_FLAG_OVERLAPPED,
+        )
+    };


Soft ask, not a blocker: per our unsafe-Rust convention, every unsafe block should carry a // SAFETY: comment explaining what invariant makes the call sound (e.g. "ReOpenFile accepts any valid handle; file outlives the call and we do not aliasingly share the returned handle before wrapping it in File"). I convinced myself of correctness for each syscall block in this diff (arg shapes, lifetimes of the buffer/OVERLAPPED, File::from_raw_handle transferring ownership, CloseHandle on the success path via Drop, slice::from_raw_parts_mut staying within the reserved capacity), but a couple of lines of SAFETY prose next to each unsafe { ... } would make the next reader's job much easier. Happy to see this addressed either here (before squash) or in a follow-up cleanup PR.

mythical-fred · 2026-07-15T16:04:25Z

+        #[cfg(windows)]
+        {
+            // PosixWriter syncs the file before renaming it into its immutable path.
+            // Windows FlushFileBuffers rejects this reader's read-only handle.
+            return Ok(());
+        }
+
+        #[cfg(unix)]
        self.file
            .sync_all()
            .map_err(|e| StorageError::stdio(e.kind(), "fsync", self.drop.path.display()))


Nit: this commit() body would read more cleanly as two #[cfg]-gated function twins (commit_windows / commit_unix) or a single #[cfg] on the whole impl block, rather than an inline #[cfg(windows)] { return Ok(()); } #[cfg(unix)] self.file.sync_all()... pattern. The current shape works but reads as if control flow depends on runtime, when it's actually compile-time exclusive. Also, please expand the inline comment to explain why Windows can skip the sync (writer already flushed pre-rename; FlushFileBuffers rejects read-only handles) — future readers will thank you.

blp · 2026-07-15T21:31:00Z

+#[cfg(unix)]
+fn reader_file(file: &Arc<File>) -> Result<Arc<File>, IoError> {
+    Ok(file.clone())
+}
+
+#[cfg(windows)]
+fn reader_file(file: &Arc<File>) -> Result<Arc<File>, IoError> {
+    use std::os::windows::io::{AsRawHandle, FromRawHandle};
+    use windows_sys::Win32::{
+        Foundation::INVALID_HANDLE_VALUE,
+        Storage::FileSystem::{
+            FILE_FLAG_OVERLAPPED, FILE_GENERIC_READ, FILE_SHARE_DELETE, FILE_SHARE_READ,
+            FILE_SHARE_WRITE, ReOpenFile,
+        },
+    };
+
+    let handle = unsafe {
+        ReOpenFile(
+            file.as_raw_handle(),
+            FILE_GENERIC_READ,
+            FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
+            FILE_FLAG_OVERLAPPED,
+        )
+    };
+    if handle == INVALID_HANDLE_VALUE {
+        Err(IoError::last_os_error())
+    } else {
+        Ok(Arc::new(unsafe { File::from_raw_handle(handle) }))
+    }
+}


Why does Windows need to clone the file handle?

blp · 2026-07-15T21:39:20Z

+#[cfg(unix)]
+type LockId = (u64, u64);
+
+#[cfg(windows)]
+type LockId = (u32, u64);


Probably best to make this a real type instead of an alias.

blp · 2026-07-15T21:42:27Z

+    let handle = unsafe {
+        ReOpenFile(
+            file.as_raw_handle(),
+            FILE_GENERIC_READ,
+            FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
+            FILE_FLAG_OVERLAPPED,
+        )
+    };
+    if handle == INVALID_HANDLE_VALUE {
+        return Err(IoError::last_os_error());
+    }
+    let file = unsafe { File::from_raw_handle(handle) };


This looks super-slow, why can't Windows read from a file handle without copying it?

blp · 2026-07-15T21:44:33Z

It appears that this file will make Feldera only with exactly rustfmt 1.96.1 and clippy 1.96.1. That doesn't make sense to me, I've got 1.93.1 here at the moment and they're fine.

blp · 2026-07-15T21:50:07Z

This would be better as a PR with a series of commits, each of which fixes one incompatibility and each of which has a commit message that explains the change.

vpopescu · 2026-07-16T06:04:15Z

Although I agree with you, and that's what I was trying to do initially, i was told to squash everything down. I am not a git expert, and it really test my patience, especially since this repo moves pretty fast.

Best I can do is close the PR and reopen it later when I want to deal with git again, as long as you all agree how you want it. I've reached my fill of dealing with git for july. It's not a blocker for me since I can work out of my fork.

blp · 2026-07-16T17:06:11Z

Although I agree with you, and that's what I was trying to do initially, i was told to squash everything down. I am not a git expert, and it really test my patience, especially since this repo moves pretty fast.

Best I can do is close the PR and reopen it later when I want to deal with git again, as long as you all agree how you want it. I've reached my fill of dealing with git for july. It's not a blocker for me since I can work out of my fork.

Who told you to squash it? It's a weird thing to ask for. It's always clearer to have each logically separate change as a separate commit.

Maybe it's this comment:

PR is 5 commits all titled "Windows compatibility" / "windows compatibility update". When you take this out of draft please squash / rewrite the commit messages...

That's a different issue. If all the commit messages are the same, then it's not very useful to have multiple commits. The lack of rationale or commit messages is the real problem for me. Why does Windows need so much file handle cloning, for example? Maybe there is some reason--I am not a Windows person--but it is hard to believe that Windows needs a file handle clone per read or write. If it does, then the change should be mentioned in the commit message and explained in a comment.

vpopescu · 2026-07-16T20:56:42Z

Will redo this in a couple of weeks when I get more time.

vpopescu mentioned this pull request Jul 13, 2026

[question] DBSP crate - Windows support? #6622

Open

mihaibudiu reviewed Jul 13, 2026

View reviewed changes

mythical-fred reviewed Jul 14, 2026

View reviewed changes

vpopescu marked this pull request as ready for review July 14, 2026 17:55

mythical-fred approved these changes Jul 14, 2026

View reviewed changes

mihaibudiu requested a review from blp July 14, 2026 23:13

mythical-fred approved these changes Jul 15, 2026

View reviewed changes

Windows compatibility for dbsp and dependencies

7392be2

vpopescu force-pushed the main branch from 0673a5d to 7392be2 Compare July 15, 2026 20:07

blp reviewed Jul 15, 2026

View reviewed changes

vpopescu closed this Jul 16, 2026

mythical-fred mentioned this pull request Jul 17, 2026

[dbsp] Windows compatibility #6671

Open

12 tasks

Uh oh!

Conversation

vpopescu commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Manual Test Plan

Breaking Changes

Uh oh!

mihaibudiu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vpopescu Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mythical-fred left a comment

Choose a reason for hiding this comment

Uh oh!

vpopescu commented Jul 14, 2026

Uh oh!

mihaibudiu commented Jul 14, 2026

Uh oh!

mythical-fred left a comment

Choose a reason for hiding this comment

Perf smell (not a blocker): read_at reopens the file for every pread

Nits

What I checked

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mythical-fred left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vpopescu commented Jul 13, 2026 •

edited

Loading

vpopescu Jul 13, 2026 •

edited

Loading

Perf smell (not a blocker): `read_at` reopens the file for every pread

vpopescu commented Jul 16, 2026 •

edited

Loading