Skip to content

perf(cli): avoid duplicate load_session in auth status (#146) #153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
adriannoes wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

perf(cli): avoid duplicate load_session in auth status (#146) #153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4a843b0
perf(cli): avoid duplicate load_session in auth status (#146)
adriannoes May 21, 2026
7224cba
fix(cli): honor auth precedence with prefetched_session (#146)
adriannoes May 21, 2026
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
93 changes: 64 additions & 29 deletions packages/cli/src/pipefy_cli/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

from __future__ import annotations

import time
from dataclasses import dataclass
from typing import Literal, NoReturn

Expand All @@ -32,7 +33,14 @@
describe_missing_oauth_vars,
ensure_public_graphql_configured,
)
from pipefy_cli.oauth import RefreshError, ensure_fresh_session, load_session
from pipefy_cli.oauth import (
RefreshError,
StoredSession,
ensure_fresh_session,
load_session,
)

_SESSION_FRESHNESS_LEEWAY_S = 60

@gbrlcustodio gbrlcustodio May 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (non-blocking): duplicates _LEEWAY_S = 60 from oauth/refresh.py:13. If either drifts, the fresh-path predicate here and the stale-check inside ensure_fresh_session will silently disagree.

suggestion: have ensure_fresh_session accept an optional session: StoredSession | None = None and skip the inner load_session when supplied. Then _populate_stored_session just calls ensure_fresh_session(session=loaded_session, ...) — freshness predicate + leeway stay in one place, _session_is_fresh goes away, and the stale path collapses to 1 load too (currently still 2).


AuthSource = Literal[
"flag-token",
Expand Down Expand Up @@ -108,9 +116,19 @@ def _cache_key(
)


def _session_is_fresh(
session: StoredSession, *, leeway_s: int = _SESSION_FRESHNESS_LEEWAY_S
) -> bool:
"""Return whether the access token is outside the refresh leeway window."""
expires_in = session.expires_in or 0
return time.time() < session.obtained_at + expires_in - leeway_s


def detect_all_sources(
pipefy_settings: PipefySettings,
auth: AuthContext,
*,
cached_stored_session: bool | None = None,
) -> list[AuthSource]:
"""Return every configured credential source, highest-precedence first.

Expand All @@ -125,24 +143,31 @@ def detect_all_sources(
)
if not describe_missing_oauth_vars(pipefy_settings):
sources.append("service-account")
if (
auth.oidc_client is not None
and load_session(
issuer=auth.oidc_client.issuer_url,
client_id=auth.oidc_client.client_id,
if auth.oidc_client is not None:
has_stored = (
cached_stored_session
if cached_stored_session is not None
else load_session(
issuer=auth.oidc_client.issuer_url,
client_id=auth.oidc_client.client_id,

@gbrlcustodio gbrlcustodio May 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: correct (the is not None binds to load_session(...), not the whole ternary) but a re-read. Splitting helps:

if cached_stored_session is None:
    cached_stored_session = (
        load_session(issuer=auth.oidc_client.issuer_url, client_id=auth.oidc_client.client_id)
        is not None
    )
if cached_stored_session:
    sources.append("stored-session")

)
is not None
)
is not None
):
sources.append("stored-session")
if has_stored:
sources.append("stored-session")
return sources


def detect_auth_source(
pipefy_settings: PipefySettings,
auth: AuthContext,
*,
cached_stored_session: bool | None = None,
) -> AuthSource:
"""Return the precedence winner among configured sources, or ``"none"``."""
sources = detect_all_sources(pipefy_settings, auth)
sources = detect_all_sources(
pipefy_settings, auth, cached_stored_session=cached_stored_session
)
return sources[0] if sources else "none"


Expand All @@ -169,6 +194,8 @@ def _raise_internal_error(detail: str) -> NoReturn:
def get_authenticated_client(
pipefy_settings: PipefySettings,
auth: AuthContext,
*,
prefetched_session: StoredSession | None = None,
) -> PipefyClient:
"""Return a facade client using the highest-precedence available auth source.

Expand All @@ -179,6 +206,9 @@ def get_authenticated_client(
tier 4 (stored user session keyed by issuer + client id). Tier 3
(service-account client credentials) is resolved from
``pipefy_settings`` alone.
prefetched_session: When set (e.g. by ``pipefy auth status`` after a
single keychain read + refresh), skips a second
``ensure_fresh_session`` round-trip for the stored-session branch.

Returns:
Shared in-process instance when settings match a prior call; otherwise
Expand All @@ -196,7 +226,11 @@ def get_authenticated_client(
typer.echo(str(exc), err=True)
raise typer.Exit(2) from exc

source = detect_auth_source(pipefy_settings, auth)
source = detect_auth_source(
pipefy_settings,
auth,
cached_stored_session=True if prefetched_session is not None else None,

@gbrlcustodio gbrlcustodio May 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick (if-minor): cached_stored_session=prefetched_session is not None or None says the same thing without the ternary.

)

if source == "none":
typer.echo(_missing_auth_message(pipefy_settings), err=True)
Expand All @@ -216,24 +250,25 @@ def get_authenticated_client(
_raise_internal_error(
"auth source 'stored-session' detected but no OIDC client is configured."
)
try:
session = ensure_fresh_session(
issuer=auth.oidc_client.issuer_url,
client_id=auth.oidc_client.client_id,
)
except RefreshError as exc:
typer.echo(
f"Stored Pipefy session could not be refreshed: {exc}. "
"Run `pipefy auth login` to sign in again.",
err=True,
)
raise typer.Exit(2) from exc
# ``detect_auth_source`` already confirmed a session is present; if it
# vanished between the two reads, fall through to the missing-auth path.
if session is None:
typer.echo(_missing_auth_message(pipefy_settings), err=True)
raise typer.Exit(2)
effective_bearer = session.access_token
if prefetched_session is not None:
effective_bearer = prefetched_session.access_token
else:
try:
session = ensure_fresh_session(
issuer=auth.oidc_client.issuer_url,
client_id=auth.oidc_client.client_id,
)
except RefreshError as exc:
typer.echo(
f"Stored Pipefy session could not be refreshed: {exc}. "
"Run `pipefy auth login` to sign in again.",
err=True,
)
raise typer.Exit(2) from exc
if session is None:
typer.echo(_missing_auth_message(pipefy_settings), err=True)
raise typer.Exit(2)
effective_bearer = session.access_token

key = _cache_key(pipefy_settings, effective_bearer)
if _cached_client is not None and _cached_signature == key:
Expand Down
62 changes: 51 additions & 11 deletions packages/cli/src/pipefy_cli/commands/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
AuthContext,
AuthSource,
OidcClient,
_session_is_fresh,

@gbrlcustodio gbrlcustodio May 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: importing _session_is_fresh across module boundaries breaks the leading-underscore "module-private" convention. Folds away if ensure_fresh_session(session=...) absorbs the optimization — see the comment on auth.py:43.

detect_all_sources,
get_authenticated_client,
)
Expand All @@ -30,6 +31,7 @@
DiscoveryPolicy,
LoginError,
RefreshError,
StoredSession,
ensure_fresh_session,
keychain_backend_name,
load_session,
Expand Down Expand Up @@ -319,22 +321,37 @@ class _StatusExit(Exception):
stderr: str | None = None


def _populate_stored_session(report: AuthStatusReport, oidc: OidcClient) -> None:
"""Populate stored-session fields on ``report``; raise ``_StatusExit`` on failure."""
def _populate_stored_session(
report: AuthStatusReport,
oidc: OidcClient,
*,
loaded_session: StoredSession | None = None,
) -> StoredSession:
"""Populate stored-session fields on ``report``; raise ``_StatusExit`` on failure.

Returns the (possibly refreshed) session for reuse by :func:`_fetch_identity`.
"""
report.issuer = oidc.issuer_url
report.keychain_backend = keychain_backend_name()
try:
fresh_session = ensure_fresh_session(
issuer=oidc.issuer_url, client_id=oidc.client_id
)
if loaded_session is not None and _session_is_fresh(loaded_session):
fresh_session = loaded_session
else:
fresh_session = ensure_fresh_session(
issuer=oidc.issuer_url, client_id=oidc.client_id
)
except RefreshError as exc:
# Branch on the RFC 6749 ``error`` field, not on ``str(exc)``. The
# message text is for humans; ``error_code`` is the machine contract.
report.state = (
"refresh-expired" if exc.error_code == "invalid_grant" else "needs-login"
)
# Best-effort expiry from the pre-refresh blob so users see *why*.
stale = load_session(issuer=oidc.issuer_url, client_id=oidc.client_id)
stale = (
loaded_session
if loaded_session is not None
else load_session(issuer=oidc.issuer_url, client_id=oidc.client_id)
)
if stale is not None:
report.access_expires_at = _iso_expiry(stale.obtained_at, stale.expires_in)
report.refresh_expires_at = _iso_expiry(
Expand Down Expand Up @@ -363,13 +380,20 @@ def _populate_stored_session(report: AuthStatusReport, oidc: OidcClient) -> None
report.refresh_expires_at = _iso_expiry(
fresh_session.obtained_at, fresh_session.refresh_expires_in
)
return fresh_session


def _fetch_identity(
report: AuthStatusReport, settings: PipefySettings, auth: AuthContext
report: AuthStatusReport,
settings: PipefySettings,
auth: AuthContext,
*,
prefetched_session: StoredSession | None = None,
) -> None:
"""Populate ``report.identity``; raise ``_StatusExit`` on transport / 401."""
client = get_authenticated_client(settings, auth)
client = get_authenticated_client(
settings, auth, prefetched_session=prefetched_session
)
try:
report.identity = asyncio.run(client.get_me())
except TransportServerError as exc:
Expand Down Expand Up @@ -399,7 +423,19 @@ def auth_status(
) -> None:
"""Print which auth source is active, the authenticated identity, and session expiry."""
settings, auth = settings_and_auth_from_ctx(ctx)
detected = detect_all_sources(settings, auth)
loaded_session: StoredSession | None = None
if auth.oidc_client is not None:
loaded_session = load_session(
issuer=auth.oidc_client.issuer_url,
client_id=auth.oidc_client.client_id,
)
detected = detect_all_sources(
settings,
auth,
cached_stored_session=loaded_session is not None
if auth.oidc_client is not None
else None,

@gbrlcustodio gbrlcustodio May 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (if-minor): the if auth.oidc_client is not None else None arm never fires usefully — detect_all_sources skips the cached_stored_session consultation entirely when oidc_client is None (auth.py:146). Just:

cached_stored_session=loaded_session is not None,

is equivalent and one line shorter.

)
source: AuthSource = detected[0] if detected else "none"
report = AuthStatusReport(auth_source=source, detected_sources=detected)
# Surface masking env vars whenever a stored session exists — that's the
Expand All @@ -421,8 +457,12 @@ def auth_status(
"but no OIDC client is configured. Please file an issue."
),
)
_populate_stored_session(report, auth.oidc_client)
_fetch_identity(report, settings, auth)
prefetched = _populate_stored_session(
report, auth.oidc_client, loaded_session=loaded_session
)
_fetch_identity(report, settings, auth, prefetched_session=prefetched)
else:
_fetch_identity(report, settings, auth)
except _StatusExit as exit_:
_render(exit_.report, json_out=json_out)
if exit_.stderr and not json_out:
Expand Down
40 changes: 40 additions & 0 deletions packages/cli/tests/test_auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,46 @@ def test_oauth_client_creds_wins_over_stored_session(clean_pipefy_env):
mock_ensure.assert_not_called()


def test_prefetched_session_skips_ensure_fresh_session(clean_pipefy_env):
"""``prefetched_session`` bypasses a second keychain read on the stored-session path."""
settings = _public_only_settings()
session = _fresh_stored_session()
with (
patch("pipefy_cli.auth.PipefyClient") as mock_pc,
patch("pipefy_cli.auth.ensure_fresh_session") as mock_ensure,
):
mock_pc.return_value = MagicMock()
get_authenticated_client(
settings,
_auth(issuer_url=_ISSUER, client_id="pipefy-cli"),
prefetched_session=session,
)
mock_ensure.assert_not_called()
mock_pc.assert_called_once_with(settings, bearer_token=session.access_token)


def test_prefetched_session_does_not_override_bearer_precedence(clean_pipefy_env):
"""``prefetched_session`` must not bypass tiers 1/2 even when supplied."""
settings = _public_only_settings()
session = _fresh_stored_session()
with (
patch("pipefy_cli.auth.PipefyClient") as mock_pc,
patch("pipefy_cli.auth.ensure_fresh_session") as mock_ensure,
):
mock_pc.return_value = MagicMock()
get_authenticated_client(
settings,
_auth(
bearer_token="explicit-bearer",
issuer_url=_ISSUER,
client_id="pipefy-cli",
),
prefetched_session=session,
)
mock_pc.assert_called_once_with(settings, bearer_token="explicit-bearer")
mock_ensure.assert_not_called()


def test_stored_session_used_when_no_other_source(clean_pipefy_env):
"""Priority 4 activates when bearer absent AND OAuth triple incomplete."""
settings = _public_only_settings()
Expand Down
Loading