diff --git a/docs/operations/adapter-remediation-ledger.md b/docs/operations/adapter-remediation-ledger.md index 360e0f9..dc3379d 100644 --- a/docs/operations/adapter-remediation-ledger.md +++ b/docs/operations/adapter-remediation-ledger.md @@ -144,10 +144,10 @@ Rollout state starts at `NOT_STARTED`; documented-unimplemented items start at | SW-03 | Composed when capability selected | `corepack pnpm exec vitest run tests/unit/service-worker-runtime.test.ts` | `fix: make Service Worker cache and removal outcomes truthful` | `FIXED_NOT_RELEASED` | false removal success | Red `unregister() === false` reported as UNREGISTERED → green FAILED | | SW-04 | Composed when capability selected | `corepack pnpm exec vitest run tests/unit/service-worker-runtime.test.ts` | `fix: make Service Worker cache and removal outcomes truthful` | `FIXED_NOT_RELEASED` | removal outcome misreport | Red removal modes always DISABLED → green outcome matrix (ABSENT/UNREGISTERED/PURGED→DISABLED, OWNERSHIP_MISMATCH→INCOMPATIBLE, FAILED→FAILED) | | SW-05 | Build gate | `corepack pnpm exec vitest run tests/unit/service-worker-build-input.test.ts` | `fix: make Service Worker cache and removal outcomes truthful` | `FIXED_NOT_RELEASED` | build admission rejection | Red tamper table (stale digest, byte length, cross-origin URL, dot segment, extension mismatch, unknown field, duplicate URL) → green; build gate decodes through the shared codec and recomputes the canonical digest | -| SW-06 | Composed when capability selected | `corepack pnpm exec vitest run tests/unit/service-worker-runtime.test.ts` | — | `NOT_STARTED` | activation handshake failure | — | -| SW-07 | Composed when capability selected | `corepack pnpm exec vitest run tests/unit/service-worker-runtime.test.ts` | — | `NOT_STARTED` | activation blocked with zero clients | — | -| SW-08 | Composed when capability selected | `corepack pnpm exec vitest run tests/unit/service-worker-runtime.test.ts` | — | `NOT_STARTED` | per-client failure escalation | — | -| SW-09 | Composed when capability selected | `corepack pnpm exec vitest run tests/unit/service-worker-runtime.test.ts` | — | `NOT_STARTED` | late install work observed | — | +| SW-06 | Composed when capability selected | `corepack pnpm exec vitest run tests/unit/service-worker-runtime.test.ts` | `fix: harden Service Worker activation and install lifecycle` | `FIXED_NOT_RELEASED` | activation handshake failure | Red foreign-source drain, source swap and 10 concurrent activations → green 24/24; replies correlate by source object identity against the captured waiting worker or controller, and activation/reset are single-flight | +| SW-07 | Composed when capability selected | `corepack pnpm exec vitest run tests/unit/service-worker-runtime.test.ts` | `fix: harden Service Worker activation and install lifecycle` | `FIXED_NOT_RELEASED` | activation blocked with zero clients | An empty in-scope client set is vacuously drained; `clients.matchAll()` failure still rejects | +| SW-08 | Composed when capability selected | `corepack pnpm exec vitest run tests/unit/service-worker-runtime.test.ts` | `fix: harden Service Worker activation and install lifecycle` | `FIXED_NOT_RELEASED` | per-client failure escalation | Per-client `postMessage` isolation; `skipWaiting()` is the commit point and its failure is REJECTED, with accepted/reload notifications sent only afterwards as best effort | +| SW-09 | Composed when capability selected | `corepack pnpm exec vitest run tests/unit/service-worker-runtime.test.ts` | `fix: harden Service Worker activation and install lifecycle` | `FIXED_NOT_RELEASED` | late install work observed | Red late-fetch case → green; a fenced worker starts no new candidate work, late response bodies are cancelled, digest throws map to a closed outcome, and a second exact-delete runs once the abandoned install settles without extending the public bound | | SW-10 | Protocol V2 migration | `corepack pnpm exec vitest run tests/unit/service-worker-runtime.test.ts` | — | `DEFERRED_TO_MIGRATION` | V1/V2 mismatch fail-close | Not closed here. Full-identity protocol V2 is an expand → dual-read → old-writer drain → contract deployment that spans releases; the prerequisite shared manifest codec and canonical digest landed with SW-05. | | WP-01 | Web Push `NOT_SELECTED` | `corepack pnpm exec vitest run tests/unit/web-push-fence-store.test.ts` | `fix: bind Web Push mutations to exact authority` | `FIXED_NOT_RELEASED` | CAS receipt rejection | Red stale/skipped/huge revision receipts → green; write and remove share one exact-next-revision validator | | WP-02 | Web Push `NOT_SELECTED` | `corepack pnpm exec vitest run tests/unit/web-push-fence-store.test.ts` | — | `DEFERRED_TO_MIGRATION` | `RECONCILIATION_REQUIRED` backlog | Deferred to the Task 16 versioned-migration PR: `MUTATION_OUTCOME_UNKNOWN` and the `RECONCILIATION_REQUIRED` lifecycle are part of the same wire/data migration as WP-03. | diff --git a/src/adapters/service-worker/service-worker-lifecycle.ts b/src/adapters/service-worker/service-worker-lifecycle.ts index 28f0c85..ffa8ad9 100644 --- a/src/adapters/service-worker/service-worker-lifecycle.ts +++ b/src/adapters/service-worker/service-worker-lifecycle.ts @@ -277,49 +277,64 @@ export function createServiceWorkerRuntime( parsed.message.sourceBuildId, ); if (!drained) { - for (const client of clients) { - client.postMessage( - createServiceWorkerMessage({ - kind: "ACTIVATE_REJECTED", - sourceBuildId: config.identity.buildId, - targetBuildId: parsed.message.sourceBuildId, - nonce, - }), - ); - } + notifyClients(clients, "ACTIVATE_REJECTED", parsed.message.sourceBuildId, nonce); return "REJECTED"; } - for (const client of clients) { - client.postMessage( - createServiceWorkerMessage({ - kind: "ACTIVATE_ACCEPTED", - sourceBuildId: config.identity.buildId, - targetBuildId: parsed.message.sourceBuildId, - nonce, - }), - ); - } - await scope.skipWaiting(); - for (const client of clients) { - client.postMessage( - createServiceWorkerMessage({ - kind: "ACTIVATED_RELOAD_REQUIRED", - sourceBuildId: config.identity.buildId, - targetBuildId: parsed.message.sourceBuildId, - nonce, - }), - ); + // SW-08. `skipWaiting()` is the activation commit. It must succeed before + // any client is told the activation was accepted, and its failure is a + // rejection rather than an accepted-then-failed activation. + try { + await scope.skipWaiting(); + } catch { + notifyClients(clients, "ACTIVATE_REJECTED", parsed.message.sourceBuildId, nonce); + return "REJECTED"; } + // Post-commit notifications are per-client best effort. + notifyClients(clients, "ACTIVATE_ACCEPTED", parsed.message.sourceBuildId, nonce); + notifyClients( + clients, + "ACTIVATED_RELOAD_REQUIRED", + parsed.message.sourceBuildId, + nonce, + ); return "ACCEPTED"; } + /** + * SW-08. One client's `postMessage()` throwing must not break the whole + * activation event; delivery is isolated per client. + */ + function notifyClients( + clients: readonly WorkerClientLike[], + kind: "ACTIVATE_REJECTED" | "ACTIVATE_ACCEPTED" | "ACTIVATED_RELOAD_REQUIRED", + targetBuildId: string, + nonce: string, + ): void { + for (const client of clients) { + try { + client.postMessage( + createServiceWorkerMessage({ + kind, + sourceBuildId: config.identity.buildId, + targetBuildId, + nonce, + }), + ); + } catch { + // A dead client cannot change the already committed activation. + } + } + } + async function drainClients( clients: readonly WorkerClientLike[], nonce: string, requesterBuildId: string, ): Promise { - if (clients.length === 0) return false; + // SW-07. No in-scope client means nothing dirty to drain, so the set is + // vacuously drained. A `clients.matchAll()` failure still rejects upstream. + if (clients.length === 0) return true; const drained = new Promise((resolve) => { const timer = setTimeout(() => { pendingActivations.delete(nonce); @@ -336,15 +351,24 @@ export function createServiceWorkerRuntime( }), ); }); + // SW-08. A client that cannot receive the drain request can never + // acknowledge it, so it fails immediately instead of holding the pending + // state until the timeout. for (const client of clients) { - client.postMessage( - createServiceWorkerMessage({ - kind: "CLIENT_DRAIN_REQUEST", - sourceBuildId: config.identity.buildId, - targetBuildId: requesterBuildId, - nonce, - }), - ); + try { + client.postMessage( + createServiceWorkerMessage({ + kind: "CLIENT_DRAIN_REQUEST", + sourceBuildId: config.identity.buildId, + targetBuildId: requesterBuildId, + nonce, + }), + ); + } catch { + const pending = pendingActivations.get(nonce); + if (pending) settlePendingActivation(nonce, pending, false); + return await drained; + } } return drained; } diff --git a/src/adapters/service-worker/service-worker-page-controller.ts b/src/adapters/service-worker/service-worker-page-controller.ts index 82be3ea..f88e4c7 100644 --- a/src/adapters/service-worker/service-worker-page-controller.ts +++ b/src/adapters/service-worker/service-worker-page-controller.ts @@ -58,6 +58,9 @@ export function createServiceWorkerPageController( let registration: ServiceWorkerRegistration | null = null; let messageListener: ((event: MessageEvent) => void) | null = null; let updateTimer: ReturnType | null = null; + /** SW-06. Single-flight command state. */ + let activationInFlight: Promise | null = null; + let resetInFlight: Promise | null = null; let stopped = false; const pendingStops = new Set<() => void>(); @@ -217,6 +220,13 @@ export function createServiceWorkerPageController( observe("client_drain", "MALFORMED"); return; } + // SW-06. An arbitrary same-origin source must not be able to close this + // page's admission. The request has to come from the worker we are + // actually waiting on or the one currently controlling us. + if (!isExpectedWorkerSource(source)) { + observe("client_drain", "SOURCE_MISMATCH"); + return; + } const rejected = isBlocked(); source.postMessage( createServiceWorkerMessage({ @@ -234,6 +244,23 @@ export function createServiceWorkerPageController( container.addEventListener("message", messageListener); } + /** + * SW-06. Source identity is checked by object identity against the + * registration's waiting/installing/active worker and the container's + * controller. An empty `event.origin` is never used as a trust signal. + */ + function isExpectedWorkerSource(source: unknown): boolean { + const expected = [ + registration?.waiting, + registration?.installing, + registration?.active, + dependencies.container?.controller, + ]; + return expected.some( + (candidate) => candidate != null && candidate === source, + ); + } + function scheduleUpdateChecks(): void { // §17.14. At most one check per 6 hours, and none while the page is hidden. if (updateTimer) return; @@ -251,7 +278,16 @@ export function createServiceWorkerPageController( * §17.11. Activation is a handshake: every controlled client must close new * admission and acknowledge within 30s. One missing client rejects it. */ - async function requestActivation(): Promise { + function requestActivation(): Promise { + // SW-06. Concurrent callers share one command: a second call must not issue + // a second nonce, a second listener or a second postMessage. + activationInFlight ??= runActivation().finally(() => { + activationInFlight = null; + }); + return activationInFlight; + } + + async function runActivation(): Promise { const waiting = registration?.waiting; if (!waiting) return Object.freeze({ kind: "NO_WAITING_WORKER" as const }); if (isBlocked()) { @@ -287,6 +323,17 @@ export function createServiceWorkerPageController( ) { return; } + // SW-06. The reply must come from the exact worker this request was + // sent to. A source swap ends the request immediately as a protocol + // mismatch rather than waiting for the drain timeout. + if (event.source !== null && event.source !== waiting) { + finish(Object.freeze({ kind: "PROTOCOL_MISMATCH" as const })); + return; + } + if (registration?.waiting !== waiting) { + finish(Object.freeze({ kind: "PROTOCOL_MISMATCH" as const })); + return; + } if ( parsed.message.kind === "ACTIVATE_REJECTED" && parsed.message.nonce === nonce @@ -329,11 +376,21 @@ export function createServiceWorkerPageController( } /** §18.10. Static caches only; the registration itself is left in place. */ - async function resetOwnedCaches(): Promise { + function resetOwnedCaches(): Promise { + // SW-06. Single-flight, like activation. + resetInFlight ??= runReset().finally(() => { + resetInFlight = null; + }); + return resetInFlight; + } + + async function runReset(): Promise { const container = dependencies.container; if (!container?.controller) { return Object.freeze({ kind: "NOT_CONTROLLED" as const }); } + // SW-06. The reply must come from the controller this request was sent to. + const requestedController = container.controller; const nonce = nonces.issue(); return new Promise((resolve) => { let settled = false; @@ -357,6 +414,18 @@ export function createServiceWorkerPageController( ) { return; } + if ( + (event.source !== null && event.source !== requestedController) || + container.controller !== requestedController + ) { + finish( + Object.freeze({ + kind: "FAILED" as const, + code: "PROTOCOL_MISMATCH", + }), + ); + return; + } finish( Object.freeze({ kind: "RESET" as const, diff --git a/src/adapters/service-worker/service-worker-static-assets.ts b/src/adapters/service-worker/service-worker-static-assets.ts index a028924..81c7ecc 100644 --- a/src/adapters/service-worker/service-worker-static-assets.ts +++ b/src/adapters/service-worker/service-worker-static-assets.ts @@ -149,6 +149,19 @@ export async function installStaticAssets( if (outcome.kind === "REJECTED") { await dependencies.caches.delete(cacheName).catch(() => false); + // SW-09. A non-cooperative fetch, digest or `cache.put` started before the + // deadline cannot be cancelled, so it may recreate the candidate cache + // after that delete. The public result already closed at the deadline; a + // second exact delete is registered once the abandoned work settles. It is + // deliberately not awaited, so the public bound is not extended. + if (deadlineExceeded) { + void installation + .catch(() => undefined) + .then(async () => { + await dependencies.caches.delete(cacheName).catch(() => false); + }) + .catch(() => undefined); + } } return outcome; } @@ -173,6 +186,11 @@ async function installCandidate( const worker = async (): Promise => { for (;;) { if (failure) return; + // SW-09. Once fenced, no new candidate work is started. + if (signal.aborted) { + failure ??= rejected("INSTALL_DEADLINE_EXCEEDED"); + return; + } const asset = queue.shift(); if (!asset) return; const outcome = await storeAsset(asset, cache, dependencies, signal); @@ -205,15 +223,22 @@ async function storeAsset( if (signal.aborted) return rejected("FETCH_FAILED"); let response: Response; try { - const fetched = await abortable( - dependencies.fetcher(asset.url, { - cache: "no-store", - credentials: "omit", - redirect: "error", - signal, - }), + // SW-09. A non-cooperative fetch that ignores the signal still settles + // later; its body is compensated so an abandoned response is not left open. + const pending = dependencies.fetcher(asset.url, { + cache: "no-store", + credentials: "omit", + redirect: "error", signal, - ); + }); + const fetched = await abortable(pending, signal); + if (fetched === ABORTED) { + void pending + .then(async (late) => { + await late.body?.cancel(); + }) + .catch(() => undefined); + } if (fetched === ABORTED) return rejected("FETCH_FAILED"); response = fetched; } catch { @@ -234,7 +259,17 @@ async function storeAsset( if (!body.ok) return rejected(body.code); const bytes = body.bytes; - const digest = await abortable(dependencies.digest(bytes), signal); + // SW-09. A digest dependency that throws becomes a closed typed outcome + // rather than an escaping rejection. + let digest: string | typeof ABORTED; + try { + digest = await abortable( + Promise.resolve(dependencies.digest(bytes)), + signal, + ); + } catch { + return rejected("INTEGRITY_MISMATCH"); + } if (digest === ABORTED) return rejected("FETCH_FAILED"); if (digest !== asset.sha256) return rejected("INTEGRITY_MISMATCH"); diff --git a/tests/unit/service-worker-runtime.test.ts b/tests/unit/service-worker-runtime.test.ts index 6175dbf..754dd74 100644 --- a/tests/unit/service-worker-runtime.test.ts +++ b/tests/unit/service-worker-runtime.test.ts @@ -49,6 +49,8 @@ function pageContainer(options: { waiting?: boolean; controlled?: boolean } = {} } as unknown as ServiceWorkerContainer; return { container, + waiting, + controlled, registration, waitingMessages, controllerMessages, @@ -384,7 +386,12 @@ describe("service worker page protocol", () => { const browser = pageContainer({ waiting: true }); const controller = pageController(browser.container); await controller.start(); - const source = { postMessage: vi.fn() }; + // SW-06. Replies are correlated by source identity, so the fake request + // comes from the registration's waiting worker. + const source = browser.waiting as unknown as { + postMessage(message: unknown): void; + }; + const sourceMessages = vi.spyOn(source, "postMessage"); browser.dispatch( createServiceWorkerMessage({ @@ -396,7 +403,7 @@ describe("service worker page protocol", () => { source, ); - expect(source.postMessage).toHaveBeenCalledWith( + expect(sourceMessages).toHaveBeenCalledWith( expect.objectContaining({ kind: "CLIENT_DRAINED", sourceBuildId: "page-build", @@ -444,7 +451,7 @@ describe("service worker page protocol", () => { nonce: request.nonce, }), cachesDeleted: 2, - }); + }, browser.controlled ?? undefined); await expect(result).resolves.toEqual({ kind: "RESET", cachesDeleted: 2 }); await controller.stop(); @@ -700,7 +707,7 @@ describe("service worker static asset install", () => { it("aborts and rolls back a candidate cache at the overall install deadline", async () => { vi.useFakeTimers(); - const deleteCache = vi.fn(async () => true); + const deleteCache = vi.fn(async (_name: string) => true); const fetcher = vi.fn( (_input: RequestInfo | URL, init?: RequestInit) => new Promise((_resolve, reject) => { @@ -725,7 +732,47 @@ describe("service worker static asset install", () => { code: "INSTALL_DEADLINE_EXCEEDED", }); expect(fetcher.mock.calls[0]?.[1]?.signal?.aborted).toBe(true); - expect(deleteCache).toHaveBeenCalledTimes(1); + // SW-09. The public result closes at the deadline with one exact delete, + // and a second exact delete is registered once the abandoned install work + // actually settles. Both target the same owned candidate cache. + await vi.advanceTimersByTimeAsync(0); + expect(deleteCache).toHaveBeenCalledTimes(2); + expect(new Set(deleteCache.mock.calls.map((call) => call[0])).size).toBe(1); + vi.useRealTimers(); + }); + + it("observes and cleans non-cooperative late install work", async () => { + vi.useFakeTimers(); + const deleteCache = vi.fn(async (_name: string) => true); + const cancel = vi.fn(async () => {}); + let releaseFetch: ((response: Response) => void) | undefined; + // A fetch that ignores the abort signal entirely. + const fetcher = vi.fn( + () => + new Promise((resolve) => { + releaseFetch = resolve; + }), + ); + + const result = installStaticAssets(manifest, { + caches: { + open: vi.fn(async () => ({ put: vi.fn() }) as unknown as Cache), + delete: deleteCache, + }, + fetcher: fetcher as typeof fetch, + digest: vi.fn(), + }); + await vi.advanceTimersByTimeAsync(SERVICE_WORKER_BOUNDS.installDeadlineMs); + await expect(result).resolves.toEqual({ + kind: "REJECTED", + code: "INSTALL_DEADLINE_EXCEEDED", + }); + + // The late response arrives after the public bound; its body is cancelled + // and no unhandled rejection escapes. + releaseFetch?.({ body: { cancel } } as unknown as Response); + await vi.advanceTimersByTimeAsync(0); + expect(cancel).toHaveBeenCalledOnce(); vi.useRealTimers(); }); @@ -807,4 +854,65 @@ describe("service worker static asset install", () => { vi.useRealTimers(); } }); + + it("accepts replies only from the captured waiting or controller source", async () => { + const browser = pageContainer({ waiting: true }); + const controller = pageController(browser.container); + await controller.start(); + const foreign = { postMessage: vi.fn() }; + + // SW-06. A same-origin but unrecognised source must not close admission. + browser.dispatch( + createServiceWorkerMessage({ + kind: "CLIENT_DRAIN_REQUEST", + sourceBuildId: "worker-build", + targetBuildId: "page-build", + nonce: "drain-foreign", + }), + foreign, + ); + expect(foreign.postMessage).not.toHaveBeenCalled(); + await controller.stop(); + }); + + it("coalesces concurrent activation and reset commands", async () => { + const browser = pageContainer({ waiting: true }); + const controller = pageController(browser.container); + await controller.start(); + + // SW-06. Ten concurrent callers issue exactly one request. + const activations = Array.from({ length: 10 }, () => + controller.requestActivation(), + ); + await Promise.resolve(); + expect(browser.waitingMessages).toHaveLength(1); + expect(new Set(activations).size).toBe(1); + + await controller.stop(); + await Promise.allSettled(activations); + }); + + it("ends an activation whose reply source was swapped", async () => { + const browser = pageContainer({ waiting: true }); + const controller = pageController(browser.container); + await controller.start(); + const activation = controller.requestActivation(); + await Promise.resolve(); + + const request = browser.waitingMessages.at(-1) as { nonce?: string }; + // A different worker answers: terminate immediately rather than waiting for + // the drain timeout. + browser.dispatch( + createServiceWorkerMessage({ + kind: "ACTIVATED_RELOAD_REQUIRED", + sourceBuildId: "worker-build", + targetBuildId: "page-build", + nonce: request.nonce, + }), + { postMessage: vi.fn() }, + ); + + await expect(activation).resolves.toEqual({ kind: "PROTOCOL_MISMATCH" }); + await controller.stop(); + }); });