Skip to content

fix(client): retain mailbox grant for persistent launch admission - #5336

Closed
Casey-allard wants to merge 2 commits into
tinylabscom:mainfrom
Casey-allard:codex/spiralcore-persistent-sdk-mailbox
Closed

Casey-allard wants to merge 2 commits into
tinylabscom:mainfrom
Casey-allard:codex/spiralcore-persistent-sdk-mailbox

Conversation

@Casey-allard

@Casey-allard Casey-allard commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

A persistent LaunchRequest carrying .mailbox(grant) currently drops the grant when boot_spec constructs MachineStartParams, so the machine can boot without the requested mailbox. Forward the request grant into canonical admission for that start. Keep ordinary persistent restarts explicitly ungranted; the grant is not added to MachineSpec or inherited from the saved definition.

Current branch validation — 2026-11-13

Updated head 9105b02b4228a744c23b9742890c39db345f96cd merges upstream main c0f15d82d89e42f0a0f5805bc656f09029c4c19d, preserving original SDK head 18b58e1ad2262a1c0bfa23d51f8e5679d3376f60 and its historical evidence. No conflict resolution or combined helper/lifecycle contribution was added; the net diff remains two SDK files.

On the updated head, full-workspace formatting and whitespace checks passed. Independent source review found no code/Rust/security blocker. Full build/test acceptance is pending: CI run 43383672321 is terminal action_required, with zero jobs/checks. The contributor account has pull-only upstream permissions; maintainer approval is needed for the fork workflow. The executed test and native results below retain their original source identities and do not qualify this merged head.

Original regression and executed evidence

The regression uses the public launch API with the hermetic Mock backend, which cannot serve guest mailboxes. Before the fix it unexpectedly boots successfully; after the fix it reaches the canonical mailbox-capability refusal before boot and removes the newly created failed definition. A second test confirms that creating a definition from a mailbox-bearing request does not grant a later ordinary start.

Validation on Linux x86_64 / Rust 1.97.1, release/locked, test-support, serial tests, two build jobs, based on upstream 28afdfe9ef7e36518ca42788d57c54f1d7275f57:

  • Actual RED: the public request regression fails before the fix; the no-inheritance control passes.
  • cargo test --release --locked -p mvm-client --features test-support --lib persistent_ -- --test-threads=1: 31 passed.
  • cargo test --release --locked -p mvm-client --features test-support --lib launch::tests:: -- --test-threads=1: 55 passed. These scopes overlap.
  • Strict release Clippy for both --lib and --tests, with test-support: pass.
  • cargo fmt -p mvm-client -- --check: pass.
  • Independent code/Rust and security source reviews: pass.

This follows the service ownership agreement in #5328 and addresses the SDK defect encountered by the current canonical Linux/Firecracker witness. This narrow fix does not establish reliable-service completion or WAN qualification. It does not edit the journal, change mailbox wire formats or close #4629. Both changed launch files remain unchanged in upstream through a707f5e817982d5140e50e05228eb155ed2200a8; the executed checks above retain their original exact source pin.

A subsequent canonical Linux/Firecracker diagnostic executed this exact SDK head with rebuilt runtime/helper and an unmodified canonical mailbox path. It recovered all 1,024 binary bytes exactly, including zeros (SHA-256 3cafacf7f3f4628dcc4da6be8a679c3d283911100632476924fc76fa0e2a20ad). The overall lifecycle gate failed: SDK Stop/remove succeeded but the original mailbox helper remained an uncollected zombie. That separate original-child ownership defect remains open; later PID disappearance does not upgrade the result. No wire-byte, compression, WAN or complete-service claim follows. The retained native report binds the executed source/artifacts, exact readback, timings and failure.

@Casey-allard

Copy link
Copy Markdown
Contributor Author

Executed native follow-up for this SDK fix: all 1,024 synthetic binary bytes were received exactly through the canonical guest mailbox on Linux x86_64/Firecracker at 18b58e1ad2262a1c0bfa23d51f8e5679d3376f60. Matching host SDK/helper, guest producer and freshly source-built runtime; one vCPU/256 MiB, no NIC, TTL180, idle pool off. No synthetic plan or manual helper launch. This confirms the persistent grant propagation reaches actual mailbox delivery.

The overall lifecycle result remains failed: canonical Stop and non-force Remove returned success, but the original helper had exited and remained a zombie at the held-identity collection check. Later absence and empty namespaces are recorded separately and do not upgrade that failure. No external waitpid/kill/repair or second guest was used. The separate owner issue and #5328 discussion track that implementation.

Observed setup 658.948 ms, functional receive 104.022 ms (including the 100 ms probe schedule), attempted lifecycle 2,821.558 ms, host-orchestrator CPU 475.934 ms. These are one-case local diagnostics, not send-only or compression/WAN measurements; wire bytes and guest CPU were unavailable.

The source-pinned report and compact JSON are now on SpiralCore main through #112, with both the positive delivery and original teardown failure retained. Native runtime preparation is measured separately. The complete networking contribution remains open.

@auser

auser commented Nov 13, 2026

Copy link
Copy Markdown
Collaborator

This PR has no current required Test/Invariant checks. A maintainer branch-update attempt returned GitHub 403 for the fork head, despite maintainer_can_modify being set. Please merge the current upstream main into codex/spiralcore-persistent-sdk-mailbox (without rewriting the source-scoped evidence) and push the fork branch so CI runs on the updated head. The SDK fix can then be queued if the required checks pass; the separate mailbox-helper lifecycle finding remains outside this narrow PR.

@Casey-allard

Copy link
Copy Markdown
Contributor Author

Merged upstream main at c0f15d82d89e42f0a0f5805bc656f09029c4c19d into the existing fork branch and pushed 9105b02b4228a744c23b9742890c39db345f96cd, as requested. This is a normal merge with original PR head 18b58e1ad2262a1c0bfa23d51f8e5679d3376f60 retained as its first parent; no history or source-scoped native evidence was rewritten.

The net diff against updated main remains only crates/mvm-client/src/launch/mod.rs and launch/tests.rs (78 additions, 4 deletions). The combined helper/lifecycle development branch was not imported. cargo fmt --all -- --check and the staged whitespace check passed. Independent source review found no code/Rust/security blocker in the one-start mailbox propagation and explicit grantless ordinary restart. This is source review and formatting verification, not a new native restart or full-workspace result.

GitHub created CI run 43383672321 for the updated head, but it is already terminal with action_required, zero jobs and zero check runs. The contributor account has pull-only upstream permissions, so upstream approval is needed to run this fork workflow. Please approve that run so the required Test/Invariant checks can execute. Their results and the merge decision remain pending.

@auser
auser added this pull request to the merge queue Nov 13, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Nov 13, 2026
@auser

auser commented Nov 13, 2026

Copy link
Copy Markdown
Collaborator

The approved fork run passed on this head, but merge-queue CI 43466249391 failed to compile against newer main. mailbox_grant::resolve now takes a third workload_ir: Option<&Path> argument; the two new call sites in crates/mvm-client/src/launch/tests.rs at lines 2063 and 2094 still pass only two. These tests construct explicit one-start grants without a workload IR, so pass None as the third argument at both sites, merge current upstream main into the fork branch, and push. GitHub removed the PR from the queue for failed merge-group checks. Please verify the updated head in PR CI; a new fork run may require approval again.

@auser
auser added this pull request to the merge queue Nov 13, 2026
@auser
auser removed this pull request from the merge queue due to a manual request Nov 13, 2026
@auser

auser commented Nov 13, 2026

Copy link
Copy Markdown
Collaborator

I could not push to the fork branch with upstream credentials. To avoid leaving your narrow SDK fix blocked on the known merge-group compile break, I preserved your original implementation and regression commits as ancestors of upstream-owned #5474, merged current main, and added the two required None workload-IR arguments. This is not a rewrite of the original work or a claim that the native cleanup gate passed. I will leave this PR open until #5474 passes on its exact head and enters the normal queue, then close it as superseded with the history linked.

@auser

auser commented Nov 13, 2026

Copy link
Copy Markdown
Collaborator

Superseded by upstream-owned #5474, now validated on its exact PR head and submitted to the normal merge queue. #5474 preserves the original 18b58e1ad2 implementation and test commit and subsequent fork merge 9105b02b42 in its ancestry, then merges current main and fixes both the IR-aware resolver calls and bounded signed-retention fixture. This fork branch is not deleted; its original evidence and discussion remain here. The native cleanup defect described above remains unresolved and is not claimed as passed. Closing the duplicate PR, not the work.

@auser auser closed this Nov 13, 2026
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.

Add dedicated bounded mailbox vsock service and call/cast API

2 participants