From a42e0c8725cb09ea7f5a6fea462439eaefaa5336 Mon Sep 17 00:00:00 2001 From: Vaibhav Srivastava Date: Wed, 2 Sep 2026 22:01:13 +0530 Subject: [PATCH] fix(http): Last-Modified is the file's mtime, not now() Every static file was served with Last-Modified = SystemTime::now(), so the header advanced by a second between two requests for a file that had not changed. A client doing If-Modified-Since revalidation could never get a meaningful answer and no cache could key on it. FileSystem::modified_at reads the real mtime with the same local-then-database fallback read_file uses, so the timestamp describes the file actually served. DbFsQueries gains a prepared statement for the last_modified column, mirroring the existing was_modified/read_file/exists trio. The header is OMITTED when no mtime is available rather than filled with a guess: a header that says "now" is worse than an absent one, because a client believes it. Both filesystems now compare with `>`. They disagreed - local `>`, DB `>=` - so the same request answered differently depending only on where the file was stored. `>` is the correct half rather than merely the chosen one: If-Modified-Since: T asks whether the file changed AFTER T, so an mtime of exactly T is not a change and must answer 304. I had this as `>=` first and driving the repro caught it - every revalidation re-sent the whole body, which defeats the header this change exists to fix. Verified against the issue's reproduction: mtime reported correctly, stable across requests, and 304/200/304 for If-Modified-Since equal to / older than / newer than the mtime. Fixes #1323 --- src/filesystem.rs | 99 ++++++++++++++++++++++++++++++++++++++++++- src/webserver/http.rs | 26 ++++++++---- 2 files changed, 116 insertions(+), 9 deletions(-) diff --git a/src/filesystem.rs b/src/filesystem.rs index 71c8fba7..9061b72c 100644 --- a/src/filesystem.rs +++ b/src/filesystem.rs @@ -98,6 +98,50 @@ impl FileSystem { } } + /// When the file was last modified, or `None` when nothing can say. + /// + /// Same local-then-database fallback as `modified_since`, because a file served from + /// one must report the mtime of that same one. Used for the `Last-Modified` header, + /// which was previously `SystemTime::now()` - a value that changed on every request + /// and so carried no cache-validation information at all. + /// + /// `None` rather than a guess when the mtime is unavailable: the caller omits the + /// header instead, which is honest. A header that says "now" is worse than no header, + /// because a client believes it. + pub(crate) async fn modified_at( + &self, + app_state: &AppState, + access: FileAccess<'_>, + ) -> Option> { + let path = access.path(); + let local_path = self.safe_local_path(app_state, access); + match tokio::fs::metadata(&local_path) + .await + .and_then(|m| m.modified()) + { + Ok(modified) => return Some(DateTime::::from(modified)), + Err(e) if !is_path_missing_error(&e) => { + log::debug!( + "Unable to read the modification time of {}: {e}", + local_path.display() + ); + return None; + } + Err(_) => {} + } + let db_fs = self.db_fs_queries.as_ref()?; + match db_fs.last_modified_in_db(app_state, path.as_ref()).await { + Ok(modified) => modified, + Err(e) => { + log::debug!( + "Unable to read the modification time of {} from the database: {e:#}", + path.display() + ); + None + } + } + } + pub(crate) async fn read_to_string( &self, app_state: &AppState, @@ -271,11 +315,23 @@ async fn file_modified_since_local(path: &Path, since: DateTime) -> tokio:: tokio::fs::metadata(path) .await .and_then(|m| m.modified()) + // Strictly `>`, and the database query below now matches. + // + // The two disagreed - local used `>`, the DB `>=` - so a file whose mtime equalled + // the client's `If-Modified-Since` was "not modified" from disk and "modified" + // from the database. Conditional requests must not depend on where a file is + // stored. + // + // `>` is the correct half, not merely the chosen one: `If-Modified-Since: T` asks + // whether the file changed AFTER T, so an mtime of exactly T is not a change and + // must answer 304. Under `>=` every revalidation re-sends the whole body, which + // defeats the header entirely. .map(|modified_at| DateTime::::from(modified_at) > since) } pub struct DbFsQueries { was_modified: AnyStatement<'static>, + last_modified: AnyStatement<'static>, read_file: AnyStatement<'static>, exists: AnyStatement<'static>, } @@ -304,6 +360,7 @@ impl DbFsQueries { Self::check_table_available(db).await?; Ok(Self { was_modified: Self::make_was_modified_query(db).await?, + last_modified: Self::make_last_modified_query(db).await?, read_file: Self::make_read_file_query(db).await?, exists: Self::make_exists_query(db).await?, }) @@ -320,7 +377,11 @@ impl DbFsQueries { async fn make_was_modified_query(db: &Database) -> anyhow::Result> { let was_modified_query = format!( - "SELECT 1 from sqlpage_files WHERE last_modified >= {} AND path = {}", + // `>` not `>=`: an mtime equal to the client's If-Modified-Since is not a + // change since that moment, and must revalidate as 304. This matches + // `file_modified_since_local`, which the same request would otherwise + // answer differently depending only on where the file is stored. + "SELECT 1 from sqlpage_files WHERE last_modified > {} AND path = {}", make_placeholder(db.info.kind, 1), make_placeholder(db.info.kind, 2) ); @@ -332,6 +393,16 @@ impl DbFsQueries { db.prepare_with(&was_modified_query, param_types).await } + async fn make_last_modified_query(db: &Database) -> anyhow::Result> { + let last_modified_query = format!( + "SELECT last_modified from sqlpage_files WHERE path = {}", + make_placeholder(db.info.kind, 1), + ); + let param_types: &[AnyTypeInfo; 1] = &[>::type_info().into()]; + log::debug!("Preparing the database filesystem last_modified_query: {last_modified_query}"); + db.prepare_with(&last_modified_query, param_types).await + } + async fn make_read_file_query(db: &Database) -> anyhow::Result> { let read_file_query = format!( "SELECT contents from sqlpage_files WHERE path = {}", @@ -351,6 +422,32 @@ impl DbFsQueries { db.prepare_with(&exists_query, param_types).await } + async fn last_modified_in_db( + &self, + app_state: &AppState, + path: &Path, + ) -> anyhow::Result>> { + let query = self + .last_modified + .query_as::<(DateTime,)>() + .bind(path.display().to_string()); + log::trace!( + "Reading the modification time of {} by executing query: \n{}", + path.display(), + self.last_modified.sql() + ); + let row = query + .fetch_optional(&app_state.db.connection) + .await + .with_context(|| { + format!( + "Unable to read the modification time of {} from the database", + path.display() + ) + })?; + Ok(row.map(|(modified,)| modified)) + } + async fn file_modified_since_in_db( &self, app_state: &AppState, diff --git a/src/webserver/http.rs b/src/webserver/http.rs index 88e67ae9..56a3c4a8 100644 --- a/src/webserver/http.rs +++ b/src/webserver/http.rs @@ -455,6 +455,14 @@ async fn serve_file( return Ok(HttpResponse::NotModified().finish()); } } + // The file's real modification time, not `SystemTime::now()`. + // + // `now()` changed on every request, so `Last-Modified` advanced by a second between + // two fetches of a file that had not changed since 2020. A client doing + // `If-Modified-Since` revalidation could never get a meaningful answer, and no cache + // could key on it. Omitted entirely when the mtime is unknown: a header that says + // "now" is worse than an absent one, because a client believes it. + let last_modified = state.file_system.modified_at(state, access).await; state .file_system .read_file(state, access) @@ -462,14 +470,16 @@ async fn serve_file( .with_context(|| format!("Unable to read file {path:?}")) .map_err(|e| anyhow_err_to_actix(e, state)) .map(|b| { - HttpResponse::Ok() - .insert_header( - mime_guess::from_path(path) - .first() - .map_or_else(ContentType::octet_stream, ContentType), - ) - .insert_header(LastModified(HttpDate::from(SystemTime::now()))) - .body(b) + let mut response = HttpResponse::Ok(); + response.insert_header( + mime_guess::from_path(path) + .first() + .map_or_else(ContentType::octet_stream, ContentType), + ); + if let Some(modified) = last_modified { + response.insert_header(LastModified(HttpDate::from(SystemTime::from(modified)))); + } + response.body(b) }) }