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) <noreply@anthropic.com>

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);

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 ?? '<unknown>'}`,
         );
@@ -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);

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;
     }
   }

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);

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<ShareAlias>`. 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(

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();
+  });
+});

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