Skip to content

feat(nfs): handle-table bound, exclusive create, and set-group-ID inheritance - #13

Merged
pi0 merged 8 commits into
mainfrom
fix/nfs
Jul 31, 2026
Merged

feat(nfs): handle-table bound, exclusive create, and set-group-ID inheritance#13
pi0 merged 8 commits into
mainfrom
fix/nfs

Conversation

@pi0x

@pi0x pi0x commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

Three items off .agents/roadmap.md, all of them NFS, plus the one rule that had
to leave src/nfs/ to be stated once (src/ownership.ts).

Bound the file handle table with an LRU (f01bf33)

Neither NFS version has a FORGET, so nothing on the wire ever tells the server
a client is done with a handle. Half of that was already closed — an entry whose
last path is detached is dropped — and what was left was one entry per path a
client currently has a name for, held for the life of the server however long ago
it looked. maxHandles caps that, least recently used first.

The decision behind it: the default is still no cap, which is what this
server has always done. Recency is stamped everywhere a handle is used
decode(), at(), pathOf(), every bind/remap attach — so the entries a
client is working through are the last ones considered, and the LRU order is
#byId's own insertion order, so nothing can fall out of step with
#byPath/#byKey. Eviction is safe by construction: #nextId only counts up,
so a handle whose entry went away answers ESTALE, never somebody else's file.
But it is not free. It costs an NFSv3 client one round trip, and an NFSv4.1
client that had the file open rather more — v4/session.ts keys open and
lock state by entry id, so the re-lookup is a different file as far as share
reservations are concerned. That asymmetry is why the number is opt-in rather
than defaulted. The root is never a victim (PUTROOTFH/PUTPUBFH/MNT encode
it from a field, not from a lookup), and neither is the entry being bound.

Exclusive create on v3 CREATE and v4.1 OPEN (6615c3c)

CREATE with EXCLUSIVE answered NFS3ERR_NOTSUPP and OPEN with
EXCLUSIVE4/EXCLUSIVE4_1 answered NFS4ERR_INVAL, because there is nowhere in
FsDriver to commit a verifier to.

There does not have to be. What EXCLUSIVE actually asks for is that a
retransmission recognise its own creation, so the verifier lives in an
ExclusiveCreates table on the shared state (src/nfs/util.ts) — one table,
so a create over v3 and a retry over v4 are one server answering once (RFC 1813
§3.3.8, RFC 5661 §18.16.3). It is bounded twice: a 120 s window, because that is
how long the original request can still plausibly be in flight, and 256 entries,
oldest insertion evicted first. Both edges fail the same safe way — the retry
stops being recognised and gets EXIST, never a wrong file — and a restart
loses the table entirely, which the class doc states rather than implying
otherwise.

Same verifier returns the existing object, a different one is EXIST, and the
entry is dropped by REMOVE, by both sides of a RENAME, and by the SETATTR
that §3.3.8 treats as the client's commit. v3's arm carries no sattr3 so it
applies no attributes; EXCLUSIVE4_1's cva_attrs go through the same
#settableOrRefuse/#applyAttrs/attrset path UNCHECKED4 does.

Set-group-ID inheritance for new entries (dfc88ef)

A driver creates everything as the server process, so each session hands the
result to its caller afterwards. The group half of that was wrong: it gave every
new entry the caller's gid, where a set-group-ID parent means the entry takes the
parent's group and a new directory takes the bit as well.

The decision was to make that one rule rather than two session patches:
src/ownership.ts is a transcription of inode_init_owner() (fs/inode.c) —
the place the kernel makes this decision for every local filesystem and makes for
no userspace one — with POSIX mkdir(2)/open(2) declining to pick between the
two arms and inode(7) stating the choice. It also clears S_ISGID on a new
group-executable whose creator is in neither the effective nor the supplementary
group the AUTH_SYS credential carried, reading CAP_FSETID as uid 0, which is
all a userspace server can see of a remote caller's capabilities.

No new stat: v3 already read the parent for wcc_data (#preOp is split so
the stat can be kept) and v4 already read it for change_info4 and the NOTDIR
check. The only added driver call is the chmod a new directory needs, and it
runs after the lchown because chown(2) clears set-group-ID in the other
order. Rename is untouched — the rule is about creation. 9P needed nothing; its
client computes both halves (v9fs_get_fsgid_for_create()) and they arrive on
the wire, so its #claim comment now says that instead of claiming to match NFS
character for character.

Deliberately left open

All four are written into .agents/roadmap.md:

  • A default for maxHandles. "It is opt-in because an eviction costs an
    NFSv4.1 client more than a re-LOOKUP: src/nfs/v4/session.ts keys open and
    lock state by entry id, so evicting a handle a client has open costs it an
    OPEN as well. Pinning entries with live v4 state — and only then picking a
    default — is what is left." A directly constructed Nfs3Session/Nfs4Session
    also builds its own table and ignores the option; every public path goes
    through the router, so that is the tests and the CLI only.
  • suppattr_exclcreat (75) is still unadvertised in src/nfs/v4/attr.ts's
    SUPPORTED_ATTRS. "Exclusive create shipped, and its verifier lives beside the
    file rather than in an attribute, so the honest value is now the full settable
    set — the attribute's 'deliberately absent, it is the OPEN step's call' note
    predates the OPEN step having made it."
  • Set-gid inheritance on the FUSE session. "FUSE is the gap, and
    #claim in src/fuse/session.ts says why it was not simply mirrored: the NFS
    sessions read the parent anyway (for wcc_data and change_info4) while
    nothing on the FUSE path does, so the rule would add an lstat to every create
    for every caller — including the daemon-is-the-caller case that currently costs
    no driver call at all — and the membership half needs FUSE_CREATE_SUPP_GROUP
    (7.38), which src/fuse/init.ts does not ask for. Both are decisions to take
    deliberately, not omissions."
  • Per-call caller credentials in FsDriver: considered, and not the answer.
    Three things against it, and they compound: "node:fs/promises has no
    credential parameter and the POSIX way to get one — setfsuid(2)/setfsgid(2)
    — is per-thread state Node does not expose and the threadpool would scramble,
    so node-fs could never implement it and unstorage/S3 have no ownership to
    implement it with; the memory driver, the only one that could, enforces no
    permissions at all, so the sole observable effect of a credential inside it is
    the initial owner and group, which src/ownership.ts now gets right; and
    invariant 5 means every session must keep the lchown path anyway for drivers
    without the extension, so the whole surface buys one driver's atomicity at the
    price of two permanent code paths." What would reopen it is a driver that
    enforces permissions itself.

Testing

pnpm test is green — lint, typecheck, and the Tier-0/Tier-1 suites, 2940 tests.
New coverage lands in test/nfs/handles.test.ts (the LRU: recency stamping, root
and bound-entry exemption, ESTALE after eviction), test/nfs/v3/session.test.ts
and test/nfs/v4/session.test.ts (both exclusive-create arms, the same-verifier
replay, the drops on REMOVE/RENAME/SETATTR, and set-gid inheritance through
both sessions), test/nfs/session.test.ts and test/nfs/server.test.ts (the
shared table across versions), and test/index.test.ts (src/ownership.ts
directly).

The Tier-2 real-mount suites were not run. pnpm test:root,
pnpm test:mount, pnpm test:nfs:mount and pnpm test:pjdfstest need sudo and
were not executed for this branch, so nothing here has been exercised against a
real kernel client. The pjdfstest numbers in .agents/pjdfstest-results.md were
not re-measured either — the note there says so, and set-gid inheritance is not
something pjdfstest covers.

🤖 Generated with Claude Code

pi0 and others added 4 commits July 31, 2026 18:50
Neither NFS version has a FORGET, so the table grew to one entry per path
a client had ever named and stayed there for the life of the server.
`maxHandles` caps it, least recently used first; the default is still no
cap, which is what this server has always done.

Recency is stamped where a handle is used — decode(), at(), pathOf() and
#attachPath() — so an entry a client is working through is never the
victim. The order is #byId's own insertion order, so nothing can fall out
of step with #byPath/#byKey. Eviction runs the same teardown #detachPath
does, and the root stays exempt: PUTROOTFH and MNT encode it from a field
rather than from a lookup.

An eviction costs the holder an ESTALE and a re-lookup, and an NFSv4.1
client that had the file open an OPEN as well, since v4 keys its state by
entry id. That is why there is no default cap.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CREATE/EXCLUSIVE answered NFS3ERR_NOTSUPP and OPEN/EXCLUSIVE4{,_1}
answered NFS4ERR_INVAL, because there is nowhere in FsDriver to commit a
verifier to. There does not have to be: what EXCLUSIVE asks for is that a
*retransmission* recognise its own creation, so the verifier lives in an
`ExclusiveCreates` table on the shared state — one table, so a create
over v3 and a retry over v4 are one server answering once (RFC 1813
§3.3.8, RFC 5661 §18.16.3).

Bounded twice: a 120 s window, since that is how long the original
request can still be in flight, and 256 entries, oldest insertion first.
Both edges fail the same safe way — the retry stops being recognised and
gets EXIST, never a wrong file. A restart still loses it; the class doc
says so rather than implying otherwise.

Same verifier returns the existing object, a different one is EXIST, and
the entry is dropped by REMOVE, by both sides of a RENAME, and by the
SETATTR §3.3.8 treats as the client's commit. v3's arm carries no sattr3
so it applies no attributes; EXCLUSIVE4_1's cva_attrs go through the same
#settableOrRefuse/#applyAttrs/attrset path UNCHECKED4 does.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A driver creates everything as the server process, so each session hands
the result to its caller afterwards. The group half of that was wrong: it
gave every new entry the caller's gid, where a set-group-ID parent means
the entry takes the *parent's* group and a new directory takes the bit as
well.

`src/ownership.ts` is that rule, once, transcribed from
`inode_init_owner()` (fs/inode.c) — the place the kernel makes this
decision for every local filesystem and makes for no userspace one — with
POSIX mkdir(2)/open(2) declining to pick between the two arms and
inode(7) stating the choice. It also clears S_ISGID on a new
group-executable whose creator is in neither the effective nor the
supplementary group AUTH_SYS carried, reading CAP_FSETID as uid 0, which
is all a userspace server can see of a remote caller's capabilities.

No new stat: v3 already read the parent for wcc_data (#preOp is now split
so the stat can be kept), v4 already read it for change_info4 and the
NOTDIR check. The only added driver call is the chmod a new directory
needs, and it runs after the lchown because chown(2) clears set-group-ID
in the other order. Rename is untouched — the rule is about creation.

9P needs nothing: its client computes both halves
(v9fs_get_fsgid_for_create()) and they arrive on the wire; its #claim
comment said it matched NFS character for character and now says that
instead. FUSE is left as a named gap — nothing on that path reads the
parent, and the membership half wants FUSE_CREATE_SUPP_GROUP (7.38),
which init.ts does not request.

Per-call caller credentials in FsDriver were evaluated and declined; the
roadmap entry now carries the reasoning and what would reopen it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The handle-table bound and exclusive create shipped, so their entries go
— replaced by what is actually left of each: a default for `maxHandles`
(which wants v4.1 open state pinned first, since that state is keyed by
entry id) and `suppattr_exclcreat`, whose "the OPEN step's call" note
predates the OPEN step making it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@vercel

vercel Bot commented Jul 31, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
mountx Ready Ready Preview Jul 31, 2026 8:22pm

Request Review

A review of the branch reproduced four defects and two wrong doc claims.

**The exclusive-create verifier was recorded too late.** Both sessions
created the file, awaited more work, and only then stored the verifier —
so a retransmission still in flight, which is the one case the feature
exists for, found EEXIST and was answered EXIST. 17 of 17 concurrent
duplicates failed. Both versions now record it through #createFile's
onCreated hook, between the winning open and the close: the last point
before an await where a duplicate cannot have overtaken.

**v3 CREATE claimed a file it had not created.** #claim ran after
open(O_WRONLY|O_CREAT), which succeeds on a file that already exists, so
an outsider's UNCHECKED create over an existing file re-owned it — and,
new on this branch, chmod'ed it, stripping set-group-ID off a file the
caller did not make. v3 now learns what it created the way v4 already
did, and claims only that. The plain open is still made on the
not-created path, so the refusals this server has always given there
(EISDIR, EACCES) do not quietly become NFS3_OK.

**The set-gid chmod bypassed the server's umask**, re-applying the
client's full requested mode: mkdir 0o775 came out 0o2775 under a set-gid
parent where a plain parent gave 0o755 — the same call, one directory
over, coming out wider. `claimNewEntry` now reads the entry back and
flips that one bit on the mode the driver actually made. The extra lstat
falls only on the set-gid path.

**ExclusiveCreates.set used a bare .slice()** where invariant 13's
documented rule is `copyBytes` — safe only for as long as every caller of
the public handleCall hands over a plain Uint8Array rather than a Buffer.

handles.ts told two half-truths about eviction: that it costs a v4.1
client an OPEN (it costs a *second* FileState, since v4 keys state by
entry id, and a DENY_WRITE stops being enforced across clients), and that
MNT hands out the root from a field (it goes through #attrOf, so a
subdirectory export's mount root is an ordinary evictable entry). Both
corrected, along with the missing floor: a cap below a READDIRPLUS page
ships handles already stale.

test/latency.ts is load-bearing for two of these: the memory driver
answers inside a microtask, so concurrent requests run through it in
lockstep and neither concurrency bug is observable against it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pi0 and others added 3 commits July 31, 2026 20:11
Two doc conflicts, both resolved by keeping main's rewrite and re-adding
this branch's entry beside it:

- `.agents/architecture.md` — main's `src/subtree.ts` paragraph now names
  four tables (`9p/locks.ts` joined it in #12); `src/ownership.ts`'s
  paragraph from this branch sits after it.
- `.agents/roadmap.md` — main's reworded xattr entry (cache-invalidation
  `notify` is not an extension and has its own entry); this branch's
  `suppattr_exclcreat` entry replaces the `CREATE`/`EXCLUSIVE` one main
  still carried, since that shipped here.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The file said "open work only" and then narrated what had shipped in
half its entries — "the extension itself shipped", "half of this already
ships", "the bound shipped and so did the correctness fix under it". A
reader looking for something to pick up had to rule out a paragraph of
history first.

The header now says what the rule actually is: finished work is not
mentioned at all, and an entry that needs a fact about what exists states
it in the present tense, as the constraint the remaining work runs into.
Every entry is rewritten to that, and the per-call-credentials entry
moves to "Decisions still binding" — it is a decision, not work.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`FileState` — opens, share reservations, byte-range locks — is keyed by
the handle-table entry id, so an LRU eviction of an entry a client had
open handed the same file a *second* `FileState` on the next LOOKUP, and
conflict detection stopped seeing the first. Reproduced with two v4.1
clients: A's OPEN4_SHARE_DENY_WRITE was silently bypassed by B after B
walked enough files to push A's entry out.

An entry now carries a pin refcount and is never a victim while it is
held. The pins are driven from the one site that adds to `#files` and the
one site that removes from it (`onFileRetained`/`onFileReleased`), so
they balance by construction rather than by auditing CLOSE, lease
revocation, sweep, DESTROY_CLIENTID and EXCHANGE_ID replacement one at a
time; `destroy()` releases what is left, since the table is the router's
and outlives the session. Two paths had to stop creating state they were
about to discard: a refused LOCK built an empty `FileState`, and LOCKU
created one to forget it.

Pinning is only about eviction — a pinned entry whose last path goes away
is still dropped, because a removed file's handles are meant to go stale.

**The cap becomes soft**, and that is the trade: when every candidate is
pinned the table exceeds `maxHandles` rather than breaking a reservation,
and it does not evict its way back down until the opens close. That is
documented where someone setting the option will read it, and it is why
`maxHandles` still has no default.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@pi0
pi0 merged commit 55a8b60 into main Jul 31, 2026
12 checks passed
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.

2 participants