[fix] Approval parks hold for 10 minutes, not 30 - #5823
Merged
Conversation
The approval park is the window a sandbox stays warm while a run waits for a human to approve or deny. It was widened from 5 to 30 minutes in #5687 so an answer arriving from a phone resumes warm instead of falling back to cold replay, which is slow. Ten minutes covers that case just as well: the failure mode it was fixing was answers landing a few minutes late, not half an hour late. Shortening it also bounds a second, unrelated exposure. A rotated provider credential stays usable on a warm sandbox until that sandbox is replaced, because `holdForMechanism` waits a fixed 10 seconds for propagation and declares delivery done without checking whether the new value arrived (`credential-delivery-port.ts`). Real propagation was measured at 12 to 20 seconds, so the push lands after the runner has already moved on and the sandbox keeps the old key. Idle parks are already short: 60 seconds generic, 120 seconds on Daytona. The approval park was the only window long enough for that exposure to matter, so it is the one worth keeping tight. This does not fix the rotation gap, which needs the delivery to verify the new value took effect rather than trusting a timer; it only stops the widened window from extending it. Tests updated to pin 600000 rather than 1800000, and the two stale "30 minutes" comments corrected. All 77 session-pool tests pass.
dosubot
Bot
added
the
size:S
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The previous commit's comment claimed the shorter window also bounds a credential-rotation exposure. That reasoning does not hold and is removed. `runCredentialDelivery` (credential-delivery-port.ts:784-855) updates the secret, verifies the slots the provider reports installing match the ones requested, and only then waits the hold. The credential really does land, and it lands while the sandbox keeps running, so how long the sandbox lives does not extend anything. The real gap there is narrower: the hold is a fixed 10s against 12-20s measured propagation, so a turn starting inside that gap can still use the old key while the runner has already reported success. That is tracked separately and is unrelated to this window. The 10-minute value stands on its own merit: it covers a phone-latency approval without holding a pool slot for half an hour on a gate nobody is coming back to.
Contributor
Railway Preview Environment
Updated at 2026-08-09T11:08:33.980Z |
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
1 participant
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.
Context
The approval park is how long a sandbox stays warm while a run sits waiting for a human to approve or deny a tool call. Answer while it is parked and the run resumes warm and continues immediately. Answer after it expires and the sandbox is gone, so the system rebuilds it and replays the conversation, which is slow.
PR #5687 widened that window from 5 minutes to 30, because answers arriving from a phone were usually landing after the 5 minutes were up and taking the slow path every time.
Mahmoud asked for 10 instead of 30.
Changes
The default approval park goes from 30 minutes to 10.
Ten still covers the case #5687 was fixing. The problem it addressed was answers landing a few minutes late, not half an hour late. The value remains overridable through
AGENTA_RUNNER_SESSION_APPROVAL_TTL_MS, so a deployment that wants longer can still set it.Correction to an earlier version of this description
An earlier draft of this PR justified the change partly as bounding a credential-rotation exposure. That reasoning was wrong and has been removed.
The claim was that a rotated provider key stays usable until the sandbox is replaced, so a longer park window meant a longer exposure. Reading
runCredentialDelivery(services/runner/src/providers/credential-delivery-port.ts:784-855) shows otherwise: the delivery updates the secret, verifies that the slots the provider reports installing match the ones requested (lines 826-835), and only then waits the hold. The credential genuinely lands, and it lands while the sandbox keeps running, so sandbox lifetime does not extend the exposure at all.There is a real but much smaller gap there: the hold is a fixed 10 seconds while measured propagation is 12 to 20, so a turn beginning inside that gap can still use the old key while the runner has already reported success. That is a roughly ten-second correctness gap, it is tracked separately, and it is unrelated to this change.
So this PR should be judged purely on its own question: is 10 minutes long enough for someone to answer an approval from their phone? If 30 was the right answer to that, this should be closed rather than merged.
Tests / notes
session-pool.test.tsupdated to pin 600000 rather than 1800000, and the test name changed from "30m approval" to "10m approval".session-identity.tsandsession-coordinator.ts:650.