fix(ci): harden cache writes and readiness cleanup - #1123
Conversation
📝 WalkthroughWalkthroughGitHub-hosted ChangesCI execution contracts
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
ndizazzo
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/tests/test_ci_client_readiness_smoke.py (1)
87-112: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the platform-universal smoke assertions signal-agnostic.
Windows shutdown sends
CTRL_BREAK_EVENT, but this test hard-codesSIGTERMexpectations in both the signal marker check and the cleanup failure message, so those assertions do not match the Windows path. Assert the platform-appropriate signal/message instead, or split the assertions by platform.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/test_ci_client_readiness_smoke.py` around lines 87 - 112, Update test_clean_sigterm_shutdown_succeeds_and_reaps_runtime so its expected signal marker and cleanup-related assertions use the platform-appropriate shutdown signal, selecting SIGTERM on POSIX and CTRL_BREAK_EVENT on Windows. Keep the existing PID and successful-exit assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/design/TESTING.md`:
- Around line 524-526: Update the shutdown requirement wording in the testing
documentation to say “requires a bounded, graceful SIGTERM shutdown,” adding the
comma and article while preserving the surrounding Unix, Windows, and SIGINT
behavior text.
---
Outside diff comments:
In `@scripts/tests/test_ci_client_readiness_smoke.py`:
- Around line 87-112: Update
test_clean_sigterm_shutdown_succeeds_and_reaps_runtime so its expected signal
marker and cleanup-related assertions use the platform-appropriate shutdown
signal, selecting SIGTERM on POSIX and CTRL_BREAK_EVENT on Windows. Keep the
existing PID and successful-exit assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b3368847-ad65-4ce4-887d-afadd4c73969
📒 Files selected for processing (5)
.agents/skills/manage-ci/SKILL.md.agents/skills/manage-ci/references/current-inventory.mddocs/design/TESTING.mdscripts/ci-client-readiness-smoke.shscripts/tests/test_ci_client_readiness_smoke.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .agents/skills/manage-ci/references/current-inventory.md
| and requires bounded graceful SIGTERM shutdown on Unix or CTRL_BREAK_EVENT | ||
| on Windows. Interactive Ctrl-C/SIGINT behavior is tested separately because | ||
| noninteractive background shell children may inherit SIGINT as ignored. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Improve the shutdown requirement wording.
Change “requires bounded graceful SIGTERM shutdown” to “requires a bounded, graceful SIGTERM shutdown.”
As indicated by the provided LanguageTool finding.
🧰 Tools
🪛 LanguageTool
[style] ~524-~524: The double modal “requires bounded” is nonstandard (only accepted in certain dialects). Consider “to be bounded”.
Context: ...dy/role=client` event, and requires bounded graceful SIGTERM shutdown on Unix or CT...
(NEEDS_FIXED)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/design/TESTING.md` around lines 524 - 526, Update the shutdown
requirement wording in the testing documentation to say “requires a bounded,
graceful SIGTERM shutdown,” adding the comma and article while preserving the
surrounding Unix, Windows, and SIGINT behavior text.
Source: Linters/SAST tools
1 participant
Outcome
This closes two reliability defects exposed by the final GitHub-hosted cold/warm controls:
Evidence
Cold GitHub-hosted main run 30548920143 attempt 1 was green, but recorded 3,251 remote sccache write errors. The four concurrent Rust shards produced 3,060 of them (94.1%); two shards failed every remote write while all tests passed. The broader warm run 30552393814 restored each shard's distinct 2.18–3.47 GB target cache and all four shards had zero misses, writes, or errors.
Other jobs remain remote-enabled because their measured reuse is valuable and their error rates are low. The exact Swift ABI warm hit reduced the full Swift producer from 35m35s to 13m23s, so this change does not disable producer caching.
The exact like-for-like warm rerun then exposed CUDA product cleanup job 90917729846: readiness, artifact verification, and launch passed; SIGINT produced no event for 15 seconds; fallback SIGTERM immediately emitted shutdown_requested and shutdown and exited. The shared smoke now starts graceful cleanup with SIGTERM instead of misclassifying a healthy product as failed.
Validation
Rollback
Remove the single SCCACHE_GHA_ENABLED=false override from main rust_crate_tests and revert the Unix readiness signal to the prior behavior. No runner variables, Depot settings, runner groups, or organization settings are changed.
Summary by CodeRabbit
SCCACHE_GHA_ENABLED=false) apply only to the intended main crate-test shards and avoid duplicate configuration.