Skip to content

Commit f6cc5dd

Browse files
committed
apply review: discriminating test, one-line changeset, trimmed comments
- delete policy-transactional-visibility.test.ts (passed with main's executor.ts swapped in - not discriminating; sequential awaits are no concurrency proof) - add a real concurrent-creates case to policies.test.ts (verified to fail on main, pass here) - changeset to one sentence per repo norm - shorten the two executor.ts comment blocks to one line each
1 parent 016ae03 commit f6cc5dd

4 files changed

Lines changed: 20 additions & 160 deletions

File tree

.changeset/policy-transactional-visibility.md

Lines changed: 1 addition & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,18 +2,4 @@
22
"@executor-js/sdk": patch
33
---
44

5-
fix: make tool-policy writes transactional
6-
7-
`policiesCreate` and `policiesUpdate` previously ran their read-decide-write
8-
(existing-row scan → position computation → create, or existence check →
9-
update → re-read) as unsequenced statements. Two concurrent policy edits
10-
could interleave their reads and writes — both computing positions or
11-
updates from the same stale snapshot, silently overwriting each other or
12-
observing torn state.
13-
14-
Both paths now run inside the same transaction wrapper the credential and
15-
integration upserts use (`fuma.transaction`, real BEGIN/COMMIT on
16-
libSQL/Postgres). Concurrent creates/updates serialize; each commits its
17-
own sequenced write, and an invocation's policy read at its call boundary
18-
sees committed state only — a revoked or blocked rule takes effect at the
19-
next invocation, never silently bypassed and never half-applied.
5+
Wrap tool-policy create and update in a transaction so concurrent edits can no longer read the same snapshot and commit duplicate positions or overwrite each other.

packages/core/sdk/src/executor.ts

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -5396,13 +5396,7 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea
53965396
try: () => ownedKeys(input.owner),
53975397
catch: (cause) => storageFailureFromUnknown("invalid owner", cause),
53985398
});
5399-
// The read-decide-write (existing-row scan → specificity-aware
5400-
// position → create) runs inside ONE transaction so two concurrent
5401-
// policy creates can never interleave their scans and both commit a
5402-
// rule at the same position, or a create observe a torn sibling
5403-
// write. Same discipline as the credential/integration upserts:
5404-
// validation + ownership checks stay outside (no DB writes), the
5405-
// sequenced DB work is atomic.
5399+
// Scan → position → insert runs atomically so concurrent creates cannot commit duplicate positions.
54065400
return yield* transaction(
54075401
Effect.gen(function* () {
54085402
const existing = yield* core.findMany("tool_policy", {
@@ -5444,11 +5438,7 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea
54445438
});
54455439
}
54465440
const where = (b: AnyCb) => b.and(byOwner(input.owner)(b), b("id", "=", input.id));
5447-
// Existence check → update → re-read inside ONE transaction: a
5448-
// concurrent update cannot interleave between the existence check and
5449-
// the write, so two racing updates both land (sequenced commits) and
5450-
// neither observes the other's torn state. The returned row is the
5451-
// committed post-update row, never a stale pre-update projection.
5441+
// Existence check, write, and re-read commit together.
54525442
return yield* transaction(
54535443
Effect.gen(function* () {
54545444
const existing = yield* core.findFirst("tool_policy", { where });

packages/core/sdk/src/policies.test.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -428,6 +428,23 @@ describe("executor.policies", () => {
428428
}),
429429
);
430430

431+
it.live("concurrent creates of equally specific rules get distinct positions", () =>
432+
Effect.gen(function* () {
433+
const executor = yield* setupExecutor();
434+
yield* Effect.all(
435+
[
436+
executor.policies.create({ owner: "org", pattern: "vercel.dns.create", action: "block" }),
437+
executor.policies.create({ owner: "org", pattern: "vercel.dns.delete", action: "block" }),
438+
],
439+
{ concurrency: "unbounded" },
440+
);
441+
442+
const rules = yield* executor.policies.list();
443+
expect(rules).toHaveLength(2);
444+
expect(new Set(rules.map((r) => r.position)).size).toBe(2);
445+
}),
446+
);
447+
431448
it.effect("create stores rules at the requested owner", () =>
432449
Effect.gen(function* () {
433450
const executor = yield* setupExecutor();

packages/core/sdk/src/policy-transactional-visibility.test.ts

Lines changed: 0 additions & 133 deletions
This file was deleted.

0 commit comments

Comments
 (0)