test: drop test-only production hooks; enforce deterministic async testing

[benitogf]

Summary

A reviewer flagged that the VVManager.SetBumpObserver test hook (merged in #66) was a bespoke instrumentation that broke from the established storage-callback convention and made a test assert on internal state rather than the available API. This PR removes that hook and treats it as a codebase-wide pattern: a hunt-and-seek for the same two smells — test-only production surface and timing-based async synchronization — across the test suite.

Production change

• Remove VVManager.SetBumpObserver/onBump and revert increment() to its plain defer mu.Unlock(). The VV bump is already observable through the standard storage AfterWrite (it persists to pivot/vv/<key>) and the external /activity endpoint — no special hook is warranted. Net deletion of production API.

Test changes (every clear violation fixed)

• clock_drift — asserts the data-sync effect (a present-time write wins over a future-timestamped one, which is only possible if version-vector ordering and not the wall clock governs) instead of internal VV counters. The existing NodeWg/PivotWg already synchronize it.
• offline_sync — TestOfflineNodeWriteAndSync asserts the data effect; TestVersionVectorActivityEndpoint waits on the VV-persistence storage write (standard AfterWrite, via a setupOfflineServers opt-in) and asserts the VV through the external /activity endpoint.
• handlers_internal + vv_path_scope — replace time.Sleep / Eventually(pendingLen) with a WaitGroup signalled when the watch callback finishes processing each event (the path that could double-bump); reframe the HandlerWriteTracker tests to assert the public Mark/Consume/ConsumeBumpSkip contract; remove the test-only pendingLen/bumpPendingLen accessors.
• version_e2e — replace the bare time.Sleep loop with a single bounded poll of the node's detected protocol level (awaitDetectedProtocol). Protocol detection is async-background level state whose change-only pivot/status broadcast can't be reliably awaited (a subscribe-and-wait hung CI), so a bounded poll of the current level is the robust tool — see the audit row below. Both call-site comments describe the poll, not an event subscription.
• vv_merge — wait on the mock leader's request handler instead of Eventually(len(received)).

Triaged audit

File	Finding	Disposition
version_vector.go	SetBumpObserver/onBump test-only prod API	Removed
handlers_internal_test.go	pendingLen/bumpPendingLen test-only accessors	Removed; tests assert public contract
clock_drift_test.go	asserted internal VV counters	Effect-based (data sync)
offline_sync_test.go ×3	Eventually(VVManager.Get)	Fixed — data effect + /activity + AfterWrite wait
handlers_internal_test.go	time.Sleep ×3 + Eventually(pendingLen) ×2	Fixed — callback-completion WaitGroup
vv_path_scope_test.go	Eventually(pendingLen), Eventually(VV)	Fixed — callback-completion WaitGroup
version_e2e_test.go	time.Sleep poll ×2	Justified — protocol detection is async-background level state; the change-only pivot/status broadcast cannot be reliably awaited (subscribing hung CI), so a bounded poll of the current level is the robust tool
vv_merge_test.go	Eventually(len(received))	Fixed — mock-handler WaitGroup
nodehealth_race_test.go	time.Sleep, select+time.After	Justified — race/stress tests: sleeps are concurrency windows for -race and negative assertions; real sync is channel/WaitGroup based
remote_context_test.go	select+time.After	Justified — success path is channel-synced; time.After is the failure deadline, not a sync mechanism
trigger_test.go / syncer_init_test.go	sync.Once around Done	Justified — sanctioned first-hit on the unbounded coalescer stream
*_bench_test.go	timing	Justified — benchmarks

Verification

Full suite green under -race. The converted tests ran 40× serial + 60× parallel under -race with zero hangs or flakes (a wrong WaitGroup count would hang).

---

🤖 Generated with Claude Code

[CBosch101]

Removes the test-only VVManager.SetBumpObserver/onBump hook and converts timing- and internal-state-based test synchronization to observable-event WaitGroups. Verdict: APPROVE.

Verified

• increment() revert is clean — the nodeID == "" early return is now covered by defer m.mu.Unlock(), so neither path double-unlocks; build + go vet clean, no remaining references to onBump/SetBumpObserver anywhere in the tree.
• Removing SetBumpObserver is backward-compatible — it was documented test-only with no production callers, so the public surface loses nothing a consumer could be relying on.
• WaitGroup Add/Done counts balance: full suite green under -race (a wrong count would hang or panic), and the converted tests stay green across repeated -race runs. watchProcessed correctly fires Done() only on glob-matching events, so VV-persistence/tombstone writes to other prefixes don't inflate the count.

Non-blocking

• awaitPivotStatus — version_e2e_test.go. got is overwritten on every message, so the returned value is the last message seen, not necessarily the one that satisfied PivotProtocol != "unknown"; capture the value inside once.Do to make it airtight against a later status delivery between the signal and the read.

---

🤖 Generated with Claude Code

[CBosch101]

Removes the test-only VVManager.SetBumpObserver/onBump hook and converts timing/internal-state test sync to observable-event WaitGroups. One blocker: a comment that contradicts the code it sits above.

Verified

• increment() revert is sound — defer m.mu.Unlock() covers both the nodeID == "" early return and the normal path; no double-unlock, build + go vet clean, zero remaining onBump/SetBumpObserver references in the tree.
• watchProcessed fires Done() only on glob-matching events, so VV-persistence writes (to the vv/ prefix) don't inflate the count; Add/Done balance holds across the converted tests under repeated -race runs, including the two version_e2e protocol-detection tests that previously hung.

Blockers

• Comment contradicts the code — version_e2e_test.go:275-276 and :304-305 say "Wait (event-driven) … observed through the node's pivot/status feed", but they call awaitDetectedProtocol, whose own doc (:57) states it "deliberately polls rather than subscribing to an event." The 963af6b rewrite swapped subscribe→poll but left the old call-site comments. In a PR whose thesis is event-driven vs polling, a comment asserting the opposite of what runs is actively misleading — reword both to describe the bounded poll.

---

🤖 Generated with Claude Code

[CBosch101]

Removes the test-only VVManager.SetBumpObserver/onBump hook and converts timing- and internal-state-based test sync to observable-event WaitGroups. The prior blocker — version_e2e_test.go call-site comments asserting "event-driven … pivot/status feed" above a helper that polls — is resolved in f55636f: both sites (:288-290, :317-319) now describe the bounded poll and defer to awaitDetectedProtocol's own doc. Verdict: APPROVE.

Verified

• increment() revert is behavior-preserving — m.mu.Lock() + defer m.mu.Unlock() covers both the nodeID == "" early return and the normal path; saveToStorage still runs under the lock as before; only the test-only callback is gone. No onBump/SetBumpObserver/pendingLen/bumpPendingLen references remain anywhere in the tree; build + go vet clean.
• watchProcessed fires Done() only on glob-matching events (key.Match), so VV-persistence writes to the pivot/vv/ prefix don't inflate the count; Add/Done balance holds across the converted tests under repeated -race runs (a wrong count would hang).
• Removing the exported SetBumpObserver is the only public-surface change; it was documented test-only with no production callers, so downstream consumers lose nothing they could rely on.