Uh oh!
There was an error while loading. Please reload this page.
fix(rag): put retrieval/indexing failures on the app-logging path (#482) - #501
Conversation
Item 1 of #482. rag_service reported every failure with a bare `print`, so a retrieval that blew up was invisible to app logging and uncountable by any rollup — and since retrieve_chunks degrades to [], which is also what "nothing relevant matched" returns, an ungrounded tutor/quiz turn was indistinguishable from a grounded one at every layer above it. Adds a module logger, moves all three print sites onto it, and emits rag.retrieval_failed / rag.chunks_dropped so the degrade rates are countable. Crucially these separate the DELIBERATE degrade from a real one. The #439 seam raises _EmbeddingDisabled whenever SAPLING_MODEL_MODE != real, which is every function-mode run — so a naive version emitted an error event on every e2e tutor turn and every e2e upload. That noise would have made the new signal worthless. _EmbeddingDisabled now logs at INFO and spends no event; only genuine failures warn and count. The two #439 transport guards caught this and are updated to assert the new channel (and to pin the no-event contract) — the invariant they protect is unchanged, only the output moved off stdout. Item 2 (poisoned embedding:None rows) was already fixed on main — the filter at the upsert drops them. Item 3 (durability inversion) is documented in architecture.md: indexing rides the NON-durable streaming route while DBOS wraps the sync route that never indexes, so a crash loses chunks behind a healthy documents row, and backfill_document_chunks.py is invoked by nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | c03c983 | Commit Preview URL Branch Preview URL | Jul 31 2026, 06:17 PM |
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Warning Review limit reached
Next review available in:28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…s mode Code review, two real findings. 1. The two new event types were emitted OUTSIDE the pinned #117 taxonomy. log_event deliberately doesn't enforce membership (it must never raise), so nothing failed — they were simply undocumented and absent from the frozenset that exists to make a rename break loudly. Registered in EVENT_TAXONOMY, the docstring table, and the exact-set test. Also recorded why they are NOT named error.*: /api/admin/analytics/errors filters on `event_type like error.*` and projects an HTTP shape (path / method / status_code / duration_ms). Renaming would fill an HTTP-request table with null-path rows; these surface via /usage/summary's by_event_type instead. Giving that feed a shape-agnostic projection is the real fix and is out of scope here. 2. test_retrieve_chunks_failure_is_logged_and_counted depended on ambient SAPLING_MODEL_MODE. _require_real_mode() runs before the mocked client is reached, so with function mode exported — which the E2E workflow tells you to export — it took the _EmbeddingDisabled branch and passed vacuously on an unrelated path. Pinned to real mode; verified it now passes with SAPLING_MODEL_MODE=function set AND unset. Six PRE-EXISTING tests in that file share the same latent dependence (task_type, returns_count, chunk_ids, dedupe). Left alone — the hermetic lane runs with the var unset by contract — but worth its own cleanup. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AndresL230
commented
Jul 31, 2026
Code reviewFound 2 issues, both fixed in c03c983.
https://github.com/SaplingLearn/Sapling/blob/5d741c9/backend/services/rag_service.py#L148-L158 Registered in
https://github.com/SaplingLearn/Sapling/blob/c03c983/backend/tests/test_rag_service.py#L389-L400 Noted, not fixed: six pre-existing tests in that file share the same latent dependence ( Checked and cleared: 🤖 Generated with Claude Code |
Part of #482.
Scope, after re-verifying the issue against
mainThe issue lists three hazards. Only one and a half were still live:
print, invisible to app loggingembedding: Nonerows persisted anywayrag_service.pyfilters them before upsert)Item 1
rag_servicereported every failure with a bareprint, so failures never reached the app-logging path and no rollup could count them. That matters most forretrieve_chunks, which degrades to[]— and[]is also what "nothing relevant matched" returns, so an ungrounded tutor/quiz turn was indistinguishable from a grounded one at every layer above.Adds a module logger, moves all three
printsites onto it, and emitsrag.retrieval_failed/rag.chunks_dropped(categoryerror) so the degrade rates are countable.The part worth reviewing
The naive version of this is wrong, and the existing #439 tests caught it.
_require_real_mode()raises_EmbeddingDisabledwheneverSAPLING_MODEL_MODE != "real"— which is every function-mode run. So a blanketexcept Exceptionwould emit an error event on every e2e tutor turn and every e2e upload, and the new signal would be pure noise the day it shipped._EmbeddingDisabledis now caught separately: INFO, no event. Only genuine failures warn and spend an event. Both #439 transport guards are updated to assert the new channel and to pin the no-event contract — the invariant they protect (no egress in function mode) is unchanged; only the output moved off stdout.Item 3 — documented, not fixed
Recorded in
docs/architecture.mdunder Known sharp edges: DBOS wraps/upload/sync, which never indexes; the route that does index is the streaming/upload, which is non-durable by design (ADR 0011). A crash between_persist_documentand the post-roll indexing task leaves a healthy-lookingdocumentsrow whose content is absent from retrieval permanently, andscripts/backfill_document_chunks.pyis invoked by nothing.Wiring a retry trigger is a real design call (where it runs, what re-drives it, how it interacts with the streaming route's X-Request-ID replay semantic), so I documented the gap rather than guessing at it — the issue itself asks for documentation as the minimum bar.
Gates
Backend pytest 1531 passed / 32 skipped (2 new), ruff clean, full local e2e cycle green (Playwright 37/37, oracles 0 findings).
🤖 Generated with Claude Code