Add failed-upload retention cleanup and legal warning banners - #3
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a failed-upload retention flow (mark-as-failed endpoint + periodic cleanup) and surfaces legal warning banners on the upload page, with a regression test for retention timing.
Changes:
- Introduces
POST /api/upload/fail/:uploadId, a failed-upload retention window constant, and a periodic cleanup worker gated byDISABLE_FAILED_UPLOAD_CLEANUP. - Updates the upload client to mark uploads as failed on non-user errors (instead of immediate server cancellation).
- Adds top/bottom legal warning banners to the upload UI, plus a test validating retention and cleanup behavior.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/routes/upload.js |
Adds failed-upload retention state, cleanup worker, and /fail endpoint; clears failed marker on resumed chunk uploads. |
test/upload.test.js |
Adds regression test validating failed-upload retention and cleanup timing via exported helpers. |
public/index.html |
Adds legal warning banners and switches client failure handling to call the new /fail endpoint. |
public/styles.css |
Styles the new legal warning banners and adjusts responsive sizing. |
.env.example |
Documents DISABLE_FAILED_UPLOAD_CLEANUP toggle. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Upload resumed after failure; clear failed marker so cleanup won't remove active data. | ||
| delete metadata.failedAt; |
There was a problem hiding this comment.
In the chunk handler, metadata.failedAt is cleared in-memory but isn’t persisted until after the chunk write. The failed-upload cleanup interval can race with an upload resuming and delete the partial file/metadata during this window (worst case: the file is deleted and then re-created by fs.open(..., 'a'), corrupting the upload). Persist the cleared failedAt (and/or set an explicit active state) before any file operations, and/or have the cleanup worker also gate deletion on lastActivity being older than the retention window.
| // Upload resumed after failure; clear failed marker so cleanup won't remove active data. | |
| delete metadata.failedAt; | |
| // Upload resumed after failure; clear failed marker and persist activity | |
| // before any file operations so cleanup logic cannot race and delete | |
| // the partial upload based on stale on-disk metadata. | |
| delete metadata.failedAt; | |
| metadata.lastActivity = new Date().toISOString(); | |
| await writeUploadMetadata(uploadId, metadata); |
| // Import the app | ||
| const { app, initialize, config } = require('../src/app'); | ||
| const { readUploadMetadata, cleanupFailedUploads, FAILED_UPLOAD_RETENTION_MS } = require('../src/routes/upload'); | ||
|
|
There was a problem hiding this comment.
The upload routes module now starts the failed-upload cleanup interval on import. This test file disables batch cleanup, but doesn’t disable failed-upload cleanup, so the background interval can make the new retention test flaky on slow runs (it may delete the partial/metadata based on wall clock time). Consider setting process.env.DISABLE_FAILED_UPLOAD_CLEANUP = 'true' in tests (and/or calling stopFailedUploadCleanup() in teardown) and invoking cleanupFailedUploads(...) explicitly when needed.
Summary
POST /api/upload/fail/:uploadIdto mark failed uploads for delayed cleanupDISABLE_FAILED_UPLOAD_CLEANUP)Verification
nodeis not available (command not found)