fix: track process-result delivery separately from a fixed 5-minute expiry - #150
Open
dvystrcil wants to merge 1 commit into
Open
fix: track process-result delivery separately from a fixed 5-minute expiry#150dvystrcil wants to merge 1 commit into
dvystrcil wants to merge 1 commit into
Conversation
…xpiry
A finished process's in-memory record (and therefore its result) was
deleted 300s after completion regardless of whether anyone had
actually retrieved it yet. A caller whose own async dispatch loop
stalls past that window -- e.g. waiting on a downstream LLM call,
retrying a flaky network hop -- comes back to a bare
{"detail": "Process not found"}, even though the command genuinely
ran and finished successfully. No way to distinguish that from 'never
existed'.
Adds delivered_at, tracked separately from finished_at. A result that
has been successfully read at least once (via the initial synchronous
/execute response, or a /execute/{id}/status poll) only needs the
original short PROCESS_EXPIRY (5 min, unchanged default) -- the
caller already has it. A result nobody has read yet gets a much
longer PROCESS_UNDELIVERED_EXPIRY (30 min default, configurable via
OPEN_TERMINAL_PROCESS_UNDELIVERED_EXPIRY / process_undelivered_expiry),
so a stalled-but-recoverable caller can still get its result when it
eventually asks.
Reproduced live: submitted a real job, hit /execute (sync wait) then
/execute/{id}/status, confirmed output delivered correctly and
delivered_at accounting matches -- plus 5 unit-level scenarios
(undelivered survives short window / expires after long window,
delivered expires on short window regardless of long-window headroom,
running processes never expire).
dvystrcil
added a commit
to dvystrcil/open-terminal-docker
that referenced
this pull request
Aug 1, 2026
…opy (#67) This repo's Dockerfile has always pulled a pre-built upstream image (FROM .../ghcr-proxy/open-webui/open-terminal:latest) and only added wrapper tooling on top. It never built or installed this repo's own vendored open_terminal/ package -- every fix this CHANGELOG has described as applied to open_terminal/main.py was real, tested, merged code that the running container never actually ran. See homelab#822 for the full incident writeup. Root cause found while investigating why homelab#720's security fix (process-log retention) wasn't live despite being merged days ago. Auditing the vendored package's full history surfaced three more real, undeployed fixes beyond that one, all now submitted upstream: - open-webui/open-terminal#148 -- configurable uvicorn keep-alive timeout (intermittent ConnectionResetError) - open-webui/open-terminal#149 -- process-log retention security fix - open-webui/open-terminal#150 -- two-tier process-result expiry (a slow caller could lose a finished command's result forever) - open-webui/open-terminal#151 -- insert_after/append_to_section/ append endpoints + a defensive replace_file_content check Plus one homelab-specific fix NOT appropriate for upstream (ties into our own GH App token-file convention, not something upstream has any hook for): refresh_github_token_env(), re-reads the current token from disk before every subprocess spawn, closing a gap BASH_ENV-based shell-profile sourcing doesn't cover (plain-shell and PTY spawn paths never source /etc/profile.d). ## What changed - Dockerfile: stage 1 now builds open_terminal from dvystrcil/open-terminal-app-fork (a real fork carrying all 5 fixes above) via git clone + pip install ., mirroring upstream's own Dockerfile exactly, instead of pulling the pre-built upstream image. TEMPORARY -- revert to a plain upstream FROM once all four PRs merge and a release picks them up. - docker.yml: resolves the fork's current commit SHA via `git ls-remote` and passes it as a build-arg on every run. Without this, Docker's build cache (keyed on RUN command text, not on what `git clone --branch main` actually fetches) would silently keep shipping whatever fork commit was cloned the FIRST time this layer built, even after new fixes land on the fork -- caught this empirically: an initial local build without the explicit build-arg produced a stale image missing later fixes despite a fresh fork push. - Removed the vendored open_terminal/ package and its tests -- dead weight now that the real fixes live in a fork with a real upstream relationship. Kept tests/test_actor_env.py (tests helpers/bible_bridge.py, which *is* deployed) and tests/__init__.py. Removed pyproject.toml, dev.sh, .python-version (all specific to developing the now-removed vendored package). - README: documents the new build shape and the incident. ## Testing Built the actual image locally (multi-stage, full apt/pip install, ~2 min), ran it, and verified all 5 fixes are present in the running container (direct imports of the sweep function, keep-alive/expiry env values, refresh_github_token_env, and the three new endpoint handlers) plus the health endpoint and all wrapper tools (kubectl, gh, yq, argocd, act) and the entrypoint's token-refresh profile. Confirmed the FORK_SHA cache-bust actually works: an explicit --build-arg forces a genuine re-clone (visible in build output, not a CACHED layer) rather than silently reusing a stale one. tests/test_actor_env.py still passes standalone (7/7) against the real helpers/bible_bridge.py, unaffected by the removed package.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A finished process's in-memory record (and therefore its result) is deleted 5 minutes after completion, regardless of whether anyone has actually retrieved it yet. I hit this with a caller whose own async dispatch loop stalled well past that window -- it came back to a bare
{"detail": "Process not found"}, even though the command had genuinely run and finished successfully. There's no way for a well-behaved caller to distinguish "result is gone because nobody read it in time" from "this process ID never existed."Concretely: submit a long or slow-to-retrieve job, have the caller's own polling loop stall for >5 minutes (network hiccup, downstream LLM call, whatever), and the result is just gone by the time it asks -- a completed command's output becomes unrecoverable, silently.
Fix
Adds
delivered_at, tracked separately fromfinished_at, toBackgroundProcess. A result that's been successfully read at least once (via the initial synchronous/executeresponse whenwaitis set, or a/execute/{id}/statuspoll) only needs the original short expiry (OPEN_TERMINAL_PROCESS_EXPIRY, still defaults to 5 minutes -- unchanged) since the caller already has it. A result nobody has read yet gets a much longer grace period (OPEN_TERMINAL_PROCESS_UNDELIVERED_EXPIRY, defaults to 30 minutes), so a stalled-but-recoverable caller can still get its result when it eventually asks.Both new settings follow the existing env-var/
config.tomlpattern (seeOPEN_TERMINAL_LOG_RETENTION).Testing
No test harness exists in this repo currently, so verified two ways:
_cleanup_expired()directly: an undelivered result survives past the short window, expires after the long window; a delivered result expires on the short window even with long-window headroom remaining; a running process never expires.POST /execute, polledGET /execute/{id}/status, confirmed the output is delivered correctly anddelivered_ataccounting matches.Happy to add these as real pytest cases in this PR if that's useful, or split this into a smaller diff if you'd rather review the expiry-tracking and the two new response call-sites separately.