feat: NDJSON elements-file mode for partition (0.46.0) - #347
Conversation
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]>
There was a problem hiding this comment.
All reported issues were addressed across 12 files
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
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]>
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
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]>
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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]>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
awalker4
left a comment
There was a problem hiding this comment.
Went through this against a local checkout — 241 unit tests pass, pylint 10.00/10, mypy clean on the touched modules, so the numbers in the description hold up.
The split-PDF path is the real substance here and it's well executed. A few things stand out as better than average:
ELEMENTS_FILE_EXTENSION_KEYas an httpx extension rather than a response header. Correct call, and the reasoning in the comment is right — a wire-controlled header would let a hostile server name any local path for a caller who is documented to open it and then delete it. Good that there's a test pinning it.ndjson_modekeyed only offAccept, never offsplit_pdf_cache_tmp_data. The comment in_before_request_unlockeddescribes a genuine failure mode from two independently-set knobs disagreeing.- The cancellation design holds up. I walked both interleavings of the publish in
_combine_chunks_to_ndjsonagainst teardown in_clear_operation: staging file,os.replace, andpending_operation_idschecked under_ndjson_lockmeans the combined file is either claimed by the caller or deleted, never both and never neither._claim_ndjson_output/_discard_ndjson_outputis a good way to name that. - Releasing
res._contentafter spilling, with a test that drives the hook rather than the helper. That's a real fix to the earlier vacuous-guard complaint, not a cosmetic one.
Three things I'd want addressed, left as inline comments: the unsplit-path contract in general.py, the corresponding README claim, and the regeneration scaffolding (which I think can just come out — details inline).
Smaller notes, none blocking:
- Empty chunks are silently dropped.
_first_non_space_charreturning""leads to acontinue, pinned as intended bytest_empty_chunk_files_are_skipped. In JSON mode an empty chunk file makesjson.loadraise, so the caller learns the document is short; in NDJSON mode a 200 with an empty body yields a silently truncated document instead. Alogger.warningper skipped chunk would at least letevent=ndjson_combined element_countbe reconciled against the chunk count. - The format sniff is
[versus "assume one JSON object per line." A multi-line pretty-printed JSON object would get split into invalid lines. Not reachable through the current endpoint, but it's an assumption rather than a check. - Mixed temp-file suffixes — cached chunks are
.json, spilled ones.ndjson, which makes glob-based cleanup and the test helpers asymmetric. _combine_chunks_to_ndjson's docstring is four paragraphs of design rationale, and the cancellation paragraph restates the comment on_ndjson_lock. Worth a trim.
Leaving this as a comment rather than a block — the asks are clear enough and I don't want to gate the release on process questions that are partly mine to answer.
| content_type=http_res.headers.get("Content-Type") or "", | ||
| raw_response=http_res, | ||
| ) | ||
| if utils.match_response(http_res, "200", "application/x-ndjson"): |
There was a problem hiding this comment.
This is the one I'd most want changed before merge.
I checked the live spec rather than the vendored copy — https://api.unstructuredapp.io/general/openapi.json, currently v1.5.69. The 200 declares application/json and text/csv only, and the string ndjson appears nowhere in the document. So the deployed server does not emit application/x-ndjson today.
Which means on the unsplit path the real server response lands here, and the application/json branch above is checked first and wins:
content-type: application/json
matches application/json -> True
matches application/x-ndjson -> False
elements gets populated and elements_file stays None, even though the caller explicitly asked for NDJSON.
Worth stressing how wide "unsplit" is, because it isn't just non-PDFs. split_size >= page_count short-circuits in _before_request_unlocked, and with MIN_PAGES_PER_SPLIT = 2 that catches every single-page PDF, plus anything with split_pdf_page=False. So this surfaces on small inputs and will read as intermittent to whoever hits it.
Knock-on: the "Known limitation" section in the description describes a path that never executes against the current API, and the server-body branch of _ndjson_elements_file / _ndjson_elements_file_async is effectively dead code until the API ships NDJSON.
What I'd prefer: when NDJSON was requested and the server returned JSON, spill that body to a file and set elements_file anyway. That keeps the documented contract true regardless of server support, and it means the mode behaves the same way for every input. The alternative is to document that elements_file can be None and guard the README example, but then callers need two code paths for one opt-in flag.
Also worth confirming with whoever owns the API: if the server 406s on an unrecognized Accept rather than ignoring it, the unsplit path raises SDKError instead, which is a different problem again.
Related: nothing currently drives client.general.partition(accept_header_override=APPLICATION_X_NDJSON) end to end. All 24 new tests are helper- or hook-level. One mock-transport test returning application/json would have caught this.
There was a problem hiding this comment.
Walking back the framing here — I called this the one I'd most want changed, and the premise underneath that is weaker than I made it sound.
I inferred "the server doesn't emit NDJSON" from the live /openapi.json. But that document is generated from the route's explicit responses={} annotations, so a runtime content-negotiation branch returning a StreamingResponse(media_type="application/x-ndjson") wouldn't necessarily show up in it. Absence there isn't absence in the server. And if you're building the client half, the reasonable assumption is that the server half is already handled — I should have asked instead of inferring from a generated artifact.
What does survive is narrower and doesn't depend on the server at all: general.py checks application/json before application/x-ndjson, so for any deployment that answers an NDJSON request with a JSON body, elements_file comes back None. That isn't hypothetical even with full support in prod — this SDK gets pointed at self-hosted and older unstructured-api images, which I'd guess is part of why combine_chunk_files_to_ndjson sniffs for [ on the chunk path in the first place. The unsplit path just doesn't have the equivalent of that sniff.
So the actual question, downgraded from "want changed" to "want confirmed": do you want the unsplit path to degrade silently to elements against an older server, or to spill the JSON body to a file so elements_file is always set? Either is defensible — I'd just rather it were a decision than an accident.
If there's a server PR landing NDJSON on /general/v0/general, point me at it and the only thing left in this thread is the README wording.
There was a problem hiding this comment.
Fixed in 358e21a, taking the option you preferred: elements_file is now set whether or not the server honors the header. The application/x-ndjson branch is checked first, and a JSON body is written out as NDJSON when NDJSON was requested, so callers need one code path.
I reproduced your spec finding independently before changing anything — v1.5.69, 200 declares application/json and text/csv only, ndjson absent from the whole document.
One correction to the scope, which is slightly wider than "every single-page PDF": get_optimal_split_size returns max(ceil(pages / concurrency), MIN_PAGES_PER_SPLIT) and the short-circuit is split_size >= page_count, so with the floor at 2 a two-page PDF is also sent whole. The README caveat and the PR description now both say "two pages or fewer".
I was deliberately explicit in the docstring and changelog that this conversion does not bound memory — the body is already read by the time it runs, and parsing adds the element list on top. It makes the contract uniform, nothing more. Overstating that is what produced the original problem.
On the missing coverage: three tests now drive partition() over a mock transport — server returns JSON, server returns NDJSON, and no override at all. I verified the first fails against the old dispatch. Aside for anyone writing similar tests: hold the client for the duration of the call, because sdk.py registers a weakref.finalize that closes the transport, so chaining off a temporary fails with a confusing 'NoneType' has no attribute 'build_request'.
On the 406 — I went and read the service, and it does not need confirming: this SDK cannot trigger it. The 406 NOT_ACCEPTABLE is gated on len(files) > 1. For a single file the check is skipped and the Accept value is never validated. This SDK sends one file per request — PartitionParameters.files is a single Files, and the split hook sends one chunk per request — so the unsplit path cannot raise SDKError because of it. Your caveat is real for direct multi-file API callers, just not reachable from here.
Reading that turned up something more consequential, though: the service does not negotiate response format on Accept at all. It selects the format from the output_format form field, and consults Accept only to pick multipart/mixed and for that multi-file guard. So NDJSON was never going to arrive via Accept regardless of what the spec declared — and the existing accept_header_override=TEXT_CSV does not make the server return CSV either; output_format does. I have corrected the docstring and the PR description, which both understated this as a spec omission, and filed the server-side work (add application/x-ndjson to output_format, emit it as a stream, update the spec, extend the multi-file allowlist) separately under the memory epic. Nothing in this PR depends on it.
Two follow-ups from a later automated pass, both fixed in ba3e7f4 and both touching this change: the conversion was iterating http_res.json() directly, so a {"detail": ...} body wrote its keys out as elements — it now goes through unmarshal_json_response. And detection keyed off accept_header_override alone while the split hook keys off the request header, so http_headers={"Accept": "application/x-ndjson"} produced exactly the split-vs-unsplit disagreement you were describing, on the same input. Both now read the same source.
| **You own the returned file and are responsible for deleting it.** | ||
|
|
||
| > [!NOTE] | ||
| > The memory saving applies to the split-PDF path, i.e. a PDF with `split_pdf_page=True` (the default). For unsplit inputs — a non-PDF file, or `split_pdf_page=False` — the response body is still read fully into memory before being written to disk, so peak memory can reach roughly twice the body size. You still get `elements_file` either way. |
There was a problem hiding this comment.
This sentence isn't true against the current API — see the comment on general.py. For unsplit inputs the server returns application/json, the JSON branch matches first, and elements_file comes back None.
That makes the example below worse than a plain failure: open(res.elements_file) raises TypeError: expected str, bytes or os.PathLike object, not NoneType, and then the finally raises the same TypeError out of Path(None) and masks the original. So the first thing a user sees is a confusing double-TypeError from copy-pasted README code.
If the contract gets made uniform in general.py then this line is fine as written. If not, it needs to say elements_file may be None and the example needs an if res.elements_file: guard.
There was a problem hiding this comment.
Softening this one too — see the reply on general.py. My claim that the server returns JSON to an NDJSON request came from the live /openapi.json, which only reflects explicit responses={} annotations, so it does not settle what the server actually does.
The narrow version still stands: if elements_file can ever come back None — an older or self-hosted deployment, say — then this example raises TypeError on the open() and then raises the same TypeError again out of Path(None) in the finally, masking the first. That is a rough first experience for copy-pasted code.
If elements_file is always set against a current server, the sentence is fine as written and this is moot.
There was a problem hiding this comment.
The contract was made uniform in general.py, so the sentence is now true as written — elements_file is always set when the header is passed.
I also corrected the caveat itself, which was wrong in a way you did not flag: it said "non-PDF, or split_pdf_page=False" and omitted small PDFs entirely. Because split_size is floored at 2 and the hook short-circuits on split_size >= page_count, one- and two-page PDFs are sent whole too. It now reads "two pages or fewer".
The double-TypeError is gone with the contract fix, and the example uses Path(...).unlink(missing_ok=True) so a failure to open the file is not masked by the cleanup.
Worth noting the same defect then turned up in my own tests — a later pass caught bare os.unlink in the finally blocks of the new end-to-end tests, masking assertion failures exactly as the README example would have. Fixed in ba3e7f4.
| assert "run: make install" in workflow | ||
|
|
||
|
|
||
| def test_partition_response_keeps_elements_file(): |
There was a problem hiding this comment.
I think this test and the two new .genignore entries can come out — the regeneration they're defending against has been hard-blocked for months.
speakeasy_sdk_generation.ymlhas 100 consecutive failed runs, oldest in the queryable window 2026-04-26, newest today.- The failure isn't transient or spec-related:
##[error]generation access blocked, i.e. Speakeasy account level. - The last generated PR that actually merged was chore: 🐝 Update SDK - Generate 0.42.10 #329 on 2026-02-04.
.speakeasy/gen.lockhasn't moved since 2026-01-18.
So no regeneration can drop elements_file, because no regeneration completes. That's roughly 38 lines that can go: this test, the 11 new .genignore lines, and the Regeneration section of the description.
If any of it does stay, the comments need rewording. Both this test and the .genignore entries assert that "the daily generation workflow would drop the row on its next run," which is now just false and will mislead the next person to read it. The test also string-matches paths inside .genignore, which is a gate that costs maintenance and catches nothing.
To be clear this is a call about where the repo is heading rather than a defect in your work — the premise you were given was reasonable, it just doesn't hold anymore. Given generation is blocked at the account level, the burden is on restoring Speakeasy rather than on defending against it. Happy to own that decision if it's easier; I'll follow up separately on whether that cron should just be turned off.
One knock-on if you do drop it: the PartitionAcceptEnum.APPLICATION_X_NDJSON assertion in here is currently the only test reference to the new enum, so it'd be worth folding that into whatever end-to-end test covers the mode.
There was a problem hiding this comment.
I verified all three of your data points before touching anything: 100 consecutive failed runs (oldest in the queryable window 2026-04-26, newest today), ##[error]generation access blocked, and .speakeasy/gen.lock unchanged since 2026-01-18. Your conclusion holds — nothing can drop the field because nothing completes.
I have left the structural decision to you rather than removing it unilaterally, since you offered to own it and it is a call about where the repo is heading.
What I did do is fix the wording, which was false regardless of that decision. The .genignore comment and this test's docstring no longer claim an imminent regeneration; they now state that generation is blocked at the account level and that the entries are insurance for when it is restored. I also posted a correction on the partitionresponse.md thread, where I had asserted the row would disappear "within a day of merging" — I had checked that the cron existed without checking that the workflow succeeds, which is on me.
One update relevant to your decision: the PartitionAcceptEnum.APPLICATION_X_NDJSON assertion here is no longer the only test reference to the enum. The new end-to-end tests in test_ndjson_elements_file.py use it in five places, so if you drop this test the enum keeps its coverage and nothing needs folding in first.
For what it is worth, my own read is to keep the two .genignore entries — they are three lines and correct whenever generation returns — and drop the string-matching of .genignore contents from this test, since that part asserts on a config file rather than on behavior. But I am happy either way and will action whatever you decide.
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]>
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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]>
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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]>
|
Thanks — this was a genuinely useful review. All three inline asks are addressed and answered in their threads; the four smaller notes are done too:
Two things worth surfacing from working through the review, both in the threads but easy to miss:
Also fixed since your review, from an automated pass over the same code: the JSON conversion was iterating the raw body so a non-array 200 wrote a dict's keys out as elements; NDJSON detection ignored Current state: 248 unit + 64 contract tests pass, pylint 10.00/10, mypy clean. The only open item is your call on the regeneration scaffolding. |
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
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]>
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="_test_unstructured_client/unit/test_ndjson_elements_file.py">
<violation number="1" location="_test_unstructured_client/unit/test_ndjson_elements_file.py:772">
P3: This cancellation test can race on slower or busy CI runs: the conversion may finish before `task.cancel()` because it relies on a fixed `time.sleep(0.3)`. Synchronizing the worker with `threading.Event` (signal started, then wait for release) would make the cancellation scenario deterministic instead of wall-clock dependent.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
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]>
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would require human review. Adds a new NDJSON mode and elements_file response field. Requires human sign-off on the public API expansion, the manual override of SDK generation via .genignore, and the design decision to delegate temp-file deletion to the caller.
Re-trigger cubic
What
Adds an opt-in NDJSON response mode to
partition()that returns elements as a path to a file on disk instead of a parsed list, and ships it as 0.46.0.PartitionResponse.elements_fileis set instead ofPartitionResponse.elements. The caller owns the file and must delete it. Requestingapplication/jsonremains the default and is entirely unchanged.Why
On the split-PDF path the SDK rebuilt the whole document in memory in order to return it: a list per chunk, a flattened list, a
json.dumpsblob increate_response, and then the SDK's re-parse of that blob — four copies live at once, with the serialization step dominating peak usage. For documents with largemetadata.image_base64payloads this is the difference between a job completing and being OOM-killed.In the new mode the per-chunk temp files are concatenated on disk and never parsed, so peak memory is roughly one chunk rather than the whole document.
How
combine_chunk_files_to_ndjsonconcatenates chunk files on disk. Each chunk is sniffed for its first non-whitespace character, so a server returningapplication/jsonstill works; chunks that are already NDJSON are copied through without parsing.ndjson_modedepends only on theAcceptheader, never onsplit_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 andres.json()raised on a body this client had itself requested.api_successful_responsesand leaving_contentset would keep the document resident regardless.TemporaryDirectory, which_clear_operationremoves as soon asafter_successreturns.Temp-file ownership
Everything this path creates is accounted for:
Security
The elements-file marker is an httpx response extension, not a response header. Extensions are populated by the transport, so a remote server cannot set the key. A header would be wire-controlled, and since callers are documented to open
elements_fileand then delete it, that would hand a hostile server an arbitrary local file to destroy. A real server body is always copied to a file this client creates.Regeneration
elements_fileis client-side only and can never come from the OpenAPI spec, so a regeneration would silently drop it. Bothgeneral.pyandmodels/operations/partition.pyare now in.genignore, andtest_regeneration_guards.pyfails if either entry is lost.Known limitation
elements_fileis set for every input, so callers need one code path. The memory saving, however, applies only to split PDFs.An input is sent whole when it is not a PDF, when
split_pdf_page=False, or when it has two pages or fewer —_before_request_unlockedshort-circuits onsplit_size >= page_countandget_optimal_split_sizefloors atMIN_PAGES_PER_SPLIT = 2. For those, the body is read fully into memory before being written to disk, so peak is roughly 2x the body rather than bounded.Bounding it means
stream=Truefor NDJSON requests, which makesraw_response.contentraise on the returned closed response — a user-visible change worth its own review. Tracked separately.Note also that the deployed API does not currently emit
application/x-ndjson, so the unsplit path reaches the JSON-to-NDJSON conversion rather than the streamed-body branch. That is not merely a spec omission: the service does not negotiate the response format onAcceptat all. It selects the format from theoutput_formatform field, and consultsAcceptonly to choosemultipart/mixedand to reject conflicting media types on multi-file uploads. NDJSON was therefore never going to arrive viaAccept.The service's
406 NOT_ACCEPTABLEon an unrecognizedAcceptis gated on multi-file uploads. This SDK sends a single file per request —PartitionParameters.filesis oneFiles, and the split-PDF hook sends one chunk per request — so that branch is unreachable from here and the unsplit path cannot raiseSDKErrorbecause of it. Server-side NDJSON support is tracked separately.Testing
_test_unstructured_client/unit/test_ndjson_elements_file.py— recombination across JSON-array / NDJSON / mixed chunk formats, order preservation, byte-exact payload round-trip, non-ASCII, temp-file lifecycle on success and failure, and regression guards for the header-spoofing and partial-output defects.pylint10.00/10;mypyclean.🤖 Generated with Claude Code