Repository navigation
Conversation
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.
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. |
There was a problem hiding this comment.
🟢 Approval recommended
The change is test-only, preserves the original required evidence checks, and adds deterministic positive/negative coverage for the updated predicate.
Pull request overview
This PR updates the Linux CLI backup signal-cleanup test logic to accept two valid diagnostic orderings (supervisor-first vs execution-first) while keeping the shared “failed cleanup evidence” requirements intact, adding deterministic regressions to prevent future drift.
Changes:
- Extracted a shared predicate (
has_failed_cli_backup_cleanup_evidence) for “failed cleanup evidence” detection that allows either explicit cleanup-uncertainty wrapper. - Added deterministic positive/negative tests to ensure both wrapper variants are accepted and missing evidence is rejected.
- Updated the existing failing-cancel assertion to use the shared predicate (no production code changes).
File summaries
| File | Description |
|---|---|
| tests/cli_backup.rs | Refactors the failed-cleanup stderr predicate to accept both valid diagnostic orderings and adds deterministic coverage for the predicate. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
The original marker-loss cause is now fixed in PR409, which includes this diagnostic assertion correction plus the bounded 250 ms → 1 s watcher-stop grace and delayed-watch regression. PR409 is the self-contained repair for #399; this PR remains open as the diagnostic-only history until maintainers choose whether to supersede it. |
|
Superseded by #409. The merged PR includes this cleanup-diagnostics test coverage together with the runtime shutdown-grace fix. |
Summary
Refs #399; partial repair only. The original missing
watch-stoppedmarker remains under investigation and this PR must not close that issue.Tracing the CLI backup signal test exposed a separate diagnostic assertion race: three of 127 traced repetitions rejected valid execution-first cleanup uncertainty because the test also required the alternative supervisor shutdown wrapper.
The test now accepts either explicit uncertainty path while retaining the common transport failure, cancellation, and watcher evidence. Deterministic captured-message coverage fails with the old predicate and passes with the correction. Negative controls remove each required piece of evidence from both wrapper variants.
No production code changes. The signal identity, non-success exit, shutdown bound, remote cancellation markers, staging cleanup, watch-stopped marker, process-group cleanup, original-state preservation, and no-catalog-commit assertions remain unchanged.
Verification
cargo fmt --allandcargo fmt --all -- --checkpassed.python3 scripts/check-build-pipeline.py cipassed: policy/generation, Clippy, workspace tests (library1452 passed, one existing ignored), Basecamp wallet isolation, native CTest2, web tests4, headless CLI, QML tests1342.The original marker failure was not reproduced during this investigation. Its assertion remains intact; no timeout increase, runtime workaround, or retry was added.