Skip to content

fix(gateway): cron cleanup on failure + session finalize guard - #12184

Open
jjjojoj wants to merge 4 commits into
NousResearch:mainfrom
jjjojoj:main
Open

fix(gateway): cron cleanup on failure + session finalize guard#12184
jjjojoj wants to merge 4 commits into
NousResearch:mainfrom
jjjojoj:main

Conversation

@jjjojoj

@jjjojoj jjjojoj commented Apr 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Two small bug fixes for the gateway runner:

#12175 — Cron ticker leaks on failure shutdown

start_gateway() returned early on should_exit_with_failure before calling cron_stop.set() / cron_thread.join(), leaking the daemon cron thread. Moved cleanup into a finally block so it always runs.

#12176/new emits on_session_finalize(session_id=None) with no prior session

_handle_reset_command() always fired on_session_finalize, even when old_entry was None (first session). Added a guard so the finalize hook only fires when there is an actual old session to finalize.

Test plan

  • Verify cron thread is joined on failure exit path
  • Verify /new on first session does not emit on_session_finalize
  • Verify /new on existing session still emits on_session_finalize with correct session_id

jjjojoj and others added 4 commits April 18, 2026 22:51
…esearch#12175)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…2294)

build_skills_system_prompt() loaded disabled set AFTER the cache check,
so config changes to skills.disabled silently had no effect until restart.
Move get_disabled_skill_names() before cache check and add sorted disabled
tuple to cache key so any change invalidates the cache.
NousResearch#12064)

Background task media delivery unpacked the (path, is_voice) tuple but
discarded the is_voice flag, sending all audio via send_document. Now
routes voice files through send_voice() when the adapter supports it.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery comp/cron Cron scheduler and job management labels Apr 23, 2026
@DavidMetcalfe

DavidMetcalfe commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Thanks for the contribution @jjjojoj. This PR is effectively superseded and can be closed. Here's why:

All three unrelated fixes have already landed on main:

The cron cleanup fix (Fix 4) is stale: The diff does not incorporate the additional cleanup steps now present on main (cron_provider.stop(), housekeeping_thread.join(), _planned_stop_watcher). The try: pass finally: approach also adds complexity beyond what's needed.

A clean replacement PR exists: PR #55031 applies the same core idea (running cleanup before returning on failure) as a minimal reorder of existing code — no new logic, no new patterns.

The diagnosis work here was useful for identifying the issue.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for identifying the failure-path lifecycle gap. The cron diagnosis is still valid on current main: gateway/run.py:20749-20752 returns before the cleanup beginning at gateway/run.py:20763.

Problems

  • The PR's synchronous cron_thread.join() is no longer compatible with the current lifecycle contract. Current main documents that joining there blocks the event loop and can drop an in-flight cron delivery; it instead uses cooperative _await_thread_exit() (gateway/run.py:20756-20775).
  • The bundled prompt-cache and background-media fixes are already present on main (agent/prompt_builder.py:1479-1489; gateway/run.py:13367-13401). The proposed finalize-hook guard conflicts with the documented nullable callback contract (gateway/slash_commands.py:222-234; website/docs/user-guide/features/hooks.md:837-845).
  • No regression test covers the failure return path.

Suggested changes

  • Salvage only the cron fix by moving the failure return after the existing cooperative cleanup sequence, then add a failure-path regression test.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-caching Sweeper risk: may break/degrade prompt caching or cache-key stability (invariant) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-massive Sweeper blast radius: massive — everyone, every turn (invariant surface) area/sessions Session lifecycle, resume, persistence, history labels Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:blast-massive Sweeper blast radius: massive — everyone, every turn (invariant surface) sweeper:risk-caching Sweeper risk: may break/degrade prompt caching or cache-key stability (invariant) sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants