diff --git a/CHANGELOG.md b/CHANGELOG.md index 17246ea..a8cfec1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). +## [Unreleased] + +### Fixed + +- 🔒 **A finished process's result could vanish before a slow caller ever read it** — a finished process's in-memory record (and its result) was deleted 300s after completion regardless of whether anyone had actually retrieved it yet. A caller whose own dispatch loop stalls past that window would come back to `{"detail": "Process not found"}` -- permanent, silent loss of a command that actually succeeded, with no way to distinguish "still running" from "result is gone." Now tracks `delivered_at` separately from `finished_at`: undelivered results get a much longer grace period (`OPEN_TERMINAL_PROCESS_UNDELIVERED_EXPIRY`, default 30 minutes), delivered results keep the original short one (`OPEN_TERMINAL_PROCESS_EXPIRY`, default 5 minutes, unchanged default), since the caller already has what it needs. + ## [0.11.34] - 2026-04-08 ### Added diff --git a/open_terminal/env.py b/open_terminal/env.py index b7bd1d1..f0bfe82 100644 --- a/open_terminal/env.py +++ b/open_terminal/env.py @@ -107,6 +107,30 @@ def _resolve_file_env(var: str, default: str = "") -> str: ) ) +# How long (in seconds) to keep a finished process's in-memory record +# AFTER its result has been successfully delivered at least once via +# GET /execute/{id}/status. Short is fine here -- the caller already has +# what it needs. +PROCESS_EXPIRY: float = float( + os.environ.get( + "OPEN_TERMINAL_PROCESS_EXPIRY", + config.get("process_expiry", 300), # 5 minutes + ) +) + +# How long (in seconds) to keep a finished process's in-memory record if +# NOBODY has successfully polled its status yet. Deliberately much longer +# than PROCESS_EXPIRY: a caller whose own dispatch loop stalls still +# needs the result to be there when it eventually recovers and asks. +# Expiring on the same short window as a delivered result turns a +# recoverable caller-side hang into a permanent, silent loss. +PROCESS_UNDELIVERED_EXPIRY: float = float( + os.environ.get( + "OPEN_TERMINAL_PROCESS_UNDELIVERED_EXPIRY", + config.get("process_undelivered_expiry", 1800), # 30 minutes + ) +) + # Minimum interval (in seconds) between log flushes during command execution. # 0 (default) = flush after every chunk (current behaviour). # Setting this to e.g. 1.0 reduces I/O pressure on high-output commands. diff --git a/open_terminal/main.py b/open_terminal/main.py index 01c0796..6f6b150 100644 --- a/open_terminal/main.py +++ b/open_terminal/main.py @@ -24,7 +24,7 @@ from fastapi.security import HTTPAuthorizationCredentials, HTTPBearer from pydantic import BaseModel, Field -from open_terminal.env import API_KEY, BINARY_FILE_MIME_PREFIXES, CORS_ALLOWED_ORIGINS, ENABLE_NOTEBOOKS, ENABLE_SYSTEM_PROMPT, ENABLE_TERMINAL, EXECUTE_DESCRIPTION, EXECUTE_TIMEOUT, FILE_BROWSER_ROOT, LOG_DIR, MAX_TERMINAL_SESSIONS, MULTI_USER, OPEN_TERMINAL_INFO, PROCESS_LOG_RETENTION, SESSION_CWD_TTL, SYSTEM_PROMPT, TERMINAL_TERM +from open_terminal.env import API_KEY, BINARY_FILE_MIME_PREFIXES, CORS_ALLOWED_ORIGINS, ENABLE_NOTEBOOKS, ENABLE_SYSTEM_PROMPT, ENABLE_TERMINAL, EXECUTE_DESCRIPTION, EXECUTE_TIMEOUT, FILE_BROWSER_ROOT, LOG_DIR, MAX_TERMINAL_SESSIONS, MULTI_USER, OPEN_TERMINAL_INFO, PROCESS_EXPIRY, PROCESS_LOG_RETENTION, PROCESS_UNDELIVERED_EXPIRY, SESSION_CWD_TTL, SYSTEM_PROMPT, TERMINAL_TERM from open_terminal.utils.runner import PipeRunner, ProcessRunner, create_runner from open_terminal.utils.fs import UserFS @@ -304,11 +304,11 @@ class BackgroundProcess: exit_code: Optional[int] = None log_task: Optional[asyncio.Task] = field(default=None, repr=False) finished_at: Optional[float] = field(default=None, repr=False) + delivered_at: Optional[float] = field(default=None, repr=False) log_path: Optional[str] = field(default=None, repr=False) _processes: dict[str, BackgroundProcess] = {} -_EXPIRY_SECONDS = 300 # auto-clean finished processes after 5 min # --------------------------------------------------------------------------- @@ -353,15 +353,26 @@ def _set_session_cwd(session_id: str | None, path: str): def _cleanup_expired(): """Remove finished processes that have expired. + Two different grace periods: a process whose finished status has been + successfully delivered to a caller at least once only needs + PROCESS_EXPIRY (short -- the caller already has the result). A + process nobody has successfully polled yet gets + PROCESS_UNDELIVERED_EXPIRY instead (much longer), so a caller whose + own dispatch loop stalls doesn't come back to find its result + already gone. + Also deletes log files older than *LOG_RETENTION_SECONDS*. """ now = time.time() - expired = [ - process_id - for process_id, background_process in _processes.items() - if background_process.finished_at - and now - background_process.finished_at > _EXPIRY_SECONDS - ] + expired = [] + for process_id, background_process in _processes.items(): + if not background_process.finished_at: + continue + if background_process.delivered_at is not None: + if now - background_process.delivered_at > PROCESS_EXPIRY: + expired.append(process_id) + elif now - background_process.finished_at > PROCESS_UNDELIVERED_EXPIRY: + expired.append(process_id) for process_id in expired: bp = _processes.pop(process_id) # Delete the log file if it has exceeded the retention period. @@ -1201,6 +1212,12 @@ async def execute( background_process.log_path, offset=0, tail=tail ) + if background_process.status != "running" and background_process.delivered_at is None: + # A synchronous wait that returned a finished result already + # delivered it to the caller -- same accounting as the dedicated + # status endpoint below. + background_process.delivered_at = time.time() + return { "id": process_id, "command": request.command, @@ -1259,6 +1276,12 @@ async def get_status( background_process.log_path, offset=offset, tail=tail ) + if background_process.status != "running" and background_process.delivered_at is None: + # First successful read of a finished process's status -- from + # here on the short PROCESS_EXPIRY grace period applies instead + # of PROCESS_UNDELIVERED_EXPIRY. + background_process.delivered_at = time.time() + return { "id": background_process.id, "command": background_process.command,