Skip to content

UN-3636 [PERF] Stop the test rig wasting CPU and hashing time#2195

Draft
chandrasekharan-zipstack wants to merge 10 commits into
mainfrom
feat/rig-extra-manifests
Draft

UN-3636 [PERF] Stop the test rig wasting CPU and hashing time#2195
chandrasekharan-zipstack wants to merge 10 commits into
mainfrom
feat/rig-extra-manifests

Conversation

@chandrasekharan-zipstack

Copy link
Copy Markdown
Contributor

What

Three independent, test-only changes to how the rig runs the backend suites. No product code touched.

Change Where
Resolve the xdist worker count in the rig instead of deferring to -n auto tests/rig/cli.py
--no-migrations backend/pyproject.toml
MD5 password hasher backend/backend/settings/test.py

Why

Worker count. -n auto calls xdist's pytest_xdist_auto_num_workers, which prefers psutil.cpu_count(logical=False) and only falls back to sched_getaffinity when psutil is absent. psutil is installed in exactly the workers and backend venvs, and it reports 1 physical core on the CI runner. Result: the three largest suites — unit-workers (723 tests), unit-backend, integration-backend (694) — ran single-threaded, while every group without psutil got 2. Only gw0/gw1 appear anywhere in the CI logs.

-n logical would fix CI but oversubscribe developer machines. Measured on a 12-core box, integration-backend: 2w=95.1s, 4w=79.4s, 8w=89.4s, 12w=121.4s. So the rig counts usable CPUs itself and caps at 8.

Migrations. pytest-django replays the full migration history into a fresh database per xdist worker. Building the schema from the models instead is worth ~51% of integration-backend.

Hasher. No PASSWORD_HASHERS override existed anywhere under backend/settings/, so every create_user() in a fixture paid Django's default 600k-iteration PBKDF2 — ~120ms a call, and the permissions/owner-management suites seed six users per test.

Measured

integration-backend on the cloud-merged tree, 4 workers, 672 passed identical in every arm:

Config pytest time
baseline 163.3s
+ --no-migrations 79.4s
+ MD5 hasher ~52s

Verification

On this branch, OSS tree:

  • tox -e groups -- integration-backend104 passed, 26 skipped, 1 xfailed in 43.8s
  • tox -e groups -- unit-backend163 passed in 15.1s
  • tox -e groups -- unit-rig86 passed

Note

Cloud CI checks out this repo at ref: main, so none of this reaches the cloud pipeline until it merges. The hasher override also has to be repeated in the cloud test settings (they derive from backend.settings.cloud, not from backend.settings.test) — Zipstack/unstract-cloud#1665.

🤖 Generated with Claude Code

https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz

chandrasekharan-zipstack and others added 10 commits July 21, 2026 16:58
load_groups() now merges extra group manifests listed in the env var
(os.pathsep-separated, REPO_ROOT-relative) onto the base tests/groups.yaml
before validation, so cross-manifest depends_on and the platform-gate
invariant are checked over the union. Name collisions are an error.

Lets a downstream repo (the cloud build) contribute its own test groups by
copying a groups.cloud.yaml into the merged tree, without editing the OSS
manifest.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz
UserContext.get_organization() ran Organization.objects.get() even with no
org id in StateStore (import time, or management commands with no request),
catching only DoesNotExist/ProgrammingError — a DB-less/unmigrated setup hit
an uncaught OperationalError. Short-circuit when there's no org id: no query,
so serializers/managers that reference org-scoped querysets at class-def can
be imported during DB-free test collection.
Address review feedback on the UNSTRACT_RIG_EXTRA_MANIFESTS overlay:

- Overlays now apply only when loading the default manifest, so an
  explicit `load_groups(path)` (test fixture, ad-hoc manifest) can no
  longer absorb a downstream repo's ambient overlay.
- `_merge_manifest` returns the merged defaults so an overlay can rename
  `platform_gate_group` instead of having it silently ignored.
- A bad path in the env var raises a ValueError naming the variable
  rather than a bare FileNotFoundError/IsADirectoryError.
- Extract `_load_manifest_dict` to single-source manifest parsing and its
  error message.
- Tests: drive the real default-manifest path; cover overlay isolation,
  overlay defaults, malformed overlay, and a missing overlay path.
- Pin the truthy branch of UserContext.get_organization so inverting the
  guard can't pass unnoticed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz
`--tier X` expanded each selected group's `depends_on` transitively, with
no tier bound. `integration-workflow-execution` and `e2e-smoke` both
declare `depends_on: [unit-sdk1, unit-workers]`, so those two unit groups
ran again in the integration leg and a third time in the e2e leg.

Tiers run as separate CI legs and the unit leg already covers them, so
dep expansion is now bounded to the requested tier. Explicitly named
groups are never dropped, and intra-tier deps (e2e-smoke -> e2e-login)
still expand and order as before. Unrun deps do not weaken gating:
`blocked_by` intersects with groups that failed in the same run.

Measured on the last main run: ~88s of unit-workers and ~48s of
unit-sdk1 re-executed per run. On the cloud CI runner the same
duplication costs ~350s.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz
* UN-3636 [FIX] Drop the ENVIRONMENT gate on the LLM mock

It did not defend the case it was added for. The threat was a worker env block
copied out of the test overlay into a real deployment, but the gate was written
into that same block, so a copy carries it. Base compose also sets
ENVIRONMENT=development on both workers that run the injection, so any
deployment derived from it satisfied the gate regardless. That left one real
case -- the mock var set alone somewhere that sets no ENVIRONMENT at all --
which holds by accident rather than design, in exchange for depending on a
variable nothing else in the codebase reads.

What actually guards the hatch is unchanged: it is off unless someone sets
UNSTRACT_LLM_MOCK_RESPONSE, and it warns once per process while active. Making
mocked spend distinguishable downstream is the defence worth having, and it
belongs on the usage record rather than in a config check.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* UN-3636 [MISC] Drop the unread ENVIRONMENT variable from compose

Nothing reads it: no service, worker, frontend or plugin looks the variable
up, and the one consumer it ever had — the LLM mock gate — was removed in the
previous commit. Dropping it everywhere keeps a dead knob from looking
load-bearing to the next reader.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ryg9chVDJQggCybpq3YoY3

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Drop lines that restate the code, trim session-specific detail, and merge
comments that duplicated each other across a module and its test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz
`load_groups(DEFAULT_MANIFEST)` merged overlays even though the caller named a
manifest explicitly, because the check compared path values. Path equality is
also spelling-sensitive, so the same file relative and absolute behaved
differently. Key on `path is None` instead: an explicit path loads exactly what
it names.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ryg9chVDJQggCybpq3YoY3
TestIdePromptComplete drives the full success path, which reaches
client_plugin_registry.get_client_plugin("subscription_usage"). In OSS no
such plugin is installed, so the lookup returns None instantly. In a tree
with the cloud plugins copied in, it resolves to a real plugin that POSTs
to the backend; with no backend running the call only fails after a
multi-second connect timeout, and _track_subscription_usage swallows the
error so the tests still pass. That accounted for ~190s of the cloud
unit-workers run.

The file already declared _PATCH_GET_PLUGIN but never applied it outside
the dedicated subscription-usage classes. Apply it as a class-scoped
fixture so the lookup is pinned to the OSS answer.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz
Three independent wins measured on the cloud-merged tree:

- `-n auto` collapsed to a single worker on any group shipping psutil,
  because xdist prefers physical cores there. Resolve the count in the
  rig instead, capped at 8 to avoid contending on the test database.
- `--no-migrations` builds the schema from the models rather than
  replaying the full migration history once per xdist worker.
- Test fixtures were paying 600k-iteration PBKDF2 per seeded user.

integration-backend fell from 163s to ~52s with an identical result set.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RnLaN45ShBbcCXWZkqCThz
@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 632881a5-a6ac-43b6-ac03-16a80d4e10a5

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/rig-extra-manifests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sonarqubecloud

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant