fix(envoy-client): declare actors lost before the engine can reallocate them

[abcxff]

No description provided.

[abcxff]

Stack for rivet-dev/rivet

Current stack:

• #5823 fix(envoy-client): declare actors lost before the engine can … 👈
• #5824 fix(rivetkit): stop lost actor generations immediately
• #5821 fix(envoy-client): signal lost actor generations synchronous …
• #5822 fix(rivetkit): isolate workflow run state per actor generati …
• #5825 fix(rivetkit-napi): key actor runtime state by generation

Dependencies:

• #5816 chore(release): update version to 2.3.23
• main

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

_{change nkyokown}

[claude]

Review

The change is small and the shared declare_actors_lost helper is a good refactor. The tests cover the deadline math and the cancel-before-send ordering. I found one likely bug and a few smaller points.

Likely bug: stale last_ping_ts after a reconnect

last_ping_ts is never reset when the connection drops, when lost fires, or when a new connection opens. engine_ping_silence_deadline_ms only returns None when the value is 0, which only happens before the first ping ever.

1. The envoy is lost and declare_actors_lost clears the actors. last_ping_ts keeps its old value.
2. The envoy reconnects and the engine starts a new actor. The actor is in ctx.actors, so the new silence branch is enabled.
3. The deadline is still old_ping + threshold - margin, which is in the past. The branch fires on the next loop iteration, before the first ping of the new connection arrives. It declares the new actor lost immediately.

Pings are about 3s apart, so there is a window for this on every reconnect after a lost event. A fix is to reset last_ping_ts (to 0 or the connect time) when a new connection opens, or inside declare_actors_lost. A test that declares lost, inserts an actor, and asserts the silence deadline is not already expired would cover it.

Smaller points

• last_ping_ts is stamped and compared with wall-clock now_millis, so a clock step can trigger or delay lost. Consider a monotonic Instant for the receipt time.
• handle_conn_close still starts its own lost_timeout from the close time, using the full threshold. The engine's clock started at its last ping refresh, so that timer can fire after the engine has reallocated. The silence branch covers this when pings were flowing. The docs could mention both paths together.
• declare_actors_lost, engine_ping_silence_deadline_ms and engine_ping_silence_expired are pub only for the tests, which widens the crate API.
• The tests cover the deadline math but not the envoy_loop branch itself, for example that it does not fire when ctx.actors is empty.
• The structured tracing::warn! fields follow the logging convention.

[the-company-company]

🔴 1 high-severity finding

_{Reviewed commit 26a3077.}

[the-company-company]

🔴 High · Ping liveness is not synchronized with the select loop or connection session

last_ping_ts is updated in forward_to_envoy without waking this loop. On a fresh connection, command replay can start actors before the ping task sends its first ping; this iteration therefore builds None, and subsequent pings do not arm a deadline until another envoy message or the 15-second KV cleanup tick. With the default 15-second lost threshold, a link that goes half-open in that window can let the engine expire the envoy before this branch runs, defeating the single-writer protection this change is meant to add. The timestamp also survives reconnects, so a reconnect after the old deadline immediately loses still-running or replayed actors before the new connection's first ping. Make ping reception/session changes an event observed by this loop (for example, a watch channel carrying the current session's last-ping value), reset it when a connection is established, and derive/restart the deadline from that event.