From cae179c21412048d08aeb94e0fe8b0f7844119de Mon Sep 17 00:00:00 2001 From: Dotta Date: Mon, 12 Oct 2026 16:39:34 -0500 Subject: [PATCH] fix(environments): preserve measured proof atomically on metadata saves Explicit null clears user metadata. Preserve only the current database proof during metadata and status saves, and invalidate it atomically on config or driver updates. Real PostgreSQL interleaving controls protect concurrent qualification writes. Co-Authored-By: Paperclip --- .../environment-instance-routes.test.ts | 12 ++++ .../src/__tests__/environment-service.test.ts | 63 ++++++++++++++++++- server/src/routes/environments.ts | 8 --- server/src/services/environments.ts | 30 ++++++--- 4 files changed, 94 insertions(+), 19 deletions(-) diff --git a/server/src/__tests__/environment-instance-routes.test.ts b/server/src/__tests__/environment-instance-routes.test.ts index f2b26355ed1..51664e01d98 100644 --- a/server/src/__tests__/environment-instance-routes.test.ts +++ b/server/src/__tests__/environment-instance-routes.test.ts @@ -21,6 +21,7 @@ const mockEnvironmentService = vi.hoisted(() => ({ list: vi.fn(), getById: vi.fn(), create: vi.fn(), + update: vi.fn(), })); const mockEnvironmentCustomImageService = vi.hoisted(() => ({ @@ -139,6 +140,16 @@ describe("environment instance routes", () => { expect(res.body.details?.code ?? res.body.code).toBe("environment_platform_probe_reserved"); expect(mockEnvironmentService.create).not.toHaveBeenCalled(); }); + it.each([null, { userNote: "replacement" }])("passes explicit user metadata replacement through without copying an old probe (%j)", async metadata => { + const existing = createEnvironment({ metadata: { userNote: "old", paperclipSandboxPlatformProbes: { "company-1": { v: 1, marker: "stale" } } } }); + mockEnvironmentService.getById.mockResolvedValue(existing); + mockEnvironmentService.update.mockResolvedValue({ ...existing, metadata }); + const app = createApp({ type: "board", source: "local_implicit", isInstanceAdmin: true }); + const res = await request(app).patch("/api/environments/env-1").send({ metadata }); + expect(res.status).toBe(200); + expect(mockEnvironmentService.update).toHaveBeenCalledExactlyOnceWith("env-1", { metadata }, { db: expect.anything() }); + expect(res.body.metadata).toEqual(metadata); + }); beforeEach(() => { mockIssueService.clearExecutionWorkspaceEnvironmentSelection.mockReset(); mockProjectService.clearExecutionWorkspaceEnvironmentSelection.mockReset(); @@ -148,6 +159,7 @@ describe("environment instance routes", () => { mockEnvironmentService.list.mockReset(); mockEnvironmentService.getById.mockReset(); mockEnvironmentService.create.mockReset(); + mockEnvironmentService.update.mockReset(); Object.values(mockEnvironmentCustomImageService).forEach((mock) => mock.mockReset()); mockEnvironmentCustomImageService.getOverview.mockResolvedValue({ activeTemplate: null, diff --git a/server/src/__tests__/environment-service.test.ts b/server/src/__tests__/environment-service.test.ts index cfa81980bf7..4f953121e34 100644 --- a/server/src/__tests__/environment-service.test.ts +++ b/server/src/__tests__/environment-service.test.ts @@ -1,6 +1,8 @@ import { randomUUID } from "node:crypto"; import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; -import { and, eq } from "drizzle-orm"; +import { and, eq, type SQL } from "drizzle-orm"; +import type { UpdateEnvironment } from "@paperclipai/shared"; +import { SANDBOX_PLATFORM_PROBE_METADATA_KEY } from "../services/environment-custom-image-runtime.js"; import { activityLog, agents, @@ -130,6 +132,65 @@ describeEmbeddedPostgres("environmentService leases", () => { return companyId; } + it("clears user metadata to null when no private platform proof exists", async () => { + const created = await svc.create({ name: "User metadata clear", driver: "sandbox", config: { provider: "daytona" }, metadata: { userNote: "remove" } }); + expect((await svc.update(created.id, { metadata: null }))?.metadata).toBeNull(); + }); + + it.each(["clear", "replace", "unrelated", "status", "config", "driver"] as const)( + "handles %s saves atomically after a concurrent platform probe record", async operation => { + const companyId = await seedCompany(); + const config = { provider: "daytona", image: `registry.example.test/runtime@sha256:${"a".repeat(64)}` }; + const staleProof = { v: 1, marker: "old" }; + const freshProof = { v: 1, marker: "new" }; + const created = await svc.create({ name: "Concurrent metadata", driver: "sandbox", config, + metadata: { userNote: "old", _paperclipManagedArchiveToken: "old-archive-token", + [SANDBOX_PLATFORM_PROBE_METADATA_KEY]: { [companyId]: staleProof } } }); + const patches: Record = { + clear: { metadata: null }, + replace: { metadata: { userNote: "replacement", [SANDBOX_PLATFORM_PROBE_METADATA_KEY]: { [companyId]: staleProof } } }, + unrelated: { description: "unrelated save" }, + status: { status: "active" }, + config: { config: { ...config, image: `registry.example.test/runtime@sha256:${"b".repeat(64)}` } }, + driver: { driver: "ssh", config: { host: "fixture.example.test", username: "fixture" } }, + }; + // Interleave the real probe UPDATE after the save is prepared but before its + // real UPDATE executes. This reproduces the stale route-read/write window. + const delayedWriteDb = { + update(table: typeof environments) { + const prepared = db.update(table); + return { + set(values: Parameters[0]) { + const statement = prepared.set(values); + return { + where(condition: SQL) { + const pending = statement.where(condition); + return { + async returning() { + expect(await svc.recordSandboxPlatformProbe({ + environmentId: created.id, companyId, config, proof: freshProof, + })).toBe(true); + return pending.returning(); + }, + }; + }, + }; + }, + }; + }, + }; + const saved = await svc.update(created.id, patches[operation], { db: delayedWriteDb as never }); + if (operation === "config" || operation === "driver") { + expect(saved?.metadata).not.toHaveProperty(SANDBOX_PLATFORM_PROBE_METADATA_KEY); + } else { + expect(saved?.metadata?.[SANDBOX_PLATFORM_PROBE_METADATA_KEY]).toEqual({ [companyId]: freshProof }); + } + if (operation === "clear") expect(saved?.metadata).toEqual({ [SANDBOX_PLATFORM_PROBE_METADATA_KEY]: { [companyId]: freshProof } }); + else expect(saved?.metadata?.userNote).toBe(operation === "replace" ? "replacement" : "old"); + if (operation !== "unrelated") expect(saved?.metadata).not.toHaveProperty("_paperclipManagedArchiveToken"); + }, + ); + it("acquires and releases a lease for a run", async () => { const { companyId, environmentId, runId } = await seedEnvironment(); diff --git a/server/src/routes/environments.ts b/server/src/routes/environments.ts index 1c253b45145..e0a24a73a15 100644 --- a/server/src/routes/environments.ts +++ b/server/src/routes/environments.ts @@ -1216,14 +1216,6 @@ export function environmentRoutes( // Persist the config change and its binding rows atomically: a binding // ref that fails validation (e.g. a deleted secret) must roll the whole // save back instead of leaving the config re-pointed with stale bindings. - if (changesProviderTarget || patch.metadata !== undefined) { - const metadata = { ...(patch.metadata ?? existing.metadata ?? {}) }; - if (changesProviderTarget) delete metadata[SANDBOX_PLATFORM_PROBE_METADATA_KEY]; - else if (existing.metadata?.[SANDBOX_PLATFORM_PROBE_METADATA_KEY] !== undefined) { - metadata[SANDBOX_PLATFORM_PROBE_METADATA_KEY] = existing.metadata[SANDBOX_PLATFORM_PROBE_METADATA_KEY]; - } - patch.metadata = metadata; - } const environment = await db.transaction(async (tx) => { const updated = await svc.update(existing.id, patch, { db: tx }); if (!updated) return null; diff --git a/server/src/services/environments.ts b/server/src/services/environments.ts index 74e96baaf83..7bf26c9d419 100644 --- a/server/src/services/environments.ts +++ b/server/src/services/environments.ts @@ -1,7 +1,7 @@ import { randomUUID } from "node:crypto"; import { SANDBOX_PLATFORM_PROBE_METADATA_KEY } from "./environment-custom-image-runtime.js"; -import { and, desc, eq, inArray, isNull, ne, sql } from "drizzle-orm"; +import { and, desc, eq, inArray, isNull, ne, sql, type SQL } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; import { agents, @@ -1135,20 +1135,30 @@ export function environmentService(db: Db) { if ("envVars" in patch && patch.envVars !== undefined) { values.envVars = (patch.envVars ?? {}) as Record; } + const changesTarget = patch.config !== undefined || patch.driver !== undefined; + let metadataUpdate: SQL | undefined; if (patch.metadata !== undefined) { - values.metadata = withoutManagedEnvironmentArchiveToken(patch.metadata ?? null); - } else if (patch.status !== undefined) { - const existingMetadata = await writeDb - .select({ metadata: environments.metadata }) - .from(environments) - .where(eq(environments.id, id)) - .then((rows) => rows[0]?.metadata ?? null); - values.metadata = withoutManagedEnvironmentArchiveToken(existingMetadata); + const userMetadata = withoutManagedEnvironmentArchiveToken(patch.metadata ?? null); + if (userMetadata) delete userMetadata[SANDBOX_PLATFORM_PROBE_METADATA_KEY]; + if (changesTarget) { + values.metadata = userMetadata; + } else { + const replacement = userMetadata === null ? sql`null::jsonb` : sql`${JSON.stringify(userMetadata)}::jsonb`; + // Preserve the current private subtree in the same statement. A route's + // earlier read must never replace a newer concurrent probe receipt. + metadataUpdate = sql`case when ${environments.metadata} ? ${SANDBOX_PLATFORM_PROBE_METADATA_KEY} + then coalesce(${replacement}, '{}'::jsonb) || jsonb_build_object( + ${SANDBOX_PLATFORM_PROBE_METADATA_KEY}::text, ${environments.metadata}->${SANDBOX_PLATFORM_PROBE_METADATA_KEY}) + else ${replacement} end`; + } + } else if (changesTarget || patch.status !== undefined) { + metadataUpdate = sql`${environments.metadata} - ${MANAGED_ENVIRONMENT_ARCHIVE_TOKEN_METADATA_KEY}`; + if (changesTarget) metadataUpdate = sql`${metadataUpdate} - ${SANDBOX_PLATFORM_PROBE_METADATA_KEY}`; } const row = await writeDb .update(environments) - .set(values) + .set({ ...values, ...(metadataUpdate ? { metadata: metadataUpdate } : {}) }) .where(eq(environments.id, id)) .returning() .then((rows) => rows[0] ?? null)