Skip to content

feat(upload): send large assets in chunks so they clear a proxy body limit - #3310

Open
mickzijdel wants to merge 30 commits into
Screenly:masterfrom
mickzijdel:feat/chunked-uploads
Open

feat(upload): send large assets in chunks so they clear a proxy body limit#3310
mickzijdel wants to merge 30 commits into
Screenly:masterfrom
mickzijdel:feat/chunked-uploads

Conversation

@mickzijdel

@mickzijdel mickzijdel commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Issues Fixed

No existing issue. This is the fix for the problem behind #3302: I run Anthias behind Cloudflare, and uploading a large video fails. #3302 made the failure legible ("File too large - it exceeds the upload size limit of the server or a proxy in front of it"); this makes the upload work.

Description

The Add asset upload posts the whole file in one request, so a large video fails wherever a reverse proxy caps the request body. Cloudflare caps every non-Enterprise plan at 100 MB, a video of a few minutes clears that easily, and the operator cannot raise it. The bytes have to arrive in several requests instead.

Server. A ranged upload carries Content-Range. Each request stages its bytes into <assetdir>/.uploads/<id>.part and answers with JSON; only the request carrying the final byte falls through to create the asset, which is then a rename within one filesystem rather than a copy. Type detection, the display name and the extension all run before staging, so a file Anthias will refuse is refused on the first chunk rather than after the operator has waited out the whole upload. A request without Content-Range takes the original path untouched.

Browser. A file larger than the chunk size is sliced and sent sequentially, each slice re-wrapped as a Blob carrying the file's own type. That last part matters: a raw Blob from File.slice() reports application/octet-stream, and the server reads the browser's type to catch a file whose extension lies about it, such as a HEIC renamed to .jpg. Without the re-wrap the chunked path would skip normalisation and the asset would render blank on the player.

Progress is measured against the whole file rather than each request. A dropped connection or a 5xx resends the same chunk twice before giving up; a 4xx is the server's considered answer and is not resent. The chunk that commits is never resent once its body has gone out — see known limitation 2.

Chunk size. upload_chunk_size_mb in ~/.anthias/anthias.conf, then the ANTHIAS_UPLOAD_CHUNK_SIZE_MB environment variable, then 16. The config file comes first because it is the rung that survives an upgrade: upgrade_containers.sh regenerates docker-compose.yml from its template on every run, so a value set anywhere on the host is not persistence. The environment variable is how a balena device sets it, through a dashboard variable the supervisor injects. Clamped to [1, 24]: the ceiling keeps a chunk under FILE_UPLOAD_MAX_MEMORY_SIZE, where Django holds it in RAM rather than spooling it to the SD card, and below the floor a large video becomes thousands of sequential requests.

Parsed defensively, because this is set once and rarely looked at again. 16m previously raised ValueError while settings imported and the container never came up — on a headless device, with no shell to work out why. Read as a decimal and rounded down, since MB is decimal and an operator writing 8.5 to fit a 10 MB cap must not be handed 16, a value that cannot clear the cap they are trying to get under.

Guards that exist because the failure they prevent is silent. A chunk is refused if it would seek past what is actually held: the alternative is a hole that reads back as zeros and an asset that is corrupt at exactly the right size. The check runs against the open file descriptor rather than the path, so the cleanup sweep cannot delete the file in between. The final chunk truncates to the declared total, so a retry that shrank cannot leave the tail of a longer earlier attempt behind. Range digits are bounded, because int() raises above 4300 digits and a client-controlled header must not reach a 500. Free space is checked on every chunk against total_bytes - held, not only on the first: total_bytes is re-read from each request and never pinned, so a declared total that grows would otherwise never be measured against the disk.

Partials live under the same one-hour deadline as every other stray file in the asset dir, and are excluded from backups, which tar that directory recursively and would otherwise carry gigabytes of a file nobody finished sending.

Known limitations

Worth stating plainly rather than leaving to be discovered:

  1. Chunks must arrive strictly sequentially, one in flight. What is tracked per upload is a byte count, not a set of received ranges, so a chunk starting past the end cannot be told apart from a resumed upload whose partial has gone. Both are treated as the latter: the partial is dropped and the operator asked to start over. Real resumability needs a received-ranges record and is not this change.
  2. A commit whose response is lost leaves the operator unable to tell whether the asset was created. The server renames the partial into place and then answers, so a lost reply is indistinguishable from a lost commit. The commit is no longer resent once its body has gone out — on a dropped socket or a 5xx, since behind a proxy the lost-commit case usually arrives as a gateway 502 rather than a socket error. A body that never finished sending cannot have committed, so that case is still retried and reported as the ordinary transport failure it is. Both the client's message and the server's 409 point at the asset list before anything else, and the modal closes so the list is visible. Removing the ambiguity itself needs a commit marker I built, could not demonstrate, and removed rather than ship protection nobody can rely on. The single-shot path has the same ambiguity and now reports it the same way.
  3. No fsync before the final rename, so power loss immediately after commit could leave the row pointing at unwritten data. Also true of the single-shot path today.

Testing

Verified through the real Add asset modal in Chromium, against a dev stack with the chunk size at 1 MB:

Source 4,195,538 bytes, sha256 10c63f2c9113aff7…
Stored asset 4,195,538 bytes, sha256 10c63f2c9113aff7…
Requests the server saw 5
Staging directory afterwards empty
Server tracebacks none

Byte-identical, through the browser, running the current bundle.

Unit tests cover the byte-exact multi-chunk round trip, ENOSPC while staging, the single-shot path staying unchanged when a stray upload id is present, rejection matrices for malformed ids and ranges (including the over-4300-digit total that used to 500), a chunk that lies about its length, a gap past the end of the partial, the final truncate, the free-space refusal on a growing total, and on the browser side the range arithmetic, the type re-wrap, the retry policy, the commit-is-never-resent rule and the error mapping.

2058 Python tests and 134 frontend tests pass, serially and under -n auto, with Redis unreachable — the no-Docker host recipe CLAUDE.md documents. ruff check, ruff format --check, mypy and tsc --noEmit are clean.

Not tested on hardware. My only Pi is in use, so the Raspberry Pi and x86 boxes on the checklist are genuinely unticked and nobody else has run this on a device either.

Since the last review

Six commits, in response to the review at 1cb0e31 and to problems found while re-auditing it.

The three findings from that review:

  • HX-Redirect was dropped across the refactor. fix(auth): answer htmx callers with HX-Redirect, not a 302 #3306 adds a check on that header, for the expired-session case, in the exact handler this branch rewrote — so whichever of the two rebased second would silently lose it, and every file over the chunk size would be dropped from the batch with the operator never sent to sign in. Now carried and acted on in both places a response is classified, in the same shape and wording as fix(auth): answer htmx callers with HX-Redirect, not a 302 #3306, so the rebase resolves to the same thing twice rather than to silence.
  • A rejection was assumed to come with a toast. fireToastFromHeader pushes nothing when there is no HX-Trigger, so an unrecognised 2xx dropped the file with no asset, no error and a completed progress bar. The test that pinned that behaviour asserted the toast list was empty, which is what it was describing: a toast that was not there.
  • Free space was checked only at byte 0, so a declared total that grew was never measured. Now per chunk.

And four found by re-auditing:

  • .tmp and .part files were served over HTTP. Three of the four transient shapes in the asset dir predate this branch — the REST API's <id>.tmp, the importer's .import-<hex>, and yt-dlp's <uuid>.<ext>.part — and all were fetchable on a LAN where the CIDR gate does not exclude clients.
  • An asset could be stored under a name we delete. A crafted multipart part named clip.tmp with Content-Type: video/tmp was stored as <uuid>.tmp, which the hourly sweep removes, leaving a row pointing at nothing.
  • The test suite wrote into the developer's real ~/anthias_assets and deleted its contents. Verified with a sentinel file, which did not survive one run.
  • The chunk size could not be set on the docker-compose install at all, which is the install this feature exists for.

Two corrections to what this PR previously claimed. int() tolerates surrounding whitespace, so the trailing-space example I gave for the defensive parsing never crashed anything — 16m did, and that alone carries the argument. And "the server clamps to [1, 24]" oversold it: the clamp bounds what the server advertises, not what arrives.

Note on overlap

This touches the same upload code as #3306, so the two conflict. The HX-Redirect handling above is deliberately identical in shape and wording to #3306's, so whichever merges second resolves to the same thing twice. Happy to rebase at any point.

Checklist

  • I have performed a self-review of my own code.
  • New and existing unit tests pass locally and on CI with my changes.
  • I have done an end-to-end test for Raspberry Pi devices.
  • I have tested my changes for x86 devices.
  • I added a documentation for the changes I have made (when necessary).

Tested end to end through a real browser against a dev stack, but not on hardware. The reverse-proxy FAQ entry that #3302 extended now documents the chunk size and where to set it.

🤖 Generated with Claude Code

mickzijdel and others added 3 commits August 20, 2026 15:57
The Add asset upload posts the whole file in one request, so a large
video fails wherever a reverse proxy caps the request body. Cloudflare
caps every non-Enterprise plan at 100 MB, which a 4K clip clears
easily, and the operator cannot raise it. The bytes have to arrive in
several requests instead.

A ranged upload carries Content-Range. Each request stages its bytes
into <assetdir>/.uploads/<stem>.part and answers with JSON; only the
request carrying the final byte falls through to create the asset,
which is then a rename within one filesystem rather than a copy.
Detection, the display name and the extension all run before staging,
so a file Anthias will refuse is refused on the first chunk rather
than after the operator waits out the whole upload. A request without
Content-Range takes the original path untouched.

Chunks for one upload must arrive strictly sequentially, one in
flight. What is tracked is a byte count, not a set of received
ranges, so a chunk starting past the end cannot be told apart from a
resumed upload whose partial has gone. Both are treated as the
latter: the partial is dropped and the operator asked to start over.
Real resumability needs a received-ranges record and is not this
change.

Several guards exist because the failure they prevent is silent. A
chunk is refused if it would seek past what is actually held, since
the alternative is a hole that reads back as zeros and an asset that
is corrupt at exactly the right size; the check runs against the open
file descriptor rather than the path, so the sweep cannot delete the
file in between. The staged name mixes in the session key, so knowing
another client's id is not enough to finish their bytes as your own
asset. An empty marker records a committed id, so retrying a request
that timed out after it succeeded cannot build a second asset. The
declared total is checked against free space with a margin before any
of it is written, because a player that fills its card stops being a
player.

ENOSPC while staging removes the partial and answers 507, matching
the single-shot path and the REST API. Range digits are bounded so a
client-controlled header cannot raise ValueError out of the view.
Chunks answer JSON with real status codes rather than the asset
table, which would otherwise re-render and fan out a websocket
refresh for an asset that does not exist yet.

Partials outlive the hourly sweep by a day, since an operator who
pauses a large upload should find their bytes still there. They are
excluded from backups, which tar the asset dir recursively and would
otherwise carry gigabytes of a file nobody finished sending. One test
fixture unlinked every entry in the asset dir and now handles a
subdirectory being there.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XQ7dFy1EnsBd5394K1UHTz
The server accepts a ranged upload; this is the half that produces
one. A file larger than the configured chunk size is sliced and sent
as sequential requests carrying Content-Range, each echoing the
upload id the previous response returned. Anything that fits in one
chunk takes the original single-request path untouched.

Each slice is re-wrapped as a Blob carrying the file's own type. A
raw Blob from File.slice() reports application/octet-stream, and the
server reads the browser's type to catch a file whose extension lies
about it, such as a HEIC renamed to .jpg. Without the re-wrap the
chunked path would skip normalisation and the asset would render
blank on the player, which is exactly the case
test_assets_upload_misnamed_heic_uses_browser_content_type exists to
prevent on the single-shot path.

Progress is measured against the whole file rather than each request,
so a large upload no longer runs from nought to a hundred once per
chunk. A dropped connection or a 5xx resends the same chunk twice
before giving up, since chunking turns one request into dozens and a
single blip should not lose an upload the operator has been waiting
on; a 4xx is the server's considered answer and is not resent.

Chunk size follows the path app_store_index_url already takes: an
environment variable, a Django setting, the template context and a
meta tag. It defaults to 16 MB and the client caps it at 24,
because above FILE_UPLOAD_MAX_MEMORY_SIZE Django spools the chunk to
/tmp, which is RAM on a stock Pi image. Sizing chunks to a proxy's
limit instead would cost that much memory per upload on a board that
may have 512 MB.

The range arithmetic lives in its own module because its failures are
silent: the server truncates to the declared total, so a wrong final
range or an off-by-one start yields an asset of exactly the right
size that will not play.

Verified end to end against a dev stack behind a proxy capped at 2 MB
with the chunk size set to 1 MB. The same 5 MB file sent as a single
request is refused with 413; sent through the Add asset modal it
arrives as five requests and the stored asset matches the source
sha256 byte for byte, with no partial left staged. Adds tests for the
round trip, disk-full while staging, the single-shot path being
unchanged when a stray upload id is present, and rejection matrices
for malformed ids and ranges. 2007 python tests and 114 frontend
tests pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XQ7dFy1EnsBd5394K1UHTz
Review of the chunked-upload branch found the client discarding every
message the server had been careful to word.

A chunk answers a full disk with 507 and the shared DISK_FULL_ERROR
text as JSON, but the message table matched `status >= 500` first and
told the operator to go read the device logs. Its comment still
claimed no 507 could reach the browser, which this branch had made
untrue. The server's own wording now passes through, and a 507 with
no readable body names the disk rather than the logs.

Type detection runs before staging, so a file Anthias refuses is
refused on the first chunk with the asset table and its own toast,
exactly as a single-shot upload is. The client could not parse that
as an acknowledgement, called it a transport failure and abandoned
the rest of the batch, where the unchunked path would have shown
"Invalid file type" and carried on. A 200 that is not an
acknowledgement is now one file rejected, toast replayed, batch
intact.

The upload id is minted client-side. The server used to mint it on
the first chunk, so a retry of that chunk after a lost response left
the staged bytes orphaned under an id the client never learned while
the upload silently continued under a second one.

Session-scoped staged filenames are gone. Nothing in the codebase
writes to the session and auth is off by default, so the salt was
always empty and the scoping was a no-op; with auth on it separated
only one operator's own tabs, while Django cycling the session key on
login would strand an upload mid-flight. It bought nothing and could
break a working upload.

The free-space margin is gone too. Refusing anything within 512 MB of
free space meant a device with 400 MB free could not take a 30 MB
video, and it reported the disk as full when it was not. Only an
upload that genuinely cannot fit is refused now. Partials return to
the same one-hour deadline as every other stray file: there is no
resume path, so holding one for a day only occupies the card, and the
offset guard already makes a swept partial fail loudly rather than
corrupt.

The commit marker that was meant to stop a replayed commit creating a
second asset is removed. Its test passed in isolation and failed in
the suite; made hermetic, it failed consistently, and the marker was
landing outside the asset dir the request was using. The guard did
not work. Shipping one that cannot be demonstrated is worse than
none, because everything downstream assumes the protection is there.
A retried commit can still duplicate an asset, as it can on the
single-shot path today; that belongs in the pull request text where
it can be weighed.

Adds tests for the length check, the free-space refusal and the final
truncate, all of which survived deletion before, and updates the
three tests that encoded the old contracts. 2010 python and 118
frontend tests pass, repeatably across consecutive runs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XQ7dFy1EnsBd5394K1UHTz
@mickzijdel
mickzijdel requested a review from a team as a code owner August 20, 2026 15:07
@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.63014% with 2 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (master@ba28647). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/anthias_server/django_project/settings.py 94.87% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master    #3310   +/-   ##
=========================================
  Coverage          ?   90.47%           
=========================================
  Files             ?       85           
  Lines             ?    10075           
  Branches          ?     1126           
=========================================
  Hits              ?     9115           
  Misses            ?      707           
  Partials          ?      253           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Codecov put the staged-upload sweep at a third covered, which is the
same gap review had flagged: nothing exercised the cleanup that
deletes abandoned partials. Its blast radius is what makes it worth
pinning. Sweeping too eagerly takes a partial out from under an
upload still in progress, and the operator loses a large upload to an
expiry error; not sweeping at all leaves the card filling with
partials nobody can reach from the UI. The test runs the real find
against one partial aged two hours and one just written, and asserts
the directory itself survives, since the next upload's first chunk
has nowhere to land otherwise.

Two error paths in the view were uncovered as well. If the filesystem
will not report free space the upload proceeds rather than being
refused, because a check that cannot run should not block a working
upload. And only ENOSPC becomes the disk-full answer: anything else
is a real fault and must surface, rather than telling the operator to
free up space that was never the problem.

The remaining line codecov lists is the rmtree branch of a test
fixture, which only runs once a chunked upload has left a directory
behind during teardown. Writing a test for that would be writing a
test about a test.

2013 python and 118 frontend tests pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XQ7dFy1EnsBd5394K1UHTz

@vpetersson-bot vpetersson-bot 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.

Reviewed as untrusted input, with attention to the things a hostile upload path would hide: traversal through the client-supplied id, symlink swaps on the staged file, holes in the reassembled file, and unbounded disk use. I found nothing malicious, and the guards you built are the right ones. I ran the branch locally: ruff check, ruff format --check and bun test (118 pass) are clean, and 644 Python tests pass — with two exceptions, below.

The integrity argument holds

This is the part I most wanted to break, so it is worth saying that I could not. I worked through the overwrite cases by hand: a retried chunk whose start < st_size, a deliberately tiny first chunk followed by a large final one, and a final chunk landing exactly at st_size. In every case seek(start) + write leaves the file at end + 1, the per-chunk file_upload.size check pins that to the declared range, and the start_bytes > fstat(fd).st_size guard makes a gap unrepresentable. The committed file is always exactly total_bytes with every byte accounted for. truncate(total_bytes) on the final chunk closes the shrinking-retry case. Measuring the open descriptor rather than the path, for the reason you give, is the right call.

The rest of the perimeter checks out too:

  • _UPLOAD_ID_RE.fullmatch on 32 lowercase hex closes traversal; O_NOFOLLOW closes the symlink swap; the 19-digit bound really does keep int() off its 4300-digit ValueError.
  • The orphan sweep in cleanup() uses os.scandir + is_file(), so .uploads is skipped rather than swept, and the existing *.tmp find cannot match *.part. The new .part sweep is the only thing that touches partials, and each chunk refreshes the mtime, so a slow upload is safe.
  • tarfile.add returns before recursing when the filter yields None, so _skip_staged_uploads genuinely prunes the subtree rather than just omitting the directory entry.

Findings

Two I would fix before merge and two smaller ones, all inline. Summarised:

  1. Two of the new tests fail without Redis — reproduced locally, and it breaks the no-Redis host recipe CLAUDE.md documents.
  2. The rationale for the chunk size is inverted. /tmp is not RAM in these containers, so staying under FILE_UPLOAD_MAX_MEMORY_SIZE forces each chunk into memory rather than keeping it out. The 16 MB number is still defensible; the reason given for it is not.
  3. int(getenv(...)) on a device variable can stop the server from starting.
  4. Known limitation 2 is described as the wrong failure. What the operator actually sees is an error, not a silent success — which changes how it should be documented.

On the overlap with #3306: no view from me on merge order, but the conflict is in assets_upload and home.ts, both of which this PR restructures substantially, so whichever lands second is a real rebase rather than a mechanical one.



@pytest.mark.django_db
def test_assets_upload_chunked_round_trip_is_byte_exact(

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.

These two tests need Redis, and the sibling tests in this file deliberately do not.

Reproduced on a host with no Redis running:

FAILED tests/test_template_views.py::test_assets_upload_chunked_round_trip_is_byte_exact
FAILED tests/test_template_views.py::test_assets_upload_final_chunk_truncates_a_longer_earlier_attempt
RuntimeError: Retry limit exceeded while trying to reconnect to the Celery result store backend.

The cause is the .mp4 payload: a video upload sets is_processing and the view dispatches normalize_video_asset.delay for real, which reaches for the Celery result backend. Every other video-upload test in this file wraps that in mock.patch('anthias_server.celery_tasks.normalize_video_asset.delay') — these two do not.

This matters beyond a local inconvenience: CLAUDE.md documents uv run pytest -m "not integration" on the host as a no-Docker, no-Redis run, and the root conftest.py force-mocks lib.utils.connect_to_redis but not Celery's broker. So the suite as documented now has two hard failures, while CI (which has Redis) stays green — the combination that leaves this kind of thing sitting for months.

Fix is either mock the delay like the neighbours do, or make the payload an image, since neither test is about the video pipeline. test_assets_upload_final_chunk_truncates_a_longer_earlier_attempt has the same problem at line 2853.

# production store index.
# Size of each request a large browser upload is split into. Kept
# under FILE_UPLOAD_MAX_MEMORY_SIZE above so a chunk is buffered in
# memory rather than spooled to FILE_UPLOAD_TEMP_DIR, which is /tmp

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.

/tmp is not RAM here, so this reasoning runs the wrong way.

The premise is true of Raspberry Pi OS on the host, but this code runs in a container, and I checked all four compose files — docker-compose.yml.tmpl, docker-compose.balena.yml.tmpl, docker-compose.dev.yml, docker-compose.test.yml. None mounts a tmpfs at /tmp for anthias-server (the only tmpfs-adjacent setting anywhere is shm_size on the viewer), and FILE_UPLOAD_TEMP_DIR is unset. So /tmp inside the container is the writable overlay layer, i.e. the SD card.

That inverts the trade-off: staying under FILE_UPLOAD_MAX_MEMORY_SIZE is what puts a chunk fully in RAM, via MemoryFileUploadHandler. Exceeding it is what would have spooled it to disk. So the chunk ceiling is not protecting the 512 MB board's memory — it is spending it, 16 MB per concurrent chunk request.

Worth being clear that this is a comment-and-rationale problem, not necessarily a code one. 16 MB resident per in-flight upload is a defensible number, and it is better than the status quo, where a 100 MB single-shot upload spooled the whole 100 MB somewhere. But the same wrong premise drives MAX_CHUNK_MB = 24 in chunking.ts, and a future maintainer reasoning from it will reach for the opposite of what they intend. Either correct both comments to say "buffered in RAM, so keep it small", or point FILE_UPLOAD_TEMP_DIR at the asset volume and size chunks to what proxies actually need.

# limit instead would cost that much memory per upload on a board
# that may have 512 MB. Lower it if a proxy in front of the device
# caps request bodies below this.
UPLOAD_CHUNK_SIZE_MB = int(getenv('ANTHIAS_UPLOAD_CHUNK_SIZE_MB', '16'))

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.

A typo in this device variable stops the server from starting.

This is the only int(getenv(...)) in settings.py, and it is unguarded: ANTHIAS_UPLOAD_CHUNK_SIZE_MB=16m (or 16 with a trailing space that a Balena variable field will happily keep) raises ValueError during settings import, and the container never comes up. On a headless device set through the Balena dashboard, the operator has no shell to work out why, and the value they typed is the sort of thing people set once and never look at again.

Two smaller things while you are here:

  • Values above 24 are silently ignored, because the cap lives only in chunkSizeFromMeta. An operator who reads the docstring and sets 32 to match their proxy gets 24 with no indication.
  • There is no lower bound either. 0.5 yields ~512 KB chunks, so a 2 GB video becomes ~4000 sequential requests, each with its own round trip and multipart parse.

Parsing defensively with a fallback to 16 and a warning, plus clamping to something like [1, 24] server-side, makes the client cap a second line of defence instead of the only one.

Comment thread src/anthias_server/app/views.py Outdated
# the sequential-only contract above.
if start_bytes > os.fstat(f.fileno()).st_size:
raise _ChunkedUploadError(
'This upload expired. Please try uploading it again.',

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.

This is the path that produces known limitation 2, and it does not fail the way the PR body describes.

The body says a commit that times out after succeeding "can produce a duplicate asset if the client retries". The mechanism is a bit different, and the difference matters for how it should be documented.

sendUpload resolves status: 0 on a dropped connection, and uploadOne treats status === 0 as retryable — including for the final chunk. So when the commit succeeds but the response is lost, the retry arrives after os.replace has already moved the partial away. os.open with O_CREAT recreates it empty, start_bytes > st_size fires here, and the client gets a 409.

What the operator sees is therefore "This upload expired. Please try uploading it again." for an upload that completed and created the asset. They do what the message says, and that is where the duplicate comes from — a second full upload, not a retried commit.

I would not ask you to build the marker you already removed. But the limitation is worth restating in these terms, because "may produce a duplicate" reads as a silent, harmless outcome, whereas the actual outcome is an error message that actively instructs the operator into the duplicate. If the message is all that is on offer, it could at least say the upload may have gone through and to check the list before retrying.

# Where chunked browser uploads stage their partial file, under the
# asset dir. Shared with the cleanup sweep and the backup filter so
# the name cannot drift from the code that writes into it.
STAGED_UPLOAD_DIR = '.uploads'

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.

Low severity, but worth a thought while the location is still cheap to change: .uploads sits inside anthias_assets, which is served over HTTP. views_files.anthias_assets resolves /anthias_assets/.uploads/<id>.part to a real path under ANTHIAS_ASSETS_ROOT, so the startswith guard passes and the file is served. That view's own comment says the DOCKER_BRIDGE_CIDR gate "does not actually exclude LAN clients" in the default no-SSL install, since REMOTE_ADDR is the bridge gateway.

In practice the 32-hex id is unguessable and there is no directory listing (IsADirectoryError becomes a 404), so this is not something I would hold the PR for. But partial uploads are the one thing in that tree that was never meant to be fetchable, and staging under ~/.anthias/ instead would take them off the HTTP surface entirely. Both live under the same mount, so the same-filesystem rename that makes the commit cheap still holds — it would cost the backup filter and the sweep path, and nothing else.

if (!retryable || attempt === CHUNK_RETRIES) break
await new Promise((r) => setTimeout(r, CHUNK_RETRY_DELAY_MS))
}
if (res === null) break

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.

Nit: this branch and the return at the end of the function are both unreachable. res is assigned on every iteration of the inner retry loop, which always runs at least once, so it is never null by the time you get here; and needsChunking guarantees at least two chunks, the last of which returns via interpretFinalResponse.

Harmless, but the trailing { kind: 'network' } reads like a real fallback for "ran out of chunks without committing", which cannot happen.



@pytest.mark.django_db
def test_assets_upload_final_chunk_truncates_a_longer_earlier_attempt(

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.

Besides the Redis dependency noted above, this test pins down a contract worth a second look: chunk one declares /40 and chunk two declares /10 for the same upload id, and the server accepts the change. total_bytes is never compared against what earlier chunks declared.

The offset and size guards mean nothing corrupt comes out of it — the committed file is exactly the new total, with every byte written — so this is not a bug. But no real client shrinks the total mid-upload, and encoding it in a test makes "the declared size may change between chunks" a supported behaviour that someone later has to preserve. If the intent is just to prove truncate works, driving it with a consistent total and a genuinely longer earlier attempt under the same total would test the same line without fixing the looser contract in place.

mickzijdel and others added 16 commits August 26, 2026 21:37
ANTHIAS_UPLOAD_CHUNK_SIZE_MB went straight through int(), so a value
like `16m` — or `16` with the trailing space a balena variable field
keeps — raised ValueError while settings imported and the container
never came up. On a headless device the operator has no shell to work
out why, and this is the sort of variable set once and never looked at
again.

Parse it the way resolve_time_zone parses TZ: fall back to the default
and say so, rather than letting any value wedge Django at startup.
Clamp to [1, 24] while here. The ceiling matched the browser's
MAX_CHUNK_MB but lived only there, so an operator who set 32 to match
their proxy silently got 24; there was no floor at all, and 0.5 turns
a 2 GB video into ~4000 sequential requests.

Also moves the block below APP_STORE_INDEX_URL — it had landed between
that constant and the comment explaining it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both complete an upload with an .mp4 payload, so the view dispatches
normalize_video_asset for real and Celery reaches for its result
store. Every other video-upload test in this file mocks that; these
two did not, so they failed on any host without Redis:

    RuntimeError: Retry limit exceeded while trying to reconnect to
    the Celery result store backend.

CLAUDE.md documents `uv run pytest -m "not integration"` as a
no-Docker, no-Redis run, and conftest force-mocks connect_to_redis but
not Celery's broker — so the suite as documented had two hard
failures while CI, which has Redis, stayed green.

Reproduced against an unreachable broker (39.9s, both failing) and
confirmed fixed the same way (0.7s, both passing).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment claimed a chunk over FILE_UPLOAD_MAX_MEMORY_SIZE would be
spooled to /tmp, "and therefore RAM on a stock Pi image". That is true
of Raspberry Pi OS on the host, but this code runs in a container, and
none of docker-compose.yml.tmpl, .balena.yml.tmpl, .balena.dev.yml.tmpl,
.dev.yml or .test.yml mounts a tmpfs at /tmp for anthias-server;
FILE_UPLOAD_TEMP_DIR is unset. So /tmp there is the writable overlay,
i.e. the SD card.

Which inverts the argument: staying under the limit is what puts each
chunk fully in RAM via MemoryFileUploadHandler. 16 MB resident per
in-flight upload is still the number I want — trading RAM for card
writes is the wrong way round on this hardware — but a maintainer
reasoning from the old comment would reach for the opposite of what
they intended. Same wrong premise was copied into MAX_CHUNK_MB.

Comments only; no behaviour change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The final chunk commits: the server renames the partial into place and
then answers. If that answer is lost, sendUpload reports status 0,
which uploadOne treated as retryable — so the commit was resent, found
the partial already moved away, recreated it empty via O_CREAT, and
tripped `start_bytes > st_size`. The operator was then told "This
upload expired. Please try uploading it again." for an upload that had
worked, did what the message said, and that second upload is where the
duplicate came from.

Two changes, one to each half:

  * a lost response is only retried for a staging chunk, which is
    idempotent — the commit is never resent. It reports a new
    `unconfirmed` failure instead, whose message points at the asset
    list rather than asking for another upload.
  * the 409 says the upload could not be resumed and to check the
    list first, since a lost commit is one of the ways to reach it.

Does not eliminate the duplicate — that needs the commit marker this
branch tried and dropped — but it stops the UI instructing the
operator into it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`.uploads/<id>.part` sits inside anthias_assets, and anthias_assets
resolves any path under ANTHIAS_ASSETS_ROOT and serves it — the
startswith guard passes fine for a partial. That view's own comment
notes the DOCKER_BRIDGE_CIDR gate does not exclude LAN clients in the
default no-SSL install, so a partial upload was fetchable by anything
on the network. The 32-hex id is unguessable and there is no listing,
so this was never urgent, but partials are the one thing in that tree
that was never meant to be fetchable.

Refuse any dot-leading path component instead of naming the staging
directory: uploaded assets are always <uuid>.<ext>, so nothing
legitimate is lost, and whatever lands there next is covered without
this list having to be kept in sync.

Not by relocating the staging dir to ~/.anthias, which was the
obvious fix and does not work: docker-compose.yml.tmpl bind-mounts
/data/.anthias and /data/anthias_assets separately, and rename(2)
across two mounts is EXDEV even when they share a filesystem.
Reproduced with those two mounts in a container:

    rename FAILED: [Errno 18] Cross-device link

The commit would have become a full copy of a multi-GB file onto an SD
card, needing twice the free space. Recorded next to STAGED_UPLOAD_DIR
so the next person does not try it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`res` was assigned on every iteration of a retry loop that always runs
at least once, so the `res === null` break was unreachable; and
needsChunking guarantees at least two chunks, the last of which
returns, so the trailing `{ kind: 'network' }` was too. It read like a
real fallback for "ran out of chunks without committing", which cannot
happen.

Both existed to satisfy the nullability of a `let res` declared
outside the retry loop. Lifting the loop into sendWithRetry, which
returns a response or keeps trying, removes the need for either. The
chunk loop then splits along the seam that was already there: the
staging chunks, which only add bytes, and the last one, which commits.
No isFinal threading through the body of the loop, and nothing left
after it for TypeScript to worry about.

Also names the request shape (UploadRequest) that the interface and
sendUpload each spelled out in full.

bun test 121 pass, tsc --noEmit clean.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The test drove truncate by declaring `/40` on the first chunk and
`/10` on the second for the same upload id. The server accepts that —
total_bytes is never compared against what earlier chunks declared —
and the guards mean nothing corrupt comes of it, so it is not a bug.
But no real client shrinks a total mid-upload, and asserting on it
made "the declared size may change between chunks" a supported
behaviour someone later has to preserve.

Same line covered with a consistent total: a stale partial left under
the id, longer than the file now being sent, and one chunk declaring
its own file's real size. Still fails with the truncate removed —
b'1'*10 + b'0'*30 instead of b'1'*10.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Same facts, fewer lines — the prose had grown to where the reasoning
was harder to find, not easier. Nothing dropped: every constraint,
gotcha and cross-reference the comments carried is still there, said
shorter.

Two things beyond wording:

  * the sequential-only contract was written out in full twice, in the
    _stage_upload_chunk docstring and again above planChunks. The
    docstring keeps it; chunking.ts points at it.
  * two comments said an abandoned partial is "held for a day". It is
    held for STAGED_UPLOAD_MAX_AGE_MIN, which is an hour. Left over
    from an earlier version of the sweep.

Verified comment-only: the Python ASTs are identical with docstrings
excluded, and home.js builds byte-for-byte the same bundle.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
parseServerError and parseUploadId were the same eight lines with a
different key.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ANTHIAS_UPLOAD_CHUNK_SIZE_MB existed only in settings.py and its test.
docker-compose.yml.tmpl lists anthias-server's environment explicitly
and Compose passes only what the file declares, so on the plain
docker-compose install — Raspberry Pi OS and x86, the install this PR
exists to fix — the container never saw the variable at all. Setting
it did nothing. Only balena worked, where the supervisor injects
device variables into every service regardless.

Declaring it in the template is half a fix: upgrade_containers.sh
regenerates docker-compose.yml from the template on every run and
passes -f explicitly, so an edit to the generated file or a
docker-compose.override.yml is reverted by the next upgrade. An
operator behind an nginx with client_max_body_size 8m would have fixed
their uploads, upgraded a month later, and had the 413s come back with
nothing connecting the two events.

So the script now also sources /etc/anthias/anthias.env before
envsubst, the same way it already sources /etc/anthias/proxy.env for
GH Screenly#3239. Unlike proxy.env it is not ansible-managed — it is where an
operator's own settings live, absent by default.

Documented in the reverse-proxy FAQ, next to the body-limit table it
belongs with, which is what the PR offered to do.

Both halves are tested, since neither is exercised by anything else:
removing the environment line or the sourcing fails the new tests.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous round exempted the final chunk from being resent on a
dropped socket, and the PR body said flatly "the commit is no longer
resent". Only half true: `retryable` exempted `status === 0` and left
the 5xx arm alone, and behind a proxy the lost-commit case usually
arrives as a gateway 502 or 504, not a socket error. So the common
shape of it was still resent — twice — into the 409 that says the
upload cannot be resumed, for an upload that had worked.

Worse, that 409's careful wording never reached the operator anyway.
Only the staging loop read the server's JSON error; the commit went
straight to interpretFinalResponse, which reads the status and not the
body, so the operator saw "Upload failed — check the file and try
again". The one message written to stop a duplicate was thrown away on
the one chunk that can produce one, and the 507 story upload-error.ts
describes had the same hole.

The discriminator is whether the request body finished going out.
XMLHttpRequest already knows — `xhr.upload`'s load event — and it
separates the two cases that were being conflated: a connection cut
mid-body cannot have committed anything, so it is safe to resend and
is reported as the plain transport failure it is; a body that went out
with nothing intelligible coming back may have committed, so it is
never resent and says so.

The stub modelled none of this — it fired no upload events at all, so
every simulated failure looked like a mid-body cut. It now streams the
body first, which is also what let the mid-commit case be tested.

Also closes the Add modal on that outcome: "check the asset list"
is not actionable with the modal sitting on top of the list.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The PR body claimed "both the browser and the server clamp it to
[1, 24]". The browser only ever had the ceiling. That matters twice:

  * a hand-edited meta tag below ~0.000001 MB floors to 0 bytes, and
    then needsChunking says to split the file while planChunks returns
    no chunks at all — `chunks[chunks.length - 1]` is undefined and
    uploadOne throws, silently, before a single request goes out.
  * the frontend tests drove the whole chunked path at 0.001 MB, a
    configuration resolve_upload_chunk_size_mb can no longer produce.
    The retry and commit policy — the part of this branch most worth
    getting right — was only ever exercised in a state no device can
    reach.

So the tests now run at 1 MB chunks over a 2.5 MB file, which is the
smallest a real device will hand the browser.

While there: the ranges test asserted three literal byte ranges that
follow from the chunk size, so it broke on that change without
anything being wrong. It now asserts what actually has to hold —
contiguous, starts at 0, ends at the last byte, one consistent total —
since a gap reads back as zeros and an off-by-one truncates.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`int(raw)` rejected `8.5`, and the fallback handed the operator 16.
MB is a decimal quantity, so 8.5 is a plausible thing to write, and
the person writing it is doing so because their proxy caps bodies at
10 MB. Falling back to 16 gives them a value that cannot clear the cap
they were trying to fit under — every chunk 413s and the upload fails
exactly as it did before this branch, with a warning line they will
never see. Every other knob on the device fails toward "still works";
this one failed toward "still broken".

Parsed as a float and floored, so 8.5 gives 8. float() also absorbs
nan, inf and the thousand-digit integer that the digit bound in
_CONTENT_RANGE_RE exists to keep away from int(), so the unparseable
path is narrower than it was, not wider.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The range, length and empty-file checks all raised before the `try`,
so the `except _ChunkedUploadError` that removes the partial only ever
fired for the 409 from the offset guard. A rejected chunk therefore
left everything staged so far — up to (n-1) × chunk_size of a file
nobody will finish — sitting on the card for an hour, invisible in the
UI, on a device where the whole reason for this feature is that the
card is small.

The client abandons an upload on any 4xx and starts over under a fresh
id, so those bytes are unreachable the moment they are refused. The id
is now resolved before anything that can fail, which is what lets the
partial be named and dropped; a malformed id is the one case with
nothing to clean up, since there is no name to derive.

Also corrects the 409's comment. It named a lost commit as the way to
reach it — the one path our own client now rules out. The reachable
causes are the sweep taking a partial after an idle hour, and a
third-party client that does resend a commit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment claimed `.uploads/<id>.part` was "the one thing here never
meant to be fetchable". It is not. Two other kinds of transient file
already live in the asset dir:

  * `.import-<hex><ext>` and its `.part`, staged by the content
    importer at up to 5 GiB — dot-leading, so the new rule caught them
    by accident rather than design.
  * `<upload_id>.tmp`, staged by the REST API's own resumable uploads
    — not dot-leading, and still served to anything on the LAN.

Both predate this branch, but a comment asserting the directory is now
clean is worse than no comment: it tells the next reader not to look.
So the claim is gone, the list is written down, and `.tmp` is refused
alongside. Nothing durable is named that way — the celery sweep
already deletes stale `*.tmp` from this directory outright, which only
works because assets are always `<uuid>.<ext>`.

The staging test also hardcoded `.uploads` while the code read the
constant, so renaming STAGED_UPLOAD_DIR left it passing against a
directory nothing writes to while real partials went back to being
served. It builds the path from the constant now, and fails if the two
come apart.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_skip_staged_uploads pruned `.uploads/` and nothing else, so the two
staging files that already lived in the asset dir still went into
every backup and every streamed download: the REST API's
`<upload_id>.tmp`, and the content importer's `.import-<hex>` and its
`.part`, which allows 5 GiB. A backup taken mid-import carried the
whole thing — the exact failure the filter exists to prevent, one
directory over.

It also had no tests. Coverage counted the function as covered because
`tar.add` calls it on every member, which is the kind of green that
means nothing: removing `filter=` from both call sites left all ten
backup tests passing. Four cases now, one per staging file, each
asserting a real asset alongside still makes it in — and all four fail
without the filter.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mickzijdel and others added 3 commits August 27, 2026 01:09
Each of these passed with the code deleted.

**The meta tag.** Removing the one line in helpers.py that puts the
chunk size into the template context left all 2028 Python tests green,
while every device fell back to the browser's own 16 MB default and
ignored whatever the operator configured. It is the only link carrying
the setting to the client, and now it is asserted end to end, the way
the app-store index next to it already was.

**The two sets of bounds.** chunking.ts cannot import a Python
constant, so [1, 24, 16] is written out twice with nothing tying the
copies together. Changing the server's ceiling to 20 left everything
green — and silently resurrects the bug the server-side clamp was
added to prevent, since a stale browser ceiling overrides the server's
answer. A test reads the three literals out of chunking.ts and
compares them.

**The rejection rule.** The server refuses some files with 200 plus an
error toast rather than a status code, and this branch went to some
trouble to keep the chunked path treating that as a refusal. No test
supplied an HX-Trigger header at all — the stub returned null for
every header — so `kind === 'error' ? 'rejected' : 'ok'` could be
replaced with `'ok'` and stay green, closing the modal and firing a
table refresh for an asset that does not exist.

Also stops setChunkSizeMb assigning over document.head. It wiped the
date-format metas home.ts reads and, since bun runs every file in one
process, leaked the chunk size into later tests. Three batch tests
passed only because their fixture file happened to be one byte long:
raising it to 2000 broke them before this change and does not now.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
"The server mints nothing now" — it does. views.py still mints an id
for any chunk that arrives without the header; what changed is that
our own client stopped relying on it.

"The client echoes the id" — it mints its own. The distinction matters
where that comment sits, on the regex that has to survive whatever a
third-party caller sends.

And screenly_migration's cross-reference to "the same guard as
views_files.anthias_assets" stopped being the same guard when that
view gained rules about transient staging files.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`cleanup_asset_dir` emptied `settings['assetdir']` on teardown. That is
one directory — ~/anthias_assets — shared by every xdist worker, so
under the `pytest -n auto` CI runs one worker's teardown deleted files
another worker was still mid-request on. It surfaced once here as
test_file_asset_upload_id_ignored_without_content_range failing in a
full run and passing in isolation, and three consecutive full runs
afterwards were clean, which is what a race looks like.

It also emptied the directory on a developer's own machine. Verified:
a sentinel file placed in ~/anthias_assets does not survive running
this one test file. That is the hazard tests/test_backup_helper.py's
fixture already goes out of its way to avoid, quoting the same reason.

Each test now gets its own asset dir under tmp_path, which pytest
cleans up, so there is nothing to empty and nothing shared.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@vpetersson-bot

Copy link
Copy Markdown
Contributor

Reviewed at 1cb0e31. The staging protocol and its integrity guards I could not break, and the line-by-line audit from the earlier round still holds at this head. My findings are all at the edges: two in how the browser classifies a response, one in a guard that reads stronger than it is.

Verification

Ran on the host with no Docker and no Redis, the recipe CLAUDE.md documents:

uv run pytest -m "not integration" 2047 passed, 63 deselected
Same, -n auto 2047 passed
bun test 128 passed
ruff check / ruff format --check clean
mypy no errors in any file this PR touches

Both regressions from the last round are genuinely fixed: the two round-trip tests mock normalize_video_asset.delay now, so the documented no-Redis host run is green, and the -n auto run is clean, which is what the shared-asset-dir fixture was about.

1. HX-Redirect is dropped on the chunked path, and #3306 is where that starts to matter

This is the one I would fix before merging, because it is a rebase hazard rather than a visible bug today.

#3306 — which you already note conflicts with this — adds an HX-Redirect check to sendUpload's load handler for exactly the expired-session case: without it a 302 is followed transparently, the login page arrives under a 200, and that reads as a stored file. It resolves { status: 'error', failure: { kind: 'auth' } } straight out of that handler.

This PR refactors that handler so it no longer produces an UploadResult — it produces a RawUploadResponse carrying status / text / trigger / bodySent, and the classification moves out to interpretFinalResponse and the chunk loop. HX-Redirect is not among the fields carried across. So whichever of the two rebases second, resolving the conflict mechanically leaves the chunk loop unable to see the header at all:

if (jsonStringField(res.text, 'upload_id') === undefined) {
fireToastFromHeader(res.trigger)
return { status: 'rejected' }
}

A non-final chunk answering 204 + HX-Redirect: /login/ has no upload_id in its empty body, so it becomes { status: 'rejected' }. The file is dropped from the batch, the loop moves on to the next file and drops that one too, and the operator is never sent to sign in. #3306's fix would silently stop applying to every file larger than the chunk size — which is precisely the set of files this PR exists to serve.

The fix is small: add redirect: string | null to RawUploadResponse, populate it in sendUpload, and act on it in both the chunk loop and interpretFinalResponse. Worth doing on this branch now rather than leaving it to conflict resolution, where it will not surface as a conflict.

2. The chunk loop assumes a rejection always comes with a toast

Same code, independent of #3306:

if (jsonStringField(res.text, 'upload_id') === undefined) {
  fireToastFromHeader(res.trigger)
  return { status: 'rejected' }
}

fireToastFromHeader returns null and pushes nothing when there is no HX-Trigger. So any 2xx that is not the acknowledgement and carries no toast — a login page, a captive-portal interstitial, a proxy's own 200 — drops the file with zero user-visible feedback: no asset appears, no error, and the progress bar completes normally.

interpretFinalResponse already draws this distinction:

return { status: kind === 'error' ? 'rejected' : 'ok' }

The chunk loop does not. The new test a non-JSON 200 is a rejection, not a transport failure pins the silent behaviour — its stub is { status: 200 } with no trigger, and it asserts toasts is empty, so the comment's "the server's own toast stands alone" is describing a toast that is not there.

Branching on the returned kind the way the final-response path does is a one-liner and closes the tail of finding 1 too.

Worth noting this is also the only thing standing between the operator and a silent bad outcome if a proxy ever strips Content-Range from the request: the server then takes the single-shot path — as designed, and as test_assets_upload_single_shot_ignores_a_stray_upload_id asserts — and stores the first chunk as a complete asset. A truncated video with an error toast is recoverable; a truncated video in silence is not.

3. The free-space guard only runs on the chunk starting at byte 0

_stage_upload_chunk gates the shutil.disk_usage check on start_bytes == 0, and total_bytes is re-read from each request without being pinned. A client whose declared total grows after the first chunk is never checked against the disk at all.

Confirmed rather than reasoned — I drove the real view with disk_usage mocked to 100 bytes free:

chunk 0:  bytes 0-9/50      ->  200   (50 < 100 free, passes)
chunk 1:  bytes 10-299/300  ->  200   (no check runs; asset committed at 300 bytes)

control:  bytes 0-9/300     ->  507   (declared up front, correctly refused)

Not a security issue — an authenticated operator can fill the card with single-shot uploads regardless — but the code and the PR body both read as though the refusal is a property of the upload, and it is a property of the first request only. Running the check on every chunk against total_bytes - os.fstat(f.fileno()).st_size is stateless, costs one statvfs per chunk, and needs no received-ranges record.

This is also the concrete consequence of the "the declared total may change between chunks" contract raised last round. That was answered by tightening the test; the looseness in the code is what produces the above. I would not ask you to pin the total — that needs state this design deliberately avoids — but the per-chunk disk check gets most of the value for free.

Smaller things

  • docker-compose.dev.yml does not pass ANTHIAS_UPLOAD_CHUNK_SIZE_MB through, so reproducing the fix in the dev stack — the setup you tested against — needs a file edit. One line, and it makes the FAQ recipe exercisable by the next person.
  • .uploads is created but never removed, so an install that once had a chunked upload keeps an empty directory forever. Cosmetic.

On the rest

I spent most of my time trying to make the reassembly produce a wrong file and could not. The fstat on the open descriptor rather than the path, O_NOFOLLOW on a client-named path, the 19-digit bound keeping int() off its ValueError, the final truncate, and os.replace within one directory are each the right call. The EXDEV argument for why .uploads cannot live under ~/.anthias is correct — separate bind mounts make the commit a copy, and on this hardware that is the wrong trade.

Two judgement calls worth recording as correct rather than merely accepted. The 24 MB ceiling is right for the stated reason: FILE_UPLOAD_MAX_MEMORY_SIZE is 26_214_400, and a 24 MB chunk plus multipart overhead lands under it with about 1 MB of headroom, so MemoryFileUploadHandler keeps it in RAM. And rounding a fractional size down is right — this knob only ever exists to get under someone's cap, so every fallback has to go that way.

The known limitations are stated honestly, and limitation 2 in particular now describes what the operator actually sees rather than a vague "may produce a duplicate". Not resending the commit once its body has gone out — including on a 5xx, since behind a proxy that is the common shape — is the right resolution given that removing the ambiguity needs a marker you could not demonstrate.

Recommendation: finding 1 before merge, since it will not announce itself during the rebase with #3306. Findings 2 and 3 are small enough to fold into the same pass. Nothing here touches the staging protocol itself.

One caveat on scope: I reviewed against a dev checkout and CI, not hardware — your own checklist leaves the Pi and x86 boxes unticked, so the end-to-end device path is still unverified by anyone.

mickzijdel and others added 7 commits September 1, 2026 16:53
ANTHIAS_UPLOAD_CHUNK_SIZE_MB reached the container on balena, where the
supervisor injects dashboard variables into every service, and nowhere
else: docker-compose.yml.tmpl lists anthias-server's environment
explicitly and Compose passes only what it declares. On the plain
docker-compose install — the one this feature exists for — setting it
did nothing.

Declaring it in the template is half an answer, because
upgrade_containers.sh regenerates docker-compose.yml from that template
on every run. An operator behind an nginx with client_max_body_size 8m
would have fixed their uploads, upgraded a month later, and had the
413s come back with nothing connecting the two events.

So the durable rung is anthias.conf, which lives on the mounted volume
and is where timezone, rotation and audio output already live.
Precedence mirrors resolve_time_zone exactly: config file, then the
env var, then 16.

Parsed defensively, because this is set once through a dashboard field
and never looked at again. A stray unit (`16m`) previously raised
ValueError while settings imported, and the container never came up —
on a headless device, with no shell to work out why. Read as a decimal
and rounded down: MB is decimal, and an operator writing 8.5 to fit a
10 MB cap must not be handed 16, a value that cannot clear the cap they
are trying to get under. Every fallback goes the same way.

Both config readers refuse anything that is not a plain file. A FIFO at
~/.anthias/anthias.conf blocked the import forever and the container
never started; a character device read until the process died. The
timezone reader is fixed alongside because it has the identical shape
and is read on every request.
`cleanup_asset_dir` emptied `settings['assetdir']` on teardown. That is
one real directory — ~/anthias_assets on a developer's machine — shared
by every xdist worker, so under `pytest -n auto` one worker's teardown
deleted files another was mid-request on. It also deleted a developer's
actual assets: verified with a sentinel file, which did not survive a
single run of that test file.

Isolating it is less obvious than it looks. `assetdir` is stored in
anthias.conf as an absolute path and `AnthiasSettings.load()` rebuilds
the key from that file — and `save()` ends in a `load()` — so patching
the dict is undone by the next settings-saving test in the same worker.
Patching `home` does not help either: `path.join(home, '/abs/path')`
discards `home` when the second argument is absolute.

So the singleton is pointed at a temp config carrying the temp paths,
written from the values already in memory. Every later `load()` then
resolves back into the temp directory, including the one in
`_isolated_settings_conf`'s teardown, which restores whatever
`conf_file` it finds rather than a remembered real path. `database` is
redirected alongside `assetdir` — `_get` rebuilds both the same way —
and the secret key is blanked rather than copied into a temp tree.

Verified in both execution modes, because the leak was invisible in
one: 6 files per serial run of test_template_views.py, 0 under
`-n auto`, where xdist happened to split the file favourably.
`anthias_assets` resolves any path under ANTHIAS_ASSETS_ROOT and serves
it, and that directory holds four kinds of transient file, three of
which predate chunked uploads:

  * `.uploads/<id>.part`   — a chunked browser upload
  * `<upload_id>.tmp`      — a resumable REST API upload
  * `.import-<hex><ext>`   — a content import, and its `.part`
  * `<uuid>.<ext>.part`    — a yt-dlp or remote-video download

All were fetchable. The view's own comment notes the DOCKER_BRIDGE_CIDR
gate does not exclude LAN clients in the default no-SSL install, so a
half-downloaded video was readable by anything on the network. The ids
are unguessable and there is no directory listing, so this was never
urgent — but these are the files in that tree that were never meant to
be handed out.

Refuses a dot-leading path component or a `.tmp`/`.part` suffix rather
than naming each shape, so whatever lands there next is covered. Nothing
durable is named that way: `_safe_ext` refuses those extensions
precisely so the celery sweep, which deletes `*.tmp` on sight, and the
backup filter cannot swallow a real asset.

The tests build their paths from STAGED_UPLOAD_DIR rather than
hardcoding `.uploads`, so renaming the constant fails them instead of
leaving them passing against a directory nothing writes to.
`tar.add` recurses, so a backup taken mid-upload carried whatever was
in flight. The filter pruned `.uploads/` and nothing else, which left
the two staging files that already lived in the asset dir: the REST
API's `<upload_id>.tmp`, and the content importer's `.import-<hex>` and
its `.part`, which allows 5 GiB. A backup taken mid-import carried the
whole thing — the exact failure the filter exists to prevent, one
directory over.

Scoped to the asset dir. `.anthias` is in the same archive and holds
the config and the database, where an atomic-write sidecar is a
plausible future name, and silently dropping one of those from a backup
is the worst outcome available here.

It also had no tests. Coverage counted the function as covered because
`tar.add` calls it on every member, which is the kind of green that
means nothing: removing `filter=` from both call sites left all ten
backup tests passing.
Three things the staging path got wrong, all found by review after the
integrity argument itself had held up.

**A rejected chunk left its partial on disk.** The range, length and
empty-file checks all raised before the `try`, so the `except` that
removes the partial only ever fired for the 409 from the offset guard.
Up to (n-1) x chunk_size of a file nobody will finish sat on the card
for an hour, invisible in the UI. The client abandons an upload on any
4xx and starts over under a fresh id, so those bytes are unreachable
the moment they are refused. The id is resolved before anything that
can fail, which is what lets the partial be named and dropped.

**Free space was checked once, at byte 0.** `total_bytes` is re-read
from each request and never pinned, so a client whose declared total
grows after the first chunk was never measured against the disk at all
— the refusal read as a property of the upload while being a property
of its first request. Now checked on every chunk against
`total_bytes - held`, which costs one statvfs and needs no state. That
covers concurrent uploads too, since a neighbour's writes shrink the
free count on their own.

**An asset could be stored under a name we delete.** `_safe_ext`
accepted any `.<alnum>` from the client's filename, so a crafted
multipart part — filename `clip.tmp`, Content-Type `video/tmp` — was
stored as `<uuid>.tmp`, which the hourly sweep removes an hour later,
leaving a row pointing at nothing. `.part` was quieter and worse: it
survived, served, and was dropped from every backup. A browser cannot
reach it (it sends application/octet-stream, which the type gate
refuses), but the invariant the serving refusals rest on has to be
true, not nearly true.

No bound on the size of an arriving chunk, deliberately. One was tried
and removed: the rationale was that a chunk over
FILE_UPLOAD_MAX_MEMORY_SIZE would be held in RAM, and it is not — those
spool to disk. What is left after the free-space check is a chunk that
fits, writing no more than an ordinary single-shot upload of the same
file, so the bound rejected working clients to prevent nothing.
Four corrections to how the browser reads a response, three of them
cases where an earlier fix handled one branch and left its sibling.

**`bodySent` was hardcoded true on the response path** while the error
path read the real flag — so the one case the flag exists to catch was
the one case it could not report. A proxy enforcing a body limit
answers and closes while the browser is still writing, and that
response arrives with `xhr.upload`'s load event never having fired. It
was being classified as "the commit may have gone through": not
retried, though it would have succeeded, and the operator sent to hunt
for an asset that provably cannot exist.

Reading a committing response now lives in one function, used by both
the single-shot path and the final chunk. That extends the ambiguity
handling to single-shot, which needed it more: it commits identically —
write the file, create the row, answer afterwards — and it is the path
every upload under the chunk size takes. It was still saying "check
your connection, or try a smaller file" for uploads that had worked.

**HX-Redirect was dropped across the refactor.** The load handler used
to return an UploadResult and now returns a RawUploadResponse, and the
header was not among the fields carried over. The htmx-auth-redirect
work adds a check on exactly that header, for exactly the
expired-session case, in exactly this handler — so whichever of the two
rebases second, resolving mechanically leaves the chunk loop unable to
see it, and every file over the chunk size is dropped from the batch
with the operator never sent to sign in. Carried and acted on in both
places a response is classified, in the same shape and wording as that
work, so the rebase resolves to the same thing twice rather than to
silence.

**A rejection was assumed to come with a toast.** `fireToastFromHeader`
pushes nothing when there is no HX-Trigger, so any 2xx that was not the
acknowledgement and carried no toast dropped the file with no asset, no
error, and a progress bar that completed — a login page, a captive
portal, a proxy's own 200.

**A 413 on a chunk blamed the file.** Right for a single-shot upload
and wrong once the file has been split: splitting it was the remedy,
and the operator's only move is a smaller chunk.
`anthias-server` already had an `environment:` block, so adding a
second one gave the mapping a duplicate key. `yaml.safe_load` accepts
that and keeps the last value, which is why the local check passed;
Compose's own parser refuses it outright:

    line 27: mapping key "environment" already defined at line 14

That took down the OpenAPI schema job, which builds the dev stack.
Validated with `docker compose config` this time rather than a
permissive YAML parser — and the same check run over the production
template and the test compose file.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sonarqubecloud

sonarqubecloud Bot commented Sep 1, 2026

Copy link
Copy Markdown

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.

2 participants