From 9e940e644d51a79b1a57a7ee27f489c70044dafd Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 6 Nov 2026 11:10:56 +0100 Subject: [PATCH 1/3] test: fix three order-dependent/flaky tests and drop the LEGACY_FLAT ratchet - users (#860): the password-offload test patched the shared PASSWORD_HELPER instance; monkeypatch's undo leaves a bound method in its __dict__ that shadows later class-level patches. Patch the class instead and assert the shared helper carries no instance attributes. - background_tasks (#861): "" is the intended "no owner" reset (it keeps a headerless worker task from adopting an ambient user, #759); document it, prove a stranded prerun never reaches the next task's owner, and unwind any stranded task binding after each test. - tenants (#855): on SQLite a losing invite can also get the retryable admission_conflict under lock contention; accept either refusal there, still requiring exactly one success and one invitation row. - Every module now follows the layout: remove LEGACY_FLAT and its dead checks, and update docs/module-layout.md and CLAUDE.md. Claude-Session: https://claude.ai/code/session_01FXHA3ZicrtZ4C5QPEgxfgq --- CLAUDE.md | 9 +++--- docs/module-layout.md | 17 +++++----- host/tests/test_module_layout.py | 20 ++---------- .../executions/tenant_context.py | 5 ++- modules/background_tasks/tests/conftest.py | 14 +++++++++ .../test_background_tasks_tenant_context.py | 31 +++++++++++++++++++ .../core/service/test_tenants_owner_race.py | 8 ++++- .../test_users_password_offload.py | 19 +++++++++--- 8 files changed, 86 insertions(+), 37 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index e619b2d84..33e26662c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -135,9 +135,10 @@ The Python package name (`feature_flags`) keys the i18n namespace, `app.state.` and table prefixes. Don't swap them: `keys.` breaks if the namespace is the meta name. -**Module layout.** New modules use the area layout below; existing modules -migrate one by one. See [Module layout](docs/module-layout.md) for root lookup consumers, -tests, compatibility shims and the migration checklist. +**Module layout.** Every module uses the area layout below +(`host/tests/test_module_layout.py` enforces it). See [Module layout](docs/module-layout.md) +for root lookup consumers, tests, compatibility shims and the checklist for moving a +downstream flat module. ```text modules/// @@ -351,7 +352,7 @@ development and testing (`session_cookie_secure`). - **No magic strings in non-test Python** (`scripts/check_hardcoded_strings.py`): `RequiresPermission(...)`, `registry.map_role(...)`, `registry.add_group(...)` items, `inertia.render(...)` keys and `depends_on=[...]` entries must be - constants from `core/constants.py` (legacy modules: `constants.py`): + constants from `core/constants.py`: `PERM_*`, role names, `_PAGE_*`, `_MODULE_*`. - **300-line cap** on `.py`/`.ts`/`.tsx`, tests included, untracked files included (`scripts/check_file_size.py`). Exempt: shadcn components under diff --git a/docs/module-layout.md b/docs/module-layout.md index 0eec7c497..11d224708 100644 --- a/docs/module-layout.md +++ b/docs/module-layout.md @@ -1,7 +1,8 @@ # Module layout -New modules use this layout. Existing modules under `modules/` migrate in -separate PRs, one module at a time. No existing module moves in PR 0. +Every module under `modules/` uses this layout; the last one moved in #862. +A downstream module still on the flat layout moves as described in +[Moving an existing module](#moving-an-existing-module). The downstream `smpy_modules` layout is the reference for areas; this repo keeps its own pytest naming and collection conventions. @@ -72,7 +73,6 @@ The 300-line cap still applies, including tests; split by responsibility. The root ratchet permits `core/`, `web/`, `ui/` and the lookup paths above. Declare a new multi-feature domain area in its `DOMAIN_AREAS` map when needed. -Do not expand `LEGACY_FLAT` to accommodate new code. ## Frontend @@ -118,8 +118,7 @@ Mirror package ownership, using `tests///test__*.py`. Keep module-wide `conftest.py` at `tests/` root. Do not add `__init__.py` in test directories. Basenames must be globally unique: default pytest import mode loads non-package tests by basename, even when they live in different folders. -Existing flat test names are grandfathered only while their module remains -in `LEGACY_FLAT`; migration renames them with the module prefix. +The root-layout test refuses a `test_*.py` directly under `tests/`. A helper shared by several test folders stays at `tests/` root as `__support.py`. The root `conftest.py` puts that directory on @@ -163,14 +162,12 @@ sys.modules[__name__] = importlib.import_module("orders.web.service") The alias makes both paths the same module object, so mocks and `monkeypatch` strings keep working. Files containing both `deprecated_module(` and `sys.modules[__name__]` are auto-discovered by the expiry test and ignored -by the root-layout test, so the module can leave `LEGACY_FLAT` while they -remain. When the old path was inside a package that moved (for example +by the root-layout test while they remain. When the old path was inside a package that moved (for example `orders.endpoints.api`), keep that package with a docstring-only `__init__.py` and alias each module in it; a package holding only aliases is ignored by the root-layout test too. A shim that is a plain re-export instead (not an alias) must be added -by hand to the expiry test's `_SHIM_MODULES` tuple and `_EXPECTED` set and kept -in the module's frozen legacy entries until removed. A completely migrated -module leaves `LEGACY_FLAT` once its Python tests are nested. +by hand to the expiry test's `_SHIM_MODULES` tuple and `_EXPECTED` set and to +the root-layout test's `DEPRECATED_ROOT` map until removed. ## Moving an existing module diff --git a/host/tests/test_module_layout.py b/host/tests/test_module_layout.py index 618b73cc5..0466e4af0 100644 --- a/host/tests/test_module_layout.py +++ b/host/tests/test_module_layout.py @@ -52,9 +52,6 @@ "settings": frozenset({"hydrate.py", "module_registry.py", "registration.py"}), } -# Frozen inventory, not a computed baseline: moves must remove their old entries. -LEGACY_FLAT: dict[str, frozenset[str]] = {} - def _is_layout_shim(path: Path) -> bool: """A moved module's old path: a deprecated alias, allowed until its remove_in.""" @@ -93,23 +90,12 @@ def test_module_root_layout(module: str) -> None: f"{module}: remove expired DEPRECATED_ROOT entries" ) allowed = ALLOWED_ROOT | DOMAIN_AREAS.get(module, frozenset()) | deprecated - legacy = LEGACY_FLAT.get(module, frozenset()) - if module in LEGACY_FLAT: - assert legacy, f"{module}: remove the empty LEGACY_FLAT entry" - assert not entries - allowed - legacy, ( - f"{module}: unexpected roots {entries - allowed - legacy}" - ) - assert legacy <= entries, f"{module}: remove stale LEGACY_FLAT entries {legacy - entries}" - assert not legacy & allowed, f"{module}: remove allowed entries from LEGACY_FLAT" + assert not entries - allowed, f"{module}: unexpected roots {entries - allowed}" if (package / "cli.py").is_file(): project = tomllib.loads((MODULES / module / "pyproject.toml").read_text())["project"] targets = [ value for group in project.get("entry-points", {}).values() for value in group.values() ] assert any(value.split(":")[0] == f"{module}.cli" for value in targets) - if module not in LEGACY_FLAT: - assert not list((MODULES / module / "tests").glob("test_*.py")), module - - -def test_legacy_modules_still_exist() -> None: - assert all((MODULES / name / name).is_dir() for name in LEGACY_FLAT) + # Tests mirror the package areas: tests///test__*.py. + assert not list((MODULES / module / "tests").glob("test_*.py")), module diff --git a/modules/background_tasks/background_tasks/executions/tenant_context.py b/modules/background_tasks/background_tasks/executions/tenant_context.py index 710afd2cc..39ea7c038 100644 --- a/modules/background_tasks/background_tasks/executions/tenant_context.py +++ b/modules/background_tasks/background_tasks/executions/tenant_context.py @@ -174,7 +174,10 @@ def restore_tenant(*, task_id: str | None, task: Any) -> None: # context, so without headers it keeps the caller's tenant and owner. eager = bool(getattr(getattr(task, "request", None), "is_eager", False)) tenant = _tenant_of(task) or (current_tenant_id.get() if eager else _default_tenant) - owner = message_owner(task=task) or (_task_owner.get() if eager else "") # "": no owner + # ``""`` (not ``None``) means "no owner" and stops :func:`stamp_owner` + # falling through to an ambient ``current_user_id``. Every prerun sets it + # afresh, so a stranded binding never reaches the next task's messages. + owner = message_owner(task=task) or (_task_owner.get() if eager else "") owner_token = _task_owner.set(owner) tenant_token = current_tenant_id.set(tenant) _tokens.set((*_tokens.get(), (task_id, tenant_token, owner_token))) diff --git a/modules/background_tasks/tests/conftest.py b/modules/background_tasks/tests/conftest.py index dd4d046c7..3d6563c99 100644 --- a/modules/background_tasks/tests/conftest.py +++ b/modules/background_tasks/tests/conftest.py @@ -11,6 +11,7 @@ import pytest from background_tasks.core import sync_db from background_tasks.core.constants import PERM_VIEW, TaskStatus +from background_tasks.executions import tenant_context from background_tasks.models import TaskExecution from sqlalchemy import select @@ -27,6 +28,19 @@ def _no_worker_audit_sink(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setattr(sync_db, "discover_audit_callback", lambda: None) +@pytest.fixture(autouse=True) +def _unwind_stranded_task_bindings() -> Iterator[None]: + """Leave no task tenant/owner binding behind for the next test (#861). + + A test that runs a prerun without its postrun would otherwise leave the + owner pinned to "no owner" for every later test in this context. + """ + yield + stack = tenant_context._tokens.get() + if stack: + tenant_context.release_tenant(task_id=stack[0][0]) + + @pytest.fixture async def _stub_celery(app) -> None: """Replace the real Celery instance with a MagicMock. diff --git a/modules/background_tasks/tests/worker/tenancy/test_background_tasks_tenant_context.py b/modules/background_tasks/tests/worker/tenancy/test_background_tasks_tenant_context.py index c4ff18c41..55574d196 100644 --- a/modules/background_tasks/tests/worker/tenancy/test_background_tasks_tenant_context.py +++ b/modules/background_tasks/tests/worker/tenancy/test_background_tasks_tenant_context.py @@ -6,9 +6,11 @@ import pytest from background_tasks.executions.tenant_context import ( + OWNER_HEADER, TENANT_HEADER, release_tenant, restore_tenant, + stamp_owner, stamp_tenant, ) from simple_module_db import TenantIsolationError, current_tenant_id, tenant_context @@ -68,3 +70,32 @@ def test_task_without_tenant_runs_unscoped(): restore_tenant(task_id="t3", task=SimpleNamespace(request=SimpleNamespace())) assert current_tenant_id.get() is None release_tenant(task_id="t3") # no token recorded — must be a no-op + + +def test_headerless_worker_task_enqueues_without_an_owner(): + restore_tenant(task_id="t4", task=SimpleNamespace(request=SimpleNamespace())) + try: + headers: dict = {} + stamp_owner(headers) + assert OWNER_HEADER not in headers + finally: + release_tenant(task_id="t4") + + +@pytest.mark.parametrize("next_owner", [None, "next-user"]) +def test_stranded_prerun_never_reaches_the_next_task(next_owner): + """#861: a prerun without its postrun must not decide the next task's owner.""" + stale = SimpleNamespace(request=SimpleNamespace(**{OWNER_HEADER: "stale-user"})) + restore_tenant(task_id="t5", task=stale) # its postrun never runs + request = SimpleNamespace(**({OWNER_HEADER: next_owner} if next_owner else {})) + restore_tenant(task_id="t6", task=SimpleNamespace(request=request)) + try: + headers: dict = {} + stamp_owner(headers) + assert headers.get(OWNER_HEADER) == next_owner + finally: + release_tenant(task_id="t6") + release_tenant(task_id="t5") # unwinds the stranded binding too + headers = {} + stamp_owner(headers) + assert OWNER_HEADER not in headers diff --git a/modules/tenants/tests/core/service/test_tenants_owner_race.py b/modules/tenants/tests/core/service/test_tenants_owner_race.py index 5a7d0e05b..82e38b7ad 100644 --- a/modules/tenants/tests/core/service/test_tenants_owner_race.py +++ b/modules/tenants/tests/core/service/test_tenants_owner_race.py @@ -126,7 +126,13 @@ async def invite() -> str: return exc.code outcomes = await asyncio.gather(*(invite() for _ in range(5))) - assert sorted(outcomes) == ["already_invited"] * 4 + ["ok"] + assert outcomes.count("ok") == 1 + # A loser on SQLite can also get SQLITE_BUSY on the writer lock under load + # (the retryable ``admission_conflict``): still a refusal (#855). + refusals = {"already_invited"} | ( + {"admission_conflict"} if backend != "postgres" else set() + ) + assert {o for o in outcomes if o != "ok"} <= refusals, outcomes async with state.session_factory() as db: pending = await InvitationService(TenantService(db), ttl_hours=1).list_pending( tenant.id diff --git a/modules/users/tests/core/password_hashing/test_users_password_offload.py b/modules/users/tests/core/password_hashing/test_users_password_offload.py index 8b7a41b0f..1ff202c32 100644 --- a/modules/users/tests/core/password_hashing/test_users_password_offload.py +++ b/modules/users/tests/core/password_hashing/test_users_password_offload.py @@ -29,14 +29,18 @@ async def test_crypto_runs_in_worker_thread(users_app, monkeypatch, operation): token = await manager.generate_reset_password_token(user) loop_thread = threading.get_ident() crypto_threads = [] + # Patch the class, not the shared ``PASSWORD_HELPER`` instance: undoing an + # instance patch leaves a bound method in its ``__dict__`` that shadows + # every later class-level patch (#860). + helper_cls = type(manager.password_helper) for name in ("hash", "verify_and_update"): - original = getattr(manager.password_helper, name) + original = getattr(helper_cls, name) - def record(*args, _original=original): + def record(self, *args, _original=original): crypto_threads.append(threading.get_ident()) - return _original(*args) + return _original(self, *args) - monkeypatch.setattr(manager.password_helper, name, record) + monkeypatch.setattr(helper_cls, name, record) original_execute = db.execute async def execute(*args, **kwargs): @@ -63,6 +67,13 @@ async def execute(*args, **kwargs): assert loop_thread not in crypto_threads +def test_shared_helper_carries_no_instance_patches(): + """#860: tests must not leave instance attributes on the shared helper.""" + from users.core.password_hashing import PASSWORD_HELPER + + assert not {"hash", "verify_and_update"} & vars(PASSWORD_HELPER).keys() + + async def test_managers_share_password_helper(users_app): async with users_app.state.sm.db.session_factory() as db: assert ( From 000a4a061fb28a789929ff1006ca4785d96eff6c Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 6 Nov 2026 11:13:28 +0100 Subject: [PATCH 2/3] docs: drop the remaining "modules migrate one at a time" wording (round 1, pass 1) Claude-Session: https://claude.ai/code/session_01FXHA3ZicrtZ4C5QPEgxfgq --- docs/framework-conventions.md | 5 ++--- docs/guide/project-structure.md | 4 ++-- docs/module-authoring.md | 2 +- docs/module-layout.md | 6 +++--- skills/simple-module-conventions/SKILL.md | 2 +- skills/simple-module-creating/SKILL.md | 2 +- 6 files changed, 10 insertions(+), 11 deletions(-) diff --git a/docs/framework-conventions.md b/docs/framework-conventions.md index 8c2cde0bf..1a22e9890 100644 --- a/docs/framework-conventions.md +++ b/docs/framework-conventions.md @@ -25,9 +25,8 @@ modules// └── web/crud/test__crud.py ``` -New scaffolded modules use `core/` and `web/`; existing modules migrate one at a -time. See [Module layout](module-layout.md) for the root lookup table, area -selection, test naming, and compatibility policy. Area initializers contain +Modules use `core/` and `web/`. See [Module layout](module-layout.md) for the +root lookup table, area selection, test naming, and compatibility policy. Area initializers contain only a docstring. Keep private helpers in the area they serve. Scaffold with `smpy create-module --dest modules/` (or diff --git a/docs/guide/project-structure.md b/docs/guide/project-structure.md index 2efede032..fbb58a24e 100644 --- a/docs/guide/project-structure.md +++ b/docs/guide/project-structure.md @@ -57,8 +57,8 @@ See [Bundled modules](/modules/) for details on each. ## Anatomy of a module -New scaffolded modules use the area layout. Existing bundled modules migrate -one by one; [Module layout](/module-layout) tracks their status and the full contract. +Every bundled module and every scaffolded module uses the area layout; +[Module layout](/module-layout) has the full contract. ```text modules/orders/ diff --git a/docs/module-authoring.md b/docs/module-authoring.md index 9c5c90bb9..06e004bb1 100644 --- a/docs/module-authoring.md +++ b/docs/module-authoring.md @@ -29,7 +29,7 @@ modules// └── web/crud/test__crud.py ``` -New modules use this layout. Existing bundled modules migrate one at a time; +Every module uses this layout; see [Module layout](module-layout.md) for the exact root contract and shim policy. Area initializers contain only a docstring; private helpers live in their area. Tests have no `__init__.py`, use globally unique `test__*.py` basenames, diff --git a/docs/module-layout.md b/docs/module-layout.md index 11d224708..848edc963 100644 --- a/docs/module-layout.md +++ b/docs/module-layout.md @@ -162,8 +162,8 @@ sys.modules[__name__] = importlib.import_module("orders.web.service") The alias makes both paths the same module object, so mocks and `monkeypatch` strings keep working. Files containing both `deprecated_module(` and `sys.modules[__name__]` are auto-discovered by the expiry test and ignored -by the root-layout test while they remain. When the old path was inside a package that moved (for example -`orders.endpoints.api`), keep that package with a docstring-only `__init__.py` +by the root-layout test while they remain. When the old path was inside a +package that moved (for example `orders.endpoints.api`), keep that package with a docstring-only `__init__.py` and alias each module in it; a package holding only aliases is ignored by the root-layout test too. A shim that is a plain re-export instead (not an alias) must be added by hand to the expiry test's `_SHIM_MODULES` tuple and `_EXPECTED` set and to @@ -191,7 +191,7 @@ One module per PR; no logic changes in a move PR. JS tests remain in `tests-js/`. 4. Add external-import shims with removal release `0.0.46`, alias shims are auto-discovered by the expiry test (re-export shims are registered by hand), - shrink the exact frozen root exceptions, and update the + drop the shim's `DEPRECATED_ROOT` entry once it expires, and update the status table below. Domain areas must be named in `DOMAIN_AREAS`. 5. Run `python -c "import ."` in a **fresh interpreter for every module**, not one interpreter importing all modules in sequence. diff --git a/skills/simple-module-conventions/SKILL.md b/skills/simple-module-conventions/SKILL.md index b0c2e951b..06786bd05 100644 --- a/skills/simple-module-conventions/SKILL.md +++ b/skills/simple-module-conventions/SKILL.md @@ -39,7 +39,7 @@ Treat 300 lines as the design pressure on every `.py`, `.ts`, and `.tsx` file (t New module settings and their app-state container live in `core/settings.py` and `core/services.py`; HTTP dependencies and services live in `web/`. Keep root lookup files in place and put non-page frontend code in `ui/`. See -[Module layout](../../docs/module-layout.md); existing modules migrate one at a time. +[Module layout](../../docs/module-layout.md). ### 3. Per-module settings: `register_module_settings` → `app.state.` diff --git a/skills/simple-module-creating/SKILL.md b/skills/simple-module-creating/SKILL.md index 2212f96ca..289bad1a7 100644 --- a/skills/simple-module-creating/SKILL.md +++ b/skills/simple-module-creating/SKILL.md @@ -62,7 +62,7 @@ root `pyproject.toml` (type-check + test paths) and the `publish-pypi` matrix in test` without further wiring. Commit the `uv.lock`, `package-lock.json` and regenerated i18n key files it leaves behind. -New modules use this area layout; existing modules migrate one at a time. +Modules use this area layout. Area initializers hold only a docstring. Private helpers stay in their area. Tests have no `__init__.py`, keep `conftest.py` at their root and use globally unique `test_orders_*.py` basenames; JS tests stay in `tests-js/`. From 9256f2d6e1b79663399740b0ac232184ced8f3f5 Mon Sep 17 00:00:00 2001 From: Anto Subash Date: Fri, 6 Nov 2026 11:23:56 +0100 Subject: [PATCH 3/3] test(background_tasks): unwind every stranded task binding, refine owner comment Claude-Session: https://claude.ai/code/session_01FXHA3ZicrtZ4C5QPEgxfgq --- .../background_tasks/executions/tenant_context.py | 5 +++-- modules/background_tasks/tests/conftest.py | 5 +++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/modules/background_tasks/background_tasks/executions/tenant_context.py b/modules/background_tasks/background_tasks/executions/tenant_context.py index 39ea7c038..eee40bd96 100644 --- a/modules/background_tasks/background_tasks/executions/tenant_context.py +++ b/modules/background_tasks/background_tasks/executions/tenant_context.py @@ -175,8 +175,9 @@ def restore_tenant(*, task_id: str | None, task: Any) -> None: eager = bool(getattr(getattr(task, "request", None), "is_eager", False)) tenant = _tenant_of(task) or (current_tenant_id.get() if eager else _default_tenant) # ``""`` (not ``None``) means "no owner" and stops :func:`stamp_owner` - # falling through to an ambient ``current_user_id``. Every prerun sets it - # afresh, so a stranded binding never reaches the next task's messages. + # falling through to an ambient ``current_user_id``. A worker prerun sets it + # afresh, so a stranded binding never reaches the next task's messages (an + # eager run deliberately inherits its caller's). owner = message_owner(task=task) or (_task_owner.get() if eager else "") owner_token = _task_owner.set(owner) tenant_token = current_tenant_id.set(tenant) diff --git a/modules/background_tasks/tests/conftest.py b/modules/background_tasks/tests/conftest.py index 3d6563c99..363a2651f 100644 --- a/modules/background_tasks/tests/conftest.py +++ b/modules/background_tasks/tests/conftest.py @@ -36,8 +36,9 @@ def _unwind_stranded_task_bindings() -> Iterator[None]: owner pinned to "no owner" for every later test in this context. """ yield - stack = tenant_context._tokens.get() - if stack: + # Loop: release_tenant unwinds to the *latest* entry for an id, so a + # repeated id (nested eager run) can leave entries below it. + while stack := tenant_context._tokens.get(): tenant_context.release_tenant(task_id=stack[0][0])