fix(spanner): enforce READY-only location aware routing and add endpoint lifecycle management

[rahul2393]

Summary

• READY-only bypass routing: isHealthy() now requires READY state instead of "not SHUTDOWN and not
  TRANSIENT_FAILURE". IDLE/CONNECTING endpoints silently fall back to the default host without emitting
  skipped_tablets.
• State-aware skipped_tablets: only TRANSIENT_FAILURE endpoints are reported in skipped_tablets so the server can refresh the client cache. Other non-ready states (IDLE, CONNECTING, absent) are skipped silently.
• Non-creating lookup path: added getIfPresent() to ChannelEndpointCache so the foreground request path never triggers blocking endpoint creation.
• Affinity fix: affinityEndpoint() uses getIfPresent() + READY check, falling back to default instead of
  forcing traffic to a stale replica.
• Raw channel creation: GrpcChannelEndpoint now uses createDecoratedChannelBuilder().build() instead of getTransportChannel(), bypassing the GAX ChannelPool wrapper that does not support getState(). The grpc-gcp dynamic channel pool is preserved since it is applied via the channelConfigurator.
• EndpointLifecycleManager (new): periodically checks channel connectivity state and uses getState(true) to warm up IDLE channels without sending application RPCs. READY channels are skipped (no-op probe). Tracks per-endpoint lastRealTrafficAt for idle eviction after 30 minutes, and requestEndpointRecreation() is called from the routing path when an evicted endpoint is needed again.
• TRANSIENT_FAILURE eviction: endpoints that report TRANSIENT_FAILURE for 3 consecutive probe cycles are evicted and their channel pools shut down. They are recreated on demand when the routing cache references them again.
• Stale endpoint cleanup: on each cache update, ChannelFinder collects all active server addresses across cached tablets and reports them to the lifecycle manager. Endpoints no longer referenced by any tablet (e.g. after a tablet migrates from server1:15000 to server2:15000) are evicted immediately instead of lingering until idle eviction.
• Stale endpoint detection: shouldSkip() detects shutdown channels from evicted endpoints, clears the cached reference, and re-lookups from the cache so routing picks up recreated endpoints.

[gemini-code-assist]

Code Review

This pull request introduces the EndpointLifecycleManager to manage the lifecycle of location-aware routing endpoints, including background probing, traffic tracking, and idle eviction. It updates the routing logic in KeyRangeCache and KeyAwareChannel to be state-aware, ensuring only READY endpoints are utilized and TRANSIENT_FAILURE states are reported. Feedback suggests expanding the session extraction logic to include partition requests and addresses a potential concurrency bottleneck in the single-threaded executor used for background endpoint creation.

[gemini-code-assist]

The ScheduledExecutorService is created with a single thread. While most operations are asynchronous gRPC calls, endpointCache.get(address) (called in createAndStartProbing) can be a blocking operation depending on the implementation of the ChannelEndpointCache. If many endpoints are being created simultaneously, this single thread could become a bottleneck. Consider using a small pool of threads or ensuring that endpointCache.get is non-blocking.

[olavloite]

This seems to be a 'grow only' map. We add new ChannelFinder keys to it, but never remove any. In theory, that means that if we open many different databases, this would create a (small) memory leak.

Also, why is the key type Object?

[rahul2393]

Good catch on both points. Fixed:

• Key is now String (the database resource path), which is naturally stable — one entry per database regardless of ChannelFinder recreation.
• KeyAwareChannel now uses a ReferenceQueue to detect when a SoftReference is cleared, and calls lifecycleManager.unregisterFinder(databaseId) to remove the entry and evict any endpoints that are no longer referenced by remaining finders.
• No strong reference to ChannelFinder is held by the lifecycle manager.

[olavloite]

I think that this method has a potential race condition, as one EndpointLifecycleManager is shared across multiple DatabaseClients (assuming that the DatabaseClient instances were created from the same Spanner instance).

This method reads from the endpoints map to determine which endpoints to evict. At the same time, another thread could be adding endpoints to this map in ensureEndpointExists(..), but that endpoint might not yet be registered as active. That will cause it to be evicted here again directly.

[rahul2393]

Fixed. ensureEndpointExists is now private and only called from inside updateActiveAddresses under activeAddressLock. The method atomically: creates endpoint state → registers the active address set → computes stale endpoints → evicts. This guarantees a newly created endpoint is always in the active set before any stale-eviction check can see it.

Also fixed a subtle bug discovered during testing: the scheduler task was previously submitted from inside
ConcurrentHashMap.computeIfAbsent, which meant the scheduler thread could run before computeIfAbsent published the entry (the node isn't visible until the mapping function returns). Background creation tasks are now scheduled after computeIfAbsent returns.

[olavloite]

I (or actually Gemini) thinks that there is a potential concurrency issue in this method:

Imagine this scenario:

1. ensureEndpointExists is called, adds the address to the endpoints map, and schedules createAndStartProbing.
2. The first attempt fails, so it schedules a retry in 60 seconds. During that 60 seconds, a cache update happens, the address is no longer referenced, and updateActiveAddresses calls evictEndpoint(address). This removes the address from the endpoints map.
3. Now the 60-second timer expires, and the retry task runs createAndStartProbing:
   1. Line 227: endpointCache.get(address) is called and it succeeds this time. A gRPC channel is created and put into the endpointCache.
   2. Line 229: It calls startProbing(address).

In startProbing:

243:     EndpointState state = endpoints.get(address);
244:     if (state == null || isShutdown.get()) {
245:       return; // Triggers here because evictEndpoint removed it!
246:     }

It sees state is null and returns.

The Bug: The channel was created in endpointCache (Line 227), but because it returned early on line 245, it was never added back to the endpoints map of EndpointLifecycleManager. Since it is not in the endpoints map, it will never be checked for idle eviction and never be probed. It is now leaked forever in the endpointCache.

[rahul2393]

Fixed. createAndStartProbing now re-checks endpoints.containsKey(address) after endpointCache.get(address) completes.
If the endpoint was evicted during channel creation, it immediately calls endpointCache.evict(address) to clean up the leaked channel. The same guard is applied before scheduling retries on failure.

[olavloite]

This try block should probably catch all possible errors (or maybe even put a try block around all code in this method). If a scheduled task fails with an uncaught exception, then all future scheduled tasks will be cancelled. Meaning that a single unexpected error in this method will cause all probes of a given endpoint to stop.

[rahul2393]

Fixed. The entire probe() method body is now wrapped in try-catch(Exception) so an uncaught exception won't cause ScheduledExecutorService to cancel all future runs of that task.

[olavloite]

I think that you should consider whether having a single executor with a single thread for all background tasks is the best choice here. Any operation that turns out to be slow (e.g. a slow network lookup) could block all other operations for a long time.

[rahul2393]

Increased to 2 threads. endpointCache.get() can involve a network-level channel setup, and a slow creation should not block probes and eviction checks for other endpoints.

[olavloite]

I think that this call can also cause a race condition (or maybe more in general: the lack of synchronization in this class). In this case, you could end up with a task that probes for an endpoint that has been evicted. And if that endpoint is added again later, then there will be two tasks for probing that endpoint. Imagine this sequence:

1. Thread A is in startProbing. It gets the state object (line 243) but has not yet executed line 253 to schedule the task.
2. Thread B runs evictEndpoint, which calls stopProbing.
3. Thread B reads state.probeFuture. Since Thread A hasn't set it yet, it is null. So Thread B skips the cancellation block.
4. Thread B returns to evictEndpoint and calls endpoints.remove(address). The endpoint is now gone from the map.
5. Thread A resumes and executes line 253. It schedules the periodic task and saves the Future in state.probeFuture.

There is now a periodic task scheduled in the scheduler for an endpoint that has been evicted.

[rahul2393]

Fixed. Both startProbing and stopProbing now synchronize on the EndpointState instance when accessing probeFuture. startProbing also re-checks endpoints.containsKey(address) inside the synchronized block to detect concurrent eviction. shutdown() similarly synchronizes on each state before cancelling.

[olavloite]

The feature LGTM.

But we need to get rid of all the Thread.sleep(..) calls in the tests. It is both a potential source for flaky tests, and something that increases test/build time (and it is typically something that 'spreads' through the code, i.e. if one test is 'allowed' to use Thread.sleep(..), then other tests assume that they can also do that)

[olavloite]

nit: (for another time, just ignore now) prefer the use of Strings.isNullOrEmpty(..)

[olavloite]

Sorry, I never got to the tests during the initial review. It is highly preferable to find some other way than Thread.sleep(..) to test this kind of thing, especially in unit tests. Maven does not run unit tests in parallel, and if many tests do this kind of thing, then the entire test suite gets slower and slower over time.

Is it possible to use a latch (or a set of latches) to guarantee that the scheduler has executed? Or do some active polling?

Especially as I see that many other tests further down are also using Thread.sleep(..).

[rahul2393]

I removed the Thread.sleep(...) calls from EndpointLifecycleManagerTest and replaced them with bounded waiting on observable conditions instead of fixed delays. That keeps the tests deterministic, avoids unnecessary wall-clock delay, and removes the flakiness risk from scheduler timing.