Skip to content

fix(session): don't prune session_map on transient list_windows() failure#128

Closed
NatBrian wants to merge 1 commit into
alexei-led:mainfrom
NatBrian:fix/prune-session-map-on-transient-list-windows-failure
Closed

fix(session): don't prune session_map on transient list_windows() failure#128
NatBrian wants to merge 1 commit into
alexei-led:mainfrom
NatBrian:fix/prune-session-map-on-transient-list-windows-failure

Conversation

@NatBrian

@NatBrian NatBrian commented Jul 2, 2026

Copy link
Copy Markdown

Problem

get_session() swallows tmux/herdr subprocess errors into None, so
list_windows() returns [] both when the multiplexer session genuinely
has zero windows and when the underlying call just failed transiently
(subprocess contention, brief tmux server hiccup, etc). Both
resolve_stale_ids() (startup) and SessionMonitor._monitor_loop's poll
(runs every ~1-2s) fed that empty result straight into
prune_session_map() without distinguishing the two cases.

In production this let a single transient tmux failure during concurrent
multi-window activity wipe session_map.json/window_state for every
tracked window in one pass — permanently breaking Telegram routing for
all of them until their next SessionStart, since nothing else
re-populates those entries. Observed on a long-running instance with
several windows active simultaneously; log excerpt:

[error] tmux subprocess failed [libtmux.common]
[info ] Pruning dead session_map entry: ccgram:@4 (window @4)
[info ] Window '@4' deleted, removing session 7be1af36-...

@4 was a live, actively-used window the whole time — the tmux call
just hiccuped for a couple seconds.

Fix

Added is_suspicious_empty_window_list() in window_resolver.py: when
list_windows() reports zero windows but session_map/window_state
still expects some, treat it as a transient failure and skip pruning for
that cycle instead of trusting it.

prune_session_map() itself is left untouched — it's meant to be a pure
trust-the-input function (existing tests assert an explicit empty
live_window_ids set prunes everything), so the guard lives at the two
call sites that actually derive live_window_ids from the fallible
external call: session.py::resolve_stale_ids and
session_monitor.py::_monitor_loop.

Test plan

  • uv run pytest tests/ -m "not integration and not e2e" -n auto — 5915 passed (2 pre-existing failures unrelated to this change, reproduced identically on clean main: missing optional edge_tts dep, one flaky shell-command test)
  • New unit tests for is_suspicious_empty_window_list (3 cases) and a regression test reproducing the exact bug at resolve_stale_ids (test_empty_list_windows_does_not_wipe_session_map)
  • make arch-guard (boundary tests) — 784 passed
  • ruff check / ruff format --check / lint_lazy_imports.py / pyright on changed files — clean

…lure

get_session() swallows tmux/herdr subprocess errors into None, so
list_windows() returns [] both when the multiplexer session genuinely
has zero windows and when the underlying call just failed transiently
(subprocess contention, brief tmux server hiccup, etc). Both
resolve_stale_ids() (startup) and the SessionMonitor poll loop
(_monitor_loop, runs every ~1-2s) fed that empty result straight into
prune_session_map() without checking which case it was.

In production this caused a single transient tmux failure during
concurrent multi-window activity to wipe session_map.json / window_state
for every tracked window in one pass — permanently breaking Telegram
routing for all of them until their next SessionStart, since nothing
else re-populates those entries.

Add is_suspicious_empty_window_list(): when list_windows() reports zero
windows but session_map/window_state still expects some, treat it as a
transient failure and skip pruning for that cycle instead of trusting
it. prune_session_map() itself is left untouched — it's meant to be a
pure trust-the-input function (existing tests assert an explicit empty
live_window_ids set prunes everything), so the guard lives at the two
call sites that actually derive live_window_ids from a fallible
external call.
@NatBrian
NatBrian force-pushed the fix/prune-session-map-on-transient-list-windows-failure branch from c091c8e to 7742362 Compare July 3, 2026 08:43
@alexei-led

Copy link
Copy Markdown
Owner

Thanks @NatBrian for reporting and investigating this. The data-loss issue is real, but I’m closing this PR rather than merging it.

The patch is not a complete fix and does not align with ccgram’s multiplexer design: it uses a state-based heuristic to guess whether an empty listing is transient, instead of distinguishing a confirmed empty listing from a failed backend query at the multiplexer boundary. That leaves destructive paths such as /sync unprotected and can also retain stale state after a genuinely empty listing.

I’ve implemented the boundary-level fix separately and will release it as a patch. For future reports, please open an issue first with the repro and logs; that makes it easier to agree on the failure contract and approach before implementation.

Thanks again for surfacing this.

@alexei-led alexei-led closed this Jul 10, 2026
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.

2 participants