Skip to content

feat(limrun): map boot and shutdown to instance leases - #4275

Merged
thymikee merged 12 commits into
callstack:mainfrom
muvaf:codex/limrun-instance-lifecycle
Nov 6, 2026
Merged

thymikee merged 12 commits into
callstack:mainfrom
muvaf:codex/limrun-instance-lifecycle

Conversation

@muvaf

@muvaf muvaf commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Limrun owns instance lifetime, so powering off its guest would leave the instance allocated. This 45-file change maps boot to create/reuse and shutdown to lease release. Created instances are deleted; attachments detach without deleting the externally owned instance. Explicit shutdown releases retained leases. Failed cleanup preserves its session, lease, and diagnostic for retry.

Unallocated provider requests fail in shared inventory before local selection, including SDK open and session-less boot. Headless boot preserves allocation guidance. Stored-lease clearing follows declared connection policy. Scope includes session-aware recovery hints and preserving local finalizer/IME ordering from #3318. CLI/MCP descriptions explain deletion and data loss.

Examples: agent-device boot, agent-device shutdown, agent-device close --shutdown. Merged #4268 covers Android text/performance.

Validation

Rebased onto df8391a63. At 378f995d1e8ed21854911293986ad718a1c27ec8, pnpm check:affected --run --head HEAD passed with exactCommit: true. Receipt, focused tests, base comparisons, independent review, and live results. Headless/inventory regressions fail on b32b21848 and pass here.

Fresh live runs cover owned Android/iOS lifecycle, native snapshots, and unallocated SDK refusals while a local simulator stays booted. Earlier evidence covers attachments, retained leases, and local shutdown. All resources were cleaned up. GitHub-owned coverage/native/device checks remain CI obligations.


Autofix with [code]smith
Open in [code]smith · Need help on this PR? Tag @codesmith-bot with what you need.

View guided diff

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 39 files

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

Comment thread website/docs/docs/limrun.md Resolved
@thymikee

thymikee commented Nov 6, 2026

Copy link
Copy Markdown
Member

Thanks for this. At e28ba51 this needs one code fix and live device evidence before merge. CI shows 13 checks with none failing. The GitHub-owned device lanes remain open obligations per the PR body, and they exercise the changed local close --shutdown route.

shutdown is now deferred for Limrun connections (connection-lease-policy.ts#L20). With no instance, the CLI sends it with the connection's platform and no leaseId. From my reading, the Limrun inventory provider returns null without a lease, the source reports declined, and the composed gateway then falls back to local inventory. A booted local simulator or emulator that matches --platform would then be shut down. On base, the CLI allocated first, so the command was refused. Impact: agent-device shutdown on an empty Limrun connection can power off an unrelated local device. The rule should be: a request that carries a provider connection never resolves a local device. Please make an unallocated provider-connection shutdown a typed no-op at its owner, either in the CLI or by refusing the local fallback in resolveCommandDevice when leaseProvider is set without a leaseId. A daemon-level test with a booted local device should assert that no local shutdownTarget is bound. Please correct me if a daemon guard already rejects lease-less provider requests; I found none.

This is a device-facing change: local Apple and Android close --shutdown moved from finalizeApplicationClose to the daemon-invoked shutdownTarget (session-close.ts#L140), and Limrun create, attach and delete is new. The PR links live evidence only through an external fork README that I could not read. It also covers no local Android emulator and no unallocated-Limrun shutdown with a booted local device. So the Android kill ordering after IME restore (#3318) is unproven at this head. At e28ba51 or later, please post these runs inline or in a reachable artifact: (1) Limrun Android and iOS, created: boot, then shutdown, with output showing provider.limrunInstanceId and the instance gone afterwards; (2) attached: boot, then shutdown, with the instance still running; (3) open, then close --shutdown on a retainOnClose lease; (4) local iOS Simulator and local Android emulator close --shutdown and standalone shutdown, each with data.shutdown.success true and the device shut down or offline; (5) shutdown on a fresh Limrun connection while a local simulator is booted, showing the local device stays booted.

Not blocking, take or leave: (a) session-close.ts#L142 now keeps the session and retries the whole teardown when any local close --shutdown step throws, where base retired it, so keep that retry branch only for a shutdown that ends a provider lease; (b) the releaseLease callback is written twice, in session-shutdown.ts#L83 and session-close.ts:145-160, so releaseTargetLease could return { shutdown, provider } for both sites; (c) ended-lease.ts#L30 drops the proxy-only guard, so every non-host-allocated provider (cloud WebDriver, Revyl, plugin providers) now forgets its stored leaseId, so name that population in the PR body or limit the change to providers whose shutdown ends the lease; (d) session-shutdown.ts#L63 scans every session for a holder, and DEVICE_IN_USE echoes that session back, so include the hint only when the holder is in the caller's tenant or workspace scope.

Could the close path be smaller? For example, pass shutdownRequested into closeLeaseDecision so close --shutdown on a provider lease, retained or not, uses the existing provider-release branch, and leave local shutdown in finalizeApplicationClose unchanged. For standalone shutdown, when the device's shutdownTarget fact declares a typed lease-release effect, handleShutdownCommand could call releaseLease from resources/lease-release.ts for the request's lease scope. That would drop the Apple, Android and contract edits, shutdownPrepared, the DeviceShutdownRuntimeOperations signature change and the duplicated callback. The first step would be an effect: 'lease-release' | 'power-off' field on the shutdownTarget fact, so the daemon reads a declaration instead of inferring from providerMode.

There are no conflicts.

@muvaf

muvaf commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the code and evidence requests at b32b21848. The live runs below are at 139db478b; the only later diff is a test binding's TypeScript typing.

The default CLI connection was already refused by tenant admission with INVALID_ARGS: tenant isolation requires lease id. I verified that with a booted local device. I also added a device-resolution guard for non-tenant clients: a provider request without a lease now returns DEVICE_NOT_FOUND with reason provider_instance_unallocated before consulting local inventory. The daemon regressions use a working local shutdown binding: on e28ba51b8 they actually complete local shutdown and fail the expected-refusal assertion; on the new head neither local selection nor shutdown is called.

I took the local-close simplification: Apple and Android lifecycle implementations now match main, including IME restoration before Android shutdown. The daemon invokes the extra shutdown operation only for provider runtimes. The callback remains explicit so the runtime supplies the target lease ID and the daemon checks it against admitted request ownership before release. I did not add a generic shutdown-effect field. The callback now shares the release result instead of duplicating its payload.

The other follow-ups are included: only callers in the holder's inventory scope receive its session address; cleanup failures retain the underlying error; stored-lease clearing stays limited to proxy/Limrun; provider-session recording is shared; pending Android cleanup no longer advertises runtime-hint support; shared command descriptions expose deletion semantics to CLI and MCP; and boot is documented as optional.

Live results on November 6, 2026:

Case Android iOS
Created: boot → shutdown shutdown.success: true, provider ID android_euna_01m6rtb6a4fgva8wpdsxgm01wk; confirmed terminated shutdown.success: true, provider ID ios_euna_01m6rtbj8jesvscdaxkqaw8wye; confirmed terminated
Retained owned lease: open → close with shutdown shutdown.success: true; instance terminated, lease already released Same
Attached: boot → shutdown shutdown.success: true; external instance android_euna_01m6rtda1mebzshaxsq984f65x stayed ready shutdown.success: true; external instance ios_euna_01m6rte5fcebztwh2wzk09174c stayed ready
Retained attachment: open → close with shutdown Detached; external instance stayed ready Same
Local: open → close with shutdown data.shutdown.success: true; emulator offline data.shutdown.success: true; simulator Shutdown
Local: boot → standalone shutdown data.shutdown.success: true; emulator offline data.shutdown.success: true; simulator Shutdown
Fresh provider connection while local device is booted CLI tenant refusal and SDK typed refusal; local emulator stayed booted Both refusals; local simulator stayed booted

Snapshots used the native Android helper matching 0.21.37-dev, and the iOS XCTest runner. Attached test instances were deleted by the harness only after verifying that agent-device left them ready. All sessions, daemons, instances, the temporary iOS simulator, and the temporary Android AVD were cleaned up.

Raw outputs, receipt, and regression logs. The exact-commit affected gate passes; nine regression assertions fail on e28ba51b8 and pass on the new head. Independent review found no outstanding issues. CI coverage and device lanes will run on the push.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 25 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

@thymikee

thymikee commented Nov 6, 2026

Copy link
Copy Markdown
Member

Thanks for the update. The code at b32b218 fixes the problems from the earlier review of e28ba51, and I found no new blocking problem. There are no conflicts. Smoke Tests is still running, so CI is not yet a result; if it fails on a close --shutdown step, treat it as related to this PR, and any other failure is likely unrelated. Merge needs Smoke Tests to pass.

Not blocking, take or leave: the rule that a request with a provider connection never resolves a local device is enforced only in resolveCommandDevice (session-device-resolution.ts#L63), so open and boot without a session can still fall back to local inventory in composed-inventory-gateways.ts:65 for an SDK client that sends leaseProvider without leaseId; and ended-lease.ts#L34 matches the 'limrun' literal in shared CLI code, although ConnectionProviderCapabilities already owns per-provider connection policy.

Could the 'limrun' check become a declared trait on LIMRUN_CONNECTION, read through connectionProviderCapabilities like leaseKind? Could the lease-less refusal also move into the provider-first inventory gateway, so no resolver can fall back to local for a provider request and the guard in resolveCommandDevice can go?

The two open threads on the headless boot hint remap and the leaked mock implementation still apply and need a fix before merge. Your live table covers the Limrun and local shutdown routes; the CLI-side refusal for a lease-less connection rests on that run.

@muvaf
muvaf force-pushed the codex/limrun-instance-lifecycle branch from b32b218 to 378f995 Compare November 6, 2026 17:31
@muvaf

muvaf commented Nov 6, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the three open bot findings and both suggestions from your review through 378f995d1.

The missing-lease refusal now belongs to the shared provider-first inventory gateway. Both request resolvers carry lease metadata into it, including SDK open and session-less boot; the resolver-local error guard is gone. Allocated sessions preserve their selected device. forgetsEndedConnectionLease is declared in connection policy, so shared CLI code no longer names Limrun. Headless boot preserves the typed refusal and allocation hint, and the provider test queues its two facts responses without leaking its implementation.

Rebased onto df8391a63, preserving #4263’s claim-retention check before provider release and #4268’s missing-interactor errors. Exact-commit validation and independent review passed. The new inventory regressions fail on b32b21848 and pass here; a proxy-session regression also protects the held target when multiple local devices are available. Fresh packaged-CLI runs cover create/reuse/delete/recreate, native snapshots, active-session refusal, and close --shutdown on Android and iOS. Packaged SDK calls for open, boot, headless boot, and shutdown without a lease all refuse with allocation guidance while a local simulator stays booted. All test resources were cleaned up.

Receipt, base comparisons, live results, and review. CI is running on the new head; the prior head's checks, including Smoke Tests, passed.

@thymikee

thymikee commented Nov 6, 2026

Copy link
Copy Markdown
Member

This PR is ready at 378f995. Both P2 findings from the earlier review are fixed, and nothing from that review remains open. All checks pass, including Smoke Tests and Coverage, there are no conflicts, and nothing else must happen before merge.

The earlier live rows for local open, close and boot still hold, because the later commits do not change local requests.

Not blocking, and you can take or leave these: session-close.ts:150 throws the kept-session cleanup error inside the try block, so the catch at line 177 replaces its hint with the generic retry hint. The hint of a kept-session cleanup report should reach the caller unchanged, ideally with one assertion on it. Also, the lease-less provider rule is stated twice as the same inline predicate, in composed-inventory-gateways.ts:52 and session-device-resolution.ts:64. One named predicate on LeaseScope beside leaseProviderRequiringRuntime could serve both.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Nov 6, 2026
@thymikee
thymikee merged commit 5c36c95 into callstack:main Nov 6, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants