Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
11 changes: 11 additions & 0 deletions docs/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -569,6 +569,17 @@ 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. An MCP server that shuts down likewise exits with status 1 instead of
waiting on a debugger it could not stop. It names the process on stderr and as
`retained_debugger_pids` in its error log.

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
2 changes: 2 additions & 0 deletions scripts/ci/scopes.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ const PATH_SCOPES = {
"src/frida/frida-bindings.d.ts",
"src/objdump/**",
"src/cli/reverseEngineeringCommands.ts",
"src/process/ownedDebuggerCleanupError.ts",
"src/process/ownedDebuggerSession.ts",
"src/rizin/**",
"src/contracts/*ToolContracts.ts",
Expand Down Expand Up @@ -103,6 +104,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
17 changes: 16 additions & 1 deletion src/cli/reverseEngineeringCommands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ import {
createRizinDebugSessionManager,
} from "../composition/debuggerSessions.js";
import { ProviderCleanupError } from "../domain/providerCleanupError.js";
import {
retainedDebuggerDiagnostics,
retainedDebuggerProcessIds,
retainedDebuggerRemediation,
} from "../process/ownedDebuggerCleanupError.js";
import {
analysisEvidenceDeliveryOptions,
cliEvidenceDeliveryInputError,
Expand Down Expand Up @@ -144,6 +149,9 @@ 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 = retainedDebuggerProcessIds(cause);
throw new ProviderCleanupError(
providerId,
sessionId === undefined ? [] : [sessionId],
Expand All @@ -158,8 +166,15 @@ const closeDebuggerCommandSession = async (
: cause instanceof Error
? cause.message
: String(cause),
...retainedDebuggerDiagnostics(cause),
},
{
cause,
operation,
...(retainedPids.length === 0
? {}
: { remediationAction: retainedDebuggerRemediation(retainedPids) }),
},
{ 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
15 changes: 14 additions & 1 deletion src/main/shutdown.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,12 @@ import type { Logger } from "pino";
import { AnalysisError } from "../domain/analysisErrorBase.js";
import { projectAnalysisError } from "../domain/analysisErrorProjection.js";
import type { JsonValue } from "../domain/jsonValue.js";
import {
OwnedDebuggerCleanupError,
retainedDebuggerDiagnostics,
retainedDebuggerProcessIds,
retainedDebuggerRemediation,
} from "../process/ownedDebuggerCleanupError.js";
import type { McpServerHandle, RuntimeDependencies } from "./types.js";
import { MCP_SHUTDOWN_FAILED } from "./messages.js";

Expand Down Expand Up @@ -103,10 +109,14 @@ export const reportMcpShutdownFailure = (
"MCP shutdown rejected",
);
dependencies.setExitCode(1);
serverLogger.error(MCP_SHUTDOWN_FAILED);
serverLogger.error(retainedDebuggerDiagnostics(cause), MCP_SHUTDOWN_FAILED);
dependencies.writeStderr(`${MCP_SHUTDOWN_FAILED}\n`);
if (cause instanceof AnalysisError)
dependencies.writeStderr(`${projectAnalysisError(cause).message}\n`);
// This process is about to end, so it is the last chance to name them.
const retainedPids = retainedDebuggerProcessIds(cause);
if (retainedPids.length > 0)
dependencies.writeStderr(`${retainedDebuggerRemediation(retainedPids)}\n`);
};

const describeShutdownFailure = (
Expand Down Expand Up @@ -134,6 +144,9 @@ const describeShutdownFailureNode = (
(typeof code === "number" && Number.isFinite(code))
? { code }
: {}),
...(cause instanceof OwnedDebuggerCleanupError
? { retained_debugger_pids: [...cause.retainedProcessIds] }
: {}),
...(cause instanceof AggregateError
? {
errors: cause.errors.map((error: unknown) =>
Expand Down
45 changes: 45 additions & 0 deletions src/process/ownedDebuggerCleanupError.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/** 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;
}
}

/** Retained debugger process IDs anywhere in a composed cleanup failure. */
export const retainedDebuggerProcessIds = (
cause: unknown,
): readonly number[] => {
const found = new Set<number>();
const visited = new WeakSet<object>();
const visit = (node: unknown): void => {
if (typeof node !== "object" || node === null || visited.has(node)) return;
visited.add(node);
if (node instanceof OwnedDebuggerCleanupError)
for (const pid of node.retainedProcessIds) found.add(pid);
if (node instanceof AggregateError)
for (const error of node.errors) visit(error);
if (node instanceof Error) visit(node.cause);
};
visit(cause);
return [...found];
};

/** Log or diagnostic fields naming those processes; empty when none are known. */
export const retainedDebuggerDiagnostics = (
cause: unknown,
): { readonly retained_debugger_pids?: number[] } => {
const pids = retainedDebuggerProcessIds(cause);
return pids.length === 0 ? {} : { retained_debugger_pids: [...pids] };
};

/** What a caller must do once REA ends with these debuggers unconfirmed. */
export const retainedDebuggerRemediation = (pids: readonly number[]): string =>
`REA exits without confirming that the debugger stopped, and no later REA command can retry this cleanup. Check process ${pids.join(", ")} and its target, and stop them if they are still running.`;
51 changes: 34 additions & 17 deletions src/process/ownedDebuggerSession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type { DebuggerAdmissionLimit } from "../config/debuggerSessionLimits.js"
import { AnalysisResourceConstraintError } from "../domain/analysisErrorCore.js";
import type { JsonValue } from "../domain/jsonValue.js";
import { parseLinuxProcessStat } from "./LinuxProcessStat.js";
import { OwnedDebuggerCleanupError } from "./ownedDebuggerCleanupError.js";

/** Result of terminating one debugger process owned by this application. */
export type OwnedDebuggerStopResult =
Expand Down Expand Up @@ -364,42 +365,58 @@ 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 };
}

/**
* 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
};
6 changes: 5 additions & 1 deletion src/server/createServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { registerCaptureTools } from "./registerCaptureTools.js";
import { registerSignalTools } from "./registerSignalTools.js";
import { registerWasmTools } from "./registerWasmTools.js";
import { registerApkSignatureTools } from "./registerApkSignatureTools.js";
import { retainedDebuggerDiagnostics } from "../process/ownedDebuggerCleanupError.js";
import { snapshotEnvironment } from "../process/snapshotEnvironment.js";
import { captureProviderEnvironment } from "./providerEnvironment.js";
import { createByteEntropyService } from "../composition/binaryDiagnostics.js";
Expand Down Expand Up @@ -305,7 +306,10 @@ export const createServer = (
previousOnclose?.();
void closeProviders().catch((cause: unknown) => {
logger.error(
{ error: cause instanceof Error ? cause.message : String(cause) },
{
error: cause instanceof Error ? cause.message : String(cause),
...retainedDebuggerDiagnostics(cause),
},
"Provider cleanup failed during server shutdown",
);
});
Expand Down
Loading
Loading