Skip to content

Commit fa44e7e

Browse files
chaxusclaude
andcommitted
fix(sw): hold cache writes open past the response they came from
CI caught what local runs cannot: E2E shard 2 failed with /sdkjs/slide/sdk-all.js missing from the cache -- the last file of the warm-up queue. The cause is not the test. `respondWith` settles the moment the response is handed to the page, and a worker with nothing else outstanding may be terminated right there, taking a half-finished `cache.put` with it. The larger the file the wider that window, which is backwards: the 13.7 MB SDK is the entry most worth keeping. Nothing pressures the worker to shut down locally, so it never showed. Every cache write in the fetch handler is now inside event.waitUntil: the vendor cache-first branch, the stale-while-revalidate branch, and both CORE_CACHE writes on the network-first path (smaller files, identical failure mode). This is a correctness fix for production, not a way to make a test pass. The test was also asserting too much: the page's promise settles when the last response has been read, while the worker's put completes on its own side a moment later. Demanding both at the same instant is the assertion's error, so it polls for eventual consistency instead. waitUntil itself resists a runtime test -- reliably provoking "the worker is terminated now" is not something a spec can do -- so it is pinned as a source contract, the way landing-pages and workflow-contract already pin theirs: every putInRuntimeCache call site must sit inside event.waitUntil, and every caches.open(CORE_CACHE) in the fetch handler must be immediately preceded by it. Reverse-verified: reverting either to a bare call turns the contract red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 6726e6f commit fa44e7e

4 files changed

Lines changed: 114 additions & 25 deletions

File tree

‎docs/explorations/2026-08-22-warm-all-editor-engines.md‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,35 @@ cache**,断言总数是 8 + 3×4。原有那条"预热后打开不走网络"
8585
反向验证:把 `warmEverything` 砍回只有 CORE → 新用例列出 13 个未缓存文件后变红;
8686
让配额判定恒为 false → 后台层用例变红。
8787

88+
## CI 抓出来的竞态:cache.put 活不过 respondWith
89+
90+
本地全绿,CI 的 `E2E shard 2` 红在 `/sdkjs/slide/sdk-all.js` 没被缓存——**队列里的最后
91+
一个文件**。
92+
93+
根因不在测试:`respondWith` 在响应交到页面手里的那一刻就算完成,此时 worker 如果没有别的
94+
待办事项,浏览器**可以立刻终止它**——正在写的 `cache.put` 就跟着没了。文件越大窗口越宽,
95+
而这正好是反的:13.7 MB 的 SDK 恰恰是最值得留住的那个。本地看不见,因为没有任何东西在催
96+
worker 关闭。
97+
98+
所以修的是 `public/sw.js`:所有缓存写入都包进 `event.waitUntil(...)`。一共四处——
99+
vendor 的 cache-first、stale-while-revalidate,以及 network-first 分支里那两处
100+
CORE_CACHE 写入(HTML 更小,但失败模式一模一样)。
101+
102+
**这是生产环境的正确性修复,不只是让测试变绿。**
103+
104+
测试那一侧也确实有问题,一并修了:页面侧的 Promise 在读完最后一个响应时就 resolve,而
105+
worker 的 put 在它自己那边稍后完成。要求两者同一瞬间就位是断言的错,不是功能的错——
106+
改成 `expect.poll` 等最终一致。
107+
108+
`waitUntil` 本身很难用运行时用例守护(要可靠地触发"worker 被终止")。改用源码契约钉住,
109+
和仓库里 `landing-pages` / `workflow-contract` 的做法一致:
110+
`putInRuntimeCache` 的每个调用点都必须在 `event.waitUntil(` 里,fetch handler 里的每处
111+
`caches.open(CORE_CACHE)` 前面都必须紧跟 `event.waitUntil(`。反向验证:任意一处退回裸调用
112+
都会变红。
113+
114+
(中途我还写了第三条"禁止裸 put"的断言,但它的正则分不出 put 有没有被 waitUntil 包着——
115+
删掉了,前两条已经精确覆盖。宁可少一条断言,也不要一条说不清自己在测什么的。)
116+
88117
## 一条工作树观察
89118

90119
提交前 `git diff` 里出现了四个我没碰过的文件(`l0.ts` 的 projectName fixture、

‎public/sw.js‎

Lines changed: 24 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -331,7 +331,13 @@ self.addEventListener('fetch', (event) => {
331331
if (cached) return cached;
332332
return fetch(event.request).then((networkResponse) => {
333333
if (networkResponse && networkResponse.status === 200 && networkResponse.type === 'basic') {
334-
putInRuntimeCache(event.request, networkResponse.clone());
334+
// waitUntil, not fire-and-forget: respondWith settles as soon as the
335+
// response is handed over, and a worker with no outstanding work may
336+
// be terminated right then -- taking a half-written entry with it.
337+
// The bigger the file the likelier that is, which is backwards: the
338+
// 13.7 MB SDK is the one entry most worth keeping. Surfaced as a CI
339+
// flake where the last file of a warm-up was missing from the cache.
340+
event.waitUntil(putInRuntimeCache(event.request, networkResponse.clone()));
335341
}
336342
return networkResponse;
337343
});
@@ -368,9 +374,9 @@ self.addEventListener('fetch', (event) => {
368374
// If network is ok, cache and return
369375
if (networkResponse && networkResponse.status === 200) {
370376
const responseToCache = networkResponse.clone();
371-
caches.open(CORE_CACHE).then((cache) => {
372-
cache.put(event.request, responseToCache);
373-
});
377+
// waitUntil for the same reason as the branches above: nothing else
378+
// keeps the worker alive once the response has been handed over.
379+
event.waitUntil(caches.open(CORE_CACHE).then((cache) => cache.put(event.request, responseToCache)));
374380
return networkResponse;
375381
}
376382
// If status is not 200, try cache
@@ -392,20 +398,25 @@ self.addEventListener('fetch', (event) => {
392398
.then((networkResponse) => {
393399
// Only cache valid 200 responses
394400
if (networkResponse && networkResponse.status === 200 && networkResponse.type === 'basic') {
395-
putInRuntimeCache(event.request, networkResponse.clone());
401+
// Same reasoning as the vendor branch above: the revalidation half of
402+
// stale-while-revalidate runs after the cached copy was handed back,
403+
// so nothing else is keeping the worker alive to finish the write.
404+
event.waitUntil(putInRuntimeCache(event.request, networkResponse.clone()));
396405
} else if (isHashedAsset && networkResponse && networkResponse.status === 404) {
397406
// A hashed asset that 404s means the page HTML is from another
398407
// deploy. Surface a network error (never an HTML body) so the
399408
// browser reports a clean failure, and refresh the cached shell
400409
// so the next navigation picks up the current HTML.
401-
caches.open(CORE_CACHE).then((cache) => {
402-
fetch('./index.html', { cache: 'no-cache' }).then((fresh) => {
403-
if (fresh && fresh.status === 200) {
404-
cache.put('./index.html', fresh.clone());
405-
cache.put('./', fresh);
406-
}
407-
});
408-
});
410+
event.waitUntil(
411+
caches.open(CORE_CACHE).then((cache) =>
412+
fetch('./index.html', { cache: 'no-cache' }).then((fresh) => {
413+
if (fresh && fresh.status === 200) {
414+
return Promise.all([cache.put('./index.html', fresh.clone()), cache.put('./', fresh)]);
415+
}
416+
return undefined;
417+
}),
418+
),
419+
);
409420
return Response.error();
410421
}
411422
return networkResponse;

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

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -52,21 +52,31 @@ test.describe('landing page warm-up', () => {
5252
// Driven directly rather than waiting on idle timing, which is unbounded.
5353
await page.evaluate(() => (window as any).__landingPrefetch.warmEverything());
5454

55-
const result = await page.evaluate(async () => {
55+
// Polled, not asserted outright. The page's promise settles when the last
56+
// response has been read; the worker's cache.put runs on its own side and
57+
// finishes shortly after. Demanding both at the same instant made this fail
58+
// in CI on the queue's final file -- true of the assertion, not the feature.
59+
const missing = async () =>
60+
page.evaluate(async () => {
61+
const lp = (window as any).__landingPrefetch;
62+
const wanted: string[] = lp.CORE.concat(...lp.ENGINES.map(lp.formatUrls));
63+
const held = new Set<string>();
64+
for (const name of await caches.keys()) {
65+
const cache = await caches.open(name);
66+
for (const request of await cache.keys()) held.add(new URL(request.url).pathname);
67+
}
68+
return wanted.filter((u) => !held.has(u));
69+
});
70+
71+
await expect.poll(async () => (await missing()).length, { timeout: 120_000 }).toBe(0);
72+
73+
const shape = await page.evaluate(() => {
5674
const lp = (window as any).__landingPrefetch;
57-
const wanted: string[] = lp.CORE.concat(...lp.ENGINES.map(lp.formatUrls));
58-
const held = new Set<string>();
59-
for (const name of await caches.keys()) {
60-
const cache = await caches.open(name);
61-
for (const request of await cache.keys()) held.add(new URL(request.url).pathname);
62-
}
63-
return { engines: lp.ENGINES, missing: wanted.filter((u) => !held.has(u)), total: wanted.length };
75+
return { engines: lp.ENGINES, total: lp.CORE.length + lp.ENGINES.length * lp.formatUrls('docx').length };
6476
});
65-
66-
expect(result.engines, 'all three editors must be warmed').toEqual(['docx', 'xlsx', 'pptx']);
67-
expect(result.missing, `not cached after the warm-up: ${result.missing.join(', ')}`).toEqual([]);
77+
expect(shape.engines, 'all three editors must be warmed').toEqual(['docx', 'xlsx', 'pptx']);
6878
// Sanity: the core (8) plus four files per editor.
69-
expect(result.total).toBe(8 + 3 * 4);
79+
expect(shape.total).toBe(8 + 3 * 4);
7080
});
7181

7282
test('the core assets end up in the service worker cache without any interaction', async ({ page }) => {

‎test/unit/sw-update.test.ts‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -423,6 +423,45 @@ describe('the runtime cache outlives deploys, so it has to be kept honest', () =
423423
});
424424
});
425425

426+
/**
427+
* A cache write has to outlive the response it came from.
428+
*
429+
* `respondWith` settles the moment the response is handed to the page, and a
430+
* worker with no outstanding work may be terminated right there -- taking a
431+
* half-finished `cache.put` with it. The larger the file the wider that window,
432+
* which is exactly backwards: the 13.7 MB SDK bundle is the entry most worth
433+
* keeping. It surfaced as a CI failure where the last file of a warm-up was
434+
* missing from the cache, and it is invisible locally because nothing pressures
435+
* the worker to shut down.
436+
*/
437+
describe('cache writes are held open past the response', () => {
438+
const src = readFileSync(resolve(__dirname, '../../public/sw.js'), 'utf8');
439+
440+
it('every runtime cache write is wrapped in event.waitUntil', () => {
441+
// One call site per caching branch: the vendor cache-first path and the
442+
// stale-while-revalidate path. (The helper's own arrow definition does not
443+
// match, which is what makes this a count of call sites.)
444+
const calls = [...src.matchAll(/putInRuntimeCache\(/g)];
445+
expect(calls.length, 'expected a call site in each caching branch').toBe(2);
446+
447+
const guarded = [...src.matchAll(/event\.waitUntil\(putInRuntimeCache\(/g)];
448+
expect(guarded.length, 'a putInRuntimeCache call is not inside event.waitUntil').toBe(calls.length);
449+
});
450+
451+
it('holds the core-cache writes open too', () => {
452+
// The HTML branch had the same shape. Smaller files, same failure mode.
453+
const fetchHandler = src.slice(src.indexOf("addEventListener('fetch'"));
454+
const coreWrites = [...fetchHandler.matchAll(/caches\.open\(CORE_CACHE\)/g)];
455+
expect(coreWrites.length, 'expected the network-first and 404-recovery writes').toBe(2);
456+
for (const match of coreWrites) {
457+
const before = fetchHandler.slice(Math.max(0, match.index - 200), match.index);
458+
expect(before, 'a CORE_CACHE write in the fetch handler is not inside event.waitUntil').toMatch(
459+
/event\.waitUntil\(\s*$/,
460+
);
461+
}
462+
});
463+
});
464+
426465
describe('bin/build.sh stamps both versions into sw.js', () => {
427466
const script = readFileSync(resolve(__dirname, '../../bin/build.sh'), 'utf8');
428467

0 commit comments

Comments
 (0)