fix: drop stale background process session notifications - #16455
Conversation
|
Context for reviewers: this is one of the small follow-up slices from the closed broad host-support PR #13370. Teknium salvaged the compression-boundary signal into #16306, and the remaining host-side pieces from #13370 are being resubmitted as narrow, independent PRs instead of reviving the original large PR. This slice is intentionally limited to its described scope; it does not vendor or bundle any external context-engine implementation. |
bcbb1e0 to
8f92ee6
Compare
8f92ee6 to
9fc919f
Compare
3afc50d to
dc2fc62
Compare
|
Refreshed this against latest Why it is still needed:
What changed in the refresh:
Validation:
Full suite note: I also tried @teknium1 this one should be ready for another look. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused session-boundary fix. The underlying issue still exists on current main: _run_process_watcher() routes by session_key (gateway/run.py:15438-15543), while /new replaces the physical ID for that same key (gateway/session.py:2089-2133).
Problems
- Checkpoint persistence is ordered before the new metadata is populated.
spawn_local()writes the checkpoint attools/process_registry.py:745/797; the PR assignsconversation_session_idlater intools/terminal_tool.py:2543(PR right side) without a subsequent write. A crash in that interval recovers an empty ID and the stale guard cannot suppress the event.
Suggested changes
- Flush the checkpoint after all watcher metadata and notification fields are assigned, and add a lifecycle regression that spawns, stamps, recovers, and verifies the captured physical session ID.
Automated hermes-sweeper review.
dc2fc62 to
3dd74b9
Compare
|
@teknium1 Thanks — your checkpoint-ordering review was right. While hardening that path, I also found and fixed two related session-boundary races:
I added deterministic regressions for spawn → stamp → recover, concurrent checkpoint writers, missing/store-failing ownership, and reset-after-initial-validation races. Current focused validation: 232 passed, plus compileall, Ruff, and |
3dd74b9 to
98a4aec
Compare
|
@teknium1 Final patch is up. It closes the stale-session notification races across adapter draining, command cancellation, and local/proxy shutdown ownership, with focused regression coverage. Ready for your review. |
… boundary Plain type=completion events built in _run_process_watcher carried only session_key (chat/thread routing) with no spawning-session stamp, so after /new (or a session switch) a completion notification from the OLD session was injected into the chat's NEW session. Main already solved this exact class for async delegations via the _classify_completion_target pre-flight (_USER_BOUNDARY_END_REASONS drop on user-closed sessions, deliver on idle-ends, follow the compression-tip chain), but the gate only ran for type=async_delegation events. Kernel salvage of #16455: - Stamp the spawning conversation's session-db id (HERMES_SESSION_ID via session-scoped env) on the ProcessSession and the pending_watchers entry at spawn time in tools/terminal_tool.py; persist it through the process registry checkpoint/restore so recovered watchers keep the stamp. - Thread the stamp into the completion_evt built by _run_process_watcher (watcher entry first, ProcessSession fallback for recovered watchers). - In _deliver_completion_notification, run the SAME pre-flight classifier for stamped type=completion events: terminal -> drop with a log (output stays available via process(action='log')), retry -> False so the watcher re-polls, deliver -> proceed. The policy has exactly one owner (_classify_completion_target); nothing is forked. Unstamped legacy events keep today's deliver-always behavior, and the async-delegation path is untouched. Based on the session-boundary approach from #16455 by @Tosko4 (original PR was over-scoped across adapters/slash-commands/cron; this lands the kernel only). Tests: completion from a /new-closed session is dropped; completion after an idle-end still delivers; unstamped legacy event delivers; retry verdict returns retryable False without adapter injection; async_delegation gate unchanged; stamp survives checkpoint recovery.
|
Thanks @Tosko4 — the session-boundary kernel of this PR landed in #85939 (rebase-merged): background-process watchers now stamp the spawning session id at spawn and completions are gated through the existing _classify_completion_target policy, so completions from a /new-closed session are dropped. The broader delivery-boundary locks across adapters/cron were deliberately not carried (superseded by the async-delegation boundary machinery that landed since). Credited in the PR body. Closing. |
Summary
session_idthrough session context asHERMES_SESSION_IDWhy
Background process notifications can arrive after a user starts a fresh session. Routing by chat/thread alone is not enough: the event may belong to an older physical session while the session key now points at a new one.
Keeping the spawning
session_idlets the gateway suppress stale synthetic turns and text-only watcher messages instead of injecting old process output into the new session. Compression continuations are handled separately so a process spawned before auto-compression can still notify the live continuation.Related Issue
No separate issue. Follow-up slice from closed broad host-support PR #13370; complementary to merged compression-boundary PR #16306.
Type of Change
Changes Made
gateway/run.py: drops stale or unverifiable stamped process notifications usingsession_key+ capturedconversation_session_id, preserves compression continuations, pins the expected process session into synthetic agent events, revalidates after Telegram topic-binding routing overrides and awaited pre-agent setup, and gives stamped process turns a narrow side-effect-free handler path: no inbound enrichment, lifecycle hooks, tool/status/notice/voice/progress/interim callbacks, streaming, generic stable-key postprocessing, goal continuation, or stale response handoff. Stamped notifications wait behind an active foreground turn without steering or interrupting it; while a stamped process agent is executing, ordinary foreground input is queued under both busy-entry paths rather than being steered into or used to interrupt the synthetic turn; dangerous-command approvals auto-deny without a platform prompt in a generation-owned approval namespace; and model-producedMEDIA:directives are intentionally suppressed because this synthetic narrow path is text-only and cannot deliver local files, images, voice, video, or documents. After executor completion, stamped turns return immediately through this narrow result path without heartbeat/inactivity sends, fallback persistence, pending-message dequeue, direct queued-response sends, or recursive foreground follow-up execution. Stamped final delivery runs directly through a per-session boundary lock shared with all session lookups that can auto-reset an expired session, explicit reset/resume/branch switching, and/stop//newgeneration invalidation, closing the last check-to-adapter.send()race for agent and text-only notifications. Process watcher delivery reconstructs named-profile sources from multiplexedagent:<profile>:...keys and selects that profile's adapter credentials, while retaining default-adapter compatibility for legacy unstamped watchers. It defers model-backed transcript hygiene for synthetic process turns and gates local/proxy model startup on both physical ownership and run generation. Reset invalidates generation before reading the execution slot; process-triggered local/proxy runs expose a concrete interruptible handle plus an identity-owned stamped-process marker before their final execution-boundary check; both identities remain published through backend completion and surrounding synthetic-turn postprocessing, so ordinary foreground input stays behind the existing adapter guard until stamped release and is then drained exactly once instead of adapter hot-looping, steering, or interrupting; explicit/stopand/newcan still cancel it, and both identities are generation/identity-CAS-published and identity-cleared together only at the true outer turn boundary. Stamped agent-cache publication is a generation-checked compare-and-set inside the cache lock, stale cleanup identity-evicts only its own cached agent, and stale process runs skip all postprocessing. Old process finalizers generation-guard slot release; one-shot MoA restoration uses exact installed-override ownership so/stoprestores the temporary override while/newreplacement state wins.gateway/slash_commands.py: routes session lookups (including auto-reset), delayed model-picker callback persistence and preflight-warning history reads, explicit reset, resume, and branch session-store mutations through the same process-delivery boundary lock.hermes_cli/context_switch_guard.py: makes gateway model-switch warning enrichment async and reads the resolved physical session history inside the runner-owned boundary instead of directly invoking the transition-capable synchronous store.gateway/platforms/base.py: exposes a runner-owned async session resolver so adapter middleware can perform transition-capable lookups through the same delivery boundary; secondary-profile adapters receive a profile-bound resolver before middleware runs so lookup and lock keys remain inagent:<profile>. When a foreground adapter task discovers an out-of-band stamped owner, it retains the adapter guard and pending-drain ownership until that stamped lifecycle releases it, then drains the retained foreground event exactly once instead of entering a recursive redispatch loop or exposing an ownerless replay gap.gateway/platforms/yuanbao.py: routes recall redaction/patching, observed-group transcript writes, quote-media lookup, and observed-media hydration through that boundary-aware resolver instead of direct synchronous store lookups.plugins/platforms/slack/adapter.py: makes Assistant thread session seeding async and boundary-resolved.plugins/platforms/telegram/adapter.py: makes unmentioned-group transcript observation async and boundary-resolved across text, location, and media handlers.cron/scheduler.py: seeds continuable thread/channel cron sessions through the live adapter's profile-bound resolver on the gateway loop and appends the seed transcript within the same delivery-boundary critical section, preventing cron auto-reset/profile races without adding a second unguarded mirror lookup.tools/terminal_tool.py: stamps background process metadata with the spawning conversation session id and flushes the completed notification configuration after spawn so crash recovery does not retain the earlier empty metadata.tools/process_registry.py: persists and recoversconversation_session_idfor background watcher checkpoints/events and serializes complete checkpoint generations so an older concurrent snapshot cannot overwrite newer metadata.tests/gateway/test_background_process_notifications.py: adds stale-boundary, missing-owner/store-failure, compression-continuation, post-validation Telegram topic-rebind, barrier-based reset-during-pre-agent and cache-publication regressions, callback/streaming side-channel suppression checks, direct guarded agent-result delivery coverage, deliberate media-only suppression, proof that stamped turns cannot dequeue or recursively execute queued foreground messages, delivery-first/reset-first boundary-lock schedules, an auto-resetting session-lookup race, a real Yuanbao middleware lookup-versus-delivery schedule, a production busy-session non-interruption regression, a realBasePlatformAdapterregression proving one retained foreground dispatch and one post-release normal turn without recursive task churn, stale deferred-replay suppression, stale local-handle publication CAS preserving replacement ownership, both foreground busy-entry paths retaining input behind a stamped process agent, named-profile adapter selection across injected/completion/running rails, a cron-seed-versus-guarded-delivery lock schedule, a model-switch-warning lookup-versus-delivery lock schedule, queued stamped-event retry coverage, approval auto-denial without visible delivery, and replacement approval-state survival after stale process cleanup.tests/gateway/test_proxy_mode.py: proves a stamped process-triggered proxy request exposes a concrete interruptible task handle, publishes the matching stamped ownership marker, queues ordinary foreground input without cancellation in interrupt and steer modes, and cleans both identities after completion or explicit reset cancellation.tests/gateway/test_session_state_cleanup.py: proves reset-owned cleanup plus generation-guarded old-turn unwind preserves a newer execution slot, and identity cache eviction cannot remove a newer agent.tests/gateway/test_moa_one_shot_restore.py: proves ownership-based restoration removes the still-owned temporary MoA override after/stop, while stale unwind cannot restore old overrides or evict replacement-session cache state after/new.tests/gateway/test_slack.py,tests/gateway/test_telegram_group_gating.py, andtests/gateway/test_multiplex_phase0.py: prove adapter middleware awaits the injected resolver rather than the synchronous store and secondary-profile resolution uses the correct profile namespace.tests/gateway/test_model_picker_persist.py: proves the delayed picker callback uses the boundary-guarded session lookup before persisting its selected model.tests/tools/test_process_registry.py: asserts checkpoint write/recovery preservesconversation_session_id, including a real spawn → stamp → recover lifecycle regression and an ordered concurrent-writer regression.tests/cron/test_scheduler.py: covers resolver-based thread/channel seeding, exact reply-key compatibility, in-boundary transcript append, and standalone fallback behavior.How to Test
notify_on_complete=Truefrom a gateway session.session_keypoints to a different physical session.Validation
origin/mainf813c7ddad6f7a4973f83737d36d5e11a5cbbe50; no additional local post-rebase test cycle was run.python3 -m compileall -q gateway/run.py gateway/slash_commands.py gateway/platforms/base.py gateway/platforms/yuanbao.py hermes_cli/context_switch_guard.py cron/scheduler.py plugins/platforms/slack/adapter.py plugins/platforms/telegram/adapter.py tools/process_registry.py tools/terminal_tool.py tests/gateway/test_background_process_notifications.py tests/gateway/test_proxy_mode.py tests/gateway/test_model_picker_persist.py tests/gateway/test_multiplex_phase0.py tests/gateway/test_slack.py tests/gateway/test_telegram_group_gating.py tests/tools/test_process_registry.py tests/cron/test_scheduler.pygit diff --check./scripts/run_tests.sh tests/gateway/test_background_process_notifications.py tests/gateway/test_proxy_mode.py tests/gateway/test_tool_response_drop_recovery.py tests/gateway/test_session_state_cleanup.py tests/gateway/test_moa_one_shot_restore.py tests/gateway/test_agent_cache.py tests/gateway/test_new_clears_last_resolved_model.py tests/gateway/test_session_model_reset.py tests/gateway/test_resume_command.py tests/gateway/test_35809_auto_reset_clean_context.py tests/gateway/test_10710_auto_reset_evicts_cached_agent.py tests/gateway/test_48031_model_switch_after_auto_reset.py tests/gateway/test_35994_reset_button_deadlock.py tests/tools/test_process_registry.py tests/tools/test_terminal_tool.py tests/tools/test_terminal_compound_background.py tests/gateway/test_session_store_runtime_stale_guard.py tests/hermes_state/test_resolve_resume_session_id.py tests/cli/test_moa_command.py tests/gateway/test_usage_command.py tests/gateway/test_title_command.py tests/gateway/test_status_command.py tests/gateway/test_model_command_custom_providers.py tests/gateway/test_undo_rewind_session.py tests/gateway/test_session_info.py tests/gateway/test_session_reset_notify.py tests/gateway/test_compress_command.py tests/gateway/test_model_picker_persist.py tests/gateway/test_model_command_async_offload.py tests/test_yuanbao_pipeline.py tests/test_yuanbao_integration.py tests/gateway/platforms/test_yuanbao_recall_db_only.py tests/gateway/test_yuanbao_media_ssrf.py tests/gateway/test_telegram_group_gating.py tests/gateway/test_slack.py tests/gateway/test_multiplex_phase0.py tests/gateway/test_multiplex_lifecycle.py tests/gateway/test_multiplex_adapter_registry.py tests/cron/test_scheduler.py tests/hermes_cli/test_context_switch_guard.py -q --tb=short— 1,319 passed../scripts/run_tests.sh -qwas attempted against the refreshed branch; the 37k-test run reached 75% before the 10-minute local timeout. Ten failures had appeared outside the touched test files; all touched/focused files above passed.Checklist
Code
Documentation & Housekeeping
cli-config.yaml.exampleupdate N/ACONTRIBUTING.md/AGENTS.mdupdate N/AScreenshots / Logs
N/A — gateway/process-routing behavior, covered by regression tests.