Skip to content

Fix findings from a full source review (core 0.3.1, actioncable 0.2.4, client 0.4.3) - #39

Merged
jpcamara merged 6 commits into
mainfrom
fix/review-findings
Jul 2, 2026
Merged

Fix findings from a full source review (core 0.3.1, actioncable 0.2.4, client 0.4.3)#39
jpcamara merged 6 commits into
mainfrom
fix/review-findings

Conversation

@jpcamara

@jpcamara jpcamara commented Jul 1, 2026

Copy link
Copy Markdown
Owner

Applies the HIGH findings and the important MEDIUMs from a full src review of all components (Rust ext, ActionCable concern, TS client, packaging, demo). Findings that live on the open PRs (#33 live-maps, #38 gap-strike) are intentionally NOT here.

Core — Rust (yrby 0.3.1)

update_ready? is now exact — fixes silent ack-and-drop of real content (empirically reproduced).
It only checked the per-client clock lower bound, but yrs's integration gate also requires origin/right-origin/parent blocks (routinely other clients') and post-Skip blocks in merged updates. A cross-client-origin delta on a server missing that client's content passed ready?; update_advances? then misread the parked result as an already-applied retry (pending doesn't move a state vector) → acked :applied and dropped. Now: trial-integration on a probe seeded with the doc's integrated state (clock check kept as a cheap pre-filter). update_advances? also gained defense in depth: a parked update reports as advancing, never a duplicate. Regression-tested with a new real-Y.js CrossClientOrigin fixture + a merged-Skip-gap Rust test.

read_text process-deadlock. A chained temporary held one read txn while acquiring a second; yrs's write-preferring lock deadlocked reader-vs-writer inside nogvl (uninterruptible — kill -9 territory). Single txn now, plus a threads-vs-writers hammer test.

TOCTOU in gap-free encoding. has_pending + encode ran in separate txns; a concurrent gappy apply between them could serve pending anyway. One txn now.

Perf: update_advances? skips its full O(doc) probe when the update carries blocks beyond the doc's SV — the common novel-update case no longer rebuilds the doc. read_xml: Lexical linebreak/tab emit \n/\t (no more "foo⏎bar""foobar").

ActionCable concern (yrby-actioncable 0.2.4)

  • Retries re-broadcast. Record-then-crash-before-broadcast previously left live subscribers permanently stale: the retry was settled :applied without distributing, and nothing else could reach them. Idempotent apply makes the duplicate free.
  • Missing document key fails closed (Y::Error) instead of silently recording under nil, broadcasting to a dead stream, and still acking (AnyCable fresh-instance + forgotten key).
  • Floor → yrby >= 0.3.1 (the ack-and-drop fix lives in core).

Client (yrby-client 0.4.3)

  • rejected() handler — an auth-rejected subscription surfaces via onError + tears down, instead of hanging at "connecting" forever while silently queueing edits.
  • Awareness content validation — a partially-malformed payload previously mutated awareness entry-by-entry with no event fired; now dry-run-validated (incl. trailing bytes inside the blob) before apply.
  • bfcache presence restore — pagehide stashes local presence, pageshow(persisted) restores it; a restored page no longer rejoins as a ghost.
  • Guarded transport sends — sync throws and promise rejections (@anycable/web) surface via onError; reliable frames stay queued.
  • CJS TypeScript typesdist/cjs/*.d.ts + per-condition types exports; fixes TS1479 for CJS consumers under node16 resolution.

Packaging / demo

  • yrby no longer ships the yrby-decoder gem's files (frozen duplicate could shadow the standalone gem across the load path — was latent in published 0.3.0).
  • Cargo.lock ships in the source gem (build the crate graph CI tested).
  • Demo: the unauthenticated audit-control endpoint (history wipe + delay injection) is no longer mounted in production.

Verification

  • Rust: 27 tests, clippy -D warnings + rustfmt clean
  • Ruby: 95 runs / 235 assertions, rubocop clean — includes the cross-client-origin resync test, fail-closed key test, retry re-broadcast tests, and the read_text deadlock hammer
  • Client: 50 tests, tsc --noEmit clean
  • The ack-and-drop bug was reproduced against 0.3.0 before fixing and its repro is now a permanent fixture/regression

🤖 Generated with Claude Code

jpcamara and others added 2 commits July 1, 2026 18:25
…, client 0.4.3)

Core (Rust):
- update_ready? is now EXACT. It only checked the per-client clock lower
  bound, but yrs's integration gate also requires origin/right-origin/parent
  blocks (routinely other clients') and post-Skip blocks sit above the lower
  bound. A cross-client-origin delta on a server missing that client's content
  passed ready?, then update_advances? misread the parked result as an
  already-applied retry (pending doesn't move a state vector): the channel
  ACKED AND DROPPED real content. Reproduced empirically. ready? now
  trial-integrates on a probe seeded with the doc's integrated state (clock
  check kept as a cheap pre-filter); advances? gained defense in depth (a
  parked update reports as advancing, never a duplicate).
- read_text deadlock: it opened a second read txn while a chained temporary
  still held the first; yrs's write-preferring lock made a concurrent writer
  deadlock the process inside nogvl (uninterruptible). Single txn now.
- TOCTOU in integrated_update: pending check and encode ran in separate
  txns, so a concurrent gappy apply between them could serve pending anyway.
  One txn now.
- update_advances? pre-filter: blocks beyond the doc's SV trivially advance;
  the common (novel-update) case skips the full O(doc) probe.
- read_xml: Lexical linebreak/tab nodes emit \n/\t instead of vanishing.

ActionCable concern (Ruby):
- A lost-ack retry re-broadcasts before acking :applied. Record-then-crash
  (or a failed broadcast) previously left live subscribers permanently stale:
  the retry skipped distribution, and nothing else could reach them.
- A missing document key fails closed (Y::Error) instead of silently
  recording under nil, broadcasting to a dead stream, and still acking
  (the AnyCable fresh-instance + forgotten-key case).
- Floor raised to yrby >= 0.3.1 (the ack-and-drop fix lives in core).

Client (TypeScript):
- rejected() handler: an auth-rejected subscription surfaces via onError and
  tears down instead of hanging at "connecting" forever, queueing edits.
- Awareness frames are content-validated (dry-run entries) before apply:
  a partially-malformed payload previously mutated awareness state entry by
  entry with no event fired. Trailing bytes inside the blob also rejected.
- bfcache restore: presence is stashed on pagehide and restored on
  pageshow(persisted) - a restored page no longer rejoins as a ghost.
- Transport sends are guarded: sync throws and promise rejections
  (@anycable/web) surface via onError instead of unwinding/unhandled.
- CJS TypeScript consumers get real CJS-flavored declarations (dist/cjs
  d.ts + per-condition types exports); fixes TS1479 under node16 resolution.

Packaging:
- yrby no longer ships the yrby-decoder gem's files (the frozen duplicate
  could shadow a newer standalone release across the load path).
- Cargo.lock ships in the source gem: source builds compile the exact crate
  graph CI tested.
- Demo: the unauthenticated audit-control endpoint (history wipe + per-write
  delay injection) is no longer mounted in production.

New real-Y.js fixture (CrossClientOrigin) + regression tests across all
layers, including a read_text-vs-writers thread hammer.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tests

Answers "did you verify every scenario?" honestly — each fix's regression
test was run against the ORIGINAL buggy code where feasible:

- read_text deadlock (H1): DISCRIMINATES. Against the reverted buggy form the
  thread hammer deadlocked so hard the process survived timeout(1)'s SIGTERM
  for 7+ minutes (threads stuck in uninterruptible nogvl) and needed SIGKILL;
  with the fix it passes in milliseconds.
- awareness half-application (client M8): DISCRIMINATES. Against the reverted
  envelope-only validation, exactly the two new tests fail — the failure
  message ("the valid entry was NOT half-applied") empirically proves the old
  partial-mutation behavior.
- TOCTOU (H2): NOT black-box reproducible — 20k racing iterations couldn't
  hit the nanoseconds-wide window even against the buggy two-transaction
  form. The new concurrency test is documented honestly as a contention net
  (catches lock-skipping/fast-path regressions); the fix's guarantee is
  structural (one transaction is atomic under the doc's lock).

New tests this commit:
- protocol.rs: integrated_update concurrency net (writer parking/healing
  gappy updates vs reader encoding; every encode must be pending-free).
- test/packaging_test.rb: gemspec file-list regressions (no decoder or
  actioncable files in the core gem, essentials + Cargo.lock present, no
  tests/artifacts packaged in any gem).
- client: bfcache tests via a window shim — pagehide stashes presence,
  pageshow(persisted) restores it, a non-persisted pageshow does not
  resurrect stale presence. (This path previously had NO coverage.)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@jpcamara

jpcamara commented Jul 2, 2026

Copy link
Copy Markdown
Owner Author

Verification addendum — every claim re-audited

Each fix's regression test was run against the original buggy code where feasible:

Fix Discrimination proof
read_text deadlock Old code: thread hammer deadlocked so hard the process survived SIGTERM for 7+ min (threads in uninterruptible nogvl), needed SIGKILL. Fixed: passes in 4ms.
Ack-and-drop (update_ready?) Reproduced empirically pre-fix; cross-client-origin + merged-Skip fixtures fail on old semantics by construction.
Awareness half-application Old code: exactly the 2 new tests fail — "the valid entry was NOT half-applied" proves the partial-mutation behavior existed.
TOCTOU (integrated_update) Honest negative: 20k racing iterations could not hit the ns-wide window even against the buggy two-txn form. The concurrency test is documented as a contention net; the guarantee is structural (single transaction).

Added in the follow-up commit: bfcache tests (previously zero coverage, via a window shim), gemspec packaging regressions, and the Rust contention net. Totals: Rust 28 / Ruby 98 / client 52, clippy+fmt+rubocop clean.

e2e on this branch: CI ran the full demo battery (durability, 4-browser, collab, Falcon, cross-process, AnyCable) — green. Locally ran the CI-excluded suites: crash_recovery ✅, all 4 audit scenarios ✅, stress 3/3 ✅ (~20k msgs, 0 server errors; one cold-start connection flake on the very first run, not reproducible).

Known remaining caveat: the linebreak/tab extraction test uses a structure modeled on @lexical/yjs storage, not bytes captured from a live Lexical editor.

Closing the last review-verification caveat surfaced the real bug: driving a
live Lexxy editor (agent-browser: type "foo", Shift+Enter, type "barbaz") and
running read_xml on the captured durable state returned "foobarbaz" — the
hand-built regression test had modeled the structure wrong. A Lexical
LineBreakNode is NOT an XmlText child with __type=linebreak; it is an embedded
**Y.Map with { __type: "linebreak" }**, the same shape as the per-text-node
metadata maps the walker deliberately skips — so the newline was being
swallowed by the metadata arm.

Fix: Out::YMap embeds now consult their __type — "linebreak" emits '\n',
"tab" emits '\t', anything else (the "text" metadata maps) stays silent, in
both walk_lexical_block and inline_lexical_text. The XmlText-child handling
stays as a defensive path.

Tests:
- fixtures/lexical_linebreak.bin: ground-truth bytes captured from the live
  editor via the lexxy-realtime test server (GET /content/:room); the new
  test asserts they extract as "foo\nbarbaz".
- The hand-built test now builds the REAL structure (Y.Map embeds, including
  a silent metadata map) instead of the imagined one.

Verified end-to-end: Doc#read_xml on the captured bytes returns "foo\nbarbaz".

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@jpcamara

jpcamara commented Jul 2, 2026

Copy link
Copy Markdown
Owner Author

Caveat closed — and it caught a real bug. Driving a live Lexxy editor (agent-browser: type foo, real Shift+Enter keystroke, type barbaz) and running read_xml on the captured durable state returned "foobarbaz" — the hand-built linebreak test had modeled the structure wrong. A Lexical LineBreakNode is an embedded Y.Map { __type: "linebreak" } (same shape as the per-text-node metadata maps the walker deliberately skips), not an XmlText child.

Fixed the Out::YMap arm to consult __type (linebreak\n, tab\t, metadata stays silent), replaced the hand-built test with the real structure, and added fixtures/lexical_linebreak.bin — ground-truth bytes from the live editor — asserting "foo\nbarbaz". Verified end-to-end via Doc#read_xml on the captured bytes. Rust 29 / Ruby 98 / clippy+fmt clean.

jpcamara and others added 3 commits July 1, 2026 21:57
Each comment now states the invariant and why it matters, without the
incident narrative (that lives in the CHANGELOG and PR).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…halves

The comment simplification cut the clause explaining why the readiness
probe seeds with integrated_update rather than the lossless encode. Restore
it compactly, and pin both reasons with tests:
- a doc carrying legacy pending must still accept unrelated healthy updates
  (a lossless seed would veto everything), and
- a dependency satisfied only by pending content is NOT ready (recording it
  would put a gap in the durable log).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two questions about this comment in a row means it wasn't carrying its
weight: name the ambiguity (retry vs parked-as-pending both leave the state
vector unchanged) and why it matters (false here = ack-and-drop).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@jpcamara
jpcamara merged commit ba11597 into main Jul 2, 2026
7 checks passed
@jpcamara
jpcamara deleted the fix/review-findings branch July 2, 2026 02:48
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