Refactor Core Architecture & Eliminate Polling

[SuperCoolPencil]

Architecture Refactor: High Level Design

This document visually illustrates the recent structural changes to the Surge architecture, moving from a tightly coupled monolithic engine to a layered, domain-driven design.

The Old Architecture (Monolithic)

In the previous design, the internal/engine package acted as a catch-all monolith. The UI and Core tightly coupled to the engine's internal workings, and state was updated via direct callbacks and polling.

graph TD
    A[TUI / CLI] --> B[internal/core]
    B --> C[internal/processing]
    C --> D((internal/engine))
    
    subgraph Monolithic Engine
        D --> E[engine/concurrent]
        D --> F[engine/single]
        D --> G[engine/transport]
        D --> H[engine/state]
        D --> I[engine/types/progress.go]
    end
    
    E -.-> |Hooks / Polling| C
    F -.-> |Hooks / Polling| C

Loading

The New Architecture (Decoupled & Layered)

The new architecture introduces strict layering and unidirectional data flow. Components are isolated, and the monolithic engine has been decomposed into strategy, scheduler, orchestrator, progress, and store. The EventBus replaces direct callback hooks, and Service provides a clean API boundary.

graph TD
    UI[TUI / CLI] --> API[Service Layer<br>internal/service]
    
    API --> ORCH[Orchestrator Layer<br>internal/orchestrator]
    
    subgraph Core Domain
        ORCH --> SCHED[Scheduler Layer<br>internal/scheduler]
        SCHED --> STRAT[Strategy Layer<br>internal/strategy]
        
        STRAT --> C[concurrent]
        STRAT --> S[single]
        
        STRAT --> PROG[Progress Tracking<br>internal/progress]
        STRAT --> TRANS[Transport / Network<br>internal/transport]
    end
    
    STRAT -.-> |Publishes Typed Events| BUS[EventBus]
    PROG -.-> |Publishes Stats| BUS
    
    BUS -.-> |Subscribed Stream| API
    API -.-> |Real-time Updates| UI
    
    ORCH --> STORE[State / DB Layer<br>internal/store]

Loading

Download Lifecycle Flow

Here is how a download request flows through the new system:

sequenceDiagram
    participant U as User (UI/CLI)
    participant S as Service
    participant O as Orchestrator
    participant SCH as Scheduler
    participant W as Strategy (Worker)
    participant B as EventBus
    
    U->>S: Add(url)
    S->>O: Enqueue Download
    O->>B: Publish(State: Queued)
    O->>SCH: Submit Task
    
    SCH->>W: Execute(Strategy)
    W->>B: Publish(State: Downloading)
    
    loop During Download
        W->>W: Fetch Chunks
        W->>B: Publish(ProgressEvent)
    end
    
    W->>SCH: Complete/Error
    W->>B: Publish(State: Completed)
    SCH->>O: Task Done

Loading

Greptile Summary

This PR decomposes the monolithic internal/engine package into layered domains (strategy, scheduler, orchestrator, progress, store, service) and replaces direct callback hooks with a typed EventBus. The change touches 152 files and corrects several previously-flagged issues: the stateProgress wrong-type assertion in the TUI, the ensureDirs data race in the store, the ReloadSettings no-op, and the broadcastLoop shutdown drain.

• New EventBus (internal/orchestrator/event_bus.go): replaces polling with a subscriber-fan-out model; drain loop before close and correct shutdown ordering (GracefulShutdown → eventBus.Shutdown) are now in place.
• enqueueResolved persistence race (internal/orchestrator/manager.go): the synchronous AddToMasterList for the queued record is called after dispatchToScheduler/pool.Add, so a fast pool worker can emit EventStarted to InputCh first; the event worker then processes EventStarted ("downloading") followed by EventQueued ("queued"), leaving the master-list entry with the wrong status during the download's lifetime.
• time.After timer leak (internal/orchestrator/event_bus.go): both broadcastMsg (non-progress path) and Publish create a time.Timer via time.After on every call without stopping it when the send branch is taken; the timers leak for 1 second after each successful send.

Confidence Score: 3/5

The enqueue path can leave a download's master-list status as 'queued' while it is actively downloading, which affects crash-recovery resume fidelity.

The ordering of dispatchToScheduler before the synchronous AddToMasterList in enqueueResolved means a fast pool worker can write EventStarted to InputCh before the queued record is persisted. The event worker then processes EventStarted then EventQueued in that order, overwriting the correct 'downloading' status with 'queued'. Any crash between that point and the next lifecycle event leaves the entry in a stale state that prevents correct resume-from-chunk behaviour.

internal/orchestrator/manager.go (enqueueResolved persist ordering) and internal/orchestrator/events.go (EventQueued handler unconditional overwrite) deserve the most attention.

Important Files Changed

Filename	Overview
internal/orchestrator/manager.go	Core lifecycle manager for enqueue/resume/cancel; dispatchToScheduler is called before the synchronous AddToMasterList in enqueueResolved, creating a race where EventStarted can arrive in InputCh before EventQueued and corrupt the download's DB status to "queued" while it is actively downloading.
internal/orchestrator/event_bus.go	New EventBus with drain-on-shutdown and subscribe/unsubscribe support; broadcastMsg and Publish use time.After without explicit timer stop, leaking a goroutine per invocation for up to 1 second on successful sends.
internal/orchestrator/events.go	StartEventWorker processes lifecycle events and persists state; EventQueued handler unconditionally overwrites any existing master-list entry with Status: "queued", which can regress a concurrent "downloading" entry set by the earlier EventStarted handler.
internal/orchestrator/pause_resume.go	Pause/Resume/ResumeBatch logic; cold-path batch resume now supplements LoadStates with master-list lookup so queued/never-started downloads can be resumed; buildResumeConfig correctly copies task list and chunk bitmap from savedState.
internal/store/db.go	ensureDirs now snapshots configured/baseDir under masterMu.RLock() before use; cleanupOrphans closure uses the captured dir parameter rather than the package-level variable; data-race concerns from previous reviews appear addressed.
internal/tui/update_events.go	stateProgress now correctly asserts state to *types.DownloadRecord then calls engineprogress.CfgProgress(dr), fixing the previously broken chunk-map visualization.
internal/service/local_service.go	ReloadSettings now delegates to lifecycle.ApplySettings(config.LoadSettings()) instead of being a no-op; service layer correctly wraps the lifecycle manager.
internal/scheduler/manager.go	uniqueFilePath now returns "" when all 100 candidates are taken, and callers can detect the failure; RunDownload uses progress.CfgProgress for safe narrowing throughout.
internal/progress/progress.go	New package centralising CfgProgress (previously duplicated across orchestrator, scheduler, and service packages); correctly guards against nil ProgressState.
internal/types/events.go	DecodeSSEMessage signature cleaned up: unused eventStr parameter and always-true bool return removed; State field correctly typed as *DownloadRecord.

Comments Outside Diff (8)

1. internal/orchestrator/pause_resume.go, line 211-232 (link)

   ResumeBatch cold path fails for downloads without a detail state file

   store.LoadStates(coldIDs) only reads from details/<id>.gob files written by SaveStateWithOptions (i.e., downloads that have been through at least one EventPaused cycle). Queued downloads that never started, and errored downloads that never saved a pause snapshot, have no detail file. For these IDs states[id] will be absent, and ResumeBatch returns "download not found or completed".

   In contrast, the single Resume path calls store.GetDownload(id) first and succeeds even when LoadState fails, because buildResumeConfig accepts a nil savedState and falls back to the master-list entry. The batch path should do the same: supplement LoadStates with a batch master-list lookup and fall through to buildResumeConfig(id, outputPath, entry, nil, settings) for IDs not present in the detail map.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: internal/orchestrator/pause_resume.go
   Line: 211-232
   
   Comment:
   **`ResumeBatch` cold path fails for downloads without a detail state file**
   
   `store.LoadStates(coldIDs)` only reads from `details/<id>.gob` files written by `SaveStateWithOptions` (i.e., downloads that have been through at least one `EventPaused` cycle). Queued downloads that never started, and errored downloads that never saved a pause snapshot, have no detail file. For these IDs `states[id]` will be absent, and `ResumeBatch` returns `"download not found or completed"`.
   
   In contrast, the single `Resume` path calls `store.GetDownload(id)` first and succeeds even when `LoadState` fails, because `buildResumeConfig` accepts a `nil` `savedState` and falls back to the master-list `entry`. The batch path should do the same: supplement `LoadStates` with a batch master-list lookup and fall through to `buildResumeConfig(id, outputPath, entry, nil, settings)` for IDs not present in the detail map.
   
   How can I resolve this? If you propose a fix, please make it concise.

2. internal/orchestrator/pause_resume.go, line 246-252 (link)

   savedState is nil when a download lives only in the master list (queued but never started or saved a pause snapshot). At this point buildResumeConfig succeeds because it falls back to entry, but the immediately following savedState.Filename dereference panics. Any ResumeBatch call that includes a download that was queued and never executed (e.g., the app was killed before the scheduler ran it) will crash the process.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: internal/orchestrator/pause_resume.go
   Line: 246-252
   
   Comment:
   `savedState` is nil when a download lives only in the master list (queued but never started or saved a pause snapshot). At this point `buildResumeConfig` succeeds because it falls back to `entry`, but the immediately following `savedState.Filename` dereference panics. Any `ResumeBatch` call that includes a download that was queued and never executed (e.g., the app was killed before the scheduler ran it) will crash the process.
   
   
   
   How can I resolve this? If you propose a fix, please make it concise.

3. internal/orchestrator/manager.go, line 331-342 (link)

   Silently dropped EventQueued means download is lost on restart

   Publish returns context.DeadlineExceeded after 1 second if InputCh is full; calling code discards the error with _ =. When this happens the download is already in the scheduler pool but StartEventWorker never sees EventQueued, so store.AddToMasterList is never called. The in-memory entry disappears on the next restart and the user has no indication the download was lost. The pool is also cleared by GracefulShutdown before state is written, so there's no recovery path.

   InputCh can fill up when broadcastLoop is delayed by slow subscribers (each non-progress event waits up to 1 s per subscriber in broadcastMsg), making this reachable without a crash — a single lagging TUI or HTTP SSE client is sufficient.

   Consider logging or surfacing a non-nil Publish error here, or using a direct store.AddToMasterList call as a synchronous fallback before the event is dispatched.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: internal/orchestrator/manager.go
   Line: 331-342
   
   Comment:
   **Silently dropped `EventQueued` means download is lost on restart**
   
   `Publish` returns `context.DeadlineExceeded` after 1 second if `InputCh` is full; calling code discards the error with `_ =`. When this happens the download is already in the scheduler pool but `StartEventWorker` never sees `EventQueued`, so `store.AddToMasterList` is never called. The in-memory entry disappears on the next restart and the user has no indication the download was lost. The pool is also cleared by `GracefulShutdown` before state is written, so there's no recovery path.
   
   `InputCh` can fill up when `broadcastLoop` is delayed by slow subscribers (each non-progress event waits up to 1 s per subscriber in `broadcastMsg`), making this reachable without a crash — a single lagging TUI or HTTP SSE client is sufficient.
   
   Consider logging or surfacing a non-nil `Publish` error here, or using a direct `store.AddToMasterList` call as a synchronous fallback before the event is dispatched.
   
   How can I resolve this? If you propose a fix, please make it concise.

4. internal/scheduler/manager.go, line 69-71 (link)

   When all 100 candidate names are already occupied, uniqueFilePath silently returns the original path — which is known to conflict (the existence check at the top of the function confirmed it). Any caller expecting a non-conflicting path from this function would silently overwrite an existing file. The fallback should either return an error-sentinel empty string or append a timestamp/random suffix to guarantee a unique result.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: internal/scheduler/manager.go
   Line: 69-71
   
   Comment:
   When all 100 candidate names are already occupied, `uniqueFilePath` silently returns the original `path` — which is known to conflict (the existence check at the top of the function confirmed it). Any caller expecting a non-conflicting path from this function would silently overwrite an existing file. The fallback should either return an error-sentinel empty string or append a timestamp/random suffix to guarantee a unique result.
   
   
   
   How can I resolve this? If you propose a fix, please make it concise.

5. cmd/root.go, line 114-124 (link)

   Unguarded type assertion on ProgressState will panic on unexpected types

   cfg.ProgressState.(*progress.DownloadProgress) is used without the comma-ok form in two places here. Because ProgressState is typed as interface{}, any value that is not *progress.DownloadProgress triggers a panic inside buildActiveDownloadChecker. The rest of the codebase consistently uses progress.CfgProgress(&cfg) for this narrowing, which handles nil and wrong-type gracefully — the new code should use the same helper.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: cmd/root.go
   Line: 114-124
   
   Comment:
   **Unguarded type assertion on `ProgressState` will panic on unexpected types**
   
   `cfg.ProgressState.(*progress.DownloadProgress)` is used without the comma-ok form in two places here. Because `ProgressState` is typed as `interface{}`, any value that is not `*progress.DownloadProgress` triggers a panic inside `buildActiveDownloadChecker`. The rest of the codebase consistently uses `progress.CfgProgress(&cfg)` for this narrowing, which handles nil and wrong-type gracefully — the new code should use the same helper.
   
   How can I resolve this? If you propose a fix, please make it concise.

6. internal/store/db.go, line 31-35 (link)

   Data race: configured and baseDir read without a lock in ensureDirs

   Configure and CloseDB write configured and baseDir under masterMu, but ensureDirs reads both without holding any lock. When SaveStateWithOptions calls ensureDirs before acquiring masterMu, a concurrent Configure or CloseDB call races with these reads. The Go race detector will flag this. Holding masterMu.RLock() for the read-only check at the top of ensureDirs would close the race.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: internal/store/db.go
   Line: 31-35
   
   Comment:
   **Data race: `configured` and `baseDir` read without a lock in `ensureDirs`**
   
   `Configure` and `CloseDB` write `configured` and `baseDir` under `masterMu`, but `ensureDirs` reads both without holding any lock. When `SaveStateWithOptions` calls `ensureDirs` before acquiring `masterMu`, a concurrent `Configure` or `CloseDB` call races with these reads. The Go race detector will flag this. Holding `masterMu.RLock()` for the read-only check at the top of `ensureDirs` would close the race.
   
   
   
   How can I resolve this? If you propose a fix, please make it concise.

7. internal/store/db.go, line 43-48 (link)

   Residual data race: cleanupOrphans(baseDir) reads package variable without a lock

   The PR applied the previous review's suggestion by reading configured and baseDir under masterMu.RLock() and snapshotting them into isConfigured/dir — but the cleanupOnce.Do closure at line 45 reads the package variable baseDir directly instead of the captured dir. This is still a concurrent read without the lock.

   Race scenario: Thread A releases masterMu.RUnlock() with dir="/path1", Thread B calls CloseDB (sets baseDir="") then Configure with /path2 (sets baseDir="/path2"), Thread A enters cleanupOnce.Do and calls cleanupOrphans("/path2") — cleaning temp files from an unrelated base directory. On Linux, if baseDir is "" when the closure fires, os.ReadDir("") traverses the current working directory and can delete any .tmp-* files found there.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: internal/store/db.go
   Line: 43-48
   
   Comment:
   **Residual data race: `cleanupOrphans(baseDir)` reads package variable without a lock**
   
   The PR applied the previous review's suggestion by reading `configured` and `baseDir` under `masterMu.RLock()` and snapshotting them into `isConfigured`/`dir` — but the `cleanupOnce.Do` closure at line 45 reads the package variable `baseDir` directly instead of the captured `dir`. This is still a concurrent read without the lock.
   
   Race scenario: Thread A releases `masterMu.RUnlock()` with `dir="/path1"`, Thread B calls `CloseDB` (sets `baseDir=""`) then `Configure` with `/path2` (sets `baseDir="/path2"`), Thread A enters `cleanupOnce.Do` and calls `cleanupOrphans("/path2")` — cleaning temp files from an unrelated base directory. On Linux, if `baseDir` is `""` when the closure fires, `os.ReadDir("")` traverses the current working directory and can delete any `.tmp-*` files found there.
   
   
   
   How can I resolve this? If you propose a fix, please make it concise.

8. internal/orchestrator/manager.go, line 312-362 (link)

   EventStarted can arrive in InputCh before EventQueued, causing status corruption

   dispatchToScheduler at line 312 calls mgr.pool.Add(cfg), which may immediately schedule a pool worker. That worker calls safeSendProgress(cfg.ProgressCh, EventStarted) — writing directly to InputCh — before enqueueResolved reaches the store.AddToMasterList call at line 342 or the Publish(EventQueued) call at line 358. The broadcastLoop then serialises them for StartEventWorker in arrival order: EventStarted is processed first (sets status "downloading"), then EventQueued is processed second (unconditional AddToMasterList with Status: "queued" at events.go:331), overwriting the correct status. Any crash between the EventQueued write and the next lifecycle event (EventComplete/EventPaused/EventError) leaves the DB entry as "queued" for a download that had already started, preventing correct crash-recovery resume.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: internal/orchestrator/manager.go
   Line: 312-362
   
   Comment:
   **EventStarted can arrive in InputCh before EventQueued, causing status corruption**
   
   `dispatchToScheduler` at line 312 calls `mgr.pool.Add(cfg)`, which may immediately schedule a pool worker. That worker calls `safeSendProgress(cfg.ProgressCh, EventStarted)` — writing directly to `InputCh` — before `enqueueResolved` reaches the `store.AddToMasterList` call at line 342 or the `Publish(EventQueued)` call at line 358. The `broadcastLoop` then serialises them for `StartEventWorker` in arrival order: `EventStarted` is processed first (sets status `"downloading"`), then `EventQueued` is processed second (unconditional `AddToMasterList` with `Status: "queued"` at `events.go:331`), overwriting the correct status. Any crash between the `EventQueued` write and the next lifecycle event (`EventComplete`/`EventPaused`/`EventError`) leaves the DB entry as `"queued"` for a download that had already started, preventing correct crash-recovery resume.
   
   How can I resolve this? If you propose a fix, please make it concise.

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
internal/orchestrator/manager.go:312-362
**EventStarted can arrive in InputCh before EventQueued, causing status corruption**

`dispatchToScheduler` at line 312 calls `mgr.pool.Add(cfg)`, which may immediately schedule a pool worker. That worker calls `safeSendProgress(cfg.ProgressCh, EventStarted)` — writing directly to `InputCh` — before `enqueueResolved` reaches the `store.AddToMasterList` call at line 342 or the `Publish(EventQueued)` call at line 358. The `broadcastLoop` then serialises them for `StartEventWorker` in arrival order: `EventStarted` is processed first (sets status `"downloading"`), then `EventQueued` is processed second (unconditional `AddToMasterList` with `Status: "queued"` at `events.go:331`), overwriting the correct status. Any crash between the `EventQueued` write and the next lifecycle event (`EventComplete`/`EventPaused`/`EventError`) leaves the DB entry as `"queued"` for a download that had already started, preventing correct crash-recovery resume.

_{Reviews (11): Last reviewed commit: "fix: handle EventRemoved state cleanup a..." | Re-trigger Greptile}

[github-actions]

Binary Size Analysis

⚠️ Size Increased

Version	Human Readable	Raw Bytes
Main	17.00 MB	17826084
PR	17.01 MB	17838372
Difference	12.00 KB	12288

[github-actions]

❌ Test Failures on windows-latest

• github.com/SurgeDM/Surge/cmd: TestAddCmdRunE_ReturnsExpectedErrors/no_running_server
• github.com/SurgeDM/Surge/cmd: TestAddCmdRunE_ReturnsExpectedErrors
• github.com/SurgeDM/Surge/cmd: TestHandleDownload_PathResolution/Absolute_Path_(Explicit)
• github.com/SurgeDM/Surge/cmd: TestHandleDownload_PathResolution/Windows_Nested_Path_Maps_Under_Default_Dir
• github.com/SurgeDM/Surge/cmd: TestHandleDownload_PathResolution
• github.com/SurgeDM/Surge/cmd: TestGetSettings_LoadError_PopulatesStartupWarnings