Conversation
- add connection health sampling for direct and ranged downloads - abort and restart stalled chunk requests after sustained slowdown - clean up streams and ignore refresh-triggered aborts
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughA connection health monitor is introduced that samples byte-rate over a sliding window, computes baselines from positive samples, and detects sustained slowdowns or near-zero transfer patterns. When degradation is detected, the monitor triggers a refresh that aborts active ranged requests and causes download retries via a new sentinel error. Progress updates feed byte totals to the monitor, and resource cleanup is enhanced for chunk abort handling. Changes
Sequence DiagramsequenceDiagram
participant Download as Download Process
participant Monitor as Connection Health Monitor
participant RangedReqs as Ranged Requests
participant FileStream as File Stream
participant RetryLogic as Retry Control
Download->>Monitor: Feed progress updates (byte totals)
Monitor->>Monitor: Sample byte-rate over sliding window
Monitor->>Monitor: Compute baseline from positive samples
loop Detection Check
Monitor->>Monitor: Detect sustained slowdown or near-zero behavior
end
alt Slowdown Detected
Monitor->>Monitor: Check online state & enforce persistence/cooldown
Monitor->>RetryLogic: Trigger refresh (CONNECTION_REFRESH_REQUESTED)
RetryLogic->>RangedReqs: Abort active ranged requests
RangedReqs->>FileStream: Close streams & clear references
RetryLogic->>Download: Retry download immediately
else Healthy Connection
Download->>Download: Continue normally
end
Download->>Monitor: Dispose on completion/abort/error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 review |
✅ Actions performedReview triggered.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae83daf641
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (lastError.message === 'CONNECTION_REFRESH_REQUESTED') { | ||
| continue; |
There was a problem hiding this comment.
When executePartDownload requests a reconnect, it aborts part.abortController; that signal stays permanently aborted. In this retry loop, the new CONNECTION_REFRESH_REQUESTED branch immediately continues without replacing part.abortController, so the next axios.get(..., signal: part.abortController.signal) in executePartDownload is already canceled and subsequent attempts fail quickly. This causes multi-part standard downloads to fail instead of recovering whenever the throughput-collapse reconnect path is triggered.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
application/src/electron/handlers/handler.ddl.ts (1)
765-802:⚠️ Potential issue | 🟡 MinorChunked refresh path swallows the
CONNECTION_REFRESH_REQUESTEDsignal.For the chunked flows (
executeParallelDownloadForParthere, andexecuteParallelDownloadat Lines 1840-1857),onReconnectsetsconnectionRefreshRequested = trueand aborts every chunk's controller — but the chunk abort handlers at Lines 896-905 and Lines 1995-2005 reject withnew Error('Aborted')regardless of why the abort happened. The localconnectionRefreshRequestedflag set here is never read, so:
executeParallelDownloadForPartre-throws'Aborted'fromPromise.all.downloadPartWithState's chunk-mode retry loop (Lines 574-615) has no'CONNECTION_REFRESH_REQUESTED'branch like the standard path at Line 645, so it falls through toawait new Promise(resolve => setTimeout(resolve, 1000 * (i + 1))).- Same applies to
executeParallelDownload→downloadPartretry loop (Lines 1379-1424 vs. the check at Line 1439).Net effect: chunked refreshes pay an extra 1‑5s exponential backoff per retry on top of the 15s
CONNECTION_HEALTH_RECONNECT_COOLDOWN_MS, and the "dedicated reconnect signal" the PR advertises only applies to the non-chunked paths. Either thread the refresh signal down to the chunk rejection or detect it at thePromise.allboundary before re-throwing.🔧 Sketch of one possible fix (chunked-part path)
Tag each chunk so its abort handler can emit the right error, and mirror the standard-path check in the retry loop:
interface ChunkState { index: number; ... abortController: AbortController; + refreshRequested?: boolean; fileStream?: fs.WriteStream; ... }onReconnect: ({ currentSpeed, baselineSpeed }) => { if (connectionRefreshRequested || this.status !== 'downloading') return; connectionRefreshRequested = true; ... for (const activeChunk of part.chunks) { + activeChunk.refreshRequested = true; activeChunk.abortController.abort(); } },chunk.abortController.signal.addEventListener('abort', () => { ... - reject(new Error('Aborted')); + reject( + new Error( + chunk.refreshRequested ? 'CONNECTION_REFRESH_REQUESTED' : 'Aborted' + ) + ); });} catch (error) { lastError = error as Error; ... if (this.status !== 'downloading') throw lastError; + if (lastError.message === 'CONNECTION_REFRESH_REQUESTED') { + continue; + } await new Promise((resolve) => setTimeout(resolve, 1000 * (i + 1))); }Apply symmetrically in
downloadChunk/executeParallelDownload/downloadPart's retry loop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@application/src/electron/handlers/handler.ddl.ts` around lines 765 - 802, The parallel-chunk flow swallows the CONNECTION_REFRESH_REQUESTED signal because onReconnect sets connectionRefreshRequested and aborts chunk controllers but chunk abort handlers always reject new Error('Aborted'); change the abort path so aborted chunks can indicate a refresh: when onReconnect runs set a flag on the part/chunk (e.g., part.connectionRefreshRequested or chunk.connectionRefreshRequested) and have downloadChunk / downloadChunkForPart's abort handlers reject a distinct Error('CONNECTION_REFRESH_REQUESTED') (or include a property on the Error) when that flag is set; update the Promise.all boundary in executeParallelDownloadForPart to detect and rethrow CONNECTION_REFRESH_REQUESTED (instead of the generic 'Aborted') and mirror the same check in the retry loops in downloadPartWithState and downloadPart so they handle CONNECTION_REFRESH_REQUESTED the same way as the non-chunked path (avoid falling through to the exponential backoff).
🧹 Nitpick comments (1)
application/src/electron/handlers/handler.ddl.ts (1)
782-782: Redundant initialobserve()calls.
createConnectionHealthMonitoralready setsobservedBytes = options.initialBytesat Line 216, so callingconnectionHealth.observe(part.downloadedBytes)immediately after creation with the same value is a no-op. Minor, but worth dropping for clarity. These same four sites each have an identical redundant call.Also applies to: 1066-1066, 1544-1544, 1857-1857
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@application/src/electron/handlers/handler.ddl.ts` at line 782, The call to connectionHealth.observe(part.downloadedBytes) is redundant because createConnectionHealthMonitor already initializes observedBytes to options.initialBytes; remove these immediate post-construction observe calls at each site (the one shown and the three other occurrences) to avoid no-op duplication. Locate where createConnectionHealthMonitor is invoked and drop the immediately-following connectionHealth.observe(...) lines (references: createConnectionHealthMonitor, connectionHealth.observe, options.initialBytes, part.downloadedBytes) so the monitor relies on its internal initialBytes initialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@application/src/electron/handlers/handler.ddl.ts`:
- Around line 765-802: The parallel-chunk flow swallows the
CONNECTION_REFRESH_REQUESTED signal because onReconnect sets
connectionRefreshRequested and aborts chunk controllers but chunk abort handlers
always reject new Error('Aborted'); change the abort path so aborted chunks can
indicate a refresh: when onReconnect runs set a flag on the part/chunk (e.g.,
part.connectionRefreshRequested or chunk.connectionRefreshRequested) and have
downloadChunk / downloadChunkForPart's abort handlers reject a distinct
Error('CONNECTION_REFRESH_REQUESTED') (or include a property on the Error) when
that flag is set; update the Promise.all boundary in
executeParallelDownloadForPart to detect and rethrow
CONNECTION_REFRESH_REQUESTED (instead of the generic 'Aborted') and mirror the
same check in the retry loops in downloadPartWithState and downloadPart so they
handle CONNECTION_REFRESH_REQUESTED the same way as the non-chunked path (avoid
falling through to the exponential backoff).
---
Nitpick comments:
In `@application/src/electron/handlers/handler.ddl.ts`:
- Line 782: The call to connectionHealth.observe(part.downloadedBytes) is
redundant because createConnectionHealthMonitor already initializes
observedBytes to options.initialBytes; remove these immediate post-construction
observe calls at each site (the one shown and the three other occurrences) to
avoid no-op duplication. Locate where createConnectionHealthMonitor is invoked
and drop the immediately-following connectionHealth.observe(...) lines
(references: createConnectionHealthMonitor, connectionHealth.observe,
options.initialBytes, part.downloadedBytes) so the monitor relies on its
internal initialBytes initialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a15c96b1-062d-4f20-b8ad-0d6f84d608e7
📒 Files selected for processing (1)
application/src/electron/handlers/handler.ddl.ts
- Recreate the abort controller when a connection refresh is requested - Remove redundant connection health observations from download paths
1 participant
Summary
Testing
application/src/electron/handlers/handler.ddl.ts.Summary by CodeRabbit
Bug Fixes