From 83a6d2c88c1a8c01376941bdf372256cc9102c6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Brandst=C3=A4tter?= Date: Mon, 2 Nov 2026 08:44:20 +0100 Subject: [PATCH] fix(telemetry): preserve client sync status on heartbeat ping --- backend/app/telemetry/service.py | 6 +- backend/tests/test_web_features.py | 76 +++++++++++++++++++ .../.openspec.yaml | 2 + .../design.md | 45 +++++++++++ .../proposal.md | 21 +++++ .../specs/central-telemetry/spec.md | 12 +++ .../preserve-client-ping-sync-status/tasks.md | 8 ++ 7 files changed, 167 insertions(+), 3 deletions(-) create mode 100644 openspec/changes/preserve-client-ping-sync-status/.openspec.yaml create mode 100644 openspec/changes/preserve-client-ping-sync-status/design.md create mode 100644 openspec/changes/preserve-client-ping-sync-status/proposal.md create mode 100644 openspec/changes/preserve-client-ping-sync-status/specs/central-telemetry/spec.md create mode 100644 openspec/changes/preserve-client-ping-sync-status/tasks.md diff --git a/backend/app/telemetry/service.py b/backend/app/telemetry/service.py index 5675621..8f4ba9b 100644 --- a/backend/app/telemetry/service.py +++ b/backend/app/telemetry/service.py @@ -7,7 +7,7 @@ from datetime import datetime, timedelta, timezone from typing import Any, Optional -from sqlalchemy import delete, select +from sqlalchemy import delete, func, select from sqlalchemy.dialects.sqlite import insert from sqlalchemy.ext.asyncio import AsyncSession @@ -84,8 +84,8 @@ async def upsert_client_ping( set_=dict( client_version=stmt.excluded.client_version, last_seen_at=stmt.excluded.last_seen_at, - last_sync_at=stmt.excluded.last_sync_at, - last_sync_ok=stmt.excluded.last_sync_ok, + last_sync_at=func.coalesce(stmt.excluded.last_sync_at, ClientConnection.last_sync_at), + last_sync_ok=func.coalesce(stmt.excluded.last_sync_ok, ClientConnection.last_sync_ok), backend_version_at_ping=stmt.excluded.backend_version_at_ping, ) ) diff --git a/backend/tests/test_web_features.py b/backend/tests/test_web_features.py index c6f9156..d026688 100644 --- a/backend/tests/test_web_features.py +++ b/backend/tests/test_web_features.py @@ -155,6 +155,82 @@ def test_client_ping(client: TestClient) -> None: assert r.status_code == 204, r.text +def test_client_ping_preserves_sync_status(client: TestClient) -> None: + h = _auth_headers(client) + # 1. Ping with sync status + r1 = client.post( + "/api/clients/ping", + headers=h, + json={ + "client_type": "tauri-desktop", + "client_version": "1.0.0", + "last_sync_ok": True, + "last_sync_at": "2026-03-30T10:00:00Z", + }, + ) + assert r1.status_code == 204, r1.text + + admin_res = client.get("/api/admin/clients", headers=h) + assert admin_res.status_code == 200 + conn = next(c for c in admin_res.json() if c["client_type"] == "tauri-desktop") + assert conn["last_sync_ok"] is True + assert conn["last_sync_at"] is not None + saved_sync_at = conn["last_sync_at"] + + # 2. Ping without sync status (heartbeat / presence check) + r2 = client.post( + "/api/clients/ping", + headers=h, + json={ + "client_type": "tauri-desktop", + "client_version": "1.0.1", + }, + ) + assert r2.status_code == 204, r2.text + + admin_res2 = client.get("/api/admin/clients", headers=h) + assert admin_res2.status_code == 200 + conn2 = next(c for c in admin_res2.json() if c["client_type"] == "tauri-desktop") + assert conn2["client_version"] == "1.0.1" + assert conn2["last_sync_ok"] is True + assert conn2["last_sync_at"] == saved_sync_at + + # 3. Ping with failed sync status + r3 = client.post( + "/api/clients/ping", + headers=h, + json={ + "client_type": "tauri-desktop", + "client_version": "1.0.1", + "last_sync_ok": False, + "last_sync_at": "2026-03-30T11:00:00Z", + }, + ) + assert r3.status_code == 204, r3.text + + admin_res3 = client.get("/api/admin/clients", headers=h) + assert admin_res3.status_code == 200 + conn3 = next(c for c in admin_res3.json() if c["client_type"] == "tauri-desktop") + assert conn3["last_sync_ok"] is False + + # 4. Subsequent heartbeat ping retains the failed sync status + r4 = client.post( + "/api/clients/ping", + headers=h, + json={ + "client_type": "tauri-desktop", + "client_version": "1.0.2", + }, + ) + assert r4.status_code == 204, r4.text + + admin_res4 = client.get("/api/admin/clients", headers=h) + assert admin_res4.status_code == 200 + conn4 = next(c for c in admin_res4.json() if c["client_type"] == "tauri-desktop") + assert conn4["client_version"] == "1.0.2" + assert conn4["last_sync_ok"] is False + + def test_oauth_complete_invalid(client: TestClient) -> None: r = client.post("/api/auth/oauth/complete", json={"exchange": str(uuid.uuid4())}) assert r.status_code == 401 diff --git a/openspec/changes/preserve-client-ping-sync-status/.openspec.yaml b/openspec/changes/preserve-client-ping-sync-status/.openspec.yaml new file mode 100644 index 0000000..a7587c5 --- /dev/null +++ b/openspec/changes/preserve-client-ping-sync-status/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-11-02 diff --git a/openspec/changes/preserve-client-ping-sync-status/design.md b/openspec/changes/preserve-client-ping-sync-status/design.md new file mode 100644 index 0000000..f9377d0 --- /dev/null +++ b/openspec/changes/preserve-client-ping-sync-status/design.md @@ -0,0 +1,45 @@ +## Context + +See `proposal.md` for motivation. Currently, `upsert_client_ping` in `backend/app/telemetry/service.py` executes: +```python +stmt = stmt.on_conflict_do_update( + index_elements=['user_email', 'client_type'], + set_=dict( + client_version=stmt.excluded.client_version, + last_seen_at=stmt.excluded.last_seen_at, + last_sync_at=stmt.excluded.last_sync_at, + last_sync_ok=stmt.excluded.last_sync_ok, + backend_version_at_ping=stmt.excluded.backend_version_at_ping, + ) +) +``` +When a client ping supplies `last_sync_at=None` or `last_sync_ok=None`, SQLite directly overwrites `ClientConnection.last_sync_at` and `ClientConnection.last_sync_ok` with NULL. + +## Goals / Non-Goals + +**Goals:** +- Preserve existing `last_sync_at` and `last_sync_ok` in `client_connections` when incoming ping omits sync status. +- Allow new sync status (including `last_sync_ok = False`) to update properly whenever provided. +- Maintain single SQL roundtrip performance (`ON CONFLICT DO UPDATE`). + +**Non-Goals:** +- Adding dedicated reset endpoints or schema migrations to `client_connections`. +- Changing how web or desktop clients construct ping requests. + +## Decisions + +### Decision 1: Use `func.coalesce` in `on_conflict_do_update` +- **Choice**: + ```python + last_sync_at=func.coalesce(stmt.excluded.last_sync_at, ClientConnection.last_sync_at), + last_sync_ok=func.coalesce(stmt.excluded.last_sync_ok, ClientConnection.last_sync_ok), + ``` +- **Rationale**: In SQL standard and SQLite, `COALESCE(new_val, old_val)` evaluates to `new_val` when `new_val IS NOT NULL`, and falls back to `old_val` when `new_val IS NULL`. For booleans, `FALSE` (0) is a non-null value, so failed syncs (`last_sync_ok=False`) correctly overwrite previous states. +- **Alternatives Considered**: + - *Read-before-write in Python*: Query existing record, merge fields in application code, and issue update. Rejected because it doubles DB roundtrips and introduces race conditions under concurrent pings. + - *CASE expression*: `case((stmt.excluded.last_sync_at.isnot(None), stmt.excluded.last_sync_at), else_=ClientConnection.last_sync_at)`. Functionally identical to `coalesce`, but `coalesce` is cleaner and standard in SQL. + +## Risks / Trade-offs + +- [Risk] If a client intentionally wanted to clear/reset sync state back to NULL via ping, `coalesce` would prevent this. + → Mitigation: Sync state in `ClientConnection` represents "last known sync outcome", which should persist across client restarts and presence pings. Resetting sync status is not a supported or required client operation. diff --git a/openspec/changes/preserve-client-ping-sync-status/proposal.md b/openspec/changes/preserve-client-ping-sync-status/proposal.md new file mode 100644 index 0000000..c8ba5d0 --- /dev/null +++ b/openspec/changes/preserve-client-ping-sync-status/proposal.md @@ -0,0 +1,21 @@ +## Why + +Clients send periodic heartbeats or presence pings (such as on web login or during background loops) where sync status (`last_sync_at` and `last_sync_ok`) is omitted. The backend `upsert_client_ping` function currently performs an unconditional SQLite `ON CONFLICT DO UPDATE` that sets `last_sync_at` and `last_sync_ok` directly to excluded values, overwriting previously recorded sync timestamps and success flags with `NULL`. This erases client sync status history and causes admin telemetry queries to report missing sync state. + +## What Changes + +- Modify `upsert_client_ping` in `app/telemetry/service.py` to use `func.coalesce` so that incoming NULL sync fields (`last_sync_at`, `last_sync_ok`) preserve the existing values in `client_connections`. +- Add test coverage verifying that subsequent pings with null sync statuses do not erase earlier recorded sync outcomes. + +## Capabilities + +### New Capabilities + + +### Modified Capabilities +- `central-telemetry`: Update client ping ingestion behavior so that heartbeats and pings omitting sync status retain prior sync status records rather than resetting them to null. + +## Impact + +- Affected Code: `backend/app/telemetry/service.py` (`upsert_client_ping`) and `backend/tests/test_web_features.py` / `backend/tests/test_telemetry.py`. +- No database migrations or API contract breaking changes; purely preserves existing row data on conflict update. diff --git a/openspec/changes/preserve-client-ping-sync-status/specs/central-telemetry/spec.md b/openspec/changes/preserve-client-ping-sync-status/specs/central-telemetry/spec.md new file mode 100644 index 0000000..edcaae2 --- /dev/null +++ b/openspec/changes/preserve-client-ping-sync-status/specs/central-telemetry/spec.md @@ -0,0 +1,12 @@ +## ADDED Requirements + +### Requirement: Client connection ping and sync state retention +The backend SHALL maintain the latest client connection details per user and client type, updating client version and last seen timestamp while preserving existing sync status when incoming pings omit sync outcome fields. + +#### Scenario: Client reports sync outcome +- **WHEN** a client submits a ping request to `/api/clients/ping` with `last_sync_at` and `last_sync_ok` +- **THEN** the backend updates the client connection record with the provided sync timestamp and success status. + +#### Scenario: Client sends heartbeat ping omitting sync outcome +- **WHEN** a client submits a ping request to `/api/clients/ping` with null or omitted `last_sync_at` and `last_sync_ok` after a previous ping recorded sync status +- **THEN** the backend updates `last_seen_at`, `client_version`, and `backend_version_at_ping` while retaining the existing `last_sync_at` and `last_sync_ok` values. diff --git a/openspec/changes/preserve-client-ping-sync-status/tasks.md b/openspec/changes/preserve-client-ping-sync-status/tasks.md new file mode 100644 index 0000000..c3f01ca --- /dev/null +++ b/openspec/changes/preserve-client-ping-sync-status/tasks.md @@ -0,0 +1,8 @@ +## 1. Backend Implementation + +- [x] 1.1 Update `upsert_client_ping` in `backend/app/telemetry/service.py` to use `func.coalesce` for `last_sync_at` and `last_sync_ok` +- [x] 1.2 Add regression unit test in `backend/tests/test_web_features.py` verifying sync status is preserved when subsequent ping omits sync status + +## 2. Verification + +- [x] 2.1 Run `pytest` on backend test suite to ensure all tests pass and no regressions occur