Repository navigation
feat: downloads UX, failed-download cleanup, live progress and uploads view - #102
khaosdoctor wants to merge 11 commits into
Conversation
 [High impact] The PR changes how Jack retries downloads, serves peer files, and reports transfer history. Do not merge yet; the earlier findings still include unintended file deletion, failed downloads, and receipts that can change another peer's upload.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const failedDownloadReaper = downloadsConfig | ||
| ? new FailedDownloadReaper({ | ||
| // Read per tick, like unlinkImportedFiles, so the UI toggle applies live. | ||
| enabled: () => configService?.getRawDownloads()?.removeFailedDownloads ?? downloadsConfig.removeFailedDownloads, |
There was a problem hiding this comment.
If removeFailedDownloads was true at startup, turning the switch off does not stop automatic deletion. The UI sends null, and updateDownloads removes the saved key. This expression then falls back to the startup value, which is still true. Failed downloads and their files keep getting deleted until jack restarts.
When configService exists, treat an absent saved key as false. Only use the startup value when that service is unavailable.
| enabled: () => configService?.getRawDownloads()?.removeFailedDownloads ?? downloadsConfig.removeFailedDownloads, | |
| enabled: () => configService | |
| ? configService.getRawDownloads()?.removeFailedDownloads ?? false | |
| : downloadsConfig.removeFailedDownloads, |
| if (entry.nextAt === undefined) { | ||
| if (entry.retries >= this.options.maxRetries || !canRetry(row)) { | ||
| await this.removeRow(row, entry.retries) |
There was a problem hiding this comment.
The reaper uses a snapshot taken before any awaited work. While it waits for an earlier import retry or deletion, a user can retry a later row whose automatic retries are spent. The reaper still deletes that row using its old state. DownloadsService.delete then cancels the restarted transfer and removes its files, or removes a file now queued for import.
Check that the row is still failed and not cancelled inside the same per-row lock used for deletion.
| ***Revert*** discards your edits. `unlinkImportedFiles` takes effect the moment | ||
| you save. **Every other key here is read at startup, so restart jack** to apply | ||
| ***Revert*** discards your edits. `unlinkImportedFiles` and | ||
| `removeFailedDownloads` take effect the moment you save. **Every other key here is read at startup, so restart jack** to apply |
There was a problem hiding this comment.
The added sentence exceeds 80 columns. The repository writing guide requires prose under website/ to wrap at 80 columns. Split this sentence across lines without changing the rendered text. This repository requirement must be satisfied before merging.
| `removeFailedDownloads` take effect the moment you save. **Every other key here is read at startup, so restart jack** to apply | |
| `removeFailedDownloads` take effect the moment you save. **Every other key here is | |
| read at startup, so restart jack** to apply |
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
🟡 Changes recommended
Critical issues allow automatic retries and file deletion to continue after the feature is disabled, while stale state checks can restart or delete user-cancelled downloads and their retained partial files.
2 open issues
What changed in this PR
Adds opt-in retries with exponential backoff for failed downloads in jack,
followed by removal when retries are exhausted.
Changes:
- Adds a scheduled reaper that uses existing management retry and delete paths.
- Exposes configuration in the settings UI and documentation, with tests for
backoff, retry limits, and cancellation exclusions.
| File | Description |
|---|---|
website/reference/configuration.md |
Documents failed-download settings and retry behavior. |
website/guide/management-ui.md |
Describes the live removal toggle. |
apps/ui/app/types/management.ts |
Adds failed-download configuration types. |
apps/ui/app/components/DownloadsSection.vue |
Adds the removal toggle and retry controls. |
apps/backend/src/modules/downloads/failed-download-reaper.ts |
Implements retry scheduling and eventual removal. |
apps/backend/src/modules/config/config.service.ts |
Updates comments about live configuration. |
apps/backend/src/modules/config/config.router.ts |
Updates API documentation for live settings. |
apps/backend/src/lib/config.ts |
Defines configuration validation and defaults. |
apps/backend/src/index.ts |
Wires the reaper into startup and shutdown. |
apps/backend/src/__tests__/failed-download-reaper.test.ts |
Tests retry timing, removal, and exclusions. |
apps/backend/src/__tests__/downloads-service.test.ts |
Adds new defaults to the configuration fixture. |
Review effort: Balanced
| const failedDownloadReaper = downloadsConfig | ||
| ? new FailedDownloadReaper({ | ||
| // Read per tick, like unlinkImportedFiles, so the UI toggle applies live. | ||
| enabled: () => configService?.getRawDownloads()?.removeFailedDownloads ?? downloadsConfig.removeFailedDownloads, |
| if (row.status !== 'failed' || row.errorCode === 'CANCELLED') | ||
| continue |
khaosdoctor
changed the title
|
|
||
| /** Starts a row for this request and returns the body to send instead of `body`. */ | ||
| track(input: CreateUploadInput, body: Blob, signal?: AbortSignal): ReadableStream<Uint8Array> { | ||
| const upload = this.repository.create(input) |
There was a problem hiding this comment.
If SQLite cannot write, for example because the disk holding the database is full, repository.create() throws out of track() before the file response is returned. The peer gets HTTP 500 even when the library file is readable.
Catch the initial insert failure and serve the file without tracking, just as later tracking writes are prevented from breaking the transfer.
| get(id: number): LiveProgress | null { | ||
| const window = this.samples.get(id) | ||
| if (!window?.length) | ||
| return null | ||
| const first = window[0]! | ||
| const last = window.at(-1)! | ||
| const elapsedMs = last.t - first.t | ||
| return { | ||
| downloadedBytes: last.bytes, | ||
| speedBps: elapsedMs > 0 | ||
| ? ((last.bytes - first.bytes) * 1000) / elapsedMs |
There was a problem hiding this comment.
get() keeps returning the last positive speed after a transfer stops receiving bytes. Old samples are removed only by record(), which does not run during a stall or retry delay. The Downloads and Uploads pages keep showing an active speed and an outdated time remaining.
Expire the speed when samples become stale and notify open pages when that happens, while keeping the last byte count.
| // Row selection for bulk actions, keyed by download id (see get-row-id) so it | ||
| // survives live updates and paging. | ||
| const rowSelection = ref<Record<string, boolean>>({}) | ||
| const selected = computed(() => (data.value?.downloads ?? []).filter(d => rowSelection.value[String(d.id)])) |
There was a problem hiding this comment.
selected includes rows that live updates have hidden from the current filter. Select two downloading rows under the Downloading filter and let one finish. That row disappears but remains a target of bulk Delete. Deleting the remaining visible selection also removes the hidden row and its completed file.
Limit bulk targets to selected rows that still match filtered, or remove their selection when they stop matching.
…history migration
| @requiresInitialization | ||
| /** | ||
| * Tell the serving jack this file arrived and how long it took. It sends files | ||
| * zero-copy, so it can see an early disconnect but not the end of a transfer. | ||
| * Fire and forget: a lost receipt only leaves that upload's speed unknown, and | ||
| * a peer too old to issue upload ids gets none. | ||
| */ | ||
| private sendUploadReceipt(uploadId: string | null, bytes: number, durationMs: number): void { |
There was a problem hiding this comment.
@requiresInitialization now decorates sendUploadReceipt instead of downloadFile. Resumed downloads can therefore fetch files from a peer whose handshake failed, including one rejected for an incompatible protocol version. The handshake runs only after the file has been saved, through an unawaited receipt call.
Move the decorator back to downloadFile and keep receipt failures contained.
| updatedAt: timestamp, | ||
| finishedAt: timestamp, | ||
| }) | ||
| .where(and(eq(uploads.id, id), eq(uploads.peerName, peerName), inArray(uploads.status, [...OPEN]))) |
There was a problem hiding this comment.
Receipts use peerName to decide who owns an upload, but API key names are optional, editable, and not unique. Two unnamed keys both become unnamed, so either peer can submit a receipt for the other's numbered upload and replace its status, byte count, and duration. The real peer's later receipt is then ignored.
Store a stable key ID for ownership and keep the name only for display.
How this was verified: Separate authenticated keys can share apiKeyName, and complete() checks that name rather than the authenticated key's identity before updating an upload.
| const upload = this.repository.create(input, expectsReceipt ? 'uploading' : 'unconfirmed') | ||
| // Bun aborts the request signal only when the peer disconnects before the | ||
| // whole body went out; a finished response never fires it. | ||
| signal?.addEventListener('abort', () => this.repository.markAborted(upload.id, 'peer disconnected'), { once: true }) |
There was a problem hiding this comment.
start() no longer checks whether the request signal has already aborted. The route waits for streamFile() before calling it, so a peer can disconnect while jack waits for the source file path. Adding a listener afterward does not replay the abort. The new row then appears as uploading for up to the 48-hour expiry instead of being marked aborted.
Check signal.aborted before attaching the listener.
| for (const peer of peers) { | ||
| const name = peerNames.get(peer.peerId) | ||
| if (name) { |
There was a problem hiding this comment.
When a configured peer's catalog becomes empty, recordCatalogSizes() writes no row for that peer. An earlier nonzero reading from the same day stays unchanged; on later days, the chart shows gaps instead of zero. Iterate over peerNames and use zero counts when a configured peer has no cached titles.
| - **Lifetime**: files imported, bytes downloaded and uploaded, the | ||
| upload/download ratio, and the number of transfers. | ||
| - **Connections**: each peer and server, and whether it is reachable. |
There was a problem hiding this comment.
The new dashboard section names use plain bold. The writing guide requires UI references, including section names, to use bold and italic. Change **Lifetime**, **Connections**, and the other section names in this list to triple-asterisk formatting. This repository requirement must be satisfied before merging.
Context Used: CLAUDE.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
What does this PR do?
Combines #100, #101 and #103 with three follow-ups, as asked, so it can be reviewed as one change. One commit per change, so they can still be read separately.
downloads.removeFailedDownloads, off by default, applies live). A failed download is retried up tofailedDownloadRetriestimes (minimum and default 3, since one network error is often enough to fail an attempt), with the delay doubling fromfailedRetryBaseDelayMsand capped atretryMaxDelayMs. If it keeps failing, the row and its files are deleted the way Delete does it. User cancels are left alone. Retry counts are in memory, so a restart starts them over.apps/ui/package.json, which stays at0.1.0. CI now passes the image'sorg.opencontainers.image.versionlabel value asAPP_VERSION. Release images showv1.2.3, branch and PR images showmain/pr-<n>. Local builds fall back to the latest git tag, thendev.speedBps/etaSeconds. The downloads and overview streams update at most once a second.uploadstable (migration0014), one row per request with its range start. Files still go out zero-copy. Counting bytes in JS cost about 7x the CPU per GiB in a local benchmark (0.30 s vs 0.04 s), so jack doesn't touch the body:aborted.X-Jack-Receipt: 1, getsX-Jack-Upload-Idin the response, and after the file is in place posts{ bytes, durationMs }to the newPOST /peer/uploads/:id/receipt. That marks the rowcompletedwith an average speed. The POST is fire-and-forget with a 10 s timeout and never affects the download.unconfirmedwith unknown size and speed. Rows still waiting after 48 h are expired tounconfirmed.Related issue
Supersedes #100, #101 and #103.
Checklist
mise run lint:fixwas run and its output is committedmise run testpasses (andmise run test:e2eif transfer/request paths changed): unit tests pass (645); e2e not run, and the peer file route and transfer loop did change