From 7cac175ec257eb6d55adc2dc39d566960af5b7d4 Mon Sep 17 00:00:00 2001 From: Simon Rosenberg <157206163+simonrosenberg@users.noreply.github.com> Date: Thu, 6 Aug 2026 18:57:56 +0200 Subject: [PATCH 1/3] fix(acp): recover credential monitor after transient errors Co-authored-by: openhands --- .../sdk/agent/acp_file_credentials.py | 40 +++++++++++++++---- tests/sdk/agent/test_acp_file_credentials.py | 32 ++++++++++++++- 2 files changed, 64 insertions(+), 8 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_file_credentials.py b/openhands-sdk/openhands/sdk/agent/acp_file_credentials.py index 443be8263f..42a6db4a75 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_file_credentials.py +++ b/openhands-sdk/openhands/sdk/agent/acp_file_credentials.py @@ -15,6 +15,7 @@ CredentialAuthorizationRejected, CredentialBindingError, CredentialConflict, + CredentialInvalidResponse, CredentialNeedsReauthentication, CredentialSyncError, ResolvedCredential, @@ -245,15 +246,21 @@ def _monitor_loop(self) -> None: value = self._read_stable(attempts=1) if value is not None: self._sync_value(value) - except (CredentialNeedsReauthentication, CredentialSyncError) as exc: + except ( + CredentialNeedsReauthentication, + CredentialConflict, + CredentialInvalidResponse, + ) as exc: self._set_error(exc) return + except CredentialSyncError as exc: + self._set_error(exc) + logger.warning("credential_binding_monitor_failed", exc_info=exc) except Exception as exc: self._set_error( CredentialSyncError("Codex credential monitoring failed.") ) logger.warning("credential_binding_monitor_failed", exc_info=exc) - return def _read_current(self) -> str | None: with self._lock: @@ -426,13 +433,32 @@ def _raise_sticky_error(self) -> None: def _refresh_authorization_state(self) -> None: revision = self._authorization_revision() - if revision is None: - return with self._lock: - if revision == self._binding_authorization_revision: + error = self._error + if ( + revision is not None + and revision != self._binding_authorization_revision + ): + self._binding_authorization_revision = revision + if isinstance(error, CredentialAuthorizationRejected): + self._error = None + return + if error is None or isinstance( + error, + ( + CredentialAuthorizationRejected, + CredentialConflict, + CredentialInvalidResponse, + CredentialNeedsReauthentication, + ), + ): return - self._binding_authorization_revision = revision - if isinstance(self._error, CredentialAuthorizationRejected): + try: + self._load() + except CredentialBindingError: + return + with self._lock: + if self._error is error: self._error = None def _authorization_revision(self) -> int | None: diff --git a/tests/sdk/agent/test_acp_file_credentials.py b/tests/sdk/agent/test_acp_file_credentials.py index 8983cc654e..ed8546bef4 100644 --- a/tests/sdk/agent/test_acp_file_credentials.py +++ b/tests/sdk/agent/test_acp_file_credentials.py @@ -98,9 +98,22 @@ def reauthorize(self) -> None: class FailingBinding(MemoryBinding): async def replace(self, expected_version: str, value: str) -> str: + self.replace_calls += 1 raise CredentialSyncError("unavailable") +class FlakyBinding(MemoryBinding): + def __init__(self, value: str) -> None: + super().__init__(value) + self.first_replace_failed = threading.Event() + + async def replace(self, expected_version: str, value: str) -> str: + if not self.first_replace_failed.is_set(): + self.first_replace_failed.set() + raise CredentialSyncError("temporarily unavailable") + return await super().replace(expected_version, value) + + def _lifecycle(binding: MemoryBinding, registry: SecretRegistry): lifecycle = create_file_credential_lifecycle( CODEX_AUTH_SECRET_NAME, @@ -239,6 +252,22 @@ def test_unstable_read_does_not_poison_lifecycle() -> None: lifecycle.close() +def test_monitor_recovers_after_transient_writeback_failure() -> None: + rotated = _auth("refresh-r1") + binding = FlakyBinding(_auth("refresh-r0")) + lifecycle, _ = _lifecycle(binding, SecretRegistry()) + assert lifecycle.path is not None + runtime = cast(Any, lifecycle) + try: + lifecycle.path.write_text(rotated, encoding="utf-8") + assert binding.first_replace_failed.wait(2) + assert runtime._monitor.is_alive() + _wait_for_value(binding, rotated) + lifecycle.flush() + finally: + lifecycle.close() + + def test_unchanged_file_does_not_write() -> None: binding = MemoryBinding(_auth("refresh-r0")) lifecycle, _ = _lifecycle(binding, SecretRegistry()) @@ -259,7 +288,7 @@ def test_ambiguous_committed_write_converges() -> None: lifecycle.close() -def test_exhausted_writeback_failure_is_sticky() -> None: +def test_writeback_failure_is_retried_after_successful_load() -> None: binding = FailingBinding(_auth("refresh-r0")) lifecycle, _ = _lifecycle(binding, SecretRegistry()) assert lifecycle.path is not None @@ -274,6 +303,7 @@ def test_exhausted_writeback_failure_is_sticky() -> None: with pytest.raises(CredentialSyncError, match="unavailable"): lifecycle.close() + assert binding.replace_calls == 3 assert runtime_dir.exists() lifecycle.discard() assert not runtime_dir.exists() From da457df55fbdf8a0860be20f35ef24053e39ddce Mon Sep 17 00:00:00 2001 From: Simon Rosenberg <157206163+simonrosenberg@users.noreply.github.com> Date: Fri, 7 Aug 2026 13:50:27 +0200 Subject: [PATCH 2/3] chore: address PR review feedback (#4403) Co-authored-by: openhands --- .../sdk/agent/acp_file_credentials.py | 10 ++++++++-- tests/sdk/agent/test_acp_file_credentials.py | 20 +++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_file_credentials.py b/openhands-sdk/openhands/sdk/agent/acp_file_credentials.py index 42a6db4a75..046a6ef721 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_file_credentials.py +++ b/openhands-sdk/openhands/sdk/agent/acp_file_credentials.py @@ -239,6 +239,7 @@ def _cleanup_runtime(self) -> None: self._closed = True def _monitor_loop(self) -> None: + failure_logged = False while not self._stop.wait(_MONITOR_INTERVAL_SECONDS): try: with self._sync_lock: @@ -246,6 +247,7 @@ def _monitor_loop(self) -> None: value = self._read_stable(attempts=1) if value is not None: self._sync_value(value) + failure_logged = False except ( CredentialNeedsReauthentication, CredentialConflict, @@ -255,12 +257,16 @@ def _monitor_loop(self) -> None: return except CredentialSyncError as exc: self._set_error(exc) - logger.warning("credential_binding_monitor_failed", exc_info=exc) + if not failure_logged: + logger.warning("credential_binding_monitor_failed", exc_info=exc) + failure_logged = True except Exception as exc: self._set_error( CredentialSyncError("Codex credential monitoring failed.") ) - logger.warning("credential_binding_monitor_failed", exc_info=exc) + if not failure_logged: + logger.warning("credential_binding_monitor_failed", exc_info=exc) + failure_logged = True def _read_current(self) -> str | None: with self._lock: diff --git a/tests/sdk/agent/test_acp_file_credentials.py b/tests/sdk/agent/test_acp_file_credentials.py index ed8546bef4..b2e22dea34 100644 --- a/tests/sdk/agent/test_acp_file_credentials.py +++ b/tests/sdk/agent/test_acp_file_credentials.py @@ -268,6 +268,26 @@ def test_monitor_recovers_after_transient_writeback_failure() -> None: lifecycle.close() +def test_monitor_logs_persistent_writeback_failure_once() -> None: + binding = FailingBinding(_auth("refresh-r0")) + lifecycle, _ = _lifecycle(binding, SecretRegistry()) + assert lifecycle.path is not None + runtime = cast(Any, lifecycle) + try: + with patch( + "openhands.sdk.agent.acp_file_credentials.logger.warning" + ) as warning: + lifecycle.path.write_text(_auth("refresh-r1"), encoding="utf-8") + deadline = time.monotonic() + 2 + while binding.replace_calls < 2 and time.monotonic() < deadline: + time.sleep(0.02) + assert binding.replace_calls >= 2 + assert runtime._monitor.is_alive() + assert warning.call_count == 1 + finally: + lifecycle.discard() + + def test_unchanged_file_does_not_write() -> None: binding = MemoryBinding(_auth("refresh-r0")) lifecycle, _ = _lifecycle(binding, SecretRegistry()) From 407ee4f0ed1dea2c1f0c3749096d69e34411ea34 Mon Sep 17 00:00:00 2001 From: Simon Rosenberg <157206163+simonrosenberg@users.noreply.github.com> Date: Fri, 7 Aug 2026 13:56:58 +0200 Subject: [PATCH 3/3] chore: address PR review feedback (#4403) Co-authored-by: openhands --- .../sdk/agent/acp_file_credentials.py | 23 +++++++- tests/sdk/agent/test_acp_file_credentials.py | 52 +++++++++++++++++++ 2 files changed, 74 insertions(+), 1 deletion(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_file_credentials.py b/openhands-sdk/openhands/sdk/agent/acp_file_credentials.py index 046a6ef721..7a96f94e86 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_file_credentials.py +++ b/openhands-sdk/openhands/sdk/agent/acp_file_credentials.py @@ -31,6 +31,7 @@ _CHATGPT_AUTH_PATH = Path(".codex") / "auth.json" _MONITOR_INTERVAL_SECONDS = 0.1 +_MONITOR_MAX_RETRY_INTERVAL_SECONDS = 5.0 _MONITOR_JOIN_TIMEOUT_SECONDS = 2.0 _STABLE_READ_DELAY_SECONDS = 0.01 _SYNC_RETRY_DELAYS: tuple[float, ...] = (0.1, 0.5) @@ -240,7 +241,8 @@ def _cleanup_runtime(self) -> None: def _monitor_loop(self) -> None: failure_logged = False - while not self._stop.wait(_MONITOR_INTERVAL_SECONDS): + retry_interval = _MONITOR_INTERVAL_SECONDS + while not self._stop.wait(retry_interval): try: with self._sync_lock: self._raise_sticky_error() @@ -248,6 +250,7 @@ def _monitor_loop(self) -> None: if value is not None: self._sync_value(value) failure_logged = False + retry_interval = _MONITOR_INTERVAL_SECONDS except ( CredentialNeedsReauthentication, CredentialConflict, @@ -260,6 +263,10 @@ def _monitor_loop(self) -> None: if not failure_logged: logger.warning("credential_binding_monitor_failed", exc_info=exc) failure_logged = True + retry_interval = min( + retry_interval * 2, + _MONITOR_MAX_RETRY_INTERVAL_SECONDS, + ) except Exception as exc: self._set_error( CredentialSyncError("Codex credential monitoring failed.") @@ -267,6 +274,10 @@ def _monitor_loop(self) -> None: if not failure_logged: logger.warning("credential_binding_monitor_failed", exc_info=exc) failure_logged = True + retry_interval = min( + retry_interval * 2, + _MONITOR_MAX_RETRY_INTERVAL_SECONDS, + ) def _read_current(self) -> str | None: with self._lock: @@ -461,6 +472,16 @@ def _refresh_authorization_state(self) -> None: return try: self._load() + except ( + CredentialAuthorizationRejected, + CredentialConflict, + CredentialInvalidResponse, + CredentialNeedsReauthentication, + ) as exc: + with self._lock: + if self._error is error: + self._error = exc + return except CredentialBindingError: return with self._lock: diff --git a/tests/sdk/agent/test_acp_file_credentials.py b/tests/sdk/agent/test_acp_file_credentials.py index b2e22dea34..f76361941c 100644 --- a/tests/sdk/agent/test_acp_file_credentials.py +++ b/tests/sdk/agent/test_acp_file_credentials.py @@ -85,9 +85,11 @@ def __init__(self, value: str) -> None: super().__init__(value) self.authorization_revision = 0 self.rejected = True + self.rejection_observed = threading.Event() async def replace(self, expected_version: str, value: str) -> str: if self.rejected: + self.rejection_observed.set() raise CredentialAuthorizationRejected("rejected") return await super().replace(expected_version, value) @@ -114,6 +116,25 @@ async def replace(self, expected_version: str, value: str) -> str: return await super().replace(expected_version, value) +class DisappearingBinding(MemoryBinding): + def __init__(self, value: str) -> None: + super().__init__(value) + self.failed_replace = False + self.failed_loads = 0 + + async def load(self) -> ResolvedCredential: + if not self.failed_replace: + return await super().load() + self.failed_loads += 1 + if self.failed_loads == 1: + raise CredentialSyncError("unavailable") + raise CredentialNeedsReauthentication("missing") + + async def replace(self, expected_version: str, value: str) -> str: + self.failed_replace = True + raise CredentialSyncError("unavailable") + + def _lifecycle(binding: MemoryBinding, registry: SecretRegistry): lifecycle = create_file_credential_lifecycle( CODEX_AUTH_SECRET_NAME, @@ -288,6 +309,21 @@ def test_monitor_logs_persistent_writeback_failure_once() -> None: lifecycle.discard() +def test_monitor_stops_when_recovery_probe_requires_reauthentication() -> None: + binding = DisappearingBinding(_auth("refresh-r0")) + lifecycle, _ = _lifecycle(binding, SecretRegistry()) + assert lifecycle.path is not None + runtime = cast(Any, lifecycle) + lifecycle.path.write_text(_auth("refresh-r1"), encoding="utf-8") + assert runtime._monitor is not None + runtime._monitor.join(timeout=2) + + assert not runtime._monitor.is_alive() + with pytest.raises(CredentialNeedsReauthentication, match="missing"): + lifecycle.flush() + lifecycle.discard() + + def test_unchanged_file_does_not_write() -> None: binding = MemoryBinding(_auth("refresh-r0")) lifecycle, _ = _lifecycle(binding, SecretRegistry()) @@ -381,6 +417,22 @@ def test_reauthorization_clears_authorization_rejection() -> None: lifecycle.close() +def test_monitor_recovers_after_reauthorization() -> None: + rotated = _auth("refresh-r1") + binding = RevokedBinding(_auth("refresh-r0")) + lifecycle, _ = _lifecycle(binding, SecretRegistry()) + assert lifecycle.path is not None + runtime = cast(Any, lifecycle) + try: + lifecycle.path.write_text(rotated, encoding="utf-8") + assert binding.rejection_observed.wait(2) + assert runtime._monitor.is_alive() + binding.reauthorize() + _wait_for_value(binding, rotated) + finally: + lifecycle.close() + + def test_runtime_state_does_not_serialize_binding_values() -> None: secret = _auth("never-serialize") binding = MemoryBinding(secret)