167 - Wait for aborted process exit before finalising schedule - #167
Conversation
|
Comment
You can also request review from a specific team by commenting 💡 If you see something that doesn't look right, check the configuration guide. |
This is currently a non-blocking check, and it will not prevent your Pull Request from being merged.The CODEOWNER of this repository has some untagged assets.As this is a public repository, asset details will not be shown. |
There was a problem hiding this comment.
Pull request overview
Changes: Bugfix (1), Test improvement (1), Documentation update (1), Meta (1)
This PR updates exec_schedule to keep schedules marked as running until an aborted child process is confirmed exited, and to consume repeated abort_running requests during shutdown waits.
Changes:
- Add
consume_abort_running()and update the abort flow to wait for child exit before clearingis_running. - Add focused unit tests covering abort supervision and state transitions.
- Update docs/config examples (YAML quoting for
"*", abort semantics, ERD source link) and refresh repo guidance/changelog entries.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
cicada/commands/exec_schedule.py |
Implements the “assume running until polled closed” behavior and consumes repeated abort requests while waiting. |
tests/test_exec_schedule.py |
New unit tests validating abort wait semantics, repeated abort consumption, and state-clearing SQL. |
tests/test_functional_main.py |
Fixes YAML for returncodes_alert and resets the Slack mock between loop iterations. |
config/example.yml |
Updates example config to quote "*" so it parses as a string instead of YAML alias syntax. |
README.md |
Documents updated abort behavior and links the editable ERD source. |
docs/erd.excalidraw |
Adds the editable Excalidraw ERD source file. |
CLAUDE.md |
Replaces prior content with authoritative repository guidance and workflow expectations. |
CHANGELOG.md |
Adds a new 0.10.3 section describing the abort-running behavior and ERD source addition. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
9d297ff to
34ce074
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (2)
.github/workflows/linter.yml:19
- This job uses a GitHub-hosted runner, which violates the organization requirement that all workflow jobs run on self-hosted
gha-production-*runners. The lint workload should use the standard production runner.
runs-on: ubuntu-22.04
.github/workflows/pytest.yml:20
- This job uses a GitHub-hosted runner, which violates the organization requirement that all workflow jobs run on self-hosted
gha-production-*runners. Use the standard medium runner (or a larger approved tier if the Docker test workload requires it).
runs-on: ubuntu-22.04
e1fb625 to
d62b993
Compare
d62b993 to
46a4deb
Compare
|
Comment
You can also request review from a specific team by commenting 💡 If you see something that doesn't look right, check the configuration guide. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cicada/commands/exec_schedule.py:283
- Deliver the shutdown signal before checking
abort_running. As written, a received SIGTERM/SIGQUIT is recorded here, but the next operation opens a database connection; becausedb_cicada()has noconnect_timeout, a database outage can delay child termination for the connection timeout plus the retry sleep. This leaves the launched process running during supervisor shutdown. Terminate immediately once the shutdown request is observed, and retry a failed delivery before performing the database check.
signal_number = shutdown_request["signal"]
if signal_number is not None and requested_stop_result is None:
requested_stop_result = ExecutionResult(-15, f"{signal.Signals(signal_number).name} received")
d6b2f13 to
8614057
Compare
Related to AP-2830
Context
Cicada currently clears
is_runningand finalizes the schedule log immediately afterabort_runningsendsSIGTERM, even though the launched process may still be running.Refer to 0.10.4 CHANGELOG for full details.
Changes
Test coverage
abort_running, 5 covering broaderexec_schedulebehaviour.abort_running, plus 15 supporting shutdown, error-handling, and database cases.abort_runningcase. It previously checked only the final log result; it now also confirmsis_runningandabort_runningare cleared after the process exits.Validation
make pytest: 162 passed, 79.72% coveragemake flake8: passed with 0 errorsmake black: passedChecklist