Skip to content

fix(client): require approximate modifier for XTRIM LIMIT - #3425

Merged
nkaradzhov merged 2 commits into
redis:masterfrom
abhijeet117:fix/xtrim-limit-requires-approximate
Aug 26, 2026
Merged

fix(client): require approximate modifier for XTRIM LIMIT#3425
nkaradzhov merged 2 commits into
redis:masterfrom
abhijeet117:fix/xtrim-limit-requires-approximate

Conversation

@abhijeet117

@abhijeet117 abhijeet117 commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

Summary

XTRIM accepted LIMIT without strategyModifier, but the server hard-rejects that combination ("syntax error, LIMIT cannot be used without the special ~ option", t_stream.c), so such calls always failed at runtime. The options are now a discriminated union: LIMIT lives on the approximate branch which requires strategyModifier ~, while exact trimming stays modifier-less or =. Runtime emission for valid inputs is unchanged; one spec case asserted an invalid = + LIMIT command and was corrected.

Testing

Compile-time regression test rejects {LIMIT} without a modifier (compiled on master, TS2578) and accepts it with ~. Parser assertions, type tests, build and lint run locally; Docker-backed integration tests run in CI.

Checklist

  • bug reproduced before fix
  • root cause identified
  • bug fixed
  • tests passed

Note

Low Risk
Client-only fix for invalid XTRIM argument combinations; callers who relied on LIMIT without ~ already failed at the server and will now fail at compile time instead.

Overview
XTRIM options are now a discriminated union: exact trimming (XTrimExactOptions) allows optional = and policy, while approximate trimming (XTrimApproximateOptions) requires strategyModifier: '~' and is the only branch where LIMIT is typed.

The command parser no longer emits LIMIT unless the approximate modifier is present, matching Redis’s rule that LIMIT without ~ is a syntax error. Spec expectations were updated accordingly (e.g. LIMIT alone no longer appears on the wire), and a compile-time types test asserts that { LIMIT: 10 } without ~ is rejected by TypeScript.

Reviewed by Cursor Bugbot for commit f41a4d8. Bugbot is set up for automated code reviews on this repo. Configure here.

@chatgpt-codex-connector chatgpt-codex-connector Bot 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fcb76a0a78

ℹ️ 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".

Comment on lines +55 to 56
if (options?.strategyModifier === '~' && options.LIMIT !== undefined) {
parser.push('LIMIT', options.LIMIT.toString());

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Reject LIMIT instead of silently dropping it

When an invalid options object reaches this runtime path—such as from JavaScript, any, or the structurally accepted typed case below—the new condition silently omits LIMIT rather than preserving the previous server error. Redis then executes a valid exact XTRIM, potentially deleting entries even though the caller requested bounded work; this also breaks the unchanged XTRIM.spec.ts “with LIMIT” parser assertion. Either emit LIMIT so Redis rejects it or throw locally instead of changing the command's meaning.

Useful? React with 👍 / 👎.

Comment on lines +11 to +14
export interface XTrimExactOptions {
strategyModifier?: '=';
/** added in 8.2 */
policy?: StreamDeletionPolicy;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Exclude LIMIT from the exact union branch

Because XTrimExactOptions does not declare LIMIT?: never and shares policy with the approximate branch, TypeScript accepts an invalid value such as { LIMIT: 10, policy: STREAM_DELETION_POLICY.KEEPREF } as XTrimOptions; excess-property checking sees LIMIT elsewhere in the union while structural assignability succeeds through the exact branch. Thus the advertised compile-time guarantee fails whenever policy is also supplied, and the parser then silently discards the accepted LIMIT.

Useful? React with 👍 / 👎.

@cursor cursor Bot 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.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

Reviewed by Cursor Bugbot for commit fcb76a0. Configure here.

Comment thread packages/client/lib/commands/XTRIM.spec.ts
@PavelPashov

Copy link
Copy Markdown
Contributor

@abhijeet117 Thanks for the PR. We’ll review it when we have capacity.

@nkaradzhov

Copy link
Copy Markdown
Collaborator

Thanks for the PR — the underlying point is correct: Redis rejects XTRIM ... LIMIT unless the approximate ~ modifier is present, so moving LIMIT onto an approximate-only branch is a reasonable, source-compatible refinement.

One change before we can merge: the existing 'with LIMIT' case in XTRIM.spec.ts (the one passing { LIMIT: 1 } with no strategyModifier) still asserts that LIMIT is emitted. With the new parser guard that call now drops LIMIT, so the test will fail — and since the specs run through tsx, the now-invalid options object won't be caught at compile time. Please update or remove that case (mirroring the ~ fix you already applied to the 'with strategyModifier, LIMIT' case) and confirm the XTRIM spec and npm run test:types -w @redis/client both pass. Optionally, XADD's inline TRIM.limit has the same latent gap if you'd like to fix it in a follow-up. Once the suite is green this should be ready for another look.

@nkaradzhov nkaradzhov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Requesting changes: the existing 'with LIMIT' case in XTRIM.spec.ts (no strategyModifier) will now fail, since the new parser guard drops LIMIT — please update/remove it and confirm the XTRIM spec + test:types pass. Details in my comment above.

@nkaradzhov

Copy link
Copy Markdown
Collaborator

Thanks for fixing up the spec — this looks good to me. LGTM.

@nkaradzhov
nkaradzhov merged commit 7809d28 into redis:master Aug 26, 2026
25 of 26 checks passed
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.

3 participants