fix(ci): bump Windows test timeout to 35m and fix dead security::secrets filter#2769
Conversation
…ilter The `rust-core-tests-windows` job was failing the production release pretest with "job has exceeded the maximum execution time of 20m0s". Cold-cache Windows compilation of the full openhuman crate takes ~19 minutes (only 12.75% sccache hit rate on the failed run), leaving no room for the post-job cache-save step within the 20-minute budget. Raise timeout to 35 minutes to survive cold-cache builds. Also fix the test filter: `-- security::secrets` matched zero tests because `security/secrets.rs` is a one-line re-export. The actual Windows ACL test module is `keyring::encrypted_store::tests`; change the filter to `keyring::encrypted_store` so the suite actually runs.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates the Windows-specific Rust core test job in the reusable CI workflow: it increases the job timeout from 20 to 35 minutes and changes the cargo test filter to run ChangesWindows CI Test Job Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.github/workflows/test-reusable.yml:
- Around line 153-159: The existing comment incorrectly references
security/secrets.rs and a non-existent test named repair_windows_acl; update it
to say that the Windows ACL tests live in
src/openhuman/keyring/encrypted_store_tests.rs and are exposed via
keyring::encrypted_store (so cargo test -- keyring::encrypted_store targets
them), remove the misleading reference to security/secrets.rs, and replace the
repair_windows_acl mention with the actual test helpers and test names
(repair_windows_acl is a helper function, while the tests are self_repair_*,
is_permission_error_*, and qualify_windows_username_*) so the comment accurately
documents the target suite referenced by the run command.
🪄 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
Run ID: d3600292-935a-4a5d-b866-a2a8011130aa
📒 Files selected for processing (1)
.github/workflows/test-reusable.yml
…ir_windows_acl repair_windows_acl is a helper function, not a test. The actual #[cfg(windows)] tests are self_repair_*, is_permission_error_*, and qualify_windows_username_*. Also clarify that security/secrets.rs is a re-export with no tests of its own (per CodeRabbit review).
Resolves conflict in .github/workflows/test-reusable.yml: took upstream's version from tinyhumansai#2769 which is a superset of what this branch had: - timeout-minutes: 35 (vs my 30) — more headroom. - Test filter fixed: cargo test -- keyring::encrypted_store (vs the dead '-- security::secrets' filter that was matching nothing because security/secrets.rs is a one-line re-export with no tests of its own). My earlier 'drop sccache' change is discarded; if the os error 10054 flake re-appears we'll fix forward, but starting from main's baseline keeps the diff minimal.
Resolves conflict in .github/workflows/test-reusable.yml — took upstream's version from tinyhumansai#2769 (35m timeout + the keyring::encrypted_store filter fix), dropping my earlier 'bump to 30 + drop sccache' commit since tinyhumansai#2769 is the better fix (the old security::secrets filter was matching nothing — TIL).
Summary
timeout-minutesfor therust-core-tests-windowsCI job from 20 → 35 minutes.-- security::secrets(matched zero tests) to-- keyring::encrypted_store(the actual module path for the Windows ACL suite).Problem
main Release Production, run 26514109166) failed on thePretest — unit + rust / Rust Core Tests (Windows — secrets ACL)job with: "The job has exceeded the maximum execution time of 20m0s".openhumancrate takes ~19 minutes (only 12.75% sccache hit rate on the failing run), leaving no time for the post-job cache-save step within the 20-minute budget — the runner was killed mid-cleanup.-- security::secretshas always matched zero tests:src/openhuman/security/secrets.rsis a one-line re-export (pub use crate::openhuman::keyring::encrypted_store::*), not a module with its own tests. The actual Windows ACL test module (repair_windows_acl,is_permission_error,icaclsgrant args, self-repair path) lives atopenhuman::keyring::encrypted_store::tests.Solution
-- keyring::encrypted_storewhich correctly matchesopenhuman::keyring::encrypted_store::tests::*, including all#[cfg(windows)]tests described in the step comment.Submission Checklist
.github/filesImpact
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/production-build-timeoutValidation Run
pnpm --filter openhuman-app format:checkValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
keyring::encrypted_storetests instead of 0 tests, and has 35 min to complete rather than 20.Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit
Release Notes
Note: This release contains internal infrastructure improvements with no user-facing changes.