fix(mount): retry unreadable incremental files#258
Conversation
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
|
Warning Review limit reached
More reviews will be available in 37 minutes and 56 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 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-worker review: no blocking findings. I could not submit a formal approval because GitHub reports this authenticated user cannot approve its own PR, so recording the review here.\n\nIndependent validation from this worktree:\n- Focused regressions passed for changed-path 404 retry, Slack deep-reply 404 retry, and explicit delete behavior.\n- Full |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the IncrementalReadNotReadyError to handle temporary 404 errors during incremental syncs by aborting the cycle and retrying without advancing the events cursor. However, the reviewer identified a critical liveness bug: if a file is created and quickly deleted on the remote, the initial creation event will permanently return a 404, causing an infinite retry loop that stalls the sync daemon. To resolve this, the reviewer suggests implementing a time-based fallback (e.g., 10 minutes) to safely treat persistent 404s as deletions and allow the cursor to advance.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if err != nil { | ||
| var httpErr *HTTPError | ||
| if errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound { | ||
| if err := s.applyRemoteDelete(remotePath, conflicted); err != nil { | ||
| return err | ||
| s.logf("changed event for %s is not readable yet; preserving events cursor for retry", remotePath) | ||
| return &IncrementalReadNotReadyError{ | ||
| Path: remotePath, | ||
| StatusCode: httpErr.StatusCode, | ||
| Code: httpErr.Code, | ||
| Message: httpErr.Message, | ||
| } | ||
| s.markIncrementalCheckpoint(pageStartCursor, pageCursor, "changed", remotePath) | ||
| continue | ||
| } |
There was a problem hiding this comment.
Potential Deadlock / Liveness Bug on File Deletion
If a file is created and then quickly deleted on the remote, the event stream will contain a file.created event followed by a file.deleted event. If these events span across different pages of the event feed, the syncer will process the file.created event first.
When calling ReadFile for this file, the remote will return a 404 Not Found because the file has already been deleted. With this new implementation, the syncer will return an IncrementalReadNotReadyError and abort the sync cycle without advancing the events cursor.
On subsequent sync cycles, the syncer will resume from the same cursor and retry the same file.created event, which will repeatedly fail with a 404 Not Found. Because the cursor is never advanced, the syncer will never reach the file.deleted event in the subsequent page, resulting in a permanent deadlock/infinite retry loop that completely stalls the sync daemon.
Recommendation
Introduce a time-based fallback (e.g., 10 minutes). If the event's timestamp is older than 10 minutes, we can safely assume the 404 is permanent (either due to deletion or a ghost event) and fall back to the old behavior of treating it as a deletion and continuing, rather than stalling the daemon indefinitely.
if err != nil {
var httpErr *HTTPError
if errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound {
var isOld bool
if event.Timestamp != "" {
if t, err := time.Parse(time.RFC3339, event.Timestamp); err == nil {
if time.Since(t) > 10*time.Minute {
isOld = true
}
}
}
if isOld {
s.logf("changed event for %s is still unreadable after 10 minutes; treating as deleted", remotePath)
if err := s.applyRemoteDelete(remotePath, conflicted); err != nil {
return err
}
s.markIncrementalCheckpoint(pageStartCursor, pageCursor, "changed", remotePath)
continue
}
s.logf("changed event for %s is not readable yet; preserving events cursor for retry", remotePath)
return &IncrementalReadNotReadyError{
Path: remotePath,
StatusCode: httpErr.StatusCode,
Code: httpErr.Code,
Message: httpErr.Message,
}
}
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. |
|
Your free trial PR review limit of 300 PRs has been reached. Please upgrade your plan to continue using CodeAnt AI. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound { | ||
| if err := s.applyRemoteDelete(remotePath, conflicted); err != nil { | ||
| return err | ||
| s.logf("changed event for %s is not readable yet; preserving events cursor for retry", remotePath) | ||
| return &IncrementalReadNotReadyError{ | ||
| Path: remotePath, | ||
| StatusCode: httpErr.StatusCode, | ||
| Code: httpErr.Code, | ||
| Message: httpErr.Message, | ||
| } |
There was a problem hiding this comment.
🚩 Permanently-404 files will block incremental cursor advancement indefinitely
The old code at syncer.go:4156-4160 treated a 404 on a changed path as a deletion, allowing the cursor to advance. The new code at syncer.go:4174-4181 returns IncrementalReadNotReadyError, which prevents cursor advancement. If a file is permanently unreachable (e.g., created then deleted across event pages, or a transient resource that never materializes), the incremental path will retry that page every cycle forever. The periodic full pull (forceFullPull at syncer.go:2510-2553, default every 20 cycles) provides a safety net—it successfully syncs the full tree—but it explicitly does NOT advance the EventsCursor (comment at syncer.go:2544: "Intentionally leave s.state.EventsCursor unchanged"). This means 19 out of 20 cycles will fail with IncrementalReadNotReadyError, and only every 20th cycle converges via full pull. This is likely an acceptable tradeoff for the "not readable yet" use case this PR targets, but it's a meaningful behavioral change for permanently-gone files worth documenting.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Reviewed the checked-out PR. Note: I made two small fixes:
Validation:
Addressed comments
Remote PR metadata reports mergeable, and the only fetched commit status was CodeRabbit success. I’m not printing |
|
relayfile-worker review update for commit c9dbbea (fix(sdk): use package version for mount downloads): no blocking findings on the SDK package-version resolution change.\n\nVerified against the PR branch in a clean temp worktree:\n- Built dependency first: |
|
Blocked by workspace mismatch.
I briefly made a local fix while following the checked-out diff, then reverted those edits with Addressed comments
|
|
✅ pr-reviewer applied fixes — committed and pushed Fixed the validated liveness issue in the PR: stale incremental Local validation:
Addressed comments
I’m not printing |
|
Reviewed the checked-out PR artifacts. Note: I fixed one validated issue in the PR change: successful full/bootstrap reconciliation now clears stale Verification:
Addressed comments
I did not print |
|
Implemented the validated liveness fix for PR #258’s incremental 404 behavior. Changed Validation run:
Addressed comments
GitHub reports PR #258 is already closed and merged, so I am not printing |
|
Implemented and verified the validated review finding in the current checkout. Changed Added regression coverage in Local validation passed:
Source checked for review comments: #258 Addressed comments
I am not printing |
1 participant
Summary
Validation
Notes