Sqlite Cache Backend - #2631
Conversation
| .{trimmed_dir}, | ||
| 0, | ||
| ); | ||
| defer allocator.free(cache_path); |
There was a problem hiding this comment.
What about moving this logic into config directly? We could create something like config.httpCacheSqlitePath() returning an nullable path name? I would remove this code from Network which is not really its place and maybe allow us to have a config flag to customize this path.
| .path = cache_path, | ||
| .err = e, | ||
| }); | ||
| return e; |
There was a problem hiding this comment.
Maybe we could show an error + disable the cache in case of error to avoid process hard stop?
but I'm not sure it's a good thing 🤔
| const body_entry = try conn.row( | ||
| "select data from body where url = $1", | ||
| .{url}, | ||
| ) orelse @panic("valid metadata must have a body"); |
There was a problem hiding this comment.
idk about that one. I would prefer logging an error and return a null body instead.
There was a problem hiding this comment.
what about assert then to get the crash report at least?
There was a problem hiding this comment.
this comment has not been addressed AFAIK
|
|
||
| var lower_name: [256]u8 = undefined; | ||
| for (meta.headers) |h| { | ||
| const name = std.ascii.lowerString(lower_name[0..h.name.len], h.name); |
There was a problem hiding this comment.
Shouldn't we handle h.name.len > 256 ?
| ); | ||
| } | ||
| for (meta.vary_headers) |h| { | ||
| const name = std.ascii.lowerString(lower_name[0..h.name.len], h.name); |
| self.pool.deinit(allocator); | ||
| } | ||
|
|
||
| pub fn Migrations(comptime migrations: []const [:0]const u8) type { |
There was a problem hiding this comment.
Should we keep src/storage/sqlite/migrations.zig?
There was a problem hiding this comment.
I'm not sure I see the reason for this to be a generic, nor what the hash provides. What case does the hash protect against? The DB getting corrupted? The developer (us) making a build-time mistake? Also, it isn't cryptographic, so whatever it does, it only does sometimes.
It might not matter for a small/simple DB schema, but I think migrations should be code, not text. There are some migrations that simply require pulling data into code.
There was a problem hiding this comment.
We no longer track hash and we also support either just SQL or code as the migration.
|
This is nice to be able to use duckdb CLI to explore the cache data :) cache D select * from metadata where must_revalidate != 0;
┌─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┬────────┬────────────┬──────────────┬─────────┬─────────────────┬──────────────────────────────────────┬───────────────────────────────┐
│ url │ status │ stored_at │ age_at_store │ max_age │ must_revalidate │ etag │ last_modified │
│ varchar │ int64 │ int64 │ int64 │ int64 │ int64 │ varchar │ varchar │
├─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┼────────┼────────────┼──────────────┼─────────┼─────────────────┼──────────────────────────────────────┼───────────────────────────────┤
│ https://improving.duckduckgo.com/t/page_home_tagline_impression?2117232&b=unknown&d=d&l=en&p=other │ 200 │ 1782224467 │ 0 │ 0 │ 1 │ NULL │ Mon, 28 Sep 1970 06:00:00 GMT │
│ https://improving.duckduckgo.com/t/page_home_searchbox_ai_impression?3479694&b=unknown&d=d&l=en&p=other&atb=v539-7… │ 200 │ 1782224467 │ 0 │ 0 │ 1 │ NULL │ Mon, 28 Sep 1970 06:00:00 GMT │
│ https://improving.duckduckgo.com/t/page_home_impression?8744613&theme=light&b=unknown&d=d&l=en&p=other&atb=v539-7&… │ 200 │ 1782224467 │ 0 │ 0 │ 1 │ NULL │ Mon, 28 Sep 1970 06:00:00 GMT │
│ https://duckduckgo.com/t.js?q=lightpanda&l=us-en&p=1&s=0&dl=en&ct=FR&bing_market=en-US&p_ent=&dp=xXvfRl5mggeuTWh4h… │ 200 │ 1782224476 │ 0 │ 0 │ 1 │ NULL │ NULL │
│ https://hn.algolia.com/public/main-6e634771f729331a3c3c.js │ 200 │ 1782226454 │ 0 │ 0 │ 1 │ "1755549896.0-2608202-407047307" │ Mon, 18 Aug 2025 20:44:56 GMT │
│ https://js.driftt.com/include/1782224700000/6nmy7z65gsxc.js │ 200 │ 1782224599 │ 0 │ 0 │ 1 │ W/"d45d26b9900d0bece786bd59b23c5006" │ Thu, 26 Mar 2026 08:37:12 GMT │
│ https://config.gorgias.chat/gorgias-chat-bundle-loader.js?applicationId=15954 │ 200 │ 1782226616 │ 3537 │ 0 │ 1 │ W/"71a-vOzeu8cLY+IjFLWstPX9XvL10zs" │ NULL │
│ https://www.googleadservices.com/pagead/conversion/971805850/?random=1782224689183&cv=11&fst=1782224689183&fmt=7&b… │ 200 │ 1782224690 │ 0 │ 0 │ 1 │ NULL │ NULL │
│ https://config.gorgias.chat/bundle-loader/01GYCCC51FF4CA6RW7XJH50SCK?source=shopify1click&shop=kit-and-ace.myshopi… │ 200 │ 1782226638 │ 3557 │ 0 │ 1 │ W/"74d-X+O5AqWK9m1LWrn+UvfhH5u8pSY" │ NULL │
│ https://www.googleadservices.com/pagead/conversion/971805850/?random=1782224718058&cv=11&fst=1782224718058&fmt=7&b… │ 200 │ 1782224719 │ 0 │ 0 │ 1 │ NULL │ NULL │
│ https://www.googleadservices.com/pagead/conversion/971805850/?random=1782224718143&cv=11&fst=1782224718143&fmt=7&b… │ 200 │ 1782224720 │ 0 │ 0 │ 1 │ NULL │ NULL │
│ https://www.googleadservices.com/pagead/conversion/971805850/?random=1782224755238&cv=11&fst=1782224755238&fmt=7&b… │ 200 │ 1782224756 │ 0 │ 0 │ 1 │ NULL │ NULL │
│ https://www.googleadservices.com/pagead/conversion/971805850/?random=1782224755314&cv=11&fst=1782224755314&fmt=7&b… │ 200 │ 1782224756 │ 0 │ 0 │ 1 │ NULL │ NULL │
│ https://improving.duckduckgo.com/t/page_home_tagline_impression?9740447&b=unknown&d=d&l=en&p=other │ 200 │ 1782225829 │ 0 │ 0 │ 1 │ NULL │ Mon, 28 Sep 1970 06:00:00 GMT │
│ https://improving.duckduckgo.com/t/page_home_searchbox_ai_impression?5392774&b=unknown&d=d&l=en&p=other&atb=v539-7… │ 200 │ 1782225829 │ 0 │ 0 │ 1 │ NULL │ Mon, 28 Sep 1970 06:00:00 GMT │
│ https://improving.duckduckgo.com/t/page_home_impression?1190434&theme=light&b=unknown&d=d&l=en&p=other&atb=v539-7&… │ 200 │ 1782225829 │ 0 │ 0 │ 1 │ NULL │ Mon, 28 Sep 1970 06:00:00 GMT │
│ https://improving.duckduckgo.com/t/page_home_tagline_impression?8429873&b=unknown&d=d&l=en&p=other │ 200 │ 1782226412 │ 0 │ 0 │ 1 │ NULL │ Mon, 28 Sep 1970 06:00:00 GMT │
│ https://improving.duckduckgo.com/t/page_home_searchbox_ai_impression?9690011&b=unknown&d=d&l=en&p=other&atb=v539-7… │ 200 │ 1782226412 │ 0 │ 0 │ 1 │ NULL │ Mon, 28 Sep 1970 06:00:00 GMT │
│ https://duckduckgo.com │ 200 │ 1782226421 │ 0 │ 0 │ 1 │ "6a3a9d36-9c21" │ NULL │
│ https://improving.duckduckgo.com/t/page_home_tagline_impression?7683633&b=unknown&d=d&l=en&p=other │ 200 │ 1782226424 │ 0 │ 0 │ 1 │ NULL │ Mon, 28 Sep 1970 06:00:00 GMT │
│ https://improving.duckduckgo.com/t/page_home_searchbox_ai_impression?8002091&b=unknown&d=d&l=en&p=other&atb=v539-7… │ 200 │ 1782226424 │ 0 │ 0 │ 1 │ NULL │ Mon, 28 Sep 1970 06:00:00 GMT │
│ https://improving.duckduckgo.com/t/page_home_impression?585745&theme=light&b=unknown&d=d&l=en&p=other&atb=v539-7&p… │ 200 │ 1782226424 │ 0 │ 0 │ 1 │ NULL │ Mon, 28 Sep 1970 06:00:00 GMT │
│ https://duckduckgo.com/ac/?q=light&kl=wt-wt&vertical=homepage │ 200 │ 1782226427 │ 0 │ 0 │ 1 │ NULL │ NULL │
│ https://duckduckgo.com/ac/?q=lightp&kl=wt-wt&vertical=homepage │ 200 │ 1782226427 │ 0 │ 0 │ 1 │ NULL │ NULL │
│ https://duckduckgo.com/ac/?q=lightpa&kl=wt-wt&vertical=homepage │ 200 │ 1782226428 │ 0 │ 0 │ 1 │ NULL │ NULL │
│ https://duckduckgo.com/ac/?q=lightpan&kl=wt-wt&vertical=homepage │ 200 │ 1782226428 │ 0 │ 0 │ 1 │ NULL │ NULL │
│ https://duckduckgo.com/ac/?q=lightpand&kl=wt-wt&vertical=homepage │ 200 │ 1782226429 │ 0 │ 0 │ 1 │ NULL │ NULL │
│ https://duckduckgo.com/ac/?q=lightpanda&kl=wt-wt&vertical=homepage │ 200 │ 1782226429 │ 0 │ 0 │ 1 │ NULL │ NULL │
│ https://duckduckgo.com/t.js?q=lightpanda&l=us-en&s=0&dl=en&ct=FR&bing_market=en-US&p_ent=&ex=-1&dp=J9gRog2IaKXlQqQ… │ 200 │ 1782226432 │ 0 │ 0 │ 1 │ NULL │ NULL │
│ https://duckduckgo.com/post3.html │ 200 │ 1782226432 │ 0 │ 0 │ 1 │ "6a3a9d69-88" │ NULL │
│ https://www.amazon.com/s/ref=nb_sb_noss?url=search-alias%3Daps&field-keywords=ces+jours+qui+disparaissent │ 200 │ 1782226530 │ 0 │ 0 │ 1 │ NULL │ NULL │
│ https://js.driftt.com/include/1782226800000/6nmy7z65gsxc.js │ 200 │ 1782226539 │ 0 │ 0 │ 1 │ W/"d45d26b9900d0bece786bd59b23c5006" │ Thu, 26 Mar 2026 08:37:12 GMT │
│ https://www.googleadservices.com/pagead/conversion/971805850/?random=1782226623304&cv=11&fst=1782226623304&fmt=7&b… │ 200 │ 1782226624 │ 0 │ 0 │ 1 │ NULL │ NULL │
└─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┴────────┴────────────┴──────────────┴─────────┴─────────────────┴──────────────────────────────────────┴───────────────────────────────┘
33 rows |
|
|
||
| const SqliteCacheMigrations = Migrations(&.{ | ||
| \\ create table metadata ( | ||
| \\ url text not null primary key, |
There was a problem hiding this comment.
I'm wondering if using the same hash as migration as primary key wouldn't be better?
|
|
||
| const cache = if (config.httpCacheDir()) |cache_dir_path| | ||
| Cache{ | ||
| const cache = if (config.httpCacheDir()) |cache_dir_path| blk: { |
There was a problem hiding this comment.
I don't think there should be this much caching logic in Network.zig.
| self.pool.deinit(allocator); | ||
| } | ||
|
|
||
| pub fn Migrations(comptime migrations: []const [:0]const u8) type { |
There was a problem hiding this comment.
I'm not sure I see the reason for this to be a generic, nor what the hash provides. What case does the hash protect against? The DB getting corrupted? The developer (us) making a build-time mistake? Also, it isn't cryptographic, so whatever it does, it only does sometimes.
It might not matter for a small/simple DB schema, but I think migrations should be code, not text. There are some migrations that simply require pulling data into code.
| \\ foreign key (url) references metadata(url) on delete cascade | ||
| \\ ) | ||
| , | ||
| \\ create table header ( |
There was a problem hiding this comment.
what about:
create table cache (
url text not null primary key,
vary bool not null,
response text not null,
body blob not null
)Only url and varyare queried. response can be a JSON representation of the cache. body is extracted from response to avoid the base64 bloating of any binary data.
|
If we want to cleanup the db at some point, for example to reduce its size, do we have an SQL request to do it easily? |
|
I had a crash with integration tests + warmup cache (and a rebased branch) |
8413ec9 to
470f713
Compare
09b854f to
fffa038
Compare
| const body_entry = try conn.row( | ||
| "select data from body where url = $1", | ||
| .{url}, | ||
| ) orelse @panic("valid metadata must have a body"); |
There was a problem hiding this comment.
this comment has not been addressed AFAIK
| \\ name text not null, | ||
| \\ value text not null, | ||
| \\ vary bool not null, | ||
| \\ primary key (url, name), |
There was a problem hiding this comment.
using url + header name as PK is a problem for response containing the same header multiple time.
| const req_headers = try iter.collect(arena); | ||
|
|
||
| const cached = cache.get(arena, .{ | ||
| const cached = try cache.get(arena, .{ |
There was a problem hiding this comment.
You should maybe catch the error and return a missing cache + a log instead of propagating the error.
There was a problem hiding this comment.
We catch where it would be missing and handle that separately within the SqliteCache impll. The error being propagated here would be something going wrong with Sqlite.
There was a problem hiding this comment.
Which still isn't a reason to fail the request
|
|
||
| fn insertMetadata(conn: Conn, meta: CachedMetadata, body: []const u8) !void { | ||
| try conn.exec( | ||
| \\ insert or replace into metadata |
There was a problem hiding this comment.
Claude complaints the FK w/ headers depends on pragma foreign_keys=on conn:
4. put correctness rests on an implicit REPLACE→FK-cascade interaction. insert or replace
into metadata (:173) only clears the old body/header rows if pragma foreign_keys=on took
effect on that connection; otherwise the plain insert into body (:188) hits a PK
conflict. Make it explicit — delete from metadata where url = $1 first — rather than
depending on a subtle corner of SQLite semantics.
|
On an expired cache hit, we pull the meta data (dupe from sqlite), pull the body (dupe from sqlite), then detect that it's expired and free everything. Because it's an arena, this may or may not be freed, so in the end we're 2x peak memory (the expired body we discarded + the new body we fetch). This is just the most extreme example, but there are others: I think there's a few more. There's a hint of more tightly coupled and more efficient code with the early exit around 'vary mismatch', but even this has duped every header by the time it's checked. Aside from that, there's still some unused |
| const req_headers = try iter.collect(arena); | ||
|
|
||
| const cached = cache.get(arena, .{ | ||
| const cached = try cache.get(arena, .{ |
There was a problem hiding this comment.
Which still isn't a reason to fail the request
| const conn = try self.pool.acquire(); | ||
| defer self.pool.release(conn); | ||
|
|
||
| try conn.begin(.immediate); |
There was a problem hiding this comment.
Serializes all all cache access. In renew, it made sense because you always turned it into an write (and, ideally, writes are less common than reads). Here, the write is the edge-case. It's better to commit the expired read, and then issue the write under a distinct transaction. You can guard against another thread updating the row by changing the delete to included stored_at = $2 and passing the read stored_at.
There was a problem hiding this comment.
Just split it back to how it was where get is meant just to get and it now returns an enum where we evict after if needed.
| "select data from body where url = $1", | ||
| .{req.url}, | ||
| ) orelse { | ||
| log.debug(.cache, "miss", .{ .url = req.url, .reason = "missing body " }); |
There was a problem hiding this comment.
But the metadata row remains. This makes my previous comment about using a more concurrent-friendly transaction more complicated to implement (since deleting the metadata here would introduce a 2nd (but still very infrequent, write path). Still possible to clean this up AND have tighter transactions, but not if that end result ends up too spaghetti.
There was a problem hiding this comment.
Merged the body into the other row so they're just colocated together now. This should prevent this issue of one existing while the other doesn't.
a28a026 to
246aa9e
Compare
|
Will fail integration tests until #3044 is merged. |
Huge max-age/Age header values previously either panicked in the i64 casts when storing metadata, or were silently truncated to their first 8 digits by the fixed-size lowercase buffer in CacheControl.parse. Cap them at 2^31 as RFC 9111 §1.2.2 prescribes and compare directives case-insensitively without truncation.
This is a new SQLite backend for the Cache. This is built off of the Cache Revalidation PR (#2570)