Skip to content

packer: don't panic on Put after Wait#414

Merged
mathieu-plak merged 1 commit into
mainfrom
fix/2106-packer-race
May 29, 2026
Merged

packer: don't panic on Put after Wait#414
mathieu-plak merged 1 commit into
mainfrom
fix/2106-packer-race

Conversation

@poolpOrg

Copy link
Copy Markdown
Contributor

Summary

  • Fix PlakarKorp/plakar#2106: backup interrupted with Ctrl+C (or any error path that calls `PackerManager.Wait` while workers are still calling Put) can panic with `send on closed channel` in `seqPackerManager.Put`.
  • Root cause: Wait closed every `packerChan[i]`; Put sent to them unconditionally. The two race.
  • Fix: replace the close-on-Wait with a dedicated `closing chan struct{}`. Put `select`s between sending and observing closing, returning `ErrShutdown` on shutdown. Run workers also observe closing and flush their tail packfile before returning. Wait is idempotent via `sync.Once`. `packerChan` itself is never closed, so no possible "send on closed channel".
  • Same shape of fix applied to `platarPackerManager` (.ptar) for consistency.

What about the lifecycle bug in snapshot.Backup?

There's an upstream issue too: `importSource` returns early on `importerErr` without waiting for `<-errch`, so workers can still be calling `Put` when `Backup` calls `PackerManager.Wait`. That's a separate fix worth doing on the plakar / snapshot side, but the packer should also not panic regardless of who calls Put with what timing — which is what this PR addresses.

Test plan

  • New `TestSeqPackerPutDuringWait` races 32 concurrent producers (200 puts each) against a Wait. Hits "send on closed channel" reliably on `main`; passes on this branch under `-race -count=50`.
  • New `TestSeqPackerPutAfterWait` pins the post-shutdown contract: Put returns an error rather than panicking.
  • Full `go test ./...` clean on kloset.
  • Full `go test ./...` clean on plakar (built against this branch via a local replace).
  • End-to-end: 5 backup runs of ~100MB each, all SIGINT'd mid-flight. 0 panics. (On `main` this is the reproducer pattern from the bug report.)

(There are two pre-existing `-race` failures in the packer test suite — `TestPackWriter_ConcurrentInternalAccess` and `TestPlatarPackerManager_RunAndWait` — both reproducible on `main` without this change. Out of scope here.)

🤖 Generated with Claude Code

When Wait closed packerChan, any in-flight Put would panic with
"send on closed channel". Snapshot.Backup has error paths that call
PackerManager.Wait while workers are still calling Put (e.g.
recordError on a Ctrl+C-interrupted backup), so the panic surfaced
in real use.

Switch from closing packerChan to a separate closing channel:
Put selects on either the send or closing and returns ErrShutdown
on shutdown. Run workers also observe closing and flush their
in-flight packfile before returning. Wait is now idempotent.

Same fix applied to the platar packer for consistency.

Adds two regression tests:
- TestSeqPackerPutDuringWait races many concurrent Put callers
  against Wait. Hits "send on closed channel" reliably without
  the fix.
- TestSeqPackerPutAfterWait pins the post-shutdown contract:
  Put returns an error instead of panicking.

Fixes PlakarKorp/plakar#2106

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

@poolpOrg

Copy link
Copy Markdown
Contributor Author

Fixes an actual bug, analysis is sound, PR seems ok after an initial review, but I need to discuss this again with @mathieu-plak as I'm not fully convinced about the draining bits.

@mathieu-plak

Copy link
Copy Markdown
Contributor

#376 big conflict energy :).

@mathieu-plak
mathieu-plak merged commit ede9e67 into main May 29, 2026
6 checks passed
@mathieu-plak
mathieu-plak deleted the fix/2106-packer-race branch May 29, 2026 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Panic on Ctrl+C

3 participants