Repository navigation
fix(approval): stop a pending approval dying on the session idle TTL (#13478) - #13494
Merged
Merged
Conversation
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No hardcoded values detected that have SSOT config equivalents! |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thinking Path
#13216 reported that approvals "expire after 30 minutes and cannot be honoured late". Reading the code showed that is not what expires: the 30-minute poll budget only bounds the turn, it never clears the pending approval, and
_approve_command_internalexecutes on approve with no coroutine waiting. #13486 fixed the message that made a live approval read as a failed command.The real expiry is one layer down and an hour later.
set_pending_approvalre-persists the session immediately (service.py:213), so the clock starts the moment the prompt is raised and nothing refreshes it while the human thinks. An hour on, the key is gone,get_sessionmisses, and_approve_command_internaltakesif not session: return {"error": "Session not found"}. The approval is unrecoverable — and a person taking more than an hour to answer a prompt is the ordinary case this feature exists for.The second half is subtler and I nearly missed it.
_restore_pending_approvalrebuilds the state from chat history and is explicitly documented as surviving restarts — it works. But it runs only insidecreate_session, which mints a new session_id, while the approve endpoint is addressed by the old one (what the persisted approval message and the GUI button carry). A working restore that the only path needing it cannot reach.What Changed
services/agent_terminal/session_manager.pyThe TTL now depends on what the session is doing.
TTL_1_HOURstill collects genuinely idle sessions — that GC is right and unchanged. A session holding a pending approval getsAPPROVAL_PENDING_SESSION_TTL(default 7 days,AUTOBOT_APPROVAL_PENDING_SESSION_TTL_SECONDS). It is deliberately long: the thing being waited on is a person, and #13481 established that an approval does not expire on a timer. This bounds the stored session, not the approval's validity.A pointer from session to conversation.
agent_terminal:approval_conversation:{session_id}→conversation_id, written only while an approval is pending and with the same long TTL.get_sessiontakes a session_id and_restore_pending_approvalneeds a conversation_id; without this the chat-history restore cannot be reached from the approve path.get_sessionrebuilds a vanished session that still owes an answer. On a miss it follows the pointer, runs the existing restore, and returns a session under the original id — so the approve request that arrives with the old id resolves. If the approval turns out to have been answered already, nothing is resurrected. An unknown session id still returnsNonecheaply and quietly, which is the common case.The rebuilt session takes
AgentRole.CHAT_AGENT, the lowest privilege. The original role is not recorded in the restored approval, and a resurrected session exists only to let a human resolve a decision that was already gated — it must never be the thing that widens what the command may do.Verification
Both halves invert-tested, each failing on exactly its own regression:
test_an_idle_session_still_expires_in_an_hourpasses in both directions on purpose — it guards the GC this change could plausibly have broken while fixing the other case.Known limit
Both the session record and the pointer live in Redis. A full flush still loses a pending approval — but that is data loss, a different failure from a TTL quietly expiring something that was never meant to be time-limited. Worth naming rather than implying this makes approvals durable against anything.
Scope
Closes #13478. The remaining #13216 children are #13479 (folding a late execution result back into the chat turn that gave up on it) and #13480 (exactly-once execution when an approve races the poll).
Closes #13478
Refs #13216
Model Used
Opus 5 (1M context)