Repository navigation
perf(ci): Collect only each shard's files on full backend runs - #127943
Merged
Merged
Conversation
github-actions
Bot
added
the
Scope: Backend
joshuarli
approved these changes
Oct 17, 2026
joshuarli
marked this pull request as ready for review
October 17, 2026 16:35
joshuarli
enabled auto-merge (squash)
October 17, 2026 16:48
Full runs had every xdist worker collect the whole suite and then hash-deselect ~95% of it. Plan full-run shards by file from checked-in per-file weights, splitting the slowest files by test across shards, and upload per-shard durations so the weights can be refreshed. The initial weights are real collected test counts; durations follow.
Replace the bootstrap test counts with per-file durations from a full CI run, keep collection order within each shard instead of running slow files first, and split files over 1/48 of a shard so a late large file can't hold up an xdist worker.
sentry-junior
Bot
force-pushed
the
jl/full-run-file-sharding
branch
from
October 17, 2026 16:50
dc577f0 to
0b666da
Compare
joshuarli
added a commit
that referenced
this pull request
Oct 17, 2026
Follow-up to retro feedback on #127938, #127939, #127941, #127942 and #127943. I checked each item against the code before changing anything. Below, each one is marked done or declined. No test case or assertion was dropped or weakened. Scheduling stays on `--dist=loadfile` with deterministic order, and xdist 3.8.0 keeps `loadscopereorder = False`. ### Filtered sharded runs (first commit) Since #127943, `pytest-selected.py` splits test files across shards before pytest applies `-m`/`-k`. Snuba's `ci` job runs sentry master's tests with `PYTEST_ADDOPTS="-k 'not __In' -m snuba_ci"` over 16 shards. As a result, most of those shards collected 0 tests and exited with code 5, and fail-fast cancelled the rest. This breaks the sentry tests job on every Snuba PR (e.g. getsentry/snuba#8556). The first commit falls back to the pre-#127943 per-test hash sharding whenever `-m` or `-k` appears in the args or in `PYTEST_ADDOPTS`. In that case every shard collects everything, and the sentry plugin keeps that shard's hash share after collection. Exit code 5 is still treated as a failure. Sentry's own backend CI passes neither flag, so it keeps per-file sharding. I reproduced Snuba's collection locally against this checkout: same args as `make test-python-ci`, the same `PYTEST_ADDOPTS` filter, `--collect-only`, 16 shards. Unsharded, 78 tests are selected. | | Shards with 0 tests (exit 5) | Tests per shard | Union of shards == unsharded (`Counter`) | |---|---|---|---| | #127943 | 8 of 16, including shards 0 and 12 | 0–23 | yes | | this fix | 0 of 16 | 2–8 | yes | New tests: `tests/sentry/testutils/pytest/test_sentry.py` runs `pytest-selected.py` and the real plugins over a throwaway project with `-m`, `-k`, and Snuba's `PYTEST_ADDOPTS` form. It checks that every shard runs something and that the shards together run each selected test exactly once. Without the fix these cases fail with exit code 5. There are also unit tests for the flag detection. getsentry's CI doesn't pass `-m`/`-k` to its `pytest-selected.py`. ### #127939: subtests in the JSON report (done) The rerun adapter in `json_report_reruns.py` treated every repeated `report.when` under a nodeid as a rerun. I reproduced this with the locked stack (pytest 9, pytest-json-report 1.5.0, rerunfailures, xdist). A test with 3 passing subtests that ran once got 3 `reruns` entries, and its `setup` moved into one of them. When a subtest failed, the only top-level traceback was "contains 1 failed subtest". pytest's exit code was still correct. - The adapter now detects `pytest.SubtestReport`. It keeps one parent `setup`/`call`/`teardown` and stores subtests in a new `subtests` list, each with its `context` (`msg`, repr'd `kwargs`), outcome, duration and longrepr. rerunfailures holds back parent reports until an attempt ends, so subtests are buffered and attached to the `call` they ran in. A genuine rerun still goes to `reruns` and carries that attempt's subtests along. - Outcome: the parent stays `passed` when every subtest passes and becomes `failed` when any subtest fails. I normalized away pytest's "subtests passed" label because `report-backend-test-failures.js` selects `outcome === 'failed'`. - Failure comments now show each failed subtest's context and traceback, up to 5 per test. - `backend-test-durations.py extract` still sums only the parent phases. The parent `call` already includes the subtest loop, so subtest durations are not added again. Since setup no longer moves into `reruns`, it is counted again. - `tests/sentry/testutils/pytest/test_json_report_reruns.py` runs the real plugin stack (json-report, this adapter, rerunfailures, xdist `-n 2`) in a subprocess. It checks that passing subtests make no reruns, a failed subtest keeps its context and assertion traceback, parent setup appears once, and a genuine rerun is recorded. Against the old adapter it fails 6 of 8 cases. ### #127941: transactional flush race (done) The race was real. The table inventory and the open-transaction check were separate statements. A row committed into a table with no pages between those two steps survived teardown. - Each database is now cleaned in one transaction: `lock_timeout = 1s`, then `LOCK TABLE <every managed table> IN EXCLUSIVE MODE` (this blocks writes but allows reads), then inventory, fingerprint check and `DELETE`. A lock timeout or deadlock falls back to Django's flush, which waits for writers the way `TRUNCATE` always has. The open-transaction check stays, both before the lock (so a known-open writer skips the 1s wait) and under it. The lock blocks writers that have already written. A transaction that has only read can still write after the lock is released. - Setting `SENTRY_LEGACY_TRANSACTIONAL_FLUSH=1` keeps Django's teardown. `pytest_unconfigure` restores the original method and clears the seeded-row cache. The cache is keyed by alias plus database `NAME`. - Stable contenttype/permission IDs are unchanged (intentional). - `tests/sentry/testutils/pytest/test_transactional_flush.py` drives `_fixture_teardown` explicitly, so it doesn't depend on test order. It covers: - ORM and raw SQL rows across default/control/secondary, plus a large TOAST value - inserting, updating and deleting seeded contenttype/permission/site rows, which forces the fallback and restores state - the `available_apps` and `serialized_rollback` fallbacks - a second connection that tries to commit into an empty table during cleanup: it blocks on the lock and no row survives - a writer already open at teardown start, cleaned up through the fallback - lock-timeout fallback, the escape hatch, and unconfigure The race tests synchronize on `pg_blocking_pids` / `pg_stat_activity`, not sleeps, and they fail with the lock disabled. - Lock cost, measured locally against the real test schema (254 default, 70 control, 6 secondary tables). A warm teardown across all DBs went from ~15–18 ms to ~23–27 ms. A cold connection went from ~85–92 ms to ~101–112 ms. That is roughly 10–20 ms per teardown against ~1.27 s for Django's flush, so the lock keeps almost all of the saving. CI agrees (see below). - Audits: - No non-internal `pg_trigger` rows exist in any test DB, so DELETE vs TRUNCATE trigger behavior doesn't matter. - The `post_migrate` receivers the skip bypasses are only Django's `create_contenttypes`, `create_permissions` and `create_default_site`. The fingerprint covers all of their rows. - Dead tuples: across 117 transactional teardowns in one session, the first-quarter median was 0.126 s and the last-quarter median 0.116 s. Teardowns did not slow down over the session. - Declined: making the plugin opt-in, separate toggles for DELETE vs the `post_migrate` skip, and non-Postgres support. Sentry tests only run on Postgres, and those backends already take Django's path. ### #127942: Relay readiness (done) `wait_until_ready()` checks the time left before each probe. The request timeout is `min(1s, remaining)`, sleeps are capped by the time left, and no probe starts after the deadline. A 200 from a probe that started before the deadline counts as ready even if it arrives late. It catches `requests.RequestException` instead of `Exception`. The failure message includes the last HTTP status or exception plus the container logs, and the container is removed even if fetching the logs fails. Every test still gets a fresh container. Unit tests with an injected clock, probe and sleep cover: immediate ready, 503→200, connection error→success, permanent 503, request timeout, near-deadline success, and bounded sleep. ### #127939: subtest isolation (done / declined) - `test_buffer.py`: each permutation now builds a fresh `SpansBuffer`, which resets `any_shard_at_limit` and the cached properties. The options and Redis setup stay shared. I re-checked with reverse permutation order and an injected early failure: exactly the injected subtest failed and the rest passed. The redis-cluster variants ran green in the CI run below. - `test_client_config.py`: declined. Each factory clears caches and builds its own users and orgs, and the org/project lookups depend on the request. I found no shared global query or cache, and reverse order already passed all 758 subtests. - Declined: documenting how to reproduce a single subtest. There's no natural place for it, and subtests can't be selected by nodeid. ### #127943: sharding boundaries (done) - Absolute roots, `..` roots, and `--rootdir` or `-c`/`--config-file` (in the args or `PYTEST_ADDOPTS`) produce paths that don't match the sentry plugin's rootdir-relative comparison. `pytest-selected.py` now falls back to the legacy hash sharding for them. `pytest_shard_plugin` raises a usage error if the rootdir isn't the working directory. CI passes the relative root `tests` and keeps the fast path. - Files outside `tests/` were not duplicated across shards. `pytest_collection_modifyitems` already drops them by `SELECTED_TESTS_FILE`; they are only imported. A new test covers this. - `tests/sentry/testutils/pytest/test_sentry.py` runs the real `pytest-selected.py`, sentry plugin and shard plugin over a throwaway project with 3 shards. It asserts that the `Counter` of unsharded nodeids equals the sum of the shard Counters. Cases: relative root, a split file, parametrized and inherited tests, an ignored dir, a root outside `tests/`, absolute root and `--rootdir` (fallback), and an actual `-n 2` run. - Duration files now record schema version, unit, commit, run id, attempt, shard index and shard count. `merge` rejects mixed runs or attempts, missing or duplicate shards, missing provenance, and negative or non-finite values. It writes a `source` block into the merged file. The checked-in weights were not regenerated. - A subtest loop is one pytest item, so `file_weight`/parts can't split it. ### CI timing These numbers come from a full `workflow_dispatch` backend run on this branch (22 shards, no selection, 22/22 green), taken before the `-m`/`-k` fix was added. That fix doesn't touch sentry's own CI path. They're compared with the master push run for a636572, the only master run that includes all five PRs. Each value is the "Run backend test" step: summed across shards, then the median and the slowest shard. | Run | Sum | Median shard | Max shard | |---|---|---|---| | master a636572 ([39051886634](https://github.com/getsentry/sentry/actions/runs/39051886634)) | 128.5 min | 365 s | 436 s | | this branch ([39056799745](https://github.com/getsentry/sentry/actions/runs/39056799745)) | 135.8 min | 378 s | 429 s | The slowest shard didn't change. The summed time is +7.3 min (+5.7%), but that is runner noise, not the lock. Comparing each shard's per-file durations, the 72 files with transactional tests moved +2.6% (+0.9 min), and the 2,452 files that never reach the flush code moved +5.6% (+16.6 min). The four new test files add 46 s in total. At 10–20 ms each across roughly 1,200 transactional tests, the lock costs well under a minute over the whole run, a small fraction of #127941's saving. Master runs before these PRs varied by ±10 min in summed time. <!-- junior-request-attribution:start --> via @joshuarli. <!-- junior-request-attribution:end --> <!-- junior-session-footer:start --> <!-- junior-conversation-id:agent%3Aaca1f40f54b2553bb3f2b5aa308eca88 --> -- [View Junior Session](https://junior-prod.sentry.dev/conversations/agent%3Aaca1f40f54b2553bb3f2b5aa308eca88) [[Sentry]](https://sentry.sentry.io/explore/conversations/agent%3Aaca1f40f54b2553bb3f2b5aa308eca88/?project=4510944073809921) <!-- junior-session-footer:end --> --------- Co-authored-by: sentry-junior[bot] <264270552+sentry-junior[bot]@users.noreply.github.com> Co-authored-by: Joshua Li <14209781+joshuarli@users.noreply.github.com>
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
1 participant
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.
On full backend runs (master pushes, the override label, full-suite triggers), every xdist worker in every shard collects the whole suite (~46.9k tests) and the pytest plugin then hash-deselects ~95% of it. That costs 73–146s per worker, about a quarter of all worker time. Locally, collecting everything takes ~105–135s; collecting one shard's files takes 2.5–7.5s.
pytest-selected.pyalready avoids this on selective runs. This PR adds a full-run path to the same shim:.pyfile under the pytest roots (minus--ignore) and weights each file using the checked-inbackend-test-durations.json. Files that file doesn't cover yet fall back to their AST test count.plan_full_shardscuts files heavier than 1/12 of a shard into equal parts, each on a different shard. It then places parts and whole files heaviest first onto the lightest shard. Each shard only gets its own files throughSELECTED_TESTS_FILE, so the sentry plugin skips the other files at collection, as it already does on selective runs.pytest_shard_plugin.py, keeps this shard's part of each split file (by nodeid hash, like today's split) and runs files slowest first.--dist=loadfilehands files out in collection order, so this keeps a slow file from starting last on a worker. The plugin is loaded throughPYTEST_PLUGINSso the xdist workers load it too.backend-test-durations-*artifacts.backend-test-durations.py mergeturns them into the checked-in file.Determinism. The plan depends only on the checkout (file tree + checked-in weights), with sorted tie-breaks. The same commit always gets the same shard layout and the same order within each shard. No worksteal, no dynamic scheduling, and
--dist=loadfilestays.Which tests share a shard changes. Today each shard gets a pseudo-random ~1/22 of every file. With this change, most files run whole on a single shard, next to a different set of neighbors, and files run slowest first. Tests that depend on test order or on shared state across files could pass or fail differently than they do now. Please weigh this against the known flaky tests.
Coverage. File listing errs wide: non-test
.pyfiles are harmless because pytest doesn't collect them either way. I ran realpytest --collect-onlyfor all 22 shards locally, and the union matched the full collection exactly: 46,855 tests, no duplicates, none missing.test_every_test_runs_exactly_oncechecks the same property for the planner and the part hashing.Unchanged. Selective runs (
SELECTED_TESTS_FILEset) keep the existingplan_shardspath and behave exactly as before. If the durations file is missing or a shard would come out empty, the shim falls back to today's hash split.Why checked-in weights. AST counts badly underestimate parametrized and inherited tests:
test_organization_agent_token.pyis estimated at 45 tests but has 1,364. A collect-only pass inselect-testswould be accurate, but that job is skipped on master and every shardneedsit, so it would add minutes to the critical path. Checked-in weights cost nothing at runtime and stay fixed per commit. If they go stale, only balance suffers, never correctness. The weights currently in the file are real collected test counts. Duration-based weights from a CI run on this branch will replace them before this leaves draft.via @joshuarli.
--
View Junior Session [Sentry]