fix: wrap datastore worker in catch_unwind to prevent poisoned locks

[tobixen]

Summary

Use std::panic::catch_unwind() around the datastore worker loop as a safety net. If any panic occurs, the worker will exit gracefully instead of poisoning the Mutex lock and leaving the server in an unusable state.

Problem

When the datastore worker thread panics (for any reason), it poisons the Mutex lock. All subsequent requests then fail with "poisoned lock: another task failed inside" errors, and the server becomes completely unusable until restart.

This is the symptom described in #405

Solution

Wrap the worker's work_loop() in catch_unwind():

• Catches any panic that occurs
• Logs the panic message
• Lets the worker exit cleanly
• The channel closes properly
• Future requests get clean "channel closed" errors instead of poisoned lock errors

This is a defense-in-depth approach - ideally panics should be replaced with proper error handling, but this safety net ensures no panic can leave the server in an unrecoverable state.

Test plan

• cargo build compiles successfully
• cargo test --package aw-datastore passes (9 tests)

🤖 Generated with Claude Code

---

Important

Wrap work_loop() in catch_unwind() in aw-datastore/src/worker.rs to handle panics gracefully and prevent poisoned locks.

• Behavior:
  • Wrap work_loop() in catch_unwind() in aw-datastore/src/worker.rs to handle panics gracefully.
  • Logs panic messages and allows the worker to exit cleanly, preventing poisoned locks.
  • Ensures future requests receive clean "channel closed" errors instead of "poisoned lock" errors.

^{This description was created by for f8cf670. You can customize this summary. It will automatically update as commits are pushed.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to f8cf670 in 36 seconds. Click for details.

• Reviewed 35 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 3 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. aw-datastore/src/worker.rs:315

• Draft comment:
  Good use of catch_unwind to safeguard against panics. Ensure the worker state (di) is unwind safe; using AssertUnwindSafe here assumes all captured state is safe to unwind.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. aw-datastore/src/worker.rs:327

• Draft comment:
  Panic payload extraction is correctly handled. Optionally, consider logging additional context or backtrace to aid debugging.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

3. aw-datastore/src/worker.rs:338

• Draft comment:
  After a panic, the worker thread exits, closing the channel. Confirm that this shutdown behavior (vs. automatically restarting a new worker) is acceptable for the server’s resilience strategy.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_cxDmxiQ85n2qLVTV

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[greptile-apps]

Greptile Summary

Wrapped the datastore worker's work_loop() in std::panic::catch_unwind() to prevent mutex lock poisoning when panics occur. This defense-in-depth fix ensures the server can't be left in an unrecoverable state where all subsequent requests fail with "poisoned lock" errors.

Key Changes

• Added catch_unwind wrapper around work_loop() call in worker thread
• Extracts and logs panic messages when caught
• Allows worker to exit cleanly, closing the channel properly
• Clients now receive clean "channel closed" errors instead of poisoned lock errors

Analysis

The fix addresses the root symptom from issue #405 where panics in the worker thread would poison the Mutex, making the entire server unusable until restart. While the PR correctly notes that panics should ideally be replaced with proper error handling, this safety net is appropriate for:

1. Defense-in-depth: Prevents any panic (expected or not) from leaving the server in an unrecoverable state
2. Improved error experience: Users get cleaner error messages instead of cryptic "poisoned lock" failures
3. Graceful degradation: Server can be restarted without manual intervention

The use of AssertUnwindSafe is appropriate here since the DatastoreWorker state is completely discarded after the panic - there's no risk of accessing corrupted state since the worker thread exits and all its owned data is dropped.

Confidence Score: 4/5

• Safe to merge with high confidence - defensive fix that prevents server lockup without introducing new risks
• Score of 4 reflects a well-implemented defensive fix with clear benefits and minimal risk. The implementation correctly uses catch_unwind to prevent lock poisoning, properly logs panics, and allows graceful degradation. The use of AssertUnwindSafe is justified since worker state is fully discarded. Not rated 5 only because this is a symptomatic fix rather than addressing the root causes of panics (lines 136, 145, 195 still use panic! for transaction errors), though the PR acknowledges this is defense-in-depth.
• No files require special attention - the single-file change is straightforward and well-tested

Important Files Changed

Filename	Overview
aw-datastore/src/worker.rs	Wraps datastore worker loop in catch_unwind to prevent lock poisoning when panics occur - defensive fix that prevents server lockup

Sequence Diagram

sequenceDiagram
    participant Client as API Client
    participant Datastore as Datastore (Main Thread)
    participant Channel as MPSC Channel
    participant Worker as DatastoreWorker Thread
    participant CatchUnwind as catch_unwind Handler
    participant SQLite as SQLite Database

    Note over Datastore,Worker: Initialization
    Datastore->>Channel: Create MPSC channel
    Datastore->>Worker: Spawn worker thread
    Worker->>CatchUnwind: Wrap work_loop in catch_unwind
    CatchUnwind->>Worker: Execute work_loop(method)
    Worker->>SQLite: Open connection
    Worker->>Worker: Initialize DatastoreInstance

    Note over Client,SQLite: Normal Operation
    Client->>Datastore: Request (e.g., insert_events)
    Datastore->>Channel: Send command
    Channel->>Worker: Deliver command
    Worker->>SQLite: Execute SQL operation
    SQLite-->>Worker: Result
    Worker->>Channel: Send response
    Channel->>Datastore: Deliver response
    Datastore-->>Client: Return result

    Note over Client,SQLite: Panic Scenario (Before Fix)
    Client->>Datastore: Request triggers panic
    Datastore->>Channel: Send command
    Channel->>Worker: Deliver command
    Worker->>SQLite: Execute operation
    SQLite-->>Worker: Unexpected error
    Worker->>Worker: panic! occurs
    Note over Worker: Thread exits, Mutex poisoned
    Client->>Datastore: Subsequent request
    Datastore--XClient: Error: "poisoned lock"
    Note over Datastore: Server unusable until restart

    Note over Client,SQLite: Panic Scenario (After Fix)
    Client->>Datastore: Request triggers panic
    Datastore->>Channel: Send command
    Channel->>Worker: Deliver command
    Worker->>SQLite: Execute operation
    SQLite-->>Worker: Unexpected error
    Worker->>Worker: panic! occurs
    Worker->>CatchUnwind: Panic caught
    CatchUnwind->>CatchUnwind: Log panic message
    CatchUnwind->>Worker: Exit gracefully
    Worker->>Channel: Channel closes cleanly
    Note over Worker: No lock poisoning
    Client->>Datastore: Subsequent request
    Datastore->>Channel: Send command
    Channel--XDatastore: Channel closed error
    Datastore-->>Client: Clean error (channel closed)
    Note over Client: Clear error, can restart server gracefully

Loading

[greptile-apps]

Greptile's behavior is changing!

From now on, if a review finishes with no comments, we will not post an additional "statistics" comment to confirm that our review found nothing to comment on. However, you can confirm that we reviewed your changes in the status check section.

_{This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR".}

[tobixen]

This pull request was created by Claude the AI. I don't know rust well enough to have a subjective point of view if this change is good or not. The prompt given to Claude was like this:

I don't know anything about rust, but wouldn't it be possible to make a general fix so that panic!(...) would cause an immediate graceful exit rather than putting the server in an unusable state?

My rationale is that a server that is down may be auto-restarted, and it's also easy to catch this through monitoring. A server that is up and listening to the port but not working at all may be a little bit harder to monitor.

I run the server through systemd, and systemd supports auto-restarting. I'm not sure how things work when utilizing the aw-qt app.

[0xbrayo]

LGTM

[0xbrayo]

I'm not sure how things work when utilizing the aw-qt app.

I believe only non-zero exit codes get restarted (that's how it works in aw-tauri, not sure about aw-qt). Since that's a crash. This would cause it to exit normally and might not be restarted. You can still throw a panic after gracefully handling the first one though. If that is deemed necessary

[codecov]

Codecov Report

❌ Patch coverage is 33.33333% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.75%. Comparing base (656f3c9) to head (f8cf670).
⚠️ Report is 15 commits behind head on master.

Files with missing lines	Patch %	Lines
aw-datastore/src/worker.rs	33.33%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #553      +/-   ##
==========================================
- Coverage   70.81%   67.75%   -3.06%     
==========================================
  Files          51       54       +3     
  Lines        2916     3142     +226     
==========================================
+ Hits         2065     2129      +64     
- Misses        851     1013     +162     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.