put more cache under mutex

src/cache/Cache.zig

@@ -12,10 +12,28 @@ io: std.Io,
 lru: Lru,
 /// Cache directory for L2 persistent cache
 cache_dir: ?[]const u8,
+/// Request handlers share one cache across httpz's thread pool. Exclusive
+/// rather than read/write because even reads mutate: `Lru` bumps access counts
+/// and drops expired entries, and an L2 hit promotes into L1. Held across L2
+/// file access too, so a reader never sees a file another thread is rewriting.
+mutex: std.Io.Mutex = .init,
 
 pub const Config = struct {
     max_entries: usize = 1_000,
     cache_dir: ?[]const u8,
+    /// How long entries survive past their TTL, in memory and on disk, so
+    /// `getStale` can serve them while the upstream is unavailable. `get` is
+    /// unaffected: it only ever returns fresh entries.
+    stale_retention_s: u64 = 0,
+};
+
+/// An entry returned by `getStale`. `expires` lets the caller say how old the
+/// data it is serving is.
+pub const Entry = struct {
+    /// Owned by the caller of `getStale`.
+    value: []const u8,
+    /// Expiry time, in milliseconds since the epoch. In the past for stale data.
+    expires: i64,
 };
 
 pub fn init(allocator: std.mem.Allocator, io: std.Io, config: Config) !*Cache {
@@ -34,6 +52,7 @@ pub fn init(allocator: std.mem.Allocator, io: std.Io, config: Config) !*Cache {
         .cache_dir = if (config.cache_dir) |d| try allocator.dupe(u8, d) else null,
     };
 
+    cache.lru.retention_ms = @intCast(config.stale_retention_s * std.time.ms_per_s);
     cache.lru.setEvictionCallback(cache, evictCallback);
 
     // Clean up expired files and populate L1 cache from L2
@@ -51,9 +70,36 @@ fn evictCallback(ctx: *anyopaque, key: []const u8) void {
     self.deleteFile(key);
 }
 
-pub fn get(self: *Cache, key: []const u8) ?[]const u8 {
-    // Check L1 (memory) first
-    if (self.lru.get(key)) |value| return value;
+/// Returns a copy of the value, owned by the caller, only if it has not
+/// expired. A copy because the cached one can be replaced or evicted by
+/// another thread the moment the lock is released.
+pub fn get(self: *Cache, allocator: std.mem.Allocator, key: []const u8) !?[]const u8 {
+    self.mutex.lockUncancelable(self.io);
+    defer self.mutex.unlock(self.io);
+
+    const now = std.Io.Timestamp.now(self.io, .real).toMilliseconds();
+    const entry = self.lookup(key, now) orelse return null;
+    if (now > entry.expires) return null;
+    return try allocator.dupe(u8, entry.value);
+}
+
+/// Returns the value whether fresh or expired-but-retained, copied as in
+/// `get`. Meant as a fallback when refreshing an expired entry has failed.
+pub fn getStale(self: *Cache, allocator: std.mem.Allocator, key: []const u8) !?Entry {
+    self.mutex.lockUncancelable(self.io);
+    defer self.mutex.unlock(self.io);
+
+    const now = std.Io.Timestamp.now(self.io, .real).toMilliseconds();
+    const entry = self.lookup(key, now) orelse return null;
+    return .{ .value = try allocator.dupe(u8, entry.value), .expires = entry.expires };
+}
+
+/// Caller must hold `mutex`. The returned value is borrowed from L1.
+fn lookup(self: *Cache, key: []const u8, now_ms: i64) ?Entry {
+    // Check L1 (memory) first. A retained-but-expired L1 entry is the newest
+    // copy there is, so it also short-circuits the disk read.
+    if (self.lru.lookup(key, now_ms)) |item|
+        return .{ .value = item.value, .expires = item.expires };
 
     // Check L2 (disk)
     const cached = self.loadFromFile(key) catch return null;
@@ -62,10 +108,14 @@ pub fn get(self: *Cache, key: []const u8) ?[]const u8 {
     // L2 exists - promote to L1
     self.lru.put(key, cached.value, cached.expires) catch return null;
 
-    return self.lru.get(key);
+    const item = self.lru.lookup(key, now_ms) orelse return null;
+    return .{ .value = item.value, .expires = item.expires };
 }
 
 pub fn put(self: *Cache, key: []const u8, value: []const u8, ttl_seconds: u64) !void {
+    self.mutex.lockUncancelable(self.io);
+    defer self.mutex.unlock(self.io);
+
     const now = std.Io.Timestamp.now(self.io, .real).toMilliseconds();
     const expires = now + @as(i64, @intCast(ttl_seconds * 1000));
 
@@ -107,8 +157,8 @@ const CacheEntry = struct {
 };
 
 /// Loads cached value from file (path calculated by the key). An error will be thrown
-/// if the file access fails OR if the data has expired.
-/// If the data has expired, the file will be deleted
+/// if the file access fails OR if the data is past its retention window, in
+/// which case the file is deleted. Expired-but-retained entries are returned.
 fn loadFromFile(self: *Cache, key: []const u8) !CacheEntry {
     if (self.cache_dir == null) return error.NoCacheDir;
 
@@ -122,8 +172,8 @@ fn loadFromFile(self: *Cache, key: []const u8) !CacheEntry {
 }
 
 /// Loads cached value from file (using a path). An error will be thrown
-/// if the file access fails OR if the data has expired.
-/// If the data has expired, the file will be deleted
+/// if the file access fails OR if the data is past its retention window, in
+/// which case the file is deleted. Expired-but-retained entries are returned.
 fn loadFromFilePath(self: *Cache, file_path: []const u8) !CacheEntry {
     const file = try std.Io.Dir.cwd().openFile(self.io, file_path, .{});
     defer file.close(self.io);
@@ -135,10 +185,10 @@ fn loadFromFilePath(self: *Cache, file_path: []const u8) !CacheEntry {
     const cached = try deserialize(self.allocator, reader);
     errdefer cached.deinit(self.allocator);
 
-    // Check if expired
+    // Check if past retention. Merely expired entries are kept: they are the
+    // fallback `getStale` serves while the upstream is down.
     const now = std.Io.Timestamp.now(self.io, .real).toMilliseconds();
-    if (cached.expires <= now) {
-        // We're expired, delete expired file
+    if (cached.expires + self.lru.retention_ms <= now) {
         self.deleteFile(cached.key);
         return error.Expired;
     }
@@ -351,12 +401,16 @@ test "L1/L2 cache flow" {
 
     const cache = try Cache.init(allocator, std.testing.io, .{ .max_entries = 10, .cache_dir = cache_dir });
     defer cache.deinit();
+    // Values from get/getStale are copies the caller owns
+    var arena: std.heap.ArenaAllocator = .init(allocator);
+    defer arena.deinit();
+    const copies = arena.allocator();
 
     // Put item in cache
     try cache.put("key1", "value1", 900);
 
     // Should be in L1
-    try std.testing.expectEqualStrings("value1", cache.get("key1").?);
+    try std.testing.expectEqualStrings("value1", (try cache.get(copies, "key1")).?);
 
     // Manually remove from L1 to simulate eviction
     if (cache.lru.map.fetchRemove("key1")) |kv| {
@@ -368,8 +422,99 @@ test "L1/L2 cache flow" {
     try std.testing.expect(cache.lru.get("key1") == null);
 
     // Get should promote from L2 to L1
-    try std.testing.expectEqualStrings("value1", cache.get("key1").?);
+    try std.testing.expectEqualStrings("value1", (try cache.get(copies, "key1")).?);
 
     // Now it should be in L1
     try std.testing.expectEqualStrings("value1", cache.lru.get("key1").?);
 }
+
+test "stale entries are retained for getStale but never returned by get" {
+    const allocator = std.testing.allocator;
+
+    var tmp_dir = std.testing.tmpDir(.{});
+    defer tmp_dir.cleanup();
+
+    var path_buf: [std.fs.max_path_bytes]u8 = undefined;
+    const cache_dir_len = try tmp_dir.dir.realPath(std.testing.io, &path_buf);
+    const cache_dir = path_buf[0..cache_dir_len];
+
+    const cache = try Cache.init(allocator, std.testing.io, .{
+        .max_entries = 10,
+        .cache_dir = cache_dir,
+        .stale_retention_s = 3600,
+    });
+    defer cache.deinit();
+    var arena: std.heap.ArenaAllocator = .init(allocator);
+    defer arena.deinit();
+    const copies = arena.allocator();
+
+    const now = std.Io.Timestamp.now(std.testing.io, .real).toMilliseconds();
+
+    // Expired a minute ago, still inside the hour of retention. Written only to
+    // L2 so the disk path is what gets exercised, as it is after a restart.
+    try cache.saveToFile("recent", "old-but-usable", now - 60_000);
+    try std.testing.expect(try cache.get(copies, "recent") == null);
+    const stale = (try cache.getStale(copies, "recent")).?;
+    try std.testing.expectEqualStrings("old-but-usable", stale.value);
+    try std.testing.expectEqual(now - 60_000, stale.expires);
+    // Now promoted to L1, and still not fresh from there either
+    try std.testing.expect(try cache.get(copies, "recent") == null);
+    try std.testing.expectEqualStrings("old-but-usable", (try cache.getStale(copies, "recent")).?.value);
+
+    // Expired two hours ago: past retention, unusable even as stale, and the
+    // file is cleaned up.
+    try cache.saveToFile("ancient", "too-old", now - 2 * 3600 * 1000);
+    try std.testing.expect(try cache.getStale(copies, "ancient") == null);
+    try std.testing.expectError(error.FileNotFound, cache.loadFromFile("ancient"));
+
+    // Fresh entries come back from both
+    try cache.put("fresh", "new", 900);
+    try std.testing.expectEqualStrings("new", (try cache.get(copies, "fresh")).?);
+    try std.testing.expectEqualStrings("new", (try cache.getStale(copies, "fresh")).?.value);
+}
+
+test "returned values survive the entry being replaced" {
+    const allocator = std.testing.allocator;
+    const cache = try Cache.init(allocator, std.testing.io, .{ .max_entries = 10, .cache_dir = null });
+    defer cache.deinit();
+
+    try cache.put("k", "first", 900);
+    const value = (try cache.get(allocator, "k")).?;
+    defer allocator.free(value);
+
+    // Before values were copied, this freed the memory `value` pointed at
+    try cache.put("k", "second", 900);
+    try std.testing.expectEqualStrings("first", value);
+}
+
+test "concurrent puts and gets on shared keys" {
+    // Many threads hammering a handful of keys through a cache small enough to
+    // keep evicting. Without the lock this corrupts the map or frees values out
+    // from under readers; under the debug allocator that crashes.
+    const allocator = std.testing.allocator;
+    const cache = try Cache.init(allocator, std.testing.io, .{ .max_entries = 4, .cache_dir = null });
+    defer cache.deinit();
+
+    const Worker = struct {
+        fn run(c: *Cache, id: usize) void {
+            var key_buf: [16]u8 = undefined;
+            var value_buf: [32]u8 = undefined;
+            for (0..2_000) |i| {
+                const key = std.fmt.bufPrint(&key_buf, "k{d}", .{i % 8}) catch unreachable;
+                const value = std.fmt.bufPrint(&value_buf, "t{d}-i{d}", .{ id, i }) catch unreachable;
+                c.put(key, value, 900) catch @panic("put failed");
+                if (c.get(std.testing.allocator, key) catch @panic("get failed")) |got| {
+                    // Whatever thread wrote it, it must be an intact value
+                    std.debug.assert(got[0] == 't');
+                    std.testing.allocator.free(got);
+                }
+            }
+        }
+    };
+
+    var threads: [8]std.Thread = undefined;
+    for (&threads, 0..) |*t, id| t.* = try std.Thread.spawn(.{}, Worker.run, .{ cache, id });
+    for (threads) |t| t.join();
+
+    try std.testing.expect(cache.lru.map.count() <= 4);
+}

src/cache/Lru.zig

@@ -1,3 +1,11 @@
+//! Size-capped map with per-entry expiry.
+//!
+//! Not thread-safe, and every operation mutates (even `get` bumps an access
+//! count and may drop an expired entry), so owners shared between threads must
+//! serialize all access with an exclusive lock. Returned slices point into
+//! entries the next `put`, eviction, or expiry frees, so copy them before
+//! releasing that lock.
+
 const std = @import("std");
 
 const Lru = @This();
@@ -7,6 +15,10 @@ allocator: std.mem.Allocator,
 io: std.Io,
 map: std.StringHashMap(Entry),
 max_entries: usize,
+/// How long an entry is kept after it expires, so it can still be served as a
+/// fallback when the upstream is down. `get` never returns such an entry; only
+/// `lookup` does. Zero drops entries as soon as they expire.
+retention_ms: i64 = 0,
 evict_fn: ?*const fn (ctx: *anyopaque, key: []const u8) void = null,
 evict_ctx: ?*anyopaque = null,
 
@@ -31,22 +43,33 @@ pub fn setEvictionCallback(self: *Lru, ctx: *anyopaque, callback: *const fn (ctx
 }
 
 pub fn get(self: *Lru, key: []const u8) ?[]const u8 {
+    const now = std.Io.Timestamp.now(self.io, .real).toMilliseconds();
+    const item = self.lookup(key, now) orelse return null;
+    if (now > item.expires) return null;
+    return item.value;
+}
+
+/// Returns the entry whether fresh or expired-but-retained, leaving the
+/// freshness decision to the caller. Entries past the retention window are
+/// dropped.
+pub fn lookup(self: *Lru, key: []const u8, now_ms: i64) ?Iterator.Item {
     var entry = self.map.getPtr(key) orelse return null;
 
-    const now = std.Io.Timestamp.now(self.io, .real).toMilliseconds();
-    if (now > entry.expires) {
+    if (now_ms > entry.expires + self.retention_ms) {
         self.remove(key);
         return null;
     }
 
     entry.access_count += 1;
-    return entry.value;
+    return .{ .key = key, .value = entry.value, .expires = entry.expires };
 }
 
 pub fn put(self: *Lru, key: []const u8, value: []const u8, expires: i64) !void {
-    if (self.map.get(key)) |old_entry| {
-        self.allocator.free(old_entry.value);
-        _ = self.map.remove(key);
+    // Replacing, not evicting: the callback is skipped, since the owner may
+    // already have persisted the new value under the same key.
+    if (self.map.fetchRemove(key)) |old| {
+        self.allocator.free(old.value.value);
+        self.allocator.free(old.key);
     }
 
     if (self.map.count() >= self.max_entries) {
@@ -54,7 +77,9 @@ pub fn put(self: *Lru, key: []const u8, value: []const u8, expires: i64) !void {
     }
 
     const key_copy = try self.allocator.dupe(u8, key);
+    errdefer self.allocator.free(key_copy);
     const value_copy = try self.allocator.dupe(u8, value);
+    errdefer self.allocator.free(value_copy);
 
     try self.map.put(key_copy, .{
         .value = value_copy,
@@ -154,4 +179,38 @@ test "LRU expired entry returns null" {
     // Get should return null for expired entry
     const result = lru.get("key1");
     try std.testing.expect(result == null);
+    // And with no retention it is gone entirely
+    try std.testing.expect(lru.lookup("key1", now) == null);
+}
+
+test "LRU retains expired entries for lookup within the retention window" {
+    var lru = try Lru.init(std.testing.allocator, std.testing.io, 10);
+    defer lru.deinit();
+    lru.retention_ms = 60_000;
+
+    const now = std.Io.Timestamp.now(std.testing.io, .real).toMilliseconds();
+    try lru.put("recent", "v1", now - 1000);
+    try lru.put("ancient", "v2", now - 120_000);
+
+    // Expired, so never served as fresh...
+    try std.testing.expect(lru.get("recent") == null);
+    // ...but still available to a caller willing to take stale data
+    const item = lru.lookup("recent", now).?;
+    try std.testing.expectEqualStrings("v1", item.value);
+    try std.testing.expectEqual(now - 1000, item.expires);
+
+    // Past the window it is dropped
+    try std.testing.expect(lru.lookup("ancient", now) == null);
+    try std.testing.expect(!lru.map.contains("ancient"));
+}
+
+test "LRU replacing a key frees the old key and value" {
+    // std.testing.allocator fails the test on a leak, which is the assertion
+    var lru = try Lru.init(std.testing.allocator, std.testing.io, 10);
+    defer lru.deinit();
+
+    try lru.put("key1", "value1", 9999999999999);
+    try lru.put("key1", "value2", 9999999999999);
+    try std.testing.expectEqualStrings("value2", lru.get("key1").?);
+    try std.testing.expectEqual(@as(u32, 1), lru.map.count());
 }

src/http/handler.zig

@@ -83,6 +83,18 @@ pub fn handleWeather(
             res.body = help.translation_page;
             return;
         }
+        // Any other `:` name is an endpoint we do not have, not a place
+        res.status = 404;
+        res.content_type = .TEXT;
+        res.body = "Unknown endpoint\n";
+        return;
+    }
+    if (isJunkLocation(location)) {
+        log.debug("rejecting junk location {s}", .{location});
+        res.status = 404;
+        res.content_type = .TEXT;
+        res.body = "Not found\n";
+        return;
     }
 
     try handleWeatherInternal(opts, req, res, location, client_ip);
@@ -117,6 +129,11 @@ fn handleWeatherInternal(
                 res.body = "Location not found (location query resolution failure)\n";
                 return;
             },
+            error.GeocodingFailed => {
+                log.warn("geocoding unavailable for query {s}", .{location_query});
+                upstreamUnavailable(res, "Location lookup is temporarily unavailable");
+                return;
+            },
             else => return err,
         }
     };
@@ -130,11 +147,23 @@ fn handleWeatherInternal(
                 res.body = "Location not found (provider fetch failure)\n";
                 return;
             },
-            else => return err,
+            error.OutOfMemory => return err,
+            // Anything else is the upstream failing (status, network, TLS, or
+            // a malformed response) with no usable cached data to fall back on
+            else => {
+                log.warn("weather unavailable for {d:.4},{d:.4}: {}", .{ location.coords.latitude, location.coords.longitude, err });
+                upstreamUnavailable(res, "Weather data is temporarily unavailable");
+                return;
+            },
         }
     };
     defer weather.deinit();
 
+    // The upstream is down and this is old data. Say so, with how long past its
+    // expiry it is, so scripts and status bars can decide whether to trust it.
+    if (weather.stale_for_s) |stale_for_s|
+        res.headers.add("X-Weather-Stale", try std.fmt.allocPrint(res.arena, "{d}", .{stale_for_s}));
+
     // Set display name for rendering
     weather.display_name = try req_alloc.dupe(u8, location.name);
 
@@ -184,6 +213,45 @@ fn handleWeatherInternal(
     try renderWeatherData(res.writer(), weather, params, render_options, now_unix_s);
 }
 
+/// A dependency we cannot work without is down. That is a 503 the client can
+/// retry, not a 500 implying we are broken.
+fn upstreamUnavailable(res: *httpz.Response, comptime message: []const u8) void {
+    res.status = 503;
+    res.content_type = .TEXT;
+    res.header("Retry-After", "60");
+    res.body = message ++ ", please try again shortly\n";
+}
+
+/// File extensions that only ever show up in requests from scanners probing
+/// for files. No place name ends in one, and letting them through costs a
+/// Nominatim lookup each time (failures are not cached) plus possibly a
+/// weather fetch.
+const junk_extensions = [_][]const u8{
+    "7z",     "asp",  "aspx", "bak", "bz2",  "cfg", "cgi",  "conf",
+    "config", "css",  "env",  "gif", "gz",   "htm", "html", "ico",
+    "ini",    "jpeg", "jpg",  "js",  "json", "jsp", "log",  "php",
+    "png",    "rar",  "sql",  "svg", "tar",  "tgz", "txt",  "webp",
+    "xml",    "yaml", "yml",  "zip",
+};
+
+/// Whether a location is a probe for something other than weather. Runs after
+/// the endpoints we do serve (favicon.ico, robots.txt, ...) are handled.
+fn isJunkLocation(location: []const u8) bool {
+    // `@domain` lookups legitimately end in what looks like an extension
+    if (location.len == 0 or location[0] == '@') return false;
+    const name = if (location[0] == '~') location[1..] else location;
+    if (name.len == 0) return true;
+    // Dotfiles: .env, .git, .aws, ...
+    if (name[0] == '.') return true;
+    // WordPress probes: wp-login.php is caught below, wp-admin is not
+    if (std.ascii.startsWithIgnoreCase(name, "wp-")) return true;
+    const dot = std.mem.lastIndexOfScalar(u8, name, '.') orelse return false;
+    const extension = name[dot + 1 ..];
+    for (junk_extensions) |junk|
+        if (std.ascii.eqlIgnoreCase(extension, junk)) return true;
+    return false;
+}
+
 fn renderWeatherData(
     writer: *std.Io.Writer,
     weather: types.WeatherData,
@@ -624,3 +692,152 @@ test "handler: an explicit unit request still overrides the pin's country" {
     try ht.expectStatus(200);
     try ht.expectBody("☀️ +20°C\n");
 }
+
+test "isJunkLocation: rejects scanner probes" {
+    const junk = [_][]const u8{
+        "sitemap.xml.gz", "wp-login.php", "wp-admin",   "WP-Admin",   ".env",
+        ".git",           "config.json",  "backup.sql", "index.HTML", "apple-touch-icon.png",
+        "ads.txt",        "~",            "~shell.php",
+    };
+    for (junk) |location| {
+        std.testing.expect(isJunkLocation(location)) catch |err| {
+            std.debug.print("expected junk: {s}\n", .{location});
+            return err;
+        };
+    }
+}
+
+test "isJunkLocation: lets real locations through" {
+    const real = [_][]const u8{
+        "London",       "St. Louis",   "St.Louis",    "Washington.DC", "48.85,2.35",
+        "50.39.107.54", "2001:db8::1", "@github.com", "@wttr.in",      "~Eiffel Tower",
+        "Mt. Hood",
+        "Москва",
+        "PDX",          "",
+    };
+    for (real) |location| {
+        std.testing.expect(!isJunkLocation(location)) catch |err| {
+            std.debug.print("expected real: {s}\n", .{location});
+            return err;
+        };
+    }
+}
+
+test "handler: junk paths and unknown endpoints are 404 without a lookup" {
+    const allocator = std.testing.allocator;
+    const MockHarness = @import("Server.zig").MockHarness;
+
+    var harness = try MockHarness.init(allocator);
+    defer harness.deinit();
+    // Any lookup reaching the provider would fail loudly with a 503 instead
+    harness.mock.fail_with = error.WeatherApiFailed;
+
+    const cases = [_][]const u8{ "sitemap.xml.gz", ":", ":bogus", ".env", "wp-admin" };
+    for (cases) |location| {
+        var ht = httpz.testing.init(.{});
+        defer ht.deinit();
+
+        const url = try std.fmt.allocPrint(allocator, "/{s}", .{location});
+        defer allocator.free(url);
+        ht.url(url);
+        ht.param("location", location);
+
+        try handleWeather(&harness.opts, ht.req, ht.res, "127.0.0.1");
+        try ht.expectStatus(404);
+    }
+}
+
+test "handler: upstream weather failure with nothing cached is a retryable 503" {
+    const allocator = std.testing.allocator;
+    const MockHarness = @import("Server.zig").MockHarness;
+
+    var harness = try MockHarness.init(allocator);
+    defer harness.deinit();
+    harness.mock.fail_with = error.WeatherApiFailed;
+
+    // The harness cache directory is shared across tests and runs, so other
+    // tests' responses for these coordinates would be served from it. An empty
+    // in-memory cache guarantees there is nothing to fall back on.
+    const Cache = @import("../cache/Cache.zig");
+    const empty_cache = try Cache.init(allocator, std.testing.io, .{
+        .cache_dir = null,
+        .stale_retention_s = WeatherProvider.stale_retention_s,
+    });
+    defer empty_cache.deinit();
+    harness.opts.provider = harness.mock.provider(empty_cache);
+    var ht = httpz.testing.init(.{});
+    defer ht.deinit();
+
+    ht.url("/73.158.64.1?format=1");
+    ht.param("location", "73.158.64.1");
+
+    try handleWeather(&harness.opts, ht.req, ht.res, "73.158.64.1");
+
+    try ht.expectStatus(503);
+    try ht.expectHeader("Retry-After", "60");
+}
+
+test "handler: stale data is served with an X-Weather-Stale header" {
+    const allocator = std.testing.allocator;
+    const MockHarness = @import("Server.zig").MockHarness;
+    const Pins = @import("../location/Pins.zig");
+    const Location = @import("../location/resolver.zig").Location;
+    const Cache = @import("../cache/Cache.zig");
+
+    var harness = try MockHarness.init(allocator);
+    defer harness.deinit();
+    harness.mock.fail_with = error.WeatherApiFailed;
+
+    // Pinned so the cache key does not depend on the GeoLite2 database
+    try harness.geoip.pins.put(
+        try Pins.parseCidr("203.0.113.0/24"),
+        "Seattle, Washington, United States",
+        .{ .latitude = 47.6, .longitude = -122.3 },
+        Location.isoFrom("US"),
+    );
+
+    // Own cache, for the same reason as the 503 test above, holding an entry
+    // for the pinned coordinates that expired five minutes ago
+    const cache = try Cache.init(allocator, std.testing.io, .{
+        .cache_dir = null,
+        .stale_retention_s = WeatherProvider.stale_retention_s,
+    });
+    defer cache.deinit();
+    const now = std.Io.Timestamp.now(std.testing.io, .real).toMilliseconds();
+    try cache.lru.put("47.6000,-122.3000", "{}", now - 5 * 60 * 1000);
+    harness.opts.provider = harness.mock.provider(cache);
+
+    var ht = httpz.testing.init(.{});
+    defer ht.deinit();
+
+    ht.url("/203.0.113.7?format=1");
+    ht.param("location", "203.0.113.7");
+
+    try handleWeather(&harness.opts, ht.req, ht.res, "203.0.113.7");
+
+    try ht.expectStatus(200);
+    try ht.expectBody("☀️ +68°F\n");
+    const pr = try ht.parseResponse();
+    const stale_for_s = try std.fmt.parseInt(i64, pr.headers.get("X-Weather-Stale").?, 10);
+    try std.testing.expect(stale_for_s >= 300 and stale_for_s < 360);
+}
+
+test "handler: fresh data has no X-Weather-Stale header" {
+    const allocator = std.testing.allocator;
+    const MockHarness = @import("Server.zig").MockHarness;
+
+    var harness = try MockHarness.init(allocator);
+    defer harness.deinit();
+
+    var ht = httpz.testing.init(.{});
+    defer ht.deinit();
+
+    ht.url("/73.158.64.1?format=1");
+    ht.param("location", "73.158.64.1");
+
+    try handleWeather(&harness.opts, ht.req, ht.res, "73.158.64.1");
+
+    try ht.expectStatus(200);
+    const pr = try ht.parseResponse();
+    try std.testing.expect(pr.headers.get("X-Weather-Stale") == null);
+}

src/location/GeoCache.zig

@@ -2,6 +2,7 @@ const std = @import("std");
 const Coordinates = @import("../Coordinates.zig");
 const Location = @import("resolver.zig").Location;
 const srf = @import("srf");
+const Lru = @import("../cache/Lru.zig");
 
 const GeoCache = @This();
 
@@ -14,6 +15,17 @@ cache: std.StringHashMap(CachedLocation),
 cache_file: ?[]const u8,
 dirty: bool,
 last_save: i64,
+/// Queries the geocoder answered with no results. Kept apart from `cache`, and
+/// in memory only with a cap and a TTL, because real places are a bounded set
+/// but nonsense queries are unbounded and chosen by whoever is scanning us.
+not_found: Lru,
+/// Request handlers share one GeoCache across httpz's thread pool. Exclusive
+/// rather than read/write because `get` may rewrite the file and `not_found`
+/// lookups mutate (see `Lru`).
+mutex: std.Io.Mutex = .init,
+
+const not_found_max_entries = 1_000;
+const not_found_ttl_ms = 24 * std.time.ms_per_hour;
 
 pub const CachedLocation = struct {
     name: []const u8,
@@ -41,6 +53,7 @@ pub fn init(allocator: std.mem.Allocator, io: std.Io, cache_file: ?[]const u8) !
         .cache_file = if (cache_file) |f| try allocator.dupe(u8, f) else null,
         .dirty = false,
         .last_save = std.Io.Timestamp.now(io, .real).toMilliseconds(),
+        .not_found = try Lru.init(allocator, io, not_found_max_entries),
     };
 }
 
@@ -58,27 +71,75 @@ pub fn deinit(self: *GeoCache) void {
         self.allocator.free(entry.value_ptr.name);
     }
     self.cache.deinit();
+    self.not_found.deinit();
     if (self.cache_file) |f| self.allocator.free(f);
 }
 
-pub fn get(self: *GeoCache, query: []const u8) ?CachedLocation {
+/// Returns a copy of the cached location, whose `name` the caller owns and
+/// must free with `allocator`. A copy because another thread's `put` can free
+/// the cached name the moment the lock is released.
+pub fn get(self: *GeoCache, allocator: std.mem.Allocator, query: []const u8) !?CachedLocation {
+    self.mutex.lockUncancelable(self.io);
+    defer self.mutex.unlock(self.io);
+
     self.saveIfNeeded();
-    return self.cache.get(query);
+    const cached = self.cache.get(query) orelse return null;
+    return .{
+        .name = try allocator.dupe(u8, cached.name),
+        .coords = cached.coords,
+        .iso_country = cached.iso_country,
+    };
 }
 
 pub fn put(self: *GeoCache, query: []const u8, location: CachedLocation) !void {
-    const key = try self.allocator.dupe(u8, query);
-    const value = CachedLocation{
-        .name = try self.allocator.dupe(u8, location.name),
+    self.mutex.lockUncancelable(self.io);
+    defer self.mutex.unlock(self.io);
+
+    const name = try self.allocator.dupe(u8, location.name);
+    errdefer self.allocator.free(name);
+
+    const gop = try self.cache.getOrPut(query);
+    if (gop.found_existing) {
+        // Keep the stored key; replacing the value used to leak both the old
+        // name and a fresh copy of the key
+        self.allocator.free(gop.value_ptr.name);
+    } else {
+        // `getOrPut` stored the caller's slice; swap in one we own
+        gop.key_ptr.* = self.allocator.dupe(u8, query) catch |err| {
+            self.cache.removeByPtr(gop.key_ptr);
+            return err;
+        };
+    }
+    gop.value_ptr.* = .{
+        .name = name,
         .coords = location.coords,
         .iso_country = location.iso_country,
     };
-    try self.cache.put(key, value);
     self.dirty = true;
 }
 
-/// Save cache to disk if dirty and enough time has passed (15 minutes)
-pub fn saveIfNeeded(self: *GeoCache) void {
+/// Records that the geocoder has no result for `query`. Only for a definitive
+/// "no such place": a geocoder that failed or was unreachable says nothing
+/// about the query, and caching that would hide real places.
+pub fn putNotFound(self: *GeoCache, query: []const u8) !void {
+    self.mutex.lockUncancelable(self.io);
+    defer self.mutex.unlock(self.io);
+
+    const now = std.Io.Timestamp.now(self.io, .real).toMilliseconds();
+    try self.not_found.put(query, "", now + not_found_ttl_ms);
+}
+
+/// Whether the geocoder recently had no result for `query`.
+pub fn isKnownNotFound(self: *GeoCache, query: []const u8) bool {
+    self.mutex.lockUncancelable(self.io);
+    defer self.mutex.unlock(self.io);
+
+    return self.not_found.get(query) != null;
+}
+
+/// Save cache to disk if dirty and enough time has passed (15 minutes).
+/// Caller must hold `mutex`.
+fn saveIfNeeded(self: *GeoCache) void {
     if (!self.dirty) return;
 
     const cache_file = self.cache_file orelse return;
@@ -208,10 +269,31 @@ test "GeoCache basic operations" {
         },
     });
 
-    const result = cache.get("London");
-    try std.testing.expect(result != null);
-    try std.testing.expectApproxEqAbs(@as(f64, 51.5074), result.?.coords.latitude, 0.0001);
-    try std.testing.expectApproxEqAbs(@as(f64, -0.1278), result.?.coords.longitude, 0.0001);
+    const result = (try cache.get(allocator, "London")) orelse return error.TestUnexpectedResult;
+    defer allocator.free(result.name);
+    try std.testing.expectEqualStrings("London, UK", result.name);
+    try std.testing.expectApproxEqAbs(@as(f64, 51.5074), result.coords.latitude, 0.0001);
+    try std.testing.expectApproxEqAbs(@as(f64, -0.1278), result.coords.longitude, 0.0001);
+}
+
+test "GeoCache replacing an entry frees the old one and keeps returned copies valid" {
+    // std.testing.allocator fails the test on a leak, which is half the assertion
+    const allocator = std.testing.allocator;
+    var cache = try GeoCache.init(allocator, std.testing.io, null);
+    defer cache.deinit();
+
+    try cache.put("Portland", .{ .name = "Portland, Maine", .coords = .{ .latitude = 43.66, .longitude = -70.26 } });
+    const first = (try cache.get(allocator, "Portland")).?;
+    defer allocator.free(first.name);
+
+    try cache.put("Portland", .{ .name = "Portland, Oregon", .coords = .{ .latitude = 45.52, .longitude = -122.68 } });
+    const second = (try cache.get(allocator, "Portland")).?;
+    defer allocator.free(second.name);
+
+    try std.testing.expectEqual(@as(u32, 1), cache.cache.count());
+    try std.testing.expectEqualStrings("Portland, Oregon", second.name);
+    // The first copy is unaffected by the entry it came from being replaced
+    try std.testing.expectEqualStrings("Portland, Maine", first.name);
 }
 
 test "GeoCache miss returns null" {
@@ -219,8 +301,39 @@ test "GeoCache miss returns null" {
     var cache = try GeoCache.init(allocator, std.testing.io, null);
     defer cache.deinit();
 
-    const result = cache.get("NonExistent");
-    try std.testing.expect(result == null);
+    try std.testing.expect(try cache.get(allocator, "NonExistent") == null);
+}
+
+test "GeoCache remembers queries with no result, separately and for a limited time" {
+    const allocator = std.testing.allocator;
+    var cache = try GeoCache.init(allocator, std.testing.io, null);
+    defer cache.deinit();
+
+    try std.testing.expect(!cache.isKnownNotFound("Nowhereville"));
+    try cache.putNotFound("Nowhereville");
+    try std.testing.expect(cache.isKnownNotFound("Nowhereville"));
+    try std.testing.expect(!cache.isKnownNotFound("Somewhereville"));
+
+    // Not a place, so not in the positive cache and never written to disk
+    try std.testing.expect(try cache.get(allocator, "Nowhereville") == null);
+    try std.testing.expectEqual(@as(u32, 0), cache.cache.count());
+    try std.testing.expect(!cache.dirty);
+
+    // Expired entries are forgotten, so the place gets another chance
+    const now = std.Io.Timestamp.now(std.testing.io, .real).toMilliseconds();
+    try cache.not_found.put("Gonesville", "", now - 1000);
+    try std.testing.expect(!cache.isKnownNotFound("Gonesville"));
+}
+
+test "GeoCache bounds the not-found set" {
+    const allocator = std.testing.allocator;
+    var cache = try GeoCache.init(allocator, std.testing.io, null);
+    defer cache.deinit();
+
+    var buf: [32]u8 = undefined;
+    for (0..not_found_max_entries * 2) |i|
+        try cache.putNotFound(try std.fmt.bufPrint(&buf, "junk-{d}", .{i}));
+    try std.testing.expect(cache.not_found.map.count() <= not_found_max_entries);
 }
 
 test "save produces SRF an operator can read" {

src/location/Ip2location.zig

@@ -194,6 +194,10 @@ pub const Cache = struct {
     path: []const u8,
     entries: std.AutoHashMap(u128, Location),
     file: ?std.Io.File,
+    /// Request handlers reach this from httpz's thread pool, outside GeoIp's
+    /// lock. Guards `entries` and the appends to `file`, where two unguarded
+    /// writers would read the same end offset and overwrite each other.
+    mutex: std.Io.Mutex = .init,
 
     /// On-disk shape of one entry.
     ///
@@ -308,6 +312,9 @@ pub const Cache = struct {
     }
 
     pub fn get(self: *Cache, ip: u128) ?Location {
+        self.mutex.lockUncancelable(self.io);
+        defer self.mutex.unlock(self.io);
+
         const entry = self.entries.get(ip) orelse return null;
         return .{
             .allocator = self.allocator,
@@ -322,11 +329,17 @@ pub const Cache = struct {
     /// Unit selection only needs the country, and doing it this way keeps that
     /// path free of both an allocation and any chance of a network request.
     pub fn getCountry(self: *Cache, ip: u128) ?[2]u8 {
+        self.mutex.lockUncancelable(self.io);
+        defer self.mutex.unlock(self.io);
+
         const entry = self.entries.getPtr(ip) orelse return null;
         return entry.iso_country;
     }
 
     pub fn put(self: *Cache, ip: u128, family: u8, loc: Location) !void {
+        self.mutex.lockUncancelable(self.io);
+        defer self.mutex.unlock(self.io);
+
         const name_copy = try self.allocator.dupe(u8, loc.name);
         errdefer self.allocator.free(name_copy);
 

src/location/resolver.zig

@@ -183,14 +183,18 @@ pub const Resolver = struct {
         // Don't bother passing this through
         if (std.mem.eql(u8, name, "favicon.ico")) return error.GeocodingFailed;
         // Check cache first
-        if (self.geocache.get(name)) |cached| {
+        if (try self.geocache.get(self.allocator, name)) |cached| {
             return Location{
                 .allocator = self.allocator,
-                .name = try self.allocator.dupe(u8, cached.name),
+                .name = cached.name,
                 .coords = cached.coords,
                 .iso_country = cached.iso_country,
             };
         }
+        if (self.geocache.isKnownNotFound(name)) {
+            log.debug("nominatim recently found nothing for {s}; not asking again", .{name});
+            return error.LocationNotFound;
+        }
 
         log.info("Calling nominatim (OpenStreetMap) to resolve place name {s} to coordinates", .{name});
         if (@import("builtin").is_test) return error.GeocodingUnavailableInUnitTest;
@@ -239,6 +243,8 @@ pub const Resolver = struct {
         defer parsed.deinit();
 
         if (parsed.value.array.items.len == 0) {
+            log.info("nominatim found nothing for {s}", .{name});
+            try self.geocache.putNotFound(name);
             return error.LocationNotFound;
         }
 
@@ -408,6 +414,22 @@ test "resolver init" {
     try std.testing.expect(resolver.airports == null);
 }
 
+test "resolver answers a recent not-found from the cache without calling nominatim" {
+    const allocator = std.testing.allocator;
+    var geocache = try GeoCache.init(allocator, std.testing.io, null);
+    defer geocache.deinit();
+    var resolver = Resolver.init(allocator, std.testing.io, null, &geocache, null);
+
+    // Without the entry the lookup reaches the network path, which unit tests
+    // stub out with a distinct error
+    try std.testing.expectError(error.GeocodingUnavailableInUnitTest, resolver.resolve("Nowhereville"));
+
+    try geocache.putNotFound("Nowhereville");
+    try std.testing.expectError(error.LocationNotFound, resolver.resolve("Nowhereville"));
+    // Also via the `~` special-location form, which geocodes the same name
+    try std.testing.expectError(error.LocationNotFound, resolver.resolve("~Nowhereville"));
+}
+
 test "resolve IP address with GeoIP" {
     const allocator = std.testing.allocator;
     const Config = @import("../Config.zig");

src/main.zig

@@ -2,6 +2,7 @@ const std = @import("std");
 const Config = @import("Config.zig");
 const Cache = @import("cache/Cache.zig");
 const MetNo = @import("weather/MetNo.zig");
+const WeatherProvider = @import("weather/Provider.zig");
 const Server = @import("http/Server.zig");
 const RateLimiter = @import("http/RateLimiter.zig");
 const GeoIp = @import("location/GeoIp.zig");
@@ -104,6 +105,7 @@ fn serve(
     const cache = try Cache.init(allocator, io, .{
         .max_entries = cfg.cache_size,
         .cache_dir = cfg.cache_dir,
+        .stale_retention_s = WeatherProvider.stale_retention_s,
     });
     defer cache.deinit();
 

src/weather/MetNo.zig

@@ -147,6 +147,14 @@ fn fetchRaw(ptr: *anyopaque, allocator: std.mem.Allocator, coords: Coordinates)
     });
 
     if (result.status != .ok) {
+        const body = response_buf[0..writer.end];
+        std.log.warn("met.no returned HTTP {d} ({s}) for {s} with User-Agent '{s}': {s}", .{
+            @intFromEnum(result.status),
+            result.status.phrase() orelse "unknown",
+            url,
+            user_agent,
+            body[0..@min(body.len, 512)],
+        });
         return error.WeatherApiFailed;
     }
 

src/weather/Mock.zig

@@ -9,6 +9,8 @@ const Mock = @This();
 allocator: std.mem.Allocator,
 responses: std.StringHashMap([]const u8),
 parse_fn: ?*const fn (ptr: *anyopaque, allocator: std.mem.Allocator, raw: []const u8) anyerror!types.WeatherData = null,
+/// When set, every fetch fails with this error, simulating an upstream outage.
+fail_with: ?anyerror = null,
 
 pub fn init(allocator: std.mem.Allocator) !Mock {
     return Mock{
@@ -37,6 +39,7 @@ pub fn addResponse(self: *Mock, coords: Coordinates, raw_json: []const u8) !void
 
 fn fetchRaw(ptr: *anyopaque, allocator: std.mem.Allocator, coords: Coordinates) ![]const u8 {
     const self: *Mock = @ptrCast(@alignCast(ptr));
+    if (self.fail_with) |err| return err;
     const key = try std.fmt.allocPrint(allocator, "{d:.4},{d:.4}", .{ coords.latitude, coords.longitude });
     defer allocator.free(key);
 

src/weather/Provider.zig

@@ -11,6 +11,14 @@ const Cache = @import("../cache/Cache.zig");
 
 const WeatherProvider = @This();
 
+const log = std.log.scoped(.weather);
+
+/// How long past its TTL a cached response may still be served when the
+/// upstream fails. With the 16-33 minute TTL below, the oldest data ever served
+/// is about two hours old; beyond that "current" conditions are fiction.
+/// Configure the weather cache with this as `Cache.Config.stale_retention_s`.
+pub const stale_retention_s: u64 = 90 * 60;
+
 ptr: *anyopaque,
 vtable: *const VTable,
 cache: *Cache,
@@ -27,11 +35,29 @@ pub fn fetch(self: WeatherProvider, allocator: std.mem.Allocator, coords: Coordi
     const cache_key = try std.fmt.bufPrint(&buf, "{d:.4},{d:.4}", .{ coords.latitude, coords.longitude });
 
     // Check cache first
-    if (self.cache.get(cache_key)) |cached_raw|
+    if (try self.cache.get(allocator, cache_key)) |cached_raw| {
+        defer allocator.free(cached_raw);
         return self.vtable.parse(self.ptr, allocator, cached_raw);
+    }
 
-    // Cache miss - fetch raw data from provider
-    const raw = try self.vtable.fetchRaw(self.ptr, allocator, coords);
+    // Cache miss - fetch raw data from provider, falling back to an expired
+    // entry if the upstream is down
+    const raw = self.vtable.fetchRaw(self.ptr, allocator, coords) catch |err| {
+        if (try self.cache.getStale(allocator, cache_key)) |stale| {
+            defer allocator.free(stale.value);
+            const now_ms = std.Io.Timestamp.now(self.cache.io, .real).toMilliseconds();
+            const stale_for_s = @divTrunc(now_ms - stale.expires, std.time.ms_per_s);
+            log.warn("weather fetch for {s} failed ({}); serving cached data that expired {d}s ago", .{
+                cache_key,
+                err,
+                stale_for_s,
+            });
+            var weather = try self.vtable.parse(self.ptr, allocator, stale.value);
+            weather.stale_for_s = stale_for_s;
+            return weather;
+        }
+        return err;
+    };
     defer allocator.free(raw);
 
     // TTL: 1000-2000 seconds (16-33 minutes) to avoid thundering herd.
@@ -78,4 +104,65 @@ test "provider fetch" {
 
     try std.testing.expect(weather1.forecast.len > 0);
     try std.testing.expect(weather2.forecast.len > 0);
+    try std.testing.expect(weather1.stale_for_s == null);
+    try std.testing.expect(weather2.stale_for_s == null);
+}
+
+test "provider serves expired cached data when the upstream fails" {
+    const Mock = @import("Mock.zig");
+    const MetNo = @import("MetNo.zig");
+
+    const cache = try Cache.init(std.testing.allocator, std.testing.io, .{
+        .max_entries = 10,
+        .cache_dir = null,
+        .stale_retention_s = stale_retention_s,
+    });
+    defer cache.deinit();
+
+    var mock = try Mock.init(std.testing.allocator);
+    defer mock.deinit();
+    mock.parse_fn = MetNo.parse;
+    mock.fail_with = error.WeatherApiFailed;
+
+    const provider = mock.provider(cache);
+    const coords = Coordinates{ .latitude = 33.4484, .longitude = -112.0740 };
+
+    // Nothing cached: the upstream error surfaces unchanged
+    try std.testing.expectError(error.WeatherApiFailed, provider.fetch(std.testing.allocator, coords));
+
+    // A response that expired ten minutes ago is better than nothing
+    const now = std.Io.Timestamp.now(std.testing.io, .real).toMilliseconds();
+    try cache.lru.put("33.4484,-112.0740", @embedFile("../tests/metno_test_data.json"), now - 10 * 60 * 1000);
+
+    const weather = try provider.fetch(std.testing.allocator, coords);
+    defer weather.deinit();
+    try std.testing.expect(weather.forecast.len > 0);
+    // Marked stale, by about the ten minutes it has been expired
+    const stale_for_s = weather.stale_for_s.?;
+    try std.testing.expect(stale_for_s >= 600 and stale_for_s < 660);
+}
+
+test "provider does not serve data past the stale retention window" {
+    const Mock = @import("Mock.zig");
+    const MetNo = @import("MetNo.zig");
+
+    const cache = try Cache.init(std.testing.allocator, std.testing.io, .{
+        .max_entries = 10,
+        .cache_dir = null,
+        .stale_retention_s = stale_retention_s,
+    });
+    defer cache.deinit();
+
+    var mock = try Mock.init(std.testing.allocator);
+    defer mock.deinit();
+    mock.parse_fn = MetNo.parse;
+    mock.fail_with = error.WeatherApiFailed;
+
+    const now = std.Io.Timestamp.now(std.testing.io, .real).toMilliseconds();
+    const too_old = now - @as(i64, @intCast(stale_retention_s * std.time.ms_per_s)) - 1000;
+    try cache.lru.put("33.4484,-112.0740", @embedFile("../tests/metno_test_data.json"), too_old);
+
+    const provider = mock.provider(cache);
+    const coords = Coordinates{ .latitude = 33.4484, .longitude = -112.0740 };
+    try std.testing.expectError(error.WeatherApiFailed, provider.fetch(std.testing.allocator, coords));
 }

src/weather/types.zig

@@ -91,6 +91,9 @@ pub const WeatherData = struct {
     current: CurrentCondition,
     forecast: []ForecastDay,
     allocator: std.mem.Allocator,
+    /// Set when the upstream was unavailable and this came from an expired
+    /// cache entry: seconds since that entry expired. Null for fresh data.
+    stale_for_s: ?i64 = null,
 
     pub fn deinit(self: WeatherData) void {
         self.allocator.free(self.location);