feat(client): add HIMPORT command family with managed fieldset lifecycle - #3381
Conversation
Adds first-class support for the Redis 8.10 HIMPORT command family (hinted hash templates): HIMPORT PREPARE/SET/DISCARD/DISCARDALL, exposed as hImportPrepare/hImportSet/hImportDiscard/hImportDiscardAll and marked @experimental. Fieldsets are server-side session state scoped to one physical connection, so the client owns the fieldset lifecycle: a shared FieldsetRegistry records registrations on the logical client, and a per-connection hook transparently pipelines PREPARE in front of SET on sessions that lack the fieldset or hold a stale field list, replays pending discards before dependent commands, and retries a SET once on 'no such fieldset' for registered fieldsets. DISCARD/DISCARDALL replies are registry-based, giving deterministic semantics across topologies, and a rejected user discard restores the session claims and knocks the synced count back so the discard replays on that connection. On a cluster each serving node client runs the same hook, so sessions prepare and reconcile lazily per node; the eager all_shards fan-out and its response-policy pins arrive with the request-response-policies work. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![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: c01a498639
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| const noop = () => {}; | ||
|
|
||
| const HIMPORT_SESSION_SUBCOMMANDS = new Set(['PREPARE', 'DISCARD', 'DISCARDALL']); |
There was a problem hiding this comment.
When a fieldset is discarded on a pool/cluster/sentinel client, only the connection executing hImportDiscard(All) is cleaned immediately; other connections rely on #executeHimport's lazy reconcile before their next HIMPORT command. Because HIMPORT SET is not rejected here and batched commands go straight to _executeMulti/_executePipeline, a later multi/pipeline routed to a connection that missed the discard can still run SET against the stale server-side fieldset after hImportDiscard(All) has resolved. Either reject HIMPORT SET in batches when managed lifecycle state is dirty or run the same discard reconcile before enqueuing the batch.
Useful? React with 👍 / 👎.
| // reconcile it away instead of silently serving stale SETs. | ||
| const { name, version } = userPrepare; | ||
| if (prepared.get(name) === version) prepared.delete(name); | ||
| if (registry.get(name)?.version === version) registry.discard(name); |
There was a problem hiding this comment.
If a client already has fs registered and a replacement hImportPrepare('fs', ...) is rejected (for example because the new list has duplicate fields), this catch path deletes the registry entry altogether. The server rejected the replacement before changing the existing session fieldset, so the previous registration should remain the source for lazy re-prepare; dropping it makes subsequent hImportSet on a new/reconnected or pooled connection stop auto-preparing fs and fail with no such fieldset even though the last successful registration was still valid.
Useful? React with 👍 / 👎.
| const existing = this.#fieldsets.get(key); | ||
| if (existing !== undefined && fieldsEqual(existing.fields, fields)) return; | ||
| this.#fieldsets.set(key, { | ||
| fields: fields.slice(), |
There was a problem hiding this comment.
RedisArgument field names can be Buffers, but fields.slice() only copies the array and keeps references to the caller's buffers. After a successful hImportPrepare, if the caller reuses or mutates one of those buffers before a reconnect or before another pooled connection lazily prepares the fieldset, the registry replays the mutated bytes and subsequent hImportSet writes values under the wrong field names. Store immutable copies of buffer fields when recording the registry entry.
Useful? React with 👍 / 👎.
| // to the queue front — a non-asap injection would let an asap SET jump its own PREPARE). | ||
| const effectiveAsap = commandOptions?.asap ?? client._commandOptions?.asap; | ||
| const effectiveChainId = commandOptions?.chainId ?? client._commandOptions?.chainId; | ||
| const injectOpts: CommandOptions = { asap: effectiveAsap, chainId: effectiveChainId }; |
There was a problem hiding this comment.
In cluster maintenance/SMIGRATED handling, queued commands are moved by slotNumber (extractCommandsForSlots ignores slotless commands), but the lazy HIMPORT PREPARE injected ahead of an HIMPORT SET drops the main command's slot metadata here. If a slot migrates while the pair is still queued, the SET can be moved to the destination without its PREPARE, so the destination executes a bare HIMPORT SET and fails with no such fieldset; carry commandOptions.slotNumber into injectOpts so the pair stays together.
Useful? React with 👍 / 👎.
| * via the policy layer, and every node client (including MOVED/rediscovered nodes and | ||
| * SMIGRATED destinations) must lazily re-prepare from the same registrations. | ||
| */ | ||
| readonly #himportRegistry = new FieldsetRegistry(); |
There was a problem hiding this comment.
Because each RedisClusterSlots allocates a fresh registry here and RedisCluster.duplicate() cannot pass the parent's instance through, a duplicate made after cluster.hImportPrepare(...) loses all HIMPORT registrations. Direct hImportSet on that duplicate then has no registry entry to lazy-prepare from and fails with no such fieldset, unlike the standalone duplicate path and the command docs' duplicate-sharing guarantee; thread the existing registry into duplicated cluster slots instead of always creating a new one.
Useful? React with 👍 / 👎.
| } else if (command === HIMPORT_DISCARD) { | ||
| const name = String(args[2]); | ||
| const sessionVersion = prepared.get(name); | ||
| if (registry.discard(name)) { |
There was a problem hiding this comment.
When hImportDiscard is called on a closed client, with an already-aborted signal, or when the queue is full, sendCommand rejects before the DISCARD reaches Redis, but the registry has already removed the fieldset here and the catch path intentionally leaves that mutation standing. Since duplicates/pools share this registry, a rejected discard can make later hImportSet calls stop auto-preparing or replay a discard even though the server never accepted the user's discard; defer the registry mutation until the command is actually accepted for sending or roll it back on pre-enqueue failures.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| try { | ||
| reply = await mainPromise; | ||
| } catch (err) { | ||
| if (command === HIMPORT_PREPARE && userPrepare !== undefined) { |
There was a problem hiding this comment.
Could we restore the previous registry entry when a replacement PREPARE fails, removing it only when no prior definition existed? Redis retains the existing fieldset after validation failure, so deleting it here breaks transparent replay for duplicates and reconnects.
| if (sessionVersion === undefined || sessionVersion < entry.version) { | ||
| prepared.set(name, entry.version); | ||
| prelude.push({ | ||
| args: ['HIMPORT', 'PREPARE', name, ...entry.fields], | ||
| // Injected-PREPARE failures are often transient (LOADING, failover) — roll back | ||
| // only the session claim, never the registration, or a valid fieldset would | ||
| // permanently lose lazy re-prepare. | ||
| rollback: () => { | ||
| if (prepared.get(name) === entry.version) prepared.delete(name); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Could we tie the SET's outcome to its injected PREPARE? If the session holds an older version and the injected PREPARE fails while the connection stays alive (e.g. ACL denies the PREPARE subcommand), the SET still runs against the stale field list and succeeds — values land under the wrong field names with no error. One option: keep the PREPARE promise and, after the SET resolves, reject (or re-verify the version) if the PREPARE had failed.
| const send = () => client.sendCommand(parser.redisArgs, commandOptions); | ||
| let mainPromise: Promise<unknown>; | ||
| if (effectiveAsap) { | ||
| // asap unshifts, so consecutive front-insertions reverse: enqueue the main command | ||
| // first, then the prelude back-to-front — the wire sees prelude order, then main. | ||
| mainPromise = send(); | ||
| for (let i = prelude.length - 1; i >= 0; i--) { | ||
| const injection = prelude[i]; | ||
| client.sendCommand(injection.args, injectOpts).catch(injection.rollback); | ||
| } | ||
| } else { | ||
| for (const injection of prelude) { | ||
| client.sendCommand(injection.args, injectOpts).catch(injection.rollback); | ||
| } | ||
| mainPromise = send(); |
There was a problem hiding this comment.
When this runs under an ASK redirect, the wire order becomes ASKING → injected PREPARE → SET, and the keyless PREPARE consumes the one-shot ASKING flag — the SET then gets MOVED again and loops until maxCommandRedirections. Would it make sense to re-send ASKING between the prelude and the main command when on an ASK chain (or prepare the target node eagerly in _handleAsk)?
…luster Address three registry-lifecycle bugs in the HIMPORT transparency layer that broke the duplicate/pool/cluster registration-sharing contract: - Rejected replacement PREPARE dropped a still-valid registration. Snapshot the prior field list and restore it on reject; a rejected fresh PREPARE still removes the name. - RedisCluster.duplicate() lost all registrations because RedisClusterSlots always allocated a fresh registry. Thread the parent registry through an internal himportRegistry option, mirroring the standalone duplicate guarantee. - A DISCARD/DISCARDALL rejected before reaching the wire (client closed/offline, aborted signal, full queue) removed the fieldset from the shared registry even though the server never saw it. Re-register on pre-enqueue failure; wire/server failures keep the existing replay-on-next-command semantics. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 492bbda127
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| mainPromise = send(); | ||
| for (let i = prelude.length - 1; i >= 0; i--) { | ||
| const injection = prelude[i]; | ||
| client.sendCommand(injection.args, injectOpts).catch(injection.rollback); |
There was a problem hiding this comment.
When an asap() HIMPORT command needs a lazy PREPARE/DISCARD and only one queue slot remains (for example commandsQueueMaxLength: 1 on a pooled/cluster connection that still holds an old or discarded fieldset), this enqueues the main HIMPORT SET first, then the required prelude is rejected as The queue is full and only its rollback runs. The already-queued SET can still execute against the stale server-side fieldset and return OK, writing values under the wrong/previous template instead of failing or preparing first; the capacity check needs to reserve/enqueue the prelude+main atomically or reject the whole operation.
Useful? React with 👍 / 👎.
| prelude.push({ | ||
| args: ['HIMPORT', 'PREPARE', name, ...entry.fields], |
There was a problem hiding this comment.
During an ASK redirection for cluster.hImportSet() on a destination connection that has not prepared the fieldset yet, _handleAsk queues ASKING and then calls this hook; the injected HIMPORT PREPARE becomes the first command after ASKING, so Redis consumes the one-time ASKING state before the redirected keyed HIMPORT SET. In slot-migration windows this can make the retry bounce with another redirection instead of executing on the importing node; the lazy PREPARE needs to run before ASKING or otherwise keep ASKING immediately in front of the SET.
Useful? React with 👍 / 👎.
| // One fieldset registry across master/replica node clients: fieldsets registered before | ||
| // a failover must be transparently re-preparable on the promoted master's connections. | ||
| // (Sentinel-monitor clients use #sentinelClientOptions and never run HIMPORT.) | ||
| this.#nodeClientOptions.himportRegistry = new FieldsetRegistry(); |
There was a problem hiding this comment.
A RedisSentinel.duplicate() created after sentinel.hImportPrepare(...) constructs a new internal sentinel and this constructor always installs a fresh FieldsetRegistry, while duplicate() only spreads the original options. The duplicate therefore loses the parent's registrations even though the new HIMPORT docs state that duplicate() shares them, so a direct dup.hImportSet(...) cannot lazy-prepare and fails with no such fieldset; thread the existing sentinel registry into duplicates as the standalone and cluster paths do.
Useful? React with 👍 / 👎.
…SK chains Two more HIMPORT transparency-layer correctness fixes from PR review: - Tie the SET reply to its lazily-injected PREPARE. If that PREPARE is rejected while the connection stays alive (e.g. ACL denies the PREPARE subcommand), the SET would otherwise run against the stale server-side field list and silently store values under the wrong field names. Hold the PREPARE promise and, once the SET resolves, surface the PREPARE error and drop the session claim instead of returning the misleading OK. - Re-issue ASKING on ASK-redirect chains. A keyless PREPARE/DISCARD pipelined ahead of the SET consumes the one-shot ASKING flag, so the SET is redirected again until maxCommandRedirections. The cluster ASK handler now marks the command options (askRedirect), and the hook re-issues ASKING as the last prelude entry so it lands immediately before the SET. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
Reviewed by Cursor Bugbot for commit 994ac8d. Configure here.
| prepared.syncedDiscardCount = Math.min(prepared.syncedDiscardCount, countAtInjection - 1); | ||
| }; | ||
| if (collapseToDiscardAll) { | ||
| prelude.push({ args: ['HIMPORT', 'DISCARDALL'], rollback: () => rollback(pending) }); |
There was a problem hiding this comment.
Reconcile collapse to DISCARDALL destroys untracked raw fieldsets
Low Severity
The collapseToDiscardAll optimization replaces individual HIMPORT DISCARD commands with a single HIMPORT DISCARDALL when all tracked entries are pending. However, DISCARDALL removes all server-side fieldsets on the session—including any set up via raw sendCommand—while the equivalent individual DISCARD commands would only remove the managed (tracked) names. The comment says "nothing this session holds is worth keeping," but fieldsets created outside the managed layer would be silently destroyed.
Reviewed by Cursor Bugbot for commit 994ac8d. Configure here.
| userDiscard = { restore, removed, countAfter: registry.discardCount }; | ||
| } | ||
| prepared.clear(); | ||
| } |
There was a problem hiding this comment.
DISCARDALL with empty registry loses prepared entries on failure
Low Severity
In the HIMPORT_DISCARDALL branch, prepared.clear() runs unconditionally, but userDiscard (which drives error-path rollback of prepared entries) is only set when registryReply > 0. When the registry is empty yet the session holds stale prepared entries awaiting reconciliation, a wire failure permanently loses those session claims with no rollback—the captured restore snapshot is never used.
Reviewed by Cursor Bugbot for commit 994ac8d. Configure here.
2 participants
This pull request adds first-class support for the Redis 8.10
HIMPORTcommand family (hinted hash templates):HIMPORT PREPARE,HIMPORT SET,HIMPORT DISCARD, andHIMPORT DISCARDALL, exposed ashImportPrepare/hImportSet/hImportDiscard/hImportDiscardAlland marked@experimental.Fieldsets are server-side session state scoped to a single physical connection, so a naive client binding breaks under pools, reconnects, and clusters. Per the HIMPORT client HLD, the client owns the fieldset lifecycle: a shared
FieldsetRegistryrecords registrations on the logical client (standalone, pool, cluster, sentinel), and a per-connection hook transparently pipelinesPREPAREin front ofSETon sessions that lack the fieldset or hold a stale field list, replays pending discards before dependent commands, and retries aSETonce onno such fieldsetfor registered fieldsets.DISCARD/DISCARDALLreplies are registry-based (registered-and-removed), giving deterministic semantics across topologies, and a rejected discard restores the session bookkeeping so the discard replays on that connection.On a cluster, every serving node client runs the same hook, so sessions prepare and reconcile lazily per node with no HIMPORT-specific routing code. The eager
all_shardsfan-out for the session commands (server command tips) and its response-policy handling arrive with the request/response policies work (#2996), which upgrades cluster support from lazy-per-node to fully tip-compliant.Behavior notes: raw
sendCommand(['HIMPORT', ...])bypasses the managed layer;duplicate()shares the parent's registrations whilecreatePool()does not; discarded fieldsets may linger on idle pooled connections (memory only, reconciled before their next HIMPORT command).🤖 Generated with Claude Code
Note
High Risk
Large new command path with optimistic registry/session bookkeeping, injected wire commands, and rollback/reconcile logic across standalone, pool, cluster, and sentinel—errors could cause wrong hash field mapping or inconsistent session state.
Overview
Adds experimental Redis 8.10 HIMPORT support (
hImportPrepare,hImportSet,hImportDiscard,hImportDiscardAll) so hinted hash templates work across reconnects, pools, cluster nodes, and sentinel failovers.A shared
FieldsetRegistryon the logical client records prepared field lists; each connection keepsPreparedFieldsetsfor what that socket’s session actually holds. Typed HIMPORT calls go through#executeHimport, which pipelines lazyPREPAREand reconcileDISCARD/DISCARDALLahead ofSET, returns registry-based discard replies, gatesSETon failed injectedPREPARE, retriesSETonce onno such fieldsetwhen still registered, and re-issuesASKINGon cluster ASK chains (askRedirect). Session state is cleared on socket errors,reset(), and socket swap;duplicate()shares the registry; pools/cluster/sentinel inject one registry per topology (pools always get a new registry).MULTI/pipeline rejectPREPARE/DISCARD/DISCARDALLclient-side; rawsendCommand(['HIMPORT', ...])skips the managed layer.Reviewed by Cursor Bugbot for commit 994ac8d. Bugbot is set up for automated code reviews on this repo. Configure here.