fix(relayfile): narrow fork commit conflict check to per-touched-path#371
Conversation
|
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. |
|
Warning Review limit reached
Next review available in: 50 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesPer-path fork revision tracking
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ForkOverlay
participant RelayFileStore
participant ParentWorkspace
Client->>ForkOverlay: write or delete path
ForkOverlay->>RelayFileStore: record BaseRevision
Client->>RelayFileStore: commit fork
RelayFileStore->>ParentWorkspace: read revisions for overlay paths
RelayFileStore->>ParentWorkspace: apply commit when revisions match
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 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. |
CommitForkWithValidator compared the whole workspace revision counter against the forks parent revision, so any write anywhere in the workspace invalidated every other in-flight forks commit, even ones touching fully disjoint paths. Measured before this fix: 38-88% parent_moved on fully disjoint /agent-N/* paths at 2/4/8 concurrent agents (docs/multi-agent-collaboration-assessment.md gap #2). ForkOverlayEntry gains a BaseRevision field, pinned at first-touch in writeForkOverlayLocked and DeleteForkFile (later writes/deletes to the same path within the same fork do not update it). CommitForkWithValidator now iterates the forks overlay and compares each paths BaseRevision against the live ws.Files[path].Revision (a missing live path compares as revision 0) instead of the single workspace counter. fork.ParentRevision is kept, unused for gating, reserved for a future rebase primitive. Verified: TestForkCommitReturnsParentMovedConflict (two forks racing the SAME path, second still correctly fails), TestCommitForkDetectsLiveDirectoryChildRemoval (a live parent delete/rename of a path a fork touched is still detected, both as a straight delete and as delete+write-elsewhere), and TestForkOverlayBaseRevisionPinnedAtFirstTouch (base revision does not drift across repeated writes to the same path within one fork). TestAssessForkContention now asserts (not just logs) parent_moved < 5% for disjoint-path agents: measured 0.0% for 2/4/8 agents in both tight-loop and 20ms-think-time trials, down from the 38-88% baseline. Co-authored-by: Codex <noreply@openai.com>
37baa1f to
0876c36
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/multi-agent-collaboration-assessment.md`:
- Around line 16-24: Update the “Guarantee it actually provides” section in
docs/multi-agent-collaboration-assessment.md to clearly label its shared-mount
conflict-detection analysis as historical pre-PR `#370` behavior, or replace it
with the guarantee provided after the fix. Ensure the surrounding status and gap
discussion no longer implies that the missing detection remains an active
operational limitation.
In `@internal/relayfile/store.go`:
- Around line 1769-1783: Update the fork commit validation around fork.Overlay
and BaseRevision so legacy entries with an unset BaseRevision do not use "0" for
per-file comparison. Preserve the previous workspace-wide ParentRevision
validation for these legacy forks, while retaining the current per-path revision
checks for entries with an explicit BaseRevision.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6483bfd8-b2db-4583-b87e-4395ad434f71
📒 Files selected for processing (5)
docs/multi-agent-collaboration-assessment.mdinternal/httpapi/assessment_fork_contention_test.gointernal/httpapi/server_test.gointernal/relayfile/fork_revision_test.gointernal/relayfile/store.go
Addresses CodeRabbit review on PR #371. A ForkOverlayEntry persisted by a pre-Phase-1 binary has BaseRevision as the Go zero value "" (the field didn't exist yet). The commit check was treating that the same as the new code's explicit "0" (meaning "path didn't exist at first touch"), which is wrong and can produce incorrect results for an in-flight fork that survives a rolling deploy across this change - new code always sets an explicit "0" for a genuinely-new path, so an empty string unambiguously means "we don't know this path's real base revision," not "it didn't exist." Now falls back to the old whole-workspace ParentRevision check for that specific fork when BaseRevision is empty, matching what a pre-Phase-1 binary would have done, while modern entries keep the new per-path check. Also fixes a doc-accuracy issue CodeRabbit caught: the shared-mount "Guarantee it actually provides" section still described gap #1's pre-fix behavior in the present tense after the gap list already marked it fixed. New tests: TestCommitForkLegacyOverlayEntryFallsBackToWorkspaceCheck covers both the succeeds-when-unchanged and conflicts-on-unrelated-write cases for a simulated legacy entry.
2 participants
Summary
Phase 1 of the goal-B fix series (stacked on #370 — this PR is targeted against that branch, not
main, so the diff below is Phase 1 only).CommitForkWithValidatorcompared a single whole-workspace revision counter against the fork'sParentRevision, so any write anywhere in the workspace invalidated every other in-flight fork's commit — even forks touching fully disjoint paths. Measured before this fix: 38-88%parent_movedon fully disjoint/agent-N/*paths at 2/4/8 concurrent agents (docs/multi-agent-collaboration-assessment.mdgap #2).What changed
ForkOverlayEntrygains aBaseRevisionfield, pinned at first-touch (write or delete) within a fork — later writes/deletes to the same path don't move it.CommitForkWithValidatornow compares each overlaid path'sBaseRevisionagainst the live current revision of that specific path, instead of the single workspace-wide counter.fork.ParentRevisionis kept (unused for gating) for a future rebase primitive.Measured result
TestAssessForkContentionnow asserts (not just logs)parent_moved < 5%for disjoint-path agents. 0.0% measured for 2/4/8 agents in both tight-loop and 20ms-think-time variants, down from the 38-88% baseline — confirmed with 3 repeated runs.Live cross-machine proof (not just unit tests): stood up the fixed server on
finn-mac-mini, drove concurrent fork-creating agents from two other genuinely separate physical machines (this laptop +sf-mac-mini) against it over Tailscale — 8 agents total (4 per machine), 649/649 disjoint-path fork commits succeeded, 0parent_moved. Separately confirmed a genuine same-path race between forks created on two different physical machines still resolves correctly (one commits, the other getsparent_movedwith the correctcurrentRevision).Multi-agent workflow note
Implemented by Codex running on
finn-mac-mini(viacodex exec, workspace-write sandbox), from a detailed spec. Codex's sandbox couldn't commit/push (no git write access, no outbound DNS) or run the fullinternal/httpapisuite (couldn't bind loopback ports) — I picked those up directly: reviewed every diff line-by-line, rebased onto #370's branch to resolve doc/test file drift (Codex's sandbox had an older base since it started before #370's second commit landed), reconciled the assessment doc and contention test by hand, and ran full verification (go build,go vet, fullgo test ./..., 3x repeated contention runs, plus the live cross-machine proof above) myself outside that sandbox.Test plan
go build ./...,go vet ./...— clean, run directly onfinn-mac-mini(not just in Codex's sandbox)go test ./...— all green, run directly (this is the suite Codex's sandbox couldn't complete)TestAssessForkContention— 0.0% parent_moved for 2/4/8 agents, both variants, ×3 repeated runsTestForkCommitReturnsParentMovedConflict,TestForkCommitAllowsDisjointParentWrite,TestCommitForkDetectsLiveDirectoryChildRemoval,TestForkOverlayBaseRevisionPinnedAtFirstTouch— all passing🤖 Generated with multi-agent assistance (Codex implementing on a remote machine, Claude reviewing/reconciling/verifying) as part of the goal-B initiative.