From f19fce375ca1de5c5f177df1306f1414bc1f3cf8 Mon Sep 17 00:00:00 2001 From: Emil Lerch Date: Wed, 4 Nov 2026 09:20:46 -0800 Subject: [PATCH] avoid calling nominatum when we are being asked for crap --- src/location/GeoCache.zig | 55 +++++++++++++++++++++++++++++++++++++++ src/location/resolver.zig | 22 ++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/src/location/GeoCache.zig b/src/location/GeoCache.zig index 957d4f1..a274287 100644 --- a/src/location/GeoCache.zig +++ b/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,13 @@ 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, + +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 +49,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,6 +67,7 @@ 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); } @@ -77,6 +87,19 @@ pub fn put(self: *GeoCache, query: []const u8, location: CachedLocation) !void { self.dirty = true; } +/// 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 { + 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 { + return self.not_found.get(query) != null; +} + /// Save cache to disk if dirty and enough time has passed (15 minutes) pub fn saveIfNeeded(self: *GeoCache) void { if (!self.dirty) return; @@ -223,6 +246,38 @@ test "GeoCache miss returns null" { try std.testing.expect(result == 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(cache.get("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" { const allocator = std.testing.allocator; var cache = try GeoCache.init(allocator, std.testing.io, null); diff --git a/src/location/resolver.zig b/src/location/resolver.zig index 1896076..92c0cea 100644 --- a/src/location/resolver.zig +++ b/src/location/resolver.zig @@ -191,6 +191,10 @@ pub const Resolver = struct { .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");