fix: join download workers before closing the upload channel

core's Download returns without waiting on its worker group when the
iterator reports an error, so surfacing one through Iter.Err left workers
sending into a channel the caller had already closed. The iterator now
always reports a nil error and stashes the real one, read after Download
returns.

The circuit breaker cancelled only the upload context, which left
downloads running full speed against a remote refusing them: every file
stayed in staging and every reservation came back, so a broken remote
filled local disk faster than a working one. It now stops the download
iterator instead.

A failed move leaves the local copy in place, so the byte reservation
cannot be handed back until the file is removed. A confirmed-short object
is deleted rather than left under a name verification would count as
archived forever.

Also: release the reservation when opening the staging file fails, report
results even when an upload errored, bound the recorded errors, and drop
the reporter's lock before writing so terminal latency cannot throttle
downloads.

internal/pipeline/download.go

@@ -8,6 +8,7 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+	"sync/atomic"
 
 	"github.com/iyear/tdl/core/dcpool"
 	"github.com/iyear/tdl/core/downloader"
@@ -29,8 +30,14 @@ type DownloadOptions struct {
 	Report func(Stats)
 
 	// acquire reserves staging space before a download starts, blocking until
-	// there is room. Unset means no bound.
+	// there is room. Unset means no bound. release hands a reservation back for
+	// an item that never reaches a download.
 	acquire func(context.Context, int64) error
+	release func(int64)
+
+	// stop, when set, ends iteration cleanly from another goroutine — used to
+	// halt downloads once the destination has stopped accepting uploads.
+	stop *atomic.Bool
 	// onReady hands a completed file to the upload leg; onFailed says nothing
 	// was staged, so whatever acquire reserved must be given back.
 	onReady  func(tgsource.Item)
@@ -39,12 +46,13 @@ type DownloadOptions struct {
 
 // Download fetches every item in seq into the staging directory.
 //
-// Each file is written to <name>.part and renamed to <name> only once the
-// downloader reports it complete, so a name without the suffix is always a
-// whole file. That is what lets the upload half treat "the file exists" as
-// "the file is finished" — the property the shell pipeline had to approximate
-// with a filename convention plus an age guard, because it could not see
-// inside tdl.
+// Each file is written to <name>.part and renamed to <name> only once its size
+// matches what Telegram reported, so a name without the suffix is always a whole
+// file. Uploads are driven by completion rather than by scanning for that, but
+// the invariant still matters: it is what makes a leftover file from an
+// interrupted run safe to keep and a leftover .part safe to delete. The shell
+// pipeline could only approximate it with a filename convention plus an age
+// guard, because it could not see inside tdl.
 //
 // A failed item does not abort the run: it is recorded in the returned outcomes
 // and the rest continue, matching what a partial `tdl dl` pass did.
@@ -61,6 +69,10 @@ func Download(ctx context.Context, seq iter.Seq2[tgsource.Item, error], o Downlo
 
 	it := newElemIter(seq, o.Staging, o.Takeout)
 	it.acquire = o.acquire
+	it.release = o.release
+	if o.stop != nil {
+		it.stopped = o.stop
+	}
 	defer func() { _ = it.Close() }()
 
 	prog := newProgress(func(e *elem, err error) error {
@@ -85,6 +97,12 @@ func Download(ctx context.Context, seq iter.Seq2[tgsource.Item, error], o Downlo
 	}).Download(ctx, o.Limit)
 
 	outcomes, stats := prog.results()
+	// The iterator's failure is read only now, after Download has joined every
+	// worker. Reporting it through Iter.Err would have made Download skip that
+	// join entirely.
+	if err == nil {
+		err = it.failure
+	}
 	return outcomes, stats, err
 }
 

internal/pipeline/download_test.go

@@ -1,6 +1,7 @@
 package pipeline
 
 import (
+	"context"
 	"errors"
 	"os"
 	"path/filepath"
@@ -141,12 +142,11 @@ func TestElemIterRejectsUnsafeNames(t *testing.T) {
 	if it.Next(t.Context()) {
 		t.Fatal("iterator accepted a name that escapes the staging directory")
 	}
-	err := it.Err()
-	if err == nil {
-		t.Fatal("Err() = nil after rejecting an unsafe name")
+	if it.failure == nil {
+		t.Fatal("no failure recorded after rejecting an unsafe name")
 	}
-	if !strings.Contains(err.Error(), "message 7") {
-		t.Errorf("error should name the message, got: %v", err)
+	if !strings.Contains(it.failure.Error(), "message 7") {
+		t.Errorf("failure should name the message, got: %v", it.failure)
 	}
 }
 
@@ -183,8 +183,8 @@ func TestElemIterOpensPartFilesAndPropagatesWalkErrors(t *testing.T) {
 		if it.Next(t.Context()) {
 			t.Fatal("Next() = true after a walk error")
 		}
-		if !errors.Is(it.Err(), want) {
-			t.Errorf("Err() = %v, want %v", it.Err(), want)
+		if !errors.Is(it.failure, want) {
+			t.Errorf("failure = %v, want %v", it.failure, want)
 		}
 	})
 }
@@ -288,3 +288,112 @@ func TestFinishAcceptsExactSize(t *testing.T) {
 		t.Errorf("final size = %d, want 2048", info.Size())
 	}
 }
+
+// Err must always report nil, however badly iteration went.
+//
+// core's Download skips wg.Wait entirely when Iter.Err is non-nil
+// (downloader.go:65-68), returning while its workers are still running. The
+// pipeline closes its upload channel as soon as Download returns, so a non-nil
+// Err here means workers send on a closed channel and the process panics —
+// on every Ctrl-C, since cancellation is one of the ways iteration stops.
+func TestElemIterNeverReportsErrToTheDownloader(t *testing.T) {
+	staging := t.TempDir()
+
+	cases := map[string]func() *elemIter{
+		"walk error": func() *elemIter {
+			seq := func(yield func(tgsource.Item, error) bool) {
+				yield(tgsource.Item{}, errors.New("boom"))
+			}
+			return newElemIter(seq, staging, false)
+		},
+		"unsafe name": func() *elemIter {
+			seq := func(yield func(tgsource.Item, error) bool) {
+				yield(testItem(t, 1, "../escape", 10), nil)
+			}
+			return newElemIter(seq, staging, false)
+		},
+		"cancelled": func() *elemIter {
+			seq := func(yield func(tgsource.Item, error) bool) {
+				yield(testItem(t, 2, "a.mp4", 10), nil)
+			}
+			return newElemIter(seq, staging, false)
+		},
+	}
+
+	for name, build := range cases {
+		t.Run(name, func(t *testing.T) {
+			it := build()
+			defer func() { _ = it.Close() }()
+
+			ctx := t.Context()
+			if name == "cancelled" {
+				cancelled, cancel := context.WithCancel(ctx)
+				cancel()
+				ctx = cancelled
+			}
+
+			for it.Next(ctx) {
+			}
+			if err := it.Err(); err != nil {
+				t.Errorf("Err() = %v, want nil — a non-nil Err makes Download abandon its workers", err)
+			}
+			if it.failure == nil {
+				t.Error("the real failure was not stashed")
+			}
+		})
+	}
+}
+
+// A caller-set stop flag ends iteration without looking like a failure, which is
+// how the circuit breaker halts downloads.
+func TestElemIterStopsOnFlagWithoutRecordingFailure(t *testing.T) {
+	staging := t.TempDir()
+	seq := func(yield func(tgsource.Item, error) bool) {
+		for i := 1; i <= 5; i++ {
+			if !yield(testItem(t, i, "a.mp4", 10), nil) {
+				return
+			}
+		}
+	}
+	it := newElemIter(seq, staging, false)
+	defer func() { _ = it.Close() }()
+
+	if !it.Next(t.Context()) {
+		t.Fatal("first Next() = false")
+	}
+	it.stopped.Store(true)
+
+	if it.Next(t.Context()) {
+		t.Error("Next() = true after the stop flag was set")
+	}
+	if it.failure != nil {
+		t.Errorf("failure = %v, want nil — stopping is not a failure", it.failure)
+	}
+}
+
+// A reservation must come back when the item never reaches a download, or the
+// budget shrinks by that much for the rest of the run.
+func TestElemIterReturnsReservationWhenOpenFails(t *testing.T) {
+	// A staging path that is a file, not a directory, makes OpenFile fail.
+	staging := filepath.Join(t.TempDir(), "not-a-dir")
+	if err := os.WriteFile(staging, []byte("x"), 0o600); err != nil {
+		t.Fatalf("seed: %v", err)
+	}
+
+	seq := func(yield func(tgsource.Item, error) bool) {
+		yield(testItem(t, 1, "a.mp4", 4096), nil)
+	}
+	it := newElemIter(seq, staging, false)
+	defer func() { _ = it.Close() }()
+
+	var acquired, released int64
+	it.acquire = func(_ context.Context, n int64) error { acquired += n; return nil }
+	it.release = func(n int64) { released += n }
+
+	if it.Next(t.Context()) {
+		t.Fatal("Next() succeeded with an unusable staging directory")
+	}
+	if acquired != released {
+		t.Errorf("acquired %d bytes but released %d — the reservation leaked", acquired, released)
+	}
+}

internal/pipeline/elem.go

@@ -8,6 +8,7 @@ import (
 	"iter"
 	"os"
 	"path/filepath"
+	"sync/atomic"
 
 	"github.com/gotd/td/tg"
 
@@ -59,37 +60,57 @@ func finalPath(staging string, it tgsource.Item) string {
 // bridges them without this code owning a goroutine or a channel, which is why
 // Walk returns a sequence in the first place.
 type elemIter struct {
-	next    func() (tgsource.Item, error, bool)
-	stop    func()
-	staging string
-	takeout bool
+	next     func() (tgsource.Item, error, bool)
+	stopPull func()
+	staging  string
+	takeout  bool
 
 	// acquire reserves staging space for the next item. Blocking here is what
 	// makes backpressure work: core's Download calls Next from its dispatch
-	// loop, so a blocked Next stops new downloads starting without stopping the
-	// uploads that free the space.
+	// loop (downloader.go:38), so a blocked Next stops new downloads starting
+	// without stopping the uploads that free the space.
 	acquire func(context.Context, int64) error
+	// release hands a reservation back when the item never reaches a download.
+	release func(int64)
 
 	current *elem
-	err     error
 
-	// opened records every file handle so a run can close them all. The
-	// downloader never closes what To() hands it, and a leak here is thousands
-	// of descriptors on a full archive run.
+	// failure holds why iteration stopped, and Err deliberately does not return
+	// it. core's Download skips wg.Wait entirely when Iter.Err is non-nil
+	// (downloader.go:65-68), abandoning workers that are still running — which
+	// would let this package tear down its upload channel underneath them. So
+	// Next reports "no more items" and the caller reads failure() afterwards,
+	// guaranteeing every worker has finished first.
+	failure error
+	// stopped ends iteration without an error, for a caller that has decided the
+	// run cannot usefully continue. Supplied by the caller so it can be set from
+	// another goroutine without racing on the iterator itself.
+	stopped *atomic.Bool
+
+	// opened records every file handle. finish closes each one on the normal
+	// path, so this is not what keeps descriptors from leaking; it is the
+	// backstop for items that were opened but never reached finish, which is
+	// what an aborted iteration leaves behind.
 	opened []*os.File
 }
 
 func newElemIter(seq iter.Seq2[tgsource.Item, error], staging string, takeout bool) *elemIter {
-	next, stop := iter.Pull2(seq)
-	return &elemIter{next: next, stop: stop, staging: staging, takeout: takeout}
+	next, stopPull := iter.Pull2(seq)
+	return &elemIter{
+		next:     next,
+		stopPull: stopPull,
+		staging:  staging,
+		takeout:  takeout,
+		stopped:  new(atomic.Bool),
+	}
 }
 
 func (i *elemIter) Next(ctx context.Context) bool {
-	if i.err != nil {
+	if i.failure != nil || i.stopped.Load() {
 		return false
 	}
 	if err := ctx.Err(); err != nil {
-		i.err = err
+		i.failure = err
 		return false
 	}
 
@@ -98,7 +119,7 @@ func (i *elemIter) Next(ctx context.Context) bool {
 		return false
 	}
 	if err != nil {
-		i.err = err
+		i.failure = err
 		return false
 	}
 
@@ -106,20 +127,25 @@ func (i *elemIter) Next(ctx context.Context) bool {
 	// os.Create: the error names the message, and the run continues instead of
 	// failing on a path that could never have worked.
 	if err := naming.Safe(item.Name); err != nil {
-		i.err = fmt.Errorf("message %d: %w", item.MessageID, err)
+		i.failure = fmt.Errorf("message %d: %w", item.MessageID, err)
 		return false
 	}
 
 	if i.acquire != nil {
 		if err := i.acquire(ctx, item.Size()); err != nil {
-			i.err = err
+			i.failure = err
 			return false
 		}
 	}
 
 	f, err := os.OpenFile(partPath(i.staging, item), os.O_CREATE|os.O_RDWR, 0o600)
 	if err != nil {
-		i.err = fmt.Errorf("open destination for message %d: %w", item.MessageID, err)
+		// The reservation is handed back here because this item will never
+		// reach a download, so no OnDone will ever release it for us.
+		if i.release != nil {
+			i.release(item.Size())
+		}
+		i.failure = fmt.Errorf("open destination for message %d: %w", item.MessageID, err)
 		return false
 	}
 	i.opened = append(i.opened, f)
@@ -129,11 +155,14 @@ func (i *elemIter) Next(ctx context.Context) bool {
 }
 
 func (i *elemIter) Value() downloader.Elem { return i.current }
-func (i *elemIter) Err() error             { return i.err }
+
+// Err always reports nil so core's Download reaches wg.Wait and joins its
+// workers. See the failure field.
+func (i *elemIter) Err() error { return nil }
 
 // Close releases the pull iterator and every file the walk opened.
 func (i *elemIter) Close() error {
-	i.stop()
+	i.stopPull()
 	var firstErr error
 	for _, f := range i.opened {
 		if err := f.Close(); err != nil && firstErr == nil {

internal/pipeline/pipeline.go

@@ -5,7 +5,10 @@ import (
 	"errors"
 	"fmt"
 	"iter"
+	"os"
+	"path/filepath"
 	"sync"
+	"sync/atomic"
 
 	"github.com/rclone/rclone/fs"
 	"golang.org/x/sync/semaphore"
@@ -53,6 +56,11 @@ func (r Result) Failed() []Outcome {
 	return out
 }
 
+// maxRecordedErrors bounds what a run keeps from a failing remote. Past this,
+// the pattern is established and joining thousands of identical strings just
+// makes the final message unreadable.
+const maxRecordedErrors = 10
+
 // Run downloads every item and uploads each one as it completes.
 //
 // This is the whole reason for the rewrite. run.sh could not see inside tdl, so
@@ -60,13 +68,17 @@ func (r Result) Failed() []Outcome {
 // `du -sk` every ten seconds, and enforced its disk cap by sending SIGSTOP and
 // SIGCONT to the tdl process. None of that exists here. Completion is a function
 // returning. The cap is a semaphore: a download acquires its own size before
-// starting and releases it only once the upload has confirmed, so when the
-// remote is slow the acquire blocks and downloads pause on their own.
+// starting and releases it once the file is off local disk, so when the remote
+// is slow the acquire blocks and downloads pause on their own.
 //
-// Blocking in the iterator is safe by construction — core's Download calls
-// Iter.Next from its dispatch loop while workers run in an errgroup, so a
-// blocked Next stalls new work without stopping the uploads that free the budget
-// (downloader.go:36-63).
+// Blocking in the iterator is safe, but not for the reason it first appears.
+// core's Download calls Iter.Next from its dispatch loop while workers run in an
+// errgroup, so a blocked Next stalls new work without stopping the uploads that
+// free the budget. What is *not* safe is reporting an error through Iter.Err:
+// Download then returns without joining its workers (downloader.go:65-68), and
+// tearing down the upload channel underneath them panics. So elemIter always
+// reports a nil Err and stashes the real one, which Download's return
+// guarantees is safe to read.
 func Run(ctx context.Context, seq iter.Seq2[tgsource.Item, error], o Options) (Result, error) {
 	if o.Uploads <= 0 {
 		o.Uploads = 1
@@ -84,37 +96,51 @@ func Run(ctx context.Context, seq iter.Seq2[tgsource.Item, error], o Options) (R
 	budget := newBudget(o.Budget)
 	uploads := make(chan tgsource.Item, o.Uploads)
 
-	// Upload workers own the release side of the budget, so every path out of
-	// one — success, failure, cancellation — must release, or the run deadlocks
-	// with downloads waiting on space that is never freed.
 	var (
-		wg         sync.WaitGroup
-		mu         sync.Mutex
-		uploadErrs []error
-		streak     int
-		tripped    bool
+		wg      sync.WaitGroup
+		mu      sync.Mutex
+		errs    []error
+		nErrs   int
+		streak  int
+		tripped bool
 	)
-	upCtx, tripRun := context.WithCancel(ctx)
-	defer tripRun()
+
+	// stopDownloads ends the download side once the destination has stopped
+	// accepting work. It stops the iterator rather than cancelling a context,
+	// because cancelling only the uploads would leave downloads running at full
+	// speed against a remote that is refusing them — every file staying on disk,
+	// every reservation released on the way out. A broken remote would fill the
+	// local disk faster than a working one does.
+	stopDownloads := new(atomic.Bool)
 
 	for range o.Uploads {
 		wg.Add(1)
 		go func() {
 			defer wg.Done()
 			for it := range uploads {
-				err := up.upload(upCtx, it)
+				err := up.upload(ctx, it)
+
+				if err != nil {
+					// MoveFile leaves the local copy in place when it fails, so
+					// the reservation cannot simply be handed back — the bytes
+					// are still on disk. Removing the file first is what keeps
+					// the cap honest.
+					if rerr := os.Remove(filepath.Join(o.Staging, it.Name)); rerr != nil && !os.IsNotExist(rerr) {
+						err = errors.Join(err, fmt.Errorf("and it is still in staging: %w", rerr))
+					}
+				}
 				budget.release(it.Size())
 
 				mu.Lock()
 				if err != nil {
-					uploadErrs = append(uploadErrs, err)
+					if nErrs < maxRecordedErrors {
+						errs = append(errs, err)
+					}
+					nErrs++
 					streak++
 					if streak >= o.MaxFailures && !tripped {
-						// A remote that fails this many times running is not
-						// going to recover on its own, and continuing just fills
-						// staging until the disk does.
 						tripped = true
-						tripRun()
+						stopDownloads.Store(true)
 					}
 				} else {
 					streak = 0
@@ -132,20 +158,24 @@ func Run(ctx context.Context, seq iter.Seq2[tgsource.Item, error], o Options) (R
 		Takeout: o.Takeout,
 		Report:  o.Report,
 		acquire: budget.acquire,
+		release: budget.release,
 		onReady: func(it tgsource.Item) { uploads <- it },
 		onFailed: func(it tgsource.Item) {
 			// Nothing was staged, so the reservation has to come back here
 			// instead of from an upload that will never happen.
 			budget.release(it.Size())
 		},
+		stop: stopDownloads,
 	})
 
+	// Safe only because Download joined its workers, which is guaranteed by
+	// elemIter.Err always being nil.
 	close(uploads)
 	wg.Wait()
 
 	mu.Lock()
-	errs := append([]error(nil), uploadErrs...)
-	trip := tripped
+	joined := errors.Join(errs...)
+	trip, total := tripped, nErrs
 	mu.Unlock()
 
 	res := Result{Stats: stats, Outcomes: dlOutcomes}
@@ -153,10 +183,10 @@ func Run(ctx context.Context, seq iter.Seq2[tgsource.Item, error], o Options) (R
 	case dlErr != nil:
 		return res, dlErr
 	case trip:
-		return res, fmt.Errorf("stopping after %d consecutive upload failures: %w",
-			o.MaxFailures, errors.Join(errs...))
-	case len(errs) > 0:
-		return res, errors.Join(errs...)
+		return res, fmt.Errorf("stopped after %d consecutive upload failures (%d total): %w",
+			o.MaxFailures, total, joined)
+	case total > 0:
+		return res, fmt.Errorf("%d upload(s) failed: %w", total, joined)
 	}
 	return res, nil
 }
@@ -176,10 +206,10 @@ func (b *budget) acquire(ctx context.Context, n int64) error {
 	if b.sem == nil {
 		return nil
 	}
-	// An item larger than the whole budget could never be admitted and would
-	// block forever, so it is refused with an error that says what to change.
-	// Callers validate up front too; this is the guard for an item whose size
-	// was not known then.
+	// An item larger than the budget cannot be admitted, and semaphore.Acquire
+	// handles that by blocking until the context is cancelled rather than
+	// failing — so there is no error to surface and no guard to add here. The
+	// real protection is validateBudget refusing such a run before it starts.
 	if err := b.sem.Acquire(ctx, n); err != nil {
 		return fmt.Errorf("waiting for %d bytes of staging space: %w", n, err)
 	}

internal/pipeline/upload.go

@@ -3,6 +3,7 @@ package pipeline
 import (
 	"context"
 	"fmt"
+	"time"
 
 	"github.com/rclone/rclone/fs"
 	"github.com/rclone/rclone/fs/operations"
@@ -21,13 +22,15 @@ type uploader struct {
 // arrived at the expected size.
 //
 // MoveFile removes the local copy as part of the move, so a successful return
-// means the file is on the remote and off local disk — which is what lets the
-// byte budget be released.
+// means the file is on the remote and off local disk.
 //
-// Confirmation closes a gap the shell pipeline left open: there, a truncated
-// upload was only noticed by a later verify pass, after the local copy was
-// already gone. Re-stating the object costs one round trip per file and turns a
-// silent corruption into a retry.
+// A short object is deleted rather than left in place, and that is the part that
+// matters. Verification matches on name and non-zero size, so a truncated object
+// under the right name would be counted archived by this run and by every run
+// after it — permanently, with the local copy already gone. Removing it turns a
+// silent corruption into an absent file the next run fetches again. The shell
+// pipeline had this hole too: it noticed a bad upload only at the next verify,
+// by which point the evidence was the same.
 func (u *uploader) upload(ctx context.Context, it tgsource.Item) error {
 	if err := operations.MoveFile(ctx, u.dst, u.local, it.Name, it.Name); err != nil {
 		return fmt.Errorf("move %q to %s: %w", it.Name, u.dst.String(), err)
@@ -41,7 +44,17 @@ func (u *uploader) upload(ctx context.Context, it tgsource.Item) error {
 		return fmt.Errorf("confirm %q: %w", it.Name, err)
 	}
 	if got := obj.Size(); got != it.Size() {
-		return fmt.Errorf("confirm %q: remote has %d bytes, expected %d", it.Name, got, it.Size())
+		err := fmt.Errorf("confirm %q: remote has %d bytes, expected %d", it.Name, got, it.Size())
+		// Deleted on a fresh context: the run may already be shutting down, and
+		// leaving a plausible-looking short object behind is worse than the
+		// error that got us here.
+		delCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 30*time.Second)
+		defer cancel()
+		if derr := operations.DeleteFile(delCtx, obj); derr != nil {
+			return fmt.Errorf("%w (and it could not be removed: %v — delete it by hand "+
+				"or verify will count it archived)", err, derr)
+		}
+		return err
 	}
 	return nil
 }

internal/report/progress.go

@@ -41,10 +41,17 @@ func New(w io.Writer, total int, totalBytes int64) *Reporter {
 	return &Reporter{w: w, tty: isTerminal(w), total: total, totalBytes: totalBytes, started: time.Now()}
 }
 
-// Update renders a snapshot. Safe to call from several goroutines, and cheap
-// enough to call on every progress callback.
+// Update renders a snapshot. Safe to call from several goroutines.
+//
+// A contended update is dropped rather than queued. Every download worker calls
+// this on each progress callback, so holding the lock across the write would
+// make terminal latency — an ssh session with a slow link, say — throttle the
+// downloads themselves. A skipped frame costs nothing; the next callback is
+// milliseconds away and Finish always prints.
 func (r *Reporter) Update(s pipeline.Stats) {
-	r.mu.Lock()
+	if !r.mu.TryLock() {
+		return
+	}
 	defer r.mu.Unlock()
 
 	now := time.Now()