DRAFT: fix(fal_client): upload every part in sync multipart save

[jim-fal]

What was wrong

MultipartUpload.save — the sync, in-memory-bytes path in projects/fal_client/src/fal_client/client.py — assigned each chunk back into the name holding the source buffer:

for part_number in range(1, parts + 1):
    start = (part_number - 1) * multipart.chunk_size
    data = data[start : start + multipart.chunk_size]   # rebinds the source
    futures.append(executor.submit(multipart.upload_part, part_number, data))

After the first iteration data is no longer the payload, it is the payload's first chunk. Part 1 is correct. From part 2 on, start is already past the end of the now-truncated buffer, so the slice is empty and every remaining part is uploaded as zero bytes.

Running the shipped loop offline against a 105 MB payload (11 parts) produces [(1, 10485760), (2, 0), (3, 0), ..., (11, 0)] — 10 MB of 105 MB. Against the real CDN the upload does not silently truncate, it fails hard: FalClientHTTPError: Content length appears to be zero - refusing to proceed.

Why it was never noticed

The path is only reachable from sync SyncClient.upload() / upload_image() when the payload is in-memory bytes larger than MULTIPART_THRESHOLD (100 MB) and the repository is fal_v3. That is a rare combination — uploads that large normally come from a file path, and every neighbouring code path is correct:

• MultipartUpload.save_file re-opens the file and seeks per thread, so there is no shared name to clobber.
• AsyncMultipartUpload.save binds the slice to a separate chunk.
• The fal.toolkit multipart providers slice file.data into a new local, which does not rebind the source.

So the defect is confined to this one loop, and has been there since the feature was introduced in 9a29db9a (#413).

The fix

Bind the slice to chunk, exactly as AsyncMultipartUpload.save already does. One-line behavioural change; no signature, threading, or ordering change.

-                data = data[start : start + multipart.chunk_size]
+                chunk = data[start : start + multipart.chunk_size]
                 futures.append(
-                    executor.submit(multipart.upload_part, part_number, data)
+                    executor.submit(multipart.upload_part, part_number, chunk)
                 )

Part ordering is deliberately left alone — self._parts is appended in completion order and the server sorts by part number, which was confirmed against the real CDN with parts submitted out of order. Adding sorting here would be an unrelated change.

Regression test

test_multipart_save_uploads_every_part_in_full in projects/fal_client/tests/unit/test_client.py patches create/upload_part/complete, so it never touches the network, and uses a 1 KB payload with chunk_size=100 so it is instant. It asserts that every part number is present, that each part carries exactly the bytes it should, and that concatenating the parts in part-number order reproduces the payload byte for byte.

I also swept the repo for the same self-rebinding shape (X = X[...]) with a backreference regex. The only other matches are one-shot rebinds outside any loop (dropping a leading element, truncating a list, walking a cursor, formatting a timezone offset); none re-slices a source buffer per iteration.

How to test

Set up and run the suite from the repo root:

uv venv /tmp/fcvenv --python 3.11
VIRTUAL_ENV=/tmp/fcvenv uv pip install -e "projects/fal_client[test]"
/tmp/fcvenv/bin/python -m pytest projects/fal_client/tests/unit/ -q

Full fal_client unit suite on this branch:

........................................................................ [ 47%]
........................................................................ [ 95%]
.......                                                                  [100%]
151 passed in 6.79s

The new test fails on the unfixed code. Reverting just the one-line change and running it alone:

>       assert [uploaded[n] for n in sorted(uploaded)] == expected
E       AssertionError: assert [b'\x00\x01\x...b'', b'', ...] == [b'\x00\x01\x...QRSTUVW', ...]
E         At index 1 diff: b'' != b'defghijklmnopqrstuvwxyz...'

projects/fal_client/tests/unit/test_client.py:1138: AssertionError
=========================== short test summary info ============================
FAILED projects/fal_client/tests/unit/test_client.py::test_multipart_save_uploads_every_part_in_full
1 failed in 0.23s

Index 1 is part 2 — the first part the bug empties — which is exactly the reported failure mode.

Lint, matching the v0.3.4 pin in .pre-commit-config.yaml:

$ uvx ruff@0.3.4 format --check projects/fal_client/src/fal_client/client.py projects/fal_client/tests/unit/test_client.py
2 files already formatted
$ uvx ruff@0.3.4 check projects/fal_client/src/fal_client/client.py projects/fal_client/tests/unit/test_client.py
All checks passed!

Live CDN verification (post-fix)

The multipart code path was exercised against the real CDN, with chunk_size lowered to 5 MB so a 12 MB payload produces 3 parts without transferring 100 MB. Same script, same payload, before and after the change.

Before the fix, MultipartUpload.save (the path upload() / upload_image() take above the 100 MB threshold) failed outright, because the CDN rejects the empty second part:

save_file (path used by upload_file): HTTP 200 len=12582912 md5=a99a3904... MATCH
save (bytes) FAILED: FalClientHTTPError Content length appears to be zero - refusing to proceed

After the fix, the same call succeeds and the stored object is byte-identical to the source:

source: 12582912 bytes md5=a99a3904ee64f8c75526f354cc008106
save_file (path used by upload_file): HTTP 200 len=12582912 md5=a99a3904ee64f8c75526f354cc008106 MATCH
save   (path used by upload/upload_image): HTTP 200 len=12582912 md5=a99a3904ee64f8c75526f354cc008106 MATCH

Note the failure mode before the fix was a hard error, not silent truncation — the CDN refuses a zero-length part rather than completing a truncated object.

Not verified here

• Tests were run locally on Python 3.11 only. The CI unit matrix also covers 3.8/3.9; the change introduces no new syntax, and the test uses only threading, unittest.mock, and stdlib slicing, so it should be compatible, but that is reasoned rather than executed.
• Only the fal_client unit suite was run, not the wider repo suites.

🤖 Generated with Claude Code