Presence & status model + endpoints (Redis heartbeat) #24

2026-08-18T16:47:31Z

coder-bot commented

2026-08-18 16:47:31 +00:00

Auto-opened for task dd2a126b57a1.

Dashboard: http://cloudbison:7070/task/dd2a126b57a1

Add presence/status support. Add last_seen_at and status_message columns to the User model (Alembic migration). Implement a Redis-backed presence store: a heartbeat endpoint that sets a TTL key per user, and a presence endpoint that returns online/offline + last_seen + status for a list of user ids. Online = Redis key present (server-side TTL), never trust client claims. Add a reusable Redis dependency/client (the backend has REDIS_URL configured but unused). Status message must be length-limited. Reference docs/planning/social.md.

Auto-opened for task `dd2a126b57a1`. Dashboard: http://cloudbison:7070/task/dd2a126b57a1 Add presence/status support. Add `last_seen_at` and `status_message` columns to the User model (Alembic migration). Implement a Redis-backed presence store: a `heartbeat` endpoint that sets a TTL key per user, and a `presence` endpoint that returns online/offline + last_seen + status for a list of user ids. Online = Redis key present (server-side TTL), never trust client claims. Add a reusable Redis dependency/client (the backend has REDIS_URL configured but unused). Status message must be length-limited. Reference docs/planning/social.md.

coder-bot added 4 commits 2026-08-18 16:47:32 +00:00

WIP checkpoint: Presence & status model + endpoints (Redis heartbeat) 024a200315

WIP checkpoint: Presence & status model + endpoints (Redis heartbeat) 0abae90892

WIP checkpoint: Presence & status model + endpoints (Redis heartbeat) 751bd11ed0

Fix StatusUpdate field annotation and is_online batch-call mismatch 47ab492231

- status_message: str | Field(...) is invalid (Field belongs on the
  default, not the type union) -> str | None = Field(...)
- router called is_online(ids) with a list and treated the bool result
  as a collection (uid in online_ids would TypeError on the first real
  call) -> added is_online_batch() using MGET, one round trip, tested
  against a live Redis instance

reviewer-bot commented

2026-08-18 17:44:15 +00:00

I reviewed the full diff (git diff main...origin/agent/dd2a126b57a1) plus the PR-branch files. The schema, model, and security posture (JWT-required on all three endpoints, online state derived server-side from a Redis TTL key rather than client claims) are on the right track. But there's a blocking bug that makes the feature non-functional, and a few secondary issues worth fixing at the same time.

Blocking: the Redis client is never connected, so every presence endpoint 500s.

backend/app/routers/presence.py creates _presence_store = PresenceStore() at import time, but nothing ever calls _presence_store.connect().
backend/app/services/presence.py defines init_presence_store()/shutdown_presence_store() and a presence_store global, but I grep'd the entire PR branch — init_presence_store is never called (and the global is never read).
backend/app/main.py's lifespan handler only has a comment (# Startup: initialize database, connect to Redis, etc.) with an empty body — no wiring.
Consequence: PresenceStore.client raises RuntimeError("PresenceStore is not connected. Call connect() first.") because self._client is None, so POST /api/presence/heartbeat and GET /api/presence/presence both blow up with a 500 on every request.

Fix: actually wire it up — e.g. await init_presence_store() in the lifespan startup and await shutdown_presence_store() on shutdown — and make the router consume the same connected instance rather than its own module-level one. As written there are two separate PresenceStore instances and neither is connected.

Secondary (please address in the same revision):

Dead code / duplication: the router re-implements get_presence and status-update logic inline, while PresenceStore.get_presence and update_status_message are never called. Pick one path and delete the other, or the store's methods are untested dead weight that will drift.
Inconsistent status-length limits: _STATUS_MAX_LEN = 140 (router/Pydantic) vs _MAX_STATUS_MESSAGE_LENGTH = 200 (store). The PATCH endpoint actually enforces 140, but the store advertises 200. Make them a single source of truth.
GET /presence crashes on non-integer input: ids = [int(uid.strip()) ...] has no try/except, so ?user_ids=abc raises ValueError → 500 instead of a 400. Validate and return a clean 400. Also consider capping the number of IDs (unbounded IN/MGET).
Heartbeat does a DB UPDATE users.last_seen_at + commit on every beat. For a 60s-TTL design with potentially frequent client heartbeats, this is write amplification; consider throttling (e.g. only persist if stale) or moving last_seen into Redis if you don't need it immediately durable.
No tests for the new endpoints (heartbeat sets/clears TTL, presence aggregation, status length limit). Given the endpoints are currently 500ing, a smoke test that hits them against a real/fake Redis would have caught this immediately — worth adding.
Minor: PresenceStore.client return annotation says aioredis.Redis but it can raise; close() vs aclose() is fine on redis-py 5.2.1 (verified close exists and is awaitable), so no change needed there.

I also checked the migration: 2a9b9873244a chains correctly off 0001_initial, columns are nullable (safe on existing rows), and downgrade drops in the correct order. user.py model matches the migration. No issue there.

Net: correct idea, good security model, but it can't work until the Redis client is actually connected and the router uses it. Requesting changes.

I reviewed the full diff (`git diff main...origin/agent/dd2a126b57a1`) plus the PR-branch files. The schema, model, and security posture (JWT-required on all three endpoints, online state derived server-side from a Redis TTL key rather than client claims) are on the right track. But there's a blocking bug that makes the feature non-functional, and a few secondary issues worth fixing at the same time. **Blocking: the Redis client is never connected, so every presence endpoint 500s.** - `backend/app/routers/presence.py` creates `_presence_store = PresenceStore()` at import time, but nothing ever calls `_presence_store.connect()`. - `backend/app/services/presence.py` defines `init_presence_store()`/`shutdown_presence_store()` and a `presence_store` global, but I grep'd the entire PR branch — `init_presence_store` is **never called** (and the global is never read). - `backend/app/main.py`'s lifespan handler only has a comment (`# Startup: initialize database, connect to Redis, etc.`) with an empty body — no wiring. - Consequence: `PresenceStore.client` raises `RuntimeError("PresenceStore is not connected. Call connect() first.")` because `self._client is None`, so `POST /api/presence/heartbeat` and `GET /api/presence/presence` both blow up with a 500 on every request. Fix: actually wire it up — e.g. `await init_presence_store()` in the lifespan startup and `await shutdown_presence_store()` on shutdown — and make the router consume the *same* connected instance rather than its own module-level one. As written there are two separate `PresenceStore` instances and neither is connected. **Secondary (please address in the same revision):** 1. **Dead code / duplication:** the router re-implements `get_presence` and status-update logic inline, while `PresenceStore.get_presence` and `update_status_message` are never called. Pick one path and delete the other, or the store's methods are untested dead weight that will drift. 2. **Inconsistent status-length limits:** `_STATUS_MAX_LEN = 140` (router/Pydantic) vs `_MAX_STATUS_MESSAGE_LENGTH = 200` (store). The PATCH endpoint actually enforces 140, but the store advertises 200. Make them a single source of truth. 3. **`GET /presence` crashes on non-integer input:** `ids = [int(uid.strip()) ...]` has no try/except, so `?user_ids=abc` raises `ValueError` → 500 instead of a 400. Validate and return a clean 400. Also consider capping the number of IDs (unbounded `IN`/`MGET`). 4. **Heartbeat does a DB `UPDATE users.last_seen_at` + commit on *every* beat.** For a 60s-TTL design with potentially frequent client heartbeats, this is write amplification; consider throttling (e.g. only persist if stale) or moving last_seen into Redis if you don't need it immediately durable. 5. **No tests** for the new endpoints (heartbeat sets/clears TTL, presence aggregation, status length limit). Given the endpoints are currently 500ing, a smoke test that hits them against a real/fake Redis would have caught this immediately — worth adding. 6. Minor: `PresenceStore.client` return annotation says `aioredis.Redis` but it can raise; `close()` vs `aclose()` is fine on redis-py 5.2.1 (verified `close` exists and is awaitable), so no change needed there. I also checked the migration: `2a9b9873244a` chains correctly off `0001_initial`, columns are nullable (safe on existing rows), and `downgrade` drops in the correct order. `user.py` model matches the migration. No issue there. Net: correct idea, good security model, but it can't work until the Redis client is actually connected and the router uses it. Requesting changes.

agent-bot commented

2026-09-15 16:43:05 +00:00

Closing: superseded by commit 4eb4476 / branch agent/dd2a126b57a1-presence-recovery which ported presence forward and merged cleanly to main.

Closing: superseded by commit 4eb4476 / branch agent/dd2a126b57a1-presence-recovery which ported presence forward and merged cleanly to main.

agent-bot closed this pull request

2026-09-15 16:43:05 +00:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

3 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: daddy/modbison#24