Path.read_text() and Path.write_text() without explicit encoding
default to the system locale encoding. On Windows this is typically
cp1252, which causes UnicodeDecodeError for UTF-8 content (JSON
configs, user data, service scripts).
Add encoding="utf-8" to all read_text() and write_text() calls
across 8 CLI files, matching the pattern established in PR #50534
(security_audit_startup.py) and ruff rule PLW1514.
Fixed files:
- main.py: 4 read_text calls
- auth.py: 3 read_text calls
- banner.py: 1 read_text + 1 write_text
- service_manager.py: 1 read_text + 4 write_text
- container_boot.py: 1 read_text + 4 write_text
- doctor.py: 3 read_text calls
- uninstall.py: 2 read_text calls
- gateway.py: 1 write_text call
Several file-I/O call sites still use open() / Path.read_text() /
Path.write_text() without an explicit encoding, so they fall back to
the platform default. On Windows CN/JP/KR locales (GBK/CP932/CP949)
any non-ASCII byte in a config/state/user-content file raises
UnicodeDecodeError or UnicodeEncodeError and crashes the caller.
to the remaining hot paths:
- agent/copilot_acp_client.py: fs/read_text_file and fs/write_text_file
(Copilot's read_file / write_file tools,
directly reported in #18637 bug 2)
- agent/model_metadata.py: context-length YAML cache load + two
save sites (context probing is on the
call path of every model invocation)
- agent/nous_rate_guard.py: cross-session rate-limit JSON state
(read + atomic write via os.fdopen)
- cron/scheduler.py: user config.yaml read in run_job
- gateway/delivery.py: cron output writes for AI-generated
content, very likely non-ASCII
yaml.dump call sites also gain allow_unicode=True so the emitted
YAML preserves non-ASCII chars as-is instead of emitting \u escape
sequences.
Adds regression tests that monkeypatch builtins.open / Path.read_text
/ Path.write_text to simulate a GBK locale: each test raises
UnicodeDecodeError / UnicodeEncodeError unless the caller explicitly
passes encoding='utf-8'. Verified that the tests fail on main and
pass with this change, on Linux as well as on Windows.
Refs #18637
The three refresh tests asserted the replaced shared client gets
close()d — the exact cross-thread close#70773 removes. They now pin
the new contract: close() is NOT called from the refresh path; the
old client is retired (sockets shutdown, FD release deferred to GC).
Widen the #70773 fix beyond the three in-request cleanup sites removed in
the cherry-picked commit: every remaining path that swaps out the shared
OpenAI client could still hard-close its pool from a thread that doesn't
own the in-flight sockets (credential rotation/refresh on the turn thread,
dead-connection cleanup, gateway cache eviction, transport recovery) —
the same FD-recycle corruption vector, just rarer.
Add AIAgent._retire_shared_openai_client(): shutdown(SHUT_RDWR) all pooled
sockets (FD-safe from any thread, unblocks in-flight readers) but never
call client.close() — FD release is deferred to GC, which cannot run until
every borrowing thread has unwound its SSL BIO. Refcounting is the
ownership handshake; with no borrowers the FDs are released immediately.
Wired into:
- _replace_primary_openai_client (rotation/refresh/dead-conn cleanup)
- try_recover_primary_transport (primary_recovery)
- release_clients (gateway cache_evict)
agent.close() keeps the hard close: full teardown is a real session
boundary where no request may be in flight.
Tests: new tests/run_agent/test_70773_shared_client_fd_corruption.py
covers the three watchdog/retry sites plus retire semantics; existing
close-assertions updated to pin retire-not-close.
The streaming stale watchdog was calling
_replace_primary_openai_client() from its polling thread, which closes
the shared client's connection pool. Worker threads from previous
stale-killed attempts may still be unwinding their SSL BIOs, causing
TLS application-data to overwrite SQLite file headers via FD reuse.
This is the same corruption vector documented in #67142 for Anthropic,
where the fix was to never close the shared client from a non-owner
thread. Apply the same pattern to the OpenAI-wire path:
- Stale stream watchdog: skip shared client replacement
- Mid-tool-retry cleanup: skip shared client replacement
- Stream retry cleanup: skip shared client replacement
The request-local client is already closed via _close_request_client_once.
The shared client is replaced lazily by _ensure_primary_openai_client
on the next request, which runs on the owning thread.
Closes#70773.
CI's check:lint failed on two perfectionist rule violations introduced by the
new test file: type import ordering and missing blank line between the
parent-relative and same-directory import groups. No behavior change.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two React passive-effect timing bugs let a session switch land in the wrong
chat: activeSessionIdRef/selectedStoredSessionIdRef (use-session-state-cache)
and the composer's attachment-scope swap (use-composer-draft) both mirrored
their source props via useEffect, which fires one commit AFTER the new
session's view has already painted — a synchronous read/submit in that window
observed the outgoing session's ids/attachments.
- use-session-state-cache.ts: mirror the session refs synchronously during
render instead of a useEffect, guarded to fire only when the prop itself
changed (not unconditionally) so an imperative pin from submit.ts /
use-session-actions (e.g. a freshly resumed runtime id, intentionally not
synced to the source atom) survives an unrelated re-render.
- use-composer-draft.ts: the per-thread attachment-scope-swap effect is now a
useLayoutEffect, closing the window before paint.
- submit.ts / session-context-drift.ts: add a 3rd drift prong comparing the
composer's loaded scope (SubmitTextOptions.composerScope) against the
submit target, resolved into the same lineage-root domain
(resolveComposerSessionKey) the composer itself uses — comparing against
the raw tip id would false-positive-abort every submit into any session
that has ever auto-compressed.
- routes.ts / chat/index.tsx: the primary composer's durable scope key now
prefers the route over a possibly-stale store selection
(primaryRouteSelectedSessionId).
- use-composer-draft.ts: redacted [composer-rehydrate] diagnostic log
(counts/kinds/scope only, never raw refs) for future reports in this class.
- chat-runtime.ts: normalize attachment id values (url/path) before hashing
so a re-attach with a trailing slash or backslash path dedupes correctly.
16 files, 286 tests across the touched/dependent suites (17 files) green,
including new regression coverage for each fix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Teardown-path tests build bare runners via object.__new__ without
the liveness-guard machinery; the unguarded call raised
AttributeError in 8 tests. Same guard pattern as the start path.
Follow-up to the salvaged #69164 commits: policy forbids introducing new
HERMES_* environment variables, so the four watchdog env knobs
(HERMES_GATEWAY_LOOP_WATCHDOG / _INTERVAL / _TIMEOUT / _STRIKES) are
replaced with a single config.yaml boolean:
gateway:
loop_watchdog: true # default; false disables both guards
- gateway/config.py: new GatewayConfig.loop_watchdog field (default True),
parsed from top-level or nested gateway: form, round-trips via
to_dict/from_dict.
- gateway/run.py: _start_loop_liveness_guards() checks config.loop_watchdog
before arming the floor timer + watchdog (getattr-guarded for bare
object.__new__ runners).
- gateway/shutdown_watchdog.py: start_loop_liveness_watchdog() no longer
reads the environment; probe interval/timeout/strikes are module
constants (30s/10s/3 — ~90s to restart, matching the systemd watchdog
layer's posture).
- hermes_cli/config.py: documented gateway.loop_watchdog default so
'hermes config set gateway.loop_watchdog false' validates.
- tests: env-knob tests replaced with config-gate + round-trip tests;
the final-strike boundary test injects its probe via max_strikes
directly instead of patching the removed env helper.
- A stop() landing while the final diagnostics (critical log,
traceback dump) are executing could still reach os._exit(75) after
the pre-diagnostic check. Add a third stop_event recheck immediately
before the hard exit: diagnostics may complete, but a disarmed
watchdog never exits.
- Deterministic regressions for both windows (stop triggered from
inside logger.critical and from inside faulthandler.dump_traceback);
mutation-verified (removing the check turns both red). Frozen-loop
semantics unchanged.
Addresses the second round of the shutdown-race review on #69164.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Re-check stop_event after a missed probe (before the strike
increment) and again on entering the final-strike branch (before the
critical log, dump, and hard exit), so a normal stop() landing
between the last timeout check and the exit path can no longer be
misclassified as a freeze and trigger a supervisor restart.
- Deterministic boundary tests pin both re-checks independently
(mutation-verified: removing either check turns its own test red);
frozen-loop semantics are unchanged.
Addresses the shutdown-race review on #69164.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- A self-rescheduling 5s call_later floor timer, armed before any
adapter connects, guarantees the selector always has a finite
timeout, so the existing async defenses (polling heartbeat, timeout
guards) regain a chance to run after a zero-pending-timer stall.
- A resident daemon-thread liveness watchdog probes the loop via
call_soon_threadsafe every 30s; after 3 consecutive 10s-timeout
misses (~120s of total unresponsiveness) it dumps all thread
tracebacks and exits with the established
GATEWAY_SERVICE_RESTART_EXIT_CODE (75) so a supervisor restarts the
gateway - async-level recovery cannot run on a frozen loop.
- stop() disarms both guards before any teardown await so a busy
shutdown is never misjudged as a freeze.
HERMES_GATEWAY_LOOP_WATCHDOG=0 disables; _INTERVAL/_TIMEOUT/_STRIKES
tune the thresholds.
Fixes#69089
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
signal.SIGUSR2 and faulthandler.register() don't exist on Windows;
the bare reference raised AttributeError at import time per the
windows-footgun checker. faulthandler.enable() still covers
fatal-error dumps on all platforms.
When connected_count == 0 and at least one platform failed with a
non-retryable error, the runner exited with GATEWAY_FATAL_CONFIG_EXIT_CODE
(78) even if OTHER platforms failed for merely transient reasons.
Real-world shape (NS-609, hosted instance): WhatsApp enabled but never
paired (non-retryable whatsapp_not_paired) + Telegram TimedOut during
polling startup (retryable) => exit 78 => the gateway either goes
permanently down (supervisors honoring the exit-78 contract via
RestartPreventExitStatus / the s6 finish->125 translation from #51228) or
crash-loops (anything else). Either way Telegram never gets its retry and
the dashboard drops with every exit, so a single unpaired platform plus
one network blip disconnected every channel on the instance.
Now exit 78 is reserved for the case where ALL startup failures are
non-retryable (true config error, nothing to wait for). With mixed
failures the gateway stays alive in degraded state: the reconnect watcher
recovers the retryable platforms and the misconfigured ones stay
fatal-parked and visible in runtime status.
Three-part fix for the gateway going silently deaf after a retryable
fatal adapter error (e.g. httpx.ConnectError on Telegram):
1. **Detach-on-timeout in _connect_adapter_with_timeout** — Replaced
plain asyncio.wait_for with the task-detach pattern used by
_await_adapter_cleanup_with_timeout. asyncio.wait_for cancels the
overdue task but then waits for it to exit, so a connect() that
catches CancelledError can block recovery forever. The detach
pattern releases the runner at the deadline via
consume_detached_task_result.
2. **Ensure reconnect watcher always runs after escalation** — Added
_ensure_reconnect_watcher_running(), called after queueing a
retryable fatal error. If the reconnect watcher task has died
(exhausted restart budget, terminal exception), it is respawned
so queued platforms are never permanently stranded.
3. **Faulthandler at gateway startup** — Enabled faulthandler +
SIGUSR2 dump to a rotating file under HERMES_HOME/logs/ for
post-mortem diagnosis of future event-loop freezes.
Tests added for _ensure_reconnect_watcher_running (alive, dead,
not-started, not-running), fatal-error integration (retryable calls
ensure, non-retryable does not), and _connect_adapter_with_timeout
(timeout raises, success returns).
/api/status?profile= now passes pid_path=/path=/expected_home= to the
PID and runtime-status readers; the profile-unification fakes had
zero-arg signatures and raised TypeError. Plain /api/status call shapes
are unchanged (pinned by the existing zero-arg tests in
test_web_server.py).
Follow-up for the salvaged #70498 fix: replace the original PR's
mock-signature churn (28 lambda **kw edits, needed only because it changed
the no-profile call shape) with two targeted regression tests:
- ?profile=<name> must pass the profile's gateway.pid / gateway_state.json
paths and expected_home to the gateway status readers (HOME-anchored
per-profile state under ~/.hermes/profiles/<name>/)
- ?profile=<unknown> must 404 via _resolve_profile_dir
The production change keeps plain /api/status on the exact zero-arg calls,
so every pre-existing test passes unmodified.
When ?profile=<name> was passed to /api/status, the handler used
_config_profile_scope to set the HERMES_HOME contextvar override, but the
gateway liveness check (get_running_pid_cached) and runtime status read
(read_runtime_status) both resolve _get_process_hermes_home(), which
deliberately ignores contextvar overrides (issue #56986) — it always reads
os.environ['HERMES_HOME'] or the platform default. A named profile's
gateway identity files (~/.hermes/profiles/<name>/gateway.pid,
gateway_state.json) were therefore never found and the endpoint always
reported the profile's gateway as stopped.
Fix: when ?profile=<name> is requested, resolve the profile directory and
pass explicit profile-scoped paths:
- get_running_pid_cached(pid_path=profile_dir / 'gateway.pid')
- read_runtime_status(path=profile_dir / 'gateway_state.json')
- get_runtime_status_running_pid(..., expected_home=profile_dir)
This is the same explicit-path pattern _collect_profile_gateway_topology
already uses for per-profile gateway state, and it works within the #56986
constraint (no HERMES_HOME env mutation; read-only cross-profile access).
Plain /api/status without ?profile= keeps the exact zero-arg calls, so its
behavior — including the pid-cache signature and runtime-status fallback —
is byte-for-byte unchanged.
Fixes#69143
The .env sanitizer inferred missing newlines from known KEY= substrings
inside existing values. Plain secrets containing those bytes could therefore
be split into synthetic assignments and rewritten to disk.
Treat each physical line as the only assignment boundary and keep bytes after
the first equals sign opaque for boundary discovery. Preserve safe formatting,
null-byte removal, BOM handling, and normal one-assignment-per-line parsing.
Cover direct loading, dotenv loading, sanitization, writers, and migration
with behavioral regressions.
Fixes#29155
Follow-up to the cherry-picked #68258 base: the cross-session-stable
prefix (_cached_system_prompt_static) was only recorded on fresh
builds, so two paths silently degraded to the legacy single-breakpoint
layout (flagged in review of #68258/#69341/#69704):
- Session restore: gateway surfaces build a fresh AIAgent per turn and
restore the persisted prompt verbatim from the session DB; the static
prefix stayed None from turn 2 onward, flip-flopping the wire layout.
- Post-compression cached-prompt reuse: _invalidate_system_prompt()
clears the static prefix, and the keep-cached-prompt branch never
restored it.
Both sites now reconstruct the stable tier and adopt it ONLY when the
authoritative prompt string literally startswith() it — stable-tier
drift (skills edited, identity changed) falls back to the legacy layout
with the stored bytes untouched. Fail-open on any builder error. The
restore-path rebuild is gated on _use_prompt_caching so non-Anthropic
routes skip it entirely.
Refs #68191
Co-authored-by: JonthanaHanh <92574114+JonthanaHanh@users.noreply.github.com>
Co-authored-by: joaomarcos <joaomarcosdias444@gmail.com>
Co-authored-by: webtecnica <webtecnica@users.noreply.github.com>
The conversation loop normalizes message text right before the API call so
the request prefix is byte-identical across turns -- the stated reason is
KV cache reuse on local inference servers and better cache hit rates on
cloud providers. Cache breakpoints were injected *before* that pass, which
defeats it.
`_apply_cache_marker` rewrites a plain-string `content` into a
`[{"type": "text", ...}]` block. The normalization pass is guarded on
`isinstance(content, str)`, so every message that just got marked is
silently skipped by it and keeps its raw leading/trailing whitespace. A
message is only marked while it sits in the last-3 window, so:
turn N in the window -> marked, content "file1\nfile2\n"
turn N+1 rolled out -> plain, content "file1\nfile2"
The same logical message is sent with different bytes on consecutive
turns. The prefix stops matching at that position -- which is inside the
span the breakpoints were placed to protect -- so the reusable prefix
collapses back toward the system breakpoint on every turn. Tool results
carry a trailing newline almost by default (any shell command output), so
this is the common case, not an edge case.
Move the injection below every message mutation. Besides fixing the
whitespace divergence this stops breakpoints from being spent on messages
that the orphan sweep or the thinking-only drop is about to remove or
merge away -- a marker on a dropped message is a wasted breakpoint out of
the four available.
Nothing between the old and new call sites reads `cache_control`, and the
mutators now see the plain-string shapes they were written against.
Follow-up for salvaged PR #69141, addressing the last open review point:
cmd_prune() only set orphan_allowlist inside 'if orphans or pre_v2_orphans',
so a zero-orphan preview passed the unrestricted None sentinel down to
prune_checkpoints(), authorizing deletion of any project that became
orphaned between the preview and the rescan — with zero confirmation
calls. The allowlist is now bound unconditionally for every non-force
run (empty preview => empty allowlist); --force keeps None. Adds the
zero-orphan-preview timing regression plus allowlist-identity tests.
Address P1 from PR review: cmd_prune()'s y/N preview reads
store_status() but the confirmed deletion re-scans both the v2 and
pre-v2 layouts from scratch. A workdir that goes missing while the
human is answering the prompt gets swept in as if it had been shown
and approved.
prune_checkpoints() now accepts orphan_allowlist — a set of v2 project
hashes and/or pre-v2 shadow repo paths. When set, only orphans whose
identity is in the set are deleted; anything newly orphaned since the
scan survives the run. cmd_prune() builds this set from the exact
projects it just displayed and passed confirmation for. --force still
passes None (no preview shown, so nothing to bind to).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Requested by egilewski on #69141: the orphan confirmation flow had no
test coverage at all before this. Exercises hermes_cli.checkpoints.cmd_prune
directly against pre-v2-only and mixed (v2 + pre-v2) fake stores —
decline aborts with nothing deleted, accept deletes both layouts,
--force and --keep-orphans skip the prompt as expected.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
store_status()["projects"] only ever covered v2 metadata, so the
`hermes checkpoints prune` confirmation prompt was blind to pre-v2
base/<hash>/HEAD shadow repos that prune_checkpoints() deletes
separately via shutil.rmtree — a pre-v2-only or mixed store could
lose checkpoint history without ever hitting the confirmation.
Extract the pre-v2 scan into _pre_v2_shadow_repos() and have both
store_status() (preview, new pre_v2_projects key) and
prune_checkpoints() (deletion) read from it, so the CLI prompt can
no longer diverge from what actually gets removed.
Addresses review from egilewski on #69141.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Builds on this PR's diagnosis by @Frowtek: a missing workdir is
ambiguous (deleted project vs. an unmounted external volume / network
share / VPN not yet up), so it's not safe evidence for a destructive
GC sweep — especially one that runs unattended at startup.
- cli.py / gateway/run.py: the startup auto-maintenance sweep now
always passes delete_orphans=False to maybe_auto_prune_checkpoints().
It still prunes by retention_days, size cap, and legacy archives —
none of which require guessing whether a project was deleted or is
just temporarily unreachable.
- hermes_cli/config.py: drop the now-unused delete_orphans default.
- hermes_cli/checkpoints.py: `hermes checkpoints prune` (the explicit,
human-invoked path) now previews the orphan project list and asks
for confirmation before deleting, unless -f/--force is passed.
- Docs updated (EN + zh-Hans) to match.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two compression-tip hydration tests simulated legacy state by emptying
the parent AFTER end_session(compression) — exactly the durable write
the new closed-parent guard refuses. Reordered: empty first, close
second. The tests' actual contract (old id hydrates from the live tip)
is unchanged and still pinned.
The salvaged drift check compared durable rows to the in-memory snapshot
by content and ran in both modes. Two problems:
1. In-place compaction (the default) archives non-destructively — drift
cannot lose data there, and the strict-prefix content comparison
failed against seeded histories, aborting every in-place compaction
(5 test failures in test_in_place_compaction.py).
2. Content equality wedges on sessions with legal in-memory mutation of
past turns (multimodal compression, retry replacement) — the same
permanent-abort shape as #14694.
Now rotation-only and length-based: abort only when the durable parent
has MORE rows than the snapshot (a writer committed in the lease window).
Dead helper _durable_history_matches_snapshot removed.
The runtime-repair change passes repair_observer= to update_managed_uv/
ensure_uv; the autouse fixture fakes had zero-arg signatures and raised
TypeError through the mock. Sibling test file to the PR's own suite.
Follow-up on the #70186 salvage. The cherry-picked repair pinned the
candidate to the exact current CPython patch (e.g. 3.11.14). Verified
live with uv 0.11.19: every published python-build-standalone artifact
for 3.11.14 links vulnerable SQLite 3.50.4 — even with --reinstall — so
the exact-patch pin made the repair permanently impossible on the
installs that need it most (repair_vulnerable_runtime returned
'failed: could not provision a fixed private Python runtime').
Request the minor line (3.11) instead — the same resolution a fresh
'uv python install' would make, still inside requires-python — and
tighten the drift gate to 'same minor, no downgrade'. E2E-verified
end-to-end on a real vulnerable venv: repair_vulnerable_runtime()
provisioned 3.11.15, built + smoke-tested the sibling venv, cut over,
and reported SQLite 3.50.4 → 3.53.1 with the old venv parked for
rollback.
Follow-up fixes on top of the cherry-picked guard:
- verify_sqlite_integrity(): an oversized (max_bytes-exceeding) database
now still fails on a zeroed/invalid header — previously valid=True was
set before the header check, so a >1GiB zeroed state.db (the exact
#68474 signature at 95MB scale) passed as valid.
- _run_pre_update_backup(): the guard referenced get_hermes_home before a
later function-local 'from hermes_constants import get_hermes_home'
shadowed it → UnboundLocalError swallowed by the snapshot try/except,
silently disabling the post-snapshot check AND the snapshot-id output.
Alias the import explicitly.
- Import _quick_snapshot_root where used (was NameError in all 3 guards).
- tests/hermes_cli/test_state_db_guard.py: real-SQLite E2E coverage —
valid/zeroed/truncated files, oversized header gate, copy+verify
roundtrip, snapshot restore flow, and the live _run_pre_update_backup
path against a temp HERMES_HOME with mid-flight zeroing.
Problem:
On Windows, state.db could be silently replaced with 95MB of null bytes
during a desktop update (v0.19.0). The pre-update snapshot was valid, but
the live file was destroyed and the update reported exit code 0, masking
the data loss. Sessions between the snapshot and the update were
irrecoverable.
Root cause analysis:
The update flow (Desktop Electron → hermes-setup.exe → hermes update)
kills the backend process tree via taskkill /T /F, then pauses Windows
gateways, creates a pre-update snapshot, runs git pull + pip install, and
resumes gateways. On Windows, a force-killed process holding state.db
(SQLite WAL mode) can leave the file open to races with antivirus/NTFS
filter drivers, or the gateway resume can encounter a partially-recovered
WAL state that results in a zeroed file — all while exit code 0 reports
success.
Fix — three layers of defense:
1. Emergency desktop-side backup (pre-flight):
- New function in Electron main.ts reads the
SQLite header, logs it, and takes a timestamped emergency copy of
state.db BEFORE the backend is killed or the updater is spawned.
Runs in both the Tauri-updater path (Windows) and the in-app update
path (Posix). Prunes to the 2 most recent emergency backups.
2. Pre-update integrity verification (Python CLI):
- After creates the pre-update snapshot,
checks the LIVE state.db file (header +
PRAGMA integrity_check). If corrupted, checks whether the snapshot
copy is valid and warns the user. The update still proceeds because
the snapshot is the recovery path.
3. Post-update auto-restore (Python CLI):
- After the update completes (both git-pull and ZIP paths), verify
state.db integrity. If corrupted/zeroed, automatically restore from
the pre-update snapshot and re-verify. This catches the exact case
where state.db was destroyed mid-update but the snapshot was valid.
New functions in hermes_cli/backup.py:
- verify_sqlite_integrity(path, check_header, run_pragma, max_bytes)
→ Three-stage check: file size, SQLite header magic, PRAGMA
integrity_check. Configurable max_bytes to avoid reading huge DBs.
- copy_db_and_verify(src, dst)
→ Like _safe_copy_db() but verifies the destination after backup.
Fixes#68474
When a background terminal() command backgrounds its own long-lived
child (`node server.js &`, `sleep 300 &`), the grandchild inherits the
write end of the reader thread's stdout pipe. The direct bash child
exits promptly, but the pipe never reaches EOF while the grandchild
lives — so `_reader_loop`'s blocking `read1()` parked the thread
forever, `session.exited` never flipped on its own, and
`notify_on_complete` was silently lost. `_reconcile_local_exit`
(#17327) only runs lazily from poll()/wait(), so nothing autonomous
ever surfaced the exit; each occurrence also leaked a reader thread
and pipe fd for the grandchild's lifetime.
Fix: on POSIX, drain via select() with a short poll interval and stop
shortly after the direct child exits even if the pipe hasn't EOF'd —
the same pattern the foreground path uses in
tools/environments/base.py::_wait_for_process (#8340). Windows pipes
don't support select(), so the blocking path is kept there with the
existing lazy reconcile as the safety net; mocked/iterator stdout
streams (no usable fileno) also keep the historical path.
Fixes#68915
With the eager pre-warm removed (PR #66783), slash.exec is the only spawn
path — and it runs on the RPC thread pool, so two concurrent worker-routed
commands on a fresh session could both see slash_worker=None and each fork
a full stdio-MCP-fleet worker (the _attach_worker race loser leaking
unclosed). Add a per-session spawn lock with a double-check, plus a
regression test racing two slash.exec calls through handle_request.
Also maps Ne0teric's contributor email.
Every slash_worker child runs its own MCP discovery (#61891), which
forks the full configured stdio MCP fleet — on a config with a handful
of stdio servers that is ~20 OS processes per worker once npx/cmd
wrappers are counted. The gateway pre-warmed a worker for every session
at create/build time, and sessions held by a live transport are (by
design) never reaped, so a desktop app left open for days accumulates
one fleet per retained session. On a real setup this reached ~120
processes across 6 sessions and pushed Windows commit charge to the
point where CreateProcess started failing system-wide ("Not enough
memory resources are available to process this command").
slash.exec already spawns a worker on demand when the session has none
and already recovers from a dead worker the same way, so the eager
pre-warm is pure pre-warming:
- drop the pre-warm in the deferred session-build path
- drop the pre-warm in _init_session
- make _restart_slash_worker a no-op for sessions that never spawned a
worker (the next slash.exec builds one with the current session
key/model, so no stale-key worker can exist)
Only sessions that actually run a worker-routed slash command now pay
for a fleet. Cost: the first such command in a session takes the CLI
build + MCP discovery hit that session.create used to absorb.
Tests: the two create/close-race guards now assert the build thread
never constructs a worker (the notify-unregister guarantees are kept);
the restart-orphan guard seeds a live worker so the close path is still
exercised; new test pins the restart no-op for workerless sessions.
Follow-up hardening on top of the salvaged #69745 guard, addressing both
review findings:
- Drift detection no longer re-reads the file. _reload_target performs ONE
checked read and derives both the drift check and the entry parse from that
same raw snapshot (_detect_external_drift now takes the raw text). The old
second read swallowed OSError as 'no drift', so a read failure between the
two reads let replace/remove/apply_batch rewrite the file from a stale view,
discarding externally added entries.
- Invalid UTF-8 now counts as unreadable: the checked read catches
UnicodeDecodeError and mutations return the preservation refusal instead of
raising (or worse, rewriting bytes we can't round-trip).
- USER.md is covered by the same guard (shared _reload_target path) and now
pinned by an explicit test.
Tests: read-once structural invariant, invalid-UTF-8 refusal with
byte-identical file, user-store refusal.
`_read_file` degraded any read failure to `[]`, conflating "file exists but
couldn't be read" with "empty store". That is a silent, total data-loss bug on
the `add` path.
`add` re-reads the file under lock, appends the new entry, and rewrites the
WHOLE file from the parsed entries. It deliberately skips the drift guard
(#42874: "appending never clobbers existing content") — but that reasoning only
holds when the reload actually saw the file. When `read_text` raises
(an external editor momentarily holding the file on Windows, a permission
change, a filesystem/EINTR blip), `_read_file` returns `[]`, so `add` treats
the store as empty and rewrites the file down to just the new entry — every
prior memory gone — while returning `success: True`.
Reproduced with a transient read failure during `add`:
entries on disk before : 3 (dark-mode pref, deadline, deploy target)
add("A brand new fact") : success=True
entries on disk after : 1 ("A brand new fact") <-- the other 2 wiped
replace/remove/apply_batch were shielded only incidentally — an empty view
means `old_text` never matches, so they abort before writing — but they still
returned a misleading "no entry matched" instead of naming the real problem.
Fix: distinguish unreadable from empty. `_read_entries_checked` returns
`(entries, read_ok)`, with `read_ok=False` only when the file exists but can't
be read; absent/empty stays a clean `([], True)`. `_reload_target` returns a
`_READ_FAILED` sentinel in that case without touching in-memory state, and all
four mutation paths (add, replace, remove, apply_batch) refuse the write with a
clear "retry in a moment" error. This is the same posture as the drift guard
and the pairing/checkpoint fixes: never rewrite a file from a view that isn't
the real one. `_read_file` keeps its `[]`-on-error contract for the read-only
`load_from_disk` caller, which never persists.
tests/tools/test_memory_tool.py: new TestUnreadableFileDoesNotWipeMemory —
add/replace/remove/apply_batch all refuse and leave the file byte-identical on
a transient read failure, plus controls that an absent file is still a clean
empty store and the happy path is undisturbed. The four refusal tests fail on
main. Full suite: 90 passed, 1 pre-existing failure (`test_deduplication_on_load`,
a UnicodeDecodeError unrelated to this change, identical on clean main).
Named endpoints from the providers: mapping (and legacy custom_providers:
list) never appear in the ACP model selector: _build_model_state lists
only the canonical current provider's catalog, and canonical provider
enumeration does not include user-defined named endpoints. The TUI
/model picker already renders these entries (#47039, implemented for the
TUI surface only), so editor clients silently hide endpoints the user
configured — e.g. an OpenAI-compatible Bedrock Mantle Responses provider.
Add _named_custom_provider_catalogs(), sourcing entries from
get_compatible_custom_providers() (covers both config shapes), and append
its models to the selector payload. Choice ids use the custom:<name>
slug shape so custom:<name>:<model> selections round-trip through
parse_model_input / resolve_runtime_provider unchanged on set_session_model.
Declared models (default_model + models) survive failed live /models
discovery — some OpenAI-compatible endpoints expose no /models route yet
serve their declared models fine. Honors providers.<name>.enabled: false
and discover_models: false.
Verified: scripts/run_tests.sh tests/acp/ — 318 passed, 0 failed;
scripts/check-windows-footguns.py clean.
When two consecutive compactions each failed to clear the threshold, the
anti-thrashing breaker blocked automatic compaction PERMANENTLY for the
life of the session: nothing decremented _ineffective_compression_count
(or _fallback_compression_streak) while blocked, so a session whose
middle region was briefly too small to compact never auto-compacted
again — it grew unbounded until the provider's hard context limit, and
only /new or /reset recovered it.
Recovery is a probation probe, not amnesty: after
_ANTI_THRASH_RECOVERY_SECONDS (300s) of continuous block the gate grants
exactly ONE attempt by dropping tripped counters to 1 strike (persisted,
so sibling agents on the same session row — gateway hygiene — unblock
too). An ineffective probe re-trips the guard on the next real-usage
verdict and the next recovery waits a full fresh window, so the worst
case in a truly incompressible session is one compaction attempt per
window — bounded, not thrash.
The recovery clock is armed lazily on the first BLOCKED evaluation and
is deliberately not durable: a restart that loads a durable tripped
counter (#69872) starts a full fresh window blocked, preserving the
restart-must-never-disarm contract (#54923).
Fixes#14694
