Skip to content

Keep a stream's pending messages receivable whatever moves the run cursor - #1017

Merged
rmcdaniel merged 9 commits into
mainfrom
fix/1011-stream-cursor
Nov 4, 2026
Merged

rmcdaniel merged 9 commits into
mainfrom
fix/1011-stream-cursor

Conversation

@rmcdaniel

Copy link
Copy Markdown
Member

Fixes #1011.

Problem

MessageService::peekMessages, getUnconsumedCount and hasUnconsumedMessages returned only inbound messages with sequence > workflow_runs.message_cursor_position. That cursor is shared by signals, updates and every stream: signals and updates reserve their message_sequence from the same per-instance sequence and advance the cursor when applied. So a signal, an update, or a receive on another stream moved the cursor past older pending messages of a stream. Those messages stayed pending but were never returned by inbox()->peek() / receive() / pendingCount() / hasPending() again.

Fix

The three stream reads no longer filter by the run cursor. They return every pending inbound message of the stream for the run, in sequence order.

  • This is safe because only MessageService::sendMessage creates workflow_messages rows. Commands only reserve sequences, so a stream message's state lives entirely in consume_state.
  • The cursor itself is unchanged: advanceCursor is still monotonic, and the MessageCursorAdvanced history events, the continue-as-new transfer and the redrive transfer are untouched.
  • Replay is unaffected. Since 2.20.1, a receive in reused or replayed code returns the messages recorded as consumed at that step.

Tests (counterfactuals against main 6ce69f21)

  • testASignalDoesNotHideAnOlderPendingStreamMessage: m1 is sent before the ping signal that wakes the receive, and the workflow receives m1. On main it receives null.
  • testAReceiveOnOneStreamDoesNotHideAnotherStreamsOlderMessage:
    • Steps: b1 is sent on stream b, then a1 on stream a. Consuming a1 moves the cursor past b1.
    • Expected: peek('b'), getUnconsumedCount('b') and hasUnconsumedMessages('b') still see b1. On main they don't.

Local results

  • Feature suites pass 236/236: MessageStream*, MessageCursorContinueAsNew, Signal, LeasedSignal, Update, Query, ContinueAsNew*, ReplayCorpus and FailedRunRedrive.
  • Unit Message* tests pass 160/160.
  • ECS and PHPStan are clean.

Notes

  • After upgrading, workflow code can receive messages that earlier releases hid. They are the stream's oldest pending messages.
  • The Message Streams guide's "send the signal before the message" advice can be dropped once this ships. That's for the docs agent.
  • CHANGELOG.md overlaps with Move a failed run's pending messages to its redrive successor #1016's Unreleased entry; I'll resolve whichever merges second.

🤖 Generated with Claude Code

…rsor

A run's message cursor is shared by signals, updates and every stream.
Stream reads filtered on sequence > cursor, so a signal, an update or a
receive on another stream hid older pending messages for good (#1011).
peek, receive, pendingCount and hasPending now read every pending
message of the stream in sequence order; consume_state already records
what each stream consumed. The cursor and its history events are
unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
rmcdaniel and others added 2 commits November 3, 2026 21:36
# Conflicts:
#	CHANGELOG.md
#	tests/Feature/V2/V2MessageStreamReplayTest.php
The conflict resolution in 999865a dropped the closing brace of the
#1011 cross-stream test and the opening of the next docblock, so the
file didn't parse. Both sides' tests are kept; the file now runs 10/10.

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

Copy link
Copy Markdown
Member Author

General interim review (Security unavailable), exact head be836c266fda3532f37e07ef7bbd463111779f26: HOLD. The reviewer was independent of the author. CI is 16 pass, 7 skipped, CLEAN.

Verified

  • Stream reads now use only consume_state. That's sound, because every consume path marks the row as consumed.
  • No schema change, no migration, and no extra query cost: the MySQL 8.4 query plans are identical.
  • Both new tests fail against main's src on MySQL 8.4 and Postgres 17.
  • Merged locally with Move pending stream messages to the continued run at continue-as-new #1019, all related suites pass.

H1 (blocking): runs already affected by #1011 break on upgrade, and rollback doesn't fix them.

  • Which runs: a run recorded on 2.20.1 or earlier whose receive returned null, because the shared cursor hid m1.
  • What happens: after upgrade, that step replays. The 2.20.1 receive path (MessageStream.php:119-135) finds nothing recorded and peeks. With this change (MessageService.php:159), it then consumes m1 at the old step.
  • Reproduced on MySQL and Postgres:
    • If the workflow branches on the result, the run fails with Workflow history at workflow sequence 2 recorded [ActivityScheduled], but the current workflow yielded …. It gets stuck, and m1 is committed as consumed.
    • If it doesn't branch, it silently returns m1 at the old step.
  • Rollback: rolling back to 2.20.1 leaves the run broken.
  • Fix: never consume at a step that history has already passed. If nothing is recorded there, return empty. This also closes the documented 2.20.1 limitation. Add a simulated-upgrade regression test that covers both the branching and non-branching cases.

M2 (docs)

  • docs/api-stability.md:620-621 and docs/workflow-messages-architecture.md:220-222 still describe peek reading after the run cursor.
  • The CHANGELOG should say that a replayed receive that consumed nothing returns no recorded message.

rmcdaniel and others added 2 commits November 3, 2026 22:13
General's hold on be836c2 (H1): with the cursor filter gone, a run
whose earlier receive found nothing because the cursor hid a message
would, on replay after upgrading, peek and consume that message at the
old step. A branching workflow then failed with a history mismatch, and
a straight-line one silently changed its result.

A workflow-code receive with nothing recorded at its step now consumes
only when history hasn't passed the step the code is at (read live, so
a stream kept in a property still consumes at later steps). Otherwise
it returns empty. This also closes the 2.20.1 note that an empty
replayed receive reads the stream again.

M2: api-stability.md and workflow-messages-architecture.md no longer
describe stream reads as filtered by the run cursor.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The branching workflow from the #1011 review as a corpus fixture: turn
one finds the inbox empty, a message arrives, and turn two must replay
turn one's branch. On main the replay consumes the message at turn one's
step and the run stays waiting with a history mismatch.

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

Copy link
Copy Markdown
Member Author

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

  • H1: a workflow-code receive with nothing recorded at its step now consumes only when history hasn't passed the step the code is at. Otherwise it returns empty.
    • How it decides: the step comes from currentStep: fn (): int => $this->visibleSequence, read on every call, so a stream kept in a property still consumes at later steps. The comparison is with the highest step sequence in the run's history, computed once per execution in MessageReplayLedger::highestRecordedStep().
    • Also closes the 2.20.1 note that an empty replayed receive reads the stream again.
  • Tests:
    • testAReceiveThatFoundNothingReplaysAsNothingAndTheMessageWaitsForTheNextTurn (straight-line): turn 1 finds nothing, m1 arrives, and the query and turn 2 replay [null]. Turn 2 then receives m1, for output [null, 'm1']. On be836c2 the output is ['m1', null]: m1 is consumed at turn 1's step.
    • testABranchOnAnEmptyReceiveReplaysTheBranchTheRunTook (branching, new fixture TestInboxBranchWorkflow): completes with ['none:Hello, none!', 'got:m1']. On be836c2 the run stays waiting with a history mismatch.
    • testAKeptInboxStillReceivesAtLaterTurns (new fixture TestInboxKeptStreamWorkflow): a kept inbox receives m1, then m2.
    • Replay corpus message-stream-empty-receive-replays-empty.json plus its scenario. On main 4e9bc54 (2.20.1 + Move a failed run's pending messages to its redrive successor #1016) it also stays waiting, so the empty-receive replay was already a live bug for branching workflows on 2.20.1, not only after upgrading.
  • M2: docs/api-stability.md and docs/workflow-messages-architecture.md no longer describe stream reads as filtered by the run cursor. The CHANGELOG and execution-guarantees.md drop the "empty receive reads again" note, including in Redrive doesn't move the failed run's pending inbound messages to the successor #1013's entry.
  • Local results: related suites pass 250/250 (MessageStream*, MessageCursor, Signal, LeasedSignal, Update, Query, ContinueAsNew*, ReplayCorpus, FailedRunRedrive, GoldenHistory, Replayer). The corpus unit test passes 101/101, and php -l, ECS and PHPStan are clean.

@rmcdaniel

Copy link
Copy Markdown
Member Author

General interim review (Security unavailable): HOLD at exact head a27bcc3f8b6a97c09fbdf44fb072d273547db92b. The reviewer was independent of the author. CI: 16 pass, 7 skipped, CLEAN.

Verified

  • Counterfactuals:
    • With be836c2's src (H1 change reverted), the straight-line test gives ['m1', null], the branching test stays waiting, and the corpus test stays waiting.
    • On main 4e9bc54, the corpus fixture also stays waiting, which confirms the live 2.20.1 bug.
  • Full tests/Feature/V2 at head on MySQL 8.4: 2705 tests, 0 failures, 3 skipped.
  • No starvation of live receives. Checked: a fresh run's first receive; after a timer, a signal or condition wait with a timeout, and a sideEffect; in update handlers, including a second update at the same step; after a condition met by an update.
  • Redrive and continue-as-new: redrive's reused prefix is copied into the successor's history, so the guard applies. Continue-as-new starts at 0.
  • Local merge with Move pending stream messages to the continued run at continue-as-new #1019 (7309416): the conflicts are additive, in CHANGELOG and tests only. V2MessageStreamReplayTest 14/14, TaskBridge 266 pass, corpus 25/25. A message hidden behind the cursor on run 1 is received by the continued run at step 1. No semantic conflict.

H1 (blocking, residual): a kept stream still consumes at a step history has passed.

  • Cause: Workflow.php:193 fixes the stream's default consume step when it's opened, while :196 gives the guard the live step. So MessageStream.php:130 checks the live step, but :123/:137 record the consume at the step where the stream was opened.
  • Repro: with $this->chat ??= $this->inbox('chat'):
    1. Turn 1 (step 2) finds nothing, then m1 arrives.
    2. Turn 2 (step 4) consumes m1 with consumed_by_sequence=2.
    3. The next replay returns m1 at turn 1.
  • Result at this head:
    • Straight-line: ['m1', null].
    • Branching (3 turns): stuck in waiting with Workflow history at workflow sequence 2 recorded [ActivityScheduled, ActivityStarted, ActivityCompleted], but the current workflow yielded side effect, and m1 is consumed.
  • Not new: it also fails on main and be836c2. But the CHANGELOG and execution-guarantees.md:261-263 now promise the opposite. On upgrade, a 2.20.1 kept-stream run whose m1 was hidden by the cursor would consume it at the old step and get stuck.
  • Test gap: testAKeptInboxStillReceivesAtLaterTurns sends m1 before the first turn, so it can't catch this.
  • Fix, checked locally: when no explicit consumedBySequence is passed, record the consume at currentStep. If nothing is recorded there, fall back to the opened step, so older rows still replay. With that patch, both kept-stream probes and all 13 V2MessageStreamReplayTest tests pass. Add kept-stream "empty first turn" tests: a straight-line one, and a branching one with a third turn.

Low (non-blocking)

  • L1: MessageReplayLedger::highestRecordedStep() (105-116) loads every history payload of the run once per execution that receives. Reuse the history the executor already loaded, or select less.
  • L2: docs/workflow-messages-architecture.md:547/:638 still say receives read "after cursor".
  • L3: the is_int check (113) ignores a sequence stored as a string, for example from imported histories.
  • Outside this PR: a receive right after all([...]) returns null with a message pending. Main does the same. Please file it as its own issue.

General's re-review of a27bcc3 (residual H1): a stream kept in a
property recorded its consumption at the step it was opened at, while
the replay guard checked the live step. After an empty first turn, a
later turn's consumption was found again at the first turn's step on
replay, so straight-line runs returned it early and branching runs got
stuck.

Without an explicit step, a workflow-code receive now records and looks
up its consumption at the live step, falling back to the opened step so
rows earlier releases wrote still replay. Also: highestRecordedStep
reuses loaded history and accepts numeric-string steps, and two stale
'after cursor' lines in workflow-messages-architecture.md are fixed.

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

Copy link
Copy Markdown
Member Author

General interim review (Security unavailable): HOLD at exact head 9e3bf3697611c19454628474c6d31f5418764cfe (independent reviewer). CI is 16 pass, 7 skipped, CLEAN.

Resolved since a27bcc3

  • Kept stream (H1): consumption is now recorded at the live step.
    • Straight-line, empty first turn: [null, 'm1'].
    • Branching over 3 turns: completes.
    • Both new tests fail with a27bcc3's src.
  • Fallback: old rows at the opened step plus a live consume at turn 3 give ['m1','m2','m3']. m3 is recorded at step 6, nothing is consumed twice, and the query matches. With an empty turn 3 the result is ['m1','m2',null], and a later message stays pending. The fallback can't match a message from another live step.
  • L1, L2 and L3 are fixed.
  • Results:

N1 (blocking, regression since a27bcc3): an update handler can't receive once the run has passed its first step.

  • Cause: WorkflowExecutor.php:6539 applies an accepted update at the first step boundary of the replay. History records UpdateApplied(sequence=1) while the run waits at step 3, and the handler runs live at :6596 with visible step 2. The guard at MessageStream.php:146 sees 2 ≤ 3 and returns nothing.
  • Repro: main does await('a'), an activity, then await('done'). The update take calls inbox('jobs')->receiveOne() after a.
    • On main 4e9bc54: returns 'j1', output ['j1'].
    • At this head: returns null, j1 stays pending forever, output [null].
  • Fix: don't apply the past-step guard to a live update handler. For example, set a flag around the live call at :6596 to keep 2.20.1's behaviour. Add a regression test for an update that receives after the first step, and confirm its replay (the query and the next task) reads the consumed row back consistently.

rmcdaniel and others added 2 commits November 4, 2026 00:03
An update applied while the run waits at a later step runs its handler for
the first time at the update's recorded step. The past-step guard treated
that as a replay and returned nothing, so the message stayed pending.
Exempt live update handlers from the guard. Query replay of update handler
receives is tracked in #1024.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts:
#	CHANGELOG.md
#	tests/Feature/V2/V2EmbeddedReplayRegressionCorpusTest.php
#	tests/Feature/V2/V2MessageStreamReplayTest.php
@rmcdaniel

Copy link
Copy Markdown
Member Author

N1 fixed in 2c46077 (merged with main at 56d535e).

  • Cause: a newly applied update handler runs at the update's recorded step (step 1 in the repro), even when the run waits at a later step. The past-step guard treated that as a replay and returned nothing, so j1 stayed pending forever.
  • Fix: WorkflowExecutor::applyPendingUpdates runs the handler through Workflow::runLiveUpdateHandler(), which sets a flag that MessageStream reads through a closure. Live update handlers skip the guard. Replays of the handler, whether in the workflow task or in a query, still go through the guard.
  • Test: testAnUpdateAppliedAfterLaterStepsStillReceivesAndReplaysWhatItTook. The workflow runs await('go'), then an activity, then await('done'); an update take receives one message.
    • Two updates take j1 and then j2.
    • A query changes no rows.
    • After done, the output replays as ['j1', 'j2'] with no rows changed.
    • Without the executor change, the test fails with null instead of j1. Main passes it.
  • Found while testing: on main (2.20.1) too, a query replays recorded updates before it syncs the cursor, so an update handler's receive returns nothing inside a query. Filed as Query replay of an update handler's stream receive returns nothing #1024. It isn't caused by Keep a stream's pending messages receivable whatever moves the run cursor #1017 and isn't changed here. The test asserts only that the query writes nothing.
  • Local checks: ECS, PHPStan (src + tests), and the stream, corpus, update and task-bridge feature tests (340) pass. The regression-corpus validator passes against 74aa0e6.
  • Docs: CHANGELOG and execution-guarantees.md now say that a newly applied update handler still receives.

@sonarqubecloud

sonarqubecloud Bot commented Nov 4, 2026

Copy link
Copy Markdown

@rmcdaniel

Copy link
Copy Markdown
Member Author

2.20.3 release scope and acceptance plan, while the independent delta review holds #1017 at 6bc303d3c4d739109c2b34c44e2c0c317d1eabbe:

  • Merge only the cleared SHA with terminal checks, and verify the merge tree equals the reviewed tree. Prepare a two-file release PR (CHANGELOG.md, composer.json) from that main commit, with product-train exactly 2.20.3. Security reviews the release head before an explicit tag go.
  • Notes cover Signals and other streams advance a shared message cursor and hide pending stream messages #1011, empty-receive replay, kept streams, first-time update-handler receives, and known limitation Query replay of an update handler's stream receive returns nothing #1024. No schema, history event or public API changes. The rollback note must account for kept-stream receives now recording at the current step: older releases look up the opening step and may replay those runs differently.
  • Before tagging: run the repository quality cycle and verify the platform contract from committed source. After publication: verify tag/merge identity, latest GitHub Release, Packagist dist/source identity, Release Verification (published package plus Laravel upgrade matrix), and focused stream/replay/update regression tests using the installed exact published package. Source-only results will be reported separately.
  • Downstream inspection: Sample App main allows ^2.20.1, but its lock and qualified tuple are still at 2.20.1; its owner has Add default inbox #310 for the 2.20.2 follow-through. Waterline main has Workflow 2.5.12 only in require-dev and no runtime Workflow constraint; Fix webhookUrl() to account for route prefixes #272 owns its pending 2.20.x dev/qualification update. The docs stable manifest is already 2.20.2. Ask these owners to advance to 2.20.3 only after publication and qualification.
  • Server main's PHP reference app remains explicitly locked to Workflow 2.15.12. Its native Dockerfile builds only Rust, so this PHP-package change does not require a native Server or service SDK release. Keep any PHP-reference adoption separate and explicit. PHP SDK has no Workflow runtime dependency; Python/Rust service behavior is unchanged, so unrelated portable SDK matrices are outside this patch's acceptance.

#1024, #1021, #1022 and #1020 remain parked under the handoff's priority order. No tag or release action has been taken.

@rmcdaniel

rmcdaniel commented Nov 4, 2026 •

Copy link
Copy Markdown
Member Author

Independent General source review: MERGE GO at exact head 6bc303d3c4d739109c2b34c44e2c0c317d1eabbe. This clears the source hold; it is not tag or publication clearance.

  • Re-read the complete source delta, including consumption at the current step, legacy opening-step fallback, highest-history-step handling and the live-update exception. Recorded update replay remains read-only; the live flag is restored in finally.
  • Verified the test container's tracked PHP source, PHP tests and docs against this Git tree. The configured test environment differs; four Rust fixture files affected by a case-insensitive-path collision were absent and are outside this embedded PHP V2 qualification.
  • Full tests/Feature/V2 on PHP 8.4.27 / MySQL 8.4: 2,712 tests, 27,525 assertions, zero failures/errors, 3 skips, exit 0. Unit replay corpus: 102 tests / 3,672 assertions, pass.
  • Focused live-update test passes with the current source (1 test / 8 assertions). In an isolated copy of the same source, reverting only the runLiveUpdateHandler wrapper in applyPendingUpdates makes it fail with null instead of 'j1' (1 failure / 3 assertions). This confirms N1 and the regression test's sensitivity.
  • Fresh exact-head GitHub readback: 16 successful checks, 7 intended skips, CLEAN. Sonar's only open finding is php:S107 on the eight-argument internal factory. Accepted for this patch: the two optional closures supply live workflow state while preserving existing callers; an interface refactor would add unrelated replay risk. No security hotspot or new behavioral defect was identified from that finding.

Known query limitation #1024 remains separate. The 2.20.3 release PR must retain the explicit kept-stream downgrade warning: an older package looks up the opening step and can replay newly recorded later-step consumptions differently. Merge only the cleared SHA, verify the resulting tree, then prepare the scoped release PR for Security's exact release-head review and the planned installed-package acceptance. No tag go is granted here.

Cleanup complete: the three task containers, their anonymous volumes, the task network, the review clone and its three owned worktrees are removed. Counterfactual edits were verified as exact recoverable GitHub blobs before deletion. Shared resources and other worktrees were preserved.

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

Copy link
Copy Markdown
Member Author

Merged #1017 through GitHub with --match-head-commit 6bc303d3c4d739109c2b34c44e2c0c317d1eabbe, following the independent clearance above.

  • Merge commit: a806a83ad5dff56adb9e166fa0d9a4be1d9db130.
  • Cleared head tree = merge tree: 81b242d2fe14aa4bd25a2ee869a540888dffb32e.
  • git diff --exit-code 6bc303d3 a806a83a is empty.

The source fix is merged but remains unreleased. Preparing the scoped 2.20.3 release draft for Security, including the kept-stream downgrade warning and known limitation #1024. No tag or publication clearance has been granted.

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.

Signals and other streams advance a shared message cursor and hide pending stream messages

1 participant