Arnike/paperclip
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(environments): partial unique index to dedup managed sandbox rows (#8247)

Browse Source 
## 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 <noreply@paperclip.ing>
This commit is contained in:
Devin Foley
2026-06-17 16:09:45 -07:00
committed by GitHub
parent e59eb1080d
commit f3e01c63bd
5 changed files with 128 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files
server/src/__tests__/environment-service.test.ts
+68
View File
@@ -300,6 +300,74 @@ describeEmbeddedPostgres("environmentService leases", () => {
expect((rows[0]?.metadata as Record<string, unknown>)?.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({
server/src/services/environments.ts
+15 -19
View File
@@ -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);
},
/**