Skip to content

fix(tus): fail closed on incomplete uploads without discarding safe retries - #1294

Open
ferhatelmas wants to merge 1 commit into
masterfrom
ferhat/tus
Open

fix(tus): fail closed on incomplete uploads without discarding safe retries#1294
ferhatelmas wants to merge 1 commit into
masterfrom
ferhat/tus

Conversation

@ferhatelmas

@ferhatelmas ferhatelmas commented Aug 3, 2026

Copy link
Copy Markdown
Member

What kind of change does this PR introduce?

Bug fix

What is the current behavior?

Upstream gracefully ends the proxy stream on request abort so the datastore sees a clean EOF and a truncated PATCH resolves a successful partial write with a phantom offset.

What is the new behavior?

Incoming message and internal cancellation signal are carried into the datastore through async local storage. After write resolves, bytes written are compared to content length, or abort/complete/cancel for chunked bodies, and accordingly upload is destroyed than resumed to prevent silent corruption.

Mutation is tracked to stay resumable if errors came before durable store changes.
Also, harden remove for genuine upstream gaps to delete part, clear cache, handle per key delete errors.

Additional context

  • Upload-Checksum is silently ignored as before.
  • Ultimate solution is in upstream, which I will follow up, and checksum extension will provide the missing guarantee even if byte arithmetric holds.

Copilot AI review requested due to automatic review settings August 3, 2026 16:23
@ferhatelmas
ferhatelmas requested a review from a team as a code owner August 3, 2026 16:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the TUS upload pipeline against silently accepting truncated/incomplete requests by propagating request/cancellation context into the datastore layer, then invalidating uploads when writes are incomplete or ambiguous—while attempting to preserve safe retries when no durable mutation occurred.

Changes:

  • Add an AsyncLocalStorage-backed request context and a writeWithRequestCompletion() guard that invalidates uploads on length/offset mismatch, aborted/incomplete bodies, or write errors.
  • Wrap FileStore and S3Store writes to apply completion checks, and add more robust S3 artifact cleanup (multipart abort + object/part/cache deletion with retries and error aggregation).
  • Add/extend integration and unit tests covering truncated PATCH/POST behavior, lock contention, S3 mutation failure paths, and Upload-Checksum being ignored.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/storage/protocols/tus/request-context.ts Introduces ALS request context + write completion/invalidation logic.
src/storage/protocols/tus/request-context.test.ts Unit tests for invalidation/retention behavior under various request conditions.
src/storage/protocols/tus/file-store.ts Wraps file writes with request-completion checks and mutation tracking.
src/storage/protocols/tus/file-store.test.ts Tests safe retry vs invalidation behavior around file write stream failures and concurrency limits.
src/storage/protocols/tus/s3-store.ts Wraps S3 writes, tracks mutations, and implements a more robust remove/cleanup sequence.
src/storage/protocols/tus/s3-store.test.ts Tests multipart abort ordering, cleanup retries/aggregation, and mutation-tracked invalidation behavior.
src/storage/protocols/tus/index.ts Re-exports the new request-context helpers.
src/http/routes/tus/index.ts Wraps TUS server handling to establish ALS context and propagate internal cancellation signals.
src/http/routes/tus/lifecycle.test.ts Adds coverage asserting Upload-Checksum remains ignored.
src/test/tus.test.ts Adds end-to-end tests for truncated uploads and S3 edge cases (lock contention, mutation failure cleanup).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/storage/protocols/tus/file-store.ts
Comment thread src/storage/protocols/tus/s3-store.ts Outdated
@coveralls

coveralls commented Aug 3, 2026

Copy link
Copy Markdown

Coverage Report for CI Build 30833546350

Coverage increased (+0.2%) to 80.652%

Details

  • Coverage increased (+0.2%) from the base build.
  • Patch coverage: 8 uncovered changes across 2 files (124 of 132 lines covered, 93.94%).
  • 1 coverage regression across 1 file.

Uncovered Changes

File Changed Covered %
src/storage/protocols/tus/s3-store.ts 65 58 89.23%
src/storage/protocols/tus/request-context.ts 49 48 97.96%
Total (4 files) 132 124 93.94%

Coverage Regressions

1 previously-covered line in 1 file lost coverage.

File Lines Losing Coverage Coverage
src/http/routes/s3/index.ts 1 87.42%

Coverage Stats

Coverage Status
Relevant Lines: 12775
Covered Lines: 10731
Line Coverage: 84.0%
Relevant Branches: 7584
Covered Branches: 5689
Branch Coverage: 75.01%
Branches in Coverage %: Yes
Coverage Strength: 432.88 hits per line

💛 - Coveralls

…etries

Signed-off-by: ferhat elmas <elmas.ferhat@gmail.com>
@ferhatelmas

Copy link
Copy Markdown
Member Author

@claude review always

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/test/tus.test.ts:274

  • The comment says this helper waits up to 5 seconds, but the deadline is set to 10 seconds. Align the comment and implementation to avoid confusion when diagnosing test timeouts.
  // It can wait up to 5 seconds for the lock.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants