HTTP Caching by mookums · Pull Request #1934 · lightpanda-io/browser

mookums · 2026-03-20T08:36:10Z

This adds caching at the HTTP level, allowing the use of a local directory as a RFC 9111 cache.

karlseguin

This needs unit tests. I realize that testing the cache, that uses the file system, is going to be a challenge. But there's just too much code and cases not to have this well covered. We do have unit tests which make http requests to the internal test http server. Need to add routes that return the desired headers. On startup (testing.zig tests:beforeAll) wipe some temp folder and set that as the cache.

karlseguin · 2026-03-24T00:03:39Z

src/network/cache/FsCache.zig

+    self.dir.close();
+}
+
+pub fn cache(self: *FsCache) Cache {


Is this used?

karlseguin · 2026-03-24T00:08:45Z

src/network/cache/FsCache.zig

+const HASHED_TMP_PATH_LEN = HASHED_PATH_LEN + 4;
+
+fn hashKey(key: []const u8) [HASHED_KEY_LEN]u8 {
+    const h = std.hash.Wyhash.hash(0, key);


Wyhash is a general purpose hashing function, you'll get collisions, which could result in the wrong cached page being returned.

karlseguin · 2026-03-24T00:10:50Z

src/network/cache/FsCache.zig

+    return hex;
+}
+
+fn serializeMeta(writer: *std.Io.Writer, meta: *const CachedMetadata) !void {


A lot of writing / parsing code. Did you consider either using zon or, if space is an issue, a json array?

Whatever you do, I'd add a version number as the first field in the header.

karlseguin · 2026-03-24T00:16:16Z

src/network/cache/FsCache.zig

+        meta_file.close();
+    }
+    errdefer self.dir.deleteFile(&meta_tmp_path) catch {};
+    try self.dir.rename(&meta_tmp_path, &meta_path);


I think there are error (and panic)cases where the meta file sticks around. Not sure how problematic it is to have a meta but no body file.

What are the advantages of splitting this up into 2 files? I feel that a single file would have less edge-cases (e.g. successful write to one file, but not the other) and could result in fewer system calls.

karlseguin · 2026-03-24T02:46:04Z

src/network/cache/FsCache.zig

+        const body_file = try self.dir.createFile(&body_tmp_path, .{});
+        errdefer {
+            body_file.close();
+            self.dir.deleteFile(&body_tmp_path) catch {};


Can writeAll result in a partial write? Because, if you do partially write, and then this fails, you now have garbage in the file that you might fetch in get

karlseguin · 2026-03-24T03:21:55Z

src/network/cache/Cache.zig

+        }
+
+        // no-store: must not be stored
+        if (cc.no_store) return null;


we could exit the loop immediately if that's the cache? Don't even need to store this in CacheControl:

cc = CacheControl.parse(hdr.value) orelse return null;

That's true for all exit paths.

karlseguin · 2026-03-24T03:23:30Z

src/network/cache/Cache.zig

+        if (has_authorization and !cc.is_public and cc.s_maxage == null) return null;
+
+        // Only cache 200 for now. Technically, we can cache others.
+        switch (status) {


Entire body was buffered and headers parsed..this should be done much earlier.

karlseguin · 2026-03-24T03:23:55Z

src/network/cache/Cache.zig

+    }
+
+    pub fn deinit(self: CachedMetadata, allocator: std.mem.Allocator) void {
+        allocator.free(self.url);


Arena, ideally from the arenapool.

karlseguin · 2026-03-24T03:26:29Z

src/network/cache/Cache.zig

+            } else if (std.ascii.eqlIgnoreCase(directive, "must-revalidate")) {
+                cc.must_revalidate = true;
+            } else if (std.ascii.eqlIgnoreCase(directive, "immutable")) {
+                cc.immutable = true;


We don't use a a lot of these. Should we? If not, should we bother parsing them and storing them in the meta?

karlseguin · 2026-03-24T03:28:21Z

src/network/cache/Cache.zig

+        if (cc.max_age == null) return null;
+
+        // Set-Cookie without explicit public
+        if (has_set_cookie and !cc.is_public) return null;


We should never cache anything that isn't public. While we're the user-agent, the cache doesn't have any isolation and acts much more like a reverse proxy's cache.

karlseguin · 2026-03-24T03:35:10Z

src/network/cache/Cache.zig

+            } else if (std.ascii.startsWithIgnoreCase(directive, "s-maxage=")) {
+                cc.s_maxage = std.fmt.parseInt(u64, directive[9..], 10) catch null;
+                // s-maxage takes precedence over max-age
+                cc.max_age = cc.s_maxage orelse cc.max_age;


we only serialize max_age, so if the header is defined as s-maxage=1000,max-age=0 it will do the opposite and max-age will take precedence over s-maxage.

karlseguin · 2026-03-24T03:35:30Z

src/network/cache/FsCache.zig

+    // cache-control
+    try writer.print("{d}\n", .{meta.cache_control.max_age orelse 0});
+    try writer.print("{}\n{}\n{}\n{}\n", .{
+        meta.cache_control.max_age != null,


do we ever cache something with no max-age? What does a max-age of 0 mean?

mrdimidium · 2026-03-24T11:59:57Z

src/Config.zig

    };
 }

+pub fn cacheDir(self: *const Config) ?[]const u8 {


This should have a default folder, something like $XDG_CACHE_DIR/lightpanda

mrdimidium · 2026-03-24T12:02:39Z

src/browser/ScriptManager.zig

+        //     self.debug_transfer_intercept_state = @intFromEnum(transfer._intercept_state);
+        //     self.debug_transfer_auth_challenge = transfer._auth_challenge != null;
+        //     self.debug_transfer_easy_id = if (transfer._conn) |c| @intFromPtr(c.easy) else 0;
+        // }


Why are you removing this log?

mrdimidium · 2026-03-24T12:04:45Z

src/browser/HttpClient.zig

+    }
+};
+
+pub const LiveTransfer = struct {


At least to reduce the number of conflicts

mookums marked this pull request as ready for review March 20, 2026 09:10

mookums added 14 commits March 20, 2026 02:17

create Cache interface file

1b5ec78

add cache dir to configuration opts

ff7d5e4

use renamed LiveTransfer instead of Transfer

15e1a89

allow Mime parse to use []const u8

4228c66

use Response instead of LiveTransfer in callbacks

9315e8b

add headerIterator to Http Response

6dad03e

add CachedResponse variant to Response

fede2c3

add basic FsCache impl

bbd2a70

use enum approach instead of vtable

136fea6

create cache owned by the network struct

5213287

add basic caching support

00e0c22

cache headers along with response

8684c87

use CacheControl and Vary

030c490

add more http caching rules

d34cae5

mookums force-pushed the http-cache branch from 4d34bd4 to d34cae5 Compare March 20, 2026 09:22

store type_buf and sub_type_buf in Mime

bb088d0

karlseguin reviewed Mar 24, 2026

View reviewed changes

krichprollsch requested a review from mrdimidium March 24, 2026 10:54

mrdimidium reviewed Mar 24, 2026

View reviewed changes

Conversation

mookums commented Mar 20, 2026

Uh oh!

karlseguin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants