Skip to content

Commit fd86688

Browse files
Sbussisoclaude
andcommitted
fix(cleanup): rewrite log-cleanup UNION as function-form for SQLAlchemy 2.x
Sentry has been firing this nightly for weeks (alert OPENSENTRY-COMMAND-1, surfaced in this morning's email digest): AttributeError: 'CompoundSelect' object has no attribute 'union' File "app/main.py", line 244, in _log_cleanup_loop .union(select([AuditLog.org_id]).distinct()) Root cause: the cleanup loop in ``app.main._log_cleanup_loop`` chained ``.union(...)`` calls across the five log tables to collect distinct org_ids: select(StreamAccessLog.org_id).distinct() .union(select(McpActivityLog.org_id).distinct()) .union(select(AuditLog.org_id).distinct()) # <- raises here ... In SQLAlchemy 2.x (``pyproject.toml`` pins ``>=2.0.48``) the FIRST ``.union()`` returns a ``CompoundSelect``, which doesn't expose a ``.union()`` method — only ``Select`` does. The 1.x-era pattern of chaining unions was deprecated in 1.4 and removed in 2.0. The chained form had been silently broken since Tigris was removed and this loop landed; the cleanup body sat inside a ``try/except`` that logged "Cleanup failed" and moved on, so per-org log retention (Free 30d / Pro 90d / Pro Plus 365d) wasn't actually getting applied — the backend's audit / motion / notification / MCP-activity / stream-access log tables were growing unbounded. The fix uses the function form ``union(a, b, c, ...)`` which is the SQLAlchemy 2.x-compatible composition (and also reads more cleanly than a chain). The ``union`` symbol was already imported at line 301, so the change is a one-block rewrite. Tests ----- ``tests/test_log_cleanup_union.py`` (3 cases): - ``test_union_query_executes_without_attributeerror`` — pins the failure mode by building + executing the query against an empty DB. If a future refactor reintroduces the chained form, this fails at execute time, not silently inside the loop's try/except. - ``test_union_query_returns_distinct_org_ids_across_log_tables`` seeds rows for two orgs across multiple log tables and asserts the deduped set comes back as expected (smoke for the cleanup's first pass). - ``test_union_query_handles_empty_tables`` covers fresh-install day-one. Suite: 235 → 238 passing. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent 6fb5fcb commit fd86688

2 files changed

Lines changed: 145 additions & 5 deletions

File tree

backend/app/main.py

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -298,14 +298,24 @@ async def _log_cleanup_loop():
298298
now = datetime.now(tz=timezone.utc).replace(tzinfo=None)
299299
# One query collects every org_id we're holding logs for.
300300
# UNION across the log tables — small set, runs once a day.
301+
#
302+
# The function form `union(a, b, c, ...)` is the SQLAlchemy
303+
# 2.x-compatible way to compose this. The chained form
304+
# `select(...).union(...).union(...)` works on the first
305+
# call (returns a CompoundSelect) but the SECOND `.union()`
306+
# raises ``AttributeError: 'CompoundSelect' object has no
307+
# attribute 'union'`` — Sentry caught this firing nightly
308+
# in production (alert OPENSENTRY-COMMAND-1, Apr 2026).
301309
from sqlalchemy import union, select
302310
org_rows = (
303311
db.execute(
304-
select(StreamAccessLog.org_id).distinct()
305-
.union(select(McpActivityLog.org_id).distinct())
306-
.union(select(AuditLog.org_id).distinct())
307-
.union(select(MotionEvent.org_id).distinct())
308-
.union(select(Notification.org_id).distinct())
312+
union(
313+
select(StreamAccessLog.org_id).distinct(),
314+
select(McpActivityLog.org_id).distinct(),
315+
select(AuditLog.org_id).distinct(),
316+
select(MotionEvent.org_id).distinct(),
317+
select(Notification.org_id).distinct(),
318+
)
309319
).all()
310320
)
311321
org_ids = {row[0] for row in org_rows if row[0]}
Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,130 @@
1+
"""Regression tests for the log-cleanup UNION query in ``app.main._log_cleanup_loop``.
2+
3+
The cleanup loop UNIONs distinct org_ids across the five log tables once
4+
per day so it can iterate orgs and apply each org's tier-specific
5+
retention. The original implementation chained ``.union()`` calls on the
6+
``select(...).distinct()`` results — which works on a ``Select`` but
7+
NOT on a ``CompoundSelect`` (the result of the previous ``.union()``).
8+
9+
In SQLAlchemy 2.x that produces ``AttributeError: 'CompoundSelect'
10+
object has no attribute 'union'`` and the cleanup silently fails inside
11+
its ``try/except``. Sentry caught it firing nightly in production
12+
(alert OPENSENTRY-COMMAND-1, Apr 2026); these tests pin the
13+
SQLAlchemy-2.x-correct ``union(a, b, c, ...)`` function form so a future
14+
refactor can't quietly reintroduce the same chain pattern.
15+
16+
We don't run the entire ``_log_cleanup_loop`` (async, infinite, sleeps
17+
24h) — we exercise just the query construction + execution against the
18+
same five models, which is where the bug actually was.
19+
"""
20+
21+
from datetime import datetime, timezone
22+
23+
from sqlalchemy import select, union
24+
25+
from app.core.database import SessionLocal
26+
from app.models.models import (
27+
AuditLog,
28+
McpActivityLog,
29+
MotionEvent,
30+
Notification,
31+
StreamAccessLog,
32+
)
33+
34+
35+
def _utcnow_naive() -> datetime:
36+
"""Match the pattern the cleanup loop uses for log timestamps."""
37+
return datetime.now(tz=timezone.utc).replace(tzinfo=None)
38+
39+
40+
def _build_org_id_union():
41+
"""Mirror exactly the UNION expression in app.main._log_cleanup_loop.
42+
43+
Kept as a helper so the test asserts on the same shape of query the
44+
loop builds — drift between this and main.py would still slip
45+
through, but at least the SQLAlchemy compatibility surface is shared.
46+
"""
47+
return union(
48+
select(StreamAccessLog.org_id).distinct(),
49+
select(McpActivityLog.org_id).distinct(),
50+
select(AuditLog.org_id).distinct(),
51+
select(MotionEvent.org_id).distinct(),
52+
select(Notification.org_id).distinct(),
53+
)
54+
55+
56+
# ── Tests ────────────────────────────────────────────────────────────
57+
58+
59+
def test_union_query_executes_without_attributeerror():
60+
"""The exact failure mode in production: building + executing the union
61+
must not raise ``AttributeError: 'CompoundSelect' object has no
62+
attribute 'union'``. If a future change reintroduces the chained
63+
``.union(...).union(...)`` form, this test fails at construction-or-
64+
execute time, not silently inside the cleanup loop's try/except.
65+
"""
66+
db = SessionLocal()
67+
try:
68+
# No data needed — the query itself was unbuildable in the broken
69+
# version. We just need the SQLAlchemy compiler to walk the tree.
70+
result = db.execute(_build_org_id_union()).all()
71+
assert isinstance(result, list)
72+
finally:
73+
db.close()
74+
75+
76+
def test_union_query_returns_distinct_org_ids_across_log_tables():
77+
"""End-to-end: insert rows for two orgs across multiple log tables,
78+
run the union query, and confirm the deduped set comes back. This
79+
pins the *behaviour* of the cleanup's first pass, not just the
80+
SQL-API compatibility.
81+
"""
82+
db = SessionLocal()
83+
try:
84+
now = _utcnow_naive()
85+
86+
# Org A appears in three tables; Org B appears in one. The union
87+
# should yield {A, B} — duplicates collapsed by the inner DISTINCTs
88+
# and the UNION (which dedupes by default in SQLAlchemy).
89+
db.add(StreamAccessLog(
90+
org_id="org_A", user_id="u1", camera_id="cam_1", node_id="node_1",
91+
ip_address="127.0.0.1", user_agent="t", accessed_at=now,
92+
))
93+
db.add(StreamAccessLog(
94+
org_id="org_A", user_id="u2", camera_id="cam_1", node_id="node_1",
95+
ip_address="127.0.0.1", user_agent="t", accessed_at=now,
96+
))
97+
db.add(McpActivityLog(
98+
org_id="org_A", tool_name="list_cameras", key_name="k",
99+
status="ok", duration_ms=1, timestamp=now,
100+
))
101+
db.add(AuditLog(
102+
org_id="org_A", event="evt", user_id="u1",
103+
ip_address="127.0.0.1", details="{}", timestamp=now,
104+
))
105+
db.add(Notification(
106+
org_id="org_B", kind="motion_detected", audience="all",
107+
title="t", body="b", severity="info", created_at=now,
108+
))
109+
db.commit()
110+
111+
rows = db.execute(_build_org_id_union()).all()
112+
org_ids = {row[0] for row in rows if row[0]}
113+
114+
assert org_ids == {"org_A", "org_B"}
115+
finally:
116+
db.close()
117+
118+
119+
def test_union_query_handles_empty_tables():
120+
"""Fresh deployment with zero log rows: the union must still execute
121+
cleanly and return an empty result. Without this, a brand-new node
122+
on day one would hit the cleanup loop and crash on its first
123+
nightly tick (or, with the original bug, fail silently).
124+
"""
125+
db = SessionLocal()
126+
try:
127+
rows = db.execute(_build_org_id_union()).all()
128+
assert rows == []
129+
finally:
130+
db.close()

0 commit comments

Comments
 (0)