fix(webhooks): offload persist() I/O to blocking thread#2006
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe ChangesWebhook persistence blocking fix
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/openhuman/webhooks/router.rs`:
- Around line 523-525: The detach of tokio::task::spawn_blocking(do_write) in
persist() can allow older snapshots to overwrite newer ones; modify persist()
(the block around tokio::task::spawn_blocking and the do_write closure) to
serialize disk writes—either by introducing a single-threaded write queue/actor
(spawn a dedicated writer task that accepts snapshot+generation via an mpsc
channel) or by adding a monotonic generation counter stored with each snapshot
and have the spawned writer drop writes whose generation is older than the
latest seen; ensure register(), unregister(), and unregister_skill() continue to
call persist() but persist() enqueues the write or tags it with the generation
so writes are executed or persisted strictly in snapshot order.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ad4ee46-6747-47db-a20b-495fe1894f5d
📒 Files selected for processing (1)
src/openhuman/webhooks/router.rs
std::fs::write and create_dir_all in persist() blocked the tokio worker thread on every register/unregister call. Offload to spawn_blocking when a runtime is available; fall back to inline I/O for sync contexts (tests, CLI).
8cdaa57 to
6e2b80e
Compare
Summary
WebhookRouter::persist()now offloadsstd::fs::writeandcreate_dir_alltotokio::task::spawn_blockingwhen called from an async context.Problem
persist()used synchronousstd::fs::writeandstd::fs::create_dir_alldirectly on the tokio worker thread. Everyregister(),unregister(), andunregister_skill()call blocked the async runtime for the duration of the disk write. Under rapid webhook registration churn this stalls concurrent tasks and causes latency spikes.Solution
Extract the I/O into a closure and dispatch it via
tokio::task::spawn_blockingwhenHandle::try_current()succeeds. When no runtime is present (unit tests, one-shot CLI), the closure runs inline so existing tests remain deterministic without requiring#[tokio::test].Submission Checklist
persist_and_load_roundtriptest covers the inline fallback path; async path exercised by integration tests in CI.Impact
Related
Closes #1933
Summary by CodeRabbit