Fix E2E snapshot tests#626
Conversation
Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: yHSJ <josh@securitybot.info>
WalkthroughThis PR switches CI runners to ubuntu-22.04 across matrices and jobs, increases snapshot timeouts (10→15), replaces inline disk-reclaim steps with a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions
participant Runner as Runner VM
participant Script as scripts/cleanup-runner.sh
participant FS as File System / Package Manager
participant Docker as Docker Daemon
participant Swap as Swap Subsystem
Note over GH,Runner: CI invokes cleanup step on ubuntu-22.04 runner
GH->>Runner: checkout + chmod +x ./scripts/cleanup-runner.sh
Runner->>Script: execute cleanup-runner.sh
Script->>FS: probe disk (df) & detect arch / AGENT_TOOLSDIRECTORY
alt x86 & GitHub-hosted
Script->>FS: remove large package sets (distro-specific)
else other
Script->>FS: conservative removals
end
Script->>Docker: docker image prune --all --force
Docker-->>Script: reclaimed bytes
Script->>Swap: swapoff / remove swapfile (if present)
Swap-->>Script: confirmation
Script->>FS: remove unused runner tool/cache dirs (uses AGENT_TOOLSDIRECTORY)
FS-->>Script: final disk metrics
Script->>Runner: print saved space summary
Runner->>GH: workflow logs include summary
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
🔇 Additional comments (7)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @scripts/cleanup-runner.sh:
- Around line 42-46: The function formatByteCount uses numfmt with "${1}000"
which incorrectly assumes 1000 bytes per KB; change it to compute bytes using
binary KB (multiply the input by 1024) before passing to numfmt so --to=iec-i
(binary units) and the input unit system match; update the formatByteCount
function to perform shell arithmetic to multiply its argument by 1024 and feed
that value to numfmt (i.e., replace the current string-concatenation approach)
so saved-space calculations are accurate.
- Around line 145-156: The loop currently warns about missing paths in the
to_remove array but still passes the full to_remove array to sudo rm -rf, which
can fail under set -e; change the logic to build a filtered array (e.g.,
existing_to_remove) by iterating over "${to_remove[@]}" and appending only items
that pass [ -e "$element" ] to existing_to_remove, emit the warning for
non-existent items as you already do, then only call sudo rm -rf
"${existing_to_remove[@]}" (and skip the rm entirely if existing_to_remove is
empty) to avoid attempting to delete non-existent paths.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/continuous-integration.ymlscripts/cleanup-runner.sh
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: End-to-end snapshot tests (preview, 2, 680, true, 15, 10.5.3)
- GitHub Check: End-to-end snapshot tests (preprod, 1, 182, 15, 10.5.3)
- GitHub Check: Build aarch64/linux
- GitHub Check: Build riscv32
- GitHub Check: Build wasm32
- GitHub Check: Build x86_64/windows
- GitHub Check: Build aarch64/macos
- GitHub Check: Test the CLI
- GitHub Check: Build x86_64/linux
- GitHub Check: Benches
- GitHub Check: Test coverage
- GitHub Check: Analyze (rust)
🔇 Additional comments (13)
scripts/cleanup-runner.sh (11)
1-5: Nice error handling setup, mate!The
set -euo pipefailis spot on for catching issues early. Good to see proper attribution to the Rust repo too - standing on the shoulders of giants and all that.
7-15: Solid architecture check!Clean and simple detection logic. No worries here.
17-25: Good defensive coding with that parameter expansion!The
${RUNNER_ENVIRONMENT:-}pattern prevents errors if the var isn't set. Chef's kiss.
27-40: Clean utility functions!The separation line printer and space calculator are both doing their jobs nicely.
48-72: Nice defensive programming with the negative check!Handling the case where saved space goes negative with a warning is smart. Sometimes cleanup operations write temp files, so this is a good safety net.
74-85: Clean output formatting!Nice use of the separation lines for readable output. Makes the logs look proper.
158-167: Nice wrapper for measuring impact!Clean pattern for tracking how much space each operation frees up. No issues here.
169-208: Solid package cleanup with nice error handling!Love the DPkg lock timeout setting and the fallback error handling for autoremove and clean. The conditional logic for different runner types is well thought out too.
210-218: Docker cleanup looks good!Nice touch showing the images before pruning. The
|| trueis essential here since some runners might not have docker images at all.
220-225: Swap cleanup is straightforward!The
|| trueon both commands is smart - swap might not be configured on all runners.
227-245: Main execution flow is well structured!The sequence of operations makes sense, and the before/after reporting will be helpful for debugging. Nice work orchestrating all the cleanup steps.
.github/workflows/continuous-integration.yml (2)
193-193: Timeout bump looks reasonable!Increasing from 10 to 15 minutes gives the E2E tests more breathing room. The
fromJSONpattern also lets you override it manually via workflow dispatch if needed - that's handy for debugging when things are running slow.Also applies to: 198-198
250-252: Script invocation pattern is correct!Chmod then execute is the way to go. Clean replacement of the inline commands.
However, keep in mind the critical issue I flagged in the cleanup script itself - the
rmcommand will bail when it hits non-existent files. You'll want to fix that before this workflow can run successfully.
Codecov Report✅ All modified and coverable lines are covered by tests. 🚀 New features to boost your workflow:
|
Signed-off-by: yHSJ <josh@securitybot.info>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/continuous-integration.yml (1)
178-180: Mate, you've got a commented-out condition here – clean it up or bring it back from the dead!Right, so you've left this conditional commented out like a deleted scene that didn't make the director's cut. If you're dead set on always using the buildjet runner now, just yeet that commented line entirely. But if there's a chance you'll need to gate this again later, might be worth adding a proper explanation comment about why it's temporarily disabled.
Just keep in mind that running buildjet for every PR might rack up the dollarydoos compared to the standard ubuntu-latest runner.
🔎 Proposed cleanup
If the condition is truly obsolete, remove it entirely:
steps: - id: alt-runner - # if: (github.event_name == 'push' || github.event_name == 'workflow_dispatch') && github.ref == 'refs/heads/main' run: echo "runner=buildjet-4vcpu-ubuntu-2204" >> $GITHUB_OUTPUTOr if you're planning to re-enable it, add context:
- id: alt-runner + # Temporarily disabled to ensure E2E tests complete successfully (see PR #626) + # TODO: Re-enable once disk space issues are fully resolved # if: (github.event_name == 'push' || github.event_name == 'workflow_dispatch') && github.ref == 'refs/heads/main' run: echo "runner=buildjet-4vcpu-ubuntu-2204" >> $GITHUB_OUTPUT
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/continuous-integration.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: End-to-end snapshot tests (preview, 2, 680, true, 15, 10.5.3)
- GitHub Check: End-to-end snapshot tests (preprod, 1, 182, 15, 10.5.3)
- GitHub Check: Build aarch64/macos
- GitHub Check: Build x86_64/linux
- GitHub Check: Test the CLI
- GitHub Check: Build wasm32
- GitHub Check: Build riscv32
- GitHub Check: Benches
- GitHub Check: Build aarch64/linux
- GitHub Check: Test coverage
- GitHub Check: Build x86_64/windows
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
.github/workflows/continuous-integration.yml (2)
192-192: Noice! Extra breathing room for those snapshot tests.Bumping the timeout from 10 to 15 minutes is a solid move, especially if the tests were timing out before. The
fromJSONhandling looks spot on, and you've kept the flexibility to override it via workflow_dispatch. This should give your E2E tests enough time to do their thing without rushing like a Mad Max chase scene.Also applies to: 197-197
248-251: Script's locked and loaded—no issues here!Spot on moving the disk cleanup to a dedicated script. The verification shows it's all shipshape and Bristol fashion:
✓ Script exists at
scripts/cleanup-runner.sh
✓ It's a solid cleanup utility with proper error handling and GitHub Runner detection
✓ Thechmod +xis actually necessary, not redundant – the script doesn't have execute permissions in git (mode 100644), so the workflow needs to set them on the flyThe script itself is impressively thorough – it handles unused packages, Docker images, swap storage, and unused files with good defensive programming (checking for GitHub Runner environment, architecture detection, etc.). Exactly the kind of thing that keeps CI workflows running smooth like butter.
Ready to ship! 🚀
Signed-off-by: yHSJ <josh@securitybot.info>
| key: cargo-${{ matrix.environments.target }}-${{ matrix.environments.runner }}-${{ steps.timestamp.outputs.value }} | ||
| restore-keys: | | ||
| cargo-${{ matrix.environments.target }} | ||
| cargo-${{ matrix.environments.target }}-${{ matrix.environments.runner }} | ||
| # Only restore cache for other types of events (e.g. Pull requests). | ||
| - uses: actions/cache/restore@v4 | ||
| if: github.event_name != 'push' | ||
| with: | ||
| path: ${{ env.RUST_CACHE_PATH }} | ||
| key: cargo-${{ matrix.environments.target }} | ||
| key: cargo-${{ matrix.environments.target }}-${{ matrix.environments.runner }} | ||
| restore-keys: | | ||
| cargo-${{ matrix.environments.target }} | ||
| cargo-${{ matrix.environments.target }}-${{ matrix.environments.runner }} |
There was a problem hiding this comment.
Why change the cache key? This will never succeed in fetching the right cache 🤨
There was a problem hiding this comment.
Because I believed that it was building things with a different ubuntu version than the tests are running, so there were issues in linking. Figured I could cache it by runner, but obviously that was very wrong lol
Signed-off-by: yHSJ <josh@securitybot.info>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @.github/workflows/continuous-integration.yml:
- Around line 179-180: The commented-out gating condition on the workflow (the
commented "if: (github.event_name == 'push' || github.event_name ==
'workflow_dispatch') && github.ref == 'refs/heads/main'") removes the intended
restriction and causes the run step that sets runner=buildjet-4vcpu-ubuntu-2404
to execute for all non-draft PRs and branch pushes; restore the original
conditional by uncommenting or re-adding that "if:" check (or explicitly add the
intended branch/event guard) so the runner assignment only occurs for main
branch pushes or whatever scoped event was intended, ensuring the "run: echo
\"runner=buildjet-4vcpu-ubuntu-2404\" >> $GITHUB_OUTPUT" line is only executed
under that condition.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/continuous-integration.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Build x86_64/windows
- GitHub Check: Build riscv32
- GitHub Check: Build aarch64/macos
- GitHub Check: Build x86_64/linux
- GitHub Check: Benches
- GitHub Check: Build wasm32
- GitHub Check: Build aarch64/linux
- GitHub Check: Test coverage
- GitHub Check: Test the CLI
- GitHub Check: Analyze (rust)
🔇 Additional comments (2)
.github/workflows/continuous-integration.yml (2)
192-192: Crikey, giving the tests some breathing room—nice one!Bumping the timeouts from 10 to 15 minutes for both preprod and preview networks is a solid move to prevent those frustrating timeout failures. The manual override via
workflow_dispatchinputs is still there if you need to dial it up further during debugging.This aligns perfectly with your PR objectives to let the E2E snapshots run to completion. Good stuff!
Also applies to: 197-197
250-251: This cleanup script is a ripper—solid implementation, mate!The
./scripts/cleanup-runner.shexists and it's a beaut. The script's got all the defensive moves locked down:set -euo pipefailfor robustness, proper GitHub runner detection, architecture sniffing, and space measurements before and after the cleanup operations. It methodically removes unused packages, Docker images, swap storage, and orphaned files while checking whether things actually exist before yelling about 'em. Thechmod +xon line 250 is spot-on necessary too—without it, the script wouldn't run because the file doesn't have execute permissions by default.This approach beats inline commands like a speedrunner beats a TAS—cleaner, reusable, and battle-tested against the Rust project's own runner constraints. Proper bit of work here.
Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: yHSJ <josh@securitybot.info>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/continuous-integration.yml (1)
250-251: Script-based cleanup is the way—but let's tidy up the permissions dance.Pulling the disk reclamation logic into a dedicated script is a solid move, especially if you're borrowing battle-tested cleanup code from the Rust repo. Keeps the workflow lean, makes the logic reusable, and you can iterate on the script without touching the YAML every time. Total level-up from inline commands sprawling across multiple lines.
One tiny nitpick though: you're doing the
chmod +xtwo-step here in the workflow, which means the script isn't committed as executable in the repo. That's not breaking anything, but it's a bit like putting on your shoes before your socks—technically works, but there's a cleaner sequence.🔎 Streamline by committing script as executable
Instead of:
- name: Reclaim Disk Space run: | chmod +x ./scripts/cleanup-runner.sh ./scripts/cleanup-runner.shCommit the script with the executable bit set, then simplify to:
- name: Reclaim Disk Space run: ./scripts/cleanup-runner.shTo set executable on commit:
git update-index --chmod=+x scripts/cleanup-runner.sh git commit -m "chore: make cleanup-runner.sh executable"Or just:
chmod +x scripts/cleanup-runner.sh git add scripts/cleanup-runner.sh git commit -m "chore: make cleanup-runner.sh executable"On Unix-like systems, Git tracks the executable bit by default, so future checkouts will have it ready to roll.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/continuous-integration.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: End-to-end snapshot tests (preview, 2, 680, true, 15, 10.5.3)
- GitHub Check: End-to-end snapshot tests (preprod, 1, 182, 15, 10.5.3)
- GitHub Check: clippy
- GitHub Check: Build x86_64/linux
- GitHub Check: Build x86_64/windows
- GitHub Check: Build aarch64/linux
- GitHub Check: Build riscv32
- GitHub Check: Build aarch64/macos
- GitHub Check: Build wasm32
- GitHub Check: Test coverage
- GitHub Check: Benches
- GitHub Check: Test the CLI
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
.github/workflows/continuous-integration.yml (1)
192-192: Timeout bump looks fair dinkum, but where'd the magic number come from?Boosting the timeout from 10 to 15 minutes aligns with your mission to stop these E2E tests from choking mid-run. The 50% increase gives your snapshots breathing room, and you're keeping the override escape hatch via
workflow_dispatchintact—nice safety net there, mate.That said, did you pull 15 minutes from actual test runs that were cutting it close, or is this more of a "let's throw an extra 5 at it and see what sticks" vibe? If you've got logs showing tests consistently hitting 12-13 minutes, then 15 is bang on. If not, you might either be too stingy (still timing out) or too generous (masking performance regressions that should be surfaced).
Worth a quick sanity check: do a test run on both preprod and preview to confirm 15 minutes covers your slowest legitimate runs with a bit of buffer. You don't want to be back here next week bumping it to 20 because the safety margin was too tight, yeah?
Also applies to: 197-197
Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: yHSJ <josh@securitybot.info>
…reviously used Signed-off-by: yHSJ <josh@securitybot.info>
Signed-off-by: yHSJ <josh@securitybot.info>
2 participants
This PR updates the CI workflow to allow the tests to run to completion (increasing timeout and reclaiming more disk space via a script from the Rust repository)
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.