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

Author: vaibhav8a

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<DateTime<Utc>> {
+        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::<Utc>::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<Utc>) -> 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::<Utc>::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<AnyStatement<'static>> {
         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<AnyStatement<'static>> {
+        let last_modified_query = format!(
+            "SELECT last_modified from sqlpage_files WHERE path = {}",
+            make_placeholder(db.info.kind, 1),
+        );
+        let param_types: &[AnyTypeInfo; 1] = &[<str as Type<Postgres>>::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<AnyStatement<'static>> {
         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<Option<DateTime<Utc>>> {
+        let query = self
+            .last_modified
+            .query_as::<(DateTime<Utc>,)>()
+            .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,

src/webserver/http.rs

@@ -455,21 +455,31 @@ 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)
         .await
         .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)
         })
 }