fix(exec): ignore OS workqueue threads in pre-fork thread check - #1424
Merged
Conversation
Contributor
PR Review SummarySize
Affected crates
Blast radius — ContainedThis PR touches: source code Updated automatically on each push to this PR. |
kipz
added a commit
to kipz/nono
that referenced
this pull request
Jul 16, 2026
kipz
marked this pull request as ready for review
July 16, 2026 09:29
Signed-off-by: James Carnegie <me@kipz.org>
kipz
force-pushed
the
kipz/macos-fork-workqueue-threads
branch
from
July 16, 2026 10:35
9169af4 to
f877064
Compare
SequeI
approved these changes
Jul 16, 2026
SequeI
left a comment
Member
There was a problem hiding this comment.
LGTM, this makes sense to me, no big loss in raising the thread limit slightly. Thanks!
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked Issue
Closes #1423
Summary
On macOS the
Supervisedpre-fork thread-count check counted OS-managed libdispatch/GCD workqueue threads thatSecurity.frameworkspawns forSecTrustSettings*XPC during proxy CA setup. Their count scales with load, so under concurrency the total intermittently exceededMAX_CRYPTO_THREADS(7) and the fork was refused (Cannot fork: process has 8 threads (max 7 with crypto pool)). Those OS threads are parked and fork-safe (covered bylibmalloc'spthread_atforkhandlers).An earlier version of this PR counted only nono's own named threads to exclude the unnamed OS workqueue threads. Review caught two problems with that approach: unnamed threads spawned by nono itself (e.g. via bare
std::thread::spawn) would silently bypass the check, and the new mach query leaked a port per thread. Rather than fix both by naming every thread in the codebase, this PR now just raisesMAX_CRYPTO_THREADS(7 -> 12) to absorb the observed workqueue variance, keeping the original total-thread-count check unchanged.Agent Disclosure (if applicable)
This PR was generated by an AI coding agent (Claude) on behalf of the contributor. Files consulted:
crates/nono-cli/src/exec_strategy.rs(theSupervisedfork-safety check andget_thread_count). No code was adapted from third-party sources. Complies with repository requirements: nounwrap/expect,NonoErrorused for error propagation, DCO sign-off present.Test Plan
make ciclean: clippy-D warnings -D clippy::unwrap_used, fmt-check, full suite passing.Checklist
CHANGELOG.mdif neededAgent Compliance Check (Required for AI/Automated PRs)