Skip to content

test: cover cached EVALSHA_RO after EVAL_RO ACL revocation #3579

Closed as not planned

Description

@aetherxeg-source

Following #3563 and the merged fix in #3567, I'd like to add one focused regression test for the direct, cached read-only script path.

The existing integration tests exercise a missing script falling back to EVAL_RO and the read-only MULTI path. I couldn't find a test for the following sequence:

  1. With a Redis ACL user allowed EVAL_RO and EVALSHA_RO but denied EVAL and EVALSHA, flush the script cache.
  2. Call a read-only defined script once. The initial NOSCRIPT should fall back to EVAL_RO and populate the cache.
  3. Revoke only EVAL_RO from that user; verify a direct EVAL_RO call now fails with NOPERM.
  4. Call the defined script again; it should still succeed through cached EVALSHA_RO.

This would catch an unintended fallback or command-selection regression that the initial-load and transaction cases don't directly exercise. The proposed change is test-only, in packages/client/lib/client/index.spec.ts, without changing command behavior or public APIs.

I've prepared a small candidate diff against master at 4ce935514016e9f40561740343d5b9d831044631. The test has not yet been run against a native Redis instance or the project's full suite, so I'm not reporting a runtime pass or an existing defect.

Would this regression fit the project's test coverage? If so, I'll run it in the existing harness and submit a focused PR for review.

Activity

  1. nkaradzhov commented on Nov 4, 2026

    @nkaradzhov
    Collaborator

    Thanks, but we'll pass on this one.

    The existing loads a missing script with EVAL_RO test already covers both halves. Its user is denied EVALSHA, so the test only passes if the client tries EVALSHA_RO first and then falls back to EVAL_RO. The client keeps no "loaded" state and falls back only on NOSCRIPT. A second call with the script already cached therefore takes the same EVALSHA_RO path. An extra integration test for that case would add ACL setup to the suite without catching anything new.

    Closing as not planned. If you find a case where a read-only script uses the wrong command, please open an issue with a reproduction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions