Refactor: top 5 fixes (concurrency, circuit breaker, redaction, session safety, migration tests) - #6
Open
mrSamDev wants to merge 5 commits into
Open
Refactor: top 5 fixes (concurrency, circuit breaker, redaction, session safety, migration tests)#6mrSamDev wants to merge 5 commits into
mrSamDev wants to merge 5 commits into
Conversation
…te connections SessionStore and ReceiptLedger now use threading.local to give each calling thread one persistent connection reused across all its operations, instead of opening/closing a connection on every single _op_* call. WAL mode and busy_timeout are set once per connection, not re-executed per operation. Both stores gain a close() method that releases all tracked connections. main() now closes both stores in a finally block on shutdown. The threading.local approach preserves the existing concurrency model: a blocked write on one thread does not block reads on another thread (WAL allows concurrent readers). The existing concurrency tests (test_store_write_does_not_block_event_loop, test_write_waits_under_contention_not_locked_error) confirm no regression. New tests verify connection reuse (sqlite3.connect is not called per-op) and that close() releases connections.
Session.receipt_file_ids is now a read-only property backed by a Pydantic PrivateAttr. Direct assignment (session.receipt_file_ids = [...]) raises AttributeError instead of silently being dropped by save(), which deliberately doesn't persist the list to avoid clobbering concurrent atomic appends. The list is still mutable in place via add_file_id() and clear_receipts() on the Session model. Construction with receipt_file_ids=[...] still works via an __init__ override that routes the kwarg to the private attribute. Updated two production call sites that assigned the field directly: - receipt_input.py: session.receipt_file_ids = list + [file_id] → session.add_file_id(file_id) - job_processor.py: session.receipt_file_ids = [] → session.clear_receipts() The silent footgun (save() drops receipt_file_ids changes) is now a loud AttributeError at the point of mutation, not a silent no-op at save time.
Each provider in the pool now has a circuit breaker: after failure_threshold (3 by default) consecutive failures, the provider is skipped for cooldown_seconds (60s by default). This prevents a downed provider from being retried on every receipt in every batch, burning the call budget and time. When all providers' circuits are open, they are all tried anyway (better to attempt and fail than to do nothing). A successful extraction resets the breaker. The breaker state is guarded by the existing threading.Lock since extract_receipt runs in asyncio.to_thread worker threads. Tests verify: circuit opens after N failures, resets on success, closes after cooldown, and all-open circuits still try.
_SENSITIVE_PATTERNS is now a list of (compiled_pattern, replacement_template)
tuples instead of bare compiled patterns. The redact() function iterates
each (pattern, replacement) pair and applies pattern.sub(replacement).
This replaces the brittle heuristic where the replacement strategy was
selected via "password" in pat.pattern.lower() — which happened to work for
the current patterns but would silently break if a pattern with a capture
group that doesn't contain 'password' in its source were added.
Also improves redaction coverage:
- Quoted values with spaces ("hunter 2 extra") are fully redacted
- Authorization: Bearer and Basic auth headers are redacted
- openai_api_key values with dots/slashes are fully redacted (via generic
api_key pattern, which handles any non-whitespace value)
- Removed the redundant openai_api_key-specific pattern
Known limitation: unquoted multi-word values (password = hunter 2 extra)
only have the first token redacted. Quoted values are the supported way to
log multi-word secrets. This limitation is pre-existing and documented in
the code.
The schema migrations run on every startup but were previously tested only against fresh DBs (which go 0→current trivially). The interesting path — a DB at an older schema version that needs migration — was untested. Added three migration tests: SessionStore: - v1 → v5: tests the full migration chain (add lease_expiry, add report_title, create index, drop lease_expiry). Verifies columns match a fresh DB, lease_expiry is gone, report_title exists, index exists, and data survives. - v3 → v5: tests the lease_expiry drop path (existing production DBs created before the cross-process lease was removed). Verifies lease_expiry is dropped cleanly and report_title data survives. ReceiptLedger: - v1 → v2: tests adding failure_reason and delivered_at columns. Verifies columns match a fresh DB, new columns exist, indexes survive, and data (including Decimal totals) survives the migration. Each test creates a DB at the old schema version with raw SQL, inserts test data, opens it with the store class (triggering _migrate), and asserts the final schema and data match expectations.
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
Five targeted refactors and hardening changes addressing concurrency footguns, provider resilience, redaction correctness, and test coverage gaps.
Changes
1. Persistent thread-local SQLite connections
SessionStoreandReceiptLedgernow usethreading.localto reuse one persistent connection per thread, instead of opening/closing a connection on every_op_*call. WAL mode andbusy_timeoutare set once per connection. Both stores gain aclose()method, called bymain()in afinallyblock on shutdown.The existing concurrency model is preserved — a blocked write on one thread does not block reads on another (WAL). Existing concurrency tests confirm no regression.
2.
Session.receipt_file_idsread-only propertyDirect assignment now raises
AttributeErrorinstead of silently being dropped bysave()(which deliberately does not persist the list to avoid clobbering concurrent atomic appends). Construction viareceipt_file_ids=[...]still works via an__init__override routing to a private attribute. Mutations go throughadd_file_id()/clear_receipts().Updated two production call sites:
receipt_input.py:session.receipt_file_ids = list + [file_id]→session.add_file_id(file_id)job_processor.py:session.receipt_file_ids = []→session.clear_receipts()3. Circuit breaker for
ProviderPoolEach provider now has a circuit breaker: after
failure_threshold(default 3) consecutive failures, the provider is skipped forcooldown_seconds(default 60s). Prevents a downed provider from burning call budget on every receipt in every batch. When all circuits are open, all are tried anyway. A successful extraction resets the breaker. State is guarded by the existingthreading.Lock.4.
redact()explicit per-pattern replacement templates_SENSITIVE_PATTERNSis now a list of(compiled_pattern, replacement_template)tuples. This replaces the brittle heuristic selecting strategy via"password" in pat.pattern.lower(), which would silently break for patterns whose capture groups do not containpasswordin their source. Also expands coverage: quoted multi-word values,Authorization: Bearer/Basicheaders, and genericapi_keyvalues with dots/slashes.5. Migration tests against actual old-schema DBs
Schema migrations run on every startup but were previously only tested against fresh DBs. Added tests covering the interesting migration paths:
SessionStore: v1→v5 (full chain), v3→v5 (lease_expiry drop)ReceiptLedger: v1→v2 (addfailure_reason,delivered_at)Each test creates a DB at the old schema with raw SQL, inserts data, opens it with the store class (triggering
_migrate), and asserts final schema + data integrity.Test plan
pytest tests/unit/test_sessions.pypytest tests/unit/test_pool.pypytest tests/unit/test_logging_redaction.pypytest tests/unit/test_ledger.py