From af07a413ee6de05b6c736d93aee27e455ccc0eef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20T=C3=B6rcsv=C3=A1ri?= Date: Fri, 28 Aug 2026 18:19:29 +0200 Subject: [PATCH] ysync bug 07 UP side: superseded sheet switch never adopts onto the new screen + apply envelope sheet tag/guard (subsheet self-reference fix) Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_013MnySXALJiYsRQ5mCrxgwX --- .../07-bug-sheet-switch-stale-down-hook.md | 52 +++++++- tests/kicad/apply-sheet-guard.spec.ts | 119 ++++++++++++++++++ wasm/bindings/eeschema_embind.cpp | 36 ++++++ web/pcbjam-shared | 2 +- .../src/wasm/collab/kicad-binding.test.ts | 27 ++++ .../src/wasm/collab/kicad-binding.ts | 14 ++- .../src/wasm/collab/sheet-manager.test.ts | 46 +++++++ .../src/wasm/collab/sheet-manager.ts | 17 ++- 8 files changed, 306 insertions(+), 7 deletions(-) create mode 100644 tests/kicad/apply-sheet-guard.spec.ts 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 ae2d2c4..f921fbe 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:** FIXED 2026-07-03 — see [17](17-fixes-bugs-01-07.md) (batch 4: destroyed-flag hook + switch retry; fix direction 2 still open) +**Status:** FIXED 2026-07-03 — see [17](17-fixes-bugs-01-07.md) (batch 4: destroyed-flag hook + switch retry). **UP side CLOSED 2026-08-28** — see "8/28 closure" below. ## Where @@ -83,3 +83,53 @@ Repro (2026-07-03): `web/standalone/src/wasm/collab/ysync-repros.test.ts` — 07 REAL kicad-binding + REAL yjs with only `connectKicadDoc` faked; the cold-switch gap is held open and the stale hook's emit lands in the old sheet's doc). Both `it.fails`. See [16](16-repro-suite-results-and-empirical-findings.md). + +## 8/28 closure — the UP-side window was NOT sub-frame; it corrupted a project + +**Field case (staging, `mega-demo-v2`, Arduino Mega repo-as-project):** reload +showed *"The entire schematic could not be loaded … Could not load sheet +'…/Arduino Mega 2560/ATMEGA2560-16AU.kicad_sch' because it already appears as a +direct ancestor … IO_ERROR: Unable to open for reading"*. The two lines are one +event: KiCad's ancestor check blanks the filename and falls through to +`loadFile("")`. The materialized `ATMEGA2560-16AU.kicad_sch` held the ROOT's +content — all three of the root's `(sheet …)` items with identical uuids (one of +them `Sheetfile "ATMEGA2560-16AU.kicad_sch"` → self-reference) and 49/52 of the +root's symbols; its own items were gone. + +**Mechanism (both halves of this doc, chained):** + +1. `doSwitch(root)` was parked on `await ensureRoom()` / `await activate()` + (cold or passive-gateway room). The user entered the subsheet; C++ moved the + active screen and JS queued `switchTo(sub)`. The `requestedPath` guard only + ran BEFORE `doSwitch` started, so the root switch resumed, bound the root doc + and **adopted it onto the subsheet's screen**: doc-only roots (the root's + items, `(sheet …)` entries included) added, the subsheet's own items removed. +2. `doSwitch(sub)` then bound the subsheet room. Its bind wrote the contaminated + screen into the subsheet doc (empty room → file-seed + editor-snapshot + baseline; populated room → the next save-all uploaded the contaminated + file). From then on every reload materialized the self-referencing sheet. + +`doApplyItems` applies with `commit.Add(item, aFrame->GetScreen())` — whatever +screen is active when the deferred coroutine runs — so nothing on the C++ side +could refuse the wrong-screen apply. + +**Fix (two layers, red→green):** + +- `sheet-manager.ts doSwitch`: `superseded()` re-checks `requestedPath` after + every await and bails before binding/adopting; the room stays warm (parked). + Test: `sheet-manager.test.ts` "a switch superseded DURING its connect never + binds/adopts onto the new screen". +- Envelope tag (fix direction 2): `bindKicadCollab(…, { sheetPath })` stamps + every `applyItems` wire (remote change + adopt) with `sheet: ` (`itemsWireDeltaSchema.sheet`, optional); `eeschema_embind.cpp + applyTargetsShownSheet()` drops an envelope whose `sheet` is not the shown + screen's filename (suffix match on the MEMFS absolute path) and logs + `[collab] applyItems dropped`. Untagged envelopes keep the legacy behaviour + (single-file tools, older clients). Tests: `kicad-binding.test.ts` (tag + present on adopt + remote applies, absent when unbound), + `sheet-manager.test.ts` (each room bound with its own path), + `tests/kicad/apply-sheet-guard.spec.ts` (e2e: foreign-tagged apply dropped, + own-tagged + untagged applied). + +**Data repair:** an already-corrupted sheet doc is authoritative — re-upload the +original `.kicad_sch` for that path (the room re-seeds from the new file). diff --git a/tests/kicad/apply-sheet-guard.spec.ts b/tests/kicad/apply-sheet-guard.spec.ts new file mode 100644 index 0000000..63a5d18 --- /dev/null +++ b/tests/kicad/apply-sheet-guard.spec.ts @@ -0,0 +1,119 @@ +import type { Page } from "@playwright/test"; +import { test, expect } from "./fixtures"; + +/** + * ysync bug 07 (UP side) — `kicadCollabApplyItems` runs deferred on whatever + * sheet is shown WHEN IT RUNS. The binding stamps each envelope with its + * room's project-relative sheet path; the tool must drop an envelope whose + * `sheet` is not the shown screen (8/28 mega-demo-v2: the root's `(sheet …)` + * items were applied onto a subsheet, which then referenced itself and could + * not be loaded). Untagged envelopes keep the legacy behaviour. + */ + +type Mod = { + kicadOpenFile(p: string): unknown; + kicadCollabApplyItems(j: string): unknown; + kicadSaveSchematic(p: string): unknown; +}; +type FS = { + mkdirTree(p: string): void; + writeFile(p: string, d: string): void; + readFile(p: string, o: { encoding: "utf8" }): string; +}; + +const BOOT_TIMEOUT = 150000; +const DIR = "/home/kicad/documents/Arduino Mega 2560"; +const REL = "Arduino Mega 2560/root.kicad_sch"; + +const FIXTURE = `(kicad_sch + (version 20250114) + (generator "eeschema") + (generator_version "9.0") + (uuid "11111111-1111-1111-1111-111111111111") + (paper "A4") + (lib_symbols) + (wire (pts (xy 50.8 50.8) (xy 101.6 50.8)) (stroke (width 0) (type default)) (uuid "22222222-0000-0000-0000-000000000001")) + (sheet_instances (path "/" (page "1"))) +) +`; + +const text = (label: string, uuid: string) => + `(text "${label}" (exclude_from_sim no) (at 60.96 60.96 0) (effects (font (size 1.27 1.27))) (uuid "${uuid}"))`; +const WRONG = "33333333-0000-0000-0000-0000000000aa"; +const RIGHT = "33333333-0000-0000-0000-0000000000bb"; +const PLAIN = "33333333-0000-0000-0000-0000000000cc"; + +async function saveRead(page: Page): Promise { + return page.evaluate((dir) => { + const w = window as unknown as { FS: FS; Module: Mod }; + const out = `${dir}/probe.kicad_sch`; + w.Module.kicadSaveSchematic(out); + return w.FS.readFile(out, { encoding: "utf8" }); + }, DIR); +} + +test.describe("eeschema applyItems sheet guard (ysync bug 07 UP side)", () => { + test.describe.configure({ timeout: 420000 }); + + test("an envelope tagged for another sheet is dropped; the shown sheet's and untagged ones apply", async ({ + page, + testLogger, + }) => { + await page.goto("/kicad/eeschema.html"); + await expect(page.locator("#canvas")).toBeVisible({ timeout: BOOT_TIMEOUT }); + await page.waitForFunction( + () => { + const m = (window as unknown as { Module?: Partial }).Module; + return typeof m?.kicadOpenFile === "function" && typeof m?.kicadCollabApplyItems === "function"; + }, + null, + { timeout: BOOT_TIMEOUT }, + ); + await page.waitForFunction( + () => + !!window.wxElementRegistry && + window.wxElementRegistry + .findAll({ visible: true }) + .some((e) => /Frame$/.test(e.typeName) || (e.name || "").endsWith("Frame")), + null, + { timeout: BOOT_TIMEOUT }, + ); + await page.evaluate( + ({ dir, content }) => { + const w = window as unknown as { FS: FS; Module: Mod }; + w.FS.mkdirTree(dir); + w.FS.writeFile(`${dir}/root.kicad_sch`, content); + w.Module.kicadOpenFile(`${dir}/root.kicad_sch`); + }, + { dir: DIR, content: FIXTURE }, + ); + await expect.poll(async () => (await saveRead(page)).includes("22222222-0000"), { + timeout: 60000, + intervals: [500], + }).toBe(true); + + // Applies are serialized on the tool's coroutine: wrong → right → plain. + await page.evaluate( + ({ wrong, right, plain, rel }) => { + const m = (window as unknown as { Module: Mod }).Module; + m.kicadCollabApplyItems(JSON.stringify({ added: [{ sexpr: wrong }], sheet: "Arduino Mega 2560/ATMEGA2560-16AU.kicad_sch" })); + m.kicadCollabApplyItems(JSON.stringify({ added: [{ sexpr: right }], sheet: rel })); + m.kicadCollabApplyItems(JSON.stringify({ added: [{ sexpr: plain }] })); + }, + { wrong: text("Wrong", WRONG), right: text("Right", RIGHT), plain: text("Plain", PLAIN), rel: REL }, + ); + + await expect.poll(async () => (await saveRead(page)).includes(PLAIN), { + timeout: 25000, + intervals: [400], + }).toBe(true); + const saved = await saveRead(page); + expect(saved, "shown-sheet envelope applied").toContain(RIGHT); + expect(saved, "foreign-sheet envelope dropped").not.toContain(WRONG); + expect( + testLogger.consoleLogs.some((l) => l.includes("[collab] applyItems dropped")), + "drop is logged", + ).toBe(true); + expect([...testLogger.consoleLogs, ...testLogger.errors].some((s) => s.includes("Aborted(")), "no WASM abort").toBe(false); + }); +}); diff --git a/wasm/bindings/eeschema_embind.cpp b/wasm/bindings/eeschema_embind.cpp index e8a1bdc..33690f5 100644 --- a/wasm/bindings/eeschema_embind.cpp +++ b/wasm/bindings/eeschema_embind.cpp @@ -997,8 +997,44 @@ void doApply( SCH_EDIT_FRAME* aFrame, const json& aDelta ) // sheet (sch_editor_control.cpp Paste pattern) — then the loaded items are detached, // matched by uuid against the live model (replace), lib-relinked for symbols, and // committed. Runs inside the apply COROUTINE (see kicadCollabApplyItems). +// ysync bug 07 (UP side): an apply is queued for the room of ONE sheet but runs +// deferred on whatever screen is active when it runs. The JS binding stamps the +// envelope with its sheet's project-relative path; if that is not the screen +// shown now, adding the items here would put another sheet's items on this +// screen (8/28: the root's `(sheet …)` entries landed in a subsheet, which then +// referenced itself and failed to load). Drop it — the binding for the shown +// sheet reconciles on its own bind. +static bool applyTargetsShownSheet( SCH_EDIT_FRAME* aFrame, const json& aWire ) +{ + auto it = aWire.find( "sheet" ); + + if( it == aWire.end() || !it->is_string() ) + return true; // untagged (single-file tools, older clients): legacy behaviour + + const std::string want = it->get(); + SCH_SCREEN* cur = currentScreen( aFrame ); + const std::string have = cur ? toUtf8( cur->GetFileName() ) : std::string(); + + if( have == want ) + return true; + + // `have` is absolute (MEMFS project dir + relative path); `want` is relative. + if( have.size() > want.size() && have[have.size() - want.size() - 1] == '/' + && have.compare( have.size() - want.size(), want.size(), want ) == 0 ) + return true; + + EM_ASM( { console.warn( "[collab] applyItems dropped: envelope sheet " + UTF8ToString( $0 ) + + " != shown sheet " + UTF8ToString( $1 ) ); }, + want.c_str(), have.c_str() ); + return false; +} + + void doApplyItems( SCH_EDIT_FRAME* aFrame, const json& aWire ) { + if( !applyTargetsShownSheet( aFrame, aWire ) ) + return; + SCHEMATIC& sch = aFrame->Schematic(); s_applyingRemote = true; diff --git a/web/pcbjam-shared b/web/pcbjam-shared index 0f4d3a1..5014383 160000 --- a/web/pcbjam-shared +++ b/web/pcbjam-shared @@ -1 +1 @@ -Subproject commit 0f4d3a1e336fd5398fc071bd3a4181983b7eb3fb +Subproject commit 50143831109cd59e32db0e2c62c0e1f2320dd005 diff --git a/web/standalone/src/wasm/collab/kicad-binding.test.ts b/web/standalone/src/wasm/collab/kicad-binding.test.ts index 0652705..1980d4e 100644 --- a/web/standalone/src/wasm/collab/kicad-binding.test.ts +++ b/web/standalone/src/wasm/collab/kicad-binding.test.ts @@ -134,6 +134,33 @@ describe("bindKicadCollab — two editors over relayed Y.Docs", () => { } }); + it("stamps every apply envelope (adopt + remote change) with the binding's sheetPath", () => { + const { a, b } = pair(); + const edA = new FakeEditor(); + const edB = new FakeEditor(); + const bindA = bindKicadCollab(a, edA, { sheetPath: "Arduino Mega 2560/root.kicad_sch" }); + const bindB = bindKicadCollab(b, edB, { sheetPath: "Arduino Mega 2560/root.kicad_sch" }); + seedEditor(edA, FP); + bindA.seed(); + bindB.seed(); // adopt apply + edA.localUpsert(`(segment (start 0 0) (end 1 1) (uuid "seg-1"))`, null, "added"); // remote apply + expect(edB.applied.length).toBeGreaterThanOrEqual(2); + for (const json of edB.applied) { + expect((JSON.parse(json) as { sheet?: string }).sheet).toBe("Arduino Mega 2560/root.kicad_sch"); + } + // Untagged binding (single-file tools) leaves the envelope alone. + const { a: c, b: d } = pair(); + const edC = new FakeEditor(); + const edD = new FakeEditor(); + bindKicadCollab(c, edC).seed(); + seedEditor(edC, FP); + bindKicadCollab(d, edD).seed(); + edC.localUpsert(`(segment (start 0 0) (end 1 1) (uuid "seg-2"))`, null, "added"); + for (const json of edD.applied) { + expect("sheet" in (JSON.parse(json) as object)).toBe(false); + } + }); + it("a remote apply does not bounce back to the originator", () => { const { edA, edB, bindA, bindB } = setup(); seedEditor(edA, FP); diff --git a/web/standalone/src/wasm/collab/kicad-binding.ts b/web/standalone/src/wasm/collab/kicad-binding.ts index d9bf294..1650dbe 100644 --- a/web/standalone/src/wasm/collab/kicad-binding.ts +++ b/web/standalone/src/wasm/collab/kicad-binding.ts @@ -115,9 +115,19 @@ export function bindKicadCollab( * server-side; this keeps the client honest and quiet. */ readOnly?: boolean; + /** + * Project-relative path of the sheet this binding serves. Stamped on + * every applyItems envelope so the C++ side can refuse to apply it onto + * a different (now-active) screen — ysync bug 07 UP side, the 8/28 + * root-items-in-subsheet corruption. + */ + sheetPath?: string; }, ): KicadBinding { const readOnly = opts?.readOnly === true; + const sheetPath = opts?.sheetPath; + const tagged = (wire: ItemsWireDelta): ItemsWireDelta => + sheetPath === undefined ? wire : { ...wire, sheet: sheetPath }; // Version skew guard — callers bind AFTER the provider's initial sync, so the // doc's version is authoritative here (an empty room reads as v1 and is // stamped CURRENT by the first write). A read-only viewer never writes, but @@ -218,7 +228,7 @@ export function bindKicadCollab( removed: wire.removed.length, }); try { - bridge.applyItems(JSON.stringify(wire)); + bridge.applyItems(JSON.stringify(tagged(wire))); } catch (err) { // Symmetric with the DOWN hook's backstop above (findings C-7): a throw // here would otherwise unwind through Yjs's transaction cleanup inside @@ -461,7 +471,7 @@ export function bindKicadCollab( `+${adoptWire.added.length} ~${adoptWire.changed.length} -${adoptWire.removed.length}`, ); if (isEmptyItemsWireDelta(adoptWire)) return; // editor already matches — baseline only - bridge.applyItems(JSON.stringify(adoptWire)); + bridge.applyItems(JSON.stringify(tagged(adoptWire))); } return { diff --git a/web/standalone/src/wasm/collab/sheet-manager.test.ts b/web/standalone/src/wasm/collab/sheet-manager.test.ts index 5211a65..769d8ba 100644 --- a/web/standalone/src/wasm/collab/sheet-manager.test.ts +++ b/web/standalone/src/wasm/collab/sheet-manager.test.ts @@ -109,6 +109,16 @@ describe("sheet-manager warm pool", () => { expect(connectKicadDoc).toHaveBeenCalledTimes(2); }); + it("binds each room with its own sheetPath (apply envelope tag, bug 07 UP side)", async () => { + const m = makeManager(); + await m.switchTo("a.kicad_sch"); + await m.switchTo("sub/b.kicad_sch"); + expect(bindKicadCollab.mock.calls.map((c) => (c[2] as { sheetPath?: string }).sheetPath)).toEqual([ + "a.kicad_sch", + "sub/b.kicad_sch", + ]); + }); + it("first switch binds + seeds the active sheet", async () => { const m = makeManager(); await m.switchTo("a.kicad_sch"); @@ -307,6 +317,42 @@ describe("sheet-manager lifecycle hardening (findings C-1/C-4/C-5)", () => { expect(events).toEqual(["bind:a.kicad_sch", "clear", "bind:b.kicad_sch"]); }); + it("a switch superseded DURING its connect never binds/adopts onto the new screen (ysync bug 07 UP-side)", async () => { + // Real-world corruption (8/28, mega-demo-v2): the root's switch was still + // awaiting its room connect when the user entered a subsheet. The root + // switch then completed, bound the ROOT doc while the editor showed the + // SUBSHEET, and its adopt replaced the subsheet's items with the root's + // (incl. the `(sheet …)` pointing at the subsheet itself) — which the + // subsheet's own bind then wrote into the subsheet's room. + const m = makeManager(); + let releaseRoot!: () => void; + connectKicadDoc.mockImplementationOnce( + ({ room }: { room: string }) => + new Promise((res) => { + releaseRoot = () => { + const session = { room, doc: makeDoc(), provider: { destroy: vi.fn() } }; + sessions.push(session); + res(session); + }; + }), + ); + const rootSwitch = m.switchTo("root.kicad_sch"); + await new Promise((r) => setTimeout(r, 0)); // root switch parked on its connect + const subSwitch = m.switchTo("sub.kicad_sch"); // C++ already shows `sub` + releaseRoot(); + await Promise.all([rootSwitch, subSwitch]); + + // Nothing was ever bound/seeded while the editor showed a different sheet. + expect(bindings).toHaveLength(1); + expect(bindings[0]!.seed).toHaveBeenCalledTimes(1); + expect(m.active()?.sheetPath).toBe("sub.kicad_sch"); + // The root room stays warm (parked) — the superseded switch is not a failure. + expect(sessions.map((s) => s.room).sort()).toEqual([ + "S:P:root.kicad_sch", + "S:P:sub.kicad_sch", + ]); + }); + it("switchTo rejects SexprVersionError terminally — no retry, queue stays usable (C-5)", async () => { vi.useFakeTimers(); try { diff --git a/web/standalone/src/wasm/collab/sheet-manager.ts b/web/standalone/src/wasm/collab/sheet-manager.ts index b2687fb..1564c5a 100644 --- a/web/standalone/src/wasm/collab/sheet-manager.ts +++ b/web/standalone/src/wasm/collab/sheet-manager.ts @@ -256,6 +256,17 @@ export function createSheetCollabManager(opts: SheetManagerOptions): SheetCollab async function doSwitch(sheetPath: string): Promise { if (activePath === sheetPath) return; + // The editor navigated AGAIN while this switch was awaiting (ysync bug 07, + // UP side): binding + adopting now would apply THIS room's doc onto the + // screen of the newer sheet — the 8/28 mega-demo corruption, where the + // root doc's items (its `(sheet …)` entries included) replaced a subsheet's + // and were then written into the subsheet's room. Bail; the queued switch + // for the newer path binds the right room. The room stays warm (parked). + const superseded = (): boolean => { + if (requestedPath === sheetPath) return false; + clog(`[sheet] switch to ${sheetPath} superseded by ${requestedPath} — not binding`); + return true; + }; // Detach the OLD binding FIRST (before any await): the editor already navigated to // the new sheet, so the old binding's observer must stop applying remote edits onto @@ -279,19 +290,19 @@ export function createSheetCollabManager(opts: SheetManagerOptions): SheetCollab if (hadActive) opts.onActiveChange?.(null); const room = await ensureRoom(sheetPath); - if (destroyed) return; + if (destroyed || superseded()) return; // Gateway transport: a passively-warmed sheet holds no doc state yet — // activate() is the real sync barrier (no-op for other providers and on // revisits). Runs BEFORE the watch detaches so catch-up updates still // mark `dirty` for the adopt decision below. await room.session.provider.activate?.(); - if (destroyed) return; + if (destroyed || superseded()) return; // Activating: stop tracking parked updates and bind the (warm) doc to the editor. room.detachWatch?.(); room.detachWatch = undefined; - const binding = bindKicadCollab(room.doc, bridge, { readOnly: opts.readOnly }); + const binding = bindKicadCollab(room.doc, bridge, { readOnly: opts.readOnly, sheetPath }); room.binding = binding; if (!room.seeded) {