fix(arch-043,arch-052): deterministic system tenant lookup; async-safe file metadata
Check Cross-Plugin Imports / check (push) Has been cancelled
Check Cross-Plugin Imports / check (push) Has been cancelled
This commit is contained in:
@@ -107,6 +107,10 @@ class Settings(BaseSettings):
|
|||||||
rate_limit_webhook_max: int = 100 # incoming webhooks
|
rate_limit_webhook_max: int = 100 # incoming webhooks
|
||||||
rate_limit_webhook_window: int = 60 # 1 minute
|
rate_limit_webhook_window: int = 60 # 1 minute
|
||||||
|
|
||||||
|
# System tenant — used by seeding/plugins that need a well-known default
|
||||||
|
# tenant (must match scripts/seed_admin.py slug).
|
||||||
|
system_tenant_slug: str = "default"
|
||||||
|
|
||||||
# LLM Cost Overrun Protection (B.17)
|
# LLM Cost Overrun Protection (B.17)
|
||||||
llm_monthly_budget_usd: float = 100.0 # per-tenant monthly LLM budget
|
llm_monthly_budget_usd: float = 100.0 # per-tenant monthly LLM budget
|
||||||
llm_hard_cutoff: bool = True # block LLM calls when budget exceeded
|
llm_hard_cutoff: bool = True # block LLM calls when budget exceeded
|
||||||
|
|||||||
@@ -355,6 +355,23 @@ async def close_engine() -> None:
|
|||||||
_migration_session_factory = None
|
_migration_session_factory = None
|
||||||
|
|
||||||
|
|
||||||
|
async def get_system_tenant(db: AsyncSession):
|
||||||
|
"""Return the well-known system tenant, or ``None`` if it does not exist.
|
||||||
|
|
||||||
|
Resolves by configured slug (``settings.system_tenant_slug``, default
|
||||||
|
``"default"`` as created by ``scripts/seed_admin.py``) instead of an
|
||||||
|
arbitrary first row, so multi-tenant databases stay deterministic.
|
||||||
|
"""
|
||||||
|
from sqlalchemy import select
|
||||||
|
|
||||||
|
from app.config import get_settings
|
||||||
|
from app.models.tenant import Tenant # lazy: models import this module's Base
|
||||||
|
|
||||||
|
slug = get_settings().system_tenant_slug
|
||||||
|
result = await db.execute(select(Tenant).where(Tenant.slug == slug).limit(1))
|
||||||
|
return result.scalar_one_or_none()
|
||||||
|
|
||||||
|
|
||||||
def reset_engine_for_testing(engine: AsyncEngine) -> async_sessionmaker[AsyncSession]:
|
def reset_engine_for_testing(engine: AsyncEngine) -> async_sessionmaker[AsyncSession]:
|
||||||
"""Replace all global engines with a test engine. Returns a session factory.
|
"""Replace all global engines with a test engine. Returns a session factory.
|
||||||
|
|
||||||
|
|||||||
+40
-12
@@ -501,11 +501,38 @@ async def save_with_metadata(
|
|||||||
}
|
}
|
||||||
|
|
||||||
|
|
||||||
|
async def get_file_metadata_async(path: str) -> dict[str, Any]:
|
||||||
|
"""Awaitable variant of :func:`get_file_metadata` (ARCH-052).
|
||||||
|
|
||||||
|
Safe to call from inside a running event loop — never creates a
|
||||||
|
nested one. For local storage this is plain filesystem access; for
|
||||||
|
S3 and other async backends the backend's ``exists()`` is awaited.
|
||||||
|
"""
|
||||||
|
backend = get_storage_backend()
|
||||||
|
if isinstance(backend, LocalStorage):
|
||||||
|
full_path = backend._full_path(path)
|
||||||
|
if not os.path.exists(full_path):
|
||||||
|
return {"size": None, "modified": None, "exists": False}
|
||||||
|
stat = os.stat(full_path)
|
||||||
|
return {
|
||||||
|
"size": stat.st_size,
|
||||||
|
"modified": stat.st_mtime,
|
||||||
|
"exists": True,
|
||||||
|
}
|
||||||
|
# S3 or other async backends — await the backend directly
|
||||||
|
if not await backend.exists(path):
|
||||||
|
return {"size": None, "modified": None, "exists": False}
|
||||||
|
return {"size": None, "modified": None, "exists": True}
|
||||||
|
|
||||||
|
|
||||||
def get_file_metadata(path: str) -> dict[str, Any]:
|
def get_file_metadata(path: str) -> dict[str, Any]:
|
||||||
"""Read metadata of a stored file without loading its content.
|
"""Read metadata of a stored file without loading its content.
|
||||||
|
|
||||||
Works with the *local* storage backend. For S3, use the S3 client
|
Works with the *local* storage backend without touching the event
|
||||||
``stat_object`` API directly.
|
loop. For S3 and other async-only backends this drives the check
|
||||||
|
through ``asyncio.run``; calling it from inside a running event loop
|
||||||
|
raises ``RuntimeError`` — use :func:`get_file_metadata_async` there
|
||||||
|
instead (ARCH-052).
|
||||||
|
|
||||||
Parameters
|
Parameters
|
||||||
----------
|
----------
|
||||||
@@ -530,14 +557,15 @@ def get_file_metadata(path: str) -> dict[str, Any]:
|
|||||||
"modified": stat.st_mtime,
|
"modified": stat.st_mtime,
|
||||||
"exists": True,
|
"exists": True,
|
||||||
}
|
}
|
||||||
# S3 or other backends — fall back to exists() check
|
# Async-only backend outside a running loop is fine; inside one we
|
||||||
import asyncio as _asyncio
|
# must never build a nested event loop.
|
||||||
|
|
||||||
loop = _asyncio.new_event_loop()
|
|
||||||
try:
|
try:
|
||||||
exists = loop.run_until_complete(backend.exists(path))
|
asyncio.get_running_loop()
|
||||||
if not exists:
|
except RuntimeError:
|
||||||
return {"size": None, "modified": None, "exists": False}
|
pass
|
||||||
return {"size": None, "modified": None, "exists": True}
|
else:
|
||||||
finally:
|
raise RuntimeError(
|
||||||
loop.close()
|
"get_file_metadata() cannot be used with async storage backends "
|
||||||
|
"inside a running event loop — use get_file_metadata_async()"
|
||||||
|
)
|
||||||
|
return asyncio.run(get_file_metadata_async(path))
|
||||||
|
|||||||
@@ -246,11 +246,12 @@ class AutomationPlugin(BasePlugin):
|
|||||||
from app.plugins.builtins.automation.prebuilt.report_agent import create_report_agent
|
from app.plugins.builtins.automation.prebuilt.report_agent import create_report_agent
|
||||||
from sqlalchemy import select as sa_select
|
from sqlalchemy import select as sa_select
|
||||||
|
|
||||||
# Get first tenant + admin user for seeding
|
# Get system tenant + admin user for seeding (ARCH-043:
|
||||||
|
# deterministic slug lookup instead of arbitrary first row)
|
||||||
|
from app.core.db import get_system_tenant
|
||||||
from app.models.user import User
|
from app.models.user import User
|
||||||
from app.models.tenant import Tenant
|
|
||||||
tenant_result = await db.execute(sa_select(Tenant).limit(1))
|
tenant = await get_system_tenant(db)
|
||||||
tenant = tenant_result.scalar_one_or_none()
|
|
||||||
if tenant:
|
if tenant:
|
||||||
user_result = await db.execute(
|
user_result = await db.execute(
|
||||||
sa_select(User)
|
sa_select(User)
|
||||||
@@ -418,16 +419,16 @@ class AutomationPlugin(BasePlugin):
|
|||||||
from another plugin's manifest. Uses plugin name prefixing for conflict resolution."""
|
from another plugin's manifest. Uses plugin name prefixing for conflict resolution."""
|
||||||
from sqlalchemy import select
|
from sqlalchemy import select
|
||||||
|
|
||||||
# Get default tenant_id from the first tenant in the DB
|
# Get system tenant for contributions (ARCH-043: deterministic slug
|
||||||
from app.models.tenant import Tenant
|
# lookup instead of arbitrary first row)
|
||||||
|
from app.core.db import get_system_tenant
|
||||||
from app.plugins.builtins.automation.models import AutomationCronJob
|
from app.plugins.builtins.automation.models import AutomationCronJob
|
||||||
from app.plugins.builtins.automation.services import (
|
from app.plugins.builtins.automation.services import (
|
||||||
AgentService,
|
AgentService,
|
||||||
AutomationService,
|
AutomationService,
|
||||||
CronJobService,
|
CronJobService,
|
||||||
)
|
)
|
||||||
tenant_result = await db.execute(select(Tenant).limit(1))
|
tenant = await get_system_tenant(db)
|
||||||
tenant = tenant_result.scalar_one_or_none()
|
|
||||||
default_tenant_id = tenant.id if tenant else None
|
default_tenant_id = tenant.id if tenant else None
|
||||||
if default_tenant_id is None:
|
if default_tenant_id is None:
|
||||||
logger.warning("No tenant found — skipping plugin contributions registration")
|
logger.warning("No tenant found — skipping plugin contributions registration")
|
||||||
|
|||||||
@@ -0,0 +1,131 @@
|
|||||||
|
# Regression tests for Block A fixes: ARCH-043 (system tenant) and ARCH-052 (storage metadata).
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import asyncio
|
||||||
|
import uuid
|
||||||
|
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
|
||||||
|
# --- ARCH-043: get_system_tenant resolves by configured slug ---
|
||||||
|
|
||||||
|
|
||||||
|
class TestGetSystemTenant:
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_resolves_by_slug_not_first_row(self, db_session):
|
||||||
|
from sqlalchemy import select
|
||||||
|
|
||||||
|
from app.config import get_settings
|
||||||
|
from app.core.db import get_system_tenant
|
||||||
|
from app.models.tenant import Tenant
|
||||||
|
|
||||||
|
settings = get_settings()
|
||||||
|
slug = settings.system_tenant_slug
|
||||||
|
|
||||||
|
# Insert a decoy first and the real system tenant second,
|
||||||
|
# so a limit(1)-style query would pick the wrong row.
|
||||||
|
decoy = Tenant(name='Decoy Org', slug=f'decoy-{uuid.uuid4().hex[:8]}')
|
||||||
|
db_session.add(decoy)
|
||||||
|
system = Tenant(name='Default Org', slug=slug)
|
||||||
|
db_session.add(system)
|
||||||
|
await db_session.commit()
|
||||||
|
|
||||||
|
try:
|
||||||
|
tenant = await get_system_tenant(db_session)
|
||||||
|
assert tenant is not None
|
||||||
|
assert tenant.slug == slug
|
||||||
|
# sanity: decoy really is the first row
|
||||||
|
first = (await db_session.execute(select(Tenant).limit(1))).scalar_one()
|
||||||
|
assert first.id != tenant.id
|
||||||
|
finally:
|
||||||
|
await db_session.delete(decoy)
|
||||||
|
await db_session.delete(system)
|
||||||
|
await db_session.commit()
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_returns_none_when_missing(self, db_session):
|
||||||
|
from sqlalchemy import delete
|
||||||
|
|
||||||
|
from app.core.db import get_system_tenant
|
||||||
|
from app.models.tenant import Tenant
|
||||||
|
|
||||||
|
await db_session.execute(delete(Tenant))
|
||||||
|
await db_session.commit()
|
||||||
|
|
||||||
|
tenant = await get_system_tenant(db_session)
|
||||||
|
assert tenant is None
|
||||||
|
|
||||||
|
|
||||||
|
# --- ARCH-052: async metadata without nested event loops ---
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.fixture
|
||||||
|
def tmp_storage(tmp_path, monkeypatch):
|
||||||
|
from app.core.storage import LocalStorage, reset_storage_backend
|
||||||
|
|
||||||
|
monkeypatch.setenv('STORAGE_PATH', str(tmp_path))
|
||||||
|
monkeypatch.setenv('STORAGE_BACKEND', 'local')
|
||||||
|
reset_storage_backend()
|
||||||
|
backend = LocalStorage(base_path=str(tmp_path))
|
||||||
|
yield backend
|
||||||
|
reset_storage_backend()
|
||||||
|
|
||||||
|
|
||||||
|
class TestGetFileMetadataAsync:
|
||||||
|
def _use_local(self, tmp_storage, monkeypatch):
|
||||||
|
monkeypatch.setenv('STORAGE_PATH', str(tmp_storage.base_path))
|
||||||
|
monkeypatch.setenv('STORAGE_BACKEND', 'local')
|
||||||
|
from app.core.storage import reset_storage_backend
|
||||||
|
|
||||||
|
reset_storage_backend()
|
||||||
|
import app.core.storage as storage_mod
|
||||||
|
|
||||||
|
storage_mod._storage_backend = tmp_storage
|
||||||
|
|
||||||
|
def teardown_method(self):
|
||||||
|
from app.core.storage import reset_storage_backend
|
||||||
|
|
||||||
|
reset_storage_backend()
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_awaitable_inside_running_loop(self, tmp_storage, monkeypatch):
|
||||||
|
self._use_local(tmp_storage, monkeypatch)
|
||||||
|
|
||||||
|
from app.core.storage import get_file_metadata_async
|
||||||
|
|
||||||
|
await tmp_storage.save('meta/async.txt', b'async metadata')
|
||||||
|
|
||||||
|
meta = await get_file_metadata_async('meta/async.txt')
|
||||||
|
assert meta['exists'] is True
|
||||||
|
assert meta['size'] == len(b'async metadata')
|
||||||
|
assert meta['modified'] is not None
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_non_existing_inside_running_loop(self, tmp_storage, monkeypatch):
|
||||||
|
self._use_local(tmp_storage, monkeypatch)
|
||||||
|
|
||||||
|
from app.core.storage import get_file_metadata_async
|
||||||
|
|
||||||
|
meta = await get_file_metadata_async('nonexistent/async.txt')
|
||||||
|
assert meta['exists'] is False
|
||||||
|
assert meta['size'] is None
|
||||||
|
assert meta['modified'] is None
|
||||||
|
|
||||||
|
def test_sync_wrapper_still_works(self, tmp_storage, monkeypatch):
|
||||||
|
self._use_local(tmp_storage, monkeypatch)
|
||||||
|
|
||||||
|
from app.core.storage import get_file_metadata
|
||||||
|
|
||||||
|
asyncio.run(tmp_storage.save('meta/sync.txt', b'sync metadata'))
|
||||||
|
meta = get_file_metadata('meta/sync.txt')
|
||||||
|
assert meta['exists'] is True
|
||||||
|
assert meta['size'] == len(b'sync metadata')
|
||||||
|
|
||||||
|
def test_sync_wrapper_non_existing(self, tmp_storage, monkeypatch):
|
||||||
|
self._use_local(tmp_storage, monkeypatch)
|
||||||
|
|
||||||
|
from app.core.storage import get_file_metadata
|
||||||
|
|
||||||
|
meta = get_file_metadata('nonexistent/sync.txt')
|
||||||
|
assert meta == {'size': None, 'modified': None, 'exists': False}
|
||||||
Reference in New Issue
Block a user