fix(webui): bound /api/crons/recent so the 30s cron poll cannot time out

[Captain-Slaphead]

Thinking Path

startCronPolling in static/panels.js polls /api/crons/recent?since=... every 30 s with api()'s default 30 s client timeout. _handle_cron_recent enriches each completion with its cron session id by reading the agent state.db, and the page uses that id to raise the sidebar unread marker. If the enrichment is unavailable, the marker is the only thing lost — the completions list itself is built before enrichment and returned either way.

Three failure modes have now been reviewed:

1. The read was unbounded in time. The first revision ran the enrichment on a daemon worker under a 2 s join(). A fresh thread has no entry in api/profiles.py's thread-local, so get_active_hermes_home() resolved against the process profile, and a detached thread cannot be guaranteed to stop.
2. The lock-wait bound is not a deadline. The second revision deleted the worker and passed a 0.25 s timeout to sqlite3.connect. Review correctly noted that this bounds only the wait for the write lock: if the state database holds many cron sessions or the read itself is slow, the statement still runs past the client's 30 s budget, and deleting the worker removed its deadline without an equivalent replacement.
3. Holding the cursor replays completions. The second revision also left _cronPollSince untouched while session_lookup_failed was set. That keeps the completion eligible, but it re-serves it on every poll: opening a job to clear its unread badge only clears it until the next poll, and once the 50-entry toast memory evicts the older keys, their toasts appear again.

The two review rounds pull in opposite directions — round one requires not dropping a completion whose lookup failed, round three requires not holding it forever. The resolution is the middle path a maintainer suggested: advance the cursor, remember the un-enriched completions on the page, fetch their detail separately, and give up after a bounded number of attempts.

What Changed

Reworked again: the read gets a real wall-clock deadline, and the retry moves to the client.

• api/agent_sessions.py: open_state_db_readonly takes an optional deadline_s. It is installed as a sqlite3 progress handler that aborts the statement with sqlite3.OperationalError once the wall-clock budget is spent, so the read is bounded whatever the table size and whatever the lock contention. This is the piece the previous revision lacked. timeout still bounds the lock wait and is now set from the same budget.
• api/routes.py: _latest_cron_session_info_for_jobs takes deadline_s instead of busy_timeout_s, passes it to the connection, and adds a row cap to the scan (max(200, 8 × requested jobs)). A bounded failure still re-raises, so the handler can report session_lookup_failed; the unbounded default path is unchanged and still degrades to an empty result.
• static/panels.js: the poll now advances _cronPollSince past every completion it is handed, enriched or not. A completion whose lookup failed is recorded in _cronPendingDetails (bounded to 50 entries). _cronRetryPendingDetails re-requests that narrow window separately, writes the sidebar marker when the detail finally resolves, and drops the entry after 5 attempts. The toast dedupe set is gone: with a cursor that always advances, a completion is delivered once.
• tests/test_cron_recent_poll_timeout.py: rewritten around the new shape. Adds a proof that the progress-handler deadline aborts a slow scan, a proof that a fast read is unaffected, a row-cap assertion using a trace callback, two node harness tests for the page-side pending set (badge not resurrected, toast not repeated, give-up after the bound), and keeps the three regressions review asked for — named-profile lookup, timeout-then-success recovery, and no leftover threads.
• tests/test_cron_toast_notifications.py: stub signature follows the renamed parameter.

Why It Matters

The poll drives cron-completion toasts and the sidebar unread marker. A long cron job should not produce a spurious error toast, should not lose the completion it reports, and should not resurrect an unread badge the user has already cleared. The previous revision fixed the first and broke the third.

Verification

# regression proof — previous revision restored, new tests kept
HERMES_WEBUI_TEST_STATE_DIR=/workspace/.test-state ./scripts/test.sh \
  tests/test_cron_recent_poll_timeout.py tests/test_issue5960_cron_unread_profile_scope.py
→ 11 failed, 12 passed in 4.23s

# with the fix, same two files
→ 23 passed in 4.15s

# with the fix, every cron/session/state-db file touched or adjacent
→ 101 passed in 8.40s

The pre-fix failures are the bug reproduced, not a harness artefact. All four tests/test_issue5960_cron_unread_profile_scope.py cases fail on the previous revision — _resetCronUnreadForProfileSwitch cleared a toast-dedupe set that the test's extracted-function harness does not declare, so it raised ReferenceError before running. That is the same failure the project's own Tests check reported on b4aed07. The remaining seven are the new regressions, which cannot run against a revision that has neither deadline_s nor the page-side pending set.

The two node tests run the real extracted functions from static/panels.js and assert the two round-3 findings directly: the cursor advances to the completion's timestamp even when the lookup failed, a badge cleared by the user is not restored by the retry, and the toast count stays at 1 across the replay window.

Test state was isolated via HERMES_WEBUI_TEST_STATE_DIR outside any production home; no live state directory was touched.

Not verified: the browser rendering of the sidebar marker was not exercised in a browser. The page change is confined to the poll callback and its two helpers; it is covered by the node harness and node --check static/panels.js, and the repository has no browser harness for this panel.

Contract Routing

No contract family is touched. /api/crons/recent is not listed in docs/CONTRACTS.md. The response shape is extended additively with session_lookup_failed (boolean); completions, since, session_id and message_count keep their existing names and types.

Risks / Follow-ups

• A read that hits its deadline reports session_lookup_failed, the page remembers the completion and retries separately, and gives up after 5 polls. Worst case is a missing sidebar unread marker, never a lost completion or a repeated toast.
• The scan's row cap is a safety valve, not the deadline. It is sized so the newest row for each requested job is always inside it; anything it did truncate would be retried by the page rather than mis-reported.
• No index was added. Bounding the read by wall clock removes the need, and a schema change is a much larger ask for a first contribution. If maintainers would rather have the index, that is a follow-up.
• deadline_s=0.25 sits far inside the 30 s client budget. A future caller wanting a different bound should pass one rather than edit the call site.

Release Note Wording

Fix a spurious "Request timed out" toast that appeared while a long cron job was running, and stop a cron completion from re-raising its unread badge and toast on every poll when its session detail could not be read.

Model Used

Implemented by the maintainer-side agent (deepseek-v4.1-flash via Hermes Agent) working from the two review rounds' recommendations; the before/after regression proof and the full-file suite run were produced in the same session. AI usage disclosed per CONTRIBUTING.md.

[greptile-apps]

Confidence Score: 3/5

[Medium risk] Adds timeout handling to cron session lookup.

The PR is not yet safe to merge because completed jobs can permanently lose session markers through the global row cap or retry exhaustion.

Findings

1.  Session lookup silently drops jobs ▶
2.  Retries discard unread markers ▶
3.  Delayed retry restores unread dot ▶
4.  Runtime change lacks documentation ▶

Summary

The PR bounds cron-session enrichment with a SQLite progress deadline and changes the browser poll to advance its completion cursor while retrying failed session details separately.

• Adds server and Node-backed regression tests for the bounded lookup and retry flow.
• The global session-row cap can silently omit a completed job’s session; pending retries can also expire or restore a marker after the session was viewed.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A["30-second cron poll"] --> B["Recent completions"]
  B --> C["Bounded session lookup"]
  C -->|success| D["Mark session unread"]
  C -->|failure| E["Advance cursor; retain pending detail"]
  E --> F["Separate recent-completions retry"]
  F -->|session found| D
  F -->|five failed attempts| G["Discard pending detail"]

Loading

_{Reviews (3) · Last reviewed commit: "fix(webui): give the cron completion pol..."}

[nesquena-hermes]

Summary

I read the full diff at head 4425bfbd, the complete new regression file, and the existing cron-unread/profile tests on origin/master. Suppressing the generic timeout toast at static/panels.js:13097 is appropriate, but the new daemon-thread boundary in api/routes.py:22704-22737 breaks request-profile ownership and can permanently consume a completion before its session marker is available. I would not merge this head yet.

Code reference

_latest_cron_session_info_for_jobs resolves its database through _active_state_db_path() at api/routes.py:310-320. That path ultimately uses the request thread local described at api/profiles.py:484-497. The PR moves that lookup to a fresh thread without transferring the profile context:

worker = threading.Thread(
    target=_work, name="cron-recent-session-info", daemon=True
)
worker.start()
worker.join(budget_s)
return result

A new thread has no _tls.profile, so get_active_profile_name() falls back to the process profile. A named-profile tab can therefore receive an empty or wrong-profile session_id even when the read finishes inside the budget.

There is a second lifecycle issue at static/panels.js:13107-13114: the client advances _cronPollSince before checking c.session_id. If enrichment times out, the response still contains the completion with an empty session id, the cursor advances to completed_at, and the next poll excludes that completion. The missing sidebar unread marker cannot recover on the next cycle. This conflicts with the existing completion contract covered by tests/test_issue3460_cron_session_unread.py:486-527.

The overrun helper test at tests/test_cron_recent_poll_timeout.py:128-145 also starts a 30-second daemon worker with no release event. The earlier handler test cleans up its worker; this one does not, so it leaves background test activity after teardown.

Diagnosis and recommendation

Please avoid resolving profile-scoped state inside an unscoped detached thread. The smallest reliable shape is to capture the active profile on the request thread and pass it explicitly into the lookup, resolving the database with get_hermes_home_for_profile(profile) rather than ambient thread-local state. Better still, bound the SQLite read itself with a short busy timeout so the lookup stays synchronous and no orphan worker can accumulate.

The completion cursor also needs retry semantics. If session enrichment is pending, keep that completion eligible for a later poll while deduplicating its toast by (job_id, completed_at). Do not permanently advance past it until the session lookup succeeds, or return an explicit pending marker and maintain a client-side pending set.

Test plan

Add regressions that:

1. Set a named request profile whose state database differs from the process default and assert the returned session belongs to that named profile.
2. Force the first enrichment attempt to time out, let the second succeed, and assert the completion toast appears once while the sidebar unread marker is eventually written.
3. Release and join every blocking test worker before teardown, then assert no cron-recent-session-info worker remains alive.

[Captain-Slaphead]

Reworked at b4aed07 to drop the worker thread entirely, along the lines you suggested.

• open_state_db_readonly takes an optional timeout, forwarded to sqlite3.connect; the cron path reads synchronously with a 0.25 s bound, so no orphan worker can exist and none can accumulate. _cron_recent_session_info_best_effort is gone.
• _latest_cron_session_info_for_jobs re-raises its sqlite3.Error when a bound is supplied, and the handler reports session_lookup_failed so the failure is distinguishable from a genuinely empty result.
• static/panels.js no longer advances _cronPollSince while that flag is set — the completion stays eligible on the next poll — and toasts are deduplicated by (job_id, completed_at), bounded to 50 entries. A completion with no session still advances once the lookup succeeds, so nothing is trapped.
• Tests: test_named_request_profile_uses_its_state_db covers your first point and fails against the threaded head; test_failed_lookup_is_retried_without_advancing_completion_timestamp covers the retry with a stable completion timestamp; the leaking overrun test is replaced by a no-extra-threads assertion.

Verification: 68 passed across the seven cron/session/state-db files; the new test file is 4 failed / 1 passed against the threaded head.

[Captain-Slaphead]

Reworked at the new head along the lines both rounds pointed to, plus the test failure.

The failing project test is ours, and it is fixed. _resetCronUnreadForProfileSwitch called _cronToastedCompletionKeys.clear(). The #5960 tests lift that function into a hand-built Node harness that does not declare the toast-dedupe set, so the call raised ReferenceError before the test body ran. All four test_issue5960_cron_unread_profile_scope.py cases fail on b4aed07 and pass now. The set is gone rather than guarded: with the cursor always advancing, a completion is delivered once, so no dedupe is needed.

P1 — the poll still lacked a deadline. Fixed with a real one. open_state_db_readonly now takes deadline_s and installs a sqlite3 progress handler that aborts the statement with sqlite3.OperationalError once the wall-clock budget is spent, so the read is bounded whatever the table size and whatever the lock contention — timeout only ever bounded the lock wait. The same budget is passed as the connect timeout, and the scan carries a row cap (max(200, 8 × requested jobs)). New test test_lookup_deadline_bounds_the_scan_not_just_the_lock_wait aborts a 200M-row recursive scan in well under the budget, and test_lookup_deadline_does_not_trip_a_fast_read shows a fast query is unaffected.

P1 — failed lookups replayed completions. Fixed by moving the retry to the page. The cursor now advances past every completion it is given, enriched or not. A completion whose lookup failed is recorded client-side in a bounded map, its detail is re-fetched in a separate narrow request over that window, and the entry is dropped after 5 attempts. Two node tests run the extracted functions and assert exactly the reported symptoms are gone: the cursor reaches the completion's timestamp even when the lookup failed, a badge the user cleared is not restored by the retry, and the toast count stays at 1 across the replay window. The give-up test shows the pending entry disappearing after the bound rather than replaying forever.

Regressions asked for: named-profile lookup (test_named_request_profile_uses_its_state_db), timeout-then-success recovery (test_failed_lookup_is_retried_and_recovers), and no leftover threads (test_cron_recent_does_not_leave_extra_threads) are all present. The row cap has its own assertion via a trace callback.

Evidence, same two files:

• previous revision restored, new tests kept: 11 failed, 12 passed — including all four #5960 cases
• with the fix: 23 passed
• wider cron/session/state-db set (9 files): 101 passed

No index added. Bounding the read by wall clock removes the need, and a schema change is a large ask for a first contribution. Happy to add one as a follow-up if you would rather have it.

Not verified: the sidebar marker was not exercised in a browser; there is no browser harness for this panel, so the page change rests on the node tests plus node --check.

[greptile-apps]

Session lookup silently drops jobs If other jobs have more than 200 newer cron sessions, this global limit excludes a completed job’s session before the code matches rows to jobs. No error is raised, so the response says the lookup succeeded. The client then advances its cursor without retrying, permanently losing that job’s session unread marker. The same limit also appears in the alternate query.

[greptile-apps]

Retries discard unread markers If the state database stays busy across five polls, this code deletes the pending completion after five failed lookups. The main cursor has already advanced past it, so a later successful lookup cannot recover its session unread marker.

[greptile-apps]

Delayed retry restores unread dot If a user opens a session and leaves it before this retry succeeds, the retry marks it unread again. The background check only skips a session that is actively viewed at retry time, so the user sees a stale unread dot until they open the session again.

[greptile-apps]

Runtime change lacks documentation The endpoint now signals failed enrichment so the browser can retry details separately after advancing its cursor. That changes runtime behavior and the user-facing polling workflow, but no documentation was updated. AGENTS.md requires documentation updates for those changes; this repository requirement needs to be satisfied before merging.

Context Used: AGENTS.md (source)

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

[nesquena-hermes]

Summary

I reviewed the current 17530a948 head and the updated timeout, route, client-retry, and regression-test paths. The progress-handler deadline now bounds the SQLite statement itself, and advancing the primary cursor avoids replaying the completion toast. One correctness gap remains: the new hard row cap can silently omit the requested job, yet that outcome is returned as a successful lookup. Because the page only records pending detail when session_lookup_failed is true, this omission is permanent rather than retried.

Code reference

api/routes.py:330-401 caps the newest global cron rows, fills empty defaults for every requested id, and returns them without reporting whether the cap was exhausted. The default construction is:

cur.execute(query)
results = {
    jid: {"session_id": "", "message_count": None} for jid in requested
}
requested_ids = set(requested)

The queries at api/routes.py:354-370 order all cron sessions newest-first and apply LIMIT {row_cap} before matching ids. If job A completed, then more than 200 sessions for other jobs were created, A is outside the window. The result for A stays empty without raising. _handle_cron_recent therefore leaves session_lookup_failed false at api/routes.py:22767-22792, while static/panels.js:13186-13194 only calls _cronRememberPendingDetails when that flag is true.

The test at tests/test_cron_recent_poll_timeout.py:268-301 does not cover this shape: all 5,000 rows belong to the requested job, so the newest capped row necessarily matches.

Diagnosis / recommendation

Please make cap exhaustion with unresolved requested ids explicit. A minimal safe shape is to fetch up to row_cap + 1, track whether the candidate window was truncated, and raise or return an incomplete marker whenever any requested result is still empty and more rows exist. The route can then set session_lookup_failed=true, allowing the bounded page retry to do what the comments promise. An indexed or per-request-id lookup would be better if the session-id encoding can support it without a full scan.

Do not treat every empty result as failure: a completed job may genuinely have no persisted session. The signal needs to distinguish a fully scanned result from a truncated candidate window.

Test plan

Add a fixture with one target completion older than 200 newer cron rows belonging to different jobs. Assert that the first capped response is marked incomplete, that the client remembers the target, and that a later successful detail lookup clears it without replaying the badge or toast. Also keep a fully scanned no-session case that returns an empty session_id with session_lookup_failed=false.

[nesquena-hermes]

Thanks, @Captain-Slaphead. This round fixes the earlier review points well: the enrichment now has a real read deadline on the request thread, and the retry list closes the replay-on-held-cursor problem. Gate review at exact head 17530a948e25, rebased clean onto current master (exp-v0.52.383); threat scan clean. Two new issues from the tightening, both verified.

1. SILENT: a global row cap can silently drop a completed job's session. Both lookup queries (api/routes.py:~354-370) now end in LIMIT {row_cap}, with row_cap = max(200, 8 * len(requested)), applied to all cron sessions before filtering to the requested jobs. On a box where other cron jobs produced 200+ newer sessions, the target session is outside the window, the lookup returns session_id: "" with session_lookup_failed: false, and the page never retries, so that completion never gets its unread marker. Codex reproduced it: master finds the target behind 201 newer rows, this head returns empty. Fix: drop the global LIMIT and let the read deadline you added be what bounds the query (on timeout, report session_lookup_failed: true so the page retries), or restrict to the requested job IDs before limiting. Either way, an incomplete lookup must not report success.

2. SILENT: a delayed retry can bring back an unread dot the user already cleared. _cronRetryPendingDetails() (static/panels.js:~13138) calls _markSessionCompletionUnreadIfBackground() with the message count from the retry response. That helper checks whether the session is currently open, but not whether the user already saw that count: open the cron session, leave it, and the next retry marks it unread again. Codex reproduced this with a Node probe of the real functions. Fix: before marking on retry, compare the returned message_count with the session's acknowledged/read count and skip when it's already been seen. Add a visit-then-leave-then-retry test.

Re-push and I'll re-gate.

[Captain-Slaphead]

Reworked at ae6e253c on both points.

Global cap dropped. The candidate window is no longer capped. A LIMIT over all cron rows was applied before matching the requested jobs, so a job whose session sat behind newer rows from other jobs came back empty while the lookup still reported success — and the page only retries when session_lookup_failed is true, so that marker was lost for good. The wall-clock deadline added last round is now the bound: a scan too slow to finish aborts and re-raises, so the caller reports session_lookup_failed and the page retries. Verified against your fixture shape — one target older than 5000 newer rows belonging to a different job is now found, and a fully scanned lookup with no persisted session still returns an empty session_id with session_lookup_failed=false.

A delayed retry no longer restores a cleared dot. _markSessionCompletionUnreadIfBackground now skips the mark when the session's viewed count already acknowledges that message count in the same transcript generation. Opening a cron session, leaving it, and letting the retry resolve no longer re-flags it; a genuinely newer completion still flags.

Tests: test_lookup_finds_target_behind_newer_rows_from_other_jobs, test_lookup_fully_scanned_missing_session_is_empty, test_cron_recent_no_session_is_not_a_failed_lookup, and test_visit_then_leave_then_retry_does_not_restore_unread_dot. The two new behavioural tests fail on the previous head and pass now. 50 cron/unread/state-db files: 385 passed, 6 skipped.

Not verified: ruff and eslint are not installed in my environment (CI installs them), so lint rests on the CI run.

[nesquena-hermes]

Re-gated ae6e253ce3d4 rebased onto master 8ac497df3, against my 09-29 review.

Prior findings: resolved. A target behind 5,000 newer rows is found, and a delayed retry no longer restores a dot the user has already viewed. 7 targeted tests pass and both changed JS files pass node --check.

Two ways a cron completion can still lose its session unread marker:

1. (SILENT) A slow lookup gets discarded after five tries. The new per-request deadline (api/routes.py ~22777) is good for the poll, but the lookup it bounds still scans and sorts the session table before filtering by job. On a large state.db (an isolated Agent-shaped database with 200,000 newer sessions from other jobs) the target is found in ~0.75 s without the deadline, and every 0.25 s attempt aborts. After _CRON_PENDING_ATTEMPTS (5) failures, static/panels.js ~13153 deletes the pending detail, so that completion never gets its dot. Fix: narrow the SQL to the requested job ids first, so the index does the work and the scan stays inside the deadline. Don't discard unresolved details after a fixed number of tries; back off instead.

2. (SILENT) More than 50 failed enrichments evicts one for good. _cronPendingDetails is capped at _CRON_PENDING_MAX (50) and evicts the oldest (static/panels.js ~13114). The poll cursor has already advanced past those completions, so an evicted entry is never retried. 51 failed completions drops the first one; list_jobs() has no 50-job cap. Fix: keep every unresolved completion and bound the retry request instead (process them in batches of N).