Harden instance/node ID collisions with retry + composite PK (#129)

CHANGELOG.md

@@ -12,6 +12,8 @@ Pre-1.0 note: while `pg_durable` is in major version `0`, minor releases may inc
 
   > ⚠️ **Replay-breaking for in-flight `wait_for_schedule` instances.** This change adds a recorded `utc_now()` decision before the WAIT_SCHEDULE timer, altering the orchestration's history sequence. Any durable function that was started under a `<= 0.2.3` binary and is **mid-`wait_for_schedule`** (parked on its timer) when this `.so` is loaded will fail with a duroxide nondeterminism error on replay, because its recorded history no longer matches the new code. Drain or allow such in-flight `wait_for_schedule` instances to complete before upgrading. Instances that are not currently inside a `wait_for_schedule` node are unaffected. We accepted this break (rather than introducing orchestration versioning) given the early pre-1.0 stage of the project.
 
+- **Instance/node ID collision hardening (#129):** `df.start()` now reserves IDs with `INSERT ... ON CONFLICT DO NOTHING RETURNING id` and re-rolls the random 8-hex value on collision — instances arbitrate on the `df.instances` primary key (`id`), nodes on the new composite `PRIMARY KEY (instance_id, id)` — replacing the previous `SELECT EXISTS` pre-check. Doing the conflict check at the index level (rather than a pre-check `SELECT`) closes a TOCTOU window and, for instances, an RLS blind spot where the pre-check could not see another role's rows. `df.nodes` now uses the composite `PRIMARY KEY (instance_id, id)` instead of a global single-column key, so the random 8-hex node ID is no longer the sole cross-instance collision guard. The `update-node-status` activity now scopes its `df.nodes` update by `instance_id` (a required activity-input field) and asserts it affects exactly one row. IDs stay `VARCHAR(8)` HEX; the `0.2.3 → 0.2.4` upgrade restructures the `df.nodes` keys in place (#238).
+  - **Breaking for in-flight work:** the new activity-input shape changes the string duroxide records in orchestration history, and duroxide validates activity inputs by exact equality on replay, so any instance left **in flight across the 0.2.3 → 0.2.4 binary upgrade** cannot complete. Drain or cancel in-flight instances before deploying 0.2.4. The in-place `df.nodes` key restructure also takes an `ACCESS EXCLUSIVE` lock whose duration scales with table size — run the upgrade in a maintenance window. See the #129 section of `docs/upgrade-testing.md` for the full drain-before-upgrade contract.
 - **`df.grant_usage()` / `df.revoke_usage()`:** dropped the explicit per-function `EXECUTE` allowlist. Schema `USAGE` on `df` is the real access gate for ordinary `df.*` functions, so the helpers now grant/revoke schema `USAGE`, the table privileges, and `EXECUTE` only on the sensitive functions (`df.http`, `df.grant_usage`, `df.revoke_usage`). Function signatures are unchanged and existing privileges are unaffected (#242).
 
 ### Removed

docs/E2E_TESTING.md

@@ -58,6 +58,7 @@ The test suite is organized into 23 files. Files `01`–`09` open with `SET SESS
 | `07_signals.sql` | `df.signal()` — send signals to a running workflow from within the polling loop |
 | `08_scenarios.sql` | End-to-end workflow scenarios using `playground.*` tables (ETL, parallel counts, conditional load, order processing, three-step) |
 | `09_graph_and_validation.sql` | `df.explain()` graph reuse, invalid `node_type` rejection |
+| `51_node_composite_pk.sql` | `df.nodes` composite PRIMARY KEY `(instance_id, id)` — schema contract (legacy `nodes_instance_node_key` UNIQUE promoted to the PK) and multi-node workflow regression under `instance_id`-scoped node-status updates and `df.result()` (issue #129) |
 
 ### Superuser Tests (runs as `postgres`)
 
@@ -70,6 +71,7 @@ The test suite is organized into 23 files. Files `01`–`09` open with `SET SESS
 | `14_database.sql` | Wrong-database `CREATE EXTENSION` rejection; `df.start(query, label, database)` multi-database routing |
 | `15_rls.sql` | RLS on `df.instances` / `df.nodes` / `df.vars` — per-user visibility, cross-user cancel/signal denied, column-level UPDATE, superuser bypass, per-user variable isolation |
 | `16_heartbeat.sql` | Worker heartbeat liveness — `df._worker_epoch.last_seen_at` advances over time |
+| `52_node_id_collision_across_instances.sql` | Cross-instance node-ID collision — two instances own the same 8-hex node id; asserts composite-PK coexistence, that `(instance_id, id)` addresses exactly one row, `df.result()` is instance-scoped, and a scoped `update_node_status`-style UPDATE affects exactly one row (issue #129) |
 
 ### Build-Phase Specific
 
@@ -99,7 +101,11 @@ pg_durable/
             ├── 45_connection_limit_timeout.sql
             ├── 46_connection_limit_startup_validation.sql
             ├── 47_http_dsl_disabled.sql
-            └── 48_http_allow_all.sql
+            ├── 48_http_allow_all.sql
+            ├── 49_quoted_role_names.sql
+            ├── 50_metrics_grants.sql
+            ├── 51_node_composite_pk.sql
+            └── 52_node_id_collision_across_instances.sql
 ```
 
 ## Writing New Tests

docs/upgrade-testing.md

@@ -73,7 +73,7 @@ We test against all previous versions in the same provider compatibility line. T
 | DSL construction | `df.sql()`, `df.seq()`, `df.if()`, `df.loop()`, `df.sleep()`, `df.http()` |
 | Execution | Starting and completing orchestrations |
 | Monitoring | `df.status()`, `df.result()`, `df.list_instances()`, `df.instance_info()` |
-| In-flight work | Orchestrations started before `.so` swap complete after swap |
+| In-flight work | Orchestrations started before `.so` swap complete after swap (except across an activity-input change — see #129) |
 
 **What it catches:**
 - SQL queries in Rust code referencing columns/constraints that don't exist in the old schema
@@ -100,7 +100,7 @@ This is a **chain test** (like Scenario A) — upgrade scripts are applied seque
 |------|---------------|
 | Variables | Pre-existing vars accessible via `df.getvar()` after upgrade |
 | Pre-existing instances | `df.result()`, `df.instance_info()`, and `df.list_instances()` work for instances created before upgrade |
-| In-flight work | Work started before `ALTER EXTENSION UPDATE` can still complete afterward |
+| In-flight work | Work started before `ALTER EXTENSION UPDATE` can still complete afterward (except across an activity-input change — see #129) |
 | New operations | `df.start()` works with new schema |
 
 **Priority:** High — validates the upgrade doesn't corrupt or lose existing data.
@@ -229,6 +229,15 @@ what the upgrade script handles, and any backward compatibility considerations.
 - **Scenario B2 considerations:** No data migration. Existing instances, nodes, and vars are untouched. After `ALTER EXTENSION UPDATE`, `df.debug_connection()` no longer exists; the simplified `df.grant_usage()` never references it.
 - **Dependent-object note:** The upgrade runs `DROP FUNCTION IF EXISTS df.debug_connection()` with PostgreSQL's default `RESTRICT` behavior. If a customer created their own object that depends on the function (e.g. a view or SQL function that calls it), `ALTER EXTENSION UPDATE` aborts with a dependency error and the customer must drop or repoint that object first. This is intentional for a removed debug helper — the script deliberately does not `CASCADE`, to avoid silently dropping customer-owned objects. The fresh-install (`tests/e2e/sql/18_delegated_grants.sql`) and upgrade (`scripts/test-upgrade.sh` B2 grant test) suites assert the function is absent and that `df.grant_usage()` still works after the drop.
 
+#### #129 Promote df.nodes to a composite primary key (instance_id, id)
+- **DDL change (df schema):** `df.nodes` previously had a single-column `PRIMARY KEY (id)` plus a separate composite `UNIQUE (instance_id, id)` (`nodes_instance_node_key`). The single-column key forced the random 8-hex node ID to be globally unique, so it was the sole cross-instance collision guard. Node IDs only need to be unique per instance, so the composite key is promoted to be the primary key and the global single-column key is dropped. Fresh installs (`src/lib.rs`) declare `id`/`instance_id` as `NOT NULL` and create `nodes_pkey PRIMARY KEY (instance_id, id)` directly; the upgrade script (`sql/pg_durable--0.2.3--0.2.4.sql`) restructures the existing keys in place. The three same-instance foreign keys (`nodes_left_node_same_instance_fkey`, `nodes_right_node_same_instance_fkey`, `instances_root_node_same_instance_fkey`) reference the composite key, so the upgrade drops them first, swaps the keys, then recreates them with their original `DEFERRABLE INITIALLY DEFERRED NOT VALID` definition. `nodes_instance_identity_fkey` references `df.instances`, not `df.nodes`, and is left untouched. IDs remain `VARCHAR(8)` HEX.
+- **Companion runtime change (#129):** `df.start()` now reserves the instance ID by attempting the insert itself — `INSERT INTO df.instances ... ON CONFLICT (id) DO NOTHING RETURNING id` — and re-rolling the random 8-hex ID when zero rows come back (a collision); there is no separate `SELECT EXISTS` pre-check. Because `ON CONFLICT` arbitration runs against the global `id` index *below* row-level security, this also re-rolls on collisions with another role's instance that the caller cannot `SELECT`. Node inserts use the same pattern against the composite key — `INSERT INTO df.nodes ... ON CONFLICT (instance_id, id) DO NOTHING RETURNING id` — re-rolling on a per-instance collision. `df.start()` pre-generates the root node's ID and reserves the instance with `root_node` set to that value; `insert_nodes` then inserts the root node with the same forced ID. The same-instance FK on `root_node` is `DEFERRABLE INITIALLY DEFERRED`, so it is checked only at commit, by which point the referenced root node row exists — no post-insert `UPDATE` is needed (and `df.grant_usage()` deliberately grants `UPDATE (status, updated_at)` but not `UPDATE (root_node)` on `df.instances`, so an update path would fail for ordinary df roles). The `update-node-status` activity and `df.result()` now scope their `df.nodes` lookups by `instance_id` in addition to `id`, and the activity asserts the scoped `UPDATE` affects exactly one row. `instance_id` is a **required** field of the activity input — node IDs are unique only per instance, so updating by node ID alone could silently write to a *different* instance's node. There is deliberately no node-ID-only fallback.
+- **Design note — collision handling for both ID spaces (#129):** Both IDs stay 8-hex `VARCHAR(8)` (the requested minimal change) and re-roll on conflict via `INSERT ... ON CONFLICT DO NOTHING RETURNING id`; the mechanism is symmetric and only the conflict target differs. `df.instances.id` is a *global* identifier with no natural scoping column, so its reserve arbitrates on the single-column primary key (`id`). `df.nodes.id` is always used together with its owning `instance_id`, so promoting the pre-existing `(instance_id, id)` UNIQUE to the primary key lets node inserts arbitrate per instance — the random node ID never has to be globally unique. Using `ON CONFLICT DO NOTHING` rather than a `SELECT EXISTS` pre-check closes a TOCTOU window and, for instances, an RLS blind spot: the pre-check only saw the caller's own rows, whereas `ON CONFLICT` detects a clash with any role's row at the index level. The retry bound (`MAX_ID_ATTEMPTS`) surfaces a hard error on exhaustion rather than returning an unverified ID.
+- **In-flight orchestration compatibility (#129 — breaking for in-flight work):** Adding `instance_id` to the `update-node-status` activity input changes the input string that duroxide records in orchestration history. duroxide validates activity inputs by exact equality during replay, so any orchestration that was **in flight across the binary upgrade** (it recorded the old `{node_id, status}` input under 0.2.3) fails deterministic replay under the new `.so` and cannot complete. This is an intentional break of the general "in-flight work completes after the swap" expectation (the Scenario B1 and B2 "In-flight work" rows above) **for this release**, and follows the same drain-or-recreate precedent as the v0.1.0 → v0.1.1 execution-model change (Scenario B2, below): **operators must drain in-flight instances to a terminal state before deploying 0.2.4**, or cancel and recreate any that cannot drain. Instances that completed before the upgrade are terminal and unaffected; instances started after the upgrade carry `instance_id` from their first node update and replay normally.
+- **Scenario A considerations:** Fresh-install and upgraded schemas must both end with exactly one identity constraint on `df.nodes`: `nodes_pkey PRIMARY KEY (instance_id, id)` (constraint key order `instance_id, id`), its matching unique index `nodes_pkey ON df.nodes USING btree (instance_id, id)`, and no surviving `nodes_instance_node_key` constraint or index. The recreated foreign keys keep identical names and referencing columns, so the constraint/index snapshot diff is empty.
+- **Scenario B1 considerations:** The schema change is to table constraints only; the new `.so` issues the same column lists against `df.nodes`/`df.instances`, now with `ON CONFLICT ... DO NOTHING RETURNING id`. The instance reserve arbitrates on `id` (the primary key in both old and new schemas) and the node insert arbitrates on `(instance_id, id)` — an index that exists in both the pre-0.2.4 schema (the `nodes_instance_node_key` composite UNIQUE) and the new schema (the composite primary key) — so both statements stay valid against a schema that has not run `ALTER EXTENSION UPDATE`. The pre-generated-`root_id` reserve is also old-schema-safe: `instances_root_node_same_instance_fkey` is `DEFERRABLE INITIALLY DEFERRED` in every shipped schema, so `root_node` is not checked until commit, by which point the forced-ID root node row has been inserted within the same transaction. No `UPDATE df.instances` is issued, so the change relies only on the `INSERT (..., root_node, ...)` privilege every shipped `df.grant_usage()` already grants, not on any `UPDATE (root_node)` grant. One benign residual exists against the *old* schema only: a node ID that is globally duplicated but per-instance-unique would clash with the surviving single-column `nodes_pkey (id)`, which `ON CONFLICT (instance_id, id)` does not arbitrate, so it raises just as it did before this change — astronomically rare, strictly no worse than prior behavior, and eliminated once `ALTER EXTENSION UPDATE` swaps in the composite primary key. This covers **schema** compatibility only — the SQL stays valid against the old table shape. The separate in-flight *replay* break introduced by the changed activity-input shape is documented under "In-flight orchestration compatibility" above and requires draining before upgrade.
+- **Scenario B2 considerations:** `ADD PRIMARY KEY (instance_id, id)` sets `NOT NULL` on both columns and builds a unique index over existing rows. `id` was already the old primary key (implicitly `NOT NULL`). `instance_id` carries a `nodes_instance_id_present_chk CHECK (instance_id IS NOT NULL)` constraint, but it was added `NOT VALID`, so it only guarantees rows written on 0.2.2+; in the unlikely event a database still holds pre-0.2.2 node rows with a NULL `instance_id`, the `ADD PRIMARY KEY` (and the explicit `ALTER COLUMN instance_id SET NOT NULL` that precedes it) will abort and the operator must backfill or remove those rows before retrying the upgrade. On an empty database the restructure is metadata-only; on a populated one PostgreSQL rebuilds the `df.nodes` primary-key index in place. Because `ADD PRIMARY KEY` / `ALTER COLUMN ... SET NOT NULL` take an `ACCESS EXCLUSIVE` lock on `df.nodes` and rebuild the index, on a large `df.nodes` the upgrade blocks concurrent access for a period that scales with the table's size; run `ALTER EXTENSION UPDATE` inside a maintenance window and consider `SET lock_timeout` for the session so the migration fails fast instead of queuing behind (or stalling in front of) long-running transactions. Combined with the in-flight replay break noted above, the recommended upgrade sequence is: stop new `df.start()` calls, drain or cancel in-flight instances, then run the upgrade.
+
 ### v0.2.2 → v0.2.3
 
 #### Rename duroxide provider schema to `_duroxide` for fresh installs

sql/pg_durable--0.2.3--0.2.4.sql

@@ -181,3 +181,58 @@ CREATE FUNCTION df."await_instance"(
 STRICT
 LANGUAGE c
 AS 'MODULE_PATHNAME', 'await_instance_wrapper';
+
+-- ============================================================================
+-- Promote df.nodes to a composite primary key (instance_id, id) (issue #129).
+--
+-- The single-column PRIMARY KEY (id) forced node IDs to be globally unique, so
+-- the random 8-hex node ID was the sole collision guard across every instance.
+-- Node IDs only need to be unique per instance, so the existing composite
+-- UNIQUE (instance_id, id) — already referenced by the same-instance foreign
+-- keys — is promoted to be the primary key and the global single-column key is
+-- dropped. This matches the fresh-install schema in src/lib.rs so a fresh
+-- install and an upgraded database end with identical df.nodes constraints.
+--
+-- The three same-instance foreign keys reference the composite key, so
+-- PostgreSQL will not allow dropping it (nor the old single-column PRIMARY KEY)
+-- while those foreign keys exist. Drop them first, restructure the keys, then
+-- recreate the foreign keys against the new primary key. The recreated foreign
+-- keys keep their original DEFERRABLE INITIALLY DEFERRED NOT VALID definition.
+--
+-- nodes_instance_identity_fkey references df.instances, not df.nodes, so it is
+-- left untouched. ADD PRIMARY KEY (instance_id, id) sets NOT NULL on both
+-- columns: id was already the old primary key (implicitly NOT NULL), and
+-- instance_id carries nodes_instance_id_present_chk CHECK (instance_id IS NOT
+-- NULL). That check is NOT VALID, so it only guarantees rows written on 0.2.2+;
+-- in the unlikely event a database still holds pre-0.2.2 rows with a NULL
+-- instance_id, the ALTER COLUMN ... SET NOT NULL below will abort and the
+-- operator must backfill or remove those rows before retrying the upgrade.
+-- ============================================================================
+ALTER TABLE df.nodes DROP CONSTRAINT nodes_left_node_same_instance_fkey;
+ALTER TABLE df.nodes DROP CONSTRAINT nodes_right_node_same_instance_fkey;
+ALTER TABLE df.instances DROP CONSTRAINT instances_root_node_same_instance_fkey;
+
+ALTER TABLE df.nodes DROP CONSTRAINT nodes_instance_node_key;
+ALTER TABLE df.nodes DROP CONSTRAINT nodes_pkey;
+
+ALTER TABLE df.nodes
+    ALTER COLUMN id SET NOT NULL,
+    ALTER COLUMN instance_id SET NOT NULL,
+    ADD CONSTRAINT nodes_pkey
+        PRIMARY KEY (instance_id, id);
+
+ALTER TABLE df.nodes
+    ADD CONSTRAINT nodes_left_node_same_instance_fkey
+        FOREIGN KEY (instance_id, left_node)
+        REFERENCES df.nodes (instance_id, id)
+        DEFERRABLE INITIALLY DEFERRED NOT VALID,
+    ADD CONSTRAINT nodes_right_node_same_instance_fkey
+        FOREIGN KEY (instance_id, right_node)
+        REFERENCES df.nodes (instance_id, id)
+        DEFERRABLE INITIALLY DEFERRED NOT VALID;
+
+ALTER TABLE df.instances
+    ADD CONSTRAINT instances_root_node_same_instance_fkey
+        FOREIGN KEY (id, root_node)
+        REFERENCES df.nodes (instance_id, id)
+        DEFERRABLE INITIALLY DEFERRED NOT VALID;