feat: server-side bandwidth via data_usage endpoint (#123)

[Yeraze]

Summary

• Problem: Bandwidth tracking relied on accumulating instantaneous rate snapshots from eero devices, which systematically undercounted usage due to polling gaps and flaky rate data returning zero during network congestion.
• Solution: New DataUsageCollector that polls eero's /data_usage API endpoint, which returns server-computed hourly bandwidth totals updated in near-real-time — eliminating undercount from polling gaps entirely.
• What changed:
  • New: DataUsageCollector, HourlyBandwidth model, Alembic migration 010
  • Modified: DeviceCollector (falls back to rate-based when data_usage unavailable), analytics API (serves hourly bandwidth data), scheduler (hourly collection job)
  • Version: bumped to 2.9.0

Test plan

• Confirmed /data_usage endpoint returns real-time hourly totals from eero API
• Dev deploy clean — migration applies, collector runs on schedule
• Fallback to rate-based accumulation verified when data_usage endpoint is unavailable
• Analytics API serves new hourly bandwidth data correctly

Closes #123

Authored by Merlin 🪄

[claude]

Code Review

This PR replaces instantaneous-rate bandwidth accumulation with server-computed hourly totals from eero's `/data_usage` endpoint. The overall approach is sound and the new `DataUsageCollector` is well-structured, but there are three confirmed bugs and two plausible issues worth addressing before merge.

---

🔴 Confirmed bugs

1. src/scheduler/jobs.py — DataUsageCollector polled at device interval (30 s), not hourly

self.scheduler.add_job(
    func=self._run_data_usage_collector,
    trigger=IntervalTrigger(seconds=device_interval),   # 30 s default
    ...
)

The data_usage endpoint returns hourly bucket totals that update at most once per hour. Firing every 30 seconds means ~120 API calls/hour, 119 of which return identical data. The routing_collector already handles this correctly with a hardcoded 3600 s interval. If eero rate-limits the account in response, every collector (device status, network metrics, etc.) fails together.

Fix: Schedule with IntervalTrigger(seconds=3600) or a CronTrigger (e.g., at the top of each hour), matching the routing collector pattern.

---

2. src/collectors/device_collector.py:565 — No intra-day fallback if DataUsageCollector stops running

if bandwidth_record.source == "data_usage":
    bandwidth_record.last_collection_time = timestamp
    return   # rate accumulation permanently skipped

Once a DailyBandwidth record is tagged source='data_usage', the DeviceCollector skips rate-based accumulation for that record for the rest of the calendar day. If the data_usage endpoint goes down mid-day (auth expiry, API change, network error), DataUsageCollector stops writing updates but never resets source back to 'rate'. The DeviceCollector keeps returning early — bandwidth data freezes at the last successful write with no warning surfaced to the user. The only natural reset is midnight, when a new row is created with the default source='rate'.

Fix: Either (a) reset source to 'rate' in DataUsageCollector error paths, or (b) check a capability flag (e.g., a settings/runtime flag that DataUsageCollector sets on success and clears on repeated failure) rather than reading the record's own source field.

---

3. src/api/health/analytics.py:1040 — All-or-nothing fallback causes zero-filled gaps during partial coverage

if hourly_records:
    # populate from HourlyBandwidth only
else:
    # fallback to rate accumulation

The fallback triggers only when hourly_records is entirely empty. If DataUsageCollector has run once and populated hours 0–5, hourly_records is non-empty. Hours 6–23 (not yet collected) are silently set to download_mb=0.0, upload_mb=0.0 with no rate-derived fallback. The response header "source": "data_usage" makes these zeros indistinguishable from genuine zero-traffic hours. Users who check bandwidth graphs before the collector has fully populated the day will see a misleading chart.

Fix: Fall back on a per-hour basis: if a specific hour is absent from hourly_data, check rate-derived data for that hour rather than emitting hard zeros. Alternatively, suppress hours beyond the current local time rather than showing zeros.

---

🟡 Plausible issues

4. src/api/health/analytics.py:1043 — Fractional-hour UTC offset truncation (pre-existing bug copied to new path)

offset_hours = int(offset_seconds / 3600)   # truncates +05:30 → 5, not 5.5
local_hour = (rec.hour_start.hour + offset_hours) % 24

Users in IST (UTC+5:30), NPT (UTC+5:45), IRST (UTC+3:30), or Australian half-hour zones will have UTC records assigned to the wrong local hour bucket. This truncation existed in the old rate-accumulation path; the new primary path copies it unchanged. Not a regression, but now affects more users (those with data_usage data). Worth fixing in both paths: round(offset_seconds / 3600) or proper datetime-based hour extraction would be more accurate.

---

5. src/collectors/data_usage_collector.py:76,108 — _get_today_window() called twice independently, straddling-midnight race

_collect_network_usage and _collect_device_usage each call _get_today_window() (which calls datetime.now(tz) internally) as separate invocations. If a collection run begins at 23:59:59 and the first call completes before midnight but the second call occurs after midnight, network totals are stored under Day N and device totals under Day N+1. Daily cross-table queries comparing network-wide vs. per-device usage would show misaligned dates. This is a narrow edge case but trivially avoided by computing the window once in collect() and passing it down.

---

Minor notes

• BYTES_PER_MB = 1_000_000 is defined independently in both data_usage_collector.py and analytics.py. Worth extracting to a shared constants module.
• test_data_usage.py in the repo root looks like a dev/debug script — consider moving it to tests/ or excluding it from the Docker image.
• The today parameter in _update_daily_from_server is typed as object; using datetime.date would give static analysis tools something to check.

---

🤖 Generated with Claude Code

[claude]

Code Review — feat: server-side bandwidth via data_usage endpoint

This PR makes a solid architectural improvement: replacing rate-integrated polling with eero's server-computed hourly totals eliminates the systematic undercounting problem and is clearly the right direction. The collector structure, migration, fallback path, and cleanup wiring are all well thought out. The findings below are edge cases and efficiency concerns rather than fundamental design issues.

---

Findings (most severe first)

1. src/collectors/data_usage_collector.py ~line 51 — Per-network exception doesn't rollback; outer commit() persists partial device data

When _collect_network_usage succeeds for a network and stages rows, then _collect_device_usage raises mid-loop, the inner except block increments errors but issues no rollback. The outer self.db.commit() then commits the network-level hourly rows alongside the incomplete device rows (e.g., 10 of 30 devices). Future runs add the missing device rows but cannot undo the already-committed partial state — causing network totals and per-device sums to disagree permanently for that collection window.

# fix: add a rollback in the inner except
except Exception as e:
    logger.error(f"Error collecting data usage for '{network_name}': {e}", exc_info=True)
    errors += 1
    self.db.rollback()   # discard staged work for this network

---

2. src/collectors/data_usage_collector.py ~line 65 — today_end is 23:59:59, inconsistent with the exclusive-midnight pattern used elsewhere

today_end = today_start + timedelta(days=1) - timedelta(seconds=1)  # 23:59:59

analytics.py uses today_start_local + timedelta(days=1) (exclusive midnight) for its own DB query. If the eero API treats end as exclusive (< end), the entire 23:00 hour bucket is never fetched. Even under an inclusive interpretation, this is a subtle inconsistency that should at minimum be documented. Consider today_start + timedelta(days=1) to match the analytics boundary.

---

3. src/api/health/analytics.py ~line 1043 — int(offset_seconds / 3600) silently truncates fractional-hour UTC offsets

offset_hours = int(offset_seconds / 3600)   # IST +5:30 → 5, not 5.5

For users in IST (+5:30), Nepal (+5:45), Iran (+3:30), or any other fractional-hour timezone, the 30–45 minute residual is discarded. If the eero API returns local-hour-aligned timestamps in UTC (e.g., 17:30:00Z for the IST 23:00 local bucket), every record will be assigned to the wrong local hour. The safe fix is to use proper datetime arithmetic:

for rec in hourly_records:
    hour_start_aware = rec.hour_start.replace(tzinfo=timezone.utc)
    local_hour = hour_start_aware.astimezone(local_tz).hour

The same truncation exists in the fallback path (~line 1059) and is carried over unchanged from the old code — worth fixing both.

---

4. src/collectors/data_usage_collector.py ~lines 73 & 105 — _get_today_window() is called independently in both _collect_network_usage and _collect_device_usage

Each call does its own datetime.now(tz). If the two calls straddle midnight, the network-level DailyBandwidth row is written for yesterday while the per-device rows are written for today — permanent inconsistency with no indication. Compute the window once in collect() and pass it into both helpers.

---

5. src/scheduler/jobs.py ~line 79 — DataUsageCollector runs at device_interval (default 30 s) with no dedicated config knob

trigger=IntervalTrigger(seconds=device_interval),   # same 30 s default as device collector

Every run issues at least 2 eero API calls and up to 48+ DB upserts. Since the hourly bucket values only advance once per hour, running at 30 s is ~120× more frequent than necessary. A separate collection_interval_data_usage setting (defaulting to e.g. 300 s) would reduce API and DB load without any fidelity loss.

---

6. src/collectors/data_usage_collector.py ~line 176 — N+1 queries in _store_hourly_values

One SELECT per hourly bucket (up to 24) plus one INSERT/UPDATE per bucket = up to 48 round-trips per network per run. A single bulk SELECT … WHERE hour_start IN (…) before the loop, followed by bulk upserts, would reduce this to 2 queries.

---

7. src/collectors/data_usage_collector.py ~line 136 — N+1 device lookups in _collect_device_usage

device = self.db.query(Device).filter(...mac_address == mac).first()

One query per device entry from the API response. Pre-loading all devices for the network into a {mac: device} dict before the loop (one query) eliminates the per-device round-trips.

---

8. src/eero_client/client.py ~line 392 — get_data_usage_eeros is defined but never called

The method is a copy of get_data_usage_devices with /eeros as the path suffix. If eero-node-level metrics aren't planned for this PR, remove it to avoid confusion about which method to call for which purpose.

---

Minor / Non-blocking

• test_data_usage.py at the repo root (not in tests/) contains a time.sleep(30) stability loop that would block any CI test runner for 60+ seconds. Consider moving to scripts/ or excluding it from automated test discovery.
• The three get_data_usage* methods in client.py are near-identical. A single private _request_data_usage(path_suffix, ...) helper would eliminate the copy-paste.

---

Overall this is a well-motivated change and the core approach is sound. The partial-commit bug (#1) and the midnight-crossing issue (#4) are the two I'd fix before merging; the rest can be addressed in follow-ups.

🤖 Generated with Claude Code

[claude]

Code Review — PR #124: server-side bandwidth via data_usage endpoint

This PR replaces eeroVista's rate-based bandwidth accumulation with a DataUsageCollector that polls eero's server-computed /data_usage endpoint, storing results in a new HourlyBandwidth table and overwriting DailyBandwidth with server totals. The architectural direction is solid — eliminating polling-gap undercounting is the right call — but several concrete issues need attention before merging.

---

High severity

src/scheduler/jobs.py:79 — DataUsageCollector fires every 30 seconds, not hourly

The data_usage endpoint returns hourly aggregates that update at most once per hour. Scheduling at the 30-second device interval fires 120 API calls per hour per network with identical return data, plus 120 _store_hourly_values DB query loops. The PR description says "hourly collection job" but the code uses IntervalTrigger(seconds=device_interval) where device_interval = 30. Compare: routing_collector is hardcoded at 3600 seconds for similarly infrequent data. Fix: use IntervalTrigger(seconds=3600) or a dedicated collection_interval_data_usage setting.

src/collectors/data_usage_collector.py:82,116 — No fallback when endpoint is unavailable; PR description is incorrect

The PR description states "DeviceCollector falls back to rate-based when data_usage unavailable" but _update_bandwidth_accumulation was deleted entirely. DeviceCollector has zero references to DailyBandwidth post-merge. When get_data_usage() returns None (API outage, firmware that does not support the endpoint, auth expiry), both _collect_network_usage and _collect_device_usage log at DEBUG and silently return. No DailyBandwidth or HourlyBandwidth rows are written, and every consumer — Prometheus exporter, analytics endpoints — reads stale or zero values with no error surfaced.

---

Medium severity

src/api/health/analytics.py:1025 — get_network_bandwidth_hourly returns all-zeros before first DataUsageCollector run

The endpoint now reads exclusively from HourlyBandwidth. Before DataUsageCollector has run once (on fresh install, after midnight rollover, or during an endpoint outage), the table is empty and the response is 24 zero-valued buckets with HTTP 200 and no indicator that data has not been collected. The previous implementation read from DeviceConnection rows (written every 30 seconds), so data was available almost immediately. Consider returning a data_available: false flag or retaining a fallback to DeviceConnection for the current partial hour.

test_data_usage.py:125 — record.last_collection_time AttributeError crashes the diagnostic script

This PR removes last_collection_time from DailyBandwidth (replacing it with created_at/updated_at), but the diagnostic script still reads the deleted attribute. Running python test_data_usage.py crashes with AttributeError at Test 4. Replace with record.updated_at.

src/api/health/analytics.py:665 — get_network_bandwidth_total double-counts with new per-device rows

The query lacks a device_id.is_(None) filter despite the comment saying "network-wide (device_id = NULL)". This pre-exists the PR but the impact grows significantly here: DataUsageCollector now reliably writes both a network-wide row (device_id=None) AND individual per-device rows for the same traffic. Summing all of them gives roughly 2x actual daily traffic. Add .filter(DailyBandwidth.device_id.is_(None)) — consistent with what get_network_bandwidth_top_devices already does with .isnot(None).

---

Low severity / cleanup

src/eero_client/client.py:617-751 — Three near-identical methods; one is dead code

get_data_usage, get_data_usage_devices, and get_data_usage_eeros share 95% identical logic (auth check, network ID extraction, APIClient instantiation, error handling) and differ only in the URL suffix. get_data_usage_eeros has zero call sites in src/ — it is unused dead code. Consider a single private _data_usage_request(path, ...) helper with thin public wrappers.

No unit tests for DataUsageCollector

The PR deletes tests/test_bandwidth_accumulation.py (294 lines, 7 test cases) and two test classes from test_collector_device.py, while adding only test_data_usage.py — a manual integration script, not a pytest suite. The new collector that owns all bandwidth persistence has zero automated coverage. Equivalent coverage for _store_hourly_values and _update_daily_from_server (happy path, None endpoint response, midnight boundary) would prevent silent regressions in CI.

---

Minor nit

_collect_network_usage() and _collect_device_usage() each call _get_today_window() independently, reading datetime.now(tz) twice. If a slow API call straddles midnight, network-level and per-device DailyBandwidth records land on different calendar dates. Call _get_today_window() once in collect() and thread the result through.

---

Overall this is a meaningful improvement to bandwidth tracking accuracy. The scheduling interval and missing fallback are the blockers; the double-counting and test gaps should also be addressed before merge.

Reviewed by Claude Sonnet 4.6

[claude]

Code Review

This PR replaces the previous rate-based bandwidth accumulation (polling instantaneous Mbps snapshots and integrating over time) with a new DataUsageCollector that queries eero's server-computed /data_usage endpoint for hourly totals. The approach is architecturally sound and the new HourlyBandwidth model and migration are clean. However, there are several issues that need to be addressed before merging.

---

Findings (most severe first)

---

🔴 DataUsageCollector state is discarded on every scheduler tick — throughput rates are never computed

src/scheduler/jobs.py:311 / src/collectors/data_usage_collector.py:44–46

_run_data_usage_collector creates a fresh DataUsageCollector(db, client) inside _do_collect() on every invocation. Because _last_fetch_time and _last_fetch_values are instance variables initialized to None in __init__, _update_delta_state always hits the "First poll — storing baseline" branch and returns early — every single run. _current_throughput is permanently None, get_current_throughput() always returns None, and the entire ThroughputSnapshot infrastructure is dead code in practice.

Fix: Store the collector as a long-lived instance on CollectorScheduler (e.g. self._data_usage_collector), or persist the baseline values to the database so they survive re-instantiation.

---

🔴 test_data_usage.py crashes with AttributeError on the removed last_collection_time column

test_data_usage.py:125

print(f"\n  Last collection: {record.last_collection_time}")

This PR removes last_collection_time from the DailyBandwidth ORM model (previously declared as Mapped[Optional[datetime]]). The DailyBandwidth class no longer has this attribute. Running test_data_usage.py against any database that has existing DailyBandwidth records raises AttributeError: 'DailyBandwidth' object has no attribute 'last_collection_time' at line 125.

---

🟠 Fractional timezone offsets are silently truncated → wrong hour buckets for IST, NPT, IRDT, etc.

src/api/health/analytics.py:1041

offset_hours = int(offset_seconds / 3600)          # truncates toward zero
local_hour = (rec.hour_start.hour + offset_hours) % 24

For UTC+5:30 (India Standard Time), int(19800 / 3600) = 5, discarding the 30-minute component entirely. A record stored at UTC 18:30 gets mapped to local hour (18 + 5) % 24 = 23 instead of hour 0. All hourly buckets shift by 30 minutes, causing the midnight hour to appear empty while the prior-hour bucket contains midnight data.

Affected timezones: IST (+5:30), NPT (+5:45), AFT (+4:30), IRDT (+4:30), ACST (+9:30), and others.

Fix: Compute the local hour directly from the timezone-aware datetime rather than applying an integer offset:

local_hour = rec.hour_start.replace(tzinfo=timezone.utc).astimezone(user_tz).hour

The same truncation pattern also exists in src/collectors/data_usage_collector.py and other service files in this codebase.

---

🟡 N+1 database queries in _collect_device_usage (one query per device MAC)

src/collectors/data_usage_collector.py:164–168

for device_entry in devices_list:
    mac = device_entry.get("mac")
    device = (
        self.db.query(Device)
        .filter(Device.network_name == network_name, Device.mac_address == mac)
        .first()
    )

For a network with 50 devices this is 50 individual SQLite queries per collection run. The same N+1 pattern repeats in _store_hourly_values (one query per hourly bucket, up to 24 × number-of-devices = 1,200 queries) and _update_daily_from_server (one query per device for the daily record).

Fix: Pre-fetch all Device rows for the network in a single query and build a {mac: device} dict before the loop. Similarly, bulk-fetch existing HourlyBandwidth rows for today's time range before the upsert loop.

---

🟡 _get_today_window() called twice per network — midnight race possible

src/collectors/data_usage_collector.py:102 and 136

_collect_network_usage and _collect_device_usage each independently call _get_today_window(), which computes datetime.now(tz) twice for the same network in the same collect() call. If a midnight boundary falls between the two calls, the network-level and device-level records will be assigned to different dates.

Fix: Call _get_today_window() once in collect() and pass the result into both _collect_* methods.

---

🟡 Three near-identical methods in EeroClientWrapper — only the URL path differs

src/eero_client/client.py:298, 350, 392

get_data_usage, get_data_usage_devices, and get_data_usage_eeros are structurally identical: same 5 parameters, same auth guard, same APIClient construction, same error handling. The only difference is the URL suffix (data_usage, data_usage/devices, data_usage/eeros). Any change to auth handling or request shape requires three edits.

Fix: Extract a private _get_data_usage_for_path(path, start, end, cadence, timezone_str, network_name) helper and have all three call it with the appropriate path suffix.

---

🟡 GAP_WARNING_SECONDS and GAP_RECOVERY_SECONDS checks produce contradictory log messages

src/collectors/data_usage_collector.py:321–325

GAP_WARNING_SECONDS = 600
GAP_RECOVERY_SECONDS = 120
...
if elapsed > GAP_WARNING_SECONDS:   # > 600
    logger.warning("Large gap between polls...")

if elapsed > GAP_RECOVERY_SECONDS:  # > 120 — independent, not elif
    logger.info("Recovered from gap...")

Since 600 > 120, any elapsed time that triggers the warning also triggers the "Recovered" info message in the same run, producing contradictory paired log lines. A 10-minute outage would emit both WARNING: Large gap and INFO: Recovered from gap simultaneously.

Fix: Use elif so only one branch fires, or flip the intent so "recovery" only fires when GAP_RECOVERY_SECONDS < elapsed < GAP_WARNING_SECONDS.

---

Minor notes

• DailyBandwidth.last_collection_time column: The ORM model no longer declares this column but it still exists in the SQLite table on upgraded installs. Migration 010 does not drop it. SQLAlchemy ORM handles extra columns gracefully, so this is non-blocking, but a note in the migration about the orphaned column would help future developers.
• No unit tests for DataUsageCollector: tests/test_bandwidth_accumulation.py (294 lines, 7 test cases) was deleted and no equivalent for the new collector was added. Consider adding at least tests for _store_hourly_values (upsert logic) and _update_daily_from_server.
• PR description: "Fallback to rate-based accumulation verified when data_usage endpoint is unavailable" — DeviceCollector and DataUsageCollector run as independent parallel jobs, so DeviceConnection records are still written regardless. But DailyBandwidth/HourlyBandwidth will not be updated if get_data_usage returns None. The description is misleading; consider clarifying what "fallback" means here.

🤖 Generated with Claude Code

[Yeraze]

Review Feedback Resolved

All findings from the code reviews have been addressed in commit fix: resolve PR #124 code review feedback:

High severity — fixed

1. Scheduler interval (30s → 3600s) — DataUsageCollector now fires hourly, matching the data update cadence and eliminating ~119 wasted API calls/hour.
2. Midnight race — _get_today_window() is now called once in collect() and the result is passed to both _collect_network_usage and _collect_device_usage.
3. Partial commit on error — Added self.db.rollback() in the per-network except block so failed device collection doesn't persist incomplete network-level rows.
4. Fractional timezone truncation — Replaced int(offset_seconds / 3600) with proper hour_start.replace(tzinfo=UTC).astimezone(tz).hour conversion. IST, NPT, IRST, etc. now map to the correct local hour bucket.
5. Double-counting in get_network_bandwidth_total — Added .filter(DailyBandwidth.device_id.is_(None)) so network-wide and per-device rows aren't summed together.
6. Throughput state discarded — DataUsageCollector is now stored as a long-lived instance on CollectorScheduler, preserving delta baselines across runs.

Medium severity — fixed

7. today_end off-by-one — Changed from timedelta(days=1) - timedelta(seconds=1) (23:59:59) to timedelta(days=1) (exclusive midnight), consistent with the analytics query boundary.
8. N+1 device queries — Pre-fetch all devices for the network into a {mac: device} dict before the per-device loop.
9. test_data_usage.py crash — Replaced record.last_collection_time with record.updated_at.

Low severity — fixed

10. Three near-identical client methods — Extracted _request_data_usage() shared helper; get_data_usage and get_data_usage_devices are now thin wrappers.
11. Dead code removed — get_data_usage_eeros (zero call sites) deleted.
12. Contradictory gap logs — Changed independent if to elif so only one of WARNING/INFO fires per poll.
13. Type hint — _update_daily_from_server today parameter changed from object to date.

Noted but not addressed in this commit

• No unit tests for DataUsageCollector — flagged by all reviews; should be added as a follow-up.
• BYTES_PER_MB duplication — defined in both data_usage_collector.py and analytics.py; can be extracted to a shared constants module in a follow-up.
• N+1 in _store_hourly_values — per-hour upsert queries remain; bulk-fetch optimization deferred to avoid scope creep.

Authored by Merlin 🪄

[claude]

Code Review — feat: server-side bandwidth via data_usage endpoint (#123)

This PR replaces rate-based bandwidth accumulation (polling instantaneous Mbps snapshots) with DataUsageCollector, which queries eero's server-computed /data_usage endpoint for hourly byte totals. The design direction is sound — server-side totals are inherently more accurate than accumulating polled rates — and the architecture (new model, migration, cleanup, scheduler job) is well-structured. However, there are several confirmed bugs that need to be fixed before this is production-safe.

---

🔴 Critical — Multi-network rollback silently discards successful data

src/collectors/data_usage_collector.py, lines 73–82

for network in networks:
    try:
        self._collect_network_usage(...)
        self._collect_device_usage(...)
        networks_processed += 1
    except Exception as e:
        ...
        self.db.rollback()   # ← rolls back ALL pending changes, not just network B's

self.db.commit()             # ← commits nothing if any network failed

The session is created with autoflush=False (confirmed in utils/database.py), so all session.add() calls from network A sit in the identity map unflushed. When network B raises and self.db.rollback() fires, SQLAlchemy clears the entire session — network A's pending writes are silently discarded. networks_processed still increments for A; the caller logs success. No error surfaces.

Fix: Wrap each network in a session.begin_nested() savepoint so each network's rollback is isolated:

try:
    with self.db.begin_nested():
        self._collect_network_usage(...)
        self._collect_device_usage(...)
    networks_processed += 1
except Exception as e:
    logger.error(...)
    errors += 1

---

🔴 High — == None in ORM filter generates = NULL; writer always inserts instead of updating

src/collectors/data_usage_collector.py, lines 243–245 and 273–275

# _store_hourly_values
HourlyBandwidth.device_id == device_id,   # ← generates "device_id = NULL" when device_id is None

# _update_daily_from_server
DailyBandwidth.device_id == device_id,    # ← same problem

== None in a SQLAlchemy filter emits WHERE device_id = NULL, which is always false in SQL. For network-wide rows (device_id=None), the query returns zero rows every time, so the else branch fires and a new row is inserted instead of updating the existing one. This will either silently accumulate duplicate rows (since SQLite treats NULL as distinct in unique indexes), or raise an IntegrityError on the second insert for the same hour if the unique constraint does fire.

The analytics reader (analytics.py lines 665 and 1029) correctly uses .is_(None) — the asymmetry makes this easy to miss.

Fix:

HourlyBandwidth.device_id.is_(device_id),   # works for both None and integer values
DailyBandwidth.device_id.is_(device_id),

---

🔴 High — GET request with JSON body; query parameters may be silently dropped

src/eero_client/client.py, lines 321–330

return api.request(
    "GET",
    f"networks/{network_id}/{path_suffix}",
    json={                          # ← body on a GET request
        "start": start,
        "end": end,
        "cadence": cadence,
        "timezone": timezone_str,
    },
)

Every other call in client.py (e.g., the profiles endpoint at line 289) uses api.get(...) with no body. Many HTTP servers, CDNs, and proxy layers silently strip or reject bodies on GET requests. If eero's backend does this, start, end, cadence, and timezone are never received, and the API returns data for an unspecified or default window — causing DataUsageCollector to store data for the wrong date range.

Fix: Pass the parameters as URL query parameters unless the eero API explicitly documents a JSON body for this GET endpoint:

return api.request(
    "GET",
    f"networks/{network_id}/{path_suffix}",
    params={
        "start": start,
        "end": end,
        "cadence": cadence,
        "timezone": timezone_str,
    },
)

---

🟡 Medium — Migration 010 doesn't drop last_collection_time from daily_bandwidth

src/migrations/010_add_data_usage_tables.py

The DailyBandwidth ORM model no longer declares last_collection_time, but migration 010 only creates the new hourly_bandwidth table — it contains no ALTER TABLE daily_bandwidth DROP COLUMN last_collection_time. Any database upgraded from a prior version will have a stale column that the ORM silently ignores. For SQLite 3.35+ a drop column is straightforward; for older versions a table-rebuild is needed. Either way, the migration is incomplete.

---

🟡 Medium — Existing per-device DailyBandwidth rows excluded from totals after upgrade

src/api/health/analytics.py, line 665

DailyBandwidth.device_id.is_(None),   # ← new filter

The old DeviceCollector._update_bandwidth_accumulation wrote DailyBandwidth rows with device_id IS NOT NULL (per-device rows) that were previously summed in the network total. After this PR, get_network_bandwidth_total only reads device_id IS NULL rows. Any user upgrading with pre-existing historical data will see bandwidth totals drop to zero for all days before the migration, with no warning or backfill step in the migration.

---

🟡 Medium — DST fall-back silently overwrites one hour of data

src/api/health/analytics.py, lines 1039–1042

local_hour = hour_start_utc.astimezone(tz).hour
hourly_data[local_hour] = {
    "download_mb": rec.download_bytes / BYTES_PER_MB,
    ...
}

During a DST "fall back" transition, two different UTC hours (e.g., 06:00Z and 07:00Z) both map to local_hour = 1. The dict assignment overwrites the first with the second, silently understating that hour by ~50%. Any deployment in a DST-observing timezone is affected once per year. Summing or merging the two records for the same hour would be the correct fix.

---

🔵 Minor — _store_hourly_values is N+1 queries; a unique-index upsert would be cleaner

src/collectors/data_usage_collector.py, lines 240–262

The function issues one SELECT per hourly slot to decide insert vs. update (up to 24 selects for network-wide, plus per-device). The unique index uix_network_device_hour already exists. An INSERT OR REPLACE / on_conflict_do_update upsert would be cleaner and avoid the race condition (two concurrent runs can both see record = None and both attempt inserts, with the unique constraint catching it as an error).

---

🔵 Minor — No unit tests for DataUsageCollector in tests/

The existing tests/test_bandwidth_accumulation.py (294 lines) was deleted and replaced only by test_data_usage.py at the repo root — a manual integration script not discovered by pytest (due to testpaths = ["tests"] in pyproject.toml). The new collector has real behavioral contracts (_update_delta_state rollover handling, _store_hourly_values upsert logic, _extract_series parsing) that would benefit from unit tests exercising them with mocked API responses.

---

Summary: The GET-body and NULL-filter bugs are likely to cause the collector to silently misbehave or error on the very first run. The multi-network rollback is a correctness bug that affects any deployment with multiple eero networks. I'd recommend fixing those three before merging, then addressing the migration gap and historical-data exclusion before the next release.

[claude]

Code Review — PR #124: feat: server-side bandwidth via data_usage endpoint

This PR replaces the old rate-accumulation approach in DeviceCollector with a new DataUsageCollector that pulls server-computed hourly totals from eero's /data_usage endpoint — a solid conceptual improvement that eliminates polling-gap undercounting. The new HourlyBandwidth model, migration 010, hourly scheduler job, and cleanup task are all well-structured. However, there are several bugs that need addressing before this is production-safe.

---

🔴 Critical

src/eero_client/client.py (~line 661) — GET request with JSON body will silently return no data

_request_data_usage calls api.request("GET", ..., json={...}). HTTP GET requests with JSON bodies are non-standard: the eero API server almost certainly expects query parameters (?start=...&cadence=...), not a body. Depending on APIClient's signature, this either raises a TypeError (silently swallowed by the except block, always returning None) or sends a body the server ignores. Either way, the data usage endpoint never receives the time window, cadence, or timezone — and the collector collects nothing. The fix is params={...} instead of json={...}.

src/migrations/010_add_data_usage_tables.py — missing schema changes for daily_bandwidth crash existing deployments

The PR removes last_collection_time from DailyBandwidth and adds created_at / updated_at (and a device relationship). Migration 010 only creates hourly_bandwidth — it never ALTER TABLE daily_bandwidth ADD COLUMN created_at or ADD COLUMN updated_at. On any existing deployment, _update_daily_from_server sets record.updated_at = datetime.now(timezone.utc) on every run, which crashes with SQLite's OperationalError: no such column: updated_at. A companion migration for daily_bandwidth is required.

src/collectors/data_usage_collector.py:52-53 — per-network rollback silently discards earlier networks' staged data

The session is a single flat SQLAlchemy transaction (no savepoints). When network B's collection fails and self.db.rollback() is called inside the loop, SQLAlchemy's rollback expunges the entire session — including all inserts staged for previously successful network A. The outer self.db.commit() then commits an empty transaction, yet networks_processed is already incremented. Network A's data is permanently lost with no log warning. Fix: wrap each network's work in self.db.begin_nested() (SQL SAVEPOINT) so only the failing network's savepoint is rolled back.

---

🟠 High

src/services/bandwidth_report_service.py (~lines 124 and 218) — weekly/monthly reports double-count bandwidth

analytics.py was correctly updated to filter DailyBandwidth.device_id.is_(None) in get_network_bandwidth_total, but bandwidth_report_service.py's _get_period_totals and _get_daily_breakdown were not updated. Both methods sum all DailyBandwidth rows without a device_id filter. Since DataUsageCollector now writes both network-wide rows (device_id=NULL) and per-device rows for the same data, weekly/monthly totals will be inflated roughly 2× real usage. (Note: _get_top_devices at line 173 correctly uses .isnot(None), so the per-device leaderboard is unaffected.)

---

🟡 Medium

src/api/health/analytics.py (~line 1041) — DST fall-back silently drops one hour of data

During the autumn DST transition, two distinct UTC hours (e.g., 06:00 UTC and 07:00 UTC) both map to local_hour = 1. The assignment hourly_data[local_hour] = {...} silently overwrites the first entry with the second. One full hour of bandwidth data is discarded with no warning. Consider keying on hour_start (UTC) and only converting to local time when building the response list.

---

🔵 Low / Cleanup

src/collectors/data_usage_collector.py (~line 211) — N+1 query pattern in _store_hourly_values

One SELECT per hour bucket, called once for the network and once per device. With 24 buckets × 30 devices, that's ~720 queries per hourly collection run. A single pre-fetch of existing HourlyBandwidth rows matching (network_name, device_id) for the day followed by an in-memory dict lookup would collapse this to one query per call site.

test_data_usage.py — manual integration script committed to repo root

This file calls time.sleep(30), contacts the live eero API, and writes to the real database. Committed to the root it ends up in Docker images and CI artifacts, and pytest may try to collect it. Better placed in a gitignored scratch/ directory or removed entirely before merge.

src/api/health/analytics.py (local BYTES_PER_MB) and src/collectors/data_usage_collector.py (module-level BYTES_PER_MB) — duplicated constant

Both files independently define BYTES_PER_MB = 1_000_000. A single shared location (e.g., a constants module) would prevent silent divergence if the definition ever needs to change.

---

Summary

Severity	Count
🔴 Critical	3
🟠 High	1
🟡 Medium	1
🔵 Low	3

The approach is right — server-computed totals are strictly better than rate accumulation. The critical items above (GET body, missing migration, session rollback) need to be resolved before this can ship safely.

🤖 Review generated with Claude Code

[claude]

Code Review

This PR introduces a DataUsageCollector that replaces rate-based bandwidth accumulation with eero's server-computed /data_usage endpoint. The architecture is sound and the motivation (eliminating undercount from polling gaps) is valid. However, there are several bugs that would prevent the feature from working correctly — a few are silent data-corruptors rather than obvious crashes.

---

🔴 Critical

src/eero_client/client.py ~line 682 — GET request with JSON body; parameters silently dropped or TypeError

_request_data_usage calls api.request("GET", ..., json={...}). The rest of the client uses api.get(url) with no body (see line 289 for the profiles endpoint). GET bodies are semantically undefined per RFC 9110 — servers and proxies are allowed to ignore them entirely. More concretely, if APIClient.request only accepts (method, url) positional args (per eero_patch.py's signature), passing json= as a keyword would raise TypeError on every invocation, meaning DataUsageCollector crashes silently on every run.

Fix: switch to params={"start": start, "end": end, "cadence": cadence, "timezone": timezone_str} if the API accepts query-string parameters, or use api.get(url, params=...) to match the established pattern.

---

src/collectors/data_usage_collector.py lines 50–55 — per-network rollback discards all prior networks' data

except Exception as e:
    self.db.rollback()   # rolls back the entire session — including network A's adds
    errors += 1

self.db.commit()         # commits nothing (session was just rolled back)

_collect_network_usage and _collect_device_usage call self.db.add(...) but never self.db.commit(). All adds are pending in the session. If network A succeeds and network B raises, the rollback() discards every db.add() from all previously processed networks — not just B's. Network A's hourly rows are silently lost. The return value still claims items_collected: N, misleading any monitoring that checks for errors == 0.

Fix: use a savepoint per network, or commit after each network succeeds:

self._collect_network_usage(network_name, today_window)
self._collect_device_usage(network_name, today_window)
self.db.commit()  # commit this network's data before moving on

---

🟠 High

src/services/bandwidth_report_service.py lines ~112 and ~205 — double-counting after this PR

_get_period_totals and _get_daily_breakdown query DailyBandwidth with only a network_name + date-range filter — no device_id guard. Before this PR, only per-device rows existed. After this PR, DataUsageCollector also writes network-wide rows (device_id = NULL). Both will be summed together, reporting roughly 2× the real bandwidth in any period/daily breakdown report. Note: _get_top_devices already handles this correctly with DailyBandwidth.device_id.isnot(None) — the same pattern needs to be applied here.

---

src/migrations/010_add_data_usage_tables.py line 41 — SQLite UNIQUE index doesn't enforce uniqueness for NULL device_id

CREATE UNIQUE INDEX uix_network_device_hour ON hourly_bandwidth (network_name, device_id, hour_start)

Per the SQL standard (which SQLite follows), NULL ≠ NULL in unique index comparisons. Two rows with device_id IS NULL and the same (network_name, hour_start) will not trigger a constraint violation. The ORM-side UniqueConstraint in database.py has the same limitation. This means the manual query-then-insert logic in _store_hourly_values is the only guard for network-wide rows — if the eero API ever returns duplicate time entries, two rows are silently inserted.

Fix: SQLite 3.37+ supports NULLS NOT DISTINCT in unique indexes. Alternatively, replace the NULL sentinel with a dedicated string like "__network__" to make the unique constraint work across all SQLite versions.

---

🟡 Medium

src/api/health/analytics.py line ~1042 — DST fall-back silently drops one hour of data

local_hour = hour_start_utc.astimezone(tz).hour
hourly_data[local_hour] = { ... }   # plain dict assignment, not accumulation

During the one-hour DST fall-back, two consecutive UTC hours map to the same local hour integer. The second assignment silently overwrites the first. The affected hour (e.g., 01:00 local) shows only one of its two records; the other's data is permanently lost from the response. Fix: accumulate (add download_mb/upload_mb) when local_hour already exists in hourly_data.

---

Cleanup

• BYTES_PER_MB = 1_000_000 is defined as a module constant in data_usage_collector.py and again as a local variable inside a with block in analytics.py. Move to a shared constants location.
• _store_hourly_values issues one SELECT ... .first() per hourly slot (up to 24 × N-devices queries per run). Bulk-fetch existing rows for the day before the loop, key by hour_start_naive, and branch in memory — or use SQLite's INSERT OR REPLACE / ON CONFLICT DO UPDATE to collapse each upsert to one statement.
• test_data_usage.py at repo root is a developer script and should either live in tests/ or be listed in .gitignore. No tests cover the new DataUsageCollector logic (_extract_series, _store_hourly_values, _update_daily_from_server).

---

Review generated with Claude Code