Repository navigation
SDKs: stop recommending reporting for a cancelled Join branch - #1598
Merged
Merged
Conversation
The Join doc comments in all five SDKs told the RPC that cancels a branch to report a result for it, to unblock an all-branches join or to make a cancelling join's round resettable. That is unsafe: Dex decides at commit time whether the cancellation takes effect, so the branch may already have reported, and the extra report counts it twice. State the rule instead: a canceled branch never reports and no other code can safely report for it. Cancel the join Step with its branches or bound the wait with a Timer, and never reset a cancelling join's round. Describe only the legitimate uses of ReportBranchResult. Rename the integration tests that exercise an RPC report so they no longer present it as a workaround, and add change log entries.
…reports The Join integration tests in all five SDKs still exercised an RPC that cancels a branch and reports a result for it, the pattern the Join docs now forbid. Replace that scenario with the documented safe workaround: an RPC cancels the join Step together with every branch and moves to a fallback Step. The tests assert that the canceled branch's release stays pending, that nothing consumes the reported result, and that the Flow completes through the fallback. The explicit-key RPC fork tests now fork a handoff branch that ends without reporting, and a later RPC call reports its result, which covers the legitimate RPC use of ReportBranchResult. The ReportBranchResult doc comments describe that condition: the result arrives through the RPC and no Step of that branch reports it.
The Python and TypeScript RPC fork tests hard-coded the value that the report RPC published. The RPC input now carries the result with the JoinRound, as in Go, Java, and Rust, and the tests assert that value.
longquanzheng
enabled auto-merge (squash)
November 11, 2026 05:20
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Corrects the Join helper's doc comments in all five SDKs: they no longer recommend that the RPC that cancels a branch report a result for it.
AnyOf(<join condition>, Timer)and handle the timeout in Execute. A cancelling join's round is never reset with the remaining-branches condition; the next round forks a new round.ReportBranchResultkeeps its legitimate uses (a multi-Step branch reporting from a Step other than its last; an RPC reporting for a round it forked with an explicit key) and says never to report for a cancelled branch.TestJoinRPCReportsABranchResult) with a comment explaining why they cannot double-count; their behavior is unchanged.## 1.45(unreleased)### Fixedin each SDK reference change log, and the SDKs'CHANGELOG.md. No version marker on the doc comments (a documentation correction, not an API or behavior change).Why
Review of the Join docs PR (#1594) found the SDK doc comments contradicting skills. The recommendation is unsafe: Dex decides at commit time whether a cancellation takes effect, so the cancelling RPC cannot know whether the branch already reported. If it did, the cancellation does nothing and the RPC's report counts that branch twice, so an all-branches join can fire while another branch has not reported.
doc-map: no change needed: SDK doc comments and reference change logs; skills already state the correct rule
Testing
make docsCheck,go test ./dex/, gofmt, integ package vet.scripts/check_public_docs.py, unit tests (706), ruff on changed files, integ collection../gradlew javadoc compileTestJava test.npm run docs:check, typecheck,npm test(221).make docs-check(doctests),cargo fmt --check, unit tests (141), integ tests compile.cd docs && npm run check.