fix(plugin-redis): stop idle refresh from emptying the key list (#1701)#1706
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1407abe750
ℹ️ 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".
| return try executeCommandSync(args) | ||
| } catch let error as RedisPluginError where isConnectionError(error) && !isShuttingDown { | ||
| try reconnectSync() | ||
| return try executeCommandSync(args) |
There was a problem hiding this comment.
When an EOF/IO is reported after Redis has received a write but before the reply reaches the client, this unconditional retry sends the exact same command again. That affects non-idempotent operations routed through executeCommand (e.g. INCR, LPUSH, SADD, or commands inside a transaction), so a transient disconnect can double-apply data changes instead of surfacing an ambiguous failure. Please only reconnect and retry commands that are known safe/idempotent, or reconnect without replaying the failed mutation.
Useful? React with 👍 / 👎.
| freeContextSync() | ||
| try openContextSync(selectDatabase: targetDatabase) |
There was a problem hiding this comment.
When a reconnect attempt fails because Redis is still unavailable, freeContextSync() has already cleared context; subsequent public methods hit the guard context != nil check and throw notConnected before executeCommandSyncRetrying can call reconnectSync again. In the common Redis restart case, the connection therefore remains unable to self-heal on the next command after the server is back, which is the path this change is trying to fix.
Useful? React with 👍 / 👎.
1 participant
Fixes #1701.
Root cause
The connection health monitor pinged every driver with
execute(query: "SELECT 1"). For SQL that is a harmless no-op; for Redis,SELECT 1is a real command that switches the live connection to database index 1. It returns+OK, so the monitor considered the connection healthy while every later refresh scanned the wrong (usually empty) database. That is why entries vanished after the connection sat idle, and why navigating to another database and back fixed it (that re-issuesSELECT).RedisPluginDriver.ping()already sentPINGcorrectly, but it was unreachable:DatabaseDriverhad noping()andPluginDriverAdapternever bridged it, so the monitor fell through to the destructiveSELECT 1.What changed
ping()toDatabaseDriverwith a default that keepsSELECT 1for SQL drivers, bridged it inPluginDriverAdapter, and pointed the health monitor atmainDriver.ping(). Redis now pings withPINGand never mutates the selected database.SCAN 0 MATCH ... COUNT n, which never iterates the cursor (so large keyspaces showed a partial fixed set) and ignoresOFFSET(so paging re-showed the first batch). Both now route throughKEYBROWSE, which iterates the cursor to completion (bounded at 10k), sorts, dedupes, and honorsLIMIT/OFFSET.REDIS_ERR_EOF/REDIS_ERR_IO) the connection now reconnects on the next command, replays AUTH and the selected database, and retries once, matching redis-cli. Previously commands failed until the next health check rebuilt the driver.cachedScan*fields, deduped the hiredis error-message reading, removed stray comments, cleared pre-existing lint debt in touched files.Decisions (deliberately not done)
These were evaluated against the actual code and declined because doing them would trade a correct pattern for risk or regression, not because they were skipped:
DispatchQueueis the correct way to wrap blocking I/O; a plain actor would block Swift's cooperative pool. The@unchecked Sendablehere is encapsulated (single serial queue + lock-guarded state), not a hack.escapeArgumentand the parser tokenizer are a lossless inverse and the C layer already uses explicit-lengthredisCommandArgv, so there is no injection or data-loss bug. Rewriting would overloadexecuteParameterizedapp-wide for no benefit.HGETALLetc. as maps) and regress every result-builder for marginal gain.KEYStyped in the CLI runs literally, like redis-cli; only the browse path matters and it uses SCAN. MULTI/EXEC/DISCARD is Redis's genuine transaction primitive and stays.The namespace tree view and type-aware key opening already exist; this PR fixes the real completeness defect in that flow.
Compatibility
All changes are confined to the app target and the Redis plugin bundle. No
TableProPluginKittypes change, so no plugin ABI bump or registry re-release is needed.Tests
PluginDriverAdapterPingTests: a driver with aping()override routes through it (neverexecute); a driver without one falls back toSELECT 1.RedisQueryBuilderTeststo assert the correctedKEYBROWSEoutput (complete, paginated) instead of the old single-batchSCAN.swiftlint --strictclean on changed files; fullxcodebuild testsucceeds (app + Redis plugin + tests compile and pass).The C-bridge reconnect path compiles but can only be runtime-validated against a live Redis (the standard plugin C-bridge test limitation).