Fix caller transaction graph handoff - #367
Conversation
Track the originating PostgreSQL transaction while the worker waits for a newly started graph. Use durable, bounded probes so long commits resume, rollbacks fail cleanly, transient database errors retry, and wait history remains bounded. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Haider Z (@haiderz07) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
Review verdict: changes requested
Three independent specialist passes reviewed this head for PostgreSQL transaction semantics, durable replay compatibility, and Rust reliability. The transaction-handoff direction is sound, but three production correctness/liveness issues and two regression-test gaps should be resolved before merge. CI remains the source of test results.
| Err(LoadGraphError::Permanent(error)) => return Err(error), | ||
| } | ||
| } | ||
| Ok(None) => TransactionGraphProbe::CommittedMissing, |
There was a problem hiding this comment.
High — committed is not yet guaranteed to be snapshot-visible. PostgreSQL can record the transaction as committed before ProcArrayEndTransaction removes it from the running set used by a fresh snapshot. In that window, the immediate find_visible_instance() re-read can still return None, causing a valid committed graph to be permanently classified as CommittedMissing. After observing committed, first check pg_visible_in_snapshot(origin_xid, pg_current_snapshot()); if it is false, return Retry. Declare CommittedMissing only after the xid is snapshot-visible and the graph remains absent. Please add focused coverage for this handoff boundary.
|
|
||
| match visible { | ||
| Ok(Some(row)) => { | ||
| let loaded = match tokio::time::timeout( |
There was a problem hiding this comment.
High — the two-second deadline can livelock a valid graph load. This timeout covers role validation, fetching up to MAX_GRAPH_NODES (10,000), constructing the graph, and serialization. A valid graph that consistently needs more than two seconds returns Retry forever; durable backoff and continue_as_new bound history but never let the workflow execute or fail. Keep the short deadline on visibility/transaction-status probes, but give graph loading a separate policy: a suitable statement/lock timeout plus a bounded retry budget and terminal error.
| } | ||
| Ok(None) => {} | ||
| Err(e) => { | ||
| return retry_probe(&ctx, "graph visibility check", &input.instance_id, e); |
There was a problem hiding this comment.
High — permanent database errors retry forever. Errors from visibility checks, pg_xact_status(), and the Retryable graph-load branch all become Retry without a cumulative ceiling. Conditions such as 42501 (insufficient_privilege), 42883 (undefined_function), 42P01 (undefined_table), and schema/decode incompatibilities cannot recover through polling, so the orchestration remains running indefinitely with misleading transaction-wait logs. Allowlist genuinely transient SQLSTATEs and connection failures, propagate all other errors as terminal with their SQLSTATE/message, and consider a bounded budget even for transient failures.
|
|
||
| -- Leave time for the killed query to return Retry and the next lock-blocked | ||
| -- probe to hit its bounded query timeout before releasing the table lock. | ||
| PERFORM pg_sleep(3); |
There was a problem hiding this comment.
Medium — this regression does not prove the timeout path executed. The test terminates one lock-blocked management backend and waits three seconds, but it never proves that a second probe started and reached its two-second timeout before COMMIT; that probe can simply resume after the lock is released and the test still passes. The PID filter can also select an unrelated management query. Identify the backend by query/locked relation and instance where possible, then assert an observable second retry or timeout outcome before releasing the lock.
| attempts := attempts + 1; | ||
| END LOOP; | ||
|
|
||
| IF lower(COALESCE(engine_status, '')) != 'failed' |
There was a problem hiding this comment.
Low — NULL diagnostic output bypasses the rollback assertion. In PL/pgSQL, when the status is failed and engine_output is NULL, false OR NULL evaluates to NULL, and IF NULL THEN does not enter the failure branch. Add engine_output IS NULL OR ... to both the whole-rollback and savepoint-rollback checks so missing diagnostics fail the test.
Summary
df.start()graph rowspg_xact_status()with bounded single-shot activities plus deterministic durable backoffcontinue_as_newProblem
df.start()persistsdf.instances/df.nodesin the caller transaction but commits the duroxide start separately. The graph loader previously stopped waiting after five seconds. A legal transaction held for eight seconds then committed with the control-plane row stillpending, no SQL effect, and the engine execution terminallyFailedwithInstance <id> not found after 5s.Synthetic reproducer:
The new regression also terminates a lock-blocked management backend and holds the lock long enough for the next probe to time out, proving both transient error paths retry and the effect still occurs exactly once.
Compatibility
No extension schema, ABI, or persisted
dfdata changes. HistoricalFunctionInputpayloads defaultorigin_xidtoNoneand schedulepg_durable::activity::load-function-graphwith the same raw input bytes and operation order. New starts alone use the versioned transaction-aware probe activity. Upgrade B1 passed against every supported v0.2.2-v0.2.6 schema on PostgreSQL 17 and 18.Validation
cargo fmt -p pg_durable -- --checkcargo buildandcargo clippy -- -D warningscargo pgrx test: 333 passed, 0 failed, 16 ignored each