feat(web): land the Service Worker byte pipe and app-shell precache - #951
Conversation
|
Warning Review limit reached
Next review available in: 36 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
WalkthroughThis PR adds verified ranged content reads, capped HTTP handling, client ranged-download transport, a Service Worker media pipe, app-shell precaching, and web-provider integration with browser coverage. ChangesRanged content and transport
Media Service Worker flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MediaService
participant ServiceWorker
participant MediaPipe
participant MediaBroker
participant MediaReader
MediaService->>ServiceWorker: Register worker and transfer MessagePort
ServiceWorker->>MediaPipe: Route stream request
MediaPipe->>MediaBroker: Open requested range
MediaBroker->>MediaReader: downloadRange(node, offset, length)
MediaReader-->>MediaBroker: Plaintext chunk
MediaBroker-->>MediaPipe: Headers and chunks
MediaPipe-->>ServiceWorker: Stream response
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/providers/EngineProvider.tsx (1)
62-72: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winOrder the media disposal before the client disposal.
media?.dispose()is not awaited, soclient.dispose()runs while the broker port can still be open. The broker reads throughclient(createMediaService(client)at Line 57), so an in-flight Service Worker read can resolve against a disposed client and fail.Chain the client disposal after the media disposal settles. Note that
MediaService.dispose()awaits its startup promise, so this also delays client teardown until registration settles; confirm that trade-off is acceptable.♻️ Proposed teardown ordering
- media?.dispose().catch((error: unknown) => { - console.error('[media] dispose failed', error instanceof Error ? error.message : error); - }); - client.dispose().catch((error: unknown) => { - console.error('[engine] dispose failed', error instanceof Error ? error.message : error); - }); + // The broker reads through the client, so the pipe must close first. + void Promise.resolve(media?.dispose()) + .catch((error: unknown) => { + console.error('[media] dispose failed', error instanceof Error ? error.message : error); + }) + .then(() => + client.dispose().catch((error: unknown) => { + console.error('[engine] dispose failed', error instanceof Error ? error.message : error); + }) + );🤖 Prompt for 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. In `@apps/web/src/providers/EngineProvider.tsx` around lines 62 - 72, Update the cleanup function returned by the EngineProvider effect so client disposal is chained after media disposal settles. Preserve the existing error logging for both operations, including the optional-media case, and ensure MediaService.dispose() completes before invoking client.dispose() so the broker cannot access a disposed client.
🧹 Nitpick comments (13)
crates/wasm/src/host.rs (1)
380-387: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winWipe the Rust-side plaintext copy after the
Uint8Arraycopy.
bytesholds decrypted file content.Uint8Array::fromcopies it into the JS heap, so this function is the terminal owner of the Rust-side buffer. The buffer is then dropped unwiped. The write direction already does this:push_chunkwraps its plaintext inZeroizing(Line 293).The same gap exists in
download(Line 350). Fix both in one change if you take this.🔒️ Proposed change
let bytes = engine .read() .await .read_content_range(node, offset as u64, length as u64) .await .map_err(engine_error)?; - Ok(Uint8Array::from(bytes.as_slice()).into()) + // Terminal owner of this copy: the bytes cross into the JS heap here. + let bytes = Zeroizing::new(bytes); + Ok(Uint8Array::from(bytes.as_slice()).into())As per coding guidelines: "Zeroize sensitive material after use at the terminal owner only; callees must not zero caller-owned buffers."
🤖 Prompt for 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. In `@crates/wasm/src/host.rs` around lines 380 - 387, Update the read-content flow containing read_content_range and the download flow to wrap each decrypted Rust-side byte buffer in Zeroizing before converting it with Uint8Array::from, ensuring the plaintext is wiped when the terminal owner releases it. Preserve the existing JavaScript copy and return behavior, and follow the existing push_chunk pattern without zeroizing caller-owned buffers.Source: Coding guidelines
packages/client/src/seams/http.test.ts (1)
70-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe cap bounds the accumulated total, not each chunk.
sendCappedcheckstotal + value.byteLength > maxBytesonly afterreader.read()has already materializedvalue. One chunk larger thanmaxBytesis therefore held in memory before rejection, so the true peak bound ismaxBytes + one chunk. The test gap and the doc wording both follow from this.
packages/client/src/seams/http.test.ts#L70-L91: add a case where a single chunk exceeds the cap, for exampleproducedBody(4096, 4)against a cap of 1000, and assertobserved === 4096. Every current oversize case uses 100-byte chunks against a 1000-byte cap, so thetotal === 0path never runs.crates/engine/src/seams/http.rs#L144-L150: soften "never materializes an over-cap body" to state that the JS seam aborts the drain once the accumulated body passes the cap, bounded by one chunk of overshoot.🤖 Prompt for 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. In `@packages/client/src/seams/http.test.ts` around lines 70 - 91, Update packages/client/src/seams/http.test.ts lines 70-91 by adding a sendCapped case using a single chunk larger than the limit (for example, producedBody(4096, 4) with a 1000-byte cap), asserting observed equals 4096 and that the body is cancelled. Update crates/engine/src/seams/http.rs lines 144-150 to replace the claim that an over-cap body is never materialized with wording that the JS seam aborts once accumulated data exceeds the cap, with overshoot bounded by one chunk.crates/wasm/src/seams_bridge.rs (1)
621-626: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCover omitted numeric fields in
tooLargeresults.
required_u64returns0for missing fields orNaN, socapped_countdoes not panic, but the currenttooLargecase always includeobservedandlimit. Add atooLargeresult without those fields so the Rust edge case and CI boundary cover it.🤖 Prompt for 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. In `@crates/wasm/src/seams_bridge.rs` around lines 621 - 626, Add a `tooLarge` result case to the relevant bridge test or fixture with both `observed` and `limit` omitted, then assert it is handled successfully with the existing zero-value behavior from `capped_count` and `required_u64`. Keep the existing populated `tooLarge` coverage unchanged.packages/client/test/browser/media.ts (2)
153-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReset
portRequestsalongsidereaderCalls.
cbMediaDisposeresetsreaderCallsbut leavesportRequestsat its last value. The counters serve the same purpose, so the asymmetry can mislead a later test that callsdisposeand then readsportRequests.♻️ Proposed counter reset
readerCalls = 0; + portRequests = 0;🤖 Prompt for 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. In `@packages/client/test/browser/media.ts` around lines 153 - 161, Update cbMediaDispose to reset portRequests alongside readerCalls during teardown, ensuring both test counters return to their initial state after disposal.
93-96: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace the fixed 50 ms sleep with polling on the settled role.
cbMediaEngineresolves after a single 50 ms timeout. The spec then asserts exact'leader'and'follower'roles. On a loaded CI machine the Web Locks election may not have settled in 50 ms, andcurrentRole()returns'none'. That makes the merge-blocking follower test flaky.Poll until the role leaves
'none', with a bounded deadline.♻️ Proposed polling seam
- // Let the lock election settle so the caller's role assertion is meaningful. - return new Promise<string>((resolve) => - setTimeout(() => resolve(client?.currentRole() ?? 'none'), 50) - ); + // The caller asserts an exact role, so wait for the lock election to settle. + return (async (): Promise<string> => { + for (let attempt = 0; attempt < 100; attempt += 1) { + const role = client?.currentRole() ?? 'none'; + if (role !== 'none') return role; + await new Promise((resolve) => setTimeout(resolve, 50)); + } + return 'none'; + })();🤖 Prompt for 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. In `@packages/client/test/browser/media.ts` around lines 93 - 96, Replace the fixed timeout in cbMediaEngine with bounded polling of client?.currentRole(), resolving as soon as the role is no longer 'none'. Add a deadline so polling cannot hang indefinitely, while preserving the existing role result and fallback behavior when the deadline is reached.apps/web/vite.config.ts (1)
21-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe manifest hardcodes the
/base.
/${fileName}assumesbaseis/. If the app is later deployed under a sub-path, every precached URL points at the wrong origin path and the offline app shell breaks. Read the resolved base inconfigResolvedand prefix with it.🤖 Prompt for 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. In `@apps/web/vite.config.ts` around lines 21 - 24, Update the bundle URL construction in the Vite configuration to use the resolved deployment base instead of hardcoding “/”. Capture the resolved base through the plugin’s configResolved hook and prefix each fileName in the shell mapping with that base, preserving the existing source-map filtering and sorting.packages/client/test/browser/media.spec.ts (1)
182-198: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGive the two tabs distinct fixture bytes to make the isolation assertion meaningful.
Both tabs use
localTab, so both synthesize fromTAB_SEED. The body assertions at Lines 196 and 198 therefore prove only that each response matches the shared fixture. A cross-tab registry mix-up is caught only by the 404 that a foreign ticket produces, not by the byte comparison. Parameterize the seed per tab so wrong-tab bytes also fail.🤖 Prompt for 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. In `@packages/client/test/browser/media.spec.ts` around lines 182 - 198, Update the two open-tab test around localTab so each tab is initialized with a distinct fixture seed. Use the corresponding seed when computing fromA and fromB expected bodies, preserving the existing ticket sizes and status assertions so cross-tab responses also fail on mismatched bytes.packages/client/src/media/service.test.ts (1)
309-315: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExpress the browser API compatibility check with a type assertion.
This test only checks
null === null; the real structural check happens at the cast. UseexpectTypeOf<ServiceWorkerContainer>().toExtend<ServiceWorkerContainerLike>()andexpectTypeOf<MessagePort>().toExtend<MessagePortLike>()so intent is explicit and unsafe casts are not needed.♻️ Proposed change
- it('accepts the real browser container and port types structurally', () => { - const container: ServiceWorkerContainerLike | null = null as unknown as ServiceWorkerContainer; - const port: MessagePortLike | null = null as unknown as MessagePort; - - expect(container).toBeNull(); - expect(port).toBeNull(); - }); + it('accepts the real browser container and port types structurally', () => { + expectTypeOf<ServiceWorkerContainer>().toExtend<ServiceWorkerContainerLike>(); + expectTypeOf<MessagePort>().toExtend<MessagePortLike>(); + });Add
expectTypeOfto thevitestimport.🤖 Prompt for 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. In `@packages/client/src/media/service.test.ts` around lines 309 - 315, Update the test case “accepts the real browser container and port types structurally” to use Vitest’s expectTypeOf assertions, verifying ServiceWorkerContainer extends ServiceWorkerContainerLike and MessagePort extends MessagePortLike. Add expectTypeOf to the vitest import and remove the null assignments, unsafe casts, and null expectations.Source: Path instructions
packages/client/src/sw/install.ts (1)
88-94: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHandle rejection in the
activatechain.The
installhandler on line 85 ends with.catch(ignore). This chain does not. Ifcaches.keys(),clients.claim(), orreadPrecachedUrlsrejects, the worker raises an unhandled rejection and the cache mirror stays stale with no signal. Terminate the chain the same way.♻️ Proposed change
scope.addEventListener('activate', (event) => { event.waitUntil( deleteStaleCaches(scope.caches) .then(() => scope.clients.claim()) .then(refresh) + .catch(ignore) ); });🤖 Prompt for 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. In `@packages/client/src/sw/install.ts` around lines 88 - 94, Update the activate event chain in the scope.addEventListener('activate', ...) handler to terminate with the same .catch(ignore) handling used by the install handler. Keep the existing deleteStaleCaches, clients.claim, and refresh sequence unchanged.packages/client/src/sw/precache.ts (1)
97-105: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winA hanging network stalls every navigation before the shell fallback.
The navigation path is network-first with no timeout.
fetchFnonly rejects on a hard failure. On a captive portal or a stalled connection the request stays pending until the browser default timeout, so the cached shell is not served and the UI does not boot. Consider racing the network against a short timer and falling back to the cached document.🤖 Prompt for 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. In `@packages/client/src/sw/precache.ts` around lines 97 - 105, Update the navigation branch around fetchFn so the network request races against a short timeout and rejects when the timer wins, allowing the existing cached APP_SHELL_DOCUMENT fallback to run; preserve successful network responses and rethrow the original error when no cached shell exists.packages/client/src/sw/precache.test.ts (2)
65-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test covers a manifest entry that fails to cache.
FakeCache.addAllintestDoubles.tsalways succeeds, so every case here exercises the happy path ofcache.addAll. The realCache.addAllis atomic and rejects the whole call when one entry responds non-OK. That path changes what stays cached on an update and is currently unverified. See the comment onpackages/client/src/sw/precache.tslines 47-53 for the underlying behavior.🤖 Prompt for 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. In `@packages/client/src/sw/precache.test.ts` around lines 65 - 75, Add a precache test covering an update manifest where one resource fails during Cache.addAll, using a test double that rejects atomically like the real Cache API. Assert the expected retained cache contents after the failed update, alongside the existing precacheAppShell coverage.
22-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the manifest request itself.
manifestFetchintestDoubles.tsignores its arguments, so no test proves thatreadManifestrequests/precache-manifest.jsonresolved against the origin, or that it passescache: 'no-store'. A change that dropsno-storewould let a stale manifest pin an old shell, and every test here would still pass. Capture the request in one case.🧪 Example case
+ it('requests the manifest uncached and same-origin', async () => { + const calls: Array<[string, RequestInit | undefined]> = []; + const recording = (async (input: string, init?: RequestInit) => { + calls.push([input, init]); + return new Response('["/index.html"]'); + }) as unknown as typeof fetch; + + await precacheAppShell(new FakeCacheStorage(), recording, ORIGIN); + + expect(calls[0][0]).toBe(`${ORIGIN}/precache-manifest.json`); + expect(calls[0][1]?.cache).toBe('no-store'); + });🤖 Prompt for 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. In `@packages/client/src/sw/precache.test.ts` around lines 22 - 49, Update the precacheAppShell tests to capture the Request passed to the manifest fetch, then assert that readManifest requests /precache-manifest.json resolved against ORIGIN with cache set to no-store. Adjust the manifestFetch test double as needed to expose the captured request while preserving the existing cache behavior assertions.packages/client/src/media/range.test.ts (1)
1-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the range arithmetic itself.
The suite covers
content-typeand hardening headers only. The returnedwindowandcontent-rangevalues are never asserted, and those drive the bytes the broker streams (packages/client/src/media/broker.tslines 75-112). Add cases for the suffix form (bytes=-100), the open interval (bytes=100-), alastvalue past the end (bytes=4000-99999), the malformed and multi-range fall-through to 200, and the unsafe-integer digit run that must fail closed.🧪 Example additional cases
+describe('resolveMediaRequest window', () => { + it('resolves a suffix range against the tail', () => { + const head = resolveMediaRequest('bytes=-100', SIZE, 'video/mp4'); + expect(head).toMatchObject({ status: 206, window: { offset: SIZE - 100, length: 100 } }); + expect(headerMap(head).get('content-range')).toBe(`bytes ${SIZE - 100}-${SIZE - 1}/${SIZE}`); + }); + + it('clamps a last-byte position past the end', () => { + const head = resolveMediaRequest('bytes=4000-99999', SIZE, 'video/mp4'); + expect(head).toMatchObject({ status: 206, window: { offset: 4000, length: SIZE - 4000 } }); + }); + + it('answers a malformed or multi-range spec with the whole file', () => { + expect(resolveMediaRequest('bytes=abc', SIZE, 'video/mp4').status).toBe(200); + expect(resolveMediaRequest('bytes=0-9,20-29', SIZE, 'video/mp4').status).toBe(200); + }); + + it('fails closed on a digit run past the safe-integer range', () => { + expect(resolveMediaRequest('bytes=99999999999999999999-', SIZE, 'video/mp4').status).toBe(416); + }); +});🤖 Prompt for 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. In `@packages/client/src/media/range.test.ts` around lines 1 - 47, Expand the resolveMediaRequest tests to assert both returned window values and content-range headers, covering suffix ranges (bytes=-100), open-ended ranges (bytes=100-), end values beyond SIZE (bytes=4000-99999), malformed and multi-range inputs falling back to 200, and unsafe-integer digit runs failing closed. Reuse headerMap and the existing SIZE fixture while verifying the exact safe byte window and response behavior.
🤖 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 `@apps/web/src/providers/EngineProvider.tsx`:
- Around line 96-99: Add runtime tests in EngineProvider.test.tsx covering
useMediaService(): assert it returns the provider’s media service when available
and null when no Service Worker exists. Exercise the media context field through
the provider setup while preserving the existing lifecycle and teardown
coverage.
In `@apps/web/vite.config.ts`:
- Around line 50-53: Update the Rollup output configuration in vite.config.ts to
remove the unsupported output.codeSplitting option and set output.format to
'iife' with output.inlineDynamicImports enabled. Keep SW_FILE as the entry
filename so the generated classic service worker remains /sw.js.
In `@crates/engine/tests/write_plane.rs`:
- Around line 843-848: Extend the range-read assertions in the write-plane test
around read_content_range to add a case whose offset is strictly greater than
whole.len(). Verify that this past-EOF request returns an empty result without
error, covering the manifest.size.saturating_sub(offset) path while preserving
the existing offset-equal-to-length case.
In `@packages/client/src/broadcast.ts`:
- Around line 78-79: Update the response documentation comment near the snapshot
and plaintext read descriptions to replace the incomplete phrase with “a
plaintext read carries a Blob,” preserving the surrounding wording.
In `@packages/client/src/media/broker.test.ts`:
- Around line 1-9: Add a named CI job or PR gate that explicitly runs the client
unit-test command for the new media and service-worker suites, using the
`@cipherbox/client` test filter or an equivalent dedicated client-unit command.
Keep the existing browser/Playwright coverage and ensure this gate is included
in the required CI workflow.
In `@packages/client/src/media/service.ts`:
- Around line 128-139: Scope MEDIA_PORT_REQUEST broadcasts to the client that
actually needs a port instead of triggering rebroker() in every tab. Extend
MediaPortRequest with the target client identifier, pass the known clientId
through acquirePort, and post the request only to that client. Keep
onMessage/rebroker unchanged so unrelated tabs retain their active brokers and
streams.
In `@packages/client/src/seams/types.ts`:
- Around line 92-94: Format the CappedHttpResult type declaration with the
repository’s Prettier configuration, specifically normalizing the multiline
union formatting around the response and tooLarge variants. Do not change the
type’s semantics.
In `@packages/client/src/sw/install.test.ts`:
- Around line 42-47: Apply the repository’s Prettier formatting to the union
type annotation in the ServiceWorker event dispatch method, preserving the
existing dispatch behavior and type cast.
In `@packages/client/src/sw/install.ts`:
- Around line 74-109: Track the in-flight cache reload initiated by refresh()
and reassign that promise whenever refresh runs during startup, install, or
activate. In the fetch listener, preserve navigation handling, but for
same-origin GET requests whose app-shell claim is unknown, await the learning
promise before evaluating appShellClaims and responding; add or reuse
sameOriginGet to limit this wait to eligible requests. Export the helper from
precache.ts if it is implemented there.
In `@packages/client/src/sw/pipe.ts`:
- Around line 161-173: Track each pending pull timer by request ID in the
body/pullWindow flow, and clear that timer during body.cancel before deleting
the sink or posting cb:media:close, so cancellation settles without invoking
discardPort or closing the port. In packages/client/src/sw/pipe.ts lines 161-173
update the implementation; in packages/client/src/sw/pipe.test.ts lines 176-185
add coverage that cancels a body using stalledPort(), advances beyond
pullTimeoutMs, and verifies the port remains open.
- Around line 276-285: Update sealed to copy header pairs into merged
defensively, ignoring any pair whose name or value causes Headers validation to
throw, while preserving valid headers and the forced cache-control: no-store
setting. Keep the existing response status/body behavior and do not add sniffing
or CSP headers here.
In `@packages/client/src/sw/precache.ts`:
- Around line 47-53: Update precacheAppShell in
packages/client/src/sw/precache.ts#L47-L53 to cache manifest URLs individually,
tolerate a rejected addAll for one URL, and build the prune set only from URLs
that cached successfully. In packages/client/src/sw/precache.test.ts#L65-L75,
add coverage using a FakeCache.addAll that rejects for a selected URL, asserting
other manifest entries cache and unlisted entries are pruned.
In `@packages/client/test/browser/media.spec.ts`:
- Around line 213-216: Guard the CDP-based service-worker cleanup in the test
containing the `context.newCDPSession` call so it runs only for Chromium; skip
the test or bypass this block for Firefox and WebKit projects while preserving
the existing Chromium behavior.
---
Outside diff comments:
In `@apps/web/src/providers/EngineProvider.tsx`:
- Around line 62-72: Update the cleanup function returned by the EngineProvider
effect so client disposal is chained after media disposal settles. Preserve the
existing error logging for both operations, including the optional-media case,
and ensure MediaService.dispose() completes before invoking client.dispose() so
the broker cannot access a disposed client.
---
Nitpick comments:
In `@apps/web/vite.config.ts`:
- Around line 21-24: Update the bundle URL construction in the Vite
configuration to use the resolved deployment base instead of hardcoding “/”.
Capture the resolved base through the plugin’s configResolved hook and prefix
each fileName in the shell mapping with that base, preserving the existing
source-map filtering and sorting.
In `@crates/wasm/src/host.rs`:
- Around line 380-387: Update the read-content flow containing
read_content_range and the download flow to wrap each decrypted Rust-side byte
buffer in Zeroizing before converting it with Uint8Array::from, ensuring the
plaintext is wiped when the terminal owner releases it. Preserve the existing
JavaScript copy and return behavior, and follow the existing push_chunk pattern
without zeroizing caller-owned buffers.
In `@crates/wasm/src/seams_bridge.rs`:
- Around line 621-626: Add a `tooLarge` result case to the relevant bridge test
or fixture with both `observed` and `limit` omitted, then assert it is handled
successfully with the existing zero-value behavior from `capped_count` and
`required_u64`. Keep the existing populated `tooLarge` coverage unchanged.
In `@packages/client/src/media/range.test.ts`:
- Around line 1-47: Expand the resolveMediaRequest tests to assert both returned
window values and content-range headers, covering suffix ranges (bytes=-100),
open-ended ranges (bytes=100-), end values beyond SIZE (bytes=4000-99999),
malformed and multi-range inputs falling back to 200, and unsafe-integer digit
runs failing closed. Reuse headerMap and the existing SIZE fixture while
verifying the exact safe byte window and response behavior.
In `@packages/client/src/media/service.test.ts`:
- Around line 309-315: Update the test case “accepts the real browser container
and port types structurally” to use Vitest’s expectTypeOf assertions, verifying
ServiceWorkerContainer extends ServiceWorkerContainerLike and MessagePort
extends MessagePortLike. Add expectTypeOf to the vitest import and remove the
null assignments, unsafe casts, and null expectations.
In `@packages/client/src/seams/http.test.ts`:
- Around line 70-91: Update packages/client/src/seams/http.test.ts lines 70-91
by adding a sendCapped case using a single chunk larger than the limit (for
example, producedBody(4096, 4) with a 1000-byte cap), asserting observed equals
4096 and that the body is cancelled. Update crates/engine/src/seams/http.rs
lines 144-150 to replace the claim that an over-cap body is never materialized
with wording that the JS seam aborts once accumulated data exceeds the cap, with
overshoot bounded by one chunk.
In `@packages/client/src/sw/install.ts`:
- Around line 88-94: Update the activate event chain in the
scope.addEventListener('activate', ...) handler to terminate with the same
.catch(ignore) handling used by the install handler. Keep the existing
deleteStaleCaches, clients.claim, and refresh sequence unchanged.
In `@packages/client/src/sw/precache.test.ts`:
- Around line 65-75: Add a precache test covering an update manifest where one
resource fails during Cache.addAll, using a test double that rejects atomically
like the real Cache API. Assert the expected retained cache contents after the
failed update, alongside the existing precacheAppShell coverage.
- Around line 22-49: Update the precacheAppShell tests to capture the Request
passed to the manifest fetch, then assert that readManifest requests
/precache-manifest.json resolved against ORIGIN with cache set to no-store.
Adjust the manifestFetch test double as needed to expose the captured request
while preserving the existing cache behavior assertions.
In `@packages/client/src/sw/precache.ts`:
- Around line 97-105: Update the navigation branch around fetchFn so the network
request races against a short timeout and rejects when the timer wins, allowing
the existing cached APP_SHELL_DOCUMENT fallback to run; preserve successful
network responses and rethrow the original error when no cached shell exists.
In `@packages/client/test/browser/media.spec.ts`:
- Around line 182-198: Update the two open-tab test around localTab so each tab
is initialized with a distinct fixture seed. Use the corresponding seed when
computing fromA and fromB expected bodies, preserving the existing ticket sizes
and status assertions so cross-tab responses also fail on mismatched bytes.
In `@packages/client/test/browser/media.ts`:
- Around line 153-161: Update cbMediaDispose to reset portRequests alongside
readerCalls during teardown, ensuring both test counters return to their initial
state after disposal.
- Around line 93-96: Replace the fixed timeout in cbMediaEngine with bounded
polling of client?.currentRole(), resolving as soon as the role is no longer
'none'. Add a deadline so polling cannot hang indefinitely, while preserving the
existing role result and fallback behavior when the deadline is reached.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9070e041-d803-4a13-8b53-ba328e6f76ba
📒 Files selected for processing (62)
apps/web/src/engine/createMediaService.test.tsapps/web/src/engine/createMediaService.tsapps/web/src/providers/EngineProvider.tsxapps/web/src/sw.tsapps/web/vite.config.tscrates/engine/src/content/mod.rscrates/engine/src/facade.rscrates/engine/src/seams/http.rscrates/engine/src/seams/mod.rscrates/engine/tests/content_wipe.rscrates/engine/tests/write_plane.rscrates/wasm/src/host.rscrates/wasm/src/seams_bridge.rspackages/client/package.jsonpackages/client/src/broadcast.tspackages/client/src/broadcastTransport.test.tspackages/client/src/broadcastTransport.tspackages/client/src/correlatedTransport.tspackages/client/src/engineClient.tspackages/client/src/facade.test.tspackages/client/src/facade.tspackages/client/src/index.tspackages/client/src/leaderRelay.tspackages/client/src/media/broker.test.tspackages/client/src/media/broker.tspackages/client/src/media/protocol.tspackages/client/src/media/range.test.tspackages/client/src/media/range.tspackages/client/src/media/registry.test.tspackages/client/src/media/registry.tspackages/client/src/media/service.test.tspackages/client/src/media/service.tspackages/client/src/seams/http.test.tspackages/client/src/seams/http.tspackages/client/src/seams/index.tspackages/client/src/seams/types.tspackages/client/src/sw/install.test.tspackages/client/src/sw/install.tspackages/client/src/sw/pipe.test.tspackages/client/src/sw/pipe.tspackages/client/src/sw/precache.test.tspackages/client/src/sw/precache.tspackages/client/src/sw/serviceWorker.tspackages/client/src/sw/testDoubles.tspackages/client/src/testkit.tspackages/client/src/transport.tspackages/client/src/worker/engineHost.tspackages/client/src/worker/engineWasm.tspackages/client/src/worker/protocol.tspackages/client/src/worker/serve.test.tspackages/client/src/worker/serve.tspackages/client/test/browser/conformance.worker.tspackages/client/test/browser/fakeEngine.worker.tspackages/client/test/browser/index.htmlpackages/client/test/browser/journalEngine.worker.tspackages/client/test/browser/media.spec.tspackages/client/test/browser/media.tspackages/client/test/browser/mediaEngine.worker.tspackages/client/test/browser/mediaFixture.tspackages/client/test/browser/sw.tspackages/client/test/browser/vite.config.tspackages/client/tsconfig.build.json
Addresses the CodeRabbit review on #951. - Aim `cb:media:needPort` at the tab that lacks a port. A broadcast made every other tab re-broker, and a superseded broker drops the cursors of bodies still streaming on it. - Disarm a pull deadline when its body is cancelled, so the timer cannot later discard a working port under the other bodies on it. - Build the sealed response headers pair by pair; a name or value the `Headers` grammar rejects must not fail the whole response. - Cache manifest entries one at a time. `addAll` is atomic, so a single unreachable asset dropped the entire shell. Pruning still follows the manifest, so a failed refresh does not evict a good entry. - Await the shell re-learn on a restarted worker before claiming a same-origin GET, and terminate the activate chain like the install one. - Emit the Service Worker as `iife`: with `es`, a dynamic import anywhere in its graph emits `import.meta`, which a classic worker rejects. - Cover a strictly past-EOF ranged read, and `useMediaService`.
Follows the body-only findings on the CodeRabbit review of #951. - Wrap the plaintext `download` and `downloadRange` return in `Zeroizing`. Both are the terminal owner of the Rust-side buffer once the bytes are copied into the JS heap, matching what `push_chunk` already does for the write direction. - Poll the lock election in the browser harness instead of sleeping a fixed 50 ms before asserting an exact role, and reset `portRequests` alongside `readerCalls` on dispose. - Assert the precache manifest is requested same-origin with `no-store`; a cached manifest would pin the shell to a superseded deploy.
CodeRabbit body-only findingsThe outside-diff comment and the 13 nitpicks, none of which carry threads. All verified against the branch; applied in 7b2a550 / c2bb578 unless noted. Applied
Correct, and it is repo security rule 7 rather than a nitpick:
Real flake, and it sits in the merge-blocking browser suite.
Applied. The two counters serve the same purpose and only one was reset.
Applied, with the reason sharpened: nothing proved
Both folded into the threads on Declined
The ordering observation is right and I tried it; the fix costs more than it buys, so it is out. Chaining That is not merely a test-shape problem. Your own note flags it: Against that, the failure being fixed is: a read in flight during teardown resolves against a disposed client and the media element errors. Teardown means the tab is unmounting or the engine is being rebuilt, so that stream is going away regardless. Trading an unbounded lock-release delay for a benign error on a dying stream is the wrong direction. Left as-is, and
Accurate but not actionable yet, and it is not the only place:
Correct that a stalled network delays the shell, but the proposed cure is worse than the disease. Any fixed timeout is wrong for someone: too short and a slow-but-working connection gets served a stale cached shell it never asked for, on every navigation. The browser already applies its own network timeout, and a captive portal typically fails fast rather than hanging. Deferring until there is a real measurement to size the timer against.
Fair that the test only checks
The gap is real and this is the most valuable of the nitpicks —
The analysis is right:
The point stands that both tabs synthesize from Follow-up items above are unfiled — say the word and I will open them against #655 with the usual dependency edges, or fold them into this PR if you would rather they not ship separately. Generated by Claude Code |
Deferred follow-ups now filedThe body-only comment above left four findings as follow-up work pending a decision on whether to file them. Filed against #655 with the usual dependency edges, and the reverse edge added on #641: #963 — web-client: unit-cover the media range arithmetic and give each tab distinct fixture bytes. Carries the #964 — seams: the capped fetch overshoots the cap by one chunk and the Http seam doc overclaims. Carries the One correction to the earlier reasoning while verifying it: the overshoot is not specific to the JS seam. Neither is a trust or memory-safety escape — both arms still abort the drain, and transport chunk sizes sit well below the 4 MiB content cap — so both stay out of this PR. No code change in this pass; the PR is unchanged and remains a draft. Generated by Claude Code |
The Service Worker is demoted to a dumb pipe: a media element requests an opaque /stream/ ticket URL, the worker forwards the Range over a MessageChannel port brokered at registration, and plaintext streams back through the port a window at a time. It holds no keys, no crypto, and no state beyond its ports, so a killed worker or a dead port re-brokers and re-buffers. Follower tabs route media through the leader. The same worker precaches the app shell so the UI boots offline, and never caches vault data. Under it, a ranged plaintext read: open_content_range fetches and unseals only the leaves a window covers, failing closed when a leaf's unsealed length disagrees with the manifest, and read_content_range shares the resolve and adoption gate with the whole-file read. The JS HttpSeam now enforces a true streaming peak-memory bound, mirroring desktop: a Content-Length pre-check, then a reader drain that cancels the moment the running total would pass the cap, so a gateway that omits or lies about Content-Length can no longer OOM the tab. Closes #641
Addresses the CodeRabbit review on #951. - Aim `cb:media:needPort` at the tab that lacks a port. A broadcast made every other tab re-broker, and a superseded broker drops the cursors of bodies still streaming on it. - Disarm a pull deadline when its body is cancelled, so the timer cannot later discard a working port under the other bodies on it. - Build the sealed response headers pair by pair; a name or value the `Headers` grammar rejects must not fail the whole response. - Cache manifest entries one at a time. `addAll` is atomic, so a single unreachable asset dropped the entire shell. Pruning still follows the manifest, so a failed refresh does not evict a good entry. - Await the shell re-learn on a restarted worker before claiming a same-origin GET, and terminate the activate chain like the install one. - Emit the Service Worker as `iife`: with `es`, a dynamic import anywhere in its graph emits `import.meta`, which a classic worker rejects. - Cover a strictly past-EOF ranged read, and `useMediaService`.
Follows the body-only findings on the CodeRabbit review of #951. - Wrap the plaintext `download` and `downloadRange` return in `Zeroizing`. Both are the terminal owner of the Rust-side buffer once the bytes are copied into the JS heap, matching what `push_chunk` already does for the write direction. - Poll the lock election in the browser harness instead of sleeping a fixed 50 ms before asserting an exact role, and reset `portRequests` alongside `readerCalls` on dispose. - Assert the precache manifest is requested same-origin with `no-store`; a cached manifest would pin the shell to a superseded deploy.
c2bb578 to
f59273a
Compare
The browser harnesses stood a fixed 50 ms sleep in for the election, so a loaded runner reported EngineClient's pre-election 'follower' default as if it were the outcome. Both roles now wait on an observable: leader on currentRole(), which flips only after promotion spawns the worker; follower on this tab's own queued request sitting behind another tab's held lock, matched by Web Locks client id so a tab that never requested the lock fails the wait instead of passing on the default.
f59273a to
d7fa222
Compare
`head_version` moved the head `Version` out of the version list with `into_iter().next()`. That is a bitwise `ptr::read`, and `IntoIter::drop` drops only elements `1..n`, so slot 0's `SecretBytes` — 32 bytes of content key — reached the allocator without its zeroizing `Drop`. Clone the head instead so the whole `Vec` drops in place. `open_content_range` assembled into a bare `Vec`, which the two per-leaf trust rejects abandon while it already holds verified plaintext. It is now `Zeroizing`, handed to the caller with `mem::take` so the caller stays the terminal owner. The zeroize watchdog could pass vacuously: it only scanned freed blocks of exactly one size, so a window in which none were freed tested nothing. It now counts what it inspected and asserts that count is non-zero, scans every block large enough to hold a marker, and covers the abandoned-assembly-buffer case — that test fails without the `Zeroizing` above. Also collapses `read_content` onto `read_content_range`: the range clamps to the manifest size, so the whole-file read is the unbounded window. `read_content_inner` and `open_content` are gone, and the duplicated snapshot projection with them; the `OpProgress` surface of both entry points is unchanged. Shared gateway/serve/block-store fixtures move to the testkit, their one home.
`portFor` short-circuited with a newest-wins fallback before `acquirePort` could target the owning client, so two open tabs plus a routine worker restart resolved tab A's ticket against tab B's registry — an unknown ticket, a hard 404 mid-playback, no retry. A non-anonymous client with no port of its own now gets `null` so the owner is asked; the newest-port fallback survives only for a request carrying no client identity. Waiters resolve on an adoption by their own client, not on any adoption. `sealed` forced only `cache-control` and re-emitted whatever `content-type` and CSP the port supplied, on a boundary the file itself calls untrusted. It now clamps the type through `safeMimeType` and forces `nosniff` and the sandbox CSP on every response carrying a body; the range resolver's copies stay as belt-and-braces. Drops `ok` from the head wire message. Every producer set it as exactly `status === 200 || status === 206` and nothing cross-checked the two, so `ok: true, status: 404` was representable; the status alone now decides.
Four hand-rolled `EngineHostLike` doubles re-stubbed the same eleven methods, so adding one member to the interface meant five edits. A `StubEngineHost` base whose every method rejects now carries them; the media worker's double turned out to be missing `siweChallenge` entirely, which nothing caught because no tsconfig covers `test/browser`. Three near-identical `MessagePortLike` doubles collapse into one `FakePort`, and `errorMessage` moves beside its two callers instead of being spelled a third time. Drops two tests that cannot fail: one asserted `toBeNull()` on values cast from `null`, the other counted `put` calls on a `CacheLike` that has no `put`. The prototype spy on `Uint8Array.prototype.set` goes too — it pinned implementation shape and leaked on failure; the body assertion carries the claim. `serve.ts` no longer re-discriminates the union its own switch just discriminated, `broker.ts` spells the head post and the stale-cursor guard once each, and `precache.ts` reuses `sameOriginGet` rather than re-deriving it. Comment pass: four absence-justifying comments removed, four rationales restated at multiple sites collapsed to one home each, and the `sendCapped` doc no longer calls an inclusive cap exclusive.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/client/src/seams/http.test.ts (1)
82-91: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case for a malformed
Content-Length.The seam converts the header with
Number(contentLength)and gates onNumber.isFinite(declared). A non-numeric header such asabcproducesNaN, and a negative header produces a finite value below the cap. Both fall through to the streaming path. The suite covers absent and lying-small headers but not these two. One test pins the fall-through as intended behavior rather than an accident.As per path instructions for
**/*.test.ts: "Focus on test coverage, edge cases, and test quality."🧪 Proposed additional case
+ it('ignores a non-numeric Content-Length and enforces the cap while streaming', async () => { + const body = producedBody(100, 100); + stubFetch(new Response(body.stream, { status: 200, headers: { 'content-length': 'abc' } })); + + const result = await new FetchHttp().sendCapped(GET, 1000); + + expect(result).toEqual({ kind: 'tooLarge', observed: 1100, limit: 1000 }); + expect(body.cancelled()).toBe(true); + });🤖 Prompt for 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. In `@packages/client/src/seams/http.test.ts` around lines 82 - 91, Add a test near the existing Content-Length cases for malformed declarations, covering both a non-numeric value such as “abc” and a negative value. Verify each remains on the streaming path and preserves the intended result, rather than being rejected by the declared-size cap check.Source: Path instructions
🤖 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 `@crates/engine/src/content/mod.rs`:
- Around line 228-235: Prevent plaintext loss during Vec reallocation in the
content assembly path around plaintext and extend_from_slice: either validate
length against an explicit caller-respected cap and reserve the full size once,
or implement explicit growth that zeroizes the existing buffer before replacing
it. Preserve the existing allocation-budget defense and trust checks, and add a
content-wipe test with a requested range exceeding MAX_RESOLVED_RECORD_BYTES to
exercise growth.
In `@crates/engine/tests/content_wipe.rs`:
- Around line 1-20: Add a required CI job in the existing .github/workflows
configuration, named clearly for the content-wipe suite (for example,
content_wipe or cargo-wipe), that invokes the cipherbox-engine content_wipe test
binary via cargo test -p cipherbox-engine with the allocator-backed targets and
configuration it requires. Ensure the job is part of the normal required
workflow path and explicitly runs this suite rather than relying only on
unrelated engine tests.
In `@packages/client/src/sw/install.test.ts`:
- Around line 106-198: Add a named CI job or Vitest entry for the
installServiceWorker suite in the existing client-package test workflow. Ensure
the new gate explicitly runs the install.test suite alongside the remaining
packages/client tests and is visible as a distinct named check in CI.
---
Nitpick comments:
In `@packages/client/src/seams/http.test.ts`:
- Around line 82-91: Add a test near the existing Content-Length cases for
malformed declarations, covering both a non-numeric value such as “abc” and a
negative value. Verify each remains on the streaming path and preserves the
intended result, rather than being rejected by the declared-size cap check.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 403b1b8a-6935-4cd8-a2b0-ec9e0d611336
📒 Files selected for processing (68)
apps/web/src/engine/createMediaService.test.tsapps/web/src/engine/createMediaService.tsapps/web/src/providers/EngineProvider.test.tsxapps/web/src/providers/EngineProvider.tsxapps/web/src/sw.tsapps/web/vite.config.tscrates/engine/src/content/mod.rscrates/engine/src/facade.rscrates/engine/src/seams/http.rscrates/engine/src/seams/mod.rscrates/engine/src/testkit/content.rscrates/engine/src/testkit/mod.rscrates/engine/tests/content_wipe.rscrates/engine/tests/write_plane.rscrates/wasm/src/host.rscrates/wasm/src/seams_bridge.rspackages/client/package.jsonpackages/client/src/broadcast.tspackages/client/src/broadcastTransport.test.tspackages/client/src/broadcastTransport.tspackages/client/src/correlatedTransport.tspackages/client/src/engineClient.tspackages/client/src/errorMessage.tspackages/client/src/facade.test.tspackages/client/src/facade.tspackages/client/src/index.tspackages/client/src/leaderRelay.tspackages/client/src/media/broker.test.tspackages/client/src/media/broker.tspackages/client/src/media/protocol.tspackages/client/src/media/range.test.tspackages/client/src/media/range.tspackages/client/src/media/registry.test.tspackages/client/src/media/registry.tspackages/client/src/media/service.test.tspackages/client/src/media/service.tspackages/client/src/seams/http.test.tspackages/client/src/seams/http.tspackages/client/src/seams/index.tspackages/client/src/seams/types.tspackages/client/src/sw/install.test.tspackages/client/src/sw/install.tspackages/client/src/sw/pipe.test.tspackages/client/src/sw/pipe.tspackages/client/src/sw/precache.test.tspackages/client/src/sw/precache.tspackages/client/src/sw/serviceWorker.tspackages/client/src/sw/testDoubles.tspackages/client/src/testkit.tspackages/client/src/transport.tspackages/client/src/worker/engineHost.tspackages/client/src/worker/engineWasm.tspackages/client/src/worker/protocol.tspackages/client/src/worker/serve.test.tspackages/client/src/worker/serve.tspackages/client/test/browser/conformance.worker.tspackages/client/test/browser/election.tspackages/client/test/browser/fakeEngine.worker.tspackages/client/test/browser/index.htmlpackages/client/test/browser/journalEngine.worker.tspackages/client/test/browser/leadership.tspackages/client/test/browser/media.spec.tspackages/client/test/browser/media.tspackages/client/test/browser/mediaEngine.worker.tspackages/client/test/browser/mediaFixture.tspackages/client/test/browser/sw.tspackages/client/test/browser/vite.config.tspackages/client/tsconfig.build.json
🚧 Files skipped from review as they are similar to previous changes (50)
- packages/client/src/broadcastTransport.test.ts
- packages/client/src/facade.ts
- apps/web/src/engine/createMediaService.test.ts
- packages/client/test/browser/sw.ts
- packages/client/src/correlatedTransport.ts
- packages/client/tsconfig.build.json
- packages/client/src/media/registry.test.ts
- packages/client/src/sw/serviceWorker.ts
- packages/client/src/seams/http.ts
- crates/wasm/src/seams_bridge.rs
- crates/engine/src/seams/mod.rs
- packages/client/src/engineClient.ts
- packages/client/src/worker/engineHost.ts
- apps/web/src/sw.ts
- apps/web/src/providers/EngineProvider.tsx
- packages/client/src/broadcastTransport.ts
- packages/client/test/browser/mediaEngine.worker.ts
- packages/client/src/worker/engineWasm.ts
- packages/client/src/facade.test.ts
- packages/client/test/browser/media.spec.ts
- packages/client/src/sw/precache.test.ts
- packages/client/src/leaderRelay.ts
- crates/engine/tests/write_plane.rs
- packages/client/src/seams/index.ts
- packages/client/src/media/service.test.ts
- packages/client/test/browser/mediaFixture.ts
- packages/client/test/browser/index.html
- packages/client/test/browser/conformance.worker.ts
- packages/client/package.json
- packages/client/src/worker/serve.test.ts
- apps/web/vite.config.ts
- packages/client/src/sw/pipe.test.ts
- packages/client/src/sw/pipe.ts
- packages/client/src/index.ts
- packages/client/src/sw/precache.ts
- packages/client/src/transport.ts
- packages/client/test/browser/vite.config.ts
- packages/client/src/worker/protocol.ts
- packages/client/src/media/broker.test.ts
- crates/engine/src/seams/http.rs
- packages/client/src/media/range.test.ts
- packages/client/src/media/registry.ts
- apps/web/src/engine/createMediaService.ts
- packages/client/src/media/broker.ts
- packages/client/src/media/service.ts
- packages/client/src/media/protocol.ts
- packages/client/src/broadcast.ts
- crates/wasm/src/host.rs
- packages/client/src/sw/install.ts
- crates/engine/src/facade.rs
A Zeroizing<Vec<u8>> wipes only the allocation it currently owns, so a read wider than the prealloc budget freed the outgrown buffer with plaintext still in it. Grow explicitly and let the old allocation drop through Zeroizing, and pin it with an allocator-watchdog case whose window exceeds the budget. Also cover the malformed Content-Length fall-through in the capped fetch seam.
|
Nitpick from the 2026-08-02 review — Both fall-throughs are now pinned as intended behaviour rather than an accident, as one
Either way the streaming cap is the only gate left, and the assertion is the same one the absent- and lying-small-header cases make: The proposed diff was adapted rather than pasted: it asserted
|
PR #951 added sendCapped to HttpSeam; ScriptedHttp now serves it from the same scripted response queue and applies the inclusive cap.
* fix: bring the browser mailbox seam onto the served API routes The browser Mailbox seam posted raw octet-stream bytes with idempotency-key and x-recipient headers, never polled, and treated an ack 404 as success -- a wire protocol the API has never served. Post and poll now speak the JSON PostMessageDto and messages envelope on /mailbox/messages, ack rejects a 404 instead of recording a delete that never happened, and the seam owns its own route suffix so the base URL is all a host supplies. Wires the engine mailbox conformance kit into the browser conformance switch, against an in-memory mock that enforces the same DTO validation and 8 KiB blob bound as the API, so the seam is gated by the merge-blocking browser suite. Closes #836 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DnPEfCL8sJ8xAaqAvfJc9n * test: handle aborted request bodies in the browser harness mocks An abort rejects readBody; without a rejection handler the mock routing PUT and echo endpoints leave it unhandled, which crashes the dev server under Node's default unhandled-rejection policy and fails unrelated tests in the same run. Matches the handled pattern already used by the mailbox POST. * test: satisfy the capped HTTP seam in the mailbox test double PR #951 added sendCapped to HttpSeam; ScriptedHttp now serves it from the same scripted response queue and applies the inclusive cap. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 participant
Problem
blueprint/web-client.mdresolves the #28 D4 open point by demoting the v1 decrypt Service Worker — a crypto layer doing CTR streaming over a whole buffered file — to a dumb byte pipe: a media element requests an opaque/stream/…ticket URL, the worker forwards the Range over aMessageChannelport brokered at registration, and plaintext streams back through the port. The worker holds no keys, no crypto, and no state worth keeping. The same worker precaches the app shell, because a vault you can mutate offline is a vault whose UI must boot offline.None of it existed. There was no Service Worker, no
/stream/surface, no precache, and no ranged read to sit under any of it —leaf_range_for_byte_rangeshipped in #700 with zero call sites, and the engine could only return whole files.This slice also carries the cross-cutting residual of #742/#787 (closed engine-side by #783): the browser
HttpSeamdid only aContent-Lengthpre-check, so a gateway that omits or lies aboutContent-Lengthcould still force a full-body buffer in the tab — a browser-tab OOM. Desktop already enforced a true streaming peak-memory bound; web did not.Change
A ranged plaintext read, end to end.
open_content_rangereplacesopen_content, which is now the whole-file case of it: it CID-verifies the DAG root, maps the byte window to leaf indices, fetches and unseals only those leaves, and trims. It fails closed as a trust violation if any leaf's unsealed length disagrees with what the manifest implies — a short middle leaf silently shifts every downstream byte, so that is a trust violation, not a size quirk. Preallocation is bounded by the clamped range, never the declared size.read_content_rangeshares the resolve, adoption gate, and head-version selection withread_contentthrough a newhead_versionhelper, and deliberately emits noOpProgressevents: one ranged read per seek and per buffer refill would drown the event stream.downloadRangethreads it through the WASM boundary and every TypeScript transport seam, including the follower broadcast path and the leader relay.A real streaming peak-memory bound in the JS
HttpSeam.FetchHttp.sendCappedmirrorscrates/desktop-seams/src/http.rs: aContent-Lengthpre-check before a byte is read, then aReadableStreamreader drain that cancels the moment the running total would exceedmaxBytes. The bound is exclusive, so a body exactly at the cap is admitted. The WASM bridge binds it assendCappedover a{ kind: 'response' | 'tooLarge' }result, fails closed on an unknownkind, and keeps a release-active body-length backstop so a buggy seam cannot talk the engine past the cap.The byte pipe. Everything semantic — ticket lookup, Range resolution, windowing — runs tab-side in
packages/client/src/media/**, where it is unit-testable without a worker;packages/client/src/sw/**is the forwarder. The worker owns only the ports it currently holds, so a killed worker or a dead port re-brokers and re-buffers. Bodies areReadableStreams with a zero high-water mark, so peak memory is one 1 MiB window per stream, never the file. Followers route media through the leader because the port terminates atEngineClient, whose transport already swaps under them.The app-shell precache. The
apps/webbuild emitsdist/sw.jsunhashed at the output root — a worker's scope is bounded by its own URL — from a separate single-input pass, so being self-contained is structural rather than incidental; alongside it goes aprecache-manifest.jsonof the emitted chunks. The worker caches only manifest entries, rejects cross-origin and/stream/entries, andCacheLikestructurally exposes noput, so vault data cannot reach the cache. A manifest that will not cache degrades to no offline shell, never a failed installation.Media responses carry
cache-control: no-store,x-content-type-options: nosniff, andcontent-security-policy: default-src 'none'; sandbox, and theirContent-Typeis clamped to anaudio/video/imageallowlist withimage/svg+xmlexcluded: a shared file is attacker-controlled content and/stream/is a same-origin URL.Tests
packages/client/test/browser/media.spec.ts, in the merge-blockingClient Browser Suite, against a real Service Worker, realfetchinterception, realMessageChanneland realReadableStream: 200 with exact bytes, 206 with a matchingcontent-range, 416 past EOF, 404 unknown ticket, a 2.5 MiB file streaming through three windows byte-exact, port re-broker after a real Service Worker kill, and follower media routing across two real tabs on real Web Locks and a realBroadcastChannel. The kill is a genuine CDPServiceWorker.stopAllWorkers; a negative control with the kill removed fails the assertion, so the test measures a re-broker rather than a port that never died.await response.arrayBuffer()failsaborts at the cap when Content-Length is absentandaborts at the cap when Content-Length lies small. Both assert the produced-byte count never exceeded the cap plus one chunk, which is the peak-memory claim itself. The browser suite asserts the same against a real chunked response with noContent-Length.write_plane.rscompares ranged reads against slices ofread_content. wasm-target tests pin the capped-fetch bridge, including the unknown-kindfail-closed and the over-cap backstop.re-brokers a fresh port and retries the open when a port goes silent; reverting the port-replacement orphan-erroring hangsfails a body still pulling on a port that gets replaced.Verification
All exit 0, from the worktree root:
The build output was inspected rather than assumed:
dist/sw.jssits at the root with zeroimportoccurrences and no engine-client code, anddist/precache-manifest.jsonlists exactly the emitted chunks plus/index.html, with/sw.jsand the manifest itself absent.Review gates
/simplify,/security-review, and/crypto-privacy-reviewall ran on this diff. The crypto gate confirmed the ranged path skips no check the whole-file read performs, and accepted the encode/decode fail-closed symmetry argument for the new leaf-length invariant:ContentWriter::pushseals only at exactlychunk_sizeandassemblealready carries a release-activeLinkCountMismatch, so a short middle leaf is unrepresentable rather than merely unguarded — a guard would be dead code, and the property is pinned by a release-running test instead.Findings folded in: the media
Content-Typehardening, the controlling-worker gate on ticket minting, the Rust-side capped-fetch backstop,Zeroizingon unsealed leaf plaintext, a 405 for non-GET/stream/, the delivered-length cursor advance, a pull timeout so a dead broker cannot hang a response body, per-client port routing so two open tabs stop breaking each other's streams, port-message shape validation in the worker, structuralno-storeon every synthesized response, the zero-length-chunk accounting guard, and a non-silentrevokeStreamUrl.Two findings were deferred, each with a dependency edge: #948 (pin the content version for the life of a stream —
read_content_rangere-resolves per window, so a stream can splice two versions and a large read costs one IPNS fan-out per megabyte; wants an engine-side stream handle) and #949 (the record-transport fetch is still uncapped, andcredentials: 'include'goes to third-party gateways rather than being scoped to the API origin as the blueprint specifies).No playback UI ships here — this slice lands the pipe and the registration; consuming
createStreamUrlin a preview view belongs to #807. Perblueprint/web-client.md"Open edges", no web app manifest or install prompt is added: the precache is decided, installability is deployment territory.Closes #641
Summary by CodeRabbit