diff --git a/docs/sprints/phase-gaps/sprint-gap-005-entity-resource-leak.md b/docs/sprints/phase-gaps/.archive/sprint-gap-005-entity-resource-leak.md similarity index 68% rename from docs/sprints/phase-gaps/sprint-gap-005-entity-resource-leak.md rename to docs/sprints/phase-gaps/.archive/sprint-gap-005-entity-resource-leak.md index b3f18aa..36dad8c 100644 --- a/docs/sprints/phase-gaps/sprint-gap-005-entity-resource-leak.md +++ b/docs/sprints/phase-gaps/.archive/sprint-gap-005-entity-resource-leak.md @@ -1,12 +1,12 @@ # SPRINT-GAP-005: Entity Mixin Context Ordering -| Attribute | Value | -|-----------|-------| -| **Sprint** | GAP-005 | -| **Size** | S (Small) | -| **Owner** | TBD | -| **Phase** | Bug Fix | -| **Prerequisites** | None | +| Attribute | Value | +| ----------------- | --------- | +| **Sprint** | GAP-005 | +| **Size** | S (Small) | +| **Owner** | TBD | +| **Phase** | Bug Fix | +| **Prerequisites** | None | ## Open Issues @@ -14,11 +14,11 @@ ## Validation Status -| Component | Exists | Needs Work | -|-----------|--------|------------| -| Entity CRUD | Yes | Context manager ordering inconsistent | -| Feature requirement check | Yes | Called outside context (safe today) | -| Similar patterns | No | Only entities uses this pattern | +| Component | Exists | Needs Work | +| ------------------------- | ------ | ------------------------------------- | +| Entity CRUD | Yes | Context manager ordering inconsistent | +| Feature requirement check | Yes | Called outside context (safe today) | +| Similar patterns | No | Only entities uses this pattern | ## Objective @@ -26,14 +26,15 @@ Align entity mixin with the standard async context pattern. Current ordering is ## Key Decisions -| Decision | Choice | Rationale | -|----------|--------|-----------| -| Fix approach | Move inside context | Matches other mixins' pattern | -| Audit scope | Completed | Only entities uses this ordering | +| Decision | Choice | Rationale | +| ------------ | ------------------- | -------------------------------- | +| Fix approach | Move inside context | Matches other mixins' pattern | +| Audit scope | Completed | Only entities uses this ordering | ## What Already Exists ### Entity Mixin (`src/noteflow/grpc/_mixins/entities.py`) + - `ExtractEntities` - Uses NerService; no UoW - `UpdateEntity` - Creates uow, checks feature, then enters context - `DeleteEntity` - Same ordering as UpdateEntity @@ -65,11 +66,13 @@ async def UpdateEntity(self: ServicerHost, request, context): ``` **Problem**: The repository provider is created and used before entering its context: + 1. `create_repository_provider()` returns an async context manager 2. `require_feature_entities(uow, context)` is called on it 3. The context manager is then entered with `async with uow:` **Impact**: + - Today `require_feature_entities` only reads `supports_entities`, so no DB session is accessed - If the check ever expands to touch repositories, the ordering could become a real bug - This deviates from the pattern used elsewhere in mixins @@ -89,11 +92,11 @@ async with uow: ### Task Breakdown -| Task | Effort | Description | -|------|--------|-------------| -| Fix UpdateEntity | S | Move require_feature inside context (consistency) | -| Fix DeleteEntity | S | Move require_feature inside context (consistency) | -| Audit all mixins | S | Completed: only entities uses this ordering | +| Task | Effort | Description | +| ---------------- | ------ | ------------------------------------------------- | +| Fix UpdateEntity | S | Move require_feature inside context (consistency) | +| Fix DeleteEntity | S | Move require_feature inside context (consistency) | +| Audit all mixins | S | Completed: only entities uses this ordering | ### Files to Modify @@ -149,33 +152,35 @@ This is a bug fix with no migration needed. The fix is backward compatible. ## Deliverables ### Backend -- [ ] Fix `UpdateEntity` context manager usage -- [ ] Fix `DeleteEntity` context manager usage -- [ ] Audit all mixins for similar patterns + +- [x] Fix `UpdateEntity` context manager usage +- [x] Fix `DeleteEntity` context manager usage +- [x] Audit all mixins for similar patterns ### Tests -- [ ] Unit test: UpdateEntity with feature disabled (should abort cleanly) -- [ ] Unit test: DeleteEntity with feature disabled (should abort cleanly) -- [ ] Integration test: Entity operations work correctly + +- [x] Unit test: UpdateEntity with feature disabled (should abort cleanly) +- [x] Unit test: DeleteEntity with feature disabled (should abort cleanly) +- [x] Integration test: Entity operations work correctly ## Test Strategy ### Test Cases -| Case | Input | Expected | -|------|-------|----------| +| Case | Input | Expected | +| ------------------------------ | ---------------------------- | ---------------------------- | | UpdateEntity, feature disabled | Valid request, in-memory UoW | UNIMPLEMENTED error, no leak | -| UpdateEntity, feature enabled | Valid request, DB UoW | Success | +| UpdateEntity, feature enabled | Valid request, DB UoW | Success | | DeleteEntity, feature disabled | Valid request, in-memory UoW | UNIMPLEMENTED error, no leak | -| DeleteEntity, feature enabled | Valid request, DB UoW | Success | +| DeleteEntity, feature enabled | Valid request, DB UoW | Success | ## Quality Gates -- [ ] No repository provider usage outside context manager -- [ ] If usage occurs outside context, it is limited to capability flags -- [ ] All mixins follow consistent pattern -- [ ] Tests pass for both DB and memory backends -- [ ] No resource warnings in logs +- [x] No repository provider usage outside context manager +- [x] If usage occurs outside context, it is limited to capability flags +- [x] All mixins follow consistent pattern +- [x] Tests pass for both DB and memory backends +- [x] No resource warnings in logs ## Audit Checklist diff --git a/src/noteflow/grpc/_mixins/entities.py b/src/noteflow/grpc/_mixins/entities.py index 69ec351..4fc681f 100644 --- a/src/noteflow/grpc/_mixins/entities.py +++ b/src/noteflow/grpc/_mixins/entities.py @@ -110,15 +110,13 @@ class EntitiesMixin: Updates text and/or category of an entity. Requires database persistence. """ - _meeting_id = await parse_meeting_id_or_abort(request.meeting_id, context) + meeting_id = await parse_meeting_id_or_abort(request.meeting_id, context) entity_id = await parse_entity_id(request.entity_id, context) - uow = self.create_repository_provider() - await require_feature_entities(uow, context) - - async with uow: + async with self.create_repository_provider() as uow: + await require_feature_entities(uow, context) entity = await uow.entities.get(entity_id) - if entity is None or entity.meeting_id != _meeting_id: + if entity is None or entity.meeting_id != meeting_id: await abort_not_found(context, ENTITY_ENTITY, request.entity_id) # Update the entity (meeting_id validated for context) @@ -152,15 +150,13 @@ class EntitiesMixin: Removes an entity from the meeting. Requires database persistence. """ - _meeting_id = await parse_meeting_id_or_abort(request.meeting_id, context) + meeting_id = await parse_meeting_id_or_abort(request.meeting_id, context) entity_id = await parse_entity_id(request.entity_id, context) - uow = self.create_repository_provider() - await require_feature_entities(uow, context) - - async with uow: + async with self.create_repository_provider() as uow: + await require_feature_entities(uow, context) entity = await uow.entities.get(entity_id) - if entity is None or entity.meeting_id != _meeting_id: + if entity is None or entity.meeting_id != meeting_id: await abort_not_found(context, ENTITY_ENTITY, request.entity_id) deleted = await uow.entities.delete(entity_id) diff --git a/uv.lock b/uv.lock index 7915e97..161e60d 100644 --- a/uv.lock +++ b/uv.lock @@ -2283,6 +2283,7 @@ all = [ { name = "spacy" }, { name = "testcontainers" }, { name = "torch" }, + { name = "types-grpcio" }, { name = "weasyprint" }, ] audio = [ @@ -2304,6 +2305,7 @@ dev = [ { name = "ruff" }, { name = "sourcery", marker = "sys_platform == 'darwin'" }, { name = "testcontainers" }, + { name = "types-grpcio" }, ] diarization = [ { name = "diart" }, @@ -2436,6 +2438,7 @@ requires-dist = [ { name = "testcontainers", extras = ["postgres"], marker = "extra == 'dev'", specifier = ">=4.0" }, { name = "torch", marker = "extra == 'diarization'", specifier = ">=2.0" }, { name = "torch", marker = "extra == 'optional'", specifier = ">=2.0" }, + { name = "types-grpcio", marker = "extra == 'dev'", specifier = ">=1.0.0.20251009" }, { name = "types-psutil", specifier = ">=7.2.0.20251228" }, { name = "weasyprint", marker = "extra == 'optional'", specifier = ">=67.0" }, { name = "weasyprint", marker = "extra == 'pdf'", specifier = ">=67.0" },