Skip to content

fix: emit EventError once after all retries; skip retries for 4xx errors - #541

Merged
SuperCoolPencil merged 9 commits into
mainfrom
idk
Jul 23, 2026
Merged

fix: emit EventError once after all retries; skip retries for 4xx errors#541
SuperCoolPencil merged 9 commits into
mainfrom
idk

Conversation

@SuperCoolPencil

@SuperCoolPencil SuperCoolPencil commented Jul 23, 2026

Copy link
Copy Markdown
Member

Greptile Summary

This PR revises download failure handling and scheduler shutdown behavior.

  • Emits the final error event from the scheduler only after retries are exhausted.
  • Treats most HTTP 4xx responses as permanent failures while retaining retries for 429 responses.
  • Preserves incomplete downloads and concurrent-download progress for later resumption.
  • Moves potentially blocking progress sends outside the scheduler mutex and closes the shutdown signal before waiting for workers.
  • Adds coverage for permanent HTTP errors and single error-event emission.

Confidence Score: 5/5

The PR appears safe to merge with no blocking failures remaining.

The scheduler-lock, shutdown, and stale-filename error-event failures are addressed without an eligible distinct regression.

T-Rex T-Rex Logs

What T-Rex did

  • Ran the targeted Go tests for internal/types and internal/scheduler with a focused run of Test(IsPermanentHTTP|Worker_SkipsRetriesOnPermanentError|RunDownload_DoesNotEmitEventErrorOnFailure), using -count=1 -v and a GOCACHE path.
  • Verified the current run reports that internal/types passes HTTP status handling for permanent (400, 401, 403, 404) and retryable (429, 5xx) cases, and that internal/scheduler passes both lifecycle tests.
  • Compared the current results with the pre-change runtime, which showed RunDownload emitting an EventError and four EventErrors for a permanent 403 failure, and observed that the current implementation completes with a single terminal scheduler-owned EventError.

View all artifacts

T-Rex Ran code and verified through T-Rex

Important Files Changed

Filename Overview
internal/scheduler/scheduler.go Reworks retry classification, queue accounting, error-event emission, backoff, and shutdown ordering without leaving an eligible blocking failure.
internal/scheduler/manager.go Removes per-attempt error emission and avoids discarding partial progress during concurrent-to-single fallback.
internal/orchestrator/events.go Preserves incomplete download files when processing terminal error events.
internal/strategy/concurrent/downloader.go Marks qualifying 4xx bootstrap responses as permanent HTTP failures.
internal/strategy/single/downloader.go Marks qualifying 4xx download responses as permanent HTTP failures.
internal/types/errors.go Introduces permanent HTTP error classification, excluding rate-limited responses.

Sequence Diagram

sequenceDiagram
participant W as Scheduler worker
participant D as Downloader
participant P as Progress channel
participant S as Graceful shutdown
W->>D: RunDownload
D-->>W: Return error
alt retryable and attempts remain
    W->>W: Requeue task as in-flight
    W->>P: Emit queued event outside mutex
    W->>W: Clear in-flight state and back off
else permanent or retries exhausted
    W->>P: Emit one error event outside mutex
    W->>W: Complete task accounting
end
S->>P: Close progressDone
S->>W: Wait for completion
Loading

Reviews (5): Last reviewed commit: "fix: keep tasks in-flight during retry p..." | Re-trigger Greptile

When a download failed, RunDownload emitted EventError before returning
its error to the scheduler. The scheduler then silently retried up to 3
times, so each retry called RunDownload again and emitted another
EventError — resulting in 4 error messages, 4 desktop notifications, and
4 file deletion attempts in the orchestrator for a single download.

Two bugs, one fix:

1. EventError emitted on every attempt, not just the final failure
   RunDownload was sending EventError to ProgressCh before returning.
   Since the scheduler retries transparently, the TUI and orchestrator
   both received EventError on every attempt. Now EventError is emitted
   exclusively by worker() in scheduler.go, after the retry gate — so it
   fires exactly once when the download has truly failed.

2. Permanent HTTP errors (4xx) were retried like transient failures
   The retry guard only excluded context.Canceled/DeadlineExceeded. A
   403 Forbidden (expired signed URL), 404 Not Found, or 401
   Unauthorized is a permanent server rejection; retrying 3 times wastes
   ~2s and floods the UI. Added ErrPermanentHTTP sentinel and
   IsPermanentHTTPStatus helper to types/errors.go. The single and
   concurrent downloaders now wrap 4xx (except 429 Too Many Requests,
   which is transient) with this sentinel. The scheduler's retry gate
   checks for it and fails immediately.

Affected files:
- internal/types/errors.go: ErrPermanentHTTP + IsPermanentHTTPError/Status
- internal/strategy/single/downloader.go: wrap 4xx with ErrPermanentHTTP
- internal/strategy/concurrent/downloader.go: wrap bootstrap 4xx
- internal/scheduler/manager.go: remove EventError send from RunDownload
- internal/scheduler/scheduler.go: emit EventError once; gate on permanent errors
Comment thread internal/scheduler/scheduler.go Outdated
Comment thread internal/scheduler/scheduler.go Outdated
Comment thread internal/scheduler/scheduler.go
…retries and preventing concurrent-to-sequential fallback after partial progress
…ID_UsesProvidedID to verify custom ID handling
…emature task removal and handle shutdowns correctly
@SuperCoolPencil
SuperCoolPencil merged commit aac5d59 into main Jul 23, 2026
14 checks passed
@SuperCoolPencil
SuperCoolPencil deleted the idk branch July 23, 2026 22:19
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.

1 participant