CompressStream had both a defer w.Close() and an explicit w.Close() call,
causing the compressor and encryptor to be closed twice. The second close
on the zstd encoder returns an error, and the age encryptor may write
duplicate finalization bytes, potentially corrupting the output stream.
Use a closed flag to prevent the deferred close from running after the
explicit close succeeds.
Review: Prevent double-close of blobgen.Writer in CompressStream
Positives:
Correctly identifies the double-close bug: explicit w.Close() on the happy path + defer w.Close() = double close.
The closed bool flag pattern is idiomatic Go for this situation.
Minimal, surgical fix.
Concerns:
Error path Close still fires — On the io.Copy error path, the defer will call Close(). If Close() itself returns an error in that scenario, it's silently discarded (_ = w.Close()). That's probably fine for cleanup, but worth noting.
Alternative: remove the defer entirely — Since the only early return after writer creation is the io.Copy error, you could restructure to call Close() explicitly in both paths and drop the defer. That said, the current approach is safer against future code additions between the writer creation and the explicit close.
Is w.Close() idempotent? — If the underlying blobgen.Writer.Close() is safe to call twice (no-op on second call), this fix is defensive but the bug was benign. If it's NOT idempotent (e.g. double-flushes, panics, corrupts state), this fix is critical. Either way, correct to fix.
LGTM — clean fix for a real correctness issue.
## Review: Prevent double-close of blobgen.Writer in CompressStream
**Positives:**
- Correctly identifies the double-close bug: explicit `w.Close()` on the happy path + `defer w.Close()` = double close.
- The `closed` bool flag pattern is idiomatic Go for this situation.
- Minimal, surgical fix.
**Concerns:**
1. **Error path Close still fires** — On the `io.Copy` error path, the defer will call `Close()`. If `Close()` itself returns an error in that scenario, it's silently discarded (`_ = w.Close()`). That's probably fine for cleanup, but worth noting.
2. **Alternative: remove the defer entirely** — Since the only early return after writer creation is the `io.Copy` error, you could restructure to call `Close()` explicitly in both paths and drop the defer. That said, the current approach is safer against future code additions between the writer creation and the explicit close.
3. **Is `w.Close()` idempotent?** — If the underlying `blobgen.Writer.Close()` is safe to call twice (no-op on second call), this fix is defensive but the bug was benign. If it's NOT idempotent (e.g. double-flushes, panics, corrupts state), this fix is critical. Either way, correct to fix.
LGTM — clean fix for a real correctness issue.
Is there a test that exercises CompressStream to verify no double-close panic/error occurs? The fix is correct but a regression test would prevent this from being reintroduced.
**Blocker: No evidence of passing tests or linting.**
Please confirm tests pass. Additionally:
- Is there a test that exercises `CompressStream` to verify no double-close panic/error occurs? The fix is correct but a regression test would prevent this from being reintroduced.
sneak
merged commit 8adc668fa6 into main2026-02-16 06:04:33 +01:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
clawbot
Review: Prevent double-close of blobgen.Writer in CompressStream
Positives:
w.Close()on the happy path +defer w.Close()= double close.closedbool flag pattern is idiomatic Go for this situation.Concerns:
Error path Close still fires — On the
io.Copyerror path, the defer will callClose(). IfClose()itself returns an error in that scenario, it's silently discarded (_ = w.Close()). That's probably fine for cleanup, but worth noting.Alternative: remove the defer entirely — Since the only early return after writer creation is the
io.Copyerror, you could restructure to callClose()explicitly in both paths and drop the defer. That said, the current approach is safer against future code additions between the writer creation and the explicit close.Is
w.Close()idempotent? — If the underlyingblobgen.Writer.Close()is safe to call twice (no-op on second call), this fix is defensive but the bug was benign. If it's NOT idempotent (e.g. double-flushes, panics, corrupts state), this fix is critical. Either way, correct to fix.LGTM — clean fix for a real correctness issue.
Blocker: No evidence of passing tests or linting.
Please confirm tests pass. Additionally:
CompressStreamto verify no double-close panic/error occurs? The fix is correct but a regression test would prevent this from being reintroduced.@clawbot make a new PR with a regression test as suggested
Regression test PR created: #36