carlos / platform Public

Round 3 review of 5311a1ad.

| Previous finding | Disposition | Evidence from revision 3 | |---|---|---| | R2-1. Failed-hop location | Resolved | §8.2 explicitly makes rejected hops off, including rotation-window and door-backhaul tests. | | R2-2. Unauthenticated tunnel | Resolved | §8.4 grants the tunnel mark only after certificate-verified TLS; plaintext tunnels receive no geo trust. | | R2-3. Strip/authentication ordering | Resolved | §8.1 specifies capture → strip → authenticate → decide → expose before forwarding or caching. | | R2-4. Separate lookup/source snapshots | Resolved | §5.4 returns result, source and availability together under one read lock. | | R2-5. Miss indistinguishable from off | Resolved | §§8.1a and 9 carry authenticated hit/miss/off state and source across both transports. | | R2-6. Unsupported conditional deletion | Resolved | §3 explicitly chooses unconditional deletion under the single-writer mutex. | | R2-7. Missing-pointer suppression of feed alerts | Resolved | §10.4 evaluates feed.json independently of pointer existence; §3 preserves an expired-pointer state. | | R2-8. Unsupported clearing delay | Resolved | §10.4 now specifies immediate clearing. | | R2-9. Console off propagation | Resolved | §§4.2 and 11.3 describe one edge poll followed by one console poll. | | R1-4, carried forward: destruction obligations | Partly resolved | §15 adds noncurrent-version expiration, but §3 leaves completed orphan uploads for a future sweep. After a crash before pointer publication, those blobs remain current versions and are outside that lifecycle action. AWS lifecycle documentation | | R1-7, carried forward: resource bounds | Resolved | §5.3 adds a mirror-pass deadline and moves expiry onto an independent ticker. | | R1-14, carried forward: validation comparisons | Resolved | §4.1 explicitly applies the shrink check whenever database types match, irrespective of origin. | | R1-16, carried forward: rollout verification | Partly resolved | §5.5 covers stale and unreadable listed records, but still substitutes a seven-day root-prefix listing for expected inventory. Fleet heartbeats use fleets/<fleet>/status/boxes/, so they remain omitted: agent.go:3416. |

1. P1 — §5.3: installation can reactivate an expired database, and expiry can evict its fresh replacement. The new-generation path checks size and digest, then installs without checking expiry at commit time. A download that crosses its expiry can therefore become active. If the console cannot mark the bucket pointer expired, subsequent polls can reload the same expired generation after the ticker removes it. Separately, no coordination is specified between checking the loaded generation’s age and calling Swap(nil): the ticker can inspect expired A, the mirror install fresh B, and the ticker then remove B.

Evidence: the independent ticker and unconditional installation/removal sequences are at design.md:326. The underlying reader performs no age check during lookup: mmdb.go:71.

Fix: check candidate expiry immediately before installation. Serialize generation selection, expiry and local cleanup, with expiry conditional on the inspected generation still being current. Keep network downloads outside that lock. Test a download crossing expiry, repeated polls against an unmarked expired pointer, and replacement between expiry inspection and removal.

1. P2 — §§5.4, 6, 11.3: the shared console reader lacks a protected Locate path. Swappable exposes only Lookup and Swap, while the console’s existing region-guess interface requires Locate. Consequently, the proposed wrapper cannot directly replace Config.GeoIP. Retaining or exposing its underlying *MMDB would let region guesses bypass the lock that prevents lookup against a closed mapping.

Evidence: server.go:493 declares GeoIP geo.Locator; regionguess.go:44 calls Locate. The proposed wrapper’s method set is at design.md:369.

Fix: specify Swappable.Locate, holding the same read lock through the existing MMDB.Locate operation and returning a miss when empty. Wire that wrapper into Config.GeoIP; include concurrent region guesses in the swap/close test.

1. P2 — §§6, 8.1a, 9: valid coordinate-only results disappear across hops and tunnels. §6 permits Lookup to succeed with coordinates and no country. §8.1a nevertheless converts an adopted hit without a country into off. The same visitor therefore receives coordinates on a local route but loses them when forwarded. At ingress, §9’s instruction to count a hit’s country also leaves this valid result without a defined country bucket.

Evidence: the conflicting rules are at design.md:439 and design.md:515. Coordinates already succeed independently of country in mmdb.go:88.

Fix: accept a trusted hit containing either a valid country or a valid coordinate pair. Define coordinate-only pageviews as _unknown for country aggregation while retaining their source. Test identical app headers and analytics through local, backhaul and tunnel routes.

1. P2 — §5.5: healthy boxes periodically fail the readiness check. The new five-minute freshness threshold is shorter than the normal heartbeat interval. Every healthy box can therefore become “stale” between scheduled publications, making readiness fluctuate during an otherwise completed rollout.

Evidence: agent.go:2922 schedules heartbeats at SnapshotPeriod + rand.N(time.Minute); fleet.go:119 defines that period as five minutes. Existing readers use the shared BoxStale rule at fleet.go:137.

Fix: use status.BoxStale, or explicitly choose a threshold exceeding the maximum scheduled interval with allowance for publication time. Test readiness just before the next normally jittered heartbeat.

Verdict: revise.