fix(#4097): propagate sustained RAG 5xx/408 indexing failures to backoff gate - #4151
Conversation
aheritier
left a comment
There was a problem hiding this comment.
🤖 Automated implementer agent — this comment was posted by the implementer bot from Docker Agentic Platform, not by a human developer
Review-of-record (self-review is blocked for the PR author on this instance, so recording findings as a COMMENTED review rather than an approval).
Fix correctness
pkg/rag/strategy/indexing_errors.go— newisGateArmingTransientError(L47-56) mirrorsstartBackoffRetryable's own predicate (pkg/tools/startable_backoff.go:51-59): pre-filters to*modelerrors.StatusErrorviaerrors.As, thenmodelerrors.RetryableHTTPStatus. Plain-text errors (timeouts, chunk counters) can't arm the gate here either, matching the existing 429-path guarantee.pkg/rag/strategy/vector_store.goInitialize(~L241-246, L333-341, L373-382): each errgroup goroutine records the first gate-arming transient error viaatomic.Pointer[error].CompareAndSwap(nil, &err); afterg.Wait(), ifindexed == 0and one was recorded, the error is propagated wrapped with%wso*modelerrors.StatusErrorsurvives the chain up throughManager.Initialize→rag.ToolSet.Start→StartableToolSet.startLocked/tryStartLocked. An isolated per-file failure withindexed > 0still returnsnil— unchanged.- 429 abort-on-first-failure (
errIndexingAborted) is untouched;filesToIndex == 0and cancellation short-circuit before the new block, so neither interferes. CheckAndReindexChangedFiles/file-watcher path deliberately left untouched (out of scope; #4073 lands on this seam next).
Tests
pkg/rag/strategy/vector_store_test.go:TestInitializeContinuesOnTransientModelError(isolated failure, 1-of-5 files) vsTestInitializeSurfacesSustainedTransientModelError(408/500/502/503/504, all-fail) pin the distinction;..._ConcurrentFailuresvariant withFileIndexConcurrency: 5exercises the atomic write under real concurrency (go test -raceclean).pkg/tools/builtin/rag/rag_backoff_test.go:TestRAGStartableBackoff_StatusErrorEngagesGategeneralized to 429/408/503 subtests, proving the fullManager→ToolSet.Start→StartableToolSetplumbing arms the gate for the wider status family, not just 429.
Validation (this sandbox needed sudo apt-get install -y gcc libc6-dev for CGO/treesitter): CGO_ENABLED=1 task build, task test (all packages green, including -race on the touched packages), task lint (0 golangci-lint issues, 0 custom-cop offenses, go mod tidy clean) all pass.
Process note: this PR went through two independent reviewer sub-agent passes (request_changes → doc-precision/comment/race-test fixes → approve). No findings were rejected as false positives; all were addressed.
…off gate Initialize now records the first gate-arming transient error (429/408/ retryable 5xx per startBackoffRetryable) seen during a run. If every attempted file fails the same way and none is indexed, that error is propagated instead of swallowed, so StartableToolSet's backoff gate paces the next turn. An isolated per-file failure with any successful indexing still returns nil, unchanged. Adds isGateArmingTransientError mirroring startBackoffRetryable's own *modelerrors.StatusError predicate, tests pinning both the isolated- vs-sustained distinction and the full ToolSet->StartableToolSet gate arming for 429/408/503 (plus a FileIndexConcurrency>1 race-stress variant), and updates the RAG and MCP retry-policy docs.
|
👋 This PR has merge conflicts with the base branch. Please rebase or merge the latest base branch and resolve them. I've moved it to draft and added |
84dded4 to
2e35fea
Compare
Fixes #4097 (follow-up to #4060/#4062): sustained RAG 5xx/408 indexing failures were never reaching the
StartableToolSetbackoff gate — only 429 did, becauseclassifyModelCallErrorswallowed 5xx/408 per-file insideVectorStore.Initialize, so a backend sustaining 503s/408s re-triggered a full concurrent re-index on every agent turn.Design (option (a) from the issue)
Initializenow tracks the first gate-arming transient error (429/408/retryable-5xx perstartBackoffRetryable's own predicate, via newisGateArmingTransientError) seen across the run's goroutines, usingatomic.Pointer[error].CompareAndSwapfor race-free first-writer-wins. An isolated per-file failure with any successful indexing still returnsnil— unchanged per-file-skip semantics, since indexed files persist and only the failures get retried next run. But if a run indexes zero files and at least one attempted file hit a gate-arming status, that error is now propagated (wrapped with%wthroughout the chain) so it surviveserrors.Asall the way tostartBackoffRetryable.429 is untouched — it already aborts the whole run on the first failure via the existing
errIndexingAbortedpath.Tests
pkg/rag/strategy/vector_store_test.go: isolated-vs-sustained-failure distinction at theVectorStore.Initializeunit level (408/500/502/503/504, plus aFileIndexConcurrency>1variant proving the atomic write is race-free under-race).pkg/tools/builtin/rag/rag_backoff_test.go: generalized the existing 429 gate-arming test to also cover 408/503, proving the fullManager→ToolSet.Start→StartableToolSetplumbing arms the gate for the wider status family.Docs
Updated
docs/tools/rag/index.md's retry-policy table/prose and the cross-reference indocs/tools/mcp/index.mdto describe the new behaviour instead of the "429-only" gap.Sequencing note
Issue #4073 (touches this same
pkg/rag/strategy/vector_store.goseam) is intentionally held back until this merges.Review
Reviewed by an independent reviewer sub-agent through two passes (
request_changes→ fixes →approve). No findings were rejected; all were addressed.