perf(cli): avoid duplicate load_session in auth status (#146)#153
perf(cli): avoid duplicate load_session in auth status (#146)#153adriannoes wants to merge 2 commits into
Conversation
Always run detect_auth_source before using prefetched_session so bearer and service-account tiers are not bypassed; pass cached_stored_session into detection to preserve the single load_session contract for auth status.
gbrlcustodio
left a comment
gbrlcustodio
left a comment
There was a problem hiding this comment.
Optimization is sound and the second commit correctly preserves bearer/service-account precedence (locked in by test_prefetched_session_does_not_override_bearer_precedence). Main feedback inline.
chore: PR is CONFLICTING against dev — needs a rebase before merge (likely the describe_missing_oauth_vars → describe_missing_service_account_vars rename landing on a parallel branch).
| load_session, | ||
| ) | ||
|
|
||
| _SESSION_FRESHNESS_LEEWAY_S = 60 |
There was a problem hiding this comment.
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).
| if cached_stored_session is not None | ||
| else load_session( | ||
| issuer=auth.oidc_client.issuer_url, | ||
| client_id=auth.oidc_client.client_id, |
There was a problem hiding this comment.
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")| source = detect_auth_source( | ||
| pipefy_settings, | ||
| auth, | ||
| cached_stored_session=True if prefetched_session is not None else None, |
There was a problem hiding this comment.
nitpick (if-minor): cached_stored_session=prefetched_session is not None or None says the same thing without the ternary.
| AuthContext, | ||
| AuthSource, | ||
| OidcClient, | ||
| _session_is_fresh, |
There was a problem hiding this comment.
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.
| auth, | ||
| cached_stored_session=loaded_session is not None | ||
| if auth.oidc_client is not None | ||
| else None, |
There was a problem hiding this comment.
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.
| "pipefy_cli.oauth.refresh.load_session", | ||
| "pipefy_cli.commands.auth.load_session", | ||
| "pipefy_cli.auth.load_session", | ||
| ) |
There was a problem hiding this comment.
note (non-blocking): silently under-counts if a 5th importer is added later — the new site bypasses the counter and load_calls == 1 still holds. A docstring calling that out would be enough; not a blocker.
| "Refresh failed (HTTP 400): invalid_grant", error_code="invalid_grant" | ||
| ), | ||
| ), | ||
| patch("pipefy_cli.commands.auth._session_is_fresh", return_value=False), |
There was a problem hiding this comment.
suggestion: patching _session_is_fresh couples the test to a private helper. Seeding a stale session exercises the real freshness path instead:
_seed_session(monkeypatch, obtained_at=int(time.time()) - 7200)Same applies to lines 231 and 258. Folds away entirely if ensure_fresh_session(session=...) lands.
2 participants
Summary
prefetched_sessiontoget_authenticated_clientso the stored-session branch skips a secondensure_fresh_session/ keychain read when the caller already refreshed the session.pipefy auth statuspreloads the session once, reuses it for source detection and population, and passes the refreshed session into identity fetch.load_sessionperauth statusinvocation on the stored-session path.Test plan
uv run pytest packages/cli/tests/test_auth_status.py packages/cli/tests/test_auth.py -m "not integration"uv run ruff check/ruff formaton touched filesCloses #146