[Plugin] updateIf: trap serialization failures, fix all-undefined guard bypass, doc isolation

Three follow-ups on the guarded PluginStorageRepository.updateIf primitive:

1. Trap Postgres serialization failures. Under an isolation level stricter
   than READ COMMITTED the losing concurrent updateIf writers abort with
   SQLSTATE 40001 (serialization_failure) / 40P01 (deadlock_detected) instead
   of resolving to { applied: false }. A new exported StorageSerializationError
   (in storage-query.ts, alongside StorageQueryError) carries the SQLSTATE and
   cause and explains the READ COMMITTED assumption + retry guidance. A pure,
   unit-testable mapSerializationFailure(err) helper wraps 40001/40P01 and
   rethrows everything else unchanged; updateIf's catch is
   `throw mapSerializationFailure(err)`. SQLSTATE is read from err.code (and
   err.cause.code defensively) — confirmed empirically that Kysely propagates
   node-pg's DatabaseError.code unwrapped.

2. Fix the all-undefined delta/set guard bypass (both reviewers). Presence is
   now derived from DEFINED entries: undefined-valued set fields are filtered
   and undefined delta specs skipped BEFORE the "at least one of set/delta"
   check, so an all-undefined payload throws instead of doing a no-op write
   that bumped updated_at and returned { applied: true }. undefined values in
   set/delta are documented as ignored.

3. Doc the isolation contract on the updateIf doc-comment in both types.ts
   (interface) and plugin-storage.ts (impl): { applied: false } assumes READ
   COMMITTED; under REPEATABLE READ / SERIALIZABLE losing writers throw
   StorageSerializationError — the no-oversell SAFETY invariant holds either way.

Tests: all-undefined delta/set throw (integration, both dialects); deterministic
mapSerializationFailure unit tests (40001, 40P01, .cause.code nesting, 23505
pass-through, plain Error pass-through). Full core plugin suites green on both
dialects (804 tests).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TX9YciGFRZX9aF2UcQ6rUW

Author: 2 people

packages/core/src/database/repositories/plugin-storage.ts

@@ -18,6 +18,7 @@ import {
 	jsonOrderExtract,
 	StorageQueryError,
 	isInFilter,
+	StorageSerializationError,
 } from "../../plugins/storage-query.js";
 import type {
 	StorageCollection,
@@ -40,6 +41,59 @@ function isDeltaLike(value: unknown): value is { inc?: unknown; dec?: unknown }
 	return typeof value === "object" && value !== null;
 }
 
+/**
+ * SQLSTATEs a losing concurrent `updateIf` writer aborts with under an
+ * isolation level stricter than READ COMMITTED: `40001` (serialization_failure)
+ * and `40P01` (deadlock_detected).
+ */
+const SERIALIZATION_SQLSTATES = new Set(["40001", "40P01"]);
+
+/**
+ * Best-effort extraction of a Postgres SQLSTATE from a thrown driver error.
+ *
+ * node-pg sets `.code` on its `DatabaseError`, and Kysely propagates the driver
+ * error unwrapped (confirmed: a 23505 raised through Kysely surfaces `err.code`
+ * === "23505"). We also check `.cause.code` defensively so a future wrapping
+ * layer that nests the driver error as `cause` keeps working.
+ */
+function errSqlState(err: unknown): string | undefined {
+	if (typeof err !== "object" || err === null) return undefined;
+	const code = (err as { code?: unknown }).code;
+	if (typeof code === "string") return code;
+	const cause = (err as { cause?: unknown }).cause;
+	if (typeof cause === "object" && cause !== null) {
+		const causeCode = (cause as { code?: unknown }).code;
+		if (typeof causeCode === "string") return causeCode;
+	}
+	return undefined;
+}
+
+/**
+ * Map a thrown `updateIf` error to a {@link StorageSerializationError} when it
+ * is a Postgres serialization failure (`40001`) or deadlock (`40P01`);
+ * otherwise return it UNCHANGED.
+ *
+ * Pure and synchronous so it is unit-testable without provoking a live race —
+ * `updateIf`'s catch is simply `throw mapSerializationFailure(err)`. See
+ * {@link StorageSerializationError} for why these aborts happen only under an
+ * isolation level stricter than READ COMMITTED.
+ */
+export function mapSerializationFailure(err: unknown): unknown {
+	const sqlState = errSqlState(err);
+	if (sqlState !== undefined && SERIALIZATION_SQLSTATES.has(sqlState)) {
+		return new StorageSerializationError(
+			`updateIf lost a concurrent race (SQLSTATE ${sqlState}). Its ` +
+				`{ applied: false } contract assumes READ COMMITTED (the default); under ` +
+				`REPEATABLE READ / SERIALIZABLE the losing writer aborts instead of ` +
+				`resolving to { applied: false }. Retry the call, or run it at READ ` +
+				`COMMITTED. The no-oversell safety invariant still holds — a losing ` +
+				`writer never applies.`,
+			{ cause: err, sqlState },
+		);
+	}
+	return err;
+}
+
 /**
  * Interleave a `?`-placeholder SQL string with its params into a single
  * boolean raw expression. Used as a WHERE predicate directly — wrapping it
@@ -398,13 +452,33 @@ export class PluginStorageRepository<T = unknown> implements StorageCollection<T
 	 * rows-affected > 0). A missing row and a failed guard both yield 0 rows →
 	 * `{ applied: false }`; the two are intentionally indistinguishable. Never
 	 * inserts.
+	 *
+	 * `undefined` values in `set`/`delta` are IGNORED (they carry no write);
+	 * presence is derived from the DEFINED entries, so an all-`undefined` payload
+	 * (e.g. `{ set: { name: undefined } }`) hits the "at least one of set/delta"
+	 * error rather than doing a no-op write that bumps `updated_at`.
+	 *
+	 * ISOLATION: the `{ applied: false }` contract assumes READ COMMITTED (the
+	 * default). Under REPEATABLE READ / SERIALIZABLE the losing concurrent
+	 * writers throw {@link StorageSerializationError} (SQLSTATE `40001` / `40P01`)
+	 * instead of resolving to `{ applied: false }`. The no-oversell SAFETY
+	 * invariant holds either way — a losing writer never applies.
 	 */
 	async updateIf(id: string, args: UpdateIfArgs<T>): Promise<UpdateIfResult<T>> {
 		const { where, set, delta } = args;
 
-		const setEntries: Array<[string, unknown]> = set ? Object.entries(set) : [];
+		// Derive presence from DEFINED entries — `undefined` values carry no write
+		// and must not satisfy the "at least one of set/delta" requirement below
+		// (otherwise an all-`undefined` payload would do a no-op write that still
+		// bumps `updated_at` and returns `{ applied: true }`).
+		const setEntries: Array<[string, unknown]> = set
+			? Object.entries(set).filter(([, value]) => value !== undefined)
+			: [];
 		const hasSet = setEntries.length > 0;
-		const hasDelta = delta !== undefined && Object.keys(delta).length > 0;
+
+		const definedDeltaEntries: Array<[string, unknown]> =
+			delta !== undefined ? Object.entries(delta).filter(([, spec]) => spec !== undefined) : [];
+		const hasDelta = definedDeltaEntries.length > 0;
 
 		if (!hasSet && !hasDelta) {
 			throw new Error("updateIf requires at least one of `set` or `delta`.");
@@ -414,8 +488,7 @@ export class PluginStorageRepository<T = unknown> implements StorageCollection<T
 		const deltaEntries: Array<[string, number]> = [];
 		if (hasDelta) {
 			const setFieldSet = new Set(setEntries.map(([field]) => field));
-			for (const [field, spec] of Object.entries(delta)) {
-				if (spec === undefined) continue;
+			for (const [field, spec] of definedDeltaEntries) {
 				if (setFieldSet.has(field)) {
 					throw new Error(`updateIf: field "${field}" appears in both \`set\` and \`delta\`.`);
 				}
@@ -469,7 +542,16 @@ export class PluginStorageRepository<T = unknown> implements StorageCollection<T
 			query = query.where(rawWhereExpr(whereResult.sql, whereResult.params));
 		}
 
-		const row = await query.returning("data").executeTakeFirst();
+		// The `{ applied: false }` contract holds only under READ COMMITTED. Under
+		// a stricter isolation level the losing concurrent writers abort with a
+		// serialization failure / deadlock instead; translate those to a typed,
+		// retryable error and rethrow everything else unchanged.
+		let row: { data: string } | undefined;
+		try {
+			row = await query.returning("data").executeTakeFirst();
+		} catch (err) {
+			throw mapSerializationFailure(err);
+		}
 		if (!row) return { applied: false };
 		// eslint-disable-next-line typescript/no-unsafe-type-assertion -- JSON.parse returns any; generic callers provide T
 		const data = JSON.parse(row.data) as T;

packages/core/src/plugins/storage-query.ts

@@ -25,6 +25,33 @@ export class StorageQueryError extends Error {
 	}
 }
 
+/**
+ * Error thrown when a guarded `updateIf` loses a concurrent race under an
+ * isolation level stricter than READ COMMITTED.
+ *
+ * `updateIf`'s `{ applied: false }` contract (row absent OR guard failed)
+ * assumes READ COMMITTED — the default. There, a losing concurrent writer
+ * re-evaluates the guard against the winner's freshly committed row and
+ * cleanly resolves to `{ applied: false }`. Under REPEATABLE READ /
+ * SERIALIZABLE the loser cannot re-read against a newer snapshot, so it aborts
+ * with SQLSTATE `40001` (serialization_failure) or `40P01` (deadlock_detected)
+ * instead. This error surfaces that abort so the caller can retry the write
+ * (or run it at READ COMMITTED).
+ *
+ * The no-oversell SAFETY invariant holds either way: a losing writer NEVER
+ * applies its update — it either sees `{ applied: false }` or throws here.
+ */
+export class StorageSerializationError extends Error {
+	/** The Postgres SQLSTATE that triggered this error (`40001` / `40P01`). */
+	readonly sqlState?: string;
+
+	constructor(message: string, options?: { cause?: unknown; sqlState?: string }) {
+		super(message, { cause: options?.cause });
+		this.name = "StorageSerializationError";
+		this.sqlState = options?.sqlState;
+	}
+}
+
 /**
  * Check if a value is a range filter
  */

packages/core/src/plugins/types.ts

@@ -216,6 +216,13 @@ export interface StorageCollection<T = unknown> {
 	 * arithmetic live in one statement, so N concurrent guarded decrements
 	 * serialize correctly. `applied: false` means the row was absent OR the
 	 * guard failed (the two are intentionally indistinguishable). Never inserts.
+	 *
+	 * ISOLATION: the `applied: false` contract assumes READ COMMITTED (the
+	 * default). Under REPEATABLE READ / SERIALIZABLE the losing concurrent
+	 * writers throw `StorageSerializationError` (SQLSTATE `40001` / `40P01`)
+	 * instead of resolving to `applied: false` — the caller should retry (or run
+	 * at READ COMMITTED). The no-oversell SAFETY invariant holds either way: a
+	 * losing writer never applies.
 	 */
 	updateIf(id: string, args: UpdateIfArgs<T>): Promise<UpdateIfResult<T>>;
 }

packages/core/tests/integration/plugins/storage-updateif.test.ts

@@ -179,6 +179,27 @@ describeEachDialect("Plugin storage updateIf", (dialect) => {
 		await expect(repo.updateIf("p1", { where: { sku: "A" } })).rejects.toThrow(/set.*delta/i);
 	});
 
+	it("updateIf() with an all-`undefined` delta (no set) throws and never writes", async () => {
+		// `{ stock: undefined }` has a key but no DEFINED entry — presence is
+		// derived from defined entries, so this hits the "at least one" guard
+		// instead of doing a no-op write that bumps updated_at (#reviewer flag).
+		const repo = productsRepo();
+		await repo.put("p1", { sku: "A", stock: 5, tier: 1, name: "Alpha" });
+		await expect(
+			repo.updateIf("p1", { where: { sku: "A" }, delta: { stock: undefined } }),
+		).rejects.toThrow(/set.*delta/i);
+		expect((await repo.get("p1"))?.stock).toBe(5);
+	});
+
+	it("updateIf() with an all-`undefined` set (no delta) throws and never writes", async () => {
+		const repo = productsRepo();
+		await repo.put("p1", { sku: "A", stock: 5, tier: 1, name: "Alpha" });
+		await expect(
+			repo.updateIf("p1", { where: { sku: "A" }, set: { name: undefined } }),
+		).rejects.toThrow(/set.*delta/i);
+		expect((await repo.get("p1"))?.name).toBe("Alpha");
+	});
+
 	// ── guard operator coverage ─────────────────────────────────────────────
 
 	it("updateIf() guard covers equality, multi-digit gte, in, startsWith, and a non-indexed field", async () => {

packages/core/tests/unit/plugins/plugin-storage.test.ts

@@ -1,10 +1,16 @@
 import type { Kysely } from "kysely";
 import { describe, it, expect, beforeEach, afterEach } from "vitest";
 
-import { PluginStorageRepository } from "../../../src/database/repositories/plugin-storage.js";
+import {
+	PluginStorageRepository,
+	mapSerializationFailure,
+} from "../../../src/database/repositories/plugin-storage.js";
 import type { Database } from "../../../src/database/types.js";
 import { IdentifierError } from "../../../src/database/validate.js";
-import { StorageQueryError } from "../../../src/plugins/storage-query.js";
+import {
+	StorageQueryError,
+	StorageSerializationError,
+} from "../../../src/plugins/storage-query.js";
 import { setupTestDatabase, teardownTestDatabase } from "../../utils/test-db.js";
 
 interface TestDocument {
@@ -441,3 +447,46 @@ describe("PluginStorageRepository", () => {
 		});
 	});
 });
+
+describe("mapSerializationFailure", () => {
+	// Pure/deterministic: proves the SQLSTATE trap without provoking a live race.
+	// node-pg sets `.code` on its DatabaseError and Kysely propagates it
+	// unwrapped, so a fake `{ code }` faithfully stands in for the real throw.
+
+	it("wraps a 40001 (serialization_failure) into StorageSerializationError, preserving cause", () => {
+		const original = { code: "40001", message: "could not serialize access" };
+		const mapped = mapSerializationFailure(original);
+		expect(mapped).toBeInstanceOf(StorageSerializationError);
+		const err = mapped as StorageSerializationError;
+		expect(err.sqlState).toBe("40001");
+		expect(err.cause).toBe(original);
+		expect(err.message).toMatch(/40001/);
+		expect(err.message).toMatch(/READ COMMITTED/);
+	});
+
+	it("wraps a 40P01 (deadlock_detected) into StorageSerializationError, preserving cause", () => {
+		const original = { code: "40P01", message: "deadlock detected" };
+		const mapped = mapSerializationFailure(original);
+		expect(mapped).toBeInstanceOf(StorageSerializationError);
+		const err = mapped as StorageSerializationError;
+		expect(err.sqlState).toBe("40P01");
+		expect(err.cause).toBe(original);
+	});
+
+	it("detects the SQLSTATE nested on `.cause.code` too", () => {
+		const original = { cause: { code: "40001" } };
+		const mapped = mapSerializationFailure(original);
+		expect(mapped).toBeInstanceOf(StorageSerializationError);
+		expect((mapped as StorageSerializationError).sqlState).toBe("40001");
+	});
+
+	it("passes a non-serialization SQLSTATE (23505) through unchanged", () => {
+		const original = { code: "23505", message: "duplicate key" };
+		expect(mapSerializationFailure(original)).toBe(original);
+	});
+
+	it("passes a plain Error (no code) through unchanged", () => {
+		const original = new Error("boom");
+		expect(mapSerializationFailure(original)).toBe(original);
+	});
+});