From 45bc55ec3016ac8d886c139cc50cc40a22e12698 Mon Sep 17 00:00:00 2001 From: JF Date: Fri, 21 Aug 2026 17:30:27 -0400 Subject: [PATCH] fix(attach): generous verify window + paused-stack readiness (load-flaky attach e2e) Two attach-mode e2e tests failed under full-suite CPU load (parallel unit project alongside the serial e2e project) while passing in isolation. Both root causes were general contract gaps, fixed in the implementation rather than by conforming the tests: - attachVerifyTimeoutMs default 5s -> 20s. The verify deadline only ever bites when the adapter is alive but the target is slow to report threads (js-debug child adoption on a loaded host, warming JVM) - adapter death still fails fast via the proxyGone latch, and the poll exits as soon as threads appear. A false "attach failed" on a healthy target is strictly worse for an agent than a slower genuine failure. verifyTimeout knob unchanged; the failure-by-design probe in comprehensive-mcp-tools now passes a small verifyTimeout explicitly. - Paused => usable stack on the agent-facing get_stack_trace path (ensureStackReady): an empty-but-successful stackTrace while PAUSED is nearly always a transient race (netcoredbg materializes the managed stack a beat after the post-attach pause stop - the milder sibling of #353's 0x80131302) or a frameless runtime thread tracked as current. Bounded retry (~3s, exits on first frame), fallback scan of the other stopped threads (adopts the frame-bearing one so scopes/evaluate anchor to it), honest empty + explanatory note as last resort. Not-paused and no-known-thread empty results now carry a note too. Internal callers keep the single-shot behavior. The two e2e tests keep their strict semantics (assertion diagnostics only). Follow-ups filed: #396 (threads response clobbers currentThreadId), #397 (tool:response logs success:true for failed attach). Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 6 + docs/patterns/error-handling.md | 10 +- skills/debugging/references/java.md | 2 +- skills/debugging/references/javascript.md | 2 +- skills/debugging/references/python.md | 2 +- src/server.ts | 28 ++- src/session/session-manager-data.ts | 218 ++++++++++++++---- src/session/session-manager-operations.ts | 15 +- src/utils/error-messages.ts | 2 +- .../unit/session/session-manager-dap.test.ts | 133 +++++++++++ tests/e2e/comprehensive-mcp-tools.test.ts | 3 + .../mcp-server-smoke-dotnet-attach.test.ts | 4 +- .../mcp-server-smoke-js-function-bp.test.ts | 2 +- tests/test-utils/mocks/mock-proxy-manager.ts | 4 + tests/unit/server-coverage.test.ts | 2 +- 15 files changed, 367 insertions(+), 66 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 77118917..fc2b38fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed +- **Attach verification window default raised 5s → 20s** — the `attach_to_process` thread-poll deadline only ever bites when the debug adapter is alive but the target is slow to become debuggable (js-debug child-session adoption on a loaded host, a warming JVM); adapter death still fails fast via the proxy-gone latch, and the poll returns the moment threads appear, so healthy attaches never pay for the headroom. A false "attach failed" on a healthy target is strictly worse for an agent than a slower genuine failure. Pass a small `verifyTimeout` for fast failure-by-design probes + +### Fixed +- **A paused session can no longer answer `get_stack_trace` with a silent empty success** — some adapters report a stop before the stack is materialized (netcoredbg right after the post-attach pause — the milder sibling of #353's `0x80131302`), and the tracked thread can be a frameless runtime thread. The agent-facing stack path now retries an empty-but-successful stackTrace within a bounded window (~3s, exits on first frame), falls back to scanning the other stopped threads and adopts the first one with frames (annotated via a `note` in the response, so scopes/evaluate anchor correctly), and — if everything stays frameless — returns the empty result with a `note` saying what to try instead. Not-paused and no-known-thread empty results now carry an explanatory `note` too + ### Added - **CodeLLDB ships as per-platform npm packages (esbuild pattern)** — five new packages `@debugmcp/codelldb-{win32-x64,darwin-x64,darwin-arm64,linux-x64,linux-arm64}` (versioned by the CodeLLDB release, currently 1.11.8, payload staged from the digest-pinned VSIXs) are `optionalDependencies` of `@debugmcp/mcp-debugger`, so npm installs exactly the one matching your os/cpu. Rust and C/C++ debugging now work out of the box on every platform npm serves — previously the CLI tarball bundled linux-x64 only — and the core tarball shrinks from ~54 MB to a few MB. The resolver probes the installed platform package last — after the vendor tree and after `CODELLDB_PATH` — so an explicit `CODELLDB_PATH` still overrides the auto-installed package, and installs with `--omit=optional` keep working via `CODELLDB_PATH` (#383) diff --git a/docs/patterns/error-handling.md b/docs/patterns/error-handling.md index 10ac8e39..abf43999 100644 --- a/docs/patterns/error-handling.md +++ b/docs/patterns/error-handling.md @@ -236,11 +236,15 @@ target (e.g. a busy or warming JVM) can legitimately need longer than the default window, the window is caller-configurable (issue #143): - The default lives in the protected, test-shrinkable field - `attachVerifyTimeoutMs` (5s), following the `stepGraceMs`/`pauseGraceMs` - field pattern. + `attachVerifyTimeoutMs` (20s), following the `stepGraceMs`/`pauseGraceMs` + field pattern. It is deliberately generous: adapter death fails fast + regardless (the proxy-gone latch), so the deadline only bites when the + adapter is alive but the target is slow to report threads — where a false + "attach failed" is worse than a slow genuine failure. - Callers override it per attach via the `verifyTimeout` (ms) argument on `attach_to_process` / `create_debug_session`; the value is validated - (positive finite number) and clamped to 10 minutes. + (positive finite number) and clamped to 10 minutes. Pass a small value for + fast failure-by-design probes. - The failure text comes from `ErrorMessages.attachVerifyFailed(timeoutMs, lastFailure)`, which names the `verifyTimeout` knob so a caller that hit the window on a slow target knows how to retry. diff --git a/skills/debugging/references/java.md b/skills/debugging/references/java.md index 6f24e97a..bb1b7acc 100644 --- a/skills/debugging/references/java.md +++ b/skills/debugging/references/java.md @@ -50,7 +50,7 @@ continue_execution {"sessionId": ""} // required with suspend=y to let ``` - **Deferred breakpoints work natively:** for classes not yet loaded, the JDI bridge registers a `ClassPrepareRequest` and binds the breakpoint when the JVM loads the class, then reports `verified: true`. Set breakpoints freely before or after attach — no re-sends needed. -- For a busy or warming JVM, raise `verifyTimeout` (ms) on `attach_to_process` — attach fails if no thread is reported within ~5 s by default. Attach sessions skip host-side file checks, so remote paths are fine. +- For a busy or warming JVM, raise `verifyTimeout` (ms) on `attach_to_process` — attach fails if no thread is reported within ~20 s by default. Attach sessions skip host-side file checks, so remote paths are fine. ## Quirks diff --git a/skills/debugging/references/javascript.md b/skills/debugging/references/javascript.md index 8201d3e7..da229313 100644 --- a/skills/debugging/references/javascript.md +++ b/skills/debugging/references/javascript.md @@ -53,7 +53,7 @@ set_breakpoint { "sessionId": "", "file": "/abs/path/server.js", "line continue_execution { "sessionId": "" } ``` -Shorthand: `create_debug_session { "language": "javascript", "host": "127.0.0.1", "port": 9229 }` attaches in one call. Attach is verified by polling for threads (`verifyTimeout`, default ~5000 ms). Detach with `detach_from_process { "sessionId": "", "terminateProcess": false }`; remote debugging beyond a reachable host/port requires manual configuration (e.g. an SSH tunnel). +Shorthand: `create_debug_session { "language": "javascript", "host": "127.0.0.1", "port": 9229 }` attaches in one call. Attach is verified by polling for threads (`verifyTimeout`, default ~20000 ms). Detach with `detach_from_process { "sessionId": "", "terminateProcess": false }`; remote debugging beyond a reachable host/port requires manual configuration (e.g. an SSH tunnel). ## Quirks diff --git a/skills/debugging/references/python.md b/skills/debugging/references/python.md index 7f945057..65b3bb6a 100644 --- a/skills/debugging/references/python.md +++ b/skills/debugging/references/python.md @@ -49,7 +49,7 @@ set_breakpoint { "sessionId": "", "file": "/abs/path/script.py", "line continue_execution { "sessionId": "" } ``` -Shorthand: `create_debug_session { "language": "python", "host": "127.0.0.1", "port": 5678 }` creates the session and attaches in one call. After the handshake the attach is verified by polling for threads; raise `verifyTimeout` (default ~5000 ms) for slow targets. Detach with `detach_from_process { "sessionId": "", "terminateProcess": false }`. +Shorthand: `create_debug_session { "language": "python", "host": "127.0.0.1", "port": 5678 }` creates the session and attaches in one call. After the handshake the attach is verified by polling for threads; raise `verifyTimeout` (default ~20000 ms) for slow targets. Detach with `detach_from_process { "sessionId": "", "terminateProcess": false }`. ## Quirks diff --git a/src/server.ts b/src/server.ts index 43707235..2066f257 100644 --- a/src/server.ts +++ b/src/server.ts @@ -872,7 +872,11 @@ export class DebugMcpServer { if (typeof currentThreadId !== 'number') { throw new ProxyNotRunningError(sessionId || 'unknown', 'get stack trace'); } - return this.sessionManager.getStackTraceDetailed(sessionId, currentThreadId, includeInternals); + // ensureStackReady: the thread above was resolved implicitly (the MCP tool + // has no threadId argument), so a paused session answering with zero + // frames gets the bounded readiness retry + thread scan instead of a + // confusing empty success. + return this.sessionManager.getStackTraceDetailed(sessionId, currentThreadId, includeInternals, { ensureStackReady: true }); } public async getScopes(sessionId: string, frameId: number): Promise { @@ -1080,7 +1084,7 @@ export class DebugMcpServer { return { tools: [ - { name: 'create_debug_session', description: 'Create a new debugging session. Provide host and port to attach to a running process; omit them for launch mode', inputSchema: { type: 'object', properties: { language: { type: 'string', enum: supportedLanguages, description: 'Programming language for debugging' }, name: { type: 'string', description: 'Optional session name' }, executablePath: {type: 'string', description: 'Path to language executable (optional, will auto-detect if not provided)'}, host: { type: 'string', description: 'Host to attach to for remote debugging (optional, triggers attach mode)' }, port: { type: 'number', description: 'Debug port to attach to for remote debugging (optional, triggers attach mode)' }, timeout: { type: 'number', description: 'Connection timeout in milliseconds for attach mode (default: 30000)' }, verifyTimeout: { type: 'number', description: 'Attach mode only: how long to wait (ms) for the debugger to report at least one thread after attaching before failing the attach (default: 5000, max: 600000)' }, adapterConfig: { type: 'object', description: 'Attach mode only: adapter-specific attach configuration merged into the attach config (see attach_to_process)', additionalProperties: true } }, required: ['language'] } }, + { name: 'create_debug_session', description: 'Create a new debugging session. Provide host and port to attach to a running process; omit them for launch mode', inputSchema: { type: 'object', properties: { language: { type: 'string', enum: supportedLanguages, description: 'Programming language for debugging' }, name: { type: 'string', description: 'Optional session name' }, executablePath: {type: 'string', description: 'Path to language executable (optional, will auto-detect if not provided)'}, host: { type: 'string', description: 'Host to attach to for remote debugging (optional, triggers attach mode)' }, port: { type: 'number', description: 'Debug port to attach to for remote debugging (optional, triggers attach mode)' }, timeout: { type: 'number', description: 'Connection timeout in milliseconds for attach mode (default: 30000)' }, verifyTimeout: { type: 'number', description: 'Attach mode only: how long to wait (ms) for the debugger to report at least one thread after attaching before failing the attach (default: 20000, max: 600000)' }, adapterConfig: { type: 'object', description: 'Attach mode only: adapter-specific attach configuration merged into the attach config (see attach_to_process)', additionalProperties: true } }, required: ['language'] } }, { name: 'list_supported_languages', description: 'List all supported debugging languages with metadata', inputSchema: { type: 'object', properties: {} } }, { name: 'list_debug_sessions', description: 'List all active debugging sessions. Paused sessions include lastStop with the reason for the most recent stop (e.g. "breakpoint" vs "exception")', inputSchema: { type: 'object', properties: {} } }, { name: 'set_breakpoint', description: 'Set a breakpoint. Setting breakpoints on non-executable lines (structural, declarative) may lead to unexpected behavior', inputSchema: { type: 'object', properties: { sessionId: { type: 'string' }, file: { type: 'string', description: 'Path to the source file or Java FQCN. For Java, passing a fully-qualified class name (e.g. "com.example.MyClass" or "com.example.Outer$Inner") is preferred — it works reliably with all classloaders including custom classloaders. Alternatively, use absolute file paths.' }, line: { type: 'number', description: 'Line number where to set breakpoint. Executable statements (assignments, function calls, conditionals, returns) work best. Structural lines (function/class definitions), declarative lines (imports), or non-executable lines (comments, blank lines) may cause unexpected stepping behavior' }, ...setBreakpointExtraProps, condition: { type: 'string', description: 'Optional expression: only break (or log) when it evaluates truthy' }, logMessage: { type: 'string', description: 'Create a logpoint: instead of pausing, log this message when the line is hit. Expressions in {curly braces} are interpolated (e.g. "order={orderId} total={total}"). Messages arrive in get_output while the program runs at full speed. Supported by Python, JavaScript, Go, and Rust adapters; not by Java or .NET' }, suspendPolicy: { type: 'string', enum: ['all', 'thread'], description: 'Suspend policy when breakpoint is hit: "all" suspends all threads (default), "thread" only suspends the event thread. Only supported by the Java/JDI adapter.' } }, required: setBreakpointRequired } }, @@ -1113,7 +1117,7 @@ export class DebugMcpServer { } }, { name: 'restart_debugging', description: 'Restart the debuggee: terminate the current program (if still running) and relaunch it with the same configuration as the last start_debugging. All current breakpoints are re-applied automatically. The get_output buffer starts fresh — read from since=0 after a restart. Works while running, paused, or after the program has exited. Not available for attach sessions or sessions never launched', inputSchema: { type: 'object', properties: { sessionId: { type: 'string' } }, required: ['sessionId'] } }, - { name: 'attach_to_process', description: 'Attach to a running process for debugging. After the attach handshake the target is verified by polling for threads; if none are reported within verifyTimeout (~5s default) the attach fails and the debug proxy is torn down', inputSchema: { + { name: 'attach_to_process', description: 'Attach to a running process for debugging. After the attach handshake the target is verified by polling for threads; if none are reported within verifyTimeout (~20s default) the attach fails and the debug proxy is torn down', inputSchema: { type: 'object', properties: { sessionId: { type: 'string', description: 'Debug session ID' }, @@ -1121,7 +1125,7 @@ export class DebugMcpServer { host: { type: 'string', description: 'Host to attach to (default: localhost)' }, processId: { type: ['number', 'string'], description: 'Process ID (for local attach, language-specific)' }, timeout: { type: 'number', description: 'Connection timeout in milliseconds (default: 30000)' }, - verifyTimeout: { type: 'number', description: 'How long to wait (ms) for the debugger to report at least one thread after attaching before failing the attach (default: 5000, max: 600000). Increase for targets that are slow to become debuggable, e.g. a busy or warming JVM' }, + verifyTimeout: { type: 'number', description: 'How long to wait (ms) for the debugger to report at least one thread after attaching before failing the attach (default: 20000, max: 600000). Decrease for fast failure-by-design probes; increase for targets that are exceptionally slow to become debuggable' }, sourcePaths: { type: 'array', items: { type: 'string' }, description: 'Source paths for code mapping' }, stopOnEntry: { type: 'boolean', description: 'Stop on entry after attaching' }, justMyCode: { type: 'boolean', description: 'Only debug user code (skip library code)' }, @@ -2055,13 +2059,21 @@ export class DebugMcpServer { stopReason: lastStop?.reason, lastStop }; - // Issue #346: when the language policy hid frames, say so in the - // response instead of relying on the agent knowing filtering exists. + // Anything the result needs explaining (not paused, stack came + // from a different thread, all threads frameless) plus the + // issue #346 hidden-frames disclosure share the note field. + const notes: string[] = []; + if (stackTrace.note) { + notes.push(stackTrace.note); + } if (stackTrace.hiddenFrameCount > 0) { payload.hiddenFrames = stackTrace.hiddenFrameCount; - payload.note = stackTrace.allFramesInternal + notes.push(stackTrace.allFramesInternal ? `All ${stackTrace.totalFrameCount} frames are internal/runtime frames; showing the top internal frame so scopes and evaluate still work. Pass includeInternals: true to see the full stack.` - : `${stackTrace.hiddenFrameCount} internal frame(s) hidden — pass includeInternals: true to see them.`; + : `${stackTrace.hiddenFrameCount} internal frame(s) hidden — pass includeInternals: true to see them.`); + } + if (notes.length > 0) { + payload.note = notes.join(' '); } result = { content: [{ type: 'text', text: JSON.stringify(payload) }] }; } catch (error) { diff --git a/src/session/session-manager-data.ts b/src/session/session-manager-data.ts index ca82d299..a91ef684 100644 --- a/src/session/session-manager-data.ts +++ b/src/session/session-manager-data.ts @@ -14,6 +14,7 @@ import { redactVariableValue } from '@debugmcp/shared'; import { SessionManagerCore } from './session-manager-core.js'; +import { IProxyManager } from '../proxy/proxy-manager.js'; import { DebugProtocol } from '@vscode/debugprotocol'; import { applyVariableCaps, @@ -32,16 +33,36 @@ export interface StackTraceResult { totalFrameCount: number; hiddenFrameCount: number; allFramesInternal: boolean; + /** + * Present when the result needs explaining: the session was not paused, no + * stopped thread was known, the stack came from a different thread than the + * tracked one, or every thread stayed frameless. Surfaced to the agent in + * the get_stack_trace tool payload. + */ + note?: string; } -function emptyStackTraceResult(): StackTraceResult { - return { frames: [], totalFrameCount: 0, hiddenFrameCount: 0, allFramesInternal: false }; +function emptyStackTraceResult(note?: string): StackTraceResult { + return { frames: [], totalFrameCount: 0, hiddenFrameCount: 0, allFramesInternal: false, ...(note ? { note } : {}) }; } /** * Data retrieval functionality for session management */ export abstract class SessionManagerData extends SessionManagerCore { + /** + * How long the agent-facing stack-trace path keeps polling a PAUSED session + * whose adapter answers stackTrace with success + zero frames, before + * returning the honest empty result. Some adapters report the stop before + * the stack is materialized (netcoredbg after the post-attach pause — the + * milder sibling of issue #353's 0x80131302), so success-with-0-frames + * while paused is nearly always a transient race, not truth. The poll exits + * on the first non-empty answer, so the window costs nothing when the + * adapter is ready. Follows the attachVerifyTimeoutMs pattern. + */ + protected pausedStackReadyTimeoutMs = 3000; + protected pausedStackReadyIntervalMs = 250; + /** * Selects the appropriate adapter policy based on language */ @@ -167,75 +188,184 @@ export abstract class SessionManagerData extends SessionManagerCore { * has a frameId to anchor scopes/evaluate, and `hiddenFrameCount` + * `allFramesInternal` let the response say what was hidden. */ - async getStackTraceDetailed(sessionId: string, threadId?: number, includeInternals: boolean = false): Promise { + async getStackTraceDetailed( + sessionId: string, + threadId?: number, + includeInternals: boolean = false, + opts?: { ensureStackReady?: boolean } + ): Promise { const session = this._getSessionById(sessionId); const currentThreadId = session.proxyManager?.getCurrentThreadId(); this.logger.info(`[SM getStackTrace ${sessionId}] Entered. Requested threadId: ${threadId}, Current state: ${session.state}, Actual currentThreadId: ${currentThreadId}, includeInternals: ${includeInternals}`); - + if (!session.proxyManager || !session.proxyManager.isRunning()) { this.logger.warn(`[SM getStackTrace ${sessionId}] No active proxy.`); - return emptyStackTraceResult(); + return emptyStackTraceResult('No active debug process for this session.'); } if (session.state !== SessionState.PAUSED) { this.logger.warn(`[SM getStackTrace ${sessionId}] Session not paused. State: ${session.state}.`); - return emptyStackTraceResult(); + return emptyStackTraceResult(`Session is not paused (state: ${session.state}); stack traces are only available while paused.`); } const currentThreadForRequest = threadId || currentThreadId; if (typeof currentThreadForRequest !== 'number') { this.logger.warn(`[SM getStackTrace ${sessionId}] No effective thread ID to use.`); - return emptyStackTraceResult(); + return emptyStackTraceResult('No stopped thread is known for this session.'); } + const proxyManager = session.proxyManager; try { - this.logger.info(`[SM getStackTrace ${sessionId}] Sending DAP 'stackTrace' for threadId ${currentThreadForRequest}.`); - const response = await session.proxyManager.sendDapRequest('stackTrace', { threadId: currentThreadForRequest }); - this.logger.info(`[SM getStackTrace ${sessionId}] DAP 'stackTrace' response received. Body:`, response?.body); + let rawFrames = await this.requestRawStackFrames(sessionId, proxyManager, currentThreadForRequest); + let note: string | undefined; - // A failed DAP response (e.g. "Child session not ready ...") must not - // be flattened into an empty-but-successful stack trace (issue #124): - // propagate the failure to the caller. - if (response?.success === false) { - throw new Error(response.message || `DAP 'stackTrace' request failed`); + // A PAUSED session answering with zero frames is nearly always a + // transient adapter race (netcoredbg materializes the managed stack a + // beat after the stop event) or a frameless runtime thread being + // tracked as current. On the agent-facing path, chase the real stack + // instead of handing back a confusing empty success. + if (rawFrames.length === 0 && opts?.ensureStackReady) { + const ready = await this.waitForReadyStack(sessionId, session, proxyManager, currentThreadForRequest); + rawFrames = ready.frames; + note = ready.note; } - if (response && response.body && response.body.stackFrames) { - let frames: StackFrame[] = response.body.stackFrames.map((sf: DebugProtocol.StackFrame) => ({ - id: sf.id, name: sf.name, - file: sf.source?.path || sf.source?.name || "", - line: sf.line, column: sf.column - })); - - // Apply filtering using the language's policy - const totalFrameCount = frames.length; - let allFramesInternal = false; - const policy = this.selectPolicy(session.language); - if (policy.filterStackFrames) { - this.logger.info(`[SM getStackTrace ${sessionId}] Applying stack frame filtering for ${session.language}. Original count: ${frames.length}`); - const filtered = policy.filterStackFrames(frames, includeInternals); - // Central guarantee (issue #346): a policy filter must never leave the - // agent with zero frames when the adapter reported some — keep the top - // unfiltered frame so scopes/evaluate still have an anchor. - if (filtered.length === 0 && frames.length > 0) { - allFramesInternal = true; - frames = [frames[0]]; - } else { - frames = filtered; - } - this.logger.info(`[SM getStackTrace ${sessionId}] After filtering: ${frames.length} frames (hidden: ${totalFrameCount - frames.length}, allFramesInternal: ${allFramesInternal})`); - } + let frames: StackFrame[] = rawFrames.map((sf: DebugProtocol.StackFrame) => ({ + id: sf.id, name: sf.name, + file: sf.source?.path || sf.source?.name || "", + line: sf.line, column: sf.column + })); - this.logger.info(`[SM getStackTrace ${sessionId}] Parsed stack frames (top 3):`, frames.slice(0,3).map(f => ({name:f.name, file:f.file, line:f.line}))); - return { frames, totalFrameCount, hiddenFrameCount: totalFrameCount - frames.length, allFramesInternal }; + // Apply filtering using the language's policy + const totalFrameCount = frames.length; + let allFramesInternal = false; + const policy = this.selectPolicy(session.language); + if (policy.filterStackFrames) { + this.logger.info(`[SM getStackTrace ${sessionId}] Applying stack frame filtering for ${session.language}. Original count: ${frames.length}`); + const filtered = policy.filterStackFrames(frames, includeInternals); + // Central guarantee (issue #346): a policy filter must never leave the + // agent with zero frames when the adapter reported some — keep the top + // unfiltered frame so scopes/evaluate still have an anchor. + if (filtered.length === 0 && frames.length > 0) { + allFramesInternal = true; + frames = [frames[0]]; + } else { + frames = filtered; + } + this.logger.info(`[SM getStackTrace ${sessionId}] After filtering: ${frames.length} frames (hidden: ${totalFrameCount - frames.length}, allFramesInternal: ${allFramesInternal})`); } - this.logger.warn(`[SM getStackTrace ${sessionId}] No stackFrames in response body. Response:`, response); - throw new Error(`DAP 'stackTrace' response did not include stack frames`); + + this.logger.info(`[SM getStackTrace ${sessionId}] Parsed stack frames (top 3):`, frames.slice(0,3).map(f => ({name:f.name, file:f.file, line:f.line}))); + return { frames, totalFrameCount, hiddenFrameCount: totalFrameCount - frames.length, allFramesInternal, ...(note ? { note } : {}) }; } catch (error) { this.logger.error(`[SM getStackTrace ${sessionId}] Error getting stack trace:`, error); throw error instanceof Error ? error : new Error(String(error)); } } + /** + * Send one DAP stackTrace request and return the raw frames. A failed DAP + * response (e.g. "Child session not ready ...") must not be flattened into + * an empty-but-successful stack trace (issue #124): propagate the failure. + */ + private async requestRawStackFrames( + sessionId: string, + proxyManager: IProxyManager, + threadId: number + ): Promise { + this.logger.info(`[SM getStackTrace ${sessionId}] Sending DAP 'stackTrace' for threadId ${threadId}.`); + const response = await proxyManager.sendDapRequest('stackTrace', { threadId }); + this.logger.info(`[SM getStackTrace ${sessionId}] DAP 'stackTrace' response received. Body:`, response?.body); + + if (response?.success === false) { + throw new Error(response.message || `DAP 'stackTrace' request failed`); + } + if (!response || !response.body || !response.body.stackFrames) { + this.logger.warn(`[SM getStackTrace ${sessionId}] No stackFrames in response body. Response:`, response); + throw new Error(`DAP 'stackTrace' response did not include stack frames`); + } + return response.body.stackFrames; + } + + /** + * Bounded readiness loop for a PAUSED session whose stackTrace succeeded + * with zero frames: re-poll the same thread (the stack may not be + * materialized yet), and each round also scan the other stopped threads — + * the tracked thread may be a frameless runtime thread (finalizer, JIT) + * while the real stack lives elsewhere. A frame-bearing thread found by the + * scan is adopted as current so scopes/evaluate anchor to it. If nothing + * reports frames within the window, return the honest empty answer with a + * note telling the agent what to try instead. + */ + private async waitForReadyStack( + sessionId: string, + session: { state: SessionState }, + proxyManager: IProxyManager, + threadId: number + ): Promise<{ frames: DebugProtocol.StackFrame[]; note?: string }> { + const deadline = Date.now() + this.pausedStackReadyTimeoutMs; + while (Date.now() < deadline) { + await new Promise(resolve => setTimeout(resolve, this.pausedStackReadyIntervalMs)); + if (session.state !== SessionState.PAUSED) { + return { frames: [], note: 'The session left the paused state while waiting for the stack; it is no longer paused.' }; + } + const frames = await this.requestRawStackFrames(sessionId, proxyManager, threadId); + if (frames.length > 0) { + return { frames }; + } + const scanned = await this.scanThreadsForFrames(sessionId, proxyManager, threadId); + if (scanned) { + proxyManager.setCurrentThreadId(scanned.threadId); + this.logger.info(`[SM getStackTrace ${sessionId}] Thread ${threadId} stayed frameless; adopted thread ${scanned.threadId} which has a stack.`); + return { + frames: scanned.frames, + note: `The stopped thread ${threadId} reported no stack frames; switched to thread ${scanned.threadId}, which has one.` + }; + } + } + this.logger.warn(`[SM getStackTrace ${sessionId}] Stack stayed empty for ${this.pausedStackReadyTimeoutMs}ms while paused (threadId ${threadId}).`); + return { + frames: [], + note: `The stopped thread reported no stack frames within ${this.pausedStackReadyTimeoutMs}ms; the target may be paused in native code. Retry get_stack_trace, or use list_threads to inspect other threads.` + }; + } + + /** + * Probe the other stopped threads for one that reports stack frames. + * Probe failures on individual threads are not fatal — runtime threads may + * reject stackTrace outright. + */ + private async scanThreadsForFrames( + sessionId: string, + proxyManager: IProxyManager, + excludeThreadId: number + ): Promise<{ threadId: number; frames: DebugProtocol.StackFrame[] } | null> { + let threads: DebugProtocol.Thread[] | undefined; + try { + const response = await proxyManager.sendDapRequest('threads', {}); + threads = response?.body?.threads; + } catch (err) { + this.logger.warn(`[SM getStackTrace ${sessionId}] Thread scan could not list threads: ${err instanceof Error ? err.message : String(err)}`); + return null; + } + if (!Array.isArray(threads)) { + return null; + } + for (const thread of threads) { + if (!thread || typeof thread.id !== 'number' || thread.id === excludeThreadId) { + continue; + } + try { + const frames = await this.requestRawStackFrames(sessionId, proxyManager, thread.id); + if (frames.length > 0) { + return { threadId: thread.id, frames }; + } + } catch { + // This thread rejected stackTrace — keep scanning. + } + } + return null; + } + async getScopes(sessionId: string, frameId: number): Promise { const session = this._getSessionById(sessionId); this.logger.info(`[SM getScopes ${sessionId}] Entered. frameId: ${frameId}, Current state: ${session.state}`); diff --git a/src/session/session-manager-operations.ts b/src/session/session-manager-operations.ts index b39cc07a..efa06831 100644 --- a/src/session/session-manager-operations.ts +++ b/src/session/session-manager-operations.ts @@ -100,11 +100,20 @@ export abstract class SessionManagerOperations extends SessionManagerData { * 'threads' is polled until the debugger reports at least one thread. * If the window elapses without any threads, the attach is reported as a * failure instead of a false "paused" success (issue #124). - * Callers can widen the window per attach via the 'verifyTimeout' tool - * argument for targets that are slow to become debuggable (issue #143). + * Callers can adjust the window per attach via the 'verifyTimeout' tool + * argument (issue #143) — smaller for fast failure-by-design probes, larger + * for targets that are exceptionally slow to become debuggable. + * + * The default is deliberately generous: an adapter that dies mid-verify + * fails fast regardless (the proxyGone latch), so the deadline only ever + * bites when the adapter is alive but the target is slow to report threads + * — e.g. js-debug child-session adoption on a heavily loaded host, or a + * warming JVM — where a false "attach failed" is far worse than a slow + * genuine failure. The poll exits as soon as threads appear, so healthy + * attaches never pay for the headroom. * Protected so tests can shrink the window. */ - protected attachVerifyTimeoutMs = 5000; + protected attachVerifyTimeoutMs = 20000; protected attachVerifyIntervalMs = 250; /** diff --git a/src/utils/error-messages.ts b/src/utils/error-messages.ts index 2d224686..1986da0a 100644 --- a/src/utils/error-messages.ts +++ b/src/utils/error-messages.ts @@ -74,7 +74,7 @@ export const ErrorMessages = { * threads within the verification window — either the attach is dead * (issue #124) or the target is slow to become debuggable (issue #143) * Used in: src/session/session-manager-operations.ts - * Default window: 5 seconds, overridable per call via 'verifyTimeout' + * Default window: 20 seconds, overridable per call via 'verifyTimeout' * @param timeoutMs - The verification window in milliseconds * @param lastFailure - The last observed failure while polling 'threads' */ diff --git a/tests/core/unit/session/session-manager-dap.test.ts b/tests/core/unit/session/session-manager-dap.test.ts index 0ef13cc5..01755e90 100644 --- a/tests/core/unit/session/session-manager-dap.test.ts +++ b/tests/core/unit/session/session-manager-dap.test.ts @@ -1252,6 +1252,139 @@ describe('SessionManager - DAP Operations', () => { }); }); + describe('Paused stack readiness (empty-stack hardening)', () => { + // A PAUSED session answering stackTrace with success + zero frames is + // (nearly always) a transient adapter race — netcoredbg does this right + // after the post-attach pause. With ensureStackReady the agent-facing + // path retries within a bounded window and falls back to scanning other + // stopped threads; internal callers keep the single-shot behavior. + const READY_FRAME = { id: 7, name: 'Program.Main', source: { path: '/work/Program.cs' }, line: 12, column: 1 }; + + function setShortReadyWindow() { + (sessionManager as unknown as { pausedStackReadyTimeoutMs: number; pausedStackReadyIntervalMs: number }) + .pausedStackReadyTimeoutMs = 400; + (sessionManager as unknown as { pausedStackReadyIntervalMs: number }) + .pausedStackReadyIntervalMs = 25; + } + + it('retries an empty-but-successful stack until frames appear', async () => { + const session = await createPausedSession(); + setShortReadyWindow(); + let stackTraceCalls = 0; + dependencies.mockProxyManager.setDapRequestHandler(async (command: string) => { + if (command === 'stackTrace') { + stackTraceCalls++; + return { success: true, body: { stackFrames: stackTraceCalls < 3 ? [] : [READY_FRAME] } }; + } + return { success: true, body: {} }; + }); + + const result = await sessionManager.getStackTraceDetailed(session.id, undefined, false, { ensureStackReady: true }); + + expect(result.frames.map(f => f.name)).toEqual(['Program.Main']); + expect(stackTraceCalls).toBeGreaterThanOrEqual(3); + expect(result.note).toBeUndefined(); + }); + + it('falls back to another stopped thread when the current one stays frameless, and annotates', async () => { + const session = await createPausedSession(); // currentThreadId = 1 + setShortReadyWindow(); + dependencies.mockProxyManager.setDapRequestHandler(async (command: string, args?: { threadId?: number }) => { + if (command === 'stackTrace') { + return { success: true, body: { stackFrames: args?.threadId === 2 ? [READY_FRAME] : [] } }; + } + if (command === 'threads') { + return { success: true, body: { threads: [{ id: 1, name: '' }, { id: 2, name: 'worker' }] } }; + } + return { success: true, body: {} }; + }); + + const result = await sessionManager.getStackTraceDetailed(session.id, undefined, false, { ensureStackReady: true }); + + expect(result.frames.map(f => f.name)).toEqual(['Program.Main']); + expect(result.note).toMatch(/thread 2/); + // The frame-bearing thread is adopted so scopes/evaluate anchor to it. + expect(dependencies.mockProxyManager.getCurrentThreadId()).toBe(2); + }); + + it('returns an honest empty result with a note when no thread reports frames', async () => { + const session = await createPausedSession(); + setShortReadyWindow(); + dependencies.mockProxyManager.setDapRequestHandler(async (command: string) => { + if (command === 'stackTrace') { + return { success: true, body: { stackFrames: [] } }; + } + if (command === 'threads') { + return { success: true, body: { threads: [{ id: 1, name: '' }] } }; + } + return { success: true, body: {} }; + }); + + const result = await sessionManager.getStackTraceDetailed(session.id, undefined, false, { ensureStackReady: true }); + + expect(result.frames).toEqual([]); + expect(result.note).toMatch(/no stack frames/i); + }); + + it('does not retry or scan without ensureStackReady (internal-caller behavior unchanged)', async () => { + const session = await createPausedSession(); + dependencies.mockProxyManager.setDapRequestHandler(async (command: string) => { + if (command === 'stackTrace') { + return { success: true, body: { stackFrames: [] } }; + } + return { success: true, body: {} }; + }); + + const result = await sessionManager.getStackTraceDetailed(session.id); + + expect(result.frames).toEqual([]); + expect(dependencies.mockProxyManager.dapRequestCalls.filter(c => c.command === 'stackTrace')).toHaveLength(1); + }); + + // Raw 'continued' events are deliberately ignored while PAUSED (the + // stale-event guard in session-manager-core), so these tests leave the + // paused state the way the continue/step operations do: through + // _updateSessionState. + function forceRunning(sessionId: string) { + const managed = sessionManager.getSession(sessionId); + (sessionManager as unknown as { _updateSessionState: (s: unknown, st: SessionState) => void }) + ._updateSessionState(managed, SessionState.RUNNING); + } + + it('stops retrying when the session leaves PAUSED mid-wait', async () => { + const session = await createPausedSession(); + setShortReadyWindow(); + let stackTraceCalls = 0; + dependencies.mockProxyManager.setDapRequestHandler(async (command: string) => { + if (command === 'stackTrace') { + stackTraceCalls++; + if (stackTraceCalls === 1) { + // A continue races in between readiness polls. + forceRunning(session.id); + } + return { success: true, body: { stackFrames: [] } }; + } + return { success: true, body: {} }; + }); + + const result = await sessionManager.getStackTraceDetailed(session.id, undefined, false, { ensureStackReady: true }); + + expect(result.frames).toEqual([]); + expect(stackTraceCalls).toBe(1); + expect(result.note).toMatch(/paused/i); + }); + + it('annotates the not-paused empty result', async () => { + const session = await createPausedSession(); + forceRunning(session.id); + + const result = await sessionManager.getStackTraceDetailed(session.id); + + expect(result.frames).toEqual([]); + expect(result.note).toMatch(/not paused/i); + }); + }); + describe('Exception Breakpoints and Stop Detail (issue #220)', () => { it('threads breakOnExceptions into the ProxyConfig', async () => { const session = await sessionManager.createSession({ diff --git a/tests/e2e/comprehensive-mcp-tools.test.ts b/tests/e2e/comprehensive-mcp-tools.test.ts index 583fcb81..85ab421e 100644 --- a/tests/e2e/comprehensive-mcp-tools.test.ts +++ b/tests/e2e/comprehensive-mcp-tools.test.ts @@ -748,6 +748,9 @@ describe(`Comprehensive MCP Debugger Test — ${ALL_TOOLS.length} Tools × ${LAN sessionId: currentSessionId, port: 5678, host: 'localhost', + // Nothing is listening — failure is the expected outcome, so opt + // out of the generous default verify window to keep the sweep fast. + verifyTimeout: 2000, }); // Attach will likely fail because no process is actually listening diff --git a/tests/e2e/mcp-server-smoke-dotnet-attach.test.ts b/tests/e2e/mcp-server-smoke-dotnet-attach.test.ts index b3312e36..76393de5 100644 --- a/tests/e2e/mcp-server-smoke-dotnet-attach.test.ts +++ b/tests/e2e/mcp-server-smoke-dotnet-attach.test.ts @@ -173,8 +173,8 @@ describe.skipIf(SKIP_DOTNET)('MCP Server .NET Attach Smoke Test @requires-dotnet name: 'get_stack_trace', arguments: { sessionId } })) as { success?: boolean; stackFrames?: Array<{ name?: string; file?: string }> }; - expect(stackResponse.success).toBe(true); - expect((stackResponse.stackFrames ?? []).length).toBeGreaterThan(0); + expect(stackResponse.success, JSON.stringify(stackResponse)).toBe(true); + expect((stackResponse.stackFrames ?? []).length, JSON.stringify(stackResponse)).toBeGreaterThan(0); // Breakpoints must bind against the attached process ("No symbols have // been loaded" was the second #353 symptom). diff --git a/tests/e2e/mcp-server-smoke-js-function-bp.test.ts b/tests/e2e/mcp-server-smoke-js-function-bp.test.ts index fd5ad89e..39ce2890 100644 --- a/tests/e2e/mcp-server-smoke-js-function-bp.test.ts +++ b/tests/e2e/mcp-server-smoke-js-function-bp.test.ts @@ -359,7 +359,7 @@ describe('MCP Server JavaScript Function Breakpoints', () => { name: 'attach_to_process', arguments: { sessionId: sid, host: '127.0.0.1', port } })); - expect(attachResponse.success).toBe(true); + expect(attachResponse.success, JSON.stringify(attachResponse)).toBe(true); // Ensure the target is running (attach may leave it paused), then set the // function breakpoint while it runs: no pause frames exist, so the bridge diff --git a/tests/test-utils/mocks/mock-proxy-manager.ts b/tests/test-utils/mocks/mock-proxy-manager.ts index 5d4f6a28..88d97d0f 100644 --- a/tests/test-utils/mocks/mock-proxy-manager.ts +++ b/tests/test-utils/mocks/mock-proxy-manager.ts @@ -204,6 +204,10 @@ export class MockProxyManager extends EventEmitter implements IProxyManager { return this._currentThreadId; } + setCurrentThreadId(threadId: number): void { + this._currentThreadId = threadId; + } + // Test helpers setDapRequestHandler(handler: (command: string, args?: any) => Promise): void { this._dapRequestHandler = handler; diff --git a/tests/unit/server-coverage.test.ts b/tests/unit/server-coverage.test.ts index 5fda75e5..5f59c126 100644 --- a/tests/unit/server-coverage.test.ts +++ b/tests/unit/server-coverage.test.ts @@ -181,7 +181,7 @@ describe('Server Coverage - Error Paths and Edge Cases', () => { const result = await server.getStackTrace('test-session'); expect(mockProxy.sendDapRequest).toHaveBeenCalledWith('threads', {}); - expect(mockSessionManager.getStackTraceDetailed).toHaveBeenCalledWith('test-session', 5, false); + expect(mockSessionManager.getStackTraceDetailed).toHaveBeenCalledWith('test-session', 5, false, { ensureStackReady: true }); expect(result.frames).toHaveLength(1); });