fix(tests): Make test_shutdown.py wait for Relay to be up before proceeding#6226
Conversation
![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.
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.
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 e715c51. Configure here.
e715c51 to
f1e9280
Compare
klochek
changed the title
|
After running into the issue again, I determined that a cause of this flake is that Relay's EnvelopeBufferService might not even have initialized by the time we send the SIGTERM, and so EnvelopeBufferService can just handle the envelope payload before having a chance to respond to the shutdown. I also realized that dealing with processing on shutdown is inherently racy: there's no way to enforce a "drop if non-ephemeral" policy given that the EnvelopeBufferService can be in any state, and responding to any ready future in its select! loop (envelope bytes, or shutdown signal), so instead, adjust the test to test the thing we want: that a ready Relay will respond to the SIGTERM promptly, and not finish envelope processing. |
klochek
changed the title
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.