fix(collab): group C session-lifecycle hardening — C-1..C-7
C-1: pending-session ownership in WasmTool (registered before adoption, destroyed on every non-adoption exit incl. unmount/late-handoff guards); sheet-manager destroyed flag refuses post-destroy connects. C-2: attachKicadCollab destroys a partially-attached binding on seed throw. C-3: connectKicadDoc gains a 30s whole-path deadline + abort signal covering provider import/construction/initial sync, with partial cleanup on every loss path (incl. late-resolving construction). C-4: sheet switch clears host presence/comments/follow/drift callbacks BEFORE the new room connects (onActiveChange(null) pre-connect). C-5: switchTo rethrows SexprVersionError terminally (no retry timer, queue unpoisoned); boot surfaces it, nav hook degrades per-sheet. C-6: sibling-restage failed dial retries on 1s→30s backoff instead of latching forever; roster churn still never re-dials. C-7: terminal-error promote() now tears down every ws-driven collab ingress (shared teardownCollab) — no ticket storm under the fatal overlay; UP apply observer gains the symmetric try/catch + clean-stack re-surface. Tests: index.test.ts (new, 8), sheet-manager +3, sibling-restage +2; standalone units 135/135, tsc clean; ysync-two-tab + eeschema-subschema 7/7 kicad-chromium on the rebuilt bundle. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UZJ1pUePb4W47hGoLMYTw4
This commit is contained in:
parent
1b99b63851
commit
7d500a5f9e
8 changed files with 641 additions and 51 deletions
|
|
@ -591,6 +591,8 @@ async function maybeConnectDocSession(
|
|||
scopeId: string;
|
||||
projectId: string;
|
||||
targetPath?: string;
|
||||
/** Unmount abort — cancels the connect and destroys partials (C-1/C-3). */
|
||||
signal?: AbortSignal;
|
||||
log: (m: string) => void;
|
||||
},
|
||||
): Promise<{ session?: KicadDocSession; targetBytes?: Uint8Array }> {
|
||||
|
|
@ -599,7 +601,11 @@ async function maybeConnectDocSession(
|
|||
|
||||
const { connectKicadDoc } = await import("@/wasm/collab");
|
||||
const room = collabRoomId(opts.scopeId, opts.projectId, opts.targetPath);
|
||||
const session = await connectKicadDoc({ provider: yjsProviderConfig(), room });
|
||||
const session = await connectKicadDoc({
|
||||
provider: yjsProviderConfig(),
|
||||
room,
|
||||
signal: opts.signal,
|
||||
});
|
||||
|
||||
// Use the full doc state (meta + layout + items), NOT just item count: a
|
||||
// populated drawing sheet (pl_editor `.kicad_wks`) has zero uuid items, so an
|
||||
|
|
@ -792,7 +798,17 @@ async function startSheetCollab(
|
|||
// C++ navigation → rebind the active room to the now-shown sheet.
|
||||
registerSheetChangedHook(win as unknown as SheetChangedWindow, (abs) => {
|
||||
const rel = relativeProjectPath(opts.slug, abs);
|
||||
if (rel) void manager.switchTo(rel);
|
||||
// switchTo rejects on TERMINAL failures only (SexprVersionError — C-5);
|
||||
// transient failures retry internally. A skewed sheet mid-session can't
|
||||
// fail the whole boot anymore, so log it and leave the sheet unbound.
|
||||
if (rel) {
|
||||
manager.switchTo(rel).catch((err: unknown) => {
|
||||
opts.log(
|
||||
`[sheet] ${rel} needs a newer app version — collab disabled for this sheet: ${String(err)}`,
|
||||
);
|
||||
opts.onStatus("Collab: version skew on this sheet");
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
// C++ sheet creation ("Add Sheet") → the child .kicad_sch was just written to MEMFS by
|
||||
|
|
@ -808,7 +824,17 @@ async function startSheetCollab(
|
|||
// Warm every schematic file in the project so later sheet switches are instant.
|
||||
void manager.connectAll(sheetPaths);
|
||||
|
||||
if (opts.targetPath) await manager.switchTo(opts.targetPath);
|
||||
if (opts.targetPath) {
|
||||
try {
|
||||
await manager.switchTo(opts.targetPath);
|
||||
} catch (err) {
|
||||
// switchTo only rejects on TERMINAL failures (SexprVersionError — C-5).
|
||||
// The manager already owns the entry session + every warmed room; tear
|
||||
// it down before surfacing, or the boot error leaks the pool (C-1).
|
||||
manager.destroy();
|
||||
throw err;
|
||||
}
|
||||
}
|
||||
opts.log(`[sheet] multi-room collab active (${sheetPaths.length} sheet(s) warmed)`);
|
||||
opts.onStatus("Collab: connected");
|
||||
return manager;
|
||||
|
|
@ -1078,6 +1104,52 @@ export function WasmTool({
|
|||
const [commentsSlot, setCommentsSlot] = React.useState<HTMLDivElement | null>(null);
|
||||
const [viewportState, setViewportState] = React.useState<ViewportState | null>(null);
|
||||
const commentsRef = React.useRef<CommentsController | null>(null);
|
||||
// A doc session that connected but has not been ADOPTED by an owner yet
|
||||
// (collab handle / sheet manager). Owned here so a boot failure, an
|
||||
// open-never-settled degrade, or unmount can destroy it instead of leaking
|
||||
// the socket + doc (findings C-1).
|
||||
const pendingDocSessionRef = React.useRef<KicadDocSession | null>(null);
|
||||
// Tear down every collab surface that dispatches into the wasm on ws/doc
|
||||
// events. Shared by the unmount cleanup AND the terminal-error promote
|
||||
// (findings C-7): after a terminal wasm death, a still-connected room kept
|
||||
// delivering awareness/doc updates and each one re-entered the dead
|
||||
// instance — an unbounded ticket storm underneath the fatal overlay.
|
||||
const teardownCollab = React.useCallback(() => {
|
||||
commentsRef.current?.destroy();
|
||||
commentsRef.current = null;
|
||||
followRef.current?.destroy();
|
||||
followRef.current = null;
|
||||
presenceBridgeRef.current?.destroy();
|
||||
presenceBridgeRef.current = null;
|
||||
presenceRef.current?.destroy();
|
||||
presenceRef.current = null;
|
||||
crossAppRef.current?.destroy();
|
||||
crossAppRef.current = null;
|
||||
siblingRestageRef.current?.destroy();
|
||||
siblingRestageRef.current = null;
|
||||
driftRef.current?.stop();
|
||||
driftRef.current = null;
|
||||
// Tears down every warm room's provider/doc (the only place providers are
|
||||
// destroyed — switching sheets keeps them connected) and clears drift via
|
||||
// onActiveChange(null).
|
||||
sheetManagerRef.current?.destroy();
|
||||
sheetManagerRef.current = null;
|
||||
// The single-room (pcbnew/pl_editor) counterpart: binding + provider +
|
||||
// doc. Without this the board room's socket survived navigation.
|
||||
collabHandleRef.current?.destroy();
|
||||
collabHandleRef.current = null;
|
||||
collabDocRef.current = null;
|
||||
const pending = pendingDocSessionRef.current;
|
||||
pendingDocSessionRef.current = null;
|
||||
if (pending) {
|
||||
try {
|
||||
pending.provider.destroy();
|
||||
pending.doc.destroy();
|
||||
} catch {
|
||||
/* teardown is best-effort */
|
||||
}
|
||||
}
|
||||
}, []);
|
||||
// Unread-comments rollup for the FAB badge (comments-ux 0001 C).
|
||||
const [commentsUnread, setCommentsUnread] = React.useState({ threads: 0, mentioned: false });
|
||||
const onCommentsUnread = React.useCallback(
|
||||
|
|
@ -1291,6 +1363,11 @@ export function WasmTool({
|
|||
);
|
||||
const rec = dumper.__wxWaitDump?.();
|
||||
if (rec) append(JSON.stringify(rec));
|
||||
// Stop the ticket storm (findings C-7): every ws-driven collab ingress
|
||||
// (remote applies, presence push, comment pins, follow fit, drift saves)
|
||||
// re-armed on the next event and re-entered the DEAD instance behind
|
||||
// the overlay. Terminal means the native lifetime is over — unhook it.
|
||||
teardownCollab();
|
||||
setFatal(msg);
|
||||
setShowLog(true);
|
||||
// Arm the React-independent floor too: it stays invisible while our
|
||||
|
|
@ -1659,6 +1736,7 @@ export function WasmTool({
|
|||
scopeId,
|
||||
projectId,
|
||||
targetPath,
|
||||
signal: presyncAbort.signal,
|
||||
log: append,
|
||||
});
|
||||
} catch (error) {
|
||||
|
|
@ -1773,6 +1851,11 @@ export function WasmTool({
|
|||
const docResult = await docSessionReady;
|
||||
if ("error" in docResult) throw docResult.error;
|
||||
const { session, targetBytes } = docResult;
|
||||
// The session is connected but ownerless until a collab handle or the
|
||||
// sheet manager adopts it below — register it so every failure exit
|
||||
// (boot throw, open-never-settled, degrade, unmount) destroys it
|
||||
// instead of leaking the socket + doc (findings C-1).
|
||||
pendingDocSessionRef.current = session ?? null;
|
||||
const openResult = await driveProjectIntoTool(win, {
|
||||
tool,
|
||||
slug,
|
||||
|
|
@ -1844,7 +1927,14 @@ export function WasmTool({
|
|||
// Cross-app presence: the ROOM was joined back in the boot fan-out
|
||||
// (pure network + Y.Doc, no wasm) — only the handoff to the wasm-bound
|
||||
// presence below has to wait for the open. Settle it here.
|
||||
crossAppRef.current = (await crossAppReady) ?? null;
|
||||
const crossAppHandle = (await crossAppReady) ?? null;
|
||||
if (disposedRef.current) {
|
||||
// Unmounted while awaiting — cleanup already ran; adopting now would
|
||||
// leak a live socket behind a dead component (findings C-1).
|
||||
crossAppHandle?.destroy();
|
||||
return;
|
||||
}
|
||||
crossAppRef.current = crossAppHandle;
|
||||
// Test/debug handle (mirrors __pcbjamComments): lets the e2e assert
|
||||
// the project-room peer view without driving pixels.
|
||||
(win as { __pcbjamCrossApp?: CrossAppHandle | null }).__pcbjamCrossApp =
|
||||
|
|
@ -1889,6 +1979,17 @@ export function WasmTool({
|
|||
log: append,
|
||||
onStatus: setStatus,
|
||||
})) ?? null;
|
||||
if (sheetManagerRef.current) {
|
||||
// The manager's room pool now owns the entry session (destroy()
|
||||
// tears it down with every other warm room).
|
||||
pendingDocSessionRef.current = null;
|
||||
}
|
||||
if (disposedRef.current) {
|
||||
// Late handoff after unmount (findings C-1): cleanup already ran.
|
||||
sheetManagerRef.current?.destroy();
|
||||
sheetManagerRef.current = null;
|
||||
return;
|
||||
}
|
||||
} else {
|
||||
const collabHandle = await maybeStartCollab(win, {
|
||||
tool,
|
||||
|
|
@ -1902,6 +2003,16 @@ export function WasmTool({
|
|||
log: append,
|
||||
onStatus: setStatus,
|
||||
});
|
||||
if (collabHandle) {
|
||||
// The handle owns the session now (destroy() covers binding +
|
||||
// provider + doc).
|
||||
pendingDocSessionRef.current = null;
|
||||
}
|
||||
if (disposedRef.current) {
|
||||
// Late handoff after unmount (findings C-1): cleanup already ran.
|
||||
collabHandle?.destroy();
|
||||
return;
|
||||
}
|
||||
collabHandleRef.current = collabHandle ?? null;
|
||||
collabDocRef.current = collabHandle?.doc ?? null;
|
||||
startPresence(collabHandle?.provider, undefined, collabHandle?.doc);
|
||||
|
|
@ -1969,11 +2080,27 @@ export function WasmTool({
|
|||
}
|
||||
}
|
||||
};
|
||||
// Degrading without an adoption must not strand the pre-connected doc
|
||||
// session (findings C-1: the open-never-settled path was the most
|
||||
// reproducible leak — a live socket + doc with no owner, forever).
|
||||
const releasePendingDocSession = (why: string) => {
|
||||
const pending = pendingDocSessionRef.current;
|
||||
if (!pending) return;
|
||||
pendingDocSessionRef.current = null;
|
||||
try {
|
||||
pending.provider.destroy();
|
||||
pending.doc.destroy();
|
||||
} catch {
|
||||
/* best-effort */
|
||||
}
|
||||
append(`[collab] released unadopted doc session (${why})`);
|
||||
};
|
||||
if (openResult === "failed") {
|
||||
// The load never settled (or a legacy-wasm open timed out): entering
|
||||
// the wasm now would race the parked open chain. Boot on without
|
||||
// collab/presence — the board stays viewable, saves still route.
|
||||
append("[collab] file open never settled — collab/presence disabled for this session");
|
||||
releasePendingDocSession("open never settled");
|
||||
} else {
|
||||
try {
|
||||
await attachCollabAndPresence();
|
||||
|
|
@ -1983,7 +2110,10 @@ export function WasmTool({
|
|||
// Degrade, don't die: a residual wasm trap here (reentrancy during
|
||||
// some other parked chain) used to fail the whole boot.
|
||||
append(`[collab] attach failed — continuing without collab: ${String(err)}`);
|
||||
releasePendingDocSession("attach failed");
|
||||
}
|
||||
// A no-op attach (bridge missing / non-collab tool) adopts nothing.
|
||||
releasePendingDocSession("not adopted");
|
||||
}
|
||||
// Deferred-realtime upgrade: the scope libs source opens its stacks
|
||||
// channel-less (no socket per org lib), so promote the libs the OPEN
|
||||
|
|
@ -2024,6 +2154,19 @@ export function WasmTool({
|
|||
append(dumpTrace());
|
||||
setStatus(`Error: ${String(err)}`);
|
||||
setFatal(String(err));
|
||||
// A boot that died between the doc-room connect and its adoption
|
||||
// (open failure, read-only lock, version skew) must not strand the
|
||||
// live session behind the error overlay (findings C-1).
|
||||
const pending = pendingDocSessionRef.current;
|
||||
pendingDocSessionRef.current = null;
|
||||
if (pending) {
|
||||
try {
|
||||
pending.provider.destroy();
|
||||
pending.doc.destroy();
|
||||
} catch {
|
||||
/* best-effort */
|
||||
}
|
||||
}
|
||||
}
|
||||
})();
|
||||
|
||||
|
|
@ -2041,30 +2184,8 @@ export function WasmTool({
|
|||
presyncAbort.abort();
|
||||
win.removeEventListener("keydown", swallowBrowserSave, true);
|
||||
win.removeEventListener("keydown", chromeHotkey, true);
|
||||
commentsRef.current?.destroy();
|
||||
commentsRef.current = null;
|
||||
followRef.current?.destroy();
|
||||
followRef.current = null;
|
||||
presenceBridgeRef.current?.destroy();
|
||||
presenceBridgeRef.current = null;
|
||||
presenceRef.current?.destroy();
|
||||
presenceRef.current = null;
|
||||
crossAppRef.current?.destroy();
|
||||
crossAppRef.current = null;
|
||||
siblingRestageRef.current?.destroy();
|
||||
siblingRestageRef.current = null;
|
||||
driftRef.current?.stop();
|
||||
driftRef.current = null;
|
||||
// Tears down every warm room's provider/doc (the only place providers are
|
||||
// destroyed — switching sheets keeps them connected) and clears drift via
|
||||
// onActiveChange(null).
|
||||
sheetManagerRef.current?.destroy();
|
||||
sheetManagerRef.current = null;
|
||||
// The single-room (pcbnew/pl_editor) counterpart: binding + provider +
|
||||
// doc. Without this the board room's socket survived navigation.
|
||||
collabHandleRef.current?.destroy();
|
||||
collabHandleRef.current = null;
|
||||
collabDocRef.current = null;
|
||||
// Every collab surface + any not-yet-adopted doc session (C-1/C-7).
|
||||
teardownCollab();
|
||||
// Close the lib SyncStacks this boot opened (mirror mux + any dedicated
|
||||
// sockets); IDB caches stay. Injected sources belong to the caller.
|
||||
ownedLibsSource?.dispose?.();
|
||||
|
|
|
|||
183
web/standalone/src/wasm/collab/index.test.ts
Normal file
183
web/standalone/src/wasm/collab/index.test.ts
Normal file
|
|
@ -0,0 +1,183 @@
|
|||
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
|
||||
import * as Y from "yjs";
|
||||
|
||||
// Exercise connectKicadDoc's deadline/abort/partial-cleanup (findings C-1/C-3)
|
||||
// and attachKicadCollab's seed-throw teardown (C-2) against mocked
|
||||
// collaborators — no sockets, no wasm.
|
||||
const { connectProvider, bindKicadCollab, moduleItemsBridge } = vi.hoisted(() => ({
|
||||
connectProvider: vi.fn(),
|
||||
bindKicadCollab: vi.fn(),
|
||||
moduleItemsBridge: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock("./provider", () => ({ connectProvider }));
|
||||
vi.mock("./kicad-binding", () => ({
|
||||
bindKicadCollab,
|
||||
moduleItemsBridge,
|
||||
SexprVersionError: class SexprVersionError extends Error {},
|
||||
}));
|
||||
|
||||
import {
|
||||
attachKicadCollab,
|
||||
CollabConnectTimeoutError,
|
||||
connectKicadDoc,
|
||||
} from "./index";
|
||||
|
||||
interface FakeProvider {
|
||||
whenSynced: ReturnType<typeof vi.fn>;
|
||||
destroy: ReturnType<typeof vi.fn>;
|
||||
awareness?: { setLocalState: ReturnType<typeof vi.fn> };
|
||||
}
|
||||
|
||||
function makeProvider(opts?: { neverSync?: boolean }): FakeProvider {
|
||||
return {
|
||||
whenSynced: vi.fn(() =>
|
||||
opts?.neverSync ? new Promise<void>(() => {}) : Promise.resolve(),
|
||||
),
|
||||
destroy: vi.fn(),
|
||||
awareness: { setLocalState: vi.fn() },
|
||||
};
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
vi.useFakeTimers();
|
||||
connectProvider.mockReset();
|
||||
bindKicadCollab.mockReset();
|
||||
moduleItemsBridge.mockReset();
|
||||
moduleItemsBridge.mockReturnValue({});
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
vi.useRealTimers();
|
||||
});
|
||||
|
||||
describe("connectKicadDoc — deadline + cancellation cover the whole path (C-3)", () => {
|
||||
it("connects normally within the deadline", async () => {
|
||||
const provider = makeProvider();
|
||||
connectProvider.mockResolvedValue(provider);
|
||||
const session = await connectKicadDoc({
|
||||
provider: { kind: "none" } as never,
|
||||
room: "r",
|
||||
});
|
||||
expect(session.provider).toBe(provider);
|
||||
expect(provider.destroy).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("times out a sync that never fires and destroys doc + provider", async () => {
|
||||
const provider = makeProvider({ neverSync: true });
|
||||
connectProvider.mockResolvedValue(provider);
|
||||
const attempt = connectKicadDoc({
|
||||
provider: { kind: "none" } as never,
|
||||
room: "r",
|
||||
timeoutMs: 30_000,
|
||||
});
|
||||
const outcome = expect(attempt).rejects.toBeInstanceOf(
|
||||
CollabConnectTimeoutError,
|
||||
);
|
||||
await vi.advanceTimersByTimeAsync(30_000);
|
||||
await outcome;
|
||||
expect(provider.destroy).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("times out a stalled provider construction (lazy import hang)", async () => {
|
||||
// connectProvider never resolves — the shape of a stalled chunk fetch.
|
||||
connectProvider.mockReturnValue(new Promise(() => {}));
|
||||
const attempt = connectKicadDoc({
|
||||
provider: { kind: "none" } as never,
|
||||
room: "r",
|
||||
timeoutMs: 30_000,
|
||||
});
|
||||
const outcome = expect(attempt).rejects.toBeInstanceOf(
|
||||
CollabConnectTimeoutError,
|
||||
);
|
||||
await vi.advanceTimersByTimeAsync(30_000);
|
||||
await outcome;
|
||||
});
|
||||
|
||||
it("destroys a provider whose construction resolves after the race was lost", async () => {
|
||||
const provider = makeProvider();
|
||||
let release!: () => void;
|
||||
connectProvider.mockReturnValue(
|
||||
new Promise((res) => {
|
||||
release = () => res(provider);
|
||||
}),
|
||||
);
|
||||
const attempt = connectKicadDoc({
|
||||
provider: { kind: "none" } as never,
|
||||
room: "r",
|
||||
timeoutMs: 1_000,
|
||||
});
|
||||
const outcome = expect(attempt).rejects.toBeInstanceOf(
|
||||
CollabConnectTimeoutError,
|
||||
);
|
||||
await vi.advanceTimersByTimeAsync(1_000);
|
||||
await outcome;
|
||||
expect(provider.destroy).not.toHaveBeenCalled(); // not built yet
|
||||
release();
|
||||
await vi.advanceTimersByTimeAsync(0);
|
||||
expect(provider.destroy).toHaveBeenCalled(); // late arrival self-destroys
|
||||
});
|
||||
|
||||
it("an abort signal cancels the connect and destroys partials", async () => {
|
||||
const provider = makeProvider({ neverSync: true });
|
||||
connectProvider.mockResolvedValue(provider);
|
||||
const controller = new AbortController();
|
||||
const attempt = connectKicadDoc({
|
||||
provider: { kind: "none" } as never,
|
||||
room: "r",
|
||||
signal: controller.signal,
|
||||
});
|
||||
const outcome = expect(attempt).rejects.toThrow(/aborted/);
|
||||
await vi.advanceTimersByTimeAsync(0);
|
||||
controller.abort();
|
||||
await outcome;
|
||||
expect(provider.destroy).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("a pre-aborted signal refuses before building anything", async () => {
|
||||
const provider = makeProvider();
|
||||
connectProvider.mockResolvedValue(provider);
|
||||
const controller = new AbortController();
|
||||
controller.abort();
|
||||
await expect(
|
||||
connectKicadDoc({
|
||||
provider: { kind: "none" } as never,
|
||||
room: "r",
|
||||
signal: controller.signal,
|
||||
}),
|
||||
).rejects.toThrow();
|
||||
expect(connectProvider).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe("attachKicadCollab — a seed throw cannot leak the binding (C-2)", () => {
|
||||
it("destroys the partially-attached binding and rethrows", () => {
|
||||
const binding = {
|
||||
seed: vi.fn(() => {
|
||||
throw new Error("bridge trap during seed");
|
||||
}),
|
||||
destroy: vi.fn(),
|
||||
items: new Map(),
|
||||
};
|
||||
bindKicadCollab.mockReturnValue(binding);
|
||||
const session = { doc: new Y.Doc(), provider: makeProvider() };
|
||||
|
||||
expect(() =>
|
||||
attachKicadCollab({} as never, {} as never, session as never),
|
||||
).toThrow("bridge trap during seed");
|
||||
// The binding (global DOWN hook + doc observers) is torn down; the
|
||||
// SESSION is untouched — its owner decides what happens next.
|
||||
expect(binding.destroy).toHaveBeenCalledTimes(1);
|
||||
expect(session.provider.destroy).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("returns a working handle when seed succeeds", () => {
|
||||
const binding = { seed: vi.fn(), destroy: vi.fn(), items: new Map() };
|
||||
bindKicadCollab.mockReturnValue(binding);
|
||||
const session = { doc: new Y.Doc(), provider: makeProvider() };
|
||||
const handle = attachKicadCollab({} as never, {} as never, session as never);
|
||||
handle.destroy();
|
||||
expect(binding.destroy).toHaveBeenCalledTimes(1);
|
||||
expect(session.provider.destroy).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
});
|
||||
|
|
@ -125,20 +125,89 @@ export interface KicadDocSession {
|
|||
provider: YjsProvider;
|
||||
}
|
||||
|
||||
/** The connect path exceeded its deadline (import + construct + initial sync). */
|
||||
export class CollabConnectTimeoutError extends Error {
|
||||
constructor(room: string, timeoutMs: number) {
|
||||
super(`collab connect to ${room} exceeded ${timeoutMs}ms`);
|
||||
this.name = "CollabConnectTimeoutError";
|
||||
}
|
||||
}
|
||||
|
||||
const CONNECT_TIMEOUT_MS = 30_000;
|
||||
|
||||
/**
|
||||
* Connect a fresh Y.Doc to a provider room and wait for its authoritative
|
||||
* initial state. Used standalone by the Y.Doc-load path (materialize the file
|
||||
* from the doc BEFORE any editor exists), and as the first half of
|
||||
* `startKicadCollab`.
|
||||
*
|
||||
* The deadline + abort signal cover the WHOLE path — the provider module's
|
||||
* lazy import, provider construction, and the initial sync (findings C-3: a
|
||||
* stalled chunk fetch or a `whenSynced` that never fires used to escape every
|
||||
* timeout). On failure/timeout/abort, everything partially built is destroyed
|
||||
* (findings C-1: no orphaned doc or socket), including a provider whose
|
||||
* construction resolves only after the race was lost.
|
||||
*/
|
||||
export async function connectKicadDoc(opts: {
|
||||
provider: ProviderConfig;
|
||||
room: string;
|
||||
/** Owner teardown (unmount, boot failure) — cancels + destroys partials. */
|
||||
signal?: AbortSignal;
|
||||
/** Whole-path deadline; defaults to 30s. */
|
||||
timeoutMs?: number;
|
||||
}): Promise<KicadDocSession> {
|
||||
const timeoutMs = opts.timeoutMs ?? CONNECT_TIMEOUT_MS;
|
||||
// An already-aborted owner never gets a session — even one that could
|
||||
// connect instantly (Promise.race settles in array order for pre-settled
|
||||
// promises, so the guard must run before any construction).
|
||||
if (opts.signal?.aborted) {
|
||||
throw opts.signal.reason ?? new Error("collab connect aborted");
|
||||
}
|
||||
const doc = new Y.Doc();
|
||||
const provider = await connectProvider(doc, opts.provider, { room: opts.room });
|
||||
await provider.whenSynced();
|
||||
return { doc, provider };
|
||||
let timer: ReturnType<typeof setTimeout> | undefined;
|
||||
let onAbort: (() => void) | undefined;
|
||||
|
||||
const fail = new Promise<never>((_, reject) => {
|
||||
if (opts.signal?.aborted) {
|
||||
reject(opts.signal.reason ?? new Error("collab connect aborted"));
|
||||
return;
|
||||
}
|
||||
onAbort = () =>
|
||||
reject(opts.signal?.reason ?? new Error("collab connect aborted"));
|
||||
opts.signal?.addEventListener("abort", onAbort, { once: true });
|
||||
timer = setTimeout(
|
||||
() => reject(new CollabConnectTimeoutError(opts.room, timeoutMs)),
|
||||
timeoutMs,
|
||||
);
|
||||
});
|
||||
// The failure promise only ever loses a race on the success path — silence
|
||||
// its rejection so a post-success abort can't surface as unhandled.
|
||||
fail.catch(() => {});
|
||||
|
||||
const providerPromise = connectProvider(doc, opts.provider, {
|
||||
room: opts.room,
|
||||
});
|
||||
providerPromise.catch(() => {}); // may lose the race and reject later
|
||||
|
||||
let provider: YjsProvider | undefined;
|
||||
try {
|
||||
provider = await Promise.race([providerPromise, fail]);
|
||||
await Promise.race([provider.whenSynced(), fail]);
|
||||
return { doc, provider };
|
||||
} catch (err) {
|
||||
if (provider) {
|
||||
provider.destroy();
|
||||
} else {
|
||||
// Construction may still be in flight (or racing this very catch) —
|
||||
// whenever it lands, the late provider self-destroys.
|
||||
providerPromise.then((p) => p.destroy()).catch(() => {});
|
||||
}
|
||||
doc.destroy();
|
||||
throw err;
|
||||
} finally {
|
||||
if (timer !== undefined) clearTimeout(timer);
|
||||
if (onAbort) opts.signal?.removeEventListener("abort", onAbort);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -162,7 +231,16 @@ export function attachKicadCollab(
|
|||
const binding = bindKicadCollab(session.doc, moduleItemsBridge(mod, win), {
|
||||
readOnly: opts?.readOnly,
|
||||
});
|
||||
binding.seed(opts?.seedDoc, { editorMatchesDoc: opts?.editorMatchesDoc });
|
||||
try {
|
||||
binding.seed(opts?.seedDoc, { editorMatchesDoc: opts?.editorMatchesDoc });
|
||||
} catch (err) {
|
||||
// A partially-attached binding must not survive a seed throw (findings
|
||||
// C-2): it already owns the global DOWN hook + doc observers, and the
|
||||
// handle that could destroy it would never reach the caller. The SESSION
|
||||
// stays alive — its owner decides (degrade / retry / teardown).
|
||||
binding.destroy();
|
||||
throw err;
|
||||
}
|
||||
clog("attachKicadCollab: ready; doc items =", binding.items.size);
|
||||
|
||||
return {
|
||||
|
|
|
|||
|
|
@ -214,7 +214,23 @@ export function bindKicadCollab(
|
|||
changed: wire.changed.length,
|
||||
removed: wire.removed.length,
|
||||
});
|
||||
bridge.applyItems(JSON.stringify(wire));
|
||||
try {
|
||||
bridge.applyItems(JSON.stringify(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
|
||||
// the provider's applyUpdate. Log, then re-surface on a clean stack so
|
||||
// the global terminal-error classifier (WasmTool promote) still sees a
|
||||
// wasm death — without corrupting the doc's observer bookkeeping.
|
||||
cwarn("⬆ remote Y change: apply to editor failed", err);
|
||||
const report =
|
||||
(globalThis as { reportError?: (e: unknown) => void }).reportError ??
|
||||
((e: unknown) =>
|
||||
setTimeout(() => {
|
||||
throw e;
|
||||
}, 0));
|
||||
report(err);
|
||||
}
|
||||
};
|
||||
items.observeDeep(observer);
|
||||
|
||||
|
|
|
|||
|
|
@ -194,3 +194,88 @@ describe("sheet-manager warm pool", () => {
|
|||
expect(bindings[0]!.lastSeedOpts).toEqual({ editorMatchesDoc: true });
|
||||
});
|
||||
});
|
||||
|
||||
describe("sheet-manager lifecycle hardening (findings C-1/C-4/C-5)", () => {
|
||||
it("a connect resolving after destroy() is torn down, not registered (C-1)", async () => {
|
||||
let release!: () => void;
|
||||
const late: FakeSession = {
|
||||
room: "late",
|
||||
doc: makeDoc(),
|
||||
provider: { destroy: vi.fn() },
|
||||
};
|
||||
connectKicadDoc.mockImplementationOnce(
|
||||
() =>
|
||||
new Promise((res) => {
|
||||
release = () => res(late);
|
||||
}),
|
||||
);
|
||||
const m = makeManager();
|
||||
const warm = m.connectAll(["late.kicad_sch"]);
|
||||
m.destroy();
|
||||
release();
|
||||
await warm; // connectAll swallows the (deliberate) post-destroy failure
|
||||
expect(late.provider.destroy).toHaveBeenCalled();
|
||||
expect(late.doc.destroy).toHaveBeenCalled();
|
||||
expect(m.active()).toBeNull();
|
||||
});
|
||||
|
||||
it("clears the host's per-sheet callbacks BEFORE the new room connects (C-4)", async () => {
|
||||
const events: string[] = [];
|
||||
const m = createSheetCollabManager({
|
||||
mod: {} as never,
|
||||
win: {} as never,
|
||||
scopeId: "S",
|
||||
projectId: "P",
|
||||
provider: { kind: "none" } as never,
|
||||
seedDocForPath: () => undefined,
|
||||
onActiveChange: (active) =>
|
||||
events.push(active ? `bind:${active.sheetPath}` : "clear"),
|
||||
log: () => {},
|
||||
});
|
||||
await m.switchTo("a.kicad_sch");
|
||||
expect(events).toEqual(["bind:a.kicad_sch"]);
|
||||
|
||||
// Gate sheet b's connect so the pre-connect window is observable.
|
||||
let releaseB!: () => void;
|
||||
connectKicadDoc.mockImplementationOnce(
|
||||
({ room }: { room: string }) =>
|
||||
new Promise((res) => {
|
||||
releaseB = () =>
|
||||
res({ room, doc: makeDoc(), provider: { destroy: vi.fn() } });
|
||||
}),
|
||||
);
|
||||
const sw = m.switchTo("b.kicad_sch");
|
||||
await new Promise((r) => setTimeout(r, 0)); // let doSwitch reach the connect await
|
||||
// The old sheet's presence/comments/follow/drift were cleared while b is
|
||||
// STILL CONNECTING — the bleed window is closed.
|
||||
expect(events).toEqual(["bind:a.kicad_sch", "clear"]);
|
||||
releaseB();
|
||||
await sw;
|
||||
expect(events).toEqual(["bind:a.kicad_sch", "clear", "bind:b.kicad_sch"]);
|
||||
});
|
||||
|
||||
it("switchTo rejects SexprVersionError terminally — no retry, queue stays usable (C-5)", async () => {
|
||||
vi.useFakeTimers();
|
||||
try {
|
||||
const m = makeManager();
|
||||
const skew = Object.assign(new Error("doc written by a newer encoding"), {
|
||||
name: "SexprVersionError",
|
||||
});
|
||||
bindKicadCollab.mockImplementationOnce(() => {
|
||||
throw skew;
|
||||
});
|
||||
await expect(m.switchTo("a.kicad_sch")).rejects.toBe(skew);
|
||||
|
||||
// Terminal: no backoff timer was armed (the old behavior retried 2s→30s
|
||||
// forever and the returned promise never rejected).
|
||||
await vi.advanceTimersByTimeAsync(120_000);
|
||||
expect(bindKicadCollab).toHaveBeenCalledTimes(1);
|
||||
|
||||
// The serialization queue is not poisoned: explicit navigation works.
|
||||
await m.switchTo("b.kicad_sch");
|
||||
expect(m.active()?.sheetPath).toBe("b.kicad_sch");
|
||||
} finally {
|
||||
vi.useRealTimers();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -136,6 +136,9 @@ export function createSheetCollabManager(opts: SheetManagerOptions): SheetCollab
|
|||
// mode, entry sheet) share ONE connection instead of opening the room twice.
|
||||
const connecting = new Map<string, Promise<Room>>();
|
||||
let activePath: string | null = null;
|
||||
// destroy() ran — a connect resolving after it must tear its session down
|
||||
// instead of registering a leaked socket+doc (findings C-1).
|
||||
let destroyed = false;
|
||||
|
||||
// Coalesce rapid navigations: only the LATEST requested sheet is actually bound, and
|
||||
// switches run one-at-a-time so concurrent `onSheetChanged` events can't interleave.
|
||||
|
|
@ -183,6 +186,13 @@ export function createSheetCollabManager(opts: SheetManagerOptions): SheetCollab
|
|||
provider,
|
||||
room: collabRoomId(scopeId, projectId, sheetPath),
|
||||
});
|
||||
if (destroyed) {
|
||||
// The manager died while this connect was in flight (unmount during
|
||||
// connectAll's warm fan-out) — the session must not outlive it.
|
||||
session.provider.destroy();
|
||||
session.doc.destroy();
|
||||
throw new Error(`sheet manager destroyed while connecting ${sheetPath}`);
|
||||
}
|
||||
// Invisible observer (read-only-viewer): drop the provider's initial
|
||||
// empty awareness state before anyone can see it.
|
||||
if (opts.readOnly) session.provider.awareness?.setLocalState(null);
|
||||
|
|
@ -231,6 +241,7 @@ export function createSheetCollabManager(opts: SheetManagerOptions): SheetCollab
|
|||
// 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
|
||||
// what is now the wrong (new) active screen. Its provider/doc stay warm.
|
||||
const hadActive = activePath !== null;
|
||||
if (activePath) {
|
||||
const old = rooms.get(activePath);
|
||||
if (old?.binding) {
|
||||
|
|
@ -240,8 +251,16 @@ export function createSheetCollabManager(opts: SheetManagerOptions): SheetCollab
|
|||
}
|
||||
}
|
||||
activePath = null;
|
||||
// Clear the HOST's per-sheet callbacks BEFORE the connect window too
|
||||
// (findings C-4): presence push/release, comment pins, follow-user fit and
|
||||
// drift checks were still subscribed to the OLD room while the new sheet
|
||||
// connected and seeded — each could drive the editor (now showing the new
|
||||
// sheet) from old-room state. onActiveChange(null) is the same host
|
||||
// contract destroy() already uses; the host rebinds after the seed below.
|
||||
if (hadActive) opts.onActiveChange?.(null);
|
||||
|
||||
const room = await ensureRoom(sheetPath);
|
||||
if (destroyed) return;
|
||||
|
||||
// Activating: stop tracking parked updates and bind the (warm) doc to the editor.
|
||||
room.detachWatch?.();
|
||||
|
|
@ -282,7 +301,7 @@ export function createSheetCollabManager(opts: SheetManagerOptions): SheetCollab
|
|||
clearTimeout(retryTimer);
|
||||
retryTimer = undefined;
|
||||
}
|
||||
queue = queue
|
||||
const attempt = 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
|
||||
|
|
@ -292,11 +311,18 @@ export function createSheetCollabManager(opts: SheetManagerOptions): SheetCollab
|
|||
retryDelayMs = 2000; // bound succeeded — reset the backoff
|
||||
});
|
||||
})
|
||||
.catch((err) => {
|
||||
.catch((err: unknown) => {
|
||||
cwarn(`[sheet] switchTo(${sheetPath}) failed`, err);
|
||||
// Terminal version skew (findings C-5): retrying cannot make an
|
||||
// incompatible doc encoding bindable. No retry timer — rethrow so the
|
||||
// caller surfaces it (boot converts it into the user-visible version
|
||||
// error instead of reporting "connected" with no binding).
|
||||
if ((err as { name?: string } | null)?.name === "SexprVersionError") {
|
||||
throw 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) {
|
||||
if (!destroyed && requestedPath === sheetPath && activePath !== sheetPath) {
|
||||
retryTimer = setTimeout(() => {
|
||||
retryTimer = undefined;
|
||||
if (requestedPath === sheetPath && activePath !== sheetPath) {
|
||||
|
|
@ -307,7 +333,10 @@ export function createSheetCollabManager(opts: SheetManagerOptions): SheetCollab
|
|||
retryDelayMs = Math.min(retryDelayMs * 2, 30000);
|
||||
}
|
||||
});
|
||||
return queue;
|
||||
// The terminal rejection must reach the CALLER, but must not poison the
|
||||
// serialization queue for later explicit navigation.
|
||||
queue = attempt.catch(() => {});
|
||||
return attempt;
|
||||
}
|
||||
|
||||
async function onboard(sheetPath: string): Promise<void> {
|
||||
|
|
@ -353,6 +382,7 @@ export function createSheetCollabManager(opts: SheetManagerOptions): SheetCollab
|
|||
}
|
||||
|
||||
function destroy(): void {
|
||||
destroyed = true;
|
||||
if (retryTimer) {
|
||||
clearTimeout(retryTimer);
|
||||
retryTimer = undefined;
|
||||
|
|
|
|||
|
|
@ -182,6 +182,50 @@ describe("startSiblingRestage", () => {
|
|||
expect(connectKicadDoc).toHaveBeenCalledTimes(2);
|
||||
});
|
||||
|
||||
// C-6: a failed first dial must not latch forever — retry on backoff
|
||||
// while a peer still has the sheet open.
|
||||
it("retries a failed connect on backoff while the peer stays announced", async () => {
|
||||
connectKicadDoc
|
||||
.mockRejectedValueOnce(new Error("room down"))
|
||||
.mockImplementation(({ room }: { room: string }) => {
|
||||
const s = makeSession(room);
|
||||
sessions.push(s);
|
||||
return Promise.resolve(s);
|
||||
});
|
||||
const presence = fakePresence(["main.kicad_sch"]);
|
||||
await start(["main.kicad_sch"], presence);
|
||||
await vi.advanceTimersByTimeAsync(0);
|
||||
expect(connectKicadDoc).toHaveBeenCalledTimes(1);
|
||||
|
||||
// Roster churn alone must NOT re-dial (anti-flood invariant kept).
|
||||
presence.announce(["main.kicad_sch"]);
|
||||
presence.announce(["main.kicad_sch"]);
|
||||
expect(connectKicadDoc).toHaveBeenCalledTimes(1);
|
||||
|
||||
// The 1s backoff timer re-dials and succeeds.
|
||||
await vi.advanceTimersByTimeAsync(1_000);
|
||||
expect(connectKicadDoc).toHaveBeenCalledTimes(2);
|
||||
expect(sessions).toHaveLength(1);
|
||||
});
|
||||
|
||||
it("stops retrying once the peer leaves and the linger elapses", async () => {
|
||||
connectKicadDoc.mockRejectedValue(new Error("room down"));
|
||||
const presence = fakePresence(["main.kicad_sch"]);
|
||||
await start(["main.kicad_sch"], presence);
|
||||
await vi.advanceTimersByTimeAsync(0);
|
||||
expect(connectKicadDoc).toHaveBeenCalledTimes(1);
|
||||
|
||||
// Backoff doubles: 1s → 2s while the peer stays.
|
||||
await vi.advanceTimersByTimeAsync(1_000);
|
||||
expect(connectKicadDoc).toHaveBeenCalledTimes(2);
|
||||
await vi.advanceTimersByTimeAsync(2_000);
|
||||
expect(connectKicadDoc).toHaveBeenCalledTimes(3);
|
||||
|
||||
presence.announce([]); // peer leaves → linger releases the watch
|
||||
await vi.advanceTimersByTimeAsync(600_000);
|
||||
expect(connectKicadDoc).toHaveBeenCalledTimes(3); // no further dials
|
||||
});
|
||||
|
||||
it("a rejoin during the linger keeps the existing session", async () => {
|
||||
const presence = fakePresence(["main.kicad_sch"]);
|
||||
await start(["main.kicad_sch"], presence);
|
||||
|
|
|
|||
|
|
@ -52,10 +52,19 @@ interface Watch {
|
|||
session?: KicadDocSession;
|
||||
connecting?: Promise<void>;
|
||||
linger?: ReturnType<typeof setTimeout>;
|
||||
/** Connect failed — don't re-dial on every roster change. */
|
||||
failed?: boolean;
|
||||
/**
|
||||
* Connect failed — retry on a doubling-backoff timer (1s→30s), never on
|
||||
* roster churn (findings C-6: a failed first dial used to latch `failed`
|
||||
* forever, so a peer editing through a transient outage left MEMFS stale
|
||||
* for the whole session).
|
||||
*/
|
||||
retry?: ReturnType<typeof setTimeout>;
|
||||
retryDelayMs?: number;
|
||||
}
|
||||
|
||||
const RETRY_BASE_MS = 1000;
|
||||
const RETRY_MAX_MS = 30_000;
|
||||
|
||||
export async function startSiblingRestage(opts: {
|
||||
win: ToolWindow;
|
||||
slug: string;
|
||||
|
|
@ -174,11 +183,45 @@ export async function startSiblingRestage(opts: {
|
|||
if (flushPending && w.session) restageFromDoc(sheetPath, w.session.doc);
|
||||
}
|
||||
if (w.linger) clearTimeout(w.linger);
|
||||
if (w.retry) clearTimeout(w.retry);
|
||||
w.session?.provider.destroy();
|
||||
w.session?.doc.destroy();
|
||||
watches.delete(sheetPath);
|
||||
};
|
||||
|
||||
/** Is this sheet still announced by some peer right now? */
|
||||
const wanted = (sheetPath: string): boolean =>
|
||||
presence.peers().some((p) => p.state.sheetPath === sheetPath);
|
||||
|
||||
const dial = (sheetPath: string, watch: Watch): void => {
|
||||
watch.connecting = (async () => {
|
||||
try {
|
||||
const session = await openSession(sheetPath);
|
||||
if (session) {
|
||||
watch.session = session;
|
||||
watch.retryDelayMs = undefined; // success resets the backoff
|
||||
}
|
||||
} catch (err) {
|
||||
log(`[sibling] room connect failed for ${sheetPath}: ${String(err)}`);
|
||||
const delay = watch.retryDelayMs ?? RETRY_BASE_MS;
|
||||
watch.retryDelayMs = Math.min(delay * 2, RETRY_MAX_MS);
|
||||
if (!destroyed) {
|
||||
watch.retry = setTimeout(() => {
|
||||
watch.retry = undefined;
|
||||
if (destroyed || watch.session || watch.connecting) return;
|
||||
// Re-dial only while this exact watch is live and a peer still
|
||||
// has the sheet open — a released/lingering watch stays quiet.
|
||||
if (watches.get(sheetPath) === watch && wanted(sheetPath)) {
|
||||
dial(sheetPath, watch);
|
||||
}
|
||||
}, delay);
|
||||
}
|
||||
} finally {
|
||||
watch.connecting = undefined;
|
||||
}
|
||||
})();
|
||||
};
|
||||
|
||||
const reconcile = () => {
|
||||
if (destroyed) return;
|
||||
const open = new Set<string>();
|
||||
|
|
@ -187,7 +230,7 @@ export async function startSiblingRestage(opts: {
|
|||
if (sp && sheetSet.has(sp)) open.add(sp);
|
||||
}
|
||||
for (const sheetPath of open) {
|
||||
let w = watches.get(sheetPath);
|
||||
const w = watches.get(sheetPath);
|
||||
if (w?.linger) {
|
||||
clearTimeout(w.linger);
|
||||
w.linger = undefined;
|
||||
|
|
@ -195,17 +238,7 @@ export async function startSiblingRestage(opts: {
|
|||
if (!w) {
|
||||
const watch: Watch = {};
|
||||
watches.set(sheetPath, watch);
|
||||
watch.connecting = (async () => {
|
||||
try {
|
||||
const session = await openSession(sheetPath);
|
||||
if (session) watch.session = session;
|
||||
} catch (err) {
|
||||
watch.failed = true;
|
||||
log(`[sibling] room connect failed for ${sheetPath}: ${String(err)}`);
|
||||
} finally {
|
||||
watch.connecting = undefined;
|
||||
}
|
||||
})();
|
||||
dial(sheetPath, watch);
|
||||
}
|
||||
}
|
||||
for (const [sheetPath, w] of watches) {
|
||||
|
|
|
|||
Loading…
Reference in a new issue