Skip to content

fix(storage): allow mounted files beyond the callback payload limit - #1879

Open
joshuajbouw wants to merge 3 commits into
mainfrom
fix/mount-large-file-io
Open

fix(storage): allow mounted files beyond the callback payload limit#1879
joshuajbouw wants to merge 3 commits into
mainfrom
fix/mount-large-file-io

Conversation

@joshuajbouw

@joshuajbouw joshuajbouw commented Sep 8, 2026

Copy link
Copy Markdown
Member

Linked Issue

Closes #1878

Summary

Fix mounted writes and resizing beyond 4 MiB. The shared kernel callback had applied the per-request payload ceiling to the resulting whole-file length, making a real FSKit fsync fail with EINVAL at 4 MiB+1 byte.

Changes

  • Keep the 4 MiB request bound; validate file lengths against the signed native offset domain instead.
  • Stream old ranges, replacement bytes and zero-filled gaps into atomic content publication instead of allocating a whole-file buffer.
  • Expose streaming publication through owner, subtree and workspace adapters. Workspace publication retains branch quota checks and bounded staging when a quota is configured.
  • Cover large writes, overwrites, extension, truncation, source failures and quota refusal.
  • Bound newly materialized gap bytes to the existing 4 MiB callback budget. Oversized resize/write gaps fail before publication; ordinary contiguous growth is not capped at 4 MiB. Sparse extensions remain unsupported.

Verification

  • cargo test --locked -p astrid-kernel --lib storage_mount -- --test-threads=1: 24 PASS (original candidate).
  • cargo test --locked -p astrid-storage --lib content::store::workspace -- --test-threads=1: 24 PASS.
  • cargo clippy --locked -p astrid-kernel -p astrid-storage --all-targets -- -D warnings: PASS.
  • cargo fmt --all --check and git diff --check: PASS.
  • Real macOS 26.6.2 FSKit mount, local development-signed app build 664 and optimized candidate daemon: three 32 MiB write/fsync/SHA256-read samples completed. Previously the first sample failed EINVAL.
  • Separate actual-mount regression: 8 MiB+ write, overwrite crossing 4 MiB, zero-filled extension and gap, shrink above 4 MiB, sync, unmount/remount, SHA256 readback, truncate to zero: PASS.

Claim limits: this is correctness, not a random-write performance optimization. Replacement still rebuilds the content graph; workspace deferred records remain retained until publication. On the final 9661cba source build, mounted 32 MiB fsync took 3.63–3.93 seconds locally; all three samples and the remount regression passed. Warm OS-cache reads are not backend throughput. Linux/Windows use the shared kernel adapter, but this PR does not claim a new packaged native execution on those hosts.

Latest repair verification: cargo test -p astrid-kernel storage_mount --lib -- --quiet: 25 PASS; focused range tests: 2 PASS; cargo clippy -p astrid-kernel --lib --tests -- -D warnings, formatting and diff checks PASS. The new regression rejects both SetLength and Write gaps beyond budget, including i64::MAX, with original bytes unchanged. Exact-budget extension and shrinking remain valid. Earlier mounted measurements are retained as earlier-candidate evidence, not a rerun of this repair. CI workflow repair #1891 is included as a prerequisite until it lands.

Test Plan

Run the commands above. On an admitted writable mount, write more than 4 MiB, fsync, patch across the boundary, extend/shrink, sync and remount, then compare complete file bytes. Verify an oversized individual RPC is still rejected and a quota/source failure preserves the prior file.

AI / Tool Assistance

Assisted-by: Codex:GPT-6-Astra

Implemented and reviewed the adapter changes, source/publication ordering, integer bounds, quota preservation and regression coverage; exercised the real FSKit mount in a disposable AOS home.

Checklist

  • Linked to an issue
  • Changelog fragment added under changes/1878.fixed.md
  • I understand every change in this PR and can explain its design, risks, and validation.
  • I reviewed and tested any meaningful tool-generated output included in this PR.
  • Every non-bot, non-merge commit has a matching Signed-off-by trailer.

Copilot AI balanced review requested due to automatic review settings September 8, 2026 00:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Near-i64::MAX lengths can trigger effectively unbounded materialization under a global lock, and administrative projection still allocates the entire file.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes mounted writes and resizing beyond the 4 MiB callback payload limit through streamed, atomic content replacement.

Changes:

  • Streams range replacement, truncation, extension, and zero-filled gaps.
  • Adds streaming publication across filesystem and workspace adapters.
  • Adds regression, quota, and failure-path coverage.
File summaries
File Description
crates/astrid-storage/src/content/store/workspace/tests.rs Tests streaming, quota refusal, and atomic failures.
crates/astrid-storage/src/content/store/workspace/operations.rs Adds quota-aware streamed branch publication.
crates/astrid-storage/src/content/store/workspace/filesystem.rs Exposes streaming writes through workspace filesystems.
crates/astrid-kernel/src/storage_mount/tests.rs Updates native-offset and truncation tests.
crates/astrid-kernel/src/storage_mount/filesystem/range.rs Implements streamed range replacement.
crates/astrid-kernel/src/storage_mount/filesystem.rs Integrates streaming and native offset bounds.
changes/1878.fixed.md Documents the mounted-file fix.
Review details

Suppressed comments (2)

crates/astrid-kernel/src/storage_mount/filesystem.rs:387

  • The same unbounded-gap problem applies to random writes: a one-byte payload at an offset just below i64::MAX passes this check, after which Replacement emits the entire zero-filled gap while the global mutation lock is held. Since the default storage quota is effectively this same value, the 4 MiB payload bound no longer prevents a trivial disk-exhaustion/global-stall request. Add a practical resulting-length/materialization bound or sparse-gap support before accepting such offsets.
    if current_length.max(end_offset) > i64::MAX as u64 {

crates/astrid-kernel/src/storage_mount/filesystem.rs:334

  • For system-owner mounts this is not end-to-end streaming: admin_projection::execute calls project_file after execute_blocking, and project_file reads 0..length into one Vec before writing the host file (admin_projection.rs:112-135). Large writes/resizes can therefore exhaust memory after the authoritative content commit, returning an error while the live host projection still contains the old file. Stream ranges directly into the temporary projection file (and preserve publication/projection consistency) rather than materializing the whole value.
            range::replace(filesystem, &path, current_length, length, length, &[])?;
  • Files reviewed: 7/7 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread crates/astrid-kernel/src/storage_mount/filesystem.rs
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.

fix(storage): allow mounted files larger than one callback payload

2 participants