Skip to content

Fix handler VV bump ordering for PR #66 - #68

Closed
benitogf wants to merge 1 commit into
fix/cluster-sync-vv-divergencefrom
codex/fix-pr66-clock-drift
Closed

Fix handler VV bump ordering for PR #66#68
benitogf wants to merge 1 commit into
fix/cluster-sync-vv-divergencefrom
codex/fix-pr66-clock-drift

Conversation

@benitogf

@benitogf benitogf commented Jun 8, 2026

Copy link
Copy Markdown
Owner

Summary

  • fix the PR Fix node→pivot write loss in set/delete/set sequences #66 clock-drift CI flake by moving handler VV ownership into the successful-write AfterWriteOp path for attached storages
  • keep a handler fallback bump when no storage hook is installed, preserving raw handler tests and legacy unattached storage behavior
  • update github.com/benitogf/ooo to latest main (44aea235) and add regression coverage for attached handler AfterWrite observers

Root cause

TestClockDriftScenario waited for the pivot storage AfterWrite callback and then asserted the pivot VV. The handler path previously bumped VV only after db.SetWithMeta returned, while attached storage callbacks can run inside that write. Under scheduling pressure the test could observe the committed pivot write before the handler reached its post-write VV bump, producing the CI failure:

Pivot VV after phase 1: map[]
phase-1 push should have bumped pivot's leader counter

The fix makes attached handler writes bump in AfterWriteOp, before caller AfterWrite observers can see the write. The handler consumes the same fallback marker after the write; if the hook already ran it skips the fallback bump, otherwise it bumps for unattached storages.

Base / review note

This is a stacked PR on top of #66. Please compare against fix/cluster-sync-vv-divergence; the incremental diff is 8 files, +108/-56.

Fixes #67.

Verification

  • go test ./...
  • go test -race ./...
  • go test -run TestClockDriftScenario -count=2000 ./...
  • go test -run TestAttachedHandlerAfterWriteObservesSynchronousVVBump|TestSetVVIncrementsExactlyOnce|TestHandlerIncrementMatchesActivityScope -count=500 ./...

@benitogf

benitogf commented Jun 8, 2026

Copy link
Copy Markdown
Owner Author

Folded into #66 via commit 98fc792 on fix/cluster-sync-vv-divergence.

@benitogf benitogf closed this Jun 8, 2026

@CBosch101 CBosch101 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moves the handler VV bump into the attached-storage AfterWriteOp path so a caller waiting on AfterWrite observes the post-bump VV, keeping a handler fallback bump for unattached storages. Verdict: APPROVE.

Verified

  • go test -race ./... passes locally (the run the PR notes couldn't complete on the Windows host for lack of gcc); TestClockDriftScenario -count=300 and the targeted regression set (-count=20) clean.
  • TestAttachedHandlerAfterWriteObservesSynchronousVVBump is a real guard, not theater — it fails on the pre-fix code (the observer sees an empty VV) and passes after.
  • The ConsumeBumpSkipConsumeBumpFallback rename is complete; no stale references; go vet clean.

Non-blocking

  • Fallback marker consumed before the skip-returnsinstance.go:277. The marker is drained unconditionally, but the hook can still return without bumping at the !found check (instance.go:289) and the pull-driven skip (instance.go:310); a handler-marked write that reaches either return then bumps nowhere, because the handler fallback (handlers.go:174) finds the marker already consumed. Pre-fix this couldn't happen — the handler path returned before the key-match/pull-skip logic. Safe only while handler writes always match a configured key and never coincide with a pull-driven skip; worth confirming that invariant holds. No repro constructed; race suite is green.

🤖 Generated with Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants