From 8ca82b2a023e90b052f4a8d84f0652b7091a75bb Mon Sep 17 00:00:00 2001 From: Norair Arutshyan Date: Thu, 1 Oct 2026 14:41:24 +0100 Subject: [PATCH 1/2] fix(budget): cascade-delete orphaned categories when deleting a budget BudgetModel.categories now uses cascade="all, delete-orphan" with passive_deletes=True. Previously, deleting a budget whose category had outlived its lines nulled out budget_id and failed with an IntegrityError (400). Adds real-DB tests for both the unloaded and the already-loaded categories cases, and updates the design/tasks for the change. Refs #346 Co-Authored-By: Claude Opus 5.5 --- .../design.md | 9 ++-- .../tasks.md | 18 +++++-- services/budget/app/models/budget.py | 5 +- services/budget/tests/test_budget_crud.py | 52 ++++++++++++++++++- 4 files changed, 75 insertions(+), 9 deletions(-) diff --git a/openspec/changes/budget-fix-295-category-cascade-delete-fix/design.md b/openspec/changes/budget-fix-295-category-cascade-delete-fix/design.md index aaebfd8..d3f2695 100644 --- a/openspec/changes/budget-fix-295-category-cascade-delete-fix/design.md +++ b/openspec/changes/budget-fix-295-category-cascade-delete-fix/design.md @@ -22,10 +22,11 @@ See proposal.md - Why for the failure mode. Relevant existing state: ## Decisions -**Use `passive_deletes=True` on `BudgetModel.categories`, not ORM-level `cascade="all, delete-orphan"`.** -- `passive_deletes=True` tells SQLAlchemy "don't manage this relationship's deletes yourself, the DB's `ON DELETE CASCADE` already does it" — it skips the lazy-load-and-null-out step entirely and just issues `DELETE FROM budgets WHERE id = ...`, letting Postgres cascade. -- Alternative considered: `cascade="all, delete-orphan"`. This would make SQLAlchemy load the collection and emit an explicit `DELETE FROM budget_categories WHERE id IN (...)` before deleting the budget — functionally similar end state, but it still requires the lazy-load (same eager-load discipline the `async-persistence` capability calls for) and does redundant work the DB already does via its own `ON DELETE CASCADE`. `passive_deletes=True` is the smaller, more precise fix that matches the existing DB-level intent from migration `000014`. -- `passive_deletes=True` requires the DB constraint to actually be `ON DELETE CASCADE` (true here, confirmed in the migration) — it would be the wrong choice if the constraint were `RESTRICT`/`NO ACTION`, since then rows would just silently fail to delete. +**Use `cascade="all, delete-orphan", passive_deletes=True` together on `BudgetModel.categories`, not `passive_deletes=True` alone.** +- `passive_deletes=True` tells SQLAlchemy "don't manage this relationship's deletes yourself, the DB's `ON DELETE CASCADE` already does it" — when the collection is *unloaded* (true today, since `get_budget` doesn't eager-load `categories`), it skips the lazy-load-and-null-out step entirely and just issues `DELETE FROM budgets WHERE id = ...`, letting Postgres cascade. +- `passive_deletes=True` alone is not sufficient by itself, though: it only changes what SQLAlchemy does with an *unmanaged* collection. If `categories` is already loaded on the instance being deleted (e.g. a future caller adds `selectinload(BudgetModel.categories)` to `get_budget`, or otherwise touches the collection before `session.delete()`), plain `passive_deletes=True` still walks the already-materialized collection and nulls out each child's `budget_id`, hitting the same `IntegrityError` this change fixes — verified experimentally (SQLite, FK enforcement on). +- Pairing it with `cascade="all, delete-orphan"` closes that gap: with both set, an *unloaded* collection still takes the passive path (no lazy-load, DB cascades), and a *loaded* collection is instead cleaned up by SQLAlchemy's own delete-orphan cascade rather than being nulled out — correct either way, at the cost of one extra `DELETE FROM budget_categories WHERE id IN (...)` in the loaded case, which the DB's `ON DELETE CASCADE` would have done anyway. This is SQLAlchemy's documented pairing for "let the DB cascade when possible, but stay correct if the collection happens to be loaded." +- Both settings still require the DB constraint to actually be `ON DELETE CASCADE` (true here, confirmed in migration `000014`) — the wrong choice if the constraint were `RESTRICT`/`NO ACTION`, since then rows would just silently fail to delete in the unloaded case. ## Risks / Trade-offs diff --git a/openspec/changes/budget-fix-295-category-cascade-delete-fix/tasks.md b/openspec/changes/budget-fix-295-category-cascade-delete-fix/tasks.md index f8215ad..d687684 100644 --- a/openspec/changes/budget-fix-295-category-cascade-delete-fix/tasks.md +++ b/openspec/changes/budget-fix-295-category-cascade-delete-fix/tasks.md @@ -2,9 +2,21 @@ Workflow rule: one task group = one GitHub sub-issue (of this change's parent issue) = one PR, merged before the next group starts. -## 1. Fix budget deletion cascade for orphaned categories + +## Tracking -- [ ] 1.1 Add `passive_deletes=True` to `BudgetModel.categories` in `services/budget/app/models/budget.py` -- [ ] 1.2 Add/extend a `services/budget` test that creates a budget, adds a line (auto-creating a category), deletes the line, then deletes the budget, and verify `DELETE /budgets/{id}` returns 200 with `success: true` instead of a 400 +Parent issue: [#295](https://github.com/arutsh/OpenGrantFlow/issues/295) · [Project board](https://github.com/users/arutsh/projects/8) + +| Group | Sub-issue | Branch | State | +| --- | --- | --- | --- | +| 1. Fix budget deletion cascade for orphaned categories | [#346](https://github.com/arutsh/OpenGrantFlow/issues/346) | `Budget/fix/Issue-346/category-cascade-delete-fix-group1` | open | + +_Generated by `scripts/flow.py sync budget-fix-295-category-cascade-delete-fix` — do not edit by hand._ + + +## 1. Fix budget deletion cascade for orphaned categories — Issue #346 + +- [x] 1.1 Add `cascade="all, delete-orphan", passive_deletes=True` to `BudgetModel.categories` in `services/budget/app/models/budget.py` +- [x] 1.2 Add/extend real-DB `services/budget` tests: (a) create a budget, add a line (auto-creating a category), delete the line, delete the budget, verify success with `categories` unloaded; (b) same, but force `categories` loaded (e.g. re-fetch with `selectinload`) before deleting, verify success there too - [ ] 1.3 Run `frontend-typescript/e2e/specs/api/auth-budget-chain.spec.ts` against the local docker-compose e2e stack (or the `E2E Tests` CI workflow) and verify the DELETE-budget assertion passes - [ ] 1.4 Run `services/budget`'s test suite and lint clean; PR merged diff --git a/services/budget/app/models/budget.py b/services/budget/app/models/budget.py index 66e2464..4a3d3a4 100644 --- a/services/budget/app/models/budget.py +++ b/services/budget/app/models/budget.py @@ -80,7 +80,10 @@ class BudgetModel(Base, AuditMixin): ) reports: Mapped[list["ReportModel"]] = relationship("ReportModel", back_populates="budget") categories: Mapped[list["BudgetCategoryModel"]] = relationship( - "BudgetCategoryModel", back_populates="budget" + "BudgetCategoryModel", + back_populates="budget", + cascade="all, delete-orphan", + passive_deletes=True, ) diff --git a/services/budget/tests/test_budget_crud.py b/services/budget/tests/test_budget_crud.py index ae0359c..570fcf2 100644 --- a/services/budget/tests/test_budget_crud.py +++ b/services/budget/tests/test_budget_crud.py @@ -1,8 +1,13 @@ import uuid import pytest +from sqlalchemy import select +from sqlalchemy.orm import selectinload -from app.crud.budget_crud import create_budget, update_budget +from app.crud.budget_crud import create_budget, delete_budget, update_budget +from app.crud.budget_category_crud import create_budget_category +from app.crud.budget_line_crud import create_budget_line, delete_budget_line +from app.models.budget import BudgetModel from shared.security.current_user_context import reset_current_user_id, set_current_user_id from tests.factories.user import ValidUserFactory @@ -67,3 +72,48 @@ async def test_updated_by_reflects_the_editing_user_not_the_creator(self, db): reset_current_user_id(token) assert updated.updated_by == editor_id + + +async def _budget_with_orphaned_category(db) -> BudgetModel: + """A budget with a line added then removed, leaving its + auto-created category behind (see #295's failure mode).""" + user = ValidUserFactory() + budget = await create_budget( + session=db, user_id=user["user_id"], name="Cascade", owner_id=user["customer_id"] + ) + category = await create_budget_category( + session=db, user_id=user["user_id"], budget_id=budget.id, name="Travel" + ) + line = await create_budget_line( + session=db, + user_id=user["user_id"], + budget_id=budget.id, + category_id=category.id, + description="Flight", + amount=100.0, + ) + await delete_budget_line(session=db, budget_line=line) + return budget + + +@pytest.mark.anyio +class TestDeleteBudgetCascadesOrphanedCategory: + async def test_delete_succeeds_with_categories_unloaded(self, db): + budget = await _budget_with_orphaned_category(db) + + assert await delete_budget(session=db, budget=budget) is True + + async def test_delete_succeeds_with_categories_already_loaded(self, db): + # Distinguishes cascade+passive_deletes from passive_deletes alone, + # which only fixes the unloaded case. + budget = await _budget_with_orphaned_category(db) + + result = await db.execute( + select(BudgetModel) + .where(BudgetModel.id == budget.id) + .options(selectinload(BudgetModel.categories)) + ) + loaded_budget = result.scalar_one() + assert len(list(loaded_budget.categories)) == 1 + + assert await delete_budget(session=db, budget=loaded_budget) is True From 69eb240e95f6a3e5d5d1a76fa6c2cbb9795c5553 Mon Sep 17 00:00:00 2001 From: Norair Arutshyan Date: Thu, 1 Oct 2026 16:08:38 +0100 Subject: [PATCH 2/2] fix(e2e): pin minio to pgsty/minio so fresh CI runners can pull it quay.io/minio/minio now requires auth and minio/minio is gone from Docker Hub, so the E2E workflow failed at image pull on every branch. Switch the local and dev stacks to the pinned community build of the same server, and fold the swap into the 295 change's proposal, design and tasks. Refs #346 Co-Authored-By: Claude Opus 5.5 --- docker-compose.dev.yml | 2 +- docker-compose.local.yml | 2 +- .../design.md | 10 ++++++++++ .../proposal.md | 8 +++++--- .../tasks.md | 5 +++-- 5 files changed, 20 insertions(+), 7 deletions(-) diff --git a/docker-compose.dev.yml b/docker-compose.dev.yml index 49c23fe..6d5e441 100644 --- a/docker-compose.dev.yml +++ b/docker-compose.dev.yml @@ -27,7 +27,7 @@ services: retries: 5 minio: - image: quay.io/minio/minio:latest + image: pgsty/minio:RELEASE.2026-08-04T00-00-00Z container_name: grandflow-minio command: server /data --console-address ":9001" ports: diff --git a/docker-compose.local.yml b/docker-compose.local.yml index f350721..6e60fd8 100644 --- a/docker-compose.local.yml +++ b/docker-compose.local.yml @@ -30,7 +30,7 @@ services: - grandflow minio: - image: quay.io/minio/minio:latest + image: pgsty/minio:RELEASE.2026-08-04T00-00-00Z container_name: grandflow-minio-local command: server /data --console-address ":9001" ports: diff --git a/openspec/changes/budget-fix-295-category-cascade-delete-fix/design.md b/openspec/changes/budget-fix-295-category-cascade-delete-fix/design.md index d3f2695..ae8a828 100644 --- a/openspec/changes/budget-fix-295-category-cascade-delete-fix/design.md +++ b/openspec/changes/budget-fix-295-category-cascade-delete-fix/design.md @@ -8,6 +8,7 @@ See proposal.md - Why for the failure mode. Relevant existing state: - `BudgetModel.categories` (`services/budget/app/models/budget.py`) is a plain `relationship(..., back_populates="budget")` with no `cascade` and no `passive_deletes` setting. - `delete_budget_service` (`services/budget/app/services/budget_services.py`) calls `get_budget_service`, which does **not** eager-load `categories` in this path, then calls `session.delete(budget)` + `commit()`. - Without `passive_deletes=True`, SQLAlchemy's unit-of-work treats "this budget is being deleted" as "I must disassociate its `categories` collection" — since there's no delete cascade configured, it lazy-loads the (unloaded) collection during flush and issues an `UPDATE budget_categories SET budget_id = NULL ...` for each row, which the DB rejects (`budget_id` is `nullable=False`), surfacing as `IntegrityError` → the generic 400. +- The `minio` service in `docker-compose.local.yml` (used by the `E2E Tests` workflow) and `docker-compose.dev.yml` uses `quay.io/minio/minio:latest`. That image now needs authentication (401), and `minio/minio` no longer exists on Docker Hub (404). Developer machines still have a cached copy, but a fresh CI runner can't pull one, so e2e fails before any test runs. - Because the collection load happens inside `flush()`, which the SQLAlchemy asyncio extension runs inside a greenlet, this doesn't crash as a `MissingGreenlet` — it produces a normal `IntegrityError`, so it slipped past the `async-persistence` capability's existing "no implicit lazy-load" guard without tripping any greenlet-specific detection. ## Goals / Non-Goals @@ -15,6 +16,7 @@ See proposal.md - Why for the failure mode. Relevant existing state: **Goals:** - Make `DELETE /api/v1/budgets/{id}` succeed for a budget whose only remaining "problem" is a category row (no lines, reports, receipts, or currency conversions). - Keep the DB as the single source of truth for the cascade, rather than teaching the ORM to duplicate it. +- Restore a pullable MinIO image so the `E2E Tests` workflow can verify the fix on a fresh runner. **Non-Goals:** - Cleaning up orphaned `budget_categories` rows when a line is deleted (separate, pre-existing bookkeeping gap; not required for this fix — see proposal.md). @@ -28,7 +30,15 @@ See proposal.md - Why for the failure mode. Relevant existing state: - Pairing it with `cascade="all, delete-orphan"` closes that gap: with both set, an *unloaded* collection still takes the passive path (no lazy-load, DB cascades), and a *loaded* collection is instead cleaned up by SQLAlchemy's own delete-orphan cascade rather than being nulled out — correct either way, at the cost of one extra `DELETE FROM budget_categories WHERE id IN (...)` in the loaded case, which the DB's `ON DELETE CASCADE` would have done anyway. This is SQLAlchemy's documented pairing for "let the DB cascade when possible, but stay correct if the collection happens to be loaded." - Both settings still require the DB constraint to actually be `ON DELETE CASCADE` (true here, confirmed in migration `000014`) — the wrong choice if the constraint were `RESTRICT`/`NO ACTION`, since then rows would just silently fail to delete in the unloaded case. +**Swap the `minio` image to `pgsty/minio:RELEASE.2026-08-04T00-00-00Z`, pinned, in both `docker-compose.local.yml` and `docker-compose.dev.yml`.** +- `pgsty/minio` is a community rebuild of the same MinIO server using upstream's own Dockerfile and entrypoint. Checked against the registry and compared with the cached upstream image: same entrypoint (`docker-entrypoint.sh`) and `Cmd` (`minio`), identical env (`MINIO_ROOT_*_FILE`, `MC_CONFIG_DIR`, minisign key), and `/usr/bin/mc` is present (a symlink to `mcli`), so the existing `mc ready local` healthcheck still works. Builds exist for amd64 and arm64. The rest of the service block stays unchanged. +- Pin a release tag rather than `:latest`, so the next upstream change is a deliberate bump, not a CI break. +- Alternatives rejected: `chainguard/minio` (free tier offers `:latest` only, can't be pinned); `bitnamilegacy/minio` (frozen in August 2025, different entrypoint and env scheme, would need the service block rewritten); switching to a different S3 server such as Garage, SeaweedFS or RustFS (a storage decision far larger than a CI fix). +- Production (`docker-compose.prod.yml`) doesn't run MinIO, so it's unaffected. + ## Risks / Trade-offs - [`passive_deletes=True` silently relies on the DB constraint matching model expectations; if a future migration changes `budget_categories_budget_id_fkey` back to `RESTRICT` without updating the model, budget deletion would start failing again with a real (unmasked) `IntegrityError`] → Acceptable: that failure mode is the same generic `IntegrityError` → 400 path already in place for lines/reports, so it fails safe (blocks deletion, doesn't corrupt data) rather than silently. - [Orphaned categories with zero lines still accumulate until their budget is deleted] → Out of scope per Non-Goals; tracked separately as a known bookkeeping gap, not a correctness or security issue. +- [`pgsty/minio` is a single-maintainer community build and could go stale or disappear the same way] → The pinned tag keeps CI reproducible while it exists. If it goes away, the swap is the same one-line change per file, and moving to another S3-compatible server becomes the follow-up decision. +- [Existing dev/local `minio_data` volumes were written by an older MinIO release] → MinIO reads data written by older releases. Developers pick up the new image only when they recreate their own minio container. diff --git a/openspec/changes/budget-fix-295-category-cascade-delete-fix/proposal.md b/openspec/changes/budget-fix-295-category-cascade-delete-fix/proposal.md index 6e03845..b080710 100644 --- a/openspec/changes/budget-fix-295-category-cascade-delete-fix/proposal.md +++ b/openspec/changes/budget-fix-295-category-cascade-delete-fix/proposal.md @@ -6,8 +6,9 @@ ## What Changes -- `BudgetModel.categories` gets `passive_deletes=True` so SQLAlchemy's ORM defers to the database's existing `ON DELETE CASCADE` on `budget_categories.budget_id` instead of trying to lazy-load the collection and null out each child's non-nullable `budget_id` during flush (which raises the spurious `IntegrityError`). -- Out of scope: cleaning up an orphaned category when its last line is deleted is a separate, pre-existing bookkeeping gap (also related to the already-known global-category-namespace issue) and is not needed to fix this bug — `passive_deletes=True` makes budget deletion correct regardless of whether an orphaned category row exists. +- `BudgetModel.categories` gets `cascade="all, delete-orphan", passive_deletes=True` so SQLAlchemy defers to the database's existing `ON DELETE CASCADE` on `budget_categories.budget_id` instead of lazy-loading the collection and nulling out each child's non-nullable `budget_id` during flush (which raises the spurious `IntegrityError`), and deletes the children itself if the collection happens to be loaded (see design.md). +- The `minio` service in `docker-compose.local.yml` and `docker-compose.dev.yml` switches from `quay.io/minio/minio:latest` to the pinned community build `pgsty/minio:RELEASE.2026-08-04T00-00-00Z`. MinIO stopped publishing public images (quay.io now returns 401, Docker Hub's `minio/minio` returns 404), so a fresh CI runner can't start the e2e stack — without this, the `E2E Tests` workflow can't verify the fix above. +- Out of scope: cleaning up an orphaned category when its last line is deleted is a separate, pre-existing bookkeeping gap (also related to the already-known global-category-namespace issue) and is not needed to fix this bug — the cascade settings make budget deletion correct regardless of whether an orphaned category row exists. ## Capabilities @@ -23,4 +24,5 @@ None — this restores compliance with the already-declared `async-persistence` - `services/budget/app/models/budget.py` (`BudgetModel.categories` relationship) - Fixes `DELETE /api/v1/budgets/{id}` for any budget that ever had a line added and removed -- Unblocks `frontend-typescript/e2e/specs/api/auth-budget-chain.spec.ts` in the `E2E Tests` CI workflow +- `docker-compose.local.yml`, `docker-compose.dev.yml` (`minio` service image only; `command`, env, volumes and healthcheck unchanged) +- Unblocks `frontend-typescript/e2e/specs/api/auth-budget-chain.spec.ts` in the `E2E Tests` CI workflow, which currently fails at image pull on every branch, `main` included diff --git a/openspec/changes/budget-fix-295-category-cascade-delete-fix/tasks.md b/openspec/changes/budget-fix-295-category-cascade-delete-fix/tasks.md index d687684..4581f1f 100644 --- a/openspec/changes/budget-fix-295-category-cascade-delete-fix/tasks.md +++ b/openspec/changes/budget-fix-295-category-cascade-delete-fix/tasks.md @@ -18,5 +18,6 @@ _Generated by `scripts/flow.py sync budget-fix-295-category-cascade-delete-fix` - [x] 1.1 Add `cascade="all, delete-orphan", passive_deletes=True` to `BudgetModel.categories` in `services/budget/app/models/budget.py` - [x] 1.2 Add/extend real-DB `services/budget` tests: (a) create a budget, add a line (auto-creating a category), delete the line, delete the budget, verify success with `categories` unloaded; (b) same, but force `categories` loaded (e.g. re-fetch with `selectinload`) before deleting, verify success there too -- [ ] 1.3 Run `frontend-typescript/e2e/specs/api/auth-budget-chain.spec.ts` against the local docker-compose e2e stack (or the `E2E Tests` CI workflow) and verify the DELETE-budget assertion passes -- [ ] 1.4 Run `services/budget`'s test suite and lint clean; PR merged +- [x] 1.3 Swap the `minio` service image in `docker-compose.local.yml` and `docker-compose.dev.yml` from `quay.io/minio/minio:latest` to `pgsty/minio:RELEASE.2026-08-04T00-00-00Z` (see design.md), so the `E2E Tests` workflow can pull it on a fresh runner +- [ ] 1.4 Run `frontend-typescript/e2e/specs/api/auth-budget-chain.spec.ts` against the local docker-compose e2e stack (or the `E2E Tests` CI workflow) and verify the DELETE-budget assertion passes +- [ ] 1.5 Run `services/budget`'s test suite and lint clean; PR merged