nimmo/forgejo-auto-pr-review
Watch
1
0
Fork
You've already forked forgejo-auto-pr-review
 0
Code Issues 5 Pull requests Projects Releases Packages 1 Activity Actions

fix: raise shared-runner process headroom #25

Merged
nimmo merged 2 commits from fix/shared-runner-nproc into main 2026-07-26 10:24:16 +01:00
Conversation 3 Commits 2 Files changed 3 +24 -2
nimmo commented 2026-07-26 09:54:19 +01:00
Owner
Copy link

RLIMIT_NPROC counts every thread owned by the runner UID, including workloads outside a job's visible process namespace. The default of 64 prevents thread-based runtimes such as Node from starting on the shared Nova runner. Raise the finite default to 4096, document the shared-runner behaviour, and cover the default with a unit test.\n\nValidated with python3 -m unittest discover -s tests and nix flake check.

RLIMIT_NPROC counts every thread owned by the runner UID, including workloads outside a job's visible process namespace. The default of 64 prevents thread-based runtimes such as Node from starting on the shared Nova runner. Raise the finite default to 4096, document the shared-runner behaviour, and cover the default with a unit test.\n\nValidated with python3 -m unittest discover -s tests and nix flake check.
fix: raise shared-runner process headroom
All checks were successful
Build and publish container image / prepare-tags (push) Successful in 1s
Details
Build and publish container image / build-and-push (push) Successful in 17s
Details
AI pull-request review / review (pull_request) Successful in 4m14s
Details
7f0fc75f06
nimmo commented 2026-07-26 09:58:37 +01:00
Author
Owner
Copy link

Automated code review

Reviewed commit: 14ad1d98d0dd7c0a03c75dc1e333b1c52bdde96d

Verdict: Ready to merge

No blocking or important findings were retained, and deterministic checks passed. Suggestions remain advisory.

Overall assessment

The pull request increases the default process limit from 64 to 4096 in configuration, source code, and documentation, while adding unit tests to verify both the new default and explicit overrides. The change includes appropriate caveats regarding RLIMIT_NPROC scope on shared runners.

Blocking findings

None.

Important findings

None.

Suggestions

None.

Tests and validation

  • unit-tests: success

Questions

  • What specific hardware specifications or workload profiles on the target shared runners justify increasing the default from 64 to 4096?

Review limitations

  • Review is based solely on the supplied diff and test output. Runtime validation under concurrent multi-job execution, interaction with system-wide nproc limits, and verification of the 4096 threshold against actual runner resource quotas were not supplied.

Diff coverage

  • README.md: reviewed — included in a context-limited batch
  • reviewer.py: reviewed — included in a context-limited batch
  • tests/test_reviewer.py: reviewed — included in a context-limited batch
Review metadata
  • Reviewer: forgejo-ai-review v1
  • Model: qwen3.6:35b-a3b-q8_0
  • Guidance loaded: .forgejo/ai-review.yml, README.md
  • Commits considered: 2
  • Previous automated review: updated
  • Findings: 0
<!-- forgejo-ai-review:v1 --> ## Automated code review Reviewed commit: `14ad1d98d0dd7c0a03c75dc1e333b1c52bdde96d` ## Verdict: Ready to merge No blocking or important findings were retained, and deterministic checks passed. Suggestions remain advisory. ### Overall assessment The pull request increases the default process limit from 64 to 4096 in configuration, source code, and documentation, while adding unit tests to verify both the new default and explicit overrides. The change includes appropriate caveats regarding RLIMIT_NPROC scope on shared runners. ### Blocking findings None. ### Important findings None. ### Suggestions None. ### Tests and validation - unit-tests: **success** ### Questions - What specific hardware specifications or workload profiles on the target shared runners justify increasing the default from 64 to 4096? ### Review limitations - Review is based solely on the supplied diff and test output. Runtime validation under concurrent multi-job execution, interaction with system-wide nproc limits, and verification of the 4096 threshold against actual runner resource quotas were not supplied. ### Diff coverage - `README.md`: **reviewed** — included in a context-limited batch - `reviewer.py`: **reviewed** — included in a context-limited batch - `tests/test_reviewer.py`: **reviewed** — included in a context-limited batch <details><summary>Review metadata</summary> - Reviewer: forgejo-ai-review v1 - Model: qwen3.6:35b-a3b-q8_0 - Guidance loaded: .forgejo/ai-review.yml, README.md - Commits considered: 2 - Previous automated review: updated - Findings: 0 </details>
test: cover process limit override
All checks were successful
Build and publish container image / build-and-push (push) Successful in 13s
Details
Build and publish container image / prepare-tags (push) Successful in 0s
Details
AI pull-request review / review (pull_request) Successful in 2m6s
Details
14ad1d98d0
nimmo commented 2026-07-26 09:59:49 +01:00
Author
Owner
Copy link

Implemented the valid suggestion in commit 14ad1d9: command_preexec now has explicit test coverage confirming a configured max_processes value overrides the shared-runner default. All 44 unit tests and nix flake check pass. The README intentionally leaves isolated-runner values to operator discretion because appropriate per-UID headroom depends on runner topology and concurrent workloads.

Implemented the valid suggestion in commit 14ad1d9: command_preexec now has explicit test coverage confirming a configured max_processes value overrides the shared-runner default. All 44 unit tests and nix flake check pass. The README intentionally leaves isolated-runner values to operator discretion because appropriate per-UID headroom depends on runner topology and concurrent workloads.
nimmo commented 2026-07-26 10:02:42 +01:00
Author
Owner
Copy link

Final readiness update: the refreshed automated review covers 14ad1d9, reports no findings, and its deterministic unit tests pass. The 4096 default is based on direct shared-runner reproduction: Node startup failed with RLIMIT_NPROC values from 64 through 1024 and succeeded at 2048; 4096 provides one doubling of headroom for concurrent runner-UID workloads while retaining a finite limit. All 44 reviewer unit tests and nix flake check pass. PR #25 is ready to merge.

Final readiness update: the refreshed automated review covers 14ad1d9, reports no findings, and its deterministic unit tests pass. The 4096 default is based on direct shared-runner reproduction: Node startup failed with RLIMIT_NPROC values from 64 through 1024 and succeeded at 2048; 4096 provides one doubling of headroom for concurrent runner-UID workloads while retaining a finite limit. All 44 reviewer unit tests and nix flake check pass. PR #25 is ready to merge.
nimmo merged commit 83cc8cbd1d into main 2026-07-26 10:24:16 +01:00
nimmo deleted branch fix/shared-runner-nproc 2026-07-26 10:24:16 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
nimmo/forgejo-auto-pr-review!25
No description provided.