diff --git a/docs/architecture/debuggingHandler.md b/docs/architecture/debuggingHandler.md index dde25ef..0c69477 100644 --- a/docs/architecture/debuggingHandler.md +++ b/docs/architecture/debuggingHandler.md @@ -88,16 +88,21 @@ termination, bounded by the operation timeout. ### Restart Completion -`handleRestart()` waits for the executor's restart acknowledgement, bounded by -the configured operation timeout. It returns immediately after acknowledgement, -without an arbitrary settling delay. A continued/stopped event, even at a new -location, does not prove that all restart commands succeeded. - -If completion is not acknowledged, the handler reports an error explaining that +`handleRestart()` races the executor's acknowledgement against observation of a +fresh stopped-event sequence in the same session, bounded by the operation +timeout. An acknowledgement returns success without an arbitrary settling delay. +A fresh stop returns an explicit **completion unconfirmed** result, not success: +execution was observed after the request, but this does not prove that all +restart commands succeeded. Existing paused state, source/UI changes, continued +events alone, and stops in another session do not qualify. + +If neither signal arrives, the handler reports a timeout explaining that the target may already have restarted and the underlying request was not cancelled. It does not automatically retry or stop the session. This protects both hosts from an unresponsive restart; notably, Cortex-Debug v1.12.1 omits its -successful DAP restart response. Actual command failures still propagate. +successful DAP restart response. Actual command failures received before the +result still propagate. A later rejection after an unconfirmed result is logged; +the underlying request is not cancelled. Polling stops when the call returns. ### Root Cause Analysis diff --git a/src/debugMCPServer.ts b/src/debugMCPServer.ts index a6da21d..2cf7019 100644 --- a/src/debugMCPServer.ts +++ b/src/debugMCPServer.ts @@ -251,7 +251,7 @@ export class DebugMCPServer { // Restart debugging tool server.registerTool('restart_debugging', { - description: 'Restart the debug session from the beginning with the same configuration.', + description: 'Restart the debug session from the beginning with the same configuration. A fresh stop without an adapter acknowledgement returns an explicit completion-unconfirmed result, not confirmed success.', }, async () => this.runTool('restart_debugging', () => debuggingHandler.handleRestart())); // Add breakpoint tool diff --git a/src/debuggingHandler.ts b/src/debuggingHandler.ts index ed251b9..ec4852d 100644 --- a/src/debuggingHandler.ts +++ b/src/debuggingHandler.ts @@ -384,13 +384,48 @@ export class DebuggingHandler implements IDebuggingHandler { * Restart the debugging session */ public async handleRestart(): Promise { + const observation = new AbortController(); + let returnedUnconfirmed = false; try { if (!(await this.executor.hasActiveSession())) { throw new Error('No active debug session to restart'); } - await withTimeout( - this.executor.restart(), + const outcome = await withTimeout( + (async () => { + const sessionId = this.executor.getActiveSession()?.id; + const before = await this.executor.getCurrentDebugState(this.numNextLines); + if (observation.signal.aborted) { + throw new Error('Restart observation cancelled'); + } + const acknowledgement = this.executor.restart().then( + () => 'acknowledged', + error => { + if (returnedUnconfirmed) { + logger.warn('Restart failed after returning an unconfirmed result:', error); + } + throw error; + } + ); + if (!sessionId) { + return acknowledgement; + } + const freshStop = async () => { + while (!observation.signal.aborted) { + const state = await this.executor.getCurrentDebugState(this.numNextLines); + if (!observation.signal.aborted && state.sessionActive && state.isPaused() && + this.executor.getActiveSession()?.id === sessionId && + state.stopSequence !== null && state.stopSequence !== before.stopSequence) { + return 'observed'; + } + if (!observation.signal.aborted) { + await new Promise(resolve => setTimeout(resolve, 50)); + } + } + throw new Error('Restart observation cancelled'); + }; + return Promise.race([acknowledgement, freshStop()]); + })(), this.timeoutInSeconds * 1000, () => new Error( `Restart timed out after ${this.timeoutInSeconds}s without completion acknowledgement. ` + @@ -399,9 +434,19 @@ export class DebuggingHandler implements IDebuggingHandler { ) ); + if (outcome === 'observed') { + returnedUnconfirmed = true; + const message = 'The target stopped after the restart request; completion is unconfirmed ' + + 'because the adapter has not acknowledged it. The request has not been cancelled. ' + + 'Check get_debug_status and the Debug Console before retrying.'; + logger.warn(message); + return message; + } return 'Debug session restarted successfully'; } catch (error) { throw new Error(`Error restarting debug session: ${error}`); + } finally { + observation.abort(); } } diff --git a/src/test/restartDebugging.test.ts b/src/test/restartDebugging.test.ts index fd21d64..a265ba8 100644 --- a/src/test/restartDebugging.test.ts +++ b/src/test/restartDebugging.test.ts @@ -8,6 +8,8 @@ import { DebuggingHandler } from '../debuggingHandler'; import { CliDebuggingExecutor } from '../cli/cliDebuggingExecutor'; import { CliDebugConfiguration } from '../cli/cliConfigurationManager'; import { IDebugConfigurationManager } from '../utils/debugConfigurationManager'; +import { DebugState } from '../debugState'; +import { logger } from '../utils/logger'; suite('Restart completion (#160)', () => { function handler(restart: () => Promise, active = true, timeout = 0.05): DebuggingHandler { @@ -76,6 +78,43 @@ suite('Restart completion (#160)', () => { rejectRestart(new Error('cleanup')); } }); + + for (const scenario of ['different session', 'unchanged stop', 'running', 'UI-only change']) { + test(`does not mistake ${scenario} for restart observation (#172)`, async () => { + const executor = new CliDebuggingExecutor(); + let sessionId = 'original'; + let release: () => void = () => {}; + let reads = 0; + const state = new DebugState(); + state.sessionActive = true; + state.paused = true; + state.stopSequence = scenario === 'UI-only change' ? null : 1; + executor.hasActiveSession = async () => true; + executor.getActiveSession = () => ({ id: sessionId, name: 'test', type: 'fake' }); + executor.getCurrentDebugState = async () => { reads++; return state.clone(); }; + executor.restart = () => { + if (scenario === 'different session') { + sessionId = 'replacement'; + state.stopSequence = 2; + } else if (scenario === 'running') { + state.paused = false; + state.stopSequence = null; + } else if (scenario === 'UI-only change') { + state.currentLine = 42; + } + return new Promise(resolve => { release = resolve; }); + }; + const target = new DebuggingHandler(executor, {} as IDebugConfigurationManager, 0.1); + try { + await assert.rejects(() => target.handleRestart(), /Restart timed out/); + const readsAtTimeout = reads; + await new Promise(resolve => setTimeout(resolve, 100)); + assert.equal(reads, readsAtTimeout, 'observation polling must stop after timeout'); + } finally { + release(); + } + }); + } }); suite('Restart acknowledgement over DAP (#160)', () => { @@ -116,10 +155,14 @@ function handle(request) { stopped(); break; case 'restart': + if (mode === 'unchanged') break; send({ type: 'event', event: 'continued', body: { threadId: 1, allThreadsContinued: true } }); stopped(); if (mode === 'success') send(response); if (mode === 'failure') send({ ...response, success: false, message: 'GDB restart rejected' }); + if (mode === 'late-failure') { + setTimeout(() => send({ ...response, success: false, message: 'late GDB restart rejected' }), 150); + } break; case 'stackTrace': send({ ...response, body: { stackFrames: [{ id: 4096, name: 'main', line: 2, column: 1 }] } }); @@ -160,22 +203,44 @@ process.stdin.on('data', chunk => { } }); - for (const mode of ['success', 'failure', 'missing']) { + for (const mode of ['success', 'failure', 'missing', 'unchanged', 'late-failure']) { test(`${mode} acknowledgement after a fresh stop`, async () => { await executor.startDebugging(directory, { ...config, mode }); await executor.waitForDebugSessionReady(1_000); const before = await executor.getCurrentDebugState(); assert.equal(before.isPaused(), true); - const target = new DebuggingHandler(executor, {} as IDebugConfigurationManager, 0.1); + const target = new DebuggingHandler(executor, {} as IDebugConfigurationManager, 0.5); if (mode === 'success') { assert.match(await target.handleRestart(), /restarted successfully/); + } else if (mode === 'missing' || mode === 'late-failure') { + const start = Date.now(); + const originalWarn = logger.warn; + const warnings: string[] = []; + logger.warn = (message, ...args) => { warnings.push([message, ...args].join(' ')); }; + try { + const result = await target.handleRestart(); + assert.match(result, /completion is unconfirmed/); + assert.doesNotMatch(result, /restarted successfully/); + assert.ok(Date.now() - start < 400, 'a fresh stop must not wait for the operation timeout'); + assert.ok(warnings.some(message => message.includes('completion is unconfirmed'))); + if (mode === 'late-failure') { + await new Promise(resolve => setTimeout(resolve, 200)); + assert.ok(warnings.some(message => message.includes('late GDB restart rejected'))); + } + } finally { + logger.warn = originalWarn; + } } else { await assert.rejects(() => target.handleRestart(), mode === 'failure' ? /GDB restart rejected/ : /Restart timed out.*not been cancelled/); } const after = await executor.getCurrentDebugState(); assert.equal(after.isPaused(), true); - assert.notEqual(after.stopSequence, before.stopSequence); + if (mode === 'unchanged') { + assert.equal(after.stopSequence, before.stopSequence); + } else { + assert.notEqual(after.stopSequence, before.stopSequence); + } }); } });