Cleanup VectorSet TODOs#1810
Merged
Merged
Conversation
Signed-off-by: Tiago Napoli <tiagonapoli@microsoft.com>
b3d08bd to
9a79f68
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens validation and error-handling around RESP parsing and VectorSet commands, and adds regression tests to ensure oversized inputs are rejected safely (without overflow or excessive allocations).
Changes:
- Add a maximum RESP array argument-count guard and a dedicated parsing exception for excessive argument counts.
- Harden vector command parsing/validation (dimension limits, EF/COUNT/FILTER-EF bounds) and refactor VSIM output buffer sizing helpers.
- Add/adjust tests for the new guards and validation behavior (RESP parsing, RespReadUtils serialized span parsing, VectorSet error cases).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/standalone/Garnet.test/RespTests.cs | Adds an end-to-end socket test to ensure excessive RESP array argument counts are rejected and the session is closed. |
| test/standalone/Garnet.test/Resp/RespReadUtilsTests.cs | Adds unit tests covering valid/invalid length-prefixed record parsing for GetSerializedRecordSpan. |
| test/standalone/Garnet.test.vectorset/RespVectorSetTests.cs | Updates VectorSet error-message expectations and adds new validation/error-path tests for VADD/VSIM. |
| libs/server/Resp/Vector/VectorManager.cs | Introduces shared limits (dimensions/EF/retrieve count) and refactors VSIM buffer sizing into helpers. |
| libs/server/Resp/Vector/RespServerSessionVectors.cs | Enforces new VectorSet parameter bounds (dims/EF/COUNT/FILTER-EF) and improves related error messages. |
| libs/server/Resp/Parser/RespCommand.cs | Adds MaxRespArrayLength and throws a parsing exception when the argument count exceeds the limit. |
| libs/common/RespReadUtils.cs | Adds bounds validation for serialized-record spans based on the length prefix. |
| libs/common/Parsing/RespParsingException.cs | Adds ThrowExcessiveArgumentCount to standardize the new protocol error. |
added 2 commits
May 20, 2026 00:28
Signed-off-by: Tiago Napoli <tiagonapoli@microsoft.com>
kevin-montrose
approved these changes
May 20, 2026
badrishc
approved these changes
May 20, 2026
This was referenced May 20, 2026
kevin-montrose
pushed a commit
that referenced
this pull request
May 20, 2026
* Cleanup VectorSet TODOs (#1810) * VectorSet todos Signed-off-by: Tiago Napoli <tiagonapoli@microsoft.com> * upd Signed-off-by: Tiago Napoli <tiagonapoli@microsoft.com> * address comment Signed-off-by: Tiago Napoli <tiagonapoli@microsoft.com> --------- Signed-off-by: Tiago Napoli <tiagonapoli@microsoft.com> Co-authored-by: Tiago Napoli <tiagonapoli@microsoft.com> * fix merge Signed-off-by: Tiago Napoli <tiagonapoli@microsoft.com> * remove newline Signed-off-by: Tiago Napoli <tiagonapoli@microsoft.com> * Add missing bounds check Signed-off-by: Tiago Napoli <tiagonapoli@microsoft.com> * nit Signed-off-by: Tiago Napoli <tiagonapoli@microsoft.com> * nit Signed-off-by: Tiago Napoli <tiagonapoli@microsoft.com> * ci: add release/v1 branch to GitHub Actions triggers Add release/v1 to push/pull_request/workflow_run triggers for: - ci.yml (main CI tests) - codeql.yml (security scanning) - ci-bdnbenchmark.yml (benchmark CI) - docker-linux.yml (Docker image builds) - docker-windows.yml (Docker image builds) ADO compliance pipelines already use exclude-only patterns and trigger on all branches, so no changes needed there. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci: remove release/v1 from BDN benchmark triggers Benchmarks should only run on main pushes, not release branches. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * style: fix formatting in cluster migrate and test files Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: add AllureNUnit attribute and AllureTestBase to ClusterTryAdvanceSpanByteTests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Signed-off-by: Tiago Napoli <tiagonapoli@microsoft.com> Co-authored-by: Tiago Napoli <tiagonapoli@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RESP parsing
VectorSet commands