169 - Wait for aborted process exit before finalising schedule - #169
169 - Wait for aborted process exit before finalising schedule#169louis-pie wants to merge 1 commit into
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), Maintenance (1)
This PR adjusts exec_schedule so an abort_running request no longer clears is_running / finalizes the schedule log until the launched child process has actually exited, preventing the scheduler from considering a schedule “free” while its prior process is still alive.
Changes:
- Keep an aborted schedule marked as running until the child process exits; consume repeated
abort_runningrequests while waiting. - Add/extend tests to assert running/abort flags are cleared only after process exit.
- Update release metadata (changelog/version) and adjust local pytest coverage threshold (currently lowered).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
cicada/commands/exec_schedule.py |
Tracks whether an abort was received and waits for child exit before finalizing, while clearing repeated abort requests during shutdown. |
tests/test_exec_schedule.py |
Adds a focused unit test asserting termination happens before unset_is_running and finalize_schedule_log. |
tests/test_functional_main.py |
Extends an existing functional abort test to also assert is_running and abort_running are cleared after exit. |
CHANGELOG.md |
Adds 0.10.4 notes describing the abort-wait behavior and related test changes (and mentions reducing coverage threshold). |
setup.py |
Bumps package version to 0.10.4. |
Makefile |
Lowers --cov-fail-under from 78 to 75 (flagged for change). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| pytest: | ||
| cd $(mkfile_path) ;\ | ||
| . venv/bin/activate ;\ | ||
| pytest tests/ --verbose --cov=cicada --cov-fail-under=78 --cov-report term-missing | ||
| pytest tests/ --verbose --cov=cicada --cov-fail-under=75 --cov-report term-missing |
| Tests and CI | ||
| ~~~~~~~~~~~~ | ||
| - Add coverage that an aborted schedule is finalized only after its process exits | ||
| - Update the existing abort test to confirm the running and abort flags are cleared after process exit | ||
| - Set the minimum test coverage to 75% |
|
will instead merge #167 |
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.This is a minimal, human-reviewable alternative to #167 and is intended to replace it. It starts from current
mainand implements only the essential wait-for-exit behavior, without the broader process-supervision refactor from #167.Refer to 0.10.4 CHANGELOG for full details.
Changes
Test coverage
abort_running, 5 covering broaderexec_schedulebehavior.abort_running, 0 covering broaderexec_schedulebehavior.abort_running, 0 covering broaderexec_schedulebehavior. It now confirmsis_runningandabort_runningare cleared after the process exits.Validation
make pytest: 142 passed, 78.12% coveragemake flake8: passed with 0 errorsmake black: passedAP-2830