fix(grpc): terminate workers on server shutdown#1603
Conversation
Signed-off-by: Trevor McKay <tmgithub1@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesThe gRPC server now uses coordinated SIGINT/SIGTERM handling through gRPC shutdown flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/grpc/server/grpc_worker_infra.cpp`:
- Around line 211-216: Synchronize all access to worker_pids during shutdown:
update kill_all_workers in cpp/src/grpc/server/grpc_worker_infra.cpp (lines
211-216) to use the same mutex protecting worker_monitor_thread’s reads and
writes, and update the shutdown path in cpp/src/grpc/server/grpc_server_main.cpp
(lines 328-342) to stop and join monitor_thread before invoking kill_all_workers
if that is the chosen strategy. Ensure no worker_pids access occurs concurrently
with shutdown.
In `@cpp/tests/linear_programming/grpc/grpc_integration_test.cpp`:
- Around line 1700-1733: Update SigintDuringRunningJobShutsDownPromptly to poll
the server process’s actual exit status with waitpid(..., WNOHANG) instead of
relying on server_.is_running(), while preserving the 15-second timeout and
failure message. Ensure the child process is reaped and the test exits the loop
when it has terminated, including zombie states.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ff657884-bd78-4ba5-bbfa-6e9ac4c9e81c
📒 Files selected for processing (7)
cpp/docs/grpc-server-architecture.mdcpp/src/grpc/server/grpc_server_main.cppcpp/src/grpc/server/grpc_server_types.hppcpp/src/grpc/server/grpc_worker.cppcpp/src/grpc/server/grpc_worker_infra.cppcpp/tests/linear_programming/grpc/grpc_integration_test.cppdocs/cuopt/source/cuopt-grpc/grpc-server-architecture.md
CI Test Summary✅ All 31 test job(s) passed. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/grpc/server/grpc_worker_infra.cpp (1)
247-260: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGate
WaitForCompletion()on shutdown state.cancel_all_active_jobs_for_shutdown()clearswaiting_threads, but a late RPC can still create a freshJobWaiterafterward and miss the shutdown notification. Checkshm_ctrl->shutdown_requestedunderwaiters_mutexand return the shutdown error immediately.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/grpc/server/grpc_worker_infra.cpp` around lines 247 - 260, Update WaitForCompletion() to check shm_ctrl->shutdown_requested while holding waiters_mutex before creating or registering a new JobWaiter. If shutdown is requested, return the existing “Server shutting down” error immediately and avoid adding the waiter to waiting_threads, preserving cancel_all_active_jobs_for_shutdown() behavior for already-registered waiters.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cpp/src/grpc/server/grpc_worker_infra.cpp`:
- Around line 247-260: Update WaitForCompletion() to check
shm_ctrl->shutdown_requested while holding waiters_mutex before creating or
registering a new JobWaiter. If shutdown is requested, return the existing
“Server shutting down” error immediately and avoid adding the waiter to
waiting_threads, preserving cancel_all_active_jobs_for_shutdown() behavior for
already-registered waiters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 278ac4c5-0794-439f-91a0-ad6a43fb3e08
📒 Files selected for processing (4)
cpp/docs/grpc-server-architecture.mdcpp/src/grpc/server/grpc_server_main.cppcpp/src/grpc/server/grpc_server_types.hppcpp/src/grpc/server/grpc_worker_infra.cpp
🚧 Files skipped from review as they are similar to previous changes (3)
- cpp/docs/grpc-server-architecture.md
- cpp/src/grpc/server/grpc_server_main.cpp
- cpp/src/grpc/server/grpc_server_types.hpp
The SIGINT integration test still timed out at 15s after bounded waitpid, which points at shutdown never starting. In a multithreaded gRPC/CUDA process, async signal handlers are often never invoked because library threads mask signals. Block SIGINT/SIGTERM before spawning threads, consume them with sigwait on a dedicated thread, and add a 3s _exit watchdog if cleanup wedges. Signed-off-by: Trevor McKay <tmgithub1@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/grpc/server/grpc_server_main.cpp`:
- Around line 197-199: Update the error handling in the pthread_sigmask call to
capture its direct return code and pass that code to strerror, rather than using
errno. Preserve the existing SERVER_LOG_ERROR message context and return value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b7a209e6-7e41-4525-9a54-09c9c1606452
📒 Files selected for processing (3)
cpp/docs/grpc-server-architecture.mdcpp/src/grpc/server/grpc_server_main.cppcpp/src/grpc/server/grpc_server_types.hpp
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/docs/grpc-server-architecture.md
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/grpc/server/grpc_worker_infra.cpp (1)
202-210: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep
worker_pidsaligned with worker ids.spawn_workers()appends only successful PIDs, so a startup failure compacts the vector and shifts later entries.worker_monitor_thread()then treats the vector index as the worker id when it respawns (spawn_single_worker(dw.index)), which can restart the wrong worker/pipe slot. Pre-sizeworker_pidstoconfig.num_workersand assign byi, or store the worker id with each PID.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/grpc/server/grpc_worker_infra.cpp` around lines 202 - 210, Update spawn_workers() so worker_pids preserves worker-id indexing: pre-size it to config.num_workers and assign each successful PID at index i rather than appending. Keep failed startup entries in their corresponding slots so worker_monitor_thread() and spawn_single_worker(dw.index) continue targeting the correct worker and pipe.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/tests/linear_programming/grpc/grpc_integration_test.cpp`:
- Around line 1750-1753: Replace the fixed two-second sleep in the SIGINT
shutdown test with bounded polling through check_status(), waiting until the job
is confirmed to be PROCESSING. Assert that this state is reached before
signaling the server, while retaining the existing wait_exited assertion to
validate prompt shutdown during an active job.
- Around line 201-227: Update wait_exited so it checks the result of reap_group
before calling clear_lifecycle_state. Only clear lifecycle state and return
success when the worker process group is fully reaped; otherwise preserve the
group state for TearDown’s server_.stop() to clean up and return failure.
---
Outside diff comments:
In `@cpp/src/grpc/server/grpc_worker_infra.cpp`:
- Around line 202-210: Update spawn_workers() so worker_pids preserves worker-id
indexing: pre-size it to config.num_workers and assign each successful PID at
index i rather than appending. Keep failed startup entries in their
corresponding slots so worker_monitor_thread() and spawn_single_worker(dw.index)
continue targeting the correct worker and pipe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 667172ec-65cf-4d8f-bbba-7b5c13fc4101
📒 Files selected for processing (5)
cpp/src/grpc/server/grpc_server_main.cppcpp/src/grpc/server/grpc_server_threads.cppcpp/src/grpc/server/grpc_server_types.hppcpp/src/grpc/server/grpc_worker_infra.cppcpp/tests/linear_programming/grpc/grpc_integration_test.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- cpp/src/grpc/server/grpc_server_types.hpp
- cpp/src/grpc/server/grpc_server_main.cpp
|
/ok to test 5ae595c |
Serialize worker_pids behind a mutex so the monitor and shutdown kill path cannot race. Have the SIGINT test reap via waitpid so a zombie parent is not treated as still running. Log pthread_sigmask failures with its return code rather than errno. Signed-off-by: Trevor McKay <tmgithub1@gmail.com>
5ae595c to
bcddd9d
Compare
|
/ok to test bcddd9d |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/grpc/server/grpc_server_threads.cpp`:
- Around line 70-72: Update the worker respawn handling around
spawn_single_worker so that failures where new_pid <= 0 emit an error log
identifying the affected worker slot and the failed respawn; preserve the
existing success log for new_pid > 0.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 41e6bcf6-1a6b-4869-a12e-5dfd661dbf2f
📒 Files selected for processing (5)
cpp/src/grpc/server/grpc_server_main.cppcpp/src/grpc/server/grpc_server_threads.cppcpp/src/grpc/server/grpc_server_types.hppcpp/src/grpc/server/grpc_worker_infra.cppcpp/tests/linear_programming/grpc/grpc_integration_test.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- cpp/src/grpc/server/grpc_server_main.cpp
- cpp/src/grpc/server/grpc_server_types.hpp
- cpp/tests/linear_programming/grpc/grpc_integration_test.cpp
- cpp/src/grpc/server/grpc_worker_infra.cpp
| if (new_pid > 0) { | ||
| SERVER_LOG_INFO("[Server] Restarted worker %zu with PID %d", dw.index, new_pid); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Log respawn failures.
When spawn_single_worker fails (new_pid <= 0), nothing is logged — the slot silently stays at 0 and the failure is invisible, making a permanently degraded worker pool hard to diagnose in production.
📝 Proposed fix
if (new_pid > 0) {
SERVER_LOG_INFO("[Server] Restarted worker %zu with PID %d", dw.index, new_pid);
+ } else {
+ SERVER_LOG_ERROR("[Server] Failed to restart worker %zu", dw.index);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (new_pid > 0) { | |
| SERVER_LOG_INFO("[Server] Restarted worker %zu with PID %d", dw.index, new_pid); | |
| } | |
| if (new_pid > 0) { | |
| SERVER_LOG_INFO("[Server] Restarted worker %zu with PID %d", dw.index, new_pid); | |
| } else { | |
| SERVER_LOG_ERROR("[Server] Failed to restart worker %zu", dw.index); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/grpc/server/grpc_server_threads.cpp` around lines 70 - 72, Update the
worker respawn handling around spawn_single_worker so that failures where
new_pid <= 0 emit an error log identifying the affected worker slot and the
failed respawn; preserve the existing success log for new_pid > 0.
1 participant
Fix an oversight on grpc server shutdown. The server would not exit if a worker was running a solve. The original Python REST server killed any running jobs if the server was shutdown and set the job status accordingly. This matches that behavior.
Without this behavior, the process has to be killed from the OS or every running job id has to be explicitly canceled.