Land the context, connection-hook and reverse-cache race work on master

[djeebus]

Why

master and the commit we actually run have diverged. Our consumer pins this fork at f59b77c, which is on add-context and is not an ancestor of master — so master has never carried the seven commits below, and the RENAME/READLINK status fixes that just landed on master in #1 cannot reach us without converging the two.

This PR does the converging: it merges add-context into master. After it lands, master is the branch to pin, and there's one line of history again instead of two.

I did not write these seven commits. I'm opening this to converge the branches, not claiming a line-by-line review of them — hence the draft. Flagging below what I think actually needs a reader.

What's in it

Seven commits, in three groups:

Context threading (810c58b, f59b77c) — adds context.Context to four Handler methods and passes it through every on* handler:

Change(context.Context, billy.Filesystem) billy.Change
ToHandle(ctx context.Context, fs billy.Filesystem, path []string) []byte
FromHandle(ctx context.Context, fh []byte) (billy.Filesystem, []string, error)
InvalidateHandle(context.Context, billy.Filesystem, []byte) error

Most of the 27-file diff is this, one line per handler.

Connection hooks (8b8e0fb, f59b77c) — additive Server.OnConnect / Server.OnDisconnect, called from conn.serve. OnConnect returns both the context and the net.Conn, so a caller can wrap the connection as well as the context.

Reverse-cache race fixes (d9b8781…3776384) — searchReverseCache now holds RLock across the whole iteration rather than copying the slice out under a brief lock, and compares filesystems with == instead of reflect.DeepEqual. With tests.

What needs a reader

1. The Handler change is breaking for downstream consumers. Anything implementing Handler outside this repo must add the parameters. That also means these commits are not cherry-pickable to willscott/go-nfs without coordination — worth deciding separately whether we ever propose it upstream, since it widens the gap between this fork and upstream.

2. reflect.DeepEqual → == in searchReverseCache is a semantic change, not just a race fix. DeepEqual treated two distinct-but-structurally-identical filesystems as the same; == matches only the same instance. For a reverse cache keyed on a live filesystem that reads like the intended semantics, and dropping DeepEqual genuinely fixes the race (it walked mutable maps while other operations wrote them). Two caveats worth a conscious nod:

   • Two separate handles onto the same tree no longer share reverse-cache entries.
   • Comparing interfaces with == panics if the dynamic type is non-comparable — a billy.Filesystem implemented as a value struct containing a map, slice or func would take down the connection. Every implementation in play here is a pointer type (*memfs.Memory, osfs's *ChrootOS/*BoundOS, ours), so this is latent rather than live, but it's a sharper edge than DeepEqual had.

3. server.go carries a stray blank-line-only change. Harmless, just noise in the diff.

The ctx parameters are currently unused in CachingHandler's three methods — that's the point (they exist for implementations that do use them), but it's why the linter stays quiet about them.

Verification

I verified the merge locally rather than assuming GitHub's:

• Merges conflict-free (ort, no manual resolution) — the only overlap with Map RENAME and READLINK failures to the NFS statuses that name them #1 is nfs_onrename.go / nfs_onreadlink.go, and the changes are on different lines.
• The merged tree keeps add-context's ctx-carrying Handler, and its diff against the currently-pinned f59b77c is exactly the five files from Map RENAME and READLINK failures to the NFS statuses that name them #1 — so no API surface a consumer could trip on changes here.
• go build ./... clean; go test -race ./... green across both packages, including Map RENAME and READLINK failures to the NFS statuses that name them #1's new tests running on top of the ctx interface.
• gofmt clean, and golangci-lint v1.64.8 (the version this repo's Go workflow installs) reports zero findings.

Merging

Prefer a merge commit over a squash here: the seven commits have more than one author, and squashing them into one loses that attribution. #1 was squash-merged, which was right for a single-author PR; this one isn't that.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AeywBLV66AeSH8WuDfvv7Q

---

Generated by Claude Code

[cursor]

PR Summary

Medium Risk
Breaking Handler interface and semantic change in reverse-cache FS matching affect integrators and handle reuse; concurrent rename still uses DeepEqual on filesystem values.

Overview
The Handler API is breaking: Change, ToHandle, FromHandle, and InvalidateHandle now require context.Context, so any out-of-tree implementer must change signatures. The public interface spells the first parameter cxt on ToHandle while other methods use ctx or unnamed context.Context.

CachingHandler.searchReverseCache compares filesystems with == instead of reflect.DeepEqual, so reverse-cache reuse only applies to the same instance; separate handles to equivalent trees no longer share entries, and a non-pointer or otherwise non-comparable billy.Filesystem dynamic type can panic on ==. onRename still uses reflect.DeepEqual(fs, fs2) for the same-filesystem check, so rename validation was not aligned with the caching fix and can still walk mutable FS state under concurrency.

Server.OnConnect / OnDisconnect are additive; a hook that returns a bad or nil net.Conn from OnConnect would break the connection path. New ctx parameters are threaded through RPC handlers but are unused in CachingHandler handle methods, so cancellation or request-scoped data in those hooks is not wired through the cache layer yet.

^{Reviewed by Cursor Bugbot for commit f59b77c. Bugbot is set up for automated code reviews on this repo. Configure here.}

[djeebus]

Bugbot's summary raises one thing I did not flag in the description, and I've checked it — it's right, and it's worth a reader's attention before this merges.

onRename keeps the reflect.DeepEqual this branch removes elsewhere. On master there are exactly two places that compare filesystem values that way:

helpers/cachinghandler.go:108    if reflect.DeepEqual(candidate.f, f) {
nfs_onrename.go:36               if !reflect.DeepEqual(fs, fs2) {

This branch fixes the first and leaves the second — its only change to nfs_onrename.go is the ctx threading. So after this lands the two layers disagree: the handle cache compares filesystems by instance, while the RENAME same-filesystem check still deep-walks their internals.

The mechanism is the one 3776384 documents in its own code comment — DeepEqual traverses the filesystem's mutable maps, which other in-flight operations write. onRename runs that walk on two filesystems per RENAME, on any connection, while other connections are writing. Same pattern, same exposure.

To be straight about the evidence: this rests on reading the code plus the diagnosis the branch's own commit makes about the identical call site. I tried to pin it down with a two-connection -race harness (one renaming, one creating/removing) and the run hung before the detector reported anything, so I am not claiming a reproduced race — only that the second call site is the same construct the first was fixed for.

Not fixing it here. This PR's head is add-context, which I didn't create, so I'm not pushing commits onto it. I'd rather land this as-is and follow up on master immediately after, with:

• nfs_onrename.go:36 → fs != fs2, mirroring the caching fix. Note it inherits the same caveat I flagged for == in the description: it compares instance identity, and panics on a billy.Filesystem whose dynamic type is non-comparable. Every implementation in play is a pointer type, so that's latent — but if we'd rather not take that edge in two places, the alternative is worth discussing on the follow-up rather than here.
• The cxt → ctx parameter-name typo on ToHandle in the exported Handler interface, which Bugbot also spotted.

Neither blocks this merge; both are cheap, and I'll open them as one small PR once this is in. CI is green on this head (Build, CodeQL, Analyze).

---

Generated by Claude Code