fix(storage): serialize msg_secret reads through the db semaphore - #832
Merged
Conversation
get_msg_secret and get_msg_secret_with_ts ran on raw spawn_blocking, bypassing the single-permit db semaphore every write path acquires. A read racing a write transaction hits the shared-cache table lock on in-memory stores (SQLITE_LOCKED, which busy_timeout does not cover) and the caller treats the error as a missing secret, so an inbound addon could spuriously fail to find its parent secret. The write-behind drain made the overlap reachable: the enc-comment pipeline test flaked at roughly coin-flip rate per process. File-backed stores were shielded by WAL. Both reads now take the semaphore like the rest of the store; the flaky test passes 12/12 consecutive runs against this fix.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughTwo database read methods for message secrets— ChangesMessage Secret Read Serialization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
Benchmark Results67 unchanged benchmark(s)
|
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.
Problem
get_msg_secretandget_msg_secret_with_tsran their queries on rawspawn_blocking, bypassing the single-permitdb_semaphorethat every write path (with_retry) and most reads (with_semaphore) acquire. A read racing a write transaction hits the shared-cache table lock on in-memory stores and fails with SQLITE_LOCKED, whichbusy_timeoutdoes not cover. Both call sites inmsg_secret.rstreat a backend error as a miss, so an inbound add-on could spuriously fail to find its parent secret and surface undecrypted.The write-behind drain from #831 made the overlap reachable in practice: the detached batch write races the very next message's secret lookup. The
enc_comment_inbound_dispatches_body_with_parent_linkpipeline test flaked at roughly coin-flip rate per process (reproduced on clean main, 2 failures in 6 runs), and the failure is silent because the lookup-miss path logs nothing. File-backed stores are shielded by WAL (readers don't block on writers), so this chiefly affects in-memory stores: the test suite and any embedded consumer using:memory:.Change
Route both reads through
with_semaphore, like the rest of the store. A read that overlaps a write now waits for the permit instead of erroring out as a phantom miss.Tests
No new test: the existing pipeline test is the regression guard, and it goes from ~50% flake per process to 12/12 consecutive green runs with this fix. Full workspace suite green,
cargo clippy --all-targets -- -D warningsclean.Breaking
None.