ci: run Check on the self-hosted A1 runner (trusted events only)#1775
Conversation
Trusted events (pushes, same-repo PRs) run on the self-hosted Ampere A1 runner with a warm local /nix/store. Fork PRs keep running on GitHub-hosted ARM, so untrusted code never executes on the box. On self-hosted the Nix-setup actions (installer, magic-nix-cache, flake-checker, free-disk-space) are skipped since the box already provides them, and CARGO_TARGET_DIR is pointed outside the workspace so Rust builds stay warm across runs. The run step uses a login shell so Nix is on PATH. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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)
WalkthroughThe GitHub Actions 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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
🧹 Nitpick comments (1)
.github/workflows/check.yml (1)
62-63: 💤 Low valueConsider explicitly sourcing Nix rather than relying on login shell behavior.
The login shell (
-lflag) sources user profile files (~/.bash_profile, etc.) and system profiles (/etc/profile.d/*) to put Nix onPATH. Combined with-e, any non-zero exit in those profile files will fail the step. Profile files can have environment-specific assumptions that break in CI contexts (e.g., interactive checks, missing commands).A more explicit approach would be to conditionally source the Nix profile script only on self-hosted runners, keeping the standard Actions shell behavior on GitHub-hosted.
♻️ Alternative: explicit Nix PATH setup
- - run: nix develop --command just ci - shell: bash -leo pipefail {0} + - name: Run checks + run: | + if [[ "${RUNNER_ENVIRONMENT}" == "self-hosted" ]] && [[ -f /nix/var/nix/profiles/default/etc/profile.d/nix-daemon.sh ]]; then + source /nix/var/nix/profiles/default/etc/profile.d/nix-daemon.sh + fi + nix develop --command just ciThis keeps the default
bash -eo pipefailshell and explicitly sources Nix only where needed, reducing the risk of profile-related failures.🤖 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 @.github/workflows/check.yml around lines 62 - 63, The run step executing `nix develop --command just ci` uses a login shell with the `-l` flag in the shell configuration, which sources profile files with environment-specific assumptions that can break in CI. Remove the `-l` flag from the shell specification (change from `bash -leo pipefail {0}` to `bash -eo pipefail {0}`) to use the standard Actions shell behavior. If explicit Nix path setup is needed on certain runners, add explicit sourcing of the Nix profile script within the run command or use conditional logic to source Nix only where necessary (e.g., for self-hosted runners).
🤖 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/check.yml:
- Line 21: The `runs-on` line uses a bare `'self-hosted'` label which is too
generic and could match any self-hosted runner in the repository or
organization. Replace the `'self-hosted'` string with a specific array of labels
that uniquely identify the target Ampere A1 runner, such as an array containing
`'self-hosted'` along with additional identifying labels like `'linux'`,
`'arm64'`, or `'moq-dev'`. Adjust the specific labels to match the actual labels
configured on your self-hosted runner to ensure jobs execute on the intended
infrastructure.
- Around line 56-57: The concurrency setting (lines 11-13) groups by github.ref,
allowing different PRs to execute simultaneously on the self-hosted runner, but
all jobs share the same CARGO_TARGET_DIR at $HOME/cargo-target/moq, causing race
conditions in Cargo compilation. To fix this, modify the concurrency group to
use a global identifier (like "check" or "check-${{ github.workflow }}") instead
of github.ref to ensure only one Check job runs at a time across all PRs,
eliminating the shared directory conflict. Alternatively, if
infrastructure-level constraints already prevent concurrent execution, update
the CARGO_TARGET_DIR assignment to use a PR-specific path (such as
$HOME/cargo-target/moq-${{ github.run_id }}) and add a comment documenting the
serialization assumption.
---
Nitpick comments:
In @.github/workflows/check.yml:
- Around line 62-63: The run step executing `nix develop --command just ci` uses
a login shell with the `-l` flag in the shell configuration, which sources
profile files with environment-specific assumptions that can break in CI. Remove
the `-l` flag from the shell specification (change from `bash -leo pipefail {0}`
to `bash -eo pipefail {0}`) to use the standard Actions shell behavior. If
explicit Nix path setup is needed on certain runners, add explicit sourcing of
the Nix profile script within the run command or use conditional logic to source
Nix only where necessary (e.g., for self-hosted runners).
🪄 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: aaa23d18-9e6e-4d13-81ba-23aad0ab8b0d
📒 Files selected for processing (1)
.github/workflows/check.yml
| - if: runner.environment == 'self-hosted' | ||
| run: echo "CARGO_TARGET_DIR=$HOME/cargo-target/moq" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
Race condition: concurrent PRs share the same CARGO_TARGET_DIR on the self-hosted runner.
The concurrency setting (lines 11-13) groups by github.ref, which is unique per PR (refs/pull/123/merge). This allows concurrent execution of checks for different PRs on the same self-hosted runner. Both jobs will simultaneously read/write $HOME/cargo-target/moq, risking:
- Cargo incremental compilation corruption
- Fingerprint/lock file conflicts
- Cache invalidation and rebuilds
The PR objectives state "serialization of concurrent PRs" but the current configuration does not enforce global serialization.
🔧 Proposed fixes
Option 1 (Recommended): Serialize all Check jobs globally
concurrency:
- group: check-${{ github.ref }}
+ group: check-moq-self-hosted
cancel-in-progress: trueThis ensures only one Check job runs at a time across all PRs, eliminating the race condition. Tradeoff: PRs queue sequentially.
Option 2: Use PR-specific target directories
- if: runner.environment == 'self-hosted'
- run: echo "CARGO_TARGET_DIR=$HOME/cargo-target/moq" >> "$GITHUB_ENV"
+ run: echo "CARGO_TARGET_DIR=$HOME/cargo-target/moq-pr-${{ github.event.pull_request.number }}" >> "$GITHUB_ENV"Allows concurrent builds but loses cross-PR cache reuse. May require periodic cleanup of old PR directories.
Option 3: Document the infrastructure-level serialization
If the self-hosted runner's job queue configuration already prevents concurrent job execution (e.g., max runners = 1), document this assumption in a comment and accept that the concurrency setting only handles cancellation within a PR.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - if: runner.environment == 'self-hosted' | |
| run: echo "CARGO_TARGET_DIR=$HOME/cargo-target/moq" >> "$GITHUB_ENV" | |
| - if: runner.environment == 'self-hosted' | |
| run: echo "CARGO_TARGET_DIR=$HOME/cargo-target/moq-pr-${{ github.event.pull_request.number }}" >> "$GITHUB_ENV" |
🤖 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 @.github/workflows/check.yml around lines 56 - 57, The concurrency setting
(lines 11-13) groups by github.ref, allowing different PRs to execute
simultaneously on the self-hosted runner, but all jobs share the same
CARGO_TARGET_DIR at $HOME/cargo-target/moq, causing race conditions in Cargo
compilation. To fix this, modify the concurrency group to use a global
identifier (like "check" or "check-${{ github.workflow }}") instead of
github.ref to ensure only one Check job runs at a time across all PRs,
eliminating the shared directory conflict. Alternatively, if
infrastructure-level constraints already prevent concurrent execution, update
the CARGO_TARGET_DIR assignment to use a PR-specific path (such as
$HOME/cargo-target/moq-${{ github.run_id }}) and add a comment documenting the
serialization assumption.
Address review feedback: - runs-on now requires both self-hosted and nix labels (was a bare 'self-hosted' that could match any org runner), matching moq-pro. - CARGO_TARGET_DIR is scoped per runner ($RUNNER_NAME) so a second runner service on the same box gets its own target dir instead of racing on a shared path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Routes the
Checkworkflow to the new moq-dev self-hosted Ampere A1 runner (4 OCPU / 24 GB, warm local/nix/store) for trusted events, while fork PRs stay on GitHub-hosted ARM so untrusted code never touches the box.How it works
runs-on: ${{ github.event.pull_request.head.repo.fork && 'ubuntu-24.04-arm' || 'self-hosted' }}— the fork check is the security boundary; a popped fork PR can't compromise the box because it never runs there.free-disk-space/nix-installer/magic-nix-cache/flake-checker/Swatinem/rust-cachesteps are gated torunner.environment == 'github-hosted'(the box already has disk, Nix, and a warm store).CARGO_TARGET_DIRis moved to$HOME/cargo-target/moqon the self-hosted path so Rust builds stay warm —actions/checkoutclean and the runner's job cleanup wipe the workspace, but not$HOME.nix develop --command just cistep runs under a login shell so Nix lands on PATH (Actions shells don't source/etc/profile.d).Tradeoffs / notes
runs-on: self-hostedon a bare forkpull_requestwithout this gate, and nopull_request_targetrunning fork code on self-hosted.This PR's own
Checkrun executes on the self-hosted runner (non-fork), so it doubles as the end-to-end test.(Written by Claude)