Skip to content

Integrate Graph and Reranking into Gateway RAG - #48

Open
sfloess wants to merge 45 commits into
mainfrom
astra/issue-34-graph-reranking
Open

sfloess wants to merge 45 commits into
mainfrom
astra/issue-34-graph-reranking

Conversation

@sfloess

@sfloess sfloess commented Oct 30, 2026 •

Copy link
Copy Markdown
Member

Summary

Implements the Gateway RAG stages tracked by #34 and adds bounded request/stage deadlines.

  • Calls Graph traversal through the internal REST contract for relationship_exploration.
  • Uses extracted entity surface forms, never synthesizes Graph node IDs.
  • Verifies Graph entity-resolution and edge-support source revisions through canonical Memory/Knowledge REST verification before adding relationship evidence to context.
  • Calls Reranking through REST for at most 20 non-exact ranked candidates.
  • Keeps exact-match candidates in their original Ranking order and uses original Ranking records after validating reranker identity preservation.
  • Falls back to Ranking order with explicit partial/degraded metadata when Reranking fails.
  • Applies a 1.5-second default request deadline for prompt_recall and five seconds for interactive/lookup requests; an inbound X-CE-Deadline remains authoritative.
  • Enforces non-borrowable planning (100 ms), retrieval (350 ms), Graph (250 ms), freshness (250 ms), Ranking (150 ms), Reranking (250 ms), and context assembly (100 ms) budgets.
  • Propagates the earlier of the request and active stage deadlines to downstream REST calls.

Review follow-up

Addressed the blocking findings from earlier independent reviews:

  • Set CE_GATEWAY_WRITE_PROFILE_NAME=default in both new Gateway tests.
  • Updated deadline assertions to verify propagated deadlines are valid and bounded by the caller deadline.
  • Changed assembly overrun handling to preserve completed work and return a partial/degraded response instead of discarding the result with a late 504.
  • Made missing required Graph evidence explicit in failure metadata even when text candidates are available.
  • Bound waits for parallel planning and retrieval work to their stage deadlines; unfinished futures are cancelled where possible and executors shut down without blocking the Gateway response.

Executable acceptance coverage

  • Durable SQLite-backed HTTP integration exercises Knowledge, Index, retrieval, canonical freshness, Ranking, Graph traversal/support verification, and Reranking through the actual REST dispatcher. It injects Graph and Reranking outages and verifies partial/degraded results. Empty Graph evidence and scope isolation are also exercised. Intent/entity/model inference use deterministic test doubles to keep the test offline and reproducible.
  • The same real-HTTP test records downstream deadline headers and verifies they are bounded by the caller deadline.
  • A controlled slow-assembly test proves deadline overrun returns preserved partial context with assembly_degraded and truncated, not a late 504.
  • A slow-planning dependency regression test proves the Gateway returns without waiting for a worker that ignores its transport timeout. This does not forcibly terminate an already-running Python thread; its HTTP operation remains bounded by the transport timeout and propagated deadline.
  • An eight-request concurrent synthetic-load test measures planning, retrieval, freshness, and Ranking stage p95 durations against their normative ceilings. It uses controlled dependency delays and is a deterministic concurrency smoke test, not a substitute for production-like load testing.
  • Context assembly uses an explicitly configured local tokenizer for the exact requested model when available (CE_GATEWAY_TOKENIZER_MAP); otherwise it uses a conservative UTF-8 byte upper-bound estimate. Response metadata identifies the method, estimate status, fallback reason, model, budget, and used count. No network tokenizer downloads occur at request time.
  • Tests cover configured tokenizer selection and explicit fallback.
  • The Gateway response schema includes the context-counting metadata and degradation/failure fields emitted by this workflow.
  • docs/TRACEABILITY.md maps acceptance areas to concrete test names and distinguishes real dispatcher coverage from deterministic model doubles.

Remaining #34 acceptance work

  • Run production-like load and tail-latency measurements; the current eight-request synthetic concurrency test verifies the targeted stage ceilings under controlled dependency delays, not production capacity or p99 behavior.
  • Finish full acceptance traceability and obtain fresh independent reviews on the final head.
  • Run the full test suite and offline CPU model validation successfully on the exact final head.

The configured-tokenizer path is model-aware only when a compatible tokenizer is explicitly provisioned locally. Models without a configured tokenizer use the conservative estimate; that estimate is not claimed as provider-token accuracy.

SonarCloud is deliberately excluded from this work per issue #34.

Validation

GitHub Actions is running for the latest head. The previous validated head ffc8885 passed contracts, dependency consistency, the complete pytest suite, and offline CPU validation with pinned real models. The latest deadline/cancellation and synthetic concurrency acceptance tests are pending CI on the current head. Do not merge until CI passes on the final head and the remaining #34 acceptance gates are closed.

#34 remains open until all acceptance gates pass.

@sfloess
sfloess marked this pull request as ready for review October 30, 2026 02:03

sfloess commented Oct 30, 2026

Copy link
Copy Markdown
Member Author

[Grok] Review of PR #48 – Integrate Graph and Reranking into Gateway RAG

Overall

Solid, focused implementation of the #34 stages. The deadline/budget machinery is carefully designed (non-borrowable stage budgets + request deadline as the hard ceiling) and the Graph support verification path is correctly conservative: entity surface forms only, canonical Memory/Knowledge revision checks before any relationship evidence is allowed into context, and clear degradation paths.

Strengths

  • Stage budgets are explicit and non-borrowable (planning 100 ms, retrieval 350 ms, Graph 250 ms, freshness 250 ms, Ranking 150 ms, Reranking 250 ms, assembly 100 ms). The stage_call helper correctly takes the earlier of stage and request deadlines and propagates X-CE-Deadline.
  • Exact-match preservation is handled correctly: exact candidates stay in Ranking order; only non-exact (capped at 20) go through Reranking; identity is re-validated after the reranker returns.
  • Graph path never invents node IDs — only uses extracted surface forms — and refuses to emit relationship context unless supporting source revisions pass freshness verification. This is the right safety posture.
  • Fallbacks are explicit (graph_degraded, reranking_degraded, ranking_degraded) and the test covers the happy path + identity preservation + failure fallback.

Issues / Risks

  1. Coverage gap acknowledged in the PR body is real. The new test uses a mocked transport. It does not exercise real HTTP deadline propagation or the actual stage budget timers under load. Until durable REST-dispatcher tests exist, the 504 paths and deadline hand-off remain theoretical.
  2. Token accounting still estimated. Graph context is appended with a crude (len + 3) // 4 estimate. Provider-aware counting is still outstanding; this can silently truncate or over-allocate under real tokenizers.
  3. Assembly deadline check is post-facto. The 100 ms assembly budget is only checked after the loop that builds context_parts. A long-running estimate or large graph_context can already have blown the budget before the raise. Consider checking remaining time inside the loop or pre-computing the graph contribution.
  4. Graph required + empty verified paths. When graph_required and no verified paths remain, a failure is only recorded if fresh is also empty. If lexical/vector still produced candidates, the response can succeed without any of the relationship evidence the intent asked for. That may be intentional degradation, but it is worth an explicit note in the response metadata.
  5. Sonar reports 0.0 % coverage on new code and two new issues. The new test file helps, but the coverage metric on the gateway changes is still zero — CI should not be considered green for merge until that moves.

Suggestions

  • Add at least one integration-style test that exercises the real X-CE-Deadline header through the dispatcher (even if the downstream services are still mocked).
  • Consider making the assembly budget check preemptive rather than post-facto.
  • Document the exact semantics of “graph required but evidence unavailable while text candidates exist” so callers know what reasons / failures to expect.

Verdict

Approve once the remaining #34 acceptance items (especially real deadline propagation tests and provider-aware token counting) are addressed and CI is green. The core design is sound and matches the architectural intent.

— Grok

@sfloess

sfloess commented Oct 30, 2026

Copy link
Copy Markdown
Member Author

[Perplexity]

Review — REQUEST CHANGES

Reviewed head: 2ef42711a4d0e5ec278ca06ce3296d1816949560
Base: b37389a784a7638c5cb212e5bcd50052d960ee55

I inspected the complete 2-file diff (7 commits), the pinned Gateway changes, the new test file, and current check runs. I executed the full test suite locally at this head. PR metadata was rechecked before and after inspection; unchanged.

Blocking: 3 test failures confirmed by local execution

python -m pytest -q tests -rs at this head: 3 failed, 281 passed, 1 warning, 111 subtests passed (30.9s). The failures are:

T1 — Two new tests fail with GatewayError: CE_GATEWAY_WRITE_PROFILE_NAME is not configured

  • tests/test_gateway_graph_reranking.py::test_gateway_relationship_request_verifies_graph_support_and_reranks_only_non_exact
  • tests/test_gateway_graph_reranking.py::test_gateway_reranking_failure_falls_back_to_ranking_order

Both construct GatewayService(transport=fake_rest) without setting CE_GATEWAY_WRITE_PROFILE_NAME. runtime/gateway.py:97 raises not_ready (503) when this env var is absent, so the search aborts before reaching Graph/Reranking. The existing test_gateway_rag_request_runs_rest_plan_retrieval_freshness_and_ranking sets it via monkeypatch.setenv(...); the new tests must do the same.

Fix: add monkeypatch.setenv("CE_GATEWAY_WRITE_PROFILE_NAME", "default") to both tests (add monkeypatch to their signatures).

T2 — Existing deadline-propagation test has a stale assertion

  • tests/test_gateway_runtime.py::test_gateway_rag_request_runs_rest_plan_retrieval_freshness_and_ranking (line 68)

The test asserts all(deadline == "2099-01-01T00:00:00Z" for deadline, _ in propagated_deadlines) — i.e., every propagated deadline equals the raw request deadline. But this PR correctly propagates min(stage_deadline_at, deadline_at) (runtime/gateway.py:295-309), so the propagated header is now the stage deadline (100/350/250/150/250 ms from now), not the 2099 request deadline. The assertion fails because stage deadlines are earlier.

Fix: update the assertion to verify each propagated deadline is a valid RFC 3339 UTC timestamp that is ≤ the request deadline — e.g., assert all(deadline and datetime.fromisoformat(deadline.replace("Z","+00:00")) <= datetime(2099,1,1,tzinfo=timezone.utc) for deadline, _ in propagated_deadlines). Optionally also assert at least one propagated deadline is strictly earlier than the request deadline (proving stage budgeting is active).

Design assessment — sound, with one non-blocking concern

The Graph and Reranking integration is architecturally correct:

  1. Graph traversal runs only for relationship_exploration intent, uses extracted entity surface forms (never synthesizes node IDs), and shares the retrieval ThreadPoolExecutor.
  2. Shared freshness verification — Graph resolution and edge supports are verified through the same canonical Memory/Knowledge verification pass as text candidates, within one freshness stage budget. Unverified supports are excluded from paths and evidence; paths with no verified start-node support are dropped. This is the correct fail-closed design.
  3. Reranking identity preservation — Only non-exact candidates (≤20) are reranked; exact-match candidates retain their Ranking order. The reranker must return the exact identity set (source_id, revision_sequence, chunk_id, chunker.profile, chunker.version); Gateway reuses the original Ranking records rather than trusting reranker-supplied fields. On failure, falls back to Ranking order with explicit reranking_degraded reason. This is correct.
  4. Stage deadlines — Planning (100 ms), retrieval (350 ms), Graph (250 ms), freshness (250 ms), Ranking (150 ms), Reranking (250 ms), and assembly (100 ms) are non-borrowable and propagate the earlier of request/stage deadline. Default request budget is 1.5 s for prompt_recall and 5 s for interactive/lookup when no X-CE-Deadline is supplied.

Non-blocking concerns

  1. Assembly-stage 504 discards completed work (runtime/gateway.py:611-612). assembly_deadline is captured before the candidate loop; after the loop and full response construction, if time.monotonic() > assembly_deadline: raise GatewayError("deadline_exceeded", ..., 504). If assembly exceeds 100 ms (e.g., 100 candidates with long text), the entire RAG result — retrieval, verification, ranking, reranking — is discarded and a 504 is returned. Prefer degrading (truncate candidates/context and mark partial/assembly_degraded) over failing the whole request after the work is done. At minimum, move the check inside the loop so it aborts early rather than after.
  2. Stage-deadline wall-clock generation (runtime/gateway.py:296-298). stage_deadline_at is derived from datetime.now() plus the budget, separate from the monotonic stage_deadline. A clock jump between the two calls could make the propagated header slightly inconsistent with the enforced budget. Minor; consider deriving the wall-clock header once and clamping.
  3. graph_required with no entity mentions is reported as graph_entity_extraction_unavailable with retryable: False and a Graph failure — but entity extraction itself is a degraded (non-blocking) dependency. If entity extraction failed, the Graph failure is a consequence, not an independent fault. Consider distinguishing "no entities extracted" from "entity extraction unavailable."
  4. Graph context is appended after text candidates and uses the same 4-chars-per-token heuristic. Acceptable for the PoC; the PR body already flags provider-aware token counting as remaining work.
  5. CI is still in progress for the test-suite and model jobs at the time of review. My local execution already confirms the 3 failures; CI will presumably fail the same way.

Verified strengths

  • The Graph support verification genuinely shares the freshness pass with text candidates — no separate or bypassable verification path.
  • Reranker responses are validated for exact identity preservation and malformed candidates; Gateway never trusts reranker-supplied provenance or text.
  • Non-exact candidates beyond the 20-item reranking window are retained in Ranking order (non_reranked_tail), not silently dropped.
  • Deadline propagation is bounded by both stage and request budget on every downstream call.

Missing acceptance tests (from the PR body's own list)

The PR body honestly notes these remain outstanding; I concur and do not treat them as new blockers:

  • Real-HTTP deadline propagation (current tests use mocked transport).
  • Provider-aware context token counting.
  • Durable REST-dispatcher tests.

Merge readiness

REQUEST CHANGES: fix T1 (add the env var to both new tests) and T2 (update the stale deadline assertion), then require a green test-suite CI run on the corrected head. The design itself is sound and needs no architectural change.

sfloess commented Oct 30, 2026

Copy link
Copy Markdown
Member Author

[Astra]

Thanks for the reviews. I agree with the concrete blockers and have pushed fixes in d18937c:

  • Both new tests now set CE_GATEWAY_WRITE_PROFILE_NAME=default.
  • The existing deadline assertion now checks that propagated deadlines are valid and bounded by the caller deadline, and confirms stage deadlines are earlier.
  • Assembly now checks its budget during candidate processing and degrades the response rather than returning a late 504 that discards completed retrieval/ranking work.
  • Required Graph evidence absence is now represented explicitly in failure metadata even when text candidates are available.

The current GitHub Actions test workflow is pending. I have not independently run the suite locally. Provider-aware token accounting and durable real-HTTP deadline/dispatcher acceptance tests remain open; PR #48 is not being treated as merge-ready until validation is green and those acceptance gaps are closed.

sfloess commented Oct 30, 2026

Copy link
Copy Markdown
Member Author

[Grok] Fresh independent review of PR #48 head (d18937c)

1. Revisit of prior findings

Prior finding Status Notes
Mocked-transport only; real HTTP deadline propagation unproven Still outstanding Explicitly acknowledged in PR body. No change.
Crude token estimate (len+3)//4 Still outstanding Provider-aware counting remains future work.
Assembly 100 ms budget checked only after work Fully resolved Now checked inside the candidate loop; on overrun the already-built partial result is returned with status=partial / degraded=True instead of a late 504.
Graph-required + no verified paths only failed when fresh was also empty Fully resolved Failure is now always recorded with "required": True even when text candidates exist.
Sonar 0 % coverage on new gateway code Likely still open Not re-measured here; new tests improve the situation but full coverage of the new paths is still thin.

2. New assembly deadline handling

Correct.

  • Preemptive check inside the for candidate in ranked[:100] loop.
  • On overrun: break, set context_truncated=True, append "assembly_degraded", then (after response construction) force status="partial", degraded=True, truncated=True.
  • Work already performed (output_candidates, context_parts so far) is preserved.
  • Graph context is still attempted after the loop; if it would exceed the token budget it simply marks truncated. This is acceptable.

3. Request / stage deadlines

Non-borrowable and correctly propagated.

def stage_call(budget_ms: int):
    stage_deadline = time.monotonic() + budget_ms / 1000.0
    stage_deadline_at = datetime.now(timezone.utc) + timedelta(milliseconds=budget_ms)
    def call(...):
        remaining = min(stage remaining, request remaining)
        if remaining <= 0: raise deadline_exceeded
        extra["X-CE-Deadline"] = min(stage_deadline_at, deadline_at)...
        return request_call(..., min(timeout, remaining))
  • Each stage receives a fixed budget; unused time from earlier stages is not carried forward.
  • Downstream calls receive the earlier of the two deadlines.
  • request_call still enforces the overall request deadline as a hard ceiling.
  • The updated assertion in test_gateway_runtime.py correctly checks that every propagated deadline ≤ the caller deadline and that at least one is strictly earlier.

4. Concurrency / cancellation / timeout / failure handling

  • Parallel stages (intent/entity, retrieval + graph) use ThreadPoolExecutor; exceptions are caught per-future and converted to degradation entries. No silent drops.
  • There is still no proactive cancellation of sibling futures when one stage exhausts its budget or the request deadline fires. This is a known limitation of the executor model and is acceptable for the current scope; a future improvement would be to use a shared cancellation token or shorter per-call timeouts.
  • Reranking identity check + fallback on any GatewayError is solid.
  • Graph support verification before emission remains correctly conservative.

5. Tests

  • test_gateway_graph_reranking.py exercises the happy path (graph verification + exact preservation + non-exact rerank) and the rerank-failure fallback. Environment variable CE_GATEWAY_WRITE_PROFILE_NAME is now set.
  • test_gateway_runtime.py deadline assertion was strengthened as requested.
  • Still missing: a test that forces the assembly loop to hit the 100 ms wall under controlled time, and any test that exercises real HTTP deadline propagation through the dispatcher. Both are already listed as remaining Complete Gateway RAG request orchestration and executable acceptance #34 work.

6. New regressions / architectural concerns

None observed. The design stays consistent with the rest of the Gateway’s degradation model.


Classification

Confirmed defects
None that are merge-blocking.

Missing acceptance coverage (already called out in the PR)

  • Real-HTTP / dispatcher deadline propagation tests.
  • Provider-aware token counting.
  • Explicit assembly-budget overrun test under time pressure.

Non-blocking recommendations

  • Consider a shared cancellation primitive for the parallel stage pools so an exhausted stage/request deadline can abort siblings instead of letting them run to completion.
  • When graph context is omitted because of token budget, surface an explicit reason (e.g. graph_context_truncated) rather than only the generic token_budget_estimated.

Verdict

APPROVE WITH FOLLOW-UP WORK

The blocking findings from the previous review have been correctly addressed. The remaining items are already documented as outstanding #34 acceptance work and do not need to block this PR once CI is green.

— Grok

@sfloess

sfloess commented Oct 30, 2026

Copy link
Copy Markdown
Member Author

[Perplexity]

Re-review — APPROVE WITH FOLLOW-UP WORK

Reviewed head: d18937c920ee9544602ee1ca4674f2e53120c986
Base: b37389a784a7638c5cb212e5bcd50052d960ee55

The head advanced from 2ef42711… with three "Fix PR 48 review findings" commits. I rechecked PR metadata before and after inspection; unchanged. I inspected the complete diff between the two heads, the pinned Gateway assembly/deadline logic, the corrected tests, and current check runs. I executed the full test suite locally at this head. I did not assume the fixes were correct — I verified each one against the code and by execution.

Prior findings — disposition

Prior finding Disposition Evidence
T1: Both new tests fail with CE_GATEWAY_WRITE_PROFILE_NAME is not configured Resolved Both tests now accept monkeypatch and call monkeypatch.setenv("CE_GATEWAY_WRITE_PROFILE_NAME", "default") (tests/test_gateway_graph_reranking.py:7-8, 113-114). Verified passing individually.
T2: Existing deadline-propagation test has a stale assertion Resolved tests/test_gateway_runtime.py:66-74 now parses each propagated RFC 3339 deadline, asserts deadline <= request_deadline, and asserts any(deadline < request_deadline) — proving stage budgeting is active, not just that the header exists. Verified passing.
N1: Assembly-stage 504 discards completed work Resolved runtime/gateway.py:563-566 adds an in-loop time.monotonic() >= assembly_deadline check that breaks with assembly_degraded reason, preserving partial output_candidates/context_parts. runtime/gateway.py:613-618 replaces the 504 raise with a degradation: status="partial", degraded=True, truncated=True, assembly_degraded reason. No exception is raised on assembly overflow.
N3: Graph required_evidence_unavailable conditional on not fresh Resolved runtime/gateway.py:482-484 removes the if not fresh condition and adds required: True. The failure is always reported when Graph is required but has no verified paths.
N2: Stage wall-clock vs monotonic dual tracking Accepted Still uses monotonic for enforcement and wall-clock for the propagated header (runtime/gateway.py:296-298). Minor inconsistency under clock jumps; not blocking.
N4: 4-chars-per-token context estimate Accepted Unchanged. The PR body already flags provider-aware token counting as remaining work.

Verified by execution

  • Full suite at this head: 284 passed, 1 warning, 111 subtests passed (30.1s) — up from 281 passed + 3 failed at the previous head.
  • All 3 previously failing tests pass individually.
  • test_chroma_filters_orphan_vectors_before_search_to_preserve_valid_hit: 3/3 consecutive passes (no regression).
  • All 10 Gateway tests (graph_reranking + runtime) pass.
  • SonarCloud now reports success (was failing previously). Test-suite and model CI jobs were still in progress at review time; my local run independently confirms the suite is green.

Fresh analysis of the new assembly deadline handling

Correct. The in-loop check (runtime/gateway.py:563-566) fires at the top of each iteration; when hit, it breaks before appending the current candidate, so output_candidates and context_parts contain only candidates processed before the deadline. The after-loop check (runtime/gateway.py:613-618) handles the case where the loop completed but response construction crossed the deadline — it degrades the response in place rather than raising. Both paths set truncated=True and add assembly_degraded, so the response truthfully reports partial results.

Deadline non-borrowability and propagation — verified correct

  • Each stage_call(budget_ms) creates a fresh stage deadline at call time (runtime/gateway.py:296-309). Stages do not share budgets; each gets its own, bounded by the request deadline.
  • Every downstream call is bounded by min(stage remaining, request remaining, 30 s default) via min(timeout, remaining).
  • The propagated X-CE-Deadline header is min(stage_deadline_at, deadline_at) — always the earlier of stage and request. request_call's setdefault does not override a stage-set header.
  • Default request budget: 1.5 s for prompt_recall, 5 s for interactive/lookup when no inbound header. Inbound X-CE-Deadline remains authoritative.
  • Concurrency: stage_call closures capture immutable values; ThreadPoolExecutor threads only read them. No shared mutable state. Safe.

Confirmed defects

None. All previously confirmed defects are resolved and verified by execution.

Non-blocking recommendations

  1. Graph context is appended after the candidate loop without a deadline check (runtime/gateway.py:581-587). If the assembly deadline was hit during the candidate loop, the graph context is still appended (bounded by the token budget). Fast operation; acceptable, but a deadline check would be more consistent.
  2. In-loop >= vs after-loop > (runtime/gateway.py:563 vs runtime/gateway.py:613). Cosmetic inconsistency; functionally equivalent.
  3. request_call's if deadline is not None: (runtime/gateway.py:288) is now dead code — deadline is always set (header or default). Harmless.
  4. Stage wall-clock/monotonic dual tracking — accepted as a minor known limitation.

Missing acceptance coverage (unchanged from PR body, follow-up work)

  • Real-HTTP deadline propagation (current tests use mocked transport).
  • Provider-aware context token counting.
  • Durable REST-dispatcher tests.
  • An explicit test for the assembly-deadline degradation path (inject a slow candidate loop; assert partial/assembly_degraded/truncated rather than 504). The current tests don't exercise this branch.

Verdict

APPROVE WITH FOLLOW-UP WORK. All confirmed defects from my previous review are resolved and verified by independent local execution (284/284 passing). The new assembly deadline handling correctly preserves partial results and truthfully reports degradation. Deadline non-borrowability and propagation are correct. Remaining items are missing acceptance coverage and minor recommendations, not defects. Require the in-progress CI test-suite job to complete green on this head before merge.

sfloess commented Oct 30, 2026

Copy link
Copy Markdown
Member Author

[Grok] Re-review of PR #48 head (ffc8885)

Independent review of the current tip after the substantial acceptance-coverage expansion.

Status of prior findings

Finding Previous Now
Mocked transport only; no real HTTP deadline proof Outstanding Largely resolved – durable SQLite-backed HTTP test exercises Knowledge, Index, retrieval, freshness, Ranking, Graph, Reranking through the real dispatcher and records downstream X-CE-Deadline headers
Crude (len+3)//4 token estimate Outstanding Resolved – runtime/context_budget.py with configured local tokenizer or explicit UTF-8 byte upper-bound; response carries full context_budget metadata
Assembly budget checked only after work / late 504 Resolved Still correct; intra-loop check + final force-to-partial preserved
Graph-required evidence only failed when text empty Resolved Still correct (required: true always)
Missing assembly-overrun test Outstanding Resolved – test_gateway_assembly_deadline_returns_partial_context_instead_of_504

Assembly deadline handling

Correct and improved.

  • Check at the top of the candidate loop; break + mark assembly_degraded.
  • Candidates already processed are kept; context already built is kept.
  • Graph context is attempted only if time remains; otherwise marked degraded.
  • Final post-check forces status=partial, degraded=True, truncated=True.
  • The controlled slow-candidate test proves the partial path (no 504).

Minor observation (non-blocking): the time check is before the work of a candidate. A candidate whose own processing crosses the 100 ms boundary can still finish; the final post-check then marks partial. This is the expected “preserve completed work” trade-off and matches the test.

Deadlines (request + stage)

Still non-borrowable and correctly propagated.

  • stage_call takes min(stage remaining, request remaining) and sends the earlier wall-clock deadline downstream.
  • Real-HTTP acceptance test records the headers and asserts they are bounded by the caller deadline.
  • Default budgets (1.5 s prompt_recall / 5 s interactive) remain when no inbound X-CE-Deadline is supplied.

Token / context budget

Solid.

  • count_context_tokens never downloads; only uses an explicitly mapped local tokenizer directory or falls back to UTF-8 byte length.
  • Response always exposes counting_method, is_estimate, fallback_reason, model, max, used.
  • Schema and GATEWAY.md updated accordingly.
  • Tests cover both the configured path and the explicit fallback.

Concurrency / failure handling

  • Parallel stages still use ThreadPoolExecutor with per-future exception → degradation conversion.
  • No sibling cancellation when a stage/request deadline fires (same prior observation; acceptable for current scope).
  • Graph support verification before emission remains conservative.
  • Rerank identity validation + fallback unchanged and correct.

Tests & TRACEABILITY

  • New durable HTTP acceptance test is the right shape: real dispatcher, persistent SQLite, injected outages for Graph/Reranking, deterministic doubles only for intent/entity/cross-encoder so the test stays offline.
  • Assembly overrun, tokenizer selection, and fallback tests are present and meaningful.
  • docs/TRACEABILITY.md now maps acceptance areas to concrete test names and honestly distinguishes real dispatcher coverage from model doubles.

Remaining gaps (already documented in the PR)

  • Full degradation matrix through the durable dispatcher (empty required Graph evidence, Graph dependency failure, scope isolation, bounded cancellation).
  • Load-based measurement of the normative stage latency ceilings.
  • CI green on this exact head (Actions still the source of truth).

Confirmed defects

None that are merge-blocking.

One minor style note (non-blocking): after the response dict is built, the final assembly-deadline block both mutates the reasons list (which is already referenced by the response) and then reassigns response["degradation_reasons"]. Functionally correct; a pure reassignment without the intermediate append would be clearer.

Verdict

APPROVE WITH FOLLOW-UP WORK

The previous blocking concerns have been addressed with real executable coverage. The remaining items are already scoped as further #34 acceptance work and should not hold this PR once CI is green on the current head.

— Grok

@sfloess

sfloess commented Oct 30, 2026

Copy link
Copy Markdown
Member Author

[Perplexity] Independent Re-review of PR #48 — Gateway RAG (Graph + Reranking Integration)

Reviewed head: ffc888528fe312a5215ccd0b4d2b3e789b0a49bb (25 commits since previously reviewed d18937c920ee9544602ee1ca4674f2e53120c986)
Suite: 287 passed, 111 subtests passed locally (Python 3.14.8). CI green on GitHub for this exact head (all 5 checks SUCCESS, bound to headRefOid ffc8885…).


Prior Findings Disposition

Prior finding (at d18937c9) Status
T1: CE_GATEWAY_WRITE_PROFILE_NAME unset in 2 new tests Resolved
T2: Stale deadline assertion (deadline < request_deadline) Resolved
N1: Assembly 504 discarding completed work Resolved (in-loop break + after-loop degradation)
N3: Graph required_evidence_unavailable missing required: True Resolved
NB1: Graph context appended without deadline check Resolved (line 598)
NB2: In-loop >= vs after-loop > inconsistency Still outstanding (D5)
NB3: request_call's if deadline is not None: dead code Still present (line 291; deadline is always set)
NB4: Stage wall-clock/monotonic dual tracking Still present (lines 298–299)
MAC: Real-HTTP deadline propagation Partially addressed (M1)
MAC: Provider-aware token counting Resolved (runtime/context_budget.py)
MAC: Durable REST-dispatcher tests Resolved (expanded real-HTTP acceptance)
MAC: Assembly deadline degradation test Resolved, but encodes a false truncation (D4)

Confirmed Defects

D1 — Assembly deadline is not bounded by the request deadline

File/Function: runtime/gateway.py:560 (GatewayService.search)

assembly_deadline = time.monotonic() + 0.100

The assembly budget is a fixed 100 ms from now, not min(now + 100 ms, request_deadline). Assembly performs no request_call, so the request deadline (enforced only inside request_call/stage_call) is never checked during assembly.

Failure scenario: A request enters assembly with 50 ms of request deadline remaining. Assembly sets its own 100 ms budget and runs to completion; the response is delivered up to 100 ms past the caller's X-CE-Deadline. The contract's non-borrowable deadline guarantee is violated for the assembly stage.

Recommended fix: Compute the request deadline in monotonic terms once (request_deadline_mono) and set assembly_deadline = min(time.monotonic() + 0.100, request_deadline_mono). Add a test entering assembly with almost no request time remaining and assert the response returns before the request deadline.

D2 — 65,536-character delivery slice silently truncates after counting

File/Function: runtime/gateway.py:663

response["context"] = "\n\n".join(context_parts)[:65536]

Admission counts the complete serialized context, but delivery slices it to 65,536 characters. If the token budget is large (e.g. context_budget.max = 200000, which a modern provider window easily permits), the admitted context can be ~800 KB of text. The response delivers only the first 65,536 characters while context_budget.used reports the token count of the full string and truncated remains False.

Failure scenario: Client requests max: 200000 provider tokens. Gateway admits ~180,000 tokens of context, reports used: 180000, truncated: false, and delivers a 65,536-character prefix. The metadata describes a different string than the one delivered, and the omission is unreported.

Recommended fix: Apply the serialization limit during admission — reject candidates that push the serialized context past 65,536 characters (setting truncated/context_budget_truncated), or count and budget the delivered slice and flag omitted content explicitly.

D3 — Counting-method metadata describes the last attempted context, not the final accepted context

File/Function: runtime/gateway.py:578–582 (candidate loop), 600–606 (graph branch)

token_counting_method and token_count_fallback_reason are overwritten on every count_context_tokens call — including for candidates that are subsequently rejected for exceeding the budget — while used_tokens retains the value from the last accepted context.

Failure scenario (reproduced in sandbox): Candidate 1 is admitted via the configured tokenizer (method=configured_model_tokenizer, used=3). Candidate 2's tokenizer encode() raises, falls back to UTF-8 bytes, exceeds the budget, and is rejected. The response reports counting_method=conservative_utf8_byte_upper_bound, fallback_reason=configured_tokenizer_unavailable alongside used=3 — a byte-estimate label on a tokenizer-produced count. The same skew occurs when the candidate path falls back but the Graph branch's tokenizer call succeeds: the Graph branch updates the method but does not clear an earlier fallback_reason.

Recommended fix: Update token_counting_method/token_count_fallback_reason only inside the acceptance branch (if estimate <= budget["max"]), or recompute both from the final context_parts after the loop and Graph branch. Add a test covering the tokenizer-success-then-failure transition and the candidate-fallback/Graph-success transition.

D4 — truncated=true is set when nothing was omitted

File/Function: runtime/gateway.py:655–660; test tests/test_gateway_runtime.py::test_gateway_assembly_deadline_returns_partial_context_instead_of_504

The after-loop check unconditionally sets truncated=True when time.monotonic() > assembly_deadline, regardless of whether any candidate or Graph context was actually skipped. In the new test, the single candidate is admitted, the context is complete, and no Graph context exists — yet the test asserts result["truncated"] is True.

Per the contract, truncated marks omitted content: "Set truncated=true if this cap is reached" (100-candidate pool) and "If a retrieval path is truncated or canonical verification filters candidates … set truncated=true". Lateness alone is not truncation. The test enshrines a false positive.

Failure scenario: A client relying on truncated to decide whether to re-request with a larger limit sees truncated=true on a fully-assembled response and issues a needless retry, or treats the response as incomplete when it is not.

Recommended fix: Track a separate assembly_skipped flag set only in the in-loop break (line 569) and the Graph-deadline branch (line 598); gate the after-loop truncated=True on that flag. Report lateness through assembly_degraded + status=partial alone. Update the test to assert truncated is False when the single candidate is fully admitted, and add a second case where a second candidate is actually skipped.

D5 — In-loop >= vs after-loop > inconsistency (carried from previous review)

File/Function: runtime/gateway.py:569 vs :655

At exactly the deadline, the in-loop check (>=) breaks and skips the candidate, but the after-loop check (>) does not fire, so assembly_degraded is reported only once — from the in-loop path. Harmless today, but the two comparisons should be unified (both >=, or both >) to keep the degradation semantics single-valued.


Missing Acceptance Coverage

M1 — Non-borrowable stage budgets are not proven

The real-HTTP acceptance test sends X-CE-Deadline: 2099-01-01T00:00:00Z and permits timeouts up to 30 seconds. The assertions — that propagated deadlines precede 2099 and that some are strictly earlier — would all pass even if each downstream call reset its stage allowance to the full budget on every invocation. The deadline-propagation coverage is real-HTTP and valuable, but it does not demonstrate that a stage's budget is shared across the calls within that stage and consumed by the first call.

Recommended test: Use a finite request deadline (e.g. now + 2 s) with a small stage budget. Make the first call in a stage consume most of the allowance (e.g. sleep), then assert the second call in the same stage receives a strictly smaller X-CE-Deadline / timeout than the first, and that a third call is refused with deadline_exceeded. Capture the headers actually received by the downstream handler (including nested service-to-service calls), not only those set by the Gateway.


Non-Blocking Recommendations

  1. O(n²) tokenization. Each candidate re-tokenizes the full prospective context ("\n\n".join(context_parts + [fragment])). The 100 ms assembly check bounds the wall-clock cost by degrading gracefully, but an incremental count (tokenize the fragment once, add a constant for the separator) would be O(n) and avoid burning the assembly budget on redundant work.
  2. A single tokenizer call is not individually bounded. count_context_tokens (tokenizer load + encode) can exceed the 100 ms assembly budget; the deadline is only checked between calls. Local AutoTokenizer.from_pretrained with local_files_only=True is fast, but the contract should either accept this or the admission loop should pre-check the deadline before invoking the tokenizer.
  3. _load_tokenizer lru_cache does not cache raised exceptions — failed loads are retried on every candidate, successful loads are cached. Correct behavior; noting for clarity since the fallback path (configured_tokenizer_unavailable) can fire repeatedly within one request.
  4. request_call's if deadline is not None: (line 291) is dead code — deadline is always set (header or computed default). Carried from previous review.
  5. Stage wall-clock/monotonic dual tracking (lines 298–299) is still present. Carried from previous review.

Positive Observations

  • The new runtime/context_budget.py is well-designed: explicit local_files_only=True, trust_remote_code=False, lru_cache on loads, and three distinguishable fallback reasons (no_configured_tokenizer, configured_tokenizer_unavailable, invalid_tokenizer_map) — all verified to behave correctly in the sandbox.
  • Tokenizing the complete prospective context (including separators) rather than incrementally summing fragments is the correct semantic — provider token counts are not additive across fragments.
  • The real-HTTP acceptance test is now genuinely durable: real IndexService/KnowledgeService/GraphService with real SQLite projections, real RerankingEngine (only the cross-encoder is a deterministic double), real REST dispatcher, JSON-Schema validation of every response, and outage/scope-isolation cases — no transport mocking.
  • The schema (gateway.schema.json) is fully in sync: context_budget required in the response with the correct counting_method enum, 22 degradation reasons, required/message on failures, and the new failure codes.
  • Graph/Reranking outage handling, required-evidence reporting, and scope isolation are all correct and covered.

Verdict

REQUEST CHANGES

D1–D4 are confirmed contract violations with concrete failure scenarios and recommended fixes; D5 is a carried-forward inconsistency. M1 is a genuine gap in acceptance coverage for a headline contract guarantee (non-borrowable deadlines). All are localized to the assembly/token-budget path introduced or modified in this PR; the retrieval, freshness, ranking, reranking, Graph-integration, and outage-handling work is solid and well-tested.

@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant