fix(pegboard-envoy): use async scc methods so actor stop cannot deadlock the envoy command stream

[MasterPtato]

No description provided.

[MasterPtato]

Stack for rivet-dev/actors

• #5717 ci(publish): allow release-mode builds for preview dispatche …
• #5716 fix(pegboard-envoy): use async scc methods so actor stop can … 👈
• #5704 fix(universaldb): run postgres transaction reads concurrentl …
• #5705 test(gasoline): reproduce slow workflow pulls on postgres
• #5702 fix(universaldb): chunk postgres commit requests that exceed …
• #5703 fix(universaldb): clear postgres key ranges one statement pe …
• #5706 perf(universaldb): time each drain batch statement to locali …
• #5695 fix(universaldb): report the last error when transaction ret …
• main

Get stack: forklift get 5716
Push local edits: forklift submit
Merge when ready: forklift merge 5716

_{change rkrusokq}

[the-company-company]

🟠 1 medium-severity finding

_{Reviewed commit 9f9ceb1.}

[the-company-company]

🟠 Medium · Close executors that finish initialization after eviction

OnceCell::get() returns None while a concurrent SQLite request is still inside get_or_try_init. That request already owns an Arc to this cell, so this callback removes the map entry without adding a handle to stopped; the initializer can then publish and use an old-generation executor after the stop command has completed. Once the request releases it, the worker is dropped through the unclean channel-close path rather than the intended close() path.

Coordinate eviction with in-progress initialization (for example, retain a close/cancellation state alongside the cell and have the initializer close a handle published after eviction), so every executor evicted by the lifecycle command is cleanly closed.

[claude]

Review

Overall this is a solid, well-motivated fix (moving pegboard-envoy's remote-SQLite-executor eviction off *_sync scc calls so it can't deadlock the connection's command-path worker), with good test coverage for the deadlock scenario. One likely blocking issue in the core change:

Correctness — entry.consume() probably doesn't exist on the scc entry type

engine/packages/pegboard-envoy/src/actor_lifecycle.rs:26 and :45:

let (_, executor) = entry.consume();

Every other place in this repo that removes an owned (K, V) from a live scc OccupiedEntry uses .remove_entry(), not .consume():

• rivetkit-rust/packages/client/src/connection.rs:530
• rivetkit-rust/packages/rivetkit-core/src/registry/mod.rs:1039
• engine/packages/universalpubsub/src/chunking.rs:158

I couldn't run cargo check in this sandboxed review environment to confirm, but I'd be surprised if scc::hash_map::OccupiedEntry (the type iter_mut_async's closure hands you, per the existing pegboard-gateway/src/shared_state.rs:650 usage) exposes a consume() method — please verify with cargo check -p pegboard-envoy --tests before merging, since if it doesn't compile this is a hard blocker.

Independent of whether .consume() exists, retain_async (already used correctly elsewhere in this same diff, e.g. protocol_version.rs::refresh, and in pegboard-gateway/src/shared_state.rs::gc_in_flight_requests) seems like a simpler way to express the same eviction-with-side-collection logic without needing entry-removal-during-iteration at all, e.g.:

let mut stopped = Vec::new();
conn.remote_sqlite_executors
    .retain_async(|(actor_id, generation), executor| {
        if actor_id != &checkpoint.actor_id {
            return true;
        }
        if *generation <= stopped_generation
            && let Some(handle) = executor.get()
        {
            stopped.push(handle.clone());
        }
        false
    })
    .await;

This keeps the same semantics (evict every cached executor for the actor, but only queue the ones at or before the stopped generation for close()) while reusing a pattern the codebase already proves works.

Nice touches

• Closing evicted executors via tokio::spawn + join_all off the connection's command path is the right call, and NativeDatabaseHandle::close() is already internally timeout-bounded (SQLITE_WORKER_CLOSE_TIMEOUT in depot-client/src/worker.rs), so this doesn't reintroduce an unbounded background task risk.
• The universalpubsub/driver/memory/mod.rs comment replacing the TODO: Is retain_sync bad here? is a genuinely correct justification: the inner map is only ever reachable through the outer map's per-key lock, so nesting a retain_sync inside an already-locked retain_async closure can't contend with an async waiter. Good catch resolving that TODO instead of leaving it.
• New stop_actor_does_not_block_runtime_behind_queued_executor_lookup / shutdown_conn_actors_does_not_block_runtime_behind_queued_executor_lookup tests are a solid regression reproduction of the actual deadlock (single-threaded runtime + watchdog thread, queuing an async entry_async behind a held sync guard on the same key) rather than just asserting the happy path.
• stop_actor_evicts_only_the_stopped_actor_executors correctly covers that a different actor's executors, and a newer generation of the same actor, are left untouched/unclosed respectively.
• The epoxy protocol_version.rs/http_client.rs changes correctly thread the new async fn negotiate through its single call site, and read_async/retain_async/upsert_async are all established, already-used scc methods in this codebase.
• New CLAUDE.md bullet is appropriately concise and matches the existing style/rationale for the Mutex<HashMap<...>> / scc guidance section.

Minor

• No test currently exercises the "newer generation stays open" cache-eviction path end-to-end with an actual NativeDatabaseHandle (only via key presence/absence), but that's a reasonable scope cut given NativeDatabaseHandle needs real SQLite plumbing to construct.

🤖 Generated with Claude Code

[the-company-company]

🟠 1 medium-severity finding

_{Reviewed commit f0330e1.}

[the-company-company]

🟠 Medium · Close executors that finish initialization after eviction

OnceCell::get() returns None while a concurrent SQLite request is still inside get_or_try_init. That request already owns an Arc to this cell, so this callback removes the map entry without adding a handle to stopped; the initializer can then publish and use an old-generation executor after the stop command has completed. Once the request releases it, the worker is dropped through the unclean channel-close path rather than the intended close() path.

Coordinate eviction with in-progress initialization (for example, retain a close/cancellation state alongside the cell and have the initializer close a handle published after eviction), so every executor evicted by the lifecycle command is cleanly closed.