Backend fault classification, list-size caps, and search stats parity - #117
Merged
Conversation
…aker Cross-process SQLITE_BUSY and SQLITE_LOCKED contention surfaced through begin_transaction charged the circuit breaker and was never retried, so routine multi-process write contention could open the breaker and reject every caller's healthy requests. A shared is_sqlite_locked_error predicate classifies the locked family by SQLite result code with a message fallback, begin_transaction re-raises it uncharged after rollback, and the tool-layer retry classifier now retries it with backoff on all store and update paths. The write-queue retry loop also breaks straight to the exhaustion raise when no attempt remains, removing a dead backoff sleep that only delayed the single exhaustion breaker charge. The retry classifier docstring no longer claims write-queue coverage it does not have.
A pool-acquire TimeoutError carries two indistinguishable signals: genuine pool saturation that waits out the full acquire budget, and asyncpg's inner connection-establishment timeout that fires clearly below it when the database is unreachable. Treating both as saturation left every acquire timeout uncharged, so a blackholed database could never open the circuit breaker and every request repeated the full stall. The acquire paths in execute_write, get_connection, and begin_transaction now discriminate by elapsed wait: saturation stays uncharged while an establishment timeout charges the breaker. The pool setup callback re-raises its own TimeoutError as ConnectionDoesNotExistError so a dead pooled connection stays distinguishable from saturation and retries on the charged arm. The write retry arms also skip the exponential backoff sleep after the final failed attempt, which only added dead latency before the single exhaustion breaker charge.
execute_read on both backends counted ControlFlowError in failed_queries, so repeated client-input validation rejections raised inside read callables inflated the operator-facing failure metric. The write path already exempts ControlFlowError from failure accounting because it is normal control flow rather than a database fault. Both read paths now re-raise it before the counter arm so failed_queries reflects database health only.
…r pre-check asyncpg raises a plain ValueError for a malformed DSN authority before any network I/O, and the pgvector pre-check's catch-all wrapped it as a retryable DependencyError with exit code 69. A supervisor then restart-looped on a permanent client-side misconfiguration instead of stopping. The pre-check now classifies ValueError as ConfigurationError with exit code 78, matching the initialize path's classification of the same error.
The IPv6 bracketing in the built connection string keyed on the presence of a colon, so a host configured in bracket form was wrapped again into a double-bracketed authority. asyncpg rejects that DSN at parse time with a plain ValueError that never names the bracket cause. Bracketing is now idempotent: a host already in bracket form is used as-is.
…sponses The all-modes-failed hybrid response omitted the always-present fusion_method, fts_count, and semantic_count keys, so a client reading the documented response shape crashed on the degraded path. semantic_search_context returned no stats block at all when a metadata filter failed validation under explain_query, and the FTS and SQLite/PostgreSQL repository validation-error stats omitted the backend key the success path always carries. A shared empty-stats builder now gives every validation-error response the same stats shape as the success path, with zeroed counters and the backend key.
The tags filter on the four search tools and grep_context, and the value lists of the metadata in and not_in operators, accepted unbounded client-supplied lists. Each member binds one SQL placeholder in a single statement, so a schema-legal oversized list could overflow the backend's bind limit inside connection scope and charge the circuit breaker for purely invalid client input. Both caps are advertised in the wire schema, re-checked in each tool body behind the schema, and rejected as structured validation errors before any SQL executes.
ContextRepository.get_by_ids and both criteria-based batch delete paths bound arbitrarily long client-controlled ID and thread lists into a single IN statement, overflowing SQLite's per-statement bind-variable ceiling on large inputs. get_by_ids now fetches in bounded chunks and re-sorts the accumulated rows so the ordering contract is preserved across chunks, and the criteria deletes execute one statement per bounded chunk pair inside the same transaction. get_context_by_ids and the delete_context_batch ID and thread lists now enforce the documented 100-item cap at the tool boundary.
…rite When a concurrent delete removed a row between an atomic batch update's version capture and its compare-and-set write, the resulting conflict surfaced as a retry-the-request error even though retrying can never succeed. The atomic path now re-probes the row on the open transaction connection when the compare-and-set matches nothing and reports the standard not-found error for a vanished row, re-raising the genuine version conflict otherwise.
…ailure A configured metadata index field name longer than 50 characters produced a generated index identifier past PostgreSQL's 63-byte limit, where silent truncation breaks index reconciliation; names are now length-checked at settings construction. Two field names differing only in case collide on SQLite's case-insensitive index namespace while PostgreSQL keeps both, so case-folded duplicates are now rejected identically on both backends. A strict-mode index mismatch raised RuntimeError with exit code 1, restart-looping supervisors on a permanent misconfiguration; it now raises ConfigurationError with exit code 78, and the message states that strict mode requires pre-provisioned indexes.
…e pre-flight The migration CLI's pgvector dimension pre-flight ran only when the source database reported a dimension, so a source whose embedding_metadata table exists but is empty fell back to the configured embedding dimension without validation. An over-limit fallback then templated invalid vector DDL and crashed at index creation, leaving the target schema partially initialized. Both the target initialization pre-flight and the migration-run guard now resolve the effective dimension including the settings fallback and refuse an over-limit value before any DDL, and a dry run reports that a real run would abort.
…check The SQLite-to-PostgreSQL per-row pre-check screened every raw-bound text column except content_hash, so a NUL byte or lone surrogate there aborted the whole migration mid-transaction instead of skipping and reporting the row. The pre-check candidate list now covers content_hash, and a dry run surfaces the affected row.
…tation The environment-variables reference scoped the circuit breaker settings to SQLite only, but the PostgreSQL backend consumes all three settings as well. The hybrid search docstring and reference omitted the always-present backend key in fts_stats and semantic_stats and the adaptive_fts_mode field, and the semantic search stats example omitted the rows_returned and backend keys the runtime always emits.
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Thirteen commits hardening fault classification, input bounds, and response-shape parity across both storage backends.
Test plan