From 473b909c145a8628deec2005affba70bf850babc Mon Sep 17 00:00:00 2001 From: Ankush Gola <9536492+agola11@users.noreply.github.com> Date: Fri, 10 Jul 2026 22:19:19 +0000 Subject: [PATCH 1/4] fix(code): avoid repeated startup auto-update stalls Co-authored-by: open-swe[bot] --- libs/code/deepagents_code/main.py | 23 ++++- libs/code/deepagents_code/update_check.py | 98 +++++++++++++++++-- libs/code/tests/unit_tests/test_main.py | 52 +++++++++- .../tests/unit_tests/test_update_check.py | 85 ++++++++++++++++ 4 files changed, 246 insertions(+), 12 deletions(-) diff --git a/libs/code/deepagents_code/main.py b/libs/code/deepagents_code/main.py index 099a4174ce7..c3cf290788e 100644 --- a/libs/code/deepagents_code/main.py +++ b/libs/code/deepagents_code/main.py @@ -276,6 +276,7 @@ def _run_startup_auto_update(console: "Console") -> None: from deepagents_code._version import __version__ as cli_version from deepagents_code.config import _is_editable_install from deepagents_code.update_check import ( + clear_startup_auto_update_failure, create_update_log_path, detect_shadowed_dcode_safe, format_release_age_parenthetical, @@ -285,9 +286,11 @@ def _run_startup_auto_update(console: "Console") -> None: is_installed_version_at_least, is_update_check_enabled, mark_auto_update_default_acknowledged, + mark_startup_auto_update_failed, perform_upgrade, release_requires_prereleases, should_announce_auto_update_default, + should_skip_startup_auto_update_after_failure, upgrade_command, ) @@ -339,6 +342,19 @@ def _run_startup_auto_update(console: "Console") -> None: highlight=False, ) return + if should_skip_startup_auto_update_after_failure(latest): + update_needs_prereleases = release_requires_prereleases(latest) + cmd = upgrade_command( + include_prereleases=True if update_needs_prereleases else None, + version=latest if update_needs_prereleases else None, + ) + console.print( + f"[bold yellow]Warning:[/bold yellow] Skipping automatic update to " + f"v{latest} after a recent failed attempt. Update manually: " + f"[cyan]{cmd}[/cyan]\nContinuing with v{cli_version}.", + highlight=False, + ) + return if should_announce_auto_update_default(): # First-run consent/migration: auto-update is on only because of the # opt-out default, not an explicit choice. Announce it once and skip @@ -386,6 +402,7 @@ def _run_startup_auto_update(console: "Console") -> None: perform_upgrade(log_path=log_path, target_version=latest) ) if success: + clear_startup_auto_update_failure(latest) # If a stale `dcode` is earlier on PATH, the auto-restart would # re-exec into the old binary and the user would silently keep # running the pre-upgrade version. Detect that *before* the @@ -434,6 +451,7 @@ def _run_startup_auto_update(console: "Console") -> None: highlight=False, ) return + mark_startup_auto_update_failed(latest) update_needs_prereleases = release_requires_prereleases(latest) cmd = upgrade_command( include_prereleases=True if update_needs_prereleases else None, @@ -3636,7 +3654,9 @@ def cli_main() -> None: sys.exit(130) sys.exit(exit_code) else: - _run_startup_auto_update(console) + resume_thread = args.resume_thread # "__MOST_RECENT__", "", or None + if resume_thread is None: + _run_startup_auto_update(console) # Resolve recent-agent fallback only for actual session launches. assistant_id = _resolve_agent_arg(args) # Interactive mode - handle thread resume @@ -3647,7 +3667,6 @@ def cli_main() -> None: # Instead of resolving thread_id here with synchronous asyncio.run() # DB calls, pass the raw resume request to the TUI and let it # resolve asynchronously during startup. - resume_thread = args.resume_thread # "__MOST_RECENT__", "", or None thread_id = None if resume_thread else generate_thread_id() # Validate sandbox provider deps before spawning server subprocess diff --git a/libs/code/deepagents_code/update_check.py b/libs/code/deepagents_code/update_check.py index a5497ff9318..73f7ddd6143 100644 --- a/libs/code/deepagents_code/update_check.py +++ b/libs/code/deepagents_code/update_check.py @@ -20,6 +20,7 @@ import re import shlex import shutil +import signal import sys import tempfile import time @@ -150,6 +151,16 @@ UPDATE_LOG_MAX_FILES = 10 """Keep at most this many newest update logs.""" +STARTUP_AUTO_UPDATE_FAILURE_COOLDOWN = CACHE_TTL +"""Seconds to suppress same-version startup auto-update retries after failure.""" + +_STARTUP_AUTO_UPDATE_FAILED_VERSION_KEY = "startup_auto_update_failed_version" +_STARTUP_AUTO_UPDATE_FAILED_AT_KEY = "startup_auto_update_failed_at" +_STARTUP_AUTO_UPDATE_FAILURE_KEYS: tuple[str, ...] = ( + _STARTUP_AUTO_UPDATE_FAILED_VERSION_KEY, + _STARTUP_AUTO_UPDATE_FAILED_AT_KEY, +) + UpgradeProgressCallback = Callable[[str], Awaitable[None] | None] @@ -928,6 +939,40 @@ def _write_update_state( return True +def should_skip_startup_auto_update_after_failure(version: str) -> bool: + """Return whether startup auto-update should skip a recently failed version.""" + data = _read_update_state() + failed_version = data.get(_STARTUP_AUTO_UPDATE_FAILED_VERSION_KEY) + failed_at = data.get(_STARTUP_AUTO_UPDATE_FAILED_AT_KEY) + return bool( + failed_version == version + and isinstance(failed_at, (int, float)) + and time.time() - failed_at < STARTUP_AUTO_UPDATE_FAILURE_COOLDOWN + ) + + +def mark_startup_auto_update_failed(version: str) -> bool: + """Persist a same-version startup auto-update retry cooldown marker. + + Returns: + `True` if the marker was written, `False` otherwise. + """ + return _write_update_state( + { + _STARTUP_AUTO_UPDATE_FAILED_VERSION_KEY: version, + _STARTUP_AUTO_UPDATE_FAILED_AT_KEY: time.time(), + } + ) + + +def clear_startup_auto_update_failure(version: str) -> None: + """Clear a startup auto-update failure marker for `version` if present.""" + data = _read_update_state() + if data.get(_STARTUP_AUTO_UPDATE_FAILED_VERSION_KEY) != version: + return + _write_update_state({}, remove_keys=_STARTUP_AUTO_UPDATE_FAILURE_KEYS) + + def should_notify_update(latest: str) -> bool: """Return whether the user should be notified about version *latest*. @@ -1544,6 +1589,25 @@ async def _read_stream( await _emit_progress(progress, line) +async def _terminate_install_process(proc: asyncio.subprocess.Process) -> None: + """Terminate an install subprocess and its descendants when possible.""" + if proc.returncode is not None: + return + if os.name == "posix": + try: + os.killpg(proc.pid, signal.SIGKILL) + except ProcessLookupError: + pass + except PermissionError: + with suppress(ProcessLookupError): + proc.kill() + else: + with suppress(ProcessLookupError): + proc.kill() + with suppress(ProcessLookupError): + await proc.wait() + + async def _run_install_subprocess( cmd: str, *, @@ -1568,6 +1632,9 @@ async def _run_install_subprocess( Returns: `(success, output)` — *success* is `True` iff the subprocess exited 0. + + Raises: + asyncio.CancelledError: If the calling task is cancelled. """ timeout = _UPGRADE_TIMEOUT if log_path is None: @@ -1591,12 +1658,21 @@ async def _run_install_subprocess( log_file = None try: - proc = await asyncio.create_subprocess_shell( - cmd, - stdout=asyncio.subprocess.PIPE, - stderr=asyncio.subprocess.PIPE, - stdin=asyncio.subprocess.DEVNULL, - ) + if os.name == "posix": + proc = await asyncio.create_subprocess_shell( + cmd, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + stdin=asyncio.subprocess.DEVNULL, + start_new_session=True, + ) + else: + proc = await asyncio.create_subprocess_shell( + cmd, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, + stdin=asyncio.subprocess.DEVNULL, + ) await asyncio.wait_for( asyncio.gather( _read_stream( @@ -1617,8 +1693,7 @@ async def _run_install_subprocess( ) except TimeoutError: if proc is not None: - proc.kill() - await proc.wait() + await _terminate_install_process(proc) msg = f"Command timed out after {timeout}s: {cmd}" if log_file is not None: with suppress(OSError): @@ -1627,6 +1702,13 @@ async def _run_install_subprocess( await _emit_progress(progress, msg) logger.warning(msg) return False, msg + except asyncio.CancelledError: + if proc is not None: + await _terminate_install_process(proc) + if log_file is not None: + with suppress(OSError): + log_file.close() + raise except OSError as exc: if log_file is not None: with suppress(OSError): diff --git a/libs/code/tests/unit_tests/test_main.py b/libs/code/tests/unit_tests/test_main.py index 69fb63238ac..ae949ddcbc6 100644 --- a/libs/code/tests/unit_tests/test_main.py +++ b/libs/code/tests/unit_tests/test_main.py @@ -123,6 +123,9 @@ def test_successful_update_restarts_before_launch(self) -> None: return_value=Path("/tmp/dcode-update.log"), ), patch("deepagents_code.update_check.perform_upgrade", upgrade), + patch( + "deepagents_code.update_check.clear_startup_auto_update_failure" + ) as clear_failure, patch( "deepagents_code.main._restart_current_process", side_effect=SystemExit(0), @@ -132,6 +135,7 @@ def test_successful_update_restarts_before_launch(self) -> None: _run_startup_auto_update(console) upgrade.assert_awaited_once() + clear_failure.assert_called_once_with("9.9.9") printed = " ".join(str(c.args[0]) for c in console.print.call_args_list) assert "tail -f /tmp/dcode-update.log" in printed restart.assert_called_once_with() @@ -292,16 +296,56 @@ def test_failed_update_does_not_restart_and_continues(self) -> None: return_value="uv tool upgrade deepagents-code", ), patch("deepagents_code.update_check.perform_upgrade", upgrade), + patch( + "deepagents_code.update_check.mark_startup_auto_update_failed" + ) as mark_failed, patch("deepagents_code.main._restart_current_process") as restart, ): - # Must not raise: a failed upgrade falls through to launch. _run_startup_auto_update(console) upgrade.assert_awaited_once() + mark_failed.assert_called_once_with("9.9.9") restart.assert_not_called() printed = " ".join(str(c.args[0]) for c in console.print.call_args_list) assert "Auto-update failed" in printed + def test_recent_failure_cooldown_skips_startup_update(self) -> None: + """A same-version startup failure cooldown must bypass repeat attempts.""" + console = MagicMock() + + with ( + patch("deepagents_code.config._is_editable_install", return_value=False), + patch( + "deepagents_code.update_check.is_update_check_enabled", + return_value=True, + ), + patch( + "deepagents_code.update_check.is_auto_update_enabled", + return_value=True, + ), + patch( + "deepagents_code.update_check.get_cached_update_available", + return_value=(True, "9.9.9"), + ), + patch( + "deepagents_code.update_check.should_skip_startup_auto_update_after_failure", + return_value=True, + ) as should_skip, + patch( + "deepagents_code.update_check.upgrade_command", + return_value="uv tool install -U deepagents-code", + ), + patch("deepagents_code.update_check.perform_upgrade") as upgrade, + patch("deepagents_code.main._restart_current_process") as restart, + ): + _run_startup_auto_update(console) + + should_skip.assert_called_once_with("9.9.9") + upgrade.assert_not_called() + restart.assert_not_called() + printed = " ".join(str(c.args[0]) for c in console.print.call_args_list) + assert "recent failed attempt" in printed + def test_editable_install_skips_update(self) -> None: """Editable installs must short-circuit before any PyPI/install work.""" console = MagicMock() @@ -801,7 +845,11 @@ def test_startup_auto_update_wired_into_interactive_launch(self) -> None: no-op. """ source = inspect.getsource(cli_main) - assert "_run_startup_auto_update(console)" in source + expected = ( + "if resume_thread is None:\n" + " _run_startup_auto_update(console)" + ) + assert expected in source class TestAutoUpdateDefaultMigration: diff --git a/libs/code/tests/unit_tests/test_update_check.py b/libs/code/tests/unit_tests/test_update_check.py index c5bb44ec2d7..1c146e499af 100644 --- a/libs/code/tests/unit_tests/test_update_check.py +++ b/libs/code/tests/unit_tests/test_update_check.py @@ -6,6 +6,7 @@ import logging import os import shlex +import signal import sys import time import tomllib @@ -33,6 +34,7 @@ _requires_prerelease_dependency, _uv_tool_bin_dir, cleanup_update_logs, + clear_startup_auto_update_failure, clear_update_notified, create_update_log_path, dependency_refresh_command, @@ -71,6 +73,7 @@ is_valid_extra_name, is_valid_package_name, mark_auto_update_default_acknowledged, + mark_startup_auto_update_failed, mark_update_notified, mark_version_seen, parse_dependency_changes, @@ -84,6 +87,7 @@ set_auto_update, should_announce_auto_update_default, should_notify_update, + should_skip_startup_auto_update_after_failure, upgrade_command, upgrade_install_command, ) @@ -3847,6 +3851,33 @@ async def test_timeout_kills_process(self, tmp_path) -> None: assert success is False assert "timed out" in output + async def test_timeout_kills_process_group_on_posix(self, tmp_path) -> None: + """Timeout cleanup terminates shell descendants on POSIX.""" + if os.name != "posix": + pytest.skip("process groups are POSIX-specific") + log_path = tmp_path / "install.log" + with ( + patch("deepagents_code.update_check._UPGRADE_TIMEOUT", 0.05), + patch( + "deepagents_code.update_check.detect_install_method", + return_value="uv", + ), + patch( + "deepagents_code.update_check.shutil.which", + return_value="/usr/bin/uv", + ), + patch( + "deepagents_code.update_check._install_extra_uv_tool_command", + return_value="sleep 5", + ), + patch("deepagents_code.update_check.os.killpg", wraps=os.killpg) as killpg, + ): + success, output = await perform_install_extra("quickjs", log_path=log_path) + assert success is False + assert "timed out" in output + killpg.assert_called_once() + assert killpg.call_args.args[1] == signal.SIGKILL + async def test_oserror_includes_exception_detail(self, tmp_path) -> None: """An OSError during exec must surface the exception class + message.""" log_path = tmp_path / "install.log" @@ -4454,6 +4485,60 @@ def test_mark_tolerates_write_failure(self, config_path, tmp_path) -> None: # n assert mark_auto_update_default_acknowledged() is False +class TestStartupAutoUpdateFailureCooldown: + @pytest.fixture + def state_file(self, tmp_path): + """Override UPDATE_STATE_FILE to use a temporary file.""" + path = tmp_path / "update_state.json" + with patch("deepagents_code.update_check.UPDATE_STATE_FILE", path): + yield path + + def test_recent_same_version_skips(self, state_file) -> None: # noqa: ARG002 + """A recent startup auto-update failure suppresses the same version.""" + with patch("deepagents_code.update_check.time.time", return_value=100.0): + assert mark_startup_auto_update_failed("2.0.0") is True + with patch("deepagents_code.update_check.time.time", return_value=101.0): + assert should_skip_startup_auto_update_after_failure("2.0.0") is True + + def test_different_version_does_not_skip(self, state_file) -> None: # noqa: ARG002 + """A failure marker is scoped to the failed target version.""" + with patch("deepagents_code.update_check.time.time", return_value=100.0): + mark_startup_auto_update_failed("2.0.0") + with patch("deepagents_code.update_check.time.time", return_value=101.0): + assert should_skip_startup_auto_update_after_failure("2.0.1") is False + + def test_expired_failure_does_not_skip(self, state_file) -> None: # noqa: ARG002 + """The startup failure cooldown expires after the configured window.""" + with patch("deepagents_code.update_check.time.time", return_value=100.0): + mark_startup_auto_update_failed("2.0.0") + with patch( + "deepagents_code.update_check.time.time", + return_value=100.0 + CACHE_TTL + 1, + ): + assert should_skip_startup_auto_update_after_failure("2.0.0") is False + + def test_clear_removes_matching_failure_marker(self, state_file) -> None: + """Clearing a matching marker preserves unrelated update state.""" + mark_version_seen("1.0.0") + mark_startup_auto_update_failed("2.0.0") + + clear_startup_auto_update_failure("2.0.0") + + data = json.loads(state_file.read_text(encoding="utf-8")) + assert data["seen_version"] == "1.0.0" + assert "startup_auto_update_failed_version" not in data + assert "startup_auto_update_failed_at" not in data + + def test_clear_ignores_different_version(self, state_file) -> None: + """Clearing a different version leaves the failure marker intact.""" + mark_startup_auto_update_failed("2.0.0") + + clear_startup_auto_update_failure("2.0.1") + + data = json.loads(state_file.read_text(encoding="utf-8")) + assert data["startup_auto_update_failed_version"] == "2.0.0" + + class TestShouldNotifyUpdate: @pytest.fixture def state_file(self, tmp_path): From f66dc5401f9afc158d6bedfbb184044d1d11ec20 Mon Sep 17 00:00:00 2001 From: Mason Daugherty Date: Mon, 13 Jul 2026 02:00:56 -0400 Subject: [PATCH 2/4] cr --- libs/code/deepagents_code/update_check.py | 4 +--- libs/code/tests/unit_tests/test_update_check.py | 8 +++++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/libs/code/deepagents_code/update_check.py b/libs/code/deepagents_code/update_check.py index 73f7ddd6143..079a0c413db 100644 --- a/libs/code/deepagents_code/update_check.py +++ b/libs/code/deepagents_code/update_check.py @@ -1591,8 +1591,6 @@ async def _read_stream( async def _terminate_install_process(proc: asyncio.subprocess.Process) -> None: """Terminate an install subprocess and its descendants when possible.""" - if proc.returncode is not None: - return if os.name == "posix": try: os.killpg(proc.pid, signal.SIGKILL) @@ -1601,7 +1599,7 @@ async def _terminate_install_process(proc: asyncio.subprocess.Process) -> None: except PermissionError: with suppress(ProcessLookupError): proc.kill() - else: + elif proc.returncode is None: with suppress(ProcessLookupError): proc.kill() with suppress(ProcessLookupError): diff --git a/libs/code/tests/unit_tests/test_update_check.py b/libs/code/tests/unit_tests/test_update_check.py index 1c146e499af..88ad0b64f9d 100644 --- a/libs/code/tests/unit_tests/test_update_check.py +++ b/libs/code/tests/unit_tests/test_update_check.py @@ -3851,8 +3851,10 @@ async def test_timeout_kills_process(self, tmp_path) -> None: assert success is False assert "timed out" in output - async def test_timeout_kills_process_group_on_posix(self, tmp_path) -> None: - """Timeout cleanup terminates shell descendants on POSIX.""" + async def test_timeout_kills_process_group_after_shell_exits( + self, tmp_path + ) -> None: + """Timeout cleanup kills descendants after the POSIX shell exits.""" if os.name != "posix": pytest.skip("process groups are POSIX-specific") log_path = tmp_path / "install.log" @@ -3868,7 +3870,7 @@ async def test_timeout_kills_process_group_on_posix(self, tmp_path) -> None: ), patch( "deepagents_code.update_check._install_extra_uv_tool_command", - return_value="sleep 5", + return_value="sleep 5 & exit 0", ), patch("deepagents_code.update_check.os.killpg", wraps=os.killpg) as killpg, ): From d8bdde1c48b88db34369f8a798e5cfe9e7ca1aa1 Mon Sep 17 00:00:00 2001 From: Mason Daugherty Date: Mon, 13 Jul 2026 02:19:46 -0400 Subject: [PATCH 3/4] cr --- libs/code/deepagents_code/main.py | 52 ++++++- libs/code/deepagents_code/update_check.py | 57 +++++++- libs/code/tests/unit_tests/test_main.py | 86 +++++++++++- .../tests/unit_tests/test_update_check.py | 129 ++++++++++++++++++ 4 files changed, 308 insertions(+), 16 deletions(-) diff --git a/libs/code/deepagents_code/main.py b/libs/code/deepagents_code/main.py index 7562b497d25..4bc0a48c1b0 100644 --- a/libs/code/deepagents_code/main.py +++ b/libs/code/deepagents_code/main.py @@ -323,6 +323,11 @@ def _run_startup_auto_update(console: "Console") -> None: upgrade_command, ) + # Set to the target version while an upgrade attempt is in flight, and + # cleared on success. If `perform_upgrade` *raises* instead of returning a + # failure, the fail-soft handler below records the cooldown from this so the + # same broken target is not retried — and re-stalled — on every launch. + pending_failure_version: str | None = None try: if ( _is_editable_install() @@ -372,6 +377,10 @@ def _run_startup_auto_update(console: "Console") -> None: ) return if should_skip_startup_auto_update_after_failure(latest): + # A same-version upgrade failed recently; retrying it here would + # very likely fail again and re-stall every launch (this runs before + # the TUI). Skip for the cooldown window and point at a manual + # command instead. update_needs_prereleases = release_requires_prereleases(latest) cmd = upgrade_command( include_prereleases=True if update_needs_prereleases else None, @@ -427,10 +436,12 @@ def _run_startup_auto_update(console: "Console") -> None: highlight=False, markup=False, ) + pending_failure_version = latest success, output = asyncio.run( perform_upgrade(log_path=log_path, target_version=latest) ) if success: + pending_failure_version = None clear_startup_auto_update_failure(latest) # If a stale `dcode` is earlier on PATH, the auto-restart would # re-exec into the old binary and the user would silently keep @@ -480,26 +491,40 @@ def _run_startup_auto_update(console: "Console") -> None: highlight=False, ) return - mark_startup_auto_update_failed(latest) + persisted = mark_startup_auto_update_failed(latest) update_needs_prereleases = release_requires_prereleases(latest) cmd = upgrade_command( include_prereleases=True if update_needs_prereleases else None, version=latest if update_needs_prereleases else None, ) detail = f": {escape(output[:200])}" if output else "" - console.print( + message = ( f"[bold red]Auto-update failed{detail}[/bold red]\n" f"Run manually: [cyan]{cmd}[/cyan]\n" - f"Continuing with v{cli_version}.", - markup=True, - highlight=False, + f"Continuing with v{cli_version}." ) + if not persisted: + # The cooldown marker could not be saved (e.g. a read-only state + # dir), so this same failing upgrade would otherwise be retried on + # every launch. Surface it rather than silently re-stalling, the + # way the consent-announce path surfaces its un-persisted state. + message += ( + "\n[yellow]Note:[/yellow] this failure could not be recorded, so " + "dcode will retry this update on the next launch until the state " + "directory becomes writable." + ) + console.print(message, markup=True, highlight=False) except SystemExit: # Process replacement (and test doubles that simulate it) must not be # swallowed by the fail-soft handler below. raise except Exception: logger.warning("Startup auto-update failed", exc_info=True) + if pending_failure_version is not None: + # An exception escaped the upgrade attempt itself (not a returned + # failure), so the returned-failure branch never marked it. Record + # the cooldown here so the same target is not retried every launch. + mark_startup_auto_update_failed(pending_failure_version) console.print( "[bold yellow]Warning:[/bold yellow] Auto-update failed before startup; " "continuing with the installed version." @@ -3692,7 +3717,24 @@ def cli_main() -> None: else: resume_thread = args.resume_thread # "__MOST_RECENT__", "", or None if resume_thread is None: + # A normal (non-resume) launch runs the update path and resets + # the resume grace period, so a later resume-only stretch starts + # a fresh deferral window rather than inheriting a stale one. + from deepagents_code.update_check import ( + clear_resume_auto_update_deferral, + ) + + clear_resume_auto_update_deferral() _run_startup_auto_update(console) + else: + # Keep immediate resume launches uninterrupted, but do not let + # a resume-only workflow bypass startup updates indefinitely. + from deepagents_code.update_check import ( + should_defer_startup_auto_update_for_resume, + ) + + if not should_defer_startup_auto_update_for_resume(): + _run_startup_auto_update(console) # Resolve recent-agent fallback only for actual session launches. assistant_id = _resolve_agent_arg(args) # Interactive mode - handle thread resume diff --git a/libs/code/deepagents_code/update_check.py b/libs/code/deepagents_code/update_check.py index 079a0c413db..bae51e18f38 100644 --- a/libs/code/deepagents_code/update_check.py +++ b/libs/code/deepagents_code/update_check.py @@ -139,6 +139,9 @@ _UPGRADE_TIMEOUT = 120 # seconds """Wall-clock cap for `perform_upgrade` and `perform_install_extra`.""" +_TERMINATE_WAIT_TIMEOUT = 5 # seconds +"""Cap on reaping a killed install subprocess so cleanup cannot hang launch.""" + INSTALL_SCRIPT_COMMAND = "curl -LsSf https://langch.in/dcode | bash" """Promoted public install command for Deep Agents Code.""" @@ -154,12 +157,16 @@ STARTUP_AUTO_UPDATE_FAILURE_COOLDOWN = CACHE_TTL """Seconds to suppress same-version startup auto-update retries after failure.""" +RESUME_AUTO_UPDATE_GRACE_PERIOD = 7 * 24 * 60 * 60 +"""Seconds resumed sessions may bypass the startup auto-update path.""" + _STARTUP_AUTO_UPDATE_FAILED_VERSION_KEY = "startup_auto_update_failed_version" _STARTUP_AUTO_UPDATE_FAILED_AT_KEY = "startup_auto_update_failed_at" _STARTUP_AUTO_UPDATE_FAILURE_KEYS: tuple[str, ...] = ( _STARTUP_AUTO_UPDATE_FAILED_VERSION_KEY, _STARTUP_AUTO_UPDATE_FAILED_AT_KEY, ) +_RESUME_AUTO_UPDATE_DEFERRED_AT_KEY = "resume_auto_update_deferred_at" UpgradeProgressCallback = Callable[[str], Awaitable[None] | None] @@ -939,6 +946,30 @@ def _write_update_state( return True +def should_defer_startup_auto_update_for_resume() -> bool: + """Return whether a resumed session is still in its update grace period. + + The first resumed launch starts a fixed grace period. Later resumes do not + extend it, so a resume-only workflow eventually follows the normal startup + auto-update path. If the marker cannot be persisted, fail closed and run + the update path rather than allowing an unbounded bypass. + """ + data = _read_update_state() + now = time.time() + deferred_at = _coerce_checked_at(data.get(_RESUME_AUTO_UPDATE_DEFERRED_AT_KEY)) + if deferred_at is None or deferred_at > now: + return _write_update_state({_RESUME_AUTO_UPDATE_DEFERRED_AT_KEY: now}) + return now - deferred_at < RESUME_AUTO_UPDATE_GRACE_PERIOD + + +def clear_resume_auto_update_deferral() -> None: + """Reset the resume grace period after a normal interactive launch.""" + data = _read_update_state() + if _RESUME_AUTO_UPDATE_DEFERRED_AT_KEY not in data: + return + _write_update_state({}, remove_keys=(_RESUME_AUTO_UPDATE_DEFERRED_AT_KEY,)) + + def should_skip_startup_auto_update_after_failure(version: str) -> bool: """Return whether startup auto-update should skip a recently failed version.""" data = _read_update_state() @@ -1590,20 +1621,34 @@ async def _read_stream( async def _terminate_install_process(proc: asyncio.subprocess.Process) -> None: - """Terminate an install subprocess and its descendants when possible.""" + """Best-effort kill of an install subprocess and its descendants. + + On POSIX the sole caller starts the child in its own session + (`start_new_session=True`), so `proc.pid` doubles as the process-group id + and `killpg` reaps descendants too; do not call this on a process not + started that way or it would signal the caller's own group. + + This is teardown that runs *under* a timeout or cancellation, so it must + never raise — a stray error here would mask the failure it is cleaning up + after. Every step therefore swallows benign races (a process that already + exited, a group we cannot signal) and the final reap is time-bounded so an + unreapable child cannot hang startup before the TUI. + """ if os.name == "posix": try: os.killpg(proc.pid, signal.SIGKILL) except ProcessLookupError: pass - except PermissionError: - with suppress(ProcessLookupError): + except OSError: + # e.g. EPERM if the group contains a privileged descendant; fall + # back to killing the direct child so at least it is reaped. + with suppress(OSError): proc.kill() elif proc.returncode is None: - with suppress(ProcessLookupError): + with suppress(OSError): proc.kill() - with suppress(ProcessLookupError): - await proc.wait() + with suppress(OSError, TimeoutError): + await asyncio.wait_for(proc.wait(), timeout=_TERMINATE_WAIT_TIMEOUT) async def _run_install_subprocess( diff --git a/libs/code/tests/unit_tests/test_main.py b/libs/code/tests/unit_tests/test_main.py index 5810e163d28..9e5a40fe18f 100644 --- a/libs/code/tests/unit_tests/test_main.py +++ b/libs/code/tests/unit_tests/test_main.py @@ -348,6 +348,84 @@ def test_failed_update_does_not_restart_and_continues(self) -> None: printed = " ".join(str(c.args[0]) for c in console.print.call_args_list) assert "Auto-update failed" in printed + def test_unpersisted_failure_marker_warns_user(self) -> None: + """An unwritable cooldown marker must be surfaced, not silently dropped.""" + console = MagicMock() + upgrade = AsyncMock(return_value=(False, "pip exploded")) + + with ( + patch("deepagents_code.config._is_editable_install", return_value=False), + patch( + "deepagents_code.update_check.is_auto_update_enabled", + return_value=True, + ), + patch( + "deepagents_code.update_check.get_cached_update_available", + return_value=(True, "9.9.9"), + ), + patch( + "deepagents_code.update_check.format_release_age_parenthetical", + return_value="", + ), + patch( + "deepagents_code.update_check.create_update_log_path", + return_value=Path("/tmp/dcode-update.log"), + ), + patch( + "deepagents_code.update_check.upgrade_command", + return_value="uv tool upgrade deepagents-code", + ), + patch("deepagents_code.update_check.perform_upgrade", upgrade), + # The marker write fails (e.g. a read-only state dir). + patch( + "deepagents_code.update_check.mark_startup_auto_update_failed", + return_value=False, + ), + patch("deepagents_code.main._restart_current_process") as restart, + ): + _run_startup_auto_update(console) + + restart.assert_not_called() + printed = " ".join(str(c.args[0]) for c in console.print.call_args_list) + assert "could not be recorded" in printed + + def test_exception_during_upgrade_marks_failure(self) -> None: + """An upgrade that *raises* must still record the same-version cooldown.""" + console = MagicMock() + upgrade = AsyncMock(side_effect=RuntimeError("uv wedged")) + + with ( + patch("deepagents_code.config._is_editable_install", return_value=False), + patch( + "deepagents_code.update_check.is_auto_update_enabled", + return_value=True, + ), + patch( + "deepagents_code.update_check.get_cached_update_available", + return_value=(True, "9.9.9"), + ), + patch( + "deepagents_code.update_check.format_release_age_parenthetical", + return_value="", + ), + patch( + "deepagents_code.update_check.create_update_log_path", + return_value=Path("/tmp/dcode-update.log"), + ), + patch("deepagents_code.update_check.perform_upgrade", upgrade), + patch( + "deepagents_code.update_check.mark_startup_auto_update_failed" + ) as mark_failed, + patch("deepagents_code.main._restart_current_process") as restart, + ): + # Must not raise: the fail-soft handler swallows and continues. + _run_startup_auto_update(console) + + mark_failed.assert_called_once_with("9.9.9") + restart.assert_not_called() + printed = " ".join(str(c.args[0]) for c in console.print.call_args_list) + assert "Auto-update failed before startup" in printed + def test_recent_failure_cooldown_skips_startup_update(self) -> None: """A same-version startup failure cooldown must bypass repeat attempts.""" console = MagicMock() @@ -884,11 +962,9 @@ def test_startup_auto_update_wired_into_interactive_launch(self) -> None: no-op. """ source = inspect.getsource(cli_main) - expected = ( - "if resume_thread is None:\n" - " _run_startup_auto_update(console)" - ) - assert expected in source + assert "clear_resume_auto_update_deferral()" in source + assert "if not should_defer_startup_auto_update_for_resume():" in source + assert source.count("_run_startup_auto_update(console)") == 2 class TestAutoUpdateDefaultMigration: diff --git a/libs/code/tests/unit_tests/test_update_check.py b/libs/code/tests/unit_tests/test_update_check.py index 88ad0b64f9d..e6355f69153 100644 --- a/libs/code/tests/unit_tests/test_update_check.py +++ b/libs/code/tests/unit_tests/test_update_check.py @@ -2,6 +2,7 @@ from __future__ import annotations +import asyncio import json import logging import os @@ -32,8 +33,11 @@ _note_install_baseline, _parse_version, _requires_prerelease_dependency, + _run_install_subprocess, + _terminate_install_process, _uv_tool_bin_dir, cleanup_update_logs, + clear_resume_auto_update_deferral, clear_startup_auto_update_failure, clear_update_notified, create_update_log_path, @@ -86,6 +90,7 @@ release_requires_prereleases, set_auto_update, should_announce_auto_update_default, + should_defer_startup_auto_update_for_resume, should_notify_update, should_skip_startup_auto_update_after_failure, upgrade_command, @@ -3880,6 +3885,57 @@ async def test_timeout_kills_process_group_after_shell_exits( killpg.assert_called_once() assert killpg.call_args.args[1] == signal.SIGKILL + async def test_cancellation_terminates_process_and_propagates( + self, tmp_path + ) -> None: + """Cancelling mid-install kills the process group and re-raises.""" + if os.name != "posix": + pytest.skip("process groups are POSIX-specific") + log_path = tmp_path / "install.log" + started = asyncio.Event() + + def progress(line: str) -> None: + if "ready" in line: + started.set() + + with patch("deepagents_code.update_check.os.killpg", wraps=os.killpg) as killpg: + task = asyncio.ensure_future( + _run_install_subprocess( + "echo ready; sleep 30", progress=progress, log_path=log_path + ) + ) + # Cancel only once the subprocess is actually running (it emitted a + # line), so cancellation lands inside the install, not before it. + await asyncio.wait_for(started.wait(), timeout=5) + task.cancel() + with pytest.raises(asyncio.CancelledError): + await task + + # Cancellation must not be swallowed, and the descendant `sleep` must be + # killed via the process group rather than orphaned. + killpg.assert_called_once() + assert killpg.call_args.args[1] == signal.SIGKILL + + async def test_terminate_falls_back_to_direct_kill_on_permission_error( + self, + ) -> None: + """When `killpg` is denied, the direct child is still reaped.""" + if os.name != "posix": + pytest.skip("process groups are POSIX-specific") + proc = await asyncio.create_subprocess_shell( + "sleep 30", + stdin=asyncio.subprocess.DEVNULL, + start_new_session=True, + ) + with patch( + "deepagents_code.update_check.os.killpg", + side_effect=PermissionError, + ): + await _terminate_install_process(proc) + + # The EPERM fallback (`proc.kill()`) must have reaped the child. + assert proc.returncode is not None + async def test_oserror_includes_exception_detail(self, tmp_path) -> None: """An OSError during exec must surface the exception class + message.""" log_path = tmp_path / "install.log" @@ -4540,6 +4596,79 @@ def test_clear_ignores_different_version(self, state_file) -> None: data = json.loads(state_file.read_text(encoding="utf-8")) assert data["startup_auto_update_failed_version"] == "2.0.0" + def test_corrupted_timestamp_does_not_skip(self, state_file) -> None: + """A hand-edited non-numeric timestamp must not crash or skip.""" + state_file.write_text( + json.dumps( + { + "startup_auto_update_failed_version": "2.0.0", + "startup_auto_update_failed_at": "not-a-number", + } + ), + encoding="utf-8", + ) + assert should_skip_startup_auto_update_after_failure("2.0.0") is False + + +class TestResumeAutoUpdateGracePeriod: + @pytest.fixture + def state_file(self, tmp_path): + """Override UPDATE_STATE_FILE to use a temporary file.""" + path = tmp_path / "update_state.json" + with patch("deepagents_code.update_check.UPDATE_STATE_FILE", path): + yield path + + def test_first_resume_starts_grace_period(self, state_file) -> None: + """The first resumed launch records and uses the grace period.""" + with patch("deepagents_code.update_check.time.time", return_value=100.0): + assert should_defer_startup_auto_update_for_resume() is True + + data = json.loads(state_file.read_text(encoding="utf-8")) + assert data["resume_auto_update_deferred_at"] == pytest.approx(100.0) + + def test_repeated_resume_does_not_extend_grace_period(self, state_file) -> None: + """Repeated resumes preserve the first deferral timestamp.""" + with patch("deepagents_code.update_check.time.time", return_value=100.0): + should_defer_startup_auto_update_for_resume() + with patch("deepagents_code.update_check.time.time", return_value=200.0): + assert should_defer_startup_auto_update_for_resume() is True + + data = json.loads(state_file.read_text(encoding="utf-8")) + assert data["resume_auto_update_deferred_at"] == pytest.approx(100.0) + + def test_expired_grace_period_runs_update_path(self, state_file) -> None: # noqa: ARG002 + """Resume-only use stops bypassing startup updates after seven days.""" + from deepagents_code.update_check import RESUME_AUTO_UPDATE_GRACE_PERIOD + + with patch("deepagents_code.update_check.time.time", return_value=100.0): + should_defer_startup_auto_update_for_resume() + with patch( + "deepagents_code.update_check.time.time", + return_value=100.0 + RESUME_AUTO_UPDATE_GRACE_PERIOD, + ): + assert should_defer_startup_auto_update_for_resume() is False + + def test_normal_launch_resets_grace_period(self, state_file) -> None: + """A normal launch lets a later resume begin a fresh grace period.""" + with patch("deepagents_code.update_check.time.time", return_value=100.0): + should_defer_startup_auto_update_for_resume() + + clear_resume_auto_update_deferral() + + data = json.loads(state_file.read_text(encoding="utf-8")) + assert "resume_auto_update_deferred_at" not in data + with patch("deepagents_code.update_check.time.time", return_value=200.0): + assert should_defer_startup_auto_update_for_resume() is True + + def test_unwritable_marker_does_not_bypass_update(self, tmp_path) -> None: + """A persistence failure cannot create an unbounded resume bypass.""" + state_file = tmp_path / "missing" / "update_state.json" + with ( + patch("deepagents_code.update_check.UPDATE_STATE_FILE", state_file), + patch("pathlib.Path.mkdir", side_effect=OSError("read-only")), + ): + assert should_defer_startup_auto_update_for_resume() is False + class TestShouldNotifyUpdate: @pytest.fixture From aabade2e4c2e40add4d562edbb06c7d280dfae0d Mon Sep 17 00:00:00 2001 From: Mason Daugherty Date: Mon, 13 Jul 2026 02:29:43 -0400 Subject: [PATCH 4/4] cr --- libs/code/deepagents_code/main.py | 24 ++++++++++++++---------- libs/code/tests/unit_tests/test_main.py | 10 +++++++--- 2 files changed, 21 insertions(+), 13 deletions(-) diff --git a/libs/code/deepagents_code/main.py b/libs/code/deepagents_code/main.py index 4bc0a48c1b0..8e5f9ee860c 100644 --- a/libs/code/deepagents_code/main.py +++ b/libs/code/deepagents_code/main.py @@ -41,6 +41,11 @@ _SANDBOX_DEFAULT_SENTINEL = "\x00default" """Marker stored by `--sandbox` with no value, resolved to `[sandboxes].default`.""" +_UNPERSISTED_AUTO_UPDATE_FAILURE_NOTE = ( + "\n[yellow]Note:[/yellow] this failure could not be recorded, so dcode will " + "retry this update on the next launch until the state directory becomes writable." +) + def _handle_termination_signal(signum: int, _frame: object) -> NoReturn: """Unwind dcode on a terminating signal so owned resources are cleaned up. @@ -508,11 +513,7 @@ def _run_startup_auto_update(console: "Console") -> None: # dir), so this same failing upgrade would otherwise be retried on # every launch. Surface it rather than silently re-stalling, the # way the consent-announce path surfaces its un-persisted state. - message += ( - "\n[yellow]Note:[/yellow] this failure could not be recorded, so " - "dcode will retry this update on the next launch until the state " - "directory becomes writable." - ) + message += _UNPERSISTED_AUTO_UPDATE_FAILURE_NOTE console.print(message, markup=True, highlight=False) except SystemExit: # Process replacement (and test doubles that simulate it) must not be @@ -520,15 +521,18 @@ def _run_startup_auto_update(console: "Console") -> None: raise except Exception: logger.warning("Startup auto-update failed", exc_info=True) + message = ( + "[bold yellow]Warning:[/bold yellow] Auto-update failed before startup; " + "continuing with the installed version." + ) if pending_failure_version is not None: # An exception escaped the upgrade attempt itself (not a returned # failure), so the returned-failure branch never marked it. Record # the cooldown here so the same target is not retried every launch. - mark_startup_auto_update_failed(pending_failure_version) - console.print( - "[bold yellow]Warning:[/bold yellow] Auto-update failed before startup; " - "continuing with the installed version." - ) + persisted = mark_startup_auto_update_failed(pending_failure_version) + if not persisted: + message += _UNPERSISTED_AUTO_UPDATE_FAILURE_NOTE + console.print(message, markup=True, highlight=False) def _resolve_agent_arg(args: argparse.Namespace) -> str: diff --git a/libs/code/tests/unit_tests/test_main.py b/libs/code/tests/unit_tests/test_main.py index 9e5a40fe18f..2f6bedb5815 100644 --- a/libs/code/tests/unit_tests/test_main.py +++ b/libs/code/tests/unit_tests/test_main.py @@ -389,8 +389,10 @@ def test_unpersisted_failure_marker_warns_user(self) -> None: printed = " ".join(str(c.args[0]) for c in console.print.call_args_list) assert "could not be recorded" in printed - def test_exception_during_upgrade_marks_failure(self) -> None: - """An upgrade that *raises* must still record the same-version cooldown.""" + def test_exception_during_upgrade_warns_if_failure_marker_is_unpersisted( + self, + ) -> None: + """A raised upgrade must warn when its cooldown marker cannot be saved.""" console = MagicMock() upgrade = AsyncMock(side_effect=RuntimeError("uv wedged")) @@ -414,7 +416,8 @@ def test_exception_during_upgrade_marks_failure(self) -> None: ), patch("deepagents_code.update_check.perform_upgrade", upgrade), patch( - "deepagents_code.update_check.mark_startup_auto_update_failed" + "deepagents_code.update_check.mark_startup_auto_update_failed", + return_value=False, ) as mark_failed, patch("deepagents_code.main._restart_current_process") as restart, ): @@ -425,6 +428,7 @@ def test_exception_during_upgrade_marks_failure(self) -> None: restart.assert_not_called() printed = " ".join(str(c.args[0]) for c in console.print.call_args_list) assert "Auto-update failed before startup" in printed + assert "could not be recorded" in printed def test_recent_failure_cooldown_skips_startup_update(self) -> None: """A same-version startup failure cooldown must bypass repeat attempts."""