Skip to content

fix: process log retention never reached files with no in-memory record - #149

Open
dvystrcil wants to merge 1 commit into
open-webui:mainfrom
dvystrcil:fix/log-retention-sweep
Open

fix: process log retention never reached files with no in-memory record#149
dvystrcil wants to merge 1 commit into
open-webui:mainfrom
dvystrcil:fix/log-retention-sweep

Conversation

@dvystrcil

Copy link
Copy Markdown

Problem

_cleanup_expired()'s log-file deletion only runs when a finished log file also has a matching in-memory BackgroundProcess record:

for process_id in expired:
    bp = _processes.pop(process_id)
    if bp.log_path and bp.finished_at and now - bp.finished_at > PROCESS_LOG_RETENTION:
        os.remove(bp.log_path)

_processes is an in-memory dict that resets to empty on every process restart. The JSONL log files under LOG_DIR/processes/ live on disk (frequently a persistent volume in containerized deployments) and survive restarts. So any log file older than the last restart is permanently unreachable by this cleanup path, regardless of age -- the in-memory record it would need to be found via is simply gone.

I ran into this concretely: a long-forgotten process log (months old) was still sitting on disk, and it contained the plaintext output of a command that had echoed a secret to stdout. PROCESS_LOG_RETENTION defaults to 7 days, but nothing was actually enforcing that once the server had restarted even once since that log was written.

Fix

_cleanup_expired() now also sweeps the log directory by file mtime directly (open_terminal/utils/log.py's new sweep_expired_log_files), independent of any in-memory record. Since _cleanup_expired() runs on every process-status-check request (/execute list, individual status polls), the actual directory scan is rate-limited to once per 5 minutes (sweep_expired_log_files_rate_limited) to avoid adding a full os.listdir() on every request under active polling.

Kept the fix minimal and reactive (piggybacking on the existing _cleanup_expired() call sites) rather than introducing new background-task/scheduling infrastructure, since none currently exists in this codebase.

Testing

No test harness exists in this repo currently, so verified manually and end-to-end:

  • Unit-level: confirmed sweep_expired_log_files deletes only .jsonl files older than the retention window, leaves recent and non-.jsonl files alone, and the rate-limited wrapper correctly no-ops on a second immediate call.
  • End-to-end: started the real server, manually placed an orphaned .jsonl file (old mtime, no in-memory record -- simulating exactly the reported scenario), hit GET /execute, confirmed the file was deleted.

Happy to add a tests/ directory with these as real pytest cases if you'd like that as part of this PR rather than a separate contribution.

_cleanup_expired()'s log-file deletion only ran when a log file also
had a matching in-memory BackgroundProcess record, but that registry
resets on every restart while the JSONL log files themselves live on
disk (often a persistent volume) and survive restarts. Any log file
older than the last restart was therefore permanently unreachable by
that path regardless of age -- observed this retaining a plaintext
secret indefinitely after a command had echoed it to stdout.

_cleanup_expired() now also sweeps the log directory by mtime
directly (open_terminal/utils/log.py: sweep_expired_log_files),
independent of any in-memory record, rate-limited to once per 5
minutes since _cleanup_expired() runs on every process-status-check
request.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant