Skip to content

Commit aa7708f

Browse files
committed
fix: address Greptile review — bounded total shutdown budget
- Enforce stub_timeout_seconds_ as a single total budget across the finalize-wait and process-exit-wait phases; healthy teardown previously could take up to 2 * stub_timeout_seconds_. - Clamp the Pop timeout to INT_MAX to avoid narrowing when passing int64_t into MessageQueue::Pop(int const&). - Re-poll waitpid once after the final sleep window in WaitForStubProcessWithTimeout so a stub that exits during that second is not force-killed unnecessarily.
1 parent 1852204 commit aa7708f

1 file changed

Lines changed: 33 additions & 3 deletions

File tree

src/stub_launcher.cc

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,10 @@
2626

2727
#include "stub_launcher.h"
2828

29+
#include <algorithm>
30+
#include <chrono>
2931
#include <filesystem>
32+
#include <limits>
3033

3134
#include "pb_utils.h"
3235
#include "python_be.h"
@@ -802,17 +805,33 @@ void
802805
StubLauncher::TerminateStub()
803806
{
804807
if (is_initialized_) {
808+
// Enforce stub_timeout_seconds_ as a single total budget across the
809+
// finalize wait and the subsequent process-exit wait; without this the
810+
// healthy teardown path could take up to 2 * stub_timeout_seconds_.
805811
bool force_kill = false;
812+
int64_t remaining_seconds = stub_timeout_seconds_;
806813
if (is_healthy_) {
807-
const int64_t timeout_ms = stub_timeout_seconds_ * 1000;
814+
const int64_t total_timeout_ms = stub_timeout_seconds_ * 1000;
815+
// MessageQueue::Pop takes `int const&`; clamp to INT_MAX so a large
816+
// stub_timeout_seconds_ cannot silently narrow to a negative value.
817+
const int pop_timeout_ms = static_cast<int>(std::min<int64_t>(
818+
total_timeout_ms,
819+
static_cast<int64_t>(std::numeric_limits<int>::max())));
808820
// Finalize command does not have any arguments.
809821
std::unique_ptr<IPCMessage> ipc_message =
810822
IPCMessage::Create(shm_pool_, false /* inline_response */);
811823

812824
ipc_message->Command() = PYTHONSTUB_FinalizeRequest;
813825
stub_message_queue_->Push(ipc_message->ShmHandle());
814826
bool success = false;
815-
parent_message_queue_->Pop(timeout_ms, success);
827+
const auto pop_start = std::chrono::steady_clock::now();
828+
parent_message_queue_->Pop(pop_timeout_ms, success);
829+
const int64_t pop_elapsed_s =
830+
std::chrono::duration_cast<std::chrono::seconds>(
831+
std::chrono::steady_clock::now() - pop_start)
832+
.count();
833+
remaining_seconds =
834+
std::max<int64_t>(0, stub_timeout_seconds_ - pop_elapsed_s);
816835
if (!success) {
817836
force_kill = true;
818837
}
@@ -826,7 +845,7 @@ StubLauncher::TerminateStub()
826845

827846
if (force_kill) {
828847
KillStubProcess();
829-
} else if (!WaitForStubProcessWithTimeout(stub_timeout_seconds_)) {
848+
} else if (!WaitForStubProcessWithTimeout(remaining_seconds)) {
830849
KillStubProcess();
831850
}
832851
}
@@ -964,6 +983,17 @@ StubLauncher::WaitForStubProcessWithTimeout(int64_t timeout_seconds)
964983
sleep(1);
965984
}
966985

986+
// The process may have exited during the final sleep window; re-check once
987+
// more so we don't force-kill an already-dead process.
988+
{
989+
int status;
990+
pid_t ret = waitpid(stub_pid_, &status, WNOHANG);
991+
if (ret == stub_pid_ || ret == -1) {
992+
stub_pid_ = 0;
993+
return true;
994+
}
995+
}
996+
967997
return false;
968998
#endif
969999
}

0 commit comments

Comments
 (0)