Repository navigation
runtime: stop idle browser surfaces in the periodic sweep - #604
Merged
Merged
Conversation
A managed browser surface is the container a connector run drives. The lease manager already stops a surface that has been idle past PDPP_NEKO_IDLE_TTL_MS (default 10 minutes), but only the controller method cleanupIdleBrowserSurfaces called that check. The periodic sweep (every PDPP_NEKO_LEASE_SWEEP_INTERVAL_MS, default 30 seconds) reconciled surfaces, expired waits and retried capacity reclaim, and never ran idle cleanup. An idle, non-retained surface therefore kept running after its run ended until a later run needed its capacity slot. The sweep now also runs cleanupIdleBrowserSurfaces. That path persists each stopped surface and promotes queued leases onto the freed capacity, the same as the controller method does today. The order inside the sweep is deliberate: 1. Reconcile with the allocator first, so surfaces whose container is already gone are dropped and not sent a stop. 2. Expire waits next, so a waiter past its deadline is deferred before idle cleanup can promote it. 3. Run idle cleanup before capacity reclaim, so reclaim, which stops surfaces under pressure, runs only for waiters that idle cleanup did not serve. 4. Retry reclaim for anything still queued. An error from idle cleanup is logged and does not skip the remaining steps. The existing in-flight guard is unchanged. Retained surfaces and surfaces with an active lease are excluded by the lease manager, so the sweep does not stop them. Host-mode surfaces (PDPP_BROWSER_SURFACE_MODE=host) use the same backend tag, so they are also stopped once idle past the TTL. This is a behavior change for host mode. Worst-case idle time is the TTL plus one sweep interval, about 10 minutes 30 seconds with defaults. Defaults are unchanged. controller.cleanupIdleBrowserSurfaces stays public. The sweep calls the same underlying function, and an existing test uses the method as its seam. Tests: four sweep tests in controller-browser-surface-leases.test.ts cover a stop past the TTL that persists, no stop for a retained surface, no stop for an active lease, and a queued lease promoted after an idle_ttl stop. Before the change, the first and last failed; the other two passed because they assert that nothing happens. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
Running idle cleanup from the periodic sweep exposed three defects. remote-surface 1.5.2 picked its idle surfaces once and then stopped them one at a time. While one stop was pending, an acquire could lease another surface on that list, and cleanup then stopped it under the running lease. A stop failure also discarded the stops that had already succeeded, so they were never persisted and queued leases stayed queued. 1.5.3 claims every candidate before the first stop, re-checks each one, and returns failures in `failed` instead of throwing. The coordinator logs each failed stop. In host mode, the host answers 404 to a lease DELETE after it has lost the lease, for example after a restart. That threw, so the surface stayed ready and every sweep tick sent the DELETE again. A 404 on a DELETE now counts as released. The host contract lists the idle TTL as a release trigger. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
…surface The sweep's idle cleanup stops a surface asynchronously. Until the stop settles, the lease manager must not hand that surface to a new run. The existing tests only covered a lease that was active before the sweep, and their stops resolved at once, so nothing exercised the window. The new test holds stopSurface open, starts a run for the same profile during the stop, and asserts that the run gets surface_2 and that surface_1 stays unleased until its stop finishes. It fails when the in-flight-stop exclusion in @opendatalabs/remote-surface 1.5.3 is removed: the run is then given surface_1. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
sweepIdleBrowserSurfaces repeated the try/catch that runBestEffort in the same module already provides. Behavior is unchanged: a failed idle cleanup is logged and the sweep continues. Assisted-by: AI Signed-off-by: Tim Nunamaker <tnunamak@gmail.com>
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.
After a run ends, its managed browser surface is never stopped for being idle.
cleanupIdleSurfaces(idle TTLPDPP_NEKO_IDLE_TTL_MS, default 10 minutes) is reachable only throughcontroller.cleanupIdleBrowserSurfaces, and only a test calls that. The periodic sweep (server/index.ts:9976→sweepBrowserSurfaceLeases) reconciles with the allocator, expires waits and retries capacity reclaim, but it never runs idle cleanup. So an idle surface keeps its container until another run needs the slot.The sweep now runs
cleanupIdleBrowserSurfacesafter reconcile and wait expiry, and before the reclaim retry. Reconcile goes first so idle cleanup acts on surfaces that still exist. Wait expiry goes before it so an expired waiter is not promoted by the freed slot. Reclaim then runs only for waiters that idle cleanup did not serve. Stops are persisted and queued leases promoted by the same code the controller method already uses. An error in this step is logged and the rest of the tick still runs.controller-browser-surface-leases.test.ts)idle_ttlstopreadyhost-browser-surface-allocator.test.ts)On base the fourth test fails because the reclaim step stops the surface as
capacity_pressure, notidle_ttl. The other 45 earlier tests in the file still pass, so existing lease behavior is unchanged.Calling idle cleanup every 30 seconds exposed two defects in
@opendatalabs/remote-surface1.5.2, fixed in 1.5.3 (vana-com/remote-surface#6). It picked its idle surfaces once and stopped them one at a time, so an acquire during a pending stop could lease a listed surface that cleanup then stopped under the running lease. A stop failure also threw away the stops that had already succeeded. 1.5.3 claims all candidates before the first stop and returns failures infailed; the coordinator logs each one. The pending-stop test holds a stop open and starts a run for the same profile. It fails when the 1.5.3 claim is removed from lease selection, because the run is then given the stopping surface.The new step is a no-op in static n.eko, without a surface manager, and with direct CDP, because none of them has a dynamic allocator. Host mode (
PDPP_BROWSER_SURFACE_MODE=host) now releases an idle host lease past the TTL throughDELETE /browser-surface/leases/:id. The host keeps the profile. A 404 on that DELETE now counts as released; before, a host that had lost the lease made every sweep tick resend it. The host contract lists the idle TTL as a release trigger.Not verified: the Postgres variant of the lease tests, and a live container run. The
mutate changed production files (reference-implementation)job, which is advisory, produced no evidence. Its statement widening turns the two changed hunks intorun-coordinator.tslines 290 to 2000 and allocator lines 111 to 400, both over its 60-line bound, so it sampled nothing (scripts/mutation-falsification/run-intent.ts:361). Worst-case idle time is the TTL plus one 30-second sweep interval.Assisted-by: AI