Repository navigation
fix(mabr-route-proxy): resolve DS URLs against the DS root, scope the cache to the manifest - #29
Conversation
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (applied directly; no nested CLI in this runtime).
Reviewed head: cbe322e
Looks good. Splitting baseUrl (resolution) from scope (what the cache answers) fixes the live/live/... miss without widening the cache, and the derived path refuses a DS-root scope.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
- [native-codex]
packages/mabr-route-proxy/src/route.ts:957— the root-scope refusal only covers the derived path. With explicit options,baseUrl: <DS root>and noscopefalls back toscope = baseUrl's directory, which is the origin root, so multicast can answer any DS URL. That is exactly the setup the README now tells callers to use forbaseUrl. The README warns about it (README.md:40), but applying the samescope === new URL('/', scope).hrefrefusal in theMabrRouteProxyconstructor /rescopewould make the footgun fail loudly instead of silently. Caveat: that also changes behaviour for anyone who passes an origin-rootbaseUrltoday, so it may need its own PR. - [pr-review-toolkit:comments]
packages/mabr-route-proxy/src/route.ts:945— in derived mode anendpoint.manifestUrlthat is relative (only possible through a caller-suppliedslsEndpoint/endpoints; discovery always returns an absolute same-origin URL) resolves against the page, not the DS. A one-line doc note onRouteEndpoint.manifestUrlsaying it must be absolute would cover it.
Strengths
- Discovery uses
cache: 'no-store'and the proxy sendsno-store/reloadstraight to the network. That closes the path where objects injected at/slt/could steer the next join, and there's a test for it. stop()aborts discovery in flight, andthrowIfAborted()after the await covers the window where the response arrives just before stop. The test runs two concurrentstart()calls.rescopeis a non-exported symbol, so the publicMabrRouteProxysurface stays the same.- I ran
npm testlocally at this head: 51/51 pass.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (applied directly; no nested CLI in this runtime).
Reviewed head: ab77dbb
Looks good. The only change since the previous reviewed head is one test commit (compare reports 1 ahead, 0 behind): the SLS fixture now builds the DS /slt/7 document from multicast's models.SltType with a PresentationManifest, and the e2e gets a derived pass that gives the session no baseUrl, checks session.manifestUrl === origin + '/dash/dash.mpd', plays it through Shaka, and asserts no /dash/ request reaches HTTP. The library code is unchanged from the head I reviewed before. The two suggestions from that review still apply as written, and neither blocks merge.
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [pr-review-toolkit:tests]
packages/mabr-route-proxy/tests/route-wasm-shaka-e2e.mjs:397— the derived pass runs once (first Shaka variant,nativeingress). That covers the URL-derivation contract, which does not depend on ingress or Shaka build. If the AMT path ever starts touchingmanifestUrl, it would be cheap to run the derived pass inside the existingingressloop.
Strengths
- The
/slt/7fixture now goes through the sender's real model types instead of a hand-written XML string, so the derived pass checks the encoding the sender actually produces, not a guess at it. - The
requests.slice(derivedFrom)window keeps the new/dash/assertion from being satisfied or broken by the earlier passes, and the existing/mabr/escape assertion still runs over every pass. - The
openPagerefactor keeps the per-page error anddoneassertions identical for the old passes.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
… cache to the manifest The sender labels ROUTE objects with DS-root paths (live/seg-1.m4s). The README paired the DS manifest URL with baseUrl = the manifest's directory, so every object resolved to live/live/... and missed: playback stayed on unicast. - MabrRouteProxy gets a scope option separate from baseUrl: Content-Locations resolve against baseUrl, and only URLs under scope are cached. scope defaults to baseUrl's directory, so existing callers are unchanged. - With neither option set, RouteMabrSession.start() derives the base (DS origin root) and scope (manifest directory) from the discovered manifest and exposes session.manifestUrl, the URL to load. A manifest at the DS root is refused. Explicit options are used as given, and session.manifestUrl is the manifest path resolved against baseUrl. - Discovery reads only the Blockcast PresentationManifest, not SvcInetUrl, and no longer requires https. The same-origin check stays. - Drop the unpublished dsUrl alias; sltUrl takes a DS origin or /slt/ URL. - stop() aborts discovery in flight; discoverSlsEndpoint takes a signal. - Discovery fetches send cache: 'no-store', and the proxy sends no-store and reload requests to the network, so multicast-injected objects cannot answer /slt/ and steer the next join.
…haka The SLS fixture now emits the DS /slt/ document from multicast's SltType, encoded the way the sender encodes it, with a PresentationManifest. A new pass gives the session no baseUrl, loads session.manifestUrl and requires that no /dash/ request reaches HTTP.
no-store and reload requests go to the network without touching the hit/miss counters or onCacheEvent, as the README says.
ab77dbb to
7330834
Compare
|
@ally please review at head 7330834: rebased onto main f6370d1 after #30 (cache hit/miss telemetry). Conflicts resolved by keeping both: no-store/reload and non-GET requests bypass the cache before the hit/miss counters, so they reach neither |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (applied directly; no nested CLI in this runtime).
Reviewed head: 7330834
Looks good. This head is a rebase onto main @ f6370d1 (#30, cache hit/miss telemetry) plus one new test commit. I re-read the conflict resolution in proxy.ts: the non-GET / no-store / reload bypass at packages/mabr-route-proxy/src/proxy.ts:191-192 returns to the network before the miss counter and #report at :195-201, so bypassed requests touch neither stats nor onCacheEvent. That matches the updated onCacheEvent doc in proxy.ts. Commit 7330834 pins it: after one hit and two bypassed fetches, stats is {hits: 1, misses: 0}. The README keeps #30's telemetry example, now with sltUrl/serviceId instead of the removed baseUrl const. The DS-root base / manifest-directory scope logic is the same as the head I reviewed before.
Critical Issues (0)
Important Issues (0)
Suggestions (1)
- [pr-review-toolkit:comments]
packages/mabr-route-proxy/src/route.ts:196— the session-levelonCacheEventdoc still says it is "Called when a player request is served by ROUTE or the HTTP fallback". After this change,no-store/reloadrequests go to the network without an event, so a reader ofRouteMabrOptionsalone would expect events this hook no longer emits. Copying theproxy.tswording ("except no-store and reload") would keep the two in sync.
Strengths
- Bypass sits ahead of the counters, so DS discovery traffic (
cache: 'no-store') cannot inflate the miss rate that #30 exposes for fallback monitoring. - The new assertion runs against the same proxy right after a real hit, so it would catch a counter placed before the bypass check.
- The README conflict was resolved without leaving a reference to the removed
baseUrlconst.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (applied directly; no nested CLI in this runtime).
Reviewed head: 6fcdd61
Looks good. The only change since 7330834 is the one-line doc on RouteMabrOptions.onCacheEvent at packages/mabr-route-proxy/src/route.ts:196, which applies my previous suggestion. I checked it against the code. The bypass at packages/mabr-route-proxy/src/proxy.ts:191-192 sends non-GET/HEAD, no-store and reload requests to the network before either #report call at :197 and :201. That makes the new wording ("GET/HEAD served by ROUTE or the HTTP fallback; no-store and reload requests bypass the cache unreported") accurate, and it now matches the proxy-level doc at proxy.ts:68. There are no code changes, so nothing from the previous review needs re-reading.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The session-level and proxy-level docs now describe the same event contract, so a reader of either interface sees the bypass.
Recommended Action
- No blocking changes requested.
- Merge once the remaining required CI checks finish green.
Follow-up to #27. On main the README pairs the DS manifest URL with
baseUrl = new URL('.', manifestUrl). The sender labels ROUTE objects with DS-root paths (live/manifest.mpd,live/seg-1.m4s), so with that base every object resolves tolive/live/...and nothing is served from the ROUTE cache; playback silently stays on unicast. On main with the fake route.wasm object contract: base = manifest folder gives manifest MISS and segment MISS; base = DS root gives HIT and HIT.Changes
MabrRouteProxygets ascopeoption, separate frombaseUrl. Content-Locations resolve againstbaseUrl; only URLs underscopeare cached and answered.scopedefaults tobaseUrl's directory, so existing callers behave the same.RouteMabrSession.start()derives the base (DS origin root) and the scope (the manifest's directory) from the discovered manifest, and exposessession.manifestUrlas the URL to load. A manifest at the DS origin root makesstart()throw, because that scope would let multicast answer any DS URL. ExplicitbaseUrl/scopeare used as given, andsession.manifestUrlis then the manifest path resolved againstbaseUrl(for example a page mount).PresentationManifest(urn:blockcast:route:1), which is what the sender serializes;SvcInetUrlis no longer read. The https-only filter is dropped; the same-origin check stays.dsUrlalias is removed;sltUrltakes a DS origin or an/slt/<group>URL.stop()aborts discovery in flight, including concurrentstart()calls, anddiscoverSlsEndpointtakes asignal.cache: 'no-store', and the proxy sendsno-store/reloadrequests to the network, so objects injected over multicast at/slt/cannot steer the next join.sltUrlandsession.manifestUrl; explicit wiring discovers once and passesslsEndpoint,baseUrl(DS root) andscope(manifest directory).Release
The
mabr-route-proxy-v0.2.0tag is pushed, but its publish job failed (npm trusted publishing: package not found on the registry), so 0.2.0 is not on npm and nothing public depends ondsUrl.Tests
npm test: 51/51 (42 before). New: derived HIT with an out-of-scope object missing; root-scope refusal; explicit/mabr/mount; legacy default; abort during discovery with two concurrent starts;discoverSlsEndpointsignal; injected/slt/objects skipped on restart;SvcInetUrlignored; http DS manifest; proxy scope, rescope and the no-store/reload bypass. Each new test fails with its fix reverted.npm run test:e2e(real route.wasm, RaptorQ sidecar, Shaka debug and production builds, Chrome): all passes green, plus a new derived pass. The DS/slt/7document is now generated from multicast'sSltType, encoded the way the sender encodes it, with aPresentationManifest; the session gets nobaseUrl; Shaka playssession.manifestUrl(/dash/dash.mpd) and no/dash/request reaches HTTP. Control: the same pass with main's README pairing (baseUrl= manifest folder) fails, because the manifest request reaches HTTP.tsc --noEmitclean;.github/privacy-patterns.txtclean on the changed files.