Skip to content

feat(media): a converter receives a source handle, never the file's bytes (D104) - #344

Open
fazpu wants to merge 3 commits into
mainfrom
feat/video-source-handle
Open

feat(media): a converter receives a source handle, never the file's bytes (D104)#344
fazpu wants to merge 3 commits into
mainfrom
feat/video-source-handle

Conversation

@fazpu

@fazpu fazpu commented Sep 1, 2026

Copy link
Copy Markdown
Member

Problem

A text file fits in memory and a video does not. Under D65's convert(bytes, mime, hints) contract, the client, the HTTP process, the object store, and the converter each materialize a complete copy of the same file. Two concurrent large ingests can exhaust a worker that would have processed either one alone. That is an availability property, not an optimisation — and it is the reason no media route can be built on the current contract.

Scope

  • SourceHandlePort — bounded reads of one immutable, already-hashed source;
  • ObjectSourceHandle — the adapter over any ObjectStorePort;
  • open_stream / read_range on ObjectStorePort, implemented for the local filesystem and MinIO;
  • D104 in decisions.md, design in plan/designs/media_design.md §2.1.

Nothing consumes the handle yet. No converter signature changes here and no route behaviour changes. This is the seam a media route needs to exist at all.

Important decisions

Four reads, every one of them bounded. Stream in bounded chunks; read a half-open range [start, end); materialize to a temporary local file for decoders like FFmpeg that need a seekable path; or read the whole thing into memory after naming the limit. There is no operation that reads a source of unknown size — the hazard was never bytes in memory, it was bytes in memory without anyone having decided how many.

A read ceiling, because naming a large limit is still naming one. read_range(0, byte_size) is a whole-file read with extra steps, so a handle can carry max_read_bytes and refuse any single read above it. Without a ceiling the property is ergonomic friction; with one it is enforced. The design says which it is rather than overclaiming.

A stream is a resource, not just bytes. Underneath is a file descriptor or an HTTP body, so open_stream is a context manager released at block exit. A bare iterator would leave an early-stopping caller holding it until garbage collection, and GC timing is not a lifetime contract.

Reads never come back short, and length is verified alongside hash. A range returns exactly what was asked for or raises — a parser handed a truncated header cannot tell it from a valid one. Bytes can also hash correctly while being described by a wrong size, which would let materialization and range reads disagree about the length of one source.

Half-open ranges match every other interval in the system (D65 locators), so nobody has to remember which end is inclusive. S3 ranges are inclusive on both ends, so the exclusive end becomes end - 1 on the wire — the conversion lives in the adapter so the port stays consistent regardless of provider.

Bounds live in materialization, since that is the operation that can actually exhaust a host:

  • the caller declares what it can afford, and an oversized source is refused before any byte moves — one integer comparison, not a filled disk;
  • written bytes are verified against the recorded content hash. An immutable object cannot legitimately change, so a mismatch is corruption or a wrong key, never retryable. Without this check a truncated read becomes a short document that looks complete — the silent-loss failure D65's coverage rules exist to prevent;
  • the recorded size is a claim rather than a guarantee, so the streaming write also counts what it writes and refuses a source exceeding both the declaration and the accepted bound; and
  • the temporary file's lifetime belongs to the handle, so a route that fails mid-decode cannot leak the file it asked for.

One contract, both deployment shapes. A self-host directory tree and a cloud object store present the same three operations, so no route branches on where bytes live.

Sources inspected

Base origin/main 7c41fdb8. D38 (router), D57 (Markdown coordinate system), D61 (port boundary), D65 + plan/designs/media_design.md §2 (media routes and the converter contract). read_bytes is retained deliberately — every shipped route converts small text and has no reason to stream.

Review

Double-reviewed by two independent reviewers. Both returned BLOCK and converged on the same two blocking defects; the second found five more, including one aimed squarely at this PR's own claim.

Round 1 — teardown used unlink + rmdir, which raises directory not empty the moment a decoder writes a sidecar beside the source, leaking the directory and masking the route's own exception. And reads could come back short: LocalFS truncated past EOF, MinIO leaked botocore.ClientError on a 416 (a provider type in a port D61 keeps provider-free), and the handle checked only the recorded size, never what actually arrived.

Round 2read_range(0, byte_size) made the "no whole-file read" claim false, so the ceiling was added and the docs corrected; open_stream leaked descriptors on abandonment; the streaming guard fired only after a full 1 MiB chunk had moved for a 100-byte allowance; length was never verified against the record; SourceIdentity.mime duplicated an authority the converter signature already owns. Two tests also asserted less than their names claimed — "refused before any bytes move" only checked that no file survived — and now use a counting store asserting bytes actually served.

Concurrent materializations still have no aggregate temp-disk budget. That is worker-level admission, and it is documented as a stated boundary rather than quietly left.

Validation

  • ruff format --check and ruff check clean;
  • pyright — 0 errors;
  • pytest1303 passed, 685 skipped;
  • 29 new tests across test_source_handle.py and test_minio_store.py: ordered streaming, half-open ranges, degenerate and past-the-end ranges, cleanup on success, on exception, and with a sidecar present, refusal before any byte is served, the understated-size guard bounded to ≤101 bytes moved, hash mismatch, size mismatch, the read ceiling, zero-byte sources, temp_root=None, concurrent materializations, stream release after abandonment, and the MinIO wire translation asserting [2, 5) reaches S3 as bytes=2-4.

One pre-existing failure, test_sdk_pushes_lineage_metadata_to_e0, reproduces on base commit 7c41fdb8 with these changes stashed: mimetypes on this machine does not resolve .md, so the SDK guesses application/octet-stream. Not introduced here and not fixed here.

Extending ObjectStorePort required adding the two reads to three test fakes and raising the port-inventory count to thirteen.

Limitations

The handle has no consumer yet — the D65 converter signature still takes bytes, and changing it is the next step rather than part of this PR. The trusted structural probe (container, tracks, codecs) is not here. No video route exists and none is claimed.

Follow-up

  1. Move the converter contract onto the handle, route by route.
  2. The trusted probe, as the other half of the D65 companion amendment.
  3. Managed upload/resume, which is cloud-side and has its own contract to write.

🤖 Generated with Claude Code

https://claude.ai/code/session_01XTv3baR8R5EgEGFePkQMjK

@fazpu
fazpu force-pushed the feat/video-source-handle branch from bcee1ad to 726c79d Compare September 1, 2026 00:04
@fazpu fazpu changed the title feat(media): a converter receives a source handle, never the file's bytes (D103) feat(media): a converter receives a source handle, never the file's bytes (D104) Sep 1, 2026
@fazpu
fazpu force-pushed the feat/video-source-handle branch from 726c79d to f26b763 Compare September 1, 2026 00:05
fazpu and others added 3 commits September 1, 2026 02:36
…ytes

D104, amending D65's convert(bytes, mime, hints). Design: media_design.md
§2.1.

A text file fits in memory and a video does not. Under the bytes-first
contract the client, the HTTP process, the object store and the converter
each materialize a complete copy of the same file, so two concurrent large
ingests can exhaust a worker that would have processed either one alone.
That is an availability property, not an optimisation.

Adds SourceHandlePort with exactly three reads and deliberately no fourth:
stream in bounded chunks, read a half-open range [start, end), or
materialize to a temporary local file for decoders like FFmpeg that need a
seekable path. There is no operation returning the whole source as bytes --
the absence is the load-bearing part: a contract that offers buffering as
the convenient option gets buffering, and the failure surfaces on the first
large file in production rather than in review.

ObjectStorePort grows open_stream and read_range so the handle has
something to sit on; read_bytes stays for the small text objects every
shipped route converts. Implemented for both the local filesystem and MinIO
adapters -- S3 ranges are inclusive on both ends, so the exclusive end
becomes end-1 on the wire and every interval in the system stays half-open
(D65).

Bounds live in materialization, since that is the operation that can
exhaust a host:
- the caller declares what it can afford and an oversized source is refused
  before any byte moves -- one integer comparison, not a filled disk;
- written bytes are verified against the recorded content hash. An
  immutable object cannot legitimately change, so a mismatch is corruption
  or a wrong key, never retryable. Without the check a truncated read
  becomes a short document that looks complete -- the silent-loss failure
  D65's coverage rules exist to prevent;
- the recorded size is a claim, not a guarantee, so the streaming write
  also counts what it writes and refuses a source exceeding both the
  declaration and the accepted bound; and
- the temporary file's lifetime belongs to the handle, so a route failing
  mid-decode cannot leak the file it asked for.

One contract serves both deployment shapes: a self-host directory tree and
a cloud object store present the same three operations, so no route
branches on where bytes live.

Nothing consumes the handle yet -- no converter signature changes in this
commit and no route behaviour changes. Extending ObjectStorePort required
adding the two reads to three test fakes and raising the port-inventory
count to thirteen.

Validation: ruff format and check clean; pyright 0 errors; 1303 passed,
685 skipped. One pre-existing failure,
test_sdk_pushes_lineage_metadata_to_e0, reproduces on the base commit
7c41fdb without these changes -- mimetypes on this machine does not
resolve .md, so the SDK guesses application/octet-stream.

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

External review returned BLOCK with eight findings. All addressed.

BLOCK — teardown used unlink plus rmdir, which raises "directory not empty"
the moment a decoder writes anything beside the source it was handed: an
index, a sidecar, a split audio track. That both leaked the directory and
masked whatever the route was already raising. Now shutil.rmtree with
ignore_errors, so cleanup cannot become the failure.

BLOCK — reads could come back short. LocalFS returned a truncated slice past
EOF, MinIO let botocore's ClientError escape on a 416 (a provider type in a
port D61 keeps provider-free) and returned a short 206 body otherwise, and
the handle validated only against the recorded size, never against what
actually arrived. A stored object shorter than its record therefore produced
a silent short read. All three now return exactly the requested bytes or
raise SourceRangeError, and 416 is translated at the adapter. This is the
failure the range contract exists to prevent: a container parser handed a
truncated header cannot distinguish it from a valid one and produces
confident nonsense instead of an error.

MAJOR — max_bytes had to be positive, so a zero-byte source could not be
materialized under a zero bound; a negative bound now raises ValueError as
the caller bug it is, rather than being reported as a size refusal.

MAJOR — MinIO's open_stream and read_range had no tests at all. The fakes
were updated but nothing exercised the wire translation, so an off-by-one in
the inclusive-range conversion would have passed silently. Five tests added,
including one asserting [2, 5) reaches S3 as bytes=2-4.

MINOR — the materialized file is named with the source's own suffix.
Demuxers sniff the container from the extension before falling back to
probing, so a bare "source" costs a class of cannot-open bug for nothing.

MINOR — added read_bounded(max_bytes=...). The review's point stands: with
no in-memory read at all, a text route writes b"".join(open_stream()), which
is an unbounded read wearing a bounded read's clothes. The design intent was
never "no bytes in memory" but "no bytes in memory without someone deciding
how many", so the fourth read requires the caller to name a limit. Design
and D104 corrected — they claimed three reads and no fourth, which is no
longer true.

MINOR — tests added for zero-byte sources, temp_root=None, cleanup with a
sidecar present, a truncated stored object, and two concurrent live
materializations not sharing or co-deleting a directory.

Validation: ruff clean; pyright 0 errors; test inventory OK; 1319 passed,
687 skipped. The one failure, test_sdk_pushes_lineage_metadata_to_e0,
reproduces on the base commit without these changes — mimetypes on this
machine does not resolve .md.

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

Second independent review (Codex) converged with the first on both BLOCKs
already fixed, and found five more. All addressed.

The sharpest one was aimed at my own claim. D104 said there is no whole-file
read, but read_range(0, byte_size) is exactly that with extra steps, and
nothing capped a range or a chunk. The claim was ergonomic friction dressed
as an availability invariant. A handle now takes an optional max_read_bytes
ceiling that refuses any single range, chunk, or bounded read above it, and
the design says plainly that without a ceiling the property is friction, not
a guarantee. Overclaiming in a decision doc is worse than the gap it hid.

open_stream returned a bare Iterator, so a caller that stopped early held a
file descriptor or an HTTP body until garbage collection. GC timing is not a
resource-lifetime contract and enough abandoned reads exhaust the descriptor
limit or the connection pool. It is now a context manager at every level --
port, both adapters, handle, and the three fakes -- released when the block
exits.

The streaming guard fired only after a full default chunk had moved: a
100-byte allowance could pull a megabyte before noticing. The read is now
sized to the remaining allowance plus one byte, the extra byte being what
makes "over" detectable at all -- a read that exactly fills the allowance is
indistinguishable from one that stops there.

Materialization verified the hash but never the length, so bytes that hash
correctly could still be described by a wrong size: a source recorded as 10
bytes materialized happily at 5000 while a range read past 10 was refused --
two access paths disagreeing about one source. Now a typed
SourceSizeMismatchError, distinct from the hash error because they catch
different lies.

SourceIdentity carried a mime field while the converter contract already
takes mime as its own argument. Two authorities that can disagree, and a
route would have to know which one routing used. Dropped from the identity.

Concurrent materializations still have no aggregate temp-disk budget -- two
6 GiB copies each pass their own bound and together need 12 GiB. That is
worker-level admission, not a handle concern, so it is documented as a
stated boundary in both the design and D104 rather than quietly left.

Tests: two asserted less than their names claimed. "Refused before any bytes
move" checked only that no file survived, and the understated-size test read
its whole 5000-byte source before raising and still passed. Both now use a
counting store and assert bytes actually served (0, and <=101). Added
coverage for the ceiling, the size mismatch, and releasing a stream
abandoned after one chunk.

Validation: ruff clean; pyright 0 errors; test inventory OK; 1322 passed,
687 skipped. The single failure, test_sdk_pushes_lineage_metadata_to_e0,
reproduces on the base commit without these changes -- mimetypes on this
machine does not resolve .md.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XTv3baR8R5EgEGFePkQMjK
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.

1 participant