fix(android): Treat an unpopulated connection cache as stale (JAVA-717) - #6029
fix(android): Treat an unpopulated connection cache as stale (JAVA-717)#6029runningcode wants to merge 3 commits into
Conversation
📲 Install BuildsAndroid
|
2d9f213 to
c2f7b51
Compare
00b4937 to
7eae6db
Compare
7eae6db to
e20e3fa
Compare
e20e3fa to
2b35532
Compare
aa53f2e to
7255d5e
Compare
2b35532 to
76fef57
Compare
7255d5e to
a372da0
Compare
76fef57 to
81e7109
Compare
bec70fb to
686cc80
Compare
81e7109 to
47bf886
Compare
686cc80 to
655adfb
Compare
47bf886 to
c45669e
Compare
655adfb to
0969abb
Compare
`When network is active but not connected with permission, return DISCONNECTED` mocked an active network reporting isConnected=false alongside NetworkCapabilities describing a validated WiFi link. Those describe opposite worlds. It passes today only because the empty connection cache reads as fresh for the first two minutes of every boot (JAVA-717), which forces the legacy activeNetworkInfo path where the capability mocks are never consulted. buildInfo reports API 24, so once that bug is fixed the provider reads capabilities and the test would fail for a reason that has nothing to do with what it is named after. Fixing the mocks first keeps that failure from being buried in the commit that fixes the cache. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
lastCacheUpdateTime used 0 for "never populated" while being compared against SystemClock.uptimeMillis(), which starts at 0 at boot. For the first two minutes of every boot the empty cache therefore read as fresh, so getConnectionStatus() skipped updateCache() and fell through to the legacy activeNetworkInfo path instead of reading NetworkCapabilities. The window reopens after every unregisterNetworkCallback(), which reset the field to 0. Switching clocks does not fix this on its own: elapsedRealtimeNanos() also starts at 0 at boot. Any 0-means-unset long compared against a boot-relative clock has the same flaw; only epoch millis made it safe, because there 0 is 1970. The cache now holds a Deadline, so "never populated" is expired by construction and has no numeric value to get wrong. The provider takes an MonotonicClock in place of ICurrentDateProvider, which is what a two-minute TTL wants: it must keep counting while the device sleeps. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c45669e to
0154972
Compare
0xadam-brown
left a comment
There was a problem hiding this comment.
A couple quick comments if you want them; no blockers.
| cachedNetworkCapabilities = null; | ||
| currentNetwork = null; | ||
| lastCacheUpdateTime = timeProvider.getCurrentTimeMillis(); | ||
| cacheFreshUntil = Deadline.after(ticker, CACHE_TTL_MINUTES, TimeUnit.MINUTES); |
There was a problem hiding this comment.
Seeing this in action, I wonder if there isn't a better name than Deadline.after()? ("after" makes me think that perhaps we're offsetting the start of the deadline until the time we pass to after())
Thoughts about until()? I prefer it b/c my mind defaults to thinking that whatever I'm returned is "valid"; a deadline that's passed isn't; and until() points to the valid time segment.
(I also considered at(), which reads nicely here, but (I think) mostly because my mind is sneaking in the idea of a clock, and not reading this as a pure duration.)
Happy to defer to you...
|
|
||
| private boolean isCacheValid() { | ||
| return (timeProvider.getCurrentTimeMillis() - lastCacheUpdateTime) < CACHE_TTL_MS; | ||
| return !cacheFreshUntil.hasPassed(); |
There was a problem hiding this comment.
l: We could add a hasNotPassed() method to Deadline, as well. I bet it'll get a lot of use.
PR Stack (Clock semantics hardening)
📜 Description
AndroidConnectionStatusProviderused0to mean "cache never populated", and compared it againstSystemClock.uptimeMillis():uptimeMillis()is 0 at boot, so0does not mean "unset" — it means "populated at boot". For the first two minutes of every boot, an empty cache reads as fresh.The cache now holds a
Deadline, so "never populated" is expired by construction and has no numeric value to get wrong.Stacked on #6028, which adds the clock types this uses.
💡 Motivation and Context
💚 How did you test it?
A regression test asserts that a provider constructed at tick 0 populates the cache before reading it. It fails if the deadline is constructed fresh instead of passed, which is the shape of the original bug.
📝 Checklist
sendDefaultPIIis enabled.