Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions backend/app/telemetry/service.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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,
)
)
Expand Down
76 changes: 76 additions & 0 deletions backend/tests/test_web_features.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
schema: spec-driven
created: 2026-11-02
45 changes: 45 additions & 0 deletions openspec/changes/preserve-client-ping-sync-status/design.md
Original file line number Diff line number Diff line change
@@ -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.
21 changes: 21 additions & 0 deletions openspec/changes/preserve-client-ping-sync-status/proposal.md
Original file line number Diff line number Diff line change
@@ -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
<!-- None -->

### 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.
Original file line number Diff line number Diff line change
@@ -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.
8 changes: 8 additions & 0 deletions openspec/changes/preserve-client-ping-sync-status/tasks.md
Original file line number Diff line number Diff line change
@@ -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
Loading