fix(cu): harden runtime session termination - #911
Conversation
522b679 to
896b47e
Compare
|
@Astro-Han #911 is rebased onto the main that includes #895/#896. The Runtime typecheck and focused Computer Use/session-state suite pass locally (61 tests), and upstream typecheck/test/e2e are green. This is an independent follow-up to the actionable #892 lifecycle findings. |
Astro-Han
left a comment
There was a problem hiding this comment.
Requesting changes on 896b47e6 for one P1. CI is green; the P2 and P3 below are non-blocking.
P1
clearSession()now creates auser_stoppedsession state even when the session has never used Computer Use (packages/runtime/src/computer-use-tools.ts:1526). That record has noturnId. On the next turn,sessionState()binds the existing tombstone to the new turn at lines 592-594 instead of creating a fresh state. Desktop callsclearSession()for every complete, abort, and error event, so a normal turn with no Computer Use can make the next turn's firstobservereturnuser_stoppedbefore preflight. The rest of that turn stays blocked too. A minimal reproduction isclearSession("session-1")followed byobservewithturnId="next-turn"; the result ismaka_computer failed: user_stoppedand the backend receives zero calls. Keep the queued-call fence scoped to the turn being cleared without allowing an unbound tombstone to attach to the next turn, and add this no-CU-turn to CU-turn regression case.
P2
- The new observation lease is checked only before
list_apps,screenshot,cursor_position, andwait(packages/runtime/src/computer-use-tools.ts:1056-1067). Unlikeobserve, these four paths do not validate the lease after the host call. IfclearSession()runs while the host call is pending, all four still return success, including the screenshot payload. Add a post-call lease check before returning each result.
P3
blocked_urlis not fully terminal as described. CallinguserStopped()afterblockedUrlDetected()changes the state touser_stoppedand increments the generation (packages/runtime/src/cua-session-state.ts:116-118). Operations remain blocked, but the original blocked-URL reason is lost when Desktop clears the finished turn. Preserve the first terminal cause or narrow the stated contract.
|
@Astro-Han Addressed all findings on current main (including merged #905). P1: clearSession now creates a tombstone only for an existing Computer Use state or a currently queued invocation, and binds a queued fence to that exact turn; a normal no-CU turn no longer blocks the next turn observe. P2: list_apps, screenshot, cursor_position, and wait now revalidate their observation lease after the host call and discard late results/screenshots after clearSession. P3: terminal states are first-cause-wins, so blocked_url is not overwritten by user_stopped. Added regression tests for no-CU-turn -> next-turn observe, all four in-flight host reads, and blocked_url followed by stop. Current-main focused Computer Use/session-state suite: 67/67; Runtime typecheck and diff check pass. |
896b47e to
a097f4e
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Approved on a097f4ed after rereview. The previous P1 is fixed: a no-Computer-Use turn no longer leaves a tombstone for the next turn, while an invocation already queued in the stopped turn is still fenced. The four late read results and the first-terminal-cause issue are fixed as well. GitHub CI is green; the current-main integrated workspace build and stale check passed; Runtime tests passed 1501 with 7 skipped and no failures.
P2
applyTypedOutcomeState()runs before the post-dispatch lease check on both the semantic and generic paths (packages/runtime/src/computer-use-tools.ts:1370-1374,:1483-1498). When the backend returnsoutcome_unknown, the state first transitions toreobserve_required; validating the old lease then returnsreobserve_requiredand discards the original result. This is reproducible for bothleft_clickandclick_element. The state remains safely fenced, but the model loses the important signal that the action may already have happened and can choose the wrong retry behavior. Preserve the typed backend outcome while still requiring a fresh observation.
|
@Astro-Han Latest review-fix CI is fully green (typecheck/test/e2e). The P1/P2/P3 reproductions are now covered on current main: no-CU turn cleanup does not poison the next observe; all four in-flight host-read results are discarded after clearSession; blocked_url remains the first terminal cause. Ready for re-review. |
Summary
Follow-up for the actionable Runtime lifecycle findings from #892.
user_stoppedtombstone even when cleanup races the first invocationlist_apps,screenshot,cursor_position, andwaitwith the session observation leaseblocked_urlanduser_stoppedabsorb later lifecycle eventsRoot cause
Runtime only leased observation and mutation paths,
clearSession()mutated an existing state record instead of creating one, and terminal events used the same unrestricted transition helper as recoverable states. That allowed a queued first call or a read-only host action to continue after stop, and allowed later events to overwrite terminal status.See
docs/computer-use-runtime-hardening.md.Verification
npm --workspace @maka/runtime run typecheckgit diff --checkThis PR is independent of the conflicting cumulative #905 stack.