Simplify actor exit logging - #6644
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
https://github.com/quickwit-oss/quickwit/blob/f6a7ebb85646c530de318e702faa4e4ac150d1c7/quickwit-actors/src/spawn_builder.rs#L421
When an actor exits normally (or is killed/downstream-closed) but its finalize hook returns an error, finalize changes the final status to Panicked after fault_opt has already been set to None; this call then kills the switch without recording an actor-fault or making kill_switch.fault() available. Finalizers can perform fallible cleanup (for example source shutdown), so the actual root cause is lost in exactly the path where the final exit status is a failure; build the fault from final_exit_status or include the finalize error before calling exit.
https://github.com/quickwit-oss/quickwit/blob/f6a7ebb85646c530de318e702faa4e4ac150d1c7/quickwit-actors/src/actor_handle.rs#L104-L106
When an actor task has already panicked, actor_loop never reaches ctx.exit, so the actor state remains running even though joining the task would report ActorExitStatus::Panicked. A supervisor's next progress check can therefore enter this timeout branch first and store "<actor> stopped reporting progress" as the kill-switch fault; because the first fault wins, panic scenarios get reported as frozen actors instead of the actual panic.
https://github.com/quickwit-oss/quickwit/blob/f6a7ebb85646c530de318e702faa4e4ac150d1c7/quickwit-actors/src/spawn_builder.rs#L413-L414
For an actor that exits with ActorExitStatus::DownstreamClosed without a downstream actor having already recorded the shared kill-switch fault, this arm passes None to ctx.exit; should_activate_kill_switch still kills the pipeline, but it now uses plain kill() and emits no actor-fault while the pipeline failure logs were downgraded to debug. In those downstream-closed failure paths, the kill-switch-triggering cause is no longer visible at error level, so include the actor/phase/status as a fault here too.
https://github.com/quickwit-oss/quickwit/blob/f6a7ebb85646c530de318e702faa4e4ac150d1c7/quickwit-actors/src/actor_context.rs#L218
Because actor_state is set to failure just above this match, a heartbeat running on another Tokio worker can observe the failed state, terminate the pipeline, and call plain kill() on the same switch before this line stores the fault. In that race, kill_with_fault discards the real error because the switch is already dead, so intermittent actor failures can again have no actor-fault; record the fault before exposing the failed state or allow setting the first fault on a dead switch.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dae0f4bb8a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
1 participant
Description
The current logging is very inefficient and unhelpful - a lot of errors from all of the exiting actors, nothing informative about what actually happened.
This change saves the original fault that caused the pipeline to exit - only on a failure - and logs it, once. It also removes all of the other actor exiting lines so it should be significantly more streamlined.
Going to test on a local cluster.