Repository navigation
Conversation
A restart could not replay a spooled pending message when the platform that queued it derived its session key from different per-user isolation flags than the session store routes by, so the message was stranded in the spool forever. Symptom: `Cannot recover pending message for <key>: no session_id in flush file and session_key-to-id resolution failed` on every boot, with the payload left in `<hermes_home>/pending_messages/`, and the message never delivered. Root cause: `_pending_messages` slots are keyed by the adapter's own isolation flags (`PlatformConfig.extra`), while `SessionStore` routes by the gateway-wide `group_sessions_per_user` / `thread_sessions_per_user`. `PlatformConfig.from_dict` promotes a platform-block override into `extra` and `gateway/run_adapters.py` only `setdefault`s the gateway-wide value, so one platform legitimately keys a conversation differently from the store; a standalone gateway then hands that same key to the busy handler, so the queue head and the FIFO tail inherit it. `recover_pending_to_db` resolves a payload's key through the store alone — routing map first, then the durable row under the exact key — so a slot key written verbatim matched neither. Fix: `flush_pending_to_file` / `flush_overflow_to_file` take an optional `session_key_for` callable that maps a payload's `source` to the key the store routes it by, and record that key instead of the slot key. A value with no `source` (a raw str slot, an agent-history payload) and any derivation failure keep the slot key, so the fallback is exactly the previous behaviour. The adapter shutdown flush passes its session store's derivation (`adapter_store_key_for`); the runner's shutdown flush passes `_session_key_for_source`. Tests: tests/gateway/test_shutdown_flush_store_key.py drives the real entry points (`adapter.cancel_background_tasks()`, `runner.stop()`) and the real recovery pass with the two derivations disagreeing. evals/shutdown_flush_adapter_key_strand.py is the standalone repro: VERDICT=STRANDED on the unpatched tree, VERDICT=RECOVERED after the fix. Review follow-up (ehz0ah, NousResearch#136600): the files were clean under the lint config this branch was cut from, but main's `ruff.toml` enables RUF100 and F841 and the branch tripped seven lines there. Dropped the five `# noqa: E402` directives in the eval — E402 is not in the enabled rule set, so they suppressed nothing and RUF100 flagged them as unused — and used `_` for the two unused `_store_` tuple bindings in the tests. Added the fallback case the description claimed but the tests lacked: `test_a_derivation_result_that_names_no_session_keeps_the_slot_key`, parametrised over an empty string, `0`, `None` and a list, so a derivation that names no session cannot record a payload the next boot's resolution cannot look up.
`_recoverable_key` re-keyed every source-bearing pending value onto the store's derivation, which loses the payload's established owner when the value was queued by a synthetic producer whose `source` is partial. `RecallGuardMiddleware._interrupt_for_recall` (gateway/platforms/yuanbao.py) writes its notice straight into the matched in-flight slot, and a recall callback may arrive with no `from_account` (`DecodeMiddleware.parse_json_push` admits that shape): the notice's source then has `user_id=None` and derives the *userless* key, while the slot it was written into carries the participant. With `group_sessions_per_user` and `thread_sessions_per_user` both on, the store routes that slot key, so re-deriving drops `:u1` and `recover_pending_to_db` strands the notice on a key no session owns (its exact-key lookup matches neither the routing map nor a durable row). Before this PR the same notice recovered — the re-key introduced the regression. Fix: record the slot key when the store's own resolver can land it. It is the key the producing adapter queued the value under and the key its live drain re-dispatches it with (`base.py`'s drain passes the slot key), so it is the payload's established owner; the NousResearch#136189 repair survives for the case that motivated it, a slot key the store cannot route. `store_slot_key_routes` builds that predicate from `SessionStore.resolve_session_id_for_key` — the callable recovery itself uses — so a key nothing can resolve never suppresses the repair, and a resolver failure or an absent store falls back to the previous derivation. Both flush call sites stay at their current line counts (base.py and run_shutdown.py are over the file-length ratchet), via the keying-kwargs bundles the gateway's other call sites use. Verified by a new test pair in tests/gateway/test_shutdown_flush_store_key.py that drives the real producer, the real shutdown flush and the real durable-row recovery: the missing-sender notice keeps `:u1` and recovers into the original session, while the known-sender control is unchanged. Both assertions fail on the previous head (spooled `agent:main:telegram:group:group:G:main`, recovered 0) and on the merge base the same flow passes, matching the review's baseline.
…ct branches The recall guard's notice is the producer whose source is not the source its slot came from: ``_interrupt_for_recall`` writes it straight into the matched in-fligh⟪HERMES-CONTEXT-COMPRESSION: 1,425 of 1,625 chars omitted here by Hermes's context compressor. This is NOT part of the original tool call and must never be reproduced in new output — always write full, untruncated content.⟫
…e test ehz0ah's re-review at 5b77013 noted one cleanup: tests/gateway/test_recall_notice_flush_identity.py ends with an extra blank line. This removes it. Whitespace only — the file's 233 lines are otherwise byte-identical, no added line carries trailing whitespace, and the strict ruff config (ruff.strict.toml, W605 the only W rule selected) does not cover W391. Re-run at this head: HERMES_PYTHON=/tmp/hermes-venv/bin/python scripts/run_tests.sh \ tests/gateway/test_recall_notice_flush_identity.py tests/gateway/test_shutdown_flush_store_key.py === Summary: 2 files, 18 tests passed, 0 failed (100% complete) in 2.8s (8 workers) ===
Review round 5 on this branch kept the merge gate on two P2s in the key rule that round 4 introduced. Both reproduce on 02651f0; both are repaired here. The round-4 rule recorded the slot key whenever `resolve_session_id_for_key` could land it. Routability is a fact about a session, not about this payload: * the payload's own source can derive the key the store lands while the slot key resolves to a *different* live session. With the per-user flags overridden in the inverse direction (the platform shares group keys, the store isolates them) and a userless session already under the shared slot key, the queued message was appended to that userless transcript while its sender's session stayed empty — real `_pending_messages`, real adapter shutdown flush, real SQLite: `VERDICT=WRONG_OWNER`; * a store that cannot answer for a key at flush time (`SessionStore._db_for_key` fails closed, no reachable home for the key) answers `None` for both keys. The rule read that as "the slot key is wrong", re-derived from the notice's partial source and recorded a key no session owns, so the notice stranded with its spool file kept. Fix: ask the store about the key this payload's own source derives. `_recoverable_key` records the derivation when the store can land it, and keeps the producing slot key whenever the derivation is unconfirmed (`None`: an unavailable lookup decides nothing) or when the store knows a session under the slot key and none under the derivation — the partial-source producer round 4 repaired, whose derivation lands nowhere. When the store knows no session under either key the derivation stands, as before: recovery keeps the file either way and names the key it tried. `store_key_lands` answers with the resolver recovery itself uses, plus `_db_for_key` to tell "the store answered: no session under this key" apart from "the store could not look at all", so a miss is never read out of a lookup that never happened. A store without the resolver, a value with no `source` and any derivation failure keep the previous behaviour. Verified: `evals/shutdown_flush_adapter_key_strand.py` still `VERDICT=RECOVERED`, so the NousResearch#136189 repair is intact; the two new regressions in `tests/gateway/test_shutdown_flush_store_key.py` fail on the previous head with the very keys the review reported (`…group:900` instead of `…group:900:u1`; `…group:G:main` instead of its slot key) and pass here.
alt-glitch
added
type/bug
Author
|
Refile pointer: this PR is the same change as #136600, which GitHub auto-closed along with the submitting fork (nothing to do with the patch itself). The earlier review conversation with @JoaoMarcos44 and my replies are on that closed thread: #136600 — same commits, unchanged. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refile of #136600 after the source fork was deleted and restored. Same commits; GitHub cannot reopen the original thread.
Opened as a draft because this account cannot create a non-draft PR or mark one ready for review (shared tracker #131859). A maintainer needs to mark this ready for review.
What does this PR do?
A restart could not replay a pending message spooled at shutdown whenever the platform that queued it derived its session key from different per-user isolation flags than the session store routes by. Recovery logs
Cannot recover pending message for <key>: no session_id in flush file and session_key-to-id resolution failedon every boot and leaves the payload in<hermes_home>/pending_messages/; the message is never delivered._pending_messagesslots are keyed by the adapter's own isolation flags (PlatformConfig.extra), whileSessionStoreroutes by the gateway-widegroup_sessions_per_user/thread_sessions_per_user.PlatformConfig.from_dictpromotes every non-typed platform-block key intoextra(gateway/config.py:452), andgateway/run_adapters.py:1890onlysetdefaults the gateway-wide value into it, so one platform can legitimately key a conversation differently from the store. A standalone gateway hands that same key to the busy handler (gateway/run_adapters.py:1783,_standalone_scoped), so the queue head and the FIFO tail inherit it;_enqueue_fifothen writes into the adapter's_pending_messagesunder it (gateway/run_busy.py:105).recover_pending_to_dbresolves a payload's key through the store alone — routing map first, then the durable row under the exact key — so a slot key written verbatim matched neither, and the file was preserved forever.The fix records the key the store would route the payload by, instead of the slot key. It is deliberately the narrow write-side change: the wider adapter-vs-store key split (same conversation, two keys for batching/busy purposes) is left as it is.
Related Issue
Fixes #136189
Type of Change
Changes Made
gateway/shutdown_flush.py:flush_pending_to_fileandflush_overflow_to_filetake an optionalsession_key_forcallable that maps a payload'ssourceto the store's key for that source;_recoverable_keyapplies it per payload. A value without asource(a rawstrslot, an agent-history payload), an absent callable, a derivation that raises, or a non-strresult all keep the slot key — i.e. the exact previous behaviour.adapter_store_key_for(adapter)returns an adapter's session-store derivation orNone.gateway/platforms/base.py:4796: the adapter shutdown flush (cancel_background_tasks) passesadapter_store_key_for(self).gateway/run_shutdown.py:2013:stop()passesself._session_key_for_sourceto both flushes.tests/gateway/test_shutdown_flush_store_key.py: new tests — the two derivations disagreeing for one source; the adapter flush recording the store key; an adapter-keyed spool replayed into the store session on the next boot (1 row appended, file removed); the runner shutdown path; and the fallback contract (nosource, failing derivation, non-strresult — the last istest_a_derivation_result_that_names_no_session_keeps_the_slot_key, parametrised over"",0,Noneand a list; all four fail when the guard is reduced toreturn key).evals/shutdown_flush_adapter_key_strand.py: standalone repro,VERDICT=STRANDEDbefore /VERDICT=RECOVEREDafter.How to Test
python evals/shutdown_flush_adapter_key_strand.py— on the unpatched tree it printsspooled_keys=['agent:main:telegram:group:900:900:u1'],recovered=0,spool_remaining=1,VERDICT=STRANDED, plus the production warning. On this branch:spooled_keys=['agent:main:telegram:group:900:900'],recovered=1,replayed_content=['queued while busy'],spool_remaining=0,VERDICT=RECOVERED.HERMES_PYTHON=<python-with-pytest> scripts/run_tests.sh tests/gateway/test_shutdown_flush_store_key.py→=== Summary: 1 files, 10 tests passed, 0 failed (100% complete) in 12.6s (8 workers) ===(run on current main97a9ae8ee5with this commit applied; the shutdown-flush cluster, 10 files, is=== Summary: 10 files, 48 tests passed, 0 failed (100% complete) in 48.0s (8 workers) ===)HERMES_PYTHON=<python-with-pytest> scripts/run_tests.sh <every test file referencing cancel_background_tasks / _pending_messages / flush_pending_to_file / flush_overflow_to_file / recover_pending_to_db / recover_gateway_pending>(113 files) →=== Summary: 113 files, 932 tests passed, 0 failed, 3 skipped (100% complete) in 51.4s (8 workers) ===python scripts/check→scripts/check: 11 checks, ok(re-run on current main97a9ae8ee5with this commit applied, after the review follow-up on5c3350a9b9fffb43ec90ca91eca97c723eeaca12; before it,python scripts/check --only ruffon the same tree reportedFound 7 errors/scripts/check: 1 checks, FAILED: ruff— 5 ×RUF100 Unused noqa directive (non-enabled: E402)and 2 ×F841)Checklist
Code
fix(scope):,feat(scope):, etc.)state=allforsession_key_for/ flush-key siblings: Shutdown-flushed messages are recovered into their session on restart (salvage #75536, #106112, #87661) #114225 (merged: recovery resolves the payload key through the store) and fix(gateway): resolve flush session_key to session_id at shutdown-flush recovery #106112 (closed: resolvesession_keytosession_idat recovery) fix the read side and both assume the file already carries a store key; Desktop update hand-off scripts hold the update marker as a lock and report committed updates as committed #132354, fix(gateway): stop scraping /mnt/ PATH entries into WSL interop unit PATH [risk 0.30] #131806, fix(docs): remove phantom /skill and /q alias from hermes-agent SKILL.md #63451, [Bug]: Voice messages cached but not auto-transcribed — STT silent failure on Telegram #105315, Single-profile installs: Desktop cron ticker races the gateway and cron deliveries to WebSocket platforms fail silently #134367, [Bug]: state.db checkpoint/FTS maintenance without a writer fence diverges long-lived SessionDB handles — committed desktop sessions invisible to fresh readers (data loss) #135066 are unrelated areas. No PR mentions gateway: shutdown flush files are written under the adapter's session key, so restart recovery can never map them when adapter and gateway per-user isolation flags differ #136189.pytest tests/ -qand all tests pass — not ticked: that is the whole suite; the scoped runs in How to Test are what I ranpython scripts/checkpasses (the blocking lint and code-health checks CI runs)Documentation & Housekeeping
docs/, docstrings) — docstrings only; N/A otherwisecli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/Astr/dicthandling, no path or process APIs; the tests are host-agnosticScreenshots / Logs
Unpatched (the production warning, then the stranded spool):
This branch: