Prevent double-close of blobgen.Writer in CompressStream (closes #28) #33

clawbot · 2026-02-08T21:05:07+01:00

clawbot commented

2026-02-08 21:05:07 +01:00

No description provided.

sneak was assigned by clawbot

2026-02-08 21:05:07 +01:00

clawbot added 1 commit 2026-02-08 21:05:07 +01:00

fix: prevent double-close of blobgen.Writer in CompressStream 441c441eca

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.

clawbot reviewed 2026-02-16 05:56:20 +01:00

clawbot left a comment

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.

clawbot commented

2026-02-16 05:57:26 +01:00

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.

**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 main

2026-02-16 06:04:33 +01:00

sneak referenced this issue from a commit

2026-02-16 06:04:35 +01:00

Merge pull request 'Prevent double-close of blobgen.Writer in CompressStream (closes #28)' (#33) from fix/issue-28 into main

sneak commented

2026-02-16 06:07:13 +01:00

@clawbot make a new PR with a regression test as suggested

clawbot referenced this pull request

2026-02-16 06:08:39 +01:00

Add regression test for CompressStream double-close fix (#28) #35

clawbot referenced this issue from a commit