Repository navigation
Close dump resources on every return path - #5804
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The tar writer is closed twice on the successful path, adding an unnecessary archive trailer.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR improves dump resource cleanup and adds gohawk validation to CI.
Changes:
- Scopes metrics cleanup per service.
- Adds archive writer cleanup for early returns.
- Adds
gohawkto CI with the required gzip-reader suppression.
File summaries
| File | Summary |
|---|---|
internal/content/helpers.go |
Documents the intentional gzip-reader lifetime exception. |
internal/cmd/cli/dump/dump.go |
Refactors metrics collection and archive writer cleanup; the tar writer is still closed twice on successful dumps. |
.github/workflows/golangci-lint.yml |
Adds gohawk validation to CI. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
|
I added a commit to address the copilot's feedback and I removed the commit which adds gohawk in CI. I took a quick look in your repo and I think it is better to leave it for later. But thanks a lot for yr PR and fixing that particular issue. appreciated |
d3flex
approved these changes
Oct 14, 2026
w is still explicitly closed at lines 1004-1006, so this defer runs a second time on every successful dump. Unlike gzip.Writer.Close, tar.Writer.Close is not idempotent: each call writes another pair of end-of-archive blocks, adding an unnecessary extra trailer to every archive. Make the deferred cleanup explicitely so the tar writer is closed exactly once and add an explicit check after gz.Close() for tgz.Close so an error can be caught similar with gz and w. Signed-off-by: Ioannis Bonatakis <ybonatakis@suse.com>
d3flex
enabled auto-merge (rebase)
October 14, 2026 11:33
d3flex
changed the title
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.
Changes:
Testing:
go test ./internal/cmd/cli/dump ./internal/contentgo run github.com/kojah/gohawk@v0.3.0 ./...Additional Information
I've added gohawk to the lint workflow alongside golangci-lint, reusing the existing checkout and Go setup. It's what found the issues in this PR, so feel free to keep it around if you find it useful :) if not, you're welcome to remove it from this PR.
Checklist