fix(sdk): reconnect on dropped socket via ConnectionManager#419
Conversation
The Rust SDK's MoonClient wrapped a redis::aio::MultiplexedConnection, which opens one socket at connect time and NEVER rebuilds it. When the backend drops the connection — a server restart, an idle-timeout reap, a transient network blip — every subsequent command fails with `broken pipe` forever, and the only recovery is to reconstruct the client (i.e. restart the whole consuming process). This wedged Lunaris in production: after a live Moon daemon flip, the lunaris-mcp server's pooled connection went permanently dead and memory.recall returned `broken pipe` on every call until the MCP process was restarted. Switch MoonClient (and all 9 sub-clients, which clone the shared conn) to redis::aio::ConnectionManager, the reconnecting variant. It transparently re-establishes the socket with backoff, is Clone, and shares reconnect state across clones via Arc so a heal on any handle repairs every sub-client. connect_with_timeout preserves the existing per-command response-timeout contract via ConnectionManagerConfig (io-failsafe). Regression test tests/reconnect.rs spawns a real redis-server, drives a PING through a long-lived client, kills the server, restarts it on the same port, and asserts the SAME client recovers within a bounded window: RED (MultiplexedConnection) never recovers; GREEN (ConnectionManager) reconnects. Self-contained (spawns its own server; skips if redis-server is absent). Incidental: fixed pre-existing `moon::` rename leftovers (the crate is now `moondb`) in integration.rs and three doctests, plus a malformed FT.AGGREGATE doctest, so `cargo test -p moondb` (tests + doctests) is green alongside the new regression test. author: Tin Dang
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThe Rust SDK now uses Redis ChangesRedis connection manager migration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant MoonClient
participant Redis
Test->>Redis: Start backend and verify PING
Test->>MoonClient: Create long-lived client
Test->>Redis: Kill and restart backend
MoonClient->>Redis: Retry ping through ConnectionManager
Redis-->>MoonClient: Return PONG
MoonClient-->>Test: Confirm recovery
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…y guard Root cause of the production `memory.recall` -> `broken pipe` wedge: the Moon SDK's MoonClient wrapped a redis::aio::MultiplexedConnection, which opens one socket at connect time and never rebuilds it. After a live Moon daemon flip (or any socket drop / idle reap), every later command failed with `broken pipe` forever until the consuming process (lunaris-mcp) was restarted. Two layers, per the "both" decision: 1. SDK fix (vendor/moon bump 3f99f50 -> 0134323, moon PR pilotspace/moon#419): MoonClient now wraps redis::aio::ConnectionManager, the reconnecting variant — transparently re-establishes the socket with backoff, shares reconnect state across sub-client clones via Arc. connect_with_timeout keeps the per-command response-timeout contract via ConnectionManagerConfig. The pin moves ONLY the SDK (byte-identical to v0.8.1 otherwise) — zero server/embedded-moon delta. 2. Belt-and-suspenders guard (crate::retry): even with a reconnecting manager, the single command that races the reconnect can surface a transient error before the heal completes. `with_conn_retry` retries a read AT MOST ONCE on a classified-transient connection fault (broken pipe / connection reset|refused|aborted|closed / not connected), wired on the two recall reads (vector_search, keyword_search). Non-transient errors (missing index, NotSupported) are never retried; the cap is one retry — never an unbounded loop. Tests: - SDK: vendor/moon/sdk/rust/tests/reconnect.rs spawns a real redis-server, kills+restarts it, asserts a long-lived client recovers (RED on MultiplexedConnection, GREEN on ConnectionManager). - Guard: crate::retry unit tests — classification + retry-once-then-succeed + no-retry-on-non-transient + one-retry-cap + success-path. Red/green proven by neutering the retry branch (discriminating tests flip RED). author: Tin Dang
Problem
The Rust SDK's
MoonClientwrapped aredis::aio::MultiplexedConnection, which opens one socket at connect time and never rebuilds it. When the backend drops the connection — a server restart, an idle-timeout reap, a transient network blip — every subsequent command fails withbroken pipeforever; the only recovery is to reconstruct the client (restart the whole consuming process).This wedged Lunaris in production: after a live Moon daemon flip, the
lunaris-mcpserver's pooled connection went permanently dead andmemory.recallreturnedbroken pipeon every call until the MCP process was restarted.Fix
Switch
MoonClient(and all 9 sub-clients, which clone the sharedconn) toredis::aio::ConnectionManager— the reconnecting variant. It transparently re-establishes the socket with backoff, isClone, and shares reconnect state across clones viaArc, so a heal on any handle repairs every sub-client.connect_with_timeoutpreserves the existing per-command response-timeout contract viaConnectionManagerConfig.Test (red/green)
sdk/rust/tests/reconnect.rsspawns a realredis-server, drives a PING through a long-lived client, kills the server, restarts it on the same port, and asserts the same client recovers within a bounded window:MultiplexedConnection): never recovers → times out.ConnectionManager): reconnects → PONG.Self-contained (spawns its own server; skips cleanly if
redis-serveris absent).Scope
moondbis a standalone crate (not a member of the moon server workspace), so this does not touch the server build. Incidental: fixed pre-existingmoon::rename leftovers (crate is nowmoondb) inintegration.rs+ three doctests + one malformedFT.AGGREGATEdoctest, socargo test -p moondb(tests + doctests) is green.skip-changelog: SDK-internal reliability fix; the commit touches onlysdk/rust/*.Summary by CodeRabbit
New Features
Bug Fixes
Documentation