Skip to content

fix(orchestrator): guard chunk reads against cache unmapping - #3816

Open
Pn7Hao wants to merge 2 commits into
e2b-dev:mainfrom
Pn7Hao:fix/chunker-readat-cache-lifetime
Open

Pn7Hao wants to merge 2 commits into
e2b-dev:mainfrom
Pn7Hao:fix/chunker-readat-cache-lifetime

Conversation

@Pn7Hao

@Pn7Hao Pn7Hao commented Nov 12, 2026

Copy link
Copy Markdown

Concurrent diff-cache eviction can unmap a cache after Chunker.Slice returns but before Chunker.ReadAt copies the bytes. The NBD fault guard then reports a MemoryFaultError, bypassing File.readAt's existing CacheClosedError retry and failing the guest read.

Separate waiting for data from acquiring the mmap slice. Protect mmap-header access and keep the cache read lock through ReadAt's copy. The lock is released before fetching, so a pending close cannot deadlock a fetch that needs its own read lock. A ready streaming prefix remains readable before the full chunk is marked cached; empty reads at EOF also retain their behavior. Slice still returns borrowed storage whose lifetime belongs to its caller.

The regression test closes the cache from the existing readiness metric callback, placing eviction deterministically between readiness and copying without adding a production test hook. On unmodified df41cd32, it produces MemoryFaultError; with this change it returns CacheClosedError and leaves the destination untouched. Two additional tests cover partial streaming reads and empty reads at EOF.

Validation:

  • go test -race ./pkg/sandbox/block -run '^TestChunker_' -parallel 2 -count=1 -timeout 90s passes.
  • The new eviction regression fails on unmodified df41cd32 with the expected invalid-memory fault.
  • golangci-lint run --new-from-rev=HEAD --timeout=3m ./pkg/sandbox/block/... passes before committing.
  • Full repository validation was attempted but not completed locally: the unrestricted block-package run exhausted the local machine's memory. The scoped checks above were rerun with a 2 GiB process-group memory limit.

Copilot AI balanced review requested due to automatic review settings November 12, 2026 06:08
@cla-bot

cla-bot Bot commented Nov 12, 2026

Copy link
Copy Markdown

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: Pn7Hao.
This is most likely caused by a git client misconfiguration; please make sure to:

  1. check if your git client is configured with an email to sign commits git config --list | grep email
  2. If not, set it up using git config --global user.email email@example.com
  3. Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

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.

🟢 Approval recommended

No active review issues are recorded for the change protecting chunk reads from concurrent cache unmapping.

No open issues 🎉

What changed in this PR

This PR protects orchestrator chunk reads from concurrent cache unmapping, allowing cache-closed errors to reach the existing retry logic instead of failing guest reads with memory faults.

Changes:

  • Separates data readiness from mmap access and holds the cache read lock through copying.
  • Preserves streamed-prefix reads and borrowed Slice behavior.
  • Adds regression tests for eviction after readiness, partial streaming reads, and empty reads at EOF.
File Description
packages/​orchestrator/​pkg/​sandbox/​block/​streaming_chunk.go Guards mmap access and copying against concurrent cache closure.
packages/​orchestrator/​pkg/​sandbox/​block/​streaming_chunk_close_test.go Tests eviction handling, streamed-prefix reads, and empty EOF reads.

Review effort: Balanced

@Pn7Hao
Pn7Hao force-pushed the fix/chunker-readat-cache-lifetime branch from d1e419f to 398e645 Compare November 12, 2026 06:36
@cla-bot

cla-bot Bot commented Nov 12, 2026

Copy link
Copy Markdown

We require contributors to sign our Contributor License Agreement, and we don't have @Pn7Hao on file. You can sign our CLA at https://e2b.dev/docs/cla . Once you've signed, post a comment here that says '@cla-bot check'

@Pn7Hao

Pn7Hao commented Nov 12, 2026

Copy link
Copy Markdown
Author

@cla-bot check

@cla-bot

cla-bot Bot commented Nov 12, 2026

Copy link
Copy Markdown

We require contributors to sign our Contributor License Agreement, and we don't have @Pn7Hao on file. You can sign our CLA at https://e2b.dev/docs/cla . Once you've signed, post a comment here that says '@cla-bot check'

@cla-bot

cla-bot Bot commented Nov 12, 2026

Copy link
Copy Markdown

The cla-bot has been summoned, and re-checked this pull request!

@Pn7Hao

Pn7Hao commented Nov 12, 2026

Copy link
Copy Markdown
Author

@cla-bot check

@cla-bot cla-bot Bot added the cla-signed label Nov 12, 2026
@cla-bot

cla-bot Bot commented Nov 12, 2026

Copy link
Copy Markdown

The cla-bot has been summoned, and re-checked this pull request!

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants