Repository navigation
fix(llm): stamp sliced-file checkpoints partial when some slices never complete - #4948
rahul05ranjan wants to merge 2 commits into
Conversation
…r complete An oversized splittable file is dispatched as several FileSlice chunks. A checkpoint written by a chunk whose slice set does not cover the whole file must be stamped partial, so an interrupted run cannot cache the parent file as complete and serve the truncated content forever (Graphify-Labs#4943). Co-authored-by: rahul05ranjan <11793455+rahul05ranjan@users.noreply.github.com>
…ts (Graphify-Labs#4943) Co-authored-by: rahul05ranjan <11793455+rahul05ranjan@users.noreply.github.com>
|
Thanks for the pull request, @rahul05ranjan. 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. |
|
Closing in favor of #4947 — it landed first and covers strictly more: I reproduced the interrupted-slice case with the issue's mock harness (partial_flag false → true, warm_uncached 0 → 1 on v8), and #4947's approach (hold a sliced parent's results until its last slice returns, stamp failed-slice parents partial) fixes the interrupted path plus the failed-slice path my interim-partial stamping didn't address. Happy to help review if useful. |
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
Stops extract_corpus_parallel from checkpointing a partly extracted oversized file as complete. When a file is split into several FileSlice chunks, each chunk's checkpoint now stamps the parent file as partial unless all of its slices are in that chunk. If the run dies mid-way (crash, kill, Ctrl-C), the next run misses the cache and re-dispatches the file instead of serving truncated content. A finished run still overwrites the stamp with a clean entry in its final save, and a new test interrupts after the first slice and checks that the cache reload misses.
No blocking issues surfaced. 2 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 1181 functions depend on the 303 functions this change touches.
Health — this change adds coupling hotspots:
- worse:
extract_corpus_parallel()— 29 callers, 12 callees
Verification — 1181 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: 801 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
59 of 489 test file(s) selected (12%) via static blast radius.
tests/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_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_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— impacttests/test_pdf_slicing.py— impacttests/test_pdf_token_estimate.py— impacttests/test_provider_registry.py— impact- … and 9 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?
An oversized splittable file is dispatched as several
FileSlicechunks (expand_oversized_files→ per-chunk_checkpoint_chunksaves). If the run is interrupted before every slice's chunk returns (crash, kill, or aKeyboardInterrupt-style abort), the checkpoint written by the slices that did finish is stamped clean, so the parent file sits in the semantic cache looking complete.check_semantic_cachethen reports zero uncached files and the content that was never extracted (everything past the completed slice) is silently omitted from every later graph.Fixes #4943.
_checkpoint_chunknow stamps a sliced filepartial: Truewhenever this chunk does not contain all of the file's slices (FileSlice.index/total), merged with the existing_partial_source_filestruncation set. Interrupted runs leave a partial entry thatload_cached/check_semantic_cachetreat as a miss, so the whole file re-dispatches next run. A fully completed run is unaffected: checkpoints stay conservative (same contract an LLM-truncated slice already has), and the final full save of a completed run — the non-mergesave_semantic_cachecall the CLI makes after the corpus run — overwrites the stamp with a clean complete entry.Type of change
Verification & Invariants
Invariant: a semantic cache entry for a file may only be served as complete when every slice of that file completed; otherwise the entry must carry
partial: True.partialflag machinery already existed for truncated responses).test_interrupted_multi_slice_checkpoint_is_partial_and_missed_on_reload); verified it fails withAssertionErroron the pre-fixllm.pyand passes with the fix.How was this tested?
Reproduced #4943's report first on the plain
v8checkout with the issue's mock harness (system Python 3.13-compatible interpreter,extract_files_directpatched; one slice succeeds, the second raisesKeyboardInterrupt):Then:
test_interrupted_multi_slice_checkpoint_is_partial_and_missed_on_reload(intests/test_chunking.py): passes with the fix,AssertionErrorwithout it.test_checkpoint_scopes_cache_writes_to_chunk_files,test_truncated_chunk_is_cached_partial_and_missed_on_reload(driven individually, since the full pytest suite is not runnable in my environment — the only stubbed import wastree_sitter, which these paths do not execute; the mocked-endpoint paths above are unaffected by it).check_semantic_cacheserves warm, including the tail-section nodes.Limitations: the partial stamp makes a killed-after-chunks-but-before-final-save run re-extract the sliced files next run — conservative by design, same recovery cost a truncated-response slice already incurs. No cache-format change, so old entries remain readable.
Graphify-specific checklist
uv run python -m tools.skillgen --bless) when changing their source fragments. — N/A:graphify/llm.py's_checkpoint_chunkis runtime code, not a skillgen source fragment (verified the generatedskill-*.mdartifacts do not embed this region), so no artifact regeneration is needed.Co-authored-bytrailer, per the repo's authorship convention).