mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-10 12:37:55 +01:00
fix(recovery): date a settled orphan's first cleanup even under a scalar metadata root (#19989)
<!-- Write all pull request text in Simplified Technical English (ASD-STE100): short sentences, one instruction per sentence, simple approved vocabulary, and the active voice. --> ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - #19980 made the environment cleanup queue the owner of an unclaimed pending-cleanup item, with a due time and an overdue check (I7) > - A settled orphan's first cleanup attempt is dated by `pendingCleanupRecoveredAtMs`, which the orphaned-run-lease settle writes > - The settle merged that key with `coalesce(metadata, '{}') || ...`. For an array or scalar metadata root that produces an array, so the key is not at the top level > - The queue owner then falls back to the old `updatedAt` and can report a fresh item as overdue > - This pull request builds the merge from the object-root reader that the cleanup sweep already uses > - The benefit is that a fresh orphan is never reported overdue because of a malformed metadata root ## Linked Issues or Issue Description Refs #19980 Refs #19964 **What happened?** The review of #19980 found that the orphaned-run-lease settle wrote `pendingCleanupRecoveredAtMs` under an array metadata root as an array element. #19980 merged before this fix landed on it. **Expected behavior** The key is always top level, so the queue owner dates the first cleanup attempt correctly. **Steps to reproduce** 1. Save an active run lease whose metadata root is a JSON array. 2. Let its run end and the ownership settle move it to `pending_cleanup`. 3. Run the ownership pass: the item can read as `queue_overdue`. ## What Changed - `environment-run-lease-settlement.ts`: the merge starts from `pendingCleanupMetadataObjectSql()`, which reads a scalar or array root as an empty object. - `environment-run-lease-ownership.test.ts`: a legacy lease with an array metadata root is settled with the key at the top level. ## Verification - `cd server && npx vitest run src/__tests__/environment-run-lease-ownership.test.ts src/__tests__/environment-pending-cleanup-queue-owner.test.ts` (embedded Postgres). - Full typecheck is left to CI (local memory limit). ## Risks - Low risk. It changes only how one key is merged into a lease that the settle moves to `pending_cleanup`. A malformed root becomes an object, which the cleanup sweep already does for its own keys. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. ## Model Used - Claude Opus 5.5 (`claude-opus-5-5`), Anthropic, via Claude Code with tool use (shell, file edit, embedded Postgres tests). ## 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 - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
838e9bf3d2
commit
2fed77086f
2 files changed
+15
-2
No files matched your search
@@ -1,6 +1,6 @@
|
||||
import { spawn } from "node:child_process";
|
||||
import { randomUUID } from "node:crypto";
|
||||
import { eq } from "drizzle-orm";
|
||||
import { eq, sql } from "drizzle-orm";
|
||||
import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest";
|
||||
import {
|
||||
agents,
|
||||
@@ -257,6 +257,17 @@ suite("environment_run_lease_active on the shared ownership path", () => {
|
||||
});
|
||||
|
||||
describe("leases saved before holders were stamped", () => {
|
||||
it("dates the first cleanup attempt as a top-level key even for a scalar metadata root", async () => {
|
||||
const runId = await run("failed");
|
||||
const id = randomUUID();
|
||||
await db.insert(environmentLeases).values({ id, companyId, heartbeatRunId: runId, status: "active",
|
||||
metadata: sql`'["provider-wrote-an-array"]'::jsonb` });
|
||||
await runOwnershipReconciliationPass(db, { state: createOwnershipReportState() });
|
||||
const settled = await row(id);
|
||||
expect(settled.status).toBe("pending_cleanup");
|
||||
expect(typeof settled.metadata?.pendingCleanupRecoveredAtMs).toBe("number");
|
||||
});
|
||||
|
||||
it("keeps the run fact alone for one release on a single-host server", async () => {
|
||||
const runId = await run("failed");
|
||||
const leaseId = await lease(runId, null);
|
||||
|
||||
@@ -2,6 +2,7 @@ import { and, eq, inArray, ne, notInArray, or, sql } from "drizzle-orm";
|
||||
import { environmentLeases, environments, heartbeatRuns, nativeRunFinalizations, nativeRunResults, type Db } from "@paperclipai/db";
|
||||
import { currentControllerBootId } from "./controller-process-identity.js";
|
||||
import { writeEnvironmentCleanup } from "./environment-cleanup-notifications.js";
|
||||
import { pendingCleanupMetadataObjectSql } from "./environment-cleanup-state.js";
|
||||
import { readEnvironmentLeaseController } from "./environment-lease-controller.js";
|
||||
import { allowLegacyShutdownWorkspaceCleanup } from "./legacy-shutdown-workspace-settlement.js";
|
||||
import { nativeWorkspaceExportPendingForLease } from "./native-runtime/native-workspace-finalization-state.js";
|
||||
@@ -77,7 +78,8 @@ export async function settleEnvironmentRunLease(db: Db, record: InProgressRecord
|
||||
const flipped = await writeEnvironmentCleanup(tx, write => write.update(environmentLeases)
|
||||
.set({ status: "pending_cleanup", failureReason: ORPHANED_RUN_LEASE_FAILURE_REASON,
|
||||
// When the first cleanup attempt became due (the row keeps updatedAt).
|
||||
metadata: sql`coalesce(${environmentLeases.metadata}, '{}'::jsonb) || ${JSON.stringify({
|
||||
// A scalar or array root reads as an empty object, so the key is top level.
|
||||
metadata: sql`${pendingCleanupMetadataObjectSql()} || ${JSON.stringify({
|
||||
pendingCleanupRecoveredAtMs: context.now.getTime() })}::jsonb` })
|
||||
.where(and(eq(environmentLeases.id, lease.id), eq(environmentLeases.status, "active"),
|
||||
eq(environmentLeases.heartbeatRunId, runId),
|
||||
|
||||
Reference in new issue
Block a user