Merge pull request 'feat(metrics): наблюдаемость collab-цикла и MCP (#402, follow-up #355)' (#403) from feat/402-collab-mcp-metrics into develop

Reviewed-on: #403

apps/server/src/collaboration/collaboration.gateway.ts

@@ -1,4 +1,9 @@
-import { Hocuspocus } from '@hocuspocus/server';
+import {
+  connectedPayload,
+  Extension,
+  Hocuspocus,
+  onConnectPayload,
+} from '@hocuspocus/server';
 import { IncomingMessage } from 'http';
 import WebSocket from 'ws';
 import { AuthenticationExtension } from './extensions/authentication.extension';
@@ -25,6 +30,56 @@ import {
   CollaborationHandler,
   CollabEventHandlers,
 } from './collaboration.handler';
+import {
+  incDocLoad,
+  incDocUnload,
+  isMetricsEnabled,
+  observeCollabConnect,
+  registerDocsOpenSource,
+} from '../integrations/metrics/metrics.registry';
+
+/**
+ * #402 — collab lifecycle metrics as a lightweight hocuspocus extension.
+ *
+ * - afterLoadDocument / afterUnloadDocument (fire once PER DOCUMENT) drive the
+ *   doc load/unload counters.
+ * - collab_connect_duration_seconds: I time the onConnect→connected hook pair,
+ *   i.e. connection ACCEPTANCE (which includes the auth handshake). This is the
+ *   cleanest per-connection correlation hocuspocus exposes: both payloads carry
+ *   the SAME `request` IncomingMessage object, so a WeakMap keyed on it gives a
+ *   per-connection start with NO leak (the entry is GC'd with the request if a
+ *   connection is rejected in onAuthenticate and `connected` never fires).
+ *   I deliberately do NOT observe at afterLoadDocument: that hook fires per
+ *   DOCUMENT, not per connection, so a second client joining an already-open
+ *   doc would be missed. auth/load latencies are their own separate metrics.
+ *
+ * All helpers are no-ops when METRICS_PORT is unset; these hooks are per
+ * connect/load/unload (never per message), so there is no hot-path cost.
+ */
+class CollabMetricsExtension implements Extension {
+  // Keyed by the per-connection request object → connect start time (ms).
+  private readonly connectStarts = new WeakMap<object, number>();
+
+  async onConnect(data: onConnectPayload) {
+    this.connectStarts.set(data.request, performance.now());
+  }
+
+  async connected(data: connectedPayload) {
+    const start = this.connectStarts.get(data.request);
+    if (start !== undefined) {
+      observeCollabConnect((performance.now() - start) / 1000);
+      this.connectStarts.delete(data.request);
+    }
+  }
+
+  async afterLoadDocument() {
+    incDocLoad();
+  }
+
+  async afterUnloadDocument() {
+    incDocUnload();
+  }
+}
 
 @Injectable()
 export class CollaborationGateway {
@@ -58,9 +113,18 @@ export class CollaborationGateway {
         this.authenticationExtension,
         this.persistenceExtension,
         this.loggerExtension,
+        // #402 collab lifecycle + connect-duration metrics (no-op when off).
+        new CollabMetricsExtension(),
       ],
     });
 
+    // #402 — read-on-scrape source for collab_docs_open. Wire ONCE, gated, so
+    // nothing runs when metrics are disabled. The gauge's collect() pulls the
+    // live count from the hocuspocus instance on each scrape (no inc/dec drift).
+    if (isMetricsEnabled()) {
+      registerDocsOpenSource(() => this.hocuspocus.getDocumentsCount());
+    }
+
     if (this.withRedis) {
       this.redisClient = new RedisClient({
         host: this.redisConfig.host,

apps/server/src/collaboration/extensions/authentication.extension.ts

@@ -16,6 +16,7 @@ import { isUserDisabled } from '../../common/helpers';
 import { getPageId } from '../collaboration.util';
 import { JwtCollabPayload, JwtType } from '../../core/auth/dto/jwt-payload';
 import { resolveProvenance } from '../../common/decorators/auth-provenance.decorator';
+import { observeCollabAuth } from '../../integrations/metrics/metrics.registry';
 
 @Injectable()
 export class AuthenticationExtension implements Extension {
@@ -30,6 +31,18 @@ export class AuthenticationExtension implements Extension {
   ) {}
 
   async onAuthenticate(data: onAuthenticatePayload) {
+    // #402 — time the whole auth (verify + user/page/permission lookups) into
+    // collab_auth_duration_seconds. finally so failed auths are timed too.
+    // No-op when METRICS_PORT is unset. Behavior unchanged.
+    const start = performance.now();
+    try {
+      return await this.doAuthenticate(data);
+    } finally {
+      observeCollabAuth((performance.now() - start) / 1000);
+    }
+  }
+
+  private async doAuthenticate(data: onAuthenticatePayload) {
     const { documentName, token } = data;
     const pageId = getPageId(documentName);
 

apps/server/src/collaboration/extensions/persistence.extension.ts

@@ -41,7 +41,10 @@ import {
   HISTORY_INTERVAL,
 } from '../constants';
 import { TransclusionService } from '../../core/page/transclusion/transclusion.service';
-import { observeCollabStore } from '../../integrations/metrics/metrics.registry';
+import {
+  observeCollabLoad,
+  observeCollabStore,
+} from '../../integrations/metrics/metrics.registry';
 
 /**
  * #251 — wire format of the client→server stateless message that signals a
@@ -150,10 +153,14 @@ export class PersistenceExtension implements Extension {
     const { documentName, document } = data;
     const pageId = getPageId(documentName);
 
+    // #402 — the early return below (live doc already non-empty) does NOT touch
+    // the DB, so it is deliberately NOT timed. We only observe the real DB-load
+    // work, and only on each real-load return, tagged by the loaded doc size.
     if (!document.isEmpty('default')) {
       return;
     }
 
+    const startedAt = performance.now();
     const page = await this.pageRepo.findById(pageId, {
       includeContent: true,
       includeYdoc: true,
@@ -171,6 +178,7 @@ export class PersistenceExtension implements Extension {
       const dbState = new Uint8Array(page.ydoc);
 
       Y.applyUpdate(doc, dbState);
+      observeCollabLoad(dbState.length, (performance.now() - startedAt) / 1000);
       return doc;
     }
 
@@ -184,26 +192,42 @@ export class PersistenceExtension implements Extension {
         tiptapExtensions,
       );
 
-      Y.encodeStateAsUpdate(ydoc);
+      // Reuse this single encode for the size label (do NOT add a second one).
+      const encoded = Y.encodeStateAsUpdate(ydoc);
+      observeCollabLoad(
+        encoded.byteLength,
+        (performance.now() - startedAt) / 1000,
+      );
       return ydoc;
     }
 
     this.logger.debug(`creating fresh ydoc: ${pageId}`);
+    observeCollabLoad(0, (performance.now() - startedAt) / 1000);
     return new Y.Doc();
   }
 
   async onStoreDocument(data: onStoreDocumentPayload) {
     // #355 — time the full store (persist + post-store side effects) into
-    // collab_store_duration_seconds. No-op when METRICS_PORT is unset.
+    // collab_store_duration_seconds. #402 — also tag by document size bucket.
+    // No-op when METRICS_PORT is unset.
     const startedAt = performance.now();
+    // Default 0 so a throw before storeDocument returns still records a
+    // (smallest-bucket) observation rather than dropping the timing entirely.
+    let bytes = 0;
     try {
-      await this.storeDocument(data);
+      bytes = await this.storeDocument(data);
     } finally {
-      observeCollabStore((performance.now() - startedAt) / 1000);
+      observeCollabStore(bytes, (performance.now() - startedAt) / 1000);
     }
   }
 
-  private async storeDocument(data: onStoreDocumentPayload) {
+  /**
+   * Persist the document. Returns the serialized ydoc byte size (used as the
+   * store histogram's size_bucket). The single Y.encodeStateAsUpdate below is
+   * the ONLY serialization — its byteLength is reused for the label (no second
+   * encode).
+   */
+  private async storeDocument(data: onStoreDocumentPayload): Promise<number> {
     const { documentName, document, context } = data;
 
     const pageId = getPageId(documentName);
@@ -455,6 +479,11 @@ export class PersistenceExtension implements Extension {
 
       await this.enqueuePageHistory(page, lastUpdatedSource);
     }
+
+    // #402 — report the serialized size for the store histogram's size_bucket.
+    // ydocState is always computed above (there is no earlier no-write return in
+    // this method), so this reflects the doc that was serialized this store.
+    return ydocState.byteLength;
   }
 
   /**

apps/server/src/core/ai-chat/ai-chat.service.setup-abort.spec.ts

@@ -0,0 +1,121 @@
+import { Logger } from '@nestjs/common';
+
+// Mock the AI SDK: the turn we drive is STOPPED during the pre-streamText setup
+// phase, so no provider call must ever be made. convertToModelMessages is reached
+// (before toolsFor) so it is stubbed to an empty transcript.
+jest.mock('ai', () => ({
+  streamText: jest.fn(),
+  generateText: jest.fn(),
+  convertToModelMessages: jest.fn(async () => []),
+  stepCountIs: jest.fn(() => () => false),
+}));
+
+import { streamText } from 'ai';
+import { AiChatService } from './ai-chat.service';
+
+/**
+ * D2 — an explicit Stop DURING the external-MCP toolset build (the pre-streamText
+ * setup phase) must:
+ *   (a) unwedge the turn (stream() rejects instead of hanging at step 0), and
+ *   (b) finalize the run as 'aborted' via the outer catch's onSettled — never leak
+ *       the run row as 'running' (which would 409 every later turn in this chat).
+ *
+ * The setup phase does NOT yet observe streamText's terminal callbacks, so before
+ * the fix a hung `toolsFor` ignored the run's abort signal and never finalized.
+ * `raceAgainstAbortAndTimeout(toolsFor, effectiveSignal, ...)` now rejects the
+ * moment the run's signal aborts; the catch re-throws (signal aborted), and the
+ * outer catch settles the run 'aborted'.
+ */
+describe('AiChatService.stream — abort during external-MCP setup finalizes the run (D2)', () => {
+  const streamTextMock = streamText as unknown as jest.Mock;
+
+  function makeService(mcpClients: { toolsFor: jest.Mock }) {
+    const aiChatRepo = {
+      findById: jest.fn(async () => ({ id: 'chat-1', workspaceId: 'ws-1' })),
+      insert: jest.fn(),
+    };
+    const aiChatMessageRepo = {
+      insert: jest.fn(async () => ({ id: 'msg-1' })),
+      findAllByChat: jest.fn(async () => []),
+      update: jest.fn(async () => ({ id: 'msg-1' })),
+    };
+    const aiSettings = { resolve: jest.fn(async () => ({})) };
+    const tools = { forUser: jest.fn(async () => ({})) };
+    const svc = new AiChatService(
+      {} as never, // ai
+      aiChatRepo as never,
+      aiChatMessageRepo as never,
+      {} as never, // aiChatPageSnapshotRepo
+      aiSettings as never,
+      tools as never,
+      mcpClients as never,
+      {} as never, // aiAgentRoleRepo
+      {} as never, // pageRepo (openPage undefined -> never touched)
+      {} as never, // pageAccess
+      { isAiChatDeferredToolsEnabled: () => false } as never, // environment
+    );
+    return { svc, tools };
+  }
+
+  const body = {
+    chatId: 'chat-1',
+    messages: [
+      { id: 'm1', role: 'user', parts: [{ type: 'text', text: 'hi' }] },
+    ],
+  };
+
+  beforeEach(() => {
+    streamTextMock.mockReset();
+    jest.spyOn(Logger.prototype, 'log').mockImplementation(() => undefined as never);
+    jest.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined as never);
+    jest.spyOn(Logger.prototype, 'error').mockImplementation(() => undefined as never);
+  });
+
+  afterEach(() => jest.restoreAllMocks());
+
+  it('stops the hung toolset build, rejects, and settles the run "aborted" — never reaching streamText', async () => {
+    const runController = new AbortController();
+    // The build hangs (never resolves); the run is STOPPED mid-build. Aborting on a
+    // macrotask exercises the abort-listener path (a real user Stop during setup).
+    const toolsFor = jest.fn(() => {
+      setTimeout(() => runController.abort(new Error('user stop')), 0);
+      return new Promise(() => {}); // never settles — models a hung MCP build
+    });
+    const { svc } = makeService({ toolsFor });
+
+    const onSettled = jest.fn();
+    const begin = jest.fn(async () => ({
+      runId: 'run-1',
+      signal: runController.signal,
+    }));
+
+    const promise = svc.stream({
+      user: { id: 'user-1' } as never,
+      workspace: { id: 'ws-1' } as never,
+      sessionId: 'sess-1',
+      body: body as never,
+      res: { raw: {} } as never,
+      signal: new AbortController().signal, // socket signal (distinct from the run)
+      model: {} as never,
+      role: null,
+      runHooks: {
+        begin,
+        onAssistantSeeded: jest.fn(),
+        onStep: jest.fn(),
+        onSettled,
+      } as never,
+    });
+
+    // (a) The turn is UNWEDGED: it rejects (with the stop reason) instead of hanging.
+    await expect(promise).rejects.toThrow('user stop');
+
+    // (b) The run is finalized as 'aborted' with NO error message (a Stop, not a
+    // failure) — so the run row never leaks 'running'.
+    expect(onSettled).toHaveBeenCalledTimes(1);
+    expect(onSettled).toHaveBeenCalledWith('run-1', 'aborted', undefined);
+
+    // The build was reached, but the provider call was NEVER made (stopped at setup).
+    expect(toolsFor).toHaveBeenCalledTimes(1);
+    expect(streamTextMock).not.toHaveBeenCalled();
+  });
+});

apps/server/src/core/ai-chat/ai-chat.service.ts

@@ -58,6 +58,15 @@ import {
 // multi-search research questions are not cut off mid-investigation.
 const MAX_AGENT_STEPS = 20;
 
+// Wall-clock ceiling for building the external MCP toolset during the per-turn
+// setup phase (before streamText owns the lifecycle). Defense-in-depth ABOVE the
+// per-server connect bound in mcp-clients.service (CONNECT_TIMEOUT_MS): even if
+// that per-server timeout regressed, this outer deadline — together with the run's
+// abort signal — guarantees the setup phase can never wedge a turn at step 0 (the
+// production hang) and the run always finalizes. Generous: the per-server bound
+// should fire first in practice; this only backstops a total build stall.
+const MCP_TOOLSET_BUILD_DEADLINE_MS = 60_000;
+
 // System-prompt addendum injected ONLY on the final step (see prepareAgentStep).
 // It forbids further tool calls and tells the model to synthesize the best
 // answer it can from what it already gathered, so a tool-heavy turn never ends
@@ -176,6 +185,78 @@ export function sameInstant(
   return ta === tb;
 }
 
+/**
+ * Race `work` against an abort signal AND a wall-clock deadline, so a hung
+ * external-MCP toolset build during the pre-streamText setup phase can NEITHER
+ * wedge the turn NOR make it un-stoppable, and the run always finalizes. It
+ *  - resolves with `work`'s value when it settles first;
+ *  - REJECTS EARLY if `signal` aborts (with `signal.reason` when that is an Error,
+ *    else a generic `Error('aborted')`) — so an explicit Stop is honored mid-setup;
+ *  - REJECTS EARLY if `deadlineMs` elapses (defense-in-depth backstop);
+ *  - invokes `onLateResolve(value)` when `work` settles AFTER the race was already
+ *    lost, so the caller can release any resources that abandoned value owns
+ *    (e.g. close leased MCP clients that would otherwise leak their sockets).
+ *
+ * A rejection handler is attached to `work` so a late rejection is never an
+ * unhandledRejection; the timer is unref'd and cleared once the race settles.
+ */
+export function raceAgainstAbortAndTimeout<T>(
+  work: Promise<T>,
+  signal: AbortSignal,
+  deadlineMs: number,
+  onLateResolve?: (value: T) => void,
+): Promise<T> {
+  return new Promise<T>((resolve, reject) => {
+    let settled = false;
+    const cleanup = () => {
+      clearTimeout(timer);
+      signal.removeEventListener('abort', onAbort);
+    };
+    const onAbort = () => {
+      if (settled) return;
+      settled = true;
+      cleanup();
+      reject(
+        signal.reason instanceof Error ? signal.reason : new Error('aborted'),
+      );
+    };
+    const timer = setTimeout(() => {
+      if (settled) return;
+      settled = true;
+      cleanup();
+      reject(new Error(`setup timed out after ${deadlineMs}ms`));
+    }, deadlineMs);
+    // Do not keep the process alive just for this setup-deadline timer.
+    timer.unref?.();
+    if (signal.aborted) {
+      onAbort();
+    } else {
+      signal.addEventListener('abort', onAbort, { once: true });
+    }
+    work.then(
+      (value) => {
+        if (settled) {
+          // The race was already lost (abort/deadline): hand the abandoned value to
+          // the caller so it can release the resources that value owns.
+          onLateResolve?.(value);
+          return;
+        }
+        settled = true;
+        cleanup();
+        resolve(value);
+      },
+      (err: unknown) => {
+        // A late rejection after the race is already handled — swallow so it is
+        // never an unhandledRejection.
+        if (settled) return;
+        settled = true;
+        cleanup();
+        reject(err instanceof Error ? err : new Error(String(err)));
+      },
+    );
+  });
+}
+
 /**
  * Payload accepted from the client `useChat` POST body. We do NOT bind a strict
  * DTO (the global ValidationPipe whitelist would strip the useChat-specific
@@ -704,10 +785,40 @@ export class AiChatService implements OnModuleInit {
         instructions: [],
       };
       try {
-        external = await this.mcpClients.toolsFor(workspace.id);
+        // Bound the external-MCP toolset build by BOTH the run's abort signal and
+        // a generous wall-clock deadline. This is the pre-streamText setup phase,
+        // which streamText's terminal callbacks do NOT yet govern — so without this
+        // a hung build would hang the turn at step 0 forever (the production hang),
+        // unobservant of an explicit Stop. The deadline is defense-in-depth ABOVE
+        // the per-server connect bound in mcp-clients.service. On a LATE resolve
+        // (the race was already lost) close the abandoned toolset's leased clients
+        // so their transports are not leaked.
+        external = await raceAgainstAbortAndTimeout(
+          this.mcpClients.toolsFor(workspace.id),
+          effectiveSignal,
+          MCP_TOOLSET_BUILD_DEADLINE_MS,
+          (late) => {
+            void Promise.all(
+              late.clients.map((c) => c.close().catch(() => undefined)),
+            );
+          },
+        );
       } catch (err) {
-        // Building the external toolset must never break the turn; proceed with
-        // Docmost-only tools. Never log URLs/headers — short message only.
+        // An explicit Stop reached the RUN's signal DURING setup: re-throw so the
+        // outer catch finalizes the run as aborted — never swallow a Stop. Gated on
+        // `runId`: the re-throw exists ONLY to finalize the run, which exists only
+        // in autonomous mode. On the legacy path (no runId) `effectiveSignal` is the
+        // SOCKET signal (it aborts on a client disconnect); re-throwing there would
+        // change prior behavior and make the controller write JSON to an already-
+        // closed socket (it only attaches res.raw.on('error') in autonomous mode).
+        // So legacy keeps its prior behavior — warn + proceed, and streamText then
+        // observes the aborted socket signal.
+        if (runId && effectiveSignal.aborted) {
+          throw err;
+        }
+        // Otherwise a down/slow server (build timeout or other fault) must never
+        // break the turn: proceed with Docmost-only tools. Never log URLs/headers —
+        // short message only.
         this.logger.warn(
           `External MCP toolset unavailable: ${
             err instanceof Error ? err.message : 'unknown error'
@@ -1307,12 +1418,19 @@ export class AiChatService implements OnModuleInit {
         if (this.environment?.isAiChatResumableStreamEnabled?.()) {
           this.streamRegistry?.abortEntry(chatId, runId);
         }
+        // Distinguish an explicit Stop (the run's signal aborted during setup) from
+        // a real failure, so the run settles with the correct terminal status
+        // instead of always 'error'. onSettled/finalizeRun is idempotent, so this
+        // is safe even if a streamText callback also settles the run.
+        const settleStatus = effectiveSignal.aborted ? 'aborted' : 'error';
         await runHooks?.onSettled?.(
           runId,
-          'error',
-          err instanceof Error
-            ? err.message
-            : 'Agent run failed before streaming started',
+          settleStatus,
+          settleStatus === 'aborted'
+            ? undefined
+            : err instanceof Error
+              ? err.message
+              : 'Agent run failed before streaming started',
         );
       }
       throw err;

apps/server/src/core/ai-chat/external-mcp/mcp-clients.connect-timeout.spec.ts

@@ -0,0 +1,237 @@
+import { McpClientsService } from './mcp-clients.service';
+
+/**
+ * D1 — a HUNG MCP handshake must not POISON the per-workspace build cache.
+ *
+ * THE BUG (production hang): `createMCPClient` (inside the private `connect`) is
+ * NOT bounded by a timeout and — like @ai-sdk/mcp's tool calls — its promise does
+ * NOT settle on abort. A transient network blip mid-handshake made connect hang
+ * FOREVER. Because getOrBuildEntry caches the build PROMISE, that never-settling
+ * connect wedged EVERY later turn for the workspace (each awaited the same pending
+ * build) — step_count stuck at 0, run row leaking 'running', chat 409ing forever.
+ *
+ * THE FIX: `connectWithTimeout` races `connect` against a SETTLING timeout
+ * (CONNECT_TIMEOUT_MS). On timeout it REJECTS, so buildEntry catches it, records
+ * the server `ok:false`, and the build COMPLETES with that server skipped — the
+ * cache is never poisoned and a subsequent `toolsFor` returns instead of hanging.
+ *
+ * REACHABILITY NOTE: the smallest network-free path that exercises the fix is to
+ * spy on the private `connect` (the same harness the namespacing spec uses) —
+ * `connectWithTimeout` wraps exactly that call, so a never-resolving `connect`
+ * models a never-settling `createMCPClient` precisely, without DNS/sockets.
+ *
+ * Fake timers prove the timeout fires WITHOUT real waiting.
+ */
+
+// Mirrors the private CONNECT_TIMEOUT_MS constant in mcp-clients.service.ts.
+const CONNECT_TIMEOUT_MS = 5000;
+
+interface FakeServer {
+  id: string;
+  name: string;
+  transport: string;
+  url: string;
+  headersEnc: string | null;
+  toolAllowlist: string[] | null;
+  instructions?: string | null;
+}
+
+function server(
+  over: Partial<FakeServer> & { id: string; name: string },
+): FakeServer {
+  return {
+    transport: 'http',
+    url: 'https://example.com/mcp',
+    headersEnc: null,
+    toolAllowlist: null,
+    ...over,
+  };
+}
+
+function buildService(servers: FakeServer[]) {
+  const repoStub = { listEnabled: jest.fn().mockResolvedValue(servers) };
+  const service = new McpClientsService(repoStub as never, {} as never);
+  // Silence the expected "server unavailable" warning.
+  jest
+    .spyOn(
+      (service as unknown as { logger: { warn: (...a: unknown[]) => void } })
+        .logger,
+      'warn',
+    )
+    .mockImplementation(() => undefined);
+  return service;
+}
+
+// Spy on the private `connect` with a per-server implementation.
+function stubConnect(
+  service: McpClientsService,
+  impl: (s: FakeServer) => Promise<unknown>,
+) {
+  return jest
+    .spyOn(
+      service as unknown as { connect: (s: FakeServer) => Promise<unknown> },
+      'connect',
+    )
+    .mockImplementation(impl);
+}
+
+describe('McpClientsService.connectWithTimeout — hung connect does not poison the cache (D1)', () => {
+  beforeEach(() => jest.useFakeTimers());
+  afterEach(() => {
+    jest.clearAllTimers();
+    jest.useRealTimers();
+    jest.restoreAllMocks();
+  });
+
+  it('buildEntry completes (server recorded ok:false) when connect never settles, and toolsFor does not hang', async () => {
+    const svc = buildService([server({ id: 'id-hung', name: 'hung' })]);
+    // connect NEVER settles — models a wedged createMCPClient handshake.
+    stubConnect(svc, () => new Promise<never>(() => {}));
+
+    const toolsetPromise = svc.toolsFor('ws-1');
+    // Drive fake time past the connect bound so connectWithTimeout rejects and
+    // buildEntry catches it (records ok:false) — flushing the microtasks.
+    await jest.advanceTimersByTimeAsync(CONNECT_TIMEOUT_MS + 1);
+
+    const toolset = await toolsetPromise;
+    // The build COMPLETED with the bad server skipped (no tools, ok:false).
+    expect(Object.keys(toolset.tools)).toHaveLength(0);
+    expect(toolset.outcomes).toEqual([
+      { name: 'hung', ok: false, reason: 'MCP connect timed out after 5000ms' },
+    ]);
+    await Promise.all(toolset.clients.map((c) => c.close()));
+
+    // The cache is NOT poisoned: a subsequent turn returns (served from the warm
+    // cached entry) instead of awaiting a never-settling build.
+    const again = await svc.toolsFor('ws-1');
+    expect(Object.keys(again.tools)).toHaveLength(0);
+    await Promise.all(again.clients.map((c) => c.close()));
+  });
+
+  it('a hung server is skipped but a healthy server in the SAME build still contributes its tools', async () => {
+    const svc = buildService([
+      server({ id: 'id-hung', name: 'hung' }),
+      server({ id: 'id-ok', name: 'ok' }),
+    ]);
+    const okClient = {
+      tools: () => Promise.resolve({ search: { description: 'x' } }),
+      close: jest.fn().mockResolvedValue(undefined),
+    };
+    stubConnect(svc, (s) =>
+      s.id === 'id-hung'
+        ? new Promise<never>(() => {})
+        : Promise.resolve(okClient),
+    );
+
+    const toolsetPromise = svc.toolsFor('ws-2');
+    await jest.advanceTimersByTimeAsync(CONNECT_TIMEOUT_MS + 1);
+
+    const toolset = await toolsetPromise;
+    // Healthy server's tool survives (namespaced); hung server recorded ok:false.
+    expect(Object.keys(toolset.tools)).toEqual(['ok_search']);
+    expect(toolset.outcomes).toEqual([
+      { name: 'hung', ok: false, reason: 'MCP connect timed out after 5000ms' },
+      { name: 'ok', ok: true },
+    ]);
+    await Promise.all(toolset.clients.map((c) => c.close()));
+  });
+
+  it('closes the ORPHANED client when connect resolves LATE (after the timeout)', async () => {
+    const svc = buildService([server({ id: 'id-late', name: 'late' })]);
+    const lateClient = {
+      tools: () => Promise.resolve({}),
+      close: jest.fn().mockResolvedValue(undefined),
+    };
+    // connect resolves only AFTER the connect bound has already elapsed, so
+    // connectWithTimeout has already rejected and must close this orphan.
+    stubConnect(
+      svc,
+      () =>
+        new Promise((resolve) => {
+          setTimeout(() => resolve(lateClient), CONNECT_TIMEOUT_MS * 2);
+        }),
+    );
+
+    const toolsetPromise = svc.toolsFor('ws-3');
+    // Fire the timeout: the build completes with the server skipped.
+    await jest.advanceTimersByTimeAsync(CONNECT_TIMEOUT_MS + 1);
+    const toolset = await toolsetPromise;
+    expect(toolset.outcomes[0]?.ok).toBe(false);
+    expect(lateClient.close).not.toHaveBeenCalled();
+
+    // Now let the late connect resolve — the orphan must be closed, not leaked.
+    await jest.advanceTimersByTimeAsync(CONNECT_TIMEOUT_MS * 2);
+    expect(lateClient.close).toHaveBeenCalledTimes(1);
+
+    await Promise.all(toolset.clients.map((c) => c.close()));
+  });
+});
+
+describe('McpClientsService.buildEntry — closes a connected client whose tools() fails (leak fix)', () => {
+  beforeEach(() => jest.useFakeTimers());
+  afterEach(() => {
+    jest.clearAllTimers();
+    jest.useRealTimers();
+    jest.restoreAllMocks();
+  });
+
+  it('connect succeeds but tools() REJECTS: the client is close()d exactly once and the server is skipped, while a healthy server still contributes', async () => {
+    const svc = buildService([
+      server({ id: 'id-bad', name: 'bad' }),
+      server({ id: 'id-ok', name: 'ok' }),
+    ]);
+    // The bad server connects fine, then tools() rejects — the client would leak if
+    // buildEntry did not close it in the per-server catch (it was never registered).
+    const badClient = {
+      tools: () => Promise.reject(new Error('tools listing failed')),
+      close: jest.fn().mockResolvedValue(undefined),
+    };
+    const okClient = {
+      tools: () => Promise.resolve({ search: { description: 'x' } }),
+      close: jest.fn().mockResolvedValue(undefined),
+    };
+    stubConnect(svc, (s) =>
+      s.id === 'id-bad' ? Promise.resolve(badClient) : Promise.resolve(okClient),
+    );
+
+    const toolset = await svc.toolsFor('ws-4');
+
+    // The orphaned (never-registered) client is closed exactly once — no leak.
+    expect(badClient.close).toHaveBeenCalledTimes(1);
+    // Healthy server survives; bad server recorded ok:false and skipped.
+    expect(Object.keys(toolset.tools)).toEqual(['ok_search']);
+    expect(toolset.outcomes).toEqual([
+      { name: 'bad', ok: false, reason: 'tools listing failed' },
+      { name: 'ok', ok: true },
+    ]);
+
+    // The healthy (registered) client is NOT closed by the loop — it is owned by the
+    // cache entry and stays warm (closed only on eviction/teardown, not on lease
+    // release). Releasing the lease keeps it warm since the entry is not evicted.
+    expect(okClient.close).not.toHaveBeenCalled();
+    await Promise.all(toolset.clients.map((c) => c.close()));
+    expect(okClient.close).not.toHaveBeenCalled();
+    // The failed client is never double-closed.
+    expect(badClient.close).toHaveBeenCalledTimes(1);
+  });
+
+  it('connect succeeds but tools() HANGS (times out): the client is close()d once and the server is skipped', async () => {
+    const svc = buildService([server({ id: 'id-slow', name: 'slow' })]);
+    const slowClient = {
+      // tools() never settles -> withTimeout rejects after CONNECT_TIMEOUT_MS.
+      tools: () => new Promise<Record<string, never>>(() => {}),
+      close: jest.fn().mockResolvedValue(undefined),
+    };
+    stubConnect(svc, () => Promise.resolve(slowClient));
+
+    const toolsetPromise = svc.toolsFor('ws-5');
+    // Drive fake time past the tools() bound so withTimeout rejects.
+    await jest.advanceTimersByTimeAsync(CONNECT_TIMEOUT_MS + 1);
+    const toolset = await toolsetPromise;
+
+    expect(slowClient.close).toHaveBeenCalledTimes(1);
+    expect(Object.keys(toolset.tools)).toHaveLength(0);
+    expect(toolset.outcomes[0]?.ok).toBe(false);
+    await Promise.all(toolset.clients.map((c) => c.close()));
+  });
+});

apps/server/src/core/ai-chat/external-mcp/mcp-clients.service.ts

@@ -195,7 +195,7 @@ export class McpClientsService {
   ): Promise<{ ok: true; tools: string[] } | { ok: false; error: string }> {
     let client: McpClient | undefined;
     try {
-      client = await this.connect(server);
+      client = await this.connectWithTimeout(server, CONNECT_TIMEOUT_MS);
       const raw = await withTimeout(client.tools(), CONNECT_TIMEOUT_MS);
       return { ok: true, tools: Object.keys(raw) };
     } catch (err) {
@@ -256,10 +256,18 @@ export class McpClientsService {
     const instructions: McpServerInstruction[] = [];
 
     for (const server of servers) {
+      // Track the connected client OUTSIDE the try so the catch can close it when
+      // it was obtained but not yet registered in `clients` (e.g. tools() threw or
+      // timed out after a successful connect). `registered` flips only once the
+      // client is owned by `clients` (closed at entry teardown), so the catch never
+      // double-closes a registered client.
+      let client: McpClient | undefined;
+      let registered = false;
       try {
-        const client = await this.connect(server);
+        client = await this.connectWithTimeout(server, CONNECT_TIMEOUT_MS);
         const raw = await withTimeout(client.tools(), CONNECT_TIMEOUT_MS);
         clients.push(client);
+        registered = true;
         const allow = server.toolAllowlist;
         const picked =
           Array.isArray(allow) && allow.length > 0 ? pick(raw, allow) : raw;
@@ -290,9 +298,15 @@ export class McpClientsService {
           });
         }
       } catch (err) {
-        // A failed server is skipped — the turn proceeds with the rest. Log a
-        // short warning (never the URL/headers) so ops can see degradation, and
-        // record the outcome so the UI can show "tool X unavailable".
+        // A failed server is skipped — the turn proceeds with the rest. If connect
+        // returned a live client but a later step (tools()) threw, that client was
+        // never registered in `clients`, so close it here or its transport/socket
+        // leaks (compounding every 60s cache rebuild during a flaky-server outage).
+        if (client && !registered) {
+          void client.close().catch(() => undefined);
+        }
+        // Log a short warning (never the URL/headers) so ops can see degradation,
+        // and record the outcome so the UI can show "tool X unavailable".
         const reason = shortError(err);
         this.logger.warn(
           `External MCP server "${server.name}" unavailable: ${reason}`,
@@ -383,6 +397,55 @@ export class McpClientsService {
     return client;
   }
 
+  /**
+   * Race {@link connect} against a SETTLING timeout so a hung MCP handshake can
+   * never POISON the per-workspace build cache. `createMCPClient` (inside connect)
+   * is NOT bounded internally, and — exactly like @ai-sdk/mcp's tool calls
+   * (see wrapToolWithCallTimeout) — its promise does NOT settle on abort. So a
+   * transient network blip mid-handshake can make connect hang FOREVER. Because
+   * getOrBuildEntry caches the build PROMISE, a never-settling connect would then
+   * wedge EVERY later turn for the workspace (each awaits the same pending build,
+   * step_count stuck at 0, run row leaks 'running', chat 409s forever). Bounding
+   * connect here guarantees buildEntry always gets a client OR a rejection within
+   * `ms` — so the build completes (bad server skipped) and the cache stays clean.
+   *
+   * If connect resolves LATE (after we already rejected on the timeout), we close
+   * the orphaned client so its transport/socket is not leaked.
+   */
+  private connectWithTimeout(
+    server: Pick<AiMcpServer, 'transport' | 'url' | 'headersEnc'>,
+    ms: number,
+  ): Promise<McpClient> {
+    return new Promise<McpClient>((resolve, reject) => {
+      let settled = false;
+      const timer = setTimeout(() => {
+        settled = true;
+        reject(new Error(`MCP connect timed out after ${ms}ms`));
+      }, ms);
+      // Do not keep the process alive just for this connect-timeout timer.
+      timer.unref?.();
+      this.connect(server).then(
+        (client) => {
+          if (settled) {
+            // The race was already lost to the timeout: close the orphaned client
+            // so its socket is not leaked, and drop the late result.
+            void client.close().catch(() => undefined);
+            return;
+          }
+          clearTimeout(timer);
+          settled = true;
+          resolve(client);
+        },
+        (err: unknown) => {
+          if (settled) return; // late rejection after the timeout — already handled
+          clearTimeout(timer);
+          settled = true;
+          reject(err instanceof Error ? err : new Error(String(err)));
+        },
+      );
+    });
+  }
+
   /**
    * Decrypt the stored auth headers. Returns undefined when none are set. The
    * plaintext headers live only in this returned object and are passed straight

apps/server/src/integrations/mcp/mcp-auth.helpers.ts

@@ -149,6 +149,15 @@ export type DocmostMcpConfig = (
     has?: (uri: string) => boolean;
     evict?: (uri: string) => void;
   };
+  // Dependency-neutral metrics sink injected by McpService (mirror of the
+  // package's onMetric). The package emits generic (name, value, labels)
+  // samples; McpService maps them onto the prom-client registry. Undefined
+  // when metrics are disabled → the package no-ops.
+  onMetric?: (
+    name: string,
+    value: number,
+    labels?: Record<string, string>,
+  ) => void;
 };
 
 export interface ResolvedMcpAuth {

apps/server/src/integrations/mcp/mcp.service.ts

@@ -30,6 +30,11 @@ import {
   ResolvedMcpAuth,
 } from './mcp-auth.helpers';
 import { SandboxStore } from '../sandbox/sandbox.store';
+import {
+  isMetricsEnabled,
+  observeMcpTool,
+  incConnectTimeout,
+} from '../metrics/metrics.registry';
 
 // Minimal shape of the embedded MCP HTTP handler exported by @docmost/mcp/http.
 interface McpHttpHandler {
@@ -331,7 +336,31 @@ export class McpService implements OnModuleDestroy {
             // can store blobs in the shared in-RAM store regardless of which
             // credential variant resolved. The sink (put/has/evict + uri↔id
             // mapping) is owned by SandboxStore.asSink().
-            return { ...resolved.config, sandbox: this.sandboxStore.asSink() };
+            // Route the package's dependency-neutral metric samples onto the
+            // prom-client registry. When metrics are disabled, onMetric is
+            // undefined → the package's tool-timer/timeout hooks are a
+            // negligible-overhead no-op: the registerTool wrapper still runs a
+            // performance.now() + async try/finally per tool call, but the
+            // `onMetric?.()` short-circuits so no label/object is built. (Cost
+            // is immaterial at LLM tool-call rate.) labels?.tool is guarded
+            // defensively (the tool wrapper always sets it).
+            return {
+              ...resolved.config,
+              sandbox: this.sandboxStore.asSink(),
+              onMetric: isMetricsEnabled()
+                ? (
+                    name: string,
+                    value: number,
+                    labels?: Record<string, string>,
+                  ) => {
+                    if (name === 'mcp_tool_duration_seconds') {
+                      observeMcpTool(labels?.tool ?? 'other', value);
+                    } else if (name === 'collab_connect_timeouts_total') {
+                      incConnectTimeout();
+                    }
+                  }
+                : undefined,
+            };
           },
           {
             identify: (req: IncomingMessage) => {

apps/server/src/integrations/metrics/metrics.constants.ts

@@ -2,6 +2,11 @@
  * Perf-metrics contract (#355). These names/labels are FIXED by the already
  * deployed scrape+dashboard infra (VictoriaMetrics scraping docmost:9464,
  * Grafana dashboards, alerts). Do NOT rename them.
+ *
+ * #402 extends the #355 table with the collab-lifecycle + MCP-tool families
+ * (grouped below). These are the fixed contract from #402; same "do not rename"
+ * rule applies. Server-side families land in Pass 1; the MCP-tool histogram is
+ * fed by the MCP callback in a later pass but its NAME is fixed here now.
  */
 export const METRIC_HTTP_REQUEST_DURATION = 'http_request_duration_seconds';
 export const METRIC_DB_QUERY_DURATION = 'db_query_duration_seconds';
@@ -9,6 +14,17 @@ export const METRIC_BULLMQ_QUEUE_DEPTH = 'bullmq_queue_depth';
 export const METRIC_BULLMQ_JOB_DURATION = 'bullmq_job_duration_seconds';
 export const METRIC_COLLAB_STORE_DURATION = 'collab_store_duration_seconds';
 
+// #402 additions — collaboration lifecycle + MCP tool timing.
+export const METRIC_COLLAB_LOAD_DURATION = 'collab_doc_load_duration_seconds';
+export const METRIC_COLLAB_DOCS_OPEN = 'collab_docs_open';
+export const METRIC_COLLAB_DOC_LOADS_TOTAL = 'collab_doc_loads_total';
+export const METRIC_COLLAB_DOC_UNLOADS_TOTAL = 'collab_doc_unloads_total';
+export const METRIC_COLLAB_CONNECT_DURATION = 'collab_connect_duration_seconds';
+export const METRIC_COLLAB_CONNECT_TIMEOUTS_TOTAL =
+  'collab_connect_timeouts_total';
+export const METRIC_COLLAB_AUTH_DURATION = 'collab_auth_duration_seconds';
+export const METRIC_MCP_TOOL_DURATION = 'mcp_tool_duration_seconds';
+
 // Histogram buckets (seconds). Chosen to give useful p50/p95/p99 resolution
 // for typical web/DB latencies without exploding series cardinality.
 export const HTTP_BUCKETS = [
@@ -23,6 +39,12 @@ export const COLLAB_BUCKETS = [
 export const JOB_BUCKETS = [
   0.01, 0.05, 0.1, 0.25, 0.5, 1, 2.5, 5, 10, 30, 60, 120,
 ];
+// #402 — MCP tool-call latency. Same shape as COLLAB_BUCKETS but stretched to
+// 10s at the top: an MCP tool round-trip (LLM-driven doc ops) can be slower
+// than a single collab store, so keep resolution out to 10s.
+export const MCP_TOOL_BUCKETS = [
+  0.005, 0.025, 0.1, 0.25, 0.5, 1, 2.5, 5, 10,
+];
 
 /**
  * Extract the first SQL token (select/insert/update/delete/...) from a query,
@@ -58,6 +80,30 @@ export function firstSqlToken(sql: string | undefined): string {
   return KNOWN_SQL_TOKENS.has(token) ? token : 'other';
 }
 
+/**
+ * #402 — bucket a document byte size into ONE of four fixed labels for the
+ * collab load/store histograms' `size_bucket` label. Using the raw byte count
+ * would be a continuous, unbounded label; four coarse buckets keep series
+ * cardinality bounded (each of collab_doc_load / collab_store gets ×4 series).
+ *
+ * The SAME function is shared by both the load and store paths so their buckets
+ * can never drift apart. Non-finite / negative sizes collapse to the smallest
+ * bucket ('lt64k') as a safe default (they can't be legitimately huge and we
+ * must never throw here — this runs on every store/load when metrics are on).
+ */
+// Module-const thresholds, built once (not per observe).
+const SIZE_THRESHOLDS = { lt64k: 65536, lt256k: 262144, lt1m: 1048576 } as const;
+
+export function sizeBucket(
+  bytes: number | undefined | null,
+): 'lt64k' | 'lt256k' | 'lt1m' | 'ge1m' {
+  if (bytes == null || !Number.isFinite(bytes) || bytes < 0) return 'lt64k';
+  if (bytes < SIZE_THRESHOLDS.lt64k) return 'lt64k';
+  if (bytes < SIZE_THRESHOLDS.lt256k) return 'lt256k';
+  if (bytes < SIZE_THRESHOLDS.lt1m) return 'lt1m';
+  return 'ge1m';
+}
+
 /**
  * Whether an HTTP response must be EXCLUDED from http_request_duration_seconds.
  *

apps/server/src/integrations/metrics/metrics.registry.ts

@@ -1,5 +1,6 @@
 import {
   collectDefaultMetrics,
+  Counter,
   Histogram,
   Gauge,
   Registry,
@@ -9,11 +10,21 @@ import {
   DB_BUCKETS,
   HTTP_BUCKETS,
   JOB_BUCKETS,
+  MCP_TOOL_BUCKETS,
   METRIC_BULLMQ_JOB_DURATION,
   METRIC_BULLMQ_QUEUE_DEPTH,
+  METRIC_COLLAB_AUTH_DURATION,
+  METRIC_COLLAB_CONNECT_DURATION,
+  METRIC_COLLAB_CONNECT_TIMEOUTS_TOTAL,
+  METRIC_COLLAB_DOC_LOADS_TOTAL,
+  METRIC_COLLAB_DOC_UNLOADS_TOTAL,
+  METRIC_COLLAB_DOCS_OPEN,
+  METRIC_COLLAB_LOAD_DURATION,
   METRIC_COLLAB_STORE_DURATION,
   METRIC_DB_QUERY_DURATION,
   METRIC_HTTP_REQUEST_DURATION,
+  METRIC_MCP_TOOL_DURATION,
+  sizeBucket,
 } from './metrics.constants';
 
 /**
@@ -39,7 +50,24 @@ let httpHist: Histogram<'method' | 'route' | 'status'> | null = null;
 let dbHist: Histogram<'op'> | null = null;
 let queueDepthGauge: Gauge<'queue'> | null = null;
 let jobHist: Histogram<'queue'> | null = null;
-let collabHist: Histogram | null = null;
+// #402 — collab store now carries a size_bucket label (see sizeBucket()).
+let collabHist: Histogram<'size_bucket'> | null = null;
+// #402 collab-lifecycle + MCP instruments.
+let collabLoadHist: Histogram<'size_bucket'> | null = null;
+let docsOpenGauge: Gauge | null = null;
+let docLoadsCounter: Counter | null = null;
+let docUnloadsCounter: Counter | null = null;
+let connectTimeoutsCounter: Counter | null = null;
+let collabConnectHist: Histogram | null = null;
+let collabAuthHist: Histogram | null = null;
+let mcpToolHist: Histogram<'tool'> | null = null;
+
+// #402 — read-on-scrape source for collab_docs_open. The gauge is NEVER
+// inc/dec'd (that drifts under crashes/handoffs); instead its collect() callback
+// pulls the authoritative live count from here on every scrape. Registered once,
+// gated, by the collaboration gateway via registerDocsOpenSource(). Null until
+// then → the collect() is a no-op.
+let docsOpenSource: (() => number) | null = null;
 
 function init(): void {
   if (registry || !enabled) return;
@@ -82,10 +110,71 @@ function init(): void {
 
   collabHist = new Histogram({
     name: METRIC_COLLAB_STORE_DURATION,
-    help: 'Collaboration onStoreDocument duration in seconds',
+    help: 'Collaboration onStoreDocument duration in seconds, by document size bucket',
+    labelNames: ['size_bucket'],
     buckets: COLLAB_BUCKETS,
     registers: [registry],
   });
+
+  collabLoadHist = new Histogram({
+    name: METRIC_COLLAB_LOAD_DURATION,
+    help: 'Collaboration onLoadDocument DB-load duration in seconds, by document size bucket',
+    labelNames: ['size_bucket'],
+    buckets: COLLAB_BUCKETS,
+    registers: [registry],
+  });
+
+  docsOpenGauge = new Gauge({
+    name: METRIC_COLLAB_DOCS_OPEN,
+    help: 'Number of collaboration documents currently open in memory',
+    registers: [registry],
+    // Read-on-scrape: pull the live count from the registered source (the
+    // hocuspocus instance) so the value can never drift. No-op until a source
+    // is registered.
+    collect() {
+      if (docsOpenSource) this.set(docsOpenSource());
+    },
+  });
+
+  docLoadsCounter = new Counter({
+    name: METRIC_COLLAB_DOC_LOADS_TOTAL,
+    help: 'Total collaboration documents loaded into memory',
+    registers: [registry],
+  });
+
+  docUnloadsCounter = new Counter({
+    name: METRIC_COLLAB_DOC_UNLOADS_TOTAL,
+    help: 'Total collaboration documents unloaded from memory',
+    registers: [registry],
+  });
+
+  connectTimeoutsCounter = new Counter({
+    name: METRIC_COLLAB_CONNECT_TIMEOUTS_TOTAL,
+    help: 'Total collaboration connection setup timeouts',
+    registers: [registry],
+  });
+
+  collabConnectHist = new Histogram({
+    name: METRIC_COLLAB_CONNECT_DURATION,
+    help: 'Collaboration connection acceptance duration in seconds (onConnect→connected)',
+    buckets: COLLAB_BUCKETS,
+    registers: [registry],
+  });
+
+  collabAuthHist = new Histogram({
+    name: METRIC_COLLAB_AUTH_DURATION,
+    help: 'Collaboration onAuthenticate duration in seconds',
+    buckets: COLLAB_BUCKETS,
+    registers: [registry],
+  });
+
+  mcpToolHist = new Histogram({
+    name: METRIC_MCP_TOOL_DURATION,
+    help: 'MCP tool-call duration in seconds, by tool name',
+    labelNames: ['tool'],
+    buckets: MCP_TOOL_BUCKETS,
+    registers: [registry],
+  });
 }
 
 // Runs once when this module is first imported. Safe to call again (idempotent).
@@ -121,6 +210,46 @@ export function observeJobDuration(queue: string, seconds: number): void {
   jobHist?.observe({ queue }, seconds);
 }
 
-export function observeCollabStore(seconds: number): void {
-  collabHist?.observe(seconds);
+export function observeCollabStore(bytes: number, seconds: number): void {
+  collabHist?.observe({ size_bucket: sizeBucket(bytes) }, seconds);
+}
+
+export function observeCollabLoad(bytes: number, seconds: number): void {
+  collabLoadHist?.observe({ size_bucket: sizeBucket(bytes) }, seconds);
+}
+
+/**
+ * Register the live open-document count source for the collab_docs_open gauge.
+ * Called ONCE, gated by isMetricsEnabled(), by the collaboration gateway. The
+ * gauge reads this on every scrape (collect()); nothing inc/dec's it.
+ */
+export function registerDocsOpenSource(fn: () => number): void {
+  docsOpenSource = fn;
+}
+
+export function incDocLoad(): void {
+  docLoadsCounter?.inc();
+}
+
+export function incDocUnload(): void {
+  docUnloadsCounter?.inc();
+}
+
+export function incConnectTimeout(): void {
+  connectTimeoutsCounter?.inc();
+}
+
+export function observeCollabConnect(seconds: number): void {
+  collabConnectHist?.observe(seconds);
+}
+
+export function observeCollabAuth(seconds: number): void {
+  collabAuthHist?.observe(seconds);
+}
+
+export function observeMcpTool(tool: string, seconds: number): void {
+  // `tool` MUST be a bounded, registration-derived MCP tool name (the caller
+  // guarantees it comes from the registered-tool set) — never free-form input —
+  // so this label stays low-cardinality with no 'other' bucketing needed.
+  mcpToolHist?.observe({ tool }, seconds);
 }

apps/server/src/integrations/metrics/metrics.spec.ts

@@ -1,6 +1,23 @@
 import { FastifyRequest } from 'fastify';
 import { resolveRouteLabel } from './http-metrics.hook';
-import { firstSqlToken, isStreamingResponse } from './metrics.constants';
+import {
+  firstSqlToken,
+  isStreamingResponse,
+  sizeBucket,
+} from './metrics.constants';
+import {
+  getMetricsRegistry,
+  incConnectTimeout,
+  incDocLoad,
+  incDocUnload,
+  isMetricsEnabled,
+  observeCollabAuth,
+  observeCollabConnect,
+  observeCollabLoad,
+  observeCollabStore,
+  observeMcpTool,
+  registerDocsOpenSource,
+} from './metrics.registry';
 
 describe('resolveRouteLabel (histogram route label)', () => {
   it('uses the ROUTE TEMPLATE, never the raw URL', () => {
@@ -123,3 +140,67 @@ describe('firstSqlToken (bounded db label)', () => {
     expect(firstSqlToken('vacuum analyze')).toBe('other');
   });
 });
+
+describe('sizeBucket (#402 bounded size label)', () => {
+  it('maps sizes to the four fixed buckets at their boundaries', () => {
+    // Boundaries are exclusive-upper: <65536 → lt64k, etc.
+    expect(sizeBucket(0)).toBe('lt64k');
+    expect(sizeBucket(65535)).toBe('lt64k');
+    expect(sizeBucket(65536)).toBe('lt256k');
+    expect(sizeBucket(262143)).toBe('lt256k');
+    expect(sizeBucket(262144)).toBe('lt1m');
+    expect(sizeBucket(1048575)).toBe('lt1m');
+    expect(sizeBucket(1048576)).toBe('ge1m');
+    expect(sizeBucket(5_000_000)).toBe('ge1m');
+  });
+
+  it('falls back to the smallest bucket for invalid sizes', () => {
+    // Non-finite / negative / nullish collapse to the smallest, safe default.
+    expect(sizeBucket(-1)).toBe('lt64k');
+    expect(sizeBucket(NaN)).toBe('lt64k');
+    expect(sizeBucket(Infinity)).toBe('lt64k');
+    expect(sizeBucket(undefined)).toBe('lt64k');
+    expect(sizeBucket(null)).toBe('lt64k');
+  });
+
+  it('only ever returns one of the four fixed labels (bounded cardinality)', () => {
+    const labels = new Set(
+      [0, 65536, 262144, 1048576, -5, NaN].map((b) => sizeBucket(b)),
+    );
+    for (const l of labels) {
+      expect(['lt64k', 'lt256k', 'lt1m', 'ge1m']).toContain(l);
+    }
+  });
+});
+
+describe('metrics helpers are safe no-ops when METRICS_PORT is unset', () => {
+  // These specs run without METRICS_PORT, so the registry is never created and
+  // every observe/inc/set helper must be a cheap `?.` no-op that never throws.
+  beforeAll(() => {
+    // Guard the contract this suite depends on: if a CI env set METRICS_PORT,
+    // the assertions below would be meaningless, so fail loudly instead.
+    expect(process.env.METRICS_PORT).toBeUndefined();
+  });
+
+  it('reports metrics disabled and a null registry', () => {
+    expect(isMetricsEnabled()).toBe(false);
+    expect(getMetricsRegistry()).toBeNull();
+  });
+
+  it('does not throw from any #402 collab/MCP helper', () => {
+    expect(() => {
+      observeCollabLoad(123456, 0.01);
+      observeCollabStore(123456, 0.02);
+      observeCollabConnect(0.03);
+      observeCollabAuth(0.04);
+      observeMcpTool('some-tool', 0.05);
+      incDocLoad();
+      incDocUnload();
+      incConnectTimeout();
+      // Registering a source must not create the gauge or invoke the fn.
+      registerDocsOpenSource(() => {
+        throw new Error('docsOpenSource must NOT be called when disabled');
+      });
+    }).not.toThrow();
+  });
+});

packages/mcp/src/client.ts

@@ -137,6 +137,15 @@ export type DocmostMcpConfig = { apiUrl: string } & (
       has?: (uri: string) => boolean;
       evict?: (uri: string) => void;
     };
+    // Dependency-neutral metrics sink. When present, the client emits generic
+    // (name, value, labels) samples; the HOST maps those names onto its own
+    // metrics registry (the package never depends on prom-client or the server).
+    // Absent in standalone/stdio mode → the client is a complete no-op here.
+    onMetric?: (
+      name: string,
+      value: number,
+      labels?: Record<string, string>,
+    ) => void;
   };
 
 // Canonical UUID shape (versions 1–8, matching the `uuid` package's `validate`
@@ -173,6 +182,11 @@ export class DocmostClient {
   // op's image blobs if the final doc put throws. Null when the sink omits them.
   private sandboxHas: ((uri: string) => boolean) | null = null;
   private sandboxEvict: ((uri: string) => void) | null = null;
+  // Optional dependency-neutral metrics sink (see DocmostMcpConfig.onMetric).
+  // Null on the legacy positional form and whenever the host omits it → no-op.
+  private onMetricFn:
+    | ((name: string, value: number, labels?: Record<string, string>) => void)
+    | null = null;
   // In-flight login dedup: when the token expires, the 401 interceptor,
   // ensureAuthenticated, getCollabTokenWithReauth and the two multipart retries
   // can all call login() at once. Memoizing a single promise collapses that
@@ -222,6 +236,8 @@ export class DocmostClient {
       this.sandboxHas = config.sandbox.has ?? null;
       this.sandboxEvict = config.sandbox.evict ?? null;
     }
+    // Legacy positional form carries no onMetric → null (complete no-op).
+    this.onMetricFn = config.onMetric ?? null;
     this.client = axios.create({
       baseURL: this.apiUrl,
       // Default request timeout so a hung connection cannot wedge a per-page
@@ -447,6 +463,10 @@ export class DocmostClient {
       };
 
       connectTimer = setTimeout(() => {
+        // Only the actual 25s collab connect timeout fires here — the agent's
+        // collab connection to the server never became ready. This is the
+        // connect-vs-unload signal; the other finish() paths must NOT emit it.
+        this.onMetricFn?.("collab_connect_timeouts_total", 1);
         finish(new Error("Connection timeout to collaboration server"));
       }, CONNECT_TIMEOUT_MS);
 

packages/mcp/src/index.ts

@@ -64,6 +64,33 @@ const jsonContent = (data: any) => ({
  * REST + the collaboration WebSocket using the provided service-account
  * credentials and auto-re-authenticates.
  */
+/**
+ * Wrap a tool handler so its wall-clock duration is reported through the host's
+ * dependency-neutral sink as `mcp_tool_duration_seconds` (labelled by tool
+ * name). Pure and side-effect-free apart from the optional `onMetric` call:
+ *  - preserves the handler's exact return value (awaited);
+ *  - observes in a `finally`, so it records on BOTH success and throw, then
+ *    rethrows the original error unchanged (never swallowed);
+ *  - with no `onMetric` (standalone/stdio) it is a transparent pass-through.
+ * Exported so the timing contract can be unit-tested without a live transport.
+ */
+export function timeToolHandler(
+  name: string,
+  handler: (...args: any[]) => any,
+  onMetric?: (name: string, value: number, labels?: Record<string, string>) => void,
+): (...args: any[]) => Promise<any> {
+  return async (...handlerArgs: any[]) => {
+    const start = performance.now();
+    try {
+      return await handler(...handlerArgs);
+    } finally {
+      onMetric?.("mcp_tool_duration_seconds", (performance.now() - start) / 1000, {
+        tool: name,
+      });
+    }
+  };
+}
+
 export function createDocmostMcpServer(config: DocmostMcpConfig): McpServer {
   // Pass the whole config union through: the client branches internally on
   // credentials vs. getToken, so both the external /mcp (creds) and the
@@ -78,6 +105,26 @@ export function createDocmostMcpServer(config: DocmostMcpConfig): McpServer {
     { instructions: SERVER_INSTRUCTIONS },
   );
 
+  // Single choke point for MCP tool timing. Both `registerShared` (below) and
+  // the inline `server.registerTool(...)` calls funnel through this one method,
+  // so monkeypatching it HERE — before any tool is registered and before
+  // `registerShared` captures a reference to it — times every tool with no
+  // per-tool boilerplate. The wrapped handler records wall-clock duration and,
+  // in a `finally`, feeds the host's dependency-neutral sink
+  // `config.onMetric("mcp_tool_duration_seconds", seconds, { tool })`. The tool
+  // name is the registration name (bounded cardinality). When no onMetric is
+  // provided (standalone/stdio) the wrapper is a pure pass-through: it still
+  // returns the original result and rethrows the original error unchanged.
+  const originalRegisterTool = server.registerTool.bind(server) as (
+    ...args: any[]
+  ) => any;
+  (server as any).registerTool = (...args: any[]) => {
+    const name = args[0] as string;
+    const handler = args[args.length - 1];
+    const timedHandler = timeToolHandler(name, handler, config.onMetric);
+    return originalRegisterTool(...args.slice(0, -1), timedHandler);
+  };
+
   // Register a tool from the shared, zod-agnostic spec registry. The spec owns
   // the canonical name + model-facing description + (optional) schema builder;
   // only the execute body is supplied per call. buildShape is invoked with THIS

packages/mcp/test/mock/tool-timing-server.test.mjs

@@ -0,0 +1,87 @@
+// #402 — INTEGRATION test that locks the registerTool monkeypatch installed by
+// createDocmostMcpServer (src/index.ts). The sibling unit test
+// (test/unit/tool-timing.test.mjs) only exercises the timeToolHandler helper in
+// ISOLATION; it never constructs the server, so nothing there proves the factory
+// actually (a) wraps every registered tool through that helper and (b) labels
+// each sample with the tool's REGISTRATION name.
+//
+// Here we stand up a real McpServer via the factory, connect a real MCP Client
+// over the SDK's in-memory transport, invoke one registered tool, and assert the
+// host's onMetric sink received `("mcp_tool_duration_seconds", <number>, { tool
+// })` with tool === the exact registration name. This locks both the "monkeypatch
+// wraps tools" and "label = registration name" halves of the contract, so a
+// mutation to args.slice(0, -1) / the handler-arg detection / the capture order
+// is caught.
+import { test } from "node:test";
+import assert from "node:assert/strict";
+
+import { Client } from "@modelcontextprotocol/sdk/client/index.js";
+import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js";
+
+import { createDocmostMcpServer } from "../../build/index.js";
+
+// The tool we drive. get_workspace has NO input schema, so protocol-level input
+// validation cannot short-circuit before the handler runs — the wrapped handler
+// is guaranteed to execute (and then fail on the unreachable backend, which is
+// exactly what we want: the wrapper times in a finally on throw too).
+const TOOL_NAME = "get_workspace";
+
+test("the factory's registerTool monkeypatch times a live tool call and labels it with the registration name", async () => {
+  const calls = [];
+  const onMetric = (name, value, labels) => calls.push({ name, value, labels });
+
+  // Minimal valid credentials config. apiUrl points at a port that refuses
+  // connections immediately so the tool's backend call fails FAST (ECONNREFUSED)
+  // rather than hanging — the wrapper still emits the metric from its finally.
+  const server = createDocmostMcpServer({
+    apiUrl: "http://127.0.0.1:1",
+    email: "x@example.com",
+    password: "pw",
+    onMetric,
+  });
+
+  const [clientTransport, serverTransport] =
+    InMemoryTransport.createLinkedPair();
+
+  const client = new Client(
+    { name: "test-client", version: "0.0.0" },
+    { capabilities: {} },
+  );
+
+  await Promise.all([
+    server.connect(serverTransport),
+    client.connect(clientTransport),
+  ]);
+
+  try {
+    // Invoke the tool. The backend is unreachable, so this either resolves with
+    // an error result (isError) or rejects — both are fine. What matters is that
+    // the handler ran through the timing wrapper, which fires onMetric either way.
+    try {
+      await client.callTool({ name: TOOL_NAME, arguments: {} });
+    } catch {
+      // Tolerate the expected backend failure surfacing as a thrown protocol error.
+    }
+
+    // The wrapper must have fed exactly the timing sample for THIS tool.
+    const timing = calls.filter(
+      (c) => c.name === "mcp_tool_duration_seconds",
+    );
+    assert.ok(
+      timing.length >= 1,
+      "onMetric must receive a mcp_tool_duration_seconds sample from the wrapped handler",
+    );
+
+    const sample = timing.find((c) => c.labels && c.labels.tool === TOOL_NAME);
+    assert.ok(
+      sample,
+      `a timing sample must be labelled with the registration name "${TOOL_NAME}"; ` +
+        `got labels: ${JSON.stringify(timing.map((c) => c.labels))}`,
+    );
+    assert.equal(typeof sample.value, "number");
+    assert.ok(sample.value >= 0, "duration must be non-negative seconds");
+  } finally {
+    await client.close();
+    await server.close();
+  }
+});

packages/mcp/test/unit/tool-timing.test.mjs

@@ -0,0 +1,67 @@
+import { test } from "node:test";
+import assert from "node:assert/strict";
+
+import { timeToolHandler } from "../../build/index.js";
+
+// #402 — the registerTool wrapper times every tool through a single choke point
+// and feeds the host's dependency-neutral onMetric sink. These assert the
+// timing contract without a live transport (the factory monkeypatches
+// server.registerTool with exactly this helper).
+
+test("times a tool and preserves the handler's return value", async () => {
+  const calls = [];
+  const onMetric = (name, value, labels) => calls.push({ name, value, labels });
+
+  const handler = async (arg) => ({ ok: true, echo: arg });
+  const wrapped = timeToolHandler("get_page", handler, onMetric);
+
+  const result = await wrapped("hello");
+  // Return value passes through untouched.
+  assert.deepEqual(result, { ok: true, echo: "hello" });
+
+  // Exactly one sample, correct name/labels, numeric non-negative duration.
+  assert.equal(calls.length, 1);
+  assert.equal(calls[0].name, "mcp_tool_duration_seconds");
+  assert.deepEqual(calls[0].labels, { tool: "get_page" });
+  assert.equal(typeof calls[0].value, "number");
+  assert.ok(calls[0].value >= 0, "duration must be non-negative seconds");
+});
+
+test("standalone (no onMetric) is a transparent pass-through", async () => {
+  const handler = async (a, b) => a + b;
+  const wrapped = timeToolHandler("noop_tool", handler, undefined);
+
+  // No throw, result unchanged, and nothing to observe.
+  const result = await wrapped(2, 3);
+  assert.equal(result, 5);
+});
+
+test("still observes on throw, then rethrows the original error", async () => {
+  const calls = [];
+  const onMetric = (name, value, labels) => calls.push({ name, value, labels });
+
+  const boom = new Error("handler failed");
+  const handler = async () => {
+    throw boom;
+  };
+  const wrapped = timeToolHandler("boom_tool", handler, onMetric);
+
+  await assert.rejects(() => wrapped(), (err) => err === boom);
+
+  // Observed exactly once despite the throw.
+  assert.equal(calls.length, 1);
+  assert.equal(calls[0].name, "mcp_tool_duration_seconds");
+  assert.deepEqual(calls[0].labels, { tool: "boom_tool" });
+});
+
+test("standalone still rethrows the original error (no swallow, no observe)", async () => {
+  const boom = new Error("standalone failure");
+  const wrapped = timeToolHandler(
+    "boom_standalone",
+    async () => {
+      throw boom;
+    },
+    undefined,
+  );
+  await assert.rejects(() => wrapped(), (err) => err === boom);
+});