From e8162a0de64cafdb22a162e2213c8186ff666eb6 Mon Sep 17 00:00:00 2001 From: din Date: Sat, 19 Sep 2026 01:45:32 +0000 Subject: [PATCH 1/2] test(helper,docs): the parallel worker default follows the machine, not a flat 4 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `test_helper` forked `NCPU=4` workers on every machine while its own comment justified that number as "deliberately below the core count". On a 4-core machine those are the same number, which is the oversubscription the same paragraph warns produces wall-clock failures rather than assertion ones — and two machines that run this suite are 4-core: the developerz.ai fleet boxes that run `bin/check` as a merge gate, and CI's `ubuntu-latest` fallback runner. The default is now `(Etc.nprocessors / 2).clamp(1, 4)`: 2 on a 4-core box, 4 on an 8-core one, explicit `NCPU=` untouched. Measured on 4 pinned cores at main, halving the workers costs 9% of the gate (5m24s against 4m57s), because this suite waits far more than it computes. CLAUDE.md's CI standard moves with it — left alone it would document a default the code no longer has. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 2 +- test/test_helper.rb | 24 +++++++++++++++++++----- test/unit/ncpu_default_test.rb | 26 ++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 6 deletions(-) create mode 100644 test/unit/ncpu_default_test.rb diff --git a/CLAUDE.md b/CLAUDE.md index d247cd9..beb9d78 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -97,7 +97,7 @@ Skip step 3 → leaked sockets in children. Skip step 5 → children corrupt eac - **Never mock Redis** in integration or parity tests. Real Redis, unique namespace. - **Coverage gate.** SimpleCov **line** and **branch** coverage on `lib/` must both stay ≥90% (blocking; `minimum_coverage line: 90, branch: 90`). Branch was ratcheted from ~78% to ≥90% in #67 — keep new code at parity. The Cobertura report is still uploaded for per-file inspection. Coverage runs merge across the `minitest-parallel_fork` workers via `SimpleCov.at_fork`. - **CI** on GitHub Actions. Benchmark bot comments deltas on PRs that touch Ruby or bench inputs (`bench.yml`'s path filter: `lib/**`, `exe/**`, `bench/**`, `bin/bench-compare`, `Rakefile`, `Gemfile`, `*.gemspec`, the workflow itself); >5% regression flags it. -- **CI standard.** Cheap by construction: **one** full Ruby suite run per PR, on the newest Ruby + newest Rails, with the coverage gate folded into it (`COVERAGE=1` on the same invocation). No version matrix and no second coverage job — the gemspec's `>= 3.2` floor is held by rubocop's `TargetRubyVersion: 3.2`. Suite workers stay at `NCPU=4` (test_helper's default, deliberately below core count — the integration layer boots real swarms, so one worker per core oversubscribes and produces wall-clock failures, measured), which is why the suite runner wants roughly a 4 vCPU box. Runner selection is a **repository variable**, not a hard-coded label: `vars.WURK_CI_RUNNER` for detect/suite/parity/lint/frontend and ecosystem, `vars.WURK_BENCH_RUNNER` for bench. Both fall through to `ubuntu-latest` when unset, and a fork PR is pinned to `ubuntu-latest` unconditionally — wurk is public, a fork PR runs attacker-authored code, and that must never reach self-hosted hardware. Pointing CI at different hardware, or rolling back, is therefore a settings change rather than a PR. `release.yml` and both dependabot workflows stay on `ubuntu-latest` outright, keeping the RubyGems publish credential on a VM that is destroyed after the job. **Bench no longer sits on a fixed 8vcpu SKU** — `rake bench` gates merge at >5% (pillar 3), so its variance band is what to watch when that variable is pointed at new hardware; developerz-ai/infrastructure#1259 tracks the measurement and the one-variable rollback. Note that no static check catches a typo'd runner variable: an unknown name resolves to empty and yields green CI identical to the intended fallback, so verify the expression sites by grep, not by a green run. Every workflow declares a `concurrency` group with cancel-in-progress, and every job sets `timeout-minutes`. An `actionlint` job in test.yml runs workflow lint on every PR as a quick standalone check alongside `spec-docs` (the binary is pinned to v1.7.12 by SHA and the job is gated per-step on a `workflows` paths-filter so it always reports a status); it is not yet wired into the ruleset's required checks. Deploy/publish workflows (deploy-demo, pages, release) never auto-cancel: `cancel-in-progress: false`. +- **CI standard.** Cheap by construction: **one** full Ruby suite run per PR, on the newest Ruby + newest Rails, with the coverage gate folded into it (`COVERAGE=1` on the same invocation). No version matrix and no second coverage job — the gemspec's `>= 3.2` floor is held by rubocop's `TargetRubyVersion: 3.2`. Suite workers follow the MACHINE: `test_helper`'s default is half the cores, floored at 1 and capped at 4 (`Wurk::Test::DEFAULT_NCPU`), because the integration layer boots real swarms and one worker per core oversubscribes into wall-clock failures rather than honest assertion ones. The flat `NCPU=4` this replaces WAS one worker per core on a 4 vCPU runner — the shape it existed to avoid. On a 4 vCPU box the default now resolves to 2, measured at 5m24s against 4m57s at 4 (2026-09-19, `taskset -c 0-3 ./bin/check`): 9% of the gate, because this suite waits far more than it computes. `NCPU=` still overrides for a machine with headroom, so the suite runner wants roughly a 4 vCPU box as before. Runner selection is a **repository variable**, not a hard-coded label: `vars.WURK_CI_RUNNER` for detect/suite/parity/lint/frontend and ecosystem, `vars.WURK_BENCH_RUNNER` for bench. Both fall through to `ubuntu-latest` when unset, and a fork PR is pinned to `ubuntu-latest` unconditionally — wurk is public, a fork PR runs attacker-authored code, and that must never reach self-hosted hardware. Pointing CI at different hardware, or rolling back, is therefore a settings change rather than a PR. `release.yml` and both dependabot workflows stay on `ubuntu-latest` outright, keeping the RubyGems publish credential on a VM that is destroyed after the job. **Bench no longer sits on a fixed 8vcpu SKU** — `rake bench` gates merge at >5% (pillar 3), so its variance band is what to watch when that variable is pointed at new hardware; developerz-ai/infrastructure#1259 tracks the measurement and the one-variable rollback. Note that no static check catches a typo'd runner variable: an unknown name resolves to empty and yields green CI identical to the intended fallback, so verify the expression sites by grep, not by a green run. Every workflow declares a `concurrency` group with cancel-in-progress, and every job sets `timeout-minutes`. An `actionlint` job in test.yml runs workflow lint on every PR as a quick standalone check alongside `spec-docs` (the binary is pinned to v1.7.12 by SHA and the job is gated per-step on a `workflows` paths-filter so it always reports a status); it is not yet wired into the ruleset's required checks. Deploy/publish workflows (deploy-demo, pages, release) never auto-cancel: `cancel-in-progress: false`. - **The release tag is an output, never an input.** `release.yml` fires on a `lib/wurk/version.rb` bump landing on `main` — *not* on a tag push — derives the tag from `Wurk::VERSION` (`ReleaseHelpers.git_tag_for`), publishes the gem, and only then cuts the tag + GitHub Release, which then calls `deploy-demo` so the public demo tracks the released version. Never re-add a `tags:` trigger: a tag as input let anything that could push one (the `developerz-ai[bot]` maintainer agent did, seven times) turn the release lane red and leave a gem-less GitHub Release marked "Latest". Full rationale in `RELEASE.md`. ## Platforms diff --git a/test/test_helper.rb b/test/test_helper.rb index c3b53d2..821d03d 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -18,6 +18,8 @@ $LOAD_PATH.unshift(File.expand_path('../lib', __dir__)) +require 'etc' + # --- Per-worker Redis DB isolation ----------------------------------------- # Tests must never touch the base Redis DB (0): teardown runs FLUSHDB, which # would wipe a developer's real data. Each minitest-parallel_fork worker instead @@ -36,6 +38,13 @@ module Test WORKER_DATABASES = REDIS_DATABASES - 1 # 14 → DBs 1..14 for parallel workers DEDICATED_DB = REDIS_DATABASES # 15 → fixed-DB tests only + # Parallel worker default — HALF the cores, floored at 1, never above the + # historical 4. Read the NCPU block below for why one worker per core is the + # wrong shape for this suite; the flat 4 that used to live there WAS one per + # core on the 4-core fleet boxes that run `bin/check` as a merge gate, which + # is the class of red this scales away from (dz#4386, #522). + DEFAULT_NCPU = (Etc.nprocessors / 2).clamp(1, 4) + class << self attr_accessor :redis_url @@ -84,21 +93,26 @@ def assign_redis_db(worker_index) # of the #84 batch-TTL and #73 periodic-leader flakes. Cap the worker count to # the number of worker DBs so every worker gets a unique one. # -# The default stays 4 — deliberately below the core count of most machines that -# run this. The suite looks like a pure fan-out of independent classes, but the -# integration layer isn't: a single test boots a swarm of 4 children × 5 threads, +# The default is half the cores, capped at the historical 4 — deliberately below +# the core count, which is what this paragraph always argued for and what a flat +# 4 stopped delivering the moment the suite ran on a 4-core machine. The suite +# looks like a pure fan-out of independent classes, but the integration layer +# isn't: a single test boots a swarm of 4 children × 5 threads, # several use real BLMOVE timeouts, and some pools carry a 1s read timeout. One # worker per core therefore oversubscribes badly, and the failures it produces # are wall-clock ones (a drain that doesn't finish, a socket read that times out) # rather than honest assertion failures. Measured on a 12-core box: `NCPU=12` -# bought ~20% wall clock and cost a red build. +# bought ~20% wall clock and cost a red build. Measured on 4 cores (2026-09-19, +# `taskset -c 0-3 ./bin/check` at main): 4m57s at NCPU=4 against 5m24s at +# NCPU=2 — halving the workers costs 9%, because this suite waits far more than +# it computes. # # `NCPU` is the knob for a machine with headroom to spare, and `NCPU=1` is how # to chase an ordering flake. Clamped rather than merely capped so a typo'd # `NCPU=` (`to_i` → 0) can't fork zero workers; the ceiling is the number of # isolated Redis DBs, since two workers sharing one would FLUSHDB each other # mid-test (the root cause of the #84 and #73 flakes). -ENV['NCPU'] = (ENV['NCPU'] || 4).to_i.clamp(1, Wurk::Test::WORKER_DATABASES).to_s +ENV['NCPU'] = (ENV['NCPU'] || Wurk::Test::DEFAULT_NCPU).to_i.clamp(1, Wurk::Test::WORKER_DATABASES).to_s begin require 'minitest/parallel_fork' diff --git a/test/unit/ncpu_default_test.rb b/test/unit/ncpu_default_test.rb new file mode 100644 index 0000000..7105ea2 --- /dev/null +++ b/test/unit/ncpu_default_test.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require_relative '../test_helper' +require 'etc' + +# The parallel worker DEFAULT must never put a worker on every core. This +# suite's integration layer waits on real timeouts — a swarm of children, BLMOVE +# blocks, 1s pool read timeouts — so an oversubscribed machine fails on wall +# clock rather than on assertions, which reads as a flaky suite instead of a +# loaded one (dz#4386: a flat default of 4 on 4-core fleet boxes). +# +# Properties, never the formula: restating `[[nprocessors / 2, 1].max, 4].min` +# here would pass for any arithmetic the constant happens to contain. +class NcpuDefaultTest < Wurk::Test::UnitCase + parallelize_me! + + def test_never_forks_more_workers_than_the_machine_has_cores + assert_operator Wurk::Test::DEFAULT_NCPU, :<=, Etc.nprocessors + assert_operator Wurk::Test::DEFAULT_NCPU, :<=, 4 + end + + def test_always_forks_at_least_one_worker_and_never_shares_a_redis_db + assert_operator Wurk::Test::DEFAULT_NCPU, :>=, 1 + assert_operator Wurk::Test::DEFAULT_NCPU, :<=, Wurk::Test::WORKER_DATABASES + end +end From 20b1620edd04ad46400d0cdebd73d2d43541319d Mon Sep 17 00:00:00 2001 From: din Date: Sat, 19 Sep 2026 03:58:25 +0000 Subject: [PATCH 2/2] test(helper): state the worker rule across machines, not on this one CodeRabbit asked for `DEFAULT_NCPU == (Etc.nprocessors / 2).clamp(1, 4)`. That assertion re-derives the expression the constant already holds and passes for any arithmetic inside it, so it is not taken. The gap behind it is real though: a bounds-only test also passes if the default is hard-wired to 1 everywhere. `Wurk::Test.default_ncpu(cores)` makes the rule a function of the core count, which is the only way to state what it does on machines this one is not: 1 -> 1 2 -> 1 3 -> 1 4 -> 2 6 -> 3 8 -> 4 12 -> 4 64 -> 4 4 is the fleet box, 6 a developer laptop, 64 a CI runner. The constant is still the value every run uses, and one case pins it to the rule on this machine. Run red first: with the rule changed to one worker per core the table fails. `./bin/check` green after: 4319 runs, 0 failures, exit 0. Co-Authored-By: Claude Opus 5 (1M context) --- test/test_helper.rb | 14 ++++++++++++- test/unit/ncpu_default_test.rb | 37 ++++++++++++++++++++++++---------- 2 files changed, 39 insertions(+), 12 deletions(-) diff --git a/test/test_helper.rb b/test/test_helper.rb index 821d03d..120b003 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -43,7 +43,19 @@ module Test # wrong shape for this suite; the flat 4 that used to live there WAS one per # core on the 4-core fleet boxes that run `bin/check` as a merge gate, which # is the class of red this scales away from (dz#4386, #522). - DEFAULT_NCPU = (Etc.nprocessors / 2).clamp(1, 4) + # + # A FUNCTION OF THE CORE COUNT, not a constant derived from this machine's: + # a test can then state what the rule DOES across machines (1 -> 1, 4 -> 2, + # 8 -> 4, 64 -> 4) instead of re-deriving the same expression the constant + # already holds, which is an assertion that cannot fail. + # The historical default, and the ceiling the rule never exceeds. + WORKER_CAP = 4 + + def self.default_ncpu(cores) + (cores / 2).clamp(1, WORKER_CAP) + end + + DEFAULT_NCPU = default_ncpu(Etc.nprocessors) class << self attr_accessor :redis_url diff --git a/test/unit/ncpu_default_test.rb b/test/unit/ncpu_default_test.rb index 7105ea2..7b98b6f 100644 --- a/test/unit/ncpu_default_test.rb +++ b/test/unit/ncpu_default_test.rb @@ -3,24 +3,39 @@ require_relative '../test_helper' require 'etc' -# The parallel worker DEFAULT must never put a worker on every core. This -# suite's integration layer waits on real timeouts — a swarm of children, BLMOVE -# blocks, 1s pool read timeouts — so an oversubscribed machine fails on wall -# clock rather than on assertions, which reads as a flaky suite instead of a -# loaded one (dz#4386: a flat default of 4 on 4-core fleet boxes). +# The parallel worker default must never put a worker on every core. This +# suite's integration layer waits on real timeouts, so an oversubscribed machine +# fails on wall clock rather than on assertions, which reads as a flaky suite +# instead of a loaded one (dz#4386: a flat default of 4 on 4-core fleet boxes). # -# Properties, never the formula: restating `[[nprocessors / 2, 1].max, 4].min` -# here would pass for any arithmetic the constant happens to contain. +# The rule is tested as a FUNCTION OF THE CORE COUNT, because that is the only +# way to state what it does on machines this one is not. Asserting +# `DEFAULT_NCPU == (Etc.nprocessors / 2).clamp(1, 4)` would re-derive the +# expression the constant already holds and pass for any arithmetic inside it. class NcpuDefaultTest < Wurk::Test::UnitCase parallelize_me! + # One worker per core is the shape this rule exists to avoid, so the table is + # the assertion: half the cores, never below one, never above the historical + # four. 4 is the fleet box; 6 is a developer laptop; 64 is a CI monster. + CASES = { 1 => 1, 2 => 1, 3 => 1, 4 => 2, 6 => 3, 8 => 4, 12 => 4, 64 => 4 }.freeze + + def test_halves_the_cores_between_one_and_the_historical_four + actual = CASES.keys.to_h { |cores| [cores, Wurk::Test.default_ncpu(cores)] } + + assert_equal CASES, actual + end + def test_never_forks_more_workers_than_the_machine_has_cores - assert_operator Wurk::Test::DEFAULT_NCPU, :<=, Etc.nprocessors - assert_operator Wurk::Test::DEFAULT_NCPU, :<=, 4 + CASES.each_key do |cores| + assert_operator Wurk::Test.default_ncpu(cores), :<=, cores, + "#{cores} cores must not fork more than #{cores} workers" + end end - def test_always_forks_at_least_one_worker_and_never_shares_a_redis_db - assert_operator Wurk::Test::DEFAULT_NCPU, :>=, 1 + # The constant every run actually uses, bound to this machine. + def test_this_machine_uses_the_rule_and_stays_inside_the_redis_databases + assert_equal Wurk::Test.default_ncpu(Etc.nprocessors), Wurk::Test::DEFAULT_NCPU assert_operator Wurk::Test::DEFAULT_NCPU, :<=, Wurk::Test::WORKER_DATABASES end end