From 1bb011512bb396426a3fd107beab35748e79f960 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Mon, 24 Aug 2026 23:55:22 -0700 Subject: [PATCH 1/6] test(pg-mem): pin which FK actions the Tier 0 database actually performs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review generalized the #1161 finding to "every FK action asserted at the unit tier is unverified, ON DELETE CASCADE too." Measured it rather than adopting it, and the axis is narrower and stranger: self-reference, not the action. cross-table CASCADE / SET NULL fire, matching Postgres self-ref CASCADE / SET NULL ignored SET DEFAULT sets NULL, not the column default insert / NO ACTION violations rejected So #1109's `deleting the root CASCADEs the state away` is real coverage — thread_user_state.thread_root_id is cross-table. But messages.reply_to_message_id and messages.thread_root_id point back at messages(id), so a Tier 0 test that deletes a message and asserts what became of its descendants asserts nothing. Adds the rule where someone writing a constraint test will hit it, and a suite asserting the DEPENDENCY's behaviour so a pg-mem bump that fixes this goes red instead of leaving the doc quietly wrong. Co-Authored-By: Claude Opus 5 --- backend/TESTING.md | 18 +++ .../unit/models/pgMemFkActionFidelity.test.js | 135 ++++++++++++++++++ 2 files changed, 153 insertions(+) create mode 100644 backend/__tests__/unit/models/pgMemFkActionFidelity.test.js diff --git a/backend/TESTING.md b/backend/TESTING.md index c27529f51..bd520f970 100644 --- a/backend/TESTING.md +++ b/backend/TESTING.md @@ -80,6 +80,24 @@ The branch is controlled by `process.env.INTEGRATION_TEST === 'true'`. `__tests_ - **Tier 0 tests don't cross-import `mongoServer` / `pgDb`.** The real-services branch doesn't export them. Use the helpers; if you need direct access, add a narrow helper in `testUtils.js` that works in both tiers. - **Real PG needs `pgcrypto` for `gen_random_uuid()`.** `setupPgDb` creates the extension for Tier 1 — don't call `gen_random_uuid()` in a test that only runs under Tier 0 unless you're also registering the pg-mem function. +- **pg-mem ignores SELF-REFERENTIAL FK actions, and accepts the DDL anyway.** The axis is self-reference, not the action. Measured on pg-mem 2.9.1 and pinned in `__tests__/unit/models/pgMemFkActionFidelity.test.js`: + + | constraint | pg-mem | + |---|---| + | cross-table `ON DELETE CASCADE` | fires — matches Postgres | + | cross-table `ON DELETE SET NULL` | fires — matches Postgres | + | self-referential `ON DELETE CASCADE` | **ignored** | + | self-referential `ON DELETE SET NULL` | **ignored** | + | `ON DELETE SET DEFAULT` | sets **NULL**, not the column default | + | insert violating the FK | rejected | + | delete violating the default `NO ACTION` | rejected | + + `messages` has both shapes. `messages.pod_id → pods(id)` and `thread_user_state.thread_root_id → messages(id)` are cross-table and genuinely covered at Tier 0. `messages.reply_to_message_id` and `messages.thread_root_id` point back at `messages(id)`, so **a Tier 0 test that deletes a message and asserts what became of its descendants is asserting nothing** — it passes because the rows never changed, which is indistinguishable from passing because the action did the right thing. One such test was written, passed, and was deleted rather than kept (`__tests__/unit/models/retentionReRoot.test.js` records it). + + The general rule: **pg-mem proves SQL *shape*; only Tier 1 proves the database's *behaviour*.** A constraint whose effect you are asserting belongs in `__tests__/service/`. Split it the way `retentionReRoot.test.js` (repair, given a constructed orphaned state) and `__tests__/service/threading.retention.test.js` (that Postgres produces that state at all) do — a claim about our code at Tier 0, a claim about the database at Tier 1. + + `pgMemFkActionFidelity.test.js` asserts the *dependency's* current behaviour on purpose. If a pg-mem bump starts honouring these, that suite goes red — the signal to delete it and this rule, not to relax the assertion. + - **FK ordering matters under real PG.** `pod_members.pod_id` and `messages.pod_id` reference `pods(id) ON DELETE CASCADE`. Tests that insert raw rows must insert into `pods` first. `clearPgDb()` uses `TRUNCATE … CASCADE` to sidestep this on teardown. - **Timeouts.** Real Mongo operations are slower than in-memory. `jest.setTimeout(30000)` is set globally in `__tests__/setup.js`; avoid hardcoded shorter timeouts in Tier 1 tests. - **New test file, which tier?** Put it under `__tests__/service/` if it exercises real query semantics (Mongo index behavior, regex, ObjectId coercion, PG ILIKE, transactions). Put it under `__tests__/unit/` or similar if a mocked DB is sufficient. diff --git a/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js b/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js new file mode 100644 index 000000000..d6a2db8df --- /dev/null +++ b/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js @@ -0,0 +1,135 @@ +/** + * What pg-mem does and does not do with foreign-key actions. + * + * This suite asserts the behaviour of a DEPENDENCY, not of our code. It exists + * because that behaviour is load-bearing for every constraint test at Tier 0, + * and because a doc claim about a third-party library decays silently on the + * next upgrade. If pg-mem is bumped and starts honouring self-referential FK + * actions, these tests fail — which is the signal to delete them and the + * `backend/TESTING.md` rule they back, not to relax the assertion. + * + * The axis is SELF-REFERENCE, not the action. Measured on pg-mem 2.9.1: + * + * cross-table ON DELETE CASCADE fires (matches Postgres) + * cross-table ON DELETE SET NULL fires (matches Postgres) + * self-ref ON DELETE CASCADE IGNORED (diverges) + * self-ref ON DELETE SET NULL IGNORED (diverges) + * + * That distinction matters because `messages` has both shapes. `pod_id` and + * `thread_user_state.thread_root_id` are cross-table and genuinely covered at + * Tier 0. `reply_to_message_id` and `messages.thread_root_id` point back at + * `messages(id)`, so a Tier 0 test that deletes a message and asserts what + * happened to its descendants is asserting nothing. + * + * The DDL is accepted without complaint in every case, which is what makes + * this dangerous: declared, parsed, silently not applied. + */ + +const { newDb } = require('pg-mem'); + +// pg-mem attaches a Symbol(_id) to every row it returns, and `toEqual` +// compares symbol properties. Round-tripping through JSON drops it so the +// assertions read as plain column values — which is also how the first draft +// of this file passed as a standalone script and failed under jest. +const rows = (db, sql) => JSON.parse(JSON.stringify(db.public.many(sql))); + +const fresh = (ddl, seed) => { + const db = newDb(); + db.public.none(ddl); + db.public.none(seed); + return db; +}; + +describe('pg-mem honours cross-table FK actions', () => { + it('fires ON DELETE CASCADE', () => { + const db = fresh( + `CREATE TABLE p(id INT PRIMARY KEY); + CREATE TABLE c(id INT PRIMARY KEY, pid INT REFERENCES p(id) ON DELETE CASCADE);`, + `INSERT INTO p VALUES (1); INSERT INTO c VALUES (10, 1);`, + ); + db.public.none('DELETE FROM p WHERE id = 1;'); + expect(rows(db, 'SELECT * FROM c')).toHaveLength(0); + }); + + it('fires ON DELETE SET NULL', () => { + const db = fresh( + `CREATE TABLE p(id INT PRIMARY KEY); + CREATE TABLE c(id INT PRIMARY KEY, pid INT REFERENCES p(id) ON DELETE SET NULL);`, + `INSERT INTO p VALUES (1); INSERT INTO c VALUES (10, 1);`, + ); + db.public.none('DELETE FROM p WHERE id = 1;'); + expect(rows(db, 'SELECT * FROM c')).toEqual([{ id: 10, pid: null }]); + }); +}); + +describe('pg-mem IGNORES self-referential FK actions', () => { + // Both cases below are wrong against Postgres. They are asserted as-is so + // the divergence is pinned rather than described. + it('does not fire a self-referential ON DELETE SET NULL', () => { + const db = fresh( + `CREATE TABLE m(id INT PRIMARY KEY, + p INT REFERENCES m(id) ON DELETE SET NULL);`, + `INSERT INTO m VALUES (1, NULL), (2, 1);`, + ); + db.public.none('DELETE FROM m WHERE id = 1;'); + // Postgres would give p = null here. + expect(rows(db, 'SELECT * FROM m')).toEqual([{ id: 2, p: 1 }]); + }); + + it('does not fire a self-referential ON DELETE CASCADE', () => { + const db = fresh( + `CREATE TABLE m(id INT PRIMARY KEY, + p INT REFERENCES m(id) ON DELETE CASCADE);`, + `INSERT INTO m VALUES (1, NULL), (2, 1);`, + ); + db.public.none('DELETE FROM m WHERE id = 1;'); + // Postgres would delete row 2 with its parent. + expect(rows(db, 'SELECT * FROM m')).toEqual([{ id: 2, p: 1 }]); + }); + + it('leaves the whole chain pointing at a deleted row', () => { + // The `messages` shape exactly: two self-referential SET NULL columns. + const db = fresh( + `CREATE TABLE m(id INT PRIMARY KEY, + reply_to INT REFERENCES m(id) ON DELETE SET NULL, + root INT REFERENCES m(id) ON DELETE SET NULL);`, + `INSERT INTO m VALUES (1, NULL, NULL), (2, 1, 1), (3, 2, 1);`, + ); + db.public.none('DELETE FROM m WHERE id = 1;'); + expect(rows(db, 'SELECT * FROM m ORDER BY id')).toEqual([ + { id: 2, reply_to: 1, root: 1 }, + { id: 3, reply_to: 2, root: 1 }, + ]); + }); +}); + +describe('what pg-mem DOES enforce, so the rule is not read too broadly', () => { + it('rejects an insert that violates the constraint', () => { + const db = fresh( + `CREATE TABLE p(id INT PRIMARY KEY); + CREATE TABLE c(id INT PRIMARY KEY, pid INT REFERENCES p(id));`, + 'SELECT 1;', + ); + expect(() => db.public.none('INSERT INTO c VALUES (1, 99);')).toThrow(); + }); + + it('rejects a delete that would orphan under the default NO ACTION', () => { + const db = fresh( + `CREATE TABLE p(id INT PRIMARY KEY); + CREATE TABLE c(id INT PRIMARY KEY, pid INT REFERENCES p(id));`, + `INSERT INTO p VALUES (1); INSERT INTO c VALUES (10, 1);`, + ); + expect(() => db.public.none('DELETE FROM p WHERE id = 1;')).toThrow(); + }); + + it('gets ON DELETE SET DEFAULT wrong in a quieter way — null, not the default', () => { + const db = fresh( + `CREATE TABLE p(id INT PRIMARY KEY); + CREATE TABLE c(id INT PRIMARY KEY, pid INT DEFAULT 0 REFERENCES p(id) ON DELETE SET DEFAULT);`, + `INSERT INTO p VALUES (1); INSERT INTO c VALUES (10, 1);`, + ); + db.public.none('DELETE FROM p WHERE id = 1;'); + // Postgres would give pid = 0. + expect(rows(db, 'SELECT * FROM c')).toEqual([{ id: 10, pid: null }]); + }); +}); From c57c23502d8d09fcb3aa126b01fecea3862f0d84 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 03:12:28 -0700 Subject: [PATCH 2/6] fix(pg-mem): self-referential FK actions are plan-dependent, not ignored MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The table said "ignored" for self-referential CASCADE and SET NULL. Measured on the exact schema this suite already used, that is wrong in the direction that matters: the action IS performed, against the primary-key index and not against the row storage. Same db, same transaction, two answers for one row. DELETE FROM m WHERE id = 1; -- m(id PK, p REFERENCES m(id) ON DELETE SET NULL) SELECT * FROM m ORDER BY id -> [{id:2,p:1},{id:3,p:2}] action not applied SELECT * FROM m WHERE id = 2 -> [{id:2,p:null}] action applied Under CASCADE the same split removes row 2 from the PK index while SELECT * and count(*) still report two rows. Predicates on the FK column read stale, since that column carries no index — so inverting the assertion is not a workaround. This is worse than "ignored", which is what makes it worth correcting rather than softening. Ignoring is self-consistent: a green test is green for one knowable reason. Here the shape of the assertion query picks the answer, and both answers look like a real result. Each self-referential case now pins BOTH readings side by side, so a pg-mem bump that fixes either half goes red. 9/9 green on Node 22. Co-Authored-By: Claude Opus 5 --- backend/TESTING.md | 21 ++++- .../unit/models/pgMemFkActionFidelity.test.js | 88 ++++++++++++++----- 2 files changed, 81 insertions(+), 28 deletions(-) diff --git a/backend/TESTING.md b/backend/TESTING.md index bd520f970..ec9bad116 100644 --- a/backend/TESTING.md +++ b/backend/TESTING.md @@ -80,19 +80,32 @@ The branch is controlled by `process.env.INTEGRATION_TEST === 'true'`. `__tests_ - **Tier 0 tests don't cross-import `mongoServer` / `pgDb`.** The real-services branch doesn't export them. Use the helpers; if you need direct access, add a narrow helper in `testUtils.js` that works in both tiers. - **Real PG needs `pgcrypto` for `gen_random_uuid()`.** `setupPgDb` creates the extension for Tier 1 — don't call `gen_random_uuid()` in a test that only runs under Tier 0 unless you're also registering the pg-mem function. -- **pg-mem ignores SELF-REFERENTIAL FK actions, and accepts the DDL anyway.** The axis is self-reference, not the action. Measured on pg-mem 2.9.1 and pinned in `__tests__/unit/models/pgMemFkActionFidelity.test.js`: +- **pg-mem applies SELF-REFERENTIAL FK actions to the primary-key index and not to the row storage, so the answer depends on your query plan.** The DDL is accepted in every case. Measured on pg-mem 2.9.1 and pinned in `__tests__/unit/models/pgMemFkActionFidelity.test.js`: | constraint | pg-mem | |---|---| | cross-table `ON DELETE CASCADE` | fires — matches Postgres | | cross-table `ON DELETE SET NULL` | fires — matches Postgres | - | self-referential `ON DELETE CASCADE` | **ignored** | - | self-referential `ON DELETE SET NULL` | **ignored** | + | self-referential `ON DELETE CASCADE` | **contradicts itself** — the row is gone via the PK index, still present to a scan | + | self-referential `ON DELETE SET NULL` | **contradicts itself** — the FK reads `NULL` via the PK index, unchanged to a scan | | `ON DELETE SET DEFAULT` | sets **NULL**, not the column default | | insert violating the FK | rejected | | delete violating the default `NO ACTION` | rejected | - `messages` has both shapes. `messages.pod_id → pods(id)` and `thread_user_state.thread_root_id → messages(id)` are cross-table and genuinely covered at Tier 0. `messages.reply_to_message_id` and `messages.thread_root_id` point back at `messages(id)`, so **a Tier 0 test that deletes a message and asserts what became of its descendants is asserting nothing** — it passes because the rows never changed, which is indistinguishable from passing because the action did the right thing. One such test was written, passed, and was deleted rather than kept (`__tests__/unit/models/retentionReRoot.test.js` records it). + "Contradicts itself" is literal — one table, one transaction, two answers for the same row. Given `m(id INT PRIMARY KEY, p INT REFERENCES m(id) ON DELETE SET NULL)` seeded `(1,NULL),(2,1),(3,2)` and `DELETE FROM m WHERE id = 1`: + + ``` + SELECT * FROM m ORDER BY id → [{id:2,p:1},{id:3,p:2}] -- action NOT applied + SELECT * FROM m WHERE id = 2 → [{id:2,p:null}] -- action applied + SELECT * FROM m WHERE p = 1 → [{id:2,p:1}] -- matches the stale value + SELECT * FROM m WHERE p IS NULL → [] -- and not the applied one + ``` + + Under `CASCADE` the same split deletes row 2 from the PK index while `SELECT *` and `count(*)` still report two rows. A plan served by the PK index sees the action; a plan that scans sees the pre-delete value. `p` carries no index, so predicates on the FK column always read stale. + + **This is worse than the "pg-mem ignores them" reading it replaces** (which is what an earlier version of this rule and the first draft of that suite both said). Ignoring is at least consistent: every read agrees, and a test that passes does so for one knowable reason. Here the *shape of the assertion query* picks the answer — `expect(rows(db, 'SELECT * FROM m'))` and `expect(rows(db, 'SELECT * FROM m WHERE id = 2'))` disagree about whether the constraint fired, and both look like a real result. A green Tier 0 constraint test is therefore not evidence even of pg-mem's own behaviour, let alone Postgres's. + + `messages` has both shapes. `messages.pod_id → pods(id)` and `thread_user_state.thread_root_id → messages(id)` are cross-table and genuinely covered at Tier 0. `messages.reply_to_message_id` and `messages.thread_root_id` point back at `messages(id)`, so **a Tier 0 test that deletes a message and asserts what became of its descendants is asserting nothing** — it reports whichever answer its own `SELECT` happened to reach. One such test was written, passed, and was deleted rather than kept (`__tests__/unit/models/retentionReRoot.test.js` records it). The general rule: **pg-mem proves SQL *shape*; only Tier 1 proves the database's *behaviour*.** A constraint whose effect you are asserting belongs in `__tests__/service/`. Split it the way `retentionReRoot.test.js` (repair, given a constructed orphaned state) and `__tests__/service/threading.retention.test.js` (that Postgres produces that state at all) do — a claim about our code at Tier 0, a claim about the database at Tier 1. diff --git a/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js b/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js index d6a2db8df..262099bd9 100644 --- a/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js +++ b/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js @@ -10,19 +10,32 @@ * * The axis is SELF-REFERENCE, not the action. Measured on pg-mem 2.9.1: * - * cross-table ON DELETE CASCADE fires (matches Postgres) - * cross-table ON DELETE SET NULL fires (matches Postgres) - * self-ref ON DELETE CASCADE IGNORED (diverges) - * self-ref ON DELETE SET NULL IGNORED (diverges) + * cross-table ON DELETE CASCADE fires (matches Postgres) + * cross-table ON DELETE SET NULL fires (matches Postgres) + * self-ref ON DELETE CASCADE INCONSISTENT (diverges) + * self-ref ON DELETE SET NULL INCONSISTENT (diverges) + * + * "Inconsistent" and not "ignored": the action IS performed, against the + * PRIMARY KEY INDEX and not against the row storage. One table, one + * transaction, two answers for the same row — a plan served by the PK index + * reports the action as applied, a plan that scans reports the pre-delete + * value. The `plan-dependent` describe below pins both readings side by side. + * + * An earlier draft of this file asserted only the scanning reads and called + * the action ignored. That is the more comfortable failure and the wrong one: + * ignoring is at least self-consistent, so a green test is green for one + * knowable reason. Here the SHAPE OF THE ASSERTION QUERY picks the answer, and + * both answers look like a real result. * * That distinction matters because `messages` has both shapes. `pod_id` and * `thread_user_state.thread_root_id` are cross-table and genuinely covered at * Tier 0. `reply_to_message_id` and `messages.thread_root_id` point back at * `messages(id)`, so a Tier 0 test that deletes a message and asserts what - * happened to its descendants is asserting nothing. + * happened to its descendants is not asserting nothing — it is asserting + * whichever answer its own SELECT happened to reach. * * The DDL is accepted without complaint in every case, which is what makes - * this dangerous: declared, parsed, silently not applied. + * this dangerous: declared, parsed, applied to half the storage. */ const { newDb } = require('pg-mem'); @@ -62,32 +75,55 @@ describe('pg-mem honours cross-table FK actions', () => { }); }); -describe('pg-mem IGNORES self-referential FK actions', () => { - // Both cases below are wrong against Postgres. They are asserted as-is so - // the divergence is pinned rather than described. - it('does not fire a self-referential ON DELETE SET NULL', () => { - const db = fresh( +describe('pg-mem applies self-referential FK actions plan-dependently', () => { + // Every assertion below is wrong against Postgres in at least one of its two + // readings. They are asserted as-is so the divergence is pinned rather than + // described. The pairs are the point: same db, same transaction, same row. + const selfRef = (action) => + fresh( `CREATE TABLE m(id INT PRIMARY KEY, - p INT REFERENCES m(id) ON DELETE SET NULL);`, - `INSERT INTO m VALUES (1, NULL), (2, 1);`, + p INT REFERENCES m(id) ON DELETE ${action});`, + `INSERT INTO m VALUES (1, NULL), (2, 1), (3, 2);`, ); + + it('reports SET NULL as both applied and not applied, depending on the read', () => { + const db = selfRef('SET NULL'); db.public.none('DELETE FROM m WHERE id = 1;'); - // Postgres would give p = null here. - expect(rows(db, 'SELECT * FROM m')).toEqual([{ id: 2, p: 1 }]); + + // Scanning read: the action did NOT happen. Postgres would give p = null. + expect(rows(db, 'SELECT * FROM m ORDER BY id')).toEqual([ + { id: 2, p: 1 }, + { id: 3, p: 2 }, + ]); + // PK-index read of the SAME row: the action DID happen. + expect(rows(db, 'SELECT * FROM m WHERE id = 2')).toEqual([{ id: 2, p: null }]); }); - it('does not fire a self-referential ON DELETE CASCADE', () => { - const db = fresh( - `CREATE TABLE m(id INT PRIMARY KEY, - p INT REFERENCES m(id) ON DELETE CASCADE);`, - `INSERT INTO m VALUES (1, NULL), (2, 1);`, - ); + it('reports CASCADE as both applied and not applied, depending on the read', () => { + const db = selfRef('CASCADE'); + db.public.none('DELETE FROM m WHERE id = 1;'); + + // Scanning read: row 2 survives. Postgres would have deleted it. + expect(rows(db, 'SELECT * FROM m ORDER BY id')).toEqual([ + { id: 2, p: 1 }, + { id: 3, p: 2 }, + ]); + // count(*) agrees with the scan, so an aggregate is no safer. + expect(rows(db, 'SELECT count(*) AS c FROM m')).toEqual([{ c: 2 }]); + // PK-index read: row 2 is gone. + expect(rows(db, 'SELECT * FROM m WHERE id = 2')).toEqual([]); + }); + + it('answers predicates on the FK column from the stale value', () => { + // `p` carries no index, so both of these are scans and both read stale — + // which is why "just assert the other way round" is not the workaround. + const db = selfRef('SET NULL'); db.public.none('DELETE FROM m WHERE id = 1;'); - // Postgres would delete row 2 with its parent. - expect(rows(db, 'SELECT * FROM m')).toEqual([{ id: 2, p: 1 }]); + expect(rows(db, 'SELECT * FROM m WHERE p = 1')).toEqual([{ id: 2, p: 1 }]); + expect(rows(db, 'SELECT * FROM m WHERE p IS NULL')).toEqual([]); }); - it('leaves the whole chain pointing at a deleted row', () => { + it('leaves the whole chain pointing at a deleted row, to a scan', () => { // The `messages` shape exactly: two self-referential SET NULL columns. const db = fresh( `CREATE TABLE m(id INT PRIMARY KEY, @@ -100,6 +136,10 @@ describe('pg-mem IGNORES self-referential FK actions', () => { { id: 2, reply_to: 1, root: 1 }, { id: 3, reply_to: 2, root: 1 }, ]); + // And to a PK-index read, does not — on both columns at once. + expect(rows(db, 'SELECT * FROM m WHERE id = 2')).toEqual([ + { id: 2, reply_to: null, root: null }, + ]); }); }); From 2e2cd95fcbc8a029ffe340bbdf1e9389a7f72b54 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 03:15:07 -0700 Subject: [PATCH 3/6] =?UTF-8?q?test(pg-mem):=20isolate=20the=20variable=20?= =?UTF-8?q?with=20`id=20+=200`=20=E2=80=94=20sprint-review's=20control?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pairs already in this suite vary the projection AND the predicate at once, so they establish the answer is plan-dependent without establishing which part of the plan decides it. `WHERE id + 0 = 10` against `WHERE id = 10` is the identical predicate over identical rows with the PK index made ineligible, nothing else changed: WHERE id = 10 (pk index) -> parent_id = null WHERE id + 0 = 10 (no index) -> parent_id = 1 Reproduced before adopting. It also shows the line is index ELIGIBILITY and not equality — `>=` and `IN` are index-served too and agree with the first reading, which the previous cases could not distinguish. Control is @sprint-review's, credited in the file. 10/10 green on Node 22. Co-Authored-By: Claude Opus 5 --- backend/TESTING.md | 9 ++++++ .../unit/models/pgMemFkActionFidelity.test.js | 31 +++++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/backend/TESTING.md b/backend/TESTING.md index ec9bad116..b6d340232 100644 --- a/backend/TESTING.md +++ b/backend/TESTING.md @@ -103,6 +103,15 @@ The branch is controlled by `process.env.INTEGRATION_TEST === 'true'`. `__tests_ Under `CASCADE` the same split deletes row 2 from the PK index while `SELECT *` and `count(*)` still report two rows. A plan served by the PK index sees the action; a plan that scans sees the pre-delete value. `p` carries no index, so predicates on the FK column always read stale. + The variable is isolated by disabling the index in place — same predicate, same rows, nothing else changed: + + ``` + WHERE id = 10 (pk index) -> parent_id = null + WHERE id + 0 = 10 (no index) -> parent_id = 1 + ``` + + The line is index *eligibility*, not equality: `>=` and `IN` are index-served too and agree with the first reading. + **This is worse than the "pg-mem ignores them" reading it replaces** (which is what an earlier version of this rule and the first draft of that suite both said). Ignoring is at least consistent: every read agrees, and a test that passes does so for one knowable reason. Here the *shape of the assertion query* picks the answer — `expect(rows(db, 'SELECT * FROM m'))` and `expect(rows(db, 'SELECT * FROM m WHERE id = 2'))` disagree about whether the constraint fired, and both look like a real result. A green Tier 0 constraint test is therefore not evidence even of pg-mem's own behaviour, let alone Postgres's. `messages` has both shapes. `messages.pod_id → pods(id)` and `thread_user_state.thread_root_id → messages(id)` are cross-table and genuinely covered at Tier 0. `messages.reply_to_message_id` and `messages.thread_root_id` point back at `messages(id)`, so **a Tier 0 test that deletes a message and asserts what became of its descendants is asserting nothing** — it reports whichever answer its own `SELECT` happened to reach. One such test was written, passed, and was deleted rather than kept (`__tests__/unit/models/retentionReRoot.test.js` records it). diff --git a/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js b/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js index 262099bd9..46428f58f 100644 --- a/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js +++ b/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js @@ -21,6 +21,11 @@ * reports the action as applied, a plan that scans reports the pre-delete * value. The `plan-dependent` describe below pins both readings side by side. * + * The variable is isolated by `WHERE id + 0 = 10` vs `WHERE id = 10`: same + * predicate, same rows, index made ineligible, opposite answers. That control + * is @sprint-review's, and it is what rules out the projection or the shape of + * the WHERE clause as the thing that decides. + * * An earlier draft of this file asserted only the scanning reads and called * the action ignored. That is the more comfortable failure and the wrong one: * ignoring is at least self-consistent, so a green test is green for one @@ -114,6 +119,32 @@ describe('pg-mem applies self-referential FK actions plan-dependently', () => { expect(rows(db, 'SELECT * FROM m WHERE id = 2')).toEqual([]); }); + it('flips on `+ 0` — the same predicate with the index disabled', () => { + // The control that isolates the variable, from @sprint-review. The pairs + // above vary the projection AND the predicate together, so they show the + // answer is plan-dependent without showing WHICH part of the plan decides. + // `id + 0 = 10` is the identical predicate over the identical rows with + // the PK index made ineligible, and nothing else changed. + const db = fresh( + `CREATE TABLE m(id INT PRIMARY KEY, + parent_id INT REFERENCES m(id) ON DELETE SET NULL);`, + `INSERT INTO m VALUES (1, NULL), (10, 1);`, + ); + db.public.none('DELETE FROM m WHERE id = 1;'); + + expect(rows(db, 'SELECT * FROM m WHERE id = 10')).toEqual([ + { id: 10, parent_id: null }, // index-served: SET NULL happened + ]); + expect(rows(db, 'SELECT * FROM m WHERE id + 0 = 10')).toEqual([ + { id: 10, parent_id: 1 }, // scan: it never did + ]); + + // The line is index-ELIGIBILITY, not equality: range and IN predicates are + // served by the index too, and agree with it. + expect(rows(db, 'SELECT * FROM m WHERE id >= 10')).toEqual([{ id: 10, parent_id: null }]); + expect(rows(db, 'SELECT * FROM m WHERE id IN (10)')).toEqual([{ id: 10, parent_id: null }]); + }); + it('answers predicates on the FK column from the stale value', () => { // `p` carries no index, so both of these are scans and both read stale — // which is why "just assert the other way round" is not the workaround. From 841a2f55e6f13a74e986a8e569442023aae2f330 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 04:59:27 -0700 Subject: [PATCH 4/6] test(pg-mem): the FK column reads stale because it is unindexed, not because it is the FK MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review: the "predicates on the FK column always read stale" claim does not survive. Reproduced on pg-mem 2.9.1 — CREATE INDEX m_p_idx ON m(p) is the only difference, and both predicates flip: no index WHERE p = 1 -> [{id:2,p:1}] WHERE p IS NULL -> [] with index WHERE p = 1 -> [] WHERE p IS NULL -> [{id:2,p:null}] The "always" also contradicted the doc's own next line, which already said the boundary is index ELIGIBILITY rather than the column's role. Pins the A/B as a test, and renames the neighbouring case to name its own scope (UNINDEXED FK column) so it stops asserting the general claim. Sharper consequence now stated in the doc: adding an index to a pg-mem schema is a behaviour change, not a performance change — it can silently fix or break a test that never mentions the index. 11/11 green on Node 22. Co-Authored-By: Claude Opus 5 --- backend/TESTING.md | 9 ++++++- .../unit/models/pgMemFkActionFidelity.test.js | 24 ++++++++++++++++++- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/backend/TESTING.md b/backend/TESTING.md index b6d340232..ae454825a 100644 --- a/backend/TESTING.md +++ b/backend/TESTING.md @@ -101,7 +101,14 @@ The branch is controlled by `process.env.INTEGRATION_TEST === 'true'`. `__tests_ SELECT * FROM m WHERE p IS NULL → [] -- and not the applied one ``` - Under `CASCADE` the same split deletes row 2 from the PK index while `SELECT *` and `count(*)` still report two rows. A plan served by the PK index sees the action; a plan that scans sees the pre-delete value. `p` carries no index, so predicates on the FK column always read stale. + Under `CASCADE` the same split deletes row 2 from the PK index while `SELECT *` and `count(*)` still report two rows. A plan served by the PK index sees the action; a plan that scans sees the pre-delete value. In these examples `p` carries no index, so predicates on the FK column read stale — but that is a property of the index, not of the column's role in the FK (@sprint-review). Add `CREATE INDEX m_p_idx ON m(p)` and the same two predicates go fresh: + + ``` + no index WHERE p = 1 -> [{id:2, p:1}] WHERE p IS NULL -> [] + with index WHERE p = 1 -> [] WHERE p IS NULL -> [{id:2, p:null}] + ``` + + So the FK column is not condemned to staleness; it is simply the column nobody indexes. Which makes the whole divergence sharper and more dangerous than "self-referential FKs are broken": **adding an index changes query results**, so a schema tuning change can silently fix or break a test that never mentioned the index. The variable is isolated by disabling the index in place — same predicate, same rows, nothing else changed: diff --git a/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js b/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js index 46428f58f..66fd5f607 100644 --- a/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js +++ b/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js @@ -104,6 +104,25 @@ describe('pg-mem applies self-referential FK actions plan-dependently', () => { expect(rows(db, 'SELECT * FROM m WHERE id = 2')).toEqual([{ id: 2, p: null }]); }); + // @sprint-review's correction: an earlier draft of the doc said predicates on + // the FK column "always read stale". They don't. The FK column is simply the + // column nobody indexes — index it and the same predicates go fresh. That is + // the more alarming version of this bug, because it means adding an index is + // a behaviour change, not just a performance change. + it('an index on the FK column flips the SAME predicate to fresh', () => { + const stale = selfRef('SET NULL'); + stale.public.none('DELETE FROM m WHERE id = 1;'); + expect(rows(stale, 'SELECT * FROM m WHERE p = 1')).toEqual([{ id: 2, p: 1 }]); + expect(rows(stale, 'SELECT * FROM m WHERE p IS NULL')).toEqual([]); + + const indexed = selfRef('SET NULL'); + indexed.public.none('CREATE INDEX m_p_idx ON m(p);'); + indexed.public.none('DELETE FROM m WHERE id = 1;'); + // Same DDL, same seed, same predicate. Only the index differs. + expect(rows(indexed, 'SELECT * FROM m WHERE p = 1')).toEqual([]); + expect(rows(indexed, 'SELECT * FROM m WHERE p IS NULL')).toEqual([{ id: 2, p: null }]); + }); + it('reports CASCADE as both applied and not applied, depending on the read', () => { const db = selfRef('CASCADE'); db.public.none('DELETE FROM m WHERE id = 1;'); @@ -145,9 +164,12 @@ describe('pg-mem applies self-referential FK actions plan-dependently', () => { expect(rows(db, 'SELECT * FROM m WHERE id IN (10)')).toEqual([{ id: 10, parent_id: null }]); }); - it('answers predicates on the FK column from the stale value', () => { + it('answers predicates on an UNINDEXED FK column from the stale value', () => { // `p` carries no index, so both of these are scans and both read stale — // which is why "just assert the other way round" is not the workaround. + // The name says UNINDEXED deliberately: the test above shows an index on + // the same column flips both answers, so a name like "predicates on the FK + // column" would assert more than this case establishes. const db = selfRef('SET NULL'); db.public.none('DELETE FROM m WHERE id = 1;'); expect(rows(db, 'SELECT * FROM m WHERE p = 1')).toEqual([{ id: 2, p: 1 }]); From b7cd649f509aa147b2eef1711d9f0cebbeea8905 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 05:01:08 -0700 Subject: [PATCH 5/6] =?UTF-8?q?test(pg-mem):=20pin=20the=20CASCADE-plus-in?= =?UTF-8?q?dex=20extreme=20=E2=80=94=20invisible=20to=20every=20index,=20p?= =?UTF-8?q?resent=20to=20every=20scan?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review's extension. Under a self-referential CASCADE with an index on the FK column, WHERE id = 2, WHERE p = 1 and WHERE p IS NULL all return [] while SELECT * returns the row and count(*) reports it. Without the index, WHERE p = 1 is the one predicate that still finds it — so adding an index is a verdict change on a test that never mentions the index, and the constraint never touched the heap in either run. One correction to the reported figures: the count is 2, not 1. Row 3 survives the scan as well, so the stranded thing is the whole un-cascaded chain rather than a single orphan row. 12/12 green on Node 22. Co-Authored-By: Claude Opus 5 --- backend/TESTING.md | 4 ++- .../unit/models/pgMemFkActionFidelity.test.js | 30 +++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/backend/TESTING.md b/backend/TESTING.md index ae454825a..6411614a7 100644 --- a/backend/TESTING.md +++ b/backend/TESTING.md @@ -108,7 +108,9 @@ The branch is controlled by `process.env.INTEGRATION_TEST === 'true'`. `__tests_ with index WHERE p = 1 -> [] WHERE p IS NULL -> [{id:2, p:null}] ``` - So the FK column is not condemned to staleness; it is simply the column nobody indexes. Which makes the whole divergence sharper and more dangerous than "self-referential FKs are broken": **adding an index changes query results**, so a schema tuning change can silently fix or break a test that never mentioned the index. + So the FK column is not condemned to staleness; it is simply the column nobody indexes. Which makes the whole divergence sharper and more dangerous than "self-referential FKs are broken": **adding an index changes query results**, so a schema tuning change can silently fix or break a test that never mentioned the index. Indexing is a workaround at the *read* level while the constraint itself did nothing either way — so a stale-passing test can start reading fresh because of a commit about performance. + + CASCADE plus an FK index is the extreme (@sprint-review): after the same delete, `WHERE id = 2`, `WHERE p = 1` and `WHERE p IS NULL` all return `[]`, while `SELECT *` returns the row and `count(*)` reports it. The row is invisible to every indexed path and present on every scan at the same instant. The count there is **2**, not 1 — row 3 survives the scan too, so what is stranded is the whole un-cascaded chain rather than one orphan. The variable is isolated by disabling the index in place — same predicate, same rows, nothing else changed: diff --git a/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js b/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js index 66fd5f607..be222340b 100644 --- a/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js +++ b/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js @@ -123,6 +123,36 @@ describe('pg-mem applies self-referential FK actions plan-dependently', () => { expect(rows(indexed, 'SELECT * FROM m WHERE p IS NULL')).toEqual([{ id: 2, p: null }]); }); + // @sprint-review's extreme case. Under CASCADE, indexing the FK column moves + // the LAST remaining predicate across the line: the row is then invisible to + // every indexed path and present on every scan. pg-mem never touched the + // heap in either run — only which plans can see the change differs. + it('CASCADE plus an FK index leaves a row invisible to every indexed path and present to every scan', () => { + const db = selfRef('CASCADE'); + db.public.none('CREATE INDEX m_p_idx ON m(p);'); + db.public.none('DELETE FROM m WHERE id = 1;'); + + // Every indexed path: gone. + expect(rows(db, 'SELECT * FROM m WHERE id = 2')).toEqual([]); + expect(rows(db, 'SELECT * FROM m WHERE p = 1')).toEqual([]); + expect(rows(db, 'SELECT * FROM m WHERE p IS NULL')).toEqual([]); + + // Every scanning path: still there. Note the count is 2, not 1 — row 3 + // survives the scan as well, so this is not one orphan row but the whole + // un-cascaded chain. + expect(rows(db, 'SELECT * FROM m ORDER BY id')).toEqual([ + { id: 2, p: 1 }, + { id: 3, p: 2 }, + ]); + expect(rows(db, 'SELECT count(*)::int AS n FROM m')).toEqual([{ n: 2 }]); + + // Without the index, `WHERE p = 1` is the one predicate that still finds + // it — which is what makes "add an index" a verdict change. + const noIdx = selfRef('CASCADE'); + noIdx.public.none('DELETE FROM m WHERE id = 1;'); + expect(rows(noIdx, 'SELECT * FROM m WHERE p = 1')).toEqual([{ id: 2, p: 1 }]); + }); + it('reports CASCADE as both applied and not applied, depending on the read', () => { const db = selfRef('CASCADE'); db.public.none('DELETE FROM m WHERE id = 1;'); From c0001aabc8acf5ad79c293b7b7fb7914b50034e3 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 05:02:43 -0700 Subject: [PATCH 6/6] =?UTF-8?q?docs(pg-mem):=20retract=20the=20count=20"co?= =?UTF-8?q?rrection"=20=E2=80=94=20it=20was=20a=20fixture=20difference?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit My previous commit said @sprint-review's count(*) of 1 was wrong and the answer was 2. It was not wrong. Their guess about the cause was exactly right: the number tracks the fixture's chain length, not the bug. (1,NULL),(2,1) -> count 1 (1,NULL),(2,1),(3,2) -> count 2 (row 3 stranded as well) Both measured on pg-mem 2.9.1, CASCADE + index on the FK column. Nothing cascades in either case; only how much is left behind differs. The doc and the test comment now quote the number with its fixture instead of asserting a constant, which is what turned a shared observation into a contradiction. 12/12 green on Node 22. Co-Authored-By: Claude Opus 5 --- backend/TESTING.md | 2 +- .../__tests__/unit/models/pgMemFkActionFidelity.test.js | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/backend/TESTING.md b/backend/TESTING.md index 6411614a7..efa08f14b 100644 --- a/backend/TESTING.md +++ b/backend/TESTING.md @@ -110,7 +110,7 @@ The branch is controlled by `process.env.INTEGRATION_TEST === 'true'`. `__tests_ So the FK column is not condemned to staleness; it is simply the column nobody indexes. Which makes the whole divergence sharper and more dangerous than "self-referential FKs are broken": **adding an index changes query results**, so a schema tuning change can silently fix or break a test that never mentioned the index. Indexing is a workaround at the *read* level while the constraint itself did nothing either way — so a stale-passing test can start reading fresh because of a commit about performance. - CASCADE plus an FK index is the extreme (@sprint-review): after the same delete, `WHERE id = 2`, `WHERE p = 1` and `WHERE p IS NULL` all return `[]`, while `SELECT *` returns the row and `count(*)` reports it. The row is invisible to every indexed path and present on every scan at the same instant. The count there is **2**, not 1 — row 3 survives the scan too, so what is stranded is the whole un-cascaded chain rather than one orphan. + CASCADE plus an FK index is the extreme (@sprint-review): after the same delete, `WHERE id = 2`, `WHERE p = 1` and `WHERE p IS NULL` all return `[]`, while `SELECT *` returns the row and `count(*)` reports it. The row is invisible to every indexed path and present on every scan at the same instant. The scan count there tracks chain length, so quote it with the fixture: a two-row `(1,NULL),(2,1)` gives 1, and the three-row chain `(1,NULL),(2,1),(3,2)` gives 2, because row 3 is stranded as well. Nothing cascades in either case — what varies is only how much is left behind. The variable is isolated by disabling the index in place — same predicate, same rows, nothing else changed: diff --git a/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js b/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js index be222340b..b3b9c8afc 100644 --- a/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js +++ b/backend/__tests__/unit/models/pgMemFkActionFidelity.test.js @@ -137,9 +137,10 @@ describe('pg-mem applies self-referential FK actions plan-dependently', () => { expect(rows(db, 'SELECT * FROM m WHERE p = 1')).toEqual([]); expect(rows(db, 'SELECT * FROM m WHERE p IS NULL')).toEqual([]); - // Every scanning path: still there. Note the count is 2, not 1 — row 3 - // survives the scan as well, so this is not one orphan row but the whole - // un-cascaded chain. + // Every scanning path: still there. The count tracks the fixture's chain + // length rather than being a constant: this three-row chain leaves 2, + // because row 3 is stranded too; a two-row (1,NULL),(2,1) fixture leaves + // 1. Quote the number with its fixture or it reads as a disagreement. expect(rows(db, 'SELECT * FROM m ORDER BY id')).toEqual([ { id: 2, p: 1 }, { id: 3, p: 2 },