Repository navigation
fix(cli): preserve delayed event-watch cleanup marker - #409
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
eb30913 to
5154326
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5154326909
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| #[test] | ||
| fn cli_backup_download_signal_cleanup_allows_delayed_watch_exit() -> Result<()> { | ||
| use nix::sys::signal::Signal; | ||
|
|
There was a problem hiding this comment.
When Linux tests run in parallel, this call duplicates the existing failed-cleanup test's (SIGTERM, false) inputs, while assert_cli_backup_signal_cleanup derives instance_id only from the signal, cleanup outcome, and shared test-process PID. Both tests therefore use the same /tmp/logos_core_service_* file, and either test's RemoveFileOnDrop can unlink it before the other child reads its metadata, causing an unrelated intermittent pre-dispatch failure. Include the delay/test variant or another unique value in the instance ID.
Useful? React with 👍 / 👎.
560a9e2 to
1f7e675
Compare
Cancellation can return execution cleanup uncertainty before the supervisor adds its shutdown wrapper. Accept either explicit path while retaining transport cancellation and watcher evidence requirements. Add deterministic captured-message coverage and negative controls for both wrappers. Keep the watch-stopped marker, process-group cleanup, shutdown bounds, remote cancellation, staging, and state assertions unchanged. Validation: cargo fmt --all; cargo fmt --all -- --check; all 9 CLI backup tests; 100 traced signal-cleanup repetitions; python3 scripts/check-build-pipeline.py ci (passed, including 1342 QML tests). Refs #399. The original missing watch-stopped marker remains under investigation; this test-only change does not claim a runtime shutdown fix.
1f7e675 to
15a5b6e
Compare
Summary
Fixes #399.
The CLI backup event-watch cleanup could force-kill the watcher process group after the 250 ms grace period, before a TERM trap completed under scheduler load. That removed the
watch-stoppedevidence and caused intermittent shutdown assertion failures.Verification
cargo fmt --allcargo fmt --all -- --checkcargo test --locked --lib modules::logos_core --no-fail-fast(84 passed)cargo test --locked --test cli_backup --no-fail-fast(10 passed)python3 scripts/check-build-pipeline.py ci(passed)