feat(ai,mcp): идентичность прогона реиндекса + брендированные PageId/SlugId (#495 item 14)
Аспект A — идентичность прогона реиндекса эмбеддингов. У статуса/поллинга реиндекса не было идентичности КОНКРЕТНОГО прогона, из-за чего класс багов «это тот же прогон или новый?» чинили дважды. Теперь каждый прогон получает свой runId (crypto.randomUUID в start()), он хранится в Redis- хэше рядом с total/done/startedAt и возвращается в ReindexProgress. Эндпоинт статуса (getMasked -> MaskedAiSettings) отдаёт runId и reindexStartedAt. Клиент кеит поллинг на (runId, startedAt): смена runId = НОВЫЙ прогон (сбрасываем залатанное per-run состояние поллинга), тот же runId = тот же прогон. Всё best-effort/косметика как и остальной код прогресса: пустой/отсутствующий runId деградирует мягко и никогда не ломает реиндекс. Аспект B — брендированные PageId/SlugId в MCP-клиенте + валидация формата в серверных DTO (семейство инцидентов #435: двойная идентичность страницы — внутренний id и slugId — гонялась как голая строка и молча путалась). - packages/mcp/src/lib/page-id.ts: номинальные типы PageId/SlugId + валидирующие конструкторы asPageId/asSlugId и гварды isPageId/isSlugId (формат UUID и 10-символьного slugId). PageId протянут через единственный узел канонизации и записи: resolvePageId() теперь возвращает PageId, а ключ per-page лока (withPageLock) и точки записи в collab (mutatePageContent/replacePageContent/ updatePageContentRealtime) требуют бренд — сырой slugId/непроверенный id больше не проходит проверку типов (инвариант #260 «resolve-then-lock» теперь на уровне компилятора). Публичный вход методов остаётся строкой (это legitimно UUID ИЛИ slugId), брендируется каноническое значение. - apps/server core/page/dto: PageIdDto.pageId получает валидацию формата (@Matches, UUID или 10-символьный slugId), так что кривая/подменённая идентичность отклоняется на границе, а не падает в repo голой строкой. Тесты (все зелёные, с mutation-verify каждого): - server: getMasked отдаёт runId/reindexStartedAt; стор пишет/читает runId и мягко деградирует его до ''; DTO отклоняет кривой pageId/slugId и принимает валидный. - client: чистый хелпер reindexRunKey/isNewReindexRun (кеинг поллинга на runId). - mcp: конструкторы/гварды PageId/SlugId (валидация формата). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,49 @@
|
||||
import 'reflect-metadata';
|
||||
import { plainToInstance } from 'class-transformer';
|
||||
import { validate } from 'class-validator';
|
||||
import { PageIdDto } from './page.dto';
|
||||
|
||||
// #435: PageIdDto.pageId carries a page's DOUBLE identity (internal UUID OR
|
||||
// public 10-char slugId), both as bare strings. The DTO must accept exactly
|
||||
// those two FORMATS and reject a malformed / swapped identity at the boundary.
|
||||
async function pageIdErrors(pageId: unknown) {
|
||||
const dto = plainToInstance(PageIdDto, { pageId });
|
||||
const errors = await validate(dto as object);
|
||||
return errors.some((e) => e.property === 'pageId');
|
||||
}
|
||||
|
||||
const UUID = '019f499a-9f8c-7d68-b7be-ce100d7c6c56';
|
||||
const SLUG = 'aB3xQ7kR2p';
|
||||
|
||||
describe('PageIdDto pageId format validation', () => {
|
||||
it('accepts a canonical page UUID', async () => {
|
||||
expect(await pageIdErrors(UUID)).toBe(false);
|
||||
});
|
||||
|
||||
it('accepts a 10-char slugId', async () => {
|
||||
expect(await pageIdErrors(SLUG)).toBe(false);
|
||||
});
|
||||
|
||||
it('rejects a truncated / wrong-length slug', async () => {
|
||||
expect(await pageIdErrors('aB3xQ7kR2')).toBe(true); // 9 chars
|
||||
expect(await pageIdErrors('aB3xQ7kR2pX')).toBe(true); // 11 chars
|
||||
});
|
||||
|
||||
it('rejects a slug with an illegal character', async () => {
|
||||
expect(await pageIdErrors('aB3xQ7kR2!')).toBe(true);
|
||||
});
|
||||
|
||||
it('rejects a full URL / path-shaped identity (not the bare id)', async () => {
|
||||
expect(await pageIdErrors(`my-page-title-${SLUG}`)).toBe(true);
|
||||
expect(await pageIdErrors(`https://x/p/${SLUG}`)).toBe(true);
|
||||
});
|
||||
|
||||
it('rejects a malformed UUID', async () => {
|
||||
expect(await pageIdErrors('019f499a-9f8c-7d68-b7be')).toBe(true);
|
||||
expect(await pageIdErrors('not-a-uuid-at-all-really')).toBe(true);
|
||||
});
|
||||
|
||||
it('rejects an empty string', async () => {
|
||||
expect(await pageIdErrors('')).toBe(true);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,39 @@
|
||||
import { applyDecorators } from '@nestjs/common';
|
||||
import { Matches, ValidationOptions } from 'class-validator';
|
||||
|
||||
/**
|
||||
* A page identity at the API boundary is EITHER the internal page UUID or the
|
||||
* public 10-char slugId (page.repo.findById matches a non-UUID input as a
|
||||
* slugId). Both arrive as bare strings, which is exactly how the two got swapped
|
||||
* silently (incident family #435). This regex pins the two accepted FORMATS so a
|
||||
* malformed / cross-wired identity (a truncated slug, a full URL, an email, an
|
||||
* id from another entity kind that isn't even shaped like either) is rejected at
|
||||
* the boundary instead of falling through to a confusing 404.
|
||||
*
|
||||
* - UUID: canonical 8-4-4-4-12 hex (version-agnostic — page ids are UUIDv7,
|
||||
* so only the shape/length is enforced, matching the MCP's UUID_RE
|
||||
* and the server's isValidUUID acceptance).
|
||||
* - slugId: exactly 10 chars over [0-9A-Za-z] (nanoid `generateSlugId`).
|
||||
*
|
||||
* The two are disjoint (a UUID is 36 chars WITH dashes, a slugId 10 chars
|
||||
* WITHOUT), so a value can only satisfy one branch.
|
||||
*/
|
||||
export const PAGE_ID_OR_SLUG_ID_REGEX =
|
||||
/^(?:[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}|[0-9A-Za-z]{10})$/i;
|
||||
|
||||
/**
|
||||
* Validate that a DTO string field is a well-formed page identity (page UUID OR
|
||||
* 10-char slugId). Composed decorator so the same format rule is applied
|
||||
* consistently wherever a DTO accepts a `pageId` that may be either form.
|
||||
*/
|
||||
export function IsPageIdOrSlugId(
|
||||
validationOptions?: ValidationOptions,
|
||||
): PropertyDecorator {
|
||||
return applyDecorators(
|
||||
Matches(PAGE_ID_OR_SLUG_ID_REGEX, {
|
||||
message:
|
||||
'pageId must be a page UUID or a 10-character slugId',
|
||||
...validationOptions,
|
||||
}),
|
||||
);
|
||||
}
|
||||
@@ -9,10 +9,16 @@ import {
|
||||
import { Transform } from 'class-transformer';
|
||||
|
||||
import { ContentFormat } from './create-page.dto';
|
||||
import { IsPageIdOrSlugId } from './page-identity.validator';
|
||||
|
||||
export class PageIdDto {
|
||||
@IsString()
|
||||
@IsNotEmpty()
|
||||
// Format-validate the double identity (#435): accept only a page UUID or a
|
||||
// 10-char slugId so a malformed / swapped identity is rejected at the boundary
|
||||
// rather than passed to the repo as a bare string. Base for PageInfoDto,
|
||||
// DeletePageDto, BacklinksListDto, AddLabelsDto/RemoveLabelDto, etc.
|
||||
@IsPageIdOrSlugId()
|
||||
pageId: string;
|
||||
}
|
||||
|
||||
|
||||
@@ -99,10 +99,12 @@ describe('AiSettingsService.getMasked reindex progress', () => {
|
||||
// actually pins the progress.total branch rather than coincidentally
|
||||
// matching the DB fallback. With fix #1 the two sources agree in practice,
|
||||
// but getMasked must still return progress.total when a record is active.
|
||||
const startedAt = Date.now();
|
||||
reindexProgress.get.mockResolvedValue({
|
||||
total: 500,
|
||||
done: 120,
|
||||
startedAt: Date.now(),
|
||||
startedAt,
|
||||
runId: 'run-abc',
|
||||
});
|
||||
|
||||
const masked = await service.getMasked(WORKSPACE_ID);
|
||||
@@ -110,6 +112,10 @@ describe('AiSettingsService.getMasked reindex progress', () => {
|
||||
expect(masked.indexedPages).toBe(120); // progress.done, not DB 478
|
||||
expect(masked.totalPages).toBe(500); // progress.total, not DB 478
|
||||
expect(masked.reindexing).toBe(true);
|
||||
// The status payload must carry the run identity so the client can key its
|
||||
// poll on it (a changed runId => a NEW run).
|
||||
expect(masked.runId).toBe('run-abc');
|
||||
expect(masked.reindexStartedAt).toBe(startedAt);
|
||||
});
|
||||
|
||||
it('falls back to countIndexedPages when no reindex is active', async () => {
|
||||
@@ -121,6 +127,10 @@ describe('AiSettingsService.getMasked reindex progress', () => {
|
||||
expect(masked.indexedPages).toBe(478);
|
||||
expect(masked.totalPages).toBe(478);
|
||||
expect(masked.reindexing).toBe(false);
|
||||
// No active run -> no run identity surfaced (the client keeps its prior
|
||||
// steady-state behaviour).
|
||||
expect(masked.runId).toBeUndefined();
|
||||
expect(masked.reindexStartedAt).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -371,6 +371,12 @@ export class AiSettingsService {
|
||||
totalPages,
|
||||
// Optional hint for the client: a reindex run is currently in progress.
|
||||
reindexing: progress != null,
|
||||
// Per-run identity so the client can key its poll on a stable run id and
|
||||
// reset its per-run state when a NEW run starts. Present only while a run
|
||||
// is active; `runId` may be '' for a legacy/degraded record (the client
|
||||
// treats that as "no identity").
|
||||
runId: progress?.runId,
|
||||
reindexStartedAt: progress?.startedAt,
|
||||
};
|
||||
}
|
||||
|
||||
|
||||
@@ -149,4 +149,14 @@ export interface MaskedAiSettings {
|
||||
// True while a full workspace reindex is actively running (the counts above
|
||||
// then reflect the live run progress rather than the steady-state DB count).
|
||||
reindexing?: boolean;
|
||||
// Identity of the ACTIVE reindex run (present only while `reindexing`). The
|
||||
// client keys its poll on `runId`: a changed value means a NEW run (reset the
|
||||
// per-run poll state it latched), the same value means the run it is already
|
||||
// watching — removing the "same run or a fresh one?" ambiguity a stale
|
||||
// pre-reindex snapshot otherwise causes. Absent/empty degrades gracefully.
|
||||
runId?: string;
|
||||
// Epoch-ms the active run started (present only while `reindexing`). Paired
|
||||
// with `runId` so a run that restarts with the same (recycled) id is still
|
||||
// seen as new.
|
||||
reindexStartedAt?: number;
|
||||
}
|
||||
|
||||
@@ -48,19 +48,38 @@ describe('EmbeddingReindexProgressService', () => {
|
||||
}
|
||||
|
||||
describe('get', () => {
|
||||
it('maps a valid hash to a ReindexProgress object', async () => {
|
||||
it('maps a valid hash to a ReindexProgress object (incl. the run identity)', async () => {
|
||||
const { redis, hgetall } = makeRedis();
|
||||
hgetall.mockResolvedValue({ total: '478', done: '120', startedAt: '1000' });
|
||||
hgetall.mockResolvedValue({
|
||||
total: '478',
|
||||
done: '120',
|
||||
startedAt: '1000',
|
||||
runId: 'run-xyz',
|
||||
});
|
||||
const service = makeService(redis);
|
||||
|
||||
await expect(service.get(WORKSPACE_ID)).resolves.toEqual({
|
||||
total: 478,
|
||||
done: 120,
|
||||
startedAt: 1000,
|
||||
runId: 'run-xyz',
|
||||
});
|
||||
expect(hgetall).toHaveBeenCalledWith(KEY);
|
||||
});
|
||||
|
||||
it('degrades a missing runId to an empty string (legacy/partial record)', async () => {
|
||||
const { redis, hgetall } = makeRedis();
|
||||
// A record written before runId existed: get() must still succeed and
|
||||
// report runId='' so the client treats it as "no identity", never breaks.
|
||||
hgetall.mockResolvedValue({ total: '10', done: '3', startedAt: '5' });
|
||||
await expect(makeService(redis).get(WORKSPACE_ID)).resolves.toEqual({
|
||||
total: 10,
|
||||
done: 3,
|
||||
startedAt: 5,
|
||||
runId: '',
|
||||
});
|
||||
});
|
||||
|
||||
it('returns null for an empty hash (no record)', async () => {
|
||||
const { redis, hgetall } = makeRedis();
|
||||
hgetall.mockResolvedValue({});
|
||||
@@ -87,11 +106,17 @@ describe('EmbeddingReindexProgressService', () => {
|
||||
|
||||
it('coerces a non-finite startedAt to 0', async () => {
|
||||
const { redis, hgetall } = makeRedis();
|
||||
hgetall.mockResolvedValue({ total: '10', done: '2', startedAt: 'nope' });
|
||||
hgetall.mockResolvedValue({
|
||||
total: '10',
|
||||
done: '2',
|
||||
startedAt: 'nope',
|
||||
runId: 'run-1',
|
||||
});
|
||||
await expect(makeService(redis).get(WORKSPACE_ID)).resolves.toEqual({
|
||||
total: 10,
|
||||
done: 2,
|
||||
startedAt: 0,
|
||||
runId: 'run-1',
|
||||
});
|
||||
});
|
||||
|
||||
@@ -115,6 +140,21 @@ describe('EmbeddingReindexProgressService', () => {
|
||||
expect(multiObj.exec).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it('mints a fresh non-empty runId into the record on each start', async () => {
|
||||
const { redis, multiObj } = makeRedis();
|
||||
const service = makeService(redis);
|
||||
await service.start(WORKSPACE_ID, 1);
|
||||
await service.start(WORKSPACE_ID, 1);
|
||||
|
||||
const firstRunId = multiObj.hset.mock.calls[0][1].runId;
|
||||
const secondRunId = multiObj.hset.mock.calls[1][1].runId;
|
||||
expect(typeof firstRunId).toBe('string');
|
||||
expect(firstRunId).not.toBe('');
|
||||
// Each run gets its OWN identity so the client can tell a re-trigger apart
|
||||
// from the run it is already watching.
|
||||
expect(secondRunId).not.toBe(firstRunId);
|
||||
});
|
||||
|
||||
it('defaults the expire TTL to the full 1h record TTL', async () => {
|
||||
const { redis, multiObj } = makeRedis();
|
||||
await makeService(redis).start(WORKSPACE_ID, 478);
|
||||
|
||||
@@ -1,17 +1,28 @@
|
||||
import { Injectable, Logger } from '@nestjs/common';
|
||||
import { RedisService } from '@nestjs-labs/nestjs-ioredis';
|
||||
import { randomUUID } from 'node:crypto';
|
||||
import type { Redis } from 'ioredis';
|
||||
|
||||
/**
|
||||
* Live progress of an in-flight workspace embeddings reindex run.
|
||||
* `total` is the number of pages the run will process, `done` how many it has
|
||||
* already processed (success OR handled failure), `startedAt` the epoch-ms the
|
||||
* record was created.
|
||||
* record was created, and `runId` a per-run identity minted at `start()`.
|
||||
*
|
||||
* `runId` gives each reindex run a stable identity so a poller can tell "same
|
||||
* run I've been watching" from "a NEW run started" WITHOUT guessing from the
|
||||
* progress counters (the ambiguity that a stale pre-reindex snapshot vs a fresh
|
||||
* run otherwise causes — the bug class fixed twice under #262). It is best-
|
||||
* effort like the rest of this record: a record written before this field
|
||||
* existed (or a Redis hiccup) yields an empty `runId`, which the client must
|
||||
* treat as "no identity available" and degrade to its prior behaviour, never
|
||||
* break.
|
||||
*/
|
||||
export interface ReindexProgress {
|
||||
total: number;
|
||||
done: number;
|
||||
startedAt: number;
|
||||
runId: string;
|
||||
}
|
||||
|
||||
/** Redis key namespace for the per-workspace reindex-progress record. */
|
||||
@@ -86,12 +97,18 @@ export class EmbeddingReindexProgressService {
|
||||
): Promise<void> {
|
||||
const key = this.key(workspaceId);
|
||||
try {
|
||||
// A fresh identity per run so the client poll can key on it: a changed
|
||||
// runId means a genuinely NEW run (reset any latched per-run poll state),
|
||||
// the same runId means the run the client is already watching. Best-effort
|
||||
// like the counters — never surfaced to the user, only used to disambiguate.
|
||||
const runId = randomUUID();
|
||||
await this.redis
|
||||
.multi()
|
||||
.hset(key, {
|
||||
total: String(total),
|
||||
done: '0',
|
||||
startedAt: String(Date.now()),
|
||||
runId,
|
||||
})
|
||||
.expire(key, ttlSeconds)
|
||||
.exec();
|
||||
@@ -150,7 +167,15 @@ export class EmbeddingReindexProgressService {
|
||||
const done = Number(data.done);
|
||||
const startedAt = Number(data.startedAt);
|
||||
if (!Number.isFinite(total) || !Number.isFinite(done)) return null;
|
||||
return { total, done, startedAt: Number.isFinite(startedAt) ? startedAt : 0 };
|
||||
// `runId` degrades gracefully: a pre-existing record (written before this
|
||||
// field) or a stripped value reads as '' — the client treats that as "no
|
||||
// identity" and keeps its prior behaviour rather than breaking the poll.
|
||||
return {
|
||||
total,
|
||||
done,
|
||||
startedAt: Number.isFinite(startedAt) ? startedAt : 0,
|
||||
runId: typeof data.runId === 'string' ? data.runId : '',
|
||||
};
|
||||
} catch (err) {
|
||||
this.logger.warn(
|
||||
`reindex-progress read failed for workspace ${workspaceId}; ` +
|
||||
|
||||
Reference in New Issue
Block a user