CI: --dist loadscope in ci.yml and added test_xdist_isolation

[sujalgawas]

Summary

Closes #651

Updated the CI pytest command to use --dist loadscope with pytest-xdist.

The reason for using --dist loadscope is fixture-scoping correctness and test isolation, rather than a CI speed improvement.

Some tests use module-scoped fixtures. When pytest-xdist distributes tests across workers, tests that rely on the same module-scoped fixtures can be distributed in a way that causes the fixtures to be instantiated separately across workers.

Using:

pytest -n auto --dist loadscope

keeps tests from the same module together on the same worker, which preserves the expected module-level fixture scope and avoids the isolation issues demonstrated by test_xdist_isolation.py.

I also added test_xdist_isolation.py to demonstrate why --dist loadscope is needed when running these tests in parallel.

I did not use xdist-group because the relevant isolation requirement is at the module/fixture scope.

pytest-xdist was already included in [dev], so no dependency changes were necessary.

Testing

The following command was run multiple times to verify the behavior:

pytest -n auto --dist loadscope tests/unit/ tests/synthetic/ tests/agents/ tests/integration/

The same test suite was also run without --dist loadscope to demonstrate the difference in behavior:

pytest -n auto tests/unit/ tests/synthetic/ tests/agents/ tests/integration/

pytest -n auto --dist loadscope tests/unit/ tests/synthetic/ tests/agents/ tests/integration/

pytest -n auto tests/unit/ tests/synthetic/ tests/agents/ tests/integration/

after creating test_xdist_isolation.py

pytest -n auto --dist loadscope tests/unit/ tests/synthetic/ tests/agents/ tests/integration/

pytest -n auto tests/unit/ tests/synthetic/ tests/agents/ tests/integration/

Checklist

• Tests pass (pytest tests/unit/ tests/synthetic/ tests/agents/ tests/integration/)
• Lint clean (ruff check tokenjam/)
• Type check clean (mypy tokenjam/)
• CLAUDE.md updated (if architecture changed)
• Test spans use tests/factories.py (not raw NormalizedSpan)
• Requested @anilmurty as reviewer (or @-mentioned him above)

[greptile-apps]

Greptile Summary

The PR configures CI to keep tests from each module on one xdist worker and adds an integration test demonstrating the fixture-isolation requirement.

• Runs the Python test matrix with --dist loadscope.
• Adds OpenTelemetry collector and in-memory DuckDB isolation checks.
• Updates the CI documentation to match the workflow.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
.github/workflows/ci.yml	Adds --dist loadscope to the parallel pytest invocation so module tests execute on the same worker.
tests/integration/test_xdist_isolation.py	Adds integration coverage for module-local OpenTelemetry state and a fresh module-scoped in-memory DuckDB backend; the previously reported cross-run stale-row issue is fixed.
.claude/rules/release.md	Updates the documented CI test command to include the new xdist distribution strategy.

_{Reviews (7): Last reviewed commit: "Merge branch 'main' into speed-up-tests" | Re-trigger Greptile}

[anilmurty]

@sujalgawas thanks for the careful work here, and especially for the repeat runs and screenshots. Your diagnosis of why loadscope matters for DuckDB isolation is sound. The speed premise underneath it isn't, and I want to walk through what I measured before we decide what to do with this.

I ran both arms in a scratch worktree on identical product code, warm caches, same test set with your new file excluded from both:

-n auto (default `load`)        60.04s
-n auto --dist loadscope        62.12s

So loadscope is about 3% slower, not faster. I think the 153s → 68s you saw was a cold-cache first run: my own cold run gave 153.0s and the warm repeat of that same command gave 61.5s. loadscope constrains the scheduler's ability to balance work across workers, which makes it a stability lever rather than a speed one.

Also relevant: issue #651 is partly stale. It says "no pytest-xdist, no -n", but -n auto already landed on main and is where the 7min → ~1min win came from. This PR changes the distribution mode on top of that.

On the new test file, three problems:

1. _SHARED_DB_PATH is a fixed filename in tempfile.gettempdir(), shared by every pytest run on the machine and never cleaned. I ran two pytest invocations of that file concurrently: one passed, the other gave 7 failed / 3 passed. CLAUDE.md's "Working with concurrent agents" section requires parallel agents in parallel worktrees on one machine, so this collides with our own documented workflow.

2. It passes on stale state. Rows accumulate across runs. After two runs the file held 16 rows; I then ran test_duckdb_all_writes_completed alone, with zero writes in that run, and it passed. Greptile flagged this as P2; combined with the concurrency failure I'd call it blocking. We have a standing rule about this (Critical Rule 23) because a test that's green while enforcing nothing is worse than no test.

3. It hard-codes ~2.9s of time.sleep into every CI run of every future PR, in a PR whose purpose is to reduce CI time. It also asserts nothing about TokenJam behavior; it tests pytest's scheduler, and it was written to fail under load so that it justifies the ci.yml change in the same PR.

You flagged the uncertainty yourself in the body ("its on maintainers if its valid to have this test or not"), which I appreciate. My answer: the ci.yml change is worth keeping, framed honestly.

What I'd merge:

1. The one-line --dist loadscope change, described as scheduling stability rather than speed.
2. .claude/rules/release.md:84 updated to match. It documents the test command verbatim and goes stale on merge.
3. The demonstration test dropped, or rewritten against tmp_path_factory so it asserts on the current run's writes only.
4. Closes #651 in the PR body. Right now it's a bare "Summary Speed up CI: test matrix takes ~7-12 min (serial tests, no parallelism) #651" heading, so the issue won't auto-close.

One note in your favor that I couldn't fully confirm: test_quickstart_reads_no_config_and_opens_no_ondisk_db failed once under load and never under loadscope. That might be the real cross-worker flake you're describing. I saw it once in a cold run, so I won't claim it as evidence, but it points the right direction.

Your job names and the four required contexts are byte-identical to main, so branch protection is safe. I checked that before anything else.

[sujalgawas]

@anilmurty

1. rather then using fixed path I have just created shared_db in the test file
2. removed time.sleep()
3. changed .claude/rules/release.md:84 file
4. Closed issue 651

[anilmurty]

@sujalgawas the rewrite fixed the things that mattered — thanks for turning it around. The shared /tmp path is gone, the sleeps are gone, release.md matches, and Closes #651 is linked. I re-ran it and the test still discriminates, which was the thing I most wanted to check, since a rewrite could easily have made it pass under both modes and prove nothing:

--dist loadscope   10 passed
--dist load         2 failed, 8 passed

I also owe you a correction. I said the new test only exercises pytest's scheduler and not TokenJam. That's literally true, but I framed it as a reason to drop the test, and I was wrong about the conclusion. The pattern it simulates is real in this repo: there are 13 module- and session-scoped fixtures across 9 files, including tests/conftest.py, test_storage_backend_parity.py and six Lens UI tests. Under --dist load those get split across workers and the fixture is rebuilt per worker. That's a genuine correctness argument for loadscope, and neither of us had made it — the issue framed this as speed, which is why the speed measurement kept coming up short.

So the change should land. The framing is what needs fixing, not the change.

The measurement still stands, for the record — warm caches, same test set, both arms:

-n auto (default load)     60.04s
-n auto --dist loadscope   62.12s

loadscope is not faster. It's more correct for our fixtures, which is a better reason than the one in the issue.

What's left, all small:

1. Reframe the PR body (and ideally the commit subject) as fixture-scoping correctness rather than a speedup. Cite the module-scoped fixtures. If someone later reads "speed up CI" and benchmarks it, they'll conclude it did nothing and revert it — the honest justification is the one that survives.
2. Four unused imports in tests/integration/test_xdist_isolation.py: os, tempfile, DuckDBBackend, StorageConfig. Ruff flags all four (F401); CI only lints tokenjam/ so it won't fail the build.
3. Two leftovers from the rewrite — the db fixture at line 36 is unused, and _init_shared_db is now an autouse fixture whose body is pass. Both can go.
4. The docstrings still describe the old file-locking behaviour — IOException: Conflicting lock is held, "holds it for 250 ms", "opens the same file". None of that can happen with the in-memory fixture. A comment describing machinery that no longer exists actively misleads whoever reads this next, so it's worth a pass.
5. Missing newline at end of file.

None of that is structural. Do those and I'll merge it.

One more thing worth flagging, since you'd hit it: merging this pins CI to a flag that your test requires. If someone later removes --dist loadscope, they get 4 failures that look like a product regression rather than a config change. A line in the test's module docstring saying it is asserting the CI flag, and will fail without it, would save that person an hour.

[sujalgawas]

@anilmurty

made the requested change.

I was trying to change the branch name but it just kept getting deleted thats why the PR is closed and opened mutliple times sorry for that.

[anilmurty]

no worries. thanks for the updates and for the contribution @sujalgawas - reviewing and merging now

[anilmurty]

Thanks @sujalgawas — merging. You addressed every point, and the module docstring note is better than what I asked for: telling the next person that a --dist change will surface here as four product-looking failures will save someone real time.

Verified before merging rather than trusting the green checks:

• ruff check on the file is clean — all four unused imports gone, along with the dead db fixture and the no-op _init_shared_db.
• The test still discriminates: 10 passed under --dist loadscope, 2 failed under --dist load. A rewrite that quietly stopped proving anything was the main risk here, and it doesn't.
• Full suite under the new flag: only the three known summarize failures that also occur on main under an isolated HOME. Nothing new.
• This CI run genuinely exercised the change, since ci.yml comes from the branch on pull_request — so the flag is proven in the real job, not just locally.

The reframe is the part that matters most. "Fixture-scoping correctness" is the justification that survives someone benchmarking this later and finding no speedup; "speed up CI" would have gotten it reverted. Thanks for taking that on rather than defending the original framing.

One cosmetic leftover, not worth another round: test_duckdb_concurrent_writer_lock still has "lock" in its name, though there's no file lock anymore. Rename it if you touch the file again.