From fd42e975b9e9261de16c5e269dd3471acde3a5a4 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 5 Jul 2026 02:52:02 +0300 Subject: [PATCH] fix(#348 review round-2 F5-F6): index page_access(workspace_id) + test the workspace-cache bust MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both are direct consequences of the round-1 F1 fix (uncaching hasRestrictedPagesInWorkspace): - F5: that EXISTS(SELECT 1 FROM page_access WHERE workspace_id=?) now runs per-request on every whole-workspace list endpoint (global search + suggest, favorites, notifications, recent, created-by), and page_access only had a space_id index → a seq scan in the common zero-restriction case. Added idx_page_access_workspace_id to the perf migration (up + down) so it's an index-only existence probe. - F6: the DomainMiddleware workspace cache invalidation was untested — the int-spec passed `{}` for cacheManager, so bustWorkspaceCache's `del` threw into its own try/catch and never ran. Added a Map-backed cache double with a working del and two tests: updateSetting busts WORKSPACE_SELF_HOSTED; updateSharingSettings busts WORKSPACE_SELF_HOSTED + WORKSPACE_BY_HOST(hostname). A missed/mismatched bust key now fails the suite instead of letting a stale security-relevant workspace row (enforceSso/status) outlive the mutation. Gate: server tsc 0; workspace-repo-update-setting + page-permission-workspace-filter int-specs pass on real Postgres (the new index applies via global-setup). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../20260705T120000-perf-indexes.ts | 12 +++ .../workspace-repo-update-setting.int-spec.ts | 77 +++++++++++++++++++ 2 files changed, 89 insertions(+) diff --git a/apps/server/src/database/migrations/20260705T120000-perf-indexes.ts b/apps/server/src/database/migrations/20260705T120000-perf-indexes.ts index 20429d2f..b39112d4 100644 --- a/apps/server/src/database/migrations/20260705T120000-perf-indexes.ts +++ b/apps/server/src/database/migrations/20260705T120000-perf-indexes.ts @@ -83,6 +83,17 @@ export async function up(db: Kysely): Promise { CREATE INDEX IF NOT EXISTS idx_comments_page_id_id ON comments (page_id, id) `.execute(db); + + // page_access(workspace_id): #348 made hasRestrictedPagesInWorkspace uncached + // (F1 fix), so `EXISTS(SELECT 1 FROM page_access WHERE workspace_id=?)` now runs + // per-request on every whole-workspace list endpoint (global search + suggest, + // favorites, notifications, recent, created-by). page_access only had a + // space_id index → that EXISTS was a seq scan in the common zero-restriction + // case. This index makes it an index-only existence probe. + await sql` + CREATE INDEX IF NOT EXISTS idx_page_access_workspace_id + ON page_access (workspace_id) + `.execute(db); } export async function down(db: Kysely): Promise { @@ -92,6 +103,7 @@ export async function down(db: Kysely): Promise { await sql`DROP INDEX IF EXISTS idx_groups_name_trgm`.execute(db); await sql`DROP INDEX IF EXISTS idx_page_history_page_id`.execute(db); await sql`DROP INDEX IF EXISTS idx_comments_page_id_id`.execute(db); + await sql`DROP INDEX IF EXISTS idx_page_access_workspace_id`.execute(db); // Restore the original two-arg (dictionary-named) f_unaccent body. await sql` diff --git a/apps/server/test/integration/workspace-repo-update-setting.int-spec.ts b/apps/server/test/integration/workspace-repo-update-setting.int-spec.ts index e78b4bb9..6b021e31 100644 --- a/apps/server/test/integration/workspace-repo-update-setting.int-spec.ts +++ b/apps/server/test/integration/workspace-repo-update-setting.int-spec.ts @@ -1,7 +1,25 @@ import { Kysely } from 'kysely'; import { WorkspaceRepo } from '@docmost/db/repos/workspace/workspace.repo'; +import { CacheKey } from 'src/common/helpers/cache-keys'; import { getTestDb, destroyTestDb, createWorkspace } from './db'; +// A minimal Map-backed cache double with a working `del` (the previous `{}` stub +// made bustWorkspaceCache's `del` throw into its own try/catch, so the #348 +// invalidation was never actually exercised — review F6). +function makeCacheDouble() { + const store = new Map(); + return { + store, + get: async (k: string) => store.get(k), + set: async (k: string, v: unknown) => { + store.set(k, v); + }, + del: async (k: string) => { + store.delete(k); + }, + }; +} + /** * A — WorkspaceRepo.updateSetting jsonb-MERGE (the html-embed kill-switch * write-half). Setting a single top-level key must NOT clobber sibling @@ -60,3 +78,62 @@ describe('WorkspaceRepo.updateSetting (jsonb merge) [integration]', () => { expect(updated.settings).toEqual({ htmlEmbed: false }); }); }); + +/** + * #348 F6 — the DomainMiddleware workspace cache (WORKSPACE_SELF_HOSTED / + * WORKSPACE_BY_HOST, 15s TTL) caches security-relevant fields (enforceSso/ + * enforceMfa/status). Its correctness rests entirely on bustWorkspaceCache being + * called from every mutator. This exercises the real invalidation with a working + * cache double (not the {} stub, whose del throws-and-swallows): warm the cache + * like DomainMiddleware, mutate, and assert the busted key is gone so a stale + * workspace row can't outlive the mutation. + */ +describe('WorkspaceRepo bustWorkspaceCache invalidation [integration]', () => { + let db: Kysely; + + beforeAll(() => { + db = getTestDb(); + }); + afterAll(async () => { + await destroyTestDb(); + }); + + it('updateSetting busts the self-hosted workspace cache key', async () => { + const cache = makeCacheDouble(); + const repo = new WorkspaceRepo(db as any, cache as any); + const ws = await createWorkspace(db, { settings: {} }); + + // Warm the cache as DomainMiddleware would (self-hosted key). + cache.store.set(CacheKey.WORKSPACE_SELF_HOSTED, ws); + expect(cache.store.has(CacheKey.WORKSPACE_SELF_HOSTED)).toBe(true); + + await repo.updateSetting(ws.id, 'htmlEmbed', true); + + // The mutation must have invalidated the cached row. + expect(cache.store.has(CacheKey.WORKSPACE_SELF_HOSTED)).toBe(false); + }); + + it('updateSharingSettings busts the by-host workspace cache key too', async () => { + const cache = makeCacheDouble(); + const repo = new WorkspaceRepo(db as any, cache as any); + const ws = await createWorkspace(db, { settings: {} }); + // createWorkspace assigns a unique hostname; read it back for the by-host key. + const { hostname } = await db + .selectFrom('workspaces') + .select(['hostname']) + .where('id', '=', ws.id) + .executeTakeFirstOrThrow(); + + // Warm BOTH keys (self-hosted + by-host); the by-host bust needs the row's + // hostname, which the mutator returns from the DB. + cache.store.set(CacheKey.WORKSPACE_SELF_HOSTED, ws); + cache.store.set(CacheKey.WORKSPACE_BY_HOST(hostname as string), ws); + + await repo.updateSharingSettings(ws.id, 'allowInvite', true); + + expect(cache.store.has(CacheKey.WORKSPACE_SELF_HOSTED)).toBe(false); + expect(cache.store.has(CacheKey.WORKSPACE_BY_HOST(hostname as string))).toBe( + false, + ); + }); +});