mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-04 02:32:07 +01:00
fix(server): retain context for unconfirmed adapter stops (#14639)
## Thinking Path > - Paperclip must keep ownership of work until termination is verified. > - A Stop request waits for the adapter and its cleanup to settle. > - After 60 seconds, an unconfirmed Stop raises an error. > - The error currently lacks run, adapter, and runtime context. > - This makes it difficult to investigate which stop path is stuck. > - This change adds bounded diagnostics while preserving termination checks. ## Linked Issues or Issue Description **What happened?** An adapter can remain unsettled after its Stop request. The resulting error says termination is unverified but does not identify the adapter or run in error monitoring. The optional Sentry setup does not capture request context, so the endpoint alone cannot fill the gap. **Expected behavior** Keep the Stop unconfirmed and preserve its live execution owner. When optional Sentry is enabled, attach enough bounded context to investigate the affected run. **Steps to reproduce** 1. Register an adapter execution control and abort its controller. 2. Leave its settlement promise pending. 3. Wait for the configured Stop timeout. 4. Observe that Stop still fails, but the event now includes the run UUID, built-in adapter, native/legacy runtime, timeout duration, and abort-requested flag. **Paperclip version or commit** Master commit `17780751551b3bc1c2521f7694026c34534c46c9`; reproduced with fake timers and mocked optional error monitoring. **Deployment mode** Server execution control, including local and hosted runs. Reporting remains opt-in. Searched related Stop PRs. #14523 and #14244 address Hermes cancellation contracts; this change only adds diagnostics to the shared unconfirmed-stop timeout. ## What Changed - Use a typed timeout error with the existing message, name, and timer stack. - Pass run/adapter/runtime identity from the cancellation owner. - Add an event-local, allowlisted Sentry context without changing the default fingerprint. - Rebuild the reported exception so arbitrary provider fields cannot be serialized. - Test timeout ownership, delayed settlement, privacy boundaries, and absence of context on unrelated events. - Document the additional opt-in fields. ## Verification - `pnpm exec vitest run server/src/services/adapter-execution-control.test.ts server/src/__tests__/sentry.test.ts`: 38 passed, five real-SDK checks skipped because the optional package is not installed. - `pnpm -r typecheck` passed; server typecheck passed again after the SDK test addition. - With audited optional `@sentry/node@10.71.0` installed only in local test dependencies, `PAPERCLIP_REQUIRE_SENTRY_TEST_SDK=1 pnpm exec vitest run server/src/__tests__/run-failure-sentry-real-sdk.test.ts server/src/services/adapter-execution-control.test.ts server/src/__tests__/sentry.test.ts`: all 45 tests passed. The real SDK uses an in-memory transport; no Sentry requests are sent. - The broad local `pnpm test:run` command did not complete in the available verification window and was stopped; no full local-suite pass is claimed. `pnpm build` passed. All sharded GitHub CI checks passed on the final PR head. - Tests use fake timers and a mocked Sentry package; no provider or monitoring requests. ## Risks This is diagnostic coverage, not a claim that the underlying stop delay is fixed. Unknown adapter/runtime values become `unknown`; malformed run identifiers become `null`. No stop reason, prompt, output, provider response, credentials, or arbitrary error properties are sent. Timeout, cancellation acknowledgement, live-owner retention, and retry behavior remain unchanged. No schema changes. ## Model Used OpenAI Codex (GPT-6), with reasoning, repository inspection, and command execution. The session does not expose a more specific model revision or context-window size. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links - [x] My branch name describes the change and contains no internal Paperclip ticket id or instance-derived details - [x] I have run the targeted tests locally and they pass; full checks are in progress - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
35a4448c02
commit
0b12ca9532
8 files changed
+177
-5
No files matched your search
@@ -415,6 +415,16 @@ values become `unknown`. A signal such as `SIGKILL` does not establish who sent
|
||||
it or prove an out-of-memory kill. These fields do not change error grouping
|
||||
or run outcomes, and do not include process output or adapter result payloads.
|
||||
|
||||
An unconfirmed adapter Stop timeout has an event-local `adapter_stop` context:
|
||||
the run UUID, built-in adapter type, native/legacy runtime mode, configured
|
||||
wait duration, and whether abort was requested. Invalid identities become
|
||||
`null`, and unknown adapter/runtime values become `unknown`. The report does
|
||||
not include stop reasons, prompts, process output, provider responses, or
|
||||
credentials. It preserves default error grouping and does not acknowledge
|
||||
termination, remove the live execution control, or change the timeout. The
|
||||
context is sent only through the existing opt-in Sentry gate and never leaks
|
||||
into unrelated captures.
|
||||
|
||||
The shared reporter also attaches bounded diagnostic contexts for both legacy
|
||||
and native runs:
|
||||
|
||||
|
||||
@@ -35,6 +35,53 @@ afterEach(async () => {
|
||||
});
|
||||
|
||||
describe.skipIf(!sentryPackage)("run failure context with the real Sentry SDK", () => {
|
||||
it("keeps unconfirmed Stop context off unrelated events and drops arbitrary error fields", async () => {
|
||||
const Sentry = sentryPackage!;
|
||||
const events: Array<Record<string, unknown>> = [];
|
||||
vi.stubEnv("SENTRY_DSN_BACKEND", "https://public@example.invalid/1");
|
||||
vi.doMock("../peer-version-check.js", () => ({ checkExactPeerVersions: () => ({ ok: true }) }));
|
||||
vi.doMock("@sentry/node", () => ({
|
||||
...Sentry,
|
||||
init: (options: Record<string, unknown>) => Sentry.init({
|
||||
...options,
|
||||
transport: () => ({ send: async () => ({}), flush: async () => true }),
|
||||
beforeSend: (event: Record<string, unknown>) => { events.push(event); return event; },
|
||||
}),
|
||||
}));
|
||||
vi.resetModules();
|
||||
const { sentryReady, captureException } = await import("../sentry.js");
|
||||
const { AdapterStopTimeoutError } = await import("../services/adapter-stop-timeout.js");
|
||||
await sentryReady;
|
||||
const timeout = Object.assign(new AdapterStopTimeoutError(60_000, {
|
||||
runId: "11111111-1111-4111-8111-111111111111", adapterType: "cursor",
|
||||
runtimeMode: "legacy", abortRequested: true,
|
||||
}), {
|
||||
cause: new Error("private-provider-cause"),
|
||||
providerResponse: { headers: "private-provider-headers", body: "private-provider-body" },
|
||||
});
|
||||
captureException(timeout);
|
||||
await Promise.resolve();
|
||||
captureException(new Error("unrelated stop diagnostic fixture"));
|
||||
await Sentry.flush(2000);
|
||||
expect(events).toHaveLength(2);
|
||||
const captured = (message: string) => events.find((event) =>
|
||||
(event.exception as { values: Array<{ value: string }> }).values.some((entry) => entry.value === message),
|
||||
);
|
||||
expect(captured(timeout.message)).toMatchObject({
|
||||
tags: { error_code: "adapter_stop_unconfirmed" }, fingerprint: ["{{ default }}"],
|
||||
contexts: { adapter_stop: {
|
||||
runId: "11111111-1111-4111-8111-111111111111", adapterType: "cursor",
|
||||
runtimeMode: "legacy", abortRequested: true, timeoutMs: 60_000,
|
||||
} },
|
||||
});
|
||||
expect(JSON.stringify(events)).not.toContain("private-provider-");
|
||||
const unrelated = captured("unrelated stop diagnostic fixture");
|
||||
expect(unrelated).toBeDefined();
|
||||
expect(unrelated).not.toHaveProperty("contexts.adapter_stop");
|
||||
expect(unrelated).not.toHaveProperty("tags.error_code");
|
||||
expect(unrelated).not.toHaveProperty("fingerprint");
|
||||
});
|
||||
|
||||
it("keeps each run's identity and fingerprint off unrelated errors", async () => {
|
||||
const Sentry = sentryPackage!;
|
||||
const events: Array<Record<string, unknown>> = [];
|
||||
|
||||
@@ -127,6 +127,45 @@ describe("sentryReady", () => {
|
||||
});
|
||||
|
||||
describe("captureException", () => {
|
||||
it("adds bounded Stop timeout context only to that event", async () => {
|
||||
process.env[BACKEND_DSN_ENV] = "https://fixture@example.com/1";
|
||||
const sdk = mockSentryPackage();
|
||||
const { captureException, sentryReady } = await importFreshSentry();
|
||||
const { AdapterStopTimeoutError } = await import("../services/adapter-stop-timeout.js");
|
||||
await sentryReady;
|
||||
const error = new AdapterStopTimeoutError(60_000, {
|
||||
runId: "11111111-1111-4111-8111-111111111111",
|
||||
adapterType: "cursor", runtimeMode: "legacy", abortRequested: true,
|
||||
});
|
||||
Object.assign(error, { providerResponse: "private fixture payload" });
|
||||
captureException(error);
|
||||
const unrelated = new Error("unrelated");
|
||||
captureException(unrelated);
|
||||
expect(sdk.captureException.mock.calls[0]).toEqual([
|
||||
expect.objectContaining({ message: error.message, stack: error.stack }),
|
||||
{ tags: { error_code: "adapter_stop_unconfirmed" }, fingerprint: ["{{ default }}"], contexts: {
|
||||
adapter_stop: { runId: "11111111-1111-4111-8111-111111111111", adapterType: "cursor", runtimeMode: "legacy", abortRequested: true, timeoutMs: 60_000 },
|
||||
} },
|
||||
]);
|
||||
expect(JSON.stringify(sdk.captureException.mock.calls[0])).not.toContain("private fixture payload");
|
||||
expect(sdk.captureException.mock.calls[1]).toEqual([unrelated]);
|
||||
});
|
||||
|
||||
it("does not send arbitrary Stop diagnostic values", async () => {
|
||||
process.env[BACKEND_DSN_ENV] = "https://fixture@example.com/1";
|
||||
const sdk = mockSentryPackage();
|
||||
const { captureException, sentryReady } = await importFreshSentry();
|
||||
const { AdapterStopTimeoutError } = await import("../services/adapter-stop-timeout.js");
|
||||
await sentryReady;
|
||||
captureException(new AdapterStopTimeoutError(NaN, {
|
||||
runId: "private fixture payload", adapterType: "private fixture payload", runtimeMode: "private fixture payload",
|
||||
}));
|
||||
expect(JSON.stringify(sdk.captureException.mock.calls)).not.toContain("private fixture payload");
|
||||
expect(sdk.captureException.mock.calls[0]).toEqual([expect.any(Error), expect.objectContaining({ contexts: {
|
||||
adapter_stop: { runId: null, adapterType: "unknown", runtimeMode: "unknown", abortRequested: null, timeoutMs: null },
|
||||
} })]);
|
||||
});
|
||||
|
||||
it("is a no-op and does not throw when the gate is closed", async () => {
|
||||
const { captureException, sentryReady } = await importFreshSentry();
|
||||
await sentryReady;
|
||||
|
||||
+13
-1
@@ -56,6 +56,7 @@
|
||||
// `instrumentation.ts`.
|
||||
|
||||
import os from "node:os";
|
||||
import { AdapterStopTimeoutError } from "./services/adapter-stop-timeout.js";
|
||||
import type { RunFailureDiagnostics } from "./services/run-failure-diagnostics.js";
|
||||
import { readBuildCommit } from "./build-commit.js";
|
||||
import { checkExactPeerVersions } from "./peer-version-check.js";
|
||||
@@ -106,7 +107,18 @@ export const sentryReady: Promise<void> = dsn ? bootstrapSentry(dsn) : Promise.r
|
||||
export function captureException(error: unknown): void {
|
||||
if (!sentryHandle) return;
|
||||
try {
|
||||
sentryHandle.captureException(error);
|
||||
if (error instanceof AdapterStopTimeoutError) {
|
||||
// Event-local, fixed-shape context: no ambient scope or raw error fields.
|
||||
const exception = new Error(error.message);
|
||||
exception.stack = error.stack;
|
||||
sentryHandle.captureException(exception, {
|
||||
tags: { error_code: "adapter_stop_unconfirmed" },
|
||||
contexts: { adapter_stop: { ...error.diagnostics } },
|
||||
fingerprint: ["{{ default }}"],
|
||||
});
|
||||
} else {
|
||||
sentryHandle.captureException(error);
|
||||
}
|
||||
} catch (err) {
|
||||
// eslint-disable-next-line no-console
|
||||
console.error("[paperclip] Sentry captureException failed", err);
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import { afterEach, expect, it, vi } from "vitest";
|
||||
import { AdapterStopTimeoutError } from "./adapter-stop-timeout.js";
|
||||
import {
|
||||
adapterExecutionControls,
|
||||
captureAdapterStopOwnership,
|
||||
@@ -120,3 +121,33 @@ it("bounds Stop when an adapter does not settle", async () => {
|
||||
await assertion;
|
||||
expect(vi.getTimerCount()).toBe(0);
|
||||
});
|
||||
|
||||
it("records the exact unconfirmed run without finishing or removing its control", async () => {
|
||||
vi.useFakeTimers();
|
||||
const runId = "11111111-1111-4111-8111-111111111111";
|
||||
const control = createAdapterExecutionControl();
|
||||
await registerAdapterExecutionControl(runId, control);
|
||||
control.controller.abort();
|
||||
let finished = false;
|
||||
void control.settled.then(() => { finished = true; });
|
||||
const result = waitForAdapterStop(control.settled, 1000, {
|
||||
runId, adapterType: "claude_local", runtimeMode: "legacy", abortRequested: control.controller.signal.aborted,
|
||||
}).catch((error: unknown) => error);
|
||||
try {
|
||||
await vi.advanceTimersByTimeAsync(1000);
|
||||
const error = await result;
|
||||
expect(error).toBeInstanceOf(AdapterStopTimeoutError);
|
||||
expect((error as AdapterStopTimeoutError).diagnostics).toEqual({
|
||||
runId, adapterType: "claude_local", runtimeMode: "legacy", abortRequested: true, timeoutMs: 1000,
|
||||
});
|
||||
expect(adapterExecutionControls.get(runId)).toBe(control);
|
||||
expect(finished).toBe(false);
|
||||
expect(vi.getTimerCount()).toBe(0);
|
||||
control.finish();
|
||||
await control.settled;
|
||||
expect(finished).toBe(true);
|
||||
} finally {
|
||||
control.finish();
|
||||
adapterExecutionControls.delete(runId);
|
||||
}
|
||||
});
|
||||
@@ -1,3 +1,5 @@
|
||||
import { AdapterStopTimeoutError, type AdapterStopContext } from "./adapter-stop-timeout.js";
|
||||
|
||||
/** Live adapter ownership shared by routes and scheduler service instances. */
|
||||
export function createAdapterExecutionControl() {
|
||||
const controller = new AbortController();
|
||||
@@ -67,6 +69,7 @@ export async function registerAdapterExecutionControl(
|
||||
export async function waitForAdapterStop(
|
||||
settled: Promise<void>,
|
||||
timeoutMs = 60_000,
|
||||
diagnostics?: AdapterStopContext,
|
||||
) {
|
||||
let timer: ReturnType<typeof setTimeout> | undefined;
|
||||
try {
|
||||
@@ -76,9 +79,7 @@ export async function waitForAdapterStop(
|
||||
timer = setTimeout(
|
||||
() =>
|
||||
reject(
|
||||
new Error(
|
||||
"Execution is still stopping; termination has not been verified.",
|
||||
),
|
||||
new AdapterStopTimeoutError(timeoutMs, diagnostics),
|
||||
),
|
||||
timeoutMs,
|
||||
);
|
||||
|
||||
@@ -0,0 +1,27 @@
|
||||
import { AGENT_ADAPTER_TYPES } from "@paperclipai/shared";
|
||||
|
||||
export interface AdapterStopContext {
|
||||
runId?: string;
|
||||
adapterType?: string;
|
||||
runtimeMode?: string;
|
||||
abortRequested?: boolean;
|
||||
}
|
||||
|
||||
/** Diagnostic identity only. This error never acknowledges termination. */
|
||||
export class AdapterStopTimeoutError extends Error {
|
||||
readonly diagnostics: Readonly<Record<string, string | number | boolean | null>>;
|
||||
|
||||
constructor(timeoutMs: number, context?: AdapterStopContext) {
|
||||
super("Execution is still stopping; termination has not been verified.");
|
||||
// Keep the existing Error name and timer stack for default error grouping.
|
||||
Error.captureStackTrace?.(this, AdapterStopTimeoutError);
|
||||
this.diagnostics = Object.freeze({
|
||||
timeoutMs: Number.isSafeInteger(timeoutMs) && timeoutMs >= 0 ? timeoutMs : null,
|
||||
runId: typeof context?.runId === "string" && /^[0-9a-f]{8}(?:-[0-9a-f]{4}){3}-[0-9a-f]{12}$/i.test(context.runId)
|
||||
? context.runId : null,
|
||||
adapterType: AGENT_ADAPTER_TYPES.some((type) => type === context?.adapterType) ? context!.adapterType! : "unknown",
|
||||
runtimeMode: context?.runtimeMode === "native" || context?.runtimeMode === "legacy" ? context.runtimeMode : "unknown",
|
||||
abortRequested: typeof context?.abortRequested === "boolean" ? context.abortRequested : null,
|
||||
});
|
||||
}
|
||||
}
|
||||
@@ -29266,7 +29266,12 @@ export function heartbeatService(
|
||||
}
|
||||
|
||||
if (control) {
|
||||
await waitForAdapterStop(control.settled);
|
||||
await waitForAdapterStop(control.settled, undefined, {
|
||||
runId: run.id,
|
||||
adapterType: agent?.adapterType,
|
||||
runtimeMode: run.runtimeMode,
|
||||
abortRequested: control.controller.signal.aborted,
|
||||
});
|
||||
const stopped = await getRun(run.id);
|
||||
if (stopped && isHeartbeatRunTerminalStatus(stopped.status)) {
|
||||
if (
|
||||
|
||||
Reference in new issue
Block a user