feat(api): session attachment resource with protected mount, quotas, and sweep - #5607
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds session attachment storage and APIs. It adds protected attachment mounts, media classification, quotas, idempotent uploads, PostgreSQL lifecycle management, background cleanup, configuration, migrations, and tests. ChangesSession attachment resource
Protected attachment mounts
Session API and runtime wiring
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SessionAttachmentsRouter
participant SessionAttachmentsService
participant SessionAttachmentsDAO
participant MountsService
Client->>SessionAttachmentsRouter: Upload multipart attachment
SessionAttachmentsRouter->>SessionAttachmentsService: Validate and create attachment
SessionAttachmentsService->>MountsService: Get protected attachment mount
SessionAttachmentsService->>SessionAttachmentsDAO: Reserve pending record
SessionAttachmentsService->>MountsService: Write original bytes
SessionAttachmentsService->>SessionAttachmentsDAO: Mark attachment ready
SessionAttachmentsRouter-->>Client: Return attachment response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| *, | ||
| session_id: str = Query(...), | ||
| ) -> SessionAttachmentResponse: | ||
| content_length = request.headers.get("content-length") |
There was a problem hiding this comment.
This Content-Length check is the actual request-size bound, and it must stay ABOVE the request.form() call: declaring the file as a FastAPI File(...) parameter would make the framework spool the entire body to disk before any handler code runs, and EE deployments have no gateway body cap in front of the API. This was the most severe review finding. The chunked _read_bounded below is the second line of defense, not the first.
| return f"{_RESERVED_SLUG_PREFIX}session__{uuid5(_MOUNTS_NAMESPACE, session_id)}__{slugify_mount_name(name)}" | ||
|
|
||
|
|
||
| def is_protected_mount(mount: Mount) -> bool: |
There was a problem hiding this comment.
The whole protected-mount policy hangs on this one predicate: every generic mount operation (resolve, query, fetch, export, sign) routes through it, and the review enumerated all 21 routes to confirm none bypasses it. purpose is the durable classification; the slug match is the transitional fallback for rows written before the column existed. The mapped result is a 404 identical to a missing mount, so the wire never reveals the mount exists.
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
3daeb4f to
4fad68a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
api/oss/src/core/sessions/attachments/service.py (1)
148-157: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDelete the pending row after the object delete succeeds, not unconditionally.
_delete_original_best_effortswallows failures. If the object delete fails and the row delete then succeeds, the stored object is orphaned with no row and no tombstone. The sweep only finds objects through rows, so nothing retries that delete.If the row is left in
PENDINGinstead,reap_stale_pendingreclaims both the row and the object on a later pass. Make the row delete conditional on a successful object delete.♻️ Proposed change to keep failed object deletes retryable
except AttachmentQuotaExceeded: - await self._delete_original_best_effort( + original_deleted = await self._delete_original_best_effort( project_id=project_id, attachment=reservation.attachment, ) - await self._delete_pending_best_effort( - project_id=project_id, - attachment_id=reservation.attachment.id, - ) + if original_deleted: + await self._delete_pending_best_effort( + project_id=project_id, + attachment_id=reservation.attachment.id, + ) raiseChange
_delete_original_best_effortto report the outcome:async def _delete_original_best_effort( self, *, project_id: UUID, attachment: Attachment, - ) -> None: + ) -> bool: try: await self._original_store.delete_attachment_original( project_id=project_id, mount_id=attachment.mount_id, path=attachment.path, ) except Exception: log.error( "attachment_create: object compensation failed for %s", attachment.id, exc_info=True, ) + return False + return Trueapi/oss/tests/pytest/unit/sessions/test_attachment_state_machine.py (2)
83-113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueScope the fake reservation store by project and session.
The real DAO looks up the idempotency key with
project_idandsession_idin the predicate (api/oss/src/dbs/postgres/sessions/attachments/dao.pylines 350-366). This fake keysself.attachmentsbyidempotency_keyalone, so the same key in a different project or session collides and returnsCONFLICTinstead of creating a new reservation.
fetch_by_idempotency_keyin this fake already applies the project and session filters, so the two methods disagree. Key the dictionary by the full tuple to keep the fake faithful to the DAO contract.
549-553: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport the multipart overhead constant instead of repeating
64 * 1024.The router computes its cap as
max_raw_bytes + _ATTACHMENT_MULTIPART_OVERHEAD_BYTES. This test duplicates that overhead as a literal. If the router constant grows,oversizedno longer exceeds the cap and the test stops covering the 413 path.♻️ Proposed change to bind the test to the router constant
- oversized = service.limits.max_raw_bytes + (64 * 1024) + 1 + oversized = ( + service.limits.max_raw_bytes + _ATTACHMENT_MULTIPART_OVERHEAD_BYTES + 1 + )Extend the existing router import:
-from oss.src.apis.fastapi.sessions.router import SessionAttachmentsRouter +from oss.src.apis.fastapi.sessions.router import ( + _ATTACHMENT_MULTIPART_OVERHEAD_BYTES, + SessionAttachmentsRouter, +)api/oss/tests/pytest/unit/sessions/test_attachment_quotas.py (1)
143-171: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe fake dispatches on compiled SQL text, which couples these tests to the entity column list.
The branch at line 152 identifies the row-lock query by the presence of
session_attachments.filenamein the rendered statement. That fragment appears only becausemark_readyselects the whole entity andfilenamehappens to be in the compiled column list. If a column is renamed or removed fromSessionAttachmentDBE, this branch stops matching and the test fails at line 171 withUnexpected statement, for a reason unrelated to quota behavior.Dispatch on a stable property instead, for example the presence of
FOR UPDATEtogether with the absence of an aggregate function. The failure mode today is loud rather than silent, so this is a maintainability improvement, not a correctness defect.api/oss/src/dbs/postgres/sessions/attachments/dao.py (1)
439-452: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueThe per-row
referenced_atrefresh is redundant under the row lock.The select at Line 430 already applies
WHERE referenced_at IS NULLandFOR UPDATE. The refresh runs in the same transaction, so it reads the same snapshot and the same locked row. A concurrentreference_readycall blocks on that row lock until this transaction commits, so it cannot makereferenced_atnon-null here. The refresh therefore adds one round trip per candidate row and never changes the outcome. Consider removing it.♻️ Proposed simplification
candidates: List[Attachment] = [] for attachment_dbe in attachment_dbes: - if state == AttachmentState.READY: - await session.refresh( - attachment_dbe, - attribute_names=["referenced_at"], - ) - if attachment_dbe.referenced_at is not None: - continue candidates.append( map_attachment_dbe_to_dto(attachment_dbe=attachment_dbe) )If the refresh guards a case that the row lock does not cover, add a comment that names that case.
api/oss/tests/pytest/unit/sessions/test_attachment_dao_integration.py (1)
192-224: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding the reverse race case.
This test proves that a tombstone beats a concurrent claim. The opposite ordering is the other half of the guarantee: if
reference_readycommits first, the sweep must skip the row and must not delete the stored object. A test that setsreferenced_atbefore the sweep starts and then asserts thatdelete_originalis never called would cover that path.api/oss/src/dbs/postgres/mounts/dbas.py (1)
35-39: 🚀 Performance & Scalability | 🔵 TrivialConsider indexing the
purposepredicate.The
purposecolumn has noindex=True, andquery_mountsappliesMountDBE.purpose.is_distinct_from(...)unconditionally on every call, unlikesession_id/agent_id, which are filtered only when the caller supplies a query parameter. Confirm whether the migration adds an index (e.g. a composite(project_id, purpose)index) to keep this predicate cheap as themountstable grows.#!/bin/bash # Description: Check whether the migration adds an index covering `purpose`. fd 'oss000000020_add_session_attachments.py' | xargs -r cat -napi/oss/src/core/sessions/attachments/dtos.py (1)
37-37: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a non-negative constraint on
size.
Attachment.sizeandAttachmentCreate.sizeaccept anyint, including negative values. Downstream quota accounting (AttachmentQuotaUsage.stored_bytes,AttachmentLimits.max_per_session_bytes) assumes non-negative sizes. A negative value would silently corrupt quota math rather than fail immediately.🛡️ Proposed fix
-from pydantic import BaseModel +from pydantic import BaseModel, Fieldmedia_type: str - size: int + size: int = Field(ge=0) kind: AttachmentKind state: AttachmentStatemedia_type: str - size: int + size: int = Field(ge=0) kind: AttachmentKind idempotency_key: strAlso applies to: 52-52
api/oss/tests/pytest/acceptance/sessions/test_session_attachment_teardown.py (1)
56-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid duplicating the internal storage-key formula in the test.
_read_originalrebuilds the object-store key withf"{prefix}mounts/{project_id}/{mount_id}/{path}", mirroring_storage_keyinmounts/service.py. If that internal formula changes, this test keeps testing its own copy of the old formula instead of the real code path, and a genuine bug in key construction could go undetected.Route this through a shared test helper or the mounts service's own key builder instead of hand-rolling the format here.
[recommended_refactor_effort_placeholder]
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ee4fe5c9-7c8d-4e1a-bcc5-418e8f02ab0e
⛔ Files ignored due to path filters (1)
api/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
api/entrypoints/routers.pyapi/oss/databases/postgres/migrations/core_oss/versions/oss000000020_add_session_attachments.pyapi/oss/src/apis/fastapi/mounts/models.pyapi/oss/src/apis/fastapi/mounts/router.pyapi/oss/src/apis/fastapi/sessions/models.pyapi/oss/src/apis/fastapi/sessions/router.pyapi/oss/src/core/mounts/dtos.pyapi/oss/src/core/mounts/service.pyapi/oss/src/core/mounts/types.pyapi/oss/src/core/sessions/attachments/__init__.pyapi/oss/src/core/sessions/attachments/dtos.pyapi/oss/src/core/sessions/attachments/interfaces.pyapi/oss/src/core/sessions/attachments/media.pyapi/oss/src/core/sessions/attachments/service.pyapi/oss/src/core/sessions/attachments/types.pyapi/oss/src/dbs/postgres/mounts/dao.pyapi/oss/src/dbs/postgres/mounts/dbas.pyapi/oss/src/dbs/postgres/mounts/mappings.pyapi/oss/src/dbs/postgres/sessions/attachments/__init__.pyapi/oss/src/dbs/postgres/sessions/attachments/dao.pyapi/oss/src/dbs/postgres/sessions/attachments/dbas.pyapi/oss/src/dbs/postgres/sessions/attachments/dbes.pyapi/oss/src/dbs/postgres/sessions/attachments/mappings.pyapi/oss/src/tasks/asyncio/sessions/attachment_sweep.pyapi/oss/src/utils/env.pyapi/oss/tests/pytest/acceptance/mounts/test_attachments_mount_hidden.pyapi/oss/tests/pytest/acceptance/sessions/test_session_attachment_teardown.pyapi/oss/tests/pytest/acceptance/sessions/test_session_attachments.pyapi/oss/tests/pytest/unit/mounts/test_agent_mounts.pyapi/oss/tests/pytest/unit/mounts/test_protected_mount_policy.pyapi/oss/tests/pytest/unit/sessions/test_attachment_dao_integration.pyapi/oss/tests/pytest/unit/sessions/test_attachment_filename.pyapi/oss/tests/pytest/unit/sessions/test_attachment_media.pyapi/oss/tests/pytest/unit/sessions/test_attachment_quotas.pyapi/oss/tests/pytest/unit/sessions/test_attachment_state_machine.pyapi/oss/tests/pytest/unit/sessions/test_attachment_sweep.pyapi/oss/tests/pytest/unit/sessions/test_records_truncation.pyapi/pyproject.tomldocs/design/agent-workflows/projects/agent-multi-modality/plans/stage-1-implementation.mddocs/design/agent-workflows/projects/agent-multi-modality/protocols/stage-1.mdhosting/docker-compose/oss/nginx/nginx.conf
4fad68a to
8905cb8
Compare
dosubot
Bot
added
size:XXL
Railway Preview Environment
Updated at 2026-08-01T18:13:22.767Z |
ebb22d8 to
cb27a03
Compare
19b70b0 to
3e34770
Compare
cb27a03 to
a652f2a
Compare
3e34770 to
973ade3
Compare
|
🤖 The AI agent says: Review-body items from the CodeRabbit pass: all landed in 56bc0b6 (lockfile in 973ade3).
Three items were deliberately skipped. Computing |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
api/oss/src/core/mounts/types.py (1)
114-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd structured context to
MountProtected.
MountProtectedonly stores a free-textmessage. Sibling exceptions in this file, such asMountSlugReserved, store an identifying attribute (self.slug). Add an identifying attribute, such asmount_id, toMountProtectedso callers and logs retain the mount identity even though the router intentionally maps this exception to a generic "Mount not found" response.As per coding guidelines, "Define domain exceptions in the core layer, include structured context, and use a base exception per domain; do not raise
HTTPExceptionfrom services or DAOs."♻️ Proposed fix to add structured context
class MountProtected(MountError): - def __init__(self, message: str = "Mount is protected."): + def __init__(self, mount_id: str | None = None, message: str = "Mount is protected."): super().__init__(message) + self.mount_id = mount_idSource: Coding guidelines
api/oss/tests/pytest/unit/sessions/test_attachment_state_machine.py (1)
98-155: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd test coverage for the DELETING reservation branch.
reserve_pendingmaps an existingAttachmentState.DELETINGrow toAttachmentReservationStatus.IN_FLIGHT(Line 124-125). No test in this file exercises this branch. This fake defines the expected reservation contract for the state machine; an untested state transition can hide a mismatch with the real PostgreSQL DAO reservation logic for a duplicate idempotent request that arrives while the original attachment is mid-deletion.Add a test that creates an existing attachment in
AttachmentState.DELETING, retries the same idempotency key, and asserts the resulting behavior (for example,AttachmentUploadInFlightwith the expectedretry_after_seconds).api/oss/tests/pytest/unit/sessions/test_attachment_sweep.py (1)
157-307: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test asserts successful lock release after a normal sweep pass.
None of the tests check that
lock.valuebecomesNone(or equivalent) afterrun_attachment_sweepcompletes normally. Add an assertion in at least one happy-path test (for exampletest_sweep_removes_only_stale_unclaimed_rows) that the lock is released, so a future regression that skips or breaks thefinally/_release_lockpath is caught.♻️ Proposed addition
await _run(dao, store) assert {row.id for row in dao.rows} == {claimed_ready.id, fresh_ready.id} assert set(store.deleted) == {str(stale_pending.id), str(stale_ready.id)} + assert lock.value is None
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1cb636d7-eb17-44c4-8dbf-626ff2b00311
⛔ Files ignored due to path filters (1)
api/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
api/entrypoints/routers.pyapi/oss/databases/postgres/migrations/core_oss/versions/oss000000020_add_session_attachments.pyapi/oss/src/apis/fastapi/mounts/models.pyapi/oss/src/apis/fastapi/mounts/router.pyapi/oss/src/apis/fastapi/sessions/models.pyapi/oss/src/apis/fastapi/sessions/router.pyapi/oss/src/core/mounts/dtos.pyapi/oss/src/core/mounts/service.pyapi/oss/src/core/mounts/types.pyapi/oss/src/core/sessions/attachments/__init__.pyapi/oss/src/core/sessions/attachments/dtos.pyapi/oss/src/core/sessions/attachments/interfaces.pyapi/oss/src/core/sessions/attachments/media.pyapi/oss/src/core/sessions/attachments/service.pyapi/oss/src/core/sessions/attachments/types.pyapi/oss/src/dbs/postgres/mounts/dao.pyapi/oss/src/dbs/postgres/mounts/dbas.pyapi/oss/src/dbs/postgres/mounts/mappings.pyapi/oss/src/dbs/postgres/sessions/attachments/__init__.pyapi/oss/src/dbs/postgres/sessions/attachments/dao.pyapi/oss/src/dbs/postgres/sessions/attachments/dbas.pyapi/oss/src/dbs/postgres/sessions/attachments/dbes.pyapi/oss/src/dbs/postgres/sessions/attachments/mappings.pyapi/oss/src/tasks/asyncio/sessions/attachment_sweep.pyapi/oss/src/utils/env.pyapi/oss/tests/pytest/acceptance/mounts/test_attachments_mount_hidden.pyapi/oss/tests/pytest/acceptance/sessions/test_session_attachment_teardown.pyapi/oss/tests/pytest/acceptance/sessions/test_session_attachments.pyapi/oss/tests/pytest/unit/mounts/test_agent_mounts.pyapi/oss/tests/pytest/unit/mounts/test_protected_mount_policy.pyapi/oss/tests/pytest/unit/sessions/test_attachment_dao_integration.pyapi/oss/tests/pytest/unit/sessions/test_attachment_filename.pyapi/oss/tests/pytest/unit/sessions/test_attachment_media.pyapi/oss/tests/pytest/unit/sessions/test_attachment_quotas.pyapi/oss/tests/pytest/unit/sessions/test_attachment_state_machine.pyapi/oss/tests/pytest/unit/sessions/test_attachment_sweep.pyapi/oss/tests/pytest/unit/sessions/test_records_truncation.pyapi/pyproject.tomldocs/design/agent-workflows/projects/agent-multi-modality/plans/stage-1-implementation.mddocs/design/agent-workflows/projects/agent-multi-modality/protocols/stage-1.mdhosting/docker-compose/oss/nginx/nginx.conf
🚧 Files skipped from review as they are similar to previous changes (33)
- api/pyproject.toml
- api/oss/src/core/sessions/attachments/init.py
- api/oss/src/core/mounts/dtos.py
- api/oss/src/dbs/postgres/sessions/attachments/init.py
- api/oss/src/apis/fastapi/mounts/models.py
- api/oss/src/core/sessions/attachments/media.py
- api/oss/src/apis/fastapi/sessions/models.py
- api/oss/src/dbs/postgres/mounts/mappings.py
- hosting/docker-compose/oss/nginx/nginx.conf
- api/oss/tests/pytest/unit/sessions/test_attachment_filename.py
- api/oss/src/dbs/postgres/sessions/attachments/dbes.py
- api/entrypoints/routers.py
- api/oss/src/core/sessions/attachments/types.py
- api/oss/src/dbs/postgres/mounts/dao.py
- api/oss/tests/pytest/unit/sessions/test_attachment_media.py
- api/oss/src/dbs/postgres/sessions/attachments/mappings.py
- api/oss/src/dbs/postgres/sessions/attachments/dbas.py
- api/oss/tests/pytest/unit/mounts/test_agent_mounts.py
- api/oss/tests/pytest/unit/sessions/test_attachment_quotas.py
- api/oss/src/apis/fastapi/mounts/router.py
- api/oss/src/core/sessions/attachments/interfaces.py
- api/oss/tests/pytest/acceptance/sessions/test_session_attachments.py
- api/oss/tests/pytest/acceptance/mounts/test_attachments_mount_hidden.py
- api/oss/src/dbs/postgres/mounts/dbas.py
- api/oss/tests/pytest/acceptance/sessions/test_session_attachment_teardown.py
- api/oss/src/core/sessions/attachments/service.py
- api/oss/src/utils/env.py
- api/oss/tests/pytest/unit/sessions/test_records_truncation.py
- api/oss/src/dbs/postgres/sessions/attachments/dao.py
- api/oss/databases/postgres/migrations/core_oss/versions/oss000000020_add_session_attachments.py
- docs/design/agent-workflows/projects/agent-multi-modality/plans/stage-1-implementation.md
- api/oss/src/apis/fastapi/sessions/router.py
- api/oss/src/core/mounts/service.py
| @pytest.fixture | ||
| def anyio_backend(): | ||
| return "asyncio" | ||
|
|
||
|
|
||
| async def _run(dao, store, lock=None, *, sweep_interval_seconds=60): | ||
| await attachment_sweep.run_attachment_sweep( | ||
| attachments_dao=dao, | ||
| original_store=store, | ||
| lock_engine=lock or _FakeLockEngine(), | ||
| pending_ttl_seconds=900, | ||
| unreferenced_ttl_seconds=86_400, | ||
| sweep_interval_seconds=sweep_interval_seconds, | ||
| ) | ||
|
|
||
|
|
||
| @pytest.mark.anyio | ||
| async def test_sweep_removes_only_stale_unclaimed_rows(anyio_backend): | ||
| assert anyio_backend == "asyncio" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm whether AnyIO's pytest plugin is active for this suite, or whether pytest-asyncio strict mode silently skips these tests.
set -euo pipefail
echo "--- pytest / asyncio_mode config ---"
rg -n 'asyncio_mode|\[tool.pytest' api/pyproject.toml pytest.ini setup.cfg 2>/dev/null || true
echo "--- anyio dependency declaration ---"
rg -n 'anyio' api/pyproject.toml 2>/dev/null || true
echo "--- conftest.py anyio/asyncio references near this test dir ---"
fd conftest.py api/oss/tests | xargs -I{} sh -c 'echo "--- {} ---"; rg -n "anyio|asyncio_mode|pytest_plugins" {}' || true
echo "--- other usages of `@pytest.mark.anyio` vs `@pytest.mark.asyncio` in the suite ---"
rg -n '`@pytest.mark.anyio`' api/oss/tests | wc -l
rg -n '`@pytest.mark.asyncio`' api/oss/tests | wc -lRepository: Agenta-AI/agenta
Length of output: 739
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- dependency and plugin files containing pytest/asyncio/anyio ---"
rg -n 'pytest(-asyncio)?|asyncio_mode|anyio|plugin' pyproject.toml api/pyproject.toml requirements*.txt setup.cfg pytest.ini 2>/dev/null || true
echo "--- test file relevant sections ---"
wc -l api/oss/tests/pytest/unit/sessions/test_attachment_sweep.py
sed -n '1,350p' api/oss/tests/pytest/unit/sessions/test_attachment_sweep.py
echo "--- anyio imports/usages in api/oss/tests/pytest/unit/sessions and nearby pytest tests ---"
rg -n 'import anyio|from anyio|`@pytest`\.mark\.anyio|async def .*anyio|async def _run|test_' api/oss/tests/pytest/unit/sessions test_attachment_sweep.py 2>/dev/null || trueRepository: Agenta-AI/agenta
Length of output: 43073
🌐 Web query:
pytest-asyncio strict mode async function returns coroutine pytest-asyncio v1 default behavior
💡 Result:
In pytest-asyncio, the default mode is strict [1][2][3]. When an async test function is defined but not explicitly marked with @pytest.mark.asyncio, behavior depends on the configured asyncio mode: In strict mode (the default), pytest-asyncio does not treat the async function as a test coroutine [1][4]. If you attempt to run it, pytest may treat it as a standard synchronous test that simply returns a coroutine object [5]. Since the coroutine is never awaited, the test will likely pass (if it executes without error) or behave unexpectedly, as the actual asynchronous logic inside the function is never executed [5][6]. To correctly run an async test function in strict mode, you must explicitly mark it with @pytest.mark.asyncio [1][7]. In auto mode, pytest-asyncio automatically detects async functions and treats them as test coroutines, applying the asyncio marker implicitly, so explicit marking is not required [1][7]. The strict mode is the default behavior to ensure that pytest-asyncio coexists peacefully with other asynchronous testing plugins, as it prevents the plugin from automatically taking ownership of all async functions unless explicitly instructed to do so [1][8].
Citations:
- 1: https://pytest-asyncio.readthedocs.io/en/stable/concepts.html
- 2: https://pytest-asyncio.readthedocs.io/en/stable/reference/configuration.html
- 3: https://pytest-asyncio.readthedocs.io/en/v1.4.0/reference/configuration.html
- 4: https://pytest-asyncio.readthedocs.io/en/v0.24.0/concepts.html
- 5: https://github.com/pytest-dev/pytest-asyncio/blob/v0.20.2/pytest_asyncio/plugin.py
- 6: https://github.com/pytest-dev/pytest-asyncio/tree/refs/heads/main
- 7: https://pytest-asyncio.readthedocs.io/en/v0.20.3/reference.html
- 8: https://pytest-asyncio.readthedocs.io/en/v0.25.1/concepts.html
Run these attachment-sweep tests through the active pytest asyncio plugin.
@pytest.mark.anyio only works when AnyIO’s pytest plugin is installed/registered; this dependency list declares pytest-asyncio, not anyio, and no pytest config enables AnyIO. Add AnyIO plugin configuration if you keep these tests as anyio, or change them to @pytest.mark.asyncio; otherwise the async test bodies may never execute under the repo’s pytest setup.
The plan is the adjudicated revision after an independent critical review of the first draft; its 'Adjudicated decisions' section is the decision log. The protocol records how it was produced and grows as the work packages land. Claude-Session: https://claude.ai/code/session_01A1XQVjHPYJgVBHWSNUphtx
…and sweep A session-scoped attachment original enters through a bounded two-phase create (pending -> ready, client idempotency key), is served back by a side-effect-free content route with a session-binding check, and becomes durable only through an explicit claim route. The attachments mount is invisible to every generic mount operation via one protected-mount policy backed by a server-owned purpose column; session teardown still includes it. Per-session quotas bound count, bytes, and pending uploads. A locked sweep reaps stuck pending rows and unclaimed originals past their time-to-live. The compose gateway body cap rises to 32m; the create route bounds its own read because EE deployments have no gateway cap at all. Claude-Session: https://claude.ai/code/session_01A1XQVjHPYJgVBHWSNUphtx
The create route verifies Content-Length before the form parse (411 when absent, 413 over the cap), because declaring the file as a route parameter spooled the whole body to disk before any check ran and EE deployments have no gateway body cap. In-process create failures compensate immediately (row and object removed) so an idempotency key is never wedged behind the pending TTL; the in-flight 409 carries Retry-After. The sweep tombstones rows to a 'deleting' state under the row lock, deletes objects outside any transaction, and keeps retryable tombstones on storage errors, so claims fail closed and a storage outage delays cleanup instead of orphaning objects. Key reuse with different bytes is a dedicated 409 conflict backed by a content digest; the protected-mount exclusion moved into the query predicate; filenames are length-bounded; the public mount-create model no longer exposes the server-owned purpose field; new real-database integration tests cover the sweep-versus-claim race and the locked quota recheck. Claude-Session: https://claude.ai/code/session_01A1XQVjHPYJgVBHWSNUphtx
…as fix The mount-naming vocabulary moves to core/mounts/types.py so the DAO's protected-mount predicate derives from the same constants as the service (with a drift-guard test); the concurrent integration test gets a timeout. Three CodeRabbit findings were declined on-thread with evidence. Also completes the FastAPI UploadFile alias on the mount upload route annotation: the earlier absorb took the import hunk but silently dropped this one, leaving the committed router unable to boot even though the working tree was correct. Claude-Session: https://claude.ai/code/session_01A1XQVjHPYJgVBHWSNUphtx
The per-turn count rose to 100, so a session quota of 100 would let one full turn consume it; the 256 MB byte quota stays the real bound. Claude-Session: https://claude.ai/code/session_01A1XQVjHPYJgVBHWSNUphtx
a652f2a to
e424506
Compare
973ade3 to
97d4367
Compare
1 participant
A file attached in the agent chat has nowhere to live: the composer collects it, but there is no attachment resource, so the front end inlines base64 into the message and the runner flattens it at the prompt. This PR builds the API half of the fix, work package 1 of 4 of the Stage 1 plan (
docs/design/agent-workflows/projects/agent-multi-modality/plans/stage-1-implementation.md, committed in this PR together withprotocols/stage-1.md, the full decision trail).Before: no attachment storage exists; the only file routes are the generic mount routes, which trust the client's media type, buffer unbounded bodies in memory, and let any project member overwrite any mount file.
After:
POST /sessions/attachmentsstores an immutable original in a session-scoped mount through a bounded, idempotent, two-phase create;GET /sessions/attachments/{id}/contentserves it back with a session-binding check and the byte-verified media type;POST /sessions/attachments/referenceis how a file becomes durable (the runner claims it when accepting a turn). Nothing calls these routes yet; the runner (WP2), the SDK (WP3), and the front end (WP4) follow, stacked on this branch.The pieces, each traceable to the plan:
puremagicbehind an adapter; declared type is never trusted), per-kind caps (10/15/10 MB), apending → readyrow with a client idempotency key, immediate compensation on in-process failures, and per-session quotas (1,000 files, 256 MB, 20 pending).MountsService, backed by a server-ownedpurposecolumn; the wire answer is indistinguishable from a missing mount. Session teardown deliberately still includes it, which is what implements the settled retention rule.deletingstate, deletes objects outside any transaction, and retries on storage errors. Unclaimed uploads die after 24 h; stuck pending rows after 15 min.The change was adversarially reviewed before this PR opened; the review confirmed the mount policy is closed over every route and found three release blockers (unbounded body spooling, store I/O inside a lock-holding transaction, a wedged idempotency key after failed creates), all fixed in the second commit. The full findings-and-rulings trail is in
protocols/stage-1.md.Implicit decisions and their tradeoffs
Details in
protocols/stage-1.md; the ones worth your eyes:EDIT_SESSIONS/VIEW_SESSIONS/RUN_SESSIONS), deliberately not the dual session+mounts pairs the neighbouringSessionMountsRouteruses: requiring a mounts permission would leak the storage implementation into the authorization model.puremagicreports, because voice recordings arrive in exactly those containers and the audio cap differs from the generic one. Heuristic at the margins, deliberately.deletingtombstone state exists beyond the designed pending/ready pair so the sweep can be both safe against claims and retryable on storage errors. It never appears on the wire.Forced routes to double-check
Content-Lengthis the request-size bound (absent → 411, oversized → 413, checked before the form is parsed). A chunked upload without a length header stops working; every client we ship sends one. The alternative was a streaming multipart parser, judged not worth the machinery for a 15 MB-capped route.purposeeven though the core DTO carries it; if a future feature wants client-settable mount purposes, this is the seam to reopen.QA
ruffclean; 268 unit tests pass. Deployed to the dev stack: the migration applied cleanly on a real database, all nine acceptance tests pass live (zero skips, storage verification forced on), both real-database concurrency tests pass (sweep-versus-claim race, locked quota recheck), and a curl-level wire matrix verified the create/idempotency/conflict/download/claim/411/413/protected-mount behaviors end to end. Deployment QA caught one boot blocker both test suites had missed (a starlette-vs-FastAPIUploadFileannotation broke router import); fixed with an import alias and re-verified live. Full record inprotocols/stage-1.md. Remaining: the 15 MB audio upload through thewith-nginxprofile needs an OSS gh stack (the EE dev stack runs no nginx).https://claude.ai/code/session_01A1XQVjHPYJgVBHWSNUphtx