fix(auth): distinguish pending refresh during CLI startup - #913
Conversation
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed both commits through c2861fc, including the identity/token split, cross-process rotation locking, generation changes, cancellation/settlement, startup failure handling, read-only status, and diagnostic storage/redaction.
One P2 remains: the new continue-on-refresh-failure path reaches a banner that still interprets an unavailable token as being logged out. This misses an existing consumer of the distinction this PR introduces; details are inline.
Code-growth review: the patch is +1,031/-86, or +945 net. Separating inline test modules gives approximately +478 implementation lines, +433 test lines, and +34 documentation lines. The identity split and shared rotation-lock extraction have concrete purposes and retain the existing credential owner. The default bounded diagnostic sink also has a concrete purpose for intermittent incidents that cannot be investigated retroactively through opt-in CLI logging; it is not used as a second source of authentication state. The missing coverage is the failed token acquisition -> completed startup -> rendered authentication status path.
Non-blocking simplification: RotationDiagnostic::record contains no await points and only enqueues a detached blocking write. A synchronous record/enqueue method would remove the async wrapper and its call-site awaits, and make it clearer that returning does not mean the record has been written. This is a cleanup suggestion, not a separate correctness blocker.
Validation: the Test Suite and Static Checks workflows are successful for this exact head. I also inspected CI logs for the credential tests and CLI native-auth/startup unit tests. Local execution was unavailable because this review workspace has no Rust/Cargo toolchain; the banner finding is based on the concrete control/data flow, not a claimed end-to-end reproduction.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed the complete PR through 77c7b03 against base 62dc136, with particular attention to the change since my previous review at c2861fc. This approval supersedes my earlier request-changes decision for the startup-banner finding.
The P2 is addressed. startup_access_token now preserves AccessMiss instead of converting it to a display string. complete_session_startup retains the typed outcome, including any later acquisition failure, and passes its token-free projection to the banner. RefreshInProgress is rendered as "sign-in refreshing", reauthentication as "sign-in needs renewal", and unavailable/changed/signed-out states remain distinct. Bound account identity can supply the welcome text while an unsettled token stays withheld. Legacy authentication keeps its existing token-presence fallback. This extends the existing result flow without introducing a persistent UI authentication state machine.
Rechecked the generation-bound identity/provider path, refresh-lock waiting and reread, read-only status, cancellation/settlement, uncertain-response no-replay behavior, and diagnostic permissions, bounds and credential exclusion. No additional merge-blocking finding in this pass. Fresh credentials still avoid refresh-lock acquisition and rotation diagnostics.
The regression evidence is layered: the credential test holds the rotation lock past its test budget and checks RefreshInProgress; the native binding test preserves identity without exposing a pending token; the new isolated subprocess banner test checks both refreshing and reauthentication text, excludes "not logged in", and checks the account welcome. These cover the relevant owner boundaries. The new banner test captures subprocess output; it is not a complete 20-second startup/live-TUI reproduction, and should not be described as one.
Code-growth review: +1,123/-99, net +1,024: +490 in implementation files/sections (including comments), +497 tests, +37 documentation. The latest fix itself is +79 net: +12 implementation, +64 tests, +3 documentation. The typed propagation is small and justified. The shared credential owner and default bounded diagnostic sink retain the concrete purposes discussed in the first review. My earlier non-blocking simplification still applies: RotationDiagnostic::record has no await points, so making it a synchronous enqueue operation would remove unnecessary async/await plumbing.
Validation at submission: Static Checks passed for this exact head; the full Test Suite is still in progress. git diff --check passes and the review worktree is unchanged. Local Rust/Cargo execution and live issuer testing were unavailable. This is code-review approval; it does not claim the pending CI run has passed.
Merge Queue Status
This pull request spent 11 seconds in the queue, including 1 second running CI. Required conditions to merge
|
2 participants
Summary
Starting Astra while a native credential refresh is pending currently treats the pending flag alone as proof that rotation was interrupted and tells the user to log in again. Identity-only callers also lose the refreshing credential provider or local resume metadata. This change lets those callers retain the generation-bound identity while token acquisition waits for the existing refresh owner and re-reads the settled state.
Refreshing your sign-in…after one second without cancelling or restarting the credential request; preserve the distinction between a busy refresh, connectivity failure, and reauthentication.astra auth statusobserve the same rotation lock without refreshing tokens. Reportrefresh_in_progressfor a held lock andreauthentication_requiredfor pending intent observed after acquiring the lock.auth-refresh.jsonlfile is capped at 64 KiB; tokens and response bodies are excluded.Related issue
Related to matrixorigin/matrixflow#17987. This PR fixes the client-side startup misclassification and diagnostic gap; it does not claim that every production re-login report had the same cause or close the broader issue.
Change type
User and compatibility impact
Users with an active refresh wait for its existing owner instead of immediately receiving login advice. Normal fresh-token startup gains no artificial delay. Credential acquisition failures warn without preventing the interactive workbench from opening; native cloud initialization is skipped for that startup. A lock-wait timeout asks users to retry starting Astra. An abandoned or rejected rotation still requires login; an old refresh token is never replayed after an uncertain outcome.
astra auth status --jsonadds therefresh_in_progressstate. Native credential schema, issuer policy, server/database configuration, refresh limits, and refresh-token replay rules are unchanged. Abrupt process exit can still interrupt settlement; OS lock release does not erase durable pending intent. Requires a new Astra client build.Architecture and complexity delta
astra-credentials::nativeretains ownership of refresh locking, settlement, and local status;native_authseparates identity access from token access.Binding::snapshot, profile identity binding, native credential projection, owner auth snapshots, interactive startup, credential helpers, and native auth status.Verification
cargo test -p astra-credentials --lib: 35 passed, including live rotation status, caller cancellation, real subprocess termination/lock release, account switch while waiting, no replay after rejected/lost responses, diagnostic permissions/size/redaction.cargo test -p astra-cli --no-default-features --lib cli::native_auth::tests: 4 passed.cargo test -p astra-cli --no-default-features --lib cli::session::session_startup::tests: 19 passed.cargo test -p astra-cli --no-default-features --test native_auth_environment: 4 passed.cargo clippy -p astra-credentials --all-targets -- -D warnings: passed.cargo clippy -p astra-cli --no-default-features --lib --test native_auth_environment --no-deps -- -D warnings: passed.cargo fmt --all -- --checkandgit diff --check: passed.astra auth status --jsonagainst a pending refresh owned by another process. Startup identity, resume metadata, delayed progress, and settled-token acquisition are covered at their CLI owner boundary.__eh_framesize warning; tests still complete successfully.Final checklist