Deeper cancellation: flow the pipe token into each stage's job - #51
Conversation
The structured-concurrency between-stage cancellation is now hardened and covered by tests (PR #50), so reword the 'Deeper cancellation' planned item to show that progress while keeping the genuinely-remaining work explicit: interrupting in-flight work by flowing the token into each stage's job. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VsqPxHuJp8KckiU4FE5hHk
Structured-concurrency cancellation was only observed between stages — a stage already running was awaited to completion before the next checkpoint could throw. This adds cancellation-aware I and Let overloads that take a Func<..., CancellationToken, Task<...>> and flow the pipe's carried token into the running job, so work already in flight can be cancelled, not just observed between stages. - I overloads for value, Task and StructuredTask sources, plus the source-arg Let overloads (value / StructuredTask / deferred sources), take a trailing CancellationToken and pass the chain's token in. The StructuredTask-source paths share the carried token via a token-aware CheckedChain; the value/Task entry points own a fresh CancellationTokenSource. - A third token-aware Let stays a loud compile error (mirrors the token-free guard) so it cannot silently drop a deferred. - The Roslyn generator emits the same (..., CancellationToken) shape for every tuple arity (value / Task / StructuredTask tuple sources). - Tests cover success, in-flight interruption (jobs block on Task.Delay(Timeout.Infinite, ct) so they can only complete via the flowed-in token), token sharing across stages, the up-front cancelled-source check, owned-CTS disposal, token/token-free composition, and tuple sources. - README documents the new overloads; "Deeper cancellation" leaves Planned Features. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VsqPxHuJp8KckiU4FE5hHk
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9e59a7660
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…dex review) The generated cancellation-aware tuple I overloads used plain awaits, so a tuple job that ignored the token it was handed was not abandoned at its awaits: if the chain was cancelled before the source completed (or while the job ran), the job still ran and the chain could complete successfully despite Cancel(). The scalar token overloads gate every await with CheckedAwait; the tuple overloads now do the same — and the StructuredTask-tuple overload adds the up-front synchronous check — so cancellation is honoured between the source and the job, and after the job, even when the job ignores the token. - Make CheckedAwait internal so the generated TupleDestructuring class can reuse it. - Generator: gate each await in the three tuple token overloads with CheckedAwait; StructuredTask source also throws up-front on an already-cancelled source. - Tests: a token-ignoring tuple job still cancels (check after the source, before the job; trailing check after the job) and an already-cancelled StructuredTask tuple source throws at the call site. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VsqPxHuJp8KckiU4FE5hHk
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Check this! |
timonkrebs
left a comment
timonkrebs
left a comment
There was a problem hiding this comment.
Code Review — PR #51: Deeper cancellation
Overview
This PR correctly delivers the "deeper cancellation" feature: new I and Let overloads that take a Func<…, CancellationToken, Task<…>> and flow the pipe's own token into each running job. The design is sound, CheckedAwait routing is consistent for the new paths, and the test file is unusually thorough (mutation-verified interruption tests, disposal, token-sharing, token-free composition, and tuple sources).
CI is green on all four platforms. The core behaviour is correct. I found one pre-existing bug surfaced by this PR, one maintainability hazard introduced by this PR, and two missing tests. Details in the inline comments below.
Generated by Claude Code
timonkrebs
left a comment
timonkrebs
left a comment
There was a problem hiding this comment.
Code Review — PR #51: Deeper cancellation
Overview
This PR correctly delivers the "deeper cancellation" feature: new I and Let overloads that accept Func<…, CancellationToken, Task<…>> and flow the pipe's own token into each running job. The design is sound — ownership transfer via the source argument, CheckedAwait gating on every await, up-front synchronous cancellation checks, and correct CancellationTokenSource disposal semantics are all handled correctly. The test file is unusually thorough: mutation-verified interruption tests, disposal, token-sharing, token-free composition, and tuple sources.
CI is green on all four platforms.
Issues found
-
Pre-existing bug (FILE comment on
TupleDestructuringGenerator.cs) — The token-freeStructuredTask<({ty})>source tuple overloads skip cancellation entirely (plainawait s.ConfigureAwait(false)), while their scalar counterparts route throughCheckedChain/CheckedAwait. This PR's new cancellation-aware tuple overload correctly usesCheckedAwait, making the inconsistency visible. The token-free generated overloads should be fixed in the same way. -
Maintainability risk (inline on generator line 172) — The
StructuredTask<({ty})>cancellation-aware generated overload inlines theCheckedChainpattern instead of calling it. IfCheckedChainchanges, the generator must be updated in sync. -
Missing test (inline on
StructuredConcurrency.csline 70) — TheTask<TSource>sourceIoverload creates and owns a freshCancellationTokenSource, but there is no disposal test matchingI_ValueSource_TokenJob_OwnsAndDisposesCtsAfterAwait. -
Minor (inline on test file line 321) —
AwaitCanceledPromptly/Cancelare typed toStructuredTask<int>and should be made generic for future flexibility. -
Positive note (inline on
StructuredConcurrency.csline 75) — The ownership-transfer pattern (new StructuredTask<TResult>(task, source)) is applied correctly for theStructuredTask-source overload. ✅
Items 3 and 4 are low-risk; item 1 is the most important and pre-dates this PR but should be tracked.
Generated by Claude Code
Review item 1 (pre-existing bug): the token-free StructuredTask-tuple I overloads bypassed cancellation with plain awaits, so cancellation requested between the source completing and the projection running was silently ignored — unlike the scalar path, which routes through CheckedChain. The generated sync- and async-func StructuredTask-tuple overloads now mirror the scalar CheckedChain contract exactly: up-front synchronous check (already-cancelled source throws at the call site), source await gated by CheckedAwait, and a trailing check after the projection. New tests pin success, call-site throw, and cancellation-during-source skipping the projection for both overloads (mutation-verified against the plain-await regression). Review item 2 (maintainability): the generated StructuredTask-tuple overloads inline the CheckedChain pattern because the tuple func signature cannot be fed to CheckedChain without an adapter; emitted comments now say so and point at CheckedChain, and both CheckedChain doc comments point back at the generator so the two stay in sync. Review item 3: added I_TaskSource_TokenJob_OwnsAndDisposesCtsAfterAwait, mirroring the value-source disposal test for the Task-source token overload. Review item 4: the Cancel / AwaitCanceledPromptly test helpers are now generic over the chain's result type. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VsqPxHuJp8KckiU4FE5hHk
Summary
Structured-concurrency cancellation was only observed between stages — a stage already running was awaited to completion before the next checkpoint could throw. This implements the planned Deeper cancellation feature: cancellation-aware
IandLetoverloads that take aFunc<…, CancellationToken, Task<…>>and flow the pipe's carried token into the running job, so work already in flight can be cancelled, not just observed between stages.Changes
StructuredConcurrency.cs— cancellation-awareIoverloads (value /Task/StructuredTasksources) and source-argLetoverloads (value /StructuredTask/ deferred sources). TheStructuredTask-source paths share the carried token via a new token-awareCheckedChain; the value/Taskentry points own a freshCancellationTokenSource. A third token-awareLetstays a loud compile error (mirrors the existing token-free guard) so it can't silently drop a deferred.TupleDestructuringGenerator.cs— the Roslyn generator emits the same(…, CancellationToken)shape for every tuple arity (value /Task/StructuredTasktuple sources), so(a, b, ct) => …works too.StructuredConcurrencyCancellationTests.cs(new) — covers success, in-flight interruption, token sharing across stages, the up-front cancelled-source check, owned-CTS disposal, token/token-free composition, and tuple sources.README.md— documents the new overloads; "Deeper cancellation" leaves Planned Features.Design notes
CheckedAwait), so a job that ignores the token it was handed is nonetheless abandoned at its next await once cancellation is requested.CancellationTokenSource(external.Cancel()or a token propagated from an upstream source). Sibling-fault-triggered cancellation is intentionally not wired here —Awaitstill observes every deferred so all exceptions surface.StructuredTask-source chains share one CTS (ownership transfer); value/Taskentry points own a fresh CTS disposed with the resultingStructuredTask.Testing
All 196 tests pass in Debug and Release, zero warnings. The interruption tests block the job on
Task.Delay(Timeout.Infinite, ct)— an infinite delay can only complete via the flowed-in token, so a missing token would hang to the test's fail-fast guard rather than pass. This was validated with a mutation check (handing the job a dead token), which made the interruption test fail with its guard message.🤖 Generated with Claude Code
https://claude.ai/code/session_01VsqPxHuJp8KckiU4FE5hHk
Generated by Claude Code