Skip to content

fix(auth): memoize resolve_nous_access_token to collapse startup burst - #66016

Closed
JeffStone69 wants to merge 1 commit into
NousResearch:mainfrom
JeffStone69:fix/auth-token-resolve-memo
Closed

fix(auth): memoize resolve_nous_access_token to collapse startup burst#66016
JeffStone69 wants to merge 1 commit into
NousResearch:mainfrom
JeffStone69:fix/auth-token-resolve-memo

Conversation

@JeffStone69

Copy link
Copy Markdown
Contributor

Problem

check_tool_availability runs once per managed-tool check_fn (browser, image_gen, etc.) during banner render. Each one independently triggers a ~15s blocking Nous Portal token-refresh network call (resolve_nous_access_token -> _refresh_access_token -> httpx.post) when the stored token is expired.

On a slow/constrained host (a small monitoring CT observed this) that serial burst stretched hermes startup to many minutes, appearing "stalled" — the user had to Ctrl-C out of it. The httpx client already has a 15s connect timeout, so it isn't an infinite hang; it's N×15s where N is the number of managed-tool checks.

Fix

Add a per-process memo (5s TTL) to resolve_nous_access_token so the startup burst collapses into a single network round-trip. Only successful, non-forced resolutions are cached. force_fresh callers and insecure/ca_bundle callers bypass the cache and never populate it, so normal refresh semantics (interactive re-auth, custom TLS) are unchanged.

Verification

  • auth.py parses clean.
  • Direct test: 3 rapid resolve_nous_access_token() calls (against a fake expired-token state with the network refresh monkeypatched to count) -> exactly 1 underlying refresh, identical token returned all 3 times.
  • Caught and fixed a real UnboundLocalError (module global read before the later in-function assignment made Python treat it as local) by adding global _RESOLVE_TOKEN_CACHE.

Note

This addresses the startup slowness class. The separate exit-watchdog fix (PR #65998) covers shutdown wedges; they are different bugs.

check_tool_availability runs once per managed-tool check_fn (browser,
image_gen, etc.) during banner render. Each one independently triggers a
~15s blocking Nous Portal token-refresh network call when the stored token
is expired. On a slow/constrained host (e.g. a small monitoring CT) that
serial burst stretched startup to many minutes, appearing 'stalled'.

Add a per-process memo (5s TTL) so the burst collapses into a single network
round-trip. Only successful, non-forced resolutions are cached; force_fresh
and insecure/ca_bundle callers bypass and don't populate the cache, so
normal refresh semantics are unchanged.

Verified: 3 rapid resolve_nous_access_token() calls -> 1 underlying refresh.
@alt-glitch alt-glitch added type/perf Performance improvement or optimization P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard area/auth Authentication, OAuth, credential pools provider/nous Nous Research API (OAuth) labels Jul 17, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Comment

Memoization fix (27 lines). Simple caching of resolve_nous_access_token to collapse startup burst.

Checked diff — no security concerns, no hardcoded secrets, no test artifacts.

Looks good. No blocking issues.


Reviewed by Hermes Agent

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary (Re-review)

Verdict: LGTM / Comment

Memoization fix collapses startup burst of resolve_nous_access_token calls. Prior COMMENT review confirmed. No issues found.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for investigating the slow-start report. The current code does not support the proposed availability-path premise.

Problems

  • tools/managed_tool_gateway.py:75-93 documents and implements peek_nous_access_token() specifically so banner/status/provider availability scans avoid synchronous refresh. is_managed_tool_gateway_ready() selects that reader at tools/managed_tool_gateway.py:176-192; tests/tools/test_managed_tool_gateway.py:103-134 verifies an expired token produces no resolve_nous_access_token() call.
  • The new early return at hermes_cli/auth.py:5338-5343 precedes both _provider_state_transaction() and the existing refresh_skew_seconds check, so it can return a cached token despite changed persisted state or a caller requiring a stricter refresh window.

Suggested changes

  • Please re-scope from a reproduced current refresh path rather than the availability scan, and add a regression test for that concrete caller. Any cache must preserve the existing state and refresh-skew checks.

Automated hermes-sweeper review.

Comment thread hermes_cli/auth.py
# Memo: collapse the startup burst of managed-tool check_fns into one
# network refresh. Only cache a successful, non-forced resolution for a
# short window; force_fresh / error paths bypass and don't populate it.
if not insecure and ca_bundle is None:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This cache is consulted before _provider_state_transaction() and before _is_expiring(..., refresh_skew_seconds), so it can return a token after auth state changes or when this caller's requested skew requires a refresh. The documented banner availability path already uses peek_nous_access_token() and does not call this resolver.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 18, 2026
@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Merged via #76930 — thank you @JeffStone69. Your commit was cherry-picked, so you remain the author in git history.

The memo design was sound — correct scoping (insecure/ca_bundle bypass), thread-safe, refresh-failure never cached, and the 5s TTL is provably safe against the token's ≥120s refresh skew. The salvage extended it with one follow-up: populating the memo on the valid-token fast path too (the startup burst usually finds a valid token, and that return still paid the cross-process lock/read cost per check_fn), plus dedicated memo tests. Your core mechanism is unchanged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists provider/nous Nous Research API (OAuth) sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/perf Performance improvement or optimization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants