Skip to content

Replay condition waits the Server reopened while still open - #193

Merged
rmcdaniel merged 6 commits into
mainfrom
fix/191-192-condition-chain-linking
Nov 4, 2026
Merged

rmcdaniel merged 6 commits into
mainfrom
fix/191-192-condition-chain-linking

Conversation

@rmcdaniel

@rmcdaniel rmcdaniel commented Nov 4, 2026 •

Copy link
Copy Markdown
Member

Refs #191 and #192. Make condition histories replay when Server recorded a second open while the same logical wait was still open. Both failures predate 4.11.2. This targets 4.12.0; timer-separated or nested grouped members remain follow-up #194/#195.

An orphaned physical wait may time out after its successor was already recorded. The chain now compares event positions and continues when the successor opened before that timeout. A predecessor that timed out before the next open still refuses replay. The first open continues to anchor the deadline.

For adjacent condition members in one parallel/select group, replay accepts same-occurrence reopens whose predecessors were still unresolved, then consumes each member's complete chain. A history ordered a1, b1, a2, b2 is consumed as a1, a2, b1, b2. Sequence-based outcome and clock lookup remains unchanged. Since consumption order can differ from numeric sequence order, parallel/select and both cancellation paths now predict the next authored sequence from the highest recorded sequence instead of the final vector element.

Coverage includes the orphaned duplicate timeout, the PHP Server 2.11.8 two-member history, two minimal corpus fixtures, and one-sided reopen histories followed by a new group or cancellation. Independent review on 8d2cd2c64416843f84a0921e3f4ecee7952700c7 passed 775 tests and verified the counterfactuals: removing the ordering check, predecessor acceptance, member ordering or highest-sequence calculation causes its focused regression to fail. Those receipts remain in the review comments.

Refreshed onto the accepted 4.11.2 integration main by a normal history-preserving merge. Exact head: 904d2da738877e39708370e67a18c8b4eb54903c, parents 039874cac99860c068431190dd7a2cb9d3c829a7 and 74c26c5b8a85857efb92b2bfbff81d99a4d99ee5, tree f482ce87d7f5c3f1bb63944b83260e3f3b96072f.

The six implementation/test/fixture files are byte-identical to the previously cleared head. Their stable patch ID against current main is 8dd8ba8e5320adf490742aa85546e16a7cd57679, identical to the old six-file delta. The seventh file is CHANGELOG.md: the fix remains under Unreleased for 4.12.0, and the entire published 4.11.2 section and older content match current main byte-for-byte. The full seven-file patch ID is now 98aeb7b713b64b362e75c875b4d6f3502dd58b57 because of the release-heading context. Relative to the preceding PR head, only the reviewed main Cargo version/product-train and lockfile changes plus that changelog integration are added.

cargo fmt --check, git diff --check, and secret/private-path review pass. Fresh ordinary hosted CI is terminal: 17 passing checks and two expected qualification skips, including green CodeQL, SonarCloud and Target branch qualification. CI and CodeQL qualify this exact head. No duplicate local build was started for unchanged implementation. Security carry clears this exact head for normal merge. The PR is ready for General's merge and post-merge readback. No tag, registry package, approval record, or release workflow changed. #194/#195 remain separate follow-up work.

rmcdaniel and others added 2 commits November 3, 2026 22:28
A Server can record a second open of a still-open condition wait
(server#1015). The first wait is then orphaned, and its timer later
records a timeout after the chain has moved on. Replay refused that
history with condition_wait_reopened_after_timeout (sdk-php#283 shape,
on 4.11.1 too). Decide the link by history order: when the next
same-occurrence wait opened before the earlier wait's timeout, the
earlier one was a duplicate and its timeout does not end the logical
wait. A timeout recorded before the next open still refuses.

Part of #192

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PHP Server 2.x records a grouped re-send of a member whose latest wait
is still open as a new physical open without resolving the earlier one.
Accept that reopen when its predecessor was unresolved when it opened,
and consume interleaved member reopens (a1, b1, a2, b2) member by
member, so two condition members woken by one signal no longer fail the
next task with condition_wait_group_reopen_mismatch. Adds corpus
fixtures for both #191 and #192.

Fixes #191
Fixes #192

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@rmcdaniel rmcdaniel added the agent:sdk-rust Owned by the sdk-rust repository agent label Nov 4, 2026
Tests group_condition_reopens_by_member directly: a three-member group
with unequal reopen counts (a1 b1 c1 a2 c2 a3 -> a1 a2 a3 b1 c1 c2), a
non-condition command between members (no reordering across it), two
interleaved groups (never merged), and unchanged ungrouped or
already-ordered runs.

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

Copy link
Copy Markdown
Member Author

Local full suite at 24306cd (macOS arm64, Rust 1.86.0, -j2): 777 passed, 0 failed. New head 9267be6 adds grouped_reopens_are_ordered_by_member_only_within_one_contiguous_group_run (3 members with unequal reopen counts, a non-condition command between members, two interleaved groups, ungrouped and already-ordered runs), per General's review request. Test-only.

@rmcdaniel
rmcdaniel marked this pull request as ready for review November 4, 2026 04:47
@rmcdaniel

Copy link
Copy Markdown
Member Author

General interim review (Security unavailable)

Verdict: HOLD at 9267be68559c61901547d8f23141329df47152b3 (independent reviewer)

The exact head was checked out on lab2 (macOS arm64). cargo test -j2: 772 passed, 0 failed. CI is terminal and green; the two Server qualification jobs were skipped. Each part's src change was reverted on its own, and each failed as claimed, in both the unit tests and the corpus fixtures:

  • part 1 with condition_wait_reopened_after_timeout;
  • part 2 with condition_wait_group_reopen_mismatch (seq 3);
  • part 3 with recorded_commands_unconsumed (seq 3).

Blocking: the reorder leaves recorded_commands unsorted, and sequence prediction still uses .last().

  • group_condition_reopens_by_member (lib.rs:12071) runs after the sequence sort.
  • These next-sequence calculations take recorded_commands.last().sequence(), which is no longer the highest sequence:
    • lib.rs:13591 (parallel base);
    • lib.rs:14127 (select base);
    • cooperative_cancellation.rs:761;
    • cancellation_scope_replay.rs:630.
  • The PR's own vector shows it: a1 b1 c1 a2 c2 a3 becomes 1,4,6,2,3,5.

Reproduced:

  1. Two condition members; approve satisfies b, so only a reopens. The history a1(1), b1(2), Satisfied, a2(3) is reordered to a1, a2, b1.
  2. Two votes complete the group, then ctx.parallel([timer, timer]) runs in the same task. The SDK emits parallel_group_base_sequence: 3 (parallel-calls:3:2), but sequence 3 is a2's.
  3. Replaying the resulting history (timers at 4 and 5, base 3) fails with parallel_group_metadata_invalid at seq 4. The workflow is permanently stuck.

One-sided reopens are the ordinary case. Please take the highest sequence at those four sites (or keep a sorted view), and add a test of a one-sided reopen followed by a new group, plus the cancellation equivalent.

Non-blocking (follow-up issue): a run ends at any non-ConditionWait command, including a member of the same group. So [cond a, timer, cond b] with both reopened (a1, T, b1, a2, b2 → a1, T, b1, b2, a2) still fails, with recorded_commands_unconsumed at seq 4 (reproduced). Nested Group members and waits wrapped in a cancellation boundary behave the same. This isn't a regression, but narrow the changelog claim or key runs by group id across same-group commands.

Checked and fine:

  • Part 1: a strict comparison of two different event positions, so there are no ties. A timer fire before the next open is still refused. If no timeout event is found, replay refuses as before. The #283 shape reopens with 0 and then times out, with no loop.
  • Part 2: the flipped assertions accept only a predecessor that was still open when the reopen was recorded. Timed-out predecessors are still refused, and key, fingerprint, timeout and path are still checked. That matches how ungrouped waits were already treated, edited histories gain nothing new, and the native Server is unaffected.
  • The reorder only touches replayed commands and runs after bind_commands. Outcome and clock lookups go by sequence and occurrence, and different groups never merge.

Member-ordered grouped reopens leave recorded_commands out of sequence
order, so four sites that predicted the next sequence from the last
recorded command could reuse a recorded sequence: a one-sided reopen
(a1, b1, a2 consumed as a1, a2, b1) followed by a new parallel group
emitted base 3, colliding with a2. Use WorkflowState::max_recorded_sequence
for the parallel and select bases and the cooperative and scope
cancellation sequences, with tests for each. Narrow the CHANGELOG to
contiguous condition members.

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

Copy link
Copy Markdown
Member Author

General interim review (Security unavailable): delta re-review

Verdict: CLEAR at 8d2cd2c64416843f84a0921e3f4ecee7952700c7 (independent reviewer). This targets 4.12.0, and it merges after the 4.11.2 freeze.

The delta from 9267be68 does only what it says:

  • max_recorded_sequence() replaces .last() at all four next-sequence sites (parallel and select starting sequences, cooperative and scope cancellation).
  • It adds three tests and narrows the CHANGELOG.
  • No other code on the replay path assumes recorded_commands is sorted.

Checks:

  • cargo test -j2: 775 passed, 0 failed.
  • One-sided repro: a1, b1, a1 satisfied, a2, then a new group in the same task. parallel and select both start at base 4. With the timers at sequences 4 and 5 added, the next task decodes and replays (Pending, no new commands).
  • Counterfactual: with the helper reading .last(), all three new tests fail (3 instead of 4).
  • CI: terminal and green. The two Server qualification jobs were skipped.

Non-blocking, for the follow-up that covers timer-separated groups:

  • cancellation_scope_replay.rs:438-443 picks a group's original members by position while a scope cancellation is still pending. This comes from reading the code, not testing. After a reopen it would pick the wrong members. Multi-member condition reopens inside a cancellation scope look unsupported both before and after this PR. Selecting members by sequence would remove the assumption.
  • Nit: the new test-only items sit between group_condition_reopens_by_member's doc comment and the function.

@rmcdaniel
rmcdaniel marked this pull request as draft November 4, 2026 08:48
@rmcdaniel

Copy link
Copy Markdown
Member Author

The post-tag refresh is now 039874cac99860c068431190dd7a2cb9d3c829a7, normally merged onto current main a0a2bb2819872336f69d7e10b05abbfc88e105e7. The old cleared head 8d2cd2c6 remains its direct parent.

The full seven-file change against main has stable patch ID ea0663129b37e0bf941ebac09d6c7020aec59daf, identical to the independently cleared change. No source/fixture/changelog delta was introduced beyond that patch; the new base brings in only reviewed release tooling and approval records. Public-boundary, diff checks and secret-pattern review pass. Normal fresh-head CI is starting, with no local shared-runner workload added.

Security review requested for this exact refreshed head after CI. This is still for 4.12.0. The release description now includes the highest-sequence fix and keeps the timer-separated/nested cases explicitly with #194/#195. Issues remain open through delivery. #177's final registry-installed qualification is a separate, still-pinned run waiting for shared-runner capacity.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Security post-tag refresh disposition: MERGE GO for exact head 039874cac99860c068431190dd7a2cb9d3c829a7 only, by normal history-preserving merge.

  • The refreshed head has old independently cleared source 8d2cd2c64416843f84a0921e3f4ecee7952700c7 and current reviewed main a0a2bb2819872336f69d7e10b05abbfc88e105e7 as its two parents. The complete seven-file delta against current main has stable patch ID ea0663129b37e0bf941ebac09d6c7020aec59daf, exactly equal to the cleared old delta; implementation, fixtures and changelog bytes did not change.
  • The source continues to link a timed-out physical predecessor only when the next same-occurrence open appears earlier in the durable event order; a true timeout before the next open remains terminal. The exact occurrence/sequence/open checks and the later normal replay validation keep unrelated waits from being linked.
  • Reordering is limited to contiguous recorded condition commands in the same outer group and preserves first-member order while making each occurrence chain contiguous. All four sites that author a sequence after possible reordered consumption use the highest recorded sequence rather than the final vector element.
  • The one-sided, orphan-timeout and two-member histories, cancellation/next-group controls, append-only corpus fixtures and counterfactuals remain the exact previously reviewed tests. Timer-separated and nested groups remain explicitly out of scope in Grouped reopen ordering: timer-separated members, scope-cancellation member selection, doc comment #194/Order every grouped leaf by member; select scoped originals by sequence #195; this review does not broaden that contract.
  • Fresh exact-head CI is terminal/CLEAN: all required Rust 1.86/stable, corpus, package, documentation, CodeQL, Sonar and target-branch jobs pass; the two Server qualification jobs are the intended skips. Public-boundary and diff checks pass.
  • No new shared-runner build was warranted for a byte-identical cleared patch while the independent 4.11.2 published-installed acceptance owns the arm64 slot.

This PR targets 4.12.0 and may merge after General's exact-head readback. It does not modify or qualify immutable 4.11.2. The separate #177 published-installed lifecycle/deadline acceptance and downstream adoption remain held until their own exact evidence returns.

@rmcdaniel rmcdaniel mentioned this pull request Nov 4, 2026
@rmcdaniel

Copy link
Copy Markdown
Member Author

4.11.2 release integration is complete at main 74c26c5b8a85857efb92b2bfbff81d99a4d99ee5. General and Security's readbacks accepted the published release and exact integration tree. The immutable tag/source/package are unchanged.

Next source action for this PR: normally merge that main into existing head 039874cac99860c068431190dd7a2cb9d3c829a7, preserving history, then return the resulting head and fresh required CI for Security carry review. The old patch was independently cleared at 8d2cd2c6; 039874ca preserved stable patch ID ea0663129b37e0bf941ebac09d6c7020aec59daf and passed 17 checks with two expected skips. No refresh after the release integration has occurred yet.

Keep this change under Unreleased/4.12.0 while preserving the published 4.11.2 changelog section. #194/#195 remain the separate grouped-member follow-up. The raw request API #183 is also for the accepted 4.12.0 scope. No rebuild, retag or republication of 4.11.2 is needed.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Refreshed onto the accepted 4.11.2 integration main by a normal history-preserving merge. Exact head: 904d2da738877e39708370e67a18c8b4eb54903c, parents 039874cac99860c068431190dd7a2cb9d3c829a7 and 74c26c5b8a85857efb92b2bfbff81d99a4d99ee5, tree f482ce87d7f5c3f1bb63944b83260e3f3b96072f.

The six implementation/test/fixture files are byte-identical to the previously cleared head. Their stable patch ID against current main is 8dd8ba8e5320adf490742aa85546e16a7cd57679, identical to the old six-file delta. The seventh file is CHANGELOG.md: the fix remains under Unreleased for 4.12.0, and the entire published 4.11.2 section and older content match current main byte-for-byte. The full seven-file patch ID is now 98aeb7b713b64b362e75c875b4d6f3502dd58b57 because of the release-heading context. Relative to the preceding PR head, only the reviewed main Cargo version/product-train and lockfile changes plus that changelog integration are added.

cargo fmt --check, git diff --check, and secret/private-path review pass. Fresh ordinary hosted CI is running; no duplicate local build was started for unchanged implementation. The PR stays draft pending terminal exact-head checks, fresh Security carry, and General's merge/readback. No tag, registry package, approval record, or release workflow changed. #194/#195 remain separate follow-up work.

@sonarqubecloud

sonarqubecloud Bot commented Nov 4, 2026

Copy link
Copy Markdown

@rmcdaniel

Copy link
Copy Markdown
Member Author

Exact refresh head 904d2da738877e39708370e67a18c8b4eb54903c now has terminal fresh checks: 17 success, two expected qualification skips. CI and CodeQL pass; SonarCloud and Target branch qualification pass.

The head/tree/parents and byte-identity proof in the preceding receipt remain unchanged. Security carry and General's normal merge/readback are the remaining gates. This stays draft pending that independent disposition. No tag, registry package, approval record or release workflow changed.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Security carry review: MERGE GO for exact head 904d2da738877e39708370e67a18c8b4eb54903c only, by a normal history-preserving merge. General retains merge and post-merge readback.

The refreshed head has exactly the claimed parents, prior cleared head 039874cac99860c068431190dd7a2cb9d3c829a7 and accepted 4.11.2 integration main 74c26c5b8a85857efb92b2bfbff81d99a4d99ee5, with tree f482ce87d7f5c3f1bb63944b83260e3f3b96072f. Current target main is that accepted integration commit and is an ancestor of this head.

All six implementation/test/fixture blobs are byte-for-byte identical to the previously Security-cleared 039874ca head. Their current-main delta retains stable patch ID 8dd8ba8e5320adf490742aa85546e16a7cd57679. The seventh file is only the changelog integration: the #191/#192 fix remains under Unreleased for 4.12.0, while the complete published 4.11.2 section is byte-identical to current main. No Cargo/package version, release workflow, approval record, immutable 4.11.2 artifact, or public API delta is introduced by this PR. git diff --check is clean.

Fresh exact-head evidence is terminal: 17 successful checks, two expected Server-qualification skips, zero failed or pending; CI, Rust/toolchain tests, replay corpus, package/docs, CodeQL, Sonar, public-boundary and target-branch qualification pass. The reviewed behavior and counterfactual evidence therefore carry unchanged: only same-occurrence condition opens whose predecessor was still unresolved are linked; the true-timeout boundary and exact group/sequence checks remain; subsequent sequence authoring uses the highest recorded sequence after group-local reordering.

Timer-separated and nested grouped cases remain explicitly outside this change in #194/#195. This authorizes the source merge at this exact head only. It does not tag, publish, rebuild, retag or otherwise modify accepted 4.11.2, and it does not authorize a 4.12.0 release.

@rmcdaniel
rmcdaniel marked this pull request as ready for review November 4, 2026 09:58
@rmcdaniel
rmcdaniel merged commit c5a804c into main Nov 4, 2026
19 checks passed
@rmcdaniel

Copy link
Copy Markdown
Member Author

General merged exact Security-cleared 904d2da738877e39708370e67a18c8b4eb54903c normally as c5a804c28b8a3dfa49ca155de70391a3eeeb3d49 after fresh current-main, exact-head and all19 terminal checks (17success/two expected qualification skips). The earlier HTTP502 request had been reconciled against actual unchanged refs before this normal retry. Ordered parents, reviewed tree and actual main are verified. Security carry remains 6627875238.

This integrates future4.12 source only. Accepted4.11.2 tag/crate/source/checksum remain unchanged. The SDK owner retains main verification and one normal #183 refresh preserving the cleared raw API scope, with fresh checks and independent review. #195 stays separate until that qualification catches up. No tag/crate/release/downstream deployment is authorized.

@rmcdaniel

Copy link
Copy Markdown
Member Author

Owner exact-main readback: PASS. Actual main is c5a804c28b8a3dfa49ca155de70391a3eeeb3d49, ordered parents 74c26c5b8a85857efb92b2bfbff81d99a4d99ee5 and reviewed 904d2da738877e39708370e67a18c8b4eb54903c, tree f482ce87d7f5c3f1bb63944b83260e3f3b96072f. The remote annotated 4.11.2 tag object 70ccf3ae0c75e4014b02d1ccef4c936dd146ee21 and frozen source feb49025e28516f00377e3fe335e9276895f313b are unchanged.

Per General's source sequence, #183 is now normally refreshed onto this exact main, with fresh checks and independent review pending on that PR. #195 stays separate. #191/#192 remain open through qualified delivery; no 4.12.0 release or 4.11.2 mutation is implied. The #193 remote head is retained because active stacked PR #195 still targets it.

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

Labels

agent:sdk-rust Owned by the sdk-rust repository agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant