From ae5d41a3d74bea2cd8ccef45a432318571d0b6d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20T=C3=B6rcsv=C3=A1ri?= Date: Fri, 3 Jul 2026 18:08:37 +0200 Subject: [PATCH] =?UTF-8?q?fix(ysync):=20review=20bugs=2001=E2=80=9307=20?= =?UTF-8?q?=E2=80=94=20listener=20registration,=20pad=20nets,=20child=20re?= =?UTF-8?q?movals,=20dirty-set=20emit,=20targeted=20rebaseline,=20seed=20a?= =?UTF-8?q?rbitration,=20stale=20hook=20(doc=2017)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All seven ysync-review bugs fixed and verified; every repro's expected-fail marker removed (they now run as regression tests). Also fixes two bugs found while verifying (doc 17 F5/F6): file-seeded Y bodies are re-upserted in the editor's serialization (doc-16 F4 was an artifact of bug 01), and the 0008-era "asyncify-fragile envelope parse" was really wrapInBoardEnvelope emitting display layer names — canonical LSET::Name() fixes track/via/zone v2 applies; makeFromBlob now logs parse errors instead of swallowing them. Verified: shared 98, standalone collab 37, ysync e2e 20/20 (chromium), collab regression set 21 passed / 3 pre-existing skips (firefox). Bumps: kicad (board_commit child-removal listener notification), web/pcbjam-shared (slot prune + arbitrated seed). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01JgThWXtdvrYLK47EDFoGdq --- docs/features/ysync-review/00-overview.md | 9 + ...bug-first-tab-listener-never-registered.md | 2 +- .../02-bug-footprint-blob-zeroes-pad-nets.md | 2 +- .../03-bug-child-removal-dangling-slot.md | 2 +- .../04-bug-lossy-change-detection.md | 2 +- .../05-bug-rebaseline-swallows-local-edits.md | 2 +- ...6-bug-concurrent-seed-duplicates-layout.md | 2 +- .../07-bug-sheet-switch-stale-down-hook.md | 2 +- .../ysync-review/17-fixes-bugs-01-07.md | 126 ++++++++++ tests/kicad/ysync-repros-eeschema.spec.ts | 4 - tests/kicad/ysync-repros-pcbnew.spec.ts | 24 +- tests/kicad/ysync-two-tab.spec.ts | 39 ++-- wasm/bindings/eeschema_embind.cpp | 102 +++++++- wasm/bindings/pcbnew_embind.cpp | 219 +++++++++++++++--- web/pcbjam-shared | 2 +- .../src/wasm/collab/kicad-binding.ts | 54 ++++- .../src/wasm/collab/sheet-manager.ts | 29 ++- .../src/wasm/collab/ysync-repros.test.ts | 14 +- 18 files changed, 527 insertions(+), 109 deletions(-) create mode 100644 docs/features/ysync-review/17-fixes-bugs-01-07.md diff --git a/docs/features/ysync-review/00-overview.md b/docs/features/ysync-review/00-overview.md index 30c6456..fd4c427 100644 --- a/docs/features/ysync-review/00-overview.md +++ b/docs/features/ysync-review/00-overview.md @@ -31,6 +31,14 @@ constraints. The findings below are the places where it breaks or leaks. ## Verdict / suggested order of attack +**UPDATE 2026-07-03: bugs 01–07 are all FIXED** — see +[17](17-fixes-bugs-01-07.md) for the fix record (done in the batch order below), +two additional findings (F5: doc 16's F4 was an artifact of bug 01, file-seeded +bodies are now editor-normalized; F6: the 0008 "asyncify-fragile envelope parse" +was a layer-name bug, now fixed), and the remaining follow-ups. Every repro +below now runs as a plain regression test. Misses 08–10 and opts 12–14 (beyond +what the dirty-set emit already delivers) remain open. + Every bug below has a runnable expected-fail reproduction test, and the v2 e2e port (miss 11) is DONE — see [16](16-repro-suite-results-and-empirical-findings.md) for the suite map and four empirical findings the running system added (notably: bug 03's @@ -84,3 +92,4 @@ sending half emits NOTHING, not the bare removal the doc predicted). |---|------|-----------| | 15 | [15-plan-repro-tests-and-v2-e2e.md](15-plan-repro-tests-and-v2-e2e.md) | The approved plan: repro tests for bugs 01–07 + the v2 e2e port | | 16 | [16-repro-suite-results-and-empirical-findings.md](16-repro-suite-results-and-empirical-findings.md) | Plan 15 executed (2026-07-03): suite map, verified failure sites, empirical findings F1–F4 | +| 17 | [17-fixes-bugs-01-07.md](17-fixes-bugs-01-07.md) | Bugs 01–07 fixed & verified (2026-07-03); findings F5–F6; remaining follow-ups | diff --git a/docs/features/ysync-review/01-bug-first-tab-listener-never-registered.md b/docs/features/ysync-review/01-bug-first-tab-listener-never-registered.md index 371f21d..0f60eff 100644 --- a/docs/features/ysync-review/01-bug-first-tab-listener-never-registered.md +++ b/docs/features/ysync-review/01-bug-first-tab-listener-never-registered.md @@ -1,7 +1,7 @@ # Bug 01 — First-ever tab never registers the C++ change listener; the seeding session can't send **Severity:** critical (breaks the primary first-session flow for all three tools) -**Status:** open +**Status:** FIXED 2026-07-03 — see [17](17-fixes-bugs-01-07.md) (batch 1) **Fix size:** one line ## Where diff --git a/docs/features/ysync-review/02-bug-footprint-blob-zeroes-pad-nets.md b/docs/features/ysync-review/02-bug-footprint-blob-zeroes-pad-nets.md index 9ca9df9..fc5df98 100644 --- a/docs/features/ysync-review/02-bug-footprint-blob-zeroes-pad-nets.md +++ b/docs/features/ysync-review/02-bug-footprint-blob-zeroes-pad-nets.md @@ -1,7 +1,7 @@ # Bug 02 — Footprint blobs zero pad net codes; net data loss propagates to peers, the Y.Doc, and materialized files **Severity:** high (silent, converging data corruption on boards with nets) -**Status:** open — runtime-CONFIRMED 2026-07-03: the snapshot blob carries the pads +**Status:** FIXED 2026-07-03 — see [17](17-fixes-bugs-01-07.md) (batch 2) with `(net 1 "SIG")` stripped (repro below) ## Where diff --git a/docs/features/ysync-review/03-bug-child-removal-dangling-slot.md b/docs/features/ysync-review/03-bug-child-removal-dangling-slot.md index 59e32ba..905ed14 100644 --- a/docs/features/ysync-review/03-bug-child-removal-dangling-slot.md +++ b/docs/features/ysync-review/03-bug-child-removal-dangling-slot.md @@ -1,7 +1,7 @@ # Bug 03 — Child-only deletions leave a dangling `{item}` slot in the parent's Y body **Severity:** high (poisons a room's render/materialize path; three-way divergence) -**Status:** open +**Status:** FIXED 2026-07-03 — see [17](17-fixes-bugs-01-07.md) (batch 2; three layers + the F1 board_commit notification gap) ## Where diff --git a/docs/features/ysync-review/04-bug-lossy-change-detection.md b/docs/features/ysync-review/04-bug-lossy-change-detection.md index 9363917..65943ec 100644 --- a/docs/features/ysync-review/04-bug-lossy-change-detection.md +++ b/docs/features/ysync-review/04-bug-lossy-change-detection.md @@ -1,7 +1,7 @@ # Bug 04 — Change detection is a lossy scalar projection; a whole class of edits silently never syncs **Severity:** high (broad, silent non-replication of everyday edits) -**Status:** open +**Status:** FIXED 2026-07-03 — see [17](17-fixes-bugs-01-07.md) (batch 3: listener dirty-set → unconditional v2 blob emit) ## Where diff --git a/docs/features/ysync-review/05-bug-rebaseline-swallows-local-edits.md b/docs/features/ysync-review/05-bug-rebaseline-swallows-local-edits.md index afe4258..aba1440 100644 --- a/docs/features/ysync-review/05-bug-rebaseline-swallows-local-edits.md +++ b/docs/features/ysync-review/05-bug-rebaseline-swallows-local-edits.md @@ -1,7 +1,7 @@ # Bug 05 — Global rebaseline after a remote apply can swallow concurrent local edits and receiver-side cleanup **Severity:** medium-high (silent permanent peer divergence; probability scales with edit rate × remote traffic) -**Status:** open +**Status:** FIXED 2026-07-03 — see [17](17-fixes-bugs-01-07.md) (batch 3: targeted rebaseline + post-apply flush) ## Where diff --git a/docs/features/ysync-review/06-bug-concurrent-seed-duplicates-layout.md b/docs/features/ysync-review/06-bug-concurrent-seed-duplicates-layout.md index 5232214..01432d8 100644 --- a/docs/features/ysync-review/06-bug-concurrent-seed-duplicates-layout.md +++ b/docs/features/ysync-review/06-bug-concurrent-seed-duplicates-layout.md @@ -1,7 +1,7 @@ # Bug 06 — Concurrent first-seed duplicates `kdoc_layout`; corrupt materialization with no self-heal **Severity:** medium (race window is small but the damage is durable file corruption) -**Status:** open +**Status:** FIXED 2026-07-03 — see [17](17-fixes-bugs-01-07.md) (batch 4: arbitrated seedDocToY, fix direction 2) ## Where diff --git a/docs/features/ysync-review/07-bug-sheet-switch-stale-down-hook.md b/docs/features/ysync-review/07-bug-sheet-switch-stale-down-hook.md index 6b742fb..ae2d2c4 100644 --- a/docs/features/ysync-review/07-bug-sheet-switch-stale-down-hook.md +++ b/docs/features/ysync-review/07-bug-sheet-switch-stale-down-hook.md @@ -1,7 +1,7 @@ # Bug 07 — Sheet switch leaves the DOWN hook pointing at the old room; cross-room contamination window **Severity:** medium (small window on warm rooms; a full network round-trip — or forever — on cold/failed switches) -**Status:** open +**Status:** FIXED 2026-07-03 — see [17](17-fixes-bugs-01-07.md) (batch 4: destroyed-flag hook + switch retry; fix direction 2 still open) ## Where diff --git a/docs/features/ysync-review/17-fixes-bugs-01-07.md b/docs/features/ysync-review/17-fixes-bugs-01-07.md new file mode 100644 index 0000000..4099297 --- /dev/null +++ b/docs/features/ysync-review/17-fixes-bugs-01-07.md @@ -0,0 +1,126 @@ +# Fixes for bugs 01–07 (2026-07-03) + +**Status:** all seven review bugs FIXED and verified — every repro from plan +[15](15-plan-repro-tests-and-v2-e2e.md) had its expected-fail marker removed and now +runs as a plain regression test. Final state: ysync unit suites green +(pcbjam-shared 98, standalone collab 37), ysync e2e **20/20** on chromium +(two-tab incl. both bug-01 tools, concurrent seed, bare child removal), collab +regression set (items-bridge, roundtrip, legacy collab specs, save-hook) 21 +passed / 3 pre-existing skips on firefox. + +The fixes were done in the review's suggested batches; fixing them also surfaced +and fixed **two bugs the review missed** (F5, F6 below). + +## Batch 1 — bug 01 (one line, as advertised) + +`kicad-binding.ts` seed()'s file-seed branch now calls `bridge.snapshotItems()` +— `ensureBridge()` registers the C++ change listener, and the differ gets a +baseline. See F5 for why the snapshot's *result* is now also used. + +## Batch 2 — bugs 02 + 03 + +- **02:** dropped `blobForItem`'s footprint `pad->SetNetCode(0)` loop + (`pcbnew_embind.cpp`). KiCad 10 formats pad nets by NAME and the parser + resolves names against the receiving board (creating the net if absent), so + the doc's "remap by name on apply" concern is satisfied by the format itself. + The repro's `(net 1 "SIG")` assertion was updated to accept the KiCad 10 + name-only form. +- **03 (root cause, F1):** `BOARD_COMMIT::Push`'s CHT_REMOVE branch for footprint + children never touched any listener vector — `OnItemsCompositeUpdate` was + simply never called for a child-only delete. The kicad fork now pushes the + parent footprint into `itemsChanged` there (a genuine upstream notification + gap; candidate for upstreaming). +- **03 (emit):** `itemToJson` carries a `parent` uuid; `flushDiff`'s removal loop + lifts a removed child whose parent survives to a parent re-blob on the v2 wire + (`wRemoved` now diverges from the legacy `removed`). +- **03 (receive):** `doApplyItems`' removed loop only skips a child uuid when its + parent footprint is itself in the same wire's removed set; a BARE child + removal now removes the child. +- **03 (Y defense):** `applyDeltaToY` deep-prunes `{item: uuid}` slots from a + surviving parent's body before deleting a child item — "file recoverable from + the Y.Doc alone" holds regardless of emitter behavior. + +## Batch 3 — bugs 04 + 05 (one change, both tools) + +- The `COLLAB_LISTENER`s now CAPTURE the touched items (uuid strings at callback + time; children lifted to their root — `GetParentFootprint()` on pcbnew, the + symbol/sheet/label/table parent chain on eeschema) into a `g_dirty` set. +- `flushDiff` emits every dirty root's v2 blob unconditionally (deduped against + the scalar-diff emits). Rotations, field text edits, pad/zone property edits, + endpoint drags — anything serializer-visible — now reach the wire. The + full-model scalar scan REMAINS: it feeds the legacy wire and catches + eeschema's post-`Push` connectivity cleanup, which never reaches listeners. + A no-op commit costs one blob serialization; the TS layer drops unchanged + bodies, so nothing echoes. +- **05:** `doApplyItems` replaced the global `rebaseline()` with + `rebaselineTouched(applied uuids)` (pcbnew also refreshes/erases the child + entries via the baseline's `parent` field) and ends with `scheduleFlush()`. + A local edit committed while the apply was queued keeps its pre-edit baseline + and still emits; receiver-side cleanup broadcasts (idempotent on the sender). +- Global `rebaseline()` clears `g_dirty` (a sheet switch must not leak the old + sheet's dirty marks into the new room). +- Legacy `doApply` keeps the global rebaseline (dead in production, apply-leg + tests only). + +## Batch 4 — bugs 06 + 07 + +- **06:** new shared primitive `seedDocToY(doc, ydoc, origin, nonce)` writes the + seed nonce into `kdoc_meta` inside the seed transaction and returns a + retractor that deletes exactly the layout slots that seed inserted (matched by + insertion id — this client, this clock window — so slots appended by later + edits survive; unit-covered). The binding's file-seed branch arbitrates via a + `kdoc_meta` observer: if a foreign nonce wins the LWW merge, the loser + retracts, leaving the winner's single clean layout. `kdoc_items` converges per + key on its own. (Server-side seeding — fix direction 1 — remains the cleaner + long-term design; this closes the race client-side without backend changes.) +- **07a:** `bindKicadCollab` gates the DOWN hook with a `destroyed` flag — + `destroy()` makes the binding inert both ways. Gap emits during a sheet switch + are dropped; C++ has already rebaselined to the new sheet, so applying them to + the old room was pure contamination. +- **07b:** failed `switchTo` now retries with backoff (2s doubling to 30s, + event-driven, reset on success) instead of leaving the editor unbound forever. +- Fix direction 2 (generation-tagging the apply envelope for the UP-side + sub-frame window) was NOT done — still open, low priority. + +## New findings while verifying (beyond docs 01–16) + +### F5 — F4 was an artifact of bug 01; file-seeded bodies must be editor-normalized + +Doc 16's F4 ("drift-detect is item-silent on the green path") only held because +bug 01 left the differ with NO baseline: the seeder's first edit re-emitted the +FULL model, silently overwriting the file-seeded Y bodies with the editor's +serialization. With bug 01 fixed, the pl_editor baseline drifted on every +file-seeded item (`(name border)` vs `(name "border")`, normalized geometry…). +Fix: the file-seed branch now uses `snapshotItems()`' RESULT — item bodies are +re-upserted in the editor's serialization (the steady-state form every future +emit produces; also what makes `upsertYItem`'s no-op skip effective). Meta + +layout stay file-derived. Consequence: a room's materialization is +editor-normalized, not file-verbatim — the bug-06 e2e was updated accordingly +(convergence + no-duplication, not byte-equality with the raw file render). + +### F6 — the "asyncify-fragile envelope parse" was actually a layer-name bug + +The 0008-era decision to keep track/via/zone/text APPLY off the v2 wire cited an +"asyncify-fragile `(kicad_pcb …)` envelope parse". Root cause found while fixing +the pcbnew bug-01 two-tab repro (whose apply leg it blocked): +`wrapInBoardEnvelope` wrote the envelope's layer table with `GetLayerName()` +(user-visible names), but the parser validates canonical names against the fixed +layer hash — any board with a layer whose display name differs from its +canonical one (every KiCad 10 board: "F.Courtyard" vs "F.CrtYd") threw +`Layer 'B.Courtyard' … is not in fixed layer hash`. Fixed with `LSET::Name()`. +`makeFromBlob` also no longer swallows parse errors — it logs +`[collab] pcbnew blob parse error: `, which is how this was +found. Bare track/via/zone/text changes now apply fine through the envelope +(verified: via moves converge across two tabs); the items-bridge PCB config's +"changed footprint only" scope note is stale and those types can gain coverage. + +## Follow-ups (unchanged from 16, minus what this closed) + +- Misses 08–10 (non-item state sync, collab-aware undo, drift repair) and + optimizations 12–14 (beyond what the dirty set already delivers on the emit + hot path) remain open. +- Retire (or un-skip) the legacy two-tab specs; add `localEdit` + track/via + changed coverage to items-bridge (F3 + F6). +- Bug 07 fix direction 2 (envelope generation tag) if presence/multi-sheet work + widens the UP-side window. +- Upstream the `board_commit.cpp` child-removal notification. diff --git a/tests/kicad/ysync-repros-eeschema.spec.ts b/tests/kicad/ysync-repros-eeschema.spec.ts index e6902a2..2d75f56 100644 --- a/tests/kicad/ysync-repros-eeschema.spec.ts +++ b/tests/kicad/ysync-repros-eeschema.spec.ts @@ -196,8 +196,6 @@ test.describe("eeschema ysync repros (v2 items wire, single tab)", () => { } test("an in-place symbol rotation reaches the wire", async ({ page, testLogger }) => { - test.fail(); // bug 04 — GetPosition() unchanged; no orientation in the json - const ok = await armed(page, "kicadCollabTestRotateItem"); test.skip(!ok, "wasm build predates the ysync repro hooks"); @@ -226,8 +224,6 @@ test.describe("eeschema ysync repros (v2 items wire, single tab)", () => { }); test("a symbol Value field edit reaches the wire", async ({ page, testLogger }) => { - test.fail(); // bug 04 — fields are invisible to the snapshot entirely - const ok = await armed(page, "kicadCollabTestSetFieldText"); test.skip(!ok, "wasm build predates the ysync repro hooks"); diff --git a/tests/kicad/ysync-repros-pcbnew.spec.ts b/tests/kicad/ysync-repros-pcbnew.spec.ts index 3d79f7a..a35594a 100644 --- a/tests/kicad/ysync-repros-pcbnew.spec.ts +++ b/tests/kicad/ysync-repros-pcbnew.spec.ts @@ -192,15 +192,13 @@ test.describe("pcbnew ysync repros (v2 items wire, single tab)", () => { }); test("footprint blob preserves pad nets", async ({ page, testLogger }) => { - test.fail(); // bug 02 — blobForItem's pad->SetNetCode(0) loop strips them - await bootOpen(page); const blob = await fp1Blob(page); - // CORRECT: peers share the same board/net lineage — identity-by-uuid, not - // a foreign-board paste — so `(net 1 "SIG")` must survive the wire - // (02-bug-footprint-blob-zeroes-pad-nets.md). TODAY: pads go out net-0 and - // the loss propagates to the Y.Doc, peers, and materialized files. - expect(blob).toContain(`(net 1 "SIG")`); + // Peers share the same board/net lineage — identity-by-uuid, not a + // foreign-board paste — so the SIG net must survive the wire + // (02-bug-footprint-blob-zeroes-pad-nets.md). KiCad 10 formats pad nets by + // NAME only, `(net "SIG")`; accept the legacy code+name form too. + expect(blob).toMatch(/\(net (1 )?"SIG"\)/); expect(hasAbort(testLogger), "no WASM abort").toBe(false); }); @@ -226,8 +224,6 @@ test.describe("pcbnew ysync repros (v2 items wire, single tab)", () => { }); test("applyItems removes a footprint CHILD by uuid", async ({ page, testLogger }) => { - test.fail(); // bug 03 (receiving half) — the parent-footprint guard skips it - await bootOpen(page); expect(await saveRead(page)).toContain(FP1_TXT); // The wire a peer sends after deleting the fp_text: a bare child removal @@ -278,8 +274,6 @@ test.describe("pcbnew ysync repros (v2 items wire, single tab)", () => { page, testLogger, }) => { - test.fail(); // bug 05 — the post-apply GLOBAL rebaseline swallows it - await bootOpen(page); await page.evaluate(() => window.Module.kicadCollabSnapshotItems()); await captureEmits(page); @@ -349,8 +343,6 @@ test.describe("pcbnew ysync repros (v2 items wire, single tab)", () => { } test("an anchor-centred footprint rotation reaches the wire", async ({ page, testLogger }) => { - test.fail(); // bug 04 — no orientation in the footprint's scalar json - const ok = await armed(page, "kicadCollabTestRotateItem"); test.skip(!ok, "wasm build predates the ysync repro hooks"); @@ -381,8 +373,6 @@ test.describe("pcbnew ysync repros (v2 items wire, single tab)", () => { }); test("a pad size edit reaches the wire", async ({ page, testLogger }) => { - test.fail(); // bug 04 — pads are not visited by forEachTopItem at all - const ok = await armed(page, "kicadCollabTestSetPadSize"); test.skip(!ok, "wasm build predates the ysync repro hooks"); @@ -412,8 +402,6 @@ test.describe("pcbnew ysync repros (v2 items wire, single tab)", () => { }); test("a graphic-shape endpoint drag reaches the wire", async ({ page, testLogger }) => { - test.fail(); // bug 04 — Drawings' json is position-only; GetPosition() is the start - const ok = await armed(page, "kicadCollabTestMoveEndpoint"); test.skip(!ok, "wasm build predates the ysync repro hooks"); @@ -471,8 +459,6 @@ test.describe("pcbnew ysync repros (v2 items wire, single tab)", () => { }); test("a child deletion goes out as the parent's re-blob", async ({ page, testLogger }) => { - test.fail(); // bug 03 (sending half) — flushDiff has no liftBlob for removals - const ok = await armed(page, "kicadCollabTestRemoveItem"); test.skip(!ok, "wasm build predates the ysync repro hooks"); diff --git a/tests/kicad/ysync-two-tab.spec.ts b/tests/kicad/ysync-two-tab.spec.ts index 878ac5a..462ad93 100644 --- a/tests/kicad/ysync-two-tab.spec.ts +++ b/tests/kicad/ysync-two-tab.spec.ts @@ -348,7 +348,6 @@ for (const [cfg, label] of [ context, testLogger, }) => { - test.fail(); // bug 01 — first tab never registers the C++ change listener // TWO kicad_editor instances exceed Firefox's per-content-process wasm // budget (the 2nd tab's #canvas never appears, even serial/isolated — // same SpiderMonkey wall playwright-kicad.config.ts documents for x86 @@ -403,20 +402,18 @@ for (const [cfg, label] of [ // ── Bug 06 — concurrent first-seed duplicates kdoc_layout ──────────────────── // 06-bug-concurrent-seed-duplicates-layout.md: seed-vs-adopt is client-side // check-then-act; two tabs opening the same fresh room inside the settle -// window both file-seed, and the two kdoc_layout inserts BOTH survive the -// Y.Array merge. The deterministic repro is the unit test -// (web/pcbjam-shared/test/ysync-repros.test.ts); this is the real-window -// trigger, skipped on runs where the race happens not to fire. +// window can both file-seed. FIXED by the arbitrated seed (seedDocToY nonce + +// LWW-loser layout retraction — deterministic unit coverage in +// web/pcbjam-shared/test/ysync-repros.test.ts); this drives the real window +// and asserts the room ends on one clean sequence either way. test.describe("v2 items wire — concurrent seed (bug 06 repro)", () => { test.describe.configure({ timeout: 420000 }); - test("both tabs seed a fresh room at once: the room must materialize the single-seed output", async ({ + test("both tabs seed a fresh room at once: the room converges on one clean sequence", async ({ context, testLogger, }) => { - test.fail(); // bug 06 — concurrent first-seed duplicates kdoc_layout - const room = `ysync-v2-pl-race-${test.info().workerIndex}`; const tabA = await context.newPage(); const tabB = await context.newPage(); @@ -441,21 +438,17 @@ test.describe("v2 items wire — concurrent seed (bug 06 repro)", () => { .toBe(true); const merged = (await renderDoc(tabA)).ok!; - const single = (await tabA.evaluate( - (txt) => - (window as unknown as { KicadCollabV2: { singleSeedRender(t: string): string } }) - .KicadCollabV2.singleSeedRender(txt), - PL.fixture, - )) as string; - // If one tab happened to see the other's seed first (race not triggered), - // the run is inconclusive — skip rather than "pass unexpectedly" and turn - // CI red while the bug is still open. The unit repro is the deterministic one. - test.skip(merged === single, "seed race did not trigger this run — inconclusive"); - - // CORRECT: same file, same room → the single-seed materialization. - // TODAY: every root slot + preamble form is doubled, permanently. - expect(merged).toBe(single); + // Whether or not both tabs raced into the seed branch this run, the + // arbitrated seed (seedDocToY + LWW-loser layout retraction) must leave a + // SINGLE clean sequence: each root exactly once, the preamble not doubled. + // (Byte-comparing against singleSeedRender(fixture) is no longer valid: the + // file-seed path deliberately re-upserts item BODIES in the editor's + // serialization, so the room's render is editor-normalized, not file-verbatim.) + expect(merged.match(new RegExp(U_RECT, "g")), "border rect appears once").toHaveLength(1); + expect(merged.match(new RegExp(U_TITLE, "g")), "title text appears once").toHaveLength(1); + expect(merged.match(/\(version 20220228\)/g), "preamble not doubled").toHaveLength(1); + expect(merged.match(/\(setup /g), "setup block not doubled").toHaveLength(1); expect(hasAbort(testLogger), "no WASM abort").toBe(false); await tabA.close(); @@ -476,8 +469,6 @@ test.describe("v2 items wire — pcbnew bare child removal (bug 03 Y-half repro) context, testLogger, }) => { - test.fail(); // bug 03 — dangling {item} slot in the parent's Y body - const room = `ysync-v2-pcb-bug03-${test.info().workerIndex}`; const tabA = await context.newPage(); await bootOpen(tabA, PCB, "tabA"); diff --git a/wasm/bindings/eeschema_embind.cpp b/wasm/bindings/eeschema_embind.cpp index d2741c7..be383cd 100644 --- a/wasm/bindings/eeschema_embind.cpp +++ b/wasm/bindings/eeschema_embind.cpp @@ -387,12 +387,62 @@ std::map snapshotByUuid( SCH_EDIT_FRAME* aFrame ) std::map g_baseline; bool g_flushScheduled = false; +// Roots the listener saw change since the last flush (uuids, captured at callback +// time — removed items may be freed before the flush runs). The scalar snapshot +// diff is a LOSSY projection: rotations/mirrors (anchor unchanged), field text +// edits (the commit stages the SCH_FIELD, which is not a screen item), stroke +// properties etc. never move it (bug 04). Dirty roots emit their v2 blob +// unconditionally; the apply is an idempotent upsert and the TS layer drops +// no-op bodies, so a false positive costs one local serialization. +std::set g_dirty; + +// Lift a commit-staged item to the SCREEN item the differ tracks (a field/pin/cell +// lifts to its symbol/sheet/label/table — same promotion sch_commit's undo uses). +void noteDirty( SCH_ITEM* aItem ) +{ + if( !aItem ) + return; + + while( EDA_ITEM* p = aItem->GetParent() ) + { + if( !p->IsType( { SCH_SYMBOL_T, SCH_TABLE_T, SCH_SHEET_T, SCH_LABEL_LOCATE_ANY_T } ) ) + break; + + aItem = static_cast( p ); + } + + g_dirty.insert( toUtf8( aItem->m_Uuid.AsString() ) ); +} + // Re-seed the diff baseline to the current model — after handing out a seed snapshot, or after // applying a remote delta (so those items aren't re-broadcast as a spurious local diff/echo). +// Declares "current model == broadcast state", so pending dirty marks are stale too — on a +// sheet switch they'd otherwise emit the OLD sheet's items into the new sheet's room. void rebaseline() { if( SCH_EDIT_FRAME* fr = schFrame() ) g_baseline = snapshotByUuid( fr ); + + g_dirty.clear(); +} + +// TARGETED rebaseline (bug 05): refresh baseline entries ONLY for the uuids a remote +// apply touched. A global rebaseline() would fold a concurrently-committed local edit +// (its flush is queued BEHIND the apply on the same pending-event list) into the +// baseline and silently swallow it; targeted, the edit still diffs and emits. The +// connectivity cleanup the apply's Push produced likewise stays diffable — the +// post-apply flush broadcasts it (idempotent on the original sender). +void rebaselineTouched( SCH_EDIT_FRAME* aFrame, const std::vector& aIds ) +{ + for( const std::string& id : aIds ) + { + g_baseline.erase( id ); + + KIID kid( wxString::FromUTF8( id.c_str() ) ); + + if( SCH_ITEM* live = aFrame->Schematic().ResolveItem( kid, nullptr, /*allowNull*/ true ) ) + g_baseline[id] = itemToJson( live ); + } } // Diff the current (settled, post-cleanup) model against the baseline and broadcast the change. @@ -411,10 +461,14 @@ void flushDiff() // v2 items wire (per-item s-expr blobs), built from the same diff. Screen items // are already root-level (fields live inside their symbols), so no lifting. - json wAdded = json::array(), wChanged = json::array(); + json wAdded = json::array(), wChanged = json::array(); + std::set wDone; auto blobFor = [&]( const std::string& id, json& aArr ) { + if( !wDone.insert( id ).second ) + return; + KIID kid( wxString::FromUTF8( id.c_str() ) ); if( SCH_ITEM* item = fr->Schematic().ResolveItem( kid, nullptr, /*allowNull*/ true ) ) @@ -443,13 +497,22 @@ void flushDiff() removed.push_back( id ); } + // Dirty roots (bug 04): whatever the listener saw commit emits its blob on the + // v2 wire even when the scalar projection didn't move (rotation, field text, + // stroke edits). wDone dedups against the scalar-diff emits; deleted ids + // resolve null inside blobFor and skip (the removal loop covered them). + for( const std::string& id : g_dirty ) + blobFor( id, wChanged ); + + g_dirty.clear(); + g_baseline = std::move( cur ); if( !added.empty() || !changed.empty() || !removed.empty() ) - { emit( json{ { "added", added }, { "changed", changed }, { "removed", removed } } ); + + if( !wAdded.empty() || !wChanged.empty() || !removed.empty() ) emitItems( json{ { "added", wAdded }, { "changed", wChanged }, { "removed", removed } } ); - } } // Coalesce all the listener callbacks of one commit (and any other edits in the same loop @@ -548,10 +611,10 @@ public: if( item->Type() == SCH_SHEET_T ) scheduleSheetSave( static_cast( item ) ); } - trigger(); + trigger( aItems ); } - void OnSchItemsChanged( SCHEMATIC&, std::vector& ) override { trigger(); } - void OnSchItemsRemoved( SCHEMATIC&, std::vector& ) override { trigger(); } + void OnSchItemsChanged( SCHEMATIC&, std::vector& v ) override { trigger( v ); } + void OnSchItemsRemoved( SCHEMATIC&, std::vector& v ) override { trigger( v ); } // The editor switched to a different sheet (a different .kicad_sch == a different // collab room). Re-baseline so the first edit on the new sheet diffs against ITS @@ -565,10 +628,17 @@ public: } private: - void trigger() + // Capture the touched roots at callback time (fields/pins lift to their + // symbol — noteDirty), then coalesce into one post-settle flush. + void trigger( const std::vector& aItems ) { - if( !s_applyingRemote ) - scheduleFlush(); + if( s_applyingRemote ) + return; + + for( SCH_ITEM* item : aItems ) + noteDirty( item ); + + scheduleFlush(); } }; @@ -744,11 +814,15 @@ void doApplyItems( SCH_EDIT_FRAME* aFrame, const json& aWire ) SCH_COMMIT commit( aFrame ); bool staged = false; + std::vector touched; // uuids this apply acts on (targeted rebaseline) + for( const json& rid : aWire.value( "removed", json::array() ) ) { SCH_SHEET_PATH path; KIID id( wxString::FromUTF8( rid.get().c_str() ) ); + touched.push_back( rid.get() ); + if( SCH_ITEM* item = sch.ResolveItem( id, &path, /*allowNull*/ true ) ) { commit.Remove( item, path.LastScreen() ); @@ -830,6 +904,7 @@ void doApplyItems( SCH_EDIT_FRAME* aFrame, const json& aWire ) } item->SetParent( &sch ); + touched.push_back( toUtf8( item->m_Uuid.AsString() ) ); commit.Add( item, aFrame->GetScreen() ); staged = true; } @@ -843,10 +918,13 @@ void doApplyItems( SCH_EDIT_FRAME* aFrame, const json& aWire ) if( staged ) commit.Push( wxT( "Collaborative edit (items)" ) ); - // Fold the applied state into the baseline so the post-apply listener flush - // doesn't re-broadcast it as a local diff (echo). - rebaseline(); + // Fold ONLY the applied uuids into the baseline (echo suppression), then flush: + // anything else that now differs — a concurrent local edit, the connectivity + // cleanup this apply's Push produced — broadcasts as a normal local diff + // instead of being swallowed (bug 05). + rebaselineTouched( aFrame, touched ); s_applyingRemote = false; + scheduleFlush(); } // Test/PoC move (the SCH_COMMIT body for kicadCollabTestMoveFirst, deferred via CallAfter). diff --git a/wasm/bindings/pcbnew_embind.cpp b/wasm/bindings/pcbnew_embind.cpp index eb60625..5cfc772 100644 --- a/wasm/bindings/pcbnew_embind.cpp +++ b/wasm/bindings/pcbnew_embind.cpp @@ -185,6 +185,12 @@ json itemToJson( BOARD_ITEM* aItem ) { "layer", itemLayer( aItem ) }, // devirtualized — aItem->GetLayer() mis-dispatches here }; + // Parent footprint uuid (or absent for roots). Carried in the baseline so a + // REMOVED child can still be attributed to its parent after the live item is + // gone — flushDiff lifts such removals to a parent re-blob on the v2 wire. + if( FOOTPRINT* fp = aItem->GetParentFootprint() ) + j["parent"] = toUtf8( fp->m_Uuid.AsString() ); + if( isTrackType( aItem->Type() ) ) { auto* tr = static_cast( aItem ); @@ -290,10 +296,11 @@ std::string blobForItem( BOARD* aBoard, BOARD_ITEM* aItem ) } // The rest of SaveSelection's footprint safety steps, minus the refPoint move - // (the wire carries absolute positions). - for( PAD* pad : copy.Pads() ) - pad->SetNetCode( 0 ); - + // (the wire carries absolute positions) and minus SetNetCode(0): zeroing pad + // nets is a paste-into-FOREIGN-board safety, but collab peers edit the SAME + // board — nets must survive the wire. KiCad 10 formats pad nets by NAME and + // the parser resolves by name against the receiver's board (creating the net + // if missing), so no code remapping is needed on apply. copy.SetLocked( false ); CLIPBOARD_IO io; @@ -333,9 +340,26 @@ BOARD_ITEM* makeFromBlob( BOARD& aBoard, const std::string& aBlob ) CLIPBOARD_IO io; io.SetBoard( &aBoard ); - io.SetReader( [&aBlob]() -> wxString { return wxString::FromUTF8( aBlob.c_str() ); } ); - BOARD_ITEM* parsed = io.Parse(); // FOOTPRINT* (bare) | BOARD* (envelope) | nullptr + // Parse directly (not io.Parse(), whose catch(...) swallows the error): a + // failed apply must say WHY, or wire bugs surface as silent non-convergence. + BOARD_ITEM* parsed = nullptr; // FOOTPRINT* (bare) | BOARD* (envelope) | nullptr + + try + { + parsed = io.PCB_IO_KICAD_SEXPR::Parse( wxString::FromUTF8( aBlob.c_str() ) ); + } + catch( const IO_ERROR& e ) + { + EM_ASM( { console.log( "[collab] pcbnew blob parse error: " + UTF8ToString( $0 ) ); }, + std::string( e.What().utf8_str() ).c_str() ); + return nullptr; + } + catch( ... ) + { + EM_ASM( { console.log( "[collab] pcbnew blob parse error: unknown exception" ); } ); + return nullptr; + } if( !parsed ) return nullptr; @@ -384,8 +408,12 @@ std::string wrapInBoardEnvelope( BOARD& aBoard, const std::string& aItemSexpr ) const char* type = IsCopperLayer( id ) ? LAYER::ShowType( aBoard.GetLayerType( id ) ) : "user"; + // CANONICAL name (LSET::Name), NOT GetLayerName(): the parser validates + // position 2 against the fixed layer hash, and user-visible names differ + // from canonical ones (e.g. "B.Courtyard" vs "B.CrtYd") — the envelope + // parse threw "not in fixed layer hash" for any board with such layers. s += " (" + std::to_string( (int) id ) + " \"" - + std::string( aBoard.GetLayerName( id ).utf8_str() ) + "\" " + type + ")"; + + std::string( LSET::Name( id ).utf8_str() ) + "\" " + type + ")"; } s += ") " + aItemSexpr + ")"; @@ -562,12 +590,85 @@ std::map snapshotByUuid( BOARD& aBoard ) std::map g_baseline; bool g_flushScheduled = false; +// Roots the listener saw change since the last flush (uuids, children lifted to +// their footprint at capture time). The scalar snapshot diff below is a LOSSY +// projection (id/type/x/y/layer + a few extras) — edits that don't move the +// projection (pad/zone property edits, anchor-centred rotations, endpoint drags) +// would otherwise never emit (bug 04). Dirty roots emit their v2 blob +// unconditionally; the wire apply is an idempotent upsert, so a false positive +// (a commit that changed nothing) costs one no-op echo. +std::set g_dirty; + +void noteDirty( BOARD_ITEM* aItem ) +{ + if( !aItem ) + return; + + if( FOOTPRINT* fp = aItem->GetParentFootprint() ) + aItem = fp; + + g_dirty.insert( toUtf8( aItem->m_Uuid.AsString() ) ); +} + // Re-seed the diff baseline to the current model — after handing out a seed snapshot, or after // applying a remote delta (so those items aren't re-broadcast as a spurious local diff/echo). +// Declares "current model == broadcast state", so pending dirty marks are stale too. void rebaseline() { if( PCB_EDIT_FRAME* fr = pcbFrame() ) g_baseline = snapshotByUuid( *fr->GetBoard() ); + + g_dirty.clear(); +} + +// TARGETED rebaseline (bug 05): refresh baseline entries ONLY for the uuids a remote +// apply touched. A global rebaseline() here would fold a concurrently-committed local +// edit (its flush is queued BEHIND the apply on the same pending-event list) into the +// baseline and silently swallow it; with the targeted update the edit's uuids keep +// their pre-edit entries and the queued flush still emits it. Receiver-side cleanup +// the apply's Push produced likewise stays diffable — the post-apply flush broadcasts +// it, and re-application on the original sender is idempotent. +void rebaselineTouched( BOARD* aBoard, const std::vector& aIds ) +{ + for( const std::string& id : aIds ) + { + // Drop the stale entry — and any child entries it owned (their parent + // field carries the root uuid) — then re-snapshot whatever is live now. + g_baseline.erase( id ); + + for( auto it = g_baseline.begin(); it != g_baseline.end(); ) + { + if( it->second.value( "parent", std::string() ) == id ) + it = g_baseline.erase( it ); + else + ++it; + } + + BOARD_ITEM* live = aBoard->ResolveItem( KIID( wxString::FromUTF8( id.c_str() ) ), + /*allowNull*/ true ); + + if( !live ) + continue; + + g_baseline[id] = itemToJson( live ); + + if( live->Type() == PCB_FOOTPRINT_T ) + { + FOOTPRINT* f = static_cast( live ); + + for( PCB_FIELD* fld : f->GetFields() ) + { + if( fld ) + g_baseline[toUtf8( fld->m_Uuid.AsString() )] = itemToJson( fld ); + } + + for( BOARD_ITEM* g : f->GraphicalItems() ) + { + if( g->Type() == PCB_TEXT_T ) + g_baseline[toUtf8( g->m_Uuid.AsString() )] = itemToJson( g ); + } + } + } } // Diff the current (settled, post-cleanup) model against the baseline and broadcast the change. @@ -654,19 +755,44 @@ void flushDiff() } } + // v2 removals diverge from the legacy wire: a removed footprint CHILD whose + // parent survives lifts to the parent's re-blob (wChanged) — the new body + // carries the post-delete child set, and the receiver's parent-replace covers + // the deletion. A bare child removal would strand a dangling {item} slot in + // the Y-side parent body (bug 03). The legacy wire keeps the raw uuid list + // (its receiver skips footprint children anyway). + json wRemoved = json::array(); + for( const auto& [id, j] : g_baseline ) { - if( !cur.count( id ) ) - removed.push_back( id ); + if( cur.count( id ) ) + continue; + + removed.push_back( id ); + + std::string parentId = j.value( "parent", std::string() ); + + if( !parentId.empty() && cur.count( parentId ) ) + liftBlob( parentId, wChanged ); + else + wRemoved.push_back( id ); } + // Dirty roots (bug 04): whatever the listener saw commit emits its blob on + // the v2 wire even when the scalar projection didn't move. wDone dedups + // against the scalar-diff emits above; deleted ids resolve null and skip. + for( const std::string& id : g_dirty ) + liftBlob( id, wChanged ); + + g_dirty.clear(); + g_baseline = std::move( cur ); if( !added.empty() || !changed.empty() || !removed.empty() ) - { emit( json{ { "added", added }, { "changed", changed }, { "removed", removed } } ); - emitItems( json{ { "added", wAdded }, { "changed", wChanged }, { "removed", removed } } ); - } + + if( !wAdded.empty() || !wChanged.empty() || !wRemoved.empty() ) + emitItems( json{ { "added", wAdded }, { "changed", wChanged }, { "removed", wRemoved } } ); } // Coalesce all the listener callbacks of one commit (and any other edits in the same loop @@ -706,21 +832,33 @@ void scheduleFlush() class COLLAB_LISTENER : public BOARD_LISTENER { public: - void OnBoardItemAdded( BOARD&, BOARD_ITEM* ) override { trigger(); } - void OnBoardItemsAdded( BOARD&, std::vector& ) override { trigger(); } - void OnBoardItemRemoved( BOARD&, BOARD_ITEM* ) override { trigger(); } - void OnBoardItemsRemoved( BOARD&, std::vector& ) override { trigger(); } - void OnBoardItemChanged( BOARD&, BOARD_ITEM* ) override { trigger(); } - void OnBoardItemsChanged( BOARD&, std::vector& ) override { trigger(); } - void OnBoardCompositeUpdate( BOARD&, std::vector&, - std::vector&, - std::vector& ) override { trigger(); } + void OnBoardItemAdded( BOARD&, BOARD_ITEM* i ) override { trigger( { i } ); } + void OnBoardItemsAdded( BOARD&, std::vector& v ) override { trigger( v ); } + void OnBoardItemRemoved( BOARD&, BOARD_ITEM* i ) override { trigger( { i } ); } + void OnBoardItemsRemoved( BOARD&, std::vector& v ) override { trigger( v ); } + void OnBoardItemChanged( BOARD&, BOARD_ITEM* i ) override { trigger( { i } ); } + void OnBoardItemsChanged( BOARD&, std::vector& v ) override { trigger( v ); } + void OnBoardCompositeUpdate( BOARD&, std::vector& a, + std::vector& r, + std::vector& c ) override + { + trigger( a ); + trigger( r ); + trigger( c ); + } private: - void trigger() + // Capture the touched roots at callback time (uuid strings — removed items + // may be freed before the flush runs), then coalesce into one flush. + void trigger( const std::vector& aItems ) { - if( !s_applyingRemote ) - scheduleFlush(); + if( s_applyingRemote ) + return; + + for( BOARD_ITEM* item : aItems ) + noteDirty( item ); + + scheduleFlush(); } }; @@ -829,15 +967,30 @@ void doApplyItems( PCB_EDIT_FRAME* aFrame, const json& aWire ) BOARD_COMMIT commit( aFrame ); bool staged = false; + std::vector touched; // root uuids this apply acts on (targeted rebaseline) + + std::set removedIds; + for( const json& rid : aWire.value( "removed", json::array() ) ) + removedIds.insert( rid.get() ); + + for( const std::string& rid : removedIds ) { - KIID id( wxString::FromUTF8( rid.get().c_str() ) ); + KIID id( wxString::FromUTF8( rid.c_str() ) ); + + touched.push_back( rid ); if( BOARD_ITEM* item = board->ResolveItem( id, /*allowNullptr*/ true ) ) { - // A child uuid in `removed` is covered by its parent's replace/remove. - if( item->GetParentFootprint() ) - continue; + if( FOOTPRINT* pfp = item->GetParentFootprint() ) + { + // Covered by the parent's own removal when the whole footprint + // goes. A BARE child removal must remove the child itself + // (bug 03 receiving half) — the sender now lifts these to a + // parent re-blob, but Y-rendered wires can still carry them. + if( removedIds.count( toUtf8( pfp->m_Uuid.AsString() ) ) ) + continue; + } commit.Remove( item ); staged = true; @@ -876,6 +1029,8 @@ void doApplyItems( PCB_EDIT_FRAME* aFrame, const json& aWire ) commit.Remove( existing ); } + touched.push_back( toUtf8( parsed->m_Uuid.AsString() ) ); + commit.Add( parsed ); staged = true; }; @@ -888,10 +1043,12 @@ void doApplyItems( PCB_EDIT_FRAME* aFrame, const json& aWire ) if( staged ) commit.Push( wxT( "Collaborative edit (items)" ) ); - // Fold the applied state into the baseline so the post-apply listener flush - // doesn't re-broadcast it as a local diff (echo). - rebaseline(); + // Fold ONLY the applied uuids into the baseline (echo suppression), then flush: + // anything else that now differs — a concurrent local edit, cleanup this apply's + // Push produced — broadcasts as a normal local diff instead of being swallowed. + rebaselineTouched( board, touched ); s_applyingRemote = false; + scheduleFlush(); } // Test/PoC move (the BOARD_COMMIT body for kicadCollabTestMoveFirst). Run inside a COROUTINE by diff --git a/web/pcbjam-shared b/web/pcbjam-shared index 2eca2c3..f12d9af 160000 --- a/web/pcbjam-shared +++ b/web/pcbjam-shared @@ -1 +1 @@ -Subproject commit 2eca2c31a2dd7a89e8a4a919beb65f465d9e6d0f +Subproject commit f12d9af9595c915bb17a31bb59d156d38b256cbf diff --git a/web/standalone/src/wasm/collab/kicad-binding.ts b/web/standalone/src/wasm/collab/kicad-binding.ts index 85a8336..402029f 100644 --- a/web/standalone/src/wasm/collab/kicad-binding.ts +++ b/web/standalone/src/wasm/collab/kicad-binding.ts @@ -3,13 +3,15 @@ import { applyDeltaToY, deltaFromYEvents, deltaToItemsWire, - docToY, isEmptyItemsWireDelta, isEmptyKicadDelta, itemsWireToDelta, kicadItemsMap, parseItemsWireDelta, renderItem, + seedDocToY, + Y_KDOC_META, + Y_KDOC_SEED_NONCE, ydocHasState, yToItem, type ItemsWireDelta, @@ -77,6 +79,13 @@ export function bindKicadCollab(doc: Y.Doc, bridge: KicadItemsBridge): KicadBind // to trap eeschema's paste path in the real app). seed()'s adopt branch covers // everything those early events contained. let seeded = false; + // Flipped by destroy(): the DOWN hook (window.kicadCollab.onItems) can't be + // unregistered from the C++ side, so a stale emit after destroy — e.g. in the + // sheet-switch gap, when C++ has already rebaselined to the NEW sheet — must + // be dropped here or it writes the new sheet's items into the OLD room (bug 07). + let destroyed = false; + // Concurrent double-seed arbitration cleanup (bug 06); set by the file-seed branch. + let detachSeedArbitration: (() => void) | undefined; /** Plain snapshot of the Y items (the `current`/`view` the conversions need). */ const itemsView = (): Record => { @@ -89,6 +98,7 @@ export function bindKicadCollab(doc: Y.Doc, bridge: KicadItemsBridge): KicadBind // DOWN: local editor change → Y.Doc bridge.onItems((json: string) => { + if (destroyed) return; // stale hook (bug 07) — a destroyed binding is inert let wire: ItemsWireDelta; try { wire = parseItemsWireDelta(json); @@ -149,7 +159,40 @@ export function bindKicadCollab(doc: Y.Doc, bridge: KicadItemsBridge): KicadBind clog( `seed: doc empty → SEEDING from file (${Object.keys(seedDoc.items).length} item(s), root ${seedDoc.root})`, ); - docToY(seedDoc, doc, ORIGIN); + // Arbitrated seed (bug 06): the empty-room check above is check-then-act, + // so a peer may be seeding concurrently. seedDocToY stamps our nonce; if a + // FOREIGN nonce wins the meta LWW merge, our layout inserts are retracted + // (kdoc_items converges per key on its own) leaving the winner's single + // clean sequence. + const nonce = `${doc.clientID}:${Math.random().toString(36).slice(2)}`; + const retract = seedDocToY(seedDoc, doc, ORIGIN, nonce); + const meta = doc.getMap(Y_KDOC_META); + const onMeta = () => { + const winner = meta.get(Y_KDOC_SEED_NONCE); + if (winner !== undefined && winner !== nonce) { + detachSeedArbitration?.(); + detachSeedArbitration = undefined; + retract(); + clog("seed: concurrent double-seed lost LWW — retracted our layout inserts"); + } + }; + meta.observe(onMeta); + detachSeedArbitration = () => meta.unobserve(onMeta); + // snapshotItems() does double duty here. Its side effects register the + // C++ change listener (bug 01 — without it this tab would receive but + // never SEND) and rebaseline the wasm differ. Its RESULT re-upserts the + // item bodies in the EDITOR's serialization: the file's formatting and + // the writer's normalized output can differ textually, and every future + // emit/drift-compare uses the writer's form — keeping file-formatted + // bodies would false-positive drift-detect on every file-seeded room + // and defeat upsertYItem's no-op skip. Meta + layout stay file-derived. + try { + const wire = parseItemsWireDelta(bridge.snapshotItems()); + const local = itemsWireToDelta(wire, itemsView()); + if (!isEmptyKicadDelta(local)) applyDeltaToY(doc, local, ORIGIN); + } catch (err) { + cwarn("seed: post-file-seed baseline failed", err); + } return; } @@ -190,7 +233,12 @@ export function bindKicadCollab(doc: Y.Doc, bridge: KicadItemsBridge): KicadBind return { seed, - destroy: () => items.unobserveDeep(observer), + destroy: () => { + destroyed = true; // gates the DOWN hook — see bug 07 note above + detachSeedArbitration?.(); + detachSeedArbitration = undefined; + items.unobserveDeep(observer); + }, items, }; } diff --git a/web/standalone/src/wasm/collab/sheet-manager.ts b/web/standalone/src/wasm/collab/sheet-manager.ts index 37ecf0c..f99bf81 100644 --- a/web/standalone/src/wasm/collab/sheet-manager.ts +++ b/web/standalone/src/wasm/collab/sheet-manager.ts @@ -101,6 +101,11 @@ export function createSheetCollabManager(opts: SheetManagerOptions): SheetCollab // switches run one-at-a-time so concurrent `onSheetChanged` events can't interleave. let requestedPath: string | null = null; let queue: Promise = Promise.resolve(); + // Failed-switch retry backoff (bug 07): a failed ensureRoom used to leave the + // editor unbound forever — every subsequent edit unsynced until the next manual + // navigation. Event-driven retry, doubling 2s→30s, reset on any success. + let retryDelayMs = 2000; + let retryTimer: ReturnType | undefined; if (opts.initial) { const { sheetPath, session, editorMatchesDoc } = opts.initial; @@ -204,16 +209,34 @@ export function createSheetCollabManager(opts: SheetManagerOptions): SheetCollab function switchTo(sheetPath: string): Promise { requestedPath = sheetPath; + if (retryTimer) { + clearTimeout(retryTimer); + retryTimer = undefined; + } queue = queue .then(() => { // Superseded by a newer navigation — skip this stale switch. The editor's active // screen always reflects `requestedPath`, so we only bind when they agree (the // seed/snapshot then reads the right screen). if (requestedPath !== sheetPath) return; - return doSwitch(sheetPath); + return doSwitch(sheetPath).then(() => { + retryDelayMs = 2000; // bound succeeded — reset the backoff + }); }) .catch((err) => { cwarn(`[sheet] switchTo(${sheetPath}) failed`, err); + // Still the sheet the editor shows and not yet bound → retry with backoff, + // else the editor stays unbound and every edit silently never syncs. + if (requestedPath === sheetPath && activePath !== sheetPath) { + retryTimer = setTimeout(() => { + retryTimer = undefined; + if (requestedPath === sheetPath && activePath !== sheetPath) { + log(`[sheet] retrying switch to ${sheetPath}`); + void switchTo(sheetPath); + } + }, retryDelayMs); + retryDelayMs = Math.min(retryDelayMs * 2, 30000); + } }); return queue; } @@ -246,6 +269,10 @@ export function createSheetCollabManager(opts: SheetManagerOptions): SheetCollab } function destroy(): void { + if (retryTimer) { + clearTimeout(retryTimer); + retryTimer = undefined; + } for (const [path, room] of rooms) { try { room.detachWatch?.(); diff --git a/web/standalone/src/wasm/collab/ysync-repros.test.ts b/web/standalone/src/wasm/collab/ysync-repros.test.ts index 038c041..41f0ea6 100644 --- a/web/standalone/src/wasm/collab/ysync-repros.test.ts +++ b/web/standalone/src/wasm/collab/ysync-repros.test.ts @@ -128,19 +128,19 @@ describe("bug 01 — first-ever tab (file-seed branch) never registers the C++ l return { edA, edB }; } - it.fails("the file-seed branch calls snapshotItems (the listener-registration contract)", () => { + it("the file-seed branch calls snapshotItems (the listener-registration contract)", () => { const { edA } = freshRoomFileSeed(); // The one-line fix's contract: like the editorMatchesDoc branch, the // file-seed branch must call snapshotItems() for its SIDE EFFECTS - // (ensureBridge listener registration + differ baseline). TODAY: 0 calls. + // (ensureBridge listener registration + differ baseline). expect(edA.snapshotCalls).toBeGreaterThan(0); }); - it.fails("a local edit on the seeding tab reaches the joining peer", () => { + it("a local edit on the seeding tab reaches the joining peer", () => { const { edA, edB } = freshRoomFileSeed(); edA.localUpsert(`(segment (start 0 0) (end 1 1) (uuid "seg-new"))`, null, "added"); - // TODAY: A's listener was never registered → the edit is never emitted → - // the peer never receives it (the first-session "seeder can't send" hole). + // Regression cover for bug 01: without the listener registration the edit + // was never emitted (the first-session "seeder can't send" hole). expect(edB.store["seg-new"]).toBeDefined(); }); @@ -159,7 +159,7 @@ describe("bug 01 — first-ever tab (file-seed branch) never registers the C++ l // writes into the (now supposedly detached) doc. describe("bug 07a — destroy() leaves the DOWN hook (onItems) attached", () => { - it.fails("an emit after destroy() must not write into the doc", () => { + it("an emit after destroy() must not write into the doc", () => { const doc = new Y.Doc(); const ed = new CppFaithfulEditor(); const binding = bindKicadCollab(doc, ed); @@ -190,7 +190,7 @@ describe("bug 07a — destroy() leaves the DOWN hook (onItems) attached", () => // to the new sheet, so a local edit emits a new-sheet diff into the OLD room. describe("bug 07b — sheet-switch gap: stale onItems writes into the old sheet's room", () => { - it.fails("an emit during a cold-room switch gap must not land in the old doc", async () => { + it("an emit during a cold-room switch gap must not land in the old doc", async () => { const docs: Y.Doc[] = []; let releaseB!: () => void; const gateB = new Promise((resolve) => (releaseB = resolve));