fix(server): avoid hot restart shutdown deadlock (#9670)
## Thinking Path > - Paperclip is the open source control plane people use to manage AI agents and their work > - The server coordinates agent heartbeats and preserves eligible live runs during a hot restart > - Shutdown previously waited for all heartbeat scheduler work before capturing the hot-restart snapshot > - A deployment heartbeat can itself be in that scheduler set while waiting for the restart, creating a circular wait > - The missing snapshot prevents startup from classifying and adopting the still-running agent process > - This pull request captures the snapshot first and skips scheduler/drain waits only for an eligible hot restart > - The benefit is a single SIGTERM can restart the server without losing eligible live agent runs ## Linked Issues or Issue Description - **Preflight:** Searched open and closed PRs for the hot-restart shutdown deadlock; no duplicate found. Reproduced on `master` and confirmed this is core Paperclip behavior. - **What happened:** During a hot restart initiated by a running deployment heartbeat, the SIGTERM handler waited for `heartbeatSchedulerInFlight` before calling `prepareHotRestartShutdown()`. The heartbeat was itself in that set and waited for restart completion, so shutdown never wrote the adoption snapshot. - **Expected behavior:** An eligible hot restart captures its snapshot before waiting for scheduler work, preserves live child processes, and exits after one SIGTERM. - **Steps to reproduce:** 1. Start a heartbeat that remains active while requesting a hot restart. 2. Send SIGTERM to the server process. 3. Observe shutdown waiting on the active scheduler task and startup finding an intent without a shutdown snapshot. - **Paperclip commit:** `992389480a243b97bda214227e0767eb8c3672af` - **Deployment/install:** Self-hosted server built from source. - **Adapter:** Not adapter-specific; reproduced with a Codex heartbeat. - **Database/access:** Embedded PGlite; agent bearer context. - **Environment:** Node `v22.22.2` on `Linux 6.17.0-1015-aws aarch64 GNU/Linux`. - **Privacy:** No secrets, private logs, user paths, or internal issue references are included. ## What Changed - Add a focused shutdown coordinator that prepares hot-restart state before waiting for heartbeat scheduler idleness. - Skip scheduler-idle and graceful-drain waits only when the hot-restart service returns `skipDrain: true`. - Preserve normal graceful shutdown behavior when no eligible intent exists or preparation fails. - Add regression coverage for pending scheduler work, normal shutdown, and preparation failure. ## Verification - `pnpm exec vitest run server/src/shutdown.test.ts` — 3 passed. - `pnpm --filter @paperclipai/server typecheck` — passed. - `pnpm exec vitest run server/src/__tests__/heartbeat-process-recovery.test.ts -t 'hot-restart'` — 3 passed, 88 skipped. - `git diff --check origin/master...HEAD` — passed. ## Risks - Low-to-moderate risk: shutdown ordering changes, but only the explicitly eligible hot-restart path bypasses scheduler-idle and run-drain waits. - Normal shutdown and hot-restart preparation failures retain the existing graceful behavior. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. This is a focused bug fix and does not duplicate planned roadmap work. ## Model Used - OpenAI Codex coding agent; exact runtime model ID and context-window size are not exposed to the agent. Tool use, shell execution, repository editing, and test execution were enabled. ## 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 either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes (no documentation change required for this internal shutdown-order fix) - [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:
parent
992389480a
commit
263316609e
|
|
@ -64,6 +64,7 @@ import { getBoardClaimWarningUrl, initializeBoardClaimChallenge } from "./board-
|
|||
import { maybePersistWorktreeRuntimePorts } from "./worktree-config.js";
|
||||
import { initTelemetry, getTelemetryClient } from "./telemetry.js";
|
||||
import { conflict } from "./errors.js";
|
||||
import { coordinateHeartbeatSchedulerShutdown } from "./shutdown.js";
|
||||
import type {
|
||||
InstanceDatabaseBackupRunResult,
|
||||
InstanceDatabaseBackupTrigger,
|
||||
|
|
@ -1203,7 +1204,24 @@ export async function startServer(): Promise<StartedServer> {
|
|||
clearInterval(heartbeatSchedulerInterval);
|
||||
heartbeatSchedulerInterval = null;
|
||||
}
|
||||
await waitForHeartbeatSchedulerIdle();
|
||||
|
||||
const heartbeatShutdown = await coordinateHeartbeatSchedulerShutdown({
|
||||
signal,
|
||||
prepareHotRestartShutdown,
|
||||
waitForHeartbeatSchedulerIdle,
|
||||
});
|
||||
const skipHeartbeatDrain = heartbeatShutdown.hotRestart?.skipDrain === true;
|
||||
if (skipHeartbeatDrain) {
|
||||
logger.info(
|
||||
{ signal, hotRestart: heartbeatShutdown.hotRestart },
|
||||
"hot-restart shutdown prepared; skipping heartbeat scheduler idle wait and graceful run drain",
|
||||
);
|
||||
} else if (heartbeatShutdown.preparationError) {
|
||||
logger.error(
|
||||
{ err: heartbeatShutdown.preparationError, signal },
|
||||
"hot-restart shutdown preparation failed; falling back to graceful heartbeat run drain",
|
||||
);
|
||||
}
|
||||
|
||||
const telemetryClient = getTelemetryClient();
|
||||
if (telemetryClient) {
|
||||
|
|
@ -1211,19 +1229,6 @@ export async function startServer(): Promise<StartedServer> {
|
|||
await telemetryClient.flush();
|
||||
}
|
||||
|
||||
let skipHeartbeatDrain = false;
|
||||
if (prepareHotRestartShutdown) {
|
||||
try {
|
||||
const hotRestart = await prepareHotRestartShutdown(signal);
|
||||
skipHeartbeatDrain = hotRestart.skipDrain;
|
||||
if (skipHeartbeatDrain) {
|
||||
logger.info({ signal, hotRestart }, "hot-restart shutdown prepared; skipping graceful heartbeat run drain");
|
||||
}
|
||||
} catch (err) {
|
||||
logger.error({ err, signal }, "hot-restart shutdown preparation failed; falling back to graceful heartbeat run drain");
|
||||
}
|
||||
}
|
||||
|
||||
if (!skipHeartbeatDrain && drainHeartbeatRunsForShutdown) {
|
||||
try {
|
||||
const drain = await drainHeartbeatRunsForShutdown(signal);
|
||||
|
|
|
|||
|
|
@ -0,0 +1,94 @@
|
|||
import { describe, expect, it, vi } from "vitest";
|
||||
import { coordinateHeartbeatSchedulerShutdown } from "./shutdown.js";
|
||||
|
||||
describe("coordinateHeartbeatSchedulerShutdown", () => {
|
||||
it("captures a hot-restart snapshot without waiting for active scheduler work", async () => {
|
||||
let snapshotCaptured = false;
|
||||
const waitForHeartbeatSchedulerIdle = vi.fn(() => new Promise<void>(() => undefined));
|
||||
|
||||
const result = await coordinateHeartbeatSchedulerShutdown({
|
||||
signal: "SIGTERM",
|
||||
prepareHotRestartShutdown: vi.fn(async () => {
|
||||
snapshotCaptured = true;
|
||||
return { mode: "prepared" as const, skipDrain: true };
|
||||
}),
|
||||
waitForHeartbeatSchedulerIdle,
|
||||
});
|
||||
|
||||
expect(snapshotCaptured).toBe(true);
|
||||
expect(waitForHeartbeatSchedulerIdle).not.toHaveBeenCalled();
|
||||
expect(result).toEqual({
|
||||
hotRestart: { mode: "prepared", skipDrain: true },
|
||||
preparationError: null,
|
||||
waitedForSchedulerIdle: false,
|
||||
});
|
||||
});
|
||||
|
||||
it("preserves the scheduler idle wait for normal graceful shutdown", async () => {
|
||||
let releaseScheduler!: () => void;
|
||||
const schedulerIdle = new Promise<void>((resolve) => {
|
||||
releaseScheduler = resolve;
|
||||
});
|
||||
const waitForHeartbeatSchedulerIdle = vi.fn(() => schedulerIdle);
|
||||
let settled = false;
|
||||
|
||||
const shutdown = coordinateHeartbeatSchedulerShutdown({
|
||||
signal: "SIGTERM",
|
||||
prepareHotRestartShutdown: vi.fn(async () => ({
|
||||
mode: "not_requested" as const,
|
||||
skipDrain: false,
|
||||
})),
|
||||
waitForHeartbeatSchedulerIdle,
|
||||
}).finally(() => {
|
||||
settled = true;
|
||||
});
|
||||
|
||||
await vi.waitFor(() => expect(waitForHeartbeatSchedulerIdle).toHaveBeenCalledOnce());
|
||||
expect(settled).toBe(false);
|
||||
|
||||
releaseScheduler();
|
||||
|
||||
await expect(shutdown).resolves.toEqual({
|
||||
hotRestart: { mode: "not_requested", skipDrain: false },
|
||||
preparationError: null,
|
||||
waitedForSchedulerIdle: true,
|
||||
});
|
||||
});
|
||||
|
||||
it("waits for scheduler idle when hot-restart preparation is unavailable", async () => {
|
||||
const waitForHeartbeatSchedulerIdle = vi.fn(async () => undefined);
|
||||
|
||||
const result = await coordinateHeartbeatSchedulerShutdown({
|
||||
signal: "SIGTERM",
|
||||
prepareHotRestartShutdown: null,
|
||||
waitForHeartbeatSchedulerIdle,
|
||||
});
|
||||
|
||||
expect(waitForHeartbeatSchedulerIdle).toHaveBeenCalledOnce();
|
||||
expect(result).toEqual({
|
||||
hotRestart: null,
|
||||
preparationError: null,
|
||||
waitedForSchedulerIdle: true,
|
||||
});
|
||||
});
|
||||
|
||||
it("falls back to the scheduler idle wait when hot-restart preparation fails", async () => {
|
||||
const preparationError = new Error("snapshot failed");
|
||||
const waitForHeartbeatSchedulerIdle = vi.fn(async () => undefined);
|
||||
|
||||
const result = await coordinateHeartbeatSchedulerShutdown({
|
||||
signal: "SIGTERM",
|
||||
prepareHotRestartShutdown: vi.fn(async () => {
|
||||
throw preparationError;
|
||||
}),
|
||||
waitForHeartbeatSchedulerIdle,
|
||||
});
|
||||
|
||||
expect(waitForHeartbeatSchedulerIdle).toHaveBeenCalledOnce();
|
||||
expect(result).toEqual({
|
||||
hotRestart: null,
|
||||
preparationError,
|
||||
waitedForSchedulerIdle: true,
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
@ -0,0 +1,41 @@
|
|||
type HotRestartShutdownPreparation = {
|
||||
skipDrain: boolean;
|
||||
};
|
||||
|
||||
export async function coordinateHeartbeatSchedulerShutdown<
|
||||
TPreparation extends HotRestartShutdownPreparation,
|
||||
>(input: {
|
||||
signal: "SIGINT" | "SIGTERM";
|
||||
prepareHotRestartShutdown: ((signal: "SIGINT" | "SIGTERM") => Promise<TPreparation>) | null;
|
||||
waitForHeartbeatSchedulerIdle: () => Promise<void>;
|
||||
}): Promise<{
|
||||
hotRestart: TPreparation | null;
|
||||
preparationError: unknown;
|
||||
waitedForSchedulerIdle: boolean;
|
||||
}> {
|
||||
let hotRestart: TPreparation | null = null;
|
||||
let preparationError: unknown = null;
|
||||
|
||||
if (input.prepareHotRestartShutdown) {
|
||||
try {
|
||||
hotRestart = await input.prepareHotRestartShutdown(input.signal);
|
||||
} catch (err) {
|
||||
preparationError = err;
|
||||
}
|
||||
}
|
||||
|
||||
if (hotRestart?.skipDrain) {
|
||||
return {
|
||||
hotRestart,
|
||||
preparationError,
|
||||
waitedForSchedulerIdle: false,
|
||||
};
|
||||
}
|
||||
|
||||
await input.waitForHeartbeatSchedulerIdle();
|
||||
return {
|
||||
hotRestart,
|
||||
preparationError,
|
||||
waitedForSchedulerIdle: true,
|
||||
};
|
||||
}
|
||||
Loading…
Reference in New Issue