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

[abcxff]

No description provided.

[abcxff]

Stack for rivet-dev/rivet

Current stack:

• #5843 fix(envoy-client): keep the ping silence deadline bounded ac …
• #5838 fix(envoy-client): ignore pings from earlier connections in …
• #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 approach is sound. Extracting declare_actors_lost removes duplicated teardown, and the new loss path keeps the "cancel the lost token first" ordering. The docs note is accurate about the best-effort limit.

Potential bugs

1. Stale last_ping_ts across reconnects. last_ping_ts is only written in forward_to_envoy (connection/mod.rs). Nothing resets it when a connection closes or a new one opens. The new engine_ping_silence arm is armed whenever last_ping_ts != 0 and !ctx.actors.is_empty().
   • Scenario: the connection drops and the envoy reconnects after more than threshold - margin (about 12s with a 15s threshold). The lost timeout is cancelled on reconnect, but last_ping_ts still holds the old value. The silence deadline is already in the past, so the arm fires immediately and declares healthy actors lost.
   • Suggested fix: on connection open, reset the baseline. Either store now_millis() or set it to 0 so the deadline is only computed from pings on the current connection.
   • Add a test for the reconnect case.
2. No protection before the first ping. last_ping_ts == 0 returns None, so a half-open connection that never delivers a ping has no silence timeout. Seeding the baseline at connection open (see 1) would close this gap too.
3. Clock source. The deadline is compared against wall-clock now_millis(), so an NTP step can fire it early or late. is_ping_healthy uses the same pattern, so this is minor.

Code quality

• declare_actors_lost and the engine_ping_silence_* functions are pub only for the integration tests. Consider #[doc(hidden)].
• The message parameter is logged as reason but is a full sentence. Per CLAUDE.md, prefer a short stable label such as lost_threshold or ping_silence.
• envoy_lost_threshold_ms uses try_lock on protocol_metadata and silently falls back to 10s on contention. That is pre-existing, but it now feeds a timing-critical path.

Tests

• The deadline math and declare_actors_lost are covered. There is no test of the envoy_loop branch itself, covering the recheck after a ping arrives mid-sleep and the reconnect case from item 1.

Security and performance

• No security concerns and no new hot-path cost.

Item 1 is the one I would fix before merge.

[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.