[shehan-pplx]

Problem

Encrypted snapshot reads pass through the inner filesystem's page cache and a
file-backed loop device. Buffered loop I/O also caches the backing image and uses
the loop driver's buffered submission path. This can add memory and I/O overhead
without changing the snapshot's contents.

Direct loop I/O changes both eligible backing-file cache use and request
submission. This PR does not claim that cache duplication alone explains restore
latency, or that every workload becomes faster.

Solution

• Prefer direct I/O only for newly attached encrypted snapshot volumes.
• Keep the existing buffered/AUTOCLEAR attachment setup, then request
  LOOP_SET_DIRECT_IO on the held descriptor before handing it to dm-crypt.
• Read back the effective loop mode. Unsupported configurations retain buffered
  I/O with a warning; unrelated setup/readback errors propagate with descriptor
  cleanup.
• Preserve the mode of mounted or adopted volumes until natural detach/reopen.
  Rootfs origin/COW, fork, resize, and other named volumes remain buffered.
• Leave encryption, compression, checksums, snapshot format, sync/seal ordering,
  and the existing readiness timeout unchanged. No prefetch or snapshot rewrite.

Direct mode is not a durability guarantee. Existing flush and publication
requirements remain necessary, and individual filesystem requests may still use
buffered I/O.

Coverage

Extend existing tests rather than adding a parallel smoke family:

• Kernel-backed device tests cover effective mode, unsupported backing fallback,
  pinned inode identity, payload preservation, failed-key cleanup/retry, legacy
  buffered adoption, rootfs mode, and detach.
• The existing encrypted re-suspend smoke now retains a live process, checks all
  synthetic RAM and disk bytes, restores a published snapshot cross-node,
  mutates both payloads, then re-suspends and restores again.
• Scope the existing busy-close assertion to its owned loop rather than unrelated
  host loop devices.

Validation status

Draft while dedicated Linux/KVM qualification is in progress.

• Passed: formatting; affected node-manager library check with all features;
  node-manager and service Clippy with all targets/features and warnings denied.
• Passed: exact-candidate all-features test-binary build and 28 nonprivileged
  device-backing, encrypted-volume, and rootfs tests.
• Passed separately on Linux/KVM: all three privileged device-backing,
  busy-close/recovery, and sparse encrypted-fork tests, each with one executed
  test and zero ignored tests. The fallback case uses a verified 4 KiB-sector
  backing filesystem beneath the unchanged 512-byte loop, rather than assuming
  a filesystem name implies unsupported direct I/O. Task-owned loop, mapper,
  mount, and fixture cleanup was verified.
• Passed: Python syntax/registry validation and 25 existing smoke-helper/suite
  tests against the edited source.
• Passed on a5b091e8e's CI merge: the required Pull CI suite, including
  all 14 Kind/Firecracker shards. The encrypted re-suspend scenario
  executed with a 32 MiB RAM payload and 8 MiB disk payload, checked both mutated
  generations and retained process identity, restored cross-node, and reported
  no cleanup failures. This is correctness evidence, not a matched latency result.
  Build/image provenance was verified; the five modified production files and
  scenario match the PR head byte-for-byte in the tested synthetic merge.
  Direct-substrate CI was skipped on this test-only update; this run does not
  supply Direct-substrate execution evidence.
• Passed on a5b091e8e: the privileged tests also verify a real reflink of a
  sealed snapshot remains byte-for-byte unchanged after direct-I/O writes to
  the canonical snapshot, sync, and release. A fresh direct-mode reopen reads
  the canonical snapshot's updated payload. The old-file hash check follows
  file-scoped cache eviction; no global cache flush is used.
• The test-only payload hashing/mutation deadline scales with the opt-in RAM
  payload (96 seconds for 3 GiB); the default small test remains at 20 seconds.
  This does not extend production readiness or the one-attempt readiness probe.
• Passed: required CI on the latest head, a5b091e8e.
• Pending: matched lifecycle/performance comparisons. The 3 GiB-payload
  baseline completed its first restore and byte checks but failed its second
  snapshot publication, with multipart-completion timeouts followed by a
  successful retry response and missing-object verification. This matches the
  local test server's in-progress completion retry behavior,
  not an established direct-I/O regression or total-object size ceiling.
  The failed run remains a failure. After guarded cleanup, the remaining
  comparison uses a 1 GiB payload in the same 8 GiB guest without increasing
  production timeouts or memory limits.
• Unexpected direct-I/O ioctl/readback failure cleanup is currently supported by
  descriptor ownership review, not a dedicated fault-injection result.

Warm repeated reads or write-heavy capture can lose backing-file buffering
benefits. Runtime qualification must check capture/re-suspend as well as restore;
this draft is not a merge or deployment recommendation.

Published restore stages a new backing inode, so warming a source snapshot is
not proof that the loop's consumed inode has a warm page cache. Any source-cache
experiments must retain that distinction; cold page cache is also not a claim
of cold physical storage.

_{Created with Perplexity}

[coderabbitai]

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 for #1779 (author login shehan-pplx) fails three of the four required items. This check tests presence only, so no code inspection was needed. The contributor-auth…	Edit the PR description so that it contains all four items: - Add a "## Summary" section. State what merging the change does, in a few sentences. - Rename "## Validation status" to "## Validation", or add a "## Validation" section. Name wha…
Public-Safe Content		The PR description still ends with the attribution footer "Created with Perplexity". The footer contains a hyperlink to a Perplexity task page (an AI session thread, under /computer/tasks/). The che…	1. Edit the PR description. Delete the final "Created with" footer line, including its hyperlink. Leave the rest of the description unchanged. 2. If attribution must stay, write it as plain text with no link. Do not link to the task or chat…

Passed checks: 3 passed

Check name	Status	Explanation
Bug Fixes Name A Regression Test		The check applies only when the PR fixes a defect in application behavior under crates/, shim/, or ui/. PR #1779 (shehan-pplx) is an optimization. Its title is "node-mgr: prefer direct I/O for snapsho…
Weakened Safeguards Are Justified		The PR's own commits (37750b7, c77b557, a5b091e) touch 11 files: node-manager device backing and encrypted-volume code and tests, rootfs_dmcrypt, launch_prepare, and the smoke scenario, fixtures, a…
Agent Tooling Runs No New Commands		The check does not apply to this pull request. The PR has three commits (37750b7, c77b557, a5b091e) on top of parent 3a6e89f. Their combined diff touches 11 files, all in the node-manager Rust cra…

Full details: Bug Fixes Name A Regression Test

Explanation

The check applies only when the PR fixes a defect in application behavior under crates/, shim/, or ui/. PR #1779 (shehan-pplx) is an optimization. Its title is "node-mgr: prefer direct I/O for snapshot backing loops". Its Problem section says buffered loop I/O "can add memory and I/O overhead without changing the snapshot's contents." The Solution adds a new opt-in path: it requests LOOP_SET_DIRECT_IO for newly attached encrypted snapshot volumes and reads back the effective mode. The description does not describe a defect that the PR corrects. The Validation section includes a test-only change that scopes a busy-close assertion to the owned loop. That is a test adjustment, not an application bug fix. The check is therefore inapplicable, and the PR passes by its terms. No review threads were returned, so no other finding changes this result.

Full details: Weakened Safeguards Are Justified

Explanation

The PR's own commits (37750b7, c77b557, a5b091e) touch 11 files: node-manager device backing and encrypted-volume code and tests, rootfs_dmcrypt, launch_prepare, and the smoke scenario, fixtures, and README. None of these is a CI workflow, lint or allowlist file, nextest config, Cargo lints, AGENTS.md, CLAUDE.md, CONTRIBUTING.md, or a review-guidance file. I searched the added lines for allow/expect/ignore/skip/xfail, continue-on-error, nextest, and lints and found no matches. I used the PR's own commit range, not the merge-base diff, because the branch base is far behind main. Removed or narrowed test checks, all with replacements in the same diff: - device_backing/tests.rs: the single LoopDevice::attach pinned-inode check is now a loop over Buffered and Direct modes. It keeps matches_loop and the replacement-inode !matches_loop assertions and adds assert_loop_mode. The SmallFilesystem Drop guard is renamed to MountedFilesystem and keeps its umount behavior. - encrypted_volume/tests.rs: the fresh/non-fresh loop is generalized to named volumes and modes. The memory payload read-back assertion is kept. - The seal-time check "the seal left read-only loop devices" (host-wide /sys/block scan) is narrowed to the owned device's ro flag. The description states this: "Scope the existing busy-close assertion to its owned loop rather than unrelated host loop devices." - The smoke sealed_snapshot allocation bound is kept with a 1 GiB floor. It now scales to memory_mib * 3 MiB for opt-in large RAM payloads only. An adjacent comment states the reason ("Two incompressible generations plus metadata must still remain sparse."). The default small test keeps the original bound. The scaling is not mentioned in the PR description, but the code comment gives the reason. The rewritten smoke scenario keeps the sparse-allocation, identity, and payload checks in stronger form. No guard, lint, or allowlist was weakened, so the check passes.

Full details: Agent Tooling Runs No New Commands

Explanation

The check does not apply to this pull request. The PR has three commits (37750b7, c77b557, a5b091e) on top of parent 3a6e89f. Their combined diff touches 11 files, all in the node-manager Rust crate or the space_smoke Python harness: - crates/l40_api/node/mgr/server/src/device_backing.rs and its tests.rs - encrypted_volume.rs and its tests.rs - rootfs_dmcrypt.rs and rootfs_dmcrypt/fork.rs - service/launch_prepare.rs - space_smoke/fixtures.py, space_smoke/guest/snapshot_state.py (added), space_smoke/scenarios/README.md, and space_smoke/scenarios/local_encrypted_resuspend_preserves_cow.py A path-filtered diff over the PR range for .agents, .claude, .codex, .github, AGENTS.md, CLAUDE.md, SKILL.md, and plugin files returned nothing. The PR adds no plugin manifest, skill, command, or guideline file. It adds no hook, MCP server, LSP server, SKILL.md frontmatter key, or inline shell substitution. A diff against origin/main lists agent plugin files such as .agents/plugins/pplx-space/plugin.yaml. Those entries come from base drift, not from this PR. The origin/main comparison covers 1,633 files, so it does not reflect the PR change set.

Full details: Description Follows The Pr Template

Explanation

The PR description for #1779 (author login shehan-pplx) fails three of the four required items. This check tests presence only, so no code inspection was needed. The contributor-authored description contains these headings: "## Problem", "## Solution", "## Coverage", and "## Validation status". It has no other level-2 headings. 1. "## Summary" section: missing. The "## Problem" and "## Solution" sections do not state what merging the change does under this heading. Failed. 2. "## Validation" section: partly present. The heading "## Validation status" names the checks run and their results, so it is close to the required section. A strict match on the heading fails. This item does not change the verdict. 3. "## Severity and risk" section: missing. No section gives a severity or "does not apply". Risk text about warm reads and write-heavy capture appears under "## Validation status", not under a severity and risk heading. Failed. 4. "AI assistance:" line: missing. The footer contains only a Perplexity attribution link. No line starts with "AI assistance:". Failed. The prior FAIL is not addressed. The current description still lacks the Summary, Severity and risk, and AI assistance items. No change to the description is evident in the supplied record.

Resolution

Edit the PR description so that it contains all four items: - Add a "## Summary" section. State what merging the change does, in a few sentences. - Rename "## Validation status" to "## Validation", or add a "## Validation" section. Name what was run and the result. Keep the existing validation content. "CI on this head" is also accepted. - Add a "## Severity and risk" section. Give a severity, or write "does not apply". Then state the risk. - Add a line that starts with "AI assistance:". Write "AI assistance: None", or name the tool and the parts it drafted or changed.

Full details: Public-Safe Content

Explanation

The PR description still ends with the attribution footer "Created with Perplexity". The footer contains a hyperlink to a Perplexity task page (an AI session thread, under /computer/tasks/). The check fails on links to private chats. Nothing in the PR shows that this task page is public, so it may be a private chat link. The footer is the last line of the PR description. The description text in this run matches the prior failing run. The footer was not removed or changed. The URL is not quoted here. Other content passed the check: - The GitHub Actions run link (perplexityai/SPACE) and the raw.githubusercontent.com link (versitygw) are public resources. - The validation text reports synthetic test sizes (32 MiB, 8 MiB, 1 GiB, 3 GiB). It does not report production measurements. - The description names no internal hostnames, cloud account IDs, ARNs, or real cluster names. - The description describes no unfixed security vulnerability. - The prior run found only a generic /home/user/... placeholder in a fixture. It is not a real workstation path. I did not re-scan the full diff in this run. The verdict rests on the PR description. The prior run's diff scan found no other blocking content.

Resolution

1. Edit the PR description. Delete the final "Created with" footer line, including its hyperlink. Leave the rest of the description unchanged. 2. If attribution must stay, write it as plain text with no link. Do not link to the task or chat session. 3. Re-run this check on the edited description. No code change is needed.