fix/phantom progress on 403 - #573
Conversation
…tus codes in worker execution
…permanent error - Moves activeTasks cleanup to success path to ensure snapshot includes failing tasks - Wraps permanent HTTP status codes (401, 403, 404) with ErrPermanentHTTP so scheduler doesn't retry them 10 times
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe concurrent downloader preserves shared task offsets, updates worker failure handling, wraps permanent HTTP errors, and adds coverage for pausing during retry backoff. The change also includes formatting-only updates. ChangesConcurrent downloader behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/strategy/concurrent/worker.go`:
- Around line 169-183: Update the failed-task handling in the worker flow around
activeTasks and saveStateSnapshot so an errored task cannot contribute work to a
saved snapshot. Mark failed entries terminal and make snapshot collection skip
them, or otherwise ensure balancing completes before saveStateSnapshot runs;
preserve accurate remaining-byte accounting and queue only recoverable active
ranges.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e202a7b2-81c9-44de-8bcf-da65c18687da
📒 Files selected for processing (5)
internal/strategy/concurrent/concurrent_test.gointernal/strategy/concurrent/downloader.gointernal/strategy/concurrent/downloader_helpers_test.gointernal/strategy/concurrent/task.gointernal/strategy/concurrent/worker.go
💤 Files with no reviewable changes (1)
- internal/strategy/concurrent/concurrent_test.go
- Pushes remaining work of a failed active task back to the queue before deleting it from activeTasks, marking it terminal but preserving accurate byte accounting and queuing recoverable ranges.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/strategy/concurrent/worker.go (1)
169-182: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMake failed-task requeue and active-task removal atomic.
ActiveWorkersis decremented before the failed remainder is queued. The queue push also occurs befored.activeTasks[id]is deleted and outsided.activeMu.StealWorkandHedgeWorkcan observe the failed task between these operations and enqueue overlapping ranges. This can duplicate downloads and corrupt byte accounting.Coordinate the queue push, active-task removal, and worker-count update as one state transition. Holding
d.activeMuacross the requeue and deletion, or marking the task terminal and making all balancers skip it, prevents this race. Add a regression test that interleaves failure handling withStealWork.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/strategy/concurrent/worker.go` around lines 169 - 182, Make failed-task cleanup a single synchronized state transition in the worker failure path: coordinate the ActiveWorkers decrement, remaining-task requeue, and deletion from d.activeTasks under d.activeMu so StealWork and HedgeWork cannot observe an intermediate state or enqueue overlapping ranges. Preserve existing retry behavior, and add a regression test that interleaves failure handling with StealWork.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/strategy/concurrent/worker.go`:
- Around line 176-178: The requeue path in the worker drops SharedMaxOffset from
failed hedged task remainders, allowing duplicate work. Update
ActiveTask.RemainingTask to preserve and safely read SharedMaxOffset under
SharedMaxOffsetMu, ensure the requeued task retains that shared deduplication
state, and add a regression test covering hedged-task failure and requeue
behavior.
---
Outside diff comments:
In `@internal/strategy/concurrent/worker.go`:
- Around line 169-182: Make failed-task cleanup a single synchronized state
transition in the worker failure path: coordinate the ActiveWorkers decrement,
remaining-task requeue, and deletion from d.activeTasks under d.activeMu so
StealWork and HedgeWork cannot observe an intermediate state or enqueue
overlapping ranges. Preserve existing retry behavior, and add a regression test
that interleaves failure handling with StealWork.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: bcc21d41-d4f1-4bc2-9db0-cad9883fccc0
📒 Files selected for processing (1)
internal/strategy/concurrent/worker.go
…ueue - ActiveTask.RemainingTask now safely reads and preserves SharedMaxOffset under SharedMaxOffsetMu - Ensures requeued remainder tasks correctly retain their deduplication state
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/strategy/concurrent/task_test.go (1)
176-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd concurrency coverage for
SharedMaxOffsetinitialization.This test validates value propagation in one goroutine. It does not overlap
RemainingTask()with initialization, so it would pass even if the mutex protection were removed. Add a coordinated concurrent test and run it with the race detector.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/strategy/concurrent/task_test.go` around lines 176 - 204, Extend TestActiveTask_RemainingTask_PreservesSharedMaxOffset with a coordinated concurrent scenario that overlaps SharedMaxOffset initialization with RemainingTask(), using synchronization to control ordering and multiple goroutines as needed. Assert the resulting task preserves the initialized shared offset and run the test under the race detector to verify the mutex-protected initialization is race-free.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/strategy/concurrent/task.go`:
- Around line 58-65: Update the task construction flow to acquire
SharedMaxOffsetMu before calculating current, capture a single SharedMaxOffset
pointer snapshot, and pass that snapshot into at.currentOffset() (or its updated
offset-calculation signature). Reuse the same captured pointer for
types.Task.SharedMaxOffset so Offset and the returned task are based on one
synchronized pointer state.
---
Nitpick comments:
In `@internal/strategy/concurrent/task_test.go`:
- Around line 176-204: Extend
TestActiveTask_RemainingTask_PreservesSharedMaxOffset with a coordinated
concurrent scenario that overlaps SharedMaxOffset initialization with
RemainingTask(), using synchronization to control ordering and multiple
goroutines as needed. Assert the resulting task preserves the initialized shared
offset and run the test under the race detector to verify the mutex-protected
initialization is race-free.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d2eb459c-dc8a-48cc-a4f9-2c3811929950
📒 Files selected for processing (2)
internal/strategy/concurrent/task.gointernal/strategy/concurrent/task_test.go
- Acquires SharedMaxOffsetMu before calculating currentOffset - Captures a single SharedMaxOffset pointer snapshot to pass to currentOffset - Reuses the identical pointer snapshot for types.Task to guarantee synchronized pointer state between the offset calculation and the returned task structure
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/strategy/concurrent/task.go (1)
52-66: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the
SharedMaxOffsetcaptured byRemainingTask.
RemainingTaskalready guardsat.SharedMaxOffsetand returns the captured pointer intypes.Task. Lines 568-570 overwrite this withactive.SharedMaxOffsetbefore persisting, so snapshot deduplication can use a different pointer than the calculated remaining offset. Useremaining.SharedMaxOffsetdirectly for the snapshot deduplication map.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/strategy/concurrent/task.go` around lines 52 - 66, Update the snapshot deduplication logic to use the SharedMaxOffset captured in the task returned by RemainingTask, rather than replacing it with active.SharedMaxOffset before persistence. Locate the persistence path around the remaining-task handling and pass remaining.SharedMaxOffset directly to the deduplication map, preserving the calculated snapshot pointer.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@internal/strategy/concurrent/task.go`:
- Around line 52-66: Update the snapshot deduplication logic to use the
SharedMaxOffset captured in the task returned by RemainingTask, rather than
replacing it with active.SharedMaxOffset before persistence. Locate the
persistence path around the remaining-task handling and pass
remaining.SharedMaxOffset directly to the deduplication map, preserving the
calculated snapshot pointer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fa8f41a2-f08b-4cbf-804c-79fa87747271
📒 Files selected for processing (1)
internal/strategy/concurrent/task.go
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
1 participant
Summary by CodeRabbit
Bug Fixes
Tests