[ffumero2003]

Problem

CI on #205 failed once with a panic in production code, on one of six test jobs (Go 1.27.3, windows-latest, -race -shuffle=on):

panic: runtime error: slice bounds out of range [:-10]
github.com/Backblaze/blazer/b2.(*Writer).Write  b2/writer.go:307
github.com/Backblaze/blazer/b2.onlyWriter.Write b2/reader.go:376
io.Copy
github.com/Backblaze/blazer/b2.copyContextWithCloser.func1  b2/reader.go:392

It is timing-dependent, so any PR's CI can hit it.

Root cause

Writer.ReadFrom with a non-seekable source calls copyContext(w.ctx, w, r). copyContextWithCloser runs io.Copy in a goroutine, and on cancellation without a closer (the Writer path) it returned ctx.Err() without waiting for that goroutine. The goroutine could still call Writer.Write after ReadFrom returned and the caller closed the Writer; by then the buffer was released and p[:left] panicked. Waiting for the goroutine is not an option, because it can be blocked in r.Read indefinitely.

Changes

b2/reader.go: a write guard.

• The copy goroutine writes through guardedCopyWriter. It takes a mutex, returns errCopyAbandoned once the copy has stopped, and otherwise forwards to the real writer.
• On cancellation without a closer, copyContextWithCloser sets stopped under that lock before returning. After it returns, nothing more from r reaches the destination. It never waits for a blocked Read.
• What cancellation can wait for: at most one Write already in progress. That Write can include network calls (sendChunk → getLargeFile → b2_start_large_file, and Resume listings), but they run under w.ctx, the same context being cancelled, so they return once it ends. Only a RoundTripper that ignores the request context could hold the guard longer, which would affect every API call. This is stated in a comment at guardedCopyWriter.Write and tested.

b2/writer.go: no deadlock between cancellation and error cleanup. Copilot found that holding the guard across Writer.Write made cancellation wait for setErr's cleanup (b2_cancel_large_file and the WithCancelOnError callback), which could deadlock:

• Writer.Write is split into write(p, setErr), so the guard is released on terminal write errors, before cleanup runs.
• setErr releases emux before cleanup, so a producer in getErr doesn't wait behind a worker's cleanup.
• Cleanup can still run while Writer.write holds closeWrite.RLock. That is intended: Close takes closeWrite.Lock and waits for that cleanup anyway (below).

Writer.Close now waits for worker cleanup: a breaking change. Once setErr no longer holds emux through cleanup, Close could return before a worker's b2_cancel_large_file and errf finished, so callers would stop receiving the cleanup result before Close returned. Close now waits for the workers (defer w.wg.Wait()) on every exit path.

• Bound: the cancel RPC runs under the context returned by ctxf, so a deadline there bounds the wait (tested). errf is caller code and must return promptly.
• Deadlock rule: an errf that calls Close, or waits on the goroutine calling Close, deadlocks.
• This is documented on Writer.Close and WithCancelOnError, and in a Breaking changelog entry.

Changelog:

• Fixed: the ReadFrom contract for non-seekable readers;
• Changed (Breaking): Close waits for the cancel-on-error callback, with the deadlock rule and how to bound the wait.

Tests: b2/copy_context_test.go (#229) and b2/copy_context_cleanup_test.go.

Verification

Each break was applied alone; the named test failed, then passed after revert:

Break	Failing output
Stopped check flipped, or deleted	destination Write calls after copyContext returned = 1, want 0
Early guard release before cleanup deleted	ReadFrom waited for blocked cancel_large_file after cancellation
Cleanup back under emux	ReadFrom waited for blocked cancel_large_file after cancellation (worker failure while producer is in getErr)
Close's worker wait removed	Close returned while errf was blocked (50 of 50 runs)
Cancel RPC ignores its ctxf context	Close waited past the ctxf deadline for cancel_large_file
b2_start_large_file ignores its context (memory and file buffers)	ReadFrom waited for start_large_file after cancellation

With no B2 credentials set: go test -race -count=30 -run 'Cancellation|DelayedWrite|WriteAfterClose|CloseWaits|ReadFrom' ./b2, go test -race -count=5 -shuffle=on ./..., go vet ./... and make ci pass. The SDK panel reviewed the fix; its findings (the undocumented Close contract, an unreliable callback test) are addressed.

#252 (#245) is stacked on this branch and is retargeted to main after this merges.

Closes #229

[Copilot]

🟡 Changes recommended

Holding the mutex across Writer.Write can deadlock cancellation and ReadFrom when a synchronous error callback waits for ReadFrom to return.

1 open issue

• Avoid holding the mutex during cancellation callbacks · New

What changed in this PR

Addresses #229 by preventing a cancelled copy from writing to the B2 Writer after ReadFrom returns, without waiting for a blocked source read.

Changes:

• Guards destination writes and stops forwarding them on cancellation.
• Adds channel-controlled regression tests for delayed writes, including after Writer.Close.
• Documents pending-read behavior and adds a Fixed changelog entry.

Review: 1 Important, 1 Nit, 0 Pre-existing. Checked the supplied changes against #229 and inspected Writer error-handling paths. Tests were not run during this review.

File	Description
b2/​writer.go	Documents cancellation behavior; no executable code changes.
b2/​reader.go	Adds synchronized protection against writes after cancellation returns.
b2/​copy_context_test.go	Tests delayed-write rejection after cancellation and close.
.changes/​unreleased/​Fixed-20261029-145247.yaml	Records the cancellation fix for users.

_{Review effort: Balanced}

[david-santacruz-backblaze]

Let's give a review for the copilot finding

[ffumero2003]

@david-santacruz-backblaze The Copilot finding is fixed and the branch is synced with main. Summary:

• The deadlock: cancellation no longer waits on cancel_large_file or the WithCancelOnError callback. The copy guard and emux are both released before error cleanup runs. The fix(b2): Writer.Write panics when copyContext keeps writing after the call returns (flaky CI) #229 guarantee (no write after copyContext returns) is unchanged.
• Tests: every production change has a test that fails when it is undone (stopped check flipped and deleted, early unlock removed, cleanup put back under emux, Close's worker wait removed). The new tests use gates and channels; TestWriterCloseWaitsForErrorCallback fails 50 of 50 runs with the wait removed and passes 50 of 50 under -race with it.
• Contract change, now documented: Writer.Close waits for in-flight parts and, with WithCancelOnError, for b2_cancel_large_file and errf. The godoc on Close and WithCancelOnError says so (errf must not call or wait on Close), and there is a Changed changelog entry.

go test -race -count=5 -shuffle=on ./b2/..., go vet ./... and make ci are clean. Ready for another look.

[david-santacruz-backblaze]

Please mate, give a check of these issues:

Definitely review (verify each, then fix it or reply with a reason)

1. Changelog Changed-20261030-113335.yaml. Writer.Close now waits for the cancel-on-error callback. An errf that calls or waits on Close deadlocks. Mark the entry **Breaking:** and tell callers not to call or wait on Close from errf (AGENTS.md rule 13).
2. Close hang risk. defer w.wg.Wait() runs on every exit path. A worker stuck in errf or b2_cancel_large_file now blocks Close indefinitely. Show that this is bounded, or bound it, and add a test.
3. PR description disagrees with the diff (rule 10). It says "b2/reader.go only" and "one Fixed entry". The diff also changes writer.go (setErr, the write(p, setErr) split, the Close wait and godoc), adds copy_context_cleanup_test.go and adds a second fragment. Update the description to match the final diff and explain why the Close change is needed.

Suggestions: worth addressing

• The guard is held across Writer.write, including I/O, and copyContext cancellation blocks on it. This is safe today only because sendChunk selects on ctx.Done. With UseFileBuffer or getLargeFile, cancel would wait, which the PR says it avoids. Prove it can't happen, or test it.
• setErr cleanup (errf, network cancel) still runs under closeWrite.RLock. Confirm this is intended and covered.
• The Fixed fragment omits the new ReadFrom contract. It returns the context error without waiting, and a Read may finish later.
• Verify go test -race -count=20 -shuffle=on ./b2 and paste the result.

Suggestions: optional

• TestWriterCloseWaitsForErrorCallback uses time.After(200ms) as a negative assertion. Prefer a signal, per docs/review-lessons.md.
• Comment why guardedCopyWriter and errCopyAbandoned exist.
• Share the setup between the two new tests (b2wiretest server, release channels, callback gating).
• Rename setErr func(error) to say what it does, for example onTerminalErr. It is also used by only one caller.
• guardedCopyWriter type-asserts *Writer and calls its private methods. Consider a narrower seam.
• Fold the two closer == nil branches in copyContextWithCloser into one.
• Add Write-after-failed-Close coverage, or note that it's out of scope (#181).

Lock order (closeWrite.RLock, then guard mu, then emux) looked consistent, and the tests use channels. Both are fine.

[ffumero2003]

@david-santacruz-backblaze Thanks, all addressed (dee1563, 84b17e2, 5f1c876), and the branch is synced with main:

Definitely review

1. Changelog: fixed. Changed-20261030-113335.yaml is marked Breaking:. It says Close waits for in-flight parts and, with WithCancelOnError, for b2_cancel_large_file and errf; that an errf calling or waiting on Close deadlocks; and how to bound the wait.
2. Close hang risk: bounded and tested. The cancel RPC runs under the context ctxf returns, so a deadline there bounds Close. TestWriterCloseWaitsForCancelDeadline blocks the RPC until its context ends; making the RPC ignore that context fails with Close waited past the ctxf deadline for cancel_large_file. errf is caller code; Close and WithCancelOnError now document that it must return promptly and must not call or wait on Close.
3. PR description: rewritten to match the final diff: reader.go and writer.go (setErr, the write(p, setErr) split, the Close wait and godoc), both changelog fragments, the new test file, and why Close must wait.

Suggestions

• Guard held across Writer.write: you were right that this includes network calls. The first full chunk runs getLargeFile → b2_start_large_file (and Resume listings) under the guard. They run under w.ctx, the same context being cancelled, so they return once it ends; only a RoundTripper ignoring the request context could hold the guard longer, which would affect every API call. That bound is now a comment at guardedCopyWriter.Write. TestWriterReadFromCancellationCancelsStartLargeFile (memory and file buffers) blocks b2_start_large_file until its context ends; making it ignore the context fails with ReadFrom waited for start_large_file after cancellation.
• setErr cleanup under closeWrite.RLock: intended. Close takes closeWrite.Lock and waits for that cleanup anyway, per the contract in point 1; a comment says so and names the covering test.
• ReadFrom contract: added to the Fixed fragment (non-seekable r: returns the context error without waiting for a Read in progress, and nothing more from r is written after it returns).

Every earlier break still fails, and the full verification table is in the description. go test -race -count=30 on the cancellation tests, go test -race -count=5 -shuffle=on ./..., go vet ./... and make ci pass.