Skip to content

fix(cloudflare): a failed catalog load is cached for the isolate's lifetime #246

Description

@BlackHole1

Summary

One transient failure while loading the catalog is memoized. The isolate then serves errors for every later request, even after the assets binding recovers.

This is pre-existing on main, not introduced by #243. Filing it separately because #243 changed boot from 1 asset fetch to 1 + N.

The code

loadCatalogOnce memoizes with ??=:

// src/server/cloudflare.ts:121
function loadCatalogOnce(assets: AssetsBinding): Promise<CatalogStore> {
  catalogPromise ??= loadCatalogFromAssets(assets, {
    executableServices: Object.keys(executorModules),
  });
  return catalogPromise;
}

??= assigns only when the slot is undefined. A rejected promise is not undefined, so the failure is what gets cached.

There is a second layer with the same shape:

// src/server/cloudflare.ts:35
if (!cachedApp || cachedApp.key !== cacheKey) {
  cachedApp = { key: cacheKey, app: createCloudflareApp(env, publicOrigin) };
}

const { app } = await cachedApp.app;

cachedApp is replaced only when cacheKey changes. With a stable env the rejected app promise is reused too.

Reproduction

The assets binding fails once, then serves correctly:

let fail = true;
const assets: AssetsBinding = {
  async fetch(request) {
    if (fail) return new Response("upstream", { status: 500 });
    // ...serve /catalog/index.json + /catalog/apps-0000.json
  },
};

await expect(worker.fetch(req(), env, ctx)).rejects.toThrow("returned 500");

fail = false;
const direct = await assets.fetch(new Request("https://assets.local/catalog/index.json"));
expect(direct.status).toBe(200); // the binding is healthy again

await expect(worker.fetch(req(), env, ctx)).rejects.toThrow("returned 500"); // still poisoned

All three assertions pass on the current main and on the #243 branch.

Impact

A Workers isolate is reused across many requests. One asset hiccup turns into sustained failures until that isolate is recycled, with no self-healing in between.

Suggested fix

Clear the slot on rejection so the next request retries:

function loadCatalogOnce(assets: AssetsBinding): Promise<CatalogStore> {
  catalogPromise ??= loadCatalogFromAssets(assets, {
    executableServices: Object.keys(executorModules),
  }).catch((error: unknown) => {
    catalogPromise = undefined;
    throw error;
  });
  return catalogPromise;
}

Note that fixing catalogPromise alone is not enough. cachedApp holds its own rejected promise and needs the same treatment, otherwise the retry never reaches loadCatalogOnce.

A regression test belongs in src/server/cloudflare.test.ts, driving worker.fetch twice against a binding that fails once.

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions