| 1 | | Round 1 finding | Disposition at `ee3855fc` | |
| 2 | |---|---| |
| 3 | | 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`. | |
| 4 | | R1-2 — `off` waits beyond caller timeout | **Resolved.** `Off` uses `TryLock`, and the API returns 409 while publishing. See `internal/console/geoippublish.go:342`. | |
| 5 | | 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`. | |
| 6 | | 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. | |
| 7 | | 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`. | |
| 8 | | R1-6 — Countries overflow ordering | **Resolved.** `_other` survives sorting and becomes “other” during rendering. See `internal/console/analyticsview.go:415,425`. | |
| 9 | |
| 10 | New findings: |
| 11 | |
| 12 | 1. **P2 — Feed status writes bypass the 64 KiB read cap.** |
| 13 | `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. |
| 14 | |
| 15 | 2. **P2 — Timed-out feed checks never persist their failure count.** |
| 16 | `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. |
| 17 | |
| 18 | 3. **P2 — An unreadable feed status clears an ongoing failure alert.** |
| 19 | `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. |
| 20 | |
| 21 | 4. **P2 — An unusable mirror directory prevents the “not loaded” alert.** |
| 22 | `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. |
| 23 | |
| 24 | 5. **P2 — Published alert reasons never change while GeoIP remains unhealthy.** |
| 25 | `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. |
| 26 | |
| 27 | Validation was limited to static review. Targeted tests could not start because Go’s work directory was read-only; cross-compilation remains unverified. |
| 28 | |
| 29 | Verdict: revise. |
| 30 | |
amadan