feat(js/net): solicit announcements lazily, per interested prefix - #2775
feat(js/net): solicit announcements lazily, per interested prefix#2775kixelated wants to merge 14 commits into
Conversation
…onnection per relay Squash of the four-step origin reshape; the full narrative is in PR #2705. - Origin.Producer/Consumer: a broadcast routing table independent of any connection, mirroring rs/moq-net's origin module. origin.publish(path) creates and returns the producer; Established.publish is removed. Sessions borrow the table via the publish option on connect/accept/Reload and announce it while they last; closing a session unannounces but closes nothing, and a reconnect re-announces the untouched table. - The subscribe option feeds the table with the peer's announcements as lazily-subscribing fronts scoped to the discovering session. Local and remote entries are separate maps and a session only announces the local one, so one origin on both directions cannot echo, and consume(path) resolves local first: loopback with no round trip. - origin.request(path) mirrors Rust's dynamic origin (#1772): any attached session answers blind, answers die with their session and are re-answered by the next, never announced. Reactive origin.discovery drives the gated fallback. announce.Broadcast gained an origin mode; js/watch and moq-boy consume through origins; watch's duplicated no-discovery machinery is gone. - Both wire publishers diff announce sets by front identity, so a republish emits ended-then-active (the restart form subscribers already handle). - Connection.Shared: a reactive handle on a pooled {origin, reconnect loop} keyed by relay URL, with a short linger past the last handle. The watch and publish elements and the demo pages share one connection per relay. Closes #2628. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review fixes, from the Codex pass and the PR comments. All four share a root cause: the origin machinery assumed one feeding session at a time, while its own API (and the coming GOAWAY drain, where old and new sessions overlap on purpose) allows several. - Remote entries keep every session's front per path, newest first: [0] is the route consumers resolve, and disposing it promotes the next, emitting the retract-then-announce restart so consumers re-consume onto the fallback. Previously a second session announcing the same path closed the first session's front, and its own death then black-holed a path a live session still carried (and would never re-announce). - Answering a request goes through origin.answer(), whose withdraw vacates the slot and pokes the requests table, waking standby serving loops so an already-attached session re-answers immediately. Previously only the slot's own signal changed, which reaches requesters but not servers, so a standing request went unanswered forever despite a live standby. A loser also stays eligible: answer() reports whether it took the slot, so a session that lost the race does not mark the path as its own. - Withdrawing the last request handle tears the slot down a microtask later. An effect whose rerun was triggered by the answer resolving closes its old request and takes a new one in the same tick; tearing down in between closed the answered front and re-dialed the subscription forever (the watch blind path flapped on every resolve). - Shared.announcedBroadcast ties its origin-mapping Computed to the returned handle instead of parking it on the connection-lifetime scope, which retained one per call until the connection closed. announce.Broadcast gained the closed promise the cleanup hangs off. - Documented the Shared constructor. Each fix carries the regression test the reviews asked for: two sessions on one path with the newer dying, a standby re-answering a dead answerer's request (wire-level, concurrent sessions), same-tick request re-acquisition, and the watch-level no-flap test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…out ownership Second review round, all three on the API contract: - An origin-mode Broadcast handle now follows the announcement table unconditionally and treats discovery only as the trigger for the blind request fallback. Previously it returned early while no session was attached (discovery undefined), so a local publish never resolved without a connection, and the no-discovery branch consumed the local route once, non-reactively, so a republish left the handle holding the superseded broadcast. The table is knowledge and the request is assumption, so the table wins when both resolve. - BroadcastProps is a union requiring exactly one of connection or origin: a call with neither (a permanently dead handle) or both (a silently ignored connection) no longer compiles. - Shared lends its origin as the new Origin.Table, the non-owning surface (publish, consume, closed). Producer implements it; the borrowing type cannot express close(), which would have torn the shared origin down under every other handle on the URL. Regression tests: loopback with no session attached including the republish swap and unpublish, local-route-wins on a no-discovery origin, and the BroadcastProps type error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…the shared session API revision from review discussion. - Request.active is now table-first: it resolves whatever the table routes (a local publish with no round trip, or any announced broadcast, swapping on a republish) and falls back to a session's blind answer only when nothing does. Resolution is derived per access, so a routed path resolves synchronously. That makes request() the single per-path consume primitive. - Origin.Consumer.consume(path) is gone from the public surface, renamed to an internal get(): a one-shot snapshot that neither waits nor follows a republish is a footgun next to a reactive handle, the same reasoning that removed the sync lookup on the Rust side. - Origin.Table grows the full borrowed surface (publish, request, announced, discovery, closed) and Producer implements it with passthroughs, so holding either side never needs the consume().x() stutter. - Shared no longer exposes the established session: it is shared, so no handle may close or reconfigure it, and everything else it offered is reachable through the origin. A transport getter covers the one legitimate read (labeling the negotiated transport); stats() stays as the session-aggregate snapshot pending per-broadcast estimates (#2709). - watch's Sync takes the probe estimates as its own input instead of reaching through a connection, matching Video.Source; the blind/gated resolution in watch collapses onto request(); the demo stats page consumes node broadcasts through the origin. Also filed #2708 (lazy announce interest, both languages) from the same discussion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three failures found in adversarial review of the origin reshape, each of which leaves a page permanently dark after a condition it should ride out. A pooled connection gave up for good after a 10s outage. `Shared` built its `Reload` with the default retry window, which is short on purpose: it assumes whoever built the loop observes `closed` and reacts. Nothing observes a pooled loop, so once the window expired every handle on that URL, and every handle taken later, was bound to a loop that had stopped. Pooled loops now retry without a deadline. An auth rejection is still terminal, and now also evicts the entry so the next handle dials fresh instead of joining a dead loop. A relay may refuse or reset the announce stream without closing the session, and the forwarder swallowed that: it retracted the session's entries and exited while the origin still counted the session as discovering. Every announcement-gated consumer then waited forever for a table nothing could fill. Discovery ending under a live session now downgrades the attachment to non-discovery (and logs the cause), so `origin.discovery` flips to false and gated consumers fall back to the standing requests the session still answers. `Origin.Table.get()` was a one-shot snapshot that races a republish, kept because `Announce.Broadcast` needed to resolve an announced path. An `@internal` tag does not strip it from the emitted declarations, so it shipped as a second, race-prone way to consume by path. It is gone: the announce-gated follower holds a request across the announced window and resolves through it, which is race-free because a session no longer answers a request the table already routes. That skip is a fix in its own right; without it the follower's request could resolve to a blind answer and defeat the announcement gate. `ReloadDelay` fields are now optional so a caller can set one knob (here, `timeout: 0`) without restating the backoff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A class publishing a small derived view of its own state had no good way to expose it. Computed carries an Effect (so it needs a close(), reads undefined until its first run, and propagates on a microtask), and a hand-written object with peek/subscribe/changed is rejected by getter() as a foreign readable, so it cannot be wired into a component input. Derived names its sources up front instead of tracking them, which buys a synchronous read and no teardown. It notifies only when the derived value actually changes, matching Signal: a source can move without moving the view. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`origin.discovery` and `request.active` were hand-built objects with the Getter methods and none of the package's brand, so `getter()` classified them as foreign readables and threw: a consumer could not wire either into a component input even though both type-check as Getter. They are Derived now, which also retires the tuple-overload workarounds they had grown. `Request`'s constructor was public in the emitted declarations (`@internal` does not strip without `stripInternal`), so a caller could forge a handle no origin ever registered and whose lifecycle guarantees were therefore false. It takes the module-local factory Consumer already uses. The Producer's reader is built once rather than per property access, so `discovery` keeps its identity across reads. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… field
`exactOptionalPropertyTypes` is off, so `{ initial: maybeInitial }` built from an
optional value passes an explicit undefined, and spreading it over the defaults
took that as the answer. An undefined `initial`, `multiplier`, or `max` turned
the backoff into NaN, which redials as fast as the event loop allows; an
undefined `timeout` became an infinite retry window.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Origins section still called `origin.consume(path)` and `origin.request` on a producer, neither of which survived the API revision that made Request the one way to consume by path. The watch and publish guides still built their Broadcast components with a `connection` input, which is now `origin`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The rebase onto dev pulled in tests written against `Established.publish` and `Publisher.publish`, which this branch removes. Git merged them without a conflict because the surrounding lines never moved, so they compiled as calls into a surface that no longer exists. They publish through an origin now, which is the same coverage: the subscribe still reaches the same producer, only by way of the table rather than the session. `Video.Source` also lost the `Moq` import in the merge, since dev's copy of the file no longer needed it and this branch's `probe` input does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Deduplicating against the value at subscribe time swallowed two real edges. A source applies a change synchronously and only queues the notification, so a Derived subscribing inside that window snapshotted the already-updated value and then suppressed the flush it was waiting for. Subscribing to the source directly delivered it, which is what the hand-written getters this class replaced did. An in-place `mutate()` force-notifies precisely because the object identity cannot change, so a mapping that returns the value as-is compared it against itself and dropped the notification. Both are lost wakeups. A redundant rerun is the cheaper failure, so the view relays what its sources report and leaves the filtering to them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A serving session tracked the paths it had answered, but a path outlives its slot: withdrawing the last handle tears the slot down a microtask later, and a request taken after that teardown installs a fresh one. Both writes land in a single coalesced wakeup, so the loop saw a slot it had never answered under a path it had, skipped it, and refused to withdraw the stale answer because the path was still occupied. The new request then never resolved. The claim is on the slot, not the path. A replaced slot now reads as withdrawn. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Each `announced()` parked a cleanup closure on the handle's own scope, which has no unregister path, so every closed pump was retained until the whole Shared handle went away. Effect.run hands back a disposer that also drops itself from the parent, which is what this repeated open/close pattern wants. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The forwarder opened a root announce stream the moment a session attached, so an app that only publishes, or only consumes known paths via requests, paid announce traffic and per-session announce state for every broadcast the relay could show it. The origin already knows who is listening. `Consumer.announced(prefix)` now registers refcounted demand on the origin, and the forwarder opens one wire stream per interested prefix instead of one root stream up front. A narrow listener (watching one room) asks for that room rather than the whole relay; a prefix already covered by an open stream costs nothing. Requests deliberately do not count as interest. A session answers them with blind subscriptions, which need no announcement, so a request-only app stays silent on the wire. That is the asymmetry with the Rust side, where a request has no blind answer to fall back on. Streams are sticky once opened: they carry the routes, so closing one retracts the table entries it fed and a consumer still holding that broadcast would see it go offline and come back as a blind answer. Closing on idle needs the table to know which entries are still being read, which it does not; the saving here is not asking in the first place. Overlapping streams (a narrow prefix, then a broader one) are deduped per path, so the table holds one front per path however many streams announced it. Without that, a retraction from one stream would swap the front the other still provides, which reads as a republish and makes consumers re-subscribe for nothing. Mirrors #2762 in rs/moq-net for #2708. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72580f442c
ℹ️ 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".
| existing?.dispose(); | ||
| const dispose = origin.insertRemote(path, conn.consume(path)); | ||
| if (existing) { | ||
| existing.dispose = dispose; | ||
| existing.count += 1; |
There was a problem hiding this comment.
Track overlapping announcements per stream
When a second overlapping prefix stream announces a path already supplied by the first, this disposes the current front and installs a new conn.consume(path) before incrementing the refcount. Because Origin.#runAnnounced identity-diffs fronts, listeners observe a retraction and re-announcement, causing Announce.Broadcast to close and re-subscribe despite the path remaining continuously available. The same global count also cannot distinguish a same-stream re-announcement from a new stream contribution, so a repeated active can leave a stale route after its single matching retraction. Track each pump's active paths and change the shared count only on per-stream membership transitions, without replacing the front for duplicate coverage.
Useful? React with 👍 / 👎.
ed0816f to
75ac33e
Compare
The JS half of #2708, mirroring #2762 in
rs/moq-net. Stacked on #2705 (targetsclaude/js-origin-extract, sinceforward.tsonly exists there); retarget todevonce that merges.Problem
forwardAnnouncedopened a root announce stream the moment a session attached, whether or not anything ever read an announcement. An app that only publishes, or only consumes known paths via requests, paid announce traffic and per-session announce state for every broadcast the relay could show it.Design
Consumer.announced(prefix)registers refcounted demand in a newinterestmap onOriginState, released when the stream closes.forwardAnnouncedwatches it and opens one wire stream per interested prefix, instead of one root stream up front.announcedBroadcast/Announce.Broadcastregister automatically, since both go throughannounced().Two deliberate differences from the Rust side:
request_broadcastthere has to raise interest and wait.announced(prefix)API already carries a prefix, so a watcher of one room asks forroom/abc, not the relay root. Rust solicits its auth-derivedallowed()prefixes instead.Sticky once opened, as in #2762: streams carry the routes, so closing one retracts the entries it fed and a consumer still holding that broadcast would watch it go offline and come back as a blind answer. Closing on idle needs the table to know which entries are still being read, which it does not.
Overlapping streams are deduped per path: a narrow prefix and a later broader one share one table entry, refcounted. Without that, a retraction from one stream would swap the front the other still provides, which reads as a republish and makes consumers re-subscribe for nothing.
Failure handling is unchanged: the first stream to end, cleanly or not, is discovery lost, which downgrades the attachment so gated watchers fall back to standing requests. With the usual single root stream that is exactly the old behavior.
Tests
New in
forward.test.ts: nothing is solicited until something listens (and a request alone does not count, but still resolves); the first listener opens exactly one stream and a second listener adds none; a narrow listener asks for its own prefix, a broader one opens its own stream, the shared path is one entry that survives until the last stream retracts it, and a nested prefix adds nothing.The fake session now records solicited prefixes and hands out one producer per prefix, which is what makes the laziness observable. Existing tests that asserted on a populated table now hold an explicit listener:
forward.test.ts(2),integration.test.ts(3),reload.test.ts(1). That edit is the behavior change made visible, not a workaround.just js testandjust checkare green.(written by Fable 5)