From f3e01c63bda2cee4c1ad9560b32c71b83244d1f5 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Wed, 17 Jun 2026 16:09:45 -0700 Subject: [PATCH] fix(environments): partial unique index to dedup managed sandbox rows (#8247) 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. > - The `server/services/environments.ts` module lazily provisions a managed Kubernetes sandbox environment for each company on first heartbeat. Idempotency relies on `ensureKubernetesEnvironment` returning the single managed row per company. > - The `(company_id, driver)` unique index in the `environments` schema is partial on `driver='local'` only, so two concurrent callers (e.g. simultaneous first heartbeats from a freshly synced tenant) can both insert a `driver='sandbox'` row before either sees the other. > - The function tried to converge after the race by re-reading, picking the oldest managed row as winner, and deleting the loser. Under autocommit + read-committed, each post-insert SELECT is a fresh snapshot — A may not see B's row, B may not see A's, so both pick their own and neither deletes. Two rows survive. > - The same race fired in CI as `expected 2 to be 1` at `environment-service.test.ts:293`, gating multiple unrelated PRs on retries. > - This pull request encodes the operator-level invariant ("at most one Paperclip-managed sandbox row per company") at the DB layer with a partial unique index, then switches `ensureKubernetesEnvironment` to `INSERT … ON CONFLICT DO NOTHING` keyed on that index. Losers re-read the surviving row. > - The benefit is the race is impossible by construction — no application-side convergence loop, no test flake, and any future `ensureXyzSandboxEnvironment` that sets `managedByPaperclip=true` inherits the invariant for free. ## Linked Issues or Issue Description Paperclip issue: PAPA-783 — implement managed-sandbox dedup fix (phase 2 of the approved plan on the parent flake-investigation issue). This is the phase-2 fix for the flaky `environmentService > deduplicates concurrent managed Kubernetes environment creation` test introduced by `4ad94d0bd` (PR #7938). Failing CI runs since then on at least PRs #7595, #8233, #8215, #8212. The plan was reviewed and approved on the parent issue before implementation. Closely related (not duplicates): - PR #7938 — introduced the test and the in-process convergence loop being replaced here. - PR #7595, PR #8233, PR #8215, PR #8212 — downstream PRs affected by the flake; one of them will be rebased onto this fix as the acceptance gate. ## What Changed - `packages/db/src/schema/environments.ts`: added `environments_company_managed_sandbox_idx`, a partial unique index on `(company_id) WHERE driver='sandbox' AND (metadata->>'managedByPaperclip')::boolean = true`. The umbrella `managedByPaperclip` predicate covers any current or future Paperclip-managed sandbox flavor without needing a new index per provider. - `packages/db/src/migrations/0102_managed_sandbox_dedup_index.sql`: one-shot dedup `DELETE` keeping the oldest managed-sandbox row per `company_id` (scoped to `driver='sandbox' AND managedByPaperclip=true`), `RAISE NOTICE` if any duplicates were removed, then `CREATE UNIQUE INDEX IF NOT EXISTS environments_company_managed_sandbox_idx`. `CONCURRENTLY` is omitted because the codebase's migration runner wraps each file in a transaction (see `applyPendingMigrationsManually`); the table holds 1–3 rows per company, so the short lock is acceptable and consistent with every other migration in the repo. - `server/src/services/environments.ts`: `ensureKubernetesEnvironment` now uses `INSERT … ON CONFLICT DO NOTHING` keyed on the new index. On conflict it re-reads the surviving managed-sandbox row and returns it. Drops the post-insert convergence (re-read by `createdAt ASC, id ASC`, delete the loser) and the trailing comment that flagged "until a partial unique index is added via migration" as the proper long-term fix. - Unused `asc` import removed from `server/src/services/environments.ts`. ## Verification Local (matches the success criteria in the issue body): ``` $ cd server $ passes=0; for i in $(seq 1 20); do pnpm vitest run src/__tests__/environment-service.test.ts -t "deduplicates concurrent" \ && passes=$((passes+1)) || break done; echo "$passes/20" 20/20 $ passes=0; for i in $(seq 1 10); do pnpm vitest run src/__tests__/environment-service.test.ts \ && passes=$((passes+1)) || break done; echo "$passes/10" 10/10 ``` Adversarial fan-out stress (temporarily bumped `Array.from({ length: 8 }, …)` to `length: 32` on both dedup tests; reverted before commit): ``` $ # both dedup tests fan-out-of-32, 10 iterations 10/10 ``` Two ensure paths exist in the parent plan, but only `ensureKubernetesEnvironment` is on `master`. `ensureManagedSandboxEnvironment` (referenced by the approved plan as commit `dce9a9622`) lives only on an unmerged feature branch, not master. The plan's helper-extraction and symmetric dedup test for that path are deferred to whichever PR lands the second ensure path — it inherits the same DB invariant by setting `managedByPaperclip=true`. Discrepancy was flagged on the issue thread before implementation. Typecheck: ``` $ pnpm -C server typecheck ok ``` ## Risks Low risk. - **Migration safety.** `IF NOT EXISTS` on the index makes the migration idempotent. The dedup `DELETE` is bounded to rows matching the managed-sandbox predicate; in production this should be a no-op (no race has been reported in the wild — only in CI). On dev/CI DBs that already accumulated duplicates, the migration emits a `NOTICE` reporting the count. - **No `CONCURRENTLY`.** The migration runner wraps each `.sql` file in a transaction, which is incompatible with `CREATE INDEX CONCURRENTLY`. The `environments` table holds 1–3 rows per company and the row count is bounded by company count; the short ACCESS EXCLUSIVE during `CREATE UNIQUE INDEX` is acceptable here and matches every other index migration in the repo. - **Predicate scope.** The partial index predicate matches exactly the rows that `ensureKubernetesEnvironment` writes (`driver='sandbox'` with `metadata.managedByPaperclip=true`). Tenant-created sandbox envs (via `svc.create`) do not set this marker and are not covered — no false positives, no surprise constraint violations on unrelated inserts. ## Model Used Claude (Anthropic), `claude-opus-4-7`. Tool use: code edit + bash + filesystem search; no extended-thinking mode. ## 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 run tests locally and they pass - [x] I have added or updated tests where applicable - [x] If this change affects the UI, I have included before/after screenshots - [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 - [ ] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Paperclip --- .../0102_managed_sandbox_dedup_index.sql | 33 +++++++++ packages/db/src/migrations/meta/_journal.json | 7 ++ packages/db/src/schema/environments.ts | 5 ++ .../src/__tests__/environment-service.test.ts | 68 +++++++++++++++++++ server/src/services/environments.ts | 34 ++++------ 5 files changed, 128 insertions(+), 19 deletions(-) create mode 100644 packages/db/src/migrations/0102_managed_sandbox_dedup_index.sql diff --git a/packages/db/src/migrations/0102_managed_sandbox_dedup_index.sql b/packages/db/src/migrations/0102_managed_sandbox_dedup_index.sql new file mode 100644 index 00000000..5f9c6da4 --- /dev/null +++ b/packages/db/src/migrations/0102_managed_sandbox_dedup_index.sql @@ -0,0 +1,33 @@ +-- One-shot dedup of any pre-existing managed sandbox rows duplicated per company. +-- Keeps the oldest row per (company_id) where driver='sandbox' AND metadata.managedByPaperclip is true. +-- Scoped to Paperclip-managed rows only: tenant-created sandbox envs (without the marker) are untouched. +DO $$ +DECLARE + deleted_count integer; +BEGIN + WITH ranked AS ( + SELECT + id, + row_number() OVER ( + PARTITION BY company_id + ORDER BY created_at ASC, id ASC + ) AS rn + FROM environments + WHERE driver = 'sandbox' + AND (metadata ->> 'managedByPaperclip')::boolean = true + ), + deleted AS ( + DELETE FROM environments + WHERE id IN (SELECT id FROM ranked WHERE rn > 1) + RETURNING 1 + ) + SELECT count(*) INTO deleted_count FROM deleted; + + IF deleted_count > 0 THEN + RAISE NOTICE 'environments_company_managed_sandbox_idx dedup removed % duplicate managed-sandbox row(s)', deleted_count; + END IF; +END $$; +--> statement-breakpoint +CREATE UNIQUE INDEX IF NOT EXISTS "environments_company_managed_sandbox_idx" + ON "environments" ("company_id") + WHERE driver = 'sandbox' AND (metadata ->> 'managedByPaperclip')::boolean = true; diff --git a/packages/db/src/migrations/meta/_journal.json b/packages/db/src/migrations/meta/_journal.json index 7e19d3b4..55ff7687 100644 --- a/packages/db/src/migrations/meta/_journal.json +++ b/packages/db/src/migrations/meta/_journal.json @@ -715,6 +715,13 @@ "when": 1781490000000, "tag": "0101_plugin_company_id_tenant_isolation", "breakpoints": true + }, + { + "idx": 102, + "version": "7", + "when": 1781490100000, + "tag": "0102_managed_sandbox_dedup_index", + "breakpoints": true } ] } \ No newline at end of file diff --git a/packages/db/src/schema/environments.ts b/packages/db/src/schema/environments.ts index 7bb308d8..e64e5c41 100644 --- a/packages/db/src/schema/environments.ts +++ b/packages/db/src/schema/environments.ts @@ -21,6 +21,11 @@ export const environments = pgTable( companyDriverIdx: uniqueIndex("environments_company_driver_idx") .on(table.companyId, table.driver) .where(sql`${table.driver} = 'local'`), + companyManagedSandboxIdx: uniqueIndex("environments_company_managed_sandbox_idx") + .on(table.companyId) + .where( + sql`${table.driver} = 'sandbox' AND (${table.metadata} ->> 'managedByPaperclip')::boolean = true`, + ), companyNameIdx: index("environments_company_name_idx").on(table.companyId, table.name), }), ); diff --git a/server/src/__tests__/environment-service.test.ts b/server/src/__tests__/environment-service.test.ts index 3232301d..37788d9c 100644 --- a/server/src/__tests__/environment-service.test.ts +++ b/server/src/__tests__/environment-service.test.ts @@ -300,6 +300,74 @@ describeEmbeddedPostgres("environmentService leases", () => { expect((rows[0]?.metadata as Record)?.managedKubernetesSandbox).toBe(true); }); + it("rejects a second managed-sandbox row for the same company at the DB level", async () => { + const companyId = randomUUID(); + await db.insert(companies).values({ + id: companyId, + name: "Acme", + status: "active", + createdAt: new Date(), + updatedAt: new Date(), + }); + + const now = new Date(); + await db.insert(environments).values({ + companyId, + name: "First", + driver: "sandbox", + status: "active", + config: { provider: "kubernetes" }, + metadata: { managedByPaperclip: true, managedKubernetesSandbox: true }, + createdAt: now, + updatedAt: now, + }); + + // Partial unique index environments_company_managed_sandbox_idx rejects a + // second row matching driver='sandbox' AND managedByPaperclip=true for the + // same company. This is the DB-level invariant that replaced the previous + // application-side post-insert convergence loop. + const secondInsert = db.insert(environments).values({ + companyId, + name: "Second", + driver: "sandbox", + status: "active", + config: { provider: "kubernetes" }, + metadata: { managedByPaperclip: true, managedKubernetesSandbox: true }, + createdAt: new Date(now.getTime() + 1), + updatedAt: new Date(now.getTime() + 1), + }); + let raisedConstraint: string | null = null; + try { + await secondInsert; + } catch (error) { + raisedConstraint = + (error as { constraint_name?: string; cause?: { constraint_name?: string } }) + ?.constraint_name ?? + (error as { cause?: { constraint_name?: string } })?.cause?.constraint_name ?? + "unknown"; + } + expect(raisedConstraint).toBe("environments_company_managed_sandbox_idx"); + + // Index does NOT cover tenant-created sandbox rows (no managedByPaperclip + // marker) — operators must be able to keep multiple tenant sandbox envs. + await db.insert(environments).values({ + companyId, + name: "Tenant Sandbox", + driver: "sandbox", + status: "active", + config: { provider: "fake" }, + metadata: { tenant: true }, + createdAt: new Date(now.getTime() + 2), + updatedAt: new Date(now.getTime() + 2), + }); + + const rows = await db + .select() + .from(environments) + .where(and(eq(environments.companyId, companyId), eq(environments.driver, "sandbox"))); + expect(rows).toHaveLength(2); + }); + it("does not treat a non-kubernetes sandbox environment as the managed k8s env", async () => { const companyId = randomUUID(); await db.insert(companies).values({ diff --git a/server/src/services/environments.ts b/server/src/services/environments.ts index e4287a3c..69aaa5c8 100644 --- a/server/src/services/environments.ts +++ b/server/src/services/environments.ts @@ -1,4 +1,4 @@ -import { and, asc, desc, eq, sql } from "drizzle-orm"; +import { and, desc, eq, sql } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; import { environmentLeases, environments } from "@paperclipai/db"; import { @@ -235,7 +235,12 @@ export function environmentService(db: Db) { return toEnvironment(updated); } - const row = await db + // The partial unique index `environments_company_managed_sandbox_idx` + // (added in migration 0102) enforces "at most one Paperclip-managed + // sandbox row per company" at the DB level. Use ON CONFLICT DO NOTHING + // keyed on that index so concurrent callers can race the INSERT — only + // one succeeds; losers re-read the surviving row. + const inserted = await db .insert(environments) .values({ companyId, @@ -248,26 +253,18 @@ export function environmentService(db: Db) { createdAt: now, updatedAt: now, }) + .onConflictDoNothing({ + target: [environments.companyId], + where: sql`${environments.driver} = 'sandbox' AND (${environments.metadata} ->> 'managedByPaperclip')::boolean = true`, + }) .returning() .then((rows) => rows[0] ?? null); - if (!row) { - throw new Error("Failed to ensure kubernetes environment"); - } + if (inserted) return toEnvironment(inserted); - // Concurrency: the schema's (companyId, driver) unique index is partial - // on driver='local' only, so there is no DB constraint stopping two - // simultaneous callers (e.g. concurrent heartbeats lazily provisioning a - // new company) from both inserting a managed k8s row. Until a partial - // unique index on (companyId, driver) WHERE the managed marker exists is - // added via migration (the proper long-term fix), converge here: re-read, - // deterministically prefer the oldest managed row, and delete our own - // insert if it lost the race. Both racers compute the same winner, so - // duplicates self-heal instead of persisting. const winner = await db .select() .from(environments) .where(and(eq(environments.companyId, companyId), eq(environments.driver, "sandbox"))) - .orderBy(asc(environments.createdAt), asc(environments.id)) .then( (rows) => rows.find( @@ -277,11 +274,10 @@ export function environmentService(db: Db) { ] === true, ) ?? null, ); - if (winner && winner.id !== row.id) { - await db.delete(environments).where(eq(environments.id, row.id)); - return toEnvironment(winner); + if (!winner) { + throw new Error("Failed to ensure kubernetes environment"); } - return toEnvironment(row); + return toEnvironment(winner); }, /**