Skip to content

fix(gateway): replay the transcript spool in drop order with full fidelity on restart (supersedes #78323) - #84785

Open
briandevans wants to merge 8 commits into
NousResearch:mainfrom
briandevans:fix/gateway-spool-recovery-order-fidelity-78323
Open

briandevans wants to merge 8 commits into
NousResearch:mainfrom
briandevans:fix/gateway-spool-recovery-order-fidelity-78323

Conversation

@briandevans

@briandevans briandevans commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

supersedes #78323

Credit

@686f6c61's #78323 found both of these defects first, and diagnosed both correctly. Its _next_spool_file_id() docstring names the ordering bug exactly — "Recovery walks sorted(glob("*.json")). uuid4 names re-insert out of order after a burst of pending-cap spools" — and its append_kwargs loop is a direct attempt at the fidelity bug. That PR is not stale because it was wrong. It is stale because the ground moved under it.

de0f20ff05b (2026-08-09) landed the runtime transcript spool independently: spool_dropped_transcript_message / drain_transcript_spool, plus a new TRANSCRIPT_CAP_DROP_REASON branch inside recover_pending_to_db. That superseded #78323's own spool_transcript_messages. #78323 now reads mergeable: false, mergeable_state: dirty, has been cold since 2026-08-05 with zero reviews, and its diff still deletes import uuid and rewrites regions that no longer exist.

Both of the defects it identified are still live on main. This PR ships them against the code that exists now. Three things had to change in the execution, and they are matters of substance rather than rebase mechanics:

  1. Where the order comes from. fix(state): verify FTS rebuild write path; spool pending-cap overflow #78323 fixes ordering by renaming future spool files (time_ns + counter). Main's payloads already carry ts and a monotonic seq, stamped by spool_dropped_transcript_message. Sorting on the payload orders the spool files a user already has on disk — which are exactly the files the incident produced, and which are named pending-<uuid4>.json and cannot be retroactively renamed. A filename scheme only helps files written after it ships.
  2. Where the fix lands. fix(state): verify FTS rebuild write path; spool pending-cap overflow #78323's fidelity work edits the legacy data["text"] branch. On current main a cap-drop payload never reaches that code: it leaves the TRANSCRIPT_CAP_DROP_REASON branch via continue at shutdown_flush.py:350.
  3. Which idiom to copy. The reference implementation is 90 lines above the bug in the same file. drain_transcript_spool already orders the identical payloads with sorted(entries, key=lambda e: e[:3]) over (ts, seq, path.name). Matching it makes the two drains agree instead of introducing a third convention.

What does this PR do?

gateway/shutdown_flush.py has two consumers of the transcript spool, and they disagree about what the spool means.

drain_transcript_spool (:193) is the live drain, called from gateway/session.py once a transcript flush succeeds. It sorts on (ts, seq, path.name) and replays the full message dict through SessionStore._append_transcript_message, which forwards 15 fields.

recover_pending_to_db (:286) is the restart drain, called unconditionally from gateway/run.py:27979 after runner.start(). On the same files, it did two things wrong:

1 — Order. flush_files = sorted(flush_dir.glob("*.json")) (:308). _write_payload names files pending-<uuid4().hex>.json, so this sort is random. The payloads carry ts/seq; this path ignored them.

This is not cosmetic. SessionDB restores a conversation with ORDER BY id — AUTOINCREMENT insertion order, never timestamp — and the comment at hermes_state.py:8691-8699 says why: sorting otherwise risks "breaking tool-call/response adjacency and triggering an HTTP 400 on replay." Replaying the spool in filename order writes that inversion straight into the row ids.

2 — Fidelity. The replay forwarded four fields — session_id, role, content, timestamp — and then path.unlink() at :349 made the loss permanent. Discarded: tool_calls, tool_call_id, tool_name, the reasoning and codex columns, platform_message_id, observed, and the api_content sidecar. append_message (hermes_state.py:7643) already accepts every one, and the payload already carries them.

Two of those are load-bearing. Losing tool_calls/tool_call_id orphans a tool result from the call it answers — the exact adjacency ORDER BY id exists to protect. Losing api_content contradicts the requirement stated at the live writer (gateway/session.py:3673): the sidecar "must survive any gateway-side persistence path or the next turn's replay diverges at this row." This is such a path.

3 — Partial-failure ordering. Ordering the files only fixes the happy path. If append_message failed partway through — the DB is still unhealthy, which is the situation that created the spool — the loop kept going and wrote the later messages. The failed one is retried on a future start and gets a higher row id than the messages it originally preceded, so the inversion lands anyway, permanently. drain_transcript_spool already states the rule for the same spool: "On the first replay failure the drain stops and remaining files are kept for the next attempt (the DB is likely still unhealthy)."

This restores the contract de0f20ff05b set out in its own commit message — to "drain and replay spooled messages in drop order", with "replay failures keep the spool files for the next attempt" — on the restart path, which is the path that never implemented it.

User-visible symptom: after recovering from the FTS corruption of #78182, restarting the gateway brings the session back scrambled, with tool calls stripped of their results, and the next turn fails with an HTTP 400.

Related Issue

Refs #78182, #82616 (both closed by de0f20ff05b; the two defects above are in the recovery path that commit added and are still present on main).

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • gateway/shutdown_flush.py_order_flush_files() + _sort_number(): parse each recovery payload once and order by (ts, seq, filename), mirroring drain_transcript_spool. Unparseable payloads sort last and are handed back unparsed so the existing loop reports and preserves them exactly as before.
  • gateway/shutdown_flush.py_transcript_append_kwargs(): build the append_message call by mirroring SessionStore._append_transcript_message field for field, including its role gate on the assistant-only reasoning columns and its message_id fallback for platform_message_id. Fields are whitelisted explicitly rather than splatted from the message dict, because the payload is arbitrary JSON from disk and an unexpected key would raise TypeError and abort the recovery pass.
  • gateway/shutdown_flush.pyblocked_sessions: after a failed replay, skip that session's remaining spooled messages and leave them on disk. Scoped per session, since replay order is only defined within a session and this function drains every session in one pass. Non-transcript pending payloads are unaffected.
  • tests/gateway/test_shutdown_flush_recovery.py — new file, 10 tests.

Sibling sweep: recover_pending_to_db and drain_transcript_spool are the only two consumers of this spool (grep for TRANSCRIPT_CAP_DROP_REASON / pending-*.json over non-test sources). drain_transcript_spool was already correct on all three points, which is where the idiom came from; this PR brings the second consumer up to it and touches nothing else.

How to Test

# 1. The new regression suite
pytest tests/gateway/test_shutdown_flush_recovery.py -q          # 10 passed

# 2. Everything that already covered this file and this spool
pytest tests/gateway/test_shutdown_flush.py \
       tests/gateway/test_pending_queue_spool.py -q              # 11 passed

Fails-before / passes-after, verified per hunk by reverting each production change individually against the rest of the branch:

Reverted hunk Failing tests
ordering (_order_flush_files) 2 — test_replays_in_drop_order_when_names_disagree, test_seq_breaks_ties_within_the_same_second
fidelity (_transcript_append_kwargs) 5 — structured fields, tool-result identity, role gate, message_id fallback, epoch-0 timestamp
failure-stop (blocked_sessions) 1 — test_failure_blocks_later_messages_for_that_session_only
all three (clean origin/main) 8 of 10

The 2 that pass on main are deliberate behaviour-preservation assertions, not regression coverage: corrupt payloads stay on disk and are still reported, and payload["ts"] remains the timestamp fallback.

One planned commit was dropped after checking it. "Keep the spool file when replay fails" looked like a fourth defect, but on main path.unlink() already sits inside the try after append_message, so a failed replay never reaches it. Probed directly against unmodified origin/main — the file is retained and the test passes without any change. It would have been a commit that passes with or without the fix, so it is not here.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass — ran the targeted suites listed under "How to Test" (20 passed), not the full tree locally; leaving this unticked rather than claiming it.
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS (Darwin 25.4.0), Python 3.11

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — docstrings on the new helpers and on recover_pending_to_db
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A, no config keys
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — pure stdlib json / pathlib / sort, no platform-specific calls added
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

Related / Positioning

Deduped two ways, because neither net is sufficient alone — gh search prs indexes PR title/body text and never changed paths, while gh pr list --json files returns newest-first and only samples the head of a ~17.8k-PR queue.

By filegateway/shutdown_flush.py is touched by exactly 5 open PRs: #78323, #75536, #69980, #83620, #84131.

By text/symbolrecover_pending_to_db#75536, #78323. drain_transcript_spool#84131. shutdown_flush, TRANSCRIPT_CAP_DROP_REASON, spool_dropped_transcript_message → nothing. Note the text net missed #83620 entirely, because that PR's title is about SessionDB connection leaks; only the by-file pass caught it. Conversely the by-file recency query returned empty here — its 300-PR window spans #84168#84776, so every one of these five is older than it can see. Both passes were necessary.

Dispositions:

PR Author Relationship
#78323 @686f6c61 Superseded. Same two defects; dirty, cold since 08-05, zero reviews. Credited above.
#83620 @JoaoMarcos44 Near-adjacent — disclosed. Edits the same function at @@ -316,6 +316,14 @@, but for a different concern: closing an owned SessionDB on exception paths. Its hunks are at :316, :389, :396; mine are at :308 and :342. Disjoint, and complementary.
#84131 @CryptoDombili Adjacent, live, complementary. Its title ("preserve transcript spool chronology") is close to this one, so worth being precise: its shutdown_flush.py hunks are @@ -190,7 @@ and @@ -199,14 @@, both inside drain_transcript_spool, and it fixes ordering between the spool tier and the in-memory queue during live operation. It contains zero references to recover_pending_to_db, flush_files, or TRANSCRIPT_CAP_DROP_REASON. Neither PR changes a line the other touches.
#75536 @spfcraze Disjoint — session_key resolution on the legacy branch.
#69980 @xiaoyaner0201 Disjoint — trusted-sender envelope; hunks on the text-payload branch.

Structural argument, verified rather than asserted: the TRANSCRIPT_CAP_DROP_REASON branch this PR fixes did not exist until de0f20ff05b (2026-08-09). Checking each rival head with git merge-base --is-ancestor de0f20ff05b <head>:

#78323 head 3441cc8ba (2026-08-05)  -> PREDATES de0f20ff05b
#75536 head b1db79117 (2026-07-31)  -> PREDATES de0f20ff05b
#69980 head edf643f63 (2026-08-05)  -> PREDATES de0f20ff05b
#83620 head 6eea62f6a (2026-08-10)  -> has it
#84131 head 3485f0d71 (2026-08-12)  -> has it

The three that predate it cannot touch this branch at all. The two that postdate it are the two disclosed above, and both are line-disjoint from this diff.

Test files: deliberately placed in a new file. tests/gateway/test_shutdown_flush.py is appended near EOF by #75536, #83620 and #69980, and tests/gateway/test_pending_queue_spool.py by #84131. This PR touches neither.

Commits

Each independently green (verified by checking out every intermediate SHA and running the touched suites — 11 / 11 / 11 / 14 / 20 / 21 passing):

  1. fix(gateway): replay cap-dropped transcript spool files in drop order on restart
  2. fix(gateway): preserve structured transcript fields when recovering spooled messages
  3. fix(gateway): stop a session's spool replay after the first failed append
  4. test(gateway): cover spool replay ordering across a restart
  5. test(gateway): cover field fidelity and failure handling in spool recovery
  6. fix(gateway): fall back to the payload clock only when a timestamp is absent — addresses the review finding below; epoch 0 is a valid timestamp and the inherited or expression would have rewritten it.

… on restart

recover_pending_to_db walks sorted(flush_dir.glob("*.json")) to replay
spool files left behind by a restart. Spool files are named
pending-<uuid4>.json by _write_payload, so that sort is effectively
random: a transcript recovered after a crash comes back scrambled.

The damage is permanent rather than cosmetic. SessionDB restores a
conversation with "ORDER BY id" — AUTOINCREMENT insertion order, never
timestamp — precisely so a non-monotonic clock cannot sort an assistant
tool_calls row after its tool response, "breaking tool-call/response
adjacency and triggering an HTTP 400 on replay". Replaying the spool in
filename order writes exactly that inversion into the row ids, so the
session errors out on the next turn.

The payloads already carry the ordering fields: spool_dropped_transcript_
message stamps every one with "ts" and a monotonic "seq". The sibling
consumer of the same spool in this file, drain_transcript_spool, already
sorts on them via sorted(entries, key=lambda e: e[:3]); only the restart
path ignored them. Order on (ts, seq, filename) so the live drain and the
cross-restart drain agree.

Ordering by the payload rather than by a new file-naming scheme also
recovers spool files that were written before this change, which are the
ones an affected user already has on disk.

Payloads that cannot be parsed keep their previous treatment: they sort
last, are handed back unparsed, and are re-read inside the loop so the
existing handler reports and preserves them unchanged.
…pooled messages

The cross-restart replay of a cap-dropped transcript message forwarded
only four fields to append_message — session_id, role, content, timestamp
— and then unlinked the spool file, making the loss permanent.

Everything else on the message was discarded: tool_calls, tool_call_id,
tool_name, the reasoning columns, codex items, platform_message_id,
observed, and the api_content sidecar. append_message already accepts all
of them, and the payload already carries them, because
spool_dropped_transcript_message writes the full transcript message dict.

Two of the dropped fields are load-bearing rather than decorative. Losing
tool_calls/tool_call_id orphans a tool result from the call it answers,
which is the adjacency SessionDB's "ORDER BY id" exists to protect.
Losing api_content contradicts the requirement stated at the live writer,
that the sidecar "must survive any gateway-side persistence path or the
next turn's replay diverges at this row" — and this is such a path.

The same spool is drained two ways: by drain_transcript_spool during live
operation, which replays through SessionStore._append_transcript_message
and keeps every field, and by this function after a restart. Same files,
same payloads, different fidelity. Mirror _append_transcript_message field
for field so the outcome no longer depends on whether the gateway happened
to restart, including its role gate on the assistant-only reasoning
columns and its message_id fallback for platform_message_id.

Fields are whitelisted explicitly rather than splatted from the message
dict: the payload is arbitrary JSON from disk, and an unexpected key would
raise TypeError and abort the whole recovery pass. content is now passed
through as-is instead of being coerced to "", since an assistant tool-call
row legitimately has no content.
…pend

Ordering the spool files fixes the happy path only. If append_message
fails partway through — the DB is still unhealthy, which is the situation
that produced the spool in the first place — the loop logs the failure,
keeps that file for a later retry, and then carries on and writes the
messages that come after it.

Those later messages land now; the failed one lands on some future start.
Because SessionDB orders a conversation by AUTOINCREMENT id, the retried
message then gets a HIGHER row id than the messages it originally
preceded, and the inversion this pass just prevented is written to disk
anyway — this time permanently, since both files are gone.

drain_transcript_spool, the live drain of the same spool, already states
the rule: "On the first replay failure the drain stops and remaining
files are kept for the next attempt (the DB is likely still unhealthy)."
Apply it here too.

The block is per-session rather than global. Replay order is only defined
within a session, and this function drains every session's spool in one
pass, so a single unhealthy session must not strand the others' messages
on disk. Non-transcript pending payloads are unaffected.
Names the spool files so that filename order is the exact reverse of drop
order, which is what uuid4 names produce on average, and asserts the
replay comes back in drop order. A second case gives three payloads the
same one-second ts so only the monotonic seq can separate them.

Both fail on the previous sorted(glob()) implementation. A third case
pins the pre-existing treatment of a corrupt payload — reported by the
loop's own handler and left on disk — so the new ordering pass cannot
silently swallow one.

Lives in its own file rather than tests/gateway/test_shutdown_flush.py to
keep the restart-recovery cases together with the spool fixtures they
need.
…overy

Round-trips an assistant tool-call row carrying every field the live
writer persists and asserts each one reaches append_message: tool_calls,
tool_call_id, tool_name, the reasoning and codex columns,
platform_message_id, observed, timestamp, and the api_content sidecar.
Also pins content=None passing through uncoerced, since an assistant
tool-call row has no content, and the role gate that keeps the
assistant-only reasoning columns off a user row.

A separate case covers a failed replay: with the first of two messages
for one session rejected, neither is written and both spool files stay on
disk, while a third message for an unrelated session still recovers.

All but the two behaviour-preservation cases fail on the previous
implementation.
Copilot AI lite review requested due to automatic review settings August 12, 2026 19:53

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes restart-time replay of cap-dropped transcript spool files so recovered transcripts preserve drop order, structured message fidelity, and failure semantics consistent with the live drain path. It tightens the gateway’s shutdown/restart recovery so that a restart cannot durably scramble transcripts or strip tool-call adjacency-critical fields.

Changes:

  • Order restart-time replay of spooled transcript messages by (ts, seq, filename) by parsing payload metadata rather than relying on UUID filename sort.
  • Replay transcript spool messages with the full set of SessionDB.append_message fields (tool call metadata, reasoning columns with role gating, api_content sidecar, etc.).
  • Stop replaying further messages for a session after the first append failure, preserving remaining spool files for retry; add a dedicated regression test suite.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
gateway/shutdown_flush.py Orders restart-time spool replay by payload metadata, replays full-fidelity transcript message fields, and blocks per-session replay after first failure.
tests/gateway/test_shutdown_flush_recovery.py Adds focused regression tests for ordering, field fidelity, corrupt payload handling, and failure-stop semantics in restart-time recovery.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread gateway/shutdown_flush.py
Comment on lines +383 to +385
"observed": bool(message.get("observed")),
"timestamp": message.get("timestamp") or payload.get("ts"),
# The api_content sidecar is the exact bytes sent to the API for this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — verified and fixed in 62206b0bcd7 (current head).

timestamp now falls back only when the field is genuinely absent:

timestamp = message.get("timestamp")
if timestamp is None:
    timestamp = payload.get("ts")

Regression test: test_epoch_zero_timestamp_is_not_replaced_by_the_fallback in tests/gateway/test_shutdown_flush_recovery.py. It spools a message with timestamp: 0 under a payload ts of 999 and asserts 0 reaches append_message. It fails on the previous or expression and passes now.

One deliberate non-change for the record: platform_message_id a few lines up still uses or. That is not an oversight — it mirrors SessionStore._append_transcript_message (gateway/session.py:3671) verbatim, and having the restart drain agree with the live drain field for field is the point of this PR. Diverging there would reintroduce the split this change removes.

@briandevans

briandevans commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

CI audit — the red test slices are an infrastructure failure, not a test failure.

The failing jobs all die in the Install ripgrep (prebuilt binary) step (.github/workflows/tests.yml:60), which runs before Install uv and therefore before any Python or test code executes:

curl -sSfL --retry 3 --retry-delay 5 -o "$RG_TARBALL" ...
curl: (22) The requested URL returned error: 503     <- initial + all 3 retries
##[error]Process completed with exit code 22

That is a 503 from the GitHub release CDN serving BurntSushi/ripgrep v15.1.0 — a pinned third-party download this PR does not touch. Those jobs never reach pytest, so they report no test result.

The affected slices are non-deterministic across runs of the same code, which is the signature of a flaky external download rather than a defect:

Head Slices that failed the ripgrep download
869acc61272 1, 9
62206b0bcd7 5, 9, 11

Same tree apart from a three-line timestamp fix, different random victims.

Every slice that got past the download passed. On 869acc61272, the slice carrying the new file reported:

✓ tests/gateway/test_shutdown_flush_recovery.py (9✓, 1.1s)
=== Summary: 232 files, 2449 tests passed, 0 failed, 46 skipped (100% complete) ===

That same slice also carried tests/gateway/test_pending_queue_spool.py — the existing coverage for this spool, added with de0f20ff05b — which passed alongside it.

I can't re-run upstream workflows from a fork. If the download keeps flaking, say the word and I'll push a rebase to retrigger.

… absent

_transcript_append_kwargs chose the spooled message's timestamp with
`message.get("timestamp") or payload.get("ts")`, carried over from the
code this PR replaces. Truthiness is the wrong test: epoch 0 is a valid
timestamp and would be silently rewritten to the payload's ts, which is
the fidelity loss this PR exists to remove.

Fall back only when the field is genuinely absent.
@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery area/sessions Session lifecycle, resume, persistence, history P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state needs-decision Awaiting maintainer decision before any implementation labels Aug 12, 2026
@686f6c61

Copy link
Copy Markdown
Contributor

Thanks for the credit and the careful re-grounding against current main.

Agreed that #78323 is stale for the right reasons (post-de0f20ff05b spool layout + cap-drop branch), not because the diagnosis was wrong. Sorting on payload (ts, seq) for already-on-disk pending-<uuid4>.json is the better recovery shape now; the filename-time scheme only helped writers after ship.

Closing #78323 as superseded by this PR (FTS write-path half of #78182 already landed via #82719).

@briandevans

Copy link
Copy Markdown
Contributor Author

@686f6c61 Appreciated — and thanks for closing #78323 cleanly rather than leaving it to rot. For the record I re-checked both: #78323 is closed unmerged (2026-08-12T20:44:12Z), and the FTS write-path half of #78182 did land via #82719, merged 2026-08-09T21:10:21Z. So the split you describe is exactly what happened.

Confirming the (ts, seq) reasoning, since it is the load-bearing part: recovery reads files that are already on disk, so anything encoded in the filename is only as good as the writer that produced it — and pending-<uuid4>.json encodes nothing at all. The payload is the only record of when the message was dropped, so the sort key is (payload["ts"], payload["seq"]), which also matches what drain_transcript_spool already does live with sorted(entries, key=lambda e: e[:3]). Two details worth naming because they are where this gets subtle:

  • The key runs through _sort_number(), which coerces to float and sorts unusable values first. A raw key mixing str and int raises TypeError mid-sort and takes down the whole recovery pass, and a corrupt spool is precisely the case recovery exists for.
  • seq is not decoration — it breaks ties within the same second, which is the common case when a cap drop sheds several messages at once. Covered by test_seq_breaks_ties_within_the_same_second (tests/gateway/test_shutdown_flush_recovery.py:92), alongside test_replays_in_drop_order_when_names_disagree (:68), which fails on the old sorted(glob("*.json")) path.

A filename-time scheme would have worked for messages written after it shipped and for nothing already spooled, which is the population that matters on a restart.


Correction to my CI audit above — it is now stale, and the diagnosis in it no longer describes this PR's checks.

That comment blamed the Install ripgrep (prebuilt binary) step at .github/workflows/tests.yml:60 returning HTTP 503. After the re-run, at head 4c719aae3, that failure is gone — ripgrep installs successfully in every job. The current rollup is:

conclusion count
SUCCESS 30
SKIPPED 12
NEUTRAL 1 (osv-scanner)
FAILURE 2

All 12 Python tests / Run tests slice N/12 jobs are SUCCESS. The only substantive red is Python tests / e2e; the second FAILURE is the All required checks pass aggregate reporting it.

e2e dies in Set up Python 3.11, i.e. before Install dependencies and before any Python runs — steps 6, 7 and 8 (Run e2e tests) are all skipped:

error: Failed to install cpython-3.11.14-linux-x86_64-gnu
  Caused by: Failed to download https://github.com/astral-sh/python-build-standalone/releases/download/20260127/cpython-3.11.14%2B20260127-x86_64-unknown-linux-gnu-install_only_stripped.tar.gz
  Caused by: client error (SendRequest)
  Caused by: http2 error
  Caused by: stream error received: refused stream before processing any application logic
  Caused by: Request failed after 3 retries

So: a different third-party CDN artifact than last time (astral-sh/python-build-standalone toolchain, not BurntSushi/ripgrep), same class of failure — a pinned external download this PR does not touch, failing after its own retries, with zero tests executed in the affected job. Every job that got a Python interpreter passed.

I have left the previous audit's table in place rather than editing it, but it describes the earlier run and should not be read as current.

@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

PR: fix(gateway): replay the transcript spool in drop order with full fidelity on restart (supersedes #78323)

  1. Legacy payloads without ts/seq sort first, then by filename. _sort_number maps missing/non-numeric ts/seq to 0.0, so any spool file written by an older format (or with corrupt ordering fields) sorts ahead of all real payloads, and among themselves falls back to filename order — which is exactly the random pending-<uuid4>.json order this PR fixes. The failure is at least contained (they replay before real entries, so per-session order among old files may scramble). Consider a documented tiebreak or logging when files sort via the fallback.

  2. Held-back messages for a blocked session are skipped silently. When a message for a session fails to append, later spool files for that session continue without any log line (gateway/shutdown_flush.py recover_pending_to_db); only the first failure is reported. An operator won't know how many messages remain queued for that session until the next start. A summary warning ("kept N spool files for session X") would make the retry state visible.

  3. _transcript_append_kwargs mirrors the live writer field-for-field. The docstring says it mirrors SessionStore._append_transcript_message — if the live writer later gains a column (or changes a default), recovery silently diverges. Extracting a shared kwargs builder used by both the live drain and the recovery path would prevent drift.

  4. Minor: content no longer defaults to "" (deliberate — None is preserved for tool-call rows, and the test pins it). Confirm SessionDB.append_message handles None content across all paths, including FTS indexing, so a NULL content row doesn't trip an indexer assumption.

@briandevans

Copy link
Copy Markdown
Contributor Author

#78323 is closed unmerged; its author identified #84785 as the operative replacement. Both cover shutdown-flush recovery in gateway/shutdown_flush.py; #84785 is currently MERGEABLE/CLEAN with 45 completed checks and no failed or pending checks.

teknium1 pushed a commit that referenced this pull request Sep 18, 2026
… crashes the remaining file scans

Same class as the Bot Chat drain wedge already on this branch: every JSON-file
scan guarded "did it parse?" and then assumed the value was a dict. A file
holding `42`, `"oops"` or `[1,2,3]` (corruption, truncated write, foreign tool)
passed the guard and raised AttributeError/TypeError at the first `.get()`,
usually before a single healthy sibling was processed. Each site now treats a
non-object payload like a corrupt file under that subsystem's existing policy:

- tools/bot_relay.py::_expire_if_stale / claim_pending_envelopes — the
  envelope is skipped by the sweep and not claimed (same as unparseable).
- tools/browser_lightpanda.py::reap_orphaned_lightpanda — record unlinked,
  scan continues.
- tools/write_approval.py::list_pending / get_pending — record skipped with
  the existing "unreadable pending record" warning / None.
- tui_gateway/methods_session.py::_legacy_spawn_tree_entry / spawn_tree.load —
  scalar snapshot reads as empty / returns the existing 5000 error instead of
  violating the SpawnTreeLoadResult contract.
- hermes_cli/local_runtime/binaries.py::manifest_verified — False.
- plugins/platforms/a2a/protocol.py::load_conversation — non-dict lines are
  dropped, keeping the declared list[dict] return.
- batch_runner.py::_load_dataset / _scan_completed_prompts_by_content /
  _combine_batch_files — line skipped and counted as filtered.
- trajectory_compressor.py::process_entry_async — scalar entry passed through
  unchanged.

Ported from the source hunks of PR #114241; its gateway/shutdown_flush.py
drain_transcript_spool hunk is left to open PR #84785, and its
recover_pending_to_db / cron / bot_live_delivery / bot_mode_dm hunks are
already on this branch or on main.

(cherry picked from commit d4b5456)
teknium1 added a commit that referenced this pull request Sep 18, 2026
…orts drain_transcript_spool

A scalar or list JSON file under pending_messages/ passed json.loads and then hit
`payload.get(...)`, raising AttributeError out of the drain and leaving every
healthy cap-dropped message unreplayed. Such a file cannot be attributed to any
session, so it is skipped exactly like unparseable JSON (file preserved), and the
remaining spool entries replay in order.

Closes the last open drain_transcript_spool atom of #114240 in this PR instead of
deferring it to #84785.
teknium1 pushed a commit that referenced this pull request Sep 18, 2026
… crashes the remaining file scans

Same class as the Bot Chat drain wedge already on this branch: every JSON-file
scan guarded "did it parse?" and then assumed the value was a dict. A file
holding `42`, `"oops"` or `[1,2,3]` (corruption, truncated write, foreign tool)
passed the guard and raised AttributeError/TypeError at the first `.get()`,
usually before a single healthy sibling was processed. Each site now treats a
non-object payload like a corrupt file under that subsystem's existing policy:

- tools/bot_relay.py::_expire_if_stale / claim_pending_envelopes — the
  envelope is skipped by the sweep and not claimed (same as unparseable).
- tools/browser_lightpanda.py::reap_orphaned_lightpanda — record unlinked,
  scan continues.
- tools/write_approval.py::list_pending / get_pending — record skipped with
  the existing "unreadable pending record" warning / None.
- tui_gateway/methods_session.py::_legacy_spawn_tree_entry / spawn_tree.load —
  scalar snapshot reads as empty / returns the existing 5000 error instead of
  violating the SpawnTreeLoadResult contract.
- hermes_cli/local_runtime/binaries.py::manifest_verified — False.
- plugins/platforms/a2a/protocol.py::load_conversation — non-dict lines are
  dropped, keeping the declared list[dict] return.
- batch_runner.py::_load_dataset / _scan_completed_prompts_by_content /
  _combine_batch_files — line skipped and counted as filtered.
- trajectory_compressor.py::process_entry_async — scalar entry passed through
  unchanged.

Ported from the source hunks of PR #114241; its gateway/shutdown_flush.py
drain_transcript_spool hunk is left to open PR #84785, and its
recover_pending_to_db / cron / bot_live_delivery / bot_mode_dm hunks are
already on this branch or on main.

(cherry picked from commit d4b5456)
teknium1 added a commit that referenced this pull request Sep 18, 2026
…orts drain_transcript_spool

A scalar or list JSON file under pending_messages/ passed json.loads and then hit
`payload.get(...)`, raising AttributeError out of the drain and leaving every
healthy cap-dropped message unreplayed. Such a file cannot be attributed to any
session, so it is skipped exactly like unparseable JSON (file preserved), and the
remaining spool entries replay in order.

Closes the last open drain_transcript_spool atom of #114240 in this PR instead of
deferring it to #84785.
beardthelion added a commit to beardthelion/hermes-agent that referenced this pull request Sep 18, 2026
…drains

A JSON file that parses but is not an object (a scalar, string, or list
from corruption or a foreign writer) slipped past every scan's "bad JSON"
guard and crashed the sweep at the first subscript:

- cron/bot_chat_pending: one such receipt wedged the deferred Bot Chat
  drain on EVERY tick — sorted() raised TypeError on record["sequence"]
  before any sibling was delivered, and defer() could not allocate a
  sequence either; violates the file's own "one bad file must not wedge
  the dir" rule.
- Exact-id reads in bot_chat_delivery, scheduler_delivery,
  bot_live_delivery, and bot_mode_dm crashed with TypeError instead of
  the established "different payload" ValueError; they now fail closed
  and never overwrite a malformed receipt.
- The same shape wedged shutdown-flush recovery (no per-file guard at
  all — even unparseable JSON aborted the pass), the transcript spool
  drain, bot_relay outbox claim + stale sweep, the lightpanda reaper,
  write-approval listing, spawn_tree.list/load (a scalar snapshot would
  violate the declared RPC result contract), manifest_verified, a2a
  load_conversation, batch_runner dataset/resume/combine scans, and
  trajectory_compressor pass-through.

Each site now rejects non-dict payloads under its own existing contract:
warn-and-preserve evidence for receipt dirs, quarantine on claim for the
relay outbox, delete under the existing cleanup policy for reaper state,
fail closed for exact-id reads. Malformed lines in JSONL scans are
skipped with honest filtered_entries bookkeeping.

Regression tests cover every site, including an end-to-end
drain_in_background run that delivers the healthy sibling with a
non-dict receipt in the dir.

Related: NousResearch#87661 covers the recovery-processing boundary of one of these
sites (recover_pending_to_db) with a broader except; NousResearch#84785 carries an
equivalent check inside its spool-ordering rewrite. The other sites are
uncovered.
beardthelion added a commit to beardthelion/hermes-agent that referenced this pull request Sep 18, 2026
…drains

A JSON file that parses but is not an object (a scalar, string, or list
from corruption or a foreign writer) slipped past every scan's "bad JSON"
guard and crashed the sweep at the first subscript:

- cron/bot_chat_pending: one such receipt wedged the deferred Bot Chat
  drain on EVERY tick — sorted() raised TypeError on record["sequence"]
  before any sibling was delivered, and defer() could not allocate a
  sequence either; violates the file's own "one bad file must not wedge
  the dir" rule.
- Exact-id reads in bot_chat_delivery, scheduler_delivery,
  bot_live_delivery, and bot_mode_dm crashed with TypeError instead of
  the established "different payload" ValueError; they now fail closed
  and never overwrite a malformed receipt.
- The same shape wedged shutdown-flush recovery (no per-file guard at
  all — even unparseable JSON aborted the pass), the transcript spool
  drain, bot_relay outbox claim + stale sweep, the lightpanda reaper,
  write-approval listing, spawn_tree.list/load (a scalar snapshot would
  violate the declared RPC result contract), manifest_verified, a2a
  load_conversation, batch_runner dataset/resume/combine scans, and
  trajectory_compressor pass-through.

Each site now rejects non-dict payloads under its own existing contract:
warn-and-preserve evidence for receipt dirs, quarantine on claim for the
relay outbox, delete under the existing cleanup policy for reaper state,
fail closed for exact-id reads. Malformed lines in JSONL scans are
skipped with honest filtered_entries bookkeeping.

Regression tests cover every site, including an end-to-end
drain_in_background run that delivers the healthy sibling with a
non-dict receipt in the dir.

Related: NousResearch#87661 covers the recovery-processing boundary of one of these
sites (recover_pending_to_db) with a broader except; NousResearch#84785 carries an
equivalent check inside its spool-ordering rewrite. The other sites are
uncovered.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/gateway Gateway runner, session dispatch, delivery needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants