Skip to content

Rust #325: task snapshots on every event PHP records with a task, and describe command class/preview - #718

Closed
rmcdaniel wants to merge 5 commits into
rust-325-history-presentationfrom
rust-325-task-snapshots
Closed

rmcdaniel wants to merge 5 commits into
rust-325-history-presentationfrom
rust-325-task-snapshots

Conversation

@rmcdaniel

Copy link
Copy Markdown
Member

Part of #325. Stacked on #710 (rust-325-history-presentation); it will be retargeted to main once #710 merges.

Problem

Published PHP's WorkflowHistoryEvent::record adds a task snapshot to every event it records with the task model in hand. Native added snapshots event by event and missed several. The same workflow was driven against both servers through a side effect, an activity, a timer, a child, a signal wait and a condition wait. Native had no snapshot on these events:

  • TimerFired;
  • ChildWorkflowScheduled and ChildRunStarted;
  • SignalWaitOpened, ConditionWaitOpened and ConditionWaitSatisfied;
  • SignalApplied and the instance MessageCursorAdvanced.

Its own snapshots on TimerScheduled and the activity events also dropped fields PHP keeps.

Change

  • When an event of a type that PHP records with the task model is appended with a task and has no snapshot, native adds the task's snapshot. The list of types comes from the Workflow package's source.

  • TimerScheduled uses the full snapshot. ActivityStarted, ActivityCompleted and ActivityFailed keep PHP's fields (queue, connection, lease_expires_at, repair_count).

  • A timer task is leased to itself while its timer fires, as RunTimerTask does: it gets a workflow-task lease and one more attempt. The lease is cleared once the timer has fired, so TimerFired carries the leased timer task.

  • Described commands: each command in a run description now names the run's workflow_class and carries payload_preview_omitted, as PHP's WorkflowController does. With fresh workers on both sides, a field-by-field diff of the same workflow's description then differs only in the deployment's random request_route_name and the fingerprint hashed from it.

Record: docs/rust-server-port-checkpoints/2026-10-23-task-snapshots.md.

Checks

  • The probe now finds the same events with snapshots on both servers, with the same fields. The remaining differences are listed below.
  • New integration test events_recorded_with_a_task_carry_its_snapshot covers the timer, child, signal-wait, signal and condition-wait events.
  • Full native SQLite suite passes locally (700 tests, no failures).

Left as they are

Published PHP's WorkflowHistoryEvent::record adds the task's snapshot to each
event it records with the task model. Native now adds it centrally for those
event types, uses the full snapshot on TimerScheduled and the activity
events, and leases a timer task to itself while it fires, as RunTimerTask
does, so TimerFired carries the leased timer task.
PHP's run description names the run's workflow class on every command and
says whether the command's payload preview is omitted. Native left both out.
Only a stored external reference is read to decide the flag.
@rmcdaniel

Copy link
Copy Markdown
Member Author

Security/data-boundary review at exact 05a6110: no command payload is emitted by this diff, but the new preview flag is incorrect for the native stored-reference shape and the metadata-only query is broader than its comment.

  1. external_resolution::stored_envelope returns {codec, external_payload:{...size_bytes...}}. run_describe::oversized reads only external_storage.size_bytes. Therefore the new command payload_preview_omitted expression is always false for every valid native stored reference, including one above DW_MAX_PAYLOAD_BYTES. The existing helper tests cover only the PHP external_storage shape. Correct the predicate for the already-validated native external_payload reference and add an actual stored command reference at the limit and one byte above it; prove the command output contains only the boolean and no reference/payload fields.

  2. CASE WHEN c.payload LIKE dw-% selects and copies any inline customer payload beginning with that broad prefix before stored_envelope rejects it. That contradicts the stated inline-payload-unread boundary. Match the exact fixed native stored prefix (with a database-portable/escaped comparison) so arbitrary inline bytes are never selected merely for preview metadata; retain malformed-reference and large-inline counterfactuals.

  3. The new centralized snapshot lookup is SELECT * FROM workflow_tasks WHERE id=$1 while record also has the owning run_id. Bind workflow_run_id=$2 (or explicitly verify it) before embedding a task snapshot, and add a mismatched-run control. Random task IDs and currently correct callers are useful, but the history serializer should not be able to copy another run task if a future/internal caller supplies mismatched IDs.

The expanded task snapshot field set itself excludes task payloads and credentials and stays within the existing tenant-owned history surface. Keep that property, the stacked #710 base, and fresh exact-head/current-main qualification. This head is not accepted yet.

…nto rust-325-task-snapshots

# Conflicts:
#	rust/tests/execution/worker_history_presentation.rs
…fix; run-bound task snapshots

Security review of #718:
- oversized() reads native's external_payload envelope as well as PHP's
  external_storage, so a stored reference over the payload limit omits the
  preview (and the target name, as PHP does). Run input and output previews
  use the same check.
- The described-command query reads a payload only when its first 30
  characters equal native's stored prefix, compared case-sensitively on each
  backend (COLLATE BINARY, binary cast, PostgreSQL text).
- record() reads a task snapshot only from a task of the event's own run.

Tests: a real stored reference at the 2 MiB limit and one byte above,
upper-case/v2/malformed/large-inline counterfactuals, the query itself on
SQLite, and a mismatched-run event with no snapshot.
@rmcdaniel

Copy link
Copy Markdown
Member Author

Corrections for the Security review (comment 6376568945), at exact head 65772abac8ecb65f7bde7f85c56b80bb77054777:

  1. Preview omission for native stored references. run_describe::oversized now reads external_payload.size_bytes (native's stored envelope) as well as external_storage.size_bytes (PHP's). This also corrects the run's input/output preview flags, which had the same gap on main. As in Server 2.11.1 WorkflowController, an omitted preview also omits the command's target_name.
  2. Exact stored prefix. The described-command query reads c.payload only when its first 30 characters equal dw-native-external-payload-v1: exactly. Each backend compares case-sensitively: COLLATE BINARY on SQLite, CAST(... AS BINARY) on MySQL/MariaDB, and PostgreSQL's text =. The query is a per-backend constant (Backend::RUN_COMMANDS_SQL) built from one template. On MySQL 8.0.46 and MariaDB 10.11.19, the binary cast refuses an upper-case prefix that the utf8mb4_unicode_ci comparison would match (0 vs 1).
  3. Run-bound task snapshot. record() reads SELECT * FROM workflow_tasks WHERE id=$1 AND workflow_run_id=$2.

Tests:

  • external_payloads::commands::described_commands_omit_only_an_oversized_stored_reference (SQLite, PostgreSQL 17, MySQL 8.0): a real uploaded reference on a signal (not omitted, target_name "s"). Then the same native stored reference at 2 MiB (not omitted) and 2 MiB + 1 (omitted, no target_name). Then counterfactuals that are not native references (upper-case prefix, v2 prefix, the prefix with no valid reference after it, and a 2 MiB + 1 inline payload starting with dw-), all not omitted. Every described command is checked to contain no payload, reference, size_bytes, sha256 or ep_ identity.
  • runtime::store::record_tests::described_commands_select_no_inline_payload: the query itself on SQLite returns stored_payload only for the exact prefix.
  • runtime::store::record_tests::described_commands_read_only_the_exact_native_stored_prefix: checks each backend's exact comparison, with no LIKE.
  • runtime::store::record_tests::a_task_snapshot_comes_only_from_the_events_own_run: an event recorded on one run with another run's task carries no snapshot.

Counterfactuals: the old external_storage-only predicate fails the API test at 2 MiB + 1. Restoring LIKE 'dw-%' and the unbound lookup fails all three unit tests.

Local: SQLite cargo test --all-targets passes: lib 250, execution 453, plus the other targets. The new and affected execution tests also pass on PostgreSQL 17 and MySQL 8.0. The checkpoint records the corrections.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Security re-review at replacement head 65772abac8ecb65f7bde7f85c56b80bb77054777: two prior blockers are correctly cleared. Task snapshots are now bound by both task ID and event workflow_run_id, with a mismatched-run control; and the backend predicates compare the complete 30-byte native sentinel case-sensitively instead of broad/case-insensitive LIKE.

One data-path blocker remains. The query calculates stored_payload only from c.payload, but the real external-signal path does not put the stored reference there. Runtime::signal replaces only prepared.arguments = external.stored(); Store::signal writes that value to workflow_signal_records.arguments (s.arguments). workflow_commands.payload remains the Avro-encoded command produced before that replacement. Consequently a real oversized uploaded signal reference is never selected and its described command still reports payload_preview_omitted=false / retains target_name.

The new test masks this mismatch: it performs one real small upload, then rewrites workflow_commands.payload directly to the sentinel strings. It never asserts or varies the actual workflow_signal_records.arguments reference column. Please select the exact sentinel from the real command-specific storage path (for a signal, s.arguments; retain c.payload only for command types that actually store the reference there), still returning no ordinary inline bytes. Add a counterfactual that proves the real signal row is the source and that an actual or faithfully mutated s.arguments reference at 2 MiB+1 omits both preview and target while exact-limit, upper-case, v2, malformed, and large inline values do not. Preserve the current case-sensitive MySQL/MariaDB/SQLite controls.

No merge acceptance yet. Hosted checks are also still pending, and #718 remains stacked on #710.

Security review of #718 at 65772ab: native keeps a signal's stored input
reference in workflow_signal_records.arguments; workflow_commands.payload
holds the encoded command. Read the exact native sentinel from s.arguments
for signals and from c.payload for starts only, case-sensitively per
backend. The describe test now proves the real signal row is the source,
mutates that row around the limit with the same counterfactuals, shows an
oversized sentinel in a signal's command row does not count, and covers
the start command's row.
@rmcdaniel

Copy link
Copy Markdown
Member Author

Correction for the Security re-review (comment 6378027005), at exact head 38717d3372767b72cf17851b0bd16ca935515e81.

Query. The stored sentinel is now read from each command type's real input column:

CASE WHEN c.command_type='signal' AND <exact prefix test>(s.arguments) THEN s.arguments
     WHEN c.command_type='start'  AND <exact prefix test>(c.payload)   THEN c.payload END
  • Signals keep their reference in workflow_signal_records.arguments; Runtime::signal puts external.stored() there.
  • Starts keep it in workflow_commands.payload; Store::start binds the stored arguments there.
  • No other native command stores one: updates and queries resolve references to inline bytes.
  • The comparisons keep the per-backend case-sensitive forms: SQLite COLLATE BINARY, MySQL/MariaDB CAST(SUBSTRING(...) AS BINARY), PostgreSQL text =.

Tests.

described_commands_omit_only_an_oversized_stored_reference (SQLite, PostgreSQL 17, MySQL 8.0):

  • After a real upload and signal, the signal row's arguments is the native sentinel carrying that reference_id, and the signal's workflow_commands.payload is not.
  • workflow_signal_records.arguments is rewritten to the same reference at 2 MiB (not omitted) and 2 MiB + 1 (omitted, no target_name).
  • Counterfactuals: an upper-case prefix, a v2 prefix, the prefix with no valid reference after it, and a 2 MiB + 1 inline payload starting with dw-. None is omitted.
  • With the signal row at exactly the limit, an oversized sentinel written into the signal's own c.payload is not counted.
  • The start command's c.payload goes through the same sizes and counterfactuals.
  • Every described command is checked for no payload, reference, size, sha256 or ep_ identity.

described_commands_select_no_inline_payload runs the actual SQLite query over eight rows. It returns stored_payload only for a signal row with the exact prefix in s.arguments and a start row with the exact prefix in c.payload. It returns nothing for:

  • upper-case, near-miss and large inline signal arguments;
  • a signal whose command row holds the sentinel;
  • a large start payload;
  • an update whose payload holds the sentinel.

described_commands_read_only_the_exact_native_stored_prefix checks the exact two-branch expression on each backend.

Counterfactual. Restoring the previous c.payload-only expression fails the API test at the real signal-row 2 MiB + 1 case: payload_preview_omitted false.

Local. SQLite cargo test --all-targets passes (lib 250, execution 453, plus the other targets). The changed tests pass on PostgreSQL 17 and MySQL 8.0. The checkpoint now names the per-command columns.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Security re-review of exact head 38717d3372767b72cf17851b0bd16ca935515e81: the prior signal-storage blocker is cleared. The query now selects an exact, case-sensitive native stored-reference sentinel from workflow_signal_records.arguments for signal commands and from workflow_commands.payload for start commands only; inline payloads and wrong-column/type counterfactuals remain unread. The real-upload test proves the signal reference is absent from the command row, then exercises the actual signal row at and above the limit, and the start path is covered independently. I accept this corrective boundary. Fresh normal CI is still pending; this is not a blanket acceptance of unrelated/native parity work.

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