Track progress events instead of byte changes for stall detection

m7kvqbe1 · m7kvqbe1 · commit 8d18ffdd38cb · 2025-10-03T09:29:46.000+01:00
We can't get low level TCP socket transfer progress from the Node HTTP
module, you can only see an internal I/O buffer being filled, you then
get silence until that buffer is completely drained (transmitted) and
starts to be filled again, this means you consistently get false
positives for stall detection.
diff --git a/lib/StallDetector.ts b/lib/StallDetector.ts
@@ -7,7 +7,6 @@ export class StallDetector {
 
   private intervalId: ReturnType<typeof setInterval> | null = null
   private lastProgressTime = 0
-  private lastProgressValue = 0
   private isActive = false
 
   constructor(options: StallDetectionOptions, onStallDetected: (reason: string) => void) {
@@ -24,7 +23,6 @@ export class StallDetector {
     }
 
     this.lastProgressTime = Date.now()
-    this.lastProgressValue = 0
     this.isActive = true
 
     log(
@@ -57,14 +55,12 @@ export class StallDetector {
 
   /**
    * Update progress information
-   * @param progressValue The current progress value (bytes uploaded)
+   * @param _progressValue The current progress value (bytes uploaded) - currently unused but kept for future use
    */
-  updateProgress(progressValue: number): void {
-    // Only update progress time if the value has actually changed
-    if (progressValue !== this.lastProgressValue) {
-      this.lastProgressTime = Date.now()
-      this.lastProgressValue = progressValue
-    }
+  updateProgress(_progressValue: number): void {
+    // Only track that a progress event occurred, not the actual value
+    // This avoids false positives with NodeHttpStack's buffer behavior
+    this.lastProgressTime = Date.now()
   }
 
   /**
diff --git a/lib/upload.ts b/lib/upload.ts
@@ -119,8 +119,6 @@ export class BaseUpload {
   // upload options or HEAD response)
   private _uploadLengthDeferred: boolean
 
-
-
   constructor(file: UploadInput, options: UploadOptions) {
     // Warn about removed options from previous versions
     if ('resume' in options) {
@@ -288,7 +286,7 @@ export class BaseUpload {
    *
    * @api private
    */
-    private async _startParallelUpload(): Promise<void> {
+  private async _startParallelUpload(): Promise<void> {
     const totalSize = this._size
     let totalProgress = 0
     this._parallelUploads = []
@@ -379,8 +377,6 @@ export class BaseUpload {
         // @ts-expect-error `value` is unknown and not an UploadInput
         const upload = new BaseUpload(value, options)
 
-
-
         upload.start()
 
         // Store the upload in an array, so we can later abort them if necessary.
@@ -778,8 +774,6 @@ export class BaseUpload {
       await this.options.onUploadUrlAvailable()
     }
 
-
-
     await this._saveUploadInUrlStorage()
 
     // Upload has already been completed and we do not need to send additional
@@ -876,11 +870,10 @@ export class BaseUpload {
           }
           // Don't call _retryOrEmitError here - let the natural error flow handle it
         })
-      } else {
-        log(
-          'tus: stall detection is enabled but the HTTP stack does not support progress events, it will be disabled for this upload',
-        )
       }
+      log(
+        'tus: stall detection is enabled but the HTTP stack does not support progress events, it will be disabled for this upload',
+      )
     }
     return undefined
   }
diff --git a/test/spec/test-parallel-uploads.js b/test/spec/test-parallel-uploads.js
@@ -658,7 +658,7 @@ describe('tus', () => {
       // Track all requests to detect if fresh uploads are created
       const allRequests = []
       const originalCreateRequest = testStack.createRequest.bind(testStack)
-      testStack.createRequest = function(method, url) {
+      testStack.createRequest = (method, url) => {
         allRequests.push({ method, url })
         return originalCreateRequest(method, url)
       }
@@ -773,15 +773,13 @@ describe('tus', () => {
       await options.onSuccess.toBeCalled()
 
       // Final verification: count how many POST requests to /uploads we made
-      const postToUploads = allRequests.filter(r =>
-        r.method === 'POST' && r.url === 'https://tus.io/uploads'
+      const postToUploads = allRequests.filter(
+        (r) => r.method === 'POST' && r.url === 'https://tus.io/uploads',
       )
 
       // Should only be 3 POSTs: 2 partial + 1 final concatenation
       // If the bug exists, we'd see 4 POSTs (an extra one from the retry)
       expect(postToUploads.length).toBe(3)
     })
-
-
   })
 })
diff --git a/test/spec/test-stall-detection.js b/test/spec/test-stall-detection.js
@@ -70,7 +70,7 @@ class StallTestHttpStack extends TestHttpStack {
     return req
   }
 
-  _setupMethodStall(req, method) {
+  _setupMethodStall(req, _method) {
     const originalAbort = req.abort.bind(req)
 
     req.send = async function (body) {
@@ -92,7 +92,7 @@ class StallTestHttpStack extends TestHttpStack {
       return this._requestPromise
     }
 
-    req.abort = function() {
+    req.abort = function () {
       if (this._rejectRequest) {
         this._rejectRequest(new Error('request aborted'))
       }
@@ -234,7 +234,7 @@ async function testStallDetectionForMethod(method, uploadOptions = {}) {
   const originalLog = console.log
   let loggedMessage = ''
   console.log = (message) => {
-    loggedMessage += message + '\n'
+    loggedMessage += `${message}\n`
   }
 
   upload.start()
@@ -444,8 +444,10 @@ describe('tus-stall-detection', () => {
       expect(options.onProgress.calls.count()).toBeGreaterThan(0)
     })
 
-    it('should detect stalls when progress value does not change', async () => {
+    it('should NOT detect stalls when progress value does not change but events are still fired', async () => {
+      const file = getBlob('hello world')
       const { upload, options, testStack } = createTestUpload({
+        file,
         stallDetection: {
           enabled: true,
           checkInterval: 50,
@@ -454,14 +456,18 @@ describe('tus-stall-detection', () => {
         retryDelays: null,
       })
 
-      // Create a progress sequence that gets stuck at 300 bytes
+      // Create a progress sequence that gets stuck at 5 bytes
+      // but still fires progress events (simulating NodeHttpStack buffer behavior)
       const progressSequence = [
         { bytes: 0, delay: 10 },
-        { bytes: 100, delay: 10 },
-        { bytes: 200, delay: 10 },
-        { bytes: 300, delay: 10 },
-        // Repeat the same value to trigger value-based stall detection
-        ...Array(12).fill({ bytes: 300, delay: 30 }),
+        { bytes: 2, delay: 10 },
+        { bytes: 5, delay: 10 },
+        // Repeat the same value - with the new behavior, this should NOT trigger stall detection
+        // as long as progress events are still being fired
+        ...Array(12).fill({ bytes: 5, delay: 30 }),
+        // Eventually progress continues
+        { bytes: 8, delay: 10 },
+        { bytes: 11, delay: 10 },
       ]
 
       testStack.setNextProgressSequence(progressSequence)
@@ -472,8 +478,15 @@ describe('tus-stall-detection', () => {
       const patchReq = await testStack.nextRequest()
       expect(patchReq.method).toBe('PATCH')
 
-      const error = await options.onError.toBeCalled()
-      expect(error.message).toContain('stalled: no progress')
+      // Complete the upload successfully
+      patchReq.respondWith({
+        status: 204,
+        responseHeaders: { 'Upload-Offset': '11' },
+      })
+
+      // The upload should complete successfully without stall detection
+      await options.onSuccess.toBeCalled()
+      expect(options.onError.calls.count()).toBe(0)
       expect(options.onProgress.calls.count()).toBeGreaterThan(0)
     })
 
