From 2fed77086fbb2fe0c72efff32623c21ff277b4a2 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Sun, 8 Nov 2026 01:20:00 -0600 Subject: [PATCH] fix(recovery): date a settled orphan's first cleanup even under a scalar metadata root (#19989) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- .../environment-run-lease-ownership.test.ts | 13 ++++++++++++- .../services/environment-run-lease-settlement.ts | 4 +++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/server/src/__tests__/environment-run-lease-ownership.test.ts b/server/src/__tests__/environment-run-lease-ownership.test.ts index a8a666a0ebc..669293cf660 100644 --- a/server/src/__tests__/environment-run-lease-ownership.test.ts +++ b/server/src/__tests__/environment-run-lease-ownership.test.ts @@ -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); diff --git a/server/src/services/environment-run-lease-settlement.ts b/server/src/services/environment-run-lease-settlement.ts index fb1c79a471e..f6020a1ed6f 100644 --- a/server/src/services/environment-run-lease-settlement.ts +++ b/server/src/services/environment-run-lease-settlement.ts @@ -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),