docs(net): say who owns the broadcast behind Announce.Broadcast.active - #2640
Conversation
`active` hands out the same `Broadcast.Consumer` type that `consume()` returns, so nothing stops a caller from closing it. Doing so leaves `active` pointing at a dead consumer and every later read fails, while the path is still announced and a fresh consume() works. No in-tree caller does this, and the repo already has an idiom for the question (refcounted handles, clone() for an independent lifetime), so say which side of it `active` is on rather than reshaping the type. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 9 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughExpanded the 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@js/net/src/announced.ts`:
- Around line 192-195: Update the documentation for the handle’s borrowed
consumer and republish behavior to state that redundant announcements may retain
the existing live consumer, replacing it only when the consumer closes or the
path goes offline. Qualify the close guidance to note that
broadcast.Consumer.close() is reference-counted, so cloned consumers may keep
the shared state usable; do not imply every republish changes active identity or
every close causes reads to fail.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c266b150-aaf0-4685-bb93-36ee317912dd
📒 Files selected for processing (1)
js/net/src/announced.ts
Consumers are reference-counted, so closing the one read from `active` only kills the shared broadcast when it was the last reference; with a clone outstanding, reads through the closed handle still work. Say that rather than promising reads fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Summary
Follow-up to #2617, from a review that landed after it merged.
Announce.Broadcast.activehands out the sameBroadcast.Consumertype thatconsume()returns, so nothing stops a caller from closing it. If they do, the handle doesn't notice:activekeeps pointing at the closed consumer and every later read fails, while the path is still announced and a freshconn.consume(path)works.No in-tree caller does this (
js/watchmirrorsactiveand subscribes off it without closing), so this is an API-misuse hazard rather than a live bug.Why a doc change and not a type change
Two stronger options were suggested and both have a real downside:
close()from the exposed type. It is the sameBroadcast.Consumerthatconsume()returns, and callers legitimatelyclone()it, so a narrowed view complicates the type for everyone to stop one misuse.The repo already has an idiom for the underlying question (refcounted handles,
clone()when you want an independent lifetime, closing a borrowed handle is a holder bug). So this says which side of that idiomactiveis on, and points atclone().Happy to take the stronger version instead if the view-type cost is acceptable.
Public API changes
None. Doc comment only.
Test plan
bun run --filter='@moq/net' checkandjust js fixclean. No behavior change, so no test; a test here would only codify the misuse.(written by Opus 5)