From 875c31d69c4d1737581564c493e2ce030ca34dec Mon Sep 17 00:00:00 2001 From: m7kvqbe1 <48086589+m7kvqbe1@users.noreply.github.com> Date: Tue, 20 May 2025 11:21:12 +0100 Subject: [PATCH 01/12] Add stall detection to recover from frozen uploads This feature addresses the issue of uploads hanging indefinitely in unreliable network conditions, particularly in Node.js environments where no default timeout exists. When uploads stall due to network issues, TCP connections can enter a degraded state where no data is transferred but no error is triggered. This implementation detects such stalls and forces a retry. Implementation details: - Progress-based: Detects when no upload progress events are fired - Gracefully integrates with the existing retry mechanism - Fully configurable with sensible defaults: - 30s stall timeout (time with no progress before considering stalled) - 5s check interval (how often to check for stalls) This is especially important for uploads over satellite links, cellular networks, or other unreliable connections where TCP backoff can cause indefinite stalls. --- docs/api.md | 37 +++ lib/StallDetector.ts | 93 +++++++ lib/browser/FetchHttpStack.ts | 5 + lib/browser/XHRHttpStack.ts | 5 + lib/browser/index.ts | 24 +- lib/node/NodeHttpStack.ts | 5 + lib/node/index.ts | 24 +- lib/options.ts | 15 ++ lib/upload.ts | 93 +++++-- test/spec/browser-index.js | 1 + test/spec/helpers/utils.js | 5 + test/spec/node-index.js | 1 + test/spec/test-stall-detection.js | 412 ++++++++++++++++++++++++++++++ 13 files changed, 700 insertions(+), 20 deletions(-) create mode 100644 lib/StallDetector.ts create mode 100644 test/spec/test-stall-detection.js diff --git a/docs/api.md b/docs/api.md index 208155ad..cd4a450d 100644 --- a/docs/api.md +++ b/docs/api.md @@ -208,6 +208,34 @@ Following example will trigger up to three retries, each after 1s, 3s and 5s res retryDelays: [1000, 3000, 5000] ``` +#### stallDetection + +_Default value:_ `{ enabled: false, stallTimeout: 30000, checkInterval: 5000 }` + +An object controlling the stall detection feature, which can automatically detect when an upload has stopped making progress and trigger a retry. This is useful for recovering from frozen uploads caused by network issues that don't trigger explicit errors. + +The stall detection options are: +- `enabled`: Boolean indicating whether stall detection is active (default: `false`) +- `stallTimeout`: Time in milliseconds without progress before considering the upload stalled (default: `30000`) +- `checkInterval`: How often in milliseconds to check for stalls (default: `5000`) + +**Note:** Stall detection only works with HTTP stacks that support progress events. Currently, this includes: +- `XHRHttpStack` (browser default) - Supported +- `NodeHttpStack` (Node.js default) - Supported +- `FetchHttpStack` - Not supported + +When a stall is detected, the upload will be automatically retried according to your `retryDelays` configuration. If `retryDelays` is `null`, the stall will trigger an error instead. + +Example configuration: + +```js +stallDetection: { + enabled: true, + stallTimeout: 15000, // 15 seconds without progress + checkInterval: 2000 // Check every 2 seconds +} +``` + #### storeFingerprintForResuming _Default value:_ `true` @@ -326,6 +354,7 @@ An object used as the HTTP stack for making network requests. This is an abstrac interface HttpStack { createRequest(method: string, url: string): HttpRequest; getName(): string; + supportsProgressEvents(): boolean; } interface HttpRequest { @@ -367,6 +396,14 @@ interface HttpResponse { ``` +The `supportsProgressEvents()` method should return `true` if the HTTP stack implementation supports progress events during upload, or `false` otherwise. This is used by tus-js-client to determine whether features like stall detection can be enabled. The built-in HTTP stacks have the following support: + +- `XHRHttpStack` (browser default): Returns `true` - XMLHttpRequest supports progress events +- `NodeHttpStack` (Node.js default): Returns `true` - Node.js HTTP module supports progress events +- `FetchHttpStack`: Returns `false` - Fetch API does not support upload progress events + +If you're implementing a custom HTTP stack, you should return `true` only if your implementation can reliably call the progress handler set via `setProgressHandler` during the upload process. + #### urlStorage _Default value:_ Environment-specific implementation diff --git a/lib/StallDetector.ts b/lib/StallDetector.ts new file mode 100644 index 00000000..fdcd1c23 --- /dev/null +++ b/lib/StallDetector.ts @@ -0,0 +1,93 @@ +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 | null = null + private lastProgressTime = 0 + private isActive = 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 + } + + this.lastProgressTime = Date.now() + this.isActive = true + + log( + `tus: starting stall detection with checkInterval: ${this.options.checkInterval}ms, stallTimeout: ${this.options.stallTimeout}ms`, + ) + + // Setup periodic check + this.intervalId = setInterval(() => { + if (!this.isActive) { + 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() + } + + /** + * 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(`tus: no progress for ${timeSinceProgress}ms (limit: ${stallTimeout}ms)`) + } + + return isStalled + } + + /** + * Handle a detected stall + */ + private _handleStall(reason: string): void { + log(`tus: upload stalled: ${reason}`) + this.stop() + this.onStallDetected(reason) + } +} diff --git a/lib/browser/FetchHttpStack.ts b/lib/browser/FetchHttpStack.ts index 0524e296..9c102d57 100644 --- 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 index 3e237da0..b95b6843 100644 --- 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 index c584040d..43ada4ae 100644 --- 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 = {}) { - 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 = {}) { - 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 index ab018cd2..df3704d5 100644 --- 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 index 91516141..873bb1d8 100644 --- 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 = {}) { - 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 = {}) { - 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 index 011de2f5..3b80e68c 100644 --- a/lib/options.ts +++ b/lib/options.ts @@ -48,6 +48,15 @@ export type UploadInput = // available in React Native | ReactNativeFile +/** + * Options for configuring stall detection behavior + */ +export interface StallDetectionOptions { + enabled: boolean + stallTimeout: number // Time in ms before considering progress stalled + checkInterval: number // How often to check for stalls +} + export interface UploadOptions { endpoint?: string @@ -84,6 +93,8 @@ export interface UploadOptions { httpStack: HttpStack protocol: typeof PROTOCOL_TUS_V1 | typeof PROTOCOL_IETF_DRAFT_03 | typeof PROTOCOL_IETF_DRAFT_05 + + stallDetection?: StallDetectionOptions } export interface OnSuccessPayload { @@ -141,6 +152,10 @@ 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. + supportsProgressEvents: () => boolean } export type HttpProgressHandler = (bytesSent: number) => void diff --git a/lib/upload.ts b/lib/upload.ts index 6a0329da..c44b8f5e 100644 --- a/lib/upload.ts +++ b/lib/upload.ts @@ -3,6 +3,7 @@ import { Base64 } from 'js-base64' // provides WHATWG URL? Then we can get rid of @rollup/plugin-commonjs. import URL from 'url-parse' import { DetailedError } from './DetailedError.js' +import { StallDetector } from './StallDetector.js' import { log } from './logger.js' import { type FileSource, @@ -54,6 +55,12 @@ export const defaultOptions = { httpStack: undefined, protocol: PROTOCOL_TUS_V1 as UploadOptions['protocol'], + + stallDetection: { + enabled: false, + stallTimeout: 30000, + checkInterval: 5000, + }, } export class BaseUpload { @@ -343,6 +350,7 @@ export class BaseUpload { if (totalSize == null) { throw new Error('tus: Expected totalSize to be set') } + this._emitProgress(totalProgress, totalSize) }, // Wait until every partial upload has an upload URL, so we can add @@ -835,7 +843,37 @@ export class BaseUpload { const start = this._offset let end = this._offset + this.options.chunkSize + // Create stall detector for this request if stall detection is enabled and supported + // but don't start it yet - we'll start it after onBeforeRequest completes + let stallDetector: StallDetector | undefined + + if (this.options.stallDetection?.enabled) { + // Only enable stall detection if the HTTP stack supports progress events + if (this.options.httpStack.supportsProgressEvents()) { + stallDetector = new StallDetector( + this.options.stallDetection, + this.options.httpStack, + (reason: string) => { + // Handle stall by aborting the current request and triggering retry + if (this._req) { + this._req.abort() + } + this._retryOrEmitError(new Error(`Upload stalled: ${reason}`)) + }, + ) + // Don't start yet - will be started after onBeforeRequest + } else { + log( + 'tus: stall detection is enabled but the HTTP stack does not support progress events, it will be disabled for this upload', + ) + } + } + req.setProgressHandler((bytesSent) => { + // Update per-request stall detector if active + if (stallDetector) { + stallDetector.updateProgress() + } this._emitProgress(start + bytesSent, this._size) }) @@ -883,18 +921,20 @@ export class BaseUpload { ) } + let response: HttpResponse if (value == null) { - return await this._sendRequest(req) + response = await this._sendRequest(req, undefined, stallDetector) + } else { + if ( + this.options.protocol === PROTOCOL_IETF_DRAFT_03 || + this.options.protocol === PROTOCOL_IETF_DRAFT_05 + ) { + req.setHeader('Upload-Complete', done ? '?1' : '?0') + } + response = await this._sendRequest(req, value, stallDetector) } - if ( - this.options.protocol === PROTOCOL_IETF_DRAFT_03 || - this.options.protocol === PROTOCOL_IETF_DRAFT_05 - ) { - req.setHeader('Upload-Complete', done ? '?1' : '?0') - } - this._emitProgress(this._offset, this._size) - return await this._sendRequest(req, value) + return response } /** @@ -992,8 +1032,12 @@ export class BaseUpload { * * @api private */ - _sendRequest(req: HttpRequest, body?: SliceType): Promise { - return sendRequest(req, body, this.options) + _sendRequest( + req: HttpRequest, + body?: SliceType, + stallDetector?: StallDetector, + ): Promise { + return sendRequest(req, body, this.options, stallDetector) } } @@ -1054,18 +1098,31 @@ async function sendRequest( req: HttpRequest, body: SliceType | undefined, options: UploadOptions, + stallDetector?: StallDetector, ): Promise { if (typeof options.onBeforeRequest === 'function') { await options.onBeforeRequest(req) } - const res = await req.send(body) - - if (typeof options.onAfterResponse === 'function') { - await options.onAfterResponse(req, res) + // Start stall detection after onBeforeRequest completes but before the actual network request + if (stallDetector) { + stallDetector.start() } - return res + try { + const res = await req.send(body) + + if (typeof options.onAfterResponse === 'function') { + await options.onAfterResponse(req, res) + } + + return res + } finally { + // Always stop the stall detector when the request completes (success or failure) + if (stallDetector) { + stallDetector.stop() + } + } } /** @@ -1219,6 +1276,10 @@ export async function terminate(url: string, options: UploadOptions): Promise { + describe('integration tests', () => { + it("should not enable stall detection if HTTP stack doesn't support progress events", async () => { + // Enable debug logging temporarily + const { enableDebugLog } = await import('tus-js-client') + enableDebugLog() + + const testStack = new TestHttpStack() + // Mock the stack to not support progress events + testStack.supportsProgressEvents = () => false + + const file = getBlob('hello world') + + const options = { + httpStack: testStack, + endpoint: 'https://tus.io/uploads', + stallDetection: { + enabled: true, + }, + onError: waitableFunction('onError'), + } + + const upload = new Upload(file, options) + + // Capture console output + const originalLog = console.log + let loggedMessage = '' + console.log = (message) => { + loggedMessage += message + } + + upload.start() + + // Handle the POST request + const req = await testStack.nextRequest() + expect(req.url).toBe('https://tus.io/uploads') + expect(req.method).toBe('POST') + req.respondWith({ + status: 201, + responseHeaders: { + Location: '/uploads/12345', + }, + }) + + // Wait a bit for any async operations + await new Promise((resolve) => setTimeout(resolve, 50)) + + // Restore console.log + console.log = originalLog + + // Check that stall detection was disabled with appropriate log message + expect(loggedMessage).toContain( + 'tus: stall detection is enabled but the HTTP stack does not support progress events, it will be disabled for this upload', + ) + + // Abort to clean up + upload.abort() + }) + + it('should upload a file with stall detection enabled', async () => { + const testStack = new TestHttpStack() + // Mock the stack to support progress events + testStack.supportsProgressEvents = () => true + + const file = getBlob('hello world') + + const options = { + httpStack: testStack, + endpoint: 'https://tus.io/uploads', + stallDetection: { + enabled: true, + checkInterval: 1000, + stallTimeout: 2000, + }, + onSuccess: waitableFunction('onSuccess'), + onError: waitableFunction('onError'), + } + + const upload = new Upload(file, options) + upload.start() + + // Handle the POST request to create the upload + let req = await testStack.nextRequest() + expect(req.url).toBe('https://tus.io/uploads') + expect(req.method).toBe('POST') + + req.respondWith({ + status: 201, + responseHeaders: { + Location: '/uploads/12345', + }, + }) + + // Handle the PATCH request to upload the file + req = await testStack.nextRequest() + expect(req.url).toBe('https://tus.io/uploads/12345') + expect(req.method).toBe('PATCH') + + // Complete the upload quickly (before stall detection triggers) + req.respondWith({ + status: 204, + responseHeaders: { + 'Upload-Offset': '11', + }, + }) + + // Wait for the upload to complete successfully + await options.onSuccess.toBeCalled() + + // Make sure the error callback was not called + expect(options.onError.calls.count()).toBe(0) + }) + + it('should detect stalls and emit error when no retries configured', async () => { + const testStack = new StallSimulatingHttpStack() + const file = getBlob('hello world') + + const options = { + httpStack: testStack, + endpoint: 'https://tus.io/uploads', + stallDetection: { + enabled: true, + checkInterval: 100, // Fast check interval for testing + stallTimeout: 200, // Short timeout for testing + }, + // No retries to get immediate error + retryDelays: null, + onError: waitableFunction('onError'), + onSuccess: waitableFunction('onSuccess'), + } + + const upload = new Upload(file, options) + + // Tell the stack to simulate a stall on the next PATCH request + testStack.stallOnNextPatch = true + + upload.start() + + // Handle the POST request to create the upload + const req = await testStack.nextRequest() + expect(req.method).toBe('POST') + req.respondWith({ + status: 201, + responseHeaders: { + Location: '/uploads/12345', + }, + }) + + // The PATCH request should be sent but will stall + // Don't wait for the response since it will be aborted + + // Wait for stall detection to trigger and error to be emitted + const error = await options.onError.toBeCalled() + expect(error.message).toContain('Upload stalled') + }) + + it('should retry when stall is detected', async () => { + const testStack = new StallSimulatingHttpStack() + const file = getBlob('hello world') + + let requestCount = 0 + + const options = { + httpStack: testStack, + endpoint: 'https://tus.io/uploads', + stallDetection: { + enabled: true, + checkInterval: 100, // Fast check interval for testing + stallTimeout: 200, // Short timeout for testing + }, + // Enable retries + retryDelays: [100], + onError: waitableFunction('onError'), + onSuccess: waitableFunction('onSuccess'), + } + + const upload = new Upload(file, options) + + // Tell the stack to simulate a stall on the first PATCH request only + testStack.stallOnNextPatch = true + + upload.start() + + // Keep handling requests until success + while (true) { + const req = await testStack.nextRequest() + requestCount++ + + if (req.method === 'POST') { + req.respondWith({ + status: 201, + responseHeaders: { + Location: '/uploads/12345', + }, + }) + } else if (req.method === 'HEAD') { + req.respondWith({ + status: 200, + responseHeaders: { + 'Upload-Offset': '0', + 'Upload-Length': '11', + }, + }) + } else if (req.method === 'PATCH') { + // Complete the upload on any PATCH that isn't stalled + req.respondWith({ + status: 204, + responseHeaders: { + 'Upload-Offset': '11', + }, + }) + break + } + + // Safety check to avoid infinite loop + if (requestCount > 10) { + throw new Error('Too many requests') + } + } + + // Wait for success + await options.onSuccess.toBeCalled() + + // Error should not have been called since we retried + expect(options.onError.calls.count()).toBe(0) + + // We should have had more than 1 request (at least POST + PATCH) + expect(requestCount).toBeGreaterThan(1) + }) + + it('should not incorrectly detect stalls during onBeforeRequest delays', async () => { + const testStack = new TestHttpStack() + // Mock the stack to support progress events + testStack.supportsProgressEvents = () => true + + const file = getBlob('hello world') + + const options = { + httpStack: testStack, + endpoint: 'https://tus.io/uploads', + stallDetection: { + enabled: true, + checkInterval: 100, + stallTimeout: 200, + }, + onBeforeRequest: async (_req) => { + // Simulate a long-running operation like fetching auth tokens + await new Promise((resolve) => setTimeout(resolve, 300)) + }, + onSuccess: waitableFunction('onSuccess'), + onError: waitableFunction('onError'), + } + + const upload = new Upload(file, options) + upload.start() + + // Handle the POST request to create the upload + let req = await testStack.nextRequest() + expect(req.url).toBe('https://tus.io/uploads') + expect(req.method).toBe('POST') + + req.respondWith({ + status: 201, + responseHeaders: { + Location: '/uploads/12345', + }, + }) + + // Handle the PATCH request + req = await testStack.nextRequest() + expect(req.url).toBe('https://tus.io/uploads/12345') + expect(req.method).toBe('PATCH') + + // Complete the upload + req.respondWith({ + status: 204, + responseHeaders: { + 'Upload-Offset': '11', + }, + }) + + // Wait for the upload to complete successfully + await options.onSuccess.toBeCalled() + + // Stall detection should not have triggered during the onBeforeRequest delay + expect(options.onError.calls.count()).toBe(0) + }) + + it('should detect stalls when progress events stop mid-upload', async () => { + const testStack = new TestHttpStack() + const file = getBlob('hello world'.repeat(100)) // Larger file for multiple progress events + + let progressCallCount = 0 + let progressHandler = null + + // Override createRequest to capture and control progress events + const originalCreateRequest = testStack.createRequest.bind(testStack) + testStack.createRequest = function(method, url) { + const req = originalCreateRequest(method, url) + + if (method === 'PATCH') { + const originalSetProgressHandler = req.setProgressHandler.bind(req) + req.setProgressHandler = function(handler) { + progressHandler = handler + originalSetProgressHandler(handler) + } + + // Override send to simulate progress events that stop + const originalSend = req.send.bind(req) + req.send = async function(body) { + const result = originalSend(body) + + // Simulate some progress events then stop + if (progressHandler && body) { + const totalSize = await getBodySize(body) + // Send progress events for first 30% of upload + for (let i = 0; i <= 3; i++) { + progressCallCount++ + progressHandler(Math.floor(totalSize * 0.1 * i)) + await new Promise(resolve => setTimeout(resolve, 50)) + } + // Then stop sending progress events to simulate a stall + } + + return result + } + } + + return req + } + + const options = { + httpStack: testStack, + endpoint: 'https://tus.io/uploads', + stallDetection: { + enabled: true, + checkInterval: 100, + stallTimeout: 200, + }, + retryDelays: null, // No retries to get immediate error + onError: waitableFunction('onError'), + onProgress: waitableFunction('onProgress'), + } + + const upload = new Upload(file, options) + upload.start() + + // Handle the POST request + const req = await testStack.nextRequest() + expect(req.method).toBe('POST') + req.respondWith({ + status: 201, + responseHeaders: { + Location: '/uploads/12345', + }, + }) + + // The PATCH request will start sending progress events then stall + + // Wait for stall detection to trigger + const error = await options.onError.toBeCalled() + expect(error.message).toContain('Upload stalled') + + // Verify that we received some progress events before the stall + expect(progressCallCount).toBeGreaterThan(0) + expect(options.onProgress.calls.count()).toBeGreaterThan(0) + }) + }) +}) From 6eb57be7071b42a03127aed3db211adda33445a0 Mon Sep 17 00:00:00 2001 From: m7kvqbe1 <48086589+m7kvqbe1@users.noreply.github.com> Date: Wed, 28 May 2025 12:29:05 +0100 Subject: [PATCH 02/12] Fix exports for HttpStack modules --- lib/browser/index.ts | 2 ++ lib/node/index.ts | 1 + 2 files changed, 3 insertions(+) diff --git a/lib/browser/index.ts b/lib/browser/index.ts index 43ada4ae..5949c333 100644 --- a/lib/browser/index.ts +++ b/lib/browser/index.ts @@ -58,4 +58,6 @@ const isSupported = // Note: The exported interface must be the same as in lib/node/index.ts. // Any changes should be reflected in both files. export { Upload, defaultOptions, isSupported, canStoreURLs, enableDebugLog, DetailedError } +export { XHRHttpStack } from './XHRHttpStack.js' +export { FetchHttpStack } from './FetchHttpStack.js' export type * from '../options.js' diff --git a/lib/node/index.ts b/lib/node/index.ts index 873bb1d8..2acd8fab 100644 --- a/lib/node/index.ts +++ b/lib/node/index.ts @@ -56,4 +56,5 @@ const isSupported = true // Note: The exported interface must be the same as in lib/browser/index.ts. // Any changes should be reflected in both files. export { Upload, defaultOptions, isSupported, canStoreURLs, enableDebugLog, DetailedError } +export { NodeHttpStack } from './NodeHttpStack.js' export type * from '../options.js' From 2ef5f04c15a7f75f1f71d5274a32c6e347429802 Mon Sep 17 00:00:00 2001 From: m7kvqbe1 <48086589+m7kvqbe1@users.noreply.github.com> Date: Thu, 5 Jun 2025 21:05:40 +0100 Subject: [PATCH 03/12] Add value-based stall detection to catch stuck progress Also refactors the test-stall-detection test suite. --- lib/StallDetector.ts | 22 +- lib/upload.ts | 77 +++-- test/spec/test-stall-detection.js | 463 +++++++++++++++--------------- 3 files changed, 270 insertions(+), 292 deletions(-) diff --git a/lib/StallDetector.ts b/lib/StallDetector.ts index fdcd1c23..046c776f 100644 --- a/lib/StallDetector.ts +++ b/lib/StallDetector.ts @@ -1,23 +1,17 @@ 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 | null = null private lastProgressTime = 0 + private lastProgressValue = 0 private isActive = false - constructor( - options: StallDetectionOptions, - httpStack: HttpStack, - onStallDetected: (reason: string) => void, - ) { + constructor(options: StallDetectionOptions, onStallDetected: (reason: string) => void) { this.options = options - this.httpStack = httpStack this.onStallDetected = onStallDetected } @@ -30,6 +24,7 @@ export class StallDetector { } this.lastProgressTime = Date.now() + this.lastProgressValue = 0 this.isActive = true log( @@ -44,7 +39,7 @@ export class StallDetector { const now = Date.now() if (this._isProgressStalled(now)) { - this._handleStall('no progress events received') + this._handleStall('no progress') } }, this.options.checkInterval) } @@ -62,9 +57,14 @@ export class StallDetector { /** * Update progress information + * @param progressValue The current progress value (bytes uploaded) */ - updateProgress(): void { - this.lastProgressTime = Date.now() + 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 + } } /** diff --git a/lib/upload.ts b/lib/upload.ts index c44b8f5e..4307fcd4 100644 --- a/lib/upload.ts +++ b/lib/upload.ts @@ -850,17 +850,13 @@ export class BaseUpload { if (this.options.stallDetection?.enabled) { // Only enable stall detection if the HTTP stack supports progress events if (this.options.httpStack.supportsProgressEvents()) { - stallDetector = new StallDetector( - this.options.stallDetection, - this.options.httpStack, - (reason: string) => { - // Handle stall by aborting the current request and triggering retry - if (this._req) { - this._req.abort() - } - this._retryOrEmitError(new Error(`Upload stalled: ${reason}`)) - }, - ) + stallDetector = new StallDetector(this.options.stallDetection, (reason: string) => { + // Handle stall by aborting the current request and triggering retry + if (this._req) { + this._req.abort() + } + this._retryOrEmitError(new Error(`Upload stalled: ${reason}`)) + }) // Don't start yet - will be started after onBeforeRequest } else { log( @@ -872,7 +868,7 @@ export class BaseUpload { req.setProgressHandler((bytesSent) => { // Update per-request stall detector if active if (stallDetector) { - stallDetector.updateProgress() + stallDetector.updateProgress(start + bytesSent) } this._emitProgress(start + bytesSent, this._size) }) @@ -921,20 +917,19 @@ export class BaseUpload { ) } - let response: HttpResponse if (value == null) { - response = await this._sendRequest(req, undefined, stallDetector) - } else { - if ( - this.options.protocol === PROTOCOL_IETF_DRAFT_03 || - this.options.protocol === PROTOCOL_IETF_DRAFT_05 - ) { - req.setHeader('Upload-Complete', done ? '?1' : '?0') - } - response = await this._sendRequest(req, value, stallDetector) + return await this._sendRequest(req, undefined, stallDetector) } - return response + if ( + this.options.protocol === PROTOCOL_IETF_DRAFT_03 || + this.options.protocol === PROTOCOL_IETF_DRAFT_05 + ) { + req.setHeader('Upload-Complete', done ? '?1' : '?0') + } + + this._emitProgress(this._offset, this._size) + return await this._sendRequest(req, value, stallDetector) } /** @@ -1104,25 +1099,27 @@ async function sendRequest( await options.onBeforeRequest(req) } - // Start stall detection after onBeforeRequest completes but before the actual network request - if (stallDetector) { - stallDetector.start() - } - - try { - const res = await req.send(body) - - if (typeof options.onAfterResponse === 'function') { - await options.onAfterResponse(req, res) + const sendWithStallDetection = async (): Promise => { + if (stallDetector) { + stallDetector.start() } - return res - } finally { - // Always stop the stall detector when the request completes (success or failure) - if (stallDetector) { - stallDetector.stop() + try { + return await req.send(body) + } finally { + if (stallDetector) { + stallDetector.stop() + } } } + + const res = await sendWithStallDetection() + + if (typeof options.onAfterResponse === 'function') { + await options.onAfterResponse(req, res) + } + + return res } /** @@ -1276,10 +1273,6 @@ export async function terminate(url: string, options: UploadOptions): Promise { + const progressPromise = self.progressPromises.get(req) + if (progressPromise) { + await progressPromise + self.progressPromises.delete(req) + } + originalRespondWith(resData) + } + + // Override send to handle progress sequences + req.send = async function (body) { + this.body = body + if (body) { + this.bodySize = await getBodySize(body) + } + + const progressSequence = self.progressSequences.get(req) + if (progressSequence && this._onProgress) { + self._scheduleProgressSequence(req, progressSequence, this._onProgress) + } else if (this._onProgress) { + self._scheduleDefaultProgress(req, this._onProgress, this.bodySize) + } + + this._onRequestSend(this) + return this._requestPromise + } } - supportsProgressEvents() { - return true + _scheduleProgressSequence(req, sequence, progressHandler) { + const progressPromise = new Promise((resolve) => { + setTimeout(async () => { + for (const event of sequence) { + await new Promise((resolve) => setTimeout(resolve, event.delay || 0)) + progressHandler(event.bytes) + } + resolve() + }, 10) // Small delay to ensure stall detector is started + }) + this.progressPromises.set(req, progressPromise) + } + + _scheduleDefaultProgress(req, progressHandler, bodySize) { + const progressPromise = new Promise((resolve) => { + setTimeout(() => { + progressHandler(0) + progressHandler(bodySize) + resolve() + }, 10) // Small delay to ensure stall detector is started + }) + this.progressPromises.set(req, progressPromise) } } -// Helper to get body size -function getBodySize(body) { - if (body == null) return null - if (body instanceof Blob) return body.size - if (body.length != null) return body.length - return 0 +/** + * Common test setup helper + */ +function createTestUpload(options = {}) { + const defaultOptions = { + httpStack: new StallTestHttpStack(), + endpoint: 'https://tus.io/uploads', + onError: waitableFunction('onError'), + onSuccess: waitableFunction('onSuccess'), + onProgress: waitableFunction('onProgress'), + } + + const file = options.file || getBlob('hello world') + const uploadOptions = { ...defaultOptions, ...options } + const upload = new Upload(file, uploadOptions) + + return { upload, options: uploadOptions, testStack: uploadOptions.httpStack } +} + +/** + * Helper to handle standard upload creation flow + */ +async function handleUploadCreation(testStack, location = '/uploads/12345') { + const req = await testStack.nextRequest() + expect(req.method).toBe('POST') + req.respondWith({ + status: 201, + responseHeaders: { + Location: location, + }, + }) + return req } describe('tus-stall-detection', () => { describe('integration tests', () => { it("should not enable stall detection if HTTP stack doesn't support progress events", async () => { - // Enable debug logging temporarily const { enableDebugLog } = await import('tus-js-client') enableDebugLog() const testStack = new TestHttpStack() - // Mock the stack to not support progress events testStack.supportsProgressEvents = () => false - const file = getBlob('hello world') - - const options = { + const { upload } = createTestUpload({ httpStack: testStack, - endpoint: 'https://tus.io/uploads', - stallDetection: { - enabled: true, - }, - onError: waitableFunction('onError'), - } - - const upload = new Upload(file, options) + stallDetection: { enabled: true }, + }) // Capture console output const originalLog = console.log @@ -74,157 +189,83 @@ describe('tus-stall-detection', () => { upload.start() - // Handle the POST request const req = await testStack.nextRequest() expect(req.url).toBe('https://tus.io/uploads') expect(req.method).toBe('POST') req.respondWith({ status: 201, - responseHeaders: { - Location: '/uploads/12345', - }, + responseHeaders: { Location: '/uploads/12345' }, }) - // Wait a bit for any async operations - await new Promise((resolve) => setTimeout(resolve, 50)) - - // Restore console.log + await wait(50) console.log = originalLog - // Check that stall detection was disabled with appropriate log message expect(loggedMessage).toContain( - 'tus: stall detection is enabled but the HTTP stack does not support progress events, it will be disabled for this upload', + 'tus: stall detection is enabled but the HTTP stack does not support progress events', ) - // Abort to clean up upload.abort() }) it('should upload a file with stall detection enabled', async () => { - const testStack = new TestHttpStack() - // Mock the stack to support progress events - testStack.supportsProgressEvents = () => true - - const file = getBlob('hello world') - - const options = { - httpStack: testStack, - endpoint: 'https://tus.io/uploads', + const { upload, options, testStack } = createTestUpload({ stallDetection: { enabled: true, checkInterval: 1000, stallTimeout: 2000, }, - onSuccess: waitableFunction('onSuccess'), - onError: waitableFunction('onError'), - } + }) - const upload = new Upload(file, options) upload.start() - // Handle the POST request to create the upload - let req = await testStack.nextRequest() - expect(req.url).toBe('https://tus.io/uploads') - expect(req.method).toBe('POST') - - req.respondWith({ - status: 201, - responseHeaders: { - Location: '/uploads/12345', - }, - }) + await handleUploadCreation(testStack) - // Handle the PATCH request to upload the file - req = await testStack.nextRequest() - expect(req.url).toBe('https://tus.io/uploads/12345') - expect(req.method).toBe('PATCH') + const patchReq = await testStack.nextRequest() + expect(patchReq.url).toBe('https://tus.io/uploads/12345') + expect(patchReq.method).toBe('PATCH') - // Complete the upload quickly (before stall detection triggers) - req.respondWith({ + patchReq.respondWith({ status: 204, - responseHeaders: { - 'Upload-Offset': '11', - }, + responseHeaders: { 'Upload-Offset': '11' }, }) - // Wait for the upload to complete successfully await options.onSuccess.toBeCalled() - - // Make sure the error callback was not called expect(options.onError.calls.count()).toBe(0) }) it('should detect stalls and emit error when no retries configured', async () => { - const testStack = new StallSimulatingHttpStack() - const file = getBlob('hello world') - - const options = { - httpStack: testStack, - endpoint: 'https://tus.io/uploads', + const { upload, options, testStack } = createTestUpload({ stallDetection: { enabled: true, - checkInterval: 100, // Fast check interval for testing - stallTimeout: 200, // Short timeout for testing + checkInterval: 100, + stallTimeout: 200, }, - // No retries to get immediate error retryDelays: null, - onError: waitableFunction('onError'), - onSuccess: waitableFunction('onSuccess'), - } - - const upload = new Upload(file, options) - - // Tell the stack to simulate a stall on the next PATCH request - testStack.stallOnNextPatch = true + }) + testStack.simulateStallOnNextPatch() upload.start() - // Handle the POST request to create the upload - const req = await testStack.nextRequest() - expect(req.method).toBe('POST') - req.respondWith({ - status: 201, - responseHeaders: { - Location: '/uploads/12345', - }, - }) - - // The PATCH request should be sent but will stall - // Don't wait for the response since it will be aborted + await handleUploadCreation(testStack) - // Wait for stall detection to trigger and error to be emitted const error = await options.onError.toBeCalled() expect(error.message).toContain('Upload stalled') }) it('should retry when stall is detected', async () => { - const testStack = new StallSimulatingHttpStack() - const file = getBlob('hello world') - - let requestCount = 0 - - const options = { - httpStack: testStack, - endpoint: 'https://tus.io/uploads', + const { upload, options, testStack } = createTestUpload({ stallDetection: { enabled: true, - checkInterval: 100, // Fast check interval for testing - stallTimeout: 200, // Short timeout for testing + checkInterval: 100, + stallTimeout: 200, }, - // Enable retries retryDelays: [100], - onError: waitableFunction('onError'), - onSuccess: waitableFunction('onSuccess'), - } - - const upload = new Upload(file, options) - - // Tell the stack to simulate a stall on the first PATCH request only - testStack.stallOnNextPatch = true + }) + testStack.simulateStallOnNextPatch() upload.start() - // Keep handling requests until success + let requestCount = 0 while (true) { const req = await testStack.nextRequest() requestCount++ @@ -232,9 +273,7 @@ describe('tus-stall-detection', () => { if (req.method === 'POST') { req.respondWith({ status: 201, - responseHeaders: { - Location: '/uploads/12345', - }, + responseHeaders: { Location: '/uploads/12345' }, }) } else if (req.method === 'HEAD') { req.respondWith({ @@ -245,167 +284,113 @@ describe('tus-stall-detection', () => { }, }) } else if (req.method === 'PATCH') { - // Complete the upload on any PATCH that isn't stalled req.respondWith({ status: 204, - responseHeaders: { - 'Upload-Offset': '11', - }, + responseHeaders: { 'Upload-Offset': '11' }, }) break } - // Safety check to avoid infinite loop if (requestCount > 10) { throw new Error('Too many requests') } } - // Wait for success await options.onSuccess.toBeCalled() - - // Error should not have been called since we retried expect(options.onError.calls.count()).toBe(0) - - // We should have had more than 1 request (at least POST + PATCH) expect(requestCount).toBeGreaterThan(1) }) it('should not incorrectly detect stalls during onBeforeRequest delays', async () => { - const testStack = new TestHttpStack() - // Mock the stack to support progress events - testStack.supportsProgressEvents = () => true - - const file = getBlob('hello world') - - const options = { - httpStack: testStack, - endpoint: 'https://tus.io/uploads', + const { upload, options, testStack } = createTestUpload({ stallDetection: { enabled: true, checkInterval: 100, stallTimeout: 200, }, onBeforeRequest: async (_req) => { - // Simulate a long-running operation like fetching auth tokens - await new Promise((resolve) => setTimeout(resolve, 300)) + await wait(300) // Longer than stall timeout }, - onSuccess: waitableFunction('onSuccess'), - onError: waitableFunction('onError'), - } + }) - const upload = new Upload(file, options) upload.start() - // Handle the POST request to create the upload - let req = await testStack.nextRequest() - expect(req.url).toBe('https://tus.io/uploads') - expect(req.method).toBe('POST') - - req.respondWith({ - status: 201, - responseHeaders: { - Location: '/uploads/12345', - }, - }) + await handleUploadCreation(testStack) - // Handle the PATCH request - req = await testStack.nextRequest() - expect(req.url).toBe('https://tus.io/uploads/12345') - expect(req.method).toBe('PATCH') + const patchReq = await testStack.nextRequest() + expect(patchReq.url).toBe('https://tus.io/uploads/12345') + expect(patchReq.method).toBe('PATCH') - // Complete the upload - req.respondWith({ + patchReq.respondWith({ status: 204, - responseHeaders: { - 'Upload-Offset': '11', - }, + responseHeaders: { 'Upload-Offset': '11' }, }) - // Wait for the upload to complete successfully await options.onSuccess.toBeCalled() - - // Stall detection should not have triggered during the onBeforeRequest delay expect(options.onError.calls.count()).toBe(0) }) it('should detect stalls when progress events stop mid-upload', async () => { - const testStack = new TestHttpStack() - const file = getBlob('hello world'.repeat(100)) // Larger file for multiple progress events - - let progressCallCount = 0 - let progressHandler = null - - // Override createRequest to capture and control progress events - const originalCreateRequest = testStack.createRequest.bind(testStack) - testStack.createRequest = function(method, url) { - const req = originalCreateRequest(method, url) - - if (method === 'PATCH') { - const originalSetProgressHandler = req.setProgressHandler.bind(req) - req.setProgressHandler = function(handler) { - progressHandler = handler - originalSetProgressHandler(handler) - } - - // Override send to simulate progress events that stop - const originalSend = req.send.bind(req) - req.send = async function(body) { - const result = originalSend(body) - - // Simulate some progress events then stop - if (progressHandler && body) { - const totalSize = await getBodySize(body) - // Send progress events for first 30% of upload - for (let i = 0; i <= 3; i++) { - progressCallCount++ - progressHandler(Math.floor(totalSize * 0.1 * i)) - await new Promise(resolve => setTimeout(resolve, 50)) - } - // Then stop sending progress events to simulate a stall - } - - return result - } - } - - return req - } - - const options = { - httpStack: testStack, - endpoint: 'https://tus.io/uploads', + const file = getBlob('hello world'.repeat(100)) + const { upload, options, testStack } = createTestUpload({ + file, stallDetection: { enabled: true, checkInterval: 100, stallTimeout: 200, }, - retryDelays: null, // No retries to get immediate error - onError: waitableFunction('onError'), - onProgress: waitableFunction('onProgress'), - } + retryDelays: null, + }) - const upload = new Upload(file, options) + // Create a progress sequence that stops at 30% of the file + const fileSize = file.size + const progressSequence = [ + { bytes: 0, delay: 10 }, + { bytes: Math.floor(fileSize * 0.1), delay: 50 }, + { bytes: Math.floor(fileSize * 0.2), delay: 50 }, + { bytes: Math.floor(fileSize * 0.3), delay: 50 }, + // No more progress events after 30% + ] + + testStack.setNextProgressSequence(progressSequence) upload.start() + await handleUploadCreation(testStack) - // Handle the POST request - const req = await testStack.nextRequest() - expect(req.method).toBe('POST') - req.respondWith({ - status: 201, - responseHeaders: { - Location: '/uploads/12345', + const error = await options.onError.toBeCalled() + expect(error.message).toContain('Upload stalled') + expect(options.onProgress.calls.count()).toBeGreaterThan(0) + }) + + it('should detect stalls when progress value does not change', async () => { + const { upload, options, testStack } = createTestUpload({ + stallDetection: { + enabled: true, + checkInterval: 50, + stallTimeout: 500, }, + retryDelays: null, }) - // The PATCH request will start sending progress events then stall + // Create a progress sequence that gets stuck at 300 bytes + 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 }), + ] + + testStack.setNextProgressSequence(progressSequence) + upload.start() - // Wait for stall detection to trigger - const error = await options.onError.toBeCalled() - expect(error.message).toContain('Upload stalled') + await handleUploadCreation(testStack) - // Verify that we received some progress events before the stall - expect(progressCallCount).toBeGreaterThan(0) + const patchReq = await testStack.nextRequest() + expect(patchReq.method).toBe('PATCH') + + const error = await options.onError.toBeCalled() + expect(error.message).toContain('Upload stalled: no progress') expect(options.onProgress.calls.count()).toBeGreaterThan(0) }) }) From dc9bec4787fc51877227143bebd8aa1343acdca3 Mon Sep 17 00:00:00 2001 From: m7kvqbe1 <48086589+m7kvqbe1@users.noreply.github.com> Date: Thu, 12 Jun 2025 11:22:10 +0100 Subject: [PATCH 04/12] Fix issue with stall detection errors bubbling to consumer --- lib/upload.ts | 20 +++++++++++++++++--- test/spec/test-stall-detection.js | 6 +++--- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/lib/upload.ts b/lib/upload.ts index 4307fcd4..98517327 100644 --- a/lib/upload.ts +++ b/lib/upload.ts @@ -104,6 +104,9 @@ export class BaseUpload { // The offset of the remote upload before the latest attempt was started. private _offsetBeforeRetry = 0 + // The reason for the last stall detection, if any + private _stallReason?: string + // An array of BaseUpload instances which are used for uploading the different // parts, if the parallelUploads option is used. private _parallelUploads?: BaseUpload[] @@ -818,8 +821,16 @@ export class BaseUpload { throw new Error(`tus: value thrown that is not an error: ${err}`) } + // Include stall reason in error message if available + const errorMessage = this._stallReason + ? `tus: failed to upload chunk at offset ${this._offset} (stalled: ${this._stallReason})` + : `tus: failed to upload chunk at offset ${this._offset}` + + // Clear the stall reason after using it + this._stallReason = undefined + throw new DetailedError( - `tus: failed to upload chunk at offset ${this._offset}`, + errorMessage, err, req, undefined, @@ -851,11 +862,14 @@ export class BaseUpload { // Only enable stall detection if the HTTP stack supports progress events if (this.options.httpStack.supportsProgressEvents()) { stallDetector = new StallDetector(this.options.stallDetection, (reason: string) => { - // Handle stall by aborting the current request and triggering retry + // Handle stall by aborting the current request + // The abort will cause the request to fail, which will be caught + // in _performUpload and wrapped in a DetailedError for proper retry handling if (this._req) { + this._stallReason = reason this._req.abort() } - this._retryOrEmitError(new Error(`Upload stalled: ${reason}`)) + // Don't call _retryOrEmitError here - let the natural error flow handle it }) // Don't start yet - will be started after onBeforeRequest } else { diff --git a/test/spec/test-stall-detection.js b/test/spec/test-stall-detection.js index 74da0eeb..da7632a6 100644 --- a/test/spec/test-stall-detection.js +++ b/test/spec/test-stall-detection.js @@ -249,7 +249,7 @@ describe('tus-stall-detection', () => { await handleUploadCreation(testStack) const error = await options.onError.toBeCalled() - expect(error.message).toContain('Upload stalled') + expect(error.message).toContain('stalled:') }) it('should retry when stall is detected', async () => { @@ -357,7 +357,7 @@ describe('tus-stall-detection', () => { await handleUploadCreation(testStack) const error = await options.onError.toBeCalled() - expect(error.message).toContain('Upload stalled') + expect(error.message).toContain('stalled:') expect(options.onProgress.calls.count()).toBeGreaterThan(0) }) @@ -390,7 +390,7 @@ describe('tus-stall-detection', () => { expect(patchReq.method).toBe('PATCH') const error = await options.onError.toBeCalled() - expect(error.message).toContain('Upload stalled: no progress') + expect(error.message).toContain('stalled: no progress') expect(options.onProgress.calls.count()).toBeGreaterThan(0) }) }) From d84c2bb9f1349f33fa43130c96d40b3767db626f Mon Sep 17 00:00:00 2001 From: Marius Kleidl Date: Thu, 12 Jun 2025 14:55:14 +0200 Subject: [PATCH 05/12] Fix linting issues --- lib/upload.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/lib/upload.ts b/lib/upload.ts index 98517327..b2bc18c4 100644 --- a/lib/upload.ts +++ b/lib/upload.ts @@ -829,12 +829,7 @@ export class BaseUpload { // Clear the stall reason after using it this._stallReason = undefined - throw new DetailedError( - errorMessage, - err, - req, - undefined, - ) + throw new DetailedError(errorMessage, err, req, undefined) } if (!inStatusCategory(res.getStatus(), 200)) { From 083c0034cecbcb344b2762eb8797cb80d8e68b67 Mon Sep 17 00:00:00 2001 From: Marius Kleidl Date: Thu, 12 Jun 2025 15:03:43 +0200 Subject: [PATCH 06/12] Get rid of unnecessary inner function --- lib/upload.ts | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/lib/upload.ts b/lib/upload.ts index b2bc18c4..a0543960 100644 --- a/lib/upload.ts +++ b/lib/upload.ts @@ -1108,22 +1108,19 @@ async function sendRequest( await options.onBeforeRequest(req) } - const sendWithStallDetection = async (): Promise => { - if (stallDetector) { - stallDetector.start() - } + if (stallDetector) { + stallDetector.start() + } - try { - return await req.send(body) - } finally { - if (stallDetector) { - stallDetector.stop() - } + let res: HttpResponse + try { + res = await req.send(body) + } finally { + if (stallDetector) { + stallDetector.stop() } } - const res = await sendWithStallDetection() - if (typeof options.onAfterResponse === 'function') { await options.onAfterResponse(req, res) } From e4175c33049fba02d97d88229f045596e51025ce Mon Sep 17 00:00:00 2001 From: m7kvqbe1 <48086589+m7kvqbe1@users.noreply.github.com> Date: Tue, 22 Jul 2025 16:47:38 +0100 Subject: [PATCH 07/12] Fix parallel upload resumability on 5xx status code --- lib/upload.ts | 16 ++- test/spec/test-parallel-uploads.js | 205 +++++++++++++++++++++++++++++ 2 files changed, 219 insertions(+), 2 deletions(-) diff --git a/lib/upload.ts b/lib/upload.ts index a0543960..744cf46f 100644 --- a/lib/upload.ts +++ b/lib/upload.ts @@ -119,6 +119,8 @@ 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) { @@ -286,7 +288,7 @@ export class BaseUpload { * * @api private */ - private async _startParallelUpload(): Promise { + private async _startParallelUpload(): Promise { const totalSize = this._size let totalProgress = 0 this._parallelUploads = [] @@ -376,6 +378,9 @@ 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. @@ -715,6 +720,11 @@ export class BaseUpload { throw new DetailedError('tus: upload is currently locked; retry later', undefined, req, res) } + // For 5xx server errors, throw error to trigger retry instead of creating new upload + if (inStatusCategory(status, 500)) { + throw new DetailedError('tus: server error during resume, retrying', undefined, req, res) + } + if (inStatusCategory(status, 400)) { // Remove stored fingerprint and corresponding endpoint, // on client errors since the file can not be found @@ -731,7 +741,7 @@ export class BaseUpload { ) } - // Try to create a new upload + // Try to create a new upload (only for 4xx client errors and 3xx redirects) this.url = null await this._createUpload() } @@ -762,6 +772,8 @@ export class BaseUpload { await this.options.onUploadUrlAvailable() } + + await this._saveUploadInUrlStorage() // Upload has already been completed and we do not need to send additional diff --git a/test/spec/test-parallel-uploads.js b/test/spec/test-parallel-uploads.js index 8408ffa9..dc7f6a83 100644 --- a/test/spec/test-parallel-uploads.js +++ b/test/spec/test-parallel-uploads.js @@ -578,5 +578,210 @@ describe('tus', () => { expect(options.onProgress).toHaveBeenCalledWith(5, 11) expect(options.onProgress).toHaveBeenCalledWith(11, 11) }) + + it('should preserve upload URL in partial uploads during retry', async () => { + const testStack = new TestHttpStack() + const file = getBlob('hello') + + const options = { + httpStack: testStack, + parallelUploads: 1, // Use single parallel to focus on one upload + retryDelays: [10], + endpoint: 'https://tus.io/uploads', + onSuccess: waitableFunction(), + headers: { 'Upload-Concat': 'partial' }, // Force partial upload behavior + storeFingerprintForResuming: false, // This is key - partial uploads don't store fingerprints + } + + const upload = new Upload(file, options) + upload.start() + + // Create partial upload + let req = await testStack.nextRequest() + expect(req.url).toBe('https://tus.io/uploads') + expect(req.method).toBe('POST') + expect(req.requestHeaders['Upload-Concat']).toBe('partial') + + req.respondWith({ + status: 201, + responseHeaders: { + Location: 'https://tus.io/uploads/upload1', + }, + }) + + // PATCH request fails + req = await testStack.nextRequest() + expect(req.url).toBe('https://tus.io/uploads/upload1') + expect(req.method).toBe('PATCH') + + req.respondWith({ + status: 500, + }) + + // The key test: what happens on retry? + req = await testStack.nextRequest() + + // With the fix: should be HEAD to existing URL (resume) + expect(req.url).toBe('https://tus.io/uploads/upload1') + expect(req.method).toBe('HEAD') + + // Without the fix: would be POST to create new upload + // (because storeFingerprintForResuming: false and uploadUrl: null) + + req.respondWith({ + status: 204, + responseHeaders: { + 'Upload-Length': '5', + 'Upload-Offset': '0', + }, + }) + + // Resume PATCH + req = await testStack.nextRequest() + expect(req.url).toBe('https://tus.io/uploads/upload1') + expect(req.method).toBe('PATCH') + + req.respondWith({ + status: 204, + responseHeaders: { + 'Upload-Offset': '5', + }, + }) + + await options.onSuccess.toBeCalled() + }) + + it('should resume partial uploads on retry instead of creating fresh uploads', async () => { + const testStack = new TestHttpStack() + const file = getBlob('hello world') + + // Track all requests to detect if fresh uploads are created + const allRequests = [] + const originalCreateRequest = testStack.createRequest.bind(testStack) + testStack.createRequest = function(method, url) { + allRequests.push({ method, url }) + return originalCreateRequest(method, url) + } + + const options = { + httpStack: testStack, + parallelUploads: 2, + retryDelays: [10], + endpoint: 'https://tus.io/uploads', + onSuccess: waitableFunction(), + } + + const upload = new Upload(file, options) + upload.start() + + // First partial upload creation + let req = await testStack.nextRequest() + expect(req.url).toBe('https://tus.io/uploads') + expect(req.method).toBe('POST') + expect(req.requestHeaders['Upload-Concat']).toBe('partial') + expect(req.requestHeaders['Upload-Length']).toBe('5') + + req.respondWith({ + status: 201, + responseHeaders: { + Location: 'https://tus.io/uploads/upload1', + }, + }) + + // Second partial upload creation + req = await testStack.nextRequest() + expect(req.url).toBe('https://tus.io/uploads') + expect(req.method).toBe('POST') + expect(req.requestHeaders['Upload-Concat']).toBe('partial') + expect(req.requestHeaders['Upload-Length']).toBe('6') + + req.respondWith({ + status: 201, + responseHeaders: { + Location: 'https://tus.io/uploads/upload2', + }, + }) + + // First PATCH request succeeds + req = await testStack.nextRequest() + expect(req.url).toBe('https://tus.io/uploads/upload1') + expect(req.method).toBe('PATCH') + expect(req.requestHeaders['Upload-Offset']).toBe('0') + + req.respondWith({ + status: 204, + responseHeaders: { + 'Upload-Offset': '5', + }, + }) + + // Second PATCH request fails (network error) + req = await testStack.nextRequest() + expect(req.url).toBe('https://tus.io/uploads/upload2') + expect(req.method).toBe('PATCH') + expect(req.requestHeaders['Upload-Offset']).toBe('0') + + req.respondWith({ + status: 500, + }) + + // CRITICAL TEST: After retry delay, we should NOT see another POST to /uploads + // The next request should be HEAD to the existing upload2 URL + req = await testStack.nextRequest() + + // Verify we're not creating a fresh upload (this is the key test) + expect(req.method).not.toBe('POST') + expect(req.url).toBe('https://tus.io/uploads/upload2') + expect(req.method).toBe('HEAD') + + req.respondWith({ + status: 204, + responseHeaders: { + 'Upload-Length': '6', + 'Upload-Offset': '0', + }, + }) + + // Resume with PATCH from current offset + req = await testStack.nextRequest() + expect(req.url).toBe('https://tus.io/uploads/upload2') + expect(req.method).toBe('PATCH') + expect(req.requestHeaders['Upload-Offset']).toBe('0') + + req.respondWith({ + status: 204, + responseHeaders: { + 'Upload-Offset': '6', + }, + }) + + // Final concatenation + req = await testStack.nextRequest() + expect(req.url).toBe('https://tus.io/uploads') + expect(req.method).toBe('POST') + expect(req.requestHeaders['Upload-Concat']).toBe( + 'final;https://tus.io/uploads/upload1 https://tus.io/uploads/upload2', + ) + + req.respondWith({ + status: 201, + responseHeaders: { + Location: 'https://tus.io/uploads/upload3', + }, + }) + + 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' + ) + + // 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) + }) + + }) }) From 6913195c8cc1b032b01f86b89545a84379e9b284 Mon Sep 17 00:00:00 2001 From: m7kvqbe1 <48086589+m7kvqbe1@users.noreply.github.com> Date: Thu, 2 Oct 2025 13:49:22 +0100 Subject: [PATCH 08/12] Ensure stallDetector is supplied for HEAD and POST requests --- lib/upload.ts | 43 +++++++---- test/spec/test-stall-detection.js | 121 ++++++++++++++++++++++++++++-- 2 files changed, 141 insertions(+), 23 deletions(-) diff --git a/lib/upload.ts b/lib/upload.ts index 744cf46f..b75cdcfa 100644 --- a/lib/upload.ts +++ b/lib/upload.ts @@ -407,7 +407,9 @@ export class BaseUpload { let res: HttpResponse try { - res = await this._sendRequest(req) + // Create stall detector for final concatenation POST request + const stallDetector = this._createStallDetector() + res = await this._sendRequest(req, undefined, stallDetector) } catch (err) { if (!(err instanceof Error)) { throw new Error(`tus: value thrown that is not an error: ${err}`) @@ -638,7 +640,9 @@ export class BaseUpload { ) { req.setHeader('Upload-Complete', '?0') } - res = await this._sendRequest(req) + // Create stall detector for POST request + const stallDetector = this._createStallDetector() + res = await this._sendRequest(req, undefined, stallDetector) } } catch (err) { if (!(err instanceof Error)) { @@ -700,7 +704,9 @@ export class BaseUpload { let res: HttpResponse try { - res = await this._sendRequest(req) + // Create stall detector for HEAD request + const stallDetector = this._createStallDetector() + res = await this._sendRequest(req, undefined, stallDetector) } catch (err) { if (!(err instanceof Error)) { throw new Error(`tus: value thrown that is not an error: ${err}`) @@ -852,23 +858,15 @@ export class BaseUpload { } /** - * _addChunktoRequest reads a chunk from the source and sends it using the - * supplied request object. It will not handle the response. + * Create a stall detector if stall detection is enabled and supported. * * @api private */ - private async _addChunkToRequest(req: HttpRequest): Promise { - const start = this._offset - let end = this._offset + this.options.chunkSize - - // Create stall detector for this request if stall detection is enabled and supported - // but don't start it yet - we'll start it after onBeforeRequest completes - let stallDetector: StallDetector | undefined - + private _createStallDetector(): StallDetector | undefined { if (this.options.stallDetection?.enabled) { // Only enable stall detection if the HTTP stack supports progress events if (this.options.httpStack.supportsProgressEvents()) { - stallDetector = new StallDetector(this.options.stallDetection, (reason: string) => { + return new StallDetector(this.options.stallDetection, (reason: string) => { // Handle stall by aborting the current request // The abort will cause the request to fail, which will be caught // in _performUpload and wrapped in a DetailedError for proper retry handling @@ -878,13 +876,28 @@ export class BaseUpload { } // Don't call _retryOrEmitError here - let the natural error flow handle it }) - // Don't start yet - will be started after onBeforeRequest } else { log( 'tus: stall detection is enabled but the HTTP stack does not support progress events, it will be disabled for this upload', ) } } + return undefined + } + + /** + * _addChunktoRequest reads a chunk from the source and sends it using the + * supplied request object. It will not handle the response. + * + * @api private + */ + private async _addChunkToRequest(req: HttpRequest): Promise { + const start = this._offset + let end = this._offset + this.options.chunkSize + + // Create stall detector for this request if stall detection is enabled and supported + // but don't start it yet - we'll start it after onBeforeRequest completes + const stallDetector = this._createStallDetector() req.setProgressHandler((bytesSent) => { // Update per-request stall detector if active diff --git a/test/spec/test-stall-detection.js b/test/spec/test-stall-detection.js index da7632a6..61f6674c 100644 --- a/test/spec/test-stall-detection.js +++ b/test/spec/test-stall-detection.js @@ -22,6 +22,7 @@ class StallTestHttpStack extends TestHttpStack { this.progressSequences = new Map() this.progressPromises = new Map() this.nextProgressSequence = null + this.methodsToStall = new Set() } /** @@ -31,6 +32,19 @@ class StallTestHttpStack extends TestHttpStack { this.stallOnNextPatch = true } + /** + * Configure the stack to simulate a stall for a specific HTTP method + * + * When this is called, the specified HTTP method will stall on the next request. + * The stall is created by returning a promise that never resolves or rejects, + * simulating a network request that starts but never receives any data. + * + * @param {String} method - HTTP method to stall (e.g., 'POST', 'HEAD', 'PATCH') + */ + simulateStallForMethod(method) { + this.methodsToStall.add(method) + } + /** * Set a custom progress sequence for the next PATCH request * @param {Array} sequence - Array of {bytes: number, delay: number} objects @@ -46,24 +60,55 @@ class StallTestHttpStack extends TestHttpStack { createRequest(method, url) { const req = super.createRequest(method, url) - if (method === 'PATCH') { + if (this.methodsToStall.has(method)) { + this._setupMethodStall(req, method) + this.methodsToStall.delete(method) + } else if (method === 'PATCH') { this._setupPatchRequest(req) } return req } + _setupMethodStall(req, method) { + const originalAbort = req.abort.bind(req) + + req.send = async function (body) { + this.body = body + + // We create a promise but never resolve or reject it, this + // simulates a network request that starts but never completes + this._requestPromise = new Promise((resolve, reject) => { + this._rejectRequest = reject + this._resolveRequest = resolve + }) + + if (req._onRequestSend) { + req._onRequestSend(this) + } + + // Return the hanging promise - the caller will await this forever + // (until StallDetector calls abort after timeout) + return this._requestPromise + } + + req.abort = function() { + if (this._rejectRequest) { + this._rejectRequest(new Error('request aborted')) + } + originalAbort() + } + } + _setupPatchRequest(req) { const self = this - // Handle complete stalls if (this.stallOnNextPatch) { this.stallOnNextPatch = false req.send = async function (body) { this.body = body if (body) { this.bodySize = await getBodySize(body) - // Don't call progress handler to simulate a complete stall } this._onRequestSend(this) return this._requestPromise @@ -71,13 +116,11 @@ class StallTestHttpStack extends TestHttpStack { return } - // Handle progress sequences if (this.nextProgressSequence) { this.progressSequences.set(req, this.nextProgressSequence) this.nextProgressSequence = null } - // Override respondWith to wait for progress events const originalRespondWith = req.respondWith.bind(req) req.respondWith = async (resData) => { const progressPromise = self.progressPromises.get(req) @@ -88,7 +131,6 @@ class StallTestHttpStack extends TestHttpStack { originalRespondWith(resData) } - // Override send to handle progress sequences req.send = async function (body) { this.body = body if (body) { @@ -115,7 +157,7 @@ class StallTestHttpStack extends TestHttpStack { progressHandler(event.bytes) } resolve() - }, 10) // Small delay to ensure stall detector is started + }, 10) }) this.progressPromises.set(req, progressPromise) } @@ -126,7 +168,7 @@ class StallTestHttpStack extends TestHttpStack { progressHandler(0) progressHandler(bodySize) resolve() - }, 10) // Small delay to ensure stall detector is started + }, 10) }) this.progressPromises.set(req, progressPromise) } @@ -166,6 +208,47 @@ async function handleUploadCreation(testStack, location = '/uploads/12345') { return req } +/** + * Helper function to test stall detection for a specific request method + */ +async function testStallDetectionForMethod(method, uploadOptions = {}) { + const { enableDebugLog } = await import('tus-js-client') + enableDebugLog() + + const testStack = new StallTestHttpStack() + testStack.simulateStallForMethod(method) + + const options = { + httpStack: testStack, + stallDetection: { + enabled: true, + checkInterval: 50, + stallTimeout: 200, + }, + retryDelays: null, + ...uploadOptions, + } + + const { upload, options: testOptions } = createTestUpload(options) + + const originalLog = console.log + let loggedMessage = '' + console.log = (message) => { + loggedMessage += message + '\n' + } + + upload.start() + + const request = await testStack.nextRequest() + expect(request.method).toBe(method) + + const error = await testOptions.onError.toBeCalled() + + console.log = originalLog + + return { error, loggedMessage, request } +} + describe('tus-stall-detection', () => { describe('integration tests', () => { it("should not enable stall detection if HTTP stack doesn't support progress events", async () => { @@ -393,5 +476,27 @@ describe('tus-stall-detection', () => { expect(error.message).toContain('stalled: no progress') expect(options.onProgress.calls.count()).toBeGreaterThan(0) }) + + it('should detect stalls during POST request (upload creation)', async () => { + const { error, loggedMessage, request } = await testStallDetectionForMethod('POST') + + expect(request.url).toBe('https://tus.io/uploads') + expect(error.message).toContain('request aborted') + expect(error.message).toContain('POST') + expect(loggedMessage).toContain('starting stall detection') + expect(loggedMessage).toContain('upload stalled') + }) + + it('should detect stalls during HEAD request (resuming upload)', async () => { + const { error, loggedMessage, request } = await testStallDetectionForMethod('HEAD', { + uploadUrl: 'https://tus.io/uploads/existing', + }) + + expect(request.url).toBe('https://tus.io/uploads/existing') + expect(error.message).toContain('request aborted') + expect(error.message).toContain('HEAD') + expect(loggedMessage).toContain('starting stall detection') + expect(loggedMessage).toContain('upload stalled') + }) }) }) From 8d18ffdd38cba225df73d5b2a04447f90e2bfd48 Mon Sep 17 00:00:00 2001 From: m7kvqbe1 <48086589+m7kvqbe1@users.noreply.github.com> Date: Fri, 3 Oct 2025 09:29:46 +0100 Subject: [PATCH 09/12] Track progress events instead of byte changes for stall detection 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. --- lib/StallDetector.ts | 14 ++++------- lib/upload.ts | 15 ++++-------- test/spec/test-parallel-uploads.js | 8 +++---- test/spec/test-stall-detection.js | 37 ++++++++++++++++++++---------- 4 files changed, 37 insertions(+), 37 deletions(-) diff --git a/lib/StallDetector.ts b/lib/StallDetector.ts index 046c776f..b9b2f1b8 100644 --- a/lib/StallDetector.ts +++ b/lib/StallDetector.ts @@ -7,7 +7,6 @@ export class StallDetector { private intervalId: ReturnType | 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 index b75cdcfa..5946d603 100644 --- 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 { + private async _startParallelUpload(): Promise { 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 index dc7f6a83..a4038514 100644 --- 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 index 61f6674c..02c4d05b 100644 --- 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) }) From 4cd5c533e889d201d32c7052f4b7a6eebdc086d1 Mon Sep 17 00:00:00 2001 From: m7kvqbe1 <48086589+m7kvqbe1@users.noreply.github.com> Date: Mon, 6 Oct 2025 12:22:59 +0100 Subject: [PATCH 10/12] Implement progressiveUrlSaving for parallel uploads --- README.md | 16 ++++++++++++++++ docs/api.md | 41 +++++++++++++++++++++++++++++++++++++++++ lib/options.ts | 1 + lib/upload.ts | 45 +++++++++++++++++++++++++++++++++++++++++---- 4 files changed, 99 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 145f61f0..8417cf9e 100644 --- a/README.md +++ b/README.md @@ -55,6 +55,22 @@ input.addEventListener('change', function (e) { }) ``` +### Parallel Uploads with Progressive URL Saving + +For better fault tolerance with parallel uploads, you can enable progressive URL saving: + +```js +var upload = new tus.Upload(file, { + endpoint: 'http://localhost:1080/files/', + parallelUploads: 4, + progressiveUrlSaving: true, // Save each partial upload URL immediately + urlStorage: myThreadSafeStorage, // Your storage implementation + // ... other options +}) +``` + +When enabled, partial upload URLs are saved immediately as each completes, rather than waiting for all to finish. This improves resumability if failures occur during parallel uploads. See the [API documentation](docs/api.md#progressiveurlsaving) for implementation details. + ## Documentation - [Installation & Requirements](/docs/installation.md) diff --git a/docs/api.md b/docs/api.md index cd4a450d..8209e19d 100644 --- a/docs/api.md +++ b/docs/api.md @@ -248,6 +248,47 @@ _Default value:_ `false` A boolean indicating if the fingerprint in the URL storage will be removed once the upload is successfully completed. When this feature is enabled and the same file is uploaded again, it will create an entirely new upload instead of reusing the previous one. Furthermore, this option will only change behavior if `urlStorage` is not `null`. +#### progressiveUrlSaving + +_Default value:_ `false` + +A boolean indicating whether partial upload URLs should be saved progressively during parallel uploads. When `false` (default), all partial upload URLs must be successfully created before any are saved to storage. When `true`, each partial upload URL is saved immediately after its POST request succeeds. + +This option only has an effect when `parallelUploads` is greater than 1. Enabling this provides better fault tolerance for parallel uploads: +- If a browser crash or network failure occurs, successfully created partial uploads can still be resumed +- Earlier persistence reduces the window of data loss +- More granular progress tracking across sessions + +When using this option, your `urlStorage` implementation should handle concurrent updates safely, especially if using a database backend. Consider using a mutex or other synchronization mechanism to prevent race conditions when multiple parallel uploads save their URLs simultaneously. + +Example usage with a thread-safe storage implementation: +```js +import { Mutex } from 'async-mutex' + +class ThreadSafeUrlStorage { + constructor() { + this.mutex = new Mutex() + } + + async addUpload(fingerprint, upload) { + const release = await this.mutex.acquire() + try { + // Merge parallelUploadUrls arrays safely + // Your database operations here + } finally { + release() + } + } +} + +const upload = new tus.Upload(file, { + parallelUploads: 4, + progressiveUrlSaving: true, + urlStorage: new ThreadSafeUrlStorage(), + // ... other options +}) +``` + #### uploadLengthDeferred _Default value:_ `false` diff --git a/lib/options.ts b/lib/options.ts index 3b80e68c..883b146a 100644 --- a/lib/options.ts +++ b/lib/options.ts @@ -85,6 +85,7 @@ export interface UploadOptions { parallelUploadBoundaries?: { start: number; end: number }[] storeFingerprintForResuming: boolean removeFingerprintOnSuccess: boolean + progressiveUrlSaving: boolean uploadLengthDeferred: boolean uploadDataDuringCreation: boolean diff --git a/lib/upload.ts b/lib/upload.ts index 5946d603..87e1e26a 100644 --- a/lib/upload.ts +++ b/lib/upload.ts @@ -47,6 +47,7 @@ export const defaultOptions = { parallelUploadBoundaries: undefined, storeFingerprintForResuming: true, removeFingerprintOnSuccess: false, + progressiveUrlSaving: false, uploadLengthDeferred: false, uploadDataDuringCreation: false, @@ -361,10 +362,17 @@ export class BaseUpload { onUploadUrlAvailable: async () => { // @ts-expect-error We know that _parallelUploadUrls is defined this._parallelUploadUrls[index] = upload.url - // Test if all uploads have received an URL - // @ts-expect-error We know that _parallelUploadUrls is defined - if (this._parallelUploadUrls.filter((u) => Boolean(u)).length === parts.length) { - await this._saveUploadInUrlStorage() + + // Progressive saving: save immediately when each URL becomes available + // This allows for better fault tolerance and earlier persistence + if (this.options.progressiveUrlSaving && upload.url) { + await this._savePartialUploadUrl(index, upload.url) + } else { + // Legacy behavior: wait for all URLs before saving + // @ts-expect-error We know that _parallelUploadUrls is defined + if (this._parallelUploadUrls.filter((u) => Boolean(u)).length === parts.length) { + await this._saveUploadInUrlStorage() + } } }, } @@ -1010,6 +1018,35 @@ export class BaseUpload { this._urlStorageKey = undefined } + /** + * Save a single partial upload URL at the specified index. + * This is used for progressive URL saving during parallel uploads. + * + * The UrlStorage implementation must handle concurrent updates + * safely when using this method. + * + * @api private + */ + private async _savePartialUploadUrl(index: number, url: string): Promise { + if ( + !this.options.storeFingerprintForResuming || + !this._fingerprint + ) { + return + } + + const storedUpload: PreviousUpload = { + size: this._size, + metadata: this.options.metadata, + creationTime: new Date().toString(), + urlStorageKey: this._fingerprint, + parallelUploadUrls: this._parallelUploadUrls, + } + + const urlStorageKey = await this.options.urlStorage.addUpload(this._fingerprint, storedUpload) + this._urlStorageKey = urlStorageKey + } + /** * Add the upload URL to the URL storage, if possible. * From e9ade356e1f248909e253ec6496f3969ceede7be Mon Sep 17 00:00:00 2001 From: ollie-sutton Date: Tue, 7 Oct 2025 10:34:00 +0100 Subject: [PATCH 11/12] Progressive save update --- docs/api.md | 30 ------------------------------ lib/options.ts | 2 +- lib/upload.ts | 37 ++++--------------------------------- 3 files changed, 5 insertions(+), 64 deletions(-) diff --git a/docs/api.md b/docs/api.md index 8209e19d..5b9ca05a 100644 --- a/docs/api.md +++ b/docs/api.md @@ -259,36 +259,6 @@ This option only has an effect when `parallelUploads` is greater than 1. Enablin - Earlier persistence reduces the window of data loss - More granular progress tracking across sessions -When using this option, your `urlStorage` implementation should handle concurrent updates safely, especially if using a database backend. Consider using a mutex or other synchronization mechanism to prevent race conditions when multiple parallel uploads save their URLs simultaneously. - -Example usage with a thread-safe storage implementation: -```js -import { Mutex } from 'async-mutex' - -class ThreadSafeUrlStorage { - constructor() { - this.mutex = new Mutex() - } - - async addUpload(fingerprint, upload) { - const release = await this.mutex.acquire() - try { - // Merge parallelUploadUrls arrays safely - // Your database operations here - } finally { - release() - } - } -} - -const upload = new tus.Upload(file, { - parallelUploads: 4, - progressiveUrlSaving: true, - urlStorage: new ThreadSafeUrlStorage(), - // ... other options -}) -``` - #### uploadLengthDeferred _Default value:_ `false` diff --git a/lib/options.ts b/lib/options.ts index 883b146a..7e7ffdfc 100644 --- a/lib/options.ts +++ b/lib/options.ts @@ -117,7 +117,7 @@ export interface PreviousUpload { metadata: { [key: string]: string } creationTime: string uploadUrl?: string - parallelUploadUrls?: string[] + parallelUploadUrls?: (string | null)[] urlStorageKey: string } diff --git a/lib/upload.ts b/lib/upload.ts index 87e1e26a..344c70f2 100644 --- a/lib/upload.ts +++ b/lib/upload.ts @@ -114,7 +114,7 @@ export class BaseUpload { // An array of upload URLs which are used for uploading the different // parts, if the parallelUploads option is used. - private _parallelUploadUrls?: string[] + private _parallelUploadUrls?: (string | null)[] // True if the remote upload resource's length is deferred (either taken from // upload options or HEAD response) @@ -313,7 +313,7 @@ export class BaseUpload { })) // Create an empty list for storing the upload URLs - this._parallelUploadUrls = new Array(parts.length) + this._parallelUploadUrls = Array.apply(null, Array(this.options.parallelUploads)).map(() => null) // Generate a promise for each slice that will be resolve if the respective // upload is completed. @@ -365,8 +365,8 @@ export class BaseUpload { // Progressive saving: save immediately when each URL becomes available // This allows for better fault tolerance and earlier persistence - if (this.options.progressiveUrlSaving && upload.url) { - await this._savePartialUploadUrl(index, upload.url) + if (this.options.progressiveUrlSaving) { + await this._saveUploadInUrlStorage() } else { // Legacy behavior: wait for all URLs before saving // @ts-expect-error We know that _parallelUploadUrls is defined @@ -1018,35 +1018,6 @@ export class BaseUpload { this._urlStorageKey = undefined } - /** - * Save a single partial upload URL at the specified index. - * This is used for progressive URL saving during parallel uploads. - * - * The UrlStorage implementation must handle concurrent updates - * safely when using this method. - * - * @api private - */ - private async _savePartialUploadUrl(index: number, url: string): Promise { - if ( - !this.options.storeFingerprintForResuming || - !this._fingerprint - ) { - return - } - - const storedUpload: PreviousUpload = { - size: this._size, - metadata: this.options.metadata, - creationTime: new Date().toString(), - urlStorageKey: this._fingerprint, - parallelUploadUrls: this._parallelUploadUrls, - } - - const urlStorageKey = await this.options.urlStorage.addUpload(this._fingerprint, storedUpload) - this._urlStorageKey = urlStorageKey - } - /** * Add the upload URL to the URL storage, if possible. * From 49f87effee14d1082636cf86bdeb02caeff37cec Mon Sep 17 00:00:00 2001 From: ollie-sutton Date: Tue, 7 Oct 2025 13:47:30 +0100 Subject: [PATCH 12/12] Fix error with progressive saving not calling once the key is returned --- lib/upload.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lib/upload.ts b/lib/upload.ts index 344c70f2..72c0bb8a 100644 --- a/lib/upload.ts +++ b/lib/upload.ts @@ -313,7 +313,7 @@ export class BaseUpload { })) // Create an empty list for storing the upload URLs - this._parallelUploadUrls = Array.apply(null, Array(this.options.parallelUploads)).map(() => null) + this._parallelUploadUrls = parts.map((part) => part.uploadUrl) // Generate a promise for each slice that will be resolve if the respective // upload is completed. @@ -360,6 +360,8 @@ export class BaseUpload { // Wait until every partial upload has an upload URL, so we can add // them to the URL storage. onUploadUrlAvailable: async () => { + if (!upload.url) return + // @ts-expect-error We know that _parallelUploadUrls is defined this._parallelUploadUrls[index] = upload.url @@ -1026,12 +1028,10 @@ export class BaseUpload { private async _saveUploadInUrlStorage(): Promise { // We do not store the upload URL // - if it was disabled in the option, or - // - if no fingerprint was calculated for the input (i.e. a stream), or - // - if the URL is already stored (i.e. key is set alread). + // - if no fingerprint was calculated for the input (i.e. a stream) if ( !this.options.storeFingerprintForResuming || - !this._fingerprint || - this._urlStorageKey != null + !this._fingerprint ) { return }