[pellared]

ResetWithOptions updates decoder options, while reusable frame decoders retain values copied when created, causing inconsistent behavior after reset.

Publish immutable option snapshots and capture one per operation so in-flight work keeps its existing settings while subsequent DecodeAll calls and streams use the updated options.

Summary by CodeRabbit

• Bug Fixes

  • Improved how decoder settings are applied when resetting or reusing a decoder, including during concurrent decoding and full-buffer operations.
  • Preserved configured memory, output-size, window-size, and checksum behavior when settings change.
  • Improved dictionary handling and made failed configuration changes less likely to affect existing decoder settings.

• Tests

  • Added coverage for decoder resets, dictionaries, resource limits, checksums, concurrency, and asynchronous decoding.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: dc161311-c01c-47c8-a424-0c61d58e7749

📥 Commits

Reviewing files that changed from the base of the PR and between eeab392 and ccb0078.

📒 Files selected for processing (2)

• zstd/decoder.go
• zstd/decoder_test.go

Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.

---

📝 Walkthrough

Walkthrough

The decoder stores options in an atomic snapshot. Reset operations publish updated snapshots, and decoding paths apply options to active frame decoders. Tests cover reset behavior, concurrent decoding, limits, checksums, and dictionaries.

Changes

Decoder option snapshots

Layer / File(s)	Summary
Publish and synchronize decoder options zstd/decoder.go, zstd/framedec.go	Options are built locally, published atomically, copied during ResetWithOptions, and applied through frameDec.setOptions.
Apply snapshots to decoding paths zstd/decoder.go	Stream and stateless decoding use frame options for limits, allocation, concurrency, checksums, and dictionary lookup.
Validate reset and concurrent decoding zstd/decoder_test.go	Tests cover updated limits, checksums, dictionaries, reused asynchronous streams, rejected option changes, and concurrent DecodeAll calls.

Priority: ⚪ Not assessed

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Suggested reviewers: klauspost

Merge Risk: ⚪ Minimal · up to ccb00

No concrete decoder regression remains identified; the change is mergeable after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 4 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: applying reset options to reused frame decoders.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[klauspost]

Great spot! Looks great! I'll give it a look through.

[klauspost]

Overall looks clean, but looked for some simplifications.

[klauspost]

Nice! I will look through it a final time tomorrow and merge.

[lizthegrey]

using atomics is typically a smell that you're doing something else hinky with concurrency. is there an approach that avoids atomics?

[pellared]

using atomics is typically a smell that you're doing something else hinky with concurrency. is there an approach that avoids atomics?

Thanks, @lizthegrey. There is a non-atomic alternative: keep ordinary option state and require ResetWithOptions not to overlap any DecodeAll call.

I chose atomic.Pointer to support a narrower model where one ResetWithOptions call may overlap concurrent DecodeAll calls. The updater constructs and publishes a complete immutable options snapshot, including a cloned dictionary map. Each DecodeAll loads one snapshot, so in-flight operations retain their existing settings while subsequent operations observe the update, without adding a mutex to every decode.

The atomic is not intended to make concurrent resetters or other lifecycle operations safe. In the earlier review, Klaus explicitly requested removing the reset mutex while saying that the atomic pointer was fine to keep. I implemented that distinction and changed the regression test to use one updater.

@klauspost, does this new concern change your earlier preference? If not, I will keep the immutable snapshot approach.

[klauspost]

Yeah, the atomic pointer is a bit of a smell.

But it does seem like it is fairly cleanly separated and AFAICT it would be practically fine to do a ResetWithOptions while DecodeAll decoders are running. Of course you have no guarantee if old or new settings will be used, but it would allow stuff like adding a dict before a specific decode.

While it would seem reasonable, I am a bit hesitant to commit to it in documentation right away. But having it seems fine.

[klauspost]

Rest LGTM.