mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-03 01:27:09 +01:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Agent environments can leave pending teardown or unverified provider resources. > - Recovery must preserve those resources until ownership and workspace safety allow cleanup. > - Empty maintenance scans keep an idle instance and its database active. > - The existing rows, transaction signals, and scheduler can represent the actual recovery obligation. > - This pull request schedules pending cleanup and historical verification only while work remains. > - An empty or fully verified instance stops these scans without losing cleanup recovery. ## Linked Issues or Issue Description **What happened?** The server scanned pending environment cleanup on its maintenance tick. The run reaper also scanned it even when it recovered no runs or active leases. Historical resource verification scanned every five minutes, including when all history already had valid cleanup proof. **What did you expect?** Writes should wake cleanup. Pending rows, deferred orphan buffers, and unverified history should retain recovery deadlines. When none remain, these workers should make no periodic database or provider calls. **Steps to reproduce** Start a configured instance with no pending leases and fully verified history. Leave it idle. Observe repeated pending-cleanup and historical-verification reads from the maintenance paths. Related: #17106 supplies the preceding login-cleanup migration. #13523 improves lease-reaper query indexes. #16809 addresses reusable-sandbox recovery authority. This PR keeps those authority checks in their existing services. ## What Changed - Register two workers with the existing delivery coordinator and shared scheduler. Remove their unconditional maintenance-tick calls. Run the reaper's inline pending cleanup only after it recovers actual work. - Add commit-aware signals to the audited lease and custom-image writers. Existing transactions notify directly; other writers use one small transaction helper. No tables or migrations are added. - Derive pending cleanup deadlines from the existing row backoff and retry or claim metadata. Retain a historical retry only while an unverified candidate exists. - Register unwritten orphan buffers across runtimes that share the database handle. Buffer registration wakes recovery without a database connection. Load the existing spool at startup and retry failed or capacity-limited loads while needed. - Isolate spool failures per runtime. Attempt all buffers and persisted pending rows, then retain a retry for any failed spool. - Document producer coverage, idle behavior, and remaining blockers. Update the sleep inventory. - Repair the inherited CSS-recovery E2E test: the current build no longer emits task-page CSS. Exercise the actual lazy environment stylesheet, preserving real network failure, recovery, and bounded reload assertions. ## Verification October 24 integration update: merged current master and resolved the runtime/spool conflicts. Preserve serialized ingestion of records appended after startup. Plugin replacement failures now register deferred recovery with an idle hold; concurrent callers share one persistence attempt. The producer audit includes this new writer. - Focused cleanup selection: 270 tests now pass across five suites. The initial run lacked a built plugin SDK and executed no tests; building the SDK fixed setup. The new concurrent-flush assertion then reproduced duplicate rows; a single-flight guard fixed it and all 18 plugin integration tests passed on rerun. - Server typecheck passed. After review fixes, all 211 tests across the cleanup worker, real orphan spool, environment runtime, and plugin integration suites passed. These overlap the earlier focused selection. The rate-limit test initially expected a write after twenty idle minutes to wait; corrected it to assert that subsequent writes cannot accelerate the five-minute cadence. - Review fixes preserve retries for unreadable spool directories/records, keep historical provider checks off the startup wait while joining them at shutdown, and restore their independent five-minute minimum spacing. The complete [CI matrix](https://github.com/paperclipai/paperclip/actions/runs/40108905033) passed on final commit `79ae1589c3c7b924db4bedb337d0c7ed3d95020b`, including all E2E shards, server/workspace tests, runner checks, typecheck, build/public-install verification, and security checks. - CI found a stale E2E asset matcher: `issue-detail-*.css` is no longer emitted. Both corrected browser tests passed against a fresh production UI build and disposable local instance. Fresh Greptile review is 5/5 on that same final commit, with all review threads resolved. Previous implementation verification: - `pnpm -r typecheck` and `pnpm build` passed. - New PostgreSQL worker suite: 12 tests passed. Covers quiet configured state, verified history, writes, outer commit and rollback, retry deadlines, restart, drain, deferred orphan buffers, spool failure, pagination, provider uncertainty, shutdown, and the idle run-reaper path. - Worker and environment runtime suites after review fix: 183 tests passed. A new assertion reproduced buffer starvation before the fix. Updated transaction fault injection and isolated simulated-instance database handles. Server typecheck also passed after the fix. - Run-recovery suites: 58 tests passed. Pending-cleanup suite: 38 tests passed after updating the logging fixture to seed real orphan recovery; an idle reaper no longer invokes cleanup. - Stop and workspace-recovery integration selection: 268 tests passed; two live-provider tests skipped by their environment gate. This overlaps the worker suite and is not an additional unique-test count. - Started the full local `pnpm test:run`, then stopped that older duplicate run after CI found the fixture mismatch. The corrected 38-test suite passed locally. The full sharded [CI matrix](https://github.com/paperclipai/paperclip/actions/runs/39678174479) passed on final commit `fc0ff3c4554ca4190a8b0b81ec06ef7065b8ad2c`, including build, typecheck, runner, server, workspace, and E2E checks. This is not a claim that the local full run completed. ## Risks - Missing a future producer signal could delay discovery until restart. The producer audit is documented in `doc/idle-environment-cleanup.md`. Independent SQL writers and another server process are outside the existing process-local notification contract. - Historical candidates blocked by ownership or provider uncertainty keep a five-minute retry until they can be verified. Provider destruction, claim fencing, Stop retention, and workspace safety remain controlled by their existing services. - This does not remove orphan-active discovery or legacy shutdown settlement from run recovery. Active custom-image expiry is also still periodic. External wake coverage does not change. - Local tests prove these workers become quiet and recover. They do not establish deployed instance suspension or fleet cost savings. ## Model Used OpenAI Codex (GPT-6), with code execution and reasoning. The exact serving model ID and context size were not exposed. ## 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 (e.g. `docs/...`, `fix/...`) 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 - [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>
195 lines
8.2 KiB
TypeScript
195 lines
8.2 KiB
TypeScript
import { promises as fs } from "node:fs";
|
|
import path from "node:path";
|
|
import { createHash } from "node:crypto";
|
|
import { idleOrphanSpoolPath, registerIdleSpoolDirectory } from "./idle-local-work.js";
|
|
|
|
/**
|
|
* One orphan pending-cleanup record. The acquire builds it when the conditional
|
|
* lease insert rejects a live sandbox. The record carries the same fields a
|
|
* `pending_cleanup` lease row needs, so a later flush re-inserts it without
|
|
* loss. The `metadata` field holds the sanitized lease metadata: it carries
|
|
* secret references, not secret values, so the record is safe to persist next to
|
|
* the other durable local state. A restart-recovered record still authorizes the
|
|
* teardown, because it keeps the immutable provider, provider lease id, and
|
|
* recorded config the teardown needs.
|
|
*/
|
|
export interface DeferredOrphanCleanupRecord {
|
|
companyId: string;
|
|
environmentId: string;
|
|
executionWorkspaceId: string | null;
|
|
issueId: string | null;
|
|
heartbeatRunId: string | null;
|
|
provider: string;
|
|
providerLeaseId: string | null;
|
|
metadata: Record<string, unknown>;
|
|
}
|
|
|
|
/**
|
|
* A durable, restart-safe store for orphan pending-cleanup records. The acquire
|
|
* appends a record here when the conditional lease insert rejects a live
|
|
* sandbox, the compensating teardown fails, and every synchronous
|
|
* `pending_cleanup` database write also fails. The database is the only
|
|
* automated way to find a leaked sandbox, but the database is down at that
|
|
* instant, so the record has nowhere durable to land. The in-process buffer
|
|
* alone loses the record on a restart. This spool keeps the record on the local
|
|
* disk, next to the other durable server state, so a restart still finds it. A
|
|
* later cleanup sweep loads the spool, re-inserts each record as a durable
|
|
* `pending_cleanup` row, and removes the spooled copy.
|
|
*/
|
|
export interface SandboxOrphanCleanupSpool {
|
|
/**
|
|
* Persist one orphan record durably. Report `true` when the record reached the
|
|
* disk, or `false` on any failure, so the caller keeps the in-process buffer
|
|
* and the error log as the fallback handles. This method never throws.
|
|
*/
|
|
append(record: DeferredOrphanCleanupRecord): Promise<boolean>;
|
|
/**
|
|
* Load every persisted orphan record. The cleanup sweep calls this once after
|
|
* a restart, so the records a previous process could not land re-enter the
|
|
* flush path. This method never throws; it returns an empty list on any read
|
|
* failure and skips a malformed file.
|
|
*/
|
|
load(): Promise<DeferredOrphanCleanupRecord[]>;
|
|
/** Distinguish empty storage from unread work without discarding readable records. */
|
|
loadWithStatus?(): Promise<{ records: DeferredOrphanCleanupRecord[]; readFailed: boolean }>;
|
|
/**
|
|
* Remove one persisted orphan record after its durable `pending_cleanup` row
|
|
* lands. The removal runs after the database insert succeeds, so a crash
|
|
* between the insert and this removal leaves the spooled copy for a later
|
|
* flush. That later flush re-inserts a duplicate row, which the idempotent
|
|
* teardown handles. This order never loses the orphan. This method never
|
|
* throws.
|
|
*/
|
|
remove(record: DeferredOrphanCleanupRecord): Promise<void>;
|
|
}
|
|
|
|
// The spool writes one file per orphan record, so a removal never rewrites a
|
|
// shared file and never races a concurrent append. The file name is a stable
|
|
// hash of the record identity, so a repeated append for the same orphan
|
|
// overwrites the same file and never persists a duplicate.
|
|
const RECORD_FILE_SUFFIX = ".json";
|
|
|
|
// A unique temp-file counter, so two concurrent appends for two different
|
|
// records never share a temp path. The atomic rename then publishes each record
|
|
// without a torn write.
|
|
let tempFileCounter = 0;
|
|
|
|
// Build the stable file name for one record. Key on the provider lease id, the
|
|
// value the teardown needs, so a repeated append for the same orphan reuses the
|
|
// same file. A null provider lease id carries no identity, so fall back to a
|
|
// hash of the whole record. The hash keeps the name on the safe file-name
|
|
// charset and hides the raw identifiers from a directory listing.
|
|
function recordFileName(record: DeferredOrphanCleanupRecord): string {
|
|
const identity =
|
|
record.providerLeaseId !== null
|
|
? `lease:${record.providerLeaseId}`
|
|
: `record:${JSON.stringify(record)}`;
|
|
const hash = createHash("sha256").update(identity).digest("hex");
|
|
return `${hash}${RECORD_FILE_SUFFIX}`;
|
|
}
|
|
|
|
function isDeferredOrphanCleanupRecord(value: unknown): value is DeferredOrphanCleanupRecord {
|
|
if (typeof value !== "object" || value === null) return false;
|
|
const candidate = value as Record<string, unknown>;
|
|
const stringOrNull = (field: unknown) => field === null || typeof field === "string";
|
|
return (
|
|
typeof candidate.companyId === "string" &&
|
|
typeof candidate.environmentId === "string" &&
|
|
stringOrNull(candidate.executionWorkspaceId) &&
|
|
stringOrNull(candidate.issueId) &&
|
|
stringOrNull(candidate.heartbeatRunId) &&
|
|
typeof candidate.provider === "string" &&
|
|
stringOrNull(candidate.providerLeaseId) &&
|
|
typeof candidate.metadata === "object" &&
|
|
candidate.metadata !== null
|
|
);
|
|
}
|
|
|
|
// Write one file durably. Write the bytes to a unique temp file, flush the file
|
|
// to the disk, rename it onto the final path, then flush the directory. The
|
|
// rename is atomic on POSIX, so a reader never sees a torn record, and a crash
|
|
// after the directory flush still leaves the whole record on the disk.
|
|
async function writeFileDurable(absPath: string, data: string): Promise<void> {
|
|
tempFileCounter += 1;
|
|
const tempPath = `${absPath}.${process.pid}.${tempFileCounter}.tmp`;
|
|
const handle = await fs.open(tempPath, "w");
|
|
try {
|
|
await handle.writeFile(data, "utf8");
|
|
await handle.sync();
|
|
} finally {
|
|
await handle.close();
|
|
}
|
|
try {
|
|
await fs.rename(tempPath, absPath);
|
|
} catch (error) {
|
|
// The rename failed, so drop the temp file. Do not leave it behind, or a
|
|
// later load reads a half-published record.
|
|
await fs.rm(tempPath, { force: true }).catch(() => undefined);
|
|
throw error;
|
|
}
|
|
const dirHandle = await fs.open(path.dirname(absPath), "r");
|
|
try {
|
|
await dirHandle.sync();
|
|
} finally {
|
|
await dirHandle.close();
|
|
}
|
|
}
|
|
|
|
/**
|
|
* Build a local-disk orphan-cleanup spool. The spool writes one file per record
|
|
* under the spool directory. The default directory sits next to the other
|
|
* durable server state under the instance root. A caller can override the
|
|
* directory for a test.
|
|
*/
|
|
export function createSandboxOrphanCleanupSpool(spoolDir?: string): SandboxOrphanCleanupSpool {
|
|
const dir = spoolDir ?? idleOrphanSpoolPath();
|
|
registerIdleSpoolDirectory(dir);
|
|
|
|
async function loadWithStatus() {
|
|
let names: string[];
|
|
try { names = await fs.readdir(dir); }
|
|
catch (error) {
|
|
return { records: [], readFailed: (error as NodeJS.ErrnoException).code !== "ENOENT" };
|
|
}
|
|
const records: DeferredOrphanCleanupRecord[] = [];
|
|
let readFailed = false;
|
|
for (const name of names) {
|
|
if (!name.endsWith(RECORD_FILE_SUFFIX)) continue;
|
|
try {
|
|
const parsed: unknown = JSON.parse(await fs.readFile(path.join(dir, name), "utf8"));
|
|
if (isDeferredOrphanCleanupRecord(parsed)) records.push(parsed);
|
|
else readFailed = true;
|
|
} catch (error) {
|
|
// A concurrent successful flush can remove a listed record. Other read
|
|
// failures and malformed records remain unresolved cleanup obligations.
|
|
if ((error as NodeJS.ErrnoException).code !== "ENOENT") readFailed = true;
|
|
}
|
|
}
|
|
return { records, readFailed };
|
|
}
|
|
|
|
return {
|
|
async append(record) {
|
|
const absPath = path.join(dir, recordFileName(record));
|
|
try {
|
|
await fs.mkdir(dir, { recursive: true });
|
|
// Serialize the record only. A caught write error never enters a log,
|
|
// because it can carry a credential in its message, code, cause, or
|
|
// stack. So this method reports a boolean and never logs.
|
|
await writeFileDurable(absPath, JSON.stringify(record));
|
|
return true;
|
|
} catch {
|
|
return false;
|
|
}
|
|
},
|
|
|
|
async load() { return (await loadWithStatus()).records; },
|
|
loadWithStatus,
|
|
|
|
async remove(record) {
|
|
const absPath = path.join(dir, recordFileName(record));
|
|
await fs.rm(absPath, { force: true }).catch(() => undefined);
|
|
},
|
|
};
|
|
}
|