Skip to content

spaced: accept native filesystem upload streams - #740

Open
ravi-pplx wants to merge 7 commits into
mainfrom
ravi/guest-execution-uploads
Open

ravi-pplx wants to merge 7 commits into
mainfrom
ravi/guest-execution-uploads

Conversation

@ravi-pplx

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

Copy link
Copy Markdown
Contributor

Summary

Expose a nameable FilesystemHandle with native write_file. Share admission, frame limits and supervised execution with gRPC; bound native input waits with a configurable 30-second idle timeout. Borrowed/non-Unpin streams remain supported. Share one idle default and document zero-duration and config-literal behavior. Daemon gRPC upload behavior is unchanged. Add a state-free Debug implementation for FilesystemHandle and clarify the experimental Rust compatibility policy.

Implements native uploads from the embedding RFC.

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

Severity does not apply: new native API. Slow native uploads can reach the idle timeout; callers can configure it. Revert with 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 21, 2026 17:01
@ravi-pplx
ravi-pplx force-pushed the ravi/guest-execution-uploads branch from 25bc9fe to bae969e Compare October 21, 2026 18:19
@kumarpallav-pplx
kumarpallav-pplx requested review from ac-pplx and zibi-pplx and removed request for ac-pplx October 21, 2026 18:35

@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.

The shared upload path looks right. Admission happens before the first poll, the per-message limits and bounded relay are preserved, and accepting borrowed and !Unpin streams is nicely ergonomic. The gRPC Write refactor keeps its semantics. I'm requesting changes for two API issues on the native surface (inline):

  1. Split upload API / unnameable handle. filesystem_service() returns an opaque type whose write can't be called natively, so uploads need a separate ExecutionService clone.
  2. No idle timeout on native uploads. A stalled stream pins a payload slot and blocks same-key retries.

Non-blocking: environment validation is inconsistent between configure_process_env and native StartRequest.env (inline, README). 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/execution.rs Outdated
Comment thread crates/libs/spaced/guest/execution/README.md
@ravi-pplx
ravi-pplx force-pushed the ravi/guest-execution-uploads branch 2 times, most recently from fe9419f to 492a71f Compare October 22, 2026 16:43
Base automatically changed from ravi/guest-execution-admission to main October 22, 2026 20:26
@ravi-pplx
ravi-pplx force-pushed the ravi/guest-execution-uploads branch from 492a71f to 3047daf Compare October 22, 2026 20:26
@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-uploads branch from 129719e to f41757c Compare October 23, 2026 03:45
@zibi-pplx

Copy link
Copy Markdown
Collaborator

Follow-up review at f41757cc8ca8, including the full stack through #833.

The earlier nameable-handle and upload-idle-timeout findings are addressed, and the shared native/gRPC environment sanitization is now documented. I found no additional upload correctness issue.

Two non-blocking API points remain:

  • Add Debug for the newly public FilesystemHandle. It currently derives only Clone (source). Our API guidelines require Debug for public types. A minimal implementation describing the handle is enough; it need not expose requests, environments, or internal state.
  • Clarify the embedding API's compatibility promise. The new public ExecutionServiceConfig field breaks complete external struct literals, which the README correctly documents. Our current guidance against #[non_exhaustive] assumes workspace crates evolve together. If this embedding surface is intended for independently maintained external consumers, that assumption should be revisited explicitly. In that case, consider constructor/builder-based configuration and non-exhaustive public error enums. ExecutionServiceConfig is a struct; adding #[non_exhaustive] to it would prevent external struct literals, including ..Default::default(), so it needs a supported construction path. This is a compatibility-design question, not a request to add the attribute mechanically.

Codex-assisted follow-up review. Findings were checked against the referenced head; shutdown/drain findings are posted on their owning PRs.

Let ExecutionService::write_file accept a caller-owned stream of the
existing upload messages. Route gRPC through the same handler, preserving
admission before input polling, frame limits, backpressure and cleanup.

Document the existing bounded EnvMap configuration contract. Extend the
embedding probe with native upload/gRPC replay and environment replacement,
request precedence and running-process snapshots.
Keep the generic upload wrapper small and run its body through a borrowed
pinned stream. Preserve admission before polling and support for borrowed,
non-Unpin input without allocating a boxed stream.

Document caller-owned upload deadlines, native start environment filtering
and the upload handle. Check Send on a borrowed upload future and verify
that an oversized replacement preserves the committed file after cleanup.
Use one default across config, filesystem handles and daemon wiring.
Document zero-duration behavior and config struct literal updates.
Point embedding documentation at the guest-process crate.
@ravi-pplx
ravi-pplx force-pushed the ravi/guest-execution-uploads branch from 95bcdff to 30011f3 Compare October 23, 2026 18:54
@ravi-pplx

Copy link
Copy Markdown
Contributor Author

Addressed the follow-up review in 30011f39: added minimal Debug for FilesystemHandle and documented experimental Rust compatibility with revision pinning. Stable configuration construction and error evolution remain a follow-up before promising cross-version Rust compatibility.

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