fix: switch plugin log cache to RwLock, rename ws_send_fire_and_forget, deduplicate EAGAIN constants#410
Conversation
…t, deduplicate EAGAIN constants - Switch PLUGIN_LOG_METADATA_CACHE from Mutex to RwLock so concurrent plugin instances can read simultaneously on the common (cache-hit) path. Only the cache-miss path acquires a write lock. - Rename ws_send_fire_and_forget to ws_send_and_close to accurately reflect that the function awaits the WebSocket close handshake. - Extract EAGAIN_YIELD_THRESHOLD and MAX_EAGAIN_EMPTY_RETRIES to video/mod.rs so dav1d, av1, and vaapi_av1 share a single definition. Closes #396, closes #340, closes #302 Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
…t, deduplicate EAGAIN constants - Switch PLUGIN_LOG_METADATA_CACHE from Mutex to RwLock so concurrent plugin instances can read simultaneously on the common (cache-hit) path. Only the cache-miss path acquires a write lock. - Rename ws_send_fire_and_forget to ws_send_and_close to accurately reflect that the function awaits the WebSocket close handshake. - Extract EAGAIN_YIELD_THRESHOLD and MAX_EAGAIN_EMPTY_RETRIES to video/mod.rs so dav1d, av1, and vaapi_av1 share a single definition. Closes #396, closes #340, closes #302 Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
![staging-devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/870844?s=60&v=4){kind=small}
| // ── Decode-loop backoff constants ──────────────────────────────────────────── | ||
| // | ||
| // Shared across dav1d, rav1d (av1), and VA-API decoder loops to cap | ||
| // empty-EAGAIN retries and transition from yield to sleep. |
There was a problem hiding this comment.
🟡 Added section-divider comments violate the repository comment rules
The new “Decode-loop backoff constants” banner is a section divider comment, which AGENTS.md explicitly forbids (“Section dividers — … Use blank lines or code structure instead”). This is not a runtime issue, but it is a mandatory repository review rule violation in the PR diff.
| // ── Decode-loop backoff constants ──────────────────────────────────────────── | |
| // | |
| // Shared across dav1d, rav1d (av1), and VA-API decoder loops to cap | |
| // empty-EAGAIN retries and transition from yield to sleep. |
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| async fn ws_send_and_close( | ||
| server_url: &str, | ||
| payload: RequestPayload, | ||
| ) -> Result<(), Box<dyn std::error::Error + Send + Sync>> { |
There was a problem hiding this comment.
📝 Info: Fire-and-forget WebSocket close matches the server contract
Although ws_send_and_close closes the WebSocket immediately after sending the request, this specific caller sends RequestPayload::TuneNodeAsync; the server handles that path through handle_tune_node_fire_and_forget, which deliberately returns None rather than a ResponsePayload (apps/skit/src/websocket_handlers.rs:819-881). Therefore this rename does not drop an expected response for the current call site. If this helper is reused later for non-fire-and-forget request types, it would silently discard success/error responses, so keeping it private and narrowly used matters.
(Refers to lines 127-142)
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
| { | ||
| let cache = | ||
| PLUGIN_LOG_METADATA_CACHE.read().unwrap_or_else(std::sync::PoisonError::into_inner); | ||
| if let Some(entry) = cache.get(&key) { | ||
| return entry; | ||
| } | ||
| } | ||
|
|
||
| let mut cache = | ||
| PLUGIN_LOG_METADATA_CACHE.write().unwrap_or_else(std::sync::PoisonError::into_inner); | ||
| cache.entry(key).or_insert_with_key(|(target, level)| { | ||
| let target: &'static str = Box::leak(target.clone().into_boxed_str()); | ||
| let callsite: &'static PluginLogCallsite = Box::leak(Box::new(PluginLogCallsite::new())); |
There was a problem hiding this comment.
📝 Info: RwLock cache still avoids duplicate metadata construction after read misses
The switch from a Mutex to an RwLock introduces a read-miss/write-lock pattern, but the write path uses cache.entry(key).or_insert_with_key(...), so if two threads miss under the read lock, only the first writer constructs and leaks a PluginLogMetadata; later writers observe the occupied entry. The PluginLogCallsite::meta is set before the entry is returned from the insertion closure, preserving the existing publication invariant described around PluginLogCallsite (crates/plugin-native/src/wrapper.rs:828-840).
(Refers to lines 873-898)
Was this helpful? React with 👍 or 👎 to provide feedback.
Debug
Signed-off-by: StreamKit Devin <devin@streamkit.dev> Co-Authored-By: Claudio Costa <cstcld91@gmail.com>
2 participants
Summary
Three small, independent fixes bundled into a single PR:
perf: consider RwLock for plugin log metadata cache #396 — Switch plugin log metadata cache from
MutextoRwLockPLUGIN_LOG_METADATA_CACHEnow usesstd::sync::RwLockso concurrent plugin instances can read simultaneously on the common (cache-hit) path. Only the cache-miss path acquires a write lock.refactor(cli): rename ws_send_fire_and_forget to reflect awaited close handshake #340 — Rename
ws_send_fire_and_forget→ws_send_and_closeThe function awaits
ws_stream.close(None).await?, so "fire and forget" was misleading. Renamed tows_send_and_closealong with all call sites.refactor: deduplicate EAGAIN_YIELD_THRESHOLD backoff in dav1d.rs and av1.rs #302 — Deduplicate
EAGAIN_YIELD_THRESHOLD/MAX_EAGAIN_EMPTY_RETRIESExtracted both constants from
dav1d.rs,av1.rs, andvaapi_av1.rsintovideo/mod.rs. All three decoder modules now import from the shared location.Closes #396, closes #340, closes #302
Review & Testing Checklist for Human
RwLockread-then-write pattern inplugin_log_static_metadata()correctly handles the TOCTOU race (concurrent miss → double insert is safe becauseor_insert_with_keyis idempotent withBox::leak)ws_send_fire_and_forgetwere missed (grep verified only 2 occurrences inclient.rs)Notes
All three changes are mechanical and low-risk. The decode-loop logic is unchanged — only the constant definitions moved.
Devin Review findings addressed:
video/mod.rsper AGENTS.md guidelines.TuneNodeAsynccall site.or_insert_with_keyprevents duplicate metadata construction.Link to Devin session: https://staging.itsdev.in/sessions/a4e4de86f25c457783513c9ca85cbf95
Requested by: @streamer45
Devin Review
9d3276a(HEAD is6788c93)