Skip to content

Commit b82f9cd

Browse files
authored
Harden instance/node ID collisions with retry + composite PK (#129) (#238)
Maintainer-directed minimal change for #129 / PR #238: df.start() now retries instance-ID generation on collision (the ID stays VARCHAR(8) HEX, unchanged), and df.nodes uses a composite PRIMARY KEY (instance_id, id) so node IDs only need to be unique per instance rather than globally. The legacy nodes_instance_node_key UNIQUE constraint is promoted to the primary key. Collision handling is atomic: both the instance reserve and every node insert claim their ID via INSERT ... ON CONFLICT DO NOTHING RETURNING id and re-roll when zero rows return, so there is no check-then-insert TOCTOU window. The shared pick_id_with_retry helper treats the claim as the loop tail and returns an error on exhaustion, never an unverified ID. The instance row is reserved up front with its root_node bound to a pre-generated ID that is then forced onto the root node, satisfying the deferred same-instance FK at commit without an extra UPDATE (ordinary df roles cannot UPDATE df.instances.root_node). update_node_status now requires instance_id and always scopes its UPDATE by (instance_id, id), removing the global-ID fallback and asserting exactly one row is affected. Adding instance_id to the activity input is a duroxide replay-breaking change for orchestrations in flight across the 0.2.3 -> 0.2.4 binary upgrade, so operators must drain or recreate in-flight instances before upgrading. This is documented in docs/upgrade-testing.md alongside the instance-retry vs node-composite-PK asymmetry rationale and ADD PRIMARY KEY lock guidance. Add e2e tests 51 (composite-PK schema contract + instance_id-scoped node-status regression) and 52 (cross-instance node-ID collision). Update CHANGELOG and the E2E test inventory.
1 parent 72719a6 commit b82f9cd

11 files changed

Lines changed: 670 additions & 142 deletions

CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@ Pre-1.0 note: while `pg_durable` is in major version `0`, minor releases may inc
1212

1313
> ⚠️ **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.
1414
15+
- **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).
16+
- **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.
1517
- **`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).
1618

1719
### Removed

docs/E2E_TESTING.md

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@ The test suite is organized into 23 files. Files `01`–`09` open with `SET SESS
5858
| `07_signals.sql` | `df.signal()` — send signals to a running workflow from within the polling loop |
5959
| `08_scenarios.sql` | End-to-end workflow scenarios using `playground.*` tables (ETL, parallel counts, conditional load, order processing, three-step) |
6060
| `09_graph_and_validation.sql` | `df.explain()` graph reuse, invalid `node_type` rejection |
61+
| `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) |
6162

6263
### Superuser Tests (runs as `postgres`)
6364

@@ -70,6 +71,7 @@ The test suite is organized into 23 files. Files `01`–`09` open with `SET SESS
7071
| `14_database.sql` | Wrong-database `CREATE EXTENSION` rejection; `df.start(query, label, database)` multi-database routing |
7172
| `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 |
7273
| `16_heartbeat.sql` | Worker heartbeat liveness — `df._worker_epoch.last_seen_at` advances over time |
74+
| `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) |
7375

7476
### Build-Phase Specific
7577

@@ -99,7 +101,11 @@ pg_durable/
99101
├── 45_connection_limit_timeout.sql
100102
├── 46_connection_limit_startup_validation.sql
101103
├── 47_http_dsl_disabled.sql
102-
└── 48_http_allow_all.sql
104+
├── 48_http_allow_all.sql
105+
├── 49_quoted_role_names.sql
106+
├── 50_metrics_grants.sql
107+
├── 51_node_composite_pk.sql
108+
└── 52_node_id_collision_across_instances.sql
103109
```
104110

105111
## Writing New Tests

docs/upgrade-testing.md

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ We test against all previous versions in the same provider compatibility line. T
7373
| DSL construction | `df.sql()`, `df.seq()`, `df.if()`, `df.loop()`, `df.sleep()`, `df.http()` |
7474
| Execution | Starting and completing orchestrations |
7575
| Monitoring | `df.status()`, `df.result()`, `df.list_instances()`, `df.instance_info()` |
76-
| In-flight work | Orchestrations started before `.so` swap complete after swap |
76+
| In-flight work | Orchestrations started before `.so` swap complete after swap (except across an activity-input change — see #129) |
7777

7878
**What it catches:**
7979
- 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
100100
|------|---------------|
101101
| Variables | Pre-existing vars accessible via `df.getvar()` after upgrade |
102102
| Pre-existing instances | `df.result()`, `df.instance_info()`, and `df.list_instances()` work for instances created before upgrade |
103-
| In-flight work | Work started before `ALTER EXTENSION UPDATE` can still complete afterward |
103+
| In-flight work | Work started before `ALTER EXTENSION UPDATE` can still complete afterward (except across an activity-input change — see #129) |
104104
| New operations | `df.start()` works with new schema |
105105

106106
**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.
229229
- **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.
230230
- **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.
231231

232+
#### #129 Promote df.nodes to a composite primary key (instance_id, id)
233+
- **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.
234+
- **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.
235+
- **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.
236+
- **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.
237+
- **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.
238+
- **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.
239+
- **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.
240+
232241
### v0.2.2 → v0.2.3
233242

234243
#### Rename duroxide provider schema to `_duroxide` for fresh installs

sql/pg_durable--0.2.3--0.2.4.sql

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,3 +181,58 @@ CREATE FUNCTION df."await_instance"(
181181
STRICT
182182
LANGUAGE c
183183
AS 'MODULE_PATHNAME', 'await_instance_wrapper';
184+
185+
-- ============================================================================
186+
-- Promote df.nodes to a composite primary key (instance_id, id) (issue #129).
187+
--
188+
-- The single-column PRIMARY KEY (id) forced node IDs to be globally unique, so
189+
-- the random 8-hex node ID was the sole collision guard across every instance.
190+
-- Node IDs only need to be unique per instance, so the existing composite
191+
-- UNIQUE (instance_id, id) — already referenced by the same-instance foreign
192+
-- keys — is promoted to be the primary key and the global single-column key is
193+
-- dropped. This matches the fresh-install schema in src/lib.rs so a fresh
194+
-- install and an upgraded database end with identical df.nodes constraints.
195+
--
196+
-- The three same-instance foreign keys reference the composite key, so
197+
-- PostgreSQL will not allow dropping it (nor the old single-column PRIMARY KEY)
198+
-- while those foreign keys exist. Drop them first, restructure the keys, then
199+
-- recreate the foreign keys against the new primary key. The recreated foreign
200+
-- keys keep their original DEFERRABLE INITIALLY DEFERRED NOT VALID definition.
201+
--
202+
-- nodes_instance_identity_fkey references df.instances, not df.nodes, so it is
203+
-- left untouched. ADD PRIMARY KEY (instance_id, id) sets NOT NULL on both
204+
-- columns: id was already the old primary key (implicitly NOT NULL), and
205+
-- instance_id carries nodes_instance_id_present_chk CHECK (instance_id IS NOT
206+
-- NULL). That check is NOT VALID, so it only guarantees rows written on 0.2.2+;
207+
-- in the unlikely event a database still holds pre-0.2.2 rows with a NULL
208+
-- instance_id, the ALTER COLUMN ... SET NOT NULL below will abort and the
209+
-- operator must backfill or remove those rows before retrying the upgrade.
210+
-- ============================================================================
211+
ALTER TABLE df.nodes DROP CONSTRAINT nodes_left_node_same_instance_fkey;
212+
ALTER TABLE df.nodes DROP CONSTRAINT nodes_right_node_same_instance_fkey;
213+
ALTER TABLE df.instances DROP CONSTRAINT instances_root_node_same_instance_fkey;
214+
215+
ALTER TABLE df.nodes DROP CONSTRAINT nodes_instance_node_key;
216+
ALTER TABLE df.nodes DROP CONSTRAINT nodes_pkey;
217+
218+
ALTER TABLE df.nodes
219+
ALTER COLUMN id SET NOT NULL,
220+
ALTER COLUMN instance_id SET NOT NULL,
221+
ADD CONSTRAINT nodes_pkey
222+
PRIMARY KEY (instance_id, id);
223+
224+
ALTER TABLE df.nodes
225+
ADD CONSTRAINT nodes_left_node_same_instance_fkey
226+
FOREIGN KEY (instance_id, left_node)
227+
REFERENCES df.nodes (instance_id, id)
228+
DEFERRABLE INITIALLY DEFERRED NOT VALID,
229+
ADD CONSTRAINT nodes_right_node_same_instance_fkey
230+
FOREIGN KEY (instance_id, right_node)
231+
REFERENCES df.nodes (instance_id, id)
232+
DEFERRABLE INITIALLY DEFERRED NOT VALID;
233+
234+
ALTER TABLE df.instances
235+
ADD CONSTRAINT instances_root_node_same_instance_fkey
236+
FOREIGN KEY (id, root_node)
237+
REFERENCES df.nodes (instance_id, id)
238+
DEFERRABLE INITIALLY DEFERRED NOT VALID;

0 commit comments

Comments
 (0)