GeezerVison

On the split-PDF path the SDK rebuilt the whole document in memory to return it:
a list per chunk, a flattened list, a json.dumps blob in create_response, and the
SDK's re-parse of that blob -- four copies live at once, with the serialization
step dominating peak usage on large documents.

Passing accept_header_override=PartitionAcceptEnum.APPLICATION_X_NDJSON now
returns PartitionResponse.elements_file -- a path to an NDJSON file, one element
per line -- instead of PartitionResponse.elements. The per-chunk temp files are
concatenated on disk and never parsed, so peak memory is roughly one chunk rather
than the whole document.

Chunk files are sniffed for their first non-whitespace character, so a server
returning application/json still works; NDJSON chunks are copied through
untouched. Requesting application/json remains the default and is unchanged.

The caller owns the returned file and must delete it. It is deliberately written
outside the operation's TemporaryDirectory, which _clear_operation removes as soon
as after_success returns.

ndjson_mode depends only on the Accept header, never on split_pdf_cache_tmp_data.
Those are set by different parties, so gating on both let them disagree: the
server would return NDJSON while the hook took the JSON path and res.json() raised
on a body this client had itself requested. Both caching modes are handled -- a
cached chunk contributes its existing temp-file path, an uncached one spills its
body verbatim and then releases it, since every response is retained in
api_successful_responses and leaving _content set would keep the document resident
regardless.

general.py and models/operations/partition.py are both .genignore'd: elements_file
is client-side only and can never come from the OpenAPI spec, so a regeneration
would silently drop it. test_regeneration_guards.py fails if either entry is lost.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

Two defects found in review of the elements-file mode.

The elements-file marker was an `x-unstructured-elements-file` response header, and
any response carrying it was trusted as an SDK-created path. Headers come off the
wire, so a server could name an arbitrary local file -- and callers are documented
to open `elements_file` and then delete it, making this an arbitrary-file delete
rather than just a disclosure. The marker is now an httpx response extension, which
is populated by the transport and cannot be set remotely; a real server body is
always copied to a file this client creates.

Recombination also wrote straight to its final UUID path while recording that path
only on success, so a malformed chunk left a partial file behind under a name
nothing owned -- the combined file is deliberately outside the operation's
TemporaryDirectory, so nothing else cleaned it up. It now writes to a staging file
renamed into place atomically, and unlinks it on any exception.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

Ships the NDJSON elements-file mode.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

cubic-dev-ai Bot left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Addresses review findings on the elements-file path.

Orphaned files on failure. Both `_ndjson_elements_file` helpers create their
destination with delete=False, so a body that raises or is cancelled partway
through left the partial copy behind; they now unlink it and re-raise.
`write_chunk_body_to_temp` had the same shape -- the caller only registers the
path for cleanup once the function returns, so a failed write orphaned the file,
and a full disk is exactly the failure that repeats.

Cancellation race. Recombination runs in a worker thread that cancellation cannot
interrupt, so `_clear_operation` could tear the operation down while it was still
running; publishing the path afterwards resurrected a cleared dict entry and
orphaned the file. Publishing is now gated on the operation still being live,
under a lock that `_clear_operation` also takes when dropping
`pending_operation_ids`. Ownership is explicit either way: the success path claims
the path out of `ndjson_output_path`, so anything still recorded at teardown was
never delivered and is safe to delete.

Docs regeneration. docs/models/operations/partitionresponse.md is generated and
tracked in gen.lock, and the generation workflow runs on a daily cron, so the
elements_file row would have been dropped within a day of merging. Added to
.genignore alongside the model, and the regeneration guard now asserts the row.

README. Noted that the memory saving applies to the split-PDF path -- unsplit
inputs still buffer the body -- so the caveat is visible where the feature is
advertised rather than only in the PR. The example's cleanup used a bare unlink in
a finally, which would mask a failure to open the file with FileNotFoundError; it
now uses Path.unlink(missing_ok=True).

The spilled-body regression guard asserted against `_content` it had assigned
itself, so it could not fail if the hook stopped releasing the body. It now drives
`_elements_from_task_responses`, and was confirmed to fail with the release
removed.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

cubic-dev-ai Bot left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

The unlink-on-failure paths added in the previous commit had no coverage -- the
existing tests only walked the success roundtrip, so the cleanup could have been
removed without anything going red.

Three tests, each confirmed to fail with its corresponding unlink removed:

- write_chunk_body_to_temp: os.fdopen is patched so the write raises ENOSPC. The
  wrapper still closes the real handle, so the fd is not leaked by the test itself.
- _ndjson_elements_file and its async counterpart: a response whose byte iterator
  raises partway through. tempfile.tempdir is redirected at the test's tmp_path so
  the assertion can see whether anything was left behind.

Each asserts both halves of the contract: no file survives, and the original
exception still propagates rather than being swallowed by the cleanup.

Coverage for the general.py pair was not requested in review, but those helpers
grew the same delete=False cleanup in the same commit and had the same gap.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

cubic-dev-ai Bot left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

The orphan checks asserted that a directory held no matching files, which only
proves cleanup if the file landed in that directory to begin with. That held solely
because the helpers route through the global tempfile.tempdir the tests patch --
an assumption the tests never checked. A change that passed an explicit dir would
have made them pass while a partial file leaked into the real temp dir.

A shared `_record_created_paths` spy now wraps the temp-file factory, delegating to
the real one and recording each path it hands out. The tests assert on that path:
one file was created, it was where the test expected, and it is gone.

Verified by breaking it two ways. With the unlink removed the tests fail, as
before. With the unlink removed AND the destination pinned to an explicit dir that
ignores the patched tempdir -- the vacuous case, which the old directory-glob
assertion passed -- they now fail too.

Applied to the spill test as well. Review only raised the two copy-failure tests,
but that one asserted on an empty directory for the same reason and could go
vacuous the same way if `dir_` stopped being honored.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

cubic-dev-ai Bot left a comment

awalker4 left a comment

The deployed API does not offer application/x-ndjson -- its spec (v1.5.69) declares
only application/json and text/csv, and the string "ndjson" appears nowhere in the
document. So any request that bypasses the split-PDF hook came back as JSON, the
application/json branch matched first and won, and elements_file stayed None while
elements was populated. The opt-in was silently ignored.

That path is much wider than "non-PDF". _before_request_unlocked short-circuits when
split_size >= page_count, and get_optimal_split_size floors at MIN_PAGES_PER_SPLIT=2,
so one- and two-page PDFs are sent whole -- it would have read as intermittent to
anyone hitting it on small inputs.

The x-ndjson branch is now checked first, and a JSON body is written out as NDJSON
when NDJSON was requested. elements_file is therefore set for every input and
callers need one code path rather than two. This does not bound memory on the
unsplit path -- the body is already read and parsing adds the element list on top --
it makes the contract uniform. The memory win remains the split-PDF path, where the
hook concatenates chunk files on disk and this conversion is never reached.

Also fixes the README example, which raised TypeError from open(None) and then
raised the same TypeError again out of Path(None) in the finally, masking the first.

Adds the end-to-end coverage whose absence let this through: all 24 existing tests
were helper- or hook-level, so nothing exercised the media-type dispatch in
general.py. Three tests now drive partition() over a mock transport -- server
returns JSON, server returns NDJSON, and no override at all. The first fails against
the old dispatch. They hold the client for the call rather than chaining off a
temporary, since sdk.py registers a weakref.finalize that closes the transport.

Smaller review notes: log a warning per skipped empty chunk so element_count can be
reconciled against chunk_count; document that the non-"[" sniff assumes one JSON
value per line rather than checking; trim the _combine_chunks_to_ndjson docstring.

The regeneration comments no longer claim a daily workflow is about to drop the
field -- generation is blocked at the Speakeasy account level and gen.lock has not
moved since 2026-01. Whether that scaffolding is worth keeping is left open.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

cubic-dev-ai Bot left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Four review findings on the elements-file path, all reproduced first.

The JSON-to-NDJSON conversion iterated `http_res.json()` directly, bypassing the
response schema. A `null` body raised TypeError from iterating None, and a non-array
body was worse than that: iterating a dict yields its keys, so `{"detail": "oops"}`
was written out as a one-element document reading `"detail"`. It now goes through
`unmarshal_json_response`, so a malformed 200 fails exactly as it does on the
`elements` path, and `null` yields an empty file.

NDJSON detection keyed off `accept_header_override` alone, but `http_headers` can
replace Accept too -- and the split-PDF hook already keys off the request header.
So one caller passing `http_headers={"Accept": "application/x-ndjson"}` got
`elements_file` for a multi-page PDF and `elements` for a two-page one. Detection now
reads the header off the built request, which is the same source the hook uses.

The async path ran the parse-and-write inline, blocking the event loop for exactly
the large bodies this mode exists for. Offloaded with `asyncio.to_thread`.

Test `finally` blocks used a bare `os.unlink`, which masks an assertion failure above
it with FileNotFoundError -- the same defect flagged in the README two rounds ago,
reintroduced in the tests. Now `Path(...).unlink(missing_ok=True)`.

Also removes a duplicated "The" left in `_combine_chunks_to_ndjson`'s docstring by
the earlier trim.

Four tests added, each confirmed to fail against the pre-fix source. The async one
originally asserted `asyncio.to_thread` was called, which passed even with the
offload removed because other SDK internals use it during the same call; it now
records the thread the conversion ran on and compares it to the event-loop thread.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

cubic-dev-ai Bot left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

The API does not negotiate response format on Accept at all: it reads the
output_format form field, and only consults Accept to select multipart/mixed and to
reject conflicting media types on multi-file uploads. Saying the spec merely omits
application/x-ndjson understated it -- NDJSON was never going to arrive via Accept
regardless of what the spec declared.

Comment only; no behavior change.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

cubic-dev-ai Bot left a comment

Offloading the JSON-to-NDJSON conversion to a thread last commit removed the event
loop block but introduced a leak: a thread cannot be cancelled, so on cancellation
the conversion still runs to completion and creates its file, while the awaiting
coroutine has already raised CancelledError and discarded the path. Nothing was left
that could delete it. Reproduced -- cancel mid-conversion and an
unst_elements_*.ndjson survives.

The conversion is now awaited through asyncio.shield, which keeps a handle on the
thread's result after the caller stops waiting, and a done callback unlinks the
finished file. A conversion that raised needs no callback, since it already removes
its own partial file.

This is the same shape as the split hook's cancellation race: work that outlives the
operation that requested it, publishing a path nobody will read. It did not exist on
the inline version, which had no await point to cancel at.

Regression test asserts against the path production actually created, so it cannot
pass by observing that no file was ever made. Confirmed to fail against the plain
awaited to_thread.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

cubic-dev-ai Bot left a comment •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

The test raced. It relied on a fixed `time.sleep(0.3)` in the worker outlasting an
`await asyncio.sleep(0.05)` before `task.cancel()`. On a loaded runner the conversion
could finish first, leaving nothing to cancel, and the test would fail with
"DID NOT RAISE" -- a spurious failure rather than a real one.

Now sequenced with three `threading.Event` handshakes: the worker signals that it has
entered the conversion, the test cancels only after that, and only then releases the
worker, so completion is necessarily post-cancellation. A spy on
`_discard_elements_file` signals that cleanup ran, so the assertion waits on the
actual event rather than polling the filesystem on a timer. The `wait` timeouts are
deadlock guards, never a duration anything waits out.

Also drops the 0.02s x 200 polling loop, so the test now takes ~0.15s instead of
~0.5s, and reports "cleanup never ran" instead of a bare assertion when it fails.

Still confirmed to fail against the plain awaited to_thread. Ran 20x clean, and 10x
clean under eight busy cores.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>

cubic-dev-ai Bot left a comment

awalker4 left a comment