diff --git a/.github/workflows/backend-unit-test.yml b/.github/workflows/backend-unit-test.yml index def7245d1..1b3bc6c29 100644 --- a/.github/workflows/backend-unit-test.yml +++ b/.github/workflows/backend-unit-test.yml @@ -111,14 +111,51 @@ jobs: # || true: the existing 34 failures + 17 errors documented in #660 # produce a non-zero pytest exit. The diff job (`diff` below) is # what fails this workflow on regression. + # + # --timeout (#2019): without it a single stalled test consumed the whole + # 25-minute job budget, GitHub cancelled the job, the upload step below + # was skipped, and the failure read as `The operation was canceled` on + # whichever PR was running. Four times in one day across all three seeds + # AND both sides — the base side being plain `dev`, i.e. nothing to do + # with the PR it reddened. + # + # 300s, not the 60s this issue first proposed. Measured from a green + # run's JUnit rather than guessed: the slowest legitimate unit tests are + # test_start_agent_skip_inject's two retry cases at ~60.6s (they exercise + # three real connection retries), then 39.9s and 26.6s in + # test_1083_result_callback. A 60s cap would have failed those two on + # every PR. The cap also has to survive the runner variance that caused + # this issue in the first place — a shard ran 2.5x slower than its + # siblings — so a 60.6s test can legitimately reach ~150s. 300s clears + # that with margin and still turns a hang into a named failure: the + # observed stalls were minutes long and the budget fits ~5 of them. + # + # --timeout-method=signal (the default, stated explicitly), NOT thread — + # also a correction. `thread` dumps stacks and then kills the whole + # process, so the run aborts, the remaining tests never execute and the + # JUnit is incomplete: the diff job still fails, just with a named + # culprit in the log. `signal` raises inside the test, the suite + # CONTINUES, and the complete JUnit lets the diff job report the hang as + # an ordinary new failure by name. The logs support that shape — #1952 + # kept making progress after its 3m37s gap, so these are stalls that + # release, not one permanent hang. `signal` cannot interrupt a C call + # holding the GIL; if that ever turns out to be the case here, the + # symptom is another anonymous cancel and `thread` is the fallback. run: | cd tests python -m pytest unit/ -m "not slow" \ + --timeout=300 --timeout-method=signal \ -p randomly --randomly-seed=${{ matrix.seed }} \ --junit-xml="$GITHUB_WORKSPACE/junit-${{ matrix.side }}-${{ matrix.seed }}.xml" \ --tb=no -q || true - name: Upload JUnit XML + # always(): the timeout above should make an abnormal end unreachable, + # but the diff job is deliberately fail-closed on a missing artifact + # (correctly — a silent half-comparison is worse). Uploading whatever + # exists keeps one stalled shard from turning into a second, differently + # worded failure on the diff job (#2019). + if: always() uses: actions/upload-artifact@v7 with: name: junit-${{ matrix.side }}-${{ matrix.seed }} diff --git a/tests/registry.json b/tests/registry.json index 47f3e75fe..3e554cb8b 100644 --- a/tests/registry.json +++ b/tests/registry.json @@ -1599,6 +1599,18 @@ "skills" ], "description": "Skills-library status disclosed source repo URLs (ent#334). GET /api/skills/library/status is Depends(get_current_user) - every authenticated caller including agent-scoped MCP keys, deliberately, since the per-agent Skills tab and the MCP get_skills_library_status tool both read it - but it returned skill_service.get_library_status() raw, and that dict carries the source repo URLs: exactly the value GET /api/skills/sources is require_admin PLUS reject_agent_principal to protect (ent#237 classes repo URLs admin-sensitive; ent#293 added the second gate because an agent-scoped key resolves to its owner carrying the owner's role). Two independent failure modes, tested apart because the fixes differ: the URL as a NAME (org/repo discloses what the fleet is taught from - so every route assertion is on KEY ABSENCE, since a 'secret not in body' assertion passes on a payload that still leaks the private repo), and embedded userinfo (reject_embedded_credentials guards only NEW writes; rows predating it and rows from the unvalidated legacy-adoption path still carry https://@host). Covers the new never-raising, parse-based strip_url_credentials - urlparse itself raises ValueError on an unbalanced bracket and the only caller is get_library_status, whose comments say status must never 500 the panel, so the contract is never-raises and the test pins the premise that the input really does raise; scheme-less input, where urlparse('tok@github.com/o/r').username is None so a scheme must be assumed for the PARSE but never invented into the RETURN; userinfo with no host (hostname None, so _replace(netloc=None) would blow up in geturl); empty userinfo, which a parsed.username test reads as falsy; and the double-@ https://a@b@github.com/o/r that BOTH pre-existing scrubbers got wrong ([^@]+@ cannot cross the first @, leaving https://***@b@...). Parse-based, not regex, per skill_service._authenticated_url's house rule that the host is decided by PARSING - a looser pattern mangles a legitimate https://github.com/o/r?ref=a@b where the @ is in the query. The two free-text scrubbers (_scrub_pat, skill_source_clone.redact) stay text-oriented because git stderr carries several URLs and prose, but are widened to [^\\s/]+@ so the whole authority goes and an earlier plain URL no longer swallows a later credentialed one (redact's old [^@]+@ crossed / and newlines). Route coverage over a real TestClient rather than a direct handler call, because response_model filtering is applied by FastAPI's serialization layer and a direct call proves nothing about the wire; admin, non-admin and agent-scoped-key-on-an-admin-owned-install callers; that GET /skills/sources still returns the url (narrow the open route, don't blind the operator) and that it is userinfo-free because the strip is at the SERVICE, which is also what stops SkillSourcesPanel.vue rendering a token on the admin route. Plus a static AST guard that the route declares response_model=SkillsLibraryStatus - the allow-list IS the fix, and dropping the decorator kwarg reopens the leak with no behavioural test failing, since the handler's return value is unchanged." + }, + { + "file": "unit/test_2019_pytest_timeout_guard.py", + "feature": "#2019", + "added": "2026-08-05", + "categories": [ + "ci", + "unit", + "reliability", + "guard" + ], + "description": "backend-unit-test.yml installs pytest-timeout and never passed --timeout, so one stalled test consumed the whole 25-minute job budget, GitHub cancelled the job, the Upload JUnit XML step was skipped (no if: always()), and the failure read as 'The operation was canceled' against whichever PR was running - four times in one day across all three seeds AND both sides, the base side being plain dev. Guards the fix: a per-test --timeout is present, it is NOT the thread method (thread dumps stacks then kills the process, so the run aborts and the JUnit is incomplete - signal raises inside the test and the suite continues, giving the diff job a complete XML that names the hang as an ordinary new failure), and the value clears the slowest REAL test with room for runner variance. Bounds measured from a green run's JUnit rather than guessed: test_start_agent_skip_inject's two retry cases run ~60.6s, so the 60s the issue originally proposed would have failed them on every PR; a 2.5x-slow runner (the variance that caused #2019) puts them near 150s, hence >=180s, and <=600s so a stall still fails instead of eating the budget. Also pins if: always() on the artifact upload and the load-bearing '|| true' (#660 baseline). Assertions read the step's COMMAND with YAML comments stripped - the step documents its own flags in prose, and an earlier draft passed with '|| true' deleted because the comment still mentioned it (the #1871/ent#314 textual-scan trap, third occurrence). The step slice is indentation-bounded after a name-bounded version ran past the last step and matched the diff job's own 'if: always()'." } ] } diff --git a/tests/unit/test_2019_pytest_timeout_guard.py b/tests/unit/test_2019_pytest_timeout_guard.py new file mode 100644 index 000000000..4b0f4412d --- /dev/null +++ b/tests/unit/test_2019_pytest_timeout_guard.py @@ -0,0 +1,208 @@ +"""#2019 — a hanging unit test must name itself, not kill the shard. + +`backend-unit-test.yml` runs the unit suite under a 25-minute job timeout. It +installs `pytest-timeout` (`tests/requirements-test.txt`) and never used it, so +a single stalled test consumed the whole budget, the job was cancelled by +GitHub, the `Upload JUnit XML` step was skipped, and the failure surfaced as +`##[error]The operation was canceled` against whichever PR happened to be +running. + +Four occurrences in one day across **all three seeds and both sides** (#2010 +head/99999, #1952 base/67890, #1976 base/67890, #2018 head/12345) — including +the base side, which is plain `dev` and therefore nothing to do with the PR +being reddened. Re-running the identical seed passed, so it is timing- rather +than order-dependent: something that usually returns fast and occasionally +blocks. The #1952 log shows a 3m37s gap with zero output between two progress +lines, which is a stall, not a slow runner. + +The cost of the missing flag was not the lost minutes; it was that every +occurrence had to be diagnosed by hand from a log and produced no evidence +about which test was responsible. I misdiagnosed it once for exactly that +reason. + +These tests pin the flag, the method, and the artifact upload, so the guard +cannot be silently dropped in a future edit of the workflow. +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import pytest + +_REPO = Path(__file__).resolve().parents[2] +_WORKFLOW = _REPO / ".github" / "workflows" / "backend-unit-test.yml" + +pytestmark = pytest.mark.unit + + +@pytest.fixture(scope="module") +def workflow() -> str: + assert _WORKFLOW.is_file(), f"{_WORKFLOW} is missing" + return _WORKFLOW.read_text(encoding="utf-8") + + +def _step(workflow: str, title: str) -> str: + """Exactly ONE step's body, bounded by indentation. + + An earlier version of this helper sliced from the step title to the next + `- name:` — which runs off the end of the last step in a job and swallows + the following job. That is not hypothetical: it made the `if: always()` + assertion below match the `diff` job's own `if: always() && ...` and pass + with the guard deleted. Caught by mutation-testing this file. + + Bounding on indentation instead: a step's body is every line indented + deeper than its `- name:` marker. + """ + lines = workflow.splitlines() + start = next(i for i, l in enumerate(lines) if l.strip().startswith(f"- name: {title}")) + indent = len(lines[start]) - len(lines[start].lstrip()) + body = [lines[start]] + for line in lines[start + 1:]: + if line.strip() and (len(line) - len(line.lstrip())) <= indent: + break + body.append(line) + return "\n".join(body) + + +def _command(workflow: str, title: str) -> str: + """A step's runnable lines, with YAML comments stripped. + + Load-bearing, not tidiness. The step this file guards *documents itself* — + its comment block names `--timeout`, `--timeout-method=thread` and + `|| true` while explaining why they are there. A substring assertion over + the raw step therefore passes on the prose after the flag has been deleted + from the command, which is precisely the failure this file exists to + prevent. Caught by mutation-testing: removing `|| true` from the command + left the test green because the comment still said it. + + The same trap is recorded in `docs/memory/learnings.md` for the #1871 + `containers_run` guard and the ent#314 loader sweep. Third occurrence. + """ + out = [] + for line in _step(workflow, title).splitlines(): + stripped = line.strip() + if stripped.startswith("#"): + continue + out.append(line) + return "\n".join(out) + + +def _pytest_step(workflow: str) -> str: + """The `Run unit suite` step's COMMAND, comments removed. + + Scoped rather than searched whole-file: the nightly-style steps and the + self-test invocation also contain `pytest`, and a whole-file grep would + pass on any of them while the suite step itself lost the flag. + """ + return _command(workflow, "Run unit suite") + + +class TestTheTimeoutIsWired: + + def test_the_unit_suite_passes_a_per_test_timeout(self, workflow): + step = _pytest_step(workflow) + assert "--timeout=" in step, ( + "the unit suite runs with no per-test timeout — one hanging test " + "again consumes the 25-minute job budget and dies without naming " + "itself (#2019)" + ) + + def test_the_timeout_does_not_kill_the_run(self, workflow): + """`signal`, NOT `thread` — a correction to what the issue proposed. + + `thread` dumps stacks and then kills the process, so the run aborts, + the remaining tests never execute, and the JUnit is incomplete — the + diff job fails anyway, just with a named culprit in the log. `signal` + raises inside the offending test and the suite CONTINUES, so the + complete JUnit lets the diff job report the hang as an ordinary new + failure by name, which is the outcome worth having. + + The logs support that shape: #1952 kept making progress after its + 3m37s gap, so these are stalls that release rather than one permanent + hang. + """ + step = _pytest_step(workflow) + assert "--timeout-method=thread" not in step, ( + "`thread` aborts the whole run on the first stall, losing the " + "complete JUnit the diff job needs (#2019)" + ) + + def test_the_timeout_clears_the_slowest_real_test(self, workflow): + """The cap has to sit above every legitimate test AND above runner + variance, or it becomes a new source of red. + + Measured from a green run's JUnit, not guessed: the slowest legitimate + unit tests are `test_start_agent_skip_inject`'s two retry cases at + ~60.6s, then 39.9s and 26.6s in `test_1083_result_callback`. The 60s + this issue originally proposed would have failed the first two on every + PR. Runner variance is the whole premise of #2019 — one shard ran 2.5x + slower than its siblings — so a 60.6s test can legitimately reach + ~150s. + + Upper bound: the cap must still be small enough that a stall fails + rather than eating the 25-minute job budget. + """ + step = _pytest_step(workflow) + match = re.search(r"--timeout=(\d+)", step) + assert match, "no numeric --timeout value found" + seconds = int(match.group(1)) + assert seconds >= 180, ( + f"--timeout={seconds}s is under the ~150s a legitimate 60.6s test " + "can reach on a slow runner — this would red every PR" + ) + assert seconds <= 600, ( + f"--timeout={seconds}s is too close to the 25-minute job budget to " + "prevent the failure it is for" + ) + + def test_the_plugin_is_actually_installed(self): + """The flag is inert without the plugin, and pytest ignores unknown + `--timeout` only if some other plugin claims it — so pin the dep.""" + reqs = (_REPO / "tests" / "requirements-test.txt").read_text() + assert "pytest-timeout" in reqs + + +class TestTheArtifactSurvives: + + def test_junit_upload_runs_even_when_pytest_ends_abnormally(self, workflow): + """Without `if: always()` the upload is skipped on cancellation, so the + diff job loses that side's XML entirely and fails a second time with a + different message. The timeout should make this unreachable; belt and + braces, because the diff job is deliberately fail-closed on a missing + artifact and that is the right behaviour to keep.""" + step = _command(workflow, "Upload JUnit XML") + assert "if: always()" in step, ( + "the JUnit upload is skipped when the pytest step is cancelled, so " + "the regression-diff job sees a missing artifact instead of the " + "real result (#2019)" + ) + + +class TestTheSuiteStepIsStillWhatWeThinkItIs: + + def test_the_step_still_runs_the_unit_directory_under_three_seeds(self, workflow): + """Guards the guard: if the step is restructured, the assertions above + could pass against something that no longer runs the unit suite.""" + step = _pytest_step(workflow) + assert "unit/" in step and "--randomly-seed=" in step + + def test_the_step_still_tolerates_the_660_baseline(self, workflow): + """`|| true` is load-bearing — the documented #660 failures make pytest + exit non-zero, and the diff job is what fails the workflow. A timeout + that turned the step fatal would red every PR.""" + step = _pytest_step(workflow) + assert "|| true" in step + + +def test_the_assertions_read_the_command_not_the_comment(workflow): + """The step documents its own flags in prose, so every assertion above is + one careless helper away from testing the comment block instead of the + command. Pin the stripping directly.""" + raw = _step(workflow, "Run unit suite") + cmd = _command(workflow, "Run unit suite") + + assert "#" in raw, "the step no longer carries the comment this guards against" + assert not any(l.strip().startswith("#") for l in cmd.splitlines()) + assert "python -m pytest" in cmd, "comment stripping ate the command"