Review — end-to-end ⚠️ → ✅ (deadlock caught and fixed; pushed 604b44a)

Release v0.50.249 — bundle of 5 community PRs (plus #1356 already approved separately). The trace caught a production-crashing deadlock in the new clarify SSE handler (#1355). Fix pushed; release now safe to ship.

Squash audit

bbdacdc  #1356  fix: context window indicator overflow             (already approved on #1356)
70dac01  #1357  fix: preserve imported session source metadata     @dso2ng
f13230f  #1358  fix: collapse sidebar session lineage rows         @dso2ng
6a73680  #1359  fix: sync active session across tabs               @dso2ng
d2d464a  #1355  feat(clarify): SSE long-connection                 @fxd-jason
36d87f5         release: v0.50.249 (CHANGELOG only)
604b44a         fix(clarify-sse): inline snapshot under _lock to avoid deadlock  ← my fix

All 5 contributor commits preserve Author: + Co-authored-by: trailers. ✅

What I caught — clarify SSE handler deadlocks the moment any client connects

PR #1355 ships a new _handle_clarify_sse_stream route handler at api/routes.py:2888-2935. The original code:

with _clarify_lock:                                # acquire _lock
    _clarify_subs.setdefault(sid, []).append(q)
    initial_pending = get_clarify_pending(sid)     # ← calls clarify.get_pending
    initial_count = 1 if initial_pending else 0

But api/clarify.py:147-154:

def get_pending(session_key: str) -> dict | None:
    with _lock:                                    # ← re-acquires the SAME lock
        queue = _gateway_queues.get(session_key) or []
        if queue:
            return dict(queue[0].data)
        ...

clarify._lock = threading.Lock() is non-reentrant (api/clarify.py:16). The same thread calling acquire() twice deadlocks indefinitely. The first EventSource connection to /api/clarify/stream would have hung the SSE handler thread forever.

The existing test suite missed this because all 29 of tests/test_clarify_sse.py's tests exercise sse_subscribe/sse_unsubscribe/_clarify_sse_notify/submit_pending directly — none of them invoke the route handler. CI was green because the deadlock only manifests when a real EventSource opens the connection.

Reproduced with a 30-line harness:

DEADLOCK CONFIRMED: thread is blocked after 2s

After the fix, both empty-queue and populated-queue snapshots complete in <1ms.

What I pushed — 604b44a

Commit 604b44a:

• api/routes.py — replaced the get_clarify_pending(sid) call with inline reads of _gateway_queues and _pending under the same _lock acquisition. Mirrors the approval SSE handler's pattern at api/routes.py:2785-2793. Returns the same head-of-queue dict that get_pending would have returned.
• tests/test_pr1355_sse_handler_no_deadlock.py — 3 new tests:
  1. test_handler_snapshot_does_not_deadlock_when_queue_is_empty — runs the handler's snapshot logic in a worker thread with a 2s timeout; asserts no deadlock and (None, 0).
  2. test_handler_snapshot_does_not_deadlock_when_queue_has_entry — primes the queue via submit_pending, runs the snapshot, asserts the head entry is captured.
  3. test_routes_handler_does_not_call_get_pending_under_lock — source-level invariant: parses _handle_clarify_sse_stream, walks the with _clarify_lock: block by indentation, asserts get_clarify_pending( is NOT inside it. Locks the regression in.

Traced against upstream hermes-agent

agent.model_metadata.get_model_context_length (used in #1356 fallback) and register_gateway_notify / unregister_gateway_notify (consumed by clarify SSE) verified at /tmp/hermes-agent-fresh/tools/approval.py:382-401. The new is_cli_session/source_tag/session_source/source_label fields from #1357 are webui-only — upstream uses source_label in unrelated contexts (skills hub, web_server credentials), not as Session attributes. ✅ Cross-tool clean.

End-to-end trace — clarify SSE (post-fix)

1. Client connects to /api/clarify/stream?session_id=… (static/messages.js:1681).
2. Auth gate via check_auth(self, parsed) in server.py:76 before handle_get.
3. Route dispatch api/routes.py:1274 → _handle_clarify_sse_stream.
4. Atomic subscribe + snapshot under _clarify_lock: append queue, read _gateway_queues[sid][0].data (or fall back to _pending[sid] if list-shape isn't there), release lock.
5. SSE response headers + initial _sse(handler, 'initial', …).
6. q.get(timeout=30) loop with keepalive comments; _CLIENT_DISCONNECT_ERRORS + finally: unsubscribe on close.
7. submit_pending at api/clarify.py:138 calls _clarify_sse_notify from inside _lock — head-of-queue dict(gw_queue[0].data), count len(gw_queue). Notify-ordering preserved.
8. resolve_clarify at api/clarify.py:170-175 — pops entry, then if queue has more, notifies new head; if queue empty, notifies (None, 0) so frontend hides the card. Trailing-clarify-stuck bug solved.

This pattern incorporates all three v0.50.248 #1350 lessons (notify under lock, head-of-queue fidelity, post-pop re-emit) — exactly as the PR description claims. The only gap was the recursive lock in the handler, which my fix closes.

End-to-end trace — context indicator (#1356)

Already traced in PR #1356 review. Identical content; behavioural harness re-confirmed.

End-to-end trace — session metadata (#1357)

is_cli_session, source_tag, session_source, source_label added to:

• api/models.py:350-353 — Session.__init__ reads from kwargs (backwards-compatible default False/None).
• api/models.py:374 — METADATA_FIELDS round-trip (save → load_metadata_only → still present).
• api/models.py:470-473 — compact() returns them in sidebar/API payloads.

Population sites:

• api/routes.py:4448 sets is_cli_session=True for imported CLI sessions (pre-existing).
• The other three fields (source_tag, session_source, source_label) aren't actively populated yet by webui code — they're plumbing for future PRs. Not a regression because previously they were never read or written either.

CLI: doesn't touch these fields. ✅

End-to-end trace — lineage collapse (#1358)

Pure client-side at static/sessions.js:1219-1234. _collapseSessionLineageForSidebar groups by _lineage_root_id/lineage_root_id/parent_session_id, picks the latest tip per group by _sessionTimestampMs, attaches _lineage_collapsed_count for future UI affordances (currently set but not consumed — fine).

Edge cases:

• Empty input → []. ✅
• Single session, no lineage → kept as-is. ✅
• Two sessions same lineage → latest kept, count=2. ✅
• Mix of lineage and non-lineage sessions → non-lineage retained verbatim, lineage groups collapsed. ✅
• Iteration order: Map preserves insertion order, so non-lineage sessions appear before lineage groups. The caller's downstream sort should re-order by timestamp.

End-to-end trace — cross-tab sync (#1359)

static/sessions.js:1631-1645. storage event listener fires when ANOTHER tab sets localStorage['hermes-webui-session']. The handler:

1. Skip if not the right key, or no value, or already on this session.
2. Skip and toast if S.busy — never interrupt an active turn. ✅
3. Otherwise await loadSession(sid) + re-render sidebar.

Backed by the regression test test_storage_sync_does_not_switch_while_busy which asserts the if(S.busy) guard exists.

Cross-tool consistency

• Clarify SSE: webui-only mechanism (_clarify_sse_subscribers is module-level state). Upstream tools.approval.gateway_notify already pushed clarify events via the chat/stream SSE; this is an additive secondary path. ✅
• Session metadata fields: webui-only Session attributes; CLI uses state.db, doesn't read these. ✅
• Lineage / cross-tab: JS only, frontend-internal. ✅
• Context indicator (#1356): already cleared in prior review. ✅
• No config.yaml writes. ✅

Race / lock analysis

• Clarify SSE handler (post-fix): subscribe + snapshot atomic under _clarify_lock. No recursive acquisition. No notify-ordering race (notify called inside submit_pending's lock block). Trailing-clarify re-emit handles pop-empty case.
• _clarify_sse_notify is called inside _lock at api/clarify.py:138 (submit_pending), api/clarify.py:172 and api/clarify.py:175 (resolve_clarify). It does q.put_nowait(payload) while holding _lock. Queue's internal mutex is briefly acquired; no inversion (no other path holds queue.mutex while waiting on _clarify_lock). Safe.
• q.put_nowait on full queue: catches queue.Full and silently drops. No blocking under the lock. ✅
• Cross-tab storage listener: runs on the main JS thread, no shared mutable state outside S. The S.busy guard prevents mid-turn interruption.

Security audit

• ✅ session_id from parse_qs — used as dict key only, no path/SQL/shell.
• ✅ Auth gated via check_auth in server.py:76 for ALL GET routes.
• ✅ _sse payload via json.dumps(data) — no XSS.
• ✅ Frontend JSON.parse(ev.data) then property access — no innerHTML.
• ✅ Bounded queue (maxsize=16) prevents memory exhaustion from slow clients.
• ✅ Cross-tab listener gated on key match ('hermes-webui-session') — no untrusted keys leak in.
• ✅ loadSession(sid) in cross-tab handler — sid comes from localStorage which is same-origin, but the API call already validates the session exists (404 returned otherwise).
• ✅ Session metadata fields pure data attributes; no executable surface.

Edge-case matrix

Tests

• New regression tests I added: 3/3 pass in tests/test_pr1355_sse_handler_no_deadlock.py.
• PR #1355's own tests: 29/29 pass in tests/test_clarify_sse.py.
• Other PRs: test_session_lineage_collapse.py 1/1, test_session_cross_tab_sync.py 2/2, test_gateway_sync.py::test_imported_cli_session_metadata_survives_compact pass.
• Full suite: 3395 passed, 54 skipped, 3 xpassed, 0 failed in 16.01s on 604b44a.
• CI: Was green on 36d87f5 for 3.11/3.12/3.13. Will need to re-run on 604b44a for the new commit.

Minor observations (non-blocking)

• _lineage_collapsed_count is set but not read anywhere. Future UI affordance — fine, just dead state for now.
• Three of the four new metadata fields aren't populated by any current code path (source_tag, session_source, source_label). PR #1357 only adds the round-trip plumbing — population is presumably a follow-up. Not a regression because they were never populated before either.
• PR description claims "3444 passed"; my local count is 3395 (3392 pre-fix baseline + 3 new tests I added). The discrepancy is consistent with prior releases — likely subtest counting drift.
• Clarify dedup case at api/clarify.py:115-131 does NOT call _clarify_sse_notify. This is correct: the head-of-queue didn't change, so no event is needed for already-subscribed clients. New subscribers still get the head via the initial snapshot. ✅
• 60s health-timer reconnect in clarify frontend at static/messages.js:1715-1722 — unconditionally tears down and re-subscribes every 60s, even if the connection is healthy. This is more aggressive than approval SSE's 5s S.busy health check. Probably fine (60s churn is light), but could be refined to only reconnect if no event received in window. Out of scope.

Recommendation

✅ Approved after fix. The deadlock was a critical regression that would have manifested the first time any tab opened the clarify SSE stream — caught it via static-trace inspection of the _lock acquisition graph (the source-level test test_routes_handler_does_not_call_get_pending_under_lock locks this in). Fix mirrors the approval SSE handler's correct pattern. All other PRs in the bundle (#1356/1357/1358/1359) traced clean.

Parked at approval after 604b44a — ready for the release agent's merge/tag pipeline once CI re-runs green on the new commit.