diff --git a/backend/alembic/versions/0061_session_last_used_index.py b/backend/alembic/versions/0061_session_last_used_index.py new file mode 100644 index 000000000..8bcba601f --- /dev/null +++ b/backend/alembic/versions/0061_session_last_used_index.py @@ -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" +down_revision: str | None = "0060_conversation_favourites" +branch_labels: str | Sequence[str] | None = None +depends_on: str | Sequence[str] | None = None + +_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") diff --git a/backend/app/db/models/session.py b/backend/app/db/models/session.py index bd58b7c43..50fe59a04 100644 --- a/backend/app/db/models/session.py +++ b/backend/app/db/models/session.py @@ -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 @@ -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) diff --git a/backend/app/repositories/session.py b/backend/app/repositories/session.py index 77dece60e..1089671e1 100644 --- a/backend/app/repositories/session.py +++ b/backend/app/repositories/session.py @@ -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 @@ -25,11 +26,30 @@ 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]: @@ -37,10 +57,14 @@ async def get_user_sessions( `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` @@ -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() diff --git a/backend/app/services/session.py b/backend/app/services/session.py index fef7001c4..15a6dfebc 100644 --- a/backend/app/services/session.py +++ b/backend/app/services/session.py @@ -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) @@ -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( diff --git a/backend/app/services/user.py b/backend/app/services/user.py index 0e1fb5dc4..88e4b6e28 100644 --- a/backend/app/services/user.py +++ b/backend/app/services/user.py @@ -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) + open_sessions = await session_repo.get_user_sessions(self.db, user_id, open_only=True) return AdminUserDetail( memberships=[ AdminUserMembership( @@ -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( diff --git a/backend/tests/api/test_admin_user_detail.py b/backend/tests/api/test_admin_user_detail.py index 79a6553bb..37622e3f0 100644 --- a/backend/tests/api/test_admin_user_detail.py +++ b/backend/tests/api/test_admin_user_detail.py @@ -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 diff --git a/backend/tests/integration/test_admin_user_last_seen.py b/backend/tests/integration/test_admin_user_last_seen.py new file mode 100644 index 000000000..1a80fb353 --- /dev/null +++ b/backend/tests/integration/test_admin_user_last_seen.py @@ -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 diff --git a/backend/tests/test_sessions_pagination.py b/backend/tests/test_sessions_pagination.py index c10da50f7..a5b5a8bea 100644 --- a/backend/tests/test_sessions_pagination.py +++ b/backend/tests/test_sessions_pagination.py @@ -51,13 +51,20 @@ async def test_a_page_reports_the_total_not_its_own_length(service: SessionServi patch( "app.repositories.session.get_user_sessions", new=AsyncMock(return_value=page) ) as fetch, - patch("app.repositories.session.count_user_sessions", new=AsyncMock(return_value=17)), + patch( + "app.repositories.session.count_user_sessions", new=AsyncMock(return_value=17) + ) as count, ): result = await service.list_sessions(user_id, skip=10, limit=2) assert len(result.items) == 2 assert result.total == 17 - assert fetch.await_args.kwargs == {"active_only": True, "skip": 10, "limit": 2} + kwargs = fetch.await_args.kwargs + assert (kwargs["open_only"], kwargs["skip"], kwargs["limit"]) == (True, 10, 2) + # The same cutoff in both statements. A session lapsing between them would + # otherwise be in the page and outside the total, which is the invariant the + # client pages on breaking with nobody writing a row. + assert kwargs["now"] == count.await_args.kwargs["now"] async def test_the_page_is_what_the_caller_asked_for(service: SessionService) -> None: