page.expires is an absolute time: the normal path seeds it from
page.modified and adds the relevant TTL in seconds. The error page
instead assigns cache-dynamic-ttl directly, which is a count of minutes
and carries no base, so every error page cgit has produced since
"ui-shared: cache errors for dynamic TTL" reports
Expires: Thu, 01 Jan 1970 00:00:05 GMT
five seconds past the epoch, which is the value of the default TTL read
as seconds. The intent of that change was to stop errors being cached
forever; the effect is that they are never cached at all, since every
downstream cache is told the response was already stale.
Seed the value from page.modified and scale the TTL to seconds, matching
what calc_ttl() does for ordinary pages. prepare_context() sets
page.modified before anything can produce an error page, so the base is
always available.
Add a test. The header can be checked without any date arithmetic: a TTL
of zero has to reproduce Last-Modified exactly, and a non-zero one has
to differ from it without falling back to the epoch.
Fixes: c5975ae ("ui-shared: cache errors for "dynamic TTL"")
Assisted-by: LLM [analysis, codegen, tests]
Signed-off-by: Konstantin Ryabitsev <[email protected]>
---
I found this while looking at something else, and it looks like it has
been there since errors were first given the dynamic TTL, and the change
that introduced it was trying to do the opposite of what it ended up
doing. Its reasoning was that errors are potentially transient and so
should not be cached forever; the implementation tells every downstream
cache the response expired in 1970, so they are not cached at all.
Nothing breaks as a result, which is presumably why it went unnoticed: a
response that claims to be stale is refetched, so the failure mode is
an immediate cache miss rather than wrong content. On a site where a
crawler is walking a large namespace and collecting 404s, though, that
is a lot of requests that a proxy/CDN could have served from cache.
The test asserts the header without doing any date arithmetic, which
would otherwise mean either parsing HTTP dates in shell or assuming a
particular date(1). A zero TTL has to reproduce Last-Modified exactly,
and a non-zero one has to differ from it without landing on the epoch.
Both assertions fail before the fix and pass after it.
---
tests/t0022-error-page-headers.sh | 37 +++++++++++++++++++++++++++++++++++++
ui-shared.c | 2 +-
2 files changed, 38 insertions(+), 1 deletion(-)
diff --git a/tests/t0022-error-page-headers.sh
b/tests/t0022-error-page-headers.sh
new file mode 100755
index 0000000..51fbad8
--- /dev/null
+++ b/tests/t0022-error-page-headers.sh
@@ -0,0 +1,37 @@
+#!/bin/sh
+
+test_description='Check the Expires header on error pages'
+. ./setup.sh
+
+# cgit_vprint_error_page() derives page.expires from cache-dynamic-ttl, which
+# is a count of minutes, while page.expires is an absolute time. Checking the
+# result needs no date arithmetic: a zero TTL has to reproduce Last-Modified
+# exactly, and a non-zero one has to land somewhere else without falling back
+# to the epoch.
+
+header_value() {
+ sed -n "s/^$1: //p" "$2" | tr -d '\r'
+}
+
+test_expect_success 'a non-zero dynamic TTL expires the error page later' '
+ cgit_url "bar/commit/&id=deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"
>output &&
+ grep -q "Status: 404 Not found" output &&
+ modified=$(header_value Last-Modified output) &&
+ expires=$(header_value Expires output) &&
+ test -n "$modified" &&
+ test -n "$expires" &&
+ test "$modified" != "$expires" &&
+ case "$expires" in *1970*) return 1 ;; esac
+'
+
+test_expect_success 'a zero dynamic TTL expires the error page immediately' '
+ rm -rf cache && mkdir -p cache &&
+ printf "cache-dynamic-ttl=0\n" >>cgitrc &&
+ cgit_url "bar/commit/&id=deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"
>output &&
+ modified=$(header_value Last-Modified output) &&
+ expires=$(header_value Expires output) &&
+ test -n "$modified" &&
+ test "$modified" = "$expires"
+'
+
+test_done
diff --git a/ui-shared.c b/ui-shared.c
index df52a9b..44794d8 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -896,7 +896,7 @@ void cgit_print_error_page(int code, const char *msg, const
char *fmt, ...)
void cgit_vprint_error_page(int code, const char *msg, const char *fmt,
va_list ap)
{
- ctx.page.expires = ctx.cfg.cache_dynamic_ttl;
+ ctx.page.expires = ctx.page.modified + ctx.cfg.cache_dynamic_ttl * 60;
ctx.page.status = code;
ctx.page.statusmsg = msg;
cgit_print_layout_start();
---
base-commit: 044821677c774cd24f25f1818ea51d09cc64b006
change-id: 20260825-fix-error-page-expires-2c9f3970582e
Best regards,
--
Konstantin Ryabitsev <[email protected]>
