diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e114400d..76a11687 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -6,6 +6,33 @@ on: pull_request: branches: [main] +# One in-flight run per PR. Pushing again to an open PR supersedes the previous +# run, which used to keep burning a full ~39 billed minutes (a Windows job bills +# 2x) on a commit nobody would ever look at again. Measured over 13 days: 29 of +# 219 PR runs (13%) were still executing when the next push landed, and a single +# iterating branch racked up 23 runs. +# +# A push to main is the post-merge verification of a commit that is already +# shipped, so it must never be dropped: losing the signal for the commit in +# between two merges is exactly when you want it. +# +# `cancel-in-progress: false` is NOT enough to guarantee that. It protects a +# RUNNING run; GitHub keeps at most one running plus one PENDING run per group +# and, quoting the workflow-syntax docs, "any existing `pending` job or workflow +# in the same concurrency group will be canceled and the new queued job or +# workflow will take its place". So with every main push sharing one +# ref-keyed group, three merges landing inside one run duration silently cancel +# the middle commit's queued run. Not hypothetical here: 6 of the last 120 main +# pushes sat in a 3-merges-within-5-minutes window, one burst landing five +# merges ~15s apart. +# +# Keying pushes on the SHA gives every main commit its own group, so it can +# never be the pending run that gets displaced. PRs stay keyed on the ref, which +# is what makes a new push supersede the previous run. +concurrency: + group: ci-${{ github.workflow }}-${{ github.event_name == 'pull_request' && github.ref || github.sha }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + jobs: # ──────────────────────────────────────────────────────────────────────── # Static analysis + silent-drift gates. These are deterministic and @@ -31,6 +58,8 @@ jobs: # raw.githubusercontent.com on every run -- that fetch is rate-limited # and flaky on shared CI egress IPs (it killed the Windows job once). version: "0.11.16" + enable-cache: true + cache-dependency-glob: "uv.lock" - uses: actions/setup-python@v6 with: @@ -171,22 +200,35 @@ jobs: run: uv run python scripts/check_file_size.py # ──────────────────────────────────────────────────────────────────────── - # Test suite across every supported interpreter. pyproject declares - # `requires-python = ">=3.12"`, so the matrix is 3.12 + 3.13 (3.10/3.11 are - # intentionally out of scope). `integration` tests are deselected; `e2e` - # tests self-skip without credentials (the dedicated e2e.yml workflow runs - # them nightly against a real project). Coverage is INFORMATIONAL: the - # term-missing report is printed for visibility but NO --cov-fail-under - # threshold is enforced, so it never blocks a merge. + # Test suite. pyproject declares `requires-python = ">=3.12"`, so the + # supported interpreters are 3.12 + 3.13 (3.10/3.11 are intentionally out of + # scope). `integration` tests are deselected; `e2e` tests self-skip without + # credentials (the dedicated e2e.yml workflow runs them nightly against a + # real project). + # + # WHICH interpreters run depends on the event: + # * pull_request -> 3.12 only (the floor, and what the wheel is built on) + # * push to main -> 3.12 + 3.13 + # A 3.13-only regression is therefore caught on main rather than on the PR. + # That is an accepted trade: the two interpreters differ in nothing this + # codebase touches, a PR-time 3.13 job cost ~8 billed minutes of the ~39 a PR + # used to spend, and main runs unattended anyway. If 3.13 ever starts + # diverging in practice, put it back in the PR matrix -- that is a one-word + # change to the expression below. + # + # Coverage is INFORMATIONAL -- no --cov-fail-under threshold is enforced, so + # it can never block a merge. It also costs ~50% extra wall clock (measured: + # 152s -> 230s sequentially), so it is collected on main pushes only. Locally + # it stays one `make test-cov` away. # ──────────────────────────────────────────────────────────────────────── test: runs-on: ubuntu-latest strategy: # Don't cancel 3.12 just because 3.13 tripped (or vice versa) -- we want - # to see which interpreters pass on a given PR, not just the first failure. + # to see which interpreters pass on a given run, not just the first failure. fail-fast: false matrix: - python-version: ["3.12", "3.13"] + python-version: ${{ github.event_name == 'pull_request' && fromJSON('["3.12"]') || fromJSON('["3.12", "3.13"]') }} env: # Pin uv's interpreter to the matrix version for BOTH `sync` and `run`, # so the suite is actually exercised on each interpreter instead of @@ -198,6 +240,8 @@ jobs: - uses: astral-sh/setup-uv@v7 with: version: "0.11.16" + enable-cache: true + cache-dependency-glob: "uv.lock" - uses: actions/setup-python@v6 with: @@ -206,8 +250,23 @@ jobs: - name: Install dependencies run: uv sync --extra server + # `-n auto` fans the ~6k tests across the runner's cores (4 on a standard + # GitHub-hosted runner). Every HTTP call in this suite is mocked and the + # tests hold no shared mutable state, so it parallelises cleanly -- + # measured locally at 152s -> 37s on 4 workers, byte-identical results + # over repeated runs. `-v` is dropped: under xdist it interleaves 6k + # lines from four workers into unreadable output, and a failure prints + # its own node id anyway. + - name: Tests + if: github.event_name != 'push' + run: uv run pytest tests/ -m "not integration" -n auto + + # Same run, plus the informational coverage report. pytest-cov combines + # the per-worker data files automatically, so the totals match a + # sequential run exactly (verified: 31296/5405/9186/1121, 80%). - name: Tests (with coverage report) - run: uv run pytest tests/ -v -m "not integration" --cov --cov-report=term-missing + if: github.event_name == 'push' + run: uv run pytest tests/ -m "not integration" -n auto --cov --cov-report=term-missing # Real wheel build and focused export regression on Windows -- the only # place the issue #320 and #529 fixes can be verified against real Windows @@ -228,6 +287,8 @@ jobs: # raw.githubusercontent.com on every run -- that fetch is rate-limited # and flaky on shared CI egress IPs (it killed the Windows job once). version: "0.11.16" + enable-cache: true + cache-dependency-glob: "uv.lock" - uses: actions/setup-python@v6 with: @@ -247,29 +308,6 @@ jobs: - name: Build wheel WITH bundled UI run: uv build --wheel - # Windows does not expose os.O_NOFOLLOW. Exercise the focused export - # regression here, where the portable flags run on the real platform. - - name: Test semantic-layer export on Windows (issue #529) - run: uv run pytest tests/test_semantic_layer_service.py -k "export" -v - - # The self-update helper is a PowerShell script authored on machines that - # cannot execute it. This suite runs it for real: it proves the script - # parses, that it records the installer's exit code, and -- the branch - # that actually protects the environment -- that it installs NOTHING - # while a watched process is still alive (issue #528). The rest of the - # suite also runs here with `should_defer()` returning its real Windows - # default and a real detached spawn. - - name: Test the deferred self-update helper on Windows (issue #528) - run: uv run pytest tests/test_update_runner.py -v - - # `os.replace()` cannot rename over a file Windows still holds open, so a - # store that locks its own state file dies with WinError 5 on every write - # while passing everywhere on POSIX. That shipped: `job run - # --idempotency-key` was unusable on Windows for a full release. Anything - # doing lock-then-atomic-replace has to prove it here. - - name: Test lock-then-replace file stores on Windows (issue #427) - run: uv run pytest tests/test_job_idempotency_store.py -v - # The whole suite, as a gate. It never ran here before: a real run was 55 # failures, and nobody can gate on that, so nothing did -- which let real # Windows defects sit in the noise for entire releases. Two of them did: @@ -281,20 +319,57 @@ jobs: # fcntl does not exist there). Keeping this green is the point: a red # suite nobody can act on is worse than no suite, because it reads as # coverage while hiding things. - # The targeted steps above run against the default dependency set, but - # the full suite imports the `serve` tests, so it needs the same extras + # The specific regressions this job exists to hold down are all inside the + # suite below, and used to ALSO run as three separate named steps before + # it (`-k export` for #529, test_update_runner.py for #528, + # test_job_idempotency_store.py for #427). Those were strict subsets of + # this run -- duplicated Windows minutes, billed at 2x -- so they are + # gone. What they covered has not changed: + # * #529: Windows has no os.O_NOFOLLOW, so semantic-layer export must + # prove its portable flags on the real platform. + # * #528: the deferred self-update helper is a PowerShell script + # authored on machines that cannot run it; here it really + # executes, with a real detached spawn, and must install + # NOTHING while a watched process is still alive. + # * #427: os.replace() cannot rename over a file Windows still holds + # open, so lock-then-atomic-replace stores die with WinError 5 + # on every write while passing everywhere on POSIX. That + # shipped once -- `job run --idempotency-key` was unusable on + # Windows for a full release. + # A failure now names the test node id instead of the step, which is the + # same information. + # + # The full suite imports the `serve` tests, so it needs the same extras # the Linux job installs. Without this the gate fails on # `ModuleNotFoundError: No module named 'fastapi'` -- an artefact of the # job's own setup rather than anything about Windows. - name: Install server extras for the full run run: uv sync --extra server + # `-n auto`, same as the Linux jobs: this step was 477s of the job's 568s, + # and Windows minutes bill at 2x, so it was the single most expensive + # thing in the whole PR pipeline. + # + # Read a Windows-only flake here carefully before blaming xdist. The + # first run of this change surfaced exactly one failure, and it was a + # test asserting a 250ms wall-clock margin (test_auth_pkce), not a + # parallelism-safety bug -- four busy workers just made the stall that + # the margin never tolerated actually happen. That test was widened; + # check for the same shape first. + # + # If something genuinely does turn out to be parallel-unsafe, drop back + # to sequential by replacing `-n auto` with `-p no:xdist` -- do NOT + # disable the suite. Windows-only file-locking and atomic-replace semantics are + # exactly what it is here to catch, and parallel workers touching the + # same tmp paths are the plausible failure mode. The Linux jobs are + # unaffected either way. - name: Full test suite on Windows run: > uv run pytest tests/ --ignore=tests/test_e2e.py --ignore=tests/test_e2e_auth.py --ignore=tests/test_server_semantic_layer_routes_e2e.py + -n auto -q - name: Assert the SPA is bundled (Bug 1 fixed) diff --git a/.github/workflows/frontend.yml b/.github/workflows/frontend.yml index 094c5aad..0bea77bd 100644 --- a/.github/workflows/frontend.yml +++ b/.github/workflows/frontend.yml @@ -35,6 +35,15 @@ on: - "web/**" - ".github/workflows/frontend.yml" +# Same rationale, and the same pending-run caveat, as ci.yml: supersede an +# in-flight PR run when the branch is pushed again, but never drop a main push. +# Pushes are keyed on the SHA rather than the ref because `cancel-in-progress: +# false` only protects a RUNNING run -- a PENDING one is cancelled when a newer +# run queues into the same group. See the longer note in ci.yml. +concurrency: + group: frontend-${{ github.workflow }}-${{ github.event_name == 'pull_request' && github.ref || github.sha }} + cancel-in-progress: ${{ github.event_name == 'pull_request' }} + jobs: frontend: name: Type check + test + build (web/frontend) diff --git a/Makefile b/Makefile index 04cc9cd3..53df0c70 100644 --- a/Makefile +++ b/Makefile @@ -14,11 +14,16 @@ install-server: ## Install FastAPI/uvicorn for `kbagent serve` (web UI backend) sync: ## Sync dependencies from lockfile uv sync +# `-n auto` fans the suite across every core (~6k tests, all HTTP mocked, no +# shared mutable state). Measured 152s -> 29s on an 11-core machine. `-v` is +# dropped with it: interleaved per-worker output is unreadable, and a failing +# test prints its own node id. Use `make test-file FILE=...` for a sequential, +# verbose run while debugging a single file. test: ## Run all tests (excluding e2e — use test-e2e separately) - uv run pytest tests/ -v -m "not e2e" + uv run pytest tests/ -m "not e2e" -n auto test-unit: ## Run unit tests only (exclude integration and e2e) - uv run pytest tests/ -v -m "not integration and not e2e" + uv run pytest tests/ -m "not integration and not e2e" -n auto test-integration: ## Run integration tests only uv run pytest tests/ -v -m integration @@ -46,7 +51,7 @@ test-file: ## Run a specific test file (FILE=tests/test_cli.py) uv run pytest $(FILE) -v test-cov: ## Run the unit suite with a coverage report (informational; no threshold gate) - uv run pytest tests/ -v -m "not integration" --cov --cov-report=term-missing + uv run pytest tests/ -m "not integration" -n auto --cov --cov-report=term-missing lint: ## Run ruff linter uv run ruff check src/ tests/ scripts/ diff --git a/pyproject.toml b/pyproject.toml index f75524f6..a808a6a3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -138,6 +138,12 @@ dev = [ "pytest-cov>=5", "pytest-httpx>=0.30", "pytest-asyncio>=0.23", + # Parallel test execution. The suite is ~6k tests of in-process CliRunner + # invocations with every HTTP call mocked -- CPU-bound, no shared mutable + # state, so it scales almost linearly across workers. Sequentially it took + # ~8.5 min per interpreter in CI (three such runs per PR, one of them on a + # 2x-billed Windows runner) purely because nothing ever ran concurrently. + "pytest-xdist>=3.6", "ruff>=0.8", "ty>=0.0.33", "bandit>=1.9.4", diff --git a/tests/test_auth_pkce.py b/tests/test_auth_pkce.py index 6eb1bc1c..235d88e2 100644 --- a/tests/test_auth_pkce.py +++ b/tests/test_auth_pkce.py @@ -195,18 +195,36 @@ def test_timeout_raises_pkce_callback_timeout(self) -> None: def test_pkce_callback_timeout_is_fallback_eligible(self) -> None: assert issubclass(PkceCallbackTimeout, PkceSetupError) - def test_callback_arriving_just_before_timeout_succeeds(self) -> None: + def test_callback_arriving_before_timeout_succeeds(self) -> None: + """A callback that lands inside the deadline resolves the wait. + + The margin between the callback (0.05s) and the deadline is deliberately + wide. It used to be 0.3s, which passed on an idle machine and failed on a + busy one: under parallel CI workers on Windows a 250ms scheduling stall + is ordinary, and the test then reported a callback-handling bug that did + not exist. A generous deadline costs nothing here -- wait() returns the + moment the callback arrives, not when the timeout expires -- and nothing + is lost by widening it, because that the deadline is HONOURED is what + test_timeout_raises_pkce_callback_timeout and the sibling below assert. + """ with PkceCallbackServer(expected_state="expected-state") as server: _get_after(0.05, server.redirect_uri, {"code": "on-time", "state": "expected-state"}) - result = server.wait(timeout=0.3) + result = server.wait(timeout=5.0) assert result.code == "on-time" - def test_callback_arriving_just_after_timeout_is_not_observed(self) -> None: + def test_callback_arriving_after_timeout_is_not_observed(self) -> None: """A callback scheduled to land after the (short, injected) timeout must - not be picked up -- wait() raises PkceCallbackTimeout on schedule.""" + not be picked up -- wait() raises PkceCallbackTimeout on schedule. + + Same widened margin, for the same reason, in the other direction: the + callback must be comfortably later than the deadline even when the box + stalls. This does NOT make the test slow -- wait() raises after 0.1s and + the block exits; the pending callback then fires from a daemon timer + against a closed server, which `_get` swallows by design. + """ with PkceCallbackServer(expected_state="expected-state") as server: - _get_after(0.4, server.redirect_uri, {"code": "too-late", "state": "expected-state"}) + _get_after(5.0, server.redirect_uri, {"code": "too-late", "state": "expected-state"}) with pytest.raises(PkceCallbackTimeout): server.wait(timeout=0.1) diff --git a/uv.lock b/uv.lock index cc107a56..f0509b25 100644 --- a/uv.lock +++ b/uv.lock @@ -421,6 +421,15 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/07/6c/aa3f2f849e01cb6a001cd8554a88d4c77c5c1a31c95bdf1cf9301e6d9ef4/defusedxml-0.7.1-py2.py3-none-any.whl", hash = "sha256:a352e7e428770286cc899e2542b6cdaedb2b4953ff269a210103ec58f6198a61", size = 25604, upload-time = "2021-03-08T10:59:24.45Z" }, ] +[[package]] +name = "execnet" +version = "2.1.2" +source = { registry = "https://pypi.org/simple" } +sdist = { url = "https://files.pythonhosted.org/packages/bf/89/780e11f9588d9e7128a3f87788354c7946a9cbb1401ad38a48c4db9a4f07/execnet-2.1.2.tar.gz", hash = "sha256:63d83bfdd9a23e35b9c6a3261412324f964c2ec8dcd8d3c6916ee9373e0befcd", size = 166622, upload-time = "2025-11-12T09:56:37.75Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/ab/84/02fc1827e8cdded4aa65baef11296a9bbe595c474f0d6d758af082d849fd/execnet-2.1.2-py3-none-any.whl", hash = "sha256:67fba928dd5a544b783f6056f449e5e3931a5c378b128bc18501f7ea79e296ec", size = 40708, upload-time = "2025-11-12T09:56:36.333Z" }, +] + [[package]] name = "fastapi" version = "0.137.2" @@ -615,6 +624,7 @@ dev = [ { name = "pytest-asyncio" }, { name = "pytest-cov" }, { name = "pytest-httpx" }, + { name = "pytest-xdist" }, { name = "ruff" }, { name = "ty" }, ] @@ -649,6 +659,7 @@ dev = [ { name = "pytest-asyncio", specifier = ">=0.23" }, { name = "pytest-cov", specifier = ">=5" }, { name = "pytest-httpx", specifier = ">=0.30" }, + { name = "pytest-xdist", specifier = ">=3.6" }, { name = "ruff", specifier = ">=0.8" }, { name = "ty", specifier = ">=0.0.33" }, ] @@ -1026,6 +1037,19 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/1e/55/1fa65f8e4fceb19dd6daa867c162ad845d547f6058cd92b4b02384a44777/pytest_httpx-0.36.2-py3-none-any.whl", hash = "sha256:d42ebd5679442dc7bfb0c48e0767b6562e9bc4534d805127b0084171886a5e22", size = 20315, upload-time = "2026-04-09T13:57:18.587Z" }, ] +[[package]] +name = "pytest-xdist" +version = "3.8.0" +source = { registry = "https://pypi.org/simple" } +dependencies = [ + { name = "execnet" }, + { name = "pytest" }, +] +sdist = { url = "https://files.pythonhosted.org/packages/78/b4/439b179d1ff526791eb921115fca8e44e596a13efeda518b9d845a619450/pytest_xdist-3.8.0.tar.gz", hash = "sha256:7e578125ec9bc6050861aa93f2d59f1d8d085595d6551c2c90b6f4fad8d3a9f1", size = 88069, upload-time = "2025-07-01T13:30:59.346Z" } +wheels = [ + { url = "https://files.pythonhosted.org/packages/ca/31/d4e37e9e550c2b92a9cbc2e4d0b7420a27224968580b5a447f420847c975/pytest_xdist-3.8.0-py3-none-any.whl", hash = "sha256:202ca578cfeb7370784a8c33d6d05bc6e13b4f25b5053c30a152269fd10f0b88", size = 46396, upload-time = "2025-07-01T13:30:56.632Z" }, +] + [[package]] name = "python-dateutil" version = "2.9.0.post0"