fix(server): Always reject envelopes when shutting down and non-ephemeral#6226
fix(server): Always reject envelopes when shutting down and non-ephemeral#6226klochek wants to merge 4 commits into
Conversation
…eral When we are in the shutting down state (and non-ephemeral), we aren't guaranteed to close the channel fast enough to reject all incoming envelopes. Add back the shutdown handle, but also check to see if we're non-ephemeral. Fixes a flaky test I ran into.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| pub fn try_push(&self, envelope: Managed<Box<Envelope>>) -> Result<(), Rejected<PushError>> { | ||
| if self.addr.is_closed() { | ||
| // This happens for inflight requests when `ephemeral: false`. | ||
| let permanent_shutting_down = !self.is_ephemeral && self.shutdown_handle.shutting_down(); |
There was a problem hiding this comment.
This should be 'just' another atomic check, but worth keeping an eye on performance metrics on rollout as every request will go through this check, in case it's more than just an atomic (backed by a Tokio Watch).
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 77fa440. Configure here.
There was a problem hiding this comment.
I think we can make this more local: The envelope buffer already gets notified of the shutdown, so it could store an atomic bool on EnvelopeBufferMetrics immediately after shutdown.notified() (before calling Self::handle_shutdown).
Then try_push can simply check self.metrics.is_shutting_down. We might want to rename metrics to something more generic though, because it would now contain more than metrics.
My comment is still relevant but I won't be available to approve the PR.
3 participants
When we are in the shutting down state (and non-ephemeral), we aren't guaranteed to close the channel fast enough to reject all incoming envelopes. Add back the shutdown handle, but also check to see if we're non-ephemeral. Fixes a flaky test I ran into.