Skip to content

[codex] Fix large fan-in replay joins - #18

Closed
mwotton wants to merge 2 commits into
microsoft:mainfrom
lambdamechanic:large-fanin-join-replay
Closed

[codex] Fix large fan-in replay joins#18
mwotton wants to merge 2 commits into
microsoft:mainfrom
lambdamechanic:large-fanin-join-replay

Conversation

@mwotton

@mwotton mwotton commented May 7, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add a 1024-way completed sub-orchestration fan-in replay regression test that verifies each joined result.
  • Store schedule-to-token bindings so replay completion delivery is O(1) instead of scanning every token binding.
  • Replace the replay no-op waker with a private wake state, and register durable-future token wakers so replayed completions can drive wake-based progress.

Background

This was distilled from a spin loop bug in the wild. Parent bulk orchestrations had very large fan-in, all child SubOrchestrationCompleted events were already persisted, and no queue work remained, but replay still failed to produce a terminal parent event.

Root Cause

There were two related issues.

First, replay polled orchestration futures with a no-op waker. For large inputs, futures::future::join_all uses wake-driven readiness internally, so simply polling the parent future again is not enough: completed durable children need to wake the task. Without those wakes, a fully replayed large fan-in can return Continue instead of reaching Completed.

Second, replay completion delivery only had token-to-schedule bindings, so each completed schedule searched all bound tokens. With large fan-in, that made history replay O(n^2) just to deliver completions.

Validation

  • ./run-tests.sh
  • cargo fmt --check
  • git diff --check upstream/main...HEAD

@mwotton

mwotton commented May 7, 2026

Copy link
Copy Markdown
Contributor Author

@microsoft-github-policy-service agree

@mwotton

mwotton commented May 7, 2026

Copy link
Copy Markdown
Contributor Author

the real meat here is just the first commit which is the test that shows the bug - second commit is my attempt at fixing it, but it's entirely possible it's non-idiomatic or has problems I haven't seen.

@mwotton
mwotton force-pushed the large-fanin-join-replay branch from 564828f to 7f69487 Compare May 7, 2026 01:41
@mwotton
mwotton marked this pull request as ready for review May 7, 2026 01:41
return err;
}
if wake_state.is_woken() {
must_poll = true;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems that the purpose of must_poll is to avoid unnecessary extra polling, based on the assumption that only application of history events can wake up pending futures.

If that assumption is incorrect, then one naive fix would be to remove must_poll field entirely, and just always poll after applying every history event, and after applying all history. Redundant polling is always safe, as long as a completed future is not polled.

The risk is that CPU usage of repeatedly polling non-trivial futures could be wasteful.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

After digging into the implementation more, I observe that the root issue is OrchestrationContext::join is using futures::future::join_all which uses a non-deterministic scheduling strategy for >30 futures.

I provide an alternative fix here, which is to only use deterministic scheduling for OrchestrationContext combinators:

#19

@affandar

affandar commented May 8, 2026

Copy link
Copy Markdown
Contributor

@mwotton thank you for catching, reporting and proposing a fix!

@qbradley had a simpler approach to solving the root cause which I took, but your test case was merged as part of it.

If you want you can send the replay optimization as a separate PR and I'll be happy to review that.

@affandar affandar closed this May 8, 2026
@mwotton

mwotton commented May 14, 2026

Copy link
Copy Markdown
Contributor Author

fantastic! thanks for the merge - i'll see how it works on my fork without the optimisation and see if performance is still a problem. (I think I only saw it because I'm using a turso provider ported from the sqlite provider, but it's a macro-driven monstrosity right now, need a bit more thought before I PR that.)

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.

3 participants