fix: prevent double-close inflating zstd pool release counters#330
Merged
EdSchouten merged 3 commits intobuildbarn:mainfrom Mar 12, 2026
Merged
fix: prevent double-close inflating zstd pool release counters#330EdSchouten merged 3 commits intobuildbarn:mainfrom
EdSchouten merged 3 commits intobuildbarn:mainfrom
Conversation
In writeZstd(), the zstdReader was closed twice: once by the buffer (which takes ownership via NewCASBufferFromReader) and again by defer zstdReader.Close(). Since metricsDecoder.Close() had no double-close guard — unlike the underlying pooledDecoder — each duplicate close incremented releases_total without a corresponding acquisition, causing releases to permanently exceed acquisitions. Remove the redundant defer and add double-close protection to both metricsEncoder and metricsDecoder as a safety net.
|
Contributor
Author
|
Visualizing the issue further: |
EdSchouten
reviewed
Mar 12, 2026
Per review feedback: set Encoder/Decoder to nil on close instead of using a separate bool field. This prevents calling any methods on the object after close and matches the pattern in pooledEncoder/pooledDecoder.
EdSchouten
approved these changes
Mar 12, 2026
EdSchouten
requested changes
Mar 12, 2026
Per review feedback: preserve the gRPC status code from Pool.NewDecoder() (e.g. codes.Canceled) instead of hardcoding codes.ResourceExhausted via status.Errorf.
EdSchouten
approved these changes
Mar 12, 2026
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
2 participants
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.
Summary
Hey @EdSchouten, firstly I want to say that overall in our testing, the changes from #327 have been working very well! A tremendous thank you for taking your time to work with me and refine this approach into a very nice technical solution 🥳
After integrating with the new metrics available, I've noticed that the observed releases did not match the observed acquisitions. This PR introduces a patch fix for this issue.
defer zstdReader.Close()inwriteZstd(), the buffer takes ownership of the reader viaNewCASBufferFromReaderand closes it, so the defer was a second closemetricsEncoder.Close()andmetricsDecoder.Close()as a safety net, matching the existing pattern inpooledEncoder/pooledDecoderThe
writeZstd()path double-closes the decoder: once when the buffer consumes and closes the reader, and again viadefer. SincemetricsDecoder.Close()lacks double-close protection (unlike the underlyingpooledDecoder), each compressed ByteStream write incrementsreleases_totalan extra time. Over time this causesreleases_totalto permanently exceedacquisitions_total(observed: 336,865 releases vs 333,001 acquisitions = 3,864 excess, corresponding to the number of compressed ByteStream writes).Test plan
bazel test //pkg/zstd/... //pkg/blobstore/grpcservers/...— all passbazel test //...— all 30 tests passreleases_totalno longer exceedsacquisitions_totalafter deployment