d7fa6738e5
F4 [critical] — the anti-join `DELETE … WHERE NOT EXISTS(child)` was still racy under Postgres READ COMMITTED: a reply INSERT holds FOR KEY SHARE on the parent; the DELETE's start snapshot doesn't see the uncommitted child (NOT EXISTS true), blocks on the reply's lock, and when the reply commits the parent was only LOCKED (not modified) so EvalPlanQual does NOT re-check → the DELETE proceeds and CASCADE destroys the just-committed reply. Replaced with a transaction: SELECT the parent FOR UPDATE (conflicts with the reply's FOR KEY SHARE → serializes the concurrent reply), re-check for a child with a FRESH statement in the same tx (a new RC snapshot sees a just-committed reply), delete only if still childless (return 1) else return 0 (caller resolves). The FOR UPDATE lock is held to end-of-tx so no reply can insert between the re-check and the delete. Signature unchanged, so the service + its mocked unit tests are untouched; docstrings updated. F5 [warning] — the client Dismiss button was gated only on canComment, but the server now gates dismiss on owner-or-space-admin, so a non-owner non-admin saw a button the server 403s. `canShowDismiss` now also requires `isOwnerOrAdmin = currentUser?.user?.id === comment.creatorId || userSpaceRole === "admin"` (the same gate the comment delete-menu already uses); threaded into both call sites. F6 [warning] — added a REAL-DB int-spec (apps/server/test/integration/comment-delete-if-childless.int-spec.ts, + a createComment seeder): (a) childless → returns 1, row gone; (b) committed reply → returns 0, parent+reply survive; (c) CONCURRENCY — a second connection inserts a reply (FOR KEY SHARE) and commits mid-operation while deleteCommentIfChildless blocks on FOR UPDATE → asserts it returns 0 and both rows survive (a blind anti-join would lose the reply here). Ran against live Postgres — 3/3 pass. server tsc clean; comment jest 53 + int-spec 3 (live Postgres) pass. client tsc clean; comment vitest 56 pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
161 lines
5.1 KiB
TypeScript
161 lines
5.1 KiB
TypeScript
import { Kysely } from 'kysely';
|
|
import { CommentRepo } from '../../src/database/repos/comment/comment.repo';
|
|
import {
|
|
getTestDb,
|
|
destroyTestDb,
|
|
buildTestDb,
|
|
createWorkspace,
|
|
createSpace,
|
|
createPage,
|
|
createUser,
|
|
createComment,
|
|
} from './db';
|
|
|
|
/**
|
|
* Real-DB coverage for CommentRepo.deleteCommentIfChildless (#338 F4/F6).
|
|
*
|
|
* This is the guard that keeps an ephemeral-suggestion hard-delete from
|
|
* cascade-destroying a reply (`comments.parent_comment_id` is ON DELETE CASCADE).
|
|
* The unit tests MOCK this method to 0/1, so only an int-spec actually exercises
|
|
* the SQL — the FOR UPDATE lock-then-recheck transaction — against Postgres.
|
|
*
|
|
* The concurrency case is the whole point: a plain anti-join
|
|
* `DELETE … WHERE NOT EXISTS(child)` passes (a) and (b) but SILENTLY loses a
|
|
* reply that commits mid-operation under READ COMMITTED (EvalPlanQual does not
|
|
* re-check a merely-locked row). Test (c) reproduces exactly that interleaving
|
|
* and asserts the row + reply both survive.
|
|
*/
|
|
describe('CommentRepo.deleteCommentIfChildless [integration]', () => {
|
|
let db: Kysely<any>;
|
|
let repo: CommentRepo;
|
|
let workspaceId: string;
|
|
let spaceId: string;
|
|
let pageId: string;
|
|
let userId: string;
|
|
|
|
beforeAll(async () => {
|
|
db = getTestDb();
|
|
repo = new CommentRepo(db as any);
|
|
workspaceId = (await createWorkspace(db)).id;
|
|
spaceId = (await createSpace(db, workspaceId)).id;
|
|
pageId = (await createPage(db, { workspaceId, spaceId })).id;
|
|
userId = (await createUser(db, workspaceId)).id;
|
|
});
|
|
|
|
afterAll(async () => {
|
|
await destroyTestDb();
|
|
});
|
|
|
|
async function rowExists(id: string): Promise<boolean> {
|
|
const row = await db
|
|
.selectFrom('comments')
|
|
.select('id')
|
|
.where('id', '=', id)
|
|
.executeTakeFirst();
|
|
return Boolean(row);
|
|
}
|
|
|
|
function seedTopLevel() {
|
|
return createComment(db, {
|
|
workspaceId,
|
|
spaceId,
|
|
pageId,
|
|
creatorId: userId,
|
|
selection: 'old text',
|
|
suggestedText: 'new text',
|
|
});
|
|
}
|
|
|
|
function seedReply(parentId: string) {
|
|
return createComment(db, {
|
|
workspaceId,
|
|
spaceId,
|
|
pageId,
|
|
creatorId: userId,
|
|
parentCommentId: parentId,
|
|
});
|
|
}
|
|
|
|
it('(a) childless top-level → returns 1 and the row is gone', async () => {
|
|
const parent = await seedTopLevel();
|
|
expect(await rowExists(parent.id)).toBe(true);
|
|
|
|
const deleted = await repo.deleteCommentIfChildless(parent.id);
|
|
|
|
expect(deleted).toBe(1);
|
|
expect(await rowExists(parent.id)).toBe(false);
|
|
});
|
|
|
|
it('(b) top-level WITH a committed reply → returns 0, parent AND reply survive (gate blocks the cascade)', async () => {
|
|
const parent = await seedTopLevel();
|
|
const reply = await seedReply(parent.id);
|
|
|
|
const deleted = await repo.deleteCommentIfChildless(parent.id);
|
|
|
|
expect(deleted).toBe(0);
|
|
expect(await rowExists(parent.id)).toBe(true);
|
|
expect(await rowExists(reply.id)).toBe(true);
|
|
});
|
|
|
|
it('(c) reply COMMITS mid-operation (FOR UPDATE path) → returns 0, parent + reply survive; a blind anti-join would lose the reply', async () => {
|
|
const parent = await seedTopLevel();
|
|
|
|
// Second connection holds an open transaction that inserts a reply (taking
|
|
// FOR KEY SHARE on the parent via the FK) and does NOT commit until we open
|
|
// the gate — reproducing the "reply not yet committed" window.
|
|
const conn2 = buildTestDb();
|
|
let openGate!: () => void;
|
|
const gate = new Promise<void>((resolve) => {
|
|
openGate = resolve;
|
|
});
|
|
let replyId: string | undefined;
|
|
|
|
const sleep = (ms: number) => new Promise((r) => setTimeout(r, ms));
|
|
|
|
try {
|
|
const replyTx = conn2.transaction().execute(async (trx) => {
|
|
const row = await trx
|
|
.insertInto('comments')
|
|
.values({
|
|
workspaceId,
|
|
spaceId,
|
|
pageId,
|
|
creatorId: userId,
|
|
parentCommentId: parent.id,
|
|
})
|
|
.returning(['id'])
|
|
.executeTakeFirstOrThrow();
|
|
replyId = row.id as string;
|
|
// Hold the FOR KEY SHARE lock on the parent until the gate opens.
|
|
await gate;
|
|
});
|
|
|
|
// Let the reply INSERT acquire its lock before the delete starts.
|
|
await sleep(250);
|
|
|
|
// deleteCommentIfChildless does SELECT ... FOR UPDATE on the parent, which
|
|
// conflicts with the reply's FOR KEY SHARE, so it BLOCKS here.
|
|
const deletePromise = repo.deleteCommentIfChildless(parent.id);
|
|
|
|
// Give the delete time to reach (and block on) its FOR UPDATE, then let the
|
|
// reply commit. The delete then wakes, re-checks under the lock, sees the
|
|
// now-committed reply, and returns 0.
|
|
await sleep(250);
|
|
openGate();
|
|
await replyTx;
|
|
|
|
const deleted = await deletePromise;
|
|
|
|
expect(deleted).toBe(0);
|
|
expect(await rowExists(parent.id)).toBe(true);
|
|
expect(replyId).toBeDefined();
|
|
expect(await rowExists(replyId!)).toBe(true);
|
|
} finally {
|
|
// Always release the gate (in case an assertion threw before openGate) and
|
|
// close the extra connection so global-teardown can DROP the database.
|
|
openGate();
|
|
await conn2.destroy();
|
|
}
|
|
});
|
|
});
|