Conversation
…hunk (Graphify-Labs#3142) A chunk that times out at every recursion depth used to re-pay the full GRAPHIFY_API_TIMEOUT on each of up to 2**max_depth attempts (600s x 15 attempts = up to 2.5h at the default settings for claude-cli). Track a shared wall-clock deadline for the whole split subtree instead of granting each split a fresh full timeout. Once the deadline passes, a further timeout gives up immediately rather than committing to another full-length attempt.
![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.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Graphify review — findings
Adds a shared wall-clock deadline to _extract_with_adaptive_retry so a subtree of timeout-driven splits shares one GRAPHIFY_API_TIMEOUT budget anchored at the top-level call rather than each split re-paying the full timeout. Once that deadline passes, a further timeout now gives up on the remaining chunk immediately (returning empty results with finish_reason="stop" and a stderr warning) instead of bisecting into up to 2**max_depth more full-length attempts.
Worth a look
- Deadline anchored to _resolve_api_timeout() but each split re-pays a full timeout, so budget may never be exceeded before first split —
graphify/llm.py:2378· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 823 functions depend on the 297 functions this change touches.
Health — this change adds coupling hotspots:
- new:
deduplicate_entities()— 63 callers, 21 callees - new:
build_merge()— 53 callers, 13 callees - new:
extract_files_direct()— 17 callers, 20 callees - new:
extract_corpus_parallel()— 26 callers, 11 callees - new:
_call_claude_cli()— 31 callers, 9 callees - new:
_extract_with_adaptive_retry()— 23 callers, 11 callees - new:
dispatch_command()— 2 callers, 122 callees - new:
_call_llm()— 11 callers, 18 callees - …and 16 more — each is listed as a finding
Verification — 823 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: 532 function(s) in the blast radius were not formally verified this run
· 24 more finding(s) on lines outside this diff (see the check run).
43c71c3 to
24e84c4
Compare
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 3 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Adds a shared wall-clock deadline to _extract_with_adaptive_retry so a chunk that keeps timing out no longer re-pays the full GRAPHIFY_API_TIMEOUT on every bisection: the top-level call anchors one budget (time.monotonic() + _resolve_api_timeout()) and all splits inherit the same absolute deadline. Once that budget is spent, a further timeout gives up on the remaining chunk — returning an empty result with finish_reason="stop" and a stderr warning — instead of spawning up to 2**max_depth more full-length attempts (previously up to ~2.5h for the 600s default at max_depth=3). Context-exceeded splits are unaffected; only timeouts honor the deadline.
Worth a look
- Deadline check gives up before ever splitting when timeout exceeds budget —
graphify/llm.py:2378· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Expired shared deadline still allows sibling retry to start —
graphify/llm.py:2422· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Expired timeout budget still allows right sibling extraction —
graphify/llm.py:2422· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 823 functions depend on the 297 functions this change touches.
Health — this change adds coupling hotspots:
- new:
deduplicate_entities()— 63 callers, 21 callees - new:
build_merge()— 53 callers, 13 callees - new:
extract_files_direct()— 17 callers, 20 callees - new:
extract_corpus_parallel()— 26 callers, 11 callees - new:
_call_claude_cli()— 31 callers, 9 callees - new:
_extract_with_adaptive_retry()— 23 callers, 11 callees - new:
dispatch_command()— 2 callers, 122 callees - new:
_call_llm()— 11 callers, 18 callees - …and 16 more — each is listed as a finding
Verification — 823 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: 532 function(s) in the blast radius were not formally verified this run
Formal verification
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
· 24 more finding(s) on lines outside this diff (see the check run).
Graphify's own review bot flagged this PR: the shared subtree deadline was only checked reactively, after a timeout occurred. A sibling split reached after the budget was already spent elsewhere in the same subtree would still start (and pay for) its own fresh full-length attempt before its own reactive check caught up and gave up on it -- weakening the "one shared budget per subtree" guarantee this PR set out to establish. Add a proactive check at the top of each split: if the shared deadline has already passed before this split's own attempt has even started, skip the attempt outright instead of starting a new one the budget can no longer afford. (A related finding, that the very first depth-0 attempt "gives up before ever splitting" when it alone consumes the whole budget, turned out on investigation to be inherent to the single-timeout-budget design rather than separately fixable: if the original attempt's own client-side timeout reaches the full shared budget, there is by definition no time left for any further split regardless of where in the call tree that's decided. Not changed.)
|
Addressed the review bot's 3 findings, in 7cc0f14. Findings #2/#3 ("expired shared deadline still allows sibling retry to start" / "right sibling extraction") — real gap, fixed. The deadline was only checked reactively, after a timeout occurred. A sibling split reached after the budget was already spent elsewhere in the same subtree would still start (and pay for) its own fresh full-length attempt before its own reactive check caught up. Added a proactive check at the top of each split: if the deadline has already passed before this split's own attempt has even started, it's skipped outright. Finding #1 ("deadline check gives up before ever splitting when timeout exceeds budget") — investigated, but this turns out to be inherent to the single-shared-budget design rather than a separately fixable defect. If the depth-0 attempt's own client-side timeout reaches the full shared budget (which is the common case — a timeout fires at essentially its full configured duration, and the client timeout and the deadline both derive from the same Added |
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 2 advisory finding(s) below merit a look before merge.
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Adds a shared subtree wall-clock timeout budget to _extract_with_adaptive_retry: the top-level call anchors one GRAPHIFY_API_TIMEOUT deadline that every recursive split inherits, instead of each attempt re-paying the full timeout. Once the deadline passes, a timeout gives up on the chunk reactively (after its attempt) and a not-yet-started split is skipped proactively (before its attempt), both returning an empty result and a stderr warning. This caps the worst case for a chunk that keeps timing out at roughly one timeout rather than up to 2**max_depth of them (up to 2.5h for the 600s default at max_depth=3).
Worth a look
- Adaptive retry deadline skips splits even without a timeout —
graphify/llm.py:2326· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
- Expired timeout deadline skips splits even when no timeout occurred —
graphify/llm.py:2328· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 826 functions depend on the 300 functions this change touches.
Health — this change adds coupling hotspots:
- new:
deduplicate_entities()— 63 callers, 21 callees - new:
build_merge()— 53 callers, 13 callees - new:
extract_files_direct()— 17 callers, 20 callees - new:
extract_corpus_parallel()— 26 callers, 11 callees - new:
_call_claude_cli()— 31 callers, 9 callees - new:
_extract_with_adaptive_retry()— 24 callers, 11 callees - new:
dispatch_command()— 2 callers, 122 callees - new:
_call_llm()— 11 callers, 18 callees - …and 16 more — each is listed as a finding
Verification — 826 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit 33362d9 (diverged from this PR's base — delta is approximate).
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: 535 function(s) in the blast radius were not formally verified this run
Formal verification
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
· 24 more finding(s) on lines outside this diff (see the check run).
The proactive budget check in _extract_with_adaptive_retry skipped a
not-yet-started split purely based on wall-clock time (time.monotonic()
>= _deadline), regardless of whether any real timeout had ever happened
in the subtree. A context-exceeded or truncation ("length") split could
therefore be skipped -- returning an empty result instead of recovering
via bisection -- just because enough time had elapsed since the deadline
was anchored, contradicting the documented intent that only a real
timeout should spend the shared budget.
Thread a _timeout_hit flag (a one-element list shared by reference
across the recursion) that is only set True when a real timeout
exception is observed, and gate the proactive skip on it.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PhuCVHf7XKmFmUMULkczFa
|
Pushed Root cause: the proactive budget check ( Fix: thread a |
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Formal verification. No changes could be formally verified in this run.
Graphify review — findings
Anchors the adaptive-retry timeout to a single shared wall-clock budget per subtree so a repeatedly-timing-out chunk can no longer re-pay the full GRAPHIFY_API_TIMEOUT on each of up to 2**max_depth splits (up to ~2.5h at the 600s/depth-3 defaults). After an attempt times out and the budget is spent, _extract_with_adaptive_retry gives up and returns an empty result instead of bisecting further; a not-yet-started sibling split is also skipped proactively once a real timeout has been observed elsewhere in the subtree (tracked via a shared _timeout_hit flag). Ordinary elapsed time from successful calls, context-exceeded splits, or truncation splits never trips the skip on its own.
No blocking issues surfaced. 6 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 829 functions depend on the 303 functions this change touches.
Health — this change adds coupling hotspots:
- new:
deduplicate_entities()— 63 callers, 21 callees - new:
build_merge()— 53 callers, 13 callees - new:
extract_files_direct()— 17 callers, 20 callees - new:
extract_corpus_parallel()— 26 callers, 11 callees - new:
_call_claude_cli()— 31 callers, 9 callees - new:
_extract_with_adaptive_retry()— 25 callers, 11 callees - new:
dispatch_command()— 2 callers, 122 callees - new:
_call_llm()— 11 callers, 18 callees - …and 16 more — each is listed as a finding
Verification — 829 functions in the blast radius were not formally verified this run (proofs are advisory here).
Health delta baseline: last indexed commit fe66389 (diverged from this PR's base — delta is approximate).
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: 538 function(s) in the blast radius were not formally verified this run
Test selection
Test selection
35 of 232 test file(s) selected (15%) via static blast radius.
tests/test_backend_extras.py— impacttests/test_build.py— impacttests/test_build_merge_hyperedges_and_prune.py— impacttests/test_build_merge_shrink_guard.py— impacttests/test_carried_hyperedge_remap.py— impacttests/test_charmap_encoding.py— impacttests/test_chunking.py— impacttests/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_evidence_binding.py— impacttests/test_file_slice.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_label_retry.py— impacttests/test_labeling.py— impacttests/test_llm_backends.py— impact, changed-testtests/test_llm_parser.py— impacttests/test_llm_parser_reasoning.py— impacttests/test_no_dedup_flag.py— impacttests/test_non_string_node_ids.py— impacttests/test_ollama.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— impacttests/test_prs.py— impacttests/test_prune_sweeps_orphans.py— impacttests/test_semantic_fragment_sanitize.py— impact
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\_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
· 24 more finding(s) on lines outside this diff (see the check run).
1 participant
Fixes #3142.
Bug
_extract_with_adaptive_retrybisects a chunk onTimeoutExpired(and otherrecognized timeout errors) and retries each half. Every retry re-pays the
full
GRAPHIFY_API_TIMEOUTfrom scratch, so a chunk that keeps timing outacross the whole cascade can burn up to
2**max_depthfull timeouts —600s x (1+2+4+8) = 9,000s(~2.5h) at the default timeout andmax_depth=3.This is most visible on
--backend claude-cli, where each call spawns a fullnested
claude -psession and is far slower than a plain HTTP completion, sothe default 600s is exceeded in normal use rather than as an edge case. It
reads as a hang: the only signal between "chunk N/M done" and the eventual
give-up is silence.
The recursion depth was already capped, so this isn't unbounded — it's a
bounded cascade whose bound is much larger than anyone intends.
Fix
Give the whole split subtree for one original chunk a single shared
wall-clock deadline (anchored to one
GRAPHIFY_API_TIMEOUTallowance),instead of handing every split a fresh full timeout:
_deadline = time.monotonic() + GRAPHIFY_API_TIMEOUTand threads it through every recursive call in the subtree (both the
timeout-bisection path and the truncation/length-retry path, so a later
timeout deeper in an already-truncating subtree still respects it).
cascade gives up immediately instead of committing to another full-length
attempt — same outcome as hitting
max_depth, just reached by budgetinstead of depth.
this only changes what happens after a
TimeoutExpired-class exception.Worst case is now bounded by roughly one timeout's worth of wall time per
original chunk instead of
2**max_depthtimeouts.Testing
test_adaptive_retry_stops_when_timeout_budget_exhausted: mockstime.monotonic()to simulate the deadline being exceeded right after thefirst timeout, and asserts the cascade gives up after exactly 1 attempt
(previously it would have kept splitting and retrying).
tests/test_llm_backends.pysuite (104 tests, including the4 existing timeout/adaptive-retry tests) — all pass unchanged, since those
fakes complete instantly and never approach the default 600s budget.
ruff checkon both changed files — clean.a large, pre-existing batch of unrelated failures there trace back to a
missing
tree-sitterinstall in my environment, reproducible on a cleancheckout of
v8with no changes applied, not to this diff.I used AI-assisted tooling to help navigate the codebase and draft this
change, but the diagnosis of the fix approach and the change itself were
reviewed and verified by me before opening this PR.