Skip to content

spaced: separate graceful closure from execution drain - #827

Open
ravi-pplx wants to merge 8 commits into
ravi/guest-execution-closefrom
ravi/guest-execution-drain
Open

ravi-pplx wants to merge 8 commits into
ravi/guest-execution-closefrom
ravi/guest-execution-drain

Conversation

@ravi-pplx

@ravi-pplx ravi-pplx commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Make close() stop admission without cancelling admitted work. Add cancel() and a deadline-aware drain returning IncompleteExecutionDrain until owned work settles. Track physical I/O, worker startup/retirement, stream cleanup and watch teardown.

Interrupted read/list streams now end with Unavailable after buffered results, even with a full output queue. Worker watches retire with recoverable Unavailable at close or cancellation; Direct watches retire quietly during drain or cancellation. Producer panics terminate read/list/relay streams with Internal after buffered output. The daemon keeps cancelling before drain; wire schemas are unchanged.

Stacked on #790; implements lifecycle ownership from the embedding RFC. Admission metrics are in #833.

Validation

137 filesystem/admission tests passed on the updated stack, including regressions for producer panics and recoverable watch closure. Formatting, affected all-feature/no-default-feature Clippy and doctests passed on Linux. Native macOS and Kind/Firecracker were not rerun locally; CI is pending on this head.

Severity and risk

Moderate: interrupted output must not appear complete, and drain must retain outstanding cleanup. Embedders must consume or drop admitted futures/streams and keep the runtime alive after incomplete drain. Revert after dependent changes to roll back.

AI assistance: Codex contributed code, tests and this description.

@ravi-pplx
ravi-pplx added this pull request to stack #703 October 22, 2026 00:40
@ravi-pplx
ravi-pplx force-pushed the ravi/guest-execution-drain branch from 5e8e35b to 3f209e5 Compare October 22, 2026 16:43
@ravi-pplx
ravi-pplx force-pushed the ravi/guest-execution-drain branch 2 times, most recently from 95eae15 to 0340a93 Compare October 22, 2026 22:43
@cla-assistant

cla-assistant Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@ravi-pplx
ravi-pplx force-pushed the ravi/guest-execution-drain branch from 0340a93 to 1b51b9b Compare October 23, 2026 03:45
@zibi-pplx

Copy link
Copy Markdown
Collaborator

Follow-up review at 02131d12da24, including downstream consumers and the complete stack through #833.

Two issues are worth resolving before merge:

  1. [P2] Quiet worker-watch retirement changes downstream recovery. StreamEnd::Watch(self.lifecycle.closed()) ends an admitted worker watch with successful EOF on close/cancellation. The host relay preserves EOF. Both the native SSE consumer and WebSocket consumer treat that as terminal Closed { BackendClosed }; Unavailable enters route refresh/reconnect. This changes active daemon behavior during shutdown, despite the README saying active requests are unchanged. Please preserve the recoverable daemon status, or explicitly redesign and test the downstream shutdown contract. Updating the README alone would leave the behavior change. The new test asserting quiet EOF only proves the local implementation, not that its downstream meaning is correct.

  2. [P2] A producer panic can turn partial output into successful EOF. The error tails in filesystem/read.rs, filesystem/list.rs, and the new worker relay use once(failure_rx).filter_map(|failure| ... failure.ok().map(Err)). A producer panic drops the sender; the receive error is discarded, so buffered output is followed by clean EOF. A disposable fault-injection test against stream_file reproduced this after consuming a chunk. Ordinary cancellation still returned Unavailable. Please send an explicit completion value on every normal exit (for example, Result<(), Status>) and map an unexpectedly dropped sender to Internal. A small shared terminal-channel helper could keep the three paths consistent. Read/list already had this failure mode; the detached worker relay adds another instance. I did not identify ordinary external input that currently triggers the panic.

Documentation follow-ups:

  • Carry over the cancellation-grace correction posted on spaced: close operation admission consistently #790: service cancellation reaches worker kill tokens immediately; the grace applies to request cancellation, not universally to cancel() or drain expiry.
  • The stream_file comment still promises immediate release when ended completes, but it now awaits file.into_std() to settle physical I/O. Update that latency/ownership statement.
  • The shutdown recipe should mention closing stdin before admission closes: close_stdin and send_signal are refused after close, just like send_input. This is an explanation of the existing policy, not a request to reopen control admission.

Validation: the previous macOS unused-import finding is fixed and targeted macOS Clippy now passes. The temporary panic probe failed as described; the cancellation probe passed. Probes were removed. The latest stack-head CI run has passed all four Kind/Firecracker shards and its aggregate gate. The earlier rapid aggregate failure belonged to a cancelled run, not a non-main-base rejection.

Codex-assisted follow-up review. No implementation changes are included in this comment.

Close admission without cancelling admitted work, and expose explicit
cancellation. Bound drain waits with a typed incomplete-cleanup result
while retaining ownership for a later drain.

Track blocking I/O, worker startup and retirement, stream cleanup and watch
teardown through cancellation. Keep the daemon's cancel-before-drain path
and existing wire contracts.
Cancellation could end worker read/list relays and Direct listings with
successful EOF after partial output. Carry the terminal Unavailable status
beside the bounded result queue, as descriptor reads already do, so a full
queue cannot hide the error or hold up cleanup.

Cover worker RPC reads/listings, Direct listing and empty/full relay queues.
All three regressions fail without the fix. Preserve ordinary completion,
buffer ordering and graceful admission closure.
Closing and Drained enforce the same admission policy. Successful drain is
already established by waiting for owned work; remove the extra state and
write without changing repeated or concurrent drain behavior.
Distinguish finite read/list streams from passive watches when the relay
ends. Read/list cancellation still reports partial output as Unavailable;
watch closure completes quietly, matching Direct watches.

Clarify the close/drain documentation and exercise worker watches under
both admission closure and parent cancellation. The regression fails
before the fix; existing interrupted read/list coverage still passes.
Distinguish worker-watch retirement at close from Direct retirement
during drain or cancellation. Preserve both shutdown paths.
Include uploads and filesystem mutations in the graceful-close contract.
Move process_event and process_exit into their Linux-only consumer so
non-Linux test builds do not fail Clippy on unused imports.
@ravi-pplx
ravi-pplx force-pushed the ravi/guest-execution-drain branch from 02131d1 to 856fece Compare October 23, 2026 16:47

This branch has not been deployed

No deployments
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.

2 participants