Skip to content

spaced: close operation admission consistently - #790

Open
ravi-pplx wants to merge 4 commits into
ravi/guest-execution-uploadsfrom
ravi/guest-execution-close
Open

ravi-pplx wants to merge 4 commits into
ravi/guest-execution-uploadsfrom
ravi/guest-execution-close

Conversation

@ravi-pplx

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

Copy link
Copy Markdown
Contributor

Summary

Add idempotent ExecutionService::close() to refuse new calls and cancel admitted work without cancelling the caller's token. Share the closure check across process, filesystem and environment operations. This PR's close is abortive; #827 adds graceful closure and bounded drain. Clarify that request cancellation can receive grace, while service cancellation can terminate identity workers immediately.

Stacked on #740; follows the embedding RFC. Active-service daemon behavior and wire contracts are unchanged.

Validation

418 affected tests passed on the rebased stack, including five privileged probes. 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: shutdown needs consistent admission closure. Embedding callers must handle the new result from environment configuration. Direct I/O cleanup can outlast cancellation grace. Revert with dependent changes to roll back.

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

@ravi-pplx
ravi-pplx force-pushed the ravi/guest-execution-close branch from 68c7d84 to b77ee70 Compare October 21, 2026 17:24
@ravi-pplx
ravi-pplx force-pushed the ravi/guest-execution-close branch from b77ee70 to adbc139 Compare October 21, 2026 18:19
@ravi-pplx
ravi-pplx added this pull request to stack #703 October 21, 2026 18:36
@ravi-pplx
ravi-pplx force-pushed the ravi/guest-execution-close branch from adbc139 to e398bc1 Compare October 21, 2026 18:36
@kumarpallav-pplx
kumarpallav-pplx requested review from ac-pplx and zibi-pplx and removed request for ac-pplx October 21, 2026 18:36

@zibi-pplx zibi-pplx left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for ac9bc093. Owning a child of the supplied token fixes the issue I was going to raise, that close() would cancel the caller's token, and the new assertions cover it. The entry-point checks and waking uploads that are waiting for Open look good, and the biased select is cancellation-safe.

Requesting changes for one remaining lifecycle issue (inline): close() still aborts work that was already admitted, while the docs say it "may still finish". Non-blocking: require_running is duplicated, and configure_process_env could use a typed error (both inline). The same Kind/Firecracker CI note as #702 applies.

Review prepared with AI assistance (Claude Code); findings checked against the PR head.

Comment thread crates/libs/spaced/guest/execution/src/execution.rs Outdated
Comment thread crates/libs/spaced/guest/execution/src/filesystem/router.rs
Comment thread crates/libs/spaced/guest/execution/src/execution.rs
@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-close branch from 7343afa to 7c632eb Compare October 23, 2026 03:45
@zibi-pplx

Copy link
Copy Markdown
Collaborator

Follow-up review at 7c632eb124ab, including the graceful-close/drain implementation in #827.

The earlier caller-token isolation, duplicated admission check, and explanation for returning Status from environment configuration are addressed. #827 supplies the separate admission-only close() and abortive cancel(), so I am not repeating the superseded graceful-close request here.

[P2, documentation] The advertised worker cancellation grace does not apply to service shutdown.

The close() documentation and README say supervised mutations can finish during cancellation_grace, after which identity workers are terminated. However, each worker's stop_token is a child of the pool/service shutdown token, and supervise_worker calls child.kill().await as soon as it is cancelled. An in-flight batch or rename can therefore lose its worker immediately and produce an unknown outcome; it does not receive the stated grace.

Please distinguish request cancellation, which can receive the supervisor grace, from service/parent cancellation, which can kill workers immediately. The same distinction needs to carry into #827's README, because drain-deadline expiry requests service cancellation too. Correcting the documentation preserves the current abortive behavior; changing token ownership would be a separate shutdown-policy change.

This worker-kill behavior predates the stack; the issue here is the new public promise.

Codex-assisted follow-up review, verified by tracing cancellation from the service through the pool to the worker supervisor.

@ravi-pplx
ravi-pplx force-pushed the ravi/guest-execution-close branch from 7c632eb to 0967526 Compare October 23, 2026 16:47
Expose idempotent closure across execution handles. Reject filesystem and
environment calls after cancellation, and wake uploads waiting for Open.

Retain active-service behavior and the wire contract. Complete drain
ownership and timeout reporting remain separate work.
Own a child of the caller's cancellation token and share it with the
filesystem router. Parent cancellation still closes the service, while
close and drain leave the caller's token alone.

Extend the closure regression to check both directions and document that
already-admitted same-key retry waiters can outlast closure.
@ravi-pplx
ravi-pplx force-pushed the ravi/guest-execution-close branch from 0967526 to aa8c274 Compare October 23, 2026 18:54

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