Skip to content

fix(collab): dedup binary frames by content in replay-cursor test (BUG-1924)#797

Merged
xarmian merged 1 commit into
mainfrom
fix/bug-1924-collab-test-flake
Jul 3, 2026
Merged

fix(collab): dedup binary frames by content in replay-cursor test (BUG-1924)#797
xarmian merged 1 commit into
mainfrom
fix/bug-1924-collab-test-flake

Conversation

@xarmian

@xarmian xarmian commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

TestRoomManagerCursorSuppressedDuringReplay flaked on main (run 28679940189, both Go jobs, "binary frames seen: want 4, got 5"), turning the #793 merge run red with no code cause. Root cause is a designed race, not a product bug: runConn (manager.go:497-514) deliberately starts the writer before replayTo so live broadcasts during a long replay aren't lost — the documented tradeoff is that an op appended during the replay window can be delivered twice (once live, once via replay), tolerated by Yjs idempotency. The test's raw frame count encoded only one outcome of that race.

The test now counts distinct binary payloads (each of the 4 expected ops has a unique payload: seeded 0x01/0x02/0x03 + live 0xFF), so the designed duplicate is tolerated while a genuinely dropped op still caps the count at 3 and fails. A comment block cites the design so nobody reverts to raw counting.

Invariants preserved (independently reviewed)

  • Dropped-op detection: distinct-count still requires all 4 unique ops.
  • Replay-mid cursor-leak detection (the test's reason to exist): cursor id=4 arriving before all 4 distinct ops still fails — and is now stronger, since a duplicate can no longer pad the count toward 4.
  • Deliberately given up: flagging duplicate delivery itself as a failure — that's the documented designed behavior.

Test plan

  • go test ./internal/collab/ -run TestRoomManagerCursorSuppressedDuringReplay -count=200 -race — 200/200 pass (401s)
  • go test ./internal/collab/ -race and full go test ./... — green
  • golangci-lint run ./... — 0 issues
  • Test-only diff; no production or store surface

Review trail

Codex R1 (assertion-semantics lens): converged, no defects; sole informational note is the documented tradeoff above.

Refs: BUG-1924 (docapp). Context: flake surfaced during the #792-#794 bearer/visibility merge train.

https://claude.ai/code/session_01CL1pBjNpPUX6SWkuAuYXHS

…G-1924)

TestRoomManagerCursorSuppressedDuringReplay flaked with "binary frames
seen: want 4, got 5" because runConn deliberately starts the writer
before replayTo, so a live op appended during the replay window can be
legitimately delivered twice (once live, once via replay), tolerated
by Yjs idempotency. Count distinct op payloads instead of raw frames
so the designed duplicate is tolerated while a truly dropped op still
fails the assertion.
@xarmian
xarmian merged commit f29fb65 into main Jul 3, 2026
4 checks passed
@xarmian
xarmian deleted the fix/bug-1924-collab-test-flake branch July 3, 2026 22:56
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.

1 participant