Skip to content

Rust #325: retry children and keep their timeout options as published PHP does - #869

Merged
rmcdaniel merged 3 commits into
mainfrom
rust-325-child-retries
Oct 29, 2026
Merged

rmcdaniel merged 3 commits into
mainfrom
rust-325-child-retries

Conversation

@rmcdaniel

Copy link
Copy Markdown
Member

Part of #325.

What changes

Native refused retry_policy, execution_timeout_seconds and run_timeout_seconds on start_child_workflow with 422 unsupported_request_field. It now does what published Server 2.11.7 / Workflow 2.15.7 do (ChildWorkflowRetryPolicy, applyStartChildWorkflow, startChildRetryIfAvailable):

  • Recorded options. A retry snapshot and a timeout snapshot go on:

    • the call;
    • ChildWorkflowScheduled and ChildRunStarted;
    • the child's WorkflowStarted.

    A policy with no known key records nothing. Each timeout sets its own deadline on the child run.

  • Retry. A failed or timed-out child run is retried as a new run of the same child instance, under a new link, while the policy has attempts left and the failure is retryable.

    • The retry keeps the call and the execution deadline, and starts its own run deadline.
    • Its task waits out the backoff.
    • The parent records ChildRunStarted with retry_attempt, retry_of_child_workflow_run_id and retry_backoff_seconds, without null fields, and keeps waiting.
  • No retry. The parent sees the child fail when:

    • the failure is marked non-retryable;
    • the policy names the failure's type or class;
    • or attempts run out.
  • Description. The child summary follows the call to its latest run, so the parent stays described as waiting for the child.

  • Timed-out child. Its ChildRunFailed now names closed_reason: timed_out, as PHP's does; the call keeps failed. The first native recording of the new fixture found this.

Fixture child-retries

Seven cases over raw worker HTTP on protocol 1.20:

  • retry then complete, with backoff and both timeouts;
  • attempts exhausted;
  • a non-retryable type;
  • the non_retryable flag;
  • an empty policy;
  • timeouts only;
  • two run timeouts.

IDs are labels and deadlines are recorded by presence, with how a retry's deadlines relate to the first run's.

Local checks

  • child-retries. On a fresh published 2.11.7 harness and on native built from this branch, both pass and compare accepts the pair.
  • Neighboring fixtures. child-descriptions, two-child-workflows, parent-close-cancellation and direct-child-cancel-before-claim pass on that native image.
  • Rust suites. After rebasing on Rust #325: move the parity reference to published Server 2.11.7 #867: library 362, integration 638, plus the smaller binaries, all passing. The new native tests are in rust/tests/execution/child_retries.rs.
  • Node unit suite. 6376 pass. PhpImageContextTest fails the same way on a clean main checkout here. The heartbeat cleanup test hit an 82 ms deadline while an image build loaded the host, and passed when rerun alone.

Not covered

From reading the code, not from a recording:

  • native's cooperative cancellation delivery expects one link per call, so it refuses one delivered to a retried child;
  • child runs, retried or not, still inherit no memos or search attributes.

Checkpoint.

🤖 Generated with Claude Code

Native refused retry_policy, execution_timeout_seconds and
run_timeout_seconds on start_child_workflow. It now records the retry and
timeout snapshots on the call, the parent's child events and the child's
started event, gives each child run its deadlines, and retries a failed
or timed-out child as a new run of the same child instance under a new
link while the policy has attempts left and the failure is retryable
(Workflow's startChildRetryIfAvailable). The retry keeps the execution
deadline, starts its own run deadline and waits out the backoff. The
child summary follows the call to its latest run, so the parent stays
described as waiting for the child.

A timed-out child's ChildRunFailed now names closed_reason timed_out, as
PHP's does.

The child-retries fixture records seven cases against published 2.11.7.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@rmcdaniel rmcdaniel left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security source review at exact head 8c2e217d3b1282669265d6743dd91c4853b3b38a found one bounded-availability correction and one already-documented compatibility gap.\n\n1. completion_rules admits any positive/nonnegative i64 for the new child timeouts and retry backoff, while children.rs computes deadlines with DateTime + chrono::Duration::seconds(value) at initial start and retry. Extreme valid inputs (including i64::MAX) can overflow/panic duration/date construction instead of producing a controlled protocol response. Please use checked duration/date arithmetic or a qualified upper bound and add an extreme-value control for the new child path. This is an authenticated-worker robustness issue, not evidence of exploitation or an incident.\n\n2. The PR body correctly notes that cooperative cancellation after a retry is not covered. The failure is deterministic: retry inserts another workflow_links row for the same parent run/sequence, while validate_child_cancellation_boundary requires exactly one link and also requires original scheduled/started link identities to equal it. A cancellation delivered after the first retry therefore refuses before targeting the current child. Because this is a new pre-release feature combination, I am treating it as a non-incident compatibility item: preferably follow the call row current resolved run/link and accept original scheduled plus latest-started history; otherwise reject that option combination explicitly and track the narrower limitation.\n\nNo credential, tenant-boundary, publication, or existing released-surface finding was identified in the reviewed retry logic. I will re-check the exact correction head; normal hosted fixture failures and Rust qualification remain with the owner.

The retry read its next run number from MAX() of an unsigned MySQL
column, which does not decode as a signed integer, and repeated a
placeholder in one statement, which MySQL's statement templates refuse.
Read the latest run as a row and bind the run count twice. The
child-retries fixture passes on MariaDB 10.11, PostgreSQL 17 and SQLite.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rmcdaniel

Copy link
Copy Markdown
Member Author

The first CI run failed mysql-execution (mariadb-10.11, corpus): native could not start a child retry on MySQL. Two causes, both reproduced locally against the pinned MariaDB 10.11 image:

  • MAX(run_number) of MySQL's unsigned column does not decode as a signed integer (column_decode);
  • the run_count update repeated a placeholder, which native's MySQL statement templates refuse.

f6c58c79 reads the latest run as a row and binds the value twice. child-retries now passes on native with MariaDB 10.11, PostgreSQL 17 and SQLite, and no runtime panic or storage error was logged.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Security re-check at f6c58c7: the 12-line delta is confined to the MySQL/MariaDB latest-run query and unique placeholder binding. It does not change the admitted i64 timeout/backoff timestamp arithmetic or the retry-plus-cooperative-cancellation link selection described in my review, so those two items remain. The reported normal storage-suite absence of a panic is useful regression evidence but does not exercise the extreme admitted value that triggers the bounded-availability concern. I will review the exact correction head when pushed; no unrelated suite rerun is requested.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Risk-tier correction after reviewing the actual exposure boundary: neither item is a Security merge hold for this pre-release native feature. The extreme-value path is real robustness debt, but it requires an authenticated worker to submit an extreme admitted value, the Rust Cargo profile does not opt into panic=abort, and no current production/native released surface is exposed. The retry-plus-cooperative-cancellation behavior is likewise a new combination already called out as incomplete. Please track both for correction or explicit rejection before RC/qualified native release; they should not by themselves delay this isolated pre-release merge. Existing-surface regressions, tenant-boundary issues, credential exposure, or deployed availability impact would still block. I will review a correction whenever the owner schedules it.

Security's review of #869 found that an admitted i64::MAX child timeout
or backoff could overflow chrono's date arithmetic. A child timeout or
backoff entry too far away is now refused at admission (422
invalid_child_timeout or invalid_child_retry_policy), as a timer delay
or activity backoff is, and every child deadline uses checked arithmetic.

Cooperative cancellation delivered to a retried child stays refused and
is tracked in #873.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rmcdaniel

Copy link
Copy Markdown
Member Author

Security review disposition (eb7abf8d):

  1. Extreme timeout or backoff. A child timeout or backoff entry too far away for a timestamp is now refused at admission with 422 invalid_child_timeout or invalid_child_retry_policy, as a timer delay or activity backoff is. Every child deadline (first run, retry run, retry availability) now uses checked arithmetic. Controls:
    • a_child_deadline_or_backoff_too_far_away_is_refused_not_overflowed sends i64::MAX for each field, then completes the same task normally;
    • unit test a_deadline_too_far_away_is_refused_rather_than_overflowing.
  2. Retry plus cooperative cancellation. Confirmed: delivery to a retried child is refused with 409 cancellation_delivery_shape_mismatch. Published 2.11.7's retry ChildRunStarted omits cancellation_policy, so the right boundary shape needs a PHP recording first. Tracked in Rust #325: cooperative cancellation delivered to a retried child #873, linked from the port doc and the checkpoint.

The previous head f6c58c79 finished CI with no failures. The native retry tests (5) pass locally on this head.

@rmcdaniel

rmcdaniel commented Oct 29, 2026 •

Copy link
Copy Markdown
Member Author

Security exact-head follow-up at eb7abf8d87938658dce3ac72bfa3b4c0d27df50a: the availability finding is addressed. Initial child deadlines, retry deadlines, and retry availability now use checked duration/date arithmetic, so an extreme authenticated-worker value produces a controlled refusal instead of a panic. The i64::MAX controls cover both the helper and the end-to-end task path. The retry-plus-cooperative-cancellation combination is explicitly tracked in #873 pending the PHP boundary recording; that remains an unreleased compatibility limitation, not a Security merge hold.

One narrow wording/normalization nuance remains: Laravel array validation also accepts associative JSON objects, while the admission-time backoff guard currently inspects only as_array(). An extreme associative backoff therefore reaches the same checked 422 refusal when retry is attempted rather than necessarily at admission. This does not restore the panic path or change the no-hold disposition; broadening that guard or qualifying the admission wording can ride as pre-RC correctness cleanup.

No credential, tenant-boundary, existing released-surface, or deployed-availability regression was identified in this correction. Security source review is complete with no merge hold; hosted exact-head CI remains the normal owner gate.

@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant