Isolate per-item write failures and add write-queue health visibility - #16
Conversation
…health visibility MonitorStore's flush wrapped a whole 100-item batch in one SQLite transaction: any single bad item rolled back and silently dropped every other item in that batch, with no retry and no way to tell it happened. The queue-full drop path was similarly silent (log line only). Now each queued item writes inside its own SAVEPOINT, so one item's failure only rolls back that item. Transient SQLite errors (lock contention, SQLITE_BUSY) get bounded retries with backoff before being treated as a permanent per-item failure. A new DatabaseWriterHealth snapshot (queue depth/capacity, dropped/failed counts, writer-alive, last error) is exposed via MonitorStore.health() for a future degraded-storage UI indicator, and a writer-thread crash (e.g. failing to open the DB file) now records a last_error instead of just flipping an internal flag nothing reads. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 13 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMonitorStore now exposes writer health metrics and improves SQLite write durability. Queue drops are counted, transient failures are retried, item failures are isolated, transaction failures are recorded, and writer crashes update health state. ChangesMonitor writer durability
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MonitorStore
participant WriterThread
participant SQLite
MonitorStore->>WriterThread: enqueue monitor item
WriterThread->>SQLite: write item in savepoint
SQLite-->>WriterThread: success or transient error
WriterThread->>SQLite: retry transient error with backoff
WriterThread->>MonitorStore: record failure or crash error
MonitorStore-->>MonitorStore: return health snapshot
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_monitor_store_write_durability.py (1)
55-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd commit-failure recovery coverage.
The tests do not exercise the rollback path in
MonitorStore._flush()after successful item writes and a failed transaction commit. Add a controlled connection failure at commit time. Assert that the transaction persists no completed items and thatfailed_countincludes every item that had reported success.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_monitor_store_write_durability.py` around lines 55 - 178, Add a test in the durability test suite targeting MonitorStore._flush() that uses a controlled database connection failure during transaction commit after multiple item writes report success. Assert reopening the store finds none of those items persisted, and verify health().failed_count equals the number of completed items affected by the failed commit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/meshchat/services/monitor_store.py`:
- Around line 105-112: Move initialization of _health_lock, _dropped_count,
_failed_count, _last_error, and related writer-visible state before
self._writer.start() in the monitor store initialization flow. Ensure
_writer_loop() can safely call _record_error() immediately, and prevent the
subsequent _available assignment from overwriting an early writer failure.
---
Nitpick comments:
In `@tests/test_monitor_store_write_durability.py`:
- Around line 55-178: Add a test in the durability test suite targeting
MonitorStore._flush() that uses a controlled database connection failure during
transaction commit after multiple item writes report success. Assert reopening
the store finds none of those items persisted, and verify health().failed_count
equals the number of completed items affected by the failed commit.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 67404188-890c-4bba-8a41-8cfca64c4132
📒 Files selected for processing (2)
src/meshchat/services/monitor_store.pytests/test_monitor_store_write_durability.py
|
|
||
| # Guards the counters below, which are written from the writer | ||
| # thread and read from the GUI/caller thread via health(). | ||
| self._health_lock = threading.Lock() | ||
| self._dropped_count = 0 | ||
| self._failed_count = 0 | ||
| self._last_error: str | None = None | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Initialize writer state before starting the writer thread.
self._writer.start() runs before _health_lock, health counters, and _last_error exist. If the writer fails immediately, _writer_loop() calls _record_error() before _health_lock exists. The error handler then fails and loses the original startup error. The later assignment to _available = True can also overwrite the writer failure state.
Initialize all writer-visible state before calling self._writer.start().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/meshchat/services/monitor_store.py` around lines 105 - 112, Move
initialization of _health_lock, _dropped_count, _failed_count, _last_error, and
related writer-visible state before self._writer.start() in the monitor store
initialization flow. Ensure _writer_loop() can safely call _record_error()
immediately, and prevent the subsequent _available assignment from overwriting
an early writer failure.
…re item loop
Without an explicit BEGIN, Python sqlite3 does not auto-issue one before
SAVEPOINT (it only auto-issues BEGIN before DML statements). As a result,
each SAVEPOINT item_write became the outermost transaction; RELEASE
committed immediately, destroying batch efficiency and making the
completed-items cleanup logic incorrect on COMMIT failure.
Fix: issue conn.execute("BEGIN") inside the with conn: block before the
item loop so all per-item SAVEPOINTs nest inside a real transaction.
Also guard the ROLLBACK TO SAVEPOINT item_write / RELEASE pair in
_write_item_with_retry with a try/except: if SAVEPOINT itself fails
(before savepoint_active is set), the subsequent ROLLBACK would raise
"no such savepoint" and crash the item loop rather than just failing
the one item.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
MonitorStore._flush()wrapped an entire 100-item batch in one SQLite transaction — a single bad item raised inside the loop,with conn:rolled back the whole transaction, and every other item in that batch was silently lost along with it (log line only, no retry, no per-item isolation)._enqueue) was similarly silent — alog.warningand nothing else.Changes
SAVEPOINT(_write_item_with_retry), so one item's failure rolls back only that item — the rest of the batch's transaction is unaffected.database is locked,SQLITE_BUSY) get up to 3 retries with exponential backoff (50ms base) before the item is treated as a permanent failure. Non-transient errors (bad data, programming errors) fail immediately without retrying.DatabaseWriterHealthdataclass, returned byMonitorStore.health(): queue depth/capacity, dropped-item count (queue-full drops), failed-item count (permanent write failures), writer-thread-alive flag, and last error message. Intended for a future degraded-storage indicator in the UI — not wired into the UI in this PR (out of scope; kept focused).last_errorvia the health snapshot instead of only flipping an internal_availableflag nothing else read.with conn:rolls back the entire transaction in that case — no double-counting of items already marked failed at the per-item level.Risk assessment
Contained to
MonitorStore's writer internals; the public write API (save_packet,save_message, etc.) is unchanged. Retry adds up to ~350ms of extra latency per item only when a transient lock error actually occurs — negligible for a background writer thread, never on the GUI thread.Test plan
pytest -q— 360 passed (7 new intest_monitor_store_write_durability.py)ruff check src tests scripts— cleanmypy src/meshchat— cleanhealth().failed_count; a transient error that clears within the retry budget still succeeds; a transient error that exhausts retries fails only that item (unrelated item in the same batch still lands); a non-transient error is not retried; queue-full drops are counted; a writer-thread open failure markswriter_alive=Falsewith alast_errorset.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes