From 767ac9e7e2b0d801577514290bc04c5d63815dc2 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 27 Jun 2026 03:33:33 +0300 Subject: [PATCH] fix(share): guard alias swap/rename against concurrent-delete race; share unique-violation helpers Address PR #227 re-review (comment 2193). - Stability: `updatePageId`/`updateAlias` now `executeTakeFirstOrThrow`, so a row reaped by a concurrent `removeAlias` between the read and the UPDATE (READ COMMITTED) raises `NoResultError` instead of returning `undefined`. The service maps that to a retryable `ConflictException` (`ALIAS_PAGE_RACE`) rather than a 200-without-alias (swap) or a generic 400 from `undefined.id` (rename). Tests cover both branches. - Simplification: drop the redundant secondary "unexpected unique index" warn and the now-unused `UNIQUE_ALIAS_INDEX` const (the constraint name is already logged unconditionally; both index branches still distinguish "Alias already taken" vs ALIAS_PAGE_RACE). - Architecture: extract `isUniqueViolation`/`violatedConstraint` into database/utils.ts; adopt them in the share-alias service and favorite.repo (the bare `23505` check). ai-agent-roles (#222) is on a separate unmerged branch and should adopt them after #227 merges (noted at the helpers). Helper unit test added. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../core/share/share-alias.service.spec.ts | 63 +++++++++++++++++++ .../src/core/share/share-alias.service.ts | 47 ++++++++------ .../database/repos/favorite/favorite.repo.ts | 5 +- .../share-alias/share-alias.repo.spec.ts | 10 ++- .../repos/share-alias/share-alias.repo.ts | 20 +++++- .../src/database/unique-violation.spec.ts | 51 +++++++++++++++ apps/server/src/database/utils.ts | 29 +++++++++ 7 files changed, 198 insertions(+), 27 deletions(-) create mode 100644 apps/server/src/database/unique-violation.spec.ts diff --git a/apps/server/src/core/share/share-alias.service.spec.ts b/apps/server/src/core/share/share-alias.service.spec.ts index 236fd1ec..471ccf8e 100644 --- a/apps/server/src/core/share/share-alias.service.spec.ts +++ b/apps/server/src/core/share/share-alias.service.spec.ts @@ -1,4 +1,5 @@ import { BadRequestException, ConflictException } from '@nestjs/common'; +import { NoResultError } from 'kysely'; import { ShareAliasService } from './share-alias.service'; /** @@ -355,6 +356,68 @@ describe('ShareAliasService', () => { } }); + it('maps a concurrent-delete race in the SWAP branch to a retryable 409 (not a 200-without-alias)', async () => { + const { service, shareAliasRepo } = makeService(); + // Name points at another page; reassign confirmed -> swap branch. + shareAliasRepo.findByAliasAndWorkspace.mockResolvedValue({ + id: 'a-1', + alias: 'foo', + pageId: 'p-other', + }); + // A concurrent removeAlias deleted the row between read and UPDATE, so the + // repo's executeTakeFirstOrThrow finds 0 rows and throws NoResultError. + shareAliasRepo.updatePageId.mockRejectedValue( + new NoResultError({} as any), + ); + + try { + await service.setAlias({ + workspaceId: 'ws-1', + pageId: 'p-1', + creatorId: 'u-1', + alias: 'foo', + confirmReassign: true, + }); + fail('expected ConflictException'); + } catch (err) { + // Crucially NOT a resolved 200 carrying `undefined` as the alias. + expect(err).toBeInstanceOf(ConflictException); + expect((err as ConflictException).getResponse()).toMatchObject({ + code: 'ALIAS_PAGE_RACE', + }); + } + }); + + it('maps a concurrent-delete race in the RENAME branch to a retryable 409 (not a generic 400)', async () => { + const { service, shareAliasRepo } = makeService(); + // New slug is free, but the page already owns an alias we rename in place. + shareAliasRepo.findByAliasAndWorkspace.mockResolvedValue(undefined); + shareAliasRepo.findByPageId.mockResolvedValue({ + id: 'a-1', + alias: 'te', + pageId: 'p-1', + }); + // The row vanished before the UPDATE; repo throws NoResultError rather + // than returning undefined (which would dereference undefined.id -> 400). + shareAliasRepo.updateAlias.mockRejectedValue(new NoResultError({} as any)); + + try { + await service.setAlias({ + workspaceId: 'ws-1', + pageId: 'p-1', + creatorId: 'u-1', + alias: 'ted', + }); + fail('expected ConflictException'); + } catch (err) { + expect(err).toBeInstanceOf(ConflictException); + expect(err).not.toBeInstanceOf(BadRequestException); + expect((err as ConflictException).getResponse()).toMatchObject({ + code: 'ALIAS_PAGE_RACE', + }); + } + }); + it('maps a non-unique-violation db error to BadRequest (Failed to set alias)', async () => { const { service, shareAliasRepo } = makeService(); shareAliasRepo.findByAliasAndWorkspace.mockResolvedValue(undefined); diff --git a/apps/server/src/core/share/share-alias.service.ts b/apps/server/src/core/share/share-alias.service.ts index dceae71b..8b05eabb 100644 --- a/apps/server/src/core/share/share-alias.service.ts +++ b/apps/server/src/core/share/share-alias.service.ts @@ -11,21 +11,21 @@ import { Page, ShareAlias } from '@docmost/db/types/entity.types'; import { isValidShareAlias, normalizeShareAlias } from './share-alias.util'; import { InjectKysely } from 'nestjs-kysely'; import { KyselyDB } from '@docmost/db/types/kysely.types'; -import { executeTx } from '@docmost/db/utils'; - -/** Postgres unique_violation. Two unique indexes can raise it on this table. */ -const PG_UNIQUE_VIOLATION = '23505'; +import { + executeTx, + isUniqueViolation, + violatedConstraint, +} from '@docmost/db/utils'; +import { NoResultError } from 'kysely'; /** - * Unique index names from the share_aliases migrations. The `postgres@3.x` - * driver (kysely-postgres-js) surfaces the violated constraint as - * `err.constraint_name` (NOT `.constraint`); we keep `.constraint` only as a - * defensive fallback for other drivers. - * - ALIAS: `(workspace_id, alias)` -> the vanity NAME is taken. + * Unique index name from the share_aliases migrations whose violation we map to + * a DISTINCT, non-misleading outcome: * - PAGE_ID: partial `(workspace_id, page_id) WHERE page_id IS NOT NULL` * -> a concurrent writer already gave THIS page an alias. + * The `(workspace_id, alias)` index (the vanity NAME being taken) needs no + * constant: it is the default "Alias already taken" mapping. */ -const UNIQUE_ALIAS_INDEX = 'share_aliases_workspace_id_alias_unique'; const UNIQUE_PAGE_ID_INDEX = 'share_aliases_workspace_id_page_id_unique'; export interface ResolvedAliasTarget { @@ -171,11 +171,23 @@ export class ShareAliasService { ) { throw err; } + // The row we read was deleted (concurrent `removeAlias`) before our UPDATE + // matched it, so `executeTakeFirstOrThrow` found no row. Surface a + // retryable conflict instead of a 200-without-alias (swap branch) or a + // generic 400 from dereferencing `undefined.id` (rename branch). + if (err instanceof NoResultError) { + this.logger.warn( + 'share alias update matched no row (concurrent-delete race)', + ); + throw new ConflictException({ + message: 'The address changed concurrently, please retry', + code: 'ALIAS_PAGE_RACE', + }); + } // A unique index fired. Which one decides the message — always log the // constraint so the race is diagnosable. - if (err?.code === PG_UNIQUE_VIOLATION) { - const constraint: string | undefined = - err?.constraint_name ?? err?.constraint; + if (isUniqueViolation(err)) { + const constraint = violatedConstraint(err); this.logger.warn( `share alias unique violation on ${constraint ?? ''}`, ); @@ -189,13 +201,8 @@ export class ShareAliasService { code: 'ALIAS_PAGE_RACE', }); } - // `(workspace_id, alias)` (UNIQUE_ALIAS_INDEX) or any other/unknown - // unique index: treat as the vanity name being claimed first. - if (constraint && constraint !== UNIQUE_ALIAS_INDEX) { - this.logger.warn( - `unexpected unique index ${constraint} mapped to "Alias already taken"`, - ); - } + // `(workspace_id, alias)` or any other/unknown unique index: treat as + // the vanity name being claimed first. throw new ConflictException({ message: 'Alias already taken' }); } this.logger.error(err); diff --git a/apps/server/src/database/repos/favorite/favorite.repo.ts b/apps/server/src/database/repos/favorite/favorite.repo.ts index 54b21135..8f152e60 100644 --- a/apps/server/src/database/repos/favorite/favorite.repo.ts +++ b/apps/server/src/database/repos/favorite/favorite.repo.ts @@ -7,7 +7,7 @@ import { executeWithCursorPagination } from '@docmost/db/pagination/cursor-pagin import { jsonObjectFrom } from 'kysely/helpers/postgres'; import { ExpressionBuilder, SelectQueryBuilder, sql } from 'kysely'; import { DB } from '@docmost/db/types/db'; -import { dbOrTx } from '@docmost/db/utils'; +import { dbOrTx, isUniqueViolation } from '@docmost/db/utils'; export const FavoriteType = { PAGE: 'page', @@ -29,7 +29,8 @@ export class FavoriteRepo { .returningAll() .executeTakeFirst(); } catch (err: any) { - if (err?.code === '23505') return undefined; + // Idempotent favorite: a duplicate (already-favorited) is not an error. + if (isUniqueViolation(err)) return undefined; throw err; } } diff --git a/apps/server/src/database/repos/share-alias/share-alias.repo.spec.ts b/apps/server/src/database/repos/share-alias/share-alias.repo.spec.ts index 505ba9b5..42f296a7 100644 --- a/apps/server/src/database/repos/share-alias/share-alias.repo.spec.ts +++ b/apps/server/src/database/repos/share-alias/share-alias.repo.spec.ts @@ -94,7 +94,9 @@ describe('ShareAliasRepo', () => { return builder; }), returning: jest.fn(() => builder), - executeTakeFirst: jest.fn().mockResolvedValue({ id: 'a-1' }), + // Retarget uses executeTakeFirstOrThrow so a row reaped by a concurrent + // delete (0 rows matched) raises NoResultError instead of returning undefined. + executeTakeFirstOrThrow: jest.fn().mockResolvedValue({ id: 'a-1' }), }; const db = { updateTable: jest.fn(() => builder) } as unknown as KyselyDB; const repo = new ShareAliasRepo(db); @@ -121,7 +123,11 @@ describe('ShareAliasRepo', () => { return builder; }), returning: jest.fn(() => builder), - executeTakeFirst: jest.fn().mockResolvedValue({ id: 'a-1', alias: 'ted' }), + // Rename uses executeTakeFirstOrThrow so a row reaped by a concurrent + // delete (0 rows matched) raises NoResultError instead of returning undefined. + executeTakeFirstOrThrow: jest + .fn() + .mockResolvedValue({ id: 'a-1', alias: 'ted' }), }; const db = { updateTable: jest.fn(() => builder) } as unknown as KyselyDB; const repo = new ShareAliasRepo(db); diff --git a/apps/server/src/database/repos/share-alias/share-alias.repo.ts b/apps/server/src/database/repos/share-alias/share-alias.repo.ts index 39c94a81..dedea832 100644 --- a/apps/server/src/database/repos/share-alias/share-alias.repo.ts +++ b/apps/server/src/database/repos/share-alias/share-alias.repo.ts @@ -92,6 +92,12 @@ export class ShareAliasRepo { * Rename an existing alias row in place (the vanity-slug edit, e.g. * `te` -> `ted`). Keeps the row's id/page_id/creator so the page's single * alias pointer is preserved — only the human-readable name changes. + * + * Uses `executeTakeFirstOrThrow`: if a concurrent `delete` reaps this row + * between the service's read and this UPDATE (READ COMMITTED), the UPDATE + * matches 0 rows and kysely throws `NoResultError` rather than returning + * `undefined` for a `Promise`. The service maps that to a + * retryable conflict instead of dereferencing `undefined.id`. */ async updateAlias( id: string, @@ -105,7 +111,7 @@ export class ShareAliasRepo { .where('id', '=', id) .where('workspaceId', '=', workspaceId) .returning(this.baseFields) - .executeTakeFirst(); + .executeTakeFirstOrThrow(); } /** @@ -127,7 +133,15 @@ export class ShareAliasRepo { .execute(); } - /** Retarget an existing alias to a new page (the "swap" operation). */ + /** + * Retarget an existing alias to a new page (the "swap" operation). + * + * Uses `executeTakeFirstOrThrow`: if a concurrent `delete` reaps this row + * between the service's read and this UPDATE, the UPDATE matches 0 rows and + * kysely throws `NoResultError` instead of returning `undefined` into the 200 + * response (a "success" with no alias). The service maps that to a retryable + * conflict. + */ async updatePageId( id: string, pageId: string, @@ -140,7 +154,7 @@ export class ShareAliasRepo { .where('id', '=', id) .where('workspaceId', '=', workspaceId) .returning(this.baseFields) - .executeTakeFirst(); + .executeTakeFirstOrThrow(); } async delete( diff --git a/apps/server/src/database/unique-violation.spec.ts b/apps/server/src/database/unique-violation.spec.ts new file mode 100644 index 00000000..2567942a --- /dev/null +++ b/apps/server/src/database/unique-violation.spec.ts @@ -0,0 +1,51 @@ +import { isUniqueViolation, violatedConstraint } from './utils'; + +/** + * Unit tests for the driver-bound Postgres unique-violation helpers extracted + * from the share-alias service (and now shared with favorite.repo). They encode + * two `kysely-postgres-js` / `postgres@3.x` quirks: the SQLSTATE is the string + * `'23505'`, and the violated index name arrives as `constraint_name` (with + * `constraint` only a fallback for other drivers). + */ +describe('isUniqueViolation', () => { + it('is true for a 23505 error', () => { + expect(isUniqueViolation({ code: '23505' })).toBe(true); + }); + + it('is false for any other code', () => { + expect(isUniqueViolation({ code: '08006' })).toBe(false); + }); + + it('is false when there is no code / not an object', () => { + expect(isUniqueViolation({})).toBe(false); + expect(isUniqueViolation(null)).toBe(false); + expect(isUniqueViolation(undefined)).toBe(false); + expect(isUniqueViolation(new Error('boom'))).toBe(false); + }); +}); + +describe('violatedConstraint', () => { + it('reads the postgres@3.x `constraint_name` field', () => { + expect( + violatedConstraint({ code: '23505', constraint_name: 'idx_a' }), + ).toBe('idx_a'); + }); + + it('falls back to `constraint` when `constraint_name` is absent', () => { + expect(violatedConstraint({ code: '23505', constraint: 'idx_b' })).toBe( + 'idx_b', + ); + }); + + it('prefers `constraint_name` over `constraint` when both are present', () => { + expect( + violatedConstraint({ constraint_name: 'idx_a', constraint: 'idx_b' }), + ).toBe('idx_a'); + }); + + it('is undefined when neither field is present', () => { + expect(violatedConstraint({ code: '23505' })).toBeUndefined(); + expect(violatedConstraint(null)).toBeUndefined(); + expect(violatedConstraint(undefined)).toBeUndefined(); + }); +}); diff --git a/apps/server/src/database/utils.ts b/apps/server/src/database/utils.ts index 9ed16cbb..84c3693b 100644 --- a/apps/server/src/database/utils.ts +++ b/apps/server/src/database/utils.ts @@ -33,6 +33,35 @@ export function dbOrTx( } } +/** Postgres `unique_violation` SQLSTATE — raised when a write hits a UNIQUE index. */ +const PG_UNIQUE_VIOLATION = '23505'; + +/** + * Whether `err` is a Postgres unique-violation (SQLSTATE `23505`). THE single + * check so repos/services stop re-hardcoding the magic code. + * + * NOTE (#222): `core/ai-chat/roles/ai-agent-roles.service.ts` still carries its + * own inline `23505` check on a separate, unmerged branch; it should adopt this + * helper (and {@link violatedConstraint}) after #227 lands. + */ +export function isUniqueViolation(err: unknown): boolean { + return (err as { code?: unknown } | null | undefined)?.code === PG_UNIQUE_VIOLATION; +} + +/** + * The name of the UNIQUE index/constraint a `23505` error violated, or + * undefined. The `kysely-postgres-js` / `postgres@3.x` driver surfaces it as + * `err.constraint_name` (NOT `.constraint`); `.constraint` is kept only as a + * defensive fallback for other drivers. + */ +export function violatedConstraint(err: unknown): string | undefined { + const e = err as + | { constraint_name?: string; constraint?: string } + | null + | undefined; + return e?.constraint_name ?? e?.constraint; +} + /** * Bind a JS array/object as a `jsonb` column value, working around a postgres * driver double-encoding quirk. THE single implementation — repos that persist