Skip to content

sandbox: bound managed control writes by the operation deadline - #518

Merged
Pedro Henrique Penna (ppenna) merged 1 commit into
devfrom
agents/fix-issue-464-validate-implementation-0b0b6d41
Oct 13, 2026
Merged

Pedro Henrique Penna (ppenna) merged 1 commit into
devfrom
agents/fix-issue-464-validate-implementation-0b0b6d41

Conversation

@ppenna

@ppenna Pedro Henrique Penna (ppenna) commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Summary

On Windows, a managed control endpoint that authenticated a session and then stopped reading could hold sandbox exec, start, or stop past --timeout with no limit. exec, ping, stop, and the attach exchange took their deadline only after sending their request, and the named-pipe transport wrote each record with a synchronous os.write(), which blocks while the pipe's inbound quota is full. Only the closure of the pipe released it, and then with a broken pipe rather than a timeout.

  • Each operation (attach, ping, exec, stop) now takes its deadline before it sends its request and bounds both the request and the response with it. Both transports' write_all() take that deadline, and a write that outlives it fails with TimeoutError: managed control request timed out, as a read already failed with managed control response timed out.
  • The Windows transport opens the pipe with FILE_FLAG_OVERLAPPED and reads and writes it with overlapped transfers. A transfer that is still pending at its deadline is cancelled with CancelIoEx(), and every transfer is settled with GetOverlappedResult() before its buffer can be released, including when KeyboardInterrupt interrupts the wait, which WaitForMultipleObjects() lets Ctrl+C do on the main thread. This replaces the PeekNamedPipe() read polling, so a read now completes as soon as data arrives. ERROR_BROKEN_PIPE, ERROR_NO_DATA, and ERROR_PIPE_NOT_CONNECTED still raise ControlEndpointClosed, and connection retries keep their errno policy: Python maps CreateFile() errors, including the ERROR_PIPE_BUSY of the reconnect transition, to the errno values that the CRT gave for os.open().
  • The socket transport sends in deadline-bounded slices rather than with one sendall(), which only the socket timeout that the last read had left, at most 0.25 s, bounded.
  • On both transports, a write that moves no bytes raises ControlEndpointClosed at once rather than retrying until the deadline, as an empty read does and as the previous named-pipe writer did.
  • A session closes when a record transfer fails in either direction, since a record cut short would misframe every later one, and it then refuses further use with ValueError: managed control session is closed. A refusal or reset that arrives as a complete record leaves the session usable, as the managed-lifecycle scenario requires. close() is idempotent.
  • doc/usage.md describes --timeout as bounding each managed control request and its response.

Notes

  • Cancellation does not use _winapi.Overlapped.cancel(): CPython clears the object's pending flag right after its CancelIoEx(), so releasing the object before GetOverlappedResult() could free memory that Windows still writes. Calling CancelIoEx() directly keeps the flag set, and the destructor of a pending object cancels and waits, or leaks rather than frees. A code review found this window; the new interrupted-write test covers the path.
  • Empirically on Windows NPFS: a 4 KiB overlapped write into a 1 KiB quota with a stalled reader stays pending; after cancellation it settles as (0, ERROR_OPERATION_ABORTED) and the pipe no longer holds any of it.
  • As before, the connection and the attach exchange each get their own --timeout budget; the issue reviewed this without filing it.

Validation

All results are for the current head, d4f88af, on dev at b1dadb7, unless noted.

  • Local (Windows 11, Python 3.14.7):
    • ruff check, ruff format --check, compileall, and pyright for Linux and Windows pass.
    • The full validate-nvx suite passes (980 tests, 111 skipped), as do the adversarial tests (70) and the CLI help checks. The control-session tests pass in 25 of 25 repeated runs.
    • Against dev's control_session.py, the new stalled-pipe test fails: the request stays blocked until the test endpoint's 10 s fallback drain, and the operation then reports a response timeout. The deadline-wiring and session-closure tests fail too.
  • Remote bare metal: the hosts' existing checkouts were dirty, so each host ran an isolated clone of this change with release v0.1.0-dev.8c3cc0abfb46, whose guest, kernel, and OpenVMM inputs equal this change's. Its kernel and initramfs hashes match the guest artifacts of CI run 38297793018, whose ubuntu-smoke-scratch.ext4 supplied the public sandbox scenarios.
    • On prometheus32 (Linux/KVM, Python 3.14), prometheus30 (Linux/MSHV, Azure Linux 3.0, Python 3.12), and prometheus28 (Windows/WHP, Windows Server 2025, Python 3.14):
      • verify passes, and the validate-nvx unit suites (980 tests) and adversarial tests pass, including the Linux-only socket tests on Linux and the named-pipe tests on Windows.
      • test-microvm --scenario managed-lifecycle --scenario managed-exec-config --scenario sandbox-lifecycle passes against real OpenVMM on each backend.
    • The previous head, 6c95d51ce on dev at 196511e, which lacked only the zero-byte write check from review, also passed these scenarios on each host, in 4 of 4 runs on WHP, and its control-session tests passed in 25 of 25 repeated runs on prometheus32 and prometheus28.
    • On prometheus28, the issue's reproduction against dev's control_session.py was still blocked after 0.361 s with a 0.05 s timeout, and returned only when the server closed the pipe, with BrokenPipeError. Against this change, it raised TimeoutError: managed control request timed out after 0.052 s while the server stayed connected. The new stalled-endpoint test failed against dev's code after 10.3 s.

Fixes #464

Copilot AI balanced review requested due to automatic review settings October 12, 2026 20:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The socket writer mishandles a zero-byte send as a timeout rather than an endpoint closure.

1 open finding
What changed in this PR

Bounds managed control request writes and responses by operation deadlines across socket and Windows named-pipe transports.

Changes:

  • Adds deadline-aware socket and overlapped named-pipe transfers.
  • Closes sessions after failed record transfers.
  • Adds regression tests and updates timeout documentation.
File Description
scripts/​nvx_tools/​control_session.py Implements bounded transfers and session closure.
scripts/​test_control_session.py Adds transport and timeout regression tests.
scripts/​test_microvm_tests.py Updates named-pipe connection mocking.
doc/​usage.md Clarifies managed control timeout semantics.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread scripts/nvx_tools/control_session.py Outdated
Fixes #464.

The managed control client sent every record without a deadline. exec,
ping, and stop took their deadline only once they had sent their
request, as the attach exchange did once it had sent the capability,
and the Windows transport wrote each record with a synchronous
os.write() to the named pipe. A control endpoint that authenticated the
session and then stopped reading, such as a stalled OpenVMM, therefore
held a request larger than the pipe's free inbound quota, and with it
`sandbox exec`, `start`, or `stop`, past the --timeout that bounds the
control operation, without limit. Only the closure of the pipe released
it, and then with a broken pipe rather than a timeout.

Reproduction
------------

The issue's self-contained reproduction authenticates the unmodified
client over a real byte-mode named pipe with a 1 KiB quota, stops
reading, and sends a valid 4,093-byte exec request with a 0.05 s
response timeout. Against the previous control_session.py on Windows,
with Python 3.14.7, the call was still blocked after 0.36 s and returned
only when the reproduction closed the server, with BrokenPipeError. With
this change, it fails with "managed control request timed out" after
0.06 s while the server keeps the pipe open.

Fix
---

- Each operation, the attach exchange, ping, exec, and stop, takes its
  deadline before it sends its request and bounds both the request and
  the response with it, and each transport's write_all() takes that
  deadline. A write that outlives it fails with the TimeoutError
  "managed control request timed out", as a read that outlives it
  already failed with "managed control response timed out".
- The Windows transport opens the pipe for overlapped I/O and reads and
  writes it with overlapped transfers. A transfer that is still pending
  at its deadline is cancelled, and every transfer is waited for until
  it ends, since Windows uses its buffer until then, even when an
  exception such as KeyboardInterrupt ends the wait. The wait uses
  WaitForMultipleObjects(), which Ctrl+C interrupts on the main thread.
  Cancellation calls CancelIoEx() rather than Overlapped.cancel(), which
  marks a transfer as ended at once, so that a transfer that an
  exception releases at any point is waited for or keeps its buffer.
  Overlapped reads replace the PeekNamedPipe() polling, which bounded
  reads but has no counterpart for writes, and a read now completes as
  soon as data arrives rather than at the next 10 ms poll.
  ERROR_BROKEN_PIPE, ERROR_NO_DATA, and ERROR_PIPE_NOT_CONNECTED still
  report a closed endpoint. Connection retries keep their errno policy:
  Python maps the errors of CreateFile(), including the ERROR_PIPE_BUSY
  of the reconnect transition, to the errno values that the CRT gave
  for os.open().
- The socket transport sends in deadline-bounded slices rather than with
  one sendall(), which the socket timeout that the last read had left,
  at most 0.25 s, bounded instead of the operation deadline.
- On both transports, a write that moves no bytes reports a closed
  endpoint rather than retrying until the deadline, as an empty read
  does and as the previous named-pipe writer did.
- A session closes when a record transfer fails in either direction,
  since the record may be cut short and any later record would then not
  frame correctly, and it then refuses further operations with "managed
  control session is closed". A refusal or reset that arrives as a
  complete record leaves the session usable, as the managed-lifecycle
  scenario requires, and close() is idempotent.
- doc/usage.md now describes --timeout as bounding each managed control
  request and its response.

Tests
-----

- Over a real named pipe whose endpoint authenticates the session and
  then stops reading, a 4,093-byte exec request with a 0.25 s timeout
  fails with the request timeout within 2 s of the deadline, while the
  endpoint keeps the pipe open. The session then refuses a ping, and
  the endpoint receives less than the request before the pipe closes.
  Against the previous control_session.py, the request stayed blocked
  until the test endpoint drained the pipe after 10 s, and the operation
  then failed with a response timeout.
- On Linux, a 60 KiB exec request to a Unix-domain socket peer that does
  not read fails the same way, and the peer receives part of the request
  and then the end of the stream.
- With a fixed clock, attach, ping, exec, and stop each write with the
  deadline with which they then read, and a write or read that fails
  closes the session once and refuses later operations without another
  transfer.
- An interrupted named-pipe write is cancelled before the
  KeyboardInterrupt propagates, and leaves nothing in the pipe.
- On both transports, a write that moves no bytes reports a closed
  endpoint at once.
- Named-pipe reads time out without losing data that arrives later, and
  report a closed endpoint only after the data that the server wrote
  before it closed the pipe. The connection retry test now stubs
  CreateFile().

Validation
----------

- On a Windows 11 workstation with Python 3.14.7, ruff, pyright for
  Linux and Windows, compileall, the validate-nvx unit suites, the
  adversarial tests, and the CLI help checks pass, and the
  control-session tests pass in 25 of 25 repeated runs.
- Bare-metal Linux/KVM, Linux/MSHV on Azure Linux 3.0, and Windows/WHP
  on Windows Server 2025 hosts ran an isolated clone of this change with
  the release package that CI built for 8c3cc0a, whose guest, kernel,
  and OpenVMM inputs equal this commit's, and with that CI run's Ubuntu
  scratch template. On each, the validate-nvx unit suites pass, the
  socket tests on Linux and the named-pipe tests on Windows included,
  and `test-microvm --scenario managed-lifecycle --scenario
  managed-exec-config --scenario sandbox-lifecycle` passes against real
  OpenVMM, on WHP in 4 of 4 runs. The control-session tests pass in 25
  of 25 repeated runs on the KVM and WHP hosts.
- On the WHP host, the issue's reproduction against dev's
  control_session.py was still blocked after 0.36 s and ended only when
  the server closed the pipe; against this change, it timed out after
  0.05 s. The new stalled-endpoint test failed against dev's
  control_session.py after 10.3 s.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI balanced review requested due to automatic review settings October 12, 2026 21:00
@ppenna
Pedro Henrique Penna (ppenna) force-pushed the agents/fix-issue-464-validate-implementation-0b0b6d41 branch from b13a177 to d4f88af Compare October 12, 2026 21:00

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Needs a closer look

Low-level Windows overlapped-I/O cancellation and buffer-lifetime handling warrant final human review despite extensive coverage.

0 open findings

1 resolved since last review

🧠 Review effort: Balanced

@ppenna
Pedro Henrique Penna (ppenna) merged commit 6d964b2 into dev Oct 13, 2026
29 checks passed
@ppenna
Pedro Henrique Penna (ppenna) deleted the agents/fix-issue-464-validate-implementation-0b0b6d41 branch October 13, 2026 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Track managed sandbox lifecycle bugs: on Windows, provision waits out its timeout for a lock file that refuses access for good

2 participants