Repository navigation
fix(gateway): replay transcript spool in drop order with full fidelity, hold back failed sessions (fixes #123658) - #132680
Sahilvishnaliya wants to merge 1 commit into
Conversation
…y, hold back failed sessions (fixes NousResearch#123658)
teknium1
left a comment
There was a problem hiding this comment.
PR Review — #132680 (head fa3891a5)
Verdict: comment — functionally correct re-implementation of a fix already in the queue; duplicate of #122718 (salvage of #84785, @briandevans). Merge decision is Teknium's.
Context. After a gateway restart recover_pending_to_db replays cap-dropped transcript spool files; on main it walks them in pending-<uuid4>.json name order (random), appends only role/content/timestamp, and continues past a failed append so a session's later rows land ahead of the failed one (#123658, confirmed live on main in the issue triage).
Verification (this diff applied to origin/main @ 8b66a510, run against the invariant tests of #122718 tests/gateway/test_shutdown_flush_recovery.py + the existing test_shutdown_flush.py): 14 passed, 1 failed. Drop-order replay, out-of-range ts, "replayed row is the row the live writer writes" (tool_calls / tool_call_id / tool_name / reasoning / platform id / sidecar / display fields) and per-session hold-back all pass. The one failure is only the hold-back summary log: #122718 emits Held back N spooled transcript file(s) … sess-1 (2) so an operator can see files were left on disk; here the skipped files are silent after the first warning.
Differences from #122718 (none in its favour):
- No tests. The body's 11 passed are the pre-existing suite, which never asserted order or fidelity.
_sort_keyparses every spool file once to sort and the loop parses it again; #122718 reads once._ASSISTANT_ONLY_KEYSis a hand copy ofgateway.session_transcript._ASSISTANT_ONLY_KEYS(the drift I already flagged on #122718; a shared kwargs builder insession_transcript.pyis the shape that stays correct — neither PR has it yet).- #84785 has carried this fix since Aug 12 and #122718 since Sep 25 with @briandevans's authorship preserved; this PR arrived Oct 4 with the same three mechanisms and no credit.
Cluster: #117310 / #123585 route cap-dropped recovery to the owning profile store on the same branch of _recover_one_payload; whichever lands second takes a small rebase. I'd close this as duplicate once #122718 lands.
unsupportedpastels
left a comment
There was a problem hiding this comment.
Thanks @Sahilvishnaliya for working on this. #123658 is now fixed on main by #133932. As @teknium1 noted in review, this PR takes the same approach as the earlier #84785 and #122718 work: (ts, seq, name) order, full-field replay, and holding back a failed session's later files. #133932 carried that work with its authors' commits.
Closing as superseded by #133932.
Fixes #123658. recover_pending_to_db sorted pending-uuid names (random) and replayed role/content/timestamp only, continuing past failures. Now sorts by (ts,seq,name) like the live drain, replays full fields (tool calls/reasoning/platform ids/sidecar/display), and holds back a failed session's later files while other sessions continue. Tests: test_shutdown_flush (11 passed).