Skip to content

Make workflow-code message sends and receives act once across replays - #1012

Merged
rmcdaniel merged 3 commits into
mainfrom
fix/1010-replay-safe-messages
Nov 4, 2026
Merged

rmcdaniel merged 3 commits into
mainfrom
fix/1010-replay-safe-messages

Conversation

@rmcdaniel

Copy link
Copy Markdown
Member

Fixes #1010 for a 2.20.1 patch. Triage and sweep: #1010 comment 6618558654.

Problem

Workflow code runs again on every later workflow task and on every query.

  • outbox()->sendReference() called MessageService::sendMessage directly, so each replay wrote a new outbound and inbound row.
  • inbox()->receive() / receiveOne() peeked the current pending set, so a replay could return different messages or none, and could consume a newer message under an earlier step's sequence.
  • Query replay, WorkflowReplayer and the redrive definition check all go through QueryStateReplayer, so they could write or consume too.

Fix

Workflow::messages() / inbox() / outbox() now give workflow code a MessageStream that carries a per-execution MessageReplayLedger, plus a read-only flag (! commandDispatchEnabled). Query replay already sets that flag.

  • Send. The stream matches the earliest outbound row this run wrote with the same channel, stream, target, payload_reference, correlation_id and idempotency_key that this execution hasn't already matched. It returns that row with no write. It writes only when no such row is left. In read-only mode it returns an unsaved model.
    • Matching by content plus a claimed set keeps intended identical sends separate. It also means a run with duplicates from earlier releases stops adding more.
  • Receive. The stream returns the inbound messages this run consumed at this consumed_by_sequence on this stream, in sequence order, with a per-execution offset. It consumes from the pending set only when nothing is recorded. In read-only mode it never consumes.
  • MessageService itself is unchanged. Direct sends outside workflow code behave as before.

Tests (counterfactuals against 7a758480)

  • V2MessageStreamReplayTest:
    • Send per turn with queries (the docs-agent repro): outbound and inbound rows go 1→2→2→2→3, and each query leaves workflow_messages byte-identical. Fails on base.
    • Receive replays the consumed message: a query returns ['m1'] and consumes nothing, turn 2 takes m2, and m3 stays pending. Fails on base, where the query returns [null].
    • Identical sends stay separate: 2 copies per turn give 2 then 4 rows, stable across a query.
    • Duplicates from earlier releases: after injecting 2 extra ref:1 rows, each later turn adds exactly one row, and queries add none.
  • Replay corpus: message-stream-send-once.json plus the scenario testMessageStreamSendCorpusWritesEachSendOnceAcrossReplaysAndQueries. Fails on base at step 1 (4 rows instead of 2).
  • Existing suites pass locally:
    • V2MessageStreamAuthoringTest and unit MessageStreamTest.
    • The V2 Query, Update, LeasedSignal, MessageCursor, Replayer, GoldenHistory, ReplayCorpus and FailedRunRedrive feature tests (234/234).
    • ReplayRegressionCorpusTest (99/99).
    • ECS and PHPStan clean.

Limits, in the CHANGELOG

  • Rows that earlier releases duplicated are left in place. Reads don't deduplicate them, and no migration can tell a replay duplicate from an intended identical send, so consumers should deduplicate by idempotency_key.
  • A replayed receive whose original call found nothing reads the stream again, because no history records an empty receive.
  • Signals and other streams advance a shared message cursor and hide pending stream messages #1011, the shared message cursor that hides older pending messages, is separate.

🤖 Generated with Claude Code

Workflow code runs again on every later workflow task and on every
query. outbox()->sendReference() wrote a new outbound and inbound row
each time, and inbox()->receive() could return different messages and
consume a newer one under an earlier step (#1010).

Each execution now keeps a ledger. A send returns the earliest matching
row this run already wrote and has not matched yet, and writes only when
none is left. A receive returns the messages this run consumed at that
step and stream. A read-only replay (query, replayer, redrive definition
check) never writes or consumes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rmcdaniel
rmcdaniel marked this pull request as ready for review November 4, 2026 00:09
@rmcdaniel

Copy link
Copy Markdown
Member Author

Terminal hosted CI on exact head f608dff1f258676979466ee269f9faaba9feee43: 16 pass, 7 skipped, 0 failing. Merge state is CLEAN.

  • The diff against main 7a758480 is 9 files:
    • source: MessageStream.php, Workflow.php and the new Support/MessageReplayLedger.php;
    • tests: the new V2MessageStreamReplayTest and its 2 fixtures, the corpus fixture message-stream-send-once.json and its scenario;
    • CHANGELOG.md.
  • Counterfactuals: against base source, the send test, the receive test and the corpus scenario all fail. The corpus scenario sees 4 rows instead of 2 at step 1.
  • Pending: PR CI runs the selected unit and feature subsets. A full local unit-suite comparison against main 7a758480 is running, to show that its environment-only failures are the same set on both trees. I'll post the result here.
    Requesting General interim review (Security unavailable).

@rmcdaniel

Copy link
Copy Markdown
Member Author

General interim review (Security unavailable; operator instruction at cloud#293 6617758869): exact head f608dff1f258676979466ee269f9faaba9feee43: HOLD.

Reviewed by an independent reviewer, not the authoring agent.

Confirmed:

  • The named tests fail on 7a75848 and pass at the head (bounded PHP 8.5 + MySQL 8.4). The corpus scenario, ReplayRegressionCorpus 99/99, MessageStream 6/6 and V2FailedRunRedrive 123/123 also pass.
  • Query, the replayer, conformance and the redrive definition check are write-free when the stream is opened at the call site.
  • Matching is deterministic (by sequence, then id); continue-as-new doesn't match across generations; the run row is locked FOR UPDATE, so concurrent tasks serialize.
  • There's no schema or history-event change.

H1 (blocking; a new regression where a send is lost): readOnly is captured when the stream is created (Workflow.php:195; MessageStream.php:32/123/196), not checked at call time. A workflow that keeps its stream in a property ($this->replies ??= $this->outbox(...)) and first opens it inside a replayed update handler (dispatch disabled, WorkflowExecutor.php:6462) gets a read-only stream for the rest of that task. Every later new send returns an unsaved model and writes nothing, and a kept inbox never consumes.

  • Repro (start → attemptUpdate('note','a') → signal → activity): the head writes outbound ["note:a"], with no ref:main row, while the workflow reports ref:main sent. 2.20.0 writes ref:main (with duplicates).
  • Fix: evaluate read-only per call (e.g. fn (): bool => ! $this->commandDispatchEnabled); the PR's tests stay green with this. Add the repro as a regression test.

M2 (required before release, at least as documentation): redrive successors still re-send sends from the reused history (2 rows vs 1), because matching is scoped to the current run, and reused receives differ. Either match against the redrive source chain, or state it as a known limitation and narrow the CHANGELOG's "act once instead of on every replay" claim.

M3 (CHANGELOG and docs):

  • Say that messages consumed under the wrong step by replays or queries on 2.17.1–2.20.0 stay consumed and won't be returned, so operators should check for them.
  • Cross-reference Signals and other streams advance a shared message cursor and hide pending stream messages #1011 next to the "receive after a signal" advice: the signal must come first.
  • Fix docs/architecture/execution-guarantees.md:253-256, which claims idempotencyKey drops duplicates at ingress; the key is only stored.

Lower:

  • L4: matching is by content only, with no step identity, so a workflow send can match an identical row written outside workflow code in the same run.
  • L5: the "already matched" NOT IN list grows per send, which is quadratic and risks Postgres's 65,535 bind limit on very long runs. Track matches per content key or offset instead.
  • L6: WorkflowFiberRunner.php:1238 (internal, unused) applies updates with dispatch off.

Re-review on the new head.

General's hold on f608dff:
- H1: a stream kept in a property and first opened by an update handler
  that replays with dispatch off stayed read-only, so later sends wrote
  nothing. Read-only is now checked on every call.
- M2: a redrive successor matched only its own rows, so it re-sent and
  re-consumed over the reused prefix. Sends and receives now match rows
  of this run and the runs it was redriven from.
- L5: matches are counted per send identity instead of listing every
  matched id, so lookups stay bounded.
- M3: the CHANGELOG names lost consumptions on affected releases, #1011
  and content matching; execution-guarantees.md no longer claims that
  idempotencyKey drops duplicates.

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

Copy link
Copy Markdown
Member Author

Addressed General's interim hold on f608dff1. New head b267cc8a62527a8a56c744df0d2b149ca2118ef3: hosted CI 16 pass, 7 skipped, 0 failing; merge state CLEAN.

  • H1, read-only checked per call. MessageStream::forWorkflowCode now takes readOnly: fn (): bool => ! $this->commandDispatchEnabled, and every send and receive evaluates it. A stream kept in a property and first opened by an update handler during a replay, with dispatch off, writes again once dispatch is back on.
    • New regression test testAStreamFirstOpenedInAReplayedUpdateHandlerStillWritesLaterSends, from your repro (start → attemptUpdate('note','a') → signal → activity). It expects outbound ['note:a','ref:main']. On f608dff1 it fails with ['note:a'].
  • M2, redrive chain. Sends and receives now match rows of this run and the runs it was redriven from. The ledger follows WorkflowStarted.recovery_kind=redrive → continued_from_run_id within the same instance, at most 1000 hops, computed once per execution.
    • New test testARedriveSuccessorReturnsTheSourcesReceiveAndSend: the source receives m1 and sends ref:m1, then fails, and m2 arrives. The redriven successor returns m1, sends no new row (outbound stays ['ref:m1']), and m2 stays pending. On f608dff1 it fails: the successor received null and sent ref:.
  • L5. Matches are now counted per send identity (a JSON of channel, stream, target, reference, correlation id and idempotency key) with skip(offset). There's no NOT IN list anymore, so binds stay constant.
  • M3.
    • The CHANGELOG now says that messages consumed under the wrong step on 2.20.0 and earlier stay consumed, and how to check for them.
    • The "receive after a signal" advice now says to send the signal before the message, and points to Signals and other streams advance a shared message cursor and hide pending stream messages #1011.
    • Content matching (your L4) is documented, with the advice to give workflow sends their own idempotency key. The claim is narrowed to name later tasks, queries and redrive prefixes.
    • docs/architecture/execution-guarantees.md: idempotencyKey is stored and ingress does not drop duplicates. A bullet describes the workflow-code replay behavior.
  • L6. No change. WorkflowFiberRunner.php:1238 is internal and unused, and now that read-only is checked per call, dispatch-off there can't leave a stream read-only.

Local results at b267cc8a:

  • V2MessageStreamReplayTest 6/6.
  • Related feature suites 237/237: MessageStream*, MessageCursor, Query, Update, LeasedSignal, Replayer, GoldenHistory, ReplayCorpus and FailedRunRedrive.
  • ECS and PHPStan clean.
  • Full unit suite against main 7a758480: 5,808 test cases. The same 9 environment-only failures occur on both trees (PublicBoundaryCheck ×3, two DefaultWorkflowControlPlaneRoutingContract, InspectionGuard, testRecordedLocalClassStillRedrivesAfterItsAliasIsRemoved, concurrent MySQL/SQLite import, published-migrations install). None are new and none are fixed.

@rmcdaniel

Copy link
Copy Markdown
Member Author

General interim review (Security unavailable), exact head b267cc8a62527a8a56c744df0d2b149ca2118ef3: HOLD

The reviewer was independent of the author. CI on this head is finished with 0 failing, and the merge state is CLEAN.

Confirmed fixed, both in the code and by counterfactual (PHP 8.4 + MySQL 8.4):

  • H1: read-only is checked on every call (Workflow.php:195). The new test fails with src at f608dff, sending only ['note:a'].
  • M2: the redrive chain is followed only through recovery_kind=redrive links in the same instance, with a cap and a cycle guard. The plan limits source rows to the reused prefix. The new test fails at f608 (received null, sent 'ref:').
  • L5: matches are counted per send identity with skip(), so the NOT IN list is gone.
  • M3: the CHANGELOG and execution-guarantees text are accurate.

All 6 V2MessageStreamReplayTest tests fail against base src. At this head, the feature suites pass 270/270 on MySQL (MessageStream*, MessageCursor, Query, Update, UpdateTailCheckpoint, LeasedSignal, Replayer, GoldenHistory, EmbeddedReplayRegressionCorpus, FailedRunRedrive, FailedRunRedrivePlan), and the unit MessageStream and MessageStreamCursor tests pass 19/19.

New blocking finding (Medium): a distinct send is silently dropped on MySQL/MariaDB.

  • Cause: MessageStream::recordedSend (src/V2/MessageStream.php:263-283) matches content with SQL =, which follows the column collation. Laravel's MySQL default, utf8mb4_unicode_ci, ignores case, accents and trailing spaces, but the send identity key is byte-exact.
  • Effect: a later send with different content matches the earlier row, writes nothing, and returns the earlier row. Examples are doc:README after doc:Readme, doc:résumé after doc:resume, and doc:a after doc:a.
  • Reproduced on this head with MySQL 8.4: the outbound list is [first] only, and the workflow sees the same row id for both sends. At 7a75848 the second send is written. On SQLite the case pair writes both rows.
  • Fix: use the SQL query only to find candidates. Then filter strictly in PHP with === over channel, stream, target, reference, correlation id and idempotency key, and take the offset-th match. Don't use SQL skip() for the offset. Add these three pairs as a regression test (the PR's MySQL job runs changed test files).

Low, docs only: on replay, a receive whose original call found nothing can return a message that a later call consumed at the same step. This applies to a stream kept in a property, because it captures its default sequence when created. The CHANGELOG says such a receive "reads the stream again"; please add that it can also return that later call's message. Don't change sequence capture in a patch release.

Follow-up issue (predates this PR): redrive copies only the cursor, so messages still pending on the source run at redrive time never reach the successor. At base, the successor received null while m2 was pending. The new test's "m2 stays pending" assertion hides this. Please file it separately.

I'll re-review the new head.

General's hold on b267cc8: on MySQL's case- and accent-insensitive
collation, SQL = matched doc:README to an earlier doc:Readme row (and
résumé to resume, 'a ' to 'a'), so the second send wrote nothing. The
query now only narrows candidates; sends compare all six fields and
receives compare the stream key exactly in PHP, and the offset is
applied to the exact matches.

Also widens the CHANGELOG note on empty receives and records #1013 in
the redrive test.

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

sonarqubecloud Bot commented Nov 4, 2026

Copy link
Copy Markdown

@rmcdaniel

Copy link
Copy Markdown
Member Author

Addressed General's interim hold on b267cc8a. New head 6928037b1c7edd2130d5ab341e775d06c4f0f22e: hosted CI 16 pass, 7 skipped, 0 failing; merge state CLEAN.

Hosted CI: 16 pass, 7 skipped, 0 failing, merge state CLEAN.

  • Collation match (Medium). MessageStream::recordedSend now uses SQL only to narrow the candidates. It then filters in PHP by comparing all six fields exactly against the raw stored values (channel, stream, target, payload_reference, correlation_id, idempotency_key), and takes the offset-th exact match, with no SQL skip(). recordedReceive likewise compares stream_key exactly in PHP before slicing by offset and limit.
    • New regression test testSendsThatDifferOnlyByCaseAccentOrTrailingSpaceStaySeparate. It sends doc:Readme/doc:README, doc:resume/doc:résumé and doc:a/doc:a , then replays after a signal. It expects 6 outbound rows in order, and the workflow sees 6 distinct row ids.
    • On MySQL 8.4 with b267cc8a's source it fails, writing only ['doc:Readme','doc:resume','doc:a']. It passes at this head.
  • Docs (Low). The CHANGELOG's empty-receive bullet now says that such a receive can return a newer message, or a message a later call consumed at the same step (for example, through a stream kept in a property). The content-matching bullet says the match is exact, whatever the collation. Sequence capture is unchanged.
  • Follow-up filed: Redrive doesn't move the failed run's pending inbound messages to the successor #1013, where redrive doesn't move the failed run's pending inbound messages to the successor. The redrive test's "m2 stays pending" assertion now says it records current behavior (Redrive doesn't move the failed run's pending inbound messages to the successor #1013), not a requirement.

Local results at this head:

  • On MySQL 8.4.13:
    • V2MessageStreamReplayTest 7/7.
    • Related feature suites 271/271: MessageStream*, MessageCursor, Query, Update, UpdateTailCheckpoint, LeasedSignal, Replayer, GoldenHistory, ReplayCorpus and FailedRunRedrive*.
    • Unit MessageStreamTest + ReplayRegressionCorpusTest 105/105.
  • ECS and PHPStan clean.
  • My first MySQL run errored partway through because the local MySQL container died; the rerun with more memory is the one reported.

@rmcdaniel

Copy link
Copy Markdown
Member Author

General interim review (Security unavailable), exact head 6928037b1c7edd2130d5ab341e775d06c4f0f22e: CLEAR. The reviewer was independent of the author. CI is 16 pass, 7 skipped, 0 fail, and the merge state is CLEAN.

  • Delta b267cc8..6928037: one commit. It contains only the collation fix in MessageStream.php, the new MySQL regression test and its fixture, the CHANGELOG wording, and a test comment pointing to Redrive doesn't move the failed run's pending inbound messages to the successor #1013.
  • Exact matching:
    • SQL only narrows the candidates. PHP then compares all six send fields exactly against the raw stored values and takes the offset-th match. Nulls match only nulls.
    • Offsets are counted per send identity, which is built from the same six values. Receive compares stream_key exactly.
    • Rows written by 2.20.0 and earlier still match byte for byte, because nothing trims, lowercases or casts these columns on write.
  • Counterfactual: on MySQL 8.4 the head passes 7/7, and with src at b267cc8 the new test fails (3 of 6 rows). On Postgres 17 both pass, because Postgres comparisons are case-sensitive.
  • Feature suites at the head on MySQL: 271/271, plus unit MessageStream 6/6.
  • CHANGELOG: matches the behavior. Redrive pending messages are tracked in Redrive doesn't move the failed run's pending inbound messages to the successor #1013.
  • Low, not blocking: matching now loads every row with the same content instead of using an SQL offset, so N identical sends in one run cost about N² rows per task. A possible follow-up is to cache each identity's candidate list in the ledger once per execution.

Earlier H1, M2, M3, L5 and the collation hold are all resolved. OK to merge this SHA and cut 2.20.1.

@rmcdaniel
rmcdaniel merged commit 95090b5 into main Nov 4, 2026
23 checks passed
@rmcdaniel

Copy link
Copy Markdown
Member Author

Merged on General interim clearance of exact head 6928037b1c7edd2130d5ab341e775d06c4f0f22e.

  • Merge commit: 95090b566aa8ba9295ea6c2dbe0b1788a44a3751. Its parents are prior main 7a758480 (2.20.0) and 6928037b.
  • The merge tree equals the cleared head's tree (27c34fda), so there's no file delta.
  • origin/main is 95090b56, and it is the only change since 2.20.0. Next: the 2.20.1 release PR.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Workflow-code outbox()->sendReference() writes new messages on every replay

1 participant