feat: allow setting stale-while-revalidate and stale-if-error headers

[itslenny]

What kind of change does this PR introduce?

feature

What is the current behavior?

A stale-while-revalidate header is only added for mismatched etag and stale-if-error is never set

What is the new behavior?

• Set stale-while-revalidate and stale-if-error headers based on env config
• For mismatched etags use the configured value if set (otherwise fallback to existing behavior - 30s hard coded)
• Correctly merge and de-dupe cache control directives

This PR should not change any behavior unless RESPONSE_STALE_WHILE_REVALIDATE or RESPONSE_STALE_IF_ERROR are set.

[coveralls]

Coverage Report for CI Build 35236007126

Warning

Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes.
Quick fix: rebase this PR. Learn more →

Coverage increased (+0.1%) to 83.172%

Details

• Coverage increased (+0.1%) from the base build.
• Patch coverage: 54 of 54 lines across 3 files are fully covered (100%).
• No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	14167
Covered Lines:	12225
Line Coverage:	86.29%
Relevant Branches:	8658
Covered Branches:	6759
Branch Coverage:	78.07%
Branches in Coverage %:	Yes
Coverage Strength:	3056.13 hits per line

---

💛 - Coveralls

[claude]

I reviewed this PR and didn't find any bugs. Because it introduces new RFC 9111 quoted-string-aware parsing/merge logic for Cache-Control headers and changes which side (object metadata vs. new env config) wins when directives collide, a human look would still be worthwhile.

What was reviewed: the new splitCacheControlDirectives/mergeCacheControlDirectives module (quoted-string, escaped-quote, and unterminated-quote edge cases — all covered by new unit tests); the refactored setCacheControlHeader-related logic in renderer.ts and head.ts that now conditionally builds s-maxage/stale-while-revalidate/stale-if-error/must-revalidate; and the new RESPONSE_STALE_WHILE_REVALIDATE/RESPONSE_STALE_IF_ERROR config plumbing. Checked that base (object) Cache-Control directives always take precedence over the new admin-configured directives in mergeCacheControlDirectives — this is by design (dedup skips additions whose name already exists in base) rather than a bug, but it does mean an uploader-supplied cache-control header can suppress the operator's stale-while-revalidate/stale-if-error settings.

Extended reasoning...

Overview

This PR adds a new src/storage/renderer/cache-control.ts module implementing RFC 9111/9110-aware Cache-Control directive splitting and merging, wires two new env-configurable directives (stale-while-revalidate, stale-if-error) through src/config.ts/.env.sample, and refactors renderer.ts/head.ts to build Cache-Control headers by merging computed directives with the object's base metadata.cacheControl instead of simply pushing onto an array. Test coverage is substantial: a dedicated 86-line unit test file for the merge/split logic (covering quoted values, escaped quotes, unterminated quotes, case-insensitivity, and empty-element handling) plus 332 new lines of renderer-level tests exercising the new config-driven directives.

Security risks

No injection, auth, or data-exposure risk was found. The change only affects the Cache-Control response header's content and precedence rules; it does not touch access control or object retrieval. One behavioral nuance worth flagging for human awareness: mergeCacheControlDirectives always lets the object's own (upload-time-controlled) cache-control metadata win over the newly added operator-configured stale-while-revalidate/stale-if-error directives, since additions are skipped when their directive name already appears in the base. This was investigated as a candidate issue and ruled out as intentional dedup design (matching the documented function contract and test expectations), not a functional bug, but it is a precedence choice a maintainer may want to confirm matches their intent for the new operator-level knobs.

Level of scrutiny

This lands in the "moderate" tier: it's not touching auth/crypto/permissions, but it is new parsing logic with several non-obvious edge cases (quoted strings, escaped quotes, unterminated quotes) sitting in a response-header code path used on every asset/image request. The presence of four related candidate issues raised during automated bug hunting (all around merge precedence and quote-parsing edge cases, all ultimately ruled out) is itself a signal that this code has enough subtlety to warrant a second pair of eyes, even though the current test suite appears to correctly pin the chosen behavior.

Other factors

Test coverage is strong and specifically targets the tricky parts of the new logic (quoted commas, backslash escapes, unterminated quotes, case-insensitive directive names). The config additions (RESPONSE_STALE_WHILE_REVALIDATE, RESPONSE_STALE_IF_ERROR) follow the existing envNonNegativeInteger pattern used elsewhere in config.ts. No CLAUDE.md conventions apply. Given zero reported bugs but genuine parsing/precedence complexity, I'm deferring rather than approving outright so a human can confirm the base-wins-over-config precedence is the desired behavior.

[depthfirst-app]

🟡 Severity: MEDIUM

With RESPONSE_STALE_WHILE_REVALIDATE enabled, this unconditionally adds stale serving to responses from the authenticated object route, including objects whose bucket has changed from public to private. A shared cache can return previously public bytes during revalidation, bypassing the route’s current authorization check and exposing newly private content.
Helpful? Add 👍 / 👎

💡 Fix Suggestion

Suggestion: The stale-while-revalidate directive should not be unconditionally emitted for all responses. For authenticated routes (or routes where a bucket can transition from public to private), a shared cache honoring this directive will serve stale bytes to any requester—including now-unauthorized ones—during the revalidation window. Two complementary mitigations are recommended:

1. Immediate targeted fix (same location): Gate stale-while-revalidate on s-maxage also being set (i.e., etag && this.sMaxAge > 0). Since s-maxage is the directive that tells shared caches they may store and re-serve the response, tying stale-while-revalidate to its presence prevents the stale-serving window from opening on responses that are not already targeted at shared-cache storage.

2. Full mitigation (architectural): Pass the route's visibility context (public vs. authenticated) into the renderer (e.g., via RenderOptions) and only emit stale-while-revalidate (and s-maxage) for genuinely public routes. For authenticated/private routes, emit private or no-store to prevent shared caches from ever storing the response.

⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.

Suggested change

	if (responseStaleWhileRevalidate > 0) {
	directives.push(`stale-while-revalidate=${responseStaleWhileRevalidate}`)
	if (responseStaleWhileRevalidate > 0 && etag && this.sMaxAge > 0) {
	directives.push(`stale-while-revalidate=${responseStaleWhileRevalidate}`)