fix(llm): stop bisecting hollow responses, retry the same chunk instead (#2880) - #2887
fix(llm): stop bisecting hollow responses, retry the same chunk instead (#2880)#2887chintondutta wants to merge 2 commits into
Conversation
…ad (Graphify-Labs#2880) _response_is_hollow() collapses two unrelated failure modes into one finish_reason="length" relabel: a genuine truncation (benefits from bisection — smaller input, shorter output, fits) and a hollow response (HTTP 200 with empty/unparseable content from a rate limit, transport hiccup, refusal, or reasoning-only reply). Bisecting a hollow response cannot recover it, since both halves come from the same misbehaving backend and come back hollow too — costing up to 2**max_retry_depth (15 at the default depth of 3) billed calls that are all guaranteed to fail. Tag hollow responses as finish_reason="hollow" instead of "length" at all 5 call sites, and give _extract_with_adaptive_retry a dedicated bounded same-chunk retry (3 attempts, small backoff) for that case before falling through to the existing give-up handling — bisection is never reached for a hollow response. Also expose max_retry_depth as GRAPHIFY_MAX_RETRY_DEPTH (previously a Python-API kwarg only, with no way for a graphify extract operator to lower it as a field mitigation), following the existing _resolve_max_retries() env-var convention. An explicit max_retry_depth= kwarg still wins over the env var.
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Formal verification. 1 change(s) tested, no difference found (not proven).
Graphify review — findings
Exposes GRAPHIFY_MAX_RETRY_DEPTH (via new _resolve_max_retry_depth) so operators can cap or disable chunk bisection depth without a code change. Splits hollow-response handling out of the bisection path: hollow results now set finish_reason="hollow" and get a small bounded number of same-chunk retries with backoff (_HOLLOW_RETRY_ATTEMPTS, _HOLLOW_RETRY_BACKOFF_SECONDS) across all backends instead of being relabeled as truncation and bisected. Updates the README env-var table, adaptive-retry docstrings, and tests accordingly (#2880).
Worth a look
- Claude backend returns non-OpenAI finish_reason value —
graphify/llm.py:1407· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 780 functions depend on the 304 functions this change touches.
Health — this change adds coupling hotspots:
- new:
deduplicate_entities()— 63 callers, 21 callees - new:
build_merge()— 46 callers, 14 callees - new:
extract_files_direct()— 17 callers, 20 callees - new:
extract_corpus_parallel()— 27 callers, 11 callees - new:
_call_claude_cli()— 31 callers, 9 callees - new:
dispatch_command()— 2 callers, 117 callees - new:
_extract_with_adaptive_retry()— 20 callers, 10 callees - new:
_call_llm()— 11 callers, 18 callees - …and 15 more — each is listed as a finding
Verification — 780 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: 511 function(s) in the blast radius were not formally verified this run
Formal verification
No difference found (not proven): No behavior difference found in \_bedrock\_response\_text (not a proof).
The verifier ran both versions of \_bedrock\_response\_text on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
Verification did not run: Verification did not run for \_call\_azure.
The verification could not execute (an environment/toolchain issue, not a statement about the code).
Guarantee: No guarantee, the check itself did not complete.
Note: Detail: harness produced no verdict (rc=124): timeout after 30s
Could not verify: Could not verify \_call\_bedrock.
The verifier did not have enough to check \_call\_bedrock, 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: not verifiable: all 200 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly ParamValidationError — names the real obstacle, not a sampling gap)
Verification did not run: Verification did not run for \_call\_claude.
The verification could not execute (an environment/toolchain issue, not a statement about the code).
Guarantee: No guarantee, the check itself did not complete.
Note: Detail: harness produced no verdict (rc=124): timeout after 30s
Could not verify: Could not verify \_call\_claude\_cli.
The verifier did not have enough to check \_call\_claude\_cli, 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: not verifiable: all 200 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly RuntimeError — names the real obstacle, not a sampling gap)
Verification did not run: Verification did not run for \_call\_openai\_compat.
The verification could not execute (an environment/toolchain issue, not a statement about the code).
Guarantee: No guarantee, the check itself did not complete.
Note: Detail: harness produced no verdict (rc=124): timeout after 30s
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: parameter `root` is annotated `Path` — outside the synthesizable primitive/collection set
Could not verify: Could not verify \_extract\_with\_adaptive\_retry.
The verifier did not have enough to check \_extract\_with\_adaptive\_retry, 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: parameter `root` is annotated `Path` — outside the synthesizable primitive/collection set
· 23 more finding(s) on lines outside this diff (see the check run).
…er vocabulary Addresses an automated review finding on Graphify-Labs#2887: the comment above _call_claude's finish_reason normalization claimed the "OpenAI-compat finish_reason vocabulary", which no longer literally covers "hollow" since that value isn't returned by any backend's actual API.
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Formal verification. 1 change(s) tested, no difference found (not proven).
Graphify review — findings
Splits hollow-response handling out from the truncation/bisection path in graphify/llm.py: _response_is_hollow results now tag finish_reason="hollow" (a graphify-internal sentinel applied uniformly across all 5 backends) and get a small bounded number of same-chunk retries with backoff instead of being bisected. Adds _resolve_max_retry_depth / GRAPHIFY_MAX_RETRY_DEPTH so operators can cap or disable truncation bisection without a code change, documented in the README env-var table. Updates backend call sites (_call_openai_compat, _call_claude, _call_claude_cli, _call_azure, _call_bedrock) and their comments accordingly, with matching test coverage.
Worth a look
- GRAPHIFY_MAX_RETRY_DEPTH=0 disables bisection but nothing wires the hollow-retry attempts —
graphify/llm.py:436· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review Execution auto-disposal is off for this run; enable it (with sandbox isolation) to have Graphify try to confirm or refute this automatically.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 780 functions depend on the 304 functions this change touches.
Health — this change adds coupling hotspots:
- new:
deduplicate_entities()— 63 callers, 21 callees - new:
build_merge()— 46 callers, 14 callees - new:
extract_files_direct()— 17 callers, 20 callees - new:
extract_corpus_parallel()— 27 callers, 11 callees - new:
_call_claude_cli()— 31 callers, 9 callees - new:
dispatch_command()— 2 callers, 117 callees - new:
_extract_with_adaptive_retry()— 20 callers, 10 callees - new:
_call_llm()— 11 callers, 18 callees - …and 15 more — each is listed as a finding
Verification — 780 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: 511 function(s) in the blast radius were not formally verified this run
Formal verification
No difference found (not proven): No behavior difference found in \_bedrock\_response\_text (not a proof).
The verifier ran both versions of \_bedrock\_response\_text on many inputs and saw identical behavior every time. Strong evidence the change is safe, but evidence, not a proof.
Guarantee: Empirical: differential testing (both versions run on many generated inputs). A divergence on an untested input remains possible, so this is 'no counterexample found', not 'proven equivalent'.
Note: An input the sampler did not try could still differ.
Verification did not run: Verification did not run for \_call\_azure.
The verification could not execute (an environment/toolchain issue, not a statement about the code).
Guarantee: No guarantee, the check itself did not complete.
Note: Detail: harness produced no verdict (rc=124): timeout after 30s
Could not verify: Could not verify \_call\_bedrock.
The verifier did not have enough to check \_call\_bedrock, 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: not verifiable: all 200 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly ParamValidationError — names the real obstacle, not a sampling gap)
Verification did not run: Verification did not run for \_call\_claude.
The verification could not execute (an environment/toolchain issue, not a statement about the code).
Guarantee: No guarantee, the check itself did not complete.
Note: Detail: harness produced no verdict (rc=124): timeout after 30s
Could not verify: Could not verify \_call\_claude\_cli.
The verifier did not have enough to check \_call\_claude\_cli, 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: not verifiable: all 200 sampled inputs raised on both versions — the function never executed, so 'no divergence' would be vacuous (mostly RuntimeError — names the real obstacle, not a sampling gap)
Verification did not run: Verification did not run for \_call\_openai\_compat.
The verification could not execute (an environment/toolchain issue, not a statement about the code).
Guarantee: No guarantee, the check itself did not complete.
Note: Detail: harness produced no verdict (rc=124): timeout after 30s
· 23 more finding(s) on lines outside this diff (see the check run).
|
Looked into both flagged findings, neither holds up on inspection:
Both look like false positives from the unverified/consensus-only findings. Treating this as clear to merge on these two points. |
|
Thanks for tackling this @chintondutta - your diagnosis was right. We landed a fix for #2880 in v0.9.48 via #2888, which took the same approach (a distinct hollow finish-reason routed through a bounded same-chunk retry) and additionally kept the retry inside the timeout-recovery guard and honored GRAPHIFY_MAX_RETRY_DEPTH=0 for the hollow path too. Closing this as superseded, but the contribution and the framing were genuinely helpful. Release: https://github.com/Graphify-Labs/graphify/releases/tag/v0.9.48 |
Summary
Fixes #2880.
_response_is_hollow()collapses two unrelated failure modes into onefinish_reason="length"relabel:max_completion_tokensmid-output. Bisecting is the correct recovery: smaller input → shorter output → fits.For the hollow case, bisecting cannot converge: both halves come from the same misbehaving backend and come back hollow too. Every hollow response costs
2**max_retry_depthbilled calls instead of one — with the defaultmax_retry_depth=3, up to 15 calls per chunk, all of which fail.Changes
_call_openai_compat,_call_claude,_call_claude_cli,_call_azure,_call_bedrock) now tag a hollow response asfinish_reason="hollow"instead of overloading"length"._extract_with_adaptive_retrygives"hollow"its own bounded recovery: up to_HOLLOW_RETRY_ATTEMPTS(3) retries on the same, unsplit chunk with backoff, before giving up on the chunk (marked partial, same "don't cache as authoritative, re-dispatch next run" handling the truncation give-up path already uses). Bisection is never reached for a hollow response.max_retry_depthis now also resolvable from a newGRAPHIFY_MAX_RETRY_DEPTHenv var (previously a Python-API kwarg only, so agraphify extractoperator had no way to lower it as a field mitigation without a code change). An explicitmax_retry_depth=kwarg still wins over the env var. Documented in the README's env var table.Scope
This covers asks 1 and 3 from #2880. Asks 2 (a per-chunk/global call budget) and 4 (emitting the
tokens:cost line on abnormal termination) touchcli.py's run loop and signal handling and are a bigger, separate change — planning to follow up with those in a second PR once this one lands, so they don't block the core fix.Test plan
test_adaptive_retry_bisects_on_hollow_ollama_response→test_adaptive_retry_retries_same_chunk_on_hollow_response, asserting the chunk is retried at its original size and never split.test_adaptive_retry_gives_up_on_persistently_hollow_chunk(never bisects, gives up after_HOLLOW_RETRY_ATTEMPTS, result marked partial).test_adaptive_retry_backs_off_between_hollow_retries(guards against hot-looping)._call_openai_compathollow-relabel tests to assertfinish_reason == "hollow".test_resolve_max_retry_depth_default_and_envandtest_extract_corpus_parallel_explicit_max_retry_depth_wins_over_env.uv run --frozen pytest tests/ -q --tb=short— full suite green (4787 passed, 11 skipped, pre-existing skips unrelated to this change).uv run ruff check graphify tests— clean.uv run pyright graphify/llm.py— same 11 pre-existing errors as onv8before this change (confirmed viagit stash), none introduced by this PR.