fix(upload): tell the operator why an asset upload failed - #3302
Conversation
|
Very nice! I didn't consider this use case but it's a good one to support indeed. |
|
Great! Let me know if you want any changes. Are you happy for me to follow-up with a PR to chunk uploads to stay under the limit? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3302 +/- ##
=========================================
Coverage ? 90.33%
=========================================
Files ? 85
Lines ? 9942
Branches ? 1098
=========================================
Hits ? 8981
Misses ? 708
Partials ? 253 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Yeah let's do chunk'd uploads as a follow-up. |
|
@mickzijdel Actually, can't merge this since the commit aren't signed. Sign the commits and we should be good! |
The Add asset upload path is raw XHR (uploadFiles / uploadOne in
home.ts) rather than an htmx-managed request, so nothing replays a
server toast for it and every failure resolved to the same string:
"Upload failed — check the file and try again". For the one status an
operator can act on, that sends them to the wrong place entirely.
A 413 on this path never comes from Anthias. Django runs with
DATA_UPLOAD_MAX_MEMORY_SIZE = None and the Caddy sidecar that
enable_ssl.sh installs sets `request_body { max_size 0 }`, so the
limit always belongs to a reverse proxy the operator put in front of
the device — Cloudflare caps request bodies at 100 MB on its Free and
Pro plans, which a 4K clip clears easily. The response is generated at
the edge and never reaches Django, so there is no HX-Trigger toast to
replay and the status code is the whole signal. Told to check the
file, the operator inspects a file that is fine while the actual fix
sits in their CDN configuration.
Map 413 to a message that names the size limit and says it belongs to
the server or a proxy, and 403 to a stale-page message for the CSRF
case. 5xx points at the device logs. Everything else keeps the
original wording, so an unsupported file type reads as it always did.
A transport failure is reported separately, and deliberately without
a diagnosis. A proxy enforcing a body limit answers and closes while
the browser is still writing the file, and the browser does not always
salvage that response — Firefox drops it once enough of the body is
still queued, where Chromium keeps it. So a size rejection can arrive
here indistinguishable from a dropped connection, and the message
names both causes rather than guessing between them. Whenever the
status does survive, the specific 413 message above is still what the
operator sees.
There is no 507 branch. The device does fill up, but assets_upload
answers ENOSPC with 200 plus an HX-Trigger toast carrying the shared
DISK_FULL_ERROR string, which fireToastFromHeader already displays;
only the REST API returns 507, and this uploader never calls it.
Adding one would have introduced a second wording for a message
anthias_common.utils centralises precisely so it cannot drift.
UploadResult becomes a discriminated union to carry the status from
uploadOne up to the batch toast, and upload errors sit on screen for
8s rather than the 4s default since they are the longest strings the
store carries and a dismissed toast cannot be recalled.
Verified against a dev stack behind Caddy with a 1 MB request-body
cap, driving the real Add asset modal in both browsers: a 2 MB file
draws the size message in each, a 36 MB file draws it in Chromium and
the transport message in Firefox, a 100 KB file still uploads and
creates its row, and the same files on master draw the old generic
string. The specs cover the mapping, the status surviving the trip up
from uploadOne, the toast lifetime, a batch aborting on the first
transport failure without attempting the rest, and a partial batch
still committing the rows that succeeded.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XQ7dFy1EnsBd5394K1UHTz
The reverse-proxy FAQ warns that a proxy in front of Anthias breaks
uploads, but only covers the rewritten `Host` header and the CSRF
rejection that follows. A request-body cap is the other way a proxy
breaks uploads, and it presents differently: everything works except
large videos, so the operator has no reason to connect it to their
proxy at all.
Anthias never generates that failure itself. Django runs with
DATA_UPLOAD_MAX_MEMORY_SIZE = None and the Caddy sidecar sets
`request_body { max_size 0 }`, so any size limit reached belongs to
something the operator put in front of the device. nginx is the
common surprise at 1 MB by default, and Cloudflare rejects bodies
over 100 MB on Free and Pro.
The Cloudflare row states the ceiling is set by plan and can be
lowered but not raised, since the dashboard exposes a maximum upload
size that only moves downward.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XQ7dFy1EnsBd5394K1UHTz
7f6f39d to
e53d970
Compare
|
|
Did that @vpetersson (not sure if the force-push notifies you) |
|
It's live! Thanks @mickzijdel! |
vpetersson-bot
left a comment
vpetersson-bot
left a comment
There was a problem hiding this comment.
Review
Fetched the branch and checked it adversarially: verified every factual claim in the description against the code, ran the suite, typechecked, and mutation-tested the new specs. The change does what it says, and I found nothing malicious or accidental hiding in it.
What I verified
- Scope matches the description: 5 files, TypeScript plus one YAML doc block. No Python, no Dockerfiles, no workflows, no
package.jsonorbun.lockchange, so nothing new runs at install time or in CI beyond the test files. - No hidden characters. Scanned the diff for zero-width, bidi-override and NBSP: nothing. Every non-ASCII byte is an em dash or one arrow, all in comments or in operator copy that already used them.
- No injection surface. All five messages are static constants, nothing from
responseTextor a response header reaches them, and toasts render throughx-textin_toasts.html, notx-html. - The tests are real. I mutated
home.tsthree ways (discarded the status, dropped thettlMsargument, removed the batchbreak) and each mutation fails a test.bun testgives 97 pass / 0 fail andbunx tsc --noEmitis clean. CI runsbun run test, so these gate. Worth knowing thattscis not in CI here (lint:checkandformat:checkare no-ops), so the type cleanliness is yours, not enforced. - Every server-side claim holds:
DATA_UPLOAD_MAX_MEMORY_SIZE = None(settings.py:730), Caddyrequest_body { max_size 0 }(bin/enable_ssl.sh:157), ENOSPC answered with 200 plusDISK_FULL_ERROR(views.py:509), and 507 only in the REST mixin, which this uploader never calls.ttlMsis honoured by the store (vendor.ts:53), so the 8s is not a no-op, and.app-toast__msghasmin-width: 0with nonowrap, so the longer strings wrap rather than clip.
Docs commit: accurate and consistent with the table right above it. Goldmark has GFM tables on and docs-prose already styles table/th/td (website/src/main.css:608), so it renders. client_max_body_size 0, LimitRequestBody 0 as the Apache default, and the 100 MB Cloudflare Free/Pro ceiling all check out. The FAQ JSON-LD flattens the table through plainify, but the existing Host table does the same, so that is consistent rather than new.
Five notes inline, plus two small drifts not worth their own thread:
- The doc comment above
uploadOne(home.ts:458) still describes the old bare-string results ("'ok' on a 2xx"). - The description says 11 new tests. There are 14, 7 per file.
Only the 5xx one is worth acting on before merge, since it is a few lines and it undercuts the PR's own thesis. The rest are fine as follow-ups.
| return 'Upload rejected — reload the page and try again' | ||
| } | ||
|
|
||
| if (status >= 500) { |
There was a problem hiding this comment.
Proxy-generated 5xx blames the device. This is the one I would fix before merge.
The premise of the PR is that some statuses cannot have come from Anthias, but only 413 gets that treatment. 502, 503, 504 and Cloudflare's 520-527 are also generated upstream of the device, and they all land on "check the device logs". In your own scenario, a Cloudflare tunnel, a long video upload hitting a 524 timeout is a likely failure, and this sends the operator to a log that shows nothing wrong: the same wrong-place problem the PR sets out to fix, one status family over.
A gateway branch before the >= 500 catch-all would cover it, something like "Couldn't reach the device, a proxy in front of it gave up. Check the tunnel or proxy config."
| // itself, so stop the batch rather than hammering on. | ||
| aborted = true | ||
| failure = result.failure | ||
| break |
There was a problem hiding this comment.
A 413 aborts the rest of the batch, and the message does not say so.
Behaviour is unchanged from master, so this is not a regression, but the new wording makes it visible. "File too large" reads as a statement about one file, while files 2..N of the selection were silently never attempted. The operator has no way to tell which files are now stored.
Two ways out. Keep the abort and add "the remaining files were not uploaded" to the message, or (better, and matching the per-file failure handling the comments in this function claim) treat 413 like rejected and keep going, since the next file may well be under the cap. The case that bites is a mixed selection: one 4K video plus a handful of images, where every image is skipped because of the video.
|
|
||
| // Everything else reached Anthias and was refused; keep the original | ||
| // wording so unsupported-type reads the way it always has. | ||
| return 'Upload failed — check the file and try again' |
There was a problem hiding this comment.
401, 407 and 429 fall through to the generic message.
A proxy in front of a publicly reachable device commonly enforces its own auth, and Cloudflare rate-limits. Those produce exactly the "check the file and try again" misdirection this PR exists to remove, and no amount of looking at the file will help.
Related, and cheaper: a Cloudflare WAF block is a 403, which currently reads "reload the page and try again". Softening that copy so it does not foreclose the proxy would cover both the CSRF case and the WAF case without a new branch.
| // 'error' — transport failure / non-2xx. Aborts the batch, and | ||
| // carries the failure so the toast can say why — see | ||
| // home/upload-error. | ||
| type UploadResult = |
There was a problem hiding this comment.
Nit, but it is an inconsistency inside a single PR: UploadResult discriminates on status (a string) while UploadFailure discriminates on kind and uses status for the HTTP number. So result.status === 'error' and result.failure.status === 413 sit two lines apart in uploadOne meaning entirely different things. Naming this union's tag kind too would make both read the same way.
|
|
||
| // CSRF rejection. An expired session does not land here — authorized | ||
| // answers 302 to /login/, which XHR follows transparently. | ||
| if (status === 403) { |
There was a problem hiding this comment.
This comment is correct (an unauthenticated request does get a 302 to /login/, lib/auth.py:427, and XHR follows it), but it stops one step short of where that lands: the login page returns 200 with no HX-Trigger, so uploadOne resolves ok, the file counts as a success, the modal closes and the table refreshes with no new row. Session auth is live in this stack, so it is reachable, if niche.
Pre-existing and outside the scope of this PR, so not a change request. Worth a few more words here though, because as written the comment reads as though the case is handled, and the next reader will take it that way.
vpetersson-bot
left a comment
vpetersson-bot
left a comment
There was a problem hiding this comment.
Requesting the 5xx change before merge. Everything else in my review stands as follow-ups, and the rest of the PR is good: claims verified, specs mutation-checked, nothing risky in the diff.
The reason this one is a blocker rather than a nit is that it contradicts the PR's own argument. The change is built on "this status cannot have come from Anthias, so do not point the operator at Anthias", and 413 is not the only status in that class. 502, 503, 504 and Cloudflare's 520-527 are all generated upstream of the device too, and right now they send the operator to a device log that shows nothing wrong. In the setup that motivated the PR, a Cloudflare tunnel carrying a large video, 524 is a likely outcome.
Suggestion inline on the branch, splitting the gateway statuses out ahead of the generic 5xx case. Two things to note about it:
>= 520covers Cloudflare's520-527block without enumerating it. Nothing standard lives up there, so the range is safe.- I left the punctuation of the new string plain rather than copying the em dash from the line below it. Match whichever you prefer.
Worth adding a spec alongside the existing 500 and 502 cases in upload-error.test.ts: 502 and 524 on the new message, 500 still on the old one. That pins the boundary, which is the part likely to drift later.
| if (status >= 500) { | ||
| return ( | ||
| 'The server failed while handling the upload — check the device logs' | ||
| ) | ||
| } |
There was a problem hiding this comment.
| if (status >= 500) { | |
| return ( | |
| 'The server failed while handling the upload — check the device logs' | |
| ) | |
| } | |
| // 502 / 503 / 504, and Cloudflare's 520-527, are generated by the | |
| // proxy or tunnel in front of the device rather than by Anthias, so | |
| // the device logs show nothing. Name the hop that actually failed. | |
| if (status === 502 || status === 503 || status === 504 || status >= 520) { | |
| return ( | |
| 'Could not reach the device: a proxy or tunnel in front of it ' + | |
| 'gave up. Check its logs and its timeout settings' | |
| ) | |
| } | |
| if (status >= 500) { | |
| return ( | |
| 'The server failed while handling the upload — check the device logs' | |
| ) | |
| } |
The existing 500 line is unchanged, this only lifts the gateway statuses out ahead of it. 524 is the one I would expect an operator to hit first on a tunnel, since it fires on a slow upload that is otherwise working.
3 participants
Issues Fixed
No existing issue for this one. I ran into accessing Anthias behind a Cloudflare tunnel. However, uploading a large video failed, and the UI just says
Upload failed — check the file and try again. So I checked the console, and there I can see it is actually a Cloudflare 413 error about the file being too large which Anthias doesn't display.Claude Opus 5 did the actual fixing but I did a lot of prodding to keep the change minimal. I have a plan for follow-up PRs to finish/fix the chunkable uploads feature which would bypass this issue entirely, but having clearer errors would be a good step regardless.
Description
The Add asset upload path is raw XHR (
uploadFiles/uploadOneinhome.ts) rather than an htmx-managed request, so nothing replays a server toast for it and every failure resolved to that one string.A 413 on this path never comes from Anthias. Django runs with
DATA_UPLOAD_MAX_MEMORY_SIZE = None, and the Caddy sidecar thatbin/enable_ssl.shinstalls setsrequest_body { max_size 0 }. So the limit always belongs to a reverse proxy the operator put in front of the device. Cloudflare caps request bodies at 100 MB on its Free and Pro plans, which a 4K video easily exceeds. The response is generated at the edge and never reaches Django, so there is noHX-Triggertoast to replay and the status code is the only signal.What this changes:
Transport failures are reported separately, and deliberately without a diagnosis. A proxy enforcing a body limit answers and closes while the browser is still writing the file, and the browser does not always salvage that response. Firefox drops it once enough of the body is still queued, where Chromium keeps it. A size rejection can therefore arrive indistinguishable from a dropped connection, so that message names both causes rather than guessing between them. When the status does survive, you still get the specific 413 message. We only hedge when the browser genuinely did not tell us.
Two things I deliberately did not do:
assets_uploadanswersENOSPCwith a 200 plus anHX-Triggertoast carrying the sharedDISK_FULL_ERRORstring, whichfireToastFromHeaderalready displays. Only the REST API returns 507, and this uploader never calls it. Adding one would have meant a second wording for a messageanthias_common.utilscentralises.HEADto the upload URL after a transport failure, to see whether the route was still alive) and then dropped it. It needs a timeout, a 5xx-is-unhealthy rule and anavigator.onLinecheck just to be correct, it still misfires on a recovered wifi blip or a mid-upload restart, and I never measured how often the 413 actually gets lost at realistic Cloudflare sizes. Naming both causes costs nothing and is never wrong.UploadResultbecomes a discriminated union to carry the status fromuploadOneup to the batch toast. Upload errors sit on screen for 8s rather than the 4s default, since they are the longest strings the store carries and a dismissed toast cannot be brought back.Testing
I ran a dev stack behind Caddy with a 1 MB
request_bodycap, which reproduces Cloudflare's behaviour at a size you can iterate on, and drove the real Add asset modal in both browsers:The same files on
masterall produce the old generic string.11 new unit tests (
bun test, 97 passing overall) cover the status mapping, the status surviving the trip up fromuploadOne, the toast lifetime, a batch aborting on the first transport failure without attempting the rest, and a partial batch still committing the rows that succeeded. I mutation-checked them: dropping the batchbreak, discarding the status, removing either mapping, dropping the toast lifetime, and skipping the success commit each make them fail.bunx tsc --noEmitis clean. No Python changed, so no server behaviour changed.Documentation
The reverse-proxy FAQ already warned that a proxy breaks uploads, but only for the rewritten
Hostheader and the CSRF rejection that follows. A body-size cap is the other way a proxy breaks uploads, and it looks different from the outside: everything works except large videos, so you have no reason to suspect your proxy. The second commit adds that case to the same answer, with the directive for nginx, Apache and Caddy, and a note that Cloudflare's ceiling is set by your plan and can be lowered but not raised.Checklist
Frontend and FAQ only, tested in Chromium and Firefox against a dev stack rather than on a Pi or an x86 device. I only have one Pi and it is currently being used all the time so I can't test it.
🤖 Generated with Claude Code