Skip to content

Commit 6b7fbdf

Browse files
committed
fix(offline): wait for page control before warming editor assets
1 parent 6e344d2 commit 6b7fbdf

5 files changed

Lines changed: 121 additions & 4 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@ notes. Entries describe what users experience, not internal refactors.
99

1010
### Fixed
1111

12+
- First-visit editor preparation waits for the service worker to control the
13+
page, so warmed font resources remain available in its offline cache.
1214
- A failed recovery snapshot no longer removes an older document when storage
1315
remains full after a retry.
1416
- Recovery files that exceed the storage budget are rejected before their
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
# PR 279: first-visit editor cache race
2+
3+
The Docker E2E run at commit `5ae9dfdf` failed its landing-page cache assertion
4+
in shard 2. The other 109 cases in that shard passed. The retained trace from
5+
GitHub Actions run `39027953673` showed that the sole missing entry was
6+
`/sdkjs/common/libfont/engine/fonts.js`.
7+
8+
The font request succeeded, but occurred after `serviceWorker.ready` resolved
9+
and before `clients.claim()` controlled the first-visit page. It populated only
10+
the browser HTTP cache. The prefetcher's request deduplication then prevented a
11+
controlled retry that would populate the worker's offline cache.
12+
13+
`public/landing-prefetch.js` now waits for actual page control before issuing
14+
warm-up requests. It installs the control listener before rechecking the
15+
controller, removes listeners when settled, and cancels the wait on departure
16+
or worker upgrade. Resuming keeps the URL eligible for a new request. Existing
17+
controlled pages continue immediately, and the upgrade gate still applies.
18+
19+
Two deterministic regressions first failed against the previous implementation:
20+
one verifies that a resolved readiness promise cannot start an uncontrolled
21+
fetch; the other checks cancellation, late control changes and retry after
22+
returning. Both pass with the fix, along with all 15 landing-prefetch unit tests.
23+
Independent read-only review also checked upgrade and BFCache interactions and
24+
reported no actionable findings.
25+
26+
The browser assertion now reports the actual list of missing URLs rather than
27+
only their count. Its existing 120-second deadline is unchanged.
28+
29+
## Verification
30+
31+
- Full unit coverage: 6,290 tests in 238 files pass; 89.56% lines and 83.08%
32+
branches. All repository coverage gates pass.
33+
- A fresh production Docker image passes all four landing-prefetch Chromium
34+
cases, including all editor engines cached, background core caching, actual
35+
font-catalog requests and an editor open served from the warmed worker cache.
36+
- Archive integrity, repository-wide formatting, oxlint, refreshed workspace
37+
declarations and application TypeScript checks pass.
38+
39+
```sh
40+
pnpm run test:coverage --maxWorkers=2
41+
pnpm run format:check
42+
pnpm run lint:ts
43+
docker build -t document:pr279-ci .
44+
docker run -d --rm --name document-pr279-ci -p 127.0.0.1:4199:80 document:pr279-ci
45+
E2E_BASE_URL=http://127.0.0.1:4199 E2E_PORT=4199 pnpm exec playwright test \
46+
test/e2e/landing-prefetch.spec.ts --workers=1 --reporter=line
47+
docker stop document-pr279-ci
48+
```

‎public/landing-prefetch.js‎

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -201,10 +201,41 @@
201201
* serves the vendored tree cache-first -- a miss there re-requests the file
202202
* even when the browser still holds a copy.
203203
*/
204+
// ready can resolve before clients.claim() controls this first-visit page.
205+
// Fetching in that gap warms only the HTTP cache, then requested[url] would
206+
// prevent the controlled retry that actually populates the worker cache.
207+
function waitForControl(registration) {
208+
var workers = navigator.serviceWorker;
209+
if (workers.controller) return Promise.resolve(!leaving && watchRegistration(registration));
210+
return new Promise(function (resolve) {
211+
var signal = aborter && aborter.signal;
212+
var finished = false;
213+
function done(allowed) {
214+
if (finished) return;
215+
finished = true;
216+
workers.removeEventListener('controllerchange', changed);
217+
if (signal) signal.removeEventListener('abort', cancelled);
218+
resolve(allowed);
219+
}
220+
function cancelled() {
221+
done(false);
222+
}
223+
function changed() {
224+
if (leaving || upgrading || (signal && signal.aborted)) done(false);
225+
else if (workers.controller) done(watchRegistration(registration));
226+
}
227+
workers.addEventListener('controllerchange', changed);
228+
if (signal) signal.addEventListener('abort', cancelled, { once: true });
229+
changed();
230+
});
231+
}
232+
204233
function warm(url) {
205234
if (!navigator.serviceWorker) return warmRequest(url);
206235
return navigator.serviceWorker.ready
207-
.then(watchRegistration)
236+
.then(function (registration) {
237+
return watchRegistration(registration) ? waitForControl(registration) : false;
238+
})
208239
.then(function (allowed) {
209240
if (allowed) return warmRequest(url);
210241
})

‎test/e2e/landing-prefetch.spec.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ test.describe('landing page warm-up', () => {
6868
return wanted.filter((u) => !held.has(u));
6969
});
7070

71-
await expect.poll(async () => (await missing()).length, { timeout: 120_000 }).toBe(0);
71+
await expect.poll(missing, { timeout: 120_000, intervals: [1000] }).toEqual([]);
7272

7373
const shape = await page.evaluate(() => {
7474
const lp = (window as any).__landingPrefetch;

‎test/unit/landing-prefetch.test.ts‎

Lines changed: 38 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ describe('landing page warm-up', () => {
116116
});
117117

118118
describe('landing warm-up during a worker upgrade', () => {
119-
function setup(effectiveType?: string) {
119+
function setup(effectiveType?: string, controlled = true) {
120120
const src = readFileSync(resolve(__dirname, '../../public/landing-prefetch.js'), 'utf8');
121121
const win = Object.assign(new EventTarget(), {
122122
requestIdleCallback: (callback: () => void) => callback(),
@@ -128,7 +128,10 @@ describe('landing warm-up during a worker upgrade', () => {
128128
waiting: null as (EventTarget & { state: string; scriptURL?: string }) | null,
129129
installing: null as (EventTarget & { state: string }) | null,
130130
});
131-
const workers = Object.assign(new EventTarget(), { ready: Promise.resolve(registration) });
131+
const workers = Object.assign(new EventTarget(), {
132+
ready: Promise.resolve(registration),
133+
controller: controlled ? ({} as object | null) : null,
134+
});
132135
const fetchMock = vi.fn((_url: string, _init?: RequestInit) => Promise.resolve(okResponse()));
133136
new Function('window', 'document', 'navigator', 'fetch', 'AbortController', src)(
134137
win,
@@ -140,6 +143,39 @@ describe('landing warm-up during a worker upgrade', () => {
140143
return { win, doc, cta, registration, workers, fetchMock, hook: win.__landingPrefetch! };
141144
}
142145

146+
it('waits for actual page control after ready before fetching a cacheable resource', async () => {
147+
const { workers, hook, fetchMock } = setup(undefined, false);
148+
const cached = new Set<string>();
149+
fetchMock.mockImplementation((url) => {
150+
if (workers.controller) cached.add(url);
151+
return Promise.resolve(okResponse());
152+
});
153+
const pending = hook.warm('/sdkjs/common/libfont/engine/fonts.js');
154+
await new Promise((resolve) => setTimeout(resolve, 0));
155+
expect(fetchMock).not.toHaveBeenCalled();
156+
expect(cached.size).toBe(0);
157+
158+
workers.controller = {};
159+
workers.dispatchEvent(new Event('controllerchange'));
160+
await pending;
161+
expect(cached.has('/sdkjs/common/libfont/engine/fonts.js')).toBe(true);
162+
});
163+
164+
it('settles a control wait on departure and leaves the URL retryable after returning', async () => {
165+
const { workers, win, hook, fetchMock } = setup(undefined, false);
166+
const pending = hook.warm('/awaiting-control.js');
167+
await new Promise((resolve) => setTimeout(resolve, 0));
168+
win.dispatchEvent(new Event('beforeunload'));
169+
await pending;
170+
workers.controller = {};
171+
workers.dispatchEvent(new Event('controllerchange'));
172+
expect(fetchMock).not.toHaveBeenCalled();
173+
174+
win.dispatchEvent(new Event('pointerdown'));
175+
await hook.warm('/awaiting-control.js');
176+
expect(fetchMock.mock.calls.filter(([url]) => url === '/awaiting-control.js')).toHaveLength(1);
177+
});
178+
143179
it('holds a waiting upgrade without poisoning URLs, then warms after controllerchange', async () => {
144180
const { registration, workers, win, hook, fetchMock } = setup();
145181
registration.waiting = Object.assign(new EventTarget(), { state: 'installed' });

0 commit comments

Comments
 (0)