Skip to content

nfsproxy: preserve caching handler through middleware chain - #3551

Open
Merlin0220 wants to merge 1 commit into
e2b-dev:mainfrom
Merlin0220:fix/nfs-proxy-caching-handler-passthrough
Open

nfsproxy: preserve caching handler through middleware chain#3551
Merlin0220 wants to merge 1 commit into
e2b-dev:mainfrom
Merlin0220:fix/nfs-proxy-caching-handler-passthrough

Conversation

@Merlin0220

@Merlin0220 Merlin0220 commented Aug 8, 2026

Copy link
Copy Markdown

Fixes #3552.

Problem

The NFS proxy creates a helpers.CachingHandler to cache directory listings by their NFS cookie verifier. However, the caching handler is subsequently wrapped by tracing, metrics, logging, and recovery middleware.

go-nfs detects verifier caching support by asserting the outermost handler as nfs.CachingHandler:

handler, ok := userHandle.(nfs.CachingHandler)

The middleware handlers only implemented nfs.Handler, so this assertion failed before reaching the underlying caching handler. Because the recovery middleware is always installed, verifier caching was bypassed regardless of whether the optional tracing, metrics, or logging middleware was enabled.

As a result, paginated READDIR and READDIRPLUS requests could not reuse the directory listing associated with their verifier and had to read the directory again.

Fix

Preserve nfs.CachingHandler through the complete middleware chain by forwarding VerifierFor and DataForVerifier to the wrapped handler.

Behavior Before After
Outermost handler implements nfs.CachingHandler No Yes
Verifier cache reaches helpers.CachingHandler No Yes
Directory listings can be reused across paginated requests No Yes

Changes

  • Add nfs.CachingHandler compile-time interface assertions to the tracing, metrics, logging, and recovery handlers.
  • Forward VerifierFor and DataForVerifier through each middleware layer.
  • Preserve panic recovery for verifier cache operations in the recovery handler.
  • Add a middleware-chain test that verifies the outer handler satisfies nfs.CachingHandler and forwards arguments and return values to the underlying handler.

Testing

go test ./pkg/nfsproxy/... -count=1
go test -race ./pkg/nfsproxy/... -count=1

Risk: activeVerifiers Cache Key Collision Across Sandboxes

Enabling verifier caching introduces a potential cache-key collision in
activeVerifiers. If the mount path and an enumerated file name are identical,
they may produce the same ID. This could cause cache entries to be reused across
different sandboxes, violating cache isolation and potentially leading to
incorrect verifier or file-handle resolution.

If this scenario is possible, the current cache should not be used as-is.
The cache should either be disabled or redesigned with mount-point-scoped, collision-resistant keys.

This risk is related to #3555.
The proposed solution is to allocate an independent file-handle cache for each
mount point, using the mount-point identity as the cache
shard key. This prevents cache eviction, lookup, and synchronization in one
sandbox from affecting another.

@cla-bot

cla-bot Bot commented Aug 8, 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: shangyan02.
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

@Merlin0220
Merlin0220 force-pushed the fix/nfs-proxy-caching-handler-passthrough branch from 1b7e69d to 0ff32c7 Compare August 8, 2026 08:38
@cla-bot

cla-bot Bot commented Aug 8, 2026

Copy link
Copy Markdown

We require contributors to sign our Contributor License Agreement, and we don't have @Merlin0220 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'

@Merlin0220 Merlin0220 closed this Aug 8, 2026
@cla-bot cla-bot Bot added the cla-signed label Aug 8, 2026
@cla-bot

cla-bot Bot commented Aug 8, 2026

Copy link
Copy Markdown

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

@Merlin0220 Merlin0220 reopened this Aug 8, 2026
@cla-bot

cla-bot Bot commented Aug 8, 2026

Copy link
Copy Markdown

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

@Merlin0220

Copy link
Copy Markdown
Author

We require contributors to sign our Contributor License Agreement, and we don't have @Merlin0220 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 check

@cla-bot

cla-bot Bot commented Aug 8, 2026

Copy link
Copy Markdown

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

@leonmeijer leonmeijer left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This exposes one process-wide verifier cache across every mounted sandbox, but the pinned go-nfs caching handler derives a verifier only from the namespace-relative path and entry names and DataForVerifier does not validate the path or filesystem. Two sandboxes with the same directory path and names can therefore collide and receive each others cached FileInfo listings on subsequent READDIR pages. Isolate verifier state by mounted filesystem or include and validate mount identity before enabling this passthrough.

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.

nfsproxy: middleware chain hides CachingHandler and disables directory verifier caching

2 participants