FN-9138: guard same-millisecond approval audit ID collisions
Validate that lifecycle guards prevent duplicate deterministic approval audit IDs during concurrent writes. - Add database-free coverage for losing and successful guarded approval transitions. - Add PostgreSQL overlap probes for decisions, completions, and physical ID uniqueness. - Document the deterministic audit identity and lifecycle ordering guarantees. Files changed: docs/storage.md | 2 +- .../approval-request-audit-id-race.test.ts | 115 ++++++++++++++++++ .../postgres/approval-request-lifecycle.pg.test.ts | 132 ++++++++++++++++++++- .../async-stores/async-approval-request-store.ts | 10 ++ 4 files changed, 253 insertions(+), 6 deletions(-) Fusion-Task-Id: FN-9138 Fusion-Task-Lineage: ccfc084f-2cd8-4549-8a42-242167139b8d Co-authored-by: Fusion (runfusion.ai) <noreply@runfusion.ai>
This commit is contained in:
gsxdsm
@@ -511,7 +511,7 @@ The `tasks.cumulativeActiveMs` and `tasks.executionCompletedAt` columns are the
|
||||
| `agents` | Agent registry/state/task assignment metadata. |
|
||||
| `agentHeartbeats` | Heartbeat run events linked to agents (`agentId` FK cascade). |
|
||||
| `approval_requests` | Durable approval request records: requester actor snapshot, target action payload (category/action/resource/context), lifecycle status (`pending`/`approved`/`denied`/`completed`), optional task/run context, and requested/decided/completed timestamps. |
|
||||
| `approval_request_audit_events` | Append-only audit trail for approval requests. PostgreSQL uses the physical `(project_id, id)` identity, while public `ApprovalRequestStore.getAuditHistory` scopes bound audit-history reads; Command Center analytics remains intentionally unbound-tolerant. The ownership trigger normalizes only NULL/exact `''`, so a whitespace-only binding is stored literally. Rows store event type (`created`/`approved`/`denied`/`completed`), immutable actor snapshot, optional note, and deterministic per-request ordering by `(createdAt, rowid)`. |
|
||||
| `approval_request_audit_events` | Append-only audit trail for approval requests. PostgreSQL uses the physical `(project_id, id)` identity, while public `ApprovalRequestStore.getAuditHistory` scopes bound audit-history reads; Command Center analytics remains intentionally unbound-tolerant. The ownership trigger normalizes only NULL/exact `''`, so a whitespace-only binding is stored literally. IDs are deterministic `aprevt-<eventType>-<requestId>-<createdAt>` values: request identity and lifecycle guards ensure one same-type event per request/timestamp within a partition, while different event types are distinct; history ties use lifecycle rank before ID. Rows store event type (`created`/`approved`/`denied`/`completed`), immutable actor snapshot, and optional note. |
|
||||
| `secrets` | Encrypted secret KV rows (`key` unique) with raw BLOB `value_ciphertext` + per-row random `nonce` (AES-256-GCM), per-secret `access_policy` CHECK (`auto`/`prompt`/`deny`), env-materialization metadata (`env_exportable`, `env_export_key`), and read-audit fields (`last_read_at`, `last_read_by`). Plaintext is never written to the database. |
|
||||
| `task_documents` | Task-scoped document metadata/content keyed by `(taskId, key)` with current revision pointer. |
|
||||
| `task_document_revisions` | Immutable revision history for task documents (content snapshots by revision). |
|
||||
|
||||
@@ -0,0 +1,115 @@
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import {
|
||||
decideApprovalRequest,
|
||||
markApprovalRequestCompleted,
|
||||
} from "../async-stores/async-approval-request-store.js";
|
||||
import type { AsyncDataLayer, DbTransaction } from "../postgres/data-layer.js";
|
||||
import type { ApprovalRequestStatus } from "../types.js";
|
||||
|
||||
const REQUESTER = { actorId: "agent-1", actorType: "agent" as const, actorName: "Requester" };
|
||||
const DECIDER = { actorId: "user-1", actorType: "user" as const, actorName: "Decider" };
|
||||
const NOW = "2026-08-16T23:22:00.000Z";
|
||||
|
||||
/**
|
||||
* Build only the Drizzle chain surface exercised by the real transition helpers.
|
||||
* The stale read followed by an empty guarded-update returning result is the
|
||||
* production racer's state after another transaction commits between its read
|
||||
* and conditional update.
|
||||
*/
|
||||
function createRacingLayer(status: ApprovalRequestStatus, updateWins: boolean) {
|
||||
const auditInsert = vi.fn(async () => undefined);
|
||||
const row = {
|
||||
id: "apr-race",
|
||||
status,
|
||||
requesterActorId: REQUESTER.actorId,
|
||||
requesterActorType: REQUESTER.actorType,
|
||||
requesterActorName: REQUESTER.actorName,
|
||||
targetActionCategory: "other",
|
||||
targetActionOperation: "test",
|
||||
targetActionSummary: "test",
|
||||
targetResourceType: "task",
|
||||
targetResourceId: "FN-9138",
|
||||
targetContext: {},
|
||||
taskId: null,
|
||||
runId: null,
|
||||
requestedAt: "2026-08-16T23:00:00.000Z",
|
||||
decidedAt: status === "approved" ? "2026-08-16T23:01:00.000Z" : null,
|
||||
completedAt: null,
|
||||
createdAt: "2026-08-16T23:00:00.000Z",
|
||||
updatedAt: "2026-08-16T23:00:00.000Z",
|
||||
};
|
||||
const tx = {
|
||||
select: () => ({ from: () => ({ where: () => Promise.resolve([row]) }) }),
|
||||
update: () => ({
|
||||
set: () => ({
|
||||
where: () => ({ returning: () => Promise.resolve(updateWins ? [{ id: row.id }] : []) }),
|
||||
}),
|
||||
}),
|
||||
insert: () => ({ values: auditInsert }),
|
||||
} as unknown as DbTransaction;
|
||||
const layer = {
|
||||
projectId: "fn-9138-project",
|
||||
transactionImmediate: async <T>(fn: (transaction: DbTransaction) => Promise<T>) => fn(tx),
|
||||
} as AsyncDataLayer;
|
||||
return { layer, auditInsert };
|
||||
}
|
||||
|
||||
/*
|
||||
FNXC:ApprovalAuditIdentity 2026-08-16-23:22:
|
||||
A same-millisecond same-type audit ID is safe only while the lifecycle writes at
|
||||
most one successful transition of that type. This database-free seam simulates a
|
||||
stale non-terminal racer: its guarded update returns no rows, so real helpers
|
||||
throw the dashboard-mapped transition conflict before reaching the append-only
|
||||
insert. Keep a successful control beside each losing case so this test fails if
|
||||
its stub no longer exercises the production audit path.
|
||||
*/
|
||||
describe("approval audit deterministic-ID guarded-update race", () => {
|
||||
it.each([
|
||||
["approved", "pending", "approved"] as const,
|
||||
["denied", "pending", "denied"] as const,
|
||||
["completed", "approved", "completed"] as const,
|
||||
])("rejects a stale %s racer before it mints a second audit row", async (eventType, staleStatus, targetStatus) => {
|
||||
vi.useFakeTimers({ toFake: ["Date"] });
|
||||
vi.setSystemTime(new Date(NOW));
|
||||
try {
|
||||
const { layer, auditInsert } = createRacingLayer(staleStatus, false);
|
||||
|
||||
const operation = eventType === "completed"
|
||||
? markApprovalRequestCompleted(layer, "apr-race", { actor: DECIDER })
|
||||
: decideApprovalRequest(layer, "apr-race", targetStatus as "approved" | "denied", { actor: DECIDER });
|
||||
|
||||
await expect(operation).rejects.toThrow(`Invalid approval request transition: ${staleStatus} -> ${targetStatus}`);
|
||||
expect(auditInsert).not.toHaveBeenCalled();
|
||||
} finally {
|
||||
vi.useRealTimers();
|
||||
}
|
||||
});
|
||||
|
||||
it.each([
|
||||
["approved", "pending", "approved"] as const,
|
||||
["denied", "pending", "denied"] as const,
|
||||
["completed", "approved", "completed"] as const,
|
||||
])("inserts one deterministic %s audit ID after a successful guarded update", async (eventType, staleStatus, targetStatus) => {
|
||||
vi.useFakeTimers({ toFake: ["Date"] });
|
||||
vi.setSystemTime(new Date(NOW));
|
||||
try {
|
||||
const { layer, auditInsert } = createRacingLayer(staleStatus, true);
|
||||
if (eventType === "completed") {
|
||||
await markApprovalRequestCompleted(layer, "apr-race", { actor: DECIDER });
|
||||
} else {
|
||||
await decideApprovalRequest(layer, "apr-race", targetStatus as "approved" | "denied", { actor: DECIDER });
|
||||
}
|
||||
|
||||
expect(auditInsert).toHaveBeenCalledTimes(1);
|
||||
expect(auditInsert.mock.calls[0]?.[0]).toMatchObject({
|
||||
projectId: "fn-9138-project",
|
||||
id: `aprevt-${eventType}-apr-race-${NOW}`,
|
||||
requestId: "apr-race",
|
||||
eventType,
|
||||
createdAt: NOW,
|
||||
});
|
||||
} finally {
|
||||
vi.useRealTimers();
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -1,7 +1,9 @@
|
||||
import { it, expect, vi, beforeAll, beforeEach, afterEach, afterAll } from "vitest";
|
||||
import { describe, it, expect, vi, beforeAll, beforeEach, afterEach, afterAll } from "vitest";
|
||||
import { ApprovalRequestStore } from "../../agents/approval-request-store.js";
|
||||
import type { AsyncDataLayer } from "../../postgres/data-layer.js";
|
||||
import * as schema from "../../postgres/schema/index.js";
|
||||
import {
|
||||
PG_AVAILABLE,
|
||||
pgDescribe,
|
||||
createSharedPgTaskStoreTestHarness,
|
||||
type SharedPgTaskStoreHarness,
|
||||
@@ -34,11 +36,19 @@ WHAT THESE DO AND DO NOT COVER, measured by reverting each guard in turn rather
|
||||
- requester-ownership on redemption -> 1 of 6 fails when removed
|
||||
- the `AND status = ?` guard on the UPDATE -> 0 fail when removed
|
||||
|
||||
That last line is the honest limit. The in-transaction re-read already rejects a replay single-threaded,
|
||||
so the guard only earns its keep against a racer committing BETWEEN the read and the write — which needs
|
||||
two concurrent transactions these tests do not create. The guard stays because the race is real; it is
|
||||
simply not what is verified here. Do not read a green run as proof of it.
|
||||
The guarded `AND status = ?` update is now pinned by the always-running
|
||||
`approval-request-audit-id-race.test.ts` stale-read simulation and the barrier-overlapped
|
||||
PostgreSQL double-decision/double-completion probes below. The pure test proves the exact
|
||||
empty-returning branch; PostgreSQL may validly serialize its loser to the transition matrix.
|
||||
*/
|
||||
describe("approval request lifecycle PostgreSQL availability", () => {
|
||||
it("fails closed when a required PostgreSQL probe would otherwise be skipped", () => {
|
||||
if (process.env.FUSION_PG_REQUIRED === "1") {
|
||||
expect(PG_AVAILABLE).toBe(true);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
pgDescribe("approval request lifecycle security (PostgreSQL)", () => {
|
||||
const h: SharedPgTaskStoreHarness = createSharedPgTaskStoreTestHarness({
|
||||
prefix: "fusion_sat_test",
|
||||
@@ -184,4 +194,116 @@ pgDescribe("approval request lifecycle security (PostgreSQL)", () => {
|
||||
});
|
||||
expect(completed.status).toBe("completed");
|
||||
});
|
||||
|
||||
it("rejects an exact same-millisecond audit primary-key duplicate", async () => {
|
||||
vi.useFakeTimers({ toFake: ["Date"] });
|
||||
try {
|
||||
const tiedAt = new Date("2026-08-16T23:22:00.000Z");
|
||||
vi.setSystemTime(tiedAt);
|
||||
const store = await seed("apr-audit-primary-key");
|
||||
const [created] = await store.getApprovalAuditHistory(ctx.layer.db, "apr-audit-primary-key");
|
||||
expect(created).toBeDefined();
|
||||
const duplicateError = await h.adminDb().insert(schema.project.approvalRequestAuditEvents).values({
|
||||
projectId: "__legacy_unscoped__",
|
||||
id: created!.id,
|
||||
requestId: created!.requestId,
|
||||
eventType: created!.eventType,
|
||||
actorId: created!.actor.actorId,
|
||||
actorType: created!.actor.actorType,
|
||||
actorName: created!.actor.actorName,
|
||||
note: created!.note ?? null,
|
||||
createdAt: created!.createdAt,
|
||||
}).then(() => null, (error: unknown) => error as { cause?: unknown });
|
||||
expect(duplicateError).toMatchObject({ cause: { code: "23505" } });
|
||||
} finally {
|
||||
vi.useRealTimers();
|
||||
}
|
||||
});
|
||||
|
||||
it.each([
|
||||
["pending double approve", "approved", "approved"] as const,
|
||||
["pending double deny", "denied", "denied"] as const,
|
||||
["pending approve versus deny", "approved", "denied"] as const,
|
||||
])("runs overlapping %s decisions without duplicate audit IDs", async (_name, first, second) => {
|
||||
const store = await seed(`apr-race-${first}-${second}`);
|
||||
const requestId = `apr-race-${first}-${second}`;
|
||||
vi.useFakeTimers({ toFake: ["Date"] });
|
||||
try {
|
||||
vi.setSystemTime(new Date("2026-08-16T23:22:00.000Z"));
|
||||
let release!: () => void;
|
||||
const barrier = new Promise<void>((resolve) => { release = resolve; });
|
||||
const windows: Array<{ started: number; settled: number }> = [];
|
||||
const racer = async (status: "approved" | "denied") => {
|
||||
await barrier;
|
||||
const window = { started: performance.now(), settled: Number.NaN };
|
||||
windows.push(window);
|
||||
try {
|
||||
return await store.decideApprovalRequest(ctx.layer, requestId, status, { actor: DECIDER });
|
||||
} finally {
|
||||
window.settled = performance.now();
|
||||
}
|
||||
};
|
||||
const racers = [racer(first), racer(second)];
|
||||
await Promise.resolve();
|
||||
release();
|
||||
const outcomes = await Promise.allSettled(racers);
|
||||
expect(windows).toHaveLength(2);
|
||||
expect(Math.max(...windows.map((window) => window.started))).toBeLessThan(
|
||||
Math.min(...windows.map((window) => window.settled)),
|
||||
);
|
||||
expect(outcomes.filter((outcome) => outcome.status === "fulfilled")).toHaveLength(1);
|
||||
expect(outcomes.filter((outcome) => outcome.status === "rejected")).toHaveLength(1);
|
||||
const loser = outcomes.find((outcome) => outcome.status === "rejected") as PromiseRejectedResult;
|
||||
expect(String(loser.reason)).toMatch(/Invalid approval request transition/);
|
||||
expect(String(loser.reason)).not.toMatch(/duplicate key|unique constraint/i);
|
||||
const winner = outcomes.find((outcome) => outcome.status === "fulfilled") as PromiseFulfilledResult<{ status: string }>;
|
||||
expect((await store.getApprovalRequest(ctx.layer.db, requestId))?.status).toBe(winner.value.status);
|
||||
const history = await store.getApprovalAuditHistory(ctx.layer.db, requestId);
|
||||
expect(history.filter((event) => event.eventType === winner.value.status)).toHaveLength(1);
|
||||
expect(new Set(history.map((event) => `${event.eventType}:${event.createdAt}`)).size).toBe(history.length);
|
||||
} finally {
|
||||
vi.useRealTimers();
|
||||
}
|
||||
});
|
||||
|
||||
it("runs overlapping approved double completion without duplicate audit IDs", async () => {
|
||||
const store = await seed("apr-race-completed");
|
||||
await store.decideApprovalRequest(ctx.layer, "apr-race-completed", "approved", { actor: DECIDER });
|
||||
vi.useFakeTimers({ toFake: ["Date"] });
|
||||
try {
|
||||
vi.setSystemTime(new Date("2026-08-16T23:22:00.000Z"));
|
||||
let release!: () => void;
|
||||
const barrier = new Promise<void>((resolve) => { release = resolve; });
|
||||
const windows: Array<{ started: number; settled: number }> = [];
|
||||
const racer = async () => {
|
||||
await barrier;
|
||||
const window = { started: performance.now(), settled: Number.NaN };
|
||||
windows.push(window);
|
||||
try {
|
||||
return await store.markApprovalRequestCompleted(ctx.layer, "apr-race-completed", { actor: DECIDER });
|
||||
} finally {
|
||||
window.settled = performance.now();
|
||||
}
|
||||
};
|
||||
const outcomes = [racer(), racer()];
|
||||
await Promise.resolve();
|
||||
release();
|
||||
const settled = await Promise.allSettled(outcomes);
|
||||
expect(Math.max(...windows.map((window) => window.started))).toBeLessThan(
|
||||
Math.min(...windows.map((window) => window.settled)),
|
||||
);
|
||||
expect(settled.filter((outcome) => outcome.status === "fulfilled")).toHaveLength(1);
|
||||
expect(settled.filter((outcome) => outcome.status === "rejected")).toHaveLength(1);
|
||||
const loser = settled.find((outcome) => outcome.status === "rejected") as PromiseRejectedResult;
|
||||
expect(String(loser.reason)).toMatch(/Invalid approval request transition/);
|
||||
expect(String(loser.reason)).not.toMatch(/duplicate key|unique constraint/i);
|
||||
expect((await store.getApprovalRequest(ctx.layer.db, "apr-race-completed"))?.status).toBe("completed");
|
||||
const history = await store.getApprovalAuditHistory(ctx.layer.db, "apr-race-completed");
|
||||
expect(history.filter((event) => event.eventType === "completed")).toHaveLength(1);
|
||||
expect(new Set(history.map((event) => `${event.eventType}:${event.createdAt}`)).size).toBe(history.length);
|
||||
} finally {
|
||||
vi.useRealTimers();
|
||||
}
|
||||
});
|
||||
|
||||
});
|
||||
|
||||
@@ -122,6 +122,16 @@ function rowToAuditEvent(row: ApprovalRequestAuditEventRow): ApprovalRequestAudi
|
||||
*
|
||||
* FNXC:ApprovalAnalyticsIsolation 2026-07-14-01:04:
|
||||
* Audit events must carry the bound layer's project ID at write time because request IDs alone do not provide a reliable tenant ownership join for Command Center intervention analytics. The live `(project_id, id)` key also permits deterministic audit IDs to collide safely across partitions; preserve the explicit value so the ownership trigger observes blank writes unchanged.
|
||||
*
|
||||
* FNXC:ApprovalAuditIdentity 2026-08-16-23:50:
|
||||
* A deterministic ID collision needs one physical project partition, request ID,
|
||||
* event type, and millisecond. `created` cannot repeat because the request-row
|
||||
* primary key rejects ID reuse first; the transition matrix rejects same-state
|
||||
* and terminal replays; and a concurrent non-terminal loser gets an empty
|
||||
* status-guarded update before this append runs. The database-free
|
||||
* `approval-request-audit-id-race.test.ts` pins that last branch, while the
|
||||
* fail-closed PostgreSQL lifecycle overlap probes confirm it end to end. Keep
|
||||
* this format unchanged unless a reachable writer defeats all three barriers.
|
||||
*/
|
||||
async function appendAuditEvent(
|
||||
tx: DbTransaction,
|
||||
|
||||
Reference in New Issue
Block a user