fix(api): read last-seen from every session, count only usable ones - #1279
Conversation
The admin drawer took both figures off the user's *active* sessions, and both were wrong for it. Somebody who has signed out has no active session row at all, so `last_seen_at` came back null and the drawer said "Never signed in" - most accounts most of the time, and the opposite of the truth on the one field the drawer exists to answer. Worse, that is exactly the case the field is there to separate from an account created and never used, which now renders identically. Last-seen is a fact about every session somebody has ever had, so the read takes the whole history. The reverse on the other figure: nothing sweeps a session that simply lapses - the row stays `is_active` until the next refresh finds it expired and declines it - so a session nobody can use was counted as open. "Open" now means `is_active AND expires_at > now()`, in the repository rather than at one call site, so the user's own devices list stops offering an expired row to revoke as well. `active_only` is `open_only` for that reason: the old name described the column, not the question. `newest_session_at` stays scoped to the open ones - it is read beside their count, and "newest session August" under "0 open sessions" is a sentence about nothing. Verified with four integration cases, two of which fail against the old read: a user whose every session is inactive (last-seen is their most recent, open count zero) and an `is_active` row already past `expires_at` (not open, and still counted for last-seen). `make test` at 100%, `make lint` green. Closes #1256
|
You have reached your Codex usage limits for security reviews. Please try again later. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a238358cd1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Codex, P2 on this branch: the history read materialized and sorted every session a user has ever had to look at the first timestamp. Nothing prunes that table - a refresh deactivates the row it used and inserts another - so a year-old account carries thousands of rows and the admin drawer got slower for exactly the accounts an administrator is most likely to open. `limit=1`, which the ordering already makes correct: most-recently-used first, so the head is the whole answer. The open-sessions read stays a list because its length is one of the two figures, and it is bounded by what is actually still usable. An API test pins both scopes and the bound in one place: the calls are `open_only=False, limit=1` then `open_only=True`, in that order.
|
Fixed in 4780808 — right, and it lands hardest on exactly the accounts an administrator opens. Nothing prunes
An API test pins both scopes and the bound in one place — the calls are @codex review |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4780808e35
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…cutoff Two more from Codex on this branch, both fair. **`LIMIT 1` bounded the rows, not the work.** `sessions` had an index on `user_id` alone while every read of it orders by `last_used_at, id`, so Postgres still fetched a user's whole history and sorted it to answer with one row - and nothing prunes that history. `0061` adds `(user_id, last_used_at DESC, id)` and drops the single-column index, which this one leads on: Postgres would never choose it and every insert would still maintain it. `id` is in the index because `last_used_at` ties on two sign-ins in the same moment and the page order has to be total. **Two statements, two cutoffs.** `list_sessions` asks for a page and then for the total, and each `_open` read its own `datetime.now(UTC)` - so a session lapsing between them was in the page and outside the count, with nobody writing a row. `_open` takes the caller's `now`, and the service computes one per response. The test pins that the two calls carry the same value. Verified: the chain applies and downgrades against pgvector, `alembic check` reports no drift from the models, and the index reads `btree (user_id, last_used_at DESC, id)` on the built database. `make test` at 100%, `make lint` green. Nothing under `docs/` describes this table's indexes, so the drift check's suggestion of `architecture.md` has nothing to say here. Note for whoever merges: `feat/rag-reranker` carries a different `0061`. Whichever lands second renumbers.
|
Both fixed in ac83559. The index. Right that The cutoff. Also right, and it breaks without a concurrent write: One thing for whoever merges: @codex review |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Release commit for 0.0.324. Bumps `backend/pyproject.toml`, `backend/uv.lock` and `frontend/package.json` to 0.0.324 and adds the CHANGELOG entry. Releases #1256: the admin drawer read both of its session figures off the active sessions, and both questions wanted a different set - last-seen is a fact about every session a user ever had, and "open" has to exclude a row that lapsed without anything sweeping it. Verified on #1279 against a real Postgres, because both halves are a `WHERE` clause: every session inactive, no session ever, an `is_active` row past `expires_at`, and an unexpired row beside an inactive one. The first and third fail against the old read. `make lint` and `make test` green, platform layer at 100%. The three smaller items on the issue were already fixed on `main` in #1239. `scripts/check_backticks.py` and `make lint-spelling` clean.
1 participant
The admin drawer took both of its session figures off the same read — the user's active sessions — and both were wrong for it.
"Never signed in" for anybody who had signed out
A user who signs out, or has their sessions revoked, has no active session row at all.
last_seen_atcame back null and the drawer rendered "Never signed in." That is most accounts most of the time, and it is the opposite of the truth — on the one field the drawer exists to answer, and in exactly the case the field is there to separate from created and never used.Where somebody was last seen is a fact about every session they have ever had, so the read takes the whole history (
open_only=False) and the head of it — most-recently-used first — is the answer.An expired session counted as open
Nothing sweeps a session that simply lapses: the row stays
is_activeuntil the next refresh finds it pastexpires_atand declines it. So a session nobody can use was reported as open."Open" now means
is_active AND expires_at > now(), and it lives inapp/repositories/session.pyrather than at one call site — which is why the flag isopen_onlyand notactive_only: the old name described the column, not the question. The user's own devices list goes through the same two functions, so it stops offering an expired row to revoke as well; that surface had the identical defect and it is one predicate, so it is fixed here rather than filed.newest_session_atstays scoped to the open ones — it is read beside their count, and "newest session August" under "0 open sessions" is a sentence about nothing.How it was verified
backend/tests/integration/test_admin_user_last_seen.py, against a real Postgres because both halves are aWHEREclause:last_seen_atis their most recent session's timestamp, open count 0,newest_session_atnull;last_seen_atstill null (the distinction the field exists for);is_activerow pastexpires_at→ not open, and still counted for last-seen;The first and third fail against the old read; the other two pin what was already right.
make lintandmake test(100% on the platform layer) green.tests/test_sessions_pagination.pypinned the old kwarg name and now pins the new one.The three smaller items on the issue were already fixed on
mainin #1239: the drawer's last-seen field renders-rather than "Never signed in" when the detail request fails, and both the membership rows and the Activity link are plain text rather than destinations that 404.Closes #1256