fix(ui): keep GUI auth status in sync via periodic polling - #1239
fix(ui): keep GUI auth status in sync via periodic polling#1239andychoquette wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
waiting-on-maintainers
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| """ | ||
| if self._runner.is_running("auth_status") or self._runner.is_running("creds_source"): | ||
| return | ||
| self.refresh_status(quiet=True) |
There was a problem hiding this comment.
The poll runs refresh_status(quiet=True), which calls the probes (get_credentials_source / check_authentication_status) through get_boto3_client → get_boto3_session(config=...) without force_refresh=True. Those resolve to the @lru_cached session (_get_boto3_session_for_profile).
This is deliberately different from the files_changed handler, which force-refreshes the session (_get_session_background → get_boto3_session(force_refresh=True)) precisely so out-of-process credential rewrites are picked up.
The motivating comment (lines 41-45 / 115-118) says the poll exists to catch credentials "changed out-of-process" and in-place rewrites the watcher misses. But for static credentials (HOST_PROVIDED access keys / session tokens in ~/.aws/credentials), boto3 reads and caches them in the session object on first use. An in-place rewrite — a previously-invalid creds file being repaired, or new keys swapped in — won't be re-read by the stale cached session, so the poll keeps reporting the old state until something else force-refreshes.
Pure expiry of an existing token is still detected (the API call rejects it), and DCM credential_process profiles self-refresh, so those cases work. The gap is specifically in-place changes to static credentials — part of what the poll is documented to cover. Consider force-refreshing the session in the quiet poll path so the backstop actually reflects on-disk changes.
e1ef41e to
7cb8fa7
Compare
| """ | ||
| if self._runner.is_running("auth_status") or self._runner.is_running("creds_source"): | ||
| return | ||
| self._refresh_status(quiet=True) |
There was a problem hiding this comment.
The poll re-probes with the cached boto3 session — it does not force_refresh, unlike the file-watcher path.
_poll_auth_status → _refresh_status(quiet=True) runs check_authentication_status/get_credentials_source, both of which call get_boto3_session() without force_refresh=True. That returns the lru_cached session (_get_boto3_session_for_profile), whose credentials are resolved/held in memory. By contrast, the files_changed path deliberately calls api.get_boto3_session(force_refresh=True) before refreshing.
Consequence for the scenarios this backstop is meant to cover (per the comment on lines 41-44 / 115-118):
- Credential expiry of refreshable (DCM
custom-process) creds is still caught, because botocore re-runs the credential process when the cached creds near expiry and a failure surfaces asNEEDS_LOGIN. ✅ - Out-of-process changes to static host-provided credentials (e.g. an in-place rewrite of
~/.aws/credentials, or a logout that theQFileSystemWatchermisses) are not picked up: the cached session keeps serving the stale in-memory creds, so the poll re-confirms the old state. ✗
If the intent is for the poll to reflect out-of-process credential changes that the watcher misses (which is the stated rationale), it likely needs to force a session-cache invalidation (e.g. get_boto3_session(force_refresh=True)) before re-probing.
7cb8fa7 to
fb2fecf
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
aefb0f2 to
8be049d
Compare
|
|
||
| status = auth_module._deadline_authentication_status | ||
| if status is not None: | ||
| timer = getattr(status, "_poll_timer", None) |
There was a problem hiding this comment.
This new autouse fixture runs for every test in the suite and, in _stop_auth_status_poll_timer(), does import deadline.client.ui.deadline_authentication_status. That module imports from qtpy.QtCore import ... at module top, which raises when no Qt binding (PySide6) is installed.
The GUI test conftest (test/unit/deadline_client/ui/gui/conftest.py) deliberately supports running the suite without PySide6 by collect_ignore-ing the Qt test files when importlib.util.find_spec("PySide6") is None. This root autouse fixture bypasses that guard: with the gui extra absent, the fixture setup would raise ImportError/QtBindingsNotFoundError and turn every test in the suite into an error, not just the GUI ones.
In practice the hatch test envs all set features = ["gui"], so PySide6 is always present in CI and this stays latent — but it makes the whole suite hard-depend on the optional GUI binding, contrary to the no-PySide6 path the gui conftest goes out of its way to preserve. Consider guarding the import (e.g. skip when importlib.util.find_spec("PySide6") is None, or wrap the import in a try/except ImportError).
65f2d0b to
c27c3e2
Compare
| """ | ||
| if self._runner.is_running("auth_status") or self._runner.is_running("creds_source"): | ||
| return | ||
| self._refresh_status(quiet=True) |
There was a problem hiding this comment.
Polling turns any transient probe failure into a spurious UI transition every 30s.
_poll_auth_status → _refresh_status(quiet=True) → check_authentication_status, which returns CONFIGURATION_ERROR (or NEEDS_LOGIN for a monitor profile) on any exception from the deadline:ListFarms probe — including transient network errors, timeouts, and throttling (_session.py:424). There is no retry or debounce.
Because _set_auth_status now emits only on change, a single transient failure while a user is steadily AUTHENTICATED will flip the UI to "Configuration error" / "Log in", then flip back on the next successful poll — a recurring flicker for anyone on a flaky network. Before this change the status was recomputed only on user-driven/file-change events, so this continuous exposure is new.
Consider only downgrading the cached AUTHENTICATED state on a definitive auth failure (e.g. distinguish credential/expiry errors from transient service/network errors), or requiring N consecutive quiet failures before emitting a regression, so a momentary blip does not repeatedly toggle the UI.
f200169 to
26fe6be
Compare
The GUI only re-checked authentication status reactively: on construction, on QFileSystemWatcher events for ~/.aws and ~/.deadline, and on explicit login/logout from a dialog. None of those fire when credentials expire in place or are changed out-of-process (e.g. a `deadline auth logout` from a terminal), so an open GUI kept showing "Authenticated" indefinitely. Add a QTimer to DeadlineAuthenticationStatus that re-probes auth status every 30s as a backstop, independent of the file watcher. The poll uses a quiet refresh that skips the reset-to-None (so widgets don't flash "Refreshing" each tick) and emits change signals only when a value actually differs, so a steady authenticated state produces no UI churn. A tick is skipped while a refresh is already in flight. The quiet poll force-refreshes the boto3 session before probing, so out-of-process credential changes the file watcher misses (an in-place rewrite of ~/.aws/credentials, or a logout) are re-read rather than validated against the stale cached session. To avoid a transient network or throttling blip flipping the UI to "Log in"/error and back every interval, a previously-AUTHENTICATED state is only downgraded after N consecutive quiet-poll failures; user-driven refreshes still surface errors immediately. Polling is ref-counted and scoped to the lifetime of a visible auth-status widget (started in the widget's constructor, released on its `destroyed` signal), so no background AWS probes run in a headless process. Add a root autouse test fixture that stops the poll timer before and after every test (guarded so it's a no-op when the optional GUI extra / PySide6 is absent), so the process-wide singleton's timer can't run alongside an unrelated test under pytest-xdist. Signed-off-by: Andy Choquette <78888816+andychoquette@users.noreply.github.com>
26fe6be to
6165019
Compare
| # after N consecutive failures, so a momentary blip does not flip the | ||
| # widget to "Log in"/"error" and back every poll interval. User-driven | ||
| # refreshes (quiet=False) still surface the error immediately. | ||
| if quiet and self.__auth_status == api.AwsAuthenticationStatus.AUTHENTICATED: |
There was a problem hiding this comment.
The transient-failure debounce added here is placed on the wrong handler, so it does not actually prevent the flicker it targets.
check_authentication_status (_session.py:420-430) catches every probe exception internally and returns AUTHENTICATED / NEEDS_LOGIN / CONFIGURATION_ERROR — it does not raise. So a transient network/throttling blip during the deadline:ListFarms probe resolves the background task successfully (with a CONFIGURATION_ERROR/NEEDS_LOGIN value) and flows through _on_auth_status_success, not _on_auth_status_error.
_on_auth_status_success unconditionally resets self.__consecutive_poll_failures = 0 and calls _set_auth_status(result, ...), which emits on change → the UI still flips AUTHENTICATED → "Log in"/"error" on a single transient blip and back on the next poll. The __consecutive_poll_failures counter here in _on_auth_status_error only ever increments if _refresh_auth_status itself raises (e.g. the get_boto3_session(force_refresh=True) call throws) — not for the ordinary probe-failure path the debounce is documented to cover.
To actually debounce transient probe failures, the consecutive-failure gating needs to run in the success handler based on the returned status (i.e. treat a quiet transition away from AUTHENTICATED as the thing to debounce), not only when the task errors.
2 participants
What was the problem/requirement? (What/Why)
When the GUI is left open and the Deadline authentication status changes to a non-authenticated state — credentials expiring in place, or being changed out-of-process (e.g. a
deadline auth logoutfrom a terminal) — the GUI did not update. It kept showing "Authenticated" indefinitely.The GUI only re-checked authentication status reactively:
DeadlineAuthenticationStatus,QFileSystemWatcherevents for~/.awsand~/.deadline,None of these fire for in-place credential expiry or out-of-process changes. In particular,
deadline auth logoutdelegates to Deadline Cloud Monitor, which invalidates a token cache without necessarily modifying a watched file, so the file watcher does not reliably detect it.What was the solution? (How)
Added a
QTimertoDeadlineAuthenticationStatusthat re-probes authentication status every 30s as a backstop, independent of the file watcher. To keep this from causing visible churn:refresh_status(quiet=True)) skips the reset-to-Noneso widgets don't flash "Refreshing" on every tick.All existing auth-status widgets (config/settings dialog, submit dialog) pick this up through the signals they already connect to — no widget changes needed.
What is the impact of this change?
While a GUI is open, the auth-status widgets reflect the true state within ~30s of a change, and the submit button enables/disables accordingly. Cost is a low-frequency
deadline:ListFarmsprobe every 30s while a GUI is open. No public API or CLI contract changes.How was this change tested?
Added unit tests in
test/unit/deadline_client/ui/gui/test_deadline_authentication_status.pycovering timer setup/interval, poll runs a quiet refresh, poll skipped when a refresh is in flight, quiet refresh preserves cached values, loud refresh clears them, no signal when status is unchanged, and signals fire on the AUTHENTICATED → NEEDS_LOGIN transition.Ran the new tests plus related dialog/auth GUI suites (
test_settings_dialogue.py,test_submit_job_to_deadline_dialog.py) — all pass.Manually verified in the GUI: logged in, opened the GUI, ran
deadline auth logoutfrom a separate terminal, and confirmed the widget flipped to the "Log in" prompt within the poll interval without interacting with the GUI.Have you run the unit tests? Yes.
Have you run the integration tests? No (change is GUI/auth-status logic, covered by unit tests).
Was this change documented?
refresh_statusdocuments the newquietparameter, and the new timer/quiet-refresh methods are documented.Does this PR introduce new dependencies?
Is this a breaking change?
No.
refresh_statusgains an optionalquietparameter that defaults to the prior behavior, so existing callers are unaffected.Does this change impact security?
No. This change does not create or modify files/directories, and does not change credential handling — it only re-runs the existing authentication-status probe on a timer.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.