Improve Codebase :: Structural Issues

[81reap]

I have started on this path with #1382 but I thought it would be worth creating an issue to track all of the changes I'm thinking about that will come after this. This audit was done with the help of Claude.

D1. FileCache::get holds a lock across a database round trip

file_cache.rs:118:

if let Some(cached) = self.cache.read().await.get(path) {
    ...
    match app_state.file_system.modified_since(app_state, access, ...).await {   // :125-129

The read guard is a temporary of the if let scrutinee, so it stays alive for the whole body. The .await at :125-129 therefore runs while the cache's read lock is held. With the database-backed filesystem that await is a full SQL round trip (filesystem.rs:349-383).

tokio::sync::RwLock is write-fair, so a waiting writer (:187, :199) blocks every subsequent reader behind it. And cache_stale_duration_ms defaults to 0 outside production (app_config.rs:419-421), so in development every request takes this branch.

The comment at :147 reads // Read lock is released, which is true of that point in the function but not of where the I/O happens.

Suggested fix. Store Arc<Cached<T>> in the map. get can then clone the entry and drop the guard in one statement. last_checked_at is already an AtomicU64 shared through the Arc, so the existing freshness update keeps working.

D2. A new HTTP client is built for every sqlpage.fetch() call

fetch.rs:69 calls make_http_client(...) per fetch and discards it when the future completes, so no
keep-alive connection is ever reused. With system_root_ca_certificates enabled, http_client.rs:71-75 also clones the entire root certificate store and rebuilds the rustls configuration each time.

There are four client construction sites in the codebase, three deriving the setting from AppConfig and one (telemetry) from the environment.

awc::Client is !Send, which is presumably why it is not stored on AppState. The natural sharing unit is therefore the thread. A thread-local memo keyed on the system_root_ca_certificates boolean would mirror the process-wide NATIVE_CERTIFICATES: OnceLock that already exists one level down.

D3. The served-asset list is restated in five places

Adding or renaming an embedded asset requires edits in five files, and two of the five failure modes are runtime errors rather than compile errors:

Place	What it holds	If you forget it
build.rs:10-16	the asset names	build fails loudly
static_content.rs:34-57	one function per asset, plus the MIME type	compile error
utils.rs:28-34	the .filename.txt sidecar convention	compile error
http.rs:578-582	five .service() registrations	404 at runtime
template_helpers.rs:180-184	five match arms	"unknown static file" at render time

build.rs already generates icons.rs and includes it via include! (template_helpers.rs:213), so the
generator pattern exists in the same file. Widening the asset list to carry the MIME type and emitting a table would collapse the five restatements into one.

We would suggest doing A3 in #1385 (the hashing fix) and adding one asset-endpoint test first.

D4. Span-field rendering exists twice, and the tests cover the copy

telemetry.rs:647-687 renders span fields for log output and is called from on_event at :520.
telemetry.rs:693-727 is a #[cfg(test)] function that duplicates the same dedup rules.

Both tests (:822, :839) call the test copy. The production function has no test coverage.

The fork exists because the production function takes a registry::Scope, which is only obtainable from a live subscriber, while the fields it needs are plain HashMaps. A small accumulator with a push(&HashMap) method called inside the loop would let both drive the same logic, and the existing assertions would then cover the code that actually runs.

D5. The Accept header is parsed twice per 404, by two rules that disagree

ResponseFormat::from_accept_header (http.rs:64-83) bakes its HTML default in at parse time, so it cannot express "the client stated no preference". main_handler needs exactly that distinction for 404s, so it parses the header a second time at :505 and applies its own predicate at :507.

One consequence looks intentional and one does not:

• Accept: */* → the second predicate says "not HTML", so these clients get the plain-text 404. We traced this to commit 5ade60fa ("Return plain text 404 for non-HTML requests") and believe it is deliberate. Any change here should preserve it.
• Accept: application/json, text/html → the second predicate says "HTML", so _default_404.sql runs; but render_sql (:221-223) independently picks the JSON renderer. The 404 page is executed as HTML and rendered as JSON.

Negotiating once and passing the result down would remove the second predicate while keeping the */*
behaviour explicit rather than emergent.

D6. Server-Timing is missing on the json and csv response paths

PageContext::Body is constructed at three places. Two of them call add_server_timing_header first
(render.rs:382, :396); the json streaming path (:301) and the csv path (:320) do not.

The header is suppressed in production, so this only affects development. tests/server_timing/ covers HTML pages and redirects, never json or csv, which is why it went unnoticed.

A single private finish_headers() that adds the header and hands over the builder would make the three paths agree by construction.

D7. preregister_static_templates compiles every file in sqlpage/templates/, not just templates

templates.rs:97 iterates STATIC_TEMPLATES.files() with no extension filter and compiles each one at :107, propagating errors up through AllTemplates::init to lib.rs:122.

sqlpage/templates/README.md is therefore compiled as a Handlebars template at startup. It is harmless today, but a Handlebars syntax error in that README would make the server refuse to start.

The resulting entry is also permanently unreachable, because every lookup goes through template_path
(:114-121), which always sets the extension to handlebars.

For contrast, the migration loader does filter: sqlx-core-oldapi-0.6.56/src/migrate/source.rs:41-42 skips files that do not match its naming pattern, which is why sqlpage/migrations/README.md is harmless.

A three-line extension guard would make the directory's contents and the component namespace the same set.

D8. Crate packaging ships a working directory and omits the licence

Cargo.toml:11:

include = ["/src", "/README.md", "/build.rs", "/sqlpage", "/frontend/dist"]

Cargo.toml:7 declares license = "MIT", and there is no license-file key. Cargo auto-includes only
Cargo.toml, Cargo.lock, the readme and the licence-file. LICENSE.txt exists at the repository root but is neither listed nor auto-included, so the published crate contains no copy of the MIT licence text.

Meanwhile /sqlpage ships more than the templates the build needs. The only build-time consumer under that path is include_dir!(".../sqlpage/templates") at templates.rs:81. The other tracked files there are:

• sqlpage/sqlpage.db — a 4096-byte empty SQLite file, tracked in git despite .gitignore listing it
• sqlpage/sqlpage.json — a local development configuration
• sqlpage/private_cache_bypass_test.sql — a test fixture, while tests/ itself is not included

Naming the actual build inputs would fix both:

include = ["/src", "/build.rs", "/frontend/dist", "/sqlpage/templates", "/README.md", "/LICENSE.txt"]

Relocating the tracked .db is a separate and riskier change and is not needed for the licence fix.