Skip to content

fix(windows): resolve POSIX-only compatibility issues (#92, #95) - #98

Open
Harshitmishra001 wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
Harshitmishra001:fix-windows-compatibility
Open

fix(windows): resolve POSIX-only compatibility issues (#92, #95)#98
Harshitmishra001 wants to merge 1 commit into
NVIDIA-NeMo:mainfrom
Harshitmishra001:fix-windows-compatibility

Conversation

@Harshitmishra001

Copy link
Copy Markdown

What does this PR do?

Resolves unconditional POSIX imports and asserts across the codebase that crash the framework on Windows.

  • Add platform guards for sandbox resource limits and executor death classification

  • Handle missing fcntl and fallback to SQLite native locking

  • Provide graceful fallbacks for resource memory limits in eval pipeline

  • Prevent asyncio subprocess failure on missing start_new_session/pass_fds args

  • Add regression tests for sandbox guards and executor

Related issues

Fixes #92
Fixes #95

Checklist

  • Code follows the project style (uv run ruff check . and uv run ruff format --check . pass)
  • Tests added/updated and passing (uv run pytest)
  • Docs updated if behavior or public APIs changed
  • New source files carry an SPDX license header

…NVIDIA-NeMo#95)

Resolves unconditional POSIX imports and asserts across the codebase that crash the framework on Windows.

- Add platform guards for sandbox resource limits and executor death classification

- Handle missing fcntl and fallback to SQLite native locking

- Provide graceful fallbacks for resource memory limits in eval pipeline

- Prevent asyncio subprocess failure on missing start_new_session/pass_fds args

- Add regression tests for sandbox guards and executor

Signed-off-by: Harshitmishra001 <hmharsh123@gmail.com>
@Harshitmishra001
Harshitmishra001 force-pushed the fix-windows-compatibility branch from e30a408 to 32c0037 Compare August 5, 2026 10:54
@Hotragn

Hotragn commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Nice — this is broader than #92 and picks up both sites I flagged in #95 that weren't in any issue. Confirming they're addressed correctly:

  • _bash_session.py:173 — gating pass_fds / start_new_session behind sys.platform != "win32" is the right fix, and it's the blocker that actually stops run() on Windows (the os.killpg calls further down were never reached). The hasattr(os, "killpg") fallbacks to proc.kill() / proc.terminate() cover the teardown paths too.
  • _memory_monitor.py — guarded.
  • debug_handler.pyhasattr(signal, "SIGUSR2") matches the convention already used at nooa_cli/commands/eval.py:47, which should make it uncontroversial in review.

Two things worth changing before this lands.

1. The fcntl fallback silently removes session mutual exclusion

try:
    import fcntl
except ImportError:
    fcntl = None
...
if fcntl is not None:
    fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB)

On Windows fcntl is None, so no lock is taken at all. That doesn't degrade the guard — it removes it. _acquire_session_lock's only job is to fail:

Acquire an exclusive flock on lock_path... Raises SessionAlreadyActiveError, annotated with the recorded owner PID so the caller can tell the user exactly which process is holding the session.

With the flock skipped, SessionAlreadyActiveError becomes unreachable on Windows and two agents can both open the same session and write the same SQLite store, with the mechanism that exists to prevent exactly that quietly doing nothing. Measured on native Windows 11 (ARM64, Python 3.14) — two handles on one lock file, first the #98 path, then the msvcrt equivalent from #84:

#98 path:    holder 1 ACQUIRED
#98 path:    holder 2 ACQUIRED        -> 2 concurrent holders
msvcrt path: holder 1 ACQUIRED
msvcrt path: holder 2 refused (OSError: Permission denied)  -> 1 holder

(Two handles in one process rather than two processes — Windows byte-range locks are per-handle, so it's a valid check, and for the #98 path it makes no difference since no lock is requested either way.)

The msvcrt form from #84 is close to a drop-in, because it raises OSError, which is already the exception your except OSError: arm catches to build SessionAlreadyActiveError with the owner PID:

os.lseek(fd, 0, os.SEEK_SET)
msvcrt.locking(fd, msvcrt.LK_NBLCK, 1)     # acquire
os.lseek(fd, 0, os.SEEK_SET)
msvcrt.locking(fd, msvcrt.LK_UNLCK, 1)     # release

The lseek is load-bearing rather than defensive: msvcrt.locking locks a range at the current file position, so without it a lock taken after any read covers a different byte than the unlock. Windows also drops byte-range locks at process exit, so the "a genuine crash doesn't wedge the next run" property in the docstring still holds.

Also, the PR description says "fallback to SQLite native locking" — I don't see SQLite-level locking in the diff, just the skipped flock. Worth either implementing the msvcrt path or adjusting the description, since a reviewer reading the summary would reasonably assume the guarantee survives.

2. "Fixes #95" — this fixes the bugs, not the gate

#95 is specifically about CI running only on ubuntu-latest, so nothing catches this class before it ships. This PR touches no .github/ files, so if it closes #95 the regression guard goes away with it and the next unconditional POSIX import lands the same way — including in the code this PR just fixed, none of which CI can currently exercise on Windows.

Suggest dropping Fixes #95 and leaving it open, or splitting it. Separately, this PR is what unblocks the CI job: my note in #95 was that a windows-latest import smoke test goes red until the fcntl and SIGUSR2 guards land, and this lands both. Happy to send that job as a small follow-up PR on top of this one once it merges — it's a few lines and seconds of runtime. Not going to open it in parallel and create a conflict with yours.

Two smaller notes: this also fixes #84 and #85 (the sqlite.py and debug_handler.py changes are exactly what those ask for), so adding them to the Fixes list would close them properly rather than leaving them open behind a merged fix. And the checklist has "New source files carry an SPDX license header" unticked — no new source files here as far as I can see, so that's probably just an unticked N/A, but CI runs scripts/check_license_headers.py so worth confirming.

For context on why I was poking at this rather than sending a competing PR: #92 was yours, and #84/#85's reporter had offered those, so I filed #95 about the missing gate instead and left the code fixes alone. Glad it's all in one place.

🤖🤖🤖

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants