Skip to content
Merged
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
45 changes: 45 additions & 0 deletions backend/alembic/versions/0061_session_last_used_index.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
"""The index every read of a user's sessions actually wants (#1256).

All three of them ask the same question - this user's sessions, most recently
used first: the devices list, its page count, and the admin drawer's last-seen.
The table had an index on `user_id` alone, so answering meant fetching a user's
rows and sorting them.

That is fine for a week-old account and not for a year-old one. Nothing prunes
`sessions`: a refresh deactivates the row it used and inserts another, so the
history grows for as long as somebody keeps signing in, and the admin drawer got
slower for exactly the accounts an administrator is most likely to open.

`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 - the same reason the query orders on
it.

The single-column index goes: this one leads on `user_id`, so Postgres would
never choose it and every insert would still maintain it.
"""

from collections.abc import Sequence

from sqlalchemy import text

from alembic import op

revision: str = "0061_session_last_used_index"
Comment thread
DEENUU1 marked this conversation as resolved.
down_revision: str | None = "0060_conversation_favourites"
Comment thread
DEENUU1 marked this conversation as resolved.
branch_labels: str | Sequence[str] | None = None
Comment thread
DEENUU1 marked this conversation as resolved.
depends_on: str | Sequence[str] | None = None
Comment thread
DEENUU1 marked this conversation as resolved.

_COMPOSITE = "sessions_user_id_last_used_at_idx"
_SINGLE = "sessions_user_id_idx"


def upgrade() -> None:
# `text(...)` for the descending column: a plain string would be quoted as
# one identifier named "last_used_at DESC".
op.create_index(_COMPOSITE, "sessions", ["user_id", text("last_used_at DESC"), "id"])
op.drop_index(_SINGLE, table_name="sessions")


def downgrade() -> None:
op.create_index(_SINGLE, "sessions", ["user_id"], unique=False)
op.drop_index(_COMPOSITE, table_name="sessions")
23 changes: 21 additions & 2 deletions backend/app/db/models/session.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
import uuid
from datetime import UTC, datetime

from sqlalchemy import Boolean, DateTime, ForeignKey, String, Text
from sqlalchemy import Boolean, DateTime, ForeignKey, Index, String, Text
from sqlalchemy import text as sa_text
from sqlalchemy.dialects.postgresql import UUID
from sqlalchemy.orm import Mapped, mapped_column, relationship

Expand All @@ -14,10 +15,28 @@ class Session(Base):
"""User session model for tracking active login sessions."""

__tablename__ = "sessions"
__table_args__ = (
# Every read of a user's sessions is "this user's, most recently used
# first" - the devices list, its page count, and the admin drawer's
# last-seen. Nothing prunes this table (a refresh deactivates the row it
# used and inserts another), so on a long-lived account the leading
# column alone left Postgres sorting a year of rows to answer with one
# (#1256). `id` is in it because `last_used_at` ties on two sign-ins in
# the same moment and the page order has to be total.
Index(
"sessions_user_id_last_used_at_idx",
"user_id",
sa_text("last_used_at DESC"),
"id",
),
)

id: Mapped[uuid.UUID] = mapped_column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4)
# No index of its own: the composite above leads on this column, so a
# single-column one is a second index Postgres would never choose and every
# insert would still maintain.
user_id: Mapped[uuid.UUID] = mapped_column(
UUID(as_uuid=True), ForeignKey("users.id", ondelete="CASCADE"), nullable=False, index=True
UUID(as_uuid=True), ForeignKey("users.id", ondelete="CASCADE"), nullable=False
)
refresh_token_hash: Mapped[str] = mapped_column(String(255), nullable=False, index=True)
device_name: Mapped[str | None] = mapped_column(String(255), nullable=True)
Expand Down
39 changes: 32 additions & 7 deletions backend/app/repositories/session.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
"""Session repository (PostgreSQL async)."""

from datetime import UTC, datetime
from typing import Any
from uuid import UUID

from sqlalchemy import func, select, update
from sqlalchemy import Select, func, select, update
from sqlalchemy.ext.asyncio import AsyncSession

from app.db.models.session import Session
Expand All @@ -25,22 +26,45 @@ async def get_by_refresh_token_hash(db: AsyncSession, token_hash: str) -> Sessio
return result.scalar_one_or_none()


def _open(query: Select[tuple[Any]], *, now: datetime) -> Select[tuple[Any]]:
"""Narrow a session query to the ones actually still usable.

`is_active` alone is not that. A session is deactivated when somebody signs
out or an administrator revokes it, but nothing sweeps the ones that simply
ran out: the row stays `is_active` until the next refresh finds it expired
and declines it. So a query on `is_active` counts sessions that cannot be
used again, which is how the admin drawer reported open sessions for an
account whose every session had lapsed (#1256).

`now` is the caller's, not this function's, because a response can be built
from two statements: `SessionService.list_sessions` asks for a page and then
for the total, and a session lapsing between them would otherwise be in the
page and outside the count.
"""
return query.where(Session.is_active.is_(True), Session.expires_at > now)


async def get_user_sessions(
db: AsyncSession,
user_id: UUID,
*,
active_only: bool = True,
open_only: bool = True,
now: datetime | None = None,
skip: int = 0,
limit: int | None = None,
) -> list[Session]:
"""Get sessions for a user, most recently used first.

`limit` of None returns every session, which is what the callers that
revoke or validate one need. The listing route passes a page.

`open_only=False` is the whole history, which is what answers "when were
they last here": somebody who has signed out has no open session and has
very much been here.
"""
query = select(Session).where(Session.user_id == user_id)
if active_only:
query = query.where(Session.is_active.is_(True))
if open_only:
query = _open(query, now=now or datetime.now(UTC))
# `last_used_at` alone is not a total order - a user who signs in twice in
# the same request cycle gets two rows with the same timestamp, and an
# unstable order means a row can appear on two pages or on neither. `id`
Expand All @@ -56,12 +80,13 @@ async def count_user_sessions(
db: AsyncSession,
user_id: UUID,
*,
active_only: bool = True,
open_only: bool = True,
now: datetime | None = None,
) -> int:
"""How many sessions the user has, for the page count."""
query = select(func.count(Session.id)).where(Session.user_id == user_id)
if active_only:
query = query.where(Session.is_active.is_(True))
if open_only:
query = _open(query, now=now or datetime.now(UTC))
return (await db.execute(query)).scalar_one()


Expand Down
14 changes: 9 additions & 5 deletions backend/app/services/session.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,10 +70,10 @@ async def create_session(
)

async def get_user_sessions(self, user_id: UUID) -> list[Session]:
return await session_repo.get_user_sessions(self.db, user_id, active_only=True)
return await session_repo.get_user_sessions(self.db, user_id, open_only=True)

async def count_user_sessions(self, user_id: UUID) -> int:
return await session_repo.count_user_sessions(self.db, user_id, active_only=True)
async def count_user_sessions(self, user_id: UUID, *, now: datetime | None = None) -> int:
return await session_repo.count_user_sessions(self.db, user_id, open_only=True, now=now)

async def validate_refresh_token(self, refresh_token: str) -> Session | None:
token_hash = _hash_token(refresh_token)
Expand Down Expand Up @@ -113,10 +113,14 @@ async def list_sessions(
the caller pages on it: revoking the last session on a page has to leave
the client able to work out that the page is gone.
"""
# One cutoff for both statements. A session lapsing between them would
# otherwise be in the page and outside the total, which breaks the
# invariant the caller pages on without anybody writing a row.
now = datetime.now(UTC)
sessions = await session_repo.get_user_sessions(
self.db, user_id, active_only=True, skip=skip, limit=limit
self.db, user_id, open_only=True, now=now, skip=skip, limit=limit
)
total = await self.count_user_sessions(user_id)
total = await self.count_user_sessions(user_id, now=now)
return SessionListResponse(
items=[
SessionRead(
Expand Down
35 changes: 25 additions & 10 deletions backend/app/services/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,17 +110,31 @@ async def has_any(self) -> bool:
async def admin_detail(self, user_id: UUID) -> AdminUserDetail:
"""Where this person has access, when they were last here, what is open.

Three reads rather than one because they are three tables, and they are
here rather than in the drawer because a client assembling them would
make three round trips to answer one question - and would have to know
that "no sessions ever" and "no sessions now" are different answers.
Four reads rather than one because they are three tables and two
questions, and they are here rather than in the drawer because a client
assembling them would make as many round trips to answer one question -
and would have to know that "no sessions ever" and "no sessions now" are
different answers.

**Two scopes, not one.** Where somebody was last seen is a fact about
every session they have ever had; how many are open is a fact about the
ones still usable. Reading both off the open set answered "Never signed
in" for anybody who had signed out - which is most accounts most of the
time, and the opposite of the truth on the field this drawer exists for
(#1256).

The user is fetched first so an unknown id is a 404 rather than an empty
detail about nobody.
"""
await self.get_by_id(user_id)
memberships = await organization_repo.list_for_user(self.db, user_id)
sessions = await session_repo.get_user_sessions(self.db, user_id, active_only=True)
# `limit=1` because the query is ordered most-recently-used first and the
# head is the whole answer. Nothing prunes this table - every refresh
# deactivates a row and inserts another - so a year-old account has
# thousands of rows and reading them all to look at one grows without
# bound.
last_used = await session_repo.get_user_sessions(self.db, user_id, open_only=False, limit=1)
Comment thread
DEENUU1 marked this conversation as resolved.
open_sessions = await session_repo.get_user_sessions(self.db, user_id, open_only=True)
return AdminUserDetail(
memberships=[
AdminUserMembership(
Expand All @@ -132,11 +146,12 @@ async def admin_detail(self, user_id: UUID) -> AdminUserDetail:
)
for organization, role in memberships
],
# The sessions come back most-recently-used first, so the head is
# both answers: when they were last here, and the newest one open.
last_seen_at=sessions[0].last_used_at if sessions else None,
active_sessions=len(sessions),
newest_session_at=max((s.created_at for s in sessions), default=None),
last_seen_at=last_used[0].last_used_at if last_used else None,
active_sessions=len(open_sessions),
# Of the open ones: it is read beside their count, and "newest
# session August" under "0 open sessions" is a sentence about
# nothing.
newest_session_at=max((s.created_at for s in open_sessions), default=None),
)

async def admin_list_with_counts(
Expand Down
19 changes: 19 additions & 0 deletions backend/tests/api/test_admin_user_detail.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,25 @@ async def test_last_seen_is_the_newest_activity_and_the_count_is_what_is_open()
assert body["newest_session_at"].startswith("2026-08-19T09:00")


async def test_the_two_figures_are_two_scopes_and_the_history_read_is_bounded() -> None:
"""Last-seen is every session; open is the usable ones. And the history read
takes one row: nothing prunes `sessions` - every refresh deactivates a row
and inserts another - so reading a year of them to look at the head grows
without bound."""
service = UserService(AsyncMock())
reads = AsyncMock(return_value=[_session(last_used=NOW, created=NOW)])
with (
patch.object(service, "get_by_id", new=AsyncMock()),
patch(f"{MODULE}.organization_repo.list_for_user", new=AsyncMock(return_value=[])),
patch(f"{MODULE}.session_repo.get_user_sessions", new=reads),
):
async with _client(service=service) as client:
await client.get(ENDPOINT)

scopes = [call.kwargs for call in reads.await_args_list]
assert scopes == [{"open_only": False, "limit": 1}, {"open_only": True}]


async def test_an_account_that_has_never_signed_in_says_so_rather_than_nothing() -> None:
"""`null` is not zero. A dormant account and one that was created and never
used are different decisions, and the drawer has to be able to tell them
Expand Down
119 changes: 119 additions & 0 deletions backend/tests/integration/test_admin_user_last_seen.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
"""When the admin drawer says somebody was last here, and what it calls open.

Both figures used to come off the same read - the user's *active* sessions - and
both were wrong for it. Somebody who has signed out has no active session row at
all, so "when were they last here" came back null and the drawer answered "Never
signed in" for most accounts most of the time: the one case the field exists to
tell apart from an account created and never used. And nothing sweeps a session
that simply ran out, so a row past `expires_at` stays `is_active` until the next
refresh declines it and was counted as open (#1256).

Here rather than in the unit suite because both halves are a `WHERE` clause: what
`is_active AND expires_at > now()` selects is Postgres's answer, not a mock's.
"""

from __future__ import annotations

import uuid
from datetime import UTC, datetime, timedelta

import pytest

from app.db.models.session import Session
from app.db.models.user import User
from app.services.user import UserService

pytestmark = pytest.mark.anyio

NOW = datetime(2026, 8, 20, 9, 0, tzinfo=UTC)


async def _user(db) -> User:
user = User(
id=uuid.uuid4(),
email=f"{uuid.uuid4().hex}@example.com",
hashed_password="x",
is_active=True,
created_at=NOW - timedelta(days=90),
)
db.add(user)
await db.flush()
return user


async def _session(
db,
user: User,
*,
last_used_at: datetime,
is_active: bool = True,
expires_at: datetime | None = None,
) -> Session:
session = Session(
id=uuid.uuid4(),
user_id=user.id,
refresh_token_hash=uuid.uuid4().hex,
is_active=is_active,
created_at=last_used_at,
last_used_at=last_used_at,
expires_at=expires_at or datetime.now(UTC) + timedelta(days=7),
)
db.add(session)
await db.flush()
return session


class TestLastSeenIsEverySession:
async def test_somebody_who_signed_out_has_still_been_here(self, db) -> None:
user = await _user(db)
await _session(db, user, last_used_at=NOW - timedelta(days=30), is_active=False)
last = NOW - timedelta(days=3)
await _session(db, user, last_used_at=last, is_active=False)

detail = await UserService(db).admin_detail(user.id)

assert detail.last_seen_at == last
assert detail.active_sessions == 0
# No open session, so nothing to date - the drawer reads this beside the
# count and "newest session August" under "0 open sessions" says nothing.
assert detail.newest_session_at is None

async def test_an_account_that_never_signed_in_is_still_null(self, db) -> None:
"""The distinction the field exists for, and the half that was already
right: no session ever is not the same as none open now."""
user = await _user(db)

detail = await UserService(db).admin_detail(user.id)

assert detail.last_seen_at is None
assert detail.active_sessions == 0


class TestOpenMeansUsable:
async def test_an_expired_row_is_not_an_open_session(self, db) -> None:
"""Nothing deactivates a session that lapses: the row stays `is_active`
until a refresh finds it expired and declines it."""
user = await _user(db)
await _session(
db,
user,
last_used_at=NOW - timedelta(days=40),
expires_at=datetime.now(UTC) - timedelta(days=1),
)

detail = await UserService(db).admin_detail(user.id)

assert detail.active_sessions == 0
assert detail.last_seen_at == NOW - timedelta(days=40)

async def test_an_unexpired_row_is(self, db) -> None:
user = await _user(db)
await _session(db, user, last_used_at=NOW - timedelta(days=40), is_active=False)
opened = NOW - timedelta(hours=2)
await _session(db, user, last_used_at=opened)

detail = await UserService(db).admin_detail(user.id)

assert detail.active_sessions == 1
assert detail.newest_session_at == opened
assert detail.last_seen_at == opened
Loading