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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.6
parent
36dc6040ba
commit
d0885ba552
@@ -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<void> {
|
||||
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();
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -207,7 +207,10 @@ export class PostProcessor extends EventEmitter {
|
||||
private async runPostProcessing(packageId: string, options: PostProcessOptions): Promise<void> {
|
||||
// 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) => {
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -424,12 +424,14 @@ export async function streamToFile(opts: StreamOptions): Promise<StreamResult> {
|
||||
// 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<StreamResult> {
|
||||
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<StreamResult> {
|
||||
} 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 {}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user