diff --git a/server/src/__tests__/environment-instance-routes.test.ts b/server/src/__tests__/environment-instance-routes.test.ts index f2b26355ed..51664e01d9 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 cfa81980bf..4f953121e3 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 1c253b4514..e0a24a73a1 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 74e96baaf8..7bf26c9d41 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)