diff --git a/.changeset/tidy-graphs-stop.md b/.changeset/tidy-graphs-stop.md new file mode 100644 index 00000000000..36cb39b2c5a --- /dev/null +++ b/.changeset/tidy-graphs-stop.md @@ -0,0 +1,5 @@ +--- +"effect": patch +--- + +Make Graph node and edge allocation fail atomically after allocating `Number.MAX_SAFE_INTEGER`. diff --git a/packages/effect/src/Graph.ts b/packages/effect/src/Graph.ts index d0564ac9120..bd0db07a292 100644 --- a/packages/effect/src/Graph.ts +++ b/packages/effect/src/Graph.ts @@ -1460,8 +1460,13 @@ export const addNode = ( mutable: MutableGraph, data: N ): NodeIndex => { - const impl = getMutableImplForMutation(mutable) + assertMutable(mutable) + const impl = internal.toImpl(mutable) const nodeIndex = impl.nextNodeIndex + if (nodeIndex === internal.exhausted) { + throw new GraphError({ message: "Node index allocation exhausted" }) + } + csr.invalidate(mutable) // Add node data impl.nodes.set(nodeIndex, data) @@ -1471,7 +1476,7 @@ export const addNode = ( impl.reverseAdjacency.set(nodeIndex, []) // Update graph allocators - impl.nextNodeIndex = impl.nextNodeIndex + 1 + impl.nextNodeIndex = nodeIndex === Number.MAX_SAFE_INTEGER ? internal.exhausted : nodeIndex + 1 return nodeIndex } @@ -2244,7 +2249,8 @@ export const addEdge = ( target: NodeIndex, data: E ): EdgeIndex => { - const impl = getMutableImplForMutation(mutable) + assertMutable(mutable) + const impl = internal.toImpl(mutable) // Validate that both nodes exist if (!impl.nodes.has(source)) { @@ -2255,6 +2261,10 @@ export const addEdge = ( } const edgeIndex = impl.nextEdgeIndex + if (edgeIndex === internal.exhausted) { + throw new GraphError({ message: "Edge index allocation exhausted" }) + } + csr.invalidate(mutable) // Create edge data const edgeData: Edge = { source, target, data } @@ -2285,7 +2295,7 @@ export const addEdge = ( } // Update allocators - impl.nextEdgeIndex = impl.nextEdgeIndex + 1 + impl.nextEdgeIndex = edgeIndex === Number.MAX_SAFE_INTEGER ? internal.exhausted : edgeIndex + 1 // Only invalidate cycle flag if the graph was acyclic // Adding edges cannot remove cycles from cyclic graphs diff --git a/packages/effect/src/internal/graph.ts b/packages/effect/src/internal/graph.ts index f18c87e8d13..cd442fd2b89 100644 --- a/packages/effect/src/internal/graph.ts +++ b/packages/effect/src/internal/graph.ts @@ -9,6 +9,12 @@ import { hasProperty } from "../Predicate.ts" /** @internal */ export const TypeId = "~effect/collections/Graph" +/** @internal */ +export const exhausted = Symbol("effect/Graph/exhausted") + +/** @internal */ +export type Allocator = Graph.NodeIndex | typeof exhausted + /** @internal */ export interface GraphImpl extends Iterable, Equal.Equal @@ -20,8 +26,8 @@ export interface GraphImpl> adjacency: Map> reverseAdjacency: Map> - nextNodeIndex: Graph.NodeIndex - nextEdgeIndex: Graph.EdgeIndex + nextNodeIndex: Allocator + nextEdgeIndex: Allocator acyclic: Option.Option toJSON(): unknown } @@ -202,8 +208,18 @@ export const hydrate = ( graph.reverseAdjacency.get(edge.source)!.push(edge.index) } } - graph.nextNodeIndex = snapshot.nodes.length === 0 ? 0 : snapshot.nodes[snapshot.nodes.length - 1].index + 1 - graph.nextEdgeIndex = snapshot.edges.length === 0 ? 0 : snapshot.edges[snapshot.edges.length - 1].index + 1 + const lastNodeIndex = snapshot.nodes.length === 0 ? undefined : snapshot.nodes[snapshot.nodes.length - 1].index + graph.nextNodeIndex = lastNodeIndex === undefined + ? 0 + : lastNodeIndex === Number.MAX_SAFE_INTEGER + ? exhausted + : lastNodeIndex + 1 + const lastEdgeIndex = snapshot.edges.length === 0 ? undefined : snapshot.edges[snapshot.edges.length - 1].index + graph.nextEdgeIndex = lastEdgeIndex === undefined + ? 0 + : lastEdgeIndex === Number.MAX_SAFE_INTEGER + ? exhausted + : lastEdgeIndex + 1 graph.acyclic = Option.none() return graph as unknown as Graph.Graph } diff --git a/packages/effect/test/Graph.test.ts b/packages/effect/test/Graph.test.ts index 6f177b6e202..6670bad0b5a 100644 --- a/packages/effect/test/Graph.test.ts +++ b/packages/effect/test/Graph.test.ts @@ -148,6 +148,71 @@ describe("Graph", () => { }) }) + it("allocates the maximum safe node index once and then fails atomically", () => { + const graph = Graph.fromSnapshot({ + type: "directed", + nodes: [{ index: Number.MAX_SAFE_INTEGER - 1, data: "A" }], + edges: [] + }) + + const mutable = Graph.beginMutation(graph) + assert.strictEqual(Graph.addNode(mutable, "B"), Number.MAX_SAFE_INTEGER) + for (let i = 0; i < 2; i++) { + assertGraphError(() => Graph.addNode(mutable, "unreachable"), "Node index allocation exhausted") + } + assert.deepStrictEqual(Array.from(mutable), [ + [Number.MAX_SAFE_INTEGER - 1, "A"], + [Number.MAX_SAFE_INTEGER, "B"] + ]) + assert.strictEqual(Graph.nodeCount(mutable), 2) + }) + + it("allocates the maximum safe edge index once and then fails atomically", () => { + const graph = Graph.fromSnapshot({ + type: "directed", + nodes: [{ index: 0, data: "A" }, { index: 2, data: "B" }], + edges: [{ index: Number.MAX_SAFE_INTEGER - 1, source: 0, target: 2, data: "first" }] + }) + + const mutable = Graph.beginMutation(graph) + assert.strictEqual(Graph.addEdge(mutable, 0, 2, "last"), Number.MAX_SAFE_INTEGER) + assert.strictEqual(Graph.isAcyclic(mutable), true) + for (let i = 0; i < 2; i++) { + assertGraphError(() => Graph.addEdge(mutable, 0, 2, "unreachable"), "Edge index allocation exhausted") + } + assert.deepStrictEqual(Array.from(Graph.edges(mutable)), [ + [Number.MAX_SAFE_INTEGER - 1, { source: 0, target: 2, data: "first" }], + [Number.MAX_SAFE_INTEGER, { source: 0, target: 2, data: "last" }] + ]) + assert.strictEqual(Graph.isAcyclic(mutable), true) + assert.strictEqual(Graph.edgeCount(mutable), 2) + }) + + it("hydrates maximum safe indexes as exhausted and validates edge endpoints first", () => { + const mutable = Graph.beginMutation(Graph.fromSnapshot({ + type: "directed", + nodes: [{ index: 0, data: "A" }, { index: Number.MAX_SAFE_INTEGER, data: "B" }], + edges: [{ index: Number.MAX_SAFE_INTEGER, source: 0, target: Number.MAX_SAFE_INTEGER, data: "edge" }] + })) + + assertGraphError(() => Graph.addNode(mutable, "unreachable"), "Node index allocation exhausted") + assertGraphError( + () => Graph.addEdge(mutable, 1, Number.MAX_SAFE_INTEGER, "unreachable"), + "Node 1 does not exist" + ) + assertGraphError( + () => Graph.addEdge(mutable, 0, 1, "unreachable"), + "Node 1 does not exist" + ) + assertGraphError( + () => Graph.addEdge(mutable, 0, Number.MAX_SAFE_INTEGER, "unreachable"), + "Edge index allocation exhausted" + ) + assert.deepStrictEqual(Graph.successors(mutable, 0), [Number.MAX_SAFE_INTEGER]) + assert.strictEqual(Graph.nodeCount(mutable), 2) + assert.strictEqual(Graph.edgeCount(mutable), 1) + }) + it("rejects invalid snapshot indexes", () => { assertGraphError( () => diff --git a/packages/effect/test/schema/Graph.test.ts b/packages/effect/test/schema/Graph.test.ts index 1972b3fabc8..7f6267bcb62 100644 --- a/packages/effect/test/schema/Graph.test.ts +++ b/packages/effect/test/schema/Graph.test.ts @@ -136,6 +136,33 @@ describe("Schema.Graph", () => { assert.strictEqual(edgeIndex, 5) }) + it("decodes maximum safe indexes into exhausted allocators", () => { + const decoded = decodeDirected({ + type: "directed", + nodes: [{ index: 0, data: "A" }, { index: Number.MAX_SAFE_INTEGER, data: "B" }], + edges: [{ index: Number.MAX_SAFE_INTEGER, source: 0, target: Number.MAX_SAFE_INTEGER, data: 1 }] + }) + + const mutable = Graph.beginMutation(decoded) + throws(() => Graph.addNode(mutable, "unreachable"), (error) => { + assert.strictEqual(error instanceof Graph.GraphError, true) + if (error instanceof Graph.GraphError) { + assert.strictEqual(error.message, "Node index allocation exhausted") + } + }) + throws(() => Graph.addEdge(mutable, 0, Number.MAX_SAFE_INTEGER, 2), (error) => { + assert.strictEqual(error instanceof Graph.GraphError, true) + if (error instanceof Graph.GraphError) { + assert.strictEqual(error.message, "Edge index allocation exhausted") + } + }) + assert.deepStrictEqual(encodeDirected(Graph.endMutation(mutable)), { + type: "directed", + nodes: [{ index: 0, data: "A" }, { index: Number.MAX_SAFE_INTEGER, data: "B" }], + edges: [{ index: Number.MAX_SAFE_INTEGER, source: 0, target: Number.MAX_SAFE_INTEGER, data: 1 }] + }) + }) + it("does not serialize removed trailing allocation history", () => { const graph = Graph.directed((mutable) => { const a = Graph.addNode(mutable, "A")