SEP-1934: Tolerate concurrent scaffold churn in the import-boundary walk - #1439
Merged
Conversation
The scaffolder writes real packages into app/sep/apps/ by design, and the scaffold tests remove them in teardown. Under xdist those tests can land on a different worker than the import-boundary guards, which walk the same tree, so a path listed a moment earlier can be gone by the time it is read. The guards then failed with a FileNotFoundError that said nothing about a real boundary violation. Route every disk walk through one tolerant parse helper that catches FileNotFoundError only: a path that no longer exists is not a module to check, while a decode or parse failure still raises rather than silently narrowing the guard. Give the import-time collector the same injectable-modules seam the deferred collector already had, which removes a duplicated read-and-parse loop and lets the rule be driven over a synthetic violating tree instead of only a green one. Extract the orchestrator scan into its own collector on the same helper, and make the apps-tree coverage comparison ordering-safe by sampling the narrow walk first, the wider one second, and dropping paths that no longer exist. No name-based skip: scaffold package names are not uniformly underscore- prefixed, the same prefix would also match app/sep/apps/__init__.py, and a name skip would leave a real module unchecked.
marcuscruz-percona
requested review from
peter-o-addo and
yyyyyyyan
as code owners
September 1, 2026 16:11
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The rglob() traversal can still raise when a scaffold directory disappears before parsing begins.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Hardens import-boundary tests against concurrent scaffold creation and removal.
Changes:
- Centralizes tolerant module parsing.
- Adds synthetic violation and race regression tests.
- Makes coverage and orchestrator scans ordering-safe.
File summaries
| File | Description |
|---|---|
tests/app/sep/test_import_boundary.py |
Adds race-tolerant collectors and regression coverage. |
Review details
Suppressed comments (3)
tests/app/sep/test_import_boundary.py:504
- Use em dashes in this rendered prose instead of the two literal
--sequences.
collector, so the rule the live-tree case asserts vacuously -- a green tree yields
an empty list either way -- is pinned against a tree that does violate it. The
tests/app/sep/test_import_boundary.py:754
- Use an em dash here;
--is rendered as two literal hyphens in the docstring.
real module behind it -- which is what the concurrent-scaffold failure looked like.
tests/app/sep/test_import_boundary.py:376
- Use an em dash here;
--remains two literal hyphens in rendered documentation.
gone -- a permission failure, say.
- Files reviewed: 1/1 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ished read The tolerant read closes the race only on Python 3.13, where the glob machinery happens to swallow OSError while recursing. This project supports 3.11.9 and up and CI runs 3.11.9, whose _RecursiveWildcardSelector._iterate_directories guards its scandir with "except PermissionError" alone, so a scaffold package removed after it was listed but before the descent reaches it aborts the whole walk before any read happens. Route every walk through one os.walk-based collector whose onerror handler skips a directory that is gone and re-raises anything else, so the tolerance no longer depends on which interpreter runs the suite. Symlinked directories stay unwalked, matching the previous behaviour. Also correct the dangling-symlink case's docstring: the scaffold test it cited symlinks the app directory, not a module, so the claim that a walk yields that exact shape was wrong. The case still pins what it always pinned, that a read behind a link with no target is tolerated. Use em dashes in the prose the file adds, matching the rest of the tree.
yyyyyyyan
approved these changes
Sep 2, 2026
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
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.
Summary
FileNotFoundErroronly, so a path that vanished between listing and reading is treated as "not a module to check" instead of failing the run with an error that says nothing about a real boundary violation. A decode or parse failure still raises, so the guard cannot silently narrow to whatever happens to read cleanly. No name-based skip: scaffold package names are not uniformly underscore-prefixed, the same prefix would also matchapp/sep/apps/__init__.py, and a name skip would leave a real module unchecked.rglob+read_textinline in the test body, now extracted into a collector on the same helper; the apps-tree coverage assert compared a fresh walk against a stale module-scoped fixture, and is now ordering-safe (narrow walk sampled first, wider walk second, non-existent paths dropped), so neither half of a scaffold test's churn can fail it.modulesseam the deferred collector already had. That removes a duplicated read-and-parse loop and letstest_no_module_imports_another_app_package's rule be driven over a synthetic violating tree — previously it could only ever be asserted against a green one, so it passed vacuously.OSErrorwhile recursing, but CI runs 3.11.9 and_RecursiveWildcardSelector._iterate_directoriesthere guards itsscandirwithexcept PermissionErroralone — so a package removed after it was listed aborts the walk before any read happens. Every walk now goes through oneos.walk-based collector whoseonerrorhandler skips a directory that is gone and re-raises anything else; a directory we may not read still fails loudly. Symlinked directories stay unwalked, matching the previous behaviour.test_scaffold.pyare unchanged, and no scheduler setting moved.Tested
Baseline first, on the unfixed file: 10 rounds of the two racing files (5
loadscope+ 5loadfile) produced 22FileNotFoundErrors across 9 failing rounds, hitting all four sites — including the orchestrator scan and the coverage assert.The 3.11 window is reproduced directly: running that interpreter's algorithm verbatim under a
scandirwrapper that removes the doomed directory right after the parent is listed raisesFileNotFoundError: [Errno 2] No such file or directory: .../doomed, while the replacement walker completes.Negative control: strip the tolerant read and keep the new tests, and 6 of them fail with exactly the reported
FileNotFoundError; restore it and they pass. They are guarding something.make test(worksteal, the adopted scheduler): 11361 passed, 434 skipped, 0FileNotFoundErrortest_import_boundary.py+apps/framework/test_scaffold.py(10loadscope, 10loadfile): 20/20 exit 0, 0FileNotFoundErrortests/app/sepunderloadscopeand again underloadfile: 6924 passed, 387 skipped each, 0FileNotFoundErrortests/app/sep/test_import_boundary.py: 85 passedmake lint: cleanRepeated green runs are evidence, not proof — this race is latent under
workstealrather than closed by it, which is why the fix targets the observer instead of the scheduler. The remaining consecutive-run evidence is cheap to add if a reviewer wants it.Checklist
make test)make run-pre-commit)make makemigrations) — N/A, no model changechangelog.d/if the change is user-facing (make changelog-add), or confirmed N/A (internal-only change) — N/A, test-only