Skip to content

feat: Catch and handle SIGTERM: - #104

Merged
sitaowang1998 merged 89 commits into
y-scope:mainfrom
sitaowang1998:worker_sigterm
Apr 24, 2025
Merged

feat: Catch and handle SIGTERM:#104
sitaowang1998 merged 89 commits into
y-scope:mainfrom
sitaowang1998:worker_sigterm

Conversation

@sitaowang1998

@sitaowang1998 sitaowang1998 commented Apr 9, 2025

Copy link
Copy Markdown
Collaborator
  • In a scheduler, flag all running threads to stop and then exit.
  • In a worker, forward it to its task executor and handle the result.
  • In a task executor, run the default handler (unless overridden by user).

Description

This pr resolves #66 by gracefully ending a component when receiving a SIGTERM.

When a scheduler receives a SIGTERM, it finishes all current tasks, e.g. scheduling task to worker, garbage collection, failure recover, etc, and then exits with SIGTERM.

When a worker receives a SIGTERM, if it has no task executor, it exits immediately with SIGTERM.
If the worker has a task executor, it sends a SIGTERM to the task executor and waits for it to exit. Normally the task executor exits immediately, and the worker sets the task as failed. If a task installs a signal handler and catches SIGTERM, then the task executor completes the execution of the task, and worker handles the task output as usual. Then the worker exits with SIGTERM.

This pr also adds integration tests for worker signal handling.

Checklist

  • The PR satisfies the contribution guidelines.
  • This is a breaking change and that has been indicated in the PR title, OR this isn't a
    breaking change.
  • Necessary docs have been updated, OR no docs need to be updated.

Validation performed

  • GitHub workflows pass
  • Unit tests pass in dev container
  • Integration tests pass in dev container

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Enhanced signal handling in scheduler and worker for clean shutdown on SIGTERM.
    • Implemented a global stop flag for coordinated termination across components.
    • Added child process and task executor process ID tracking.
    • Introduced new test tasks and integration tests to validate signal handling and termination behavior.
  • Bug Fixes

    • Fixed exit code reporting to reflect signal-based termination accurately.
  • Chores

    • Centralized configuration for integration tests with global variables.
    • Updated build configurations to include new source and test files.

@sitaowang1998
sitaowang1998 requested a review from a team as a code owner April 9, 2025 15:11
@coderabbitai

coderabbitai Bot commented Apr 9, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

This change replaces the previous per-instance StopToken mechanism with a singleton StopFlag for global stop signaling across both scheduler and worker components. Signal handlers for SIGTERM are introduced in both the scheduler and worker processes, invoking the new StopFlag to initiate shutdown. The worker process now tracks the child task executor PID and, upon receiving SIGTERM, signals the child for termination. The polling loops for tasks and heartbeats check the global stop flag and exit gracefully when set. Error codes and process exit codes are updated to reflect signal-based termination. New tests and utilities for signal handling are also added.

Changes

Files/Group Change Summary
src/spider/utils/StopFlag.hpp, src/spider/utils/StopFlag.cpp Introduce StopFlag singleton class for global, thread-safe stop signaling with static methods for requesting, checking, and resetting stop state.
src/spider/utils/StopToken.hpp Refactor StopToken to static singleton-like interface (now unused in main code), deleting constructors and making all methods static.
src/spider/scheduler/scheduler.cpp, src/spider/worker/worker.cpp Add SIGTERM signal handler to set stop flag; refactor polling/heartbeat loops to use StopFlag; update exit codes for signal-based termination.
src/spider/scheduler/SchedulerServer.cpp, src/spider/scheduler/SchedulerServer.hpp Remove StopToken from server interface and implementation; use StopFlag for stop signaling.
src/spider/CMakeLists.txt Replace StopToken with StopFlag in build; add new child PID tracking sources.
src/spider/worker/ChildPid.hpp, src/spider/worker/ChildPid.cpp Add ChildPid singleton class for tracking and updating the child process PID in a signal-safe manner.
src/spider/worker/Process.hpp, src/spider/worker/Process.cpp Add get_pid() method to Process class for retrieving process ID.
src/spider/worker/TaskExecutor.hpp, src/spider/worker/TaskExecutor.cpp Add get_pid() method to TaskExecutor class for retrieving process ID.
src/spider/worker/task_executor.cpp Add new error code for signal handler installation; shift subsequent error codes.
tests/CMakeLists.txt Add new signal_test shared library with signal handling test sources.
tests/worker/signal-test.hpp, tests/worker/signal-test.cpp Add signal handling and sleep test tasks for integration testing of signal propagation.
tests/scheduler/test-SchedulerServer.cpp Remove StopToken usage from scheduler server test.
tests/integration/test_signal.py Add integration tests for SIGTERM handling, verifying worker and task executor shutdown and signal propagation.
tests/integration/utils.py Add utility to allocate a free TCP port for tests; introduce global scheduler port variable.
tests/integration/client.py, tests/integration/test_client.py, tests/integration/test_scheduler_worker.py Refactor to use global storage URL and scheduler port variables for test configuration.

Sequence Diagram(s)

sequenceDiagram
    participant OS as Operating System
    participant Scheduler as Scheduler Process
    participant Worker as Worker Process
    participant TaskExec as Task Executor Process

    OS->>Scheduler: Send SIGTERM
    Scheduler->>Scheduler: stop_scheduler_handler()
    Scheduler->>StopFlag: request_stop()
    Scheduler->>Scheduler: Main loop checks is_stop_requested()
    Scheduler->>Scheduler: Exit gracefully

    OS->>Worker: Send SIGTERM
    Worker->>Worker: stop_task_handler()
    Worker->>StopFlag: request_stop()
    Worker->>ChildPid: get_pid()
    alt If child PID exists
        Worker->>TaskExec: Send SIGTERM
    end
    Worker->>Worker: Main loop checks is_stop_requested()
    Worker->>Worker: Exit gracefully
Loading

Assessment against linked issues

Objective Addressed Explanation
Stop polling for new task when receiving SIGTERM (immediate stop, graceful in-progress handling, cleanup) (#66)

Possibly related PRs

  • y-scope/spider#37: Introduced the original StopToken class and its integration; this PR replaces and extends that mechanism with a global StopFlag and improved signal handling.

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f576518 and e82151c.

📒 Files selected for processing (1)
  • docs/src/user-docs/guides-quick-start.md (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/src/user-docs/guides-quick-start.md
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: non-storage-unit-tests (ubuntu-24.04)
  • GitHub Check: non-storage-unit-tests (ubuntu-22.04)
  • GitHub Check: lint

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@sitaowang1998
sitaowang1998 requested a review from davidlion April 9, 2025 15:13

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
src/spider/worker/worker.cpp (3)

206-218: Consider returning std::optional<Task> rather than a bool.
Using bool plus out-parameter may obscure the reason for failure. Returning an optional Task would be more expressive.


370-373: Use consistent negation style.
Instead of if (false == optional_task.has_value()), consider if (!optional_task). This minor style tweak improves readability.


531-535: Infinite wait block might benefit from a logging statement.
Consider logging that the program is idling indefinitely to indicate the state if --no-exit is enabled.

 while (true) {
+    spdlog::info("Worker is idle and will not exit. Waiting indefinitely...");
     std::this_thread::sleep_for(std::chrono::seconds(1));
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0273157 and 7875aff.

📒 Files selected for processing (5)
  • src/spider/scheduler/scheduler.cpp (7 hunks)
  • src/spider/utils/StopToken.hpp (1 hunks)
  • src/spider/worker/task_executor.cpp (4 hunks)
  • src/spider/worker/worker.cpp (13 hunks)
  • tests/integration/test_no_exit.py (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/spider/scheduler/scheduler.cpp (1)
src/spider/worker/worker.cpp (2)
  • heartbeat_loop (115-152)
  • heartbeat_loop (115-120)
tests/integration/test_no_exit.py (1)
tests/integration/client.py (3)
  • get_task_state (160-168)
  • storage (79-82)
  • submit_job (85-136)
src/spider/worker/worker.cpp (6)
src/spider/worker/WorkerClient.hpp (1)
  • fail_task_id (34-35)
src/spider/core/Task.hpp (2)
  • input (190-190)
  • input (190-190)
src/spider/storage/MetadataStorage.hpp (16)
  • conn (27-27)
  • conn (29-29)
  • conn (30-30)
  • conn (32-33)
  • conn (37-40)
  • conn (42-48)
  • conn (50-51)
  • conn (53-54)
  • conn (56-57)
  • conn (59-63)
  • conn (66-67)
  • conn (69-73)
  • conn (75-75)
  • conn (76-76)
  • conn (77-78)
  • conn (80-80)
src/spider/worker/DllLoader.hpp (1)
  • instance (12-17)
src/spider/worker/TaskExecutor.cpp (2)
  • error (34-37)
  • error (34-34)
src/spider/storage/mysql/MySqlStorage.cpp (2)
  • fetch_task (628-634)
  • fetch_task (628-628)
🪛 Cppcheck (2.10-2)
src/spider/worker/worker.cpp

[performance] 177-177: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.

(useInitializationList)

⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: lint
  • GitHub Check: non-storage-unit-tests (ubuntu-22.04)
  • GitHub Check: non-storage-unit-tests (ubuntu-24.04)
🔇 Additional comments (37)
src/spider/utils/StopToken.hpp (5)

4-4: Added <csignal> for signal-safe type

The inclusion of <csignal> is necessary to use the std::sig_atomic_t type that's being introduced.


9-9: Updated request_stop() to use signal-safe operations

Changed from using a boolean value to using 1 to work with the new std::sig_atomic_t type.


11-11: Modified stop_requested() to check for non-zero value

This change is consistent with the type change and ensures proper evaluation in a signal-safe manner.


13-13: Updated reset() to use signal-safe operations

Changed from using a boolean value to using 0 to work with the new std::sig_atomic_t type.


16-16: Changed m_stop type to be signal-safe

Switching from std::atomic<bool> to std::sig_atomic_t volatile is an appropriate change for a signal handler context. The volatile qualifier prevents compiler optimizations that could interfere with signal handling, and std::sig_atomic_t guarantees atomic access even in signal handlers.

src/spider/scheduler/scheduler.cpp (9)

2-2: Added <csignal> for signal handling

Necessary for using std::signal and signal handler functions later in the code.


36-39: Updated error codes to include signal handling errors

Added a new error code cSignalHandleErr and adjusted subsequent error codes accordingly. This ensures consistent error reporting for signal handler installation failures.


45-46: Added global stop token and signal handler

The global g_stop_token and stop_scheduler_handler function work together to implement graceful shutdown behavior when receiving SIGTERM. The NOLINT directive is appropriate as this global variable is necessary for signal handling.

Also applies to: 48-52


72-72: Added command-line option for no-exit behavior

The --no-exit option allows users to specify that the scheduler should not exit when receiving SIGTERM.


164-164: Added no-exit flag parsing logic

This code correctly parses and stores the --no-exit command-line option.

Also applies to: 181-183


190-196: Added signal handler installation for SIGTERM

This code installs the stop_scheduler_handler function as a handler for SIGTERM signals, but only if the --no-exit option is specified. It also includes proper error handling if the installation fails.


242-245: Using global stop token for scheduler server

The scheduler server now uses the global g_stop_token instead of a local one, ensuring consistent shutdown behavior across the application.


254-254: Updated thread references to use global stop token

Both the heartbeat and cleanup threads now use the global g_stop_token, ensuring they respond to the same stop signals as the rest of the application.

Also applies to: 262-262


272-276: Added infinite loop for no-exit mode

When --no-exit is true, the program enters an infinite loop after normal shutdown processing, keeping the process alive as specified by the PR requirements.

src/spider/worker/task_executor.cpp (4)

3-3: Added <csignal> for signal handling

Necessary for using std::signal and signal handler functions later in the code.


35-37: Added empty signal handler for SIGTERM

This "do nothing" handler effectively causes the task executor to ignore SIGTERM signals, as specified in the PR requirements.


71-76: Updated error codes to include signal handling errors

Added a new error code cSignalHandleErr and adjusted subsequent error codes accordingly. This ensures consistent error reporting for signal handler installation failures.


121-125: Added signal handler installation for SIGTERM

This code installs the empty sigterm_handler function as a handler for SIGTERM signals, ensuring the task executor ignores these signals. It also includes proper error handling if the installation fails.

tests/integration/test_no_exit.py (5)

1-21: Added necessary imports for integration testing

The imports cover all required components for signal handling, process management, and Spider's client functionalities.


24-50: Implemented helper function to start processes with no-exit flag

The start_scheduler_worker_no_exit function properly sets up both the scheduler and worker processes with the necessary command-line arguments, including the --no-exit flag for the worker.


55-65: Created fixture for test environment setup and teardown

The scheduler_worker_no_exit fixture handles the lifecycle of test processes, ensuring they're properly initialized before tests and cleaned up afterward.


67-94: Implemented test for worker SIGTERM handling - Part 1

The first part of the test verifies that the worker process doesn't exit when receiving a SIGTERM signal and that new tasks remain in the "ready" state without being processed.


95-101: Implemented test for worker SIGTERM handling - Part 2

The second part of the test verifies that the worker continues to ignore SIGTERM signals and properly cleans up resources at the end of the test.

src/spider/worker/worker.cpp (14)

2-2: LGTM on including <csignal>.
Necessary for handling signals. No issues detected.


50-54: Constants for error codes look good.
These new constants clarify specific error conditions.


82-82: Command-line option for --no-exit is well-defined.
Approach is straightforward, enabling or disabling exit-on-SIGTERM behaviour.


156-173: Graceful fetch loop integrates the stop token effectively.
The loop neatly checks stop_requested(). This design helps with graceful shutdown during task polling.


175-204: Argument buffer handling is clear and robust.
Returning std::optional on parsing errors is a clean approach. The logging provides sufficient context too.

🧰 Tools
🪛 Cppcheck (2.10-2)

[performance] 177-177: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.

(useInitializationList)


220-245: Good modular design in setup_task.
Consolidating connection creation, task fetching, and argument preparation enhances readability and maintainability.


429-429: Default value for no_exit is clear.
Fine approach to initialize it as false and update based on parsed arguments.


446-448: Conditional assignment of no_exit is appropriate.
Straightforward logic to set the flag.


457-463: Signal handler installation aligns with --no-exit.
Using std::signal is appropriate here given the requirement. No further concerns.


513-513: Passing global stop token to heartbeat thread is sensible.
No issues noted with the references for maintaining heartbeat until stop is requested.


525-525: Passing global stop token to the task loop is consistent.
Ensures both loops have the same exit condition.


59-60:

Details

❓ Verification inconclusive

Check thread-safety for global StopToken.
You are using a non-const global variable, which can introduce concurrency issues. Confirm that StopToken operations are fully thread-safe under all conditions.


🏁 Script executed:

#!/bin/bash
# Description: Search for StopToken class usage to confirm thread-safety assumptions.
ast-grep --pattern $'class StopToken {\n  $$$\n}'

Length of output: 48


Attention: Verify Thread-Safety in StopToken Implementation

The initial verification using ast-grep did not produce any output, which leaves the thread-safety of the global StopToken uncertain. Please manually verify that the StopToken class implements proper synchronization (e.g., atomic operations or locking mechanisms) for all its mutable operations. In particular, review the source file(s) containing the StopToken implementation to ensure that simultaneous accesses from multiple threads are handled safely, or consider refactoring to mitigate potential concurrency issues.


278-352:

Details

❓ Verification inconclusive

Validate coverage for error-handling paths.
This new function is essential to handle success/failure. Ensure unit tests or integration tests cover each branch of this logic (retry, deadlock, parse failures, etc.).


🏁 Script executed:

#!/bin/bash
# Description: Search tests referencing `handle_executor_result`.
rg 'handle_executor_result' -A 8

Length of output: 1332


Action: Ensure Comprehensive Test Coverage for All Error-Handling Branches

Please verify that there are unit or integration tests explicitly covering each failure and retry path in handle_executor_result. In particular, confirm tests exist for the following cases:

  • Storage Connection Failure: When storage_factory->provide_storage_connection() returns a StorageErr, ensuring that the error is logged and the function returns false.
  • Executor Failure: When executor.succeed() returns false, confirming that a warning is logged and metadata_store->task_fail is invoked.
  • Result Parsing Failure: When executor.get_result_buffers() returns an empty optional, triggering an error log and corresponding metadata update.
  • Output Parsing Failure: When parse_outputs does not return valid outputs, ensuring the error path is exercised and handled.
  • Submission Retry Logic: The retry loop for submitting the result—particularly ensuring that deadlock errors are retried properly and non-deadlock errors immediately log an error and exit.

The current usage in worker.cpp suggests that the function is invoked in production code; thus, dedicated tests covering these branches are critical to avoid regressions.


62-66:

Details

❓ Verification inconclusive

Confirm signal-handler safety.
Signal handlers must only call functions that are async-signal-safe. Ensure request_stop() is guaranteed safe.


🏁 Script executed:

#!/bin/bash
# Description: Check if StopToken::request_stop() has disclaimers about signal safety.
rg 'StopToken::request_stop' -A 5

Length of output: 33


Action Required: Confirm Signal-Handler Safety for request_stop()

The recent search for indications of async-signal-safety in the StopToken::request_stop() implementation did not return any documentation or code comments confirming that the function is guaranteed safe to call from within a signal handler. In signal handling contexts, it is crucial that only async-signal-safe functions are invoked.

Please verify the following:

  • Inspect the implementation of StopToken::request_stop() (and any functions it calls) for usage of non async-signal-safe operations (e.g., memory allocation, locking, I/O).
  • Confirm that any inline comments or documentation explicitly state that this function is safe to be called from a signal handler.
  • Ensure that no unsafe operations are performed that could compromise reliability when handling signals.

If no explicit assurance is present, please review and potentially refactor the code to meet async-signal-safety requirements.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (3)
tests/integration/test_singal.py (3)

60-64: time.sleep(5) is brittle; poll for readiness & ensure clean shutdown

Sleeping for a fixed interval slows the suite and can still be wrong on slow CI runners. Replace with an exponential back‑off that polls the scheduler’s TCP port (or a health‑check endpoint) until it is open or a timeout elapses.

Also, worker_process.kill() in teardown is unconditional – if the worker already exited, kill() raises ProcessLookupError on some platforms. Guard it:

for proc in (worker_process, scheduler_process):
    if proc.poll() is None:
        proc.kill()
    proc.wait(timeout=10)

This guarantees no orphaned children remain.


178-181: Left‑over commented cleanup risks leaking test data

remove_job for graph_id is commented out. Re‑enable it to keep the database clean:

-# remove_job(storage, graph_id)
+remove_job(storage, graph_id)

1-1: Minor: File name typo

The file is called test_singal.py. Renaming it to test_signal.py avoids future confusion and keeps naming consistent with the subject matter.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3809b06 and daab477.

📒 Files selected for processing (4)
  • src/spider/worker/task_executor.cpp (2 hunks)
  • tests/integration/test_singal.py (1 hunks)
  • tests/worker/signal-test.cpp (1 hunks)
  • tests/worker/signal-test.hpp (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • src/spider/worker/task_executor.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/worker/signal-test.hpp
  • tests/worker/signal-test.cpp
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/integration/test_singal.py (1)
tests/integration/client.py (4)
  • get_task_outputs (139-157)
  • get_task_state (160-168)
  • storage (79-82)
  • submit_job (85-136)
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: lint
  • GitHub Check: non-storage-unit-tests (ubuntu-24.04)
  • GitHub Check: non-storage-unit-tests (ubuntu-22.04)

Comment thread tests/integration/test_singal.py
Comment thread tests/integration/test_singal.py Outdated
Comment thread tests/integration/test_singal.py
Comment thread tests/integration/test_singal.py Outdated
@sitaowang1998
sitaowang1998 requested a review from davidlion April 17, 2025 20:25

@davidlion davidlion left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you go over coderabbit's concerns. Many of them seem worth addressing.

At minimum it is correct that the file tests/integration/test_singal.py is a typo.

In the future, ideally we should try to address these prior to requesting review.

Comment thread src/spider/worker/worker.cpp
@sitaowang1998
sitaowang1998 requested a review from davidlion April 22, 2025 05:29
Comment thread src/spider/scheduler/scheduler.cpp Outdated
Comment thread src/spider/worker/worker.cpp Outdated
Comment thread src/spider/worker/worker.cpp Outdated
Comment thread src/spider/scheduler/scheduler.cpp Outdated
Comment thread src/spider/worker/ChildPid.hpp Outdated
sitaowang1998 and others added 5 commits April 23, 2025 15:54
Co-authored-by: davidlion <davidlion2@protonmail.com>
Co-authored-by: davidlion <davidlion2@protonmail.com>
Co-authored-by: davidlion <davidlion2@protonmail.com>
Co-authored-by: davidlion <davidlion2@protonmail.com>
Co-authored-by: davidlion <davidlion2@protonmail.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/spider/worker/worker.cpp (1)

63-78: Well-implemented SIGTERM handler.

The signal handler appropriately sets the global stop flag and propagates the termination signal to the task executor. Using kill() in a signal handler is safe as it's async-signal-safe.

Two suggestions for enhanced reliability:

  1. Consider adding volatile to the pid variable for signal-safety
  2. Consider handling any error returned by kill()
auto stop_task_handler(int signal) -> void {
    if (SIGTERM == signal) {
        spider::core::StopFlag::request_stop();
        // Send SIGTERM to task executor
-       pid_t const pid = spider::core::ChildPid::get_pid();
+       volatile pid_t const pid = spider::core::ChildPid::get_pid();
        if (pid > 0) {
            // NOLINTNEXTLINE(misc-include-cleaner)
-           kill(pid, SIGTERM);
+           if (kill(pid, SIGTERM) < 0) {
+               // We can't log in signal handlers, but we could set a global error flag
+               // that can be checked later in a safe context
+           }
        }
    }
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f3b9ff9 and 60ac968.

📒 Files selected for processing (3)
  • src/spider/scheduler/scheduler.cpp (8 hunks)
  • src/spider/worker/ChildPid.hpp (1 hunks)
  • src/spider/worker/worker.cpp (11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/spider/scheduler/scheduler.cpp
  • src/spider/worker/ChildPid.hpp
🧰 Additional context used
🪛 Cppcheck (2.10-2)
src/spider/worker/worker.cpp

[performance] 178-178: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.

(useInitializationList)

⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: lint
  • GitHub Check: non-storage-unit-tests (ubuntu-24.04)
  • GitHub Check: non-storage-unit-tests (ubuntu-22.04)
🔇 Additional comments (13)
src/spider/worker/worker.cpp (13)

1-5: Appropriate additional headers for signal handling.

The added headers support the new signal handling functionality, particularly unistd.h for pid_t type and kill() function, and csignal for SIGTERM handling.


48-49: Good replacement of StopToken with global StopFlag and ChildPid.

The switch from local StopToken instances to a global StopFlag singleton allows coordinated shutdown across components, and the new ChildPid header provides signal-safe child process tracking.


54-58: Well-defined error constants.

These new error constants provide clear identification for different failure scenarios, improving error diagnostics and making the code more maintainable.


126-133: Good migration to global StopFlag in heartbeat loop.

The heartbeat loop now correctly checks the global stop flag instead of a local token, enabling coordinated shutdown across components.


158-159: Consistent StopFlag usage for critical failures.

Using the global stop flag for critical failures in the heartbeat loop ensures the worker properly initiates shutdown when storage connections repeatedly fail.


166-181: Improved fetch_task signature and graceful exit mechanism.

The return type change to std::optional allows the function to cleanly signal when a stop is requested, providing a structured way to exit the task loop. The function now properly handles termination requests while waiting for tasks.

🧰 Tools
🪛 Cppcheck (2.10-2)

[performance] 178-178: Variable 'm_id' is assigned in constructor body. Consider performing initialization in initialization list.

(useInitializationList)


209-210: Changed shared_ptr to unique_ptr for storage connection.

Good change - since the connection isn't shared with other components, using unique_ptr is more appropriate and avoids unnecessary reference counting overhead.


356-364: Properly handling task loop shutdown.

The task loop now correctly checks the global stop flag and handles the optional task value returned by fetch_task, ensuring graceful shutdown when a termination signal is received.


389-396: Good child process tracking and termination logic.

The code correctly:

  1. Gets and stores the executor's PID
  2. Double-checks if stop was requested during setup
  3. Quickly terminates the child if shutdown is already in progress

This ensures a clean and consistent shutdown process with proper signal propagation.


400-401: Properly resetting child PID after task execution.

Resetting the PID to 0 after task execution ensures that subsequent SIGTERM signals won't be sent to terminated processes, preventing potential errors.


412-412: Appropriate signal exit base value.

The exit code base value for signals (128) follows the common convention in Unix/Linux systems where exit codes for processes terminated by signals are typically calculated as 128 + signal number.


452-462: Well-implemented signal handler installation.

The code correctly:

  1. Sets up the signal action structure
  2. Properly initializes the signal mask
  3. Sets the SA_RESTART flag, which is good for automatically restarting interrupted system calls
  4. Handles installation errors appropriately

Note: The NOLINT directives are appropriate until the work in ystdlib-cpp (issue #58) to add support for signal-related functions in wrapped_facade_headers is completed.


527-530: Proper exit code handling for signal termination.

Setting the exit code to cSignalExitBase + SIGTERM when a SIGTERM is received allows the parent process to distinguish between normal termination and signal-based termination, which is useful for monitoring and orchestration tools.

@davidlion davidlion changed the title feat: Graceful exit when receiving SIGTERM. feat: Catch and handle SIGTERM: Apr 23, 2025

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
docs/src/user-docs/guides-quick-start.md (2)

227-231: Clarify imperative form and improve list items
Use the imperative mood and correct pluralisation for clarity and consistency. Consider this diff:

-To stop the cluster, sends `SIGTERM` to the scheduler and all workers.
+To stop the cluster, send `SIGTERM` to the scheduler and all workers.

-A scheduler finishes the current tasks, e.g. scheduling task to worker, garbage collection, failure
-recovery, etc, then exits with `SIGTERM`.
+The scheduler finishes the current tasks (e.g., scheduling tasks to workers, garbage collection, failure
+recovery, etc.), then exits with `SIGTERM`.

232-237: Fix grammar and add missing articles for readability
Insert the missing comma after “Normally” and include “the” before “worker” and “task executor” for better readability. For example:

-When a worker receives `SIGTERM`, if it has no task executor, it exits immediately with `SIGTERM`.
+When a worker receives `SIGTERM`, if it has no task executor, it exits immediately with `SIGTERM`.

-If the worker has a task executor, it sends a `SIGTERM` to the task executor and waits for it to exit. Normally a task executor exits immediately, and the worker sets the task as failed. If a task installs a signal handler and catches SIGTERM, then the task executor completes the execution of the task, and worker handles the task output as usual. Then the worker exits with SIGTERM.
+If the worker has a task executor, it sends a `SIGTERM` to the task executor and waits for it to exit.  
+Normally, the task executor exits immediately, and the worker sets the task as failed. If the task executor has a signal handler installed and catches `SIGTERM`, it completes the execution of the task, and the worker handles the task output as usual. Then the worker exits with `SIGTERM`.
🧰 Tools
🪛 LanguageTool

[typographical] ~234-~234: Consider adding a comma after ‘Normally’ for more clarity.
Context: ...task executor and waits for it to exit. Normally a task executor exits immediately, and ...

(RB_LY_COMMA)


[uncategorized] ~236-~236: You might be missing the article “the” here.
Context: ...ompletes the execution of the task, and worker handles the task output as usual. Then ...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)


[uncategorized] ~236-~236: You might be missing the article “a” here.
Context: ...ut as usual. Then the worker exits with SIGTERM. # Next steps In future guides, we'll...

(AI_EN_LECTOR_MISSING_DETERMINER_A)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 60ac968 and f576518.

📒 Files selected for processing (1)
  • docs/src/user-docs/guides-quick-start.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/src/user-docs/guides-quick-start.md

[typographical] ~234-~234: Consider adding a comma after ‘Normally’ for more clarity.
Context: ...task executor and waits for it to exit. Normally a task executor exits immediately, and ...

(RB_LY_COMMA)


[uncategorized] ~236-~236: You might be missing the article “the” here.
Context: ...ompletes the execution of the task, and worker handles the task output as usual. Then ...

(AI_EN_LECTOR_MISSING_DETERMINER_THE)


[uncategorized] ~236-~236: You might be missing the article “a” here.
Context: ...ut as usual. Then the worker exits with SIGTERM. # Next steps In future guides, we'll...

(AI_EN_LECTOR_MISSING_DETERMINER_A)

⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: non-storage-unit-tests (ubuntu-22.04)
  • GitHub Check: lint
  • GitHub Check: non-storage-unit-tests (ubuntu-24.04)
🔇 Additional comments (1)
docs/src/user-docs/guides-quick-start.md (1)

225-225: Heading style is consistent
The top-level header “Exiting the cluster” matches the hierarchy used for other major sections in this guide.

@sitaowang1998
sitaowang1998 requested a review from davidlion April 23, 2025 23:37
@sitaowang1998
sitaowang1998 merged commit 4830d26 into y-scope:main Apr 24, 2025
@sitaowang1998
sitaowang1998 deleted the worker_sigterm branch April 24, 2025 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Stop polling for new task when we receive sigterm signal

2 participants