refactor: unify download progress state management using SessionReset and effective total size passing

[SuperCoolPencil]

Greptile Summary

This PR unifies download progress state management by introducing a SessionReset() method (replacing ad-hoc three-field resets), adding an idempotency guard to SetTotalSize (preventing clock reset when the confirmed size matches the known size), and threading effectiveTotalSize consistently to both concurrent and single-threaded downloaders. The removal of the post-download cfg.State.SetTotalSize call is safe because the concurrent downloader already calls SetTotalSize internally when it receives response headers.

Confidence Score: 5/5

Safe to merge — all findings are minor style/hardening suggestions with no blocking correctness issues.

The core logic changes are correct and well-tested. The removed SetTotalSize call was verified to be redundant (the concurrent downloader updates state internally). The only open items are a missing ActiveWorkers reset in SessionReset() (practically safe since workers exit before the fallback) and inconsistent mutex usage in the new tests.

internal/engine/types/progress.go — consider resetting ActiveWorkers in SessionReset()

Important Files Changed

Filename	Overview
internal/download/manager.go	Passes effectiveTotalSize instead of cfg.TotalSize to both downloaders and replaces the three-line manual reset with SessionReset(). Removal of the post-download cfg.State.SetTotalSize call is safe — the concurrent downloader already calls SetTotalSize internally (line 672 of concurrent/downloader.go), so the post-download call was redundant.
internal/engine/types/progress.go	Adds SessionReset() (comprehensive state wipe for fallback) and an idempotency guard to SetTotalSize (prevents clock reset when the same size is re-confirmed). Minor gap: ActiveWorkers is not reset in SessionReset().
internal/engine/types/progress_test.go	Adds TestProgressState_SetTotalSize_Idempotent and TestProgressState_SessionReset with good coverage. Tests directly access mutex-protected struct fields without holding the lock, which is inconsistent with the struct's locking discipline.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: internal/engine/types/progress.go
Line: 227-251

Comment:
**`SessionReset` does not reset `ActiveWorkers`**

After a failed concurrent download, `ActiveWorkers` may be non-zero if workers haven't fully decremented by the time `d.Download()` returns (e.g. via a deferred cleanup race). Leaving `ActiveWorkers` un-reset means the TUI could briefly display a stale connection count between the fallback detection and the moment the single-threaded downloader sets its own `ActiveWorkers`.

```suggestion
func (ps *ProgressState) SessionReset() {
	ps.mu.Lock()
	defer ps.mu.Unlock()

	ps.Downloaded.Store(0)
	ps.VerifiedProgress.Store(0)
	ps.SessionStartBytes = 0
	ps.StartTime = time.Now()
	ps.SavedElapsed = 0
	ps.Done.Store(false)
	ps.Paused.Store(false)
	ps.Pausing.Store(false)
	ps.Error.Store(nil)
	ps.ActiveWorkers.Store(0)
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: internal/engine/types/progress_test.go
Line: 51-68

Comment:
**Direct access to mutex-protected field in test**

`ps.StartTime` is protected by `ps.mu` (as documented in the struct comment). The test writes to it directly (`ps.StartTime = originalStartTime`) and reads it back without holding the lock. While safe in a sequential test, Go's `-race` detector can flag unsynchronized accesses even when no goroutine is concurrently running, and it sets an inconsistent precedent for future test authors.

Consider adding a helper or a lock-guarded setter, or at minimum using `ps.mu.Lock()`/`ps.mu.Unlock()` around the direct field accesses in the test to align with the struct's locking discipline.

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "test: add idempotency tests for SetTotal..." | Re-trigger Greptile}

Greptile also left 2 inline comments on this PR.

[greptile-apps]

SessionReset does not reset ActiveWorkers

After a failed concurrent download, ActiveWorkers may be non-zero if workers haven't fully decremented by the time d.Download() returns (e.g. via a deferred cleanup race). Leaving ActiveWorkers un-reset means the TUI could briefly display a stale connection count between the fallback detection and the moment the single-threaded downloader sets its own ActiveWorkers.

Suggested change

	func (ps *ProgressState) SessionReset() {
	ps.mu.Lock()
	defer ps.mu.Unlock()
	ps.Downloaded.Store(0)
	ps.VerifiedProgress.Store(0)
	ps.SessionStartBytes = 0
	ps.StartTime = time.Now()
	ps.SavedElapsed = 0
	ps.Done.Store(false)
	ps.Paused.Store(false)
	ps.Pausing.Store(false)
	ps.Error.Store(nil)
	// Clear mirrors error status
	for i := range ps.Mirrors {
	ps.Mirrors[i].Error = false
	}
	// Reset chunk tracking if initialized
	if ps.BitmapWidth > 0 {
	ps.ChunkBitmap = make([]byte, len(ps.ChunkBitmap))
	ps.ChunkProgress = make([]int64, ps.BitmapWidth)
	}
	}
	func (ps *ProgressState) SessionReset() {
	ps.mu.Lock()
	defer ps.mu.Unlock()
	ps.Downloaded.Store(0)
	ps.VerifiedProgress.Store(0)
	ps.SessionStartBytes = 0
	ps.StartTime = time.Now()
	ps.SavedElapsed = 0
	ps.Done.Store(false)
	ps.Paused.Store(false)
	ps.Pausing.Store(false)
	ps.Error.Store(nil)
	ps.ActiveWorkers.Store(0)

Prompt To Fix With AI

This is a comment left during a code review.
Path: internal/engine/types/progress.go
Line: 227-251

Comment:
**`SessionReset` does not reset `ActiveWorkers`**

After a failed concurrent download, `ActiveWorkers` may be non-zero if workers haven't fully decremented by the time `d.Download()` returns (e.g. via a deferred cleanup race). Leaving `ActiveWorkers` un-reset means the TUI could briefly display a stale connection count between the fallback detection and the moment the single-threaded downloader sets its own `ActiveWorkers`.

```suggestion
func (ps *ProgressState) SessionReset() {
	ps.mu.Lock()
	defer ps.mu.Unlock()

	ps.Downloaded.Store(0)
	ps.VerifiedProgress.Store(0)
	ps.SessionStartBytes = 0
	ps.StartTime = time.Now()
	ps.SavedElapsed = 0
	ps.Done.Store(false)
	ps.Paused.Store(false)
	ps.Pausing.Store(false)
	ps.Error.Store(nil)
	ps.ActiveWorkers.Store(0)
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

Direct access to mutex-protected field in test

ps.StartTime is protected by ps.mu (as documented in the struct comment). The test writes to it directly (ps.StartTime = originalStartTime) and reads it back without holding the lock. While safe in a sequential test, Go's -race detector can flag unsynchronized accesses even when no goroutine is concurrently running, and it sets an inconsistent precedent for future test authors.

Consider adding a helper or a lock-guarded setter, or at minimum using ps.mu.Lock()/ps.mu.Unlock() around the direct field accesses in the test to align with the struct's locking discipline.

Prompt To Fix With AI

This is a comment left during a code review.
Path: internal/engine/types/progress_test.go
Line: 51-68

Comment:
**Direct access to mutex-protected field in test**

`ps.StartTime` is protected by `ps.mu` (as documented in the struct comment). The test writes to it directly (`ps.StartTime = originalStartTime`) and reads it back without holding the lock. While safe in a sequential test, Go's `-race` detector can flag unsynchronized accesses even when no goroutine is concurrently running, and it sets an inconsistent precedent for future test authors.

Consider adding a helper or a lock-guarded setter, or at minimum using `ps.mu.Lock()`/`ps.mu.Unlock()` around the direct field accesses in the test to align with the struct's locking discipline.

How can I resolve this? If you propose a fix, please make it concise.