Repository navigation
fix(llm): cancel queued chunks when on_chunk_done raises - #5187
MaxWang007 wants to merge 1 commit into
Conversation
extract_corpus_parallel submits every chunk before handling callbacks. The executor then waits for that whole queue, so a callback error still runs the model calls that have not started. Cancel those futures, let in-flight calls finish, and re-raise the original exception (Graphify-Labs#5184). Co-Authored-By: Cursor <cursoragent@cursor.com>
|
Thanks for the pull request, @MaxWang007. A maintainer will review it soon. Want to talk it through while it is in review? Come join us on our Discord server. For longer-form discussion there is also GitHub Discussions. A couple of things that speed up review: make sure the test suite passes on Python 3.10 and 3.13, and that the change keeps extraction deterministic. |
github-actions
Bot
added
the
contributor
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
No issues found by static checks — no coupling regressions or blocking issues in the code graph. That is not the same as safe to merge: see what was not checked.
Not checked: tests were not run; formal verification proved 0 of 1 changed function(s) (1 not verified).
Formal verification. PR-changed functions: 0/1 verified (0 proven, 0 may-equivalent, 0 distinguished) · 1 not verified (1 unsupported).
Not verified on this run: extract\_corpus\_parallel (unsupported).
Graphify review — findings
Cancels still-queued chunks in extract_corpus_parallel when handling a finished chunk raises an Exception, such as an on_chunk_done callback failure, so queued model calls no longer run after the callback has failed. Chunks already running finish while the pool drains, and the original exception propagates unchanged. A new test uses a two-worker barrier to show the callback's error surfaces and that chunks queued behind the in-flight pair are never called.
No blocking issues surfaced. 2 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1277 functions depend on the 308 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
extract_corpus_parallel()— 32 callers, 15 callees
Verification — 1277 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 901 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
66 of 564 test file(s) selected (12%) via static blast radius.
tests/test_5072_splittable_under_cap_retry.py— impacttests/test_backend_env_isolation.py— impacttests/test_backend_extras.py— impacttests/test_backend_opt_in.py— impacttests/test_build.py— impacttests/test_build_located_semantic_identity.py— impacttests/test_build_merge_dedup_scope.py— impacttests/test_build_merge_ghost_fold_4955.py— impacttests/test_build_merge_hyperedges_and_prune.py— impacttests/test_build_merge_shrink_guard.py— impacttests/test_c_include_resolution.py— impacttests/test_carried_hyperedge_remap.py— impacttests/test_charmap_encoding.py— impacttests/test_chunking.py— impact, changed-testtests/test_claude_cli_backend.py— impacttests/test_corrupt_graph_json.py— impacttests/test_cpp_forward_declarations.py— impacttests/test_cross_extension_reexport_self_cycle.py— impacttests/test_dedup.py— impacttests/test_dedup_remaps_hyperedges.py— impacttests/test_dedup_shrink_refuses_force_write.py— impacttests/test_dedup_survivor_richness.py— impacttests/test_evidence_binding.py— impacttests/test_ext_tagged_id_clash.py— impacttests/test_file_id_collision_4854.py— impacttests/test_file_slice.py— impacttests/test_file_truncation_warning.py— impacttests/test_global_graph.py— impacttests/test_go_qualified_resolution.py— impacttests/test_hyperedge_member_shapes.py— impacttests/test_image_vision.py— impacttests/test_injection_sentinel_coverage.py— impacttests/test_issue_3472_source_file_collision.py— impacttests/test_js_member_collisions.py— impacttests/test_label_retry.py— impacttests/test_labeling.py— impacttests/test_llm_backends.py— impacttests/test_llm_null_token_counts.py— impacttests/test_llm_parser.py— impacttests/test_llm_parser_reasoning.py— impacttests/test_merge_sweeps_reextract_orphans.py— impacttests/test_no_dedup_flag.py— impacttests/test_non_string_node_ids.py— impacttests/test_ollama.py— impacttests/test_ollama_ctx_sizing.py— impacttests/test_ollama_live.py— impacttests/test_ollama_native_chat.py— impacttests/test_ollama_retry_cap.py— impacttests/test_oversized_document_slicing.py— impacttests/test_partial_cache.py— impact- … and 16 more
Selection is safe under the controlled-regression assumption; always-run tests + a periodic full run are the backstops. Advisory — it never changes the check verdict.
Formal verification
Could not verify: Could not verify extract\_corpus\_parallel.
The verifier did not have enough to check extract\_corpus\_parallel, so it is saying so rather than guessing. No false assurance is the whole point.
Guarantee: No guarantee either way, this is an honest abstention, not a pass.
Note: Reason: no capturable inputs from the test suite; property tier: parameter `on_chunk_done` is annotated `Callable | None` — outside the synthesizable primitive/collection set
· 1 more finding(s) on lines outside this diff (see the check run).
What does this PR do?
extract_corpus_parallelsubmits every chunk before it runson_chunk_done. When that callback raises, the executor still waits for the whole queue, so the remaining model calls keep going (#5184).Cancel futures that have not started, let calls that are already running finish, and re-raise the original callback exception.
Fixes #5184
Type of change
Verification & Invariants
A callback failure must not start model calls that are still queued. Work already in flight may finish. The exception that leaves the function is the callback's exception.
A worker can still pick up one more chunk in the gap after a call returns and before the callback raises. That call has started, so it is not cancelled. Chunks still sitting in the queue are. The one-worker path already stops at the callback, because it never pre-submits.
How was this tested?
Fake model, no network. The new test failed on
v8(52cb282, all six chunks ran) and passes with this change. Repeated it 8 times.229 passed, 1 skipped in the chunking and LLM backend suites.
Graphify-specific checklist
uv run python -m tools.skillgen --bless) when changing their source fragments.