avoid calling nominatum when we are being asked for crap

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);

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");