From d362858f46aaadf55778d71bd9fe7e6d1f18f848 Mon Sep 17 00:00:00 2001 From: Renzo F <45401804+rennf93@users.noreply.github.com> Date: Mon, 20 Jul 2026 07:54:38 +0200 Subject: [PATCH] fix(notification): ack notifications can join the caller's transaction (#603) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The release engine's bell notification for a just-originated proposal inserted through a fresh session while the proposal task sat uncommitted in the engine's own transaction — the related_task_id FK rejected the row and the ping was silently lost (caught live in the postgres log; the DB-free Telegram DM still went out). send_ack_notification now accepts db_session, forwarded to _create_notification so the insert joins the caller's transaction, and the release engine passes its session. The other five callers pass no task_id or reference committed tasks. Co-authored-by: Renn F --- CHANGELOG.md | 1 + roboco/services/notification.py | 9 +- roboco/services/release_manager_engine.py | 6 +- .../test_notification_ack_session_race.py | 89 +++++++++++++++++++ 4 files changed, 103 insertions(+), 2 deletions(-) create mode 100644 tests/integration/test_notification_ack_session_race.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 71ea9ebf..257c925b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), - **`/auth/login` no longer 422s (#580).** FastAPI had demoted the db dependency to a query parameter on the login route. - **Task cancellation closes the task's own PR, and the bulk branch sweep spares live dependents (#593).** Every cancel path now best-effort-closes the recorded open PR for the task and its cascaded descendants; the stale-branch sweep excludes branches still recorded by a non-terminal task or serving as a live child's resolved parent branch. - **Release/readiness hardening basket.** The root PR base resolves the project's env ladder instead of literal master; version detection accepts manifest variants and never crashes the sweep; unconfigured social platforms are skipped instead of becoming pending-forever failures (#545); the readiness sweep's CI wait polls the prod rung; and check-runs dedupe per name so a cancelled duplicate can't mask a green run. +- **The release-proposal bell notification survives its own transaction.** `send_ack_notification` inserted the notification row through a fresh session while the proposal task sat uncommitted in the origination engine's transaction, so the `related_task_id` FK rejected it and the panel-bell ping was silently lost (the Telegram DM, which is DB-free, still went out). The service now accepts the caller's session so the insert joins the same transaction, and the release engine passes it. - **Panel/ops hardening basket.** Session links target the owning task (the `/work-sessions/` route never existed); PM review turns are restart-safe; dialog triggers behind tooltips fire again and dotted composition ids render; `git pull` on a target branch became a hard sync to origin; the `git_provider` migration renumbered to 076 restoring a single Alembic head; CI-watch fixed its own regression on roboco-api (#563); agent commits no longer carry model self-attribution; and cockpit CI assertions became delta-based so the one-process run's row leaks can't flake them (#577, #578). ## [0.25.0] - 2026-07-16 diff --git a/roboco/services/notification.py b/roboco/services/notification.py index bbf15e84..89906dfc 100644 --- a/roboco/services/notification.py +++ b/roboco/services/notification.py @@ -611,6 +611,7 @@ class NotificationService: body: str, priority: NotificationPriority = NotificationPriority.NORMAL, task_id: UUID | str | None = None, + db_session: AsyncSession | None = None, ) -> None: """Send a free-form ack-required notification (PM/Board only). @@ -625,6 +626,11 @@ class NotificationService: """ subject = body.split("\n", 1)[0][:200] or "Notification" related_task_id = str(task_id) if task_id is not None else None + # A caller notifying about a row its OWN open transaction just + # created must pass db_session so the insert joins that transaction + # — a fresh session can't see the uncommitted task and the + # related_task_id FK rejects the row (the 0.26.0 release-proposal + # bell notification was lost exactly this way). await self._create_notification( CreateNotificationParams( notification_type=NotificationType.ALERT, @@ -634,7 +640,8 @@ class NotificationService: subject=subject, body=body, related_task_id=related_task_id, - ) + ), + db_session=db_session, ) async def send_broadcast_notification( diff --git a/roboco/services/release_manager_engine.py b/roboco/services/release_manager_engine.py index a93c9122..2fee1404 100644 --- a/roboco/services/release_manager_engine.py +++ b/roboco/services/release_manager_engine.py @@ -201,7 +201,11 @@ class ReleaseManagerEngine(BaseService): ) try: await NotificationService().send_ack_notification( - from_agent="system", to_agent="ceo", body=body, task_id=str(task.id) + from_agent="system", + to_agent="ceo", + body=body, + task_id=str(task.id), + db_session=self.session, ) except Exception as exc: self.log.warning("release CEO notify failed (best-effort)", error=str(exc)) diff --git a/tests/integration/test_notification_ack_session_race.py b/tests/integration/test_notification_ack_session_race.py new file mode 100644 index 00000000..0789290a --- /dev/null +++ b/tests/integration/test_notification_ack_session_race.py @@ -0,0 +1,89 @@ +"""send_ack_notification must be able to join the caller's transaction. + +The origination engines notify about a task their OWN open transaction just +created; a fresh session cannot see that uncommitted row, so the +notifications.related_task_id FK rejects the insert and the bell +notification is silently lost (the 0.26.0 release-proposal case, caught in +the live postgres log). Real-Postgres coverage for both directions. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, cast +from uuid import uuid4 + +import pytest +from roboco.db.tables import AgentTable, NotificationTable, TaskTable +from roboco.models.base import ( + AgentRole, + AgentStatus, + Complexity, + TaskNature, + TaskStatus, + TaskType, + Team, +) +from roboco.services.notification import NotificationService +from sqlalchemy import select + +if TYPE_CHECKING: + from uuid import UUID + + from sqlalchemy.ext.asyncio import AsyncSession + + +async def _seed(db_session: AsyncSession) -> tuple[AgentTable, TaskTable]: + unique = uuid4().hex[:6] + agent = AgentTable( + id=uuid4(), + name=f"ceo-{unique}", + slug=f"ceo-{unique}", + role=AgentRole.CEO, + team=None, + status=AgentStatus.ACTIVE, + model_config={}, + system_prompt="x", + capabilities=[], + permissions={}, + metrics={}, + ) + db_session.add(agent) + await db_session.flush() + task = TaskTable( + id=uuid4(), + title="Release proposal: v9.9.9", + description="d", + acceptance_criteria=["ac"], + task_type=TaskType.ADMINISTRATIVE, + nature=TaskNature.NON_TECHNICAL, + status=TaskStatus.PENDING, + team=Team.BOARD, + created_by=agent.id, + estimated_complexity=Complexity.LOW, + ) + db_session.add(task) + # Flushed but NOT committed — exactly the engine's state at notify time. + await db_session.flush() + return agent, task + + +@pytest.mark.asyncio +async def test_ack_notification_joins_the_callers_transaction( + db_session: AsyncSession, +) -> None: + agent, task = await _seed(db_session) + await NotificationService().send_ack_notification( + from_agent=agent.slug, + to_agent=agent.slug, + body="v9.9.9 ready", + task_id=cast("UUID", task.id), + db_session=db_session, + ) + row = ( + await db_session.execute( + select(NotificationTable).where( + NotificationTable.related_task_id == task.id + ) + ) + ).scalar_one() + assert row.subject == "v9.9.9 ready"