Skip to content

zlib: fix zstd reset - #65867

Closed
jasnell wants to merge 3 commits into
nodejs:mainfrom
jasnell:jasnell/zstd-reset
Closed

zlib: fix zstd reset#65867
jasnell wants to merge 3 commits into
nodejs:mainfrom
jasnell:jasnell/zstd-reset

Conversation

@jasnell

@jasnell jasnell commented Sep 6, 2026

Copy link
Copy Markdown
Member

Handful of additional bits I noticed while poking around zstd..

  • Preserve dictionaries and params when zstd is reset.
  • Update missing documentation.
  • Reject invalid dictionaries (they would previously no-op)

The rejection is technically a behavior change but zstd is still experimental.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. zlib Issues and PRs related to the zlib module and its compression dependencies. labels Sep 6, 2026
@nodejs-github-bot

This comment was marked as outdated.

@codecov

codecov Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 59.45946% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.16%. Comparing base (0de4fcc) to head (aa7a2fe).
⚠️ Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
src/node_zlib.cc 53.12% 11 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #65867      +/-   ##
==========================================
- Coverage   90.17%   90.16%   -0.01%     
==========================================
  Files         771      771              
  Lines      265470   265506      +36     
  Branches    50463    50455       -8     
==========================================
+ Hits       239383   239404      +21     
+ Misses      17055    17048       -7     
- Partials     9032     9054      +22     
Files with missing lines Coverage Δ
lib/zlib.js 98.42% <100.00%> (+0.26%) ⬆️
src/node_zlib.cc 80.11% <53.12%> (+0.19%) ⬆️

... and 29 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nodejs-github-bot

This comment was marked as outdated.

@jasnell
jasnell requested review from panva and trivikr September 8, 2026 00:23
@trivikr trivikr added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Sep 8, 2026
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Preserve dictionaries and params when zstd is reset.

Update missing documentation.

Signed-off-by: James M Snell <jasnell@gmail.com>
Assisted-by: Opencode
Signed-off-by: James M Snell <jasnell@gmail.com>
Signed-off-by: James M Snell <jsnell@cloudflare.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@panva panva added the commit-queue PRs queued for automated landing through the Commit Queue. label Sep 11, 2026
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Sep 11, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Commit Queue failed

This pull request has multiple commits, but no landing policy was selected.

Add commit-queue-squash PRs the Commit Queue should land as one squashed commit. to land it as one commit, or commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. to land the commits separately.

The pull request was removed from the Commit Queue and labeled commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. . After resolving the failure, remove that label and add commit-queue PRs queued for automated landing through the Commit Queue. to retry.

Full Commit Queue output
- Loading data for nodejs/node/pull/65867
✔  Done loading data for nodejs/node/pull/65867
----------------------------------- PR info ------------------------------------
Title      zlib: fix zstd reset (#65867)
Author     James M Snell <jasnell@gmail.com> (@jasnell)
Branch     jasnell:jasnell/zstd-reset -> nodejs:main
Labels     c++, zlib, author ready, needs-ci, commit-queue
Commits    3
 - zlib: fix zstd reset
 - doc: fill in missing zstd docs
 - zlib: reject invalid zstd dictionaries
Committers 1
 - James M Snell <jasnell@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/65867
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/65867
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 06 Sep 2026 22:38:20 GMT
   ✔  Approvals: 3
   ✔  - Trivikram Kamat (@trivikr): https://github.com/nodejs/node/pull/65867#pullrequestreview-5175385386
   ✔  - Filip Skokan (@panva) (TSC): https://github.com/nodejs/node/pull/65867#pullrequestreview-5142709758
   ✔  - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/65867#pullrequestreview-5175283787
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2026-09-11T05:48:47Z: https://ci.nodejs.org/job/node-test-pull-request/77338/
- Querying data for job/node-test-pull-request/77338/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 65867
From https://github.com/nodejs/node
 * branch                  refs/pull/65867/merge -> FETCH_HEAD
✔  Fetched commits as 0de4fcceb937..aa7a2fea5e5d
--------------------------------------------------------------------------------
[main 5fca60480f] zlib: fix zstd reset
 Author: James M Snell <jsnell@cloudflare.com>
 Date: Sun Sep 6 15:27:21 2026 -0700
 3 files changed, 116 insertions(+), 6 deletions(-)
 create mode 100644 test/parallel/test-zlib-zstd-reset.js
[main a1489f96f2] doc: fill in missing zstd docs
 Author: James M Snell <jsnell@cloudflare.com>
 Date: Sun Sep 6 15:34:06 2026 -0700
 1 file changed, 7 insertions(+), 1 deletion(-)
[main 0102bb6978] zlib: reject invalid zstd dictionaries
 Author: James M Snell <jsnell@cloudflare.com>
 Date: Sun Sep 6 15:35:23 2026 -0700
 3 files changed, 27 insertions(+), 3 deletions(-)
   ✔  Patches applied
There are 3 commits in the PR. Attempting autorebase.
(node:435) [DEP0190] DeprecationWarning: Passing args to a child process with shell option true can lead to security vulnerabilities, as the arguments are not escaped, only concatenated.
(Use `node --trace-deprecation ...` to show where the warning was created)
Rebasing (2/6)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
zlib: fix zstd reset

Preserve dictionaries and params when zstd is reset.

Update missing documentation.

Signed-off-by: James M Snell <jasnell@gmail.com>
Assisted-by: Opencode
PR-URL: https://github.com/nodejs/node/pull/65867
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
--------------------------------------------------------------------------------
[detached HEAD 1ac6b06900] zlib: fix zstd reset
 Author: James M Snell <jsnell@cloudflare.com>
 Date: Sun Sep 6 15:27:21 2026 -0700
 3 files changed, 116 insertions(+), 6 deletions(-)
 create mode 100644 test/parallel/test-zlib-zstd-reset.js
Rebasing (3/6)
Rebasing (4/6)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
doc: fill in missing zstd docs

Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: https://github.com/nodejs/node/pull/65867
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
--------------------------------------------------------------------------------
[detached HEAD cbfcbfd8f8] doc: fill in missing zstd docs
 Author: James M Snell <jsnell@cloudflare.com>
 Date: Sun Sep 6 15:34:06 2026 -0700
 1 file changed, 7 insertions(+), 1 deletion(-)
Rebasing (5/6)
Rebasing (6/6)
Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
zlib: reject invalid zstd dictionaries

Signed-off-by: James M Snell <jsnell@cloudflare.com>
PR-URL: https://github.com/nodejs/node/pull/65867
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
--------------------------------------------------------------------------------
[detached HEAD c12bd90915] zlib: reject invalid zstd dictionaries
 Author: James M Snell <jsnell@cloudflare.com>
 Date: Sun Sep 6 15:35:23 2026 -0700
 3 files changed, 27 insertions(+), 3 deletions(-)
Successfully rebased and updated refs/heads/main.
--------------------------------------------------------------------------------
   ℹ  Add `commit-queue-squash` label to land the PR as one commit, or `commit-queue-rebase` to land as separate commits.

View workflow run

jasnell added a commit that referenced this pull request Sep 11, 2026
Preserve dictionaries and params when zstd is reset.

Update missing documentation.

Signed-off-by: James M Snell <jasnell@gmail.com>
Assisted-by: Opencode
PR-URL: #65867
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
jasnell added a commit that referenced this pull request Sep 11, 2026
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #65867
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
jasnell added a commit that referenced this pull request Sep 11, 2026
Signed-off-by: James M Snell <jsnell@cloudflare.com>
PR-URL: #65867
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
@jasnell

jasnell commented Sep 11, 2026

Copy link
Copy Markdown
Member Author

Landed in 9e7f67f...c143041

@jasnell jasnell closed this Sep 11, 2026
@jasnell jasnell removed the commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. label Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. zlib Issues and PRs related to the zlib module and its compression dependencies.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants