Skip to content

fix(shared): dispatch redis TTL and Lua calls on the detected client - #29

Merged
ExorTek merged 1 commit into
masterfrom
fix/shared-redis-dialect
Aug 6, 2026
Merged

fix(shared): dispatch redis TTL and Lua calls on the detected client#29
ExorTek merged 1 commit into
masterfrom
fix/shared-redis-dialect

Conversation

@ExorTek

@ExorTek ExorTek commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Stacked on #28.

The internal setWithTTL helper tried the ioredis argument form and fell back only if it threw:

try   { await client.set(key, value, 'PX', px); }
catch { await client.set(key, value, { PX: px }); }

node-redis does not throw on that call. It accepts it, ignores the trailing arguments, and stores the key with no expiry — so the fallback never ran and the TTL was silently dropped.

Measured against a real server:

authorization code saved with a 60s TTL
  ioredis     PTTL = 60000
  node-redis  PTTL = -1      (never expires)

Everything written through this helper lost its lifetime on node-redis: OAuth 2 authorization codes, PAR request URIs, device codes, opaque entries. This is the quietest defect in the set — nothing throws, nothing logs.

incr-store had the same problem in its eval call, where it at least failed loudly because Redis rejects a script invoked with no keys.

Changes

  • detectDialect and evalScript added to the shared redis helpers, so there is one answer to which convention a client wants.
  • Both call sites routed through it.
  • incr reports driver failures through the binding package's wrap, so callers get their own error class and a code instead of a bare driver reply.
  • Fake clients in the unit suites now declare which driver they imitate, and the shared suite asserts both argument forms rather than one.

Copilot AI lite review requested due to automatic review settings August 6, 2026 09:10

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@ExorTek
ExorTek force-pushed the fix/shared-redis-dialect branch from 9fda475 to 713d42a Compare August 6, 2026 09:14
Base automatically changed from fix/jwt-redis-dialect to master August 6, 2026 09:17
`setWithTTL` tried the ioredis positional form and fell back to the
options form only if the first call threw. node-redis does not throw on it:
it accepts `set(key, value, 'PX', ms)`, ignores the trailing arguments, and
stores the key with no expiry. The fallback therefore never ran, and every
caller of this helper silently lost its TTL on node-redis — including the
oauth2 authorization-code, PAR and device-code stores, where the whole point
of the write is that it expires quickly.

`incr-store` had the same problem in its `eval` call. That one at least
failed loudly, because Redis rejects a script invoked with no keys.

Adds `detectDialect` and `evalScript` to the shared redis helpers so there is
one answer to "which convention does this client want", and routes both call
sites through it. `incr` now also reports a driver failure through the
binding package's `wrap`, so callers get their own error class and a `code`
to branch on instead of a bare driver reply.

The fake clients in the unit suites now declare which driver they imitate
(ioredis exposes `status`), since that is what the code keys on, and the
shared suite asserts both argument forms rather than just one.
@ExorTek
ExorTek force-pushed the fix/shared-redis-dialect branch from 713d42a to 12ae013 Compare August 6, 2026 09:17
@ExorTek
ExorTek merged commit 828f4ae into master Aug 6, 2026
3 checks passed
@ExorTek
ExorTek deleted the fix/shared-redis-dialect branch August 6, 2026 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants