fix(core): F2 cleanup PR B — self-heal observability (W133-a + W134) (QwenLM#4460)

doudouOUC · web-flow · commit 0c043093907f · 2026-05-23T23:23:30.000+08:00
* fix(core): F2 cleanup PR B — self-heal observability (W133-a + W134) W93 declined as already satisfied by W1 fix in QwenLM#4336 commit 6 (spawnEntry's catch already calls forceShutdown which runs the full cleanup table — listener removal, timer clear, subscriber detach, sweep+disconnect, onClosed eviction). Source-verified non-repro. W133-a: McpClient.onerror now captures the error in a private `lastTransportError` field (reset at each connect()); the W120 silent-drop block at mcp-pool-entry.ts:346 reads it via the new `getLastTransportError()` getter and appends `: <error.message>` to the lastError string on the emitted 'failed' event. Preserves the literal "silent transport drop" prefix invariant for log-grep backward compat — pre-fix marker stays a substring. W134: sweepAndDisconnect now returns SweepResult instead of void — { pidSweepError?, disconnectError?, descendantsFound?, descendantsSignaled? }. The silent-drop fire-and-forget caller chains to inspect the result and emits a structured warn log when either pid-sweep threw OR sigtermPids partially signaled (signaled < found) — surfaces orphan-process pressure without inflating PR scope (no new SSE event or SDK reducer state; deferred to W134-followup if maintainers want metrics). forceShutdown / doRestart sweep callers ignore the return value (JS implicit-void at await sites preserves behavior). 4 new tests in mcp-transport-pool.test.ts covering W133-a happy path + fallback (no prior onerror) + W134 pidSweepError + W134 partial-signal failure modes. Module-mocks pid-descendants.js for controllable sweep behavior, and debugLogger.js to observe warn calls (production logger is session-gated and a no-op in tests). Singleton-stub debugLogger mock so production module-load `createDebugLogger('McpPool:Entry')` and the test's retrieval get the same vi.fn instances. Verification: - tsc clean: packages/core, packages/cli (server.ts pre-existing errors unchanged) - F2 transport-pool: 32/32 pass (28 pre-existing + 4 new) - mcp-client: 46/46 pass - eslint --max-warnings 0 clean on 3 touched files Part of QwenLM#4175 QwenLM#4336 follow-up bucket. * fix(core): QwenLM#4460 round 1 fold-in — 4 copilot doc/comment threads adopted T1 [copilot mcp-pool-entry.ts:116 — stale line ref in SweepResult JSDoc]: replaced `mcp-pool-entry.ts:383` with stable method-anchor reference to the W120 silent-drop block inside `statusChangeListener`. Line numbers drift on every edit; method names don't. T2 [copilot mcp-pool-entry.ts:453 — `?? 0` ambiguous in warn payload]: silent-drop warn log now prints `descendantsFound=unknown` and `descendantsSignaled=unknown` when the values are undefined (only reachable in the pidSweepError branch — sweep threw before assignment). Operators triaging the warn can now distinguish "sweep succeeded but found 0 descendants" from "sweep itself threw, count is genuinely unmeasured". Locked in via a new assertion in the W134 pidSweepError test. T3 [copilot mcp-client.ts:116 — brittle line refs in lastTransportError JSDoc]: replaced `mcp-pool-entry.ts:346` and `mcp-client.ts:130` with stable method/block names (the `statusChangeListener` silent- drop block; the `client.onerror` arrow inside connect()). Same fix applied to the parallel comment in mcp-transport-pool.test.ts:730 for consistency. T4 [copilot mcp-transport-pool.test.ts:797 — singleton-stub mock comment contradictory]: rewrote the comment to unambiguously describe what the mock DOES (factory body runs once; inner arrow returns the same object on every call) instead of the prior hypothetical phrasing ("Returning a fresh object would have...") which read as a description of current behavior at first glance. All 4 are doc/comment fixes — zero behavior change apart from the T2 string format ('unknown' instead of '0'). Verified: - 32/32 mcp-transport-pool.test.ts pass - tsc clean on packages/core - eslint --max-warnings 0 clean on 3 touched files * fix(core): QwenLM#4460 round 2 fold-in — remove dead SweepResult.disconnectError field T5 [wenshao mcp-pool-entry.ts:134 — `disconnectError` is dead data]: glm-5.1 review caught that the field was populated when `client.disconnect()` threw (line 844) but no consumer ever read it — the silent-drop `.then()` handler gated only on `pidSweepError` and partial-signal; `forceShutdown` and `doRestart` ignore the return; no test asserted on it. Removed the field from `SweepResult` and the assignment in the disconnect catch. The pre-existing `debugLogger.error(`client.disconnect failed for ...`)` inside `sweepAndDisconnect` already gives operators the signal — adding it to the outer silent-drop warn would have been duplicate noise. If a future consumer needs to gate logic on disconnect failures, re-add the field + reader at that point. Verification: 32/32 mcp-transport-pool.test.ts pass; tsc + eslint clean on the touched file.
diff --git a/packages/core/src/tools/mcp-client.ts b/packages/core/src/tools/mcp-client.ts
@@ -102,6 +102,21 @@ export class McpClient {
   private transport: Transport | undefined;
   private status: MCPServerStatus = MCPServerStatus.DISCONNECTED;
   private isDisconnecting = false;
+  /**
+   * F2 (#4175 follow-up — W133-a): captures the most recent error
+   * delivered to the SDK Client's `onerror` callback. The pool entry's
+   * W120 silent-drop block (the DISCONNECTED-on-active branch inside
+   * `PoolEntry.statusChangeListener`) reads this via
+   * `getLastTransportError()` to thread the upstream cause (EPIPE,
+   * OAuth 401, server crash) into the `'failed'` event's `lastError`
+   * string instead of emitting only the synthetic
+   * `'transport disconnected (silent transport drop)'` marker. Reset
+   * at the top of `connect()` so a successful reconnect clears stale
+   * state. No reset on `disconnect()` — McpClient instances are GC'd
+   * at pool entry teardown; field staleness has no observable
+   * consumer post-disconnect.
+   */
+  private lastTransportError?: Error;
 
   constructor(
     private readonly serverName: string,
@@ -123,6 +138,12 @@ export class McpClient {
    */
   async connect(): Promise<void> {
     this.isDisconnecting = false;
+    // F2 (#4175 follow-up — W133-a): clear stale upstream error from
+    // any prior connect/disconnect cycle. The W120 silent-drop reader
+    // is otherwise satisfied by `undefined` and falls back to the
+    // synthetic marker — but a stale error from a previous incarnation
+    // would mis-attribute a fresh transport drop to an old cause.
+    this.lastTransportError = undefined;
     this.updateStatus(MCPServerStatus.CONNECTING);
     try {
       this.transport = await this.createTransport();
@@ -131,6 +152,13 @@ export class McpClient {
         if (this.isDisconnecting) {
           return;
         }
+        // F2 (#4175 follow-up — W133-a): capture the upstream error
+        // BEFORE the synchronous `updateStatus(DISCONNECTED)` cascades
+        // to PoolEntry's statusChangeListener. The listener's W120
+        // silent-drop block reads `lastTransportError` inline; setting
+        // it ahead of `updateStatus` guarantees the field is populated
+        // by the time the listener fires.
+        this.lastTransportError = error;
         debugLogger.error(`MCP ERROR (${this.serverName}):`, error.toString());
         this.updateStatus(MCPServerStatus.DISCONNECTED);
       };
@@ -304,6 +332,20 @@ export class McpClient {
     return t.pid;
   }
 
+  /**
+   * F2 (#4175 follow-up — W133-a): expose the most recent SDK Client
+   * `onerror` payload so PoolEntry's W120 silent-drop block can thread
+   * the upstream cause (EPIPE, OAuth 401, server-side crash) into the
+   * `'failed'` event's `lastError` string. Returns `undefined` if no
+   * error has been observed since the last `connect()`. Caller falls
+   * back to the synthetic marker on `undefined`. Population site: the
+   * `client.onerror` arrow inside `connect()` (this file). Consumer:
+   * the W120 silent-drop block inside `PoolEntry.statusChangeListener`.
+   */
+  getLastTransportError(): Error | undefined {
+    return this.lastTransportError;
+  }
+
   async readResource(
     uri: string,
     options?: { signal?: AbortSignal },
diff --git a/packages/core/src/tools/mcp-pool-entry.ts b/packages/core/src/tools/mcp-pool-entry.ts
@@ -107,6 +107,35 @@ export interface PooledConnection {
   release(): void;
 }
 
+/**
+ * F2 (#4175 follow-up — W134): structured outcome of
+ * `PoolEntry.sweepAndDisconnect`. The silent-drop fire-and-forget
+ * caller (the W120 silent-drop block inside `statusChangeListener`)
+ * reads this off the chained promise to surface orphan-process
+ * pressure to operators via a structured `warn` log. `forceShutdown`
+ * and `doRestart` callers ignore the return — their own catch paths
+ * carry richer error signals already.
+ *
+ * Both `descendantsFound` and `descendantsSignaled` are tracked
+ * because partial-signal failure (`signaled < found`) is itself
+ * orphan-process-pressure evidence even when the sweep itself does
+ * NOT throw — e.g. a child exited between `listDescendantPids` and
+ * `sigtermPids`, OR EPERM on a child the daemon doesn't own.
+ *
+ * Internal — NOT exported. The sweep result never crosses the wire
+ * (pool events stay shape-compatible per `PoolEvent` union); this
+ * type only exists to give the in-process caller something to chain
+ * a log decision on.
+ */
+interface SweepResult {
+  /** Set when `listDescendantPids` itself threw (sandbox blocking pgrep, ESRCH on root, etc.). */
+  pidSweepError?: Error;
+  /** Number of descendant pids `listDescendantPids` returned. Undefined if root pid unavailable or sweep threw. */
+  descendantsFound?: number;
+  /** Number of descendant pids `sigtermPids` successfully signaled. May be < `descendantsFound`. */
+  descendantsSignaled?: number;
+}
+
 /**
  * Internal pool-entry record. Created once per `ConnectionId`,
  * holds the shared `McpClient` + its tool/prompt snapshots + ref
@@ -343,11 +372,25 @@ export class PoolEntry {
         // 'failed' event and can route any pending callTool promises
         // to MCPCallInterruptedError. Mirrors forceShutdown's
         // emit→detach ordering at line 583-593.
+        //
+        // F2 (#4175 follow-up — W133-a): thread the upstream
+        // McpClient.onerror cause (EPIPE, OAuth 401, server crash)
+        // into `lastError` instead of emitting only the synthetic
+        // marker. Pre-fix the only diagnostic carrier was the synthetic
+        // string; operators triaging a 'failed' event had to grep
+        // daemon `--debug` logs for the matching `MCP ERROR (...)` line
+        // out of band. Now the actual error message is on the wire.
+        // Preserves the literal `"silent transport drop"` substring so
+        // any operator log-grep tooling that targets the pre-fix marker
+        // keeps matching post-fix.
+        const upstreamError = this.client.getLastTransportError();
         this.emit({
           kind: 'failed',
           serverName: this.serverName,
           generation: this._generation,
-          lastError: 'transport disconnected (silent transport drop)',
+          lastError: upstreamError
+            ? `transport disconnected (silent transport drop): ${upstreamError.message}`
+            : 'transport disconnected (silent transport drop)',
         });
         // Detach all subscriber views (W122 cleanup). Snapshot keys
         // because detach mutates `subscribers`.
@@ -381,7 +424,48 @@ export class PoolEntry {
         // Errors inside the chain log at warn/error via
         // `sweepAndDisconnect`'s own catches.
         void this.sweepAndDisconnect('silent_drop').then(
-          () => {
+          (result) => {
+            // F2 (#4175 follow-up — W134): surface orphan-process
+            // pressure to operators. Two failure shapes worth a
+            // structured `warn` here:
+            //   (a) `pidSweepError`: pid-discovery itself threw
+            //       (pgrep blocked by sandbox, ESRCH at root pid,
+            //       etc.). We may have leaked descendants we never
+            //       enumerated.
+            //   (b) Partial signal: discovery succeeded but
+            //       `sigtermPids` killed fewer than discovered
+            //       (some children already exited between listing
+            //       and signaling, OR EPERM on a child the daemon
+            //       doesn't own). Less alarming than (a) but still
+            //       worth surfacing during silent drops.
+            // Pre-fix `void` discarded both signals; the only
+            // observability path was tailing `--debug warn+` for
+            // the inner `sweepAndDisconnect` log line out of band.
+            const partialSignal =
+              result.descendantsFound !== undefined &&
+              result.descendantsSignaled !== undefined &&
+              result.descendantsSignaled < result.descendantsFound;
+            if (result.pidSweepError !== undefined || partialSignal) {
+              // F2 (#4175 follow-up — copilot review T2 on #4460):
+              // log `'unknown'` instead of `0` when the count fields are
+              // undefined. They are undefined ONLY in the
+              // `pidSweepError` branch (the throw happened before
+              // assignment); operators triaging the warn should be able
+              // to distinguish "0 found" (sweep succeeded, no children
+              // — unusual but possible if grandchildren already exited)
+              // from "not measured" (sweep itself threw, count is
+              // genuinely unknown). Logging `0` for both was factually
+              // ambiguous.
+              debugLogger.warn(
+                `PoolEntry ${this.id} silent-drop sweep observability: ` +
+                  `descendantsFound=${result.descendantsFound ?? 'unknown'}, ` +
+                  `descendantsSignaled=${result.descendantsSignaled ?? 'unknown'}, ` +
+                  `pidSweepError=${result.pidSweepError?.message ?? 'none'}. ` +
+                  `Possible orphan-process pressure — operator should ` +
+                  `check for lingering subprocess descendants of the dead ` +
+                  `transport.`,
+              );
+            }
             this.updateGlobalStatus();
           },
           () => {
@@ -716,21 +800,36 @@ export class PoolEntry {
    * usually indicates a transport bug worth surfacing). Pre-W35
    * `doRestart` had logged both at `debug` — production
    * observability gap that masked PID exhaustion.
+   *
+   * F2 (#4175 follow-up — W134): now returns a `SweepResult` so the
+   * silent-drop fire-and-forget caller (which `void`-discards the
+   * promise and would otherwise lose the orphan-process-pressure
+   * signal entirely) can chain a structured warn log when either pid
+   * sweep threw or `sigtermPids` partially signaled. The `forceShutdown`
+   * and `doRestart` callers continue to ignore the return value (their
+   * caller-side `await` discards it) — those paths already carry rich
+   * error signals via their own catches and don't need the extra
+   * surface. The internal log lines stay unchanged for backward
+   * compat with existing log-tail tooling.
    */
-  private async sweepAndDisconnect(reason: string): Promise<void> {
+  private async sweepAndDisconnect(reason: string): Promise<SweepResult> {
+    const result: SweepResult = {};
     try {
       const rootPid = this.client.getTransportPid?.();
       if (rootPid !== undefined) {
         const descendants = await listDescendantPids(rootPid);
         if (descendants.length > 0) {
-          const signaled = sigtermPids(descendants);
+          result.descendantsFound = descendants.length;
+          result.descendantsSignaled = sigtermPids(descendants);
           debugLogger.debug(
-            `Sent SIGTERM to ${signaled}/${descendants.length} descendants ` +
+            `Sent SIGTERM to ${result.descendantsSignaled}/${descendants.length} descendants ` +
               `of pid ${rootPid} for ${this.id} (${reason})`,
           );
         }
       }
     } catch (err) {
+      result.pidSweepError =
+        err instanceof Error ? err : new Error(String(err));
       debugLogger.warn(
         `Descendant pid sweep failed for ${this.id} (${reason}): ${String(
           err,
@@ -740,10 +839,19 @@ export class PoolEntry {
     try {
       await this.client.disconnect();
     } catch (err) {
+      // Disconnect failure is rare (usually a transport bug worth
+      // surfacing) and gets a structured error log here. No
+      // SweepResult field captures this — the silent-drop chain
+      // doesn't gate the outer warn on it (the inner error log
+      // already gives operators the signal), and forceShutdown /
+      // doRestart callers ignore the return entirely. Per wenshao
+      // review #4460: was previously stored on `SweepResult.disconnectError`
+      // but had no reader — removed as dead data.
       debugLogger.error(
         `client.disconnect failed for ${this.id} (${reason}): ${String(err)}`,
       );
     }
+    return result;
   }
 
   /**
diff --git a/packages/core/src/tools/mcp-transport-pool.test.ts b/packages/core/src/tools/mcp-transport-pool.test.ts
