mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(notifications): stop tick-wide row locks + poisoned-session swallows behind the PendingRollbackError 500s (#743)
* fix(notifications): release re-escalation row locks per row; stop swallowing DB errors in notify_get The re-escalation sweep ran one tick-wide transaction, so each CAS claim's row lock was held across every remaining delivery until the single commit — a concurrent mark-read UPDATE on a claimed row starved into the 60s lock_timeout. The sweep now commits per row (claim commit releases the lock before delivery and makes the burned slot durable), re-fetches each row by snapshotted id so one row's rollback can't expire the rest of the tick, and savepoints each recipient's delivery. notify_get's bare except swallowed the resulting LockNotAvailableError into a false "notification not found" and returned a poisoned session to the commit-at-send middleware, which blew up with PendingRollbackError; it now catches only the two domain outcomes. defer_after_commit's listeners fire on SAVEPOINT release too, which would have drained deferred telegram/bus work before real durability — they now skip savepoint boundaries via get_nested_transaction() (the root get_transaction() is non-None inside the listener even at a real commit). acknowledge_for_recipient's Redis dedup-clear moved before the flush so the row lock never spans a Redis round-trip. The five best-effort CEO-notify swallows that persist notification rows are savepointed. * fix(services): contain swallowed best-effort DB write failures instead of poisoning the session Sweep of the same class as the notify_get incident: broad except-Exception handlers that swallow a failure whose try-body writes through the shared session leave the session rollback-pending, and the verb/request then dies later with PendingRollbackError at commit-at-send. Confirmed-dangerous sites now run the write inside a savepoint (safe since defer_after_commit skips savepoint boundaries): ceo_approve's verified-stamp, completion/pitch/postmortem-style CEO notifies, _inherit_upstream_base, _link_commit_to_task (covers every commit route), board-program LEARN records, the QA/PR-gate/PM-merge verified-stamps, and the documenter->PM handoff. _ack_pending_wake_notifications gets the same treatment so a wake-ack failure can't fail the A2A read. telegram_inbound's per-update loop and intake confirm roll back explicitly instead (their success paths commit mid-flow, so a savepoint doesn't fit). A swallowed savepoint rollback fully expires any ORM object mutated inside the block, and the next attribute read raises MissingGreenlet — strictly worse than the original bug. The two paths that keep using the object after the swallow (doc handoff's envelope build, base inheritance's claim continuation) refresh it in the except path; regression tests run against a real session and were verified to fail with the refresh reverted. * test: shape mocked session.execute results so sync accessors stop leaking unawaited coroutines An AsyncMock's auto-created children are themselves AsyncMock, so production code that correctly awaits session.execute() and then calls sync accessors (.scalars().all(), .scalar_one_or_none()) on the result was silently collecting unawaited coroutines in 22 test files — 80 RuntimeWarnings per unit run, and in test_flow_soup_guard one mock raised a real TypeError that a coincidentally-matching invalid_state envelope masked. Each affected fixture now returns a plain MagicMock shaped like a real Result. Zero AsyncMock warnings remain. * docs: document per-row sweep commits and the savepoint/refresh containment pattern --------- Co-authored-by: Renn F <rennf93@users.noreply.github.com>
This commit is contained in:
@@ -35,6 +35,15 @@ def _make_deps(**overrides: Any) -> ChoreographerDeps:
|
||||
__aexit__=AsyncMock(return_value=False),
|
||||
)
|
||||
)
|
||||
# Findings-ledger reads (ReviewFindingsRepository.list_for_task) go
|
||||
# through session.execute — an unconfigured AsyncMock's awaited result
|
||||
# is itself an AsyncMock, so a plain sync `.scalars()` call on it leaks
|
||||
# an unawaited coroutine. Empty scalars result (no findings).
|
||||
task.session.execute = AsyncMock(
|
||||
return_value=MagicMock(
|
||||
scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[])))
|
||||
)
|
||||
)
|
||||
repo = base["evidence_repo"]
|
||||
for method in (
|
||||
"list_unread_a2a",
|
||||
|
||||
@@ -40,6 +40,15 @@ def _make_deps(**overrides: Any) -> ChoreographerDeps:
|
||||
"evidence_repo": AsyncMock(),
|
||||
}
|
||||
base.update(overrides)
|
||||
# Findings-ledger reads (ReviewFindingsRepository.list_for_task) go
|
||||
# through session.execute — an unconfigured AsyncMock's awaited result
|
||||
# is itself an AsyncMock, so a plain sync `.scalars()` call on it leaks
|
||||
# an unawaited coroutine. Empty scalars result (no findings).
|
||||
base["task"].session.execute = AsyncMock(
|
||||
return_value=MagicMock(
|
||||
scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[])))
|
||||
)
|
||||
)
|
||||
repo = base["evidence_repo"]
|
||||
for method in (
|
||||
"list_unread_a2a",
|
||||
|
||||
@@ -41,6 +41,15 @@ def _over_cap_project() -> MagicMock:
|
||||
|
||||
|
||||
def _make_deps(task_svc: AsyncMock, **overrides: Any) -> ChoreographerDeps:
|
||||
# Findings-ledger reads (ReviewFindingsRepository.list_for_task) go
|
||||
# through session.execute — an unconfigured AsyncMock's awaited result
|
||||
# is itself an AsyncMock, so a plain sync `.scalars()` call on it leaks
|
||||
# an unawaited coroutine. Empty scalars result (no findings).
|
||||
task_svc.session.execute = AsyncMock(
|
||||
return_value=MagicMock(
|
||||
scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[])))
|
||||
)
|
||||
)
|
||||
base: dict[str, Any] = {
|
||||
"task": task_svc,
|
||||
"work_session": AsyncMock(),
|
||||
|
||||
@@ -37,6 +37,15 @@ def _choreographer(
|
||||
repo.similar_memory = AsyncMock(return_value=similar_memory_out)
|
||||
task_svc = AsyncMock()
|
||||
task_svc.agent_for.return_value = MagicMock(role="developer")
|
||||
# Findings-ledger reads (ReviewFindingsRepository.list_for_task) go
|
||||
# through session.execute — an unconfigured AsyncMock's awaited result
|
||||
# is itself an AsyncMock, so a plain sync `.scalars()` call on it leaks
|
||||
# an unawaited coroutine. Empty scalars result (no findings).
|
||||
task_svc.session.execute = AsyncMock(
|
||||
return_value=MagicMock(
|
||||
scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[])))
|
||||
)
|
||||
)
|
||||
choreo = object.__new__(Choreographer)
|
||||
choreo._deps = MagicMock(evidence_repo=repo, task=task_svc)
|
||||
return choreo, repo
|
||||
|
||||
@@ -22,6 +22,15 @@ def _make_deps(**overrides: Any) -> ChoreographerDeps:
|
||||
"evidence_repo": AsyncMock(),
|
||||
}
|
||||
base.update(overrides)
|
||||
# Findings-ledger reads (ReviewFindingsRepository.list_for_task) go
|
||||
# through session.execute — an unconfigured AsyncMock's awaited result
|
||||
# is itself an AsyncMock, so a plain sync `.scalars()` call on it leaks
|
||||
# an unawaited coroutine. Empty scalars result (no findings).
|
||||
base["task"].session.execute = AsyncMock(
|
||||
return_value=MagicMock(
|
||||
scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[])))
|
||||
)
|
||||
)
|
||||
repo = base["evidence_repo"]
|
||||
for method in (
|
||||
"list_unread_a2a",
|
||||
@@ -215,6 +224,7 @@ async def test_i_documented_succeeds_and_transitions() -> None:
|
||||
task_svc.cell_pm_for_team.return_value = MagicMock(id=uuid4())
|
||||
task_svc.session = MagicMock()
|
||||
task_svc.session.flush = AsyncMock()
|
||||
task_svc.session.refresh = AsyncMock()
|
||||
task_svc.session.begin_nested = MagicMock(
|
||||
return_value=MagicMock(
|
||||
__aenter__=AsyncMock(return_value=None),
|
||||
@@ -252,6 +262,7 @@ def _doc_success_task_svc(task_id: Any, doc_id: Any) -> AsyncMock:
|
||||
task_svc.cell_pm_for_team.return_value = MagicMock(id=uuid4())
|
||||
task_svc.session = MagicMock()
|
||||
task_svc.session.flush = AsyncMock()
|
||||
task_svc.session.refresh = AsyncMock()
|
||||
task_svc.session.begin_nested = MagicMock(
|
||||
return_value=MagicMock(
|
||||
__aenter__=AsyncMock(return_value=None),
|
||||
@@ -352,6 +363,7 @@ async def test_i_documented_survives_handoff_failure() -> None:
|
||||
task_svc.cell_pm_for_team.return_value = MagicMock(id=uuid4())
|
||||
task_svc.session = MagicMock()
|
||||
task_svc.session.flush = AsyncMock()
|
||||
task_svc.session.refresh = AsyncMock()
|
||||
task_svc.session.begin_nested = MagicMock(
|
||||
return_value=MagicMock(
|
||||
__aenter__=AsyncMock(return_value=None),
|
||||
|
||||
@@ -40,6 +40,15 @@ def _dev_agent_task_svc() -> tuple[AsyncMock, UUID]:
|
||||
task_svc.list_in_progress_for_agent.return_value = []
|
||||
# Default: lane clear (no earlier incomplete sibling).
|
||||
task_svc.has_earlier_incomplete_code_sibling.return_value = False
|
||||
# Findings-ledger reads (ReviewFindingsRepository.list_for_task) go
|
||||
# through session.execute — an unconfigured AsyncMock's awaited result
|
||||
# is itself an AsyncMock, so a plain sync `.scalars()` call on it leaks
|
||||
# an unawaited coroutine. Empty scalars result (no findings).
|
||||
task_svc.session.execute = AsyncMock(
|
||||
return_value=MagicMock(
|
||||
scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[])))
|
||||
)
|
||||
)
|
||||
return task_svc, uuid4()
|
||||
|
||||
|
||||
|
||||
@@ -53,6 +53,18 @@ def _make_deps(**overrides: Any) -> ChoreographerDeps:
|
||||
scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[])))
|
||||
)
|
||||
)
|
||||
# That same verified-stamp now runs inside its own savepoint (a mid-flush
|
||||
# failure must not poison the shared session) — an unconfigured
|
||||
# AsyncMock's begin_nested() call returns a raw unawaited coroutine,
|
||||
# which `async with` cannot use. Same shape as the execute default above;
|
||||
# a test's own explicit begin_nested config (e.g. submit_root's) is the
|
||||
# identical shape, so overwriting it here is a no-op for those tests.
|
||||
base["task"].session.begin_nested = MagicMock(
|
||||
return_value=MagicMock(
|
||||
__aenter__=AsyncMock(return_value=None),
|
||||
__aexit__=AsyncMock(return_value=False),
|
||||
)
|
||||
)
|
||||
return ChoreographerDeps(**base)
|
||||
|
||||
|
||||
|
||||
@@ -78,6 +78,7 @@ async def test_claim_review_returns_evidence_inline() -> None:
|
||||
task_svc.list_in_progress_for_agent.return_value = []
|
||||
task_svc.list_paused_for_agent.return_value = []
|
||||
task_svc.qa_claim.return_value = t_claimed
|
||||
_stub_empty_ledger(task_svc.session)
|
||||
work_svc = AsyncMock()
|
||||
git_svc = AsyncMock()
|
||||
git_svc.diff.return_value = "+++ diff content"
|
||||
@@ -146,6 +147,7 @@ async def test_claim_review_marks_evidence_inspected() -> None:
|
||||
task_svc.list_in_progress_for_agent.return_value = []
|
||||
task_svc.list_paused_for_agent.return_value = []
|
||||
task_svc.qa_claim.return_value = t_claimed
|
||||
_stub_empty_ledger(task_svc.session)
|
||||
git_svc = AsyncMock()
|
||||
git_svc.diff.return_value = ""
|
||||
deps = _make_deps(task=task_svc, git=git_svc)
|
||||
|
||||
@@ -51,6 +51,15 @@ def _make_deps(**overrides: Any) -> ChoreographerDeps:
|
||||
__aexit__=AsyncMock(return_value=False),
|
||||
)
|
||||
)
|
||||
# Findings-ledger reads (ReviewFindingsRepository.list_for_task) go
|
||||
# through session.execute — an unconfigured AsyncMock's awaited result
|
||||
# is itself an AsyncMock, so a plain sync `.scalars()` call on it leaks
|
||||
# an unawaited coroutine. Empty scalars result (no findings).
|
||||
task.session.execute = AsyncMock(
|
||||
return_value=MagicMock(
|
||||
scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[])))
|
||||
)
|
||||
)
|
||||
repo = base["evidence_repo"]
|
||||
for method in (
|
||||
"list_unread_a2a",
|
||||
|
||||
@@ -31,6 +31,15 @@ def _make_deps(**overrides: Any) -> ChoreographerDeps:
|
||||
"evidence_repo": AsyncMock(),
|
||||
}
|
||||
base.update(overrides)
|
||||
# Findings-ledger reads (ReviewFindingsRepository.list_for_task) go
|
||||
# through session.execute — an unconfigured AsyncMock's awaited result
|
||||
# is itself an AsyncMock, so a plain sync `.scalars()` call on it leaks
|
||||
# an unawaited coroutine. Empty scalars result (no findings).
|
||||
base["task"].session.execute = AsyncMock(
|
||||
return_value=MagicMock(
|
||||
scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[])))
|
||||
)
|
||||
)
|
||||
repo = base["evidence_repo"]
|
||||
for m in (
|
||||
"list_unread_a2a",
|
||||
|
||||
@@ -644,6 +644,15 @@ async def test_evidence_valid_task_returns_ok_with_pr_diff() -> None:
|
||||
)
|
||||
task_svc = AsyncMock()
|
||||
task_svc.get.return_value = task_obj
|
||||
# Findings-ledger reads (ReviewFindingsRepository.list_for_task) go
|
||||
# through session.execute — an unconfigured AsyncMock's awaited result
|
||||
# is itself an AsyncMock, so a plain sync `.scalars()` call on it leaks
|
||||
# an unawaited coroutine. Empty scalars result (no findings).
|
||||
task_svc.session.execute = AsyncMock(
|
||||
return_value=MagicMock(
|
||||
scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[])))
|
||||
)
|
||||
)
|
||||
git_svc = AsyncMock()
|
||||
git_svc.diff.return_value = "diff --git a/foo.py b/foo.py\n+added line"
|
||||
workspace_svc = AsyncMock()
|
||||
|
||||
@@ -409,6 +409,15 @@ async def test_evidence_unassigned_task_allows_inspection() -> None:
|
||||
)
|
||||
task_svc = AsyncMock()
|
||||
task_svc.get.return_value = task_obj
|
||||
# Findings-ledger reads (ReviewFindingsRepository.list_for_task) go
|
||||
# through session.execute — an unconfigured AsyncMock's awaited result
|
||||
# is itself an AsyncMock, so a plain sync `.scalars()` call on it leaks
|
||||
# an unawaited coroutine. Empty scalars result (no findings).
|
||||
task_svc.session.execute = AsyncMock(
|
||||
return_value=MagicMock(
|
||||
scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[])))
|
||||
)
|
||||
)
|
||||
git_svc = AsyncMock()
|
||||
git_svc.diff.return_value = "diff content"
|
||||
workspace_svc = AsyncMock()
|
||||
@@ -445,6 +454,15 @@ async def test_evidence_allows_dependency_inspection() -> None:
|
||||
task_svc = AsyncMock()
|
||||
task_svc.get.return_value = target
|
||||
task_svc.list_assigned_for_agent.return_value = [callers_task]
|
||||
# Findings-ledger reads (ReviewFindingsRepository.list_for_task) go
|
||||
# through session.execute — an unconfigured AsyncMock's awaited result
|
||||
# is itself an AsyncMock, so a plain sync `.scalars()` call on it leaks
|
||||
# an unawaited coroutine. Empty scalars result (no findings).
|
||||
task_svc.session.execute = AsyncMock(
|
||||
return_value=MagicMock(
|
||||
scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[])))
|
||||
)
|
||||
)
|
||||
git_svc = AsyncMock()
|
||||
git_svc.diff.return_value = ""
|
||||
git_svc.list_changed_files.return_value = []
|
||||
|
||||
@@ -29,6 +29,15 @@ def _deps_for_evidence(
|
||||
workspace_svc: AsyncMock,
|
||||
evidence_repo: AsyncMock,
|
||||
) -> ContentActionsDeps:
|
||||
# Findings-ledger reads (ReviewFindingsRepository.list_for_task) go
|
||||
# through session.execute — an unconfigured AsyncMock's awaited result
|
||||
# is itself an AsyncMock, so a plain sync `.scalars()` call on it leaks
|
||||
# an unawaited coroutine. Empty scalars result (no findings).
|
||||
task_svc.session.execute = AsyncMock(
|
||||
return_value=MagicMock(
|
||||
scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[])))
|
||||
)
|
||||
)
|
||||
return ContentActionsDeps(
|
||||
task=task_svc,
|
||||
git=git_svc,
|
||||
|
||||
@@ -38,6 +38,17 @@ def _make_deps(**overrides: Any) -> ChoreographerDeps:
|
||||
"evidence_repo": AsyncMock(),
|
||||
}
|
||||
base.update(overrides)
|
||||
# Findings-ledger reads (ReviewFindingsRepository.list_for_task) go
|
||||
# through session.execute — an unconfigured AsyncMock's awaited result
|
||||
# is itself an AsyncMock, so a plain sync `.scalars()` call on it leaks
|
||||
# an unawaited coroutine. Empty scalars result (no findings); a test that
|
||||
# needs real ledger data monkeypatches the findings module functions
|
||||
# directly (see below), so this default never masks that.
|
||||
base["task"].session.execute = AsyncMock(
|
||||
return_value=MagicMock(
|
||||
scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[])))
|
||||
)
|
||||
)
|
||||
repo = base["evidence_repo"]
|
||||
for method in (
|
||||
"list_unread_a2a",
|
||||
|
||||
@@ -115,6 +115,18 @@ def _make_deps(agent_id: object, task_id: object) -> ChoreographerDeps:
|
||||
task_svc.agent_for.return_value = MagicMock(
|
||||
id=agent_id, role="developer", team="backend", slug="be-dev-1"
|
||||
)
|
||||
# VerbRunner uses task.session.begin_nested() as a savepoint context
|
||||
# manager — an unconfigured AsyncMock's `begin_nested()` call returns a
|
||||
# raw unawaited coroutine, which `async with` cannot use (real failure,
|
||||
# not just a warning: it was silently turning into a masking
|
||||
# "verb runner failed" invalid_state envelope instead of exercising the
|
||||
# real block path below).
|
||||
task_svc.session.begin_nested = MagicMock(
|
||||
return_value=MagicMock(
|
||||
__aenter__=AsyncMock(return_value=None),
|
||||
__aexit__=AsyncMock(return_value=False),
|
||||
)
|
||||
)
|
||||
evidence_repo = AsyncMock()
|
||||
for m in (
|
||||
"list_unread_a2a",
|
||||
|
||||
@@ -23,6 +23,14 @@ from roboco.services.gateway.choreographer import Choreographer, ChoreographerDe
|
||||
|
||||
|
||||
def _make_choreographer(*, task_service: AsyncMock, git: AsyncMock) -> Choreographer:
|
||||
# `_project_slug_for`/`ReviewFindingsRepository.list_for_task` both read
|
||||
# via session.execute — an unconfigured AsyncMock's awaited result is
|
||||
# itself an AsyncMock, so a plain sync `.scalars()`/`.scalar_one_or_none()`
|
||||
# call on it leaks an unawaited coroutine. A bare MagicMock's `.scalars()
|
||||
# .all()` already returns `[]` by default; these tests don't assert on
|
||||
# the resolved project/slug, so a default (truthy) `.scalar_one_or_none()`
|
||||
# is harmless too.
|
||||
task_service.session.execute = AsyncMock(return_value=MagicMock())
|
||||
return Choreographer(
|
||||
ChoreographerDeps(
|
||||
task=task_service,
|
||||
|
||||
@@ -33,6 +33,15 @@ def _make_deps(**overrides: Any) -> ChoreographerDeps:
|
||||
"evidence_repo": AsyncMock(),
|
||||
}
|
||||
base.update(overrides)
|
||||
# Findings-ledger reads (ReviewFindingsRepository.list_for_task) go
|
||||
# through session.execute — an unconfigured AsyncMock's awaited result
|
||||
# is itself an AsyncMock, so a plain sync `.scalars()` call on it leaks
|
||||
# an unawaited coroutine. Empty scalars result (no findings).
|
||||
base["task"].session.execute = AsyncMock(
|
||||
return_value=MagicMock(
|
||||
scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[])))
|
||||
)
|
||||
)
|
||||
repo = base["evidence_repo"]
|
||||
for method in (
|
||||
"list_unread_a2a",
|
||||
|
||||
@@ -53,6 +53,15 @@ def test_pm_claim_needs_revision_works_for_code_typed_root() -> None:
|
||||
|
||||
|
||||
def _make_deps(task_svc: AsyncMock) -> ChoreographerDeps:
|
||||
# Findings-ledger reads (ReviewFindingsRepository.list_for_task) go
|
||||
# through session.execute — an unconfigured AsyncMock's awaited result
|
||||
# is itself an AsyncMock, so a plain sync `.scalars()` call on it leaks
|
||||
# an unawaited coroutine. Empty scalars result (no findings).
|
||||
task_svc.session.execute = AsyncMock(
|
||||
return_value=MagicMock(
|
||||
scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[])))
|
||||
)
|
||||
)
|
||||
repo = AsyncMock()
|
||||
for m in (
|
||||
"list_unread_a2a",
|
||||
|
||||
@@ -0,0 +1,123 @@
|
||||
"""Tests for ContentActions.notify_get — read-one-notification (marks read).
|
||||
|
||||
`notify_get` used to swallow ANY exception from
|
||||
`get_for_recipient_and_mark_read` into `Envelope.not_found` — including a DB
|
||||
error from the mark-read UPDATE (e.g. hitting `lock_timeout`), which poisoned
|
||||
the session for the rest of the transaction and surfaced later as an opaque
|
||||
`PendingRollbackError`, while also lying to the calling agent that an
|
||||
existing notification didn't exist. The fix narrows the catch to the two real
|
||||
domain outcomes (`NotFoundError`, `PermissionError`); anything else must
|
||||
propagate so the session actually rolls back.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from unittest.mock import AsyncMock, MagicMock
|
||||
from uuid import uuid4
|
||||
|
||||
import pytest
|
||||
from roboco.services.base import NotFoundError
|
||||
from roboco.services.gateway.content_actions import ContentActions, ContentActionsDeps
|
||||
from sqlalchemy.exc import OperationalError
|
||||
|
||||
|
||||
def _make_deps(**overrides: AsyncMock) -> ContentActionsDeps:
|
||||
task = overrides.get("task", AsyncMock())
|
||||
git = overrides.get("git", AsyncMock())
|
||||
a2a = overrides.get("a2a", AsyncMock())
|
||||
journal = overrides.get("journal", AsyncMock())
|
||||
workspace = overrides.get("workspace", AsyncMock())
|
||||
notifications = overrides.get("notifications", AsyncMock())
|
||||
notification_delivery = overrides.get("notification_delivery", AsyncMock())
|
||||
return ContentActionsDeps(
|
||||
task=task,
|
||||
git=git,
|
||||
a2a=a2a,
|
||||
journal=journal,
|
||||
workspace=workspace,
|
||||
notifications=notifications,
|
||||
notification_delivery=notification_delivery,
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_notify_get_not_found_error_maps_to_not_found() -> None:
|
||||
"""A genuinely missing notification -> Envelope.not_found."""
|
||||
notification_id = uuid4()
|
||||
notif_delivery = AsyncMock()
|
||||
notif_delivery.get_for_recipient_and_mark_read.side_effect = NotFoundError(
|
||||
resource_type="Notification", resource_id=str(notification_id)
|
||||
)
|
||||
deps = _make_deps(notification_delivery=notif_delivery)
|
||||
ca = ContentActions(deps)
|
||||
|
||||
env = await ca.notify_get(agent_id=uuid4(), notification_id=notification_id)
|
||||
body = env.as_dict()
|
||||
|
||||
assert body["error"] == "not_found"
|
||||
assert str(notification_id) in body["message"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_notify_get_permission_error_maps_to_not_found() -> None:
|
||||
"""A recipient mismatch -> Envelope.not_found (never leaks a 403/details)."""
|
||||
notification_id = uuid4()
|
||||
notif_delivery = AsyncMock()
|
||||
notif_delivery.get_for_recipient_and_mark_read.side_effect = PermissionError(
|
||||
"view notification: not a recipient"
|
||||
)
|
||||
deps = _make_deps(notification_delivery=notif_delivery)
|
||||
ca = ContentActions(deps)
|
||||
|
||||
env = await ca.notify_get(agent_id=uuid4(), notification_id=notification_id)
|
||||
body = env.as_dict()
|
||||
|
||||
assert body["error"] == "not_found"
|
||||
assert str(notification_id) in body["message"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_notify_get_db_error_propagates() -> None:
|
||||
"""A DB-shaped failure (e.g. the mark-read UPDATE hitting lock_timeout)
|
||||
must NOT be swallowed into not_found — it has to propagate so the
|
||||
session actually rolls back instead of silently poisoning the
|
||||
transaction for the caller's later commit."""
|
||||
notification_id = uuid4()
|
||||
notif_delivery = AsyncMock()
|
||||
notif_delivery.get_for_recipient_and_mark_read.side_effect = OperationalError(
|
||||
"UPDATE notifications ...", {}, Exception("lock timeout")
|
||||
)
|
||||
deps = _make_deps(notification_delivery=notif_delivery)
|
||||
ca = ContentActions(deps)
|
||||
|
||||
with pytest.raises(OperationalError):
|
||||
await ca.notify_get(agent_id=uuid4(), notification_id=notification_id)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_notify_get_success_returns_notification_and_marks_read() -> None:
|
||||
"""Happy path: the resolved notification's fields land in evidence."""
|
||||
notification_id = uuid4()
|
||||
from_agent = uuid4()
|
||||
n = MagicMock()
|
||||
n.id = notification_id
|
||||
n.type = "alert"
|
||||
n.priority = "normal"
|
||||
n.subject = "subject line"
|
||||
n.body = "body text"
|
||||
n.requires_ack = False
|
||||
n.from_agent = from_agent
|
||||
|
||||
notif_delivery = AsyncMock()
|
||||
notif_delivery.get_for_recipient_and_mark_read.return_value = n
|
||||
deps = _make_deps(notification_delivery=notif_delivery)
|
||||
ca = ContentActions(deps)
|
||||
|
||||
env = await ca.notify_get(agent_id=uuid4(), notification_id=notification_id)
|
||||
body = env.as_dict()
|
||||
|
||||
assert body["error"] is None
|
||||
assert body["evidence"]["id"] == str(notification_id)
|
||||
assert body["evidence"]["subject"] == "subject line"
|
||||
assert body["evidence"]["from_agent"] == str(from_agent)
|
||||
notif_delivery.get_for_recipient_and_mark_read.assert_awaited_once()
|
||||
@@ -35,6 +35,15 @@ def _make_deps(**overrides: Any) -> ChoreographerDeps:
|
||||
"evidence_repo": AsyncMock(),
|
||||
}
|
||||
base.update(overrides)
|
||||
# Findings-ledger reads (ReviewFindingsRepository.list_for_task) go
|
||||
# through session.execute — an unconfigured AsyncMock's awaited result
|
||||
# is itself an AsyncMock, so a plain sync `.scalars()` call on it leaks
|
||||
# an unawaited coroutine. Empty scalars result (no findings).
|
||||
base["task"].session.execute = AsyncMock(
|
||||
return_value=MagicMock(
|
||||
scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[])))
|
||||
)
|
||||
)
|
||||
repo = base["evidence_repo"]
|
||||
for method in (
|
||||
"list_unread_a2a",
|
||||
|
||||
@@ -89,6 +89,15 @@ def _resubmit_root(
|
||||
task_svc.session.begin_nested = MagicMock(
|
||||
return_value=MagicMock(__aenter__=AsyncMock(), __aexit__=AsyncMock())
|
||||
)
|
||||
# Findings-ledger reads (ReviewFindingsRepository.list_for_task) go
|
||||
# through session.execute — an unconfigured AsyncMock's awaited result
|
||||
# is itself an AsyncMock, so a plain sync `.scalars()` call on it leaks
|
||||
# an unawaited coroutine. Empty scalars result (no findings).
|
||||
task_svc.session.execute = AsyncMock(
|
||||
return_value=MagicMock(
|
||||
scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[])))
|
||||
)
|
||||
)
|
||||
c = Choreographer(_make_deps(task=task_svc, git=AsyncMock()))
|
||||
# Real _project_slug_for would walk a mock session into a MagicMock slug; the
|
||||
# gate under test needs a real string slug + a controllable head SHA. Alias to
|
||||
|
||||
@@ -72,6 +72,15 @@ def _resubmit_cell(
|
||||
task_svc.session.begin_nested = MagicMock(
|
||||
return_value=MagicMock(__aenter__=AsyncMock(), __aexit__=AsyncMock())
|
||||
)
|
||||
# Findings-ledger reads (ReviewFindingsRepository.list_for_task) go
|
||||
# through session.execute — an unconfigured AsyncMock's awaited result
|
||||
# is itself an AsyncMock, so a plain sync `.scalars()` call on it leaks
|
||||
# an unawaited coroutine. Empty scalars result (no findings).
|
||||
task_svc.session.execute = AsyncMock(
|
||||
return_value=MagicMock(
|
||||
scalars=MagicMock(return_value=MagicMock(all=MagicMock(return_value=[])))
|
||||
)
|
||||
)
|
||||
c = Choreographer(_make_deps(task=task_svc, git=AsyncMock()))
|
||||
cc: Any = c
|
||||
cc._project_slug_for = AsyncMock(return_value="proj-slug")
|
||||
|
||||
Reference in New Issue
Block a user