fix(envd): mount sandbox volumes with nolock so file locks don't hang - #3511
fix(envd): mount sandbox volumes with nolock so file locks don't hang#3511tomassrnka wants to merge 1 commit into
Conversation
Any file lock (flock, fcntl/lockf) on a Volume-backed path inside a sandbox hung forever, in D state on rpc_wait_bit_killable. Blocking and non-blocking variants both wedged; reads, writes, renames and unlink were fine. That silently breaks go build, cargo, SQLite and zsh compinit. envd mounts volumes from the orchestrator's userspace NFSv3 proxy, which serves no NLM: the portmapper advertises only nfs (100003) and mountd (100005), so a GETPORT for nlockmgr (100021) answers 0. The mount string set no nolock and no local_lock, so the kernel default local_lock=none forwarded every lock to NLM, and "hard" made the client retry the bind forever. Adding nolock resolves locks locally (local_lock=all). Locks become process-local to a single sandbox, which matches volume semantics -- there is no cross-sandbox lock coordination today either, it just hangs instead of returning. Also pins the invariant envd relies on: the portmapper does not advertise nlockmgr.
PR SummaryMedium Risk Overview Reviewed by Cursor Bugbot for commit 3403c96. Bugbot is set up for automated code reviews on this repo. Configure here. |
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
|
Validated the Codecov "1 test failed" report (
No code change made in response. |
There was a problem hiding this comment.
No bugs found, but this PR intentionally changes production locking semantics for sandbox volumes (cross-sandbox flock/fcntl coordination is silently dropped instead of hanging), and the description explicitly asks reviewers to sign off on that trade-off — that's a product/semantics decision a human should confirm rather than something to shadow-approve.
What was reviewed: the nolock mount-option change and its rationale, the new envd unit test asserting the mount string, and the new portmapper test confirming nlockmgr (100021) is never advertised. The live-upgrade interaction (already-running sandboxes keep the old, hanging behavior until they get the new envd) was checked and matches what the PR description already states.
Extended reasoning...
Overview
The PR adds a single mount option, nolock, to the NFSv3 mount string envd uses for sandbox volumes (packages/envd/internal/api/init.go), plus two new tests: one asserting the mount-options string contains nolock/no conflicting local_lock, and one asserting the orchestrator's portmapper never advertises nlockmgr (100021). No other logic changes.
Security risks
None identified. This does not touch auth, crypto, or permission checks. The main externally-visible effect is a change in filesystem semantics (locks become process-local to a sandbox instead of being forwarded over NLM, which never worked anyway since the proxy doesn't implement it).
Level of scrutiny
The code diff itself is small and mechanical (one mount option + tests), but it changes real production behavior for every sandbox that mounts a Volume: cross-sandbox file-lock coordination silently stops working instead of hanging. The PR author flags this explicitly as a trade-off that needs reviewer agreement, and there's no runtime toggle since envd has no config surface — this is exactly the kind of product/semantics decision that should get a human's explicit sign-off rather than a bot shadow-approval, even though the code change is simple and well-tested.
Other factors
A reviewer (tomassrnka) already investigated and dismissed the one CI flake (TestWorkdirPermissionDenied) as unrelated to this change, with solid reasoning (unreachable code path, unrelated failure signature, rerun passed). The bug-hunting pass also found nothing, and a candidate concern about the fix not applying to volumes that survive a live envd upgrade was checked and matches the documented behavior in the PR description (old envd instances keep the old behavior until upgraded).
What broke
Every file lock (
flock,fcntl/lockf) on a Volume-backed path inside a sandbox hung forever — blocking and non-blocking variants. Reads/writes/renames/unlink were fine, so it looked healthy untilgo build,cargo, SQLite or zshcompinitsilently wedged. Reported by a Volumes private-beta customer.Root cause
envd mounts volumes from the orchestrator's userspace NFSv3 proxy, which serves no NLM. The portmapper advertises only nfs (100003) and mountd (100005), so
GETPORTfor nlockmgr (100021) answers0. The mount string set neithernolocknorlocal_lock=, so the kernel defaultlocal_lock=noneforwarded every lock to NLM, andhardmade it retry the bind forever.Reproduction
Real Linux NFSv3 client against the real proxy + real portmapper (two containers). Before, on
/proc/mountsshowinglocal_lock=none:Same mount string plus
nolock(local_lock=all): every op exits0in ~2ms,sqlite3OK. Control on local fs:0instantly.nolockmakes locks process-local to a single sandbox. Two sandboxes mounting the same volume lose cross-sandboxflock/fcntlmutual exclusion. Today that case hangs (loud); after this it silently doesn't coordinate. The ticket explicitly accepts this ("acceptable per the existing volumes semantics — no cross-sandbox lock coordination"), but it is a real behaviour change and reviewers should agree to it.What is not lost: locking still works correctly within a sandbox, which is what the broken tooling actually needs. Verified on the fixed mount — with the lock held by one process, a second process got
flock -n-> exit 1 (correctly refused) andflock -xblocked 2.5s until release, then succeeded.This ships with an envd release and there is no runtime toggle (envd has no config surface), so already-running sandboxes keep the old behaviour until they get the new envd.
Optional follow-ups (not done here)
0for 100021 — returning an error would make a recurrence fail fast instead of hanging.PROC_UNAVAILrather thanPROG_UNAVAILfor unknown programs.Linear: https://linear.app/e2b/issue/EN-1109/volume-file-locking-hangs-forever-nfsv3-nlm-never-answered