Make the directory verifier cache reachable and safe to share by ryan-sana · Pull Request #3 · e2b-dev/go-nfs

ryan-sana · 2026-10-08T09:42:50Z

Summary

When a client lists a large directory, it asks for it in pages. go-nfs can cache the listing from the first page and serve the later pages from memory, keyed by a "cookie verifier". This PR fixes three problems with that cache.

                     BEFORE                               AFTER
1. Middleware        server ─► [logging] ─► [cache]       server ─► [logging] ─► [cache]
   hides the cache         asks only here: "no"                  follows Unwrap() ───────┘ found
                           → every page re-reads the dir         → dir read once

2. Two filesystems   fs A: /src [main.go]  ─┐ same key        fs A: /src [main.go #5001] → key A
   share a listing   fs B: /src [main.go]  ─┘ → A gets B's    fs B: /src [main.go #7002] → key B
                                              files

3. Fresh listing     cookie 0 + old key → cached copy         cookie 0 → always reads the dir

The cache was invisible behind middleware. The server only checked the outermost handler. Wrapping a CachingHandler in logging, metrics or panic-recovery middleware turned the cache off, and every page re-read the whole directory. The server now follows an Unwrap() Handler method down to a CachingHandler. Middleware without Unwrap behaves as before.
Two filesystems could share a listing. The key was a hash of the path and file names, and one CachingHandler serves every mounted filesystem. Two filesystems with the same folder (two copies of a repo, two node_modules) got the same key, so one could be shown the other's files. A Linux client then reports fileid changed and returns ESTALE. The key now includes each file's ID, and a cached listing is only returned for the same path.
A fresh listing could come from the cache. A listing that starts from cookie 0 now always reads the directory; the cache is only used for later pages.

Filesystems without file IDs (in-memory ones) keep the old key, so problem 2 can still happen between two of those.

Verification

Four new tests, one per fix, each fails when its fix is reverted. go test -race ./... passes, including the existing client/server test.
golangci-lint shows the same 21 issues as master, none in the changed files.
TestOnRenameStatusFromErrno is flaky on master too (failed 8 of 30 runs), unrelated to this change.

Three fixes to how READDIR and READDIRPLUS use the cookie verifier cache: - Find the CachingHandler through middleware. The server only checked the outermost handler, so wrapping a CachingHandler in logging, metrics or panic recovery silently disabled the cache and every page re-read the whole directory. A handler that exposes the one it wraps with an `Unwrap() Handler` method is now looked through. - Tell apart listings from different filesystems. The verifier was a hash of the path and the entry names only, and one CachingHandler serves every mounted filesystem, so two filesystems holding the same path and names shared a verifier and one could be served the other's listing (sizes and file IDs a client then sees change under it). The hash now includes each entry's file ID when the filesystem reports one, and DataForVerifier only returns a listing cached for the same path. - Read the directory for a listing that starts from cookie zero. The cache was consulted for any non-zero verifier, so a client that sent its last verifier with a fresh listing could get an outdated one.

tomassrnka

Fixes 1 and 3 are right and I confirmed the premise of fix 1 in belt: on main the recovery wrapper always sits outside helpers.NewCachingHandler and none of the four handler wrappers (tracing, metrics, logged, recovery) has an Unwrap, so the verifier cache has been dead in production and every page re-reads the directory. Tests pass under -race at this head and the two helpers tests fail against master's code as the body says.

Fix 2 is in the wrong layer and I would hold the merge on it; details inline. Two more things for the body:

This changes nothing for belt until the four handler wrappers get Unwrap() nfs.Handler. Say so, so the pin bump is not merged alone.
Upstream has not touched the verifier code since our fork point, so once fix 2 is reshaped this is worth offering to willscott/go-nfs.

Two things I noticed but are pre-existing and not for this PR: every cookie-0 listing inserts into the verifier LRU, so a tree walk can evict an in-flight paged listing; and getDirListingWithVerifier resolves the handle a second time although both callers already have fs and p.

tomassrnka · 2026-10-08T11:32:14Z

+		// Two filesystems can have a directory with the same path and file names.
+		// Mixing in each file's ID gives them different verifiers.
+		if info := file.GetInfo(c); info != nil {
+			vHash.Write(binary.BigEndian.AppendUint64(nil, info.Fileid))


This is the one I would not merge as is. The verifier goes back to the client and is checked on every later page. Index cookies stay valid as long as the sorted name list is unchanged, so names are the right input. Mixing in the inode means a write-temp-and-rename of one file between page 1 and page 2 (editors, npm, git all do this) flips the verifier, we answer NFS3ERR_BAD_COOKIE, and the Linux client restarts from cookie 0. A directory under continuous rewrite never finishes listing, and that is the git-on-a-volume path EN-2491 is about.

Telling filesystems apart belongs in the cache key, not on the wire. Keep the hash as path plus names; put the filesystem identity on the cached entry (see the comment in DataForVerifier).

tomassrnka · 2026-10-08T11:32:14Z


 func (c *CachingHandler) DataForVerifier(path string, id uint64) []fs.FileInfo {
-	if cache, ok := c.activeVerifiers.Get(id); ok {
+	if cache, ok := c.activeVerifiers.Get(id); ok && cache.path == path {


Suggest making this the place that separates filesystems instead of the hash. getDirListingWithVerifier already holds the billy.Filesystem from FromHandle; store it in verifier{} next to path and require the same instance here (searchReverseCache already compares filesystems the same way). That is exact, needs no inode numbers, and closes the residual the body concedes for in-memory filesystems. It also covers two volumes on different Filestore instances with equal inode numbers, which the inode approach does not. The interface gains a parameter, but this is our fork and belt implements neither method.

tomassrnka · 2026-10-08T11:32:14Z


+// cachingHandlerOf returns the CachingHandler inside h, following Unwrap through any
+// handlers that wrap it.
+func cachingHandlerOf(h Handler) (CachingHandler, bool) {


Following Unwrap means the server calls VerifierFor/DataForVerifier on the inner handler directly, past every wrapper above it. go-nfs has no recover() anywhere, so once belt adds Unwrap to its wrappers a panic while hashing a directory entry escapes belt's recovery handler and takes the orchestrator down. Either recover around the two cache calls here, or the belt follow-up has to keep recovery innermost around the cache. Worth a sentence in the doc comment on CachingHandler either way.

tomassrnka · 2026-10-08T11:32:14Z

+// cachingHandlerOf returns the CachingHandler inside h, following Unwrap through any
+// handlers that wrap it.
+func cachingHandlerOf(h Handler) (CachingHandler, bool) {
+	for h != nil {


No bound and no typed-nil guard. A wrapper whose Unwrap returns its receiver (easy copy-paste mistake) spins this connection goroutine at 100% CPU on every READDIR. A typed-nil *CachingHandler inner passes h != nil and the CachingHandler assertion, then panics on first use. Cap the walk at a small depth and treat a nil-valued result as not found.

tomassrnka · 2026-10-08T11:32:14Z

+	return nil, false
+}
+
 func hashPathAndContents(path string, contents []fs.FileInfo) uint64 {


This function exists twice (here and in helpers) and the two had already diverged before this PR: the helpers copy length-prefixes the path, this one does not. The PR edits both in lockstep. helpers already imports this package, so export one and delete the other.

tomassrnka · 2026-10-08T11:32:14Z

+
+func (h *cachingDirHandler) VerifierFor(string, []fs.FileInfo) uint64 { return 42 }
+
+func (h *cachingDirHandler) DataForVerifier(string, uint64) []fs.FileInfo {


The fake returns h.cached regardless of path or verifier, so the test still passes if the server passes the wrong path or swaps the cookie and verifier arguments. Record the arguments and assert them. Also add the case the doc comment promises, a wrapper without Unwrap, which is belt's shape today: it should fall through to the hashed verifier and never touch the cache.

ryan-sana · 2026-10-08T13:33:10Z

This is so bad! Let me see what went wrong here with many findings. Probably I missed running proper reviewing myself.

cla-bot Bot added the cla-signed label Oct 8, 2026

ryan-sana marked this pull request as ready for review October 8, 2026 09:44

Reword the verifier cache comments in plain language

fa762bc

tomassrnka reviewed Oct 8, 2026

View reviewed changes

Navigation Menu

Repository navigation

Make the directory verifier cache reachable and safe to share - #3

Make the directory verifier cache reachable and safe to share#3
ryan-sana wants to merge 2 commits into
masterfrom
fix/verifier-cache

ryan-sana commented Oct 8, 2026 •

edited

Loading

Uh oh!

tomassrnka left a comment

Uh oh!

tomassrnka Oct 8, 2026

Uh oh!

tomassrnka Oct 8, 2026

Uh oh!

tomassrnka Oct 8, 2026

Uh oh!

tomassrnka Oct 8, 2026

Uh oh!

tomassrnka Oct 8, 2026

Uh oh!

tomassrnka Oct 8, 2026

Uh oh!

ryan-sana commented Oct 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		func (h *cachingDirHandler) VerifierFor(string, []fs.FileInfo) uint64 { return 42 }

		func (h *cachingDirHandler) DataForVerifier(string, uint64) []fs.FileInfo {

Conversation

ryan-sana commented Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Verification

Uh oh!

tomassrnka left a comment

Choose a reason for hiding this comment

Uh oh!

tomassrnka Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

tomassrnka Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

tomassrnka Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

tomassrnka Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

tomassrnka Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

tomassrnka Oct 8, 2026

Choose a reason for hiding this comment

Uh oh!

ryan-sana commented Oct 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ryan-sana commented Oct 8, 2026 •

edited

Loading