Drop RoutingStorage and Touch: GETEX already handles TTL refresh

[yrobla]

Summary

The RFC for horizontal proxyrunner scaling (THV-0047) originally proposed an LRU cache in front of Redis to reduce hot-path round-trips. After reviewing the implementation, that approach was reconsidered.

RedisStorage.Load already uses GETEX, which atomically returns the session data and refreshes the Redis TTL in a single command. That makes every piece of the proposed design unnecessary:

• The LRU cache never avoided a Redis RTT — every cache hit still called remote.Touch (Redis EXPIRE) to prevent stale reads after TTL expiry. The LRU only saved deserializing a small session payload, which is negligible compared to the RTT.
• Touch on the Storage interface is a breaking change for all implementors and leaks a Redis-specific TTL concern onto a shared interface.
• RoutingStorage added cross-replica staleness edge cases, LRU eviction/recovery logic, and a generated mock — all complexity with no measurable benefit.

This PR removes those additions. GETEX handles TTL refresh as an internal Redis concern, invisible to callers. No interface changes are needed.

Closes #4210

Type of change

• Bug fix
• New feature
• Refactoring (no behavior change)
• Dependency update
• Documentation
• Other (describe):

Test plan

• Unit tests (task test)
• Linting (task lint-fix)

Does this introduce a user-facing change?

No.

Special notes for reviewers

If Redis RTT becomes a measurable bottleneck in the future, a local TTL cache is still the right approach — but the design would look different: no per-hit remote calls, bounded staleness via local expiry, GETEX on cache miss. That is a separate decision backed by profiling data.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cbae45d38a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Copilot]

Pull request overview

Adds a new two-tier session.Storage implementation that fronts a remote storage backend (e.g., Redis) with a bounded in-memory LRU to reduce remote round-trips while preserving remote durability/authority.

Changes:

• Introduce RoutingStorage implementing the session.Storage interface with local LRU + remote fallback.
• Add a comprehensive unit test suite covering cache hits/misses, eviction, delete semantics, remote failure behavior, and constructor panics.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
pkg/transport/session/storage_routing.go	New two-tier storage implementation using hashicorp/golang-lru for local caching and a delegated remote Storage.
pkg/transport/session/storage_routing_test.go	Unit tests validating caching behavior, remote delegation, eviction, and concurrency sanity.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 69.10%. Comparing base (a685e89) to head (fb3e7df).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4294      +/-   ##
==========================================
+ Coverage   68.85%   69.10%   +0.25%     
==========================================
  Files         473      477       +4     
  Lines       47899    48135     +236     
==========================================
+ Hits        32979    33264     +285     
- Misses      12279    12287       +8     
+ Partials     2641     2584      -57     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[jerm-dro]

Do we need the LRU cache?

The original RFC I proposed mentioned LRU caching, but after seeing this implementation I've reconsidered. The PR correctly identifies that we must refresh Redis TTLs on access — something the RFC didn't account for. That constraint changes the calculus.

Every Load cache hit still calls remote.Touch (Redis EXPIRE), so every request pays a Redis RTT regardless. The LRU only saves deserializing the session payload, but sessions are small and GET and EXPIRE are both O(1) — the RTT dominates.

That means we're taking on significant complexity for a marginal saving: a new Touch method on the Storage interface (breaking change for all implementations), cross-replica staleness edge cases, LRU eviction/recovery logic, and a generated mock.

Suggestion: drop the LRU and have RedisStorage.Load use GETEX instead of GET. GETEX atomically returns the data and refreshes the TTL — the TTL refresh becomes an internal Redis concern, invisible to callers. No Touch, no interface change, no cache invalidation bugs. The RoutingStorage, mock, and Touch additions can all be removed.

If we later find Redis RTT is a bottleneck, we can add a local TTL cache in front, but that would be a different design (no per-hit remote calls, bounded staleness via local expiry, GETEX on cache miss).

[yrobla]

Closing — no changes needed. RedisStorage.Load already uses GETEX on main, which atomically returns session data and refreshes the Redis TTL in a single round-trip. The LRU cache, Touch interface method, and RoutingStorage were all predicated on TTL refresh being a separate concern, but it isn't. Nothing to merge.