Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions docs/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,16 @@ prints a failure, so it omits the same-session reuse and
`parameter`) for a new invocation and rerun. For example, a Ghidra decompile
deadline names `REA_GHIDRA_DECOMPILE_TIMEOUT_SECONDS`.

`rea debug-with-gdb` and `rea debug-with-rizin` hold their debugger only for the
command's own process. If cleanup cannot confirm the debugger stopped, they print
`cleanup_incomplete` and exit nonzero without stopping it. `remediation.action`
names the process, as does `retained_debugger_pids` in `details.diagnostics`, or
in `details.diagnostics.cleanup_error.details.diagnostics` when the command also
failed or was interrupted. Exiting closes the debugger's command and output pipes.
REA does not establish what the debugger or its target does next, so check that
process. A stdio MCP server whose client disconnects likewise exits with status 1
instead of waiting on a debugger it could not stop, and logs the failure.

If one-shot analysis fails and closing its session also fails, the original
error details remain inline and `details.cleanup_error` describes the later
close failure. Each diagnostic retains its own reported resources; the later
Expand Down
1 change: 1 addition & 0 deletions scripts/ci/scopes.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ const PATH_SCOPES = {
"tests/boundary/providers/RizinInferiorTty.test.ts",
"tests/boundary/providers/RizinTestInferiorTty.ts",
"tests/boundary/process/ownedDebuggerSession.test.ts",
"tests/boundary/cli/debuggerOneShotExit.test.ts",
"tests/boundary/cli/gdbStartupCleanupCli.test.ts",
"tests/boundary/cli/reverseEngineeringCommands.test.ts",
"tests/boundary/mcp/cutter*.test.ts",
Expand Down
19 changes: 18 additions & 1 deletion src/cli/reverseEngineeringCommands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
createRizinDebugSessionManager,
} from "../composition/debuggerSessions.js";
import { ProviderCleanupError } from "../domain/providerCleanupError.js";
import { OwnedDebuggerCleanupError } from "../process/ownedDebuggerSession.js";
import {
analysisEvidenceDeliveryOptions,
cliEvidenceDeliveryInputError,
Expand Down Expand Up @@ -144,6 +145,13 @@ const closeDebuggerCommandSession = async (
try {
await manager.closeAll();
} catch (cause) {
// This process is the only owner, so a caller gets the debugger to check
// rather than a retry that no later command could perform.
const retainedPids =
cause instanceof OwnedDebuggerCleanupError
? cause.retainedProcessIds
: [];
const identified = retainedPids.length > 0;
throw new ProviderCleanupError(
providerId,
sessionId === undefined ? [] : [sessionId],
Expand All @@ -158,8 +166,17 @@ const closeDebuggerCommandSession = async (
: cause instanceof Error
? cause.message
: String(cause),
...(identified ? { retained_debugger_pids: [...retainedPids] } : {}),
},
{
cause,
operation,
...(identified
? {
remediationAction: `REA exits without confirming that the debugger stopped, and no later REA command can retry this cleanup. Check process ${retainedPids.join(", ")} and its target, and stop them if they are still running.`,
}
: {}),
},
{ cause, operation },
);
}
};
Expand Down
2 changes: 2 additions & 0 deletions src/gdb/GdbSessionManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2164,6 +2164,8 @@ export class GdbSessionManager {
);
});
}, this.#commandTimeoutMs);
// Cleanup bounds its own wait, so a late reply must not hold the host open.
if (session.cleanupStarted) timer.unref();
pending = {
token,
command,
Expand Down
65 changes: 48 additions & 17 deletions src/process/ownedDebuggerSession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -364,42 +364,73 @@ export const stopOwnedDebuggerProcess = async (
);
};

/** What closing all sessions needs from a session it may have to retain. */
interface RetainableDebuggerSession {
readonly id: string;
readonly launched: {
readonly process: { readonly pid?: number | undefined };
};
readonly supervisor: { ref(): void; unref(): void };
}

/** Closing all sessions could not confirm that these debuggers stopped. */
export class OwnedDebuggerCleanupError extends AggregateError {
/** Process IDs when cleanup gave up. Observations, not lasting proof of identity. */
readonly retainedProcessIds: readonly number[];

constructor(
failures: readonly Error[],
message: string,
retainedProcessIds: readonly number[],
) {
super(failures, message);
this.retainedProcessIds = retainedProcessIds;
}
}

/**
* Stop every owned session and retain failed sessions for possible recovery.
* Callers must block replacement admission and drain in-flight starts until
* this settles; incomplete cleanup leaves the old owner in the map for retry.
*
* Closing all sessions means their owner is finishing, so a retained session
* stops holding the host's event loop, as a supervised provider does after
* incomplete cleanup. Ownership in this process is unchanged, and a retry
* restores the references.
*/
export const closeOwnedDebuggerSessions = async <
Session extends { id: string },
Session extends RetainableDebuggerSession,
>(
sessions: Map<string, Session>,
stop: (session: Session) => Promise<OwnedDebuggerStopResult>,
debuggerName: string,
): Promise<void> => {
const entries = [...sessions.entries()];
for (const [, session] of entries) session.supervisor.ref();
const results = await Promise.allSettled(
[...sessions.entries()].map(async ([key, session]) => ({
key,
session,
result: await stop(session),
})),
entries.map(async ([, session]) => stop(session)),
);
const failures: Error[] = [];
for (const settled of results) {
const retained: Session[] = [];
for (const [index, settled] of results.entries()) {
const [key, session] = entries[index]!;
if (settled.status === "rejected") {
failures.push(
settled.reason instanceof Error
? settled.reason
: new Error(String(settled.reason)),
);
continue;
}
const { key, session, result } = settled.value;
if (result.status === "incomplete") failures.push(new Error(result.reason));
else if (sessions.get(key) === session) sessions.delete(key);
retained.push(session);
} else if (settled.value.status === "incomplete") {
failures.push(new Error(settled.value.reason));
retained.push(session);
} else if (sessions.get(key) === session) sessions.delete(key);
}
if (failures.length > 0)
throw new AggregateError(
failures,
`One or more owned ${debuggerName} processes did not stop cleanly`,
);
if (failures.length === 0) return;
for (const session of retained) session.supervisor.unref();
throw new OwnedDebuggerCleanupError(
failures,
`One or more owned ${debuggerName} processes did not stop cleanly`,
retained.flatMap(({ launched }) => launched.process.pid ?? []),
);
Comment thread
greptile-apps[bot] marked this conversation as resolved.
Resolved
};
Loading
Loading