docs: replace a vacuous conformance test with an honest one
I added "a concurrent write is not lost to another method's failure" to the conformance suite to guard the fail-open the Task 11 review demonstrated. The implementer reported they could not make it fail against the reverted code, across 600 stress iterations. They were right. I reproduced the underlying defect directly - forcing the transaction to open before the bare write gives "revoke resolved without error: true" with the role still present - so the mechanism is real. But the test cannot reach it: Promise.all in one process does not reliably land the bare write inside the open transaction, and grant() never fails on its own. The test passed against the defective implementation, which is exactly the false assurance this suite exists to prevent. Replaced with a property that is actually guaranteed and adapter-agnostic: a rejected write leaves unrelated state intact. The rollback hazard itself is prevented structurally, by the store using no transactions, and that is now stated in a comment rather than pretended to be under test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -623,17 +623,25 @@ export function runStoreConformance(name: string, makeStore: () => Promise<Permi
|
||||
expect(assignments.denies).toEqual(["post:delete"]);
|
||||
});
|
||||
|
||||
test("a concurrent write is not lost to another method's failure", async () => {
|
||||
// A store that wraps one method in a transaction on a shared connection
|
||||
// will roll back this unrelated write and still resolve successfully.
|
||||
test("a rejected write leaves unrelated state intact", async () => {
|
||||
await store.assignRole("victim", "admin");
|
||||
await Promise.all([
|
||||
store.revokeRole("victim", "admin"),
|
||||
store.grant("other", "post:read", "allow").catch(() => undefined),
|
||||
]);
|
||||
expect((await store.assignmentsFor("victim")).roles).toEqual([]);
|
||||
await store.grant("victim", "post:read", "allow");
|
||||
// An invalid effect must be refused without disturbing anything else.
|
||||
await expect(store.grant("victim", "post:write", "bogus" as never)).rejects.toThrow();
|
||||
const assignments = await store.assignmentsFor("victim");
|
||||
expect(assignments.roles).toEqual(["admin"]);
|
||||
expect(assignments.grants).toEqual(["post:read"]);
|
||||
});
|
||||
|
||||
// NOTE: the shared-connection rollback hazard - where one method's open
|
||||
// transaction sweeps in a concurrent bare write from another method and
|
||||
// discards it, so a revoke resolves successfully while the role survives -
|
||||
// is prevented STRUCTURALLY, by the store using no transactions at all.
|
||||
// It is deliberately not covered here: reproducing it needs the bare write
|
||||
// to land inside the open transaction, which a single-process Promise.all
|
||||
// does not reliably arrange, so any such test would pass against the
|
||||
// defective implementation and give false assurance.
|
||||
|
||||
test("listSubjects with no scope returns global assignees only", async () => {
|
||||
await store.assignRole("g1", "viewer");
|
||||
await store.assignRole("s1", "editor", { tenantId: "t1" });
|
||||
|
||||
Reference in New Issue
Block a user