Add a node failure policy: retries, and what happens when retrying doesn't help - #354
Draft
Todd J. Green (tjgreen42) wants to merge 12 commits into
Draft
Add a node failure policy: retries, and what happens when retrying doesn't help#354Todd J. Green (tjgreen42) wants to merge 12 commits into
Todd J. Green (tjgreen42) wants to merge 12 commits into
Conversation
The existing tests that assert a node failure propagates now opt into max_attempts => 1, on_failure => 'fail', since the default policy retries and, inside a loop, continues.
Reworks the policy in response to self-review. Replay compatibility. serde(default) governs deserialization only -- the field is still written. duroxide matches a StartSubOrchestration against history on name and input equality, so a 0.2.7 parent emitting a "retry" key produced an envelope unequal to the one a 0.2.6 parent recorded, and every in-flight JOIN branch, RACE branch, and non-root loop child would have hit a nondeterminism error on upgrade. Both FunctionInput.retry and SubtreeInput.retry now skip serialization when they hold the legacy value -- keyed off that value rather than off Default, so a real policy still reaches subtrees. Same break class as v0.2.4 -> v0.2.5. Defaults. max_attempts is 1 and on_failure is 'fail', which is exactly what df.start() did before 0.2.7. Retrying is now opt-in. Silently converting every existing workflow's fail-fast semantics into retry-and-continue is not a change a caller should discover in production. This is why tests 13, 14, 45, 61, and 64 are back to their original form: they assert node failures and still observe them. Test 68 gains a case pinning the defaults, so "upgrading changes nothing" is a test rather than a claim. SQLSTATE classification. An error that is a property of the statement rather than of the moment is no longer retried, however high max_attempts is, since a retry reproduces it byte for byte: classes 42, 23, 28, 3D and 3F. Class 22 stays retryable deliberately -- it describes the data, which another node can change between attempts. execute_sql stamps the SQLSTATE into the message and the orchestration matches on the class. duroxide's schedule_activity_with_retry retries every error with no predicate hook, so the retry loop is hand-rolled; it emits the identical sequence of durable operations, which is what preserves replay compatibility. df.instance_activity(). Reports every non-terminal instance with its last node transition, idle time, running and failed node counts, and last error. status alone reads 'running' for a healthy eternal loop, one blocked on a signal that never arrives, and one retrying a broken node -- and 'continue' makes that last case reachable, so the loop iteration cap being gone needs a way to see it. A SECURITY INVOKER function rather than a view, because view-level RLS pass-through needs security_invoker, which is PostgreSQL 15 and this extension supports 13. Idle time is measured against clock_timestamp(): now() is the calling transaction's start time, and the worker keeps writing node timestamps after it, so a busy instance's activity reads as being in the future and falls below any threshold.
Reflects the opt-in defaults, the SQLSTATE classification, and df.instance_activity() across the changelog, API reference, user guide, and spec. Corrects the B2 note in docs/upgrade-testing.md, which claimed the opposite of the truth: a serde default does not make a new field safe to add to an orchestration input, because the field is still serialized and duroxide compares inputs by equality during replay. It is now split into the two guarantees that actually hold, plus the downgrade path. Removes docs/plan-failure-policy.md. It was scaffolding addressed to agentic workers rather than to readers of the repository, has no precedent in docs/, and had been overtaken by this rework on both the defaults and the serde claim above.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the node failure policy from
docs/spec-failure-policy.md(issue #155).The problem
Nothing is retried today, and one failed node fails the whole instance. Any workflow that runs long enough hits a deadlock, a lock timeout, or a dropped connection. A background compactor meant to run indefinitely is marked
failedon the first bad night, and nothing runs again until a human notices.What this adds
Three new
df.start()arguments:max_attempts1max_backoff'16 seconds'on_failure'fail'The defaults are the pre-0.2.7 behaviour, so the feature is entirely opt-in and upgrading changes nothing. Silently converting every existing workflow's fail-fast semantics into retry-and-continue is not a change a caller should discover in production. This is also why no existing E2E test needed editing: the five that assert a node failure still observe one.
A failing
df.sql(),df.http(), ordf.http_multipart()node is retried with exponential backoff (1s, doubling, capped atmax_backoff). The wait is a durable timer, so it holds no connection and survives a restart.Once the attempts are spent,
on_failure => 'continue'abandons the rest of the current loop iteration and starts the next one;'fail'fails the instance. Outside a loop there is no next iteration, so both settings fail the instance —'continue'is a statement about recurring work.What is not retried
An error that is a property of the statement rather than of the moment is not retried at all, however high
max_attemptsis, because a retry reproduces it byte for byte: SQLSTATE classes 42 (syntax or access rule violation), 23 (integrity constraint), 28 (invalid authorization), 3D and 3F. Everything else is retried, including everydf.http()error, which carries no SQLSTATE. Class 22 (data exception, e.g. division by zero) is retried deliberately — it describes the data, which another node can change between attempts.duroxide's
schedule_activity_with_retryretries every error with no predicate hook, so the retry loop is hand-rolled. It emits the identical sequence of durable operations, which is what preserves replay compatibility.Graph-level errors (malformed graph, unknown node type, failure to start a sub-orchestration) are not transient and still fail immediately.
df.instance_activity()Also adds a way to tell a working workflow from a wedged one:
statusalone reads'running'for a healthy eternal loop, one blocked on a signal that never arrives, and one retrying a broken node — and'continue'makes that last case reachable. This reports every non-terminal instance with its last node transition, how long it has been quiet, its running and failed node counts, and its most recent error.It is a
LANGUAGE SQLfunction rather than a view because view-level RLS pass-through needssecurity_invoker, which is PostgreSQL 15 and this extension supports 13; as an ordinary SECURITY INVOKER function the existing policies ondf.instancesanddf.nodesfilter it to the caller's own rows.This matters because the PR also removes the 100,000-iteration
df.loop()cap. That cap was never a meaningful storage bound — a large carried result exhausts storage long before the count trips — and it gave a workflow meant to run indefinitely an arbitrary expiry date. Removing it means the way to spot a loop that is running without making progress has to be something better than waiting 27 hours for it to die.Behaviour change
None by default. The one caveat applies only when you opt in: under
'continue'the loop'swhilecondition is deliberately skipped along with the rest of the iteration, because it usually reads named results the abandoned iteration never produced. The consequence, documented in the user guide: awhileloop whose body always fails never terminates.df.instance_activity()is how you see that, anddf.cancel()is how you stop it.Replay and upgrade
FunctionInput.retryandSubtreeInput.retryare not serialized when they hold the legacy policy. Aserde(default)alone is not sufficient and the first draft of this PR got that wrong: a default governs deserialization only, the field is still written, and duroxide matches aStartSubOrchestrationagainst history on name and input equality. A 0.2.7 parent emitting aretrykey would have produced an envelope unequal to the one a 0.2.6 parent recorded, failing every in-flight JOIN branch, RACE branch, and non-root loop child with a nondeterminism error. Same break class as v0.2.4 → v0.2.5.docs/upgrade-testing.mdsaid the opposite of the truth here and is corrected.sql/pg_durable--0.2.6--0.2.7.sqldrops the four-argumentdf.startand creates the seven-argument one against a new symbol (start_v3_wrapper). Both cannot coexist — a four-argument call would match both. The four-argument Ruststart_v2()stays in the binary as#[pg_extern(sql = false)], so un-upgraded schemas keep resolving to it (Scenario B1).transaction_mode => 'new'forwards the policy over the loopback session only when one was supplied; otherwise it issues the original three-positional-argumentdf.start(), which resolves on every shipped schema.Testing
Built test-first, one commit per task.
delay_for_attempt, SQLSTATE classification (permanent classes, transient classes, no marker, malformed marker), legacy deserialization for both input types, non-serialization of a legacy policy in both directions, and policy round-trip throughcontinue_as_new.tests/e2e/sql/68_failure_policy.sql— transient recovery,'continue'in a loop, no-enclosing-loop,'fail'with one attempt, defaults are legacy, and argument validation. Attempts are counted with a sequence, sincenextval()survives the rollback of the failed attempt that produced it.tests/e2e/sql/69_instance_activity.sql— live instance, idle threshold, a loop wedged under'continue'surfacing its error while still'running', terminal exclusion, and RLS isolation.Docs:
USER_GUIDE.md,docs/api-reference.md,docs/upgrade-testing.md,docs/spec-failure-policy.md,CHANGELOG.md.