Fix node→pivot write loss in set/delete/set sequences - #66
Merged
Conversation
…te loss
A node-originated write could be permanently dropped by the pivot in a
set/delete sequence on the same key: the local VV bump was skipped, so
the node pushed a stale VV the pivot rejected as VVEqual. Two causes,
both in how the pull bump-skip marks were consumed:
- cross-op steal: the post-write bump consumed either the set OR the
delete mark op-unaware, so a concurrent pulled delete's mark could
suppress a local set's bump. Now consumes only the mark matching the
write's own operation, via ooo's AfterWriteOp hook.
- push-vs-bump race: the bump ran after the storage event was
broadcast, so the event-driven peer push could read the pre-bump VV.
ooo's AfterWriteOp fires before the broadcast, closing the window.
Rewrites testClusterSync to wait for observed-state convergence with a
bounded deadline instead of counting non-deterministic async events (the
old exact-count WaitGroups, which hung to the 60s package timeout on any
drift — the original CI failure). Validated ~6900 -race runs with zero
divergence (was ~1/300).
Requires github.com/benitogf/ooo with AfterWriteOp (>= main 8a306d1).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CBosch101
approved these changes
Jun 6, 2026
CBosch101
left a comment
Contributor
There was a problem hiding this comment.
Fixes the node→pivot write loss by making the local VV bump operation-aware (consume only the mark matching this write's own op) and moving it onto ooo's AfterWriteOp so it lands before the broadcast; rewrites the cluster test to converge-or-fail-fast instead of counting non-deterministic async events. The two core race-fix claims hold against the ooo source, and both product consumers build clean against this PR. Approving.
Verified
- ooo
AfterWriteOpfires before the storage broadcast and before the legacyAfterWrite, and emits only"set"/"del"(ooostorage/layered.go:47-58,555,748) — substantiates both the "bump before broadcast" and "callerAfterWritestill sees post-bump VV" claims, and makes theop=="del"-else-setdispatch total. - Op dispatch lines up with the mark setters:
trackSet→bumpSkipSet,trackDelete→bumpSkipDelete(sync.go:552-565); each pulled write later consumes its own matching mark, so the op-specific consume introduces no mark leak. - Breakage check —
idnerdidx/bulkandidnerdidx/bundlebothgo mod tidy+go build ./...clean against this PR's pivot (which drags in the bumped ooo). Only pre-existing environmental failures remain, identical to baseline:libpcsclite/ebfe/scard(both repos) and-lstreamdocklink inbundle/collector. pivot-adjacent packages test clean; bulk's lone failure (pivot/router TestRunUploadJob_PanicSurfacesAsError) fails identically at baseline. - The PR's failing check is a pre-existing flake, not this change: macos failed on
TestClockDriftScenario(not in this diff) under-race -failfast -timeout 60s; the same commit'spushworkflow run passed all three platforms, and it passes locally under-race -count=3. - pivot suite green locally under
-race(cluster sync ×3 + full suite). Did not reproduce the full ~6900-iteration figure — sampled only.
No blockers.
This was referenced Jun 8, 2026
benitogf
force-pushed
the
fix/cluster-sync-vv-divergence
branch
from
June 8, 2026 07:05
10780f8 to
bb6abcd
Compare
TestClockDriftScenario read pivot.VVManager directly right after
PivotWg.Wait(), but the pivot Set handler increments the leader counter
AFTER db.SetWithMeta returns — i.e. after the storage AfterWrite that
drives PivotWg.Done(). Waiting on PivotWg proves the pushed data landed,
not that the counter bumped, so the read raced the bump and intermittently
saw an empty/stale VV ("0 is not greater than 0", surfaced on the macOS CI
runner). The race is pre-existing on master and independent of this
branch's production change; the bump-after-write is correct in production
(it still precedes peer fanout) — only a test reading the VV directly can
observe the gap.
Fix it the way /testing-go-backend-async prescribes — a callback hook on
the component under test, not require.Eventually: VVManager.SetBumpObserver
fires once per counter increment, and the test pairs Add(1)/Wait() with
each push. The observer is disarmed before phase 4, whose pivot write bumps
via a path the test does not assert on. Production never sets the observer.
Verified: 1500 serial -race runs, 720 parallel runs under scheduling
pressure, and 8x full -race suite — zero failures (old rate ~1/300).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
benitogf
force-pushed
the
fix/cluster-sync-vv-divergence
branch
from
June 8, 2026 09:01
e0e03f9 to
93a6531
Compare
testClusterSync waited for cross-cluster sync via requireConverged — a time.Sleep poll loop with a 20s deadline — plus a time.After wrapper around the subscription-establishment barrier. Both violate the async testing rules (no sleeps, no polling, no timing-based synchronisation). That polling was a workaround for non-deterministic event counts that no longer exist. The duplicate push-vs-pull deliveries that once made counts unpredictable are gone now that the version-vector fix on this branch dedups redundant syncs. Measured directly: every subscribed key delivers exactly one websocket message per side per mutation (things/settings/items; set/push/ delete), so exact-count WaitGroups are sound again. - deliv WaitGroup counts post-establishment ws deliveries; each operation arms Add(2) (pivot sub + node sub), triggers, Wait()s, then asserts. - wsReady (count 8) gates on each subscription's initial snapshot; its time.After wrapper is replaced by a plain Wait(). - Policies have no ws subscription (custom HTTP routes), so they wait on a policyWrites WaitGroup driven by an authStorage AfterWrite, wired through FakeServer's new onPolicyWrite parameter. - Removed requireConverged and the now-dead diagThings / tryGetThing / tryGetItem diagnostics. Verified: 100 serial + 240 parallel -race runs of TestClusterSync, plus 6x full -race suite — zero hangs, zero failures. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #65 — a node-originated write is no longer lost when a key is set, deleted from the pivot, then set again on the node.
Depends on the merged
github.com/benitogf/ooooperation-aware post-write hook (benitogf/ooo#139); go.mod is pinned to it.Test plan
🤖 Generated with Claude Code