Skip to content

fix(envd): mount sandbox volumes with nolock so file locks don't hang - #3511

Draft
tomassrnka wants to merge 1 commit into
mainfrom
fix/envd-volume-nfs-nolock
Draft

fix(envd): mount sandbox volumes with nolock so file locks don't hang#3511
tomassrnka wants to merge 1 commit into
mainfrom
fix/envd-volume-nfs-nolock

Conversation

@tomassrnka

@tomassrnka tomassrnka commented Aug 1, 2026

Copy link
Copy Markdown
Member

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 until go build, cargo, SQLite or zsh compinit silently 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 GETPORT for nlockmgr (100021) answers 0. The mount string set neither nolock nor local_lock=, so the kernel default local_lock=none forwarded every lock to NLM, and hard made it retry the bind forever.

Reproduction

Real Linux NFSv3 client against the real proxy + real portmapper (two containers). Before, on /proc/mounts showing local_lock=none:

flock -x / flock -n / fcntl lockf  -> exit 124 (timed out)
sqlite3                            -> exit 124 (timed out)
state D, wchan rpc_wait_bit_killable
  nfs_flock -> nfs3_proc_lock -> nlmclnt_proc -> nlmclnt_lock
            -> nlmclnt_call -> rpc_call_sync

Same mount string plus nolock (local_lock=all): every op exits 0 in ~2ms, sqlite3 OK. Control on local fs: 0 instantly.

⚠️ Semantics trade-off — please read

nolock makes locks process-local to a single sandbox. Two sandboxes mounting the same volume lose cross-sandbox flock/fcntl mutual 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) and flock -x blocked 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)

  • The portmapper silently returns 0 for 100021 — returning an error would make a recurrence fail fast instead of hanging.
  • The go-nfs fork returns PROC_UNAVAIL rather than PROG_UNAVAIL for unknown programs.

Linear: https://linear.app/e2b/issue/EN-1109/volume-file-locking-hangs-forever-nfsv3-nlm-never-answered

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.
@cla-bot cla-bot Bot added the cla-signed label Aug 1, 2026
@cursor

cursor Bot commented Aug 1, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Changes volume mount semantics for all new envd rollouts: fixes hangs but removes cross-sandbox lock coordination on shared volumes; requires envd release to take effect.

Overview
Sandbox volume NFS mounts now include nolock so flock and fcntl locks are handled in the guest instead of being sent to NLM. The userspace NFSv3 proxy does not run nlockmgr, so the previous default made lock calls retry forever and wedge workloads on volume paths. Locks stay process-local within one sandbox; two sandboxes on the same volume no longer coordinate locks (they used to hang). Tests lock in nolock on the mount options and that the portmapper does not advertise nlockmgr.

Reviewed by Cursor Bugbot for commit 3403c96. Bugbot is set up for automated code reviews on this repo. Configure here.

@codecov

codecov Bot commented Aug 1, 2026

Copy link
Copy Markdown

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
3740 1 3739 7
View the top 1 failed test(s) by shortest run time
github.com/e2b-dev/infra/tests/integration/internal/tests/envd::TestWorkdirPermissionDenied
Stack Traces | 1.35s run time
=== RUN   TestWorkdirPermissionDenied
=== PAUSE TestWorkdirPermissionDenied
=== CONT  TestWorkdirPermissionDenied
    process_test.go:271: Command [/bin/bash] output: event:{start:{pid:1424}}
    process_test.go:272: 
        	Error Trace:	.../tests/envd/process_test.go:272
        	Error:      	Received unexpected error:
        	            	failed to execute command /bin/bash in sandbox i43l6l0tvtuwt0v2ir5yq: invalid_argument: protocol error: incomplete envelope: unexpected EOF
        	Test:       	TestWorkdirPermissionDenied
        	Messages:   	Should be able to create restricted directory as root
--- FAIL: TestWorkdirPermissionDenied (1.35s)

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@tomassrnka

Copy link
Copy Markdown
Member Author

Validated the Codecov "1 test failed" report (TestWorkdirPermissionDenied) — it is not caused by this change. Evidence:

  1. The shard is green; this was a first-attempt flake that passed on retry. tests/integration/Makefile:127,153 runs go tool gotestsum --rerun-fails=1, so the JUnit XML Codecov ingests records both the failed first attempt and the passing rerun. Every integration-tests / run shard reports pass.

  2. The changed code is unreachable in that test. nfsOptions has exactly one consumer — mountNFS (packages/envd/internal/api/init.go:471) — which is only called from inside for _, volume := range mounts in setupNFS. TestWorkdirPermissionDenied creates a sandbox with no volume mounts and operates on /tmp/restricted-workdir, so no NFS mount is ever performed and the option string is never used.

  3. The failure signature is unrelated. invalid_argument: protocol error: incomplete envelope: unexpected EOF is a truncated Connect RPC exec stream — a transport-level hiccup, not a filesystem or mount-option error. A nolock regression would surface as a lock hang, not a truncated envelope.

No code change made in response.

@tomassrnka
tomassrnka marked this pull request as ready for review August 1, 2026 12:57

@claude claude Bot 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.

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).

@tomassrnka
tomassrnka marked this pull request as draft August 3, 2026 06:55
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.

1 participant