From d0885ba552a1365e01162a70b435f9eebeea9c23 Mon Sep 17 00:00:00 2001 From: Sucukdeluxe Date: Sun, 8 Mar 2026 18:33:06 +0100 Subject: [PATCH] Fix 16 bugs found by code review across all download modules Critical fixes: - Post-processor: remove double attempts increment (onProgress + onArchiveFailure both counted) - Post-processor: fix slot leak when signal aborted after acquireSlot - Scheduler: reset global watchdog high-water mark after stall event (prevents permanent misfires) - Pipeline/DM: fix isPathInsideDir path traversal (add trailing separator check) - Retry-manager: check per-kind exhaustion before shelve threshold (prevents bypass) - Retry-manager: add MAX_SHELVE_COUNT=5 cap to prevent infinite shelve cycling Important fixes: - Scheduler: clear retryDelays and providerCooldowns on start() - Scheduler: skip already-aborting slots in stall detection - Download-manager: fix cleanupAfterExtraction using extractDir instead of outputDir for link removal - Download-manager: add "extracting" to package normalizeSessionStatuses - Download-manager: clear activeTasks map on stop() - Download-manager: remove useless cachedDirectUrls re-insertion after success - Stream-writer: remove duplicate truncation code in error path - Stream-writer: skip alignedFlush in finally when bodyError already set (avoids 5min drain wait) - Stream-writer: re-read elapsed after speed limiter sleep for accurate window reset - Error-classifier: add HTTP 401 (Forbidden) and 410 (NotFound) classification Tests updated to match new shelve/kind-exhaustion priority and 401 classification. All 216 tests pass, build verified. Co-Authored-By: Claude Opus 4.6 --- src/main/download/download-manager.ts | 21 ++++----- src/main/download/error-classifier.ts | 10 +++++ src/main/download/pipeline.ts | 6 ++- src/main/download/post-processor.ts | 13 +++--- src/main/download/retry-manager.ts | 22 +++++++--- src/main/download/scheduler.ts | 8 +++- src/main/download/stream-writer.ts | 17 ++++---- tests/error-classifier.test.ts | 4 +- tests/retry-manager.test.ts | 63 ++++++++++++++++++++------- 9 files changed, 110 insertions(+), 54 deletions(-) diff --git a/src/main/download/download-manager.ts b/src/main/download/download-manager.ts index 37ef952..8610b31 100644 --- a/src/main/download/download-manager.ts +++ b/src/main/download/download-manager.ts @@ -88,7 +88,10 @@ function pathKey(p: string): string { } function isPathInsideDir(filePath: string, dirPath: string): boolean { - return path.resolve(filePath).toLowerCase().startsWith(path.resolve(dirPath).toLowerCase()); + const normalizedFile = path.resolve(filePath).toLowerCase(); + let normalizedDir = path.resolve(dirPath).toLowerCase(); + if (!normalizedDir.endsWith(path.sep)) normalizedDir += path.sep; + return normalizedFile.startsWith(normalizedDir) || normalizedFile === normalizedDir.slice(0, -1); } function providerLabel(p: string | null): string { @@ -752,6 +755,7 @@ export class DownloadManager extends EventEmitter { slot.abortReason = "stop"; slot.abortController.abort("stop"); } + this.activeTasks.clear(); // Reset non-finished items for (const item of Object.values(this.session.items)) { @@ -1069,16 +1073,6 @@ export class DownloadManager extends EventEmitter { this.cachedDirectUrls.delete(slot.itemId); this.retryManager.removeItem(slot.itemId); - // Cache the direct URL in case another item from same package needs it - if (result.directUrl) { - this.cachedDirectUrls.set(slot.itemId, { - url: result.directUrl, - provider: result.provider, - label: result.providerLabel || "", - skipTls: result.skipTlsVerify || false, - }); - } - logger.info(`Download complete: ${item.fileName} (${humanSize(item.downloadedBytes)})`); // Check if package is done → trigger post-processing @@ -1331,7 +1325,8 @@ export class DownloadManager extends EventEmitter { private async cleanupAfterExtraction(pkg: PackageEntry): Promise { if (this.settings.removeLinkFilesAfterExtract) { - await removeDownloadLinkArtifacts(pkg.extractDir); + // Link artifacts (.lnk files) are in the download directory, not extract dir + await removeDownloadLinkArtifacts(pkg.outputDir); } if (this.settings.removeSamplesAfterExtract) { await removeSampleArtifacts(pkg.extractDir); @@ -1594,7 +1589,7 @@ export class DownloadManager extends EventEmitter { } } for (const pkg of Object.values(this.session.packages)) { - if (pkg.status === "downloading" || pkg.status === "validating") { + if (pkg.status === "downloading" || pkg.status === "validating" || pkg.status === "extracting") { pkg.status = "queued"; pkg.updatedAt = nowMs(); } diff --git a/src/main/download/error-classifier.ts b/src/main/download/error-classifier.ts index f5e1a23..6c2fa38 100644 --- a/src/main/download/error-classifier.ts +++ b/src/main/download/error-classifier.ts @@ -241,6 +241,11 @@ export function classifyHttpStatus(ctx: HttpClassifyContext): DownloadError { httpStatus: status, }); + case status === 401: + return new DownloadError(DownloadErrorKind.Forbidden, msg, { + httpStatus: status, + }); + case status === 403: return new DownloadError(DownloadErrorKind.Forbidden, msg, { httpStatus: status, @@ -251,6 +256,11 @@ export function classifyHttpStatus(ctx: HttpClassifyContext): DownloadError { httpStatus: status, }); + case status === 410: + return new DownloadError(DownloadErrorKind.NotFound, msg, { + httpStatus: status, + }); + case status >= 500: return new DownloadError(DownloadErrorKind.ServerError, msg, { httpStatus: status, diff --git a/src/main/download/pipeline.ts b/src/main/download/pipeline.ts index f8f6c48..9b6e3dc 100644 --- a/src/main/download/pipeline.ts +++ b/src/main/download/pipeline.ts @@ -292,8 +292,10 @@ function filenameFromUrl(url: string): string { function isPathInsideDir(filePath: string, dirPath: string): boolean { const normalizedFile = path.resolve(filePath).toLowerCase(); - const normalizedDir = path.resolve(dirPath).toLowerCase(); - return normalizedFile.startsWith(normalizedDir); + let normalizedDir = path.resolve(dirPath).toLowerCase(); + // Ensure trailing separator to prevent "C:\downloads\pack-evil" matching "C:\downloads\pack" + if (!normalizedDir.endsWith(path.sep)) normalizedDir += path.sep; + return normalizedFile.startsWith(normalizedDir) || normalizedFile === normalizedDir.slice(0, -1); } function providerDisplayName(provider: DebridProvider | null): string { diff --git a/src/main/download/post-processor.ts b/src/main/download/post-processor.ts index 3ebc8f0..ee88d4d 100644 --- a/src/main/download/post-processor.ts +++ b/src/main/download/post-processor.ts @@ -207,7 +207,10 @@ export class PostProcessor extends EventEmitter { private async runPostProcessing(packageId: string, options: PostProcessOptions): Promise { // Acquire slot await this.acquireSlot(options.signal); - if (options.signal.aborted) return; + if (options.signal.aborted) { + this.releaseSlot(); + return; + } const state: PackagePostProcessState = this.states.get(packageId) || { packageId, @@ -335,13 +338,11 @@ export class PostProcessor extends EventEmitter { // Track individual archive completion if (update.archiveDone) { const archiveState = state.archives.get(update.archiveName); - if (archiveState) { + if (archiveState && update.archiveSuccess) { archiveState.attempts++; - if (update.archiveSuccess) { - archiveState.status = "done"; - } - // If not success, onArchiveFailure will handle it + archiveState.status = "done"; } + // If not success, onArchiveFailure will handle it (and increment attempts) } }, onArchiveFailure: (failure: ExtractArchiveFailure) => { diff --git a/src/main/download/retry-manager.ts b/src/main/download/retry-manager.ts index 498c0b7..0f9f776 100644 --- a/src/main/download/retry-manager.ts +++ b/src/main/download/retry-manager.ts @@ -199,6 +199,7 @@ export interface RetryDecision { const SHELVE_THRESHOLD = 15; const SHELVE_DELAY_MS = 90_000; +const MAX_SHELVE_COUNT = 5; // --------------------------------------------------------------------------- // RetryManager @@ -248,12 +249,8 @@ export class RetryManager { ? Math.min(policy.maxRetries, this.userRetryLimit) : policy.maxRetries; - // Check shelving threshold BEFORE individual kind limits - if (state.totalFailures >= SHELVE_THRESHOLD) { - return this.shelve(state, kind); - } - - // Check if this specific kind exhausted its retries + // Check if this specific kind exhausted its retries FIRST + // (shelving must not bypass per-kind limits) if (kindCount > effectiveMax) { return { shouldRetry: false, @@ -263,6 +260,19 @@ export class RetryManager { }; } + // Shelving threshold: too many total failures across all kinds + if (state.totalFailures >= SHELVE_THRESHOLD) { + if (state.shelveCount >= MAX_SHELVE_COUNT) { + return { + shouldRetry: false, + delayMs: 0, + actions: [], + reason: `Maximale Shelve-Zyklen erreicht (${MAX_SHELVE_COUNT})`, + }; + } + return this.shelve(state, kind); + } + // Retry — compute delay and actions const delayMs = this.computeDelay(policy, kindCount); const actions = this.computeActions(policy); diff --git a/src/main/download/scheduler.ts b/src/main/download/scheduler.ts index 7ba376d..dc6923e 100644 --- a/src/main/download/scheduler.ts +++ b/src/main/download/scheduler.ts @@ -96,6 +96,8 @@ export class Scheduler extends EventEmitter { this.scopedPackageIds = new Set(scopedIds || []); this.lastGlobalProgressBytes = 0; this.lastGlobalProgressAt = Date.now(); + this.retryDelays.clear(); + this.providerCooldowns.clear(); const myGeneration = this.generation; const loopIntervalMs = 120; @@ -430,6 +432,7 @@ export class Scheduler extends EventEmitter { for (const slot of this.slots.values()) { if (slot.blockedOnDiskWrite) continue; // Don't count disk waits + if (slot.abortReason !== "none") continue; // Already aborting const idleMs = now - slot.lastHeartbeatAt; if (idleMs > this.config.stallTimeoutMs) { this.emit("stall-detected", { itemId: slot.itemId, idleMs }); @@ -460,7 +463,10 @@ export class Scheduler extends EventEmitter { .filter(s => !s.blockedOnDiskWrite) .map(s => s.itemId); this.emit("global-stall", { itemIds: stalledIds }); - this.lastGlobalProgressAt = now; // Reset to avoid rapid-fire events + // Reset both timestamp and high-water mark so after retry + // (where bytesAtHeartbeat resets to 0) the watchdog doesn't misfire + this.lastGlobalProgressAt = now; + this.lastGlobalProgressBytes = totalBytes; } } diff --git a/src/main/download/stream-writer.ts b/src/main/download/stream-writer.ts index 23838b5..9d30bfa 100644 --- a/src/main/download/stream-writer.ts +++ b/src/main/download/stream-writer.ts @@ -424,12 +424,14 @@ export async function streamToFile(opts: StreamOptions): Promise { // Speed limiting if (speedLimitBps > 0) { speedLimitWindowBytes += buffer.length; - const elapsed = (nowMs() - speedLimitWindowStart) / 1000; + let elapsed = (nowMs() - speedLimitWindowStart) / 1000; if (elapsed > 0.1) { const currentRate = speedLimitWindowBytes / elapsed; if (currentRate > speedLimitBps) { const sleepMs = Math.floor(((speedLimitWindowBytes / speedLimitBps) - elapsed) * 1000); if (sleepMs > 10) await sleep(Math.min(sleepMs, 1000)); + // Re-read elapsed after sleep so window reset check is accurate + elapsed = (nowMs() - speedLimitWindowStart) / 1000; } if (elapsed >= 1) { speedLimitWindowStart = nowMs(); @@ -508,8 +510,10 @@ export async function streamToFile(opts: StreamOptions): Promise { bodyError = error; log("WARN", "Download body error", { attempt, error: errorMessage(error) }); } finally { - // Flush remaining buffered data - try { await alignedFlush(true); } catch (e) { if (!bodyError) bodyError = e; } + // Flush remaining buffered data (skip if error already — avoid 5min drain wait) + if (!bodyError) { + try { await alignedFlush(true); } catch (e) { bodyError = e; } + } // Close stream try { @@ -538,13 +542,8 @@ export async function streamToFile(opts: StreamOptions): Promise { } catch { /* best-effort */ } } - // Truncate pre-allocated file to actual written bytes on error - if (bodyError && preAllocated && totalBytes && written < totalBytes) { - try { await fs.promises.truncate(effectiveTargetPath, written); } catch {} - } - if (bodyError) { - // On error: truncate pre-allocated sparse file + // On error: truncate pre-allocated sparse file to actual written bytes if (preAllocated && totalBytes && written < totalBytes) { try { await fs.promises.truncate(effectiveTargetPath, written); } catch {} } diff --git a/tests/error-classifier.test.ts b/tests/error-classifier.test.ts index a76b113..199bacb 100644 --- a/tests/error-classifier.test.ts +++ b/tests/error-classifier.test.ts @@ -284,9 +284,9 @@ describe("classifyHttpStatus", () => { expect(err.httpStatus).toBe(503); }); - it("classifies 401 as Unknown (no special branch)", () => { + it("classifies 401 as Forbidden", () => { const err = classifyHttpStatus({ status: 401 }); - expect(err.kind).toBe(DownloadErrorKind.Unknown); + expect(err.kind).toBe(DownloadErrorKind.Forbidden); expect(err.httpStatus).toBe(401); }); diff --git a/tests/retry-manager.test.ts b/tests/retry-manager.test.ts index b13ab70..8252fff 100644 --- a/tests/retry-manager.test.ts +++ b/tests/retry-manager.test.ts @@ -441,18 +441,26 @@ describe("shelving", () => { it("shelving increments shelveCount", () => { const mgr = new RetryManager(); - // Trigger shelve twice - // First round: 15 failures -> shelve (halves to ~7) + // Use mixed kinds to avoid per-kind exhaustion before shelve threshold + // Timeout(10), RateLimited(8), ProviderBusy(8), ServerError(5), Unknown(5) + const kinds = [ + DownloadErrorKind.Timeout, + DownloadErrorKind.RateLimited, + DownloadErrorKind.ProviderBusy, + DownloadErrorKind.ServerError, + DownloadErrorKind.Unknown, + ]; + // First round: 15 failures -> shelve (3 per kind, all within limits) for (let i = 0; i < 15; i++) { - mgr.evaluate("a", mkError(DownloadErrorKind.Unknown)); + mgr.evaluate("a", mkError(kinds[i % kinds.length])); } const state1 = mgr.getState("a")!; expect(state1.shelveCount).toBe(1); - // After halving, totalFailures is ~7. Need 8 more to reach 15 again. + // After halving, totalFailures is ~7. Need more to reach 15 again. const remaining = SHELVE_THRESHOLD - state1.totalFailures; for (let i = 0; i < remaining; i++) { - mgr.evaluate("a", mkError(DownloadErrorKind.Unknown)); + mgr.evaluate("a", mkError(kinds[i % kinds.length])); } const state2 = mgr.getState("a")!; expect(state2.shelveCount).toBe(2); @@ -460,27 +468,45 @@ describe("shelving", () => { it("shelve decision always has shouldRetry=true", () => { const mgr = new RetryManager(); + // Use mixed kinds to reach shelve threshold + const kinds = [ + DownloadErrorKind.Timeout, + DownloadErrorKind.RateLimited, + DownloadErrorKind.ProviderBusy, + DownloadErrorKind.ServerError, + DownloadErrorKind.Unknown, + ]; for (let i = 0; i < 15; i++) { - mgr.evaluate("a", mkError(DownloadErrorKind.Unknown)); + mgr.evaluate("a", mkError(kinds[i % kinds.length])); } // The 15th call itself triggers shelve - // Let's re-check: the state now has halved counters. // One more batch to trigger shelve again const state = mgr.getState("a")!; const needed = SHELVE_THRESHOLD - state.totalFailures; for (let i = 0; i < needed - 1; i++) { - mgr.evaluate("a", mkError(DownloadErrorKind.Unknown)); + mgr.evaluate("a", mkError(kinds[i % kinds.length])); } - const d = mgr.evaluate("a", mkError(DownloadErrorKind.Unknown)); + const d = mgr.evaluate("a", mkError(kinds[0])); expect(d.shouldRetry).toBe(true); expect(d.delayMs).toBe(SHELVE_DELAY_MS); }); - it("shelve is checked before per-kind exhaustion", () => { + it("per-kind exhaustion is checked before shelve", () => { const mgr = new RetryManager(); - // NetworkReset has maxRetries=3. If we mix kinds to reach 15 total - // without exhausting any single kind, shelve takes priority. - // Use 5 kinds, 3 each = 15 + // NetworkReset has maxRetries=3. If we use only NetworkReset errors, + // kind exhaustion hits at 4 (before shelve at 15). + for (let i = 0; i < 3; i++) { + mgr.evaluate("a", mkError(DownloadErrorKind.NetworkReset)); + } + // 4th NetworkReset -> kind exhausted (maxRetries=3) + const d = mgr.evaluate("a", mkError(DownloadErrorKind.NetworkReset)); + expect(d.shouldRetry).toBe(false); + expect(d.reason).toContain("erschöpft"); + }); + + it("shelve triggers when mixed kinds reach threshold without any kind exhausted", () => { + const mgr = new RetryManager(); + // Use 5 kinds with high maxRetries so no single kind is exhausted const kinds = [ DownloadErrorKind.Timeout, DownloadErrorKind.ServerError, @@ -728,9 +754,16 @@ describe("exportStates() and importStates()", () => { it("shelveCount survives export/import roundtrip", () => { const mgr = new RetryManager(); - // Trigger shelve + // Trigger shelve using mixed kinds to avoid per-kind exhaustion + const kinds = [ + DownloadErrorKind.Timeout, + DownloadErrorKind.RateLimited, + DownloadErrorKind.ProviderBusy, + DownloadErrorKind.ServerError, + DownloadErrorKind.Unknown, + ]; for (let i = 0; i < 15; i++) { - mgr.evaluate("a", mkError(DownloadErrorKind.Unknown)); + mgr.evaluate("a", mkError(kinds[i % kinds.length])); } const originalShelve = mgr.getState("a")!.shelveCount; expect(originalShelve).toBeGreaterThan(0);