You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Review: feat(container-runner): keep instance warm instead of self-exiting
Small, focused change: the container-runner no longer self-exits when the last actor stops (on_destroy) or when a start fails on an otherwise-idle instance (on_start). It now stays warm and relies solely on the platform's SIGTERM (or the engine draining the /start connection after the request lifespan) to reap the instance. Docs/comments (README.md, main.rs, actor.rs) were updated consistently, no stale references to the old "exits when no actors remain" behavior found.
(Re-checked after the latest push. The commit content is unchanged from the previous review, just rebased/merged onto a newer base. Findings below still apply.)
Code quality
The removed if children().is_empty() { request_exit(...) } branches and the now-unused request_exit import in actor.rs are cleanly dropped. Logic is straightforward.
main.rs's async_main still has an else branch (non-signal EXIT cancellation, around main.rs:374-377) that the comment itself documents as unreachable today, since request_exit is only invoked by spawn_signal_handler. That's a fine documented fallback for a future actor-driven exit path, but since it's genuinely dead in practice, a tracing::warn! (or debug_assert!) on entry to that branch would make it loud if something unexpectedly triggers it instead of silently taking an untested code path.
Potential issues / things to double check
Test coverage gap (still present): container-runner/examples/e2e-test/host/run-host-tests.sh Scenario 6 (graceful actor stop, around lines 137-144) destroys the actor and then unconditionally calls stop_runner (kills the runner process) regardless of whether it already exited. Since this PR's entire purpose is changing the runner's self-exit behavior, none of the existing scenarios assert the process is still alive after the last actor is destroyed. Consider adding a kill -0 "$RUNNER_PID" check after destroy_actor (before stop_runner) in Scenario 6 to lock in the new "stays warm" behavior and catch regressions back to the old self-exit path.
Operational consideration (not a bug in this diff): the instance can no longer proactively drain itself when idle. Reclaiming an idle container now depends entirely on the engine closing the /start connection after the request lifespan, or an external SIGTERM arriving. If that signal is ever delayed or lost (engine bug, network partition, misconfigured lifespan), the container will now sit idle indefinitely instead of self-healing by exiting. Worth confirming the engine-side drain path is solid and monitored, since the failure mode shifted from "wasted exit" to "possible idle-resource leak."
Security / performance
No security-relevant surface touched (pure process-lifecycle logic, no new untrusted input handling). Performance impact is minor and net-positive for placement (avoids a cold start when the same instance is reused for the next actor).
Overall this is a reasonable, well-documented behavioral change; my main suggestion remains adding a host-test assertion that directly exercises the "stays warm" property this PR introduces.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.