fix(fs): wait for mount readability before returning background mounts - #69
Conversation
The --ready-timeout flag on ti fs mount-file-system and ti fs-vault mount-vault was parsed but never consumed on the Drive9 companion path: the commands returned as soon as the companion mount process exited successfully, so first reads during the companion warmup window could surface EAGAIN from the freshly mounted path. After the companion mount succeeds and the mount locator is written, poll the mount path until directory listing succeeds, bounded by --ready-timeout (default 30s). On timeout or cancellation the mount is left running, the locator is preserved, and the error carries the exact unmount command. The fake companions used by unit and e2e tests now create a readable mount path so they model a live background mount.
Live verification against a real backend caught a false timeout: a WebDAV mount's first directory listing crosses the companion proxy and the remote region and can legitimately exceed the previous 2s per-probe bound, so every probe was abandoned as not-ready until the overall --ready-timeout fired even though the mount was healthy. Raise the per-probe bound to 10s; its purpose is only to stop a wedged mount from blocking a probe forever.
qiffang
left a comment
There was a problem hiding this comment.
REVISE on d668e2e0a7e9a85076ed2ff143e701328ff8777f.
The direction has real value. The live runs used the pre-#1012 companion (0f0f301) and show that a post-companion wait can prevent an early return, especially while a cold WebDAV kernel session is still warming up. Wiring the previously unused --ready-timeout, preserving the locator, and providing an unmount hint are useful CLI improvements.
Two correctness blockers remain before merge:
-
--ready-timeoutand cancellation do not bound an in-flight probe.probeMountPointOncealways permits a probe to block for 10 seconds.waitForMountReadychecks its own deadline only after that probe returns, and checksctx.Done()only while sleeping between probes. Therefore--ready-timeout=1sor5scan still take about 10 seconds, and Ctrl-C during a blocked probe can also be delayed by up to 10 seconds. This contradicts the README claim that the wait is bounded by--ready-timeout.Please propagate the context/deadline into each probe and cap every probe by the remaining overall budget (and any earlier context deadline). Add discriminating tests for a blocked probe with a short timeout and for cancellation while the probe is in flight.
-
The readiness predicate and tests do not prove that a mount exists.
probeMountPointReadysucceeds for any ordinary readable directory;TestProbeMountPointReadyexplicitly treats an empty temp directory as ready, and the fake companion only performsMkdirAllplus a marker write. A companion that exits 0 without mounting anything therefore passes the new tests and can still producestatus: mounted.Please require observable active-mount evidence appropriate to each supported driver/platform, and add a regression where “companion succeeded but only a normal directory exists” remains not ready. The test must fail on the old behavior and pass only after a real mount becomes active/readable.
Scope also needs to stay precise: a successful root readdir proves only that the root was readable at that instant. It does not guarantee cold-file stat/open/read or later concurrent scans. Drive9 #1012 owns the known FUSE first-initial_sync directory race; this PR should be described and tested as an independent cross-driver CLI readiness/timeout hardening change, not as a complete fix for all post-mount cold-read EAGAIN cases.
|
@codex review |
…vidence Address review feedback on the mount readiness wait: - Every probe is now capped by min(10s, remaining --ready-timeout) and observes the command context, so a short --ready-timeout no longer waits a full probe bound and Ctrl-C during a blocked probe returns promptly instead of after the probe budget. - Readiness now requires active-mount evidence, not just a readable directory: st_dev comparison against the parent on macOS and /proc/self/mountinfo on Linux. Unsupported platforms fall back to readability only because ti mounts are unsupported there. - New regression: a companion that exits 0 without mounting leaves a plain directory that must not satisfy readiness. A darwin test mounts a real mount_webdav volume and proves the predicate passes only after the kernel mount exists. - Black-box e2e fake-companion runs enable mount evidence via the hidden TI_TEST_FAKE_MOUNT_READY control, gated by TI_ALLOW_TEST_ENDPOINTS like the other TI_TEST_* overrides.
|
@qiffang Thanks for the careful review — both blockers are addressed in bf07eb0. 1.
Discriminating tests added: 2. Readiness now requires real mount evidence. The probe is stat + active-mount evidence + readdir:
Regressions added:
e2e fake companion: the fake cannot create a real kernel mount, so black-box e2e runs pass evidence via the hidden Scope: agreed — I've reframed the PR description as cross-driver CLI readiness/timeout hardening. A successful root readdir proves the mount exists and the root is readable at that instant; cold-file first reads and concurrent scans during companion warmup remain Drive9-side behavior (#1012 for the FUSE first- Verification after the change: unit + |
qiffang
left a comment
There was a problem hiding this comment.
Review round 1 — exact head bf07eb0f. Verdict: APPROVED.
Clean, well-scoped fix: a background mount now waits for actual readability before the command returns, instead of returning a not-yet-listable mount. Verified by code reading + build + local logic tests; the one real-mount test is CI-covered.
The implementation is correct and bounded
probeMountPointReadyproves readiness with Stat → IsDir → mount-table evidence →Readdirnames(1)on the root — a background mount is only "ready" once it's an active mount the kernel can list.probeMountPointOnceruns the probe in a goroutine and abandons it on budget/ctx expiry (a wedged probe can't block forever; the orphaned goroutine closes its handle when the syscall returns).waitForMountReadypoll loop is bounded bydeadline = now + timeout: each probe budget = min(mountReadyProbeTimeout10s, remaining); breaks when remaining ≤ 0 →fs.mount_ready_timeoutapperr; ctx cancel/deadline → distinctfs.mount_ready_canceled; poll interval clamped to remaining. No unbounded wait.- Platform
mountedevidence is correct: linux reads/proc/self/mountinfo(authoritative), matches raw/clean/eval-symlinks/abs candidates, and octal-unescapes mountinfo paths (\040→space); darwin comparesst_devof the path vs its parent (a mount root is on a different device), root-path special-cased.mountready_other.gocovers unsupported platforms. - Wired into the real background-mount return paths —
drive9_companion.go:1196(fs-vault) and:1302(fs), each withopts.ReadyTimeoutand a helpful stop hint; the--ready-timeoutflag (default 30s) is plumbed throughcommands.go.
Verification
go build ./...clean.- All mountready LOGIC tests pass locally:
TestProbeMountPointReady(+subtests),TestDefaultMountPointActiveRejectsPlainDirectory, and the wait/budget/cancellation tests →ok. TestDefaultMountPointActiveAcceptsRealWebDAVMountperforms a realmount_webdav+ probe. Itt.Skips when the env can't mount; in my sandboxmount_webdavhalf-succeeded (mount created but not usable), so the test didn't skip and the probe correctly reported "budget exhausted" — an environment artifact, not a logic defect. The repo'stestCI (proper mount env) is green, covering it.- MERGEABLE;
test+license/clachecks green.
No remaining blocker. LGTM.
qiffang
left a comment
There was a problem hiding this comment.
Correction to my earlier APPROVE (exact head bf07eb0f). Verdict: CHANGES_REQUESTED — test-only; the production code is APPROVE-able.
reviewer3's independent gatekeeper pass caught a factual error in my prior review, which I verified and am correcting here.
What I got wrong
My earlier review claimed the repo's test CI "covers" TestDefaultMountPointActiveAcceptsRealWebDAVMount. That is false: .github/workflows/ci.yml's test job is runs-on: ubuntu-latest (Linux only). The test file has no build tag so it compiles on Ubuntu, but mount_webdav does not exist there, so the test hits t.Skipf(...) and is skipped on CI — it is exercised by no CI job. I incorrectly dismissed the local hang as "a sandbox artifact that CI covers"; it isn't covered.
The real test defect (should-fix before merge)
On darwin (where mount_webdav exists) TestDefaultMountPointActiveAcceptsRealWebDAVMount is environment-fragile: when mount_webdav returns 0 but the mount's first readdir wedges, the test fails after the 10s probe budget and the context-free deferred umount then hangs; reviewer3 observed repeated runs leaking 5 webdavfs_agent processes + 5 WebDAV mounts. So make test on darwin can hang and leak system mounts. Please make it an explicit opt-in darwin integration test (or safe-skip on a mounted-but-unusable env) and give the cleanup a bounded/force umount fallback, so make test neither fails, hangs, nor leaks.
The production code is sound (no code blocker)
I and reviewer3 independently confirmed: bounded readiness probe + poll loop (per-probe budget = min(10s, remaining), ctx-honoring, goroutine-abandon for wedged probes — bounded, not a leak in steady state), Linux /proc/self/mountinfo matching (octal-unescape + candidate set), Darwin st_dev comparison, and both real return paths (drive9_companion.go:1196/:1302) with --ready-timeout plumbed. The non-real-mount logic tests all pass. Once the test above is fixed (opt-in/safe-skip + bounded cleanup), this is an APPROVE on the code.
Problem
ti fs mount-file-systemandti fs-vault mount-vaultreturned as soon as the ti-drive9 companion mount process exited successfully. On a real backend the freshly mounted path is not yet usable at that point, so first reads inside the warmup window can fail withEAGAIN (Resource temporarily unavailable)and succeed only after a few seconds.Reproduced on macOS against a live us-west-2 backend with
--driver fuse(background mount + concurrent first reads on never-cached content):cat <mounted-file>→Resource temporarily unavailable(1 of 60 concurrent reads)ENOENTfrom t=+2.7s onRoot cause
--ready-timeoutwas declared and parsed intoMountFileSystemOptions.ReadyTimeout/VaultMountOptions.ReadyTimeout, but the live Drive9 companion paths (drive9MountFileSystem,drive9MountVault) never read it — a dead flag. AGENTS.md and README both promise that background mounts "wait until the mount is ready"; nothing enforced that after the mount runtime moved to the companion. Drive9's own background mount gate only probes the mount root and does not warm the data plane (its FUSE layer deliberately maps canceled/timed-out requests and HTTP 5xx to EAGAIN — mem9-ai/drive9#1006).Scope
This is an independent, cross-driver CLI readiness/timeout hardening change: when these commands exit successfully, an active kernel mount exists at the requested path and its root is readable, bounded by
--ready-timeout. A successful root readdir proves exactly that. It does not guarantee cold-file firststat/open/reador error-free concurrent scans during companion warmup — those remain Drive9-side semantics (mem9-ai/drive9#1012 tracks the FUSE first-initial_syncdirectory race, #1006 the errno mapping) and remain retryable by design.Change
internal/fs/mountready.go: after the companion mount succeeds and the mount locator is written, poll the mount path until it is an active mount that can be listed. Each probe is stat + active-mount evidence + one readdir entry (EOF accepted), capped bymin(10s, remaining --ready-timeout)and the command context, so a short--ready-timeoutnever waits a full probe bound and Ctrl-C during a blocked probe returns promptly.st_devcomparison against the parent directory on macOS (mountready_darwin.go) and/proc/self/mountinfowith octal-escape decoding on Linux (mountready_linux.go); unsupported platforms fall back to readability only because ti mounts are unsupported there.fs.mount_ready_timeout; on Ctrl-C withfs.mount_ready_canceled. In both cases the background mount is left running, the mount locator is preserved, and the error carries the exact unmount command for that mount path.drive9MountFileSystemanddrive9MountVault.TI_TEST_FAKE_MOUNT_READY=1control, gated byTI_ALLOW_TEST_ENDPOINTS=1like the existingTI_TEST_*overrides.Testing
internal/fs/mountready_test.go):--ready-timeoutis abandoned at the budget (not the 10s probe bound), and cancellation 100ms into a blocked probe returnsfs.mount_ready_canceledpromptly.TestDrive9MountRequiresActiveMountEvidence: companion exits 0 leaving only a plain directory →fs.mount_ready_timeout, locator preserved (fails against the previous implementation, which reportedmounted).TestDefaultMountPointActiveAcceptsRealWebDAVMount(darwin): mounts a realmount_webdavvolume backed by anx/net/webdavserver and proves the predicate is false before the mount and true only after the kernel mount exists and is readdirable.fsandfs-vaultmounts, plus cancellation.make test— all packages pass.make e2e— pass with the updated fake companion and test gate.lsat +0.056s after return succeeded; a 60-way concurrent read storm plusrgwalk plus a read hammer produced zero EAGAIN (pre-fix, the same scenario produced EAGAIN at +0.70s). One transient deep-directoryEIOsurfaced in the concurrentrgwalk during companion warmup; that is Drive9-side behavior outside ti's control (see Scope).lsat +0.056s succeeded; the same storm was fully clean. (An earlier 2s per-probe bound false-timed-out this run; fixed in d668e2e.)fs.mount_ready_timeout, the error carried the exact unmount command, the locator survived, that unmount command succeeded, and a follow-up--ignore-absentunmount reportedabsent.Notes
tino longer reports success before an active, readable mount exists.