carlos / platform Public

Round 7 review of 5a67a0b4.

| Previous finding | Disposition | Evidence from revision 7 | |---|---|---| | R3-1. Expiry/install races | Resolved | §5.3 retains expiry checks before download and installation, with genMu serializing selection, expiry and cleanup. | | R3-2. Unprotected console Locate | Resolved | §5.4 retains locked Swappable.Locate and assigns the wrapper to Config.GeoIP. | | R3-3. Coordinate-only results | Resolved | §8.1a accepts coordinate-only hits; §9 counts their country as _unknown. | | R3-4. Heartbeat freshness | Resolved | §5.5 retains status.BoxStale and the jittered-heartbeat test. | | R1-4, carried forward: orphan destruction | Resolved | §3 retains startup and daily orphan sweeps, followed by §15’s noncurrent-version expiration. | | R1-16, carried forward: rollout verification | Partly resolved | §5.5 defines retirement by deleting the edges record, but defers the required CLI/API operation; it also continues listing the retired box’s heartbeat for seven days. Make retirement available through the CLI and explicitly exclude retired boxes from readiness. | | R4-1. Duplicate tunnel analytics | Resolved | §§8.4 and 9 require zero remote observations and exactly one at the hub. | | R4-2. Publication deadline across backhaul | Resolved | §4.2 retains staging followed by a 202 response and a polled publication job. | | R5-1. Concurrent upload staging | Resolved | §4.2 reserves the mutex before reading the body and transfers ownership through job cleanup. | | R6-1. Plaintext tunnel cache mismatch | Partly resolved | D2 and §§8.2–8.4 preserve the hub’s answer on every tunnel transport, resolving the mechanism; §16 still instructs plan 1 to implement a “TLS-only tunnel mark.” Update that instruction to match D2. | | R6-2. Abandoned upload staging | Resolved | §4.2 specifies a dedicated staging directory, startup cleanup before admission, and restart tests covering staging and jobs. |

1. P2 — §§3–4.1, 5.3: publishing an already-expired database replaces a usable database with one every edge must reject. The publication checks never reject a GeoLite2 candidate whose build is already more than 30 days old. Such a file can pass verification, the country probe and the shrink check, replace the pointer, and cause the healthy previous blob to be swept. Under §5.3, edges then clear their readers instead of retaining the healthy generation.

Evidence: geoippublish.go:144 validates and publishes without checking candidate expiry; geoippublish.go:182 replaces the pointer and sweeps. The expiry predicate already exists at pointer.go:92.

Fix: reject expired candidates before publication, and recheck immediately before pointer replacement because upload can cross expiry. -force must not bypass this check. Test that refusal preserves the previous pointer and blob.

1. P2 — §§4.1, 5.4, 6–8: source sanitization can make a valid local hit become off downstream. §6 permits sanitization to produce an empty source, but Snapshot promises a nonempty source whenever a reader is loaded. A structurally valid custom database with a nonempty type composed entirely of discarded characters passes the specified publication checks. Ingress can then emit its country while forwarding an internal off state. A tunnelled app receives no country although the hub’s cache matches and stores against that country.

Evidence: mmdb.go:72 can sanitize a nonempty type to empty. answer.go:54 preserves the hit, while answer.go:104 converts its hop state to off when the source fails validation.

Fix: require every loaded reader’s sanitized source to satisfy the wire grammar. Reject unsuitable databases during publication and override loading, or define a valid fallback identifier used consistently everywhere. Test local, backhaul and tunnel equivalence for this metadata case.

1. P2 — §§9, 13: folding sources into _other loses the attribution history the spec requires. Manual publication accepts database types beyond the four feed products. After more than eight types contribute to a retained range, the prescribed source cap can fold DB-IP or GeoLite2 counts into _other. Their country counts remain, but the source identity needed to display their credit is gone. This contradicts §9’s historical-attribution guarantee and can omit required attribution. DB-IP licensing terms.

Evidence: pointer.go:102 derives attribution from the database-type prefix and returns none for _other. Existing bounded analytics summaries discard source keys when folding overflow: topk.go:45.

Fix: retain attribution provenance separately from bounded source counts—for example, persistent DB-IP/MaxMind flags merged by union. Test overflow across flushes, boxes and days, followed by off.

1. P3 — §§4.1, 6: the specified interfaces differ from those already committed. §4.1 declares PutFrom with io.Reader; the implemented interface requires io.ReadSeeker. §6 declares both metadata accessors as returning uint; the implementation returns time.Time for BuildEpoch and uint64 for NodeCount. Subsequent plans following these signatures will disagree with existing callers and implementations.

Evidence: store.go:248, mmdb.go:94, and mmdb.go:102.

Fix: reconcile the spec’s signatures with the committed APIs, including the Unix-seconds conversion when writing build_epoch.

Verdict: revise.