From 19c73d65fcc15c0e1bbc9ebb4b1702c8668fe6da Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 15:04:28 +0000 Subject: [PATCH 1/2] =?UTF-8?q?fix:=20five=20reported=20defects=20?= =?UTF-8?q?=E2=80=94=20migrations,=20scaffold,=20DB=20commit=20timing,=20C?= =?UTF-8?q?SS=20sourcing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #257, #258, #262, #263, #264. #263 — expression-based index emitted twice under PostgreSQL. The dedup guard in make_process_revision_directives scanned only the top level of upgrade_ops, but autogenerate nests CreateIndexOp inside a ModifyTableOps group next to the CreateTableOp. The guard therefore never saw the index the dialect had already emitted and re-injected it, so a fresh Postgres app died on its first `make migrate` with DuplicateTable. The scan now recurses through nested op groups; the same fix applies to DropIndexOp in the downgrade. #262 — scaffolded apps migrated the wrong database. The scaffold's alembic.ini resolved script_location against the invocation cwd, forcing the Makefile to `cd host`, where BootstrapSettings' cwd-relative `.env` lookup missed the repo-root file and silently fell back to the default SQLite URL. Ported this repo's two corrections into the templates (`%(here)s/migrations`, migrate from the repo root) and made env.py log the resolved URL, password masked, so a cwd mismatch is visible on the first migration instead of silent. #264 — `create-module` always wrote tests/test_module.py. With no tests/__init__.py, pytest derives the module name from the basename alone, so the second module in a repo broke collection with "import file mismatch" — and root-level pytest is what the scaffold's own `make test` runs. The template is now named after the package. #257 — get_db committed in its yield-teardown, which FastAPI runs after the response has been delivered, so create-then-immediately-use lost the race and 404'd deterministically. Added CommitBeforeResponseMiddleware, which finalizes the request's sessions at the ASGI http.response.start message — the last point still inside the request, late enough that response serialization has already run and early enough that a commit failure can still become a 500. get_db keeps its finalize as a fallback for when the middleware isn't in the stack; the session is claimed once, so the error path (which unwinds the dependency, and therefore rolls back, before the error response is sent) is unaffected. #258 — gen-pages emitted @source only for a wheel module's pages/, leaving widgets under components/ uncompiled unless the host hand-wrote a .venv-relative glob. That path cannot be spelled portably (Windows uses Lib/site-packages) and Tailwind drops a non-matching glob silently, so every widget class vanished from the build on Windows with no error. components/ is now sourced by absolute path like pages/, and surfaces in modules.assets.json. Claude-Session: https://claude.ai/code/session_015fCFfMiqGce8unVpVezJG7 --- CLAUDE.md | 4 +- docs/framework-conventions.md | 13 +- docs/guide/quickstart.md | 4 +- .../templates/host/alembic.ini | 6 +- .../templates/host/migrations/env.py | 18 +- ..._module.py.tpl => test___PACKAGE__.py.tpl} | 0 .../templates/workspace/Makefile | 10 +- framework/cli/tests/test_module_css_assets.py | 52 +++++ .../cli/tests/test_scaffolding_module.py | 23 ++- framework/db/simple_module_db/__init__.py | 3 + framework/db/simple_module_db/deps.py | 53 ++--- framework/db/simple_module_db/migrations.py | 41 +++- framework/db/simple_module_db/transaction.py | 184 ++++++++++++++++++ framework/db/tests/_models.py | 11 ++ framework/db/tests/test_migrations.py | 43 ++++ framework/db/tests/test_transaction.py | 172 ++++++++++++++++ .../simple_module_hosting/_phase_helpers.py | 8 +- .../hosting/simple_module_hosting/assets.py | 24 ++- .../hosting/tests/test_middleware_order.py | 10 +- host/migrations/env.py | 18 +- 20 files changed, 639 insertions(+), 58 deletions(-) rename framework/cli/simple_module_cli/templates/module/tests/{test_module.py.tpl => test___PACKAGE__.py.tpl} (100%) create mode 100644 framework/db/simple_module_db/transaction.py create mode 100644 framework/db/tests/test_transaction.py diff --git a/CLAUDE.md b/CLAUDE.md index 54db095f..ba1690be 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -77,11 +77,11 @@ hence `SM022`/`SM023`. See `docs/module-authoring.md` § Styling. `register_settings` → `register_menu_items` / `register_permissions` / `register_feature_flags` / `register_event_handlers` / `register_health_checks` / `register_public_routes` → `register_exception_handlers` → `register_middleware` → `register_routes(api_router, view_router)` → async `on_startup` / `on_shutdown` (reverse order). `register_public_routes(registry)` lets a module exempt anonymous/read-only routes (STAC/OGC, webhooks) from `AuthMiddleware`; rules are method-aware (`registry.add_regex(r"…/tilejson$", methods={"GET"})`), so a GET read route can be public while sibling POST/PATCH mutations under the same prefix stay gated. See [docs/framework/public-routes.md](docs/framework/public-routes.md). **Middleware pipeline** (Starlette `add_middleware` is LIFO — last added runs first). Execution order on a request: -`(ProxyHeaders, if SM_TRUSTED_PROXY) → CorrelationId → RequestLogging → GZip → SecurityHeaders → Session → → Tenant (opt-in) → Locale → InertiaLayoutData → app`. `GZip` compresses any response over 500 bytes, including the `/static` mount — the built CSS is ~139 KB raw versus ~21 KB gzipped, and uncompressed assets dominated cold page load. `ProxyHeaders` (uvicorn's `ProxyHeadersMiddleware`) is installed only when `SM_TRUSTED_PROXY` is set, sitting outermost so the `X-Forwarded-*`-corrected scheme/client IP reach everything downstream (request logs and Inertia's absolute page url). When two modules add middleware at the same dependency tier, the module that sorts **later** wraps outermost. Use `depends_on` to express relative order — don't rely on names. +`(ProxyHeaders, if SM_TRUSTED_PROXY) → CorrelationId → RequestLogging → GZip → SecurityHeaders → Session → → Tenant (opt-in) → Locale → InertiaLayoutData → CommitBeforeResponse → app`. `GZip` compresses any response over 500 bytes, including the `/static` mount — the built CSS is ~139 KB raw versus ~21 KB gzipped, and uncompressed assets dominated cold page load. `ProxyHeaders` (uvicorn's `ProxyHeadersMiddleware`) is installed only when `SM_TRUSTED_PROXY` is set, sitting outermost so the `X-Forwarded-*`-corrected scheme/client IP reach everything downstream (request logs and Inertia's absolute page url). When two modules add middleware at the same dependency tier, the module that sorts **later** wraps outermost. Use `depends_on` to express relative order — don't rely on names. **Database**: per-module `Base` via `create_module_base("")`. Every module owns its own `MetaData` (so Alembic autogenerate can attribute tables to a module), but all tables live in the host's single schema. `__tablename__` must be prefixed with the module name to avoid collisions (`orders_order`). Postgres and SQLite share the same layout. -Standard mixins in `simple_module_db.mixins`: `AuditMixin`, `SoftDeleteMixin` (bypass with `stmt.execution_options(include_deleted=True)`), `MultiTenantMixin`, `VersionedMixin`. The per-request session (`get_db`) auto-commits **only if** there are pending writes (via `after_flush` listener); otherwise rollback. Service code should **not** call `session.commit()` — flush if you need DB-assigned values. +Standard mixins in `simple_module_db.mixins`: `AuditMixin`, `SoftDeleteMixin` (bypass with `stmt.execution_options(include_deleted=True)`), `MultiTenantMixin`, `VersionedMixin`. The per-request session (`get_db`) auto-commits **only if** there are pending writes (via `after_flush` listener); otherwise rollback. Service code should **not** call `session.commit()` — flush if you need DB-assigned values. The commit fires in `CommitBeforeResponseMiddleware`, at the ASGI `http.response.start` message, so a client that creates a row and immediately reads it back in a second request sees it — FastAPI runs a `yield` dependency's exit code *after* the response is delivered, which used to make that a deterministic 404 (GH #257). `get_db` keeps the same commit in its own exit code as a fallback for when the middleware isn't in the stack; whichever runs first wins. **Migrations** live in `host/migrations/versions/` — not in module packages. `host/alembic/env.py` calls `build_module_metadata()` + `make_include_object()` so autogenerate covers every installed module and ignores host-owned tables. First migration of each module should set `branch_labels = ("",)` to enable per-module `downgrade @base`. diff --git a/docs/framework-conventions.md b/docs/framework-conventions.md index f7ea307c..2209a58d 100644 --- a/docs/framework-conventions.md +++ b/docs/framework-conventions.md @@ -75,7 +75,7 @@ InertiaLayoutDataMiddleware ``` (ProxyHeaders) → CorrelationId → RequestLogging → SecurityHeaders → Session - → → Tenant → Locale → InertiaLayoutData → app + → → Tenant → Locale → InertiaLayoutData → CommitBeforeResponse → app ``` `ProxyHeaders` is installed only when `SM_TRUSTED_PROXY` is set (uvicorn's @@ -191,7 +191,16 @@ Each request opens one session: - Read-only requests exit via rollback — cheaper, and keeps the session out of write-side profiling. - Exceptions always rollback. -Service code should not call `session.commit()` directly. Flush for intermediate reads if you need DB-assigned values, then let the dependency commit. +Service code should not call `session.commit()` directly. Flush for intermediate reads if you need DB-assigned values, then let the framework commit. + +The commit lands in `CommitBeforeResponseMiddleware`, which intercepts the ASGI +`http.response.start` message — the last point still inside the request. That is +what makes create-then-immediately-use work: FastAPI runs a `yield` dependency's +exit code after the response has been delivered, so committing there let a +follow-up request open a fresh session and 404 on a row that was already +returned to the client (GH #257). `get_db` still finalizes in its own exit code +when the middleware is absent, and the session is finalized exactly once either +way. ## Inertia diff --git a/docs/guide/quickstart.md b/docs/guide/quickstart.md index 865c890d..69512ab2 100644 --- a/docs/guide/quickstart.md +++ b/docs/guide/quickstart.md @@ -60,7 +60,9 @@ This generates a publishable starter module at `modules/orders/` with: - `orders/settings.py` — the module's `pydantic_settings` settings class. - `orders/endpoints/api.py` — starter REST endpoints at `/api/orders`. - `orders/pages/` — empty page dir; add `.tsx` pages (and a `register_routes` view router) as you build views. -- `tests/test_module.py` — pytest smoke test. +- `tests/test_orders.py` — pytest smoke test. Named after the package so a repo + with several modules doesn't collide at collection: without `tests/__init__.py`, + pytest derives the test module's name from the basename alone. You add the domain model (`models.py`), DTOs (`contracts/`), service, Inertia views, and pages yourself — the [first-module guide](/guide/first-module) walks through that. diff --git a/framework/cli/simple_module_cli/templates/host/alembic.ini b/framework/cli/simple_module_cli/templates/host/alembic.ini index a6410a6f..1850cf3a 100644 --- a/framework/cli/simple_module_cli/templates/host/alembic.ini +++ b/framework/cli/simple_module_cli/templates/host/alembic.ini @@ -1,5 +1,9 @@ [alembic] -script_location = migrations +# Resolve script_location relative to this ini file, not the invocation cwd, +# so `alembic -c host/alembic.ini ...` works from the repo root. Running from +# the repo root is what lets alembic read the repo-root .env — and therefore +# the same SM_DATABASE_URL the app itself uses. +script_location = %(here)s/migrations sqlalchemy.url = [loggers] diff --git a/framework/cli/simple_module_cli/templates/host/migrations/env.py b/framework/cli/simple_module_cli/templates/host/migrations/env.py index 58ca765c..c83b7c13 100644 --- a/framework/cli/simple_module_cli/templates/host/migrations/env.py +++ b/framework/cli/simple_module_cli/templates/host/migrations/env.py @@ -9,6 +9,7 @@ import logging from logging.config import fileConfig +from pathlib import Path from alembic import context from simple_module_db import ( @@ -19,6 +20,7 @@ ) from simple_module_hosting.settings import Settings from sqlalchemy import engine_from_config, pool +from sqlalchemy.engine import make_url logger = logging.getLogger("alembic.env") @@ -35,9 +37,21 @@ def _get_url() -> str: - """Read database URL from settings, convert async to sync driver.""" + """Read database URL from settings, convert async to sync driver. + + The resolved URL is logged because ``Settings`` reads ``.env`` relative to + the *current working directory*: run alembic from the wrong cwd and it + silently falls back to the default SQLite file while the app talks to the + configured database. Printing the target — password masked — turns that + into something you notice on the first migration instead of a schema that + lives in a database nobody reads. + """ settings = Settings() - return settings.database_url.replace("+aiosqlite", "").replace("+asyncpg", "+psycopg2") + url = settings.database_url.replace("+aiosqlite", "").replace("+asyncpg", "+psycopg2") + logger.info( + "Migrating %s (cwd=%s)", make_url(url).render_as_string(hide_password=True), Path.cwd() + ) + return url def run_migrations_offline() -> None: diff --git a/framework/cli/simple_module_cli/templates/module/tests/test_module.py.tpl b/framework/cli/simple_module_cli/templates/module/tests/test___PACKAGE__.py.tpl similarity index 100% rename from framework/cli/simple_module_cli/templates/module/tests/test_module.py.tpl rename to framework/cli/simple_module_cli/templates/module/tests/test___PACKAGE__.py.tpl diff --git a/framework/cli/simple_module_cli/templates/workspace/Makefile b/framework/cli/simple_module_cli/templates/workspace/Makefile index 856b6dda..7db00dad 100644 --- a/framework/cli/simple_module_cli/templates/workspace/Makefile +++ b/framework/cli/simple_module_cli/templates/workspace/Makefile @@ -54,12 +54,18 @@ sync-module-deps: # `upgrade heads` (plural) applies every per-module branch head; `upgrade head` # (singular) errors once a second module adds its own migration branch label. +# +# These run from the repo root, not `cd host`, so alembic and `make dev-api` +# share the same cwd — and therefore the same .env, SM_DATABASE_URL and SQLite +# path. BootstrapSettings reads `.env` relative to the cwd, so running from +# host/ silently migrates the default SQLite DB while the app uses the +# configured one. migrate: - cd host && uv run alembic upgrade heads + uv run --project host alembic -c host/alembic.ini upgrade heads migration: @test -n "$(msg)" || (echo 'Usage: make migration msg="describe the change"' && exit 1) - cd host && uv run alembic revision --autogenerate -m "$(msg)" + uv run --project host alembic -c host/alembic.ini revision --autogenerate -m "$(msg)" kill: @-pkill -f "uvicorn main:app" 2>/dev/null diff --git a/framework/cli/tests/test_module_css_assets.py b/framework/cli/tests/test_module_css_assets.py index 3a07aa27..9dbf8764 100644 --- a/framework/cli/tests/test_module_css_assets.py +++ b/framework/cli/tests/test_module_css_assets.py @@ -24,6 +24,20 @@ async def test_detects_theme_and_styles(self, make_importable_module): assert entry.styles_css is not None and entry.styles_css.name == "styles.css" assert entry.pages_dir is not None + async def test_detects_components_dir(self, make_importable_module): + """components/ is discovered so its widget classes reach Tailwind.""" + from simple_module_hosting.assets import compute_module_assets + + mod, pkg = make_importable_module("widget_mod", "Widget") + (pkg / "components").mkdir() + + result = compute_module_assets([mod]) + + assert [e.name for e in result] == ["Widget"] + assert result[0].components_dir is not None + assert result[0].components_dir.name == "components" + assert result[0].pages_dir is None + async def test_css_only_module_is_included(self, make_importable_module): """A module with CSS but no pages/ still appears — the manifest.json gap.""" from simple_module_hosting.assets import compute_module_assets @@ -115,6 +129,43 @@ async def test_source_emitted_for_wheel_modules(self, tmp_path): assert f'@source "{pages.as_posix()}/**/*.{{ts,tsx}}";' in css + async def test_source_emitted_for_wheel_module_components(self, tmp_path): + """A wheel module's components/ is scanned too, not just its pages/. + + Widgets ship under components/. Omitting them left the host to + hand-write a `.venv`-relative @source, which cannot be spelled + portably — Windows uses `Lib/site-packages`, POSIX + `lib/python3.x/site-packages` — and Tailwind drops a non-matching glob + silently, so every widget class vanished from the build on Windows. + """ + from simple_module_hosting.assets import render_modules_css + + pages = tmp_path / "gis" / "pages" + components = tmp_path / "gis" / "components" + entry = _assets(tmp_path, pages_dir=pages, components_dir=components) + css = render_modules_css([entry], in_repo=lambda _p: False) + + assert f'@source "{pages.as_posix()}/**/*.{{ts,tsx}}";' in css + assert f'@source "{components.as_posix()}/**/*.{{ts,tsx}}";' in css + + async def test_components_only_module_still_emits_source(self, tmp_path): + """A module shipping widgets but no pages/ or CSS is not skipped.""" + from simple_module_hosting.assets import render_modules_css + + components = tmp_path / "gis" / "components" + css = render_modules_css( + [_assets(tmp_path, components_dir=components)], in_repo=lambda _p: False + ) + + assert f'@source "{components.as_posix()}/**/*.{{ts,tsx}}";' in css + + async def test_in_repo_components_are_not_sourced(self, tmp_path): + """In-repo components are already covered by the host's static glob.""" + from simple_module_hosting.assets import render_modules_css + + entry = _assets(tmp_path, components_dir=tmp_path / "local" / "components") + assert "@source" not in render_modules_css([entry], in_repo=lambda _p: True) + async def test_module_without_css_emits_no_import(self, tmp_path): """Pages-only modules contribute @source but no @import.""" from simple_module_hosting.assets import render_modules_css @@ -199,6 +250,7 @@ async def test_writes_assets_json(self, tmp_path): "package_name", "package", "pages", + "components", "theme", "styles", "npm_name", diff --git a/framework/cli/tests/test_scaffolding_module.py b/framework/cli/tests/test_scaffolding_module.py index 352c6609..9a356bd6 100644 --- a/framework/cli/tests/test_scaffolding_module.py +++ b/framework/cli/tests/test_scaffolding_module.py @@ -19,7 +19,7 @@ async def test_creates_expected_module_files(self, tmp_path): "my_feature/module.py", "my_feature/endpoints/__init__.py", "my_feature/endpoints/api.py", - "tests/test_module.py", + "tests/test_my_feature.py", ".gitignore", "README.md", ]: @@ -30,6 +30,27 @@ async def test_creates_expected_module_files(self, tmp_path): # pytest try to register `tests.conftest` as a plugin twice. assert not (dest / "tests" / "__init__.py").exists() + async def test_test_file_basename_is_unique_per_module(self, tmp_path): + """Two modules in one workspace must not ship same-named test files. + + Without `tests/__init__.py`, pytest's default `prepend` import mode + derives a test module's name from its basename alone. Two modules both + shipping `tests/test_module.py` therefore collide at collection with + "import file mismatch", and a root-level `pytest` — which is what the + scaffold's own `make test` runs — reports errors instead of tests. + """ + from simple_module_cli.scaffolding import create_module + + alpha = create_module(tmp_path / "alpha", name="Alpha") + beta = create_module(tmp_path / "beta", name="Beta") + + alpha_tests = [p.name for p in (alpha / "tests").glob("test_*.py")] + beta_tests = [p.name for p in (beta / "tests").glob("test_*.py")] + + assert alpha_tests == ["test_alpha.py"] + assert beta_tests == ["test_beta.py"] + assert not set(alpha_tests) & set(beta_tests) + async def test_pyproject_declares_entry_point_and_deps(self, tmp_path): """pyproject.toml sets the entry_point and pins the framework API range.""" from simple_module_cli.scaffolding import create_module diff --git a/framework/db/simple_module_db/__init__.py b/framework/db/simple_module_db/__init__.py index 7890558c..505e87ba 100644 --- a/framework/db/simple_module_db/__init__.py +++ b/framework/db/simple_module_db/__init__.py @@ -13,10 +13,12 @@ from simple_module_db.mixins import AuditMixin, MultiTenantMixin, SoftDeleteMixin, VersionedMixin from simple_module_db.provider import DatabaseProvider, detect_provider from simple_module_db.session import DatabaseState, init_db +from simple_module_db.transaction import CommitBeforeResponseMiddleware, finalize_session __all__ = [ "AuditMixin", "AuditRecord", + "CommitBeforeResponseMiddleware", "DatabaseProvider", "DatabaseState", "MultiTenantMixin", @@ -27,6 +29,7 @@ "create_module_base", "current_tenant_id", "detect_provider", + "finalize_session", "get_db", "init_db", "make_include_object", diff --git a/framework/db/simple_module_db/deps.py b/framework/db/simple_module_db/deps.py index d0c9b7aa..47dd0f39 100644 --- a/framework/db/simple_module_db/deps.py +++ b/framework/db/simple_module_db/deps.py @@ -2,26 +2,34 @@ from __future__ import annotations -import logging import time from collections.abc import AsyncGenerator from fastapi import Request from sqlalchemy.ext.asyncio import AsyncSession -from simple_module_db.listeners import SESSION_HAS_WRITES_KEY - -_db_logger = logging.getLogger("simple_module.db") +from simple_module_db.transaction import ( + SESSION_START_KEY, + finalize_session, + register_request_session, + rollback_session, +) async def get_db(request: Request) -> AsyncGenerator[AsyncSession, None]: """Yield an async database session, auto-closing on exit. Commits only when the session has pending writes (``new``, ``dirty``, - or ``deleted`` objects). Read-only handlers still open an implicit - transaction but exit via ``rollback`` — that's one round-trip - cheaper than ``commit`` and keeps read-only queries from showing up - as writes in query logs / ``pg_stat_statements``. + or ``deleted`` objects); read-only handlers exit via ``rollback``. + + The commit itself normally happens in + :class:`~simple_module_db.transaction.CommitBeforeResponseMiddleware`, + which fires while the response is still in the server's hands. The + finalize below is the fallback for when that middleware isn't in the + stack, and a no-op when it already ran — FastAPI runs this exit code + *after* the response has been delivered, so a client that immediately + reads back what it just wrote would otherwise race the commit and lose + (GH #257). Usage in FastAPI endpoints:: @@ -30,33 +38,12 @@ async def list_items(db: AsyncSession = Depends(get_db)): ... """ factory = request.app.state.sm.db.session_factory - start = time.perf_counter() async with factory() as session: + session.info[SESSION_START_KEY] = time.perf_counter() + register_request_session(request.scope, session) try: yield session - # ``has_writes`` is set by the after_flush listener and - # survives the flush emptying session.new/.dirty/.deleted. - has_pending = bool( - session.info.get(SESSION_HAS_WRITES_KEY) - or session.new - or session.dirty - or session.deleted - ) - if has_pending: - await session.commit() - op, log_message = "commit", "db.session.commit" - log = _db_logger.info - else: - await session.rollback() - op, log_message = "read_only_rollback", "db.session.read_only" - log = _db_logger.debug - duration_ms = round((time.perf_counter() - start) * 1000, 2) - log(log_message, extra={"operation": op, "db_duration_ms": duration_ms}) + await finalize_session(session) except Exception: - await session.rollback() - duration_ms = round((time.perf_counter() - start) * 1000, 2) - _db_logger.warning( - "db.session.rollback", - extra={"operation": "rollback", "db_duration_ms": duration_ms}, - ) + await rollback_session(session) raise diff --git a/framework/db/simple_module_db/migrations.py b/framework/db/simple_module_db/migrations.py index 02879bb4..b3bdc7eb 100644 --- a/framework/db/simple_module_db/migrations.py +++ b/framework/db/simple_module_db/migrations.py @@ -132,6 +132,13 @@ def make_process_revision_directives( not strictly required (dropping the table drops the index) but it keeps autogen output readable. + The injection is idempotent, and has to be: a dialect that *can* reflect + expression-based indexes (PostgreSQL) already emits the index itself, so + re-adding it unconditionally produced a migration with two identical + ``create_index`` calls that failed on first run with ``DuplicateTable``. + Any index already named in the op tree — at any nesting depth — is left + alone, which keeps this dialect-agnostic rather than special-casing SQLite. + Call as:: context.configure( @@ -164,10 +171,34 @@ def _index_is_expression_based(index: Index) -> bool: return any(not isinstance(expr, Column) for expr in index.expressions) -def _inject_create_index_after_create_table(upgrade_ops, expression_indexes) -> None: - existing_index_names = { - getattr(op, "index_name", None) for op in upgrade_ops.ops if isinstance(op, CreateIndexOp) +def _iter_ops_recursive(container): + """Yield every op under ``container``, descending into nested op groups. + + Autogenerate does not emit a flat op list: index operations for a table are + grouped inside a ``ModifyTableOps`` container alongside the top-level + ``CreateTableOp``/``DropTableOp``. A dedup check that only looks at + ``container.ops`` therefore sees no ``CreateIndexOp`` at all and re-injects + an index the dialect already emitted — which is exactly how a dialect that + *can* reflect expression-based indexes (PostgreSQL) ended up with a + duplicate ``CREATE INDEX`` in its initial migration. + """ + for op in container.ops: + yield op + if hasattr(op, "ops"): + yield from _iter_ops_recursive(op) + + +def _existing_index_names(container, op_type) -> set[str | None]: + """Names of every ``op_type`` index op already present anywhere under ``container``.""" + return { + getattr(op, "index_name", None) + for op in _iter_ops_recursive(container) + if isinstance(op, op_type) } + + +def _inject_create_index_after_create_table(upgrade_ops, expression_indexes) -> None: + existing_index_names = _existing_index_names(upgrade_ops, CreateIndexOp) new_ops: list = [] for op in upgrade_ops.ops: new_ops.append(op) @@ -188,9 +219,7 @@ def _inject_create_index_after_create_table(upgrade_ops, expression_indexes) -> def _inject_drop_index_before_drop_table(downgrade_ops, expression_indexes) -> None: - existing_drop_names = { - getattr(op, "index_name", None) for op in downgrade_ops.ops if isinstance(op, DropIndexOp) - } + existing_drop_names = _existing_index_names(downgrade_ops, DropIndexOp) new_ops: list = [] for op in downgrade_ops.ops: if isinstance(op, DropTableOp): diff --git a/framework/db/simple_module_db/transaction.py b/framework/db/simple_module_db/transaction.py new file mode 100644 index 00000000..9f5d2e6d --- /dev/null +++ b/framework/db/simple_module_db/transaction.py @@ -0,0 +1,184 @@ +"""Commit the request's unit of work before the response leaves the server. + +``get_db`` is a FastAPI ``yield`` dependency, and FastAPI runs a yield +dependency's exit code *after* the response has been delivered. Committing +there means a client that creates a row and immediately references it by id in +a second request loses the race: the create's ``201`` reaches the client before +the create's commit runs, so the follow-up request opens a fresh session, finds +nothing, and 404s. It is deterministic rather than flaky — the follow-up +request reliably beats the post-response commit — which is why a seed script +publishes 0 pages on its first pass and all of them on a second, identical one. +See GH #257. + +This module moves the commit to the last point that is still *inside* the +request: the ASGI ``http.response.start`` message. Nothing has reached the +client yet, so a failure there can still become a 500, and every caller — +seeds, provisioning scripts, integration tests, single-run k8s jobs — gets the +guarantee without changing a line of endpoint code. + +A session is finalized exactly once. Whichever runs first wins and the other +becomes a no-op, so the framework stays correct when the middleware is absent +(a bare ``get_db``, a WebSocket, a test calling the dependency directly) and on +the error path, where FastAPI unwinds the dependency — rolling it back — +*before* the error response is sent. +""" + +from __future__ import annotations + +import json +import logging +import time + +from sqlalchemy.ext.asyncio import AsyncSession + +from simple_module_db.listeners import SESSION_HAS_WRITES_KEY + +logger = logging.getLogger("simple_module.db") + +REQUEST_SESSIONS_KEY = "sm_db_sessions" +"""Key under ASGI ``scope["state"]`` holding the sessions opened this request.""" + +SESSION_START_KEY = "sm_db_started_at" +"""``session.info`` key carrying the perf-counter reading taken at open.""" + +_FINALIZED_KEY = "sm_db_finalized" + +_INTERNAL_ERROR_BODY = json.dumps({"detail": "Internal Server Error"}).encode() + + +def _elapsed_ms(session: AsyncSession) -> float: + start = session.info.get(SESSION_START_KEY) + return round((time.perf_counter() - start) * 1000, 2) if start else 0.0 + + +def _claim(session: AsyncSession) -> bool: + """Return True if this caller is the one that gets to finalize ``session``.""" + if session.info.get(_FINALIZED_KEY): + return False + session.info[_FINALIZED_KEY] = True + return True + + +async def finalize_session(session: AsyncSession) -> None: + """Commit ``session`` if it has pending writes, else roll it back. Idempotent. + + Read-only handlers exit via ``rollback`` — one round-trip cheaper than + ``commit``, and it keeps read-only queries from showing up as writes in + query logs / ``pg_stat_statements``. + + On commit failure the session is rolled back before the error propagates, + so the caller never has to reason about a half-finalized session. + """ + if not _claim(session): + return + # ``has_writes`` is set by the after_flush listener and survives the flush + # emptying session.new/.dirty/.deleted. + has_pending = bool( + session.info.get(SESSION_HAS_WRITES_KEY) or session.new or session.dirty or session.deleted + ) + if not has_pending: + await session.rollback() + logger.debug( + "db.session.read_only", + extra={"operation": "read_only_rollback", "db_duration_ms": _elapsed_ms(session)}, + ) + return + try: + await session.commit() + except Exception: + await session.rollback() + raise + logger.info( + "db.session.commit", + extra={"operation": "commit", "db_duration_ms": _elapsed_ms(session)}, + ) + + +async def rollback_session(session: AsyncSession) -> None: + """Roll ``session`` back and mark it finalized. Idempotent.""" + if not _claim(session): + return + await session.rollback() + logger.warning( + "db.session.rollback", + extra={"operation": "rollback", "db_duration_ms": _elapsed_ms(session)}, + ) + + +def register_request_session(scope: dict, session: AsyncSession) -> None: + """Enlist ``session`` for commit-before-response, if the middleware is installed. + + A no-op when it isn't: ``get_db`` still commits in its own exit code, just + later. That keeps the dependency usable on its own — in a WebSocket handler, + a background task, or a test that never builds the middleware stack. + """ + sessions = scope.get("state", {}).get(REQUEST_SESSIONS_KEY) + if sessions is None: + return + sessions.append(session) + + +class CommitBeforeResponseMiddleware: + """Finalize this request's DB sessions before the response is transmitted. + + Pure ASGI rather than ``BaseHTTPMiddleware`` because the hook point is the + ``send`` channel, not the response object: intercepting + ``http.response.start`` is what lets the commit land before any byte is + written, and lets a commit failure still be turned into a 500. + + Install this **innermost** (add it first — Starlette's ``add_middleware`` is + LIFO) so its wrapper is the first to see the response and the commit happens + as early as possible. + """ + + def __init__(self, app): + self.app = app + + async def __call__(self, scope, receive, send) -> None: + if scope["type"] != "http": + await self.app(scope, receive, send) + return + + sessions: list[AsyncSession] = [] + scope.setdefault("state", {})[REQUEST_SESSIONS_KEY] = sessions + aborted = False + + async def send_wrapper(message) -> None: + nonlocal aborted + if aborted: + # The response we replaced is still streaming its body into a + # channel we've already closed off. Drop it. + return + if message["type"] == "http.response.start" and sessions: + try: + for session in sessions: + await finalize_session(session) + except Exception: + aborted = True + logger.exception( + "db.session.commit_failed", extra={"operation": "commit_failed"} + ) + await _send_internal_error(send) + return + await send(message) + + await self.app(scope, receive, send_wrapper) + + +async def _send_internal_error(send) -> None: + """Replace the not-yet-sent response with a 500. + + Reachable only from ``http.response.start``, so nothing has been written + and this cannot collide with a partially-sent response. + """ + await send( + { + "type": "http.response.start", + "status": 500, + "headers": [ + (b"content-type", b"application/json"), + (b"content-length", str(len(_INTERNAL_ERROR_BODY)).encode()), + ], + } + ) + await send({"type": "http.response.body", "body": _INTERNAL_ERROR_BODY}) diff --git a/framework/db/tests/_models.py b/framework/db/tests/_models.py index d153884b..b1447928 100644 --- a/framework/db/tests/_models.py +++ b/framework/db/tests/_models.py @@ -27,3 +27,14 @@ class _TenantSoftItem(_TenantBase, MultiTenantMixin, SoftDeleteMixin, table=True __tablename__ = "mt_test_soft_item" id: int | None = Field(default=None, primary_key=True) name: str = Field(max_length=100) + + +_TxnBase = create_module_base("txn_test") + + +class _TxnThing(_TxnBase, table=True): # ty: ignore[unsupported-base] + """Plain table for exercising when the request's unit of work commits.""" + + __tablename__ = "txn_test_thing" + id: int | None = Field(default=None, primary_key=True) + name: str = Field(max_length=100) diff --git a/framework/db/tests/test_migrations.py b/framework/db/tests/test_migrations.py index 7629a4d5..5dc95951 100644 --- a/framework/db/tests/test_migrations.py +++ b/framework/db/tests/test_migrations.py @@ -188,6 +188,49 @@ def test_does_not_double_inject_when_already_present(self): index_ops = [op for op in directives[0].upgrade_ops.ops if isinstance(op, CreateIndexOp)] assert len(index_ops) == 1 + def test_does_not_double_inject_when_nested_in_modify_table_ops(self): + """Autogenerate does not hand us a flat op list: index ops for a table + arrive inside a ``ModifyTableOps`` group next to the ``CreateTableOp``. + A dedup check that only scans the top level sees no ``CreateIndexOp`` + there and re-injects the index PostgreSQL already emitted, producing a + migration that dies with ``DuplicateTable`` on its first run.""" + from alembic.operations.ops import CreateIndexOp, ModifyTableOps + from simple_module_db.migrations import make_process_revision_directives + + meta = self._build_meta() + idx = next(iter(meta.tables["things"].indexes)) + nested = ModifyTableOps("things", ops=[CreateIndexOp.from_index(idx)]) + directives = self._build_directives("things", nested) + make_process_revision_directives(meta)(None, None, directives) + + assert self._count_index_ops(directives[0].upgrade_ops, CreateIndexOp) == 1 + + def test_does_not_double_inject_drop_nested_in_modify_table_ops(self): + """Same nesting applies to the downgrade: the reflected ``DropIndexOp`` + sits inside a ``ModifyTableOps`` group before the ``DropTableOp``.""" + from alembic.operations.ops import DropIndexOp, ModifyTableOps + from simple_module_db.migrations import make_process_revision_directives + + meta = self._build_meta() + directives = self._build_directives("things") + directives[0].downgrade_ops.ops.insert( + 0, ModifyTableOps("things", ops=[DropIndexOp("ix_things_email_lower", "things")]) + ) + make_process_revision_directives(meta)(None, None, directives) + + assert self._count_index_ops(directives[0].downgrade_ops, DropIndexOp) == 1 + + @staticmethod + def _count_index_ops(container, op_type) -> int: + """Count ``op_type`` ops anywhere under ``container``, nesting included.""" + total = 0 + for op in container.ops: + if isinstance(op, op_type): + total += 1 + if hasattr(op, "ops"): + total += TestProcessRevisionDirectives._count_index_ops(op, op_type) + return total + def test_ignores_column_based_indexes(self): """Plain column indexes are already handled correctly by autogenerate; the hook must not touch them.""" diff --git a/framework/db/tests/test_transaction.py b/framework/db/tests/test_transaction.py new file mode 100644 index 00000000..f690f0cb --- /dev/null +++ b/framework/db/tests/test_transaction.py @@ -0,0 +1,172 @@ +"""Tests for commit-before-response (GH #257). + +A client that creates a row and immediately references it by id used to get a +deterministic 404: FastAPI runs a ``yield`` dependency's exit code *after* the +response is delivered, so the create's ``201`` beat the create's commit and the +follow-up request opened a fresh session that saw nothing. + +These use a **file-backed** SQLite database on purpose. An in-memory SQLite URL +shares one connection across every session, which makes uncommitted writes +visible to the "second request" and hides the very race under test. +""" + +from __future__ import annotations + +from collections.abc import AsyncGenerator +from types import SimpleNamespace + +import httpx +import pytest +from _models import _TxnBase, _TxnThing +from fastapi import Depends, FastAPI +from simple_module_db.deps import get_db +from simple_module_db.listeners import register_listeners +from simple_module_db.session import init_db +from simple_module_db.transaction import CommitBeforeResponseMiddleware +from sqlalchemy.ext.asyncio import AsyncSession +from sqlmodel import select + + +def _build_app(db_state, *, with_middleware: bool = True) -> FastAPI: + """A miniature host: create flushes only, read opens its own session.""" + app = FastAPI() + if with_middleware: + app.add_middleware(CommitBeforeResponseMiddleware) + app.state.sm = SimpleNamespace(db=db_state) + + @app.post("/things", status_code=201) + async def create(name: str, db: AsyncSession = Depends(get_db)): + # Deliberately no commit() — get_db owns the unit of work, which is + # exactly the pattern the framework documents for service code. + thing = _TxnThing(name=name) + db.add(thing) + await db.flush() + return {"id": thing.id} + + @app.get("/things/{thing_id}") + async def read(thing_id: int, db: AsyncSession = Depends(get_db)): + found = ( + await db.execute(select(_TxnThing).where(_TxnThing.id == thing_id)) + ).scalar_one_or_none() + if found is None: + return {"found": False} + return {"found": True, "name": found.name} + + @app.post("/boom", status_code=201) + async def boom(db: AsyncSession = Depends(get_db)): + db.add(_TxnThing(name="doomed")) + await db.flush() + raise RuntimeError("endpoint blew up after writing") + + return app + + +@pytest.fixture +async def db_state(tmp_path) -> AsyncGenerator[object, None]: + state = init_db(f"sqlite+aiosqlite:///{tmp_path / 'txn.db'}") + try: + # The after_flush listener is what marks a session as having writes + # once flush() has emptied new/dirty/deleted — create_app registers it, + # so the fixture must too or every request looks read-only. + register_listeners(state) + async with state.engine.begin() as conn: + await conn.run_sync(_TxnBase.metadata.create_all) + yield state + finally: + await state.engine.dispose() + + +async def _client(app) -> httpx.AsyncClient: + return httpx.AsyncClient( + transport=httpx.ASGITransport(app=app, raise_app_exceptions=False), + base_url="http://test", + ) + + +class TestCommitBeforeResponse: + async def test_created_row_is_readable_on_the_very_next_request(self, db_state): + """The client-visible contract: no second pass, no retry loop. + + Note this one does *not* fail without the fix — httpx's in-process ASGI + transport awaits the whole request, teardown included, before issuing + the next one, so the follow-up can never beat the commit here. It + documents the intended behaviour; the ordering guarantee is pinned by + ``test_write_is_durable_before_the_response_is_delivered`` below, which + does fail without the middleware. + """ + async with await _client(_build_app(db_state)) as client: + created = await client.post("/things", params={"name": "page-1"}) + assert created.status_code == 201 + thing_id = created.json()["id"] + + found = await client.get(f"/things/{thing_id}") + + assert found.json() == {"found": True, "name": "page-1"} + + async def test_write_is_durable_before_the_response_is_delivered(self, db_state): + """Stronger than the round-trip above: assert against a connection that + the request never touched, at the moment the response is emitted.""" + app = _build_app(db_state) + visible_at_response_start: list[bool] = [] + + class Probe: + def __init__(self, inner): + self.inner = inner + + async def __call__(self, scope, receive, send): + async def spy(message): + if message["type"] == "http.response.start": + async with db_state.session_factory() as other: + rows = (await other.execute(select(_TxnThing))).scalars().all() + visible_at_response_start.append(bool(rows)) + await send(message) + + await self.inner(scope, receive, spy) + + async with await _client(Probe(app)) as client: + assert (await client.post("/things", params={"name": "durable"})).status_code == 201 + + assert visible_at_response_start == [True], ( + "row was not committed by the time the response left the server" + ) + + async def test_endpoint_exception_still_rolls_back(self, db_state): + """The middleware must not turn a failed request's writes into a commit.""" + async with await _client(_build_app(db_state)) as client: + assert (await client.post("/boom")).status_code == 500 + + async with db_state.session_factory() as session: + assert (await session.execute(select(_TxnThing))).scalars().all() == [] + + async def test_commit_failure_becomes_a_500(self, db_state, monkeypatch): + """A commit that blows up at response.start replaces the response rather + than shipping a 201 for work that never landed.""" + from simple_module_db import transaction + + async def explode(session): + raise RuntimeError("commit failed") + + monkeypatch.setattr(transaction, "finalize_session", explode) + + async with await _client(_build_app(db_state)) as client: + response = await client.post("/things", params={"name": "nope"}) + + assert response.status_code == 500 + assert response.json() == {"detail": "Internal Server Error"} + + async def test_still_commits_without_the_middleware(self, db_state): + """get_db keeps its own fallback finalize, so the dependency works + standalone — in a WebSocket handler, or a test that builds no stack.""" + app = _build_app(db_state, with_middleware=False) + async with await _client(app) as client: + assert (await client.post("/things", params={"name": "solo"})).status_code == 201 + + async with db_state.session_factory() as session: + names = [t.name for t in (await session.execute(select(_TxnThing))).scalars().all()] + assert names == ["solo"] + + async def test_read_only_request_is_not_committed(self, db_state): + """Read-only handlers still exit via rollback — one round-trip cheaper.""" + app = _build_app(db_state) + async with await _client(app) as client: + assert (await client.get("/things/999")).json() == {"found": False} diff --git a/framework/hosting/simple_module_hosting/_phase_helpers.py b/framework/hosting/simple_module_hosting/_phase_helpers.py index 80d37cc2..06d02496 100644 --- a/framework/hosting/simple_module_hosting/_phase_helpers.py +++ b/framework/hosting/simple_module_hosting/_phase_helpers.py @@ -21,6 +21,7 @@ ) from simple_module_core.diagnostics import Diagnostic, DiagnosticLevel from simple_module_core.exceptions import NotFoundError +from simple_module_db import CommitBeforeResponseMiddleware from starlette.exceptions import HTTPException from starlette.middleware.gzip import GZipMiddleware from starlette.middleware.sessions import SessionMiddleware @@ -83,8 +84,13 @@ def install_middleware( Order matters: last added = first executed. Execution order: (ProxyHeaders, if trusted_proxy) → CorrelationId → RequestLogging - → Security → Session → [module] → (Tenant, if multi_tenant) → Locale → Inertia. + → Security → Session → [module] → (Tenant, if multi_tenant) → Locale + → Inertia → CommitBeforeResponse. """ + # Added first, so it is innermost and its send-wrapper is the first to see + # the response: the request's DB work commits before any byte reaches the + # client, instead of in get_db's post-response exit code (GH #257). + app.add_middleware(CommitBeforeResponseMiddleware) app.add_middleware( InertiaLayoutDataMiddleware, menu_registry=menu_registry, diff --git a/framework/hosting/simple_module_hosting/assets.py b/framework/hosting/simple_module_hosting/assets.py index 2e57745d..11ce696b 100644 --- a/framework/hosting/simple_module_hosting/assets.py +++ b/framework/hosting/simple_module_hosting/assets.py @@ -47,6 +47,8 @@ THEME_CSS = "theme.css" STYLES_CSS = "styles.css" PACKAGE_JSON = "package.json" +PAGES_DIR = "pages" +COMPONENTS_DIR = "components" @dataclass(frozen=True) @@ -60,6 +62,7 @@ class ModuleAssets: theme_css: Path | None styles_css: Path | None npm_name: str | None = None + components_dir: Path | None = None def find_npm_name(pkg_root: Path) -> str | None: @@ -114,7 +117,8 @@ def compute_module_assets(modules: Sequence[ModuleBase]) -> list[ModuleAssets]: "Module '%s': package %s not importable — skipping", mod.meta.name, pkg_name ) continue - pages_dir = pkg_root / "pages" + pages_dir = pkg_root / PAGES_DIR + components_dir = pkg_root / COMPONENTS_DIR theme = pkg_root / THEME_CSS styles = pkg_root / STYLES_CSS entry = ModuleAssets( @@ -125,8 +129,9 @@ def compute_module_assets(modules: Sequence[ModuleBase]) -> list[ModuleAssets]: theme_css=theme.resolve() if theme.is_file() else None, styles_css=styles.resolve() if styles.is_file() else None, npm_name=find_npm_name(pkg_root), + components_dir=components_dir.resolve() if components_dir.is_dir() else None, ) - if entry.pages_dir or entry.theme_css or entry.styles_css: + if entry.pages_dir or entry.theme_css or entry.styles_css or entry.components_dir: result.append(entry) return result @@ -157,13 +162,23 @@ def render_modules_css( ``@import`` is emitted for *every* module, in-repo and wheel alike, because there is no static-glob equivalent for CSS. + Both ``pages/`` and ``components/`` are scanned. A wheel module's widgets + live under ``components/``, and leaving them out meant every host had to + hand-write a ``@source`` pointing into ``.venv`` — which cannot be written + portably: POSIX venvs nest under ``lib/python3.x/site-packages`` while + Windows uses ``Lib/site-packages``. Tailwind accepts a glob that matches + nothing without complaint, so the hand-written POSIX path silently dropped + every widget class on Windows. Emitting the resolved absolute path here + works on both. See GH #258. + Every path is absolute, so nothing here depends on the host's ``vite.config.ts`` — see the module docstring for why that matters. """ source_lines = [ - f'@source "{e.pages_dir.as_posix()}/**/*.{{ts,tsx}}";' + f'@source "{d.as_posix()}/**/*.{{ts,tsx}}";' for e in assets - if e.pages_dir and not in_repo(e.pages_dir) + for d in (e.pages_dir, e.components_dir) + if d and not in_repo(d) ] theme_lines = [f'@import "{e.theme_css.as_posix()}";' for e in assets if e.theme_css] style_lines = [ @@ -205,6 +220,7 @@ def render_assets_json(assets: Sequence[ModuleAssets]) -> str: "package_name": e.package_name, "package": e.package_dir.as_posix(), "pages": e.pages_dir.as_posix() if e.pages_dir else None, + "components": e.components_dir.as_posix() if e.components_dir else None, "theme": e.theme_css.as_posix() if e.theme_css else None, "styles": e.styles_css.as_posix() if e.styles_css else None, "npm_name": e.npm_name, diff --git a/framework/hosting/tests/test_middleware_order.py b/framework/hosting/tests/test_middleware_order.py index ff184363..32a5dff6 100644 --- a/framework/hosting/tests/test_middleware_order.py +++ b/framework/hosting/tests/test_middleware_order.py @@ -3,7 +3,8 @@ CLAUDE.md spells out the pipeline: CorrelationId → RequestLogging → GZip → Security → Session → - → Tenant (opt-in) → Locale → InertiaLayoutData → app + → Tenant (opt-in) → Locale → InertiaLayoutData + → CommitBeforeResponse → app Tenant/Locale must see ``request.state.user`` set by AuthMiddleware so DB queries get filtered correctly; CorrelationId must wrap everything so @@ -13,6 +14,11 @@ to be fully hidden. That inversion breaks the feature without failing any site_lock unit test, which is why the order is pinned here. +CommitBeforeResponse is innermost so its send-wrapper is the first to see +``http.response.start`` — that is what makes the request's DB work commit +before any byte reaches the client (GH #257). Anything added inside it would +run after the commit. + GZip sits inside the observability pair so those still see every request, but outside everything that produces a body — including the /static mount, which is where compression pays off most. @@ -39,6 +45,7 @@ "TenantMiddleware", "LocaleMiddleware", "InertiaLayoutDataMiddleware", + "CommitBeforeResponseMiddleware", ) _EXPECTED_SINGLE_TENANT = ( @@ -51,6 +58,7 @@ "AuthMiddleware", "LocaleMiddleware", "InertiaLayoutDataMiddleware", + "CommitBeforeResponseMiddleware", ) diff --git a/host/migrations/env.py b/host/migrations/env.py index a9392915..b3f7f1e0 100644 --- a/host/migrations/env.py +++ b/host/migrations/env.py @@ -9,6 +9,7 @@ import logging from logging.config import fileConfig +from pathlib import Path from alembic import context from simple_module_db import ( @@ -19,6 +20,7 @@ ) from simple_module_hosting.settings import Settings from sqlalchemy import engine_from_config, pool +from sqlalchemy.engine import make_url logger = logging.getLogger("alembic.env") @@ -42,9 +44,21 @@ def _get_url() -> str: - """Read database URL from settings, convert async to sync driver.""" + """Read database URL from settings, convert async to sync driver. + + The resolved URL is logged because ``Settings`` reads ``.env`` relative to + the *current working directory*: run alembic from the wrong cwd and it + silently falls back to the default SQLite file while the app talks to the + configured database. Printing the target — password masked — turns that + into something you notice on the first migration instead of a schema that + lives in a database nobody reads. + """ settings = Settings() - return settings.database_url.replace("+aiosqlite", "").replace("+asyncpg", "+psycopg2") + url = settings.database_url.replace("+aiosqlite", "").replace("+asyncpg", "+psycopg2") + logger.info( + "Migrating %s (cwd=%s)", make_url(url).render_as_string(hide_password=True), Path.cwd() + ) + return url def run_migrations_offline() -> None: From 05f70b79821c0580a58aafdb4f51d0b244ccf893 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 15 Aug 2026 16:44:55 +0000 Subject: [PATCH 2/2] fix: repair write loss from commit-before-response; finish #262 and #258 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the previous commit, from review findings. **Write loss (regression in the #257 fix).** Finalizing at http.response.start claimed the session one-shot, but work legitimately continues after that message: Starlette runs BackgroundTasks once the body is sent, and a StreamingResponse writes its body afterwards — both on the same session. Those writes flushed and never committed, and get_db's fallback no-op'd because the session was already claimed. Reproduced: a background task that adds a row persisted nothing with the middleware and persisted correctly without it. finalize_session is now re-armable rather than one-shot — each call commits whatever is pending at that moment, and get_db's exit code still runs to catch work done after the response started. The pending-write marker is cleared on settle so a committed session is never re-committed, and a failed commit is settled too so the fallback cannot retry it. Read-only requests still pay for exactly one rollback. Tests cover both the BackgroundTasks and StreamingResponse paths; both fail against the previous commit. **Test that did not test what it claimed.** test_commit_failure_becomes_a_500 stubbed finalize_session, bypassing the bookkeeping, so get_db's fallback committed the row the test called lost — it asserted only the status code and would have passed with the guarantee broken. It now injects the failure at AsyncSession.commit and asserts the table is empty. **#262 was only half fixed.** The Makefile hunk fixed later `make migrate` runs, but `smpy new` still ran the baseline autogenerate and `upgrade heads` with cwd=host/. The scaffolded default URL is itself root-relative (sqlite+aiosqlite:///./host/app.db), so that bootstrap wrote host/host/app.db while the app and the now-fixed `make migrate` use host/app.db — the same defect one step earlier, and newly divergent. Both now run from the project root, with the ini path collapsing for the flat create-host layout. **#258 was only half fixed.** Tailwind now scans a wheel module's components/, but Vite's bare-specifier fallback resolver only fires for importers under a module's pages/ prefix, so a widget's `@simple-module-py/ui` import failed to resolve. components/ now contributes its prefix and an optimizeDeps entry, in the host and in the scaffold template. Splitting the scaffold's vite.config.ts into module-assets.ts mirrors the split the host already made for the same reason, and keeps both under the 300-line cap; the migration-bootstrap tests move to their own file on the same grounds. Known limitation, now documented in the middleware: with more than one enlisted session (Depends(get_db, use_cache=False)), a failure part-way leaves earlier commits durable while the client sees a 500. Claude-Session: https://claude.ai/code/session_015fCFfMiqGce8unVpVezJG7 --- framework/cli/simple_module_cli/new.py | 32 ++++- .../host/client_app/module-assets.ts | 127 ++++++++++++++++++ .../templates/host/client_app/vite.config.ts | 98 ++------------ .../tests/test_cli_new_migration_bootstrap.py | 85 ++++++++++++ .../cli/tests/test_cli_new_regressions.py | 48 ------- framework/db/simple_module_db/transaction.py | 65 ++++++--- framework/db/tests/test_transaction.py | 71 +++++++++- host/client_app/module-assets.ts | 20 ++- 8 files changed, 379 insertions(+), 167 deletions(-) create mode 100644 framework/cli/simple_module_cli/templates/host/client_app/module-assets.ts create mode 100644 framework/cli/tests/test_cli_new_migration_bootstrap.py diff --git a/framework/cli/simple_module_cli/new.py b/framework/cli/simple_module_cli/new.py index 0cfaa462..66dee728 100644 --- a/framework/cli/simple_module_cli/new.py +++ b/framework/cli/simple_module_cli/new.py @@ -20,6 +20,25 @@ _ALEMBIC = ("uv", "run", "alembic") +def _alembic_argv(target: Path, host_dir: Path) -> list[str]: + """Alembic argv to run **from the project root** (``target``). + + Never from ``host/``. ``BootstrapSettings`` reads ``.env`` relative to the + cwd, and the scaffolded default database URL is itself root-relative + (``sqlite+aiosqlite:///./host/app.db``), so bootstrapping from ``host/`` + migrates ``host/host/app.db`` while the app — and ``make migrate``, which + now also runs from the root — use ``host/app.db``. Same defect as GH #262, + one step earlier: the scaffold's own first migration lands in a database + nothing else reads. + + In the flat ``create-host`` layout the host *is* the project root, so the + ini path collapses to ``alembic.ini``. + """ + if host_dir == target: + return [*_ALEMBIC, "-c", "alembic.ini"] + return ["uv", "run", "--project", "host", "alembic", "-c", "host/alembic.ini"] + + class Db(StrEnum): sqlite = "sqlite" postgres = "postgres" @@ -164,27 +183,30 @@ def new_project( ) return - _bootstrap_initial_migration(host_dir) + alembic = _alembic_argv(target, host_dir) + _bootstrap_initial_migration(target, host_dir, alembic) # `heads` (plural) applies every per-module branch head; `head` (singular) # errors once a second module ships its own migration branch label. - subprocess.run([*_ALEMBIC, "upgrade", "heads"], cwd=host_dir, check=False) + subprocess.run([*alembic, "upgrade", "heads"], cwd=target, check=False) typer.echo("\nSetup complete. Run `make dev` in the new directory.") typer.echo("To run the full stack in containers instead: make docker-up") if "background_tasks" in resolved: typer.echo("For background jobs, also run: docker compose up -d redis worker beat") -def _bootstrap_initial_migration(host_dir: Path) -> None: +def _bootstrap_initial_migration(target: Path, host_dir: Path, alembic: list[str]) -> None: """Autogenerate the baseline migration if the scaffold ships none. Without a real revision, ``alembic upgrade head`` is a silent no-op against an empty schema — the bundled modules' tables never exist. + + Runs from ``target`` (the project root) — see :func:`_alembic_argv`. """ versions_dir = host_dir / "migrations" / "versions" if any(p.name != "__init__.py" for p in versions_dir.glob("*.py")): return subprocess.run( - [*_ALEMBIC, "revision", "--autogenerate", "-m", "initial schema"], - cwd=host_dir, + [*alembic, "revision", "--autogenerate", "-m", "initial schema"], + cwd=target, check=False, ) diff --git a/framework/cli/simple_module_cli/templates/host/client_app/module-assets.ts b/framework/cli/simple_module_cli/templates/host/client_app/module-assets.ts new file mode 100644 index 00000000..0efacbfc --- /dev/null +++ b/framework/cli/simple_module_cli/templates/host/client_app/module-assets.ts @@ -0,0 +1,127 @@ +// What the installed Python modules contribute to the frontend build. +// +// Split out of vite.config.ts to keep each file under the 300-line cap and to +// give this one job a name. Everything here is derived from the two files +// `smpy gen-pages` writes: `modules.manifest.json` (name -> absolute pages/ +// dir) and the richer `modules.assets.json`. Both are absent until gen-pages +// has run, which is a normal state — a fresh checkout resolves to empty. +import fs from 'node:fs'; +import path from 'node:path'; + +export type Alias = { find: string; replacement: string }; + +export type ModuleAssetIndex = { + fsAllow: string[]; + optimizeEntries: string[]; + pkgJsonPaths: string[]; + /** `` prefixes for module-owned TSX (pages/ and components/). */ + pagesPrefixes: string[]; + aliases: Alias[]; + npmNames: Set; +}; + +export function loadModuleAssets(clientAppDir: string): ModuleAssetIndex { + // Load the module pages manifest written by the Python host at boot. + // Each entry points at an absolute pages/ directory — typically inside a + // pip-installed module wheel. Vite needs these in server.fs.allow so the + // dev server can read files outside the host root, and in + // optimizeDeps.entries so its dependency scanner discovers bare imports + // from wheel-installed pages and pre-bundles them. + // + // We also collect each module's package.json — wheels embed it next to + // the Python package (one level up from pages/, force-included by Hatch), + // while editable/workspace installs leave it at the source-tree module + // root (two levels up). We accept either. The dep walk in + // `collectOptimizeIncludes` uses it to reach packages a module's pages + // import directly (`sonner`, `lucide-react`, `maplibre-gl`, …). Without + // this seed, Vite's pre-bundler never sees those bare specifiers and Node + // module resolution walks up from inside .venv/site-packages — never + // reaching host/client_app/node_modules. + const manifestPath = path.resolve(clientAppDir, 'modules.manifest.json'); + const fsAllow: string[] = []; + const optimizeEntries: string[] = []; + const pkgJsonPaths: string[] = []; + const pagesPrefixes: string[] = []; + if (fs.existsSync(manifestPath)) { + const manifest = JSON.parse(fs.readFileSync(manifestPath, 'utf-8')) as Record; + for (const pagesDir of Object.values(manifest)) { + const pkgDir = path.dirname(pagesDir); + fsAllow.push(pkgDir); + optimizeEntries.push(path.join(pagesDir, '**/*.tsx')); + pagesPrefixes.push(pagesDir + path.sep); + for (const candidate of [ + path.join(pkgDir, 'package.json'), + path.join(path.dirname(pkgDir), 'package.json'), + ]) { + if (fs.existsSync(candidate)) { + pkgJsonPaths.push(candidate); + break; + } + } + } + } + + // Three things come out of modules.assets.json. + // + // 1. `server.fs.allow` entries. The dev server must be allowed to read each + // module's package dir. Read from modules.assets.json rather than + // modules.manifest.json because the manifest is keyed off `pages/`, so a + // module shipping only CSS never appears in it. + // + // 2. A convenience `#module/` alias. This is NOT required by + // `modules.generated.css` — that file imports module stylesheets by + // absolute path, so it resolves with no alias configured at all. Emitting + // an alias there made a generated file depend on this hand-owned config, + // and since `vite.config.ts` is scaffolded once and then owned by the app, + // a Python-only version bump broke every host scaffolded earlier + // (GH issue #253). The alias stays because it costs nothing. + // + // 3. An `` alias per module, so one module can import another's + // TS/TSX by package name. Aimed at the module's *Python package* dir — + // a wheel ships `site-packages/foo/**` and nothing above it, so the + // source-tree module root is not a target both layouts have. Needed in + // both: a wheel module is never in node_modules, and npm symlinks a + // workspace member onto the module root, one level too high. + // See docs/module-authoring.md § Importing another module's TS/TSX. + // + // `@tailwindcss/vite` builds its CSS import resolver with + // `createResolver({ ...config.resolve, ... })`, so `resolve.alias` governs + // CSS `@import` as well as JS — verified against @tailwindcss/vite 4.2.4. + type ModuleAsset = { + package_name: string; + package: string; + npm_name?: string | null; + // Wheel modules ship widgets here; the pages-keyed manifest never sees them. + components?: string | null; + }; + const aliases: { find: string; replacement: string }[] = []; + const npmNames = new Set(); + const assetsPath = path.resolve(clientAppDir, 'modules.assets.json'); + let assets: Record = {}; + try { + assets = JSON.parse(fs.readFileSync(assetsPath, 'utf-8')); + } catch { + // Absent until `smpy gen-pages` runs — proceed with no aliases. + } + for (const entry of Object.values(assets)) { + aliases.push({ find: `#module/${entry.package_name}`, replacement: entry.package }); + if (entry.npm_name) { + aliases.push({ find: entry.npm_name, replacement: entry.package }); + npmNames.add(entry.npm_name); + } + if (!fsAllow.includes(entry.package)) fsAllow.push(entry.package); + // Without this the bare-specifier fallback below skips widgets, and a + // component's `@simple-module-py/ui` import fails to resolve. + if (entry.components) { + const prefix = entry.components + path.sep; + if (!pagesPrefixes.includes(prefix)) pagesPrefixes.push(prefix); + optimizeEntries.push(path.join(entry.components, '**/*.tsx')); + } + } + // Keep the alias list in a stable, longest-first order. Vite matches a string + // `find` on exact equality or a `/`-bounded prefix, so `#module/gis` could not + // swallow `#module/gis_extra` in any order — this is just determinism, not a + // correctness fix. + aliases.sort((a, b) => b.find.length - a.find.length); + return { fsAllow, optimizeEntries, pkgJsonPaths, pagesPrefixes, aliases, npmNames }; +} diff --git a/framework/cli/simple_module_cli/templates/host/client_app/vite.config.ts b/framework/cli/simple_module_cli/templates/host/client_app/vite.config.ts index d3bcb058..314b3e0d 100644 --- a/framework/cli/simple_module_cli/templates/host/client_app/vite.config.ts +++ b/framework/cli/simple_module_cli/templates/host/client_app/vite.config.ts @@ -3,6 +3,7 @@ import path from 'node:path'; import tailwindcss from '@tailwindcss/vite'; import react from '@vitejs/plugin-react'; import { type Plugin, defineConfig } from 'vite'; +import { loadModuleAssets } from './module-assets'; // Force every importer (host, workspace module, wheel-installed module) // to resolve to one React copy + a single Inertia hook context. Without @@ -29,95 +30,14 @@ function findNodeModulesRoot(start: string): string { } const fsRoot = findNodeModulesRoot(__dirname); -// Load the module pages manifest written by the Python host at boot. -// Each entry points at an absolute pages/ directory — typically inside a -// pip-installed module wheel. Vite needs these in server.fs.allow so the -// dev server can read files outside the host root, and in -// optimizeDeps.entries so its dependency scanner discovers bare imports -// from wheel-installed pages and pre-bundles them. -// -// We also collect each module's package.json — wheels embed it next to -// the Python package (one level up from pages/, force-included by Hatch), -// while editable/workspace installs leave it at the source-tree module -// root (two levels up). We accept either. The dep walk in -// `collectOptimizeIncludes` uses it to reach packages a module's pages -// import directly (`sonner`, `lucide-react`, `maplibre-gl`, …). Without -// this seed, Vite's pre-bundler never sees those bare specifiers and Node -// module resolution walks up from inside .venv/site-packages — never -// reaching host/client_app/node_modules. -const manifestPath = path.resolve(__dirname, 'modules.manifest.json'); -const moduleFsAllow: string[] = []; -const moduleOptimizeEntries: string[] = []; -const modulePkgJsonPaths: string[] = []; -const modulePagesPrefixes: string[] = []; -if (fs.existsSync(manifestPath)) { - const manifest = JSON.parse(fs.readFileSync(manifestPath, 'utf-8')) as Record; - for (const pagesDir of Object.values(manifest)) { - const pkgDir = path.dirname(pagesDir); - moduleFsAllow.push(pkgDir); - moduleOptimizeEntries.push(path.join(pagesDir, '**/*.tsx')); - modulePagesPrefixes.push(pagesDir + path.sep); - for (const candidate of [ - path.join(pkgDir, 'package.json'), - path.join(path.dirname(pkgDir), 'package.json'), - ]) { - if (fs.existsSync(candidate)) { - modulePkgJsonPaths.push(candidate); - break; - } - } - } -} - -// Three things come out of modules.assets.json. -// -// 1. `server.fs.allow` entries. The dev server must be allowed to read each -// module's package dir. Read from modules.assets.json rather than -// modules.manifest.json because the manifest is keyed off `pages/`, so a -// module shipping only CSS never appears in it. -// -// 2. A convenience `#module/` alias. This is NOT required by -// `modules.generated.css` — that file imports module stylesheets by -// absolute path, so it resolves with no alias configured at all. Emitting -// an alias there made a generated file depend on this hand-owned config, -// and since `vite.config.ts` is scaffolded once and then owned by the app, -// a Python-only version bump broke every host scaffolded earlier -// (GH issue #253). The alias stays because it costs nothing. -// -// 3. An `` alias per module, so one module can import another's -// TS/TSX by package name. Aimed at the module's *Python package* dir — -// a wheel ships `site-packages/foo/**` and nothing above it, so the -// source-tree module root is not a target both layouts have. Needed in -// both: a wheel module is never in node_modules, and npm symlinks a -// workspace member onto the module root, one level too high. -// See docs/module-authoring.md § Importing another module's TS/TSX. -// -// `@tailwindcss/vite` builds its CSS import resolver with -// `createResolver({ ...config.resolve, ... })`, so `resolve.alias` governs -// CSS `@import` as well as JS — verified against @tailwindcss/vite 4.2.4. -type ModuleAsset = { package_name: string; package: string; npm_name?: string | null }; -const moduleAliases: { find: string; replacement: string }[] = []; -const moduleNpmNames = new Set(); -const assetsPath = path.resolve(__dirname, 'modules.assets.json'); -let moduleAssets: Record = {}; -try { - moduleAssets = JSON.parse(fs.readFileSync(assetsPath, 'utf-8')); -} catch { - // Absent until `smpy gen-pages` runs — proceed with no aliases. -} -for (const entry of Object.values(moduleAssets)) { - moduleAliases.push({ find: `#module/${entry.package_name}`, replacement: entry.package }); - if (entry.npm_name) { - moduleAliases.push({ find: entry.npm_name, replacement: entry.package }); - moduleNpmNames.add(entry.npm_name); - } - if (!moduleFsAllow.includes(entry.package)) moduleFsAllow.push(entry.package); -} -// Keep the alias list in a stable, longest-first order. Vite matches a string -// `find` on exact equality or a `/`-bounded prefix, so `#module/gis` could not -// swallow `#module/gis_extra` in any order — this is just determinism, not a -// correctness fix. -moduleAliases.sort((a, b) => b.find.length - a.find.length); +const { + fsAllow: moduleFsAllow, + optimizeEntries: moduleOptimizeEntries, + pkgJsonPaths: modulePkgJsonPaths, + pagesPrefixes: modulePagesPrefixes, + aliases: moduleAliases, + npmNames: moduleNpmNames, +} = loadModuleAssets(__dirname); const fakeWorkspaceImporter = path.join(fsRoot, 'package.json'); // CJS-only deps like `clsx`, `tailwind-merge`, `class-variance-authority` diff --git a/framework/cli/tests/test_cli_new_migration_bootstrap.py b/framework/cli/tests/test_cli_new_migration_bootstrap.py new file mode 100644 index 00000000..97c7cc69 --- /dev/null +++ b/framework/cli/tests/test_cli_new_migration_bootstrap.py @@ -0,0 +1,85 @@ +"""Where `smpy new` runs alembic when it bootstraps a scaffold's first migration. + +Split out of test_cli_new_regressions.py to stay under the repo's 300-line cap. +The cwd these run in is the whole point — see GH #262. +""" + +from __future__ import annotations + +from pathlib import Path + + +def test_bootstrap_initial_migration_runs_autogenerate_when_versions_empty( + tmp_path: Path, monkeypatch +) -> None: + """Issue #135: the post-install hook must call ``alembic revision + --autogenerate`` when ``migrations/versions/`` holds only ``.gitkeep``.""" + from simple_module_cli import new as new_mod + + host = tmp_path / "host" + (host / "migrations" / "versions").mkdir(parents=True) + (host / "migrations" / "versions" / ".gitkeep").touch() + + calls: list[tuple[list[str], Path]] = [] + + def fake_run(cmd, *, cwd, check): + del check + calls.append((list(cmd), Path(cwd))) + + class _Result: + returncode = 0 + + return _Result() + + monkeypatch.setattr(new_mod.subprocess, "run", fake_run) + argv = new_mod._alembic_argv(tmp_path, host) + new_mod._bootstrap_initial_migration(tmp_path, host, argv) + assert calls, "expected alembic autogenerate to run" + cmd, cwd = calls[0] + assert cmd[:7] == [ + "uv", + "run", + "--project", + "host", + "alembic", + "-c", + "host/alembic.ini", + ] + assert cmd[7:9] == ["revision", "--autogenerate"] + # GH #262: from the project root, never host/. BootstrapSettings resolves + # .env against the cwd, and the scaffolded default URL is root-relative + # (./host/app.db) — bootstrapping from host/ writes host/host/app.db, which + # neither the app nor `make migrate` ever reads. + assert cwd == tmp_path + + +def test_bootstrap_initial_migration_skips_when_revision_exists( + tmp_path: Path, monkeypatch +) -> None: + """If the user has already run ``make migration``, don't clobber their + revision by autogenerating a second baseline.""" + from simple_module_cli import new as new_mod + + host = tmp_path / "host" + (host / "migrations" / "versions").mkdir(parents=True) + (host / "migrations" / "versions" / "0001_initial.py").write_text("# revision\n") + + def fake_run(*_a, **_kw): # pragma: no cover - must not be called + raise AssertionError("alembic should not be invoked when a revision exists") + + monkeypatch.setattr(new_mod.subprocess, "run", fake_run) + new_mod._bootstrap_initial_migration(tmp_path, host, new_mod._alembic_argv(tmp_path, host)) + + +def test_alembic_argv_collapses_for_the_flat_host_layout(tmp_path: Path) -> None: + """`create-host` puts the host *at* the project root, so there is no + `host/` segment to point the ini path at.""" + from simple_module_cli import new as new_mod + + assert new_mod._alembic_argv(tmp_path, tmp_path) == [ + "uv", + "run", + "alembic", + "-c", + "alembic.ini", + ] diff --git a/framework/cli/tests/test_cli_new_regressions.py b/framework/cli/tests/test_cli_new_regressions.py index 252ebd91..ca7412cf 100644 --- a/framework/cli/tests/test_cli_new_regressions.py +++ b/framework/cli/tests/test_cli_new_regressions.py @@ -224,51 +224,3 @@ def test_sm_new_no_install_next_steps_include_initial_migration(tmp_path: Path) assert result.exit_code == 0, result.output assert 'make migration msg="initial schema"' in result.output assert "make migrate" in result.output - - -def test_bootstrap_initial_migration_runs_autogenerate_when_versions_empty( - tmp_path: Path, monkeypatch -) -> None: - """Issue #135: the post-install hook must call ``alembic revision - --autogenerate`` when ``migrations/versions/`` holds only ``.gitkeep``.""" - from simple_module_cli import new as new_mod - - host = tmp_path / "host" - (host / "migrations" / "versions").mkdir(parents=True) - (host / "migrations" / "versions" / ".gitkeep").touch() - - calls: list[tuple[list[str], Path]] = [] - - def fake_run(cmd, *, cwd, check): - del check - calls.append((list(cmd), Path(cwd))) - - class _Result: - returncode = 0 - - return _Result() - - monkeypatch.setattr(new_mod.subprocess, "run", fake_run) - new_mod._bootstrap_initial_migration(host) - assert calls, "expected alembic autogenerate to run" - cmd, cwd = calls[0] - assert cmd[:5] == ["uv", "run", "alembic", "revision", "--autogenerate"] - assert cwd == host - - -def test_bootstrap_initial_migration_skips_when_revision_exists( - tmp_path: Path, monkeypatch -) -> None: - """If the user has already run ``make migration``, don't clobber their - revision by autogenerating a second baseline.""" - from simple_module_cli import new as new_mod - - host = tmp_path / "host" - (host / "migrations" / "versions").mkdir(parents=True) - (host / "migrations" / "versions" / "0001_initial.py").write_text("# revision\n") - - def fake_run(*_a, **_kw): # pragma: no cover - must not be called - raise AssertionError("alembic should not be invoked when a revision exists") - - monkeypatch.setattr(new_mod.subprocess, "run", fake_run) - new_mod._bootstrap_initial_migration(host) diff --git a/framework/db/simple_module_db/transaction.py b/framework/db/simple_module_db/transaction.py index 9f5d2e6d..8871806b 100644 --- a/framework/db/simple_module_db/transaction.py +++ b/framework/db/simple_module_db/transaction.py @@ -51,32 +51,52 @@ def _elapsed_ms(session: AsyncSession) -> float: return round((time.perf_counter() - start) * 1000, 2) if start else 0.0 -def _claim(session: AsyncSession) -> bool: - """Return True if this caller is the one that gets to finalize ``session``.""" - if session.info.get(_FINALIZED_KEY): - return False +def _has_pending(session: AsyncSession) -> bool: + """Whether ``session`` holds work that still needs committing. + + ``has_writes`` is stamped by the after_flush listener and survives the flush + emptying ``session.new``/``.dirty``/``.deleted`` — the raw collections alone + would report a flushed-but-uncommitted write as read-only. + """ + return bool( + session.info.get(SESSION_HAS_WRITES_KEY) or session.new or session.dirty or session.deleted + ) + + +def _settle(session: AsyncSession) -> None: + """Mark the session finalized and clear the pending-write marker. + + Clearing matters: the marker is sticky, so without this a later finalize + would try to re-commit a session whose work has already landed. + """ session.info[_FINALIZED_KEY] = True - return True + session.info.pop(SESSION_HAS_WRITES_KEY, None) async def finalize_session(session: AsyncSession) -> None: - """Commit ``session`` if it has pending writes, else roll it back. Idempotent. + """Commit ``session`` if it has pending writes, else roll it back. + + Safe to call more than once, and deliberately **re-armable** rather than + one-shot. Work can still be done after the response has started — Starlette + runs ``BackgroundTasks`` once the body is sent, and a ``StreamingResponse`` + writes its body after ``http.response.start`` — and both share this request's + session. A one-shot claim would let the middleware's commit consume the + session and then silently drop those later writes: they would flush and + never commit. So each call commits whatever is pending *at that moment*, and + ``get_db``'s exit code still runs afterwards to catch the rest. Read-only handlers exit via ``rollback`` — one round-trip cheaper than ``commit``, and it keeps read-only queries from showing up as writes in - query logs / ``pg_stat_statements``. + query logs / ``pg_stat_statements``. A repeat call with nothing new pending + returns immediately rather than paying for a second rollback. On commit failure the session is rolled back before the error propagates, so the caller never has to reason about a half-finalized session. """ - if not _claim(session): - return - # ``has_writes`` is set by the after_flush listener and survives the flush - # emptying session.new/.dirty/.deleted. - has_pending = bool( - session.info.get(SESSION_HAS_WRITES_KEY) or session.new or session.dirty or session.deleted - ) - if not has_pending: + if not _has_pending(session): + if session.info.get(_FINALIZED_KEY): + return + _settle(session) await session.rollback() logger.debug( "db.session.read_only", @@ -88,6 +108,10 @@ async def finalize_session(session: AsyncSession) -> None: except Exception: await session.rollback() raise + finally: + # Settled either way: a failed commit must not be retried by the + # fallback finalize in get_db's exit code. + _settle(session) logger.info( "db.session.commit", extra={"operation": "commit", "db_duration_ms": _elapsed_ms(session)}, @@ -95,9 +119,10 @@ async def finalize_session(session: AsyncSession) -> None: async def rollback_session(session: AsyncSession) -> None: - """Roll ``session`` back and mark it finalized. Idempotent.""" - if not _claim(session): + """Roll ``session`` back and drop its pending work. Idempotent.""" + if session.info.get(_FINALIZED_KEY) and not _has_pending(session): return + _settle(session) await session.rollback() logger.warning( "db.session.rollback", @@ -151,6 +176,12 @@ async def send_wrapper(message) -> None: return if message["type"] == "http.response.start" and sessions: try: + # A request normally has exactly one session (FastAPI caches + # the dependency). With several — Depends(get_db, + # use_cache=False) — a failure part-way leaves the earlier + # commits durable while the client sees a 500. There is no + # cross-session atomicity to recover here short of two-phase + # commit; the log line below is what makes it diagnosable. for session in sessions: await finalize_session(session) except Exception: diff --git a/framework/db/tests/test_transaction.py b/framework/db/tests/test_transaction.py index f690f0cb..574a1db8 100644 --- a/framework/db/tests/test_transaction.py +++ b/framework/db/tests/test_transaction.py @@ -18,7 +18,8 @@ import httpx import pytest from _models import _TxnBase, _TxnThing -from fastapi import Depends, FastAPI +from fastapi import BackgroundTasks, Depends, FastAPI +from fastapi.responses import StreamingResponse from simple_module_db.deps import get_db from simple_module_db.listeners import register_listeners from simple_module_db.session import init_db @@ -138,15 +139,20 @@ async def test_endpoint_exception_still_rolls_back(self, db_state): async with db_state.session_factory() as session: assert (await session.execute(select(_TxnThing))).scalars().all() == [] - async def test_commit_failure_becomes_a_500(self, db_state, monkeypatch): + async def test_commit_failure_becomes_a_500_and_persists_nothing(self, db_state, monkeypatch): """A commit that blows up at response.start replaces the response rather - than shipping a 201 for work that never landed.""" - from simple_module_db import transaction + than shipping a 201 for work that never landed. - async def explode(session): + The failure is injected at ``AsyncSession.commit`` rather than at + ``finalize_session``: stubbing the finalizer would bypass its bookkeeping, + leaving get_db's fallback free to commit the row the test claims was + lost — the assertion would pass while the guarantee was broken. + """ + + async def explode(self, *args, **kwargs): raise RuntimeError("commit failed") - monkeypatch.setattr(transaction, "finalize_session", explode) + monkeypatch.setattr(AsyncSession, "commit", explode) async with await _client(_build_app(db_state)) as client: response = await client.post("/things", params={"name": "nope"}) @@ -154,6 +160,59 @@ async def explode(session): assert response.status_code == 500 assert response.json() == {"detail": "Internal Server Error"} + monkeypatch.undo() + async with db_state.session_factory() as session: + assert (await session.execute(select(_TxnThing))).scalars().all() == [] + + async def test_background_task_writes_still_commit(self, db_state): + """Starlette runs BackgroundTasks after the body is sent, on this same + session. Finalizing at response.start must not consume the session and + strand them — they flushed but never committed, losing writes silently.""" + app = _build_app(db_state) + + @app.post("/with-task", status_code=202) + async def with_task(bg: BackgroundTasks, db: AsyncSession = Depends(get_db)): + db.add(_TxnThing(name="before-response")) + await db.flush() + + async def task(): + db.add(_TxnThing(name="from-background-task")) + await db.flush() + + bg.add_task(task) + return {"ok": True} + + async with await _client(app) as client: + assert (await client.post("/with-task")).status_code == 202 + + async with db_state.session_factory() as session: + names = sorted(t.name for t in (await session.execute(select(_TxnThing))).scalars()) + assert names == ["before-response", "from-background-task"] + + async def test_streaming_response_body_writes_still_commit(self, db_state): + """A StreamingResponse writes its body *after* response.start, so work + done while streaming lands after the middleware's commit.""" + app = _build_app(db_state) + + @app.get("/stream") + async def stream(db: AsyncSession = Depends(get_db)): + db.add(_TxnThing(name="before-stream")) + await db.flush() + + async def body(): + yield b"chunk" + db.add(_TxnThing(name="during-stream")) + await db.flush() + + return StreamingResponse(body()) + + async with await _client(app) as client: + assert (await client.get("/stream")).status_code == 200 + + async with db_state.session_factory() as session: + names = sorted(t.name for t in (await session.execute(select(_TxnThing))).scalars()) + assert names == ["before-stream", "during-stream"] + async def test_still_commits_without_the_middleware(self, db_state): """get_db keeps its own fallback finalize, so the dependency works standalone — in a WebSocket handler, or a test that builds no stack.""" diff --git a/host/client_app/module-assets.ts b/host/client_app/module-assets.ts index fc652a26..11e7ba8e 100644 --- a/host/client_app/module-assets.ts +++ b/host/client_app/module-assets.ts @@ -9,7 +9,12 @@ import fs from 'node:fs'; import path from 'node:path'; -type ModuleAsset = { package_name: string; package: string; npm_name?: string | null }; +type ModuleAsset = { + package_name: string; + package: string; + npm_name?: string | null; + components?: string | null; +}; export type Alias = { find: string; replacement: string }; @@ -20,7 +25,8 @@ export type ModuleAssetIndex = { optimizeEntries: string[]; /** Each module's package.json — its deps declare what its pages may import. */ pkgJsonPaths: string[]; - /** `` prefixes, for cheaply testing "is this importer a module page?". */ + /** `` prefixes for module-owned TSX (pages/ and components/), + * for cheaply testing "is this importer module source?". */ pagesPrefixes: string[]; /** `#module/` and `` aliases. */ aliases: Alias[]; @@ -102,6 +108,16 @@ export function loadModuleAssets(clientAppDir: string): ModuleAssetIndex { npmNames.add(entry.npm_name); } if (!fsAllow.includes(entry.package)) fsAllow.push(entry.package); + // Wheel modules ship widgets under components/, which the pages-keyed + // manifest above never sees. Without these the bare-specifier fallback + // resolver below skips them and a widget's `@simple-module-py/ui` import + // dies with "Failed to resolve import" — the JS half of GH #258, whose + // CSS half is the @source emission in simple_module_hosting.assets. + if (entry.components) { + const prefix = entry.components + path.sep; + if (!pagesPrefixes.includes(prefix)) pagesPrefixes.push(prefix); + optimizeEntries.push(path.join(entry.components, '**/*.tsx')); + } } // Stable, longest-first. Vite matches a string `find` on exact equality or a