Skip to content

Cache HTTP responses based on RFC 9111 cacheable status codes #407

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kriszyp merged 3 commits into from
Apr 29, 2026
+53 −17
Merged

Cache HTTP responses based on RFC 9111 cacheable status codes #407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b7d3965
Cache HTTP responses based on RFC 9111 cacheable status codes
kriszyp Apr 24, 2026
5b3fdbd
I don't think we return server-timing header in 500 responses
kriszyp Apr 25, 2026
bff1c2b
Verify 500 responses hit source twice via call counter
kriszyp Apr 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 12 additions & 17 deletions resources/Table.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ const NULL_WITH_TIMESTAMP = new Uint8Array(9);
NULL_WITH_TIMESTAMP[8] = 0xc0; // null
const UNCACHEABLE_TIMESTAMP = Infinity; // we use this when dynamic content is accessed that we can't safely cache, and this prevents earlier timestamps from change the "last" modification
const RECORD_PRUNING_INTERVAL = 60000; // one minute
const CACHEABLE_STATUS_CODES = new Set([200, 203, 204, 206, 300, 301, 308, 404, 405, 410, 414, 501]);
Copy link
Copy Markdown
Contributor

@claude claude Bot Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The set correctly mirrors the RFC 9111 §4.2.2 heuristically-cacheable list, but this is a silent breaking change for source functions that currently return Response objects with 2xx codes not in the set — specifically 201, 202, 205, 207, 208, 226.

Before this PR, every status < 300 fell through to the caching branch. After it, any 2xx code absent from CACHEABLE_STATUS_CODES (201, 202, 205, 207, …) hits the !CACHEABLE_STATUS_CODES.has(status) branch and throws ServerError. A source returning 201 Created would now propagate an error to the client instead of caching the response.

RFC 9111 is the right authority here, but the change is undocumented in the PR body and could break integrators. Worth a note in the PR description (or a CHANGELOG entry) so it's visible in release notes.

envMngr.initSync();
const LMDB_PREFETCH_WRITES = envMngr.get(CONFIG_PARAMS.STORAGE_PREFETCHWRITES);
const LOCK_TIMEOUT = 10000;
Expand Down Expand Up @@ -4038,15 +4039,14 @@ export function makeTable(options) {
if (typeof updatedRecord !== 'object') throw new Error('Only objects can be cached and stored in tables');
if (updatedRecord.status > 0 && updatedRecord.headers) {
// if the source has a status code and headers, treat it as a response
if (updatedRecord.status >= 300) {
if (updatedRecord.status === 304) {
// revalidation of our current cached record
updatedRecord = existingRecord;
version = existingVersion;
} else {
// if the source has an error status, we need to throw an error
throw new ServerError(updatedRecord.body || 'Error from source', updatedRecord.status);
} // there are definitely more status codes to handle
const status = updatedRecord.status;
if (status === 304) {
// revalidation of our current cached record
updatedRecord = existingRecord;
version = existingVersion;
} else if (!CACHEABLE_STATUS_CODES.has(status)) {
// non-cacheable status - propagate to client without caching
throw new ServerError(updatedRecord.body || 'Error from source', status);
} else {
let headers: any;
const sourceHeaders = updatedRecord.headers;
Expand Down Expand Up @@ -4074,16 +4074,11 @@ export function makeTable(options) {
if (data !== undefined) {
// we have structured data that we have parsed
delete headers['content-type']; // don't store the content type if we have already parsed it
updatedRecord = {
headers,
data,
};
updatedRecord = { headers, data };
} else {
updatedRecord = {
headers,
body: createBlob(updatedRecord.body),
};
updatedRecord = { headers, body: createBlob(updatedRecord.body) };
}
if (status !== 200) updatedRecord.status = status;
}
}
if (typeof updatedRecord.toJSON === 'function') updatedRecord = updatedRecord.toJSON();
Expand Down
35 changes: 35 additions & 0 deletions unitTests/apiTests/basicREST-test.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,41 @@ describe('test REST calls', () => {
req.end(body);
});
});
describe('HTTP response status code caching', function () {
it('caches a cacheable 404 response and returns it with 404 status', async () => {
const response = await axios.get('http://localhost:9926/CacheOfHttp/not-found', {
validateStatus: () => true,
});
assert.equal(response.status, 404);
// second request should also return 404 (served from cache)
const response2 = await axios.get('http://localhost:9926/CacheOfHttp/not-found', {
validateStatus: () => true,
});
assert.equal(response2.status, 404);
assert(!response2.headers['server-timing'].includes('miss'));
});
it('does not cache a non-cacheable 500 response', async () => {
const callsBefore = tables.CacheOfHttp.serverErrorCalls;
const response = await axios.get('http://localhost:9926/CacheOfHttp/server-error', {
validateStatus: () => true,
});
assert.equal(response.status, 500);
// second request should also hit source (not cached)
const response2 = await axios.get('http://localhost:9926/CacheOfHttp/server-error', {
validateStatus: () => true,
});
assert.equal(response2.status, 500);
Copy link
Copy Markdown
Contributor

@claude claude Bot Apr 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test claims to verify non-caching but only asserts the status code, which would be 500 whether the response was cached or not.

A previous version of this test (commit 5b3fdbdfc) had assert(response2.headers['server-timing'].includes('miss')) here, but it was removed because 500 responses apparently don't carry a Server-Timing header. That leaves the core claim — that the source is called again rather than served from cache — completely unverified.

The 404 test uses server-timing to confirm a cache hit; this test should use an equivalent mechanism to confirm a cache miss. One reliable approach is a call counter on the fixture source function:

it('does not cache a non-cacheable 500 response', async () => {
    let callCount = 0;
    const origSource = tables.CacheOfHttp._source; // or however the source is accessible
    // wrap / intercept to count calls, then restore in after()

    const response = await axios.get('http://localhost:9926/CacheOfHttp/server-error', { validateStatus: () => true });
    assert.equal(response.status, 500);
    const response2 = await axios.get('http://localhost:9926/CacheOfHttp/server-error', { validateStatus: () => true });
    assert.equal(response2.status, 500);
    assert.equal(callCount, 2, 'source should be called for every request — 500 must not be cached');
});

Without a check like this, a regression that cached 500 responses would silently pass this test.

assert.equal(tables.CacheOfHttp.serverErrorCalls, callsBefore + 2);
});
it('does not store status in cached record for 200 responses', async () => {
// The CacheOfHttp 'created-response' source returns a 200 with a custom header
// If status 200 were stored, it would appear as a field in the raw record
Copy link
Copy Markdown
Contributor

@claude claude Bot Apr 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: The test name and comment claim to verify that status 200 is not stored in the cached record, but the assertions only check client-visible behaviour (response.status === 200, x-custom-header value). Those assertions would pass identically whether status 200 is stored or omitted. The internal-representation invariant isn't observable from HTTP responses alone. The test is still useful as a regression guard for 200 responses, but the description slightly over-sells what it proves.

// We verify that the 200 response is served correctly without redundantly caching status
const response = await axios.get('http://localhost:9926/CacheOfHttp/created-response');
assert.equal(response.status, 200);
assert.equal(response.headers.get('x-custom-header'), 'custom value');
});
});
it('post with custom response', async () => {
const response = await axios.post('http://localhost:9926/SimpleCache/35555', {
customResponse: true,
Expand Down
6 changes: 6 additions & 0 deletions unitTests/testApp/resources.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,7 @@ tables.CacheOfResource.sourcedFrom({
});

// test transparent HTTP caching straight through
tables.CacheOfHttp.serverErrorCalls = 0;
tables.CacheOfHttp.sourcedFrom({
async get(target) {
switch (target) {
Expand All @@ -194,6 +195,11 @@ tables.CacheOfHttp.sourcedFrom({
headers: { 'x-custom-header': 'custom value' },
name: 'test-sibling-to-headers',
};
case 'not-found':
return new Response('Not Found', { status: 404 });
case 'server-error':
tables.CacheOfHttp.serverErrorCalls++;
return new Response('Internal Server Error', { status: 500 });
}
},
});
Expand Down
Loading