fixup! Add stall detection to recover from frozen uploads

m7kvqbe1 · m7kvqbe1 · commit e6d0f8eca771 · 2025-05-28T12:18:46.000+01:00
diff --git a/lib/StallDetector.ts b/lib/StallDetector.ts
@@ -0,0 +1,133 @@
+import { log } from './logger.js'
+import type { StallDetectionOptions } from './options.js'
+import type { HttpStack } from './options.js'
+
+export class StallDetector {
+  private options: StallDetectionOptions
+  private httpStack: HttpStack
+  private onStallDetected: (reason: string) => void
+
+  private intervalId: ReturnType<typeof setInterval> | null = null
+  private lastProgressTime = 0
+  private isActive = false
+  private isPaused = false
+
+  constructor(
+    options: StallDetectionOptions,
+    httpStack: HttpStack,
+    onStallDetected: (reason: string) => void,
+  ) {
+    this.options = options
+    this.httpStack = httpStack
+    this.onStallDetected = onStallDetected
+  }
+
+  /**
+   * Start monitoring for stalls
+   */
+  start() {
+    if (this.intervalId) {
+      return // Already started
+    }
+
+    // Only enable stall detection if the HTTP stack supports progress events
+    if (!this._supportsProgressEvents()) {
+      log(
+        'Stall detection disabled: HTTP stack does not support progress events. Consider using chunkSize with appropriate timeouts instead.',
+      )
+      return
+    }
+
+    this.lastProgressTime = Date.now()
+    this.isActive = true
+
+    log(
+      `Starting stall detection with checkInterval: ${this.options.checkInterval}ms, stallTimeout: ${this.options.stallTimeout}ms`,
+    )
+
+    // Setup periodic check
+    this.intervalId = setInterval(() => {
+      if (!this.isActive || this.isPaused) {
+        return
+      }
+
+      const now = Date.now()
+      if (this._isProgressStalled(now)) {
+        this._handleStall('No progress events received')
+      }
+    }, this.options.checkInterval)
+  }
+
+  /**
+   * Stop monitoring for stalls
+   */
+  stop(): void {
+    this.isActive = false
+    if (this.intervalId) {
+      clearInterval(this.intervalId)
+      this.intervalId = null
+    }
+  }
+
+  /**
+   * Update progress information
+   */
+  updateProgress(): void {
+    this.lastProgressTime = Date.now()
+  }
+
+  /**
+   * Pause stall detection temporarily (e.g., during onBeforeRequest callback)
+   */
+  pause(): void {
+    this.isActive = false
+    this.isPaused = true
+  }
+
+  /**
+   * Resume stall detection after pause
+   */
+  resume(): void {
+    if (this.isPaused) {
+      this.isPaused = false
+      this.isActive = true
+      // Reset the last progress time to avoid false positives
+      this.lastProgressTime = Date.now()
+    }
+  }
+
+  /**
+   * Detect if current HttpStack supports progress events
+   */
+  private _supportsProgressEvents(): boolean {
+    // Check if the HTTP stack explicitly declares support for progress events
+    return (
+      typeof this.httpStack.supportsProgressEvents === 'function' &&
+      this.httpStack.supportsProgressEvents()
+    )
+  }
+
+  /**
+   * Check if upload has stalled based on progress events
+   */
+  private _isProgressStalled(now: number): boolean {
+    const timeSinceProgress = now - this.lastProgressTime
+    const stallTimeout = this.options.stallTimeout
+    const isStalled = timeSinceProgress > stallTimeout
+
+    if (isStalled) {
+      log(`No progress for ${timeSinceProgress}ms (limit: ${stallTimeout}ms)`)
+    }
+
+    return isStalled
+  }
+
+  /**
+   * Handle a detected stall
+   */
+  private _handleStall(reason: string): void {
+    log(`Upload stalled: ${reason}`)
+    this.stop()
+    this.onStallDetected(reason)
+  }
+}
diff --git a/lib/browser/FetchHttpStack.ts b/lib/browser/FetchHttpStack.ts
@@ -16,6 +16,11 @@ export class FetchHttpStack implements HttpStack {
   getName() {
     return 'FetchHttpStack'
   }
+
+  supportsProgressEvents(): boolean {
+    // The Fetch API does not support progress events for uploads
+    return false
+  }
 }
 
 class FetchRequest implements HttpRequest {
diff --git a/lib/browser/XHRHttpStack.ts b/lib/browser/XHRHttpStack.ts
@@ -15,6 +15,11 @@ export class XHRHttpStack implements HttpStack {
   getName() {
     return 'XHRHttpStack'
   }
+
+  supportsProgressEvents(): boolean {
+    // XMLHttpRequest supports progress events via the upload.onprogress event
+    return true
+  }
 }
 
 class XHRRequest implements HttpRequest {
diff --git a/lib/browser/index.ts b/lib/browser/index.ts
@@ -19,12 +19,32 @@ const defaultOptions = {
 
 class Upload extends BaseUpload {
   constructor(file: UploadInput, options: Partial<UploadOptions> = {}) {
-    const allOpts = { ...defaultOptions, ...options }
+    const allOpts = {
+      ...defaultOptions,
+      ...options,
+      // Deep merge stallDetection options if provided
+      ...(options.stallDetection && {
+        stallDetection: {
+          ...defaultOptions.stallDetection,
+          ...options.stallDetection,
+        },
+      }),
+    }
     super(file, allOpts)
   }
 
   static terminate(url: string, options: Partial<UploadOptions> = {}) {
-    const allOpts = { ...defaultOptions, ...options }
+    const allOpts = {
+      ...defaultOptions,
+      ...options,
+      // Deep merge stallDetection options if provided
+      ...(options.stallDetection && {
+        stallDetection: {
+          ...defaultOptions.stallDetection,
+          ...options.stallDetection,
+        },
+      }),
+    }
     return terminate(url, allOpts)
   }
 }
diff --git a/lib/node/NodeHttpStack.ts b/lib/node/NodeHttpStack.ts
@@ -28,6 +28,11 @@ export class NodeHttpStack implements HttpStack {
   getName() {
     return 'NodeHttpStack'
   }
+
+  supportsProgressEvents(): boolean {
+    // Node.js HTTP stack supports progress tracking through streams
+    return true
+  }
 }
 
 class Request implements HttpRequest {
diff --git a/lib/node/index.ts b/lib/node/index.ts
@@ -19,12 +19,32 @@ const defaultOptions = {
 
 class Upload extends BaseUpload {
   constructor(file: UploadInput, options: Partial<UploadOptions> = {}) {
-    const allOpts = { ...defaultOptions, ...options }
+    const allOpts = {
+      ...defaultOptions,
+      ...options,
+      // Deep merge stallDetection options if provided
+      ...(options.stallDetection && {
+        stallDetection: {
+          ...defaultOptions.stallDetection,
+          ...options.stallDetection,
+        },
+      }),
+    }
     super(file, allOpts)
   }
 
   static terminate(url: string, options: Partial<UploadOptions> = {}) {
-    const allOpts = { ...defaultOptions, ...options }
+    const allOpts = {
+      ...defaultOptions,
+      ...options,
+      // Deep merge stallDetection options if provided
+      ...(options.stallDetection && {
+        stallDetection: {
+          ...defaultOptions.stallDetection,
+          ...options.stallDetection,
+        },
+      }),
+    }
     return terminate(url, allOpts)
   }
 }
diff --git a/lib/options.ts b/lib/options.ts
@@ -53,49 +53,54 @@ export type UploadInput =
  */
 export interface StallDetectionOptions {
   enabled: boolean
-  stallTimeout: number      // Time in ms before considering progress stalled
-  checkInterval: number     // How often to check for stalls
-  minimumBytesPerSecond: number // For stacks without progress events
+  stallTimeout: number // Time in ms before considering progress stalled
+  checkInterval: number // How often to check for stalls
 }
 
-export interface UploadOptions {
-  endpoint?: string
-
-  uploadUrl?: string
-  metadata: { [key: string]: string }
-  metadataForPartialUploads: UploadOptions['metadata']
-  fingerprint: (file: UploadInput, options: UploadOptions) => Promise<string | null>
-  uploadSize?: number
+export type Part = { start: number; end: number }
 
-  onProgress?: (bytesSent: number, bytesTotal: number | null) => void
-  onChunkComplete?: (chunkSize: number, bytesAccepted: number, bytesTotal: number | null) => void
-  onSuccess?: (payload: OnSuccessPayload) => void
-  onError?: (error: Error | DetailedError) => void
-  onShouldRetry?: (error: DetailedError, retryAttempt: number, options: UploadOptions) => boolean
-  onUploadUrlAvailable?: () => void | Promise<void>
+export interface UploadOptions {
+  endpoint?: string | null
+  uploadUrl?: string | null
+  metadata: Record<string, string>
+  metadataForPartialUploads?: Record<string, string>
+  uploadSize?: number | null
+  onProgress: ((bytesSent: number, bytesTotal: number | null) => void) | null
+  onChunkComplete:
+    | ((chunkSize: number, bytesAccepted: number, bytesTotal: number | null) => void)
+    | null
+  onSuccess: ((payload: OnSuccessPayload) => void) | null
+  onError: ((error: Error | DetailedError) => void) | null
+  onUploadUrlAvailable: (() => void) | null
+  onShouldRetry:
+    | ((err: Error | DetailedError, retryAttempt: number, options: UploadOptions) => boolean)
+    | null
 
   overridePatchMethod: boolean
-  headers: { [key: string]: string }
+  headers: Record<string, string>
   addRequestId: boolean
-  onBeforeRequest?: (req: HttpRequest) => void | Promise<void>
-  onAfterResponse?: (req: HttpRequest, res: HttpResponse) => void | Promise<void>
+
+  onBeforeRequest: ((req: HttpRequest) => void | Promise<void>) | null
+  onAfterResponse: ((req: HttpRequest, res: HttpResponse) => void | Promise<void>) | null
 
   chunkSize: number
-  retryDelays: number[]
+  retryDelays: number[] | null
   parallelUploads: number
-  parallelUploadBoundaries?: { start: number; end: number }[]
+  parallelUploadBoundaries?: Part[] | null
   storeFingerprintForResuming: boolean
   removeFingerprintOnSuccess: boolean
   uploadLengthDeferred: boolean
   uploadDataDuringCreation: boolean
 
   urlStorage: UrlStorage
   fileReader: FileReader
+  fingerprint: (file: UploadInput, options: UploadOptions) => Promise<string | null>
+  // TODO: Types need to be double-checked
   httpStack: HttpStack
 
   protocol: typeof PROTOCOL_TUS_V1 | typeof PROTOCOL_IETF_DRAFT_03 | typeof PROTOCOL_IETF_DRAFT_05
 
-  stallDetection?: Partial<StallDetectionOptions>
+  stallDetection?: StallDetectionOptions
 }
 
 export interface OnSuccessPayload {
@@ -153,6 +158,9 @@ export type SliceResult =
 export interface HttpStack {
   createRequest(method: string, url: string): HttpRequest
   getName(): string
+  // Indicates whether this HTTP stack implementation supports progress events
+  // during upload. If false, stall detection will use overall transfer rate instead.
+  supportsProgressEvents?: () => boolean
 }
 
 export type HttpProgressHandler = (bytesSent: number) => void
diff --git a/lib/upload.ts b/lib/upload.ts
diff --git a/test/spec/helpers/utils.js b/test/spec/helpers/utils.js
diff --git a/test/spec/test-stall-detection.js b/test/spec/test-stall-detection.js

Original file line number	Diff line number	Diff line change
`@@ -16,6 +16,11 @@ export class FetchHttpStack implements HttpStack {`
`16`	`16`	`getName() {`
`17`	`17`	`return 'FetchHttpStack'`
`18`	`18`	`}`
	`19`	`+`
	`20`	`+ supportsProgressEvents(): boolean {`
	`21`	`+ // The Fetch API does not support progress events for uploads`
	`22`	`+ return false`
	`23`	`+ }`
`19`	`24`	`}`
`20`	`25`
`21`	`26`	`class FetchRequest implements HttpRequest {`
Original file line number	Diff line number	Diff line change
`@@ -15,6 +15,11 @@ export class XHRHttpStack implements HttpStack {`
`15`	`15`	`getName() {`
`16`	`16`	`return 'XHRHttpStack'`
`17`	`17`	`}`
	`18`	`+`
	`19`	`+ supportsProgressEvents(): boolean {`
	`20`	`+ // XMLHttpRequest supports progress events via the upload.onprogress event`
	`21`	`+ return true`
	`22`	`+ }`
`18`	`23`	`}`
`19`	`24`
`20`	`25`	`class XHRRequest implements HttpRequest {`
Original file line number	Diff line number	Diff line change
`@@ -28,6 +28,11 @@ export class NodeHttpStack implements HttpStack {`
`28`	`28`	`getName() {`
`29`	`29`	`return 'NodeHttpStack'`
`30`	`30`	`}`
	`31`	`+`
	`32`	`+ supportsProgressEvents(): boolean {`
	`33`	`+ // Node.js HTTP stack supports progress tracking through streams`
	`34`	`+ return true`
	`35`	`+ }`
`31`	`36`	`}`
`32`	`37`
`33`	`38`	`class Request implements HttpRequest {`