Repository navigation
Conversation
alt-glitch
added
type/bug
Ready for review — needs the draft flag clearedThis PR is filed as a draft because that is the only way our account can open a PR against this So the only missing step is yours: please mark this PR ready for review (Settings → "Ready for
If a draft PR never surfaces in your review queue, that is the whole reason this one looks untouched: |
ehz0ah
left a comment
ehz0ah
left a comment
There was a problem hiding this comment.
Request changes conclusion (formal review permission unavailable)
I reviewed 9f2ba1000b7052a2146e03f03689b4f4aa4206c4 against #136189 and current main.
The fix is aimed at the right boundary. gateway/shutdown_flush.py converts each queued event to the session store key before writing the recovery spool. The adapter and runner shutdown paths both pass the correct key resolver. Values without a source and derivation failures keep the old slot key. The new tests cover adapter shutdown, runner shutdown, restart recovery, and the fallback paths. The standalone probe reports STRANDED on the parent and RECOVERED with this patch.
I also applied this commit cleanly to current main at 97a9ae8ee55d31bebce49f1fb6b5ef775323976a. All 33 focused shutdown and recovery tests passed there, and the restart probe recovered the message.
One merge blocker remains. Current main lint rejects seven lines added by this PR. python scripts/check reports five unused # noqa: E402 comments in evals/shutdown_flush_adapter_key_strand.py, plus the unused _store_ bindings at lines 103 and 131 in tests/gateway/test_shutdown_flush_store_key.py. Please remove the stale suppressions and use _ for those unused tuple items.
The PR description also says the fallback tests cover a non-string key result, but this exact head has no such test. Please add that case or correct the description.
The broader affected-symbol run completed with 927 passes, 7 skips, and one unrelated update-command test failure. The same failure reproduces on the PR parent, so it is not caused by this change.
English verdict: Request changes on this exact head because it does not pass the current main lint gate. The shutdown recovery behavior itself is correct in the cases I verified.
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, #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.
9f2ba10 to
5c3350a
Compare
|
Both points reproduce on this head against current main, and both are fixed in Lint — confirmed exactly as you describe. On The five directives suppressed nothing — Description/test gap — also real. The fallback contract as written claimed a non- Unchanged behaviour, re-verified today. The repro prints One note on my own numbers, so nothing here is read as stronger than it is: my re-run of the affected-symbol sweep (116 files) was cut short at ~35% because this host sat at load ~10 — I am not claiming a fresh count for it, and your 927-pass run is the reference for that sweep. The two failures it did reach are a cross-session approval-queue timing assert and a compression-session test that hit a 120s API-stream timeout, in files that use |
JoaoMarcos44
left a comment
JoaoMarcos44
left a comment
There was a problem hiding this comment.
Independent Ledger-backed review of the current head
Reviewed head: 5c3350a9b9fffb43ec90ca91eca97c723eeaca12. Captured current main: 97a9ae8ee55d31bebce49f1fb6b5ef775323976a. PR comparison merge base: f86276d2ebcccf0530873c11e2965c3f1cbd5a31. Ledger run: run_2ddd83b7a18c4fe6b02e0b635e4bcaa7.
Verdict and priority boundary
The P1-labelled issue #136189 is real. I reproduced its adapter/store key mismatch on the merge base and on captured current main, and verified that this head repairs the tested ordinary pending-text paths. I did not substantiate a newly introduced P0/P1 defect or an officially in-scope security vulnerability in this patch.
I did reproduce three conditional, inherited data-integrity failures, documented below with the complete reviewer probe. They remain on both main and this head; they must not be presented as regressions introduced by this contributor. Their fault-mode severity needs maintainer prioritization; these experiments do not justify inventing a new P0/P1 blocker. This is a COMMENT review, not a blocking review decision or a claim that the entire repository is green.
Scope, mechanism, and ownership
I inspected all five PR-owned files: gateway/shutdown_flush.py, gateway/platforms/base.py, gateway/run_shutdown.py, tests/gateway/test_shutdown_flush_store_key.py, and evals/shutdown_flush_adapter_key_strand.py, plus retention, canonical identity, session recovery, profile routing, and startup readers.
Adapter busy/batching slots derive isolation from PlatformConfig.extra (base.py:2557-2567), while the durable store derives it from gateway configuration (session_recovery.py:96-103). The two legal configurations can add or remove a user suffix independently. Recovery searches the exact routing/durable key and the owning DB, not an arbitrary same-chat row (session_recovery.py:196-243). Writing a slot key unchanged can therefore preserve an unresolvable file indefinitely.
The patch places conversion at the shared derived-write boundary (shutdown_flush.py:90-168) and wires adapter shutdown and runner head/overflow shutdown (base.py:4795-4801, run_shutdown.py:2011-2019). It leaves adapter isolation semantics and recovery's existing profile/reset fences intact. Keyless raw strings and unavailable/throwing/empty/non-string derivations retain the old slot key. That fallback preserves the payload, not a guarantee that the key is recoverable.
The author's eval and tests use a fake DB and direct pending-slot injection. I executed them, but do not call them platform/provider E2E. My additional normal probe uses real SessionStore/SQLite, production _enqueue_fifo retention (including its acceptance flag), actual shutdown/spooling, close/reopen, the production resolver, persisted content/order, and a second recovery pass. It is a retention-to-durable-reader contract, not a complete transport authorization/ACK/LLM-delivery test.
Confirmed inherited findings
-
A failed spool write can erase the last pending text copy.
- Head locations:
shutdown_flush.py:77-87,base.py:4795-4803; runner siblingrun_shutdown.py:2011-2024also ignores the best-effort count before clearing. _flush_valuecatches the write failure and returnsFalse. Adapter shutdown still clears_pending_messageswithout requiring a successful durable copy.- Reproduction: genuinely retain one text event, inject
OSError(ENOSPC)at_write_payload, then callcancel_background_tasks. Observed: pending slot empty, zero spool JSON files, zero SQLite messages. The preservation assertion fails on both head and main. - Boundary: a modeled write failure, not an exhausted host disk or complete network ACK reproduction. The proof is loss of the retained text copy; it does not establish any additional external side effect.
- Follow-up direction: distinguish attempted backup from successful durable ownership, make failures observable, and decide a bounded retry/alternative retention/shutdown policy. Keeping only memory is not crash durability; a retry is not a solution to a permanently unavailable disk. No implementation is proved here.
- Head locations:
-
Append success followed by unlink failure duplicates the user row after a real process restart.
- Head locations:
_remove_replayed,shutdown_flush.py:311-320, and replay at403-407. _REPLAYED_UNREMOVABLEcorrectly suppresses repeat replay in the same process, but there is no durable replay receipt. The existing docstring explicitly describes the next-process duplication.- Reproduction: real SQLite append, selective
PermissionErrorfrom the spool file'sunlink, verify same-process second recovery is zero, close handles, then launch a fresh Python interpreter against the same temporary home and real reader. Observed:recovered=1, contents['fault-probe', 'fault-probe']. - Same failure on head and main. This proves duplicate persisted user rows, not repeated tools, agent turns, or destructive actions. The unlink error is injected; native Windows ACL behavior and kill-between-commit/unlink are not separately exercised.
- Follow-up direction: a stable spool identity and replay receipt committed atomically with the row, using existing durable infrastructure where possible. Content-based deduplication would incorrectly collapse legitimate repeated messages; an in-memory set or post-commit file marker cannot provide exactly-once crash recovery. No repair has been implemented or verified.
- Head locations:
-
Ordinary pending-message recovery can permanently invert FIFO after one failed head append.
- Head locations:
shutdown_flush.py:395-410,430-456,457-484. - The per-session
blocked_sessionsfence coversTRANSCRIPT_CAP_DROP_REASON, but the ordinary pending-text arm neither marks nor consults it after an append failure. The outer loop logs the failed head and proceeds to the tail. - Reproduction: retain head
fault-probeand tailfault-tailvia_enqueue_fifo, run actualrunner.stop, reopen SQLite, inject one terminalsqlite3.OperationalErroronly for the head append, then perform two recovery passes. Observed durable row order:['fault-tail', 'fault-probe']instead of arrival order. - Same failure on head and main. This is not merely a filename-order concern: row-ID order remains wrong after the failed message is successfully retried. It models one exhausted transient append failure, not native SQLite contention or an executed-command consequence.
- Follow-up direction: consider a shared failure-order fence across the relevant payload classes without blocking unrelated sessions. Permanent row rejection/quarantine, profile ownership, and normal replay require separate controls before recommending a patch.
- Head locations:
Security assessment
I inspected canonical identity at ingress, topic-source rewriting, profile-key generation, exact-key recovery, and owning-store selection. Topic recovery updates the event source before keying and its adapter path preserves pinned identity. Existing recovery remains fail-closed when a named profile's DB cannot be resolved. The patch does not add an external surface, weaken an allowlist, introduce executable spool contents, change spool paths/permissions, or add credential use.
No unauthorized external caller, OS-isolation escape, credential exfiltration, or contrary-to-documentation security outcome was demonstrated. Per SECURITY.md §§2.2, 2.6, and 3, routing handles are not authorization by themselves. A synthetic source/helper exception is not proof of an authorization bypass. The inherited fault findings above are reliability evidence, not an official advisory or vulnerability classification. POSIX permission enforcement, Windows ACLs, all multiplexed layouts, and credential E2E were not exhaustively exercised.
Alternative designs and related work
- Keep the old slot key: rejected by the actual baseline RED and stranded recovery probes.
- Unify every adapter slot with the store key: larger behavioral change to deliberately platform-specific batching/busy isolation (
base.py:2564-2566); not necessary for this durable-write mismatch. - Broaden recovery to approximate chat/user matches or strip suffixes: cannot reconstruct a missing user identifier in the inverse mismatch and weakens exact-key/profile/reset attribution. The existing resolver already supplies the appropriate owning-store infrastructure.
- Bind an immutable canonical key/session ID at retention: potentially preferable for a separately demonstrated mid-flight rebind contract, but requires auditing admission, merges, serialization, resets, and every producer. I have not proved that broader design superior for this stable-config bug; do not dismiss or claim it solved based on these tests.
- Shared write-boundary conversion: bounded and supported for this issue by both mismatch directions and both shutdown lanes. It does not make the whole spool subsystem fault-proof.
Inspected effective related diffs, not just titles: merged #114225 owns key-to-ID/DB resolution and fences; merged #133932 owns ordered/full-fidelity cap-drop recovery, profile-home replay, and the process-local unlink guard. Those are inherited capabilities, not newly implemented here. #123585 is closed/unmerged, not a merged predecessor. Open #123707 changes the off-loop cap-drop writer/lane; it is complementary, not the pending-slot-key conversion. No competing implementation/PR was published during this review.
Compatibility, operations, and residual trade-offs
- This is prevention for newly written flushes, not migration of already-stranded old files. The issue explicitly calls that out; automatic suffix stripping would risk misattribution. Preserve existing files for operator-directed recovery.
- Optional keyword callbacks preserve old caller shapes. Plain strings and failed derivation keep the old behavior, including potentially unresolved keys; the comment that the slot key "still works" should not be interpreted as an unconditional recovery guarantee.
- One additional derivation per event is introduced; synchronous filesystem writes and their shutdown latency are inherited. I did not run a capacity/latency benchmark.
- The tested contract assumes stable admitted source/configuration. Arbitrary hot reconfiguration or source rebinding was not proven safe. Normal pending text recovery is not full multimodal event replay or live turn execution.
- Rollback restores old write semantics and can strand newly queued messages again; it does not undo committed transcript rows. Backup/recovery policy and durable exactly-once replay are separate from this narrow key fix.
Executed verification
Environment: Windows 11 x64, PM-built Python 3.14.7, pytest 9.1.1, pytest-asyncio 1.3.0, SQLite 3.53.1. Canonical Git Bash runner; 2 workers; HERMES_TEST_FILE_RETRIES=0; clean TZ=UTC, LANG=C.UTF-8, PYTHONHASHSEED=0. No live platform/provider requests. Test-only overlays in independent checkouts; original production head unchanged.
| Frozen scope | Actual result | Exit | Command wall time |
|---|---|---|---|
Final real normal contract on main (-k retained_events) |
2 aligned controls passed, 8 key-mismatch assertions failed; 3 fault cases deselected | 1 | 10.607s |
| Identical final file/argv on head | 10 passed, 0 failed; 3 fault cases deselected | 0 | 12.266s |
Three adverse contracts on head (-k adverse_fault) |
0 passed, 3 intended preservation assertions failed; 10 normal cases deselected | 1 | 11.301s |
| Same adverse contracts on main | Same three behavioral failures | 1 | 29.203s |
| Final unfiltered head cluster, 11 files | 64 passed, 3 inherited-fault assertions failed, no skips | 1 | 30.899s |
Local main + exact candidate patch, 8 files, -k 'not adverse_fault' |
57 passed, 0 failed; the 3 known adverse assertions explicitly deselected | 0 | 27.258s |
Final head scripts/check --base f86276d2ebcccf0530873c11e2965c3f1cbd5a31 |
11 checks OK; code health: 0 blocking, 0 advisory | 0 | 62.628s |
The final cluster contains 54 committed cases plus my 10 positive cases and 3 intentionally RED adverse cases. Repeated runs are not additional unique coverage. It is not a clean 67-test pass. The filtered 57-test integration is positive stable-config evidence only; the excluded failures are fully reported and reproduced, not removed or weakened. My reviewer file is not part of the submitted PR.
The identical final reviewer file in all three checkouts has SHA-256 4ca1a77c0398d93041673cd27a513f62557e394ed85a7d16c06e5e1571b01ee7. The exact PR-owned merge-base patch used for local integration has SHA-256 9489c4a0435959f92b054e5348445da15b7733e0175ec2b5c05d6d93a2a1e2e2.
An earlier unqualified all-check invocation hit a 300s outer timeout and supplied no final stage receipt. I do not count it as pass/fail behavioral evidence. The final explicit-comparison-base command above completed all 11 checks. Initial shared-object Git fetch attempts failed before testing; the investigation then used an independent repository. The primary dirty user checkout was not modified or used for results.
Reproduction commands and complete reviewer probe
Use the checkout's PM-built interpreter selected via HERMES_PYTHON, and the canonical runner, not bare pytest. Save the code below as tests/gateway/test_review_store_key_contracts.py in isolated main/head checkouts. No production edits are required.
HERMES_TEST_FILE_RETRIES=0 scripts/run_tests.sh tests/gateway/test_review_store_key_contracts.py -j 2 -k retained_events -- --tb=short
HERMES_TEST_FILE_RETRIES=0 scripts/run_tests.sh tests/gateway/test_review_store_key_contracts.py -j 2 -k adverse_fault -- --tb=short
python scripts/check --base f86276d2ebcccf0530873c11e2965c3f1cbd5a31The second command is expected to fail on the preservation assertions on both snapshots. The subprocess arm is a genuine fresh interpreter, not an in-memory reset of the replay guard.
Complete executed reviewer probe (two parametrized functions, thirteen cases)
"""Reviewer-only real SQLite/retention/restart contracts; no platform or provider calls."""
import json
from unittest.mock import patch
import pytest
from gateway.config import GatewayConfig
from gateway.platforms.event import MessageEvent, MessageType
from gateway.session import SessionStore
from gateway.shutdown_flush import recover_pending_to_db
from tests.gateway.restart_test_helpers import make_restart_runner, make_restart_source
@pytest.mark.asyncio
@pytest.mark.parametrize('flags', [
(True, False, True, False, 'dm', None),
(True, False, True, True, 'group', '900'),
(True, False, False, False, 'group', None),
(False, False, True, False, 'group', None),
(True, True, True, False, 'group', '900'),
])
@pytest.mark.parametrize('lane', ['adapter', 'runner'])
async def test_retained_events_roundtrip_real_sqlite(tmp_path, monkeypatch, flags, lane):
import hermes_constants
import hermes_state
sg, st, ag, at, chat_type, thread = flags
monkeypatch.setenv('HERMES_HOME', str(tmp_path))
monkeypatch.setattr(hermes_constants, 'get_hermes_home', lambda: tmp_path)
monkeypatch.setattr(hermes_state, 'DEFAULT_DB_PATH', hermes_state._IMPORT_DEFAULT_DB_PATH)
config = GatewayConfig(group_sessions_per_user=sg, thread_sessions_per_user=st)
store = SessionStore(sessions_dir=tmp_path / 'sessions', config=config)
runner, adapter = make_restart_runner()
runner.config = config
runner.session_store = store
adapter.set_session_store(store)
adapter.config.extra.update(group_sessions_per_user=ag, thread_sessions_per_user=at)
source = make_restart_source(chat_id='900', chat_type=chat_type, thread_id=thread)
entry = store.get_or_create_session(source)
db = store._db_for_key(entry.session_key)
assert db is not None
slot = adapter._source_session_key(source)
assert entry.session_key == store._generate_session_key(source)
contents = ['head'] if lane == 'adapter' else ['head', 'tail']
try:
for i, text in enumerate(contents):
event = MessageEvent(text=text, message_type=MessageType.TEXT, source=source, message_id=f'review-{i}')
# Actual production retention seam, not raw ledger/queue row injection.
runner._enqueue_fifo(slot, event, adapter)
assert event._gateway_accepted is True
if lane == 'adapter':
await adapter.cancel_background_tasks()
else:
with patch('gateway.status.remove_pid_file'), patch('gateway.status.publish_runtime_status'):
await runner.stop()
files = list((tmp_path / 'pending_messages').glob('*.json'))
assert len(files) == len(contents)
assert {json.loads(p.read_text(encoding='utf-8'))['session_key'] for p in files} == {entry.session_key}
store.close_all_db_handles()
store = SessionStore(sessions_dir=tmp_path / 'sessions', config=config)
db = store._db_for_key(entry.session_key)
assert db is not None
assert recover_pending_to_db(db, session_resolver=store.resolve_session_id_for_key) == len(contents)
assert recover_pending_to_db(db, session_resolver=store.resolve_session_id_for_key) == 0
assert [m['content'] for m in db.get_messages(entry.session_id)] == contents
finally:
store.close_all_db_handles()
# Re-open through the real reader and verify persisted order/content and index.
reopened = SessionStore(sessions_dir=tmp_path / 'sessions', config=config)
try:
assert reopened.peek_session_id(entry.session_key) == entry.session_id
restored_db = reopened._db_for_key(entry.session_key)
assert [m['content'] for m in restored_db.get_messages(entry.session_id)] == contents
assert not list((tmp_path / 'pending_messages').glob('*.json'))
finally:
reopened.close_all_db_handles()
@pytest.mark.asyncio
@pytest.mark.parametrize('fault', ['spool_write', 'unlink', 'append_once'])
async def test_review_adverse_fault_contract(tmp_path, monkeypatch, fault):
"""Adversarial preservation assertions: intentionally RED if an inherited limitation remains."""
import errno
import os
import subprocess
import sys
from pathlib import Path
import hermes_constants
import hermes_state
import gateway.shutdown_flush as flush
monkeypatch.setenv('HERMES_HOME', str(tmp_path))
monkeypatch.setattr(hermes_constants, 'get_hermes_home', lambda: tmp_path)
monkeypatch.setattr(hermes_state, 'DEFAULT_DB_PATH', hermes_state._IMPORT_DEFAULT_DB_PATH)
store = SessionStore(sessions_dir=tmp_path / 'sessions', config=GatewayConfig())
runner, adapter = make_restart_runner()
runner.session_store = store
adapter.set_session_store(store)
source = make_restart_source(chat_id='901', chat_type='dm')
entry = store.get_or_create_session(source)
event = MessageEvent(text='fault-probe', message_type=MessageType.TEXT, source=source, message_id='fault-1')
runner._enqueue_fifo(adapter._source_session_key(source), event, adapter)
assert event._gateway_accepted is True
try:
if fault == 'spool_write':
def fail_write(*args, **kwargs):
raise OSError(errno.ENOSPC, 'reviewer-injected spool write failure')
monkeypatch.setattr(flush, '_write_payload', fail_write)
await adapter.cancel_background_tasks()
files = list((tmp_path / 'pending_messages').glob('*.json'))
rows = store._db_for_key(entry.session_key).get_messages(entry.session_id)
# A retained payload or durable copy must still exist; no raw-row injection.
assert bool(adapter._pending_messages) or bool(files) or bool(rows), 'accepted event lost after failed spool write'
elif fault == 'append_once':
import sqlite3
tail_event = MessageEvent(text='fault-tail', message_type=MessageType.TEXT, source=source, message_id='fault-2')
runner._enqueue_fifo(adapter._source_session_key(source), tail_event, adapter)
assert tail_event._gateway_accepted is True
with patch('gateway.status.remove_pid_file'), patch('gateway.status.publish_runtime_status'):
await runner.stop()
store.close_all_db_handles()
store = SessionStore(sessions_dir=tmp_path / 'sessions', config=GatewayConfig())
db = store._db_for_key(entry.session_key)
original_append = db.append_message
failed = []
def append_with_one_failed_head(**kwargs):
if kwargs.get('content') == 'fault-probe' and not failed:
failed.append(True)
raise sqlite3.OperationalError('reviewer-injected exhausted transient append failure')
return original_append(**kwargs)
with monkeypatch.context() as scoped:
scoped.setattr(db, 'append_message', append_with_one_failed_head)
assert recover_pending_to_db(db, session_resolver=store.resolve_session_id_for_key) == 1
assert failed == [True]
assert recover_pending_to_db(db, session_resolver=store.resolve_session_id_for_key) == 1
content = [row['content'] for row in db.get_messages(entry.session_id)]
assert content == ['fault-probe', 'fault-tail'], f'pending FIFO replay inversion: {content}'
else:
await adapter.cancel_background_tasks()
files = list((tmp_path / 'pending_messages').glob('*.json'))
assert len(files) == 1
original_unlink = Path.unlink
def fail_unlink(path, *args, **kwargs):
if path == files[0]:
raise PermissionError('reviewer-injected unlink failure')
return original_unlink(path, *args, **kwargs)
with monkeypatch.context() as scoped:
scoped.setattr(Path, 'unlink', fail_unlink)
db = store._db_for_key(entry.session_key)
assert recover_pending_to_db(db, session_resolver=store.resolve_session_id_for_key) == 1
assert len(db.get_messages(entry.session_id)) == 1
assert files[0].exists()
# Same-process guard really works; do not simulate restart by clearing it.
assert recover_pending_to_db(db, session_resolver=store.resolve_session_id_for_key) == 0
store.close_all_db_handles()
child_code = (
'import json,sys; from pathlib import Path; '
'from gateway.config import GatewayConfig; from gateway.session import SessionStore; '
'from gateway.shutdown_flush import recover_pending_to_db; '
's=SessionStore(sessions_dir=Path(sys.argv[1])/"sessions",config=GatewayConfig()); '
'd=s._db_for_key(sys.argv[2]); '
'n=recover_pending_to_db(d,session_resolver=s.resolve_session_id_for_key); '
'rows=d.get_messages(sys.argv[3]); '
'print(json.dumps({"recovered":n,"content":[r["content"] for r in rows]})); '
's.close_all_db_handles()'
)
env = dict(os.environ, HERMES_HOME=str(tmp_path), HERMES_NIX_BUILD='1')
child = subprocess.run([sys.executable, '-c', child_code, str(tmp_path), entry.session_key, entry.session_id],
cwd=Path(__file__).resolve().parents[2], env=env, capture_output=True,
text=True, encoding='utf-8', timeout=45, check=True)
receipt = json.loads(child.stdout.strip().splitlines()[-1])
assert receipt['content'] == ['fault-probe'], f'fresh-process duplicate: {receipt}'
finally:
store.close_all_db_handles()Current remote state and merge conditions
The inspected remote PR is OPEN, draft, MERGEABLE but BLOCKED, with no checks reported, and the comparison is 1 candidate commit ahead / 209 base commits behind. These are snapshot statements, not proof that CI was never executed or that the branch is approved.
The exact five-file patch applied cleanly to captured main and passed the filtered local integration above. That is an uncommitted patch-level integration result, not a rebased/pushed candidate, a validation of remote CI, or permission to merge. Before merging, maintainers still need to clear the draft when appropriate and obtain the required checks/review on the resulting current head. Revalidate if head/base changes.
The earlier review at 9f2ba1000b7052a2146e03f03689b4f4aa4206c4 identified lint and a non-string-result test gap. Both are corrected at this head: the four invalid-result cases are present at test_shutdown_flush_store_key.py:165-175 and pass; the final full local checker passes. I am not carrying those old blockers forward or claiming the other reviewer's broader test totals as my own.
Bottom line: the issue's P1 key-mismatch defect is substantiated and repaired in the tested scope; no new P0/P1 regression was substantiated. The three reproduced inherited fault modes remain explicit follow-up work, and the patch must not be advertised as exactly-once, complete multimodal, crash-proof, or platform/provider E2E recovery.
ehz0ah
left a comment
ehz0ah
left a comment
There was a problem hiding this comment.
Approval conclusion (formal approval permission unavailable)
GitHub did not accept a formal approval from this account, so this review records the approval conclusion on the exact head.
Reviewed head 5c3350a9b9fffb43ec90ca91eca97c723eeaca12 against issue #136189 and current main@97a9ae8ee55d31bebce49f1fb6b5ef775323976a.
Motivation
The reported bug reproduces on current main. A platform can add a per-user suffix to its pending-message slot key while SessionStore uses the gateway-wide isolation flags. On shutdown, the old code wrote the slot key. On the next boot, recovery could not resolve that key, so it kept the spool file and never restored the message to the transcript. My unpatched control returned VERDICT=STRANDED.
Approach
The fix converts each sourced payload to the key owned by the session store at the shared spool-write boundary. Adapter shutdown passes the store resolver through adapter_store_key_for. Runner shutdown passes _session_key_for_source for both the pending head and FIFO overflow. A value without a source, a missing resolver, an exception, an empty result, or a non-string result keeps the old slot key. Recovery itself is unchanged.
Specific changes
gateway/shutdown_flush.py adds the narrow key conversion and optional resolver arguments. gateway/platforms/base.py wires adapter shutdown to the store resolver. gateway/run_shutdown.py wires runner shutdown for both spool lanes. tests/gateway/test_shutdown_flush_store_key.py covers the mismatched keys, both shutdown paths, restart recovery, and all fallback results. evals/shutdown_flush_adapter_key_strand.py provides a direct before-and-after probe.
Risk to main
The commit applies cleanly to current main. On that composition, 29 focused shutdown and spool tests passed. The recovery probe returned VERDICT=RECOVERED, and scripts/check completed all 11 checks. The change adds no config, schema, public API, prompt surface, or new state owner. The focused validation proves transcript recovery for the tested pending-text paths. It does not claim full platform delivery after replay. The remote PR still has no hosted checks and remains a draft.
Overall assessment
The earlier lint blocker and the missing invalid-result regression case are fixed at this head. The root cause is real, the write boundary owns the correction, and the fallback keeps prior behavior when the store key cannot be derived. I found no remaining blocker in this patch.
English verdict: APPROVE 5c3350a9b9fffb43ec90ca91eca97c723eeaca12. The patch fixes the stranded-spool case on current main, preserves the fallback contract, and passes the focused validation above.
Approval recorded on the unchanged head — no code change in responseThank you. This review names Re-read at that head to check the review's own account of the patch (read-only; I am not restating the reviewer's test totals as mine):
One correction on the next step: the draft flag is not ours to clear. Re-probed on this head — GraphQL Scope stays where the review puts it: this is the pending-text spool key and nothing wider — not exactly-once restore, not full multimodal replay, not crash-proof recovery, not platform/provider E2E. The independent review filed on this same head reports its three conditional fault modes as inherited from The head stays where it is. We will push a commit only if a review asks for a specific change. |
The three inherited findings: mechanics confirmed on this head, and deliberately not fixed hereThank you — this is the most useful kind of review: it separates what our diff does from what the No code changed in response to this review. The head is still What I ranThree standalone probes, one per finding, each driving the real entry points (
Environment: Linux, Python 3.13.5, pytest 9.1.1 / pytest-asyncio 1.4.0 (copy of the shipped venv), the
So: the mechanics are exactly as you describe, and the zero behavioural delta from this diff is what the Boundaries I am not overstating, in the same spirit as your own: my probes model the failures (an injected Two things worth recording next to the findings
Neither point makes the behaviour correct. They change what a fix has to be: documented-by-design but Triage decision: not folded into this PRI am not patching any of the three here, and I would rather say why than let silence imply denial:
What would actually help, if the maintainers agree the findings are worth acting on: do you want these Is there a reading of any of the three I have got wrong — or a reason one of them is this PR's |
JoaoMarcos44
left a comment
JoaoMarcos44
left a comment
There was a problem hiding this comment.
Expanded P1/P2/P3 review: one new P2 must be fixed before merge
This is a material update to my earlier review on the same head, 5c3350a9b9fffb43ec90ca91eca97c723eeaca12. The ordinary issue #136189 is still fixed in the tested paths, but the expanded sibling-producer investigation found a new, conditional P2 recovery regression introduced by this patch. My recommendation is do not merge this head until the recall regression is corrected. No new P0/P1 has been demonstrated. The earlier three injected-fault failures remain inherited, not additional PR-owned blockers.
Expanded Ledger run: run_1509a097bc834f6980e36f61e632f63d. The earlier completed run and its report remain historical evidence. This update corrects the scope of the earlier conclusion: normal ingress canonicalization does not cover every direct pending-slot producer.
Blocking P2: partial-source Yuanbao recall overwrites an already valid queued owner
Changed boundary: gateway/shutdown_flush.py:106-112, applied at line 138 and called from base.py:4796-4797.
Preconditions are supported but non-default: a Yuanbao group/thread with both group_sessions_per_user: true and thread_sessions_per_user: true, a recall of an in-flight message without its original sender, and shutdown before the pending recall notice drains. The generic per-user thread flag is explicitly supported in gateway/config.py:602-603, website/docs/user-guide/sessions.md:937-938, and website/docs/developer-guide/gateway-session-lifecycle.md:637-638; no Yuanbao exclusion was found.
- The original participant's adapter key and store key already agree:
agent:main:yuanbao:group:group:G:main:u1. The durable session exists under that exact key. This is an aligned-flags case, not the mismatch the PR repairs. DecodeMiddleware.parse_json_pushexplicitly accepts recall callbacks withoutfrom_accountor message body (yuanbao.py:415-438).RecallGuardMiddlewarematches the active processing slot by recalled message ID (546-574). That ownership was established by normal dispatch (1677-1705).- The recall producer directly puts its notice into
adapter._pending_messages[matched_slot], bypassinghandle_message(577-590). Its replacement source is built from the callback and hasuser_id=None(524-530). The callback need not reconstruct the original participant because the producer already matched the slot. - The new
_recoverable_keytrusts that partial source and recordsagent:main:yuanbao:group:group:G:main, dropping:u1. This is a valid nonempty string, so the exception/type fallback does not help. - After reopening real SQLite, the real exact-key resolver returns no session for the replacement key:
recovered=0, no recall notice in the original transcript, and the JSON remains stranded. Baseline preserves the matched slot key and recovers the exact notice into the original session.
The bytes are not irreversibly deleted: they remain in an unresolvable spool file. The proven impact is regression of automatic recovery of a queued control notice. I have not demonstrated continued execution of a withdrawn request, user-content corruption, unauthorized access, or a security advisory. P2 here is a functional regression under a documented configuration, not an inflated P1 claim.
Parent-executed differential proof
The reviewer fixture is byte-identical in the head, merge-base, main, and local integration trees. Final SHA-256: adff0c2321cf2cb9d573e30a647ed47e3533dab192b3065533c470dbe847dcc2. It has two parametrized functions/four cases. The supplied-sender u1 controls pass on every tree; only the supported missing-sender cases fail on this head. The failures are behavioral assertions, not import/setup errors.
Canonical actual argv, with the checkout's PM interpreter selected through HERMES_PYTHON:
C:/Program Files/Git/usr/bin/bash.exe scripts/run_tests.sh tests/gateway/test_review_recall_spool_identity.py
| Tree | Result | Exit | Measured command wall time |
|---|---|---|---|
Merge base f86276d2ebcccf0530873c11e2965c3f1cbd5a31 |
4 passed, 0 failed | 0 | 5.565 s |
Captured main 97a9ae8ee55d31bebce49f1fb6b5ef775323976a |
4 passed, 0 failed | 0 | 5.691 s |
Reviewed head 5c3350a9b9fffb43ec90ca91eca97c723eeaca12 |
2 passed, 2 failed | 1 | 6.417 s |
| Captured main + exact PR-owned patch, uncommitted local integration | 2 passed, 2 failed | 1 | 21.311 s |
The fourth run uses the same final fixture bytes on the already prepared integration tree: current-main HEAD 97a9ae8ee55d31bebce49f1fb6b5ef775323976a plus the exact five-file candidate-owned patch. It reproduces the same missing-sender recovery/key assertions; known-sender controls still pass. This is patch-level local integration evidence, not a rebased/pushed candidate or remote CI result.
At fixture line 145, recovered == 1 fails with actual 0; at line 168, the spooled key lacks :u1. The known-sender controls preserve successful recovery and the exact original notice text. yuanbao.py and session_recovery.py are unchanged in the merge-base-to-head PR diff; the new key replacement is causal, not base drift.
Environment: Windows 11 x64, PM Python 3.14.7, pytest 9.1.1, pytest-asyncio 1.3.0, SQLite 3.53.1; canonical native Git Bash, 12 workers, retries disabled, isolated test home, UTC/clean runner environment. Parent receipts are recall-final-{head,mergebase,base}.{json,log} in the isolated evidence directory. The final runs followed documentation-only corrections to the delegated fixture; older hashes/receipts are not credited to these final bytes.
Proof scope is local decode -> real recall producer -> actual adapter shutdown -> real reopened SQLite/recovery. The fixture seeds the processing-ID/active-guard maps normally produced by dispatch and uses blank processing text to avoid unrelated redaction polling. It is not full dispatch/transport ACK/provider/LLM E2E. Parent source tracing checked the genuine dispatch map writer and cancellation branch: normal follow-up drain occurs on successful completion (base.py:4650-4658); task cancellation takes the distinct branch at 4660-4665, leaving the pending notice for shutdown backup.
Required merge condition and alternatives
Preserve the already-established owner for supported partial-source pending producers while retaining the ordinary adapter/store mismatch repair. Carrying the original accepted source/routing identity through the synthetic replacement is a credible direction; simply restoring every old slot key would reintroduce #136189. A blanket "missing user means invalid source" rule is also unsafe: legitimate shared/internal events can lack a user ID. Check producer ownership rather than assuming every source-bearing value came through ordinary canonicalized ingress.
Require all four recall cases to pass on the revised head, while preserving the ordinary two-direction key-mismatch, adapter-head/runner-overflow, profile, fallback, and reset controls. No production fix is implemented or validated in this review. The baseline-GREEN/head-RED comparison proves a regression; it is not a claim that a proposed repair is GREEN.
The inspected #99950 policy-unification alternative changes key derivation semantics but does not reconstruct a missing participant in this aligned-flags case. Source-bearing recovery would still receive the same partial source unless original ownership is retained, and requires schema expansion. Recording both competing keys and replaying twice would risk duplication. For the newly searched siblings, inspection of #75849 at 46c77dad0cbdc3caa41afdd6d442e3473dc66d03 shows a control/internal-event distinction and retains _build_source(..., from_account); it does not restore the missing participant or change this spool normalization. #83418 at c629a7ccd9041875334c3a60fe47eef9c4afd6ca anchors reconnect tasks and drains background work on disconnect, without changing recall identity or spool keys. These are complementary lifecycle/control work, not a demonstrated repair for this finding. This is effective-diff inspection, not execution or CI validation of those branches; other broad keyword hits were not exhaustively audited.
Other expanded results, not additional hard blockers
- Merged-slot replay ordering limitation: parent ran four real SQLite characterization/control cases. Arrival
A1,A2,B1,B2from two adapter per-user slots collapsing to one store session is recovered asA1,B1,A2,B2with sequential filenames. Heads have noseq; each slot tail restarts at zero. TheB1-before-A2inversion is filename-independent. Survival once and per-sender order are preserved. The passing test deliberately characterizes this order; it is not a failing global-order contract test. Cross-slot chronology is not established as a product contract, so I do not elevate this observation into a forced P3 blocker. The old mismatch strands these files, not irreversibly deletes them. Probe SHA-256:743ce0da8ad5035c4815ec57c11a579dd9448af10b76bc16ed1f5f4d2e0e3c3b; parent result 4 passed/exit 0 in 7.760 s. - Submitted runner-head test fidelity: the author's test places a
MessageEventinto the runner's legacypending_command_textview (test_shutdown_flush_store_key.py:129-142,session_state.py:71,239). The in-tree FIFO retains its actual head in the adapter and tails inconversation.queued_events(run_busy.py:105-114). No in-tree non-None writer for the legacy field was found. This limits that particular test's production claim; it is not a demonstrated runtime regression. My ordinary reviewer probe covers the actual head/overflow retention path. - Expanded five-file shutdown run: 27 passed, 1 failed, exit 1, command wall time 10.837 s. The failure is the unmarked POSIX-style zombie oracle at
test_gateway_shutdown.py:360. The identical focused assertion fails on both merge base and captured main.status.py:931-962intentionally skips POSIX zombie probing on Windows. This is an inherited off-host test-oracle mismatch, not a PR-owned blocker; no assertion was weakened or skipped to report green. - Sync/async binding hypothesis refuted: adapters receive the synchronous
SessionStore(run.py:3518,run_adapters.py:1357), whileAsyncSessionStoreis a separate facade (run.py:3523). The hypothesized coroutine return was not found on the production binding. That refutation does not cover direct partial-source synthetic producers, which is how the P2 escaped the initial audit.
The review remains a COMMENT submission. The authenticated account has read-only repository permissions, so this technical "fix before merge" recommendation is not a claim that I set a required blocking review decision. The PR remains draft/BLOCKED with no checks reported; do not attribute GitHub's BLOCKED state to my finding. Current-main revalidation and CI remain separate merge conditions.
The complete executed recall probe follows. The original report and its three inherited findings are preserved below for traceability.
Complete executed recall probe (two functions, four cases)
"""Reviewer-only probe: a partial-source Yuanbao recall notice must survive the shutdown flush
into the ORIGINAL durable session.
PR #136600 makes ``flush_pending_to_file`` re-derive each slot's key from the queued value's
``source`` (``_recoverable_key`` + ``adapter_store_key_for``, called from
``BasePlatformAdapter.cancel_background_tasks``). The recall guard writes its synthetic interrupt
notice *straight into* ``adapter._pending_messages[matched_slot]`` (bypassing ``handle_message``)
with a source built from the *recall callback*. The supported local recall callback shape may
omit ``from_account`` and ``msg_body`` — ``DecodeMiddleware.parse_json_push`` explicitly
admits that input — so ``RecallGuardMiddleware._build_source`` yields ``user_id=None``.
When the adapter's own isolation flags agree with the gateway-wide ones (so the slot key IS the
store key), re-deriving the key from that partial source silently drops the participant segment:
the notice is spooled under a key that neither the routing map nor the durable row knows, recovery
cannot resolve it, and the notice strands in the spool instead of replaying into the original
session. The pre-PR flush recorded the slot key verbatim, which resolved.
This probe drives the real decode -> real recall producer -> real adapter shutdown flush -> real
SessionStore/SQLite reopen/recovery, and asserts the exact notice text lands in the ORIGINAL
session with the spool drained. ``from_account=""`` is the supported partial-source failing case;
``from_account="u1"`` is a positive control proving the fixture and the recovery seam are sound.
"""
import asyncio
import json
from pathlib import Path
import pytest
from gateway.config import GatewayConfig, PlatformConfig
from gateway.platforms.yuanbao import (
DecodeMiddleware,
InboundContext,
RecallGuardMiddleware,
YuanbaoAdapter,
)
from gateway.session import SessionStore
from gateway.shutdown_flush import recover_pending_to_db
RECALLED_MSG_ID = "m-original"
GROUP_CODE = "G"
ORIGINAL_SENDER = "u1"
# Both the adapter (PlatformConfig.extra) and the store use the gateway-wide isolation flags, so
# the slot key the notice is queued under IS the key the store routes the original turn by.
FLAGS = dict(group_sessions_per_user=True, thread_sessions_per_user=True)
def _config() -> GatewayConfig:
return GatewayConfig(**FLAGS)
def _open_store(tmp_path: Path) -> SessionStore:
return SessionStore(sessions_dir=tmp_path / "sessions", config=_config())
@pytest.fixture
def hermes_home(tmp_path, monkeypatch):
"""Real SQLite under an isolated HERMES_HOME (same seam as the other reviewer probes)."""
import hermes_constants
import hermes_state
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
monkeypatch.setattr(hermes_constants, "get_hermes_home", lambda: tmp_path)
monkeypatch.setattr(hermes_state, "DEFAULT_DB_PATH", hermes_state._IMPORT_DEFAULT_DB_PATH)
return tmp_path
def _wire(tmp_path: Path):
store = _open_store(tmp_path)
adapter = YuanbaoAdapter(PlatformConfig(enabled=True, token="***"))
adapter.config.extra.update(**FLAGS)
adapter.set_session_store(store)
return store, adapter
def _original_source(adapter: YuanbaoAdapter):
"""The real group source of the message that is being processed when the recall arrives."""
return adapter.build_source(
chat_id=f"group:{GROUP_CODE}", chat_type="group",
user_id=ORIGINAL_SENDER, thread_id="main",
)
async def _noop() -> None:
return None
async def _emit_recall(adapter: YuanbaoAdapter, from_account: str) -> str:
"""Drive the real decode + real recall producer; return the notice text queued for the slot."""
raw = {
"callback_command": "Group.CallbackAfterRecallMsg",
"group_code": GROUP_CODE,
"recall_msg_seq_list": [{"msg_id": RECALLED_MSG_ID}],
}
if from_account: # otherwise exercise the supported callback without this key
raw["from_account"] = from_account
push = DecodeMiddleware.parse_json_push(raw)
assert push is not None, "the decoder must admit this recall callback"
assert push["callback_command"] == "Group.CallbackAfterRecallMsg"
assert push["from_account"] == from_account
slot = adapter._source_session_key(_original_source(adapter))
adapter._processing_msg_ids[slot] = RECALLED_MSG_ID
adapter._processing_msg_texts[slot] = "" # blank: no content-redact poll
adapter._active_sessions[slot] = asyncio.Event()
await RecallGuardMiddleware().handle(InboundContext(adapter=adapter, push=push), _noop)
notice = adapter._pending_messages.get(slot)
assert notice is not None, "the recall interrupt must queue its notice on the matched slot"
return notice.text
def _spool_keys(flush_dir: Path) -> list[str]:
return sorted(json.loads(p.read_text(encoding="utf-8"))["session_key"]
for p in flush_dir.glob("pending-*.json"))
@pytest.mark.asyncio
@pytest.mark.parametrize("from_account", ["", ORIGINAL_SENDER])
async def test_recall_notice_replays_into_the_original_session(tmp_path, hermes_home, from_account):
"""The queued notice must survive a real adapter shutdown flush and land in the original row."""
store, adapter = _wire(tmp_path)
try:
entry = store.get_or_create_session(_original_source(adapter))
slot = adapter._source_session_key(_original_source(adapter))
# Precondition: the slot key and the store key are the same string (flags agree).
assert slot == entry.session_key == store._generate_session_key(_original_source(adapter))
notice_text = await _emit_recall(adapter, from_account)
# Real production flush seam: the exact changed line in base.py.
await adapter.cancel_background_tasks()
store.close_all_db_handles()
reopened = _open_store(tmp_path)
try:
db = reopened._db_for_key(entry.session_key)
recovered = recover_pending_to_db(
db, session_resolver=reopened.resolve_session_id_for_key)
contents = [m["content"] for m in db.get_messages(entry.session_id)]
finally:
reopened.close_all_db_handles()
assert recovered == 1, (
"the recall notice was not recovered; spool keys="
f"{_spool_keys(tmp_path / 'pending_messages')}"
)
assert contents == [notice_text], f"notice text not in the original session: {contents!r}"
assert not list((tmp_path / "pending_messages").glob("*.json")), "spool not drained"
finally:
store.close_all_db_handles()
@pytest.mark.asyncio
@pytest.mark.parametrize("from_account", ["", ORIGINAL_SENDER])
async def test_flushed_key_is_the_slot_the_notice_was_queued_under(tmp_path, hermes_home,
from_account):
"""The spooled key must be the slot the notice was queued under, so recovery can resolve it."""
store, adapter = _wire(tmp_path)
try:
store.get_or_create_session(_original_source(adapter))
slot = adapter._source_session_key(_original_source(adapter))
await _emit_recall(adapter, from_account)
await adapter.cancel_background_tasks()
assert _spool_keys(tmp_path / "pending_messages") == [slot]
finally:
store.close_all_db_handles()Earlier report, preserved as historical scope
The expanded P2 verdict above supersedes any impression of unconditional endorsement from this earlier report. Its executed tests and inherited findings remain valid historical receipts.
Independent Ledger-backed review of the current head
Reviewed head: 5c3350a9b9fffb43ec90ca91eca97c723eeaca12. Captured current main: 97a9ae8ee55d31bebce49f1fb6b5ef775323976a. PR comparison merge base: f86276d2ebcccf0530873c11e2965c3f1cbd5a31. Ledger run: run_2ddd83b7a18c4fe6b02e0b635e4bcaa7.
Verdict and priority boundary
The P1-labelled issue #136189 is real. I reproduced its adapter/store key mismatch on the merge base and on captured current main, and verified that this head repairs the tested ordinary pending-text paths. I did not substantiate a newly introduced P0/P1 defect or an officially in-scope security vulnerability in this patch.
I did reproduce three conditional, inherited data-integrity failures, documented below with the complete reviewer probe. They remain on both main and this head; they must not be presented as regressions introduced by this contributor. Their fault-mode severity needs maintainer prioritization; these experiments do not justify inventing a new P0/P1 blocker. This is a COMMENT review, not a blocking review decision or a claim that the entire repository is green.
Scope, mechanism, and ownership
I inspected all five PR-owned files: gateway/shutdown_flush.py, gateway/platforms/base.py, gateway/run_shutdown.py, tests/gateway/test_shutdown_flush_store_key.py, and evals/shutdown_flush_adapter_key_strand.py, plus retention, canonical identity, session recovery, profile routing, and startup readers.
Adapter busy/batching slots derive isolation from PlatformConfig.extra (base.py:2557-2567), while the durable store derives it from gateway configuration (session_recovery.py:96-103). The two legal configurations can add or remove a user suffix independently. Recovery searches the exact routing/durable key and the owning DB, not an arbitrary same-chat row (session_recovery.py:196-243). Writing a slot key unchanged can therefore preserve an unresolvable file indefinitely.
The patch places conversion at the shared derived-write boundary (shutdown_flush.py:90-168) and wires adapter shutdown and runner head/overflow shutdown (base.py:4795-4801, run_shutdown.py:2011-2019). It leaves adapter isolation semantics and recovery's existing profile/reset fences intact. Keyless raw strings and unavailable/throwing/empty/non-string derivations retain the old slot key. That fallback preserves the payload, not a guarantee that the key is recoverable.
The author's eval and tests use a fake DB and direct pending-slot injection. I executed them, but do not call them platform/provider E2E. My additional normal probe uses real SessionStore/SQLite, production _enqueue_fifo retention (including its acceptance flag), actual shutdown/spooling, close/reopen, the production resolver, persisted content/order, and a second recovery pass. It is a retention-to-durable-reader contract, not a complete transport authorization/ACK/LLM-delivery test.
Confirmed inherited findings
-
A failed spool write can erase the last pending text copy.
- Head locations:
shutdown_flush.py:77-87,base.py:4795-4803; runner siblingrun_shutdown.py:2011-2024also ignores the best-effort count before clearing. _flush_valuecatches the write failure and returnsFalse. Adapter shutdown still clears_pending_messageswithout requiring a successful durable copy.- Reproduction: genuinely retain one text event, inject
OSError(ENOSPC)at_write_payload, then callcancel_background_tasks. Observed: pending slot empty, zero spool JSON files, zero SQLite messages. The preservation assertion fails on both head and main. - Boundary: a modeled write failure, not an exhausted host disk or complete network ACK reproduction. The proof is loss of the retained text copy; it does not establish any additional external side effect.
- Follow-up direction: distinguish attempted backup from successful durable ownership, make failures observable, and decide a bounded retry/alternative retention/shutdown policy. Keeping only memory is not crash durability; a retry is not a solution to a permanently unavailable disk. No implementation is proved here.
- Head locations:
-
Append success followed by unlink failure duplicates the user row after a real process restart.
- Head locations:
_remove_replayed,shutdown_flush.py:311-320, and replay at403-407. _REPLAYED_UNREMOVABLEcorrectly suppresses repeat replay in the same process, but there is no durable replay receipt. The existing docstring explicitly describes the next-process duplication.- Reproduction: real SQLite append, selective
PermissionErrorfrom the spool file'sunlink, verify same-process second recovery is zero, close handles, then launch a fresh Python interpreter against the same temporary home and real reader. Observed:recovered=1, contents['fault-probe', 'fault-probe']. - Same failure on head and main. This proves duplicate persisted user rows, not repeated tools, agent turns, or destructive actions. The unlink error is injected; native Windows ACL behavior and kill-between-commit/unlink are not separately exercised.
- Follow-up direction: a stable spool identity and replay receipt committed atomically with the row, using existing durable infrastructure where possible. Content-based deduplication would incorrectly collapse legitimate repeated messages; an in-memory set or post-commit file marker cannot provide exactly-once crash recovery. No repair has been implemented or verified.
- Head locations:
-
Ordinary pending-message recovery can permanently invert FIFO after one failed head append.
- Head locations:
shutdown_flush.py:395-410,430-456,457-484. - The per-session
blocked_sessionsfence coversTRANSCRIPT_CAP_DROP_REASON, but the ordinary pending-text arm neither marks nor consults it after an append failure. The outer loop logs the failed head and proceeds to the tail. - Reproduction: retain head
fault-probeand tailfault-tailvia_enqueue_fifo, run actualrunner.stop, reopen SQLite, inject one terminalsqlite3.OperationalErroronly for the head append, then perform two recovery passes. Observed durable row order:['fault-tail', 'fault-probe']instead of arrival order. - Same failure on head and main. This is not merely a filename-order concern: row-ID order remains wrong after the failed message is successfully retried. It models one exhausted transient append failure, not native SQLite contention or an executed-command consequence.
- Follow-up direction: consider a shared failure-order fence across the relevant payload classes without blocking unrelated sessions. Permanent row rejection/quarantine, profile ownership, and normal replay require separate controls before recommending a patch.
- Head locations:
Security assessment
I inspected canonical identity at ingress, topic-source rewriting, profile-key generation, exact-key recovery, and owning-store selection. Topic recovery updates the event source before keying and its adapter path preserves pinned identity. Existing recovery remains fail-closed when a named profile's DB cannot be resolved. The patch does not add an external surface, weaken an allowlist, introduce executable spool contents, change spool paths/permissions, or add credential use.
No unauthorized external caller, OS-isolation escape, credential exfiltration, or contrary-to-documentation security outcome was demonstrated. Per SECURITY.md §§2.2, 2.6, and 3, routing handles are not authorization by themselves. A synthetic source/helper exception is not proof of an authorization bypass. The inherited fault findings above are reliability evidence, not an official advisory or vulnerability classification. POSIX permission enforcement, Windows ACLs, all multiplexed layouts, and credential E2E were not exhaustively exercised.
Alternative designs and related work
- Keep the old slot key: rejected by the actual baseline RED and stranded recovery probes.
- Unify every adapter slot with the store key: larger behavioral change to deliberately platform-specific batching/busy isolation (
base.py:2564-2566); not necessary for this durable-write mismatch. - Broaden recovery to approximate chat/user matches or strip suffixes: cannot reconstruct a missing user identifier in the inverse mismatch and weakens exact-key/profile/reset attribution. The existing resolver already supplies the appropriate owning-store infrastructure.
- Bind an immutable canonical key/session ID at retention: potentially preferable for a separately demonstrated mid-flight rebind contract, but requires auditing admission, merges, serialization, resets, and every producer. I have not proved that broader design superior for this stable-config bug; do not dismiss or claim it solved based on these tests.
- Shared write-boundary conversion: bounded and supported for this issue by both mismatch directions and both shutdown lanes. It does not make the whole spool subsystem fault-proof.
Inspected effective related diffs, not just titles: merged #114225 owns key-to-ID/DB resolution and fences; merged #133932 owns ordered/full-fidelity cap-drop recovery, profile-home replay, and the process-local unlink guard. Those are inherited capabilities, not newly implemented here. #123585 is closed/unmerged, not a merged predecessor. Open #123707 changes the off-loop cap-drop writer/lane; it is complementary, not the pending-slot-key conversion. No competing implementation/PR was published during this review.
Compatibility, operations, and residual trade-offs
- This is prevention for newly written flushes, not migration of already-stranded old files. The issue explicitly calls that out; automatic suffix stripping would risk misattribution. Preserve existing files for operator-directed recovery.
- Optional keyword callbacks preserve old caller shapes. Plain strings and failed derivation keep the old behavior, including potentially unresolved keys; the comment that the slot key "still works" should not be interpreted as an unconditional recovery guarantee.
- One additional derivation per event is introduced; synchronous filesystem writes and their shutdown latency are inherited. I did not run a capacity/latency benchmark.
- The tested contract assumes stable admitted source/configuration. Arbitrary hot reconfiguration or source rebinding was not proven safe. Normal pending text recovery is not full multimodal event replay or live turn execution.
- Rollback restores old write semantics and can strand newly queued messages again; it does not undo committed transcript rows. Backup/recovery policy and durable exactly-once replay are separate from this narrow key fix.
Executed verification
Environment: Windows 11 x64, PM-built Python 3.14.7, pytest 9.1.1, pytest-asyncio 1.3.0, SQLite 3.53.1. Canonical Git Bash runner; 2 workers; HERMES_TEST_FILE_RETRIES=0; clean TZ=UTC, LANG=C.UTF-8, PYTHONHASHSEED=0. No live platform/provider requests. Test-only overlays in independent checkouts; original production head unchanged.
| Frozen scope | Actual result | Exit | Command wall time |
|---|---|---|---|
Final real normal contract on main (-k retained_events) |
2 aligned controls passed, 8 key-mismatch assertions failed; 3 fault cases deselected | 1 | 10.607s |
| Identical final file/argv on head | 10 passed, 0 failed; 3 fault cases deselected | 0 | 12.266s |
Three adverse contracts on head (-k adverse_fault) |
0 passed, 3 intended preservation assertions failed; 10 normal cases deselected | 1 | 11.301s |
| Same adverse contracts on main | Same three behavioral failures | 1 | 29.203s |
| Final unfiltered head cluster, 11 files | 64 passed, 3 inherited-fault assertions failed, no skips | 1 | 30.899s |
Local main + exact candidate patch, 8 files, -k 'not adverse_fault' |
57 passed, 0 failed; the 3 known adverse assertions explicitly deselected | 0 | 27.258s |
Final head scripts/check --base f86276d2ebcccf0530873c11e2965c3f1cbd5a31 |
11 checks OK; code health: 0 blocking, 0 advisory | 0 | 62.628s |
The final cluster contains 54 committed cases plus my 10 positive cases and 3 intentionally RED adverse cases. Repeated runs are not additional unique coverage. It is not a clean 67-test pass. The filtered 57-test integration is positive stable-config evidence only; the excluded failures are fully reported and reproduced, not removed or weakened. My reviewer file is not part of the submitted PR.
The identical final reviewer file in all three checkouts has SHA-256 4ca1a77c0398d93041673cd27a513f62557e394ed85a7d16c06e5e1571b01ee7. The exact PR-owned merge-base patch used for local integration has SHA-256 9489c4a0435959f92b054e5348445da15b7733e0175ec2b5c05d6d93a2a1e2e2.
An earlier unqualified all-check invocation hit a 300s outer timeout and supplied no final stage receipt. I do not count it as pass/fail behavioral evidence. The final explicit-comparison-base command above completed all 11 checks. Initial shared-object Git fetch attempts failed before testing; the investigation then used an independent repository. The primary dirty user checkout was not modified or used for results.
Reproduction commands and complete reviewer probe
Use the checkout's PM-built interpreter selected via HERMES_PYTHON, and the canonical runner, not bare pytest. Save the code below as tests/gateway/test_review_store_key_contracts.py in isolated main/head checkouts. No production edits are required.
HERMES_TEST_FILE_RETRIES=0 scripts/run_tests.sh tests/gateway/test_review_store_key_contracts.py -j 2 -k retained_events -- --tb=short
HERMES_TEST_FILE_RETRIES=0 scripts/run_tests.sh tests/gateway/test_review_store_key_contracts.py -j 2 -k adverse_fault -- --tb=short
python scripts/check --base f86276d2ebcccf0530873c11e2965c3f1cbd5a31The second command is expected to fail on the preservation assertions on both snapshots. The subprocess arm is a genuine fresh interpreter, not an in-memory reset of the replay guard.
Complete executed reviewer probe (two parametrized functions, thirteen cases)
"""Reviewer-only real SQLite/retention/restart contracts; no platform or provider calls."""
import json
from unittest.mock import patch
import pytest
from gateway.config import GatewayConfig
from gateway.platforms.event import MessageEvent, MessageType
from gateway.session import SessionStore
from gateway.shutdown_flush import recover_pending_to_db
from tests.gateway.restart_test_helpers import make_restart_runner, make_restart_source
@pytest.mark.asyncio
@pytest.mark.parametrize('flags', [
(True, False, True, False, 'dm', None),
(True, False, True, True, 'group', '900'),
(True, False, False, False, 'group', None),
(False, False, True, False, 'group', None),
(True, True, True, False, 'group', '900'),
])
@pytest.mark.parametrize('lane', ['adapter', 'runner'])
async def test_retained_events_roundtrip_real_sqlite(tmp_path, monkeypatch, flags, lane):
import hermes_constants
import hermes_state
sg, st, ag, at, chat_type, thread = flags
monkeypatch.setenv('HERMES_HOME', str(tmp_path))
monkeypatch.setattr(hermes_constants, 'get_hermes_home', lambda: tmp_path)
monkeypatch.setattr(hermes_state, 'DEFAULT_DB_PATH', hermes_state._IMPORT_DEFAULT_DB_PATH)
config = GatewayConfig(group_sessions_per_user=sg, thread_sessions_per_user=st)
store = SessionStore(sessions_dir=tmp_path / 'sessions', config=config)
runner, adapter = make_restart_runner()
runner.config = config
runner.session_store = store
adapter.set_session_store(store)
adapter.config.extra.update(group_sessions_per_user=ag, thread_sessions_per_user=at)
source = make_restart_source(chat_id='900', chat_type=chat_type, thread_id=thread)
entry = store.get_or_create_session(source)
db = store._db_for_key(entry.session_key)
assert db is not None
slot = adapter._source_session_key(source)
assert entry.session_key == store._generate_session_key(source)
contents = ['head'] if lane == 'adapter' else ['head', 'tail']
try:
for i, text in enumerate(contents):
event = MessageEvent(text=text, message_type=MessageType.TEXT, source=source, message_id=f'review-{i}')
# Actual production retention seam, not raw ledger/queue row injection.
runner._enqueue_fifo(slot, event, adapter)
assert event._gateway_accepted is True
if lane == 'adapter':
await adapter.cancel_background_tasks()
else:
with patch('gateway.status.remove_pid_file'), patch('gateway.status.publish_runtime_status'):
await runner.stop()
files = list((tmp_path / 'pending_messages').glob('*.json'))
assert len(files) == len(contents)
assert {json.loads(p.read_text(encoding='utf-8'))['session_key'] for p in files} == {entry.session_key}
store.close_all_db_handles()
store = SessionStore(sessions_dir=tmp_path / 'sessions', config=config)
db = store._db_for_key(entry.session_key)
assert db is not None
assert recover_pending_to_db(db, session_resolver=store.resolve_session_id_for_key) == len(contents)
assert recover_pending_to_db(db, session_resolver=store.resolve_session_id_for_key) == 0
assert [m['content'] for m in db.get_messages(entry.session_id)] == contents
finally:
store.close_all_db_handles()
# Re-open through the real reader and verify persisted order/content and index.
reopened = SessionStore(sessions_dir=tmp_path / 'sessions', config=config)
try:
assert reopened.peek_session_id(entry.session_key) == entry.session_id
restored_db = reopened._db_for_key(entry.session_key)
assert [m['content'] for m in restored_db.get_messages(entry.session_id)] == contents
assert not list((tmp_path / 'pending_messages').glob('*.json'))
finally:
reopened.close_all_db_handles()
@pytest.mark.asyncio
@pytest.mark.parametrize('fault', ['spool_write', 'unlink', 'append_once'])
async def test_review_adverse_fault_contract(tmp_path, monkeypatch, fault):
"""Adversarial preservation assertions: intentionally RED if an inherited limitation remains."""
import errno
import os
import subprocess
import sys
from pathlib import Path
import hermes_constants
import hermes_state
import gateway.shutdown_flush as flush
monkeypatch.setenv('HERMES_HOME', str(tmp_path))
monkeypatch.setattr(hermes_constants, 'get_hermes_home', lambda: tmp_path)
monkeypatch.setattr(hermes_state, 'DEFAULT_DB_PATH', hermes_state._IMPORT_DEFAULT_DB_PATH)
store = SessionStore(sessions_dir=tmp_path / 'sessions', config=GatewayConfig())
runner, adapter = make_restart_runner()
runner.session_store = store
adapter.set_session_store(store)
source = make_restart_source(chat_id='901', chat_type='dm')
entry = store.get_or_create_session(source)
event = MessageEvent(text='fault-probe', message_type=MessageType.TEXT, source=source, message_id='fault-1')
runner._enqueue_fifo(adapter._source_session_key(source), event, adapter)
assert event._gateway_accepted is True
try:
if fault == 'spool_write':
def fail_write(*args, **kwargs):
raise OSError(errno.ENOSPC, 'reviewer-injected spool write failure')
monkeypatch.setattr(flush, '_write_payload', fail_write)
await adapter.cancel_background_tasks()
files = list((tmp_path / 'pending_messages').glob('*.json'))
rows = store._db_for_key(entry.session_key).get_messages(entry.session_id)
# A retained payload or durable copy must still exist; no raw-row injection.
assert bool(adapter._pending_messages) or bool(files) or bool(rows), 'accepted event lost after failed spool write'
elif fault == 'append_once':
import sqlite3
tail_event = MessageEvent(text='fault-tail', message_type=MessageType.TEXT, source=source, message_id='fault-2')
runner._enqueue_fifo(adapter._source_session_key(source), tail_event, adapter)
assert tail_event._gateway_accepted is True
with patch('gateway.status.remove_pid_file'), patch('gateway.status.publish_runtime_status'):
await runner.stop()
store.close_all_db_handles()
store = SessionStore(sessions_dir=tmp_path / 'sessions', config=GatewayConfig())
db = store._db_for_key(entry.session_key)
original_append = db.append_message
failed = []
def append_with_one_failed_head(**kwargs):
if kwargs.get('content') == 'fault-probe' and not failed:
failed.append(True)
raise sqlite3.OperationalError('reviewer-injected exhausted transient append failure')
return original_append(**kwargs)
with monkeypatch.context() as scoped:
scoped.setattr(db, 'append_message', append_with_one_failed_head)
assert recover_pending_to_db(db, session_resolver=store.resolve_session_id_for_key) == 1
assert failed == [True]
assert recover_pending_to_db(db, session_resolver=store.resolve_session_id_for_key) == 1
content = [row['content'] for row in db.get_messages(entry.session_id)]
assert content == ['fault-probe', 'fault-tail'], f'pending FIFO replay inversion: {content}'
else:
await adapter.cancel_background_tasks()
files = list((tmp_path / 'pending_messages').glob('*.json'))
assert len(files) == 1
original_unlink = Path.unlink
def fail_unlink(path, *args, **kwargs):
if path == files[0]:
raise PermissionError('reviewer-injected unlink failure')
return original_unlink(path, *args, **kwargs)
with monkeypatch.context() as scoped:
scoped.setattr(Path, 'unlink', fail_unlink)
db = store._db_for_key(entry.session_key)
assert recover_pending_to_db(db, session_resolver=store.resolve_session_id_for_key) == 1
assert len(db.get_messages(entry.session_id)) == 1
assert files[0].exists()
# Same-process guard really works; do not simulate restart by clearing it.
assert recover_pending_to_db(db, session_resolver=store.resolve_session_id_for_key) == 0
store.close_all_db_handles()
child_code = (
'import json,sys; from pathlib import Path; '
'from gateway.config import GatewayConfig; from gateway.session import SessionStore; '
'from gateway.shutdown_flush import recover_pending_to_db; '
's=SessionStore(sessions_dir=Path(sys.argv[1])/"sessions",config=GatewayConfig()); '
'd=s._db_for_key(sys.argv[2]); '
'n=recover_pending_to_db(d,session_resolver=s.resolve_session_id_for_key); '
'rows=d.get_messages(sys.argv[3]); '
'print(json.dumps({"recovered":n,"content":[r["content"] for r in rows]})); '
's.close_all_db_handles()'
)
env = dict(os.environ, HERMES_HOME=str(tmp_path), HERMES_NIX_BUILD='1')
child = subprocess.run([sys.executable, '-c', child_code, str(tmp_path), entry.session_key, entry.session_id],
cwd=Path(__file__).resolve().parents[2], env=env, capture_output=True,
text=True, encoding='utf-8', timeout=45, check=True)
receipt = json.loads(child.stdout.strip().splitlines()[-1])
assert receipt['content'] == ['fault-probe'], f'fresh-process duplicate: {receipt}'
finally:
store.close_all_db_handles()Current remote state and merge conditions
The inspected remote PR is OPEN, draft, MERGEABLE but BLOCKED, with no checks reported, and the comparison is 1 candidate commit ahead / 209 base commits behind. These are snapshot statements, not proof that CI was never executed or that the branch is approved.
The exact five-file patch applied cleanly to captured main and passed the filtered local integration above. That is an uncommitted patch-level integration result, not a rebased/pushed candidate, a validation of remote CI, or permission to merge. Before merging, maintainers still need to clear the draft when appropriate and obtain the required checks/review on the resulting current head. Revalidate if head/base changes.
The earlier review at 9f2ba1000b7052a2146e03f03689b4f4aa4206c4 identified lint and a non-string-result test gap. Both are corrected at this head: the four invalid-result cases are present at test_shutdown_flush_store_key.py:165-175 and pass; the final full local checker passes. I am not carrying those old blockers forward or claiming the other reviewer's broader test totals as my own.
Bottom line: the issue's P1 key-mismatch defect is substantiated and repaired in the tested scope; no new P0/P1 regression was substantiated. The three reproduced inherited fault modes remain explicit follow-up work, and the patch must not be advertised as exactly-once, complete multimodal, crash-proof, or platform/provider E2E recovery.
Final expanded decision: fix the demonstrated P2 partial-source recall recovery regression before merging this head; no additional new P1/P3 hard blocker was substantiated in the examined scope. The submission remains COMMENTED, not a formal blocking review decision.
| for session_key, value in list(pending.items()): | ||
| if value is not None: | ||
| flushed += _flush_value(flush_dir, "pending", session_key, value, reason=reason, ts=ts) | ||
| key = _recoverable_key(value, session_key, session_key_for) |
There was a problem hiding this comment.
[P2] Preserve the matched owner of partial-source recall notices
For a supported Yuanbao group/thread with group_sessions_per_user and thread_sessions_per_user true, RecallGuardMiddleware accepts a callback without from_account, matches the live slot by message ID, and writes its notice directly into that matched slot. The notice source has user_id=None. Re-deriving here drops :u1 from the already-valid store/adapter key: agent:main:yuanbao:group:group:G:main:u1 becomes agent:main:yuanbao:group:group:G:main. Real SQLite recovery after close/reopen returns zero and strands the notice JSON. The same four-case decoder/producer/shutdown/recovery probe passes 4/4 on merge base and current main, but fails the two missing-sender assertions on this head and on main plus the exact PR patch; both known-sender controls pass. The full probe and limitations are in the review body. Preserve established ownership for this supported synthetic producer without undoing the ordinary adapter/store mismatch fix before merging. This is conditional recovery failure, not proven byte deletion or network/provider E2E.
`_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 #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.
The conditional P2 is reproduced on this head and fixed in
|
| Tree | spooled session_key |
recover_pending_to_db |
landed in |
|---|---|---|---|
merge base f86276d2 |
agent:main:telegram:group:group:G:main:u1 |
1 | the original session |
head 5c3350a9b9 |
agent:main:telegram:group:group:G:main |
0 | — stranded, Cannot recover pending message for …: session_key-to-id resolution failed, payload left in the spool |
| head + fix, missing sender | agent:main:telegram:group:group:G:main:u1 |
1 | the original session |
head + fix, from_account=u1 (control) |
agent:main:telegram:group:group:G:main:u1 |
1 | the original session |
That matches your reading exactly: the head introduced the regression, and the merge base is green
for the reason you gave.
What changed
_recoverable_key now records the slot key when the store can route it, and keeps the derivation
for the case that motivated #136189:
slot_key_routes(new, optional) is built bystore_slot_key_routesfrom
SessionStore.resolve_session_id_for_key— the same callable recovery hands payloads to. A slot
key it lands is recorded as it stands; a slot key it cannot land is re-keyed bysession_key_for
exactly as before.- The slot key is the established owner: it is the key the producing adapter queued the value under,
the key_interrupt_for_recallwrites into, and the key the live drain re-dispatches it with
(base.pypasses the slot key to the drain), so preferring it when it is routable restores the
pre-PR owner rather than inventing a third one. - The conservative edges are unchanged in both directions: a resolver that raises, a store without
the resolver, a value with nosource, or a derivation that does not return a non-emptystrall
keep the slot key; the derivation is used exactly when the slot key cannot be resolved.
Why this shape, not "record the slot key" and not a producer-side source repair
- Recording the slot key unconditionally would undo 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: there the slot key is precisely the key
that resolves nowhere and the derivation is what makes the payload recoverable. Which of the two
keys owns the payload is not a property of the keys' shape — both cases are "the derived key is a
strict prefix of the slot key" — it is only knowable by asking the store, so the flush asks. - Carrying the accepted source through the synthetic replacement is the other credible direction you
name, and I did not take it: the notice'ssourcealso feeds delivery and scope, and the defect is
key selection at the flush seam, not attribution. The flush rule also covers a sibling producer you
have not seen yet, without touching the recall path.
Evidence on the new head
scripts/run_tests.sh tests/gateway/test_shutdown_flush_store_key.py→
=== Summary: 1 files, 12 tests passed, 0 failed (100% complete) in 2.7s (8 workers) ===. All ten
tests already in that file — the two-derivations-disagree case, the adapter-shutdown and
runner-overflow recordings, the next-boot recovery, the no-source slot, the failing derivation and
the four invalid-result parameters — pass unmodified; the two new ones are the missing-sender
notice (keeps:u1, recovers into the original session) and the same rule throughrunner.stop().- The 14-file adjacent set (
test_shutdown_flush*.py,test_pending_*,test_profile_spool_recovery.py,
test_restart_resume_pending.py,test_gateway_shutdown.py,test_yuanbao_shutdown.py,
test_queued_final_ledger.py,platforms/test_yuanbao_recall_db_only.py) →
121 tests passed, 0 failed. - The PR's own eval entry point
evals/shutdown_flush_adapter_key_strand.pystill prints
VERDICT=RECOVEREDwith the payload spooled under the store key: the ordinary repair is intact. python scripts/check(11 checks, code-health ratchet included, basef86276d2) →ok,
0 blocking. Both flush call sites keep their files at their current line counts, which is why the
keying is passed as a bundle rather than a fourth line inbase.py/run_shutdown.py.
Boundaries
This is recovery-key selection and nothing wider. I did not verify byte-level deletion, provider or
network E2E, or delivery-ordering beyond the existing ts/seq sort, and I have not re-examined the
three inherited fault modes in this commit. The routability check costs one resolver call per spooled
value at shutdown — the same call the next boot's recovery makes for that key, on the same
_db_for_key path — and every failure path in it falls back to today's derivation.
The new commit supersedes the reviews recorded on 5c3350a9b9 as head; that commit stays in the branch
history if you want to diff the two states directly.
…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.⟫
|
Verification pass on the new head Your probe, verbatim, on the head you reviewed vs the new head
The failing spool key I see is Second branch of the same producer:
|
| tree | 6 tests |
|---|---|
merge base f86276d2 |
6 passed |
5c3350a9b9 |
3 failed, 3 passed — group missing-sender (recovery and spool key) plus direct missing-sender (recovery) |
5b77013e2d |
6 passed |
Adjacent surface on the new head
test_shutdown_flush.py, test_shutdown_flush_recovery.py, test_shutdown_flush_store_key.py,
test_gateway_shutdown.py, test_yuanbao_shutdown.py, test_yuanbao_pipeline.py,
tests/gateway/platforms/test_yuanbao_recall_db_only.py, test_pending_queue_spool.py,
test_restart_resume_pending.py, plus the two files above:
159 tests passed, 0 failed. evals/shutdown_flush_adapter_key_strand.py prints
VERDICT=RECOVERED on 5b77013e2d and VERDICT=STRANDED on f86276d2. ruff check is clean on the
added file and scripts/check --only health exits 0 on this head.
Not claimed
The three inherited fault modes are untouched by this commit: your full roundtrip probe fails those
same three contracts on the merge base (11 failed, 2 passed at f86276d2) and on a fixed tree
(3 failed, 10 passed), so they remain follow-up work rather than part of this claim. I have not
re-examined delivery ordering, provider E2E or byte-level deletion here either.
Please re-run your Ledger probe on 5b77013e2d; the fix itself is 40f62fa4, the added commit is
coverage only.
ehz0ah
left a comment
ehz0ah
left a comment
There was a problem hiding this comment.
I re-reviewed 5b77013e2dc80254e19e4dc9f3ee91b8b02a0fcd after the recall-notice fix.
The shutdown spool now uses the store key when an adapter slot cannot route, while keeping a routed slot key when the queued event has only a partial source. That preserves the original owner for sender-less Yuanbao group and direct-message recall notices. The adapter and runner paths apply the same rule to pending and overflow events.
The 18 focused shutdown and recall tests passed on this head and after a clean merge into current main at 899f93b9b86c236ec73ff23aa9e5422270c0503e. All 11 repository checks also passed in both states with no code-health findings. The standalone recovery probe reported VERDICT=RECOVERED. GitHub reports no hosted checks for this head.
The earlier recall blocker is resolved. I found no new merge blocker in the changed scope. One small cleanup remains: tests/gateway/test_recall_notice_flush_identity.py has an extra blank line at EOF, but it does not affect behavior or the repository checks.
…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) ===
The one cleanup you flagged is real, and it is fixed in
|
|
Follow-up review of PR #136600 at head The latest commit only removes the trailing blank line from the recall test; it does not change production code. The earlier Ruff/fallback-test concerns are fixed, and the two focused PR test files pass 18/18. I still would not merge this head: the key-selection helper has two conditional P2 ownership/recovery gaps.
At Repro: with per-user group/thread keys enabled, a supported Yuanbao recall callback without
A positive result from These two cases need a rule that preserves the matched owner for partial-source producers without accepting a different routable owner or treating an unavailable lookup as a definitive miss. Please add paired regressions for both directions before merging. Code locations for the two findings:
Other verification: The three fault-mode findings already raised in the earlier thread (failed spool write followed by clearing, duplicate replay after unlink failure/restart, and same-session FIFO inversion after a failed head append) remain unchanged by this patch; they are inherited, not new regressions here, and I did not rerun their fault-injection probes on this head. |
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 #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.
|
Both P2s reproduce on I reproduced each finding independently before changing anything, on the head you reviewed and with real SQLite, the way your repros do:
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. Paired regressions for both directions in
Both fail on Verification on
No full-suite result is claimed. The patch changes only which string is recorded — not the write, unlink or FIFO ordering — so the three inherited fault-mode findings are untouched by it, and I have not re-run their fault injections on this head either. One residual, stated rather than left implicit: when both keys have a live session and they differ, something has to break the tie. The rule breaks it toward the store's own admission for the payload's source — what normal turn admission uses — and the ownership regression above pins that direction. Deciding it from the session side instead would need a source-vs-origin ownership check inside resolution itself, which is a wider change than this PR's scope. PR is still |
4 participants
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: