fix(mountsync): refresh state-file mtime on every bootstrap touch(), not just per-page saveState() - #416
Conversation
…not just per-page saveState() An external supervisor that starts/monitors relayfile-mount (e.g. AgentWorkforce/sandbox's shell-level idle watchdog in buildIdleWatchedCommand) can only observe the daemon's liveness through the --state-file's on-disk mtime — it has no visibility into this process's internal bootstrapProgress.touch() calls. Before this change, the state file's mtime only advanced once per FULLY COMPLETED page: persistTraversal() calls saveState() only after every file in that page has been read via readBootstrapFiles. A single page against a large tracked-file set (17k+ files) can spend well over a minute downloading files before the file is ever touched again, even though touch() is already firing correctly per file read and per ListTree page for the *internal* watchdog. Observed in production: a resumed bounded-tree bootstrap reconcile against a 17384-file workspace made real progress (873 entries / 758 files across 4 ListTree calls in ~56s) but was killed by an external "relayfile initial sync made no progress for 60s" watchdog anyway, because no single page had finished within that window and the state file's mtime never moved. Fix: bootstrapProgress.touch() now also does a cheap os.Chtimes() on the configured state file on every touch — no full rewrite, just the mtime — so external mtime-polling supervisors see the same liveness signal the internal watchdog already had. Best-effort: a missing file (fresh bootstrap, before the first saveState()) or an unset path (zero-value bootstrapProgress) silently no-ops, matching prior behaviour exactly in those cases. Adds two regression tests: a direct unit test on touch()'s mtime refresh, and an end-to-end test that reproduces the production false- cancel shape (a slow multi-file page resumed from persisted state) and proves the state file's mtime advances mid-page, before persistTraversal's saveState() runs. Both fail against the pre-fix touch() and pass with it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughBootstrap progress now initializes the private state file before traversal and refreshes its modification time on each progress touch. State persistence uses separate private and public save paths. Tests cover direct, resumed, and first-page bootstrap activity. ChangesBootstrap liveness tracking
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: ⚪ Minimal · up to This localized change refreshes the state-file timestamp during bootstrap progress so external watchdogs can observe ongoing work; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant BootstrapContext
participant Syncer
participant StateFile
participant BootstrapProgress
participant FileReader
BootstrapContext->>StateFile: initialize private state file
BootstrapContext->>BootstrapProgress: provide state-file path
Syncer->>FileReader: read bootstrap files
FileReader->>BootstrapProgress: touch progress
BootstrapProgress->>StateFile: refresh modification time
BootstrapProgress->>Syncer: update watchdog timestamp
Syncer->>StateFile: persist private and public state
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Relayfile Eval ReviewRun: Passed: 4 | Needs human: 0 | Reviewable: 0 | Missing output: 0 | Failed: 0 | Skipped: 0 Human Review CasesNo reviewable human-review cases captured Relayfile output. |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb580185bf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/mountsync/syncer.go">
<violation number="1" location="internal/mountsync/syncer.go:3699">
P3: touch() now issues an unbounded os.Chtimes syscall on every invocation, and touch() fires once per file read and once per ListTree page in the heavy full-pull loops. On a bootstrap over many small files this turns each per-file progress signal into a full inode metadata write on the state file with no throttling, which can add measurable metadata-writeback overhead exactly in the hot per-file loop the PR is trying to keep cheap (the same reason saveState() was deliberately avoided). Throttle the mtime refresh (e.g. at most once per ~1s or per idle-window fraction) so aggregate cost stays bounded while still keeping the file's mtime well inside the external watchdog's timeout.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| // (before the first saveState()) — that's fine, this only needs to | ||
| // help once the file exists, which is exactly the resumed-large- | ||
| // state case where the false-cancel was observed. | ||
| _ = os.Chtimes(p.stateFile, now, now) |
There was a problem hiding this comment.
P3: touch() now issues an unbounded os.Chtimes syscall on every invocation, and touch() fires once per file read and once per ListTree page in the heavy full-pull loops. On a bootstrap over many small files this turns each per-file progress signal into a full inode metadata write on the state file with no throttling, which can add measurable metadata-writeback overhead exactly in the hot per-file loop the PR is trying to keep cheap (the same reason saveState() was deliberately avoided). Throttle the mtime refresh (e.g. at most once per ~1s or per idle-window fraction) so aggregate cost stays bounded while still keeping the file's mtime well inside the external watchdog's timeout.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/mountsync/syncer.go, line 3699:
<comment>touch() now issues an unbounded os.Chtimes syscall on every invocation, and touch() fires once per file read and once per ListTree page in the heavy full-pull loops. On a bootstrap over many small files this turns each per-file progress signal into a full inode metadata write on the state file with no throttling, which can add measurable metadata-writeback overhead exactly in the hot per-file loop the PR is trying to keep cheap (the same reason saveState() was deliberately avoided). Throttle the mtime refresh (e.g. at most once per ~1s or per idle-window fraction) so aggregate cost stays bounded while still keeping the file's mtime well inside the external watchdog's timeout.</comment>
<file context>
@@ -3659,16 +3659,45 @@ func (s *Syncer) HTTPClient() (*HTTPClient, bool) {
+ // (before the first saveState()) — that's fine, this only needs to
+ // help once the file exists, which is exactly the resumed-large-
+ // state case where the false-cancel was observed.
+ _ = os.Chtimes(p.stateFile, now, now)
+ }
}
</file context>
There was a problem hiding this comment.
I evaluated debouncing and am leaving this thread open for Khaliq’s call. I do not recommend adding it in this fix: each Chtimes follows a successful remote ReadFile (plus comparatively few ListTree/page signals), so even at 17k files the local metadata syscall is dominated by the network read and local materialization already paid per file. Debouncing would add concurrent timing/coordination state to bootstrapProgress and weaken the simple guarantee that every real progress signal is externally visible, while the wrapper timeout is outside this package and may vary. If production profiling shows metadata pressure, a timeout-aware rate limit can be added with an explicit contract and benchmark.
1 participant
Summary
Tonight's Daytona fleet-node relayfile-mount repair (via
AgentWorkforce/cloud'sPOST /api/v1/fleet/nodes/sandbox/{sandboxId}/relayfile-mount, cloud#3007) got past auth/binding/get/proof and failed at themountphase withFailed initial relayfile sync: exit 124, root-caused via live Cloudflare Worker log tail to a false-positive external watchdog cancellation, not a real stall:Root cause. Two watchdogs are stacked around the bootstrap reconcile:
bootstrapContext/bootstrapProgress.touch()): correctly sees per-file progress —touch()already fires perListTreepage and per individual file read inreadBootstrapFiles.AgentWorkforce/sandbox'sbuildIdleWatchedCommand, which wraps everyrelayfile-mountinvocation the sandbox orchestrator launches): can only observe the--state-file's on-disk mtime as its liveness signal — it has no visibility into this process's internal state.Before this change, the state file's mtime only advanced once per fully completed page:
persistTraversal()callssaveState()only after every file in that page has been read. A page against a large tracked-file set (this workspace: 17,384 files) can spend well over a minute downloading files before the file is ever touched again — even though real internal progress (each individual file read) is happening the whole time. The external watchdog sees a frozen mtime and kills the process at 60s despite the daemon being alive and working, which then forces the next attempt to detect "non-empty state without completed bootstrap" and restart the same expensive full reconcile — a sticky failure loop.AgentWorkforce/sandbox'sorchestrator.tsalready has a comment acknowledging half of this ("the daemon's atomic full export does not report progress until the body fully returns") and mitigates it by matching the two watchdogs' timeout values — that doesn't fix the granularity mismatch, it just delays the same failure to a bigger/slower page.Fix
bootstrapProgress.touch()now also does a cheapos.Chtimes()on the configured state file on every touch — no full rewrite, just the mtime — so external mtime-polling supervisors see the same liveness signal the internal watchdog already had. Best-effort: a missing file (fresh bootstrap, before the firstsaveState()) or an unset path (zero-valuebootstrapProgress, used in hard-cap mode before this change) silently no-ops, matching prior behavior exactly in those cases.Test plan
TestBootstrapProgressTouchRefreshesStateFileMtime— direct unit test ontouch()'s mtime refresh (backdate a real file, calltouch(), assert mtime advanced; confirm missing-file and unset-path cases don't panic).TestBootstrapProgressTouchesStateFileMidPage— end-to-end reproduction of the production shape: cycle 1 seeds real persisted (non-complete) state via an interrupted first page; cycle 2 resumes with every remaining file in one slow page (serialized reads) and polls the on-disk state file, proving its mtime advances mid-page, beforepersistTraversal'ssaveState()runs for that page.touch()(temporarily no-opped theChtimescall) and pass with the fix — confirmed not tautological.go test ./internal/mountsync/...— full package green.go test ./...— full repo green (all packages).go vet ./...— clean.make build— all three binaries (relayfile-cli,relayfile-server,relayfile-mount) build cleanly.gofmt -l— clean.Draft — not merging tonight; Khaliq reviews and merges.
🤖 Generated with Claude Code