[NancyKwongPerplexity]

Port of #828 (by @zibi-pplx) onto current main (paths moved from crates/libs to crates/l40_api). Narrow hotfix for the encrypted-suspend quarantine race.

Problem

Node-manager's gRPC server enforces a 120 s request timeout (GRPC_REQUEST_TIMEOUT), while the encrypted Firecracker capture runs detached and the client waits up to 960 s. When the server-side deadline fires, sbx-mgr sees a capture error with the runtime still live and rolls the sandbox back to paused. The detached capture then keeps going and stops the VM for the seal, so the next resume or retry quarantines the sandbox. That accounted for 43 of 47 quarantines over three days after BYOK volume growth.

Change

• capture_vm_state owns a CancellationToken that fires when the handler future drops (drop_guard), threaded through create_snapshot_inner → create_encrypted_snapshot.
• A capture still queued behind the sandbox lock when the waiter goes away is skipped.
• A capture already running finishes its Firecracker I/O and unmounts the bind, then returns Cancelled instead of calling stop_runtime_for_encrypted_seal.
• The capture I/O moved into write_encrypted_snapshot_files with a cfg(test) seam.
• Scope matches Keep encrypted runtimes alive when capture waiters expire #828: Firecracker only, Cloud Hypervisor path unchanged.

Testing

• New dropping_capture_handler_skips_queued_capture_and_keeps_running_vm: queued → 0 captures / 0 stops; running → 1 / 0; waiter kept (control) → 1 / 1.
• cargo test -p pplx-space-node-mgr --lib (encrypt/snapshot filters) pass locally. The exception is execution_fence::tests::stopped_unbound_snapshot_reaches_filesystem_without_allocating, which needs privileged host paths this sandbox lacks.
• Not validated on Kind/Firecracker.

Durable fix: #1761 (port of #837). This PR supersedes #828.

_{Created with Perplexity}

[coderabbitai]

No actionable comments were generated in the recent review.

Walkthrough

Encrypted snapshot capture now tracks whether its RPC waiter remains active. It skips queued capture work if the waiter disappears and leaves the paused runtime running if the waiter disappears before runtime shutdown. File writing is extracted into a method that binds the encrypted volume, asks Firecracker to write snapshot files, and attempts to unmount the bind. A new test covers queued, running, and waiter-retained capture cases.

Possibly related issues

• Issue 875 in perplexityai/SPACE describes the same capture race. This change addresses the node-side cancellation and runtime-stop boundary, but not the issue’s stated long-capture deadlines or ownership concerns.

Merge Risk: Moderate · up to f2e33

This change keeps a timed-out encrypted snapshot capture from stopping a VM that was rolled back to paused, which should prevent quarantine on the next resume or retry. The new unit test covers the queued, running and waiter-retained cases, but real Firecracker validation was not run. A narrow window also remains where the waiter can expire while the runtime stop is in flight. Confirm the behavior on a Kind/Firecracker environment before merging, or explicitly accept that gap.

Security architecture risk: Low · up to f2e33

The change reduces an existing suspend failure mode without an observed expansion of caller privileges or data access. It remains a best-effort fix: cancellation can still race with runtime-stop dispatch, and live Firecracker suspend/resume behavior has not been validated.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The changed path affects an authorized capture's target sandbox runtime and encrypted snapshot volume on its node. Request cancellation changes whether work starts or runtime stop proceeds; the reviewed path does not give an unauthenticated caller a new entrypoint or grant additional authority over another execution.

Trust Boundaries and Controls

• observed — CaptureVmState retains the node-admin or sandbox-manager peer authorization requirement. The capture implementation validates the snapshot execution fence before selecting the encrypted destination, rejecting a superseded execution identity.

Resilience and Maintainability Implications

• observed — The extracted writer preserves the prior bind, Firecracker write, and unmount-attempt ordering. Ordinary bind or snapshot errors still reach the cleanup attempt before result propagation; unmount failures remain logged rather than propagated. This is pre-existing best-effort cleanup behavior, not a new cleanup guarantee.

Hardening Proposals

• proposed — A follow-on lifecycle design could explicitly coordinate suspend ownership and irreversible runtime-stop authorization, rather than relying solely on a point-in-time waiter check. That would address the remaining cancellation-versus-stop race beyond this hotfix.

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

• Ignore (reviewers only)

Failed checks: 2 errors

Check name	Status	Explanation	Resolution
Description Follows The Pr Template		The PR description lacks all four required items. The body uses the headings "## Problem", "## Change", "## Testing", and a closing "Durable fix" line. It has no matching headings for the required sec…	Edit the PR description to add: - A "## Summary" section that states what merging the change does. - A "## Validation" section that names the commands or checks run and their results, or states "CI on this head". Rename or add this section;…
Public-Safe Content		The code diff passes. The four changed files (service.rs, snapshots.rs, snapshots/tests.rs, service/tests.rs) add no credentials, tokens, keys, internal hostnames, URLs, account IDs, ARNs, personal ab…	Edit the PR description (not the code): - Remove the Perplexity task link from the "Created with Perplexity" footer. Remove the footer or replace it with plain text that names no private session. - In the Problem section, remove the specifi…

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title uses the lowercase area prefix "node-mgr:" followed by a lowercase summary, with no trailing period, ticket ID, or internal link. It matches the required format. It also names the main chang…
Bug Fixes Name A Regression Test		The check applies. The PR fixes a defect in application behavior under crates/ (the encrypted-suspend quarantine race in crates/l40_api/node/mgr/server). The Validation section names a regression …
Weakened Safeguards Are Justified		PR #1760 (author NancyKwongPerplexity, per the provider record) changes four files, all under crates/l40_api/node/mgr/server/src/service*. I checked the diff for every item the check lists. - **Attrib…
Agent Tooling Runs No New Commands		The check applies to agent plugin manifests (plugin.json, plugin.yaml), SKILL.md files, skill and command files, and guideline files. The changed-file inventory for this PR (refs/coderabbit/pre-merge-…

Full details: Title check

Explanation

The title uses the lowercase area prefix "node-mgr:" followed by a lowercase summary, with no trailing period, ticket ID, or internal link. It matches the required format. It also names the main change: encrypted runtimes stay alive when capture waiters expire, which the raw summary and objectives confirm (cancellation token on capture_vm_state, paused runtime kept running, no stop on dropped waiter).

Full details: Bug Fixes Name A Regression Test

Explanation

The check applies. The PR fixes a defect in application behavior under crates/ (the encrypted-suspend quarantine race in crates/l40_api/node/mgr/server). The Validation section names a regression test, dropping_capture_handler_skips_queued_capture_and_keeps_running_vm, and gives its expected outcomes per case: queued gives 0 captures and 0 stops, running gives 1 capture and 0 stops, and the waiter-kept control gives 1 capture and 1 stop. The test is in the diff (snapshots/tests.rs, +121 lines). Its assertions encode the bug. Before the fix, nothing checks waiter_gone in the queued path, so a queued capture would run and the count would be 1, not 0. Before the fix, a running capture would also reach stop_runtime_for_encrypted_seal, so the stop count would be 1, not 0. The test therefore fails on the pre-fix code and passes on the fixed code. This is inferred from the assertions and the diff, not from a recorded run. The Validation section does not explicitly say "fails without the fix." It also does not report a run of the new test. It reports only that other node-mgr encrypt/snapshot tests passed locally. The custom check requires a named test that fails without the fix and passes with it. The named test meets that requirement on the code evidence.

Full details: Weakened Safeguards Are Justified

Explanation

PR #1760 (author NancyKwongPerplexity, per the provider record) changes four files, all under crates/l40_api/node/mgr/server/src/service*. I checked the diff for every item the check lists. - Attributes: No #[allow], #[expect], #[ignore], skip, xfail, or should_panic was added or changed. The only #[cfg(test)] additions gate a test-only field and a test-only I/O seam. - Tests and assertions: No test was deleted and no assertion was removed or loosened. The new test dropping_capture_handler_skips_queued_capture_and_keeps_running_vm adds assertions, including a retained-waiter control case. - Removed lines: Every removed line is production code that moved into the new write_encrypted_snapshot_files function, or a call-site update. None is a guard or check. The new waiter_gone cancellation checks add behavior and do not remove any. - Config and guidance: The changed-file list has no CI workflow, nextest, deny, clippy, lint, allowlist, baseline, known-failures, AGENTS.md, CLAUDE.md, CONTRIBUTING.md, or docs/rust file. No review-guidance text was added. No safeguard was weakened, so there is nothing to list.

Full details: Agent Tooling Runs No New Commands

Explanation

The check applies to agent plugin manifests (plugin.json, plugin.yaml), SKILL.md files, skill and command files, and guideline files. The changed-file inventory for this PR (refs/coderabbit/pre-merge-check/base..head) lists four modified files, all Rust sources under crates/l40_api/node/mgr/server/src/: service.rs, service/snapshots.rs, service/snapshots/tests.rs, and service/tests.rs. A path filter for plugin manifests, SKILL.md, skills/, commands/, CLAUDE.md, AGENTS.md, .claude/, .cursor, guideline, MCP, LSP, and hook names matched none of them. The diff therefore adds no agent plugin manifest entry, no SKILL.md frontmatter key, no inline shell substitution in a skill or command file, and no other agent-run content. The check is inapplicable to this PR, so it passes. The Rust changes are not agent tooling and are outside this check's scope.

Full details: Description Follows The Pr Template

Explanation

The PR description lacks all four required items. The body uses the headings "## Problem", "## Change", "## Testing", and a closing "Durable fix" line. It has no matching headings for the required sections. 1. Missing "## Summary" section. No heading names Summary. The "## Problem" and "## Change" sections do not satisfy this item. 2. Missing "## Validation" section. The "## Testing" section lists local test runs and states that Kind/Firecracker validation was not performed. Its heading is "Testing", not "Validation". The check requires the "## Validation" section by name. 3. Missing "## Severity and risk" section. No severity and no risk statement appear. The description does not say "does not apply" for severity. 4. Missing "AI assistance:" line. The description ends with a Perplexity attribution footer. It has no line that starts with "AI assistance:" and states "None" or names the tool and the drafted parts. The check judges presence only. Each missing item causes the failure.

Resolution

Edit the PR description to add: - A "## Summary" section that states what merging the change does. - A "## Validation" section that names the commands or checks run and their results, or states "CI on this head". Rename or add this section; "## Testing" alone does not meet the check. - A "## Severity and risk" section with a severity (or "does not apply") and a stated risk. - A line that starts with "AI assistance:" followed by "None", or by the tool name and the parts it drafted or changed.

Full details: Public-Safe Content

Explanation

The code diff passes. The four changed files (service.rs, snapshots.rs, snapshots/tests.rs, service/tests.rs) add no credentials, tokens, keys, internal hostnames, URLs, account IDs, ARNs, personal absolute paths, or logs. The only pattern hits were CancellationToken identifiers, which are false positives. The PR description fails the check on two points: 1. Link to a private AI session. The "Created with Perplexity" footer in the PR description links to a Perplexity task page. That page is an AI-agent session, which is a private chat or task record, not a public document. The check prohibits links to private chats or documents. 2. Production measurement and incident detail. The Problem section gives a quarantine count over a multi-day window after a production volume change. It calls this an incident. The check prohibits production measurements and incident details in the description. Both items are in the PR description, not in the diff. The description is the untrusted PR text that this check covers. The values are not quoted here, per the check's instructions. The same task link also appears in linked issue #1761, which the check treats as context only. The technical fix itself is public-safe. The failure comes from the description text only.

Resolution

Edit the PR description (not the code): - Remove the Perplexity task link from the "Created with Perplexity" footer. Remove the footer or replace it with plain text that names no private session. - In the Problem section, remove the specific quarantine count and the production time window. Replace it with a generic statement, for example: "Quarantines occur when the server deadline fires before a detached capture finishes." Keep the mechanism explanation and drop the production numbers. - Check that the description does not repeat the same task link or counts elsewhere. No change to the diff is required.

Recent review info

Run configuration

• Source mode: Auto
• Repository: perplexityai/SPACE
• Branch: nancy/encrypted-capture-keep-runtime-on-waiter-expiry
• File: .coderabbit.yaml
• Commit: f2e331f
• Layers: Repository YAML; Enforced overrides: Organization
• Review profile: CHILL
• Plan: Enterprise
• Run ID: 9555c77e-8e55-436e-8c69-4f0a63448ed9

Commits

Reviewing files that changed from the base of the PR and between 5b01662 and f2e331f.

Files selected for processing: 4

• crates/l40_api/node/mgr/server/src/service.rs
• crates/l40_api/node/mgr/server/src/service/snapshots.rs
• crates/l40_api/node/mgr/server/src/service/snapshots/tests.rs
• crates/l40_api/node/mgr/server/src/service/tests.rs

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.