geolocation: filter cached dzd measurements

[ben-dz]

geoprobe-agent should do some filtering on DZD cached measurements so that we don't get high bounces.

Keep the lowest dzd reference that's valid, but then keep a backup one that is somewhat newer so that we aren't left without one when the low one expires. Perhaps never replace the lowest one until it expires after the full timeout, but always keep a separate copy of the lowest in the most recent half-timeout interval. That way when a full timeout expires, there's a new decent measurement with at least a half-timeout lifespan left.

[ben-dz]

Architecture review findings:

• Critical: 0
• High: 1
• Medium: 3
• Low: 3

Architecture Review

Critical

None

High

• [Correctness / Stale data]: Get() uses a read lock but the backup-to-best promotion only happens in Put(). If no new offsets arrive for a sender after best expires, Get() will fall through to backup, which may itself have a worse RTT than best had. More importantly, GetBest() considers both best and backup across all senders, but Get() prefers best even if backup has a lower RTT for the same sender. This inconsistency means Get() and GetBest() can disagree about which offset is "best" for a given sender.
  • Location: main.go:224-239 (Get) vs main.go:243-260 (GetBest)
  • Recommendation: Either make Get() also consider both slots and return the lower-RTT non-expired one, or document the intentional semantic difference (best = freshest low-RTT, backup = fallback). Currently the comment says "lowest RTT seen" for both, which is misleading.

Medium

• [Testability / Time-dependent tests]: Multiple tests rely on time.Sleep with tight margins (e.g., 20ms maxAge with 25ms sleep in TestOffsetCache_BackupPromotion). The Put() method calls time.Now() internally, making it impossible to inject a fake clock. This makes tests flaky under CI load.

  • Location: main_test.go:122-146, main_test.go:148-183
  • Recommendation: Accept a func() time.Time (clock function) on offsetCache so tests can control time deterministically. Alternatively, widen the sleep margins significantly.

• [Complexity / Readability]: The Put() method packs multiple concerns into a single function: expiration check, promotion logic, best-vs-backup placement, and half-maxAge staleness refresh. The promotion-on-write design means the cache state depends on when the next write arrives, which is non-obvious.

  • Location: main.go:177-222
  • Recommendation: Add a brief block comment at the top of Put() explaining the two-slot strategy and the promotion invariant. Consider extracting the promotion logic into a (*cachedSender).maybePromote(maxAge) method for readability.

• [Eviction gap]: Evict() only removes map entries when both slots are expired, but it does not nil out individually expired slots. Over time, senders with one expired slot and one valid slot accumulate stale cachedOffset pointers that won't be GC'd until the entire sender entry is evicted.

  • Location: main.go:263-274
  • Recommendation: Inside Evict(), nil out individually expired slots (e.g., if sender.best.expired(...) { sender.best = nil }) to release memory earlier.

Low

• [Naming]: cachedOffset and cachedSender are reasonable names, but the type comment on offsetCache (line 149) still says "stores recent DZD offsets" without mentioning the two-slot best/backup strategy. The comment on backup says "lowest RTT seen in the recent half-maxAge window" but that's not exactly true -- it can also be set when backup is expired or has higher RTT, regardless of the half-maxAge window.

  • Location: main.go:149, main.go:167
  • Recommendation: Update comments to accurately describe the two-slot eviction strategy.

• [Test assertion looseness]: TestOffsetCache_BackupPromotion asserts got.RttNs != 2000 && got.RttNs != 3000 which accepts two different outcomes. Given the deterministic nature of the code (backup 2000 is also expired at 25ms with 20ms maxAge), this test may not be validating what it intends -- both best (500) and backup (2000) could be expired by the time the new Put(3000) runs, meaning 3000 becomes best directly rather than testing promotion.

  • Location: main_test.go:132-145
  • Recommendation: Use a longer maxAge or shorter initial sleep so that backup is genuinely non-expired when promotion is triggered. Assert a single expected value.

• [Backward compatibility]: The Get() method previously returned the most recent offset for a sender; now it returns the lowest-RTT non-expired offset. Callers that relied on recency semantics will silently get different behavior.

  • Location: main.go:224-239
  • Recommendation: This is internal code so the risk is low, but verify all callers (e.g., runOffsetListener, runMeasurementCycle) are compatible with the new "best RTT" semantics. In this case they are -- Get() isn't called in the hot path, only GetBest() is used for composite offset generation.

Summary

A solid improvement that prevents a single high-RTT measurement from evicting a known-good low-RTT offset. The main concerns are (1) a subtle semantic inconsistency between Get() and GetBest() for per-sender lookups, (2) time-coupled tests that will be flaky under load, and (3) the promotion logic in Put() would benefit from clearer documentation and a small refactor for readability.

[ben-dz]

Security review findings:

• Critical: 0
• High: 0
• Medium: 0
• Low: 2

Security Review

Critical

None

High

None

Medium

None

Low

• Unbounded Cache Growth (Denial of Service): The offsetCache map grows without any size limit. Each unique SenderPubkey adds a new entry, and eviction only runs on a 30-minute timer. A malicious actor sending offsets with many distinct (but authorized) sender pubkeys could grow the map until memory is exhausted. However, the practical impact is limited because offsets must pass parent authority verification and signature chain verification (VerifyOffsetChain) before being cached, which significantly restricts who can write to the cache.

  • Location: controlplane/telemetry/cmd/geoprobe-agent/main.go:177 (Put method)
  • Recommendation: Consider adding a maximum entry count to the cache as a defense-in-depth measure, evicting the oldest or least-recently-used entries when the cap is reached.

• Time-of-Check-Time-of-Use in Get vs Put: The Get method uses RLock and checks expiration, while Put uses a full Lock and also checks expiration to trigger promotion. Between a Get read and a subsequent Put write, the state could change. This is a minor concurrency concern but not exploitable since both paths handle nil/expired entries safely.

  • Location: controlplane/telemetry/cmd/geoprobe-agent/main.go:224-239
  • Recommendation: No action required; the current locking is correct for the use case. Noted for completeness.

Summary

This diff is a low-risk refactoring of an in-memory cache from a single-slot to a two-slot (best/backup) design. All changes are internal to the cache data structure with no new external inputs, no I/O changes, no injection surfaces, and no secrets handling. The existing authentication and signature verification in runOffsetListener (parent authority check + VerifyOffsetChain) is unchanged and continues to gate all cache writes. The only minor concern is the absence of a maximum cache size limit, which is a pre-existing condition not introduced by this diff.