From f1595687d90d55ac1ba00b47da7ced030f822cc1 Mon Sep 17 00:00:00 2001 From: Keaton Hoskins Date: Sat, 17 Oct 2026 10:24:06 -0700 Subject: [PATCH] fix(T13868): concurrent first docs updates no longer wedge the second device's stream Rewinding a local txn undid its conduit_attachments INSERT while its append-only conduit_attachment_versions children still pointed at it, so the immediate FK check refused and every later cloud sync failed with a bare 'FOREIGN KEY constraint failed'. Rewind, apply and replay share one frame transaction, so the rewind now defers FKs to COMMIT; the parent is back (stream or replay) before the check. conduit_attachment_versions / _approvals declared their uid columns as refs, which translate through the target's LOCAL key (slug, id): the sealer found no row and sent NULL, so peers stored versions with no doc and no parent. Those columns already hold uids; they now travel verbatim like conduit_doc_heads. A deferred FK that still fails at COMMIT now names the child row and its parent table (E_SYNC_APPLY_FK). Co-Authored-By: Claude Opus 5.5 --- .../t13868-concurrent-first-doc-update.md | 39 +++++++++++ .../core/src/store/row-identity-registry.ts | 12 ++-- .../sync/__tests__/two-device-e2e.test.ts | 65 +++++++++++++++++++ .../sync/apply/__tests__/apply-frame.test.ts | 23 +++++++ packages/core/src/store/sync/apply/applier.ts | 4 ++ packages/core/src/store/sync/apply/frame.ts | 55 +++++++++++++++- 6 files changed, 187 insertions(+), 11 deletions(-) create mode 100644 .changeset/t13868-concurrent-first-doc-update.md diff --git a/.changeset/t13868-concurrent-first-doc-update.md b/.changeset/t13868-concurrent-first-doc-update.md new file mode 100644 index 0000000000..f918f7369d --- /dev/null +++ b/.changeset/t13868-concurrent-first-doc-update.md @@ -0,0 +1,39 @@ +--- +id: t13868-concurrent-first-doc-update +tasks: [T13868] +kind: fix +summary: concurrent first docs updates on two devices no longer wedge the second device's stream with "FOREIGN KEY constraint failed"; doc version links travel as uids +--- + +When two devices each made their first `docs update` of the same synced doc, +the second device's project stream failed with "FOREIGN KEY constraint +failed" on every later `cloud sync`. The peer's segment stayed staged and the +doc diverged. + +- **The wedge.** To apply the peer's earlier segment, the second device + rewinds its own local transaction. The rewind undid its INSERT of the doc + row (`conduit_attachments`), but skips append-only children, so its own + `conduit_attachment_versions` rows still pointed at the doc and the + immediate foreign-key check refused the delete. Rewind, apply and replay + share one transaction, so the rewind now defers foreign keys to COMMIT, + as the applier already does for a key move. The doc row is back (from the + stream or the replay) before the check runs. +- **Version links arrived as NULL.** `conduit_attachment_versions` and + `conduit_attachment_approvals` declared their uid columns + (`attachment_uid`, `parent_version_uid`, `version_uid`) as references. + References are translated through the target's LOCAL key (`slug`, `id`), + so the sealer found no row and sent NULL, and every peer stored versions + with no doc and no parent. Those columns already hold uids, so they now + travel verbatim, as `conduit_doc_heads` and the slug aliases already do. +- **A refused apply names the row.** When a deferred foreign key still fails + at COMMIT, the refusal names the child row's uid and its parent table + (`E_SYNC_APPLY_FK`) instead of a bare "FOREIGN KEY constraint failed". + +A store already wedged retries the staged segment on its next sync and +applies it. Version and approval rows that peers already stored with a NULL +link are not rewritten by this change. The device that wrote them now hashes +its own rows with the link included, so the next migration pass on that +device marks the two tables for repair. The repair then journals each row once +with its real link, and that fills in the NULLs on peers. This is a one-time +update burst on those rows. Their append-only rules are declared but not +enforced, so peers apply it. diff --git a/packages/core/src/store/row-identity-registry.ts b/packages/core/src/store/row-identity-registry.ts index 0ec9b7b6c9..a725e722fa 100644 --- a/packages/core/src/store/row-identity-registry.ts +++ b/packages/core/src/store/row-identity-registry.ts @@ -693,28 +693,24 @@ export const ROW_IDENTITY: Readonly { expect(refs(b.db)).toEqual(refs(a.db)); }); + for (const firstPull of ['b', 'a'] as const) { + it(`concurrent first docs updates on both devices converge with no FK failure, ${firstPull} pulling first (T13868)`, async () => { + const storageKey = `${SHA.slice(0, 2)}/${SHA.slice(2)}.md`; + const blobJson = JSON.stringify({ + kind: 'blob', + sha256: SHA, + storageKey, + mime: 'text/markdown', + size: BODY.length, + }); + const { a, b } = await twoDevices(); + const stream = fakeStream(); + for (const r of [a, b]) uniqueDocIndexes(r.db); + const at = new Date(++clockMs).toISOString(); + write( + a.db, + `INSERT INTO docs_attachments (id, sha256, attachment_json, created_at, ref_count, slug, type) + VALUES ('doc-1', '${SHA}', '${blobJson}', '${at}', 0, 'gate-crdt', 'note');`, + ); + await push(a, stream); + await pull(b, stream); + await pull(a, stream); + // `docs add` left the bytes on A; a vault restore or bytes pull puts them on B. + for (const name of ['a', 'b']) { + const p = join(dir, name, '.cleo', 'attachments', 'sha256', storageKey); + mkdirSync(join(p, '..'), { recursive: true }); + writeFileSync(p, BODY); + } + // Each device makes its FIRST text update of the doc before seeing the other's. + for (const [r, name] of [ + [a, 'a'], + [b, 'b'], + ] as const) { + vi.stubEnv('CLEO_DIR', join(dir, name, '.cleo')); // the update's native handle is the ambient project + const out = await updateDocBySlug(join(dir, name), { + slug: 'gate-crdt', + content: `${BODY}edit from ${name}\n`, + attachedBy: `agent-${name}`, + }); + expect(out.ok, `${name}: ${JSON.stringify(out)}`).toBe(true); + await push(r, stream); + } + vi.stubEnv('CLEO_DIR', undefined); + const order = firstPull === 'b' ? [b, a] : [a, b]; + for (const r of order) await pull(r, stream); + for (const r of order) await pull(r, stream); + for (const r of [a, b]) { + expect(inboxCounts(r.db, STREAM), r.device).not.toHaveProperty('staged'); + expect(r.db.prepare('PRAGMA foreign_key_check').all(), r.device).toEqual([]); + } + const docState = (r: Replica) => ({ + doc: r.db.prepare('SELECT uid, slug FROM conduit_attachments ORDER BY uid').all(), + versions: r.db + .prepare('SELECT uid, attachment_uid FROM conduit_attachment_versions ORDER BY uid') + .all(), + head: r.db + .prepare('SELECT attachment_uid, head_version_uid FROM conduit_doc_heads ORDER BY uid') + .all(), + }); + expect(docState(a).doc).toHaveLength(1); + expect(docState(b)).toEqual(docState(a)); + }, 120_000); + } + /** * A fake Nexus for doc bytes (T13721): presign/complete and the stream * attachment routes (E32/E33), blobs in memory. `routes: false` is a server diff --git a/packages/core/src/store/sync/apply/__tests__/apply-frame.test.ts b/packages/core/src/store/sync/apply/__tests__/apply-frame.test.ts index 3b130f7a85..1c804b9196 100644 --- a/packages/core/src/store/sync/apply/__tests__/apply-frame.test.ts +++ b/packages/core/src/store/sync/apply/__tests__/apply-frame.test.ts @@ -146,6 +146,29 @@ describe('withApplyFrame (§3.2)', () => { ); }); + it('a deferred FK the COMMIT refuses names the row and its parent, and rolls back (T13868)', async () => { + const db = await store(); + let thrown: unknown; + try { + withApplyFrame(db, 'project', null, () => { + db.exec('PRAGMA defer_foreign_keys = ON'); + db.exec( + "INSERT INTO conduit_doc_slug_aliases (slug, doc_uid, uid) VALUES ('old-slug', 'no-such-doc', 'alias-uid-1')", + ); + }); + } catch (err) { + thrown = err; + } + expect(thrown).toMatchObject({ + code: 'E_SYNC_APPLY_FK', + message: expect.stringContaining( + 'conduit_doc_slug_aliases alias-uid-1 → conduit_attachments', + ), + }); + expect(n(db, 'SELECT count(*) AS n FROM conduit_doc_slug_aliases')).toBe(0); + expect(db.isTransaction).toBe(false); + }); + it('runApplyFrame awaits the preparation, then runs the synchronous body on the handle queue', async () => { const db = await store(); const out = await runApplyFrame( diff --git a/packages/core/src/store/sync/apply/applier.ts b/packages/core/src/store/sync/apply/applier.ts index c22d5f09d9..cd90c73cdb 100644 --- a/packages/core/src/store/sync/apply/applier.ts +++ b/packages/core/src/store/sync/apply/applier.ts @@ -1469,6 +1469,10 @@ function describeConflicts(cs: readonly MergeConflict[]): string { function rewindTxns(c: OpContext, plan: RebasePlan): Rewound { const kept = new Map(); const after = new Map(); + // T13868: rewind skips append-only children, so undoing a parent's INSERT + // leaves them pointing at it until the stream or the replay puts it back. + // Rewind, apply and replay share this frame's transaction: check FKs at COMMIT. + c.db.exec('PRAGMA defer_foreign_keys = ON'); withTriggersSuspended(c.db, ['capture', 'guard', 'side-effect'], 'rewind', () => { for (const l of [...plan.rewind].reverse()) { adoptNaturalRows(c.db, c.api, c.defs, l.ops); diff --git a/packages/core/src/store/sync/apply/frame.ts b/packages/core/src/store/sync/apply/frame.ts index 46cde6efd7..9c7351539c 100644 --- a/packages/core/src/store/sync/apply/frame.ts +++ b/packages/core/src/store/sync/apply/frame.ts @@ -34,10 +34,18 @@ import { finishCaptureFrame, openCaptureFrame } from '../capture.js'; import { receiveClock } from '../clock-store.js'; import { type ApplyWriteApi, createApplyWriteApi } from './write-api.js'; -/** A frame misuse: an async body, a nested call, or a call after the frame ended. */ +/** + * A frame misuse (an async body, a nested call, a call after the frame + * ended), or a deferred foreign key the frame's COMMIT refused, with the rows + * named (`E_SYNC_APPLY_FK`, T13868). + */ export class ApplyFrameError extends Error { constructor( - readonly code: 'E_SYNC_APPLY_ASYNC' | 'E_SYNC_APPLY_NESTED' | 'E_SYNC_APPLY_ENDED', + readonly code: + | 'E_SYNC_APPLY_ASYNC' + | 'E_SYNC_APPLY_NESTED' + | 'E_SYNC_APPLY_ENDED' + | 'E_SYNC_APPLY_FK', message: string, ) { super(message); @@ -45,6 +53,43 @@ export class ApplyFrameError extends Error { } } +/** + * The rows a failed COMMIT's deferred foreign keys point away from, named for + * the refusal (T13868): a bare "FOREIGN KEY constraint failed" on every sync + * says nothing about which row to repair. Null when `err` is no FK failure or + * nothing is pending (an immediate check already undid its statement). + */ +function namedFkFailure(db: DatabaseSync, err: unknown): ApplyFrameError | null { + if (!(err instanceof Error) || !err.message.includes('FOREIGN KEY constraint failed')) { + return null; + } + const rows = db.prepare('PRAGMA main.foreign_key_check').all() as Array<{ + table: string; + rowid: number | null; + parent: string; + }>; + if (rows.length === 0) return null; + const uidOf = (table: string, rowid: number | null): string | null => { + try { + const row = db + .prepare(`SELECT uid FROM main."${table.replaceAll('"', '""')}" WHERE rowid = ?`) + .get(rowid) as { uid: string | null } | undefined; + return row?.uid ?? null; + } catch { + return null; // a table without a uid column: the rowid names the row + } + }; + const named = rows + .slice(0, 5) + .map((r) => `${r.table} ${uidOf(r.table, r.rowid) ?? `rowid ${r.rowid}`} → ${r.parent}`); + const more = rows.length > 5 ? ` (+${rows.length - 5} more)` : ''; + // @sync-invariant none:local-only the frame rolls back whole; this only names the rows the deferred FK check refused + return new ApplyFrameError( + 'E_SYNC_APPLY_FK', + `FOREIGN KEY constraint failed: ${named.join('; ')}${more}`, + ); +} + /** A synchronous result: never a thenable. */ export type SyncResult = T extends PromiseLike ? never : T; @@ -132,7 +177,11 @@ export function withApplyFrame( db.exec('COMMIT'); return out; } catch (err) { - if (db.isTransaction) db.exec('ROLLBACK'); + if (db.isTransaction) { + const named = namedFkFailure(db, err); + db.exec('ROLLBACK'); + if (named) throw named; + } throw err; } finally { active = false;