carlos / platform Public

| Round 1 finding | Disposition at ee3855fc | |---|---| | R1-1 — Readiness without established database state | Resolved. Unknown pointers block readiness; off and expired pointers require empty mirrors. See internal/console/apisystemgeoip.go:140,345,366. | | R1-2 — off waits beyond caller timeout | Resolved. Off uses TryLock, and the API returns 409 while publishing. See internal/console/geoippublish.go:342. | | R1-3 — Unbounded console pointer reads | Resolved. readPointerCapped streams at most 64 KiB + 1 and preserves the conditional-write ETag. See internal/console/geoippublish.go:158. | | R1-4 — Recovery generation deleted after failed synchronization | Partly resolved; P2 remains. internal/edge/geoipmirror.go:418,438–442 retains only the immediately previous generation. With durable A followed by B and C while directory synchronization keeps failing, installing C deletes A. A crash can still recover current=A with A missing. Retain every potentially recoverable generation until synchronization succeeds, or suspend further installations. Test three generations with failures at both directory-sync sites. | | R1-5 — Rejected-upload test does not establish zero reads | Resolved at the reservation boundary. The new TestStageNeverReadsWhenTheSlotIsHeld passes a panicking reader directly to Stage. The original HTTP test remains weak, but the new test detects reads before reservation. See internal/console/geoippublish_test.go:375. | | R1-6 — Countries overflow ordering | Resolved. _other survives sorting and becomes “other” during rendering. See internal/console/analyticsview.go:415,425. |

New findings:

1. P2 — Feed status writes bypass the 64 KiB read cap. internal/console/geoipfeed.go:155: writeStatus calls Store.Get solely for the ETag, reading the entire existing feed.json into memory. An oversized object is rejected by readStatus, then read without bounds at the end of the same check, potentially exhausting console memory. Obtain the ETag through a capped streaming read, preferably retaining it from the initial status read.

1. P2 — Timed-out feed checks never persist their failure count. internal/console/geoipfeed.go:125,336: the download and final status write share the 15-minute context. When a provider stalls until that deadline, the subsequent S3 read/write receives an already-cancelled context. Repeated timeouts therefore never accumulate seven recorded failures; a fresh deployment can remain without a database and without a feed alert indefinitely. Reserve time for status persistence or give it a separate bounded context. Test repeated deadline failures using a store that honors cancellation.

1. P2 — An unreadable feed status clears an ongoing failure alert. internal/edge/geoipmirror.go:488–498: any read or parse error replaces the last known feed state with false, "". With reason 5 firing and no other reason present, one transient bucket failure produces code 0 and sends a recovery notification. Another successful read must then sustain the unchanged failure for another 30 minutes. Preserve the previous state on read/parse failures; clear it only after a valid healthy status or confirmed deletion.

1. P2 — An unusable mirror directory prevents the “not loaded” alert. internal/edge/geoipmirror.go:253–257: poll returns on MkdirAll failure before reading the pointer or feed status. On a fresh box with an unwritable directory, pointerPresent remains false and wantSince remains zero, so a published database can remain unloaded indefinitely without reason 4 firing. Read and retain the control-plane state independently of local directory preparation, then report the installation failure.

1. P2 — Published alert reasons never change while GeoIP remains unhealthy. internal/metrics/alert.go:181–190,264, internal/host/metrics_loop.go:500: the fraction evaluator preserves Value while firing, and the host publishes only transitions. An alert raised at GeoLite2 day 21 retains reason 2 after day 30, even after the database is removed and the mirror returns higher-priority reason 3. Persist changes to the active GeoIP reason without clearing the alert or restarting its sustain window.

Validation was limited to static review. Targeted tests could not start because Go’s work directory was read-only; cross-compilation remains unverified.

Verdict: revise.