Skip to content

Use shared toolhive-core redis client for session storage#5324

Merged
reyortiz3 merged 1 commit into
mainfrom
worktree-use-thv-core-redis-package-for-session-storage
May 19, 2026
Merged

Use shared toolhive-core redis client for session storage#5324
reyortiz3 merged 1 commit into
mainfrom
worktree-use-thv-core-redis-package-for-session-storage

Conversation

@reyortiz3
Copy link
Copy Markdown
Contributor

Summary

pkg/transport/session carried its own Redis connection layer (RedisConfig, SentinelConfig, RedisTLSConfig, validateRedisConfig, buildRedisClient, buildRedisTLSConfig, timeout default constants) that duplicated what toolhive-core v0.0.21 ships. PR #5318 migrated the auth server onto the shared package; vmcp session storage and the proxy-runner session storage are the remaining in-repo consumers, so this PR finishes that migration.

  • Replace the local RedisConfig/SentinelConfig/RedisTLSConfig types and the validate*/build* helpers with tcredis.Config and tcredis.NewClient. NewRedisStorage and NewRedisSessionDataStorage now take (ctx, tcredis.Config, keyPrefix, ttl); they keep the session-specific invariants (key prefix required, must end with :, ttl > 0) and delegate connection-mode validation, timeout defaults, TLS plumbing, and Ping verification to the shared package.
  • NewManagerWithRedis gains the same keyPrefix parameter so the wrapper matches the underlying storage signature. The vmcp server and proxy runner call sites build a tcredis.Config in place and pass keyPrefix separately; RedisPasswordEnvVar stays in the session package because pkg/runner reads it.
  • Tests: TestValidateRedisConfig is removed (topology checks now live in tcredis) and replaced by TestNewRedisStorageInvariants, which covers the session-specific cases (key prefix and ttl). TestNewRedisStorageACLAuth is preserved with the new signature. The "invalid TLS CA cert" manager subtest is dropped — it now exercises tcredis code.

Net result: -298 / +165 lines across 9 files; one source of Redis-connection truth shared with the auth server.

Type of change

  • Bug fix
  • New feature
  • Code refactor
  • Configuration change
  • Documentation update
  • Other

Test plan

  • Manual verification by running go test ./pkg/transport/session/... ./pkg/vmcp/server/... ./pkg/vmcp/server/sessionmanager/... ./pkg/runner/retriever/... — all pass.
  • golangci-lint on the changed packages reports 0 issues.
  • Confirmed pkg/runner's TestRunConfigBuilder_MetadataOverrides failure reproduces on main without this diff, so it is pre-existing and unrelated.

🤖 Generated with Claude Code

pkg/transport/session carried its own Redis connection layer (RedisConfig,
SentinelConfig, RedisTLSConfig, validateRedisConfig, buildRedisClient,
buildRedisTLSConfig, timeout defaults). The auth-server migration in
#5318 demonstrated the pattern for replacing that duplicated code with
toolhive-core/redis; vmcp session storage and the proxy-runner session
storage are the remaining consumers in this repo.

Replace the local RedisConfig/SentinelConfig/RedisTLSConfig types and the
validate/build helpers with tcredis.Config and tcredis.NewClient.
NewRedisStorage and NewRedisSessionDataStorage now take
(ctx, tcredis.Config, keyPrefix, ttl); the constructors keep the
session-specific invariants (key prefix required, must end with ':',
ttl > 0) and delegate connection-mode validation, timeout defaults, TLS
plumbing, and Ping verification to the shared package.

NewManagerWithRedis grows the same keyPrefix parameter so the wrapper
matches the underlying storage signature. The vmcp server and proxy
runner call sites build tcredis.Config in place (Addr, Password, DB) and
pass keyPrefix separately; RedisPasswordEnvVar stays in the session
package because pkg/runner reads it.

Tests: TestValidateRedisConfig is removed (topology checks now live in
tcredis); TestNewRedisStorageInvariants replaces it with the
session-specific cases (key-prefix and ttl). TestNewRedisStorageACLAuth
keeps its coverage but uses the new signature. The "invalid TLS CA cert"
manager subtest is dropped (now exercising tcredis code).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the size/M Medium PR: 300-599 lines changed label May 19, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2026

Codecov Report

❌ Patch coverage is 62.50000% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 68.34%. Comparing base (2ce3ef7) to head (9a463b3).

Files with missing lines Patch % Lines
pkg/runner/runner.go 0.00% 5 Missing ⚠️
pkg/vmcp/server/server.go 0.00% 5 Missing ⚠️
...kg/transport/session/session_data_storage_redis.go 60.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5324      +/-   ##
==========================================
- Coverage   68.35%   68.34%   -0.01%     
==========================================
  Files         620      620              
  Lines       63316    63255      -61     
==========================================
- Hits        43278    43230      -48     
+ Misses      16808    16798      -10     
+ Partials     3230     3227       -3     

☔ 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.

@reyortiz3 reyortiz3 merged commit a13e5ea into main May 19, 2026
45 checks passed
@reyortiz3 reyortiz3 deleted the worktree-use-thv-core-redis-package-for-session-storage branch May 19, 2026 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Medium PR: 300-599 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants