fix(relayfile,mountsync): harden three-way merge and mount-restart recovery via fuzzing + live chaos testing#376
Conversation
…ty bugs found by fuzzing Adds a Go native fuzz test (FuzzLinesThreeWayMerge) with a trackable-ground-truth harness: base/mine/theirs are generated from independent edit scripts so a non-circular reference implementation can check disjoint-edit merges exactly, and universal invariants (no panic, identity on mine==theirs/base==base, empty content on any conflict) are checked on every iteration regardless of whether a precise oracle applies. Fuzzing found three real bugs in the existing dual-alignment cross-check that ~30+ hand-written tests across two prior review passes had missed: 1. The ambiguity check compared derived merge *output* between the prefer-first/prefer-last alignments, not the alignments themselves. Correlated tie-break bias could make both runs silently agree on the same wrong answer, defeating the cross-check. Fixed by comparing the raw LCS alignment maps directly (lcsMatchesEqual) before computing any merge content, and conflicting on any map disagreement. 2. lcsMatchPreferFirst/lcsMatchPreferLast's backtrack tie-breaks used `>=` instead of strict `>`, so the two "independent" alignments weren't actually exploring opposite tie-break spaces in all cases — undermining the cross-check's premise that the two runs are genuinely independent. 3. mergeWithAlignment's sync-point-region model required a shared unchanged base line between two edits to treat them as separate regions, so genuinely disjoint adjacent edits with no such separator falsely conflicted. Replaced with a diff-hunk model (lineChangesFromAlignment + overlappingChangeComponent connected-components merge) that only treats hunks as conflicting when their base-coordinate ranges actually overlap. Also added trivial-agreement short-circuits (mine==theirs, mine==base, theirs==base) ahead of the new stricter alignment-map check, since identical content can still admit multiple valid self-alignments when it contains repeated lines, and the stricter check alone would otherwise flag these completely safe common cases as ambiguous. Validation: go build/vet/test -race clean across internal/relayfile, including replay of the 13 corpus entries the fuzzer minimized down to during discovery. Additionally ran an independent 5-minute / ~22M-execution fuzz pass (beyond corpus replay) with no new failures. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…emon restart A live multi-process chaos harness (setup/run-chaos/verify scripts under .scratch/phase6-chaos/, exercising 5 concurrent mount daemons + randomized daemon/server kill-restart against real relayauth+relayfile+mount processes, not mocks) found a real data-loss bug: a local edit made to a tracked file while its mount daemon process was killed had no watcher running to observe it and set Dirty, so a subsequent remote pull after restart could silently overwrite and lose it. The first fix attempt treated any `!Dirty && hash-differs-from-tracked` file as writeback-eligible on every pushLocal cycle, for the life of the process. That directly reopens issue #182 / PR #184: a documented incident where two mount daemons racing on the same local directory corrupted tracked state enough that legitimately server-sourced content was misclassified as local edits, producing an unbounded writeback batch that 413'd forever. PR #184's fix was specifically to require an explicit local-mutation signal (Dirty, set only by the filesystem watcher or an equivalent authoritative event) before a file becomes writeback-eligible, rather than inferring intent from hash drift alone. Reconciled by scoping the recovery to exactly one window: a new in-memory, non-persisted Syncer.recoverStartupDrift flag, true only until this process instance's first complete pushLocal pass. A freshly started process could not have observed edits made while no daemon was running, so hash drift found on its first pass is trusted and promoted to Dirty. Every pass after that keeps PR #184's invariant intact: hash drift alone is never writeback-eligible without an explicit local-mutation event. Added TestPushLocalDoesNotPromoteSteadyStateTrackedHashDrift as a direct regression for the #182 scenario (simulates stale/corrupted tracked state, not a real edit, on an already-running syncer and asserts it does not get pushed), and updated the restart-recovery tests to model an actual process restart (a fresh NewSyncer instance) rather than reusing one syncer, since recovery is now tied to process lifetime. Validation: go build/vet/test -race clean across the full repo. Re-ran the chaos harness after the reconciled fix (18min active + 90s settle, seed 260726, 25 mount restarts, 7 server restarts) — verification passed cleanly with zero errors/warnings and zero unexpected exits. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
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: 5 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 (4)
📝 WalkthroughWalkthroughThe change adds one-time startup recovery for unobserved local mount edits and revises line-based three-way merging with stricter LCS ambiguity detection, base-coordinate conflict handling, regression tests, fuzz testing, and corpus seeds. ChangesMount synchronization drift recovery
Line-based three-way merge
Estimated code review effort: 5 (Critical) | ~120 minutes 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. |
![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: 19e244c465
ℹ️ 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".
| if s.recoverStartupDrift && | ||
| exists && | ||
| !tracked.Dirty && | ||
| tracked.Hash != snapshot.Hash { |
There was a problem hiding this comment.
When a daemon stops after partially materializing a workspace, BootstrapComplete remains false while state.Files can already contain tracked files. On restart, syncReserved calls pullRemote at lines 3142-3158 before reaching this startup-drift check in pushLocal, so an offline edit to one of those files is overwritten and its hash then matches the refreshed state; the recovery branch never sees it. Mark startup drift before the bootstrap pull, or make that pull preserve locally drifted tracked files.
Useful? React with 👍 / 👎.
| if s.recoverStartupDrift && | ||
| exists && | ||
| !tracked.Dirty && | ||
| tracked.Hash != snapshot.Hash { |
There was a problem hiding this comment.
If a tracked local file is deleted while the daemon is stopped, it is absent from localRemotePaths, so this startup recovery block never marks it DeletePending. The later tracked-file pass explicitly skips missing files without that flag, and the following remote pull can recreate the file, silently discarding the offline deletion. The startup recovery pass needs to compare tracked paths missing from localFiles as well as hash drift for files that still exist.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![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
🧹 Nitpick comments (2)
internal/relayfile/merge_lines_fuzz_test.go (1)
66-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueIdentity assertions now only exercise the
mine == theirsfast path.Since
linesThreeWayMergereturns early whenmine == theirs, both calls short-circuit before any alignment work, so these invariants no longer cover the merge path they were meant to protect. Still useful as a guard on the fast path, but consider adding an assertion that exercises alignment (e.g.merge(base, mine, theirs)idempotence via re-merging the result) if broader coverage was the intent.🤖 Prompt for 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. In `@internal/relayfile/merge_lines_fuzz_test.go` around lines 66 - 70, Expand the identity assertions in the fuzz test around assertFuzzLineMergeIdentity to include a case with distinct mine and theirs inputs, so linesThreeWayMerge must perform alignment instead of taking its mine == theirs fast path. Verify idempotence by re-merging the produced result with the same inputs, while retaining the existing fast-path assertions.internal/relayfile/merge_lines.go (1)
384-401: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftWhole-file fail-closed on any alignment ambiguity may be broader than needed.
Fail-closed is the right default, but the gate is global: deleting/adding one line inside a repeated run (e.g. one of several identical blank lines) makes
firstMineMatch != lastMineMatch, so the entire file conflicts even when the other side only touched an unrelated region. Consider scoping the ambiguity to base lines where the two maps disagree and only conflicting components that overlap those lines, falling back to whole-file when the disagreement can't be localized. Fine to defer if the current conservatism is intentional for this release.🤖 Prompt for 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. In `@internal/relayfile/merge_lines.go` around lines 384 - 401, The ambiguity check around lcsMatchPreferFirst, lcsMatchPreferLast, and lcsMatchesEqual is currently file-wide; localize ambiguity handling to base-line mappings that actually differ and only mark overlapping conflict components as ambiguous. Preserve whole-file fail-closed behavior when the disagreement cannot be reliably mapped to a specific component, while allowing unrelated regions to merge normally.
🤖 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 `@internal/mountsync/syncer.go`:
- Around line 6254-6266: Prevent the startup-drift recovery block in pushLocal
from setting Dirty=true for entries already marked ReadOnly, while preserving
recovery for writable files. Also update the successful read-only revert branch
that writes remoteBytes back to localPath to explicitly clear
tracked.Dirty=false after synchronizing the hash, revision, and encoding,
ensuring reverted read-only entries do not trigger a later push.
In `@internal/relayfile/merge_lines.go`:
- Around line 205-215: Update the conflict Unit construction in the merge
conflict handling around lineMergeConflict so pure insertions where start equals
end use an insertion-point label rather than an inverted base-lines range, while
preserving the existing start+1 through end label for non-empty line spans.
---
Nitpick comments:
In `@internal/relayfile/merge_lines_fuzz_test.go`:
- Around line 66-70: Expand the identity assertions in the fuzz test around
assertFuzzLineMergeIdentity to include a case with distinct mine and theirs
inputs, so linesThreeWayMerge must perform alignment instead of taking its mine
== theirs fast path. Verify idempotence by re-merging the produced result with
the same inputs, while retaining the existing fast-path assertions.
In `@internal/relayfile/merge_lines.go`:
- Around line 384-401: The ambiguity check around lcsMatchPreferFirst,
lcsMatchPreferLast, and lcsMatchesEqual is currently file-wide; localize
ambiguity handling to base-line mappings that actually differ and only mark
overlapping conflict components as ambiguous. Preserve whole-file fail-closed
behavior when the disagreement cannot be reliably mapped to a specific
component, while allowing unrelated regions to merge normally.
🪄 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: a8622bee-e6c3-4e66-8f9d-a8f3b3f1fe46
📒 Files selected for processing (18)
internal/mountsync/syncer.gointernal/mountsync/syncer_test.gointernal/relayfile/merge_lines.gointernal/relayfile/merge_lines_fuzz_test.gointernal/relayfile/merge_lines_test.gointernal/relayfile/testdata/fuzz/FuzzLinesThreeWayMerge/07d1999fc6a107e0internal/relayfile/testdata/fuzz/FuzzLinesThreeWayMerge/1d0dcfd71bf3522ainternal/relayfile/testdata/fuzz/FuzzLinesThreeWayMerge/5ea012a7fafb2b1cinternal/relayfile/testdata/fuzz/FuzzLinesThreeWayMerge/5f0d4b7cc6d402ceinternal/relayfile/testdata/fuzz/FuzzLinesThreeWayMerge/6832e8f6c1c9f0c3internal/relayfile/testdata/fuzz/FuzzLinesThreeWayMerge/70142e91a478db72internal/relayfile/testdata/fuzz/FuzzLinesThreeWayMerge/71d372458d1c4b7dinternal/relayfile/testdata/fuzz/FuzzLinesThreeWayMerge/745722622e2c7a6cinternal/relayfile/testdata/fuzz/FuzzLinesThreeWayMerge/86ee99ba23b1ee8cinternal/relayfile/testdata/fuzz/FuzzLinesThreeWayMerge/a25fcd004db85783internal/relayfile/testdata/fuzz/FuzzLinesThreeWayMerge/ac171ad6d9939a35internal/relayfile/testdata/fuzz/FuzzLinesThreeWayMerge/df25b9f083a9cdd2internal/relayfile/testdata/fuzz/FuzzLinesThreeWayMerge/e0ef6ff03bf27653
- syncer.go: the startup-drift recovery block (recoverStartupDrift) ran before canWrite/ReadOnly were computed for the current cycle, so it could promote a read-only file's chmod-bypass tamper to Dirty=true. A read-only file's drift is a tamper to revert, not a local edit to recover — move the check after canWrite is known and require it. Also explicitly clear Dirty in the read-only revert branch so a stale flag can't survive to trigger a push if permissions are later relaxed. Added TestFreshProcessDriftRecoverySkipsReadOnlyFiles: a fresh-process sync after an offline chmod-bypass tamper must revert to server content, not push the tampered bytes. - merge_lines.go: a pure-insertion conflict (start == end, no base-line span of its own) produced an inverted range label like "base lines 6-5". Use an insertion-point label instead. Extended TestLinesMergeDifferentInsertionsAtSamePositionConflict to assert the correct label. Validation: go build/vet/test -race clean across the full repo. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
Phase 6 hardening pass on the language-agnostic three-way merge feature (#375): systematic fuzz testing of the merge algorithm, plus a live multi-process chaos test of the mount daemon under concurrent load and randomized crash/restart. Both tracks found and fixed real bugs that ~30+ hand-written tests and prior review passes had missed.
internal/relayfile/merge_lines.go— fuzz-driven fixesAdded a Go native fuzz test (
FuzzLinesThreeWayMerge) with a trackable-ground-truth harness: base/mine/theirs are generated from independent edit scripts so a non-circular reference implementation can check disjoint-edit merges exactly, plus universal invariants (no panic, identity onmine==theirs/base==base, empty content on any conflict) checked every iteration.Found and fixed 3 real bugs:
lcsMatchPreferFirst/lcsMatchPreferLast's backtrack tie-breaks used>=instead of strict>, so the two "independent" alignments weren't actually exploring opposite tie-break spaces in all cases.Validation: full repo
go build/vet/test -raceclean, 13-entry fuzz corpus replays clean, plus an independent 5-minute/~22M-execution fuzz run with no new failures.internal/mountsync/syncer.go— chaos-driven fixA live harness (
.scratch/phase6-chaos/) ran 5 concurrent realrelayfile-mountdaemons against a realrelayauth+relayfileserver, driving continuous concurrent edits while randomly killing/restarting individual daemons and the server itself. It found a genuine data-loss bug: an offline edit made to a file while its mount daemon was killed had no watcher running to observe it, so a subsequent remote pull after restart could silently overwrite and lose it.The first fix attempt was too broad — it treated any hash drift without an explicit
Dirtyflag as writeback-eligible on every sync cycle, which would have reopened a previously-fixed incident (#182/#184: a double-daemon race that corrupted tracked state and caused an infinite 413 writeback loop from misclassifying server-sourced content as local edits). Reconciled by scoping recovery to exactly one window — a new in-memory, non-persistedrecoverStartupDriftflag, true only until a process instance's first completepushLocalpass. A fresh process couldn't have observed edits made while no daemon was running, so drift found on its first pass is trusted; every pass after that keeps the original invariant intact (hash drift alone is never writeback-eligible without an explicit local-mutation event). AddedTestPushLocalDoesNotPromoteSteadyStateTrackedHashDriftas a direct regression for the #182 scenario.Validation: full repo
go build/vet/test -raceclean. Chaos harness re-run after the fix (18min active + 90s settle, 25 mount restarts, 7 server restarts) — verification passed cleanly, zero errors/warnings, zero unexpected exits.Test plan
go build ./.../go vet ./...cleango test ./... -race -count=1clean across all packagesFuzzLinesThreeWayMergecorpus replay (13 entries) passespassed: true)Needs human review before merge.
🤖 Generated with Claude Code