Repository navigation
Conversation
The operator forwards a value to its subscriber whether or not the subscriber asked for one. Every existing test subscribes with `sink`, whose demand is unlimited, so none of them can observe that. This test subscribes with an explicit demand of one value and records what arrives. It fails today: the replay consumes the single demand, and the next value is forwarded regardless.
cmux traps on launch as soon as the sidebar starts observing a workspace:
Combine/Publisher+AsyncSequence.swift:112: Fatal error: Received an
output without requesting demand
`sidebarImmediateObservationPublisher` ends in coalesceLatest, and the
sidebar reads it through `.values`. An AsyncPublisher requests one value
at a time and traps on a value it never requested, while the operator
forwards every value the moment it arrives.
`receive(subscription:)` hands the subscription downstream and then
requests unlimited demand upstream. `@Published` replays current state
synchronously inside that request, when downstream demand is still zero,
so the replay traps on arrival.
Track downstream demand and forward only against it. A value with no
demand behind it becomes the coalesced pending value and is forwarded once
demand arrives, which is already what the operator does with a value that
lands inside an open window, and it holds at most one value either way.
Forwarding on demand does not re-stamp the window: the interval paces
upstream bursts, not the consumer's pull rate. `sink` requests unlimited
demand, so every existing subscriber behaves exactly as before.
The precondition was written down as "sink-style subscribers only" and
nothing enforced it, so moving the sidebar onto `.values` broke launch
rather than failing a test.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
cmux traps on launch as soon as the sidebar starts observing a workspace:
Three launches out of three here, dead within five seconds. The
cmux-unittesthost dies the same way before a single test runs, so the suite can't be run
either.
Root cause
sidebarImmediateObservationPublisherends incoalesceLatest, and the sidebarreads it through
.values. An AsyncPublisher requests one value at a time andtraps on any value it did not request, while
coalesceLatestforwards everyvalue the moment it arrives.
receive(subscription:)hands the subscription downstream and then requestsunlimited demand upstream.
@Publishedreplays current state synchronouslyinside that request, when downstream demand is still zero, so the replay traps on
arrival. The operator has forwarded without demand since #6807 added it; the
sidebar moved onto
.valuesin #8211, which is when it started mattering.Fix
Track downstream demand and forward only against it. A value with no demand
behind it becomes the coalesced pending value and is forwarded once demand
arrives, which is already what the operator does with a value that lands inside
an open window, and it holds at most one value either way. Forwarding on demand
does not re-stamp the window: the interval paces upstream bursts, not the
consumer's pull rate.
sinkrequests unlimited demand, so every existing subscriber behaves exactly asbefore, and the sixteen tests that were already here pass untouched.
Why nothing caught it
The precondition was a comment — "sink-style subscribers only", "downstream
demand is ignored" — and nothing enforced it. Every existing test subscribed with
sink, whose unlimited demand is the one subscriber shape that cannot observethe violation, so the operator's own tests were green by construction. Moving the
sidebar onto
.valuesbroke launch rather than failing a test. Honoring demandputs the contract in the code, where a future subscriber can't quietly void it.
Tests
The failing test lands first, then the fix.
coalesceLatestDeliversNoMoreValuesThanDownstreamDemandedsubscribes with an explicit demand of one value and records what arrives; against
the operator as it stands today:
coalesceLatestForwardsValueHeldForDemandOnceDemandArrivespins the other half,so a fix that simply drops the undemanded value doesn't pass either.
With the fix,
WorkspaceSidebarObservationTestsis 18/18 and the app launchesand stays up.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes a launch crash by honoring downstream demand in
coalesceLatest. This prevents “Received an output without requesting demand” when the sidebar consumes the stream via.values.coalesceLatest; forward only when there is demand. If none, hold the latest and deliver once demand arrives without re-stamping the window.sinkbehavior is unchanged (unlimited demand).Written for commit 16e2af5. Summary will update on new commits.