Repository navigation
fix(limrun): give one lease one owner for its instance and its teardown - #4141
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Size Report
Startup median (7 runs, lower is better):
|
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
- P1 —
packages/provider-limrun/src/runtime.ts:289: release can miss an allocation. Ifrelease()runs while thesession-allocation.tsimport is pending,leaseshas no owner yet, so release usesreleaseRecoveredSession()and can finish before the allocation continues intobeginAllocation()and creates a billed instance. Publish a per-lease owner synchronously before the first await (then run validation inside that owned allocation), so release and shutdown can join it. - P1 —
packages/provider-limrun/src/runtime.ts:563-568: release during capture startup leaks the capture. IfstartLimrunHarCapture()is pending when release takes the owner,settleNetworkCapture()sees no capture, deletes the instance, and closes the owner. When startup resolves, it assigns the live capture to the now-detached owner; no later route closes that tunnel or writes its HAR. Record the in-flight capture start on the owner and make disposal await and close its eventual result, or close/refuse it if the owner was taken. - P1 —
packages/provider-limrun/src/runtime.ts:389-390: failed session initialization can hide an undeleted instance. If Limrun create returns an instance butcreateLimrunAndroidSession()fails,allocateLimrunAndroidSession()catches the setup error and swallows any rejection fromdelete()(session-allocation.ts:151-153). The assignment here never receives a session, socreateForLease()reportsrented: undefined; disposal reportsnoneand drops the owner even if the delete was unconfirmed. Preserve the created instance ID and cleanup outcome through the allocation helper, and report an unconfirmed delete aslimrun_instance_delete_unconfirmed.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Verdict: needs-changes.
Reviewed exactly merge-base f7879cf2d266ee1dd476d0342b2819bdeceb647b → head 3b60492d1bee0d55b337dd03d803513689a970cc against the coordinator's ownership requirements.
-
P1 — Publish the owner before the validation module await.
packages/provider-limrun/src/runtime.ts:289(publication is at line 351).allocate(lease)suspends atawait import('./session-allocation.ts')before an owner exists. An immediaterelease(lease)therefore takesreleaseRecoveredSession; its label listing can observe no instances, then allocation resumes and creates an instance that release never takes. The allocation succeeds after its lease was released. A cached dynamic import still yields. Move publication ahead of this await and carry validation failure through that published owner's creation outcome, or make prepublication validation synchronous. The new release regression waits for the remote create before releasing, so it misses this window. -
P1 — Own capture startup, including its pending result.
packages/provider-limrun/src/runtime.ts:563-574. Capture startup saves the session, awaits the module andstartLimrunHarCapture, and only then assignsowner.capture. Release/shutdown in that interval sees no capture, disconnects and closes the owner; the pending destination tunnel can subsequently open and be stored on an owner no longer in the map. The finalrequireLiveSessionrefuses allocation but never closes that tunnel. This is especially observable for attached instances, which remain running; SDKdisconnect()closes the instance command connection, not this separate destination tunnel. Two allocations can also both start capture; analready_activeresult from one can overwrite the other's successful capture withunavailable. Publish a per-owner pending capture state before starting it, join it on duplicate allocation, and make teardown close its eventual result with a bounded wait/late cleanup continuation. -
P1 — Carry the rented instance through session-construction failures.
packages/provider-limrun/src/runtime.ts:387-397, throughpackages/provider-limrun/src/session-allocation.ts:105-107,151-153. The retained allocation helpers still delete independently and swallow DELETE failure. For example, the Android POST returns instance X, but endpoint validation orcreateLimrunAndroidSessionfails; the helper's DELETE of X is refused. The outersessionassignment never completed, so the new create outcome saysrented: undefined; disposal returnscleanup: 'none'and removes the owner. Shutdown then succeeds without naming X, and a later allocation can create another instance. Make the owner carry the remote identity immediately after POST, before session setup; route all deletion through its disposal and preserve an uncertain outcome on failure. The iOS helper has the same gap. -
P1 — Disable SDK retries for ambiguous instance creation.
packages/provider-limrun/src/runtime.ts:389-390(client construction at lines 180-184; POSTs insession-allocation.ts:71,114). The pinned@limrun/api@0.64.1client defaults tomaxRetries = 2(src/client.ts:235) and retries transport failures/timeouts (:479), including these POSTs. If Limrun creates X but its response is lost, the SDK can POST again and return Y. The owner sees and deletes only Y, leaving X unowned. Calling the helper only once does not enforce the no-retry requirement. SetmaxRetries: 0for instance-create requests and report the ambiguous outcome without starting another create; add coverage below the SDK request boundary, since the current mock replaces it entirely. -
P1 — Bound provider work before waiting to delete the instance.
packages/provider-limrun/src/runtime.ts:474,513, throughpackages/provider-limrun/src/android.ts:159. A retained interactor can havestartAdbTunnel()pending when release takes a live owner. Cleanup awaits that promise before remote DELETE, but the SDK'sstartAdbTunnelawaitsexecFile('adb', ['connect', ...], callback)without a timeout (@limrun/api/src/instance-client.ts:1879-1887). A stuck adb process therefore blocks release and runtime shutdown forever, holds the owner inreleasing, and prevents reallocation while the instance remains billed. Instance-client connection setup during creation also has no handshake deadline, and disposal first awaitsowner.creation. REST's default five-minute request timeout is not an end-to-end lifecycle budget (the SDK clears it after fetch returns headers, before response parsing). Add explicit creation/teardown budgets that retain late resource cleanup, attempt remote deletion independently of stuck local teardown once the ID is known, and report uncertainty instead of leaving callers unresolved. The daemon's five-second plugin wrapper reports this provider as pending; it does not perform the missing deletion. The capture-status read itself already has a two-second budget, and destination-capture startup has the SDK's fifteen-second handshake budget. -
P1 — Retire ADB access before cleanup's first await and reject late tunnel completion.
packages/provider-limrun/src/android.ts:159,175,247-248. Start cleanup on a session with no tunnel: its firstawaityields. A retainedadbProvider.exec()now starts a tunnel whileadbTunnelRetiredis still unset. Cleanup resumes, sees no serial, clears the pending reference, sets retired and returns. The tunnel can then resolve and repopulateadbSerial/adbTunnelafter cleanup. Subsequent ADB calls return that cached serial before checking retired, so the session stays usable and the late tunnel has no cleanup owner. Mark closing/retired synchronously before yielding, check it before the serial fast path, and close/refuse any tunnel completing after retirement; keep cleanup's own access to an existing serial explicit. Add a race test around this window, beyond the current sequential cleanup-then-exec test.
Other checklist results: ready reuse checks live state after awaited capture startup; reallocation while an existing owner is releasing is refused; ordinary disposal clears its keep-alive interval; explicit DELETE failures retain the owner and throw. Plugin registry cleanUp catches a throwing shutdown() and still attempts plugin disposal. Multi-instance shutdown IDs survive in the hint through plugin normalization and daemon reporting, although only the first provider failure cause is kept. The file-local refusal helpers/type-only edges themselves add no separate blocking finding; the prepublication dynamic import is the material eager-closure problem above.
Validation: pnpm install --frozen-lockfile && pnpm build succeeded. Ran the focused tests once: pnpm exec vitest run --project unit-core packages/provider-limrun/src/runtime.test.ts packages/provider-limrun/src/android.test.ts src/__tests__/limrun-runtime.test.ts → 36 passed, 3 files. Did not run CI or modify reviewed source. I inspected the base implementation and new assertions; they target real base failures (duplicate creation, missed in-flight teardown, reopening, and raw-vs-typed delete failure), but did not independently rerun the author's base experiment under the one-run limit. None of those assertions exercises the six windows above.
Apex SentinelVerdict: Since the last review: 9 new · 0 still open · 5 no longer raised 9 finding(s): 4 minor, 5 nit. Verdict: approve. reached max rounds (1). 🟡 Minor (4)
🔵 Nit (5)
Reply under an inline comment with |
![apex-sentinel-staging[bot]](https://avatars.githubusercontent.com/in/5196457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Apex Sentinel: 8 new findings on 3b60492. The verdict and the full list are in the summary comment.
|
I reviewed 3b60492. The one-owner-per-lease design is sound, but release and shutdown can still hang, and a few routes need tests before this is ready. Coverage failed in coverage-crap: The open threads that still apply and must be fixed first: the disposal that never settles when a cleanup step throws (thread, same in this one), and the phase check before the await in Before merge: settle the disposal as Not blocking, and you can take or leave each: (1) Could the three-phase union and the separate disposal bookkeeping go away? Limrun could use Revyl's entry shape: I did not run vitest, check:affected, fallow or coverage-crap, so the claims that the tests fail on base and pass here rest on reading the code. I could not run the live Limrun Android check or its leak check without a key, and iOS was not exercised live. |
3b60492 to
e6c46e0
Compare
There was a problem hiding this comment.
All reported issues were addressed across 11 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
Apex Sentinel: 10 new findings on e6c46e0. The verdict and the full list are in the summary comment.
A lease's instance, keep-alive timer, network capture and app alias were recorded in four maps, none of which held anything until the instance create had answered. Two allocations of one lease therefore rented two instances, and a release or daemon close arriving mid-create found nothing to end and left an instance billed. One owner now carries the create and every resource beside it, is published in the same step that starts the create, and is dropped only once its one disposal confirms. A route that arrives late joins that disposal or is refused with what it came to, and an unconfirmed delete keeps the lease held so the next release retries it rather than reporting an instance released. The owner is published before its allocation can await anything, so a release that arrives at the create's first suspension point already owns the instance that create goes on renting, and a create refuses a profile flag this provider reads no field for as its first step rather than after it has billed an instance. An instance is named to its owner the moment Limrun answers for it, before any session is built on it, so a create that rented one and then failed still owes its end, and a delete Limrun refused is retried by the next route rather than forgotten. Both instance POSTs ask the SDK for a single attempt: the SDK's own retries would answer an ambiguous failure by renting a second instance. Every wait a teardown takes is now bounded, so nothing an SDK started without a deadline can hold a release open. A create, a destination tunnel still being opened, or a local teardown that outlives its budget is reported instead of waited on; the work is never cancelled and its owner still disposes what it makes, and the lease is given back with a warning naming what was left, because the instance itself is gone. Capture startup publishes its pending result, so two allocations that ask for traffic logs open one destination tunnel and a teardown finds the tunnel its open had not answered for. The shared host ADB endpoint is retired before teardown yields, so a command that arrives while cleanup is starting raises no tunnel, and one already being raised is ended where it answers instead of being published to nobody.
e6c46e0 to
fcaf681
Compare
|
Round 2 — all six P1s from @review #5728000115 fixed, each behind a regression test that failed first. Head: 1 — Publish the owner before the validation module awaitFixed as suggested: publication now precedes every await. Test: 2 — Own capture startup, including its pending result
Tests: 3 — Never lose an instance rented before a failure
Test: 4 — One attempt at a create whose answer was lost
Tests live in the new 5 — Bound every wait a teardown takesNew — the provider-webdriver release-record pattern from #4130 (patterns only; the code stays owner-local). Test: 6 —
|
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Round-2 review of fcaf681234b83c55556bd1a49236d75ca3dcf765.
Needs changes. The six round-1 issues are addressed: validation for a new owner runs inside its published create; in-flight capture and Android tunnel startup are published before awaits and late endpoints are retired; rented instance IDs are stored immediately after create answers; SDK create retries are disabled; teardown runs provider delete alongside bounded local cleanup; timed-out work remains observed and the budget timer is unref'd.
Blocking — packages/provider-limrun/src/runtime.ts:319-328. When a duplicate allocation sees an existing owner, it awaits profile validation before retaining that owner. If the current owner is live, release can take/delete it and remove it from leases during that await; the allocation then rereads no owner and calls beginAllocation, billing another instance for a lease whose release already completed. Snapshot the owner before validation and do not create a replacement when that observed owner was taken/closed.
Non-blocking — packages/provider-limrun/src/runtime.ts:340-347. A joiner can resume the create, call goLive, and start keep-alive before a release that took the lease is noticed. The release path disconnects that session only when disposeOwner resolves the ready create; if create-answer wait expires first, it has no session and the owner remains uncertain while the keep-alive continues. Check the releasing state before goLive and ensure any late-ready session is locally retired.
Non-blocking — packages/provider-limrun/src/runtime.ts:544-552. If createForLease built a session and then a later step fails (for example resolvePreinstalledAppId), failed outcome drops the session. Disposal sees no session and deletes the rented ID without disconnecting the instance client or retiring an Android tunnel raised during app lookup. Preserve the built session on failure or owner before later fallible work.
Non-blocking — packages/provider-limrun/src/runtime.ts:577-578. The no-instance disposal path awaits capture end without CAPTURE_RECORD_BUDGET_MS, so a stuck failures()/HAR write can prevent release and shutdown from settling despite the main path's bounded capture wait.
Bot threads (all open on this head):
- Cubic: detached disposal rejection — stale/outdated, fixed by rejection handler; capture-start owner race — stale/outdated, fixed by published starting state; optional failure typing — real but non-blocking type invariant; concurrent HAR writes — real but non-blocking corruption risk; Android retirement race — stale/outdated, fixed; prune after close — real but non-blocking retention-bound violation; test gate cleanup — real but non-blocking test hang on assertion failure; inaccurate tunnel-closed hint — real but non-blocking copy; attached cleanup wording — real but non-blocking documentation.
- Apex: disposal rejection — stale/outdated, fixed; validation await race — real blocking finding above; live capture counting — stale/outdated due moved lifecycle; first failure only on shutdown — real non-blocking diagnostic loss; permanent shutdown retry behavior — real non-blocking lifecycle concern; static import comment — stale/outdated; concurrent shutdown deletes — non-blocking capacity risk; HAR close ordering — stale/outdated; unknown rented ID count — real non-blocking shutdown reporting issue; late capture record eviction — non-blocking; detached late-capture end rejection — real non-blocking unhandled rejection/tunnel leak; attached release reports instance ended — real semantic defect requiring clarification (attached was never owned/deleted); shutdown/artifact observation after shutdown — non-blocking lifecycle concern; goLive before release check — real non-blocking finding above; failed create session resource leak — real non-blocking finding above; unbounded no-instance capture end — real non-blocking finding above; malformed app-alias comment — non-blocking nit; capture outcome comment mismatch — non-blocking documentation.
- Cubic follow-up on
lease-release.ts: fake lease ID in unknown-create hint — real non-blocking operator guidance defect;awaitLimrunWithinBudgetrejected work escapes its modeled outcome — real non-blocking; late capture end callback rejection — duplicate of Apex late-capture finding; stale Android test comment — non-blocking; unresolved capture-start test gate on assertion failure — real non-blocking test hang.
Focused provider-limrun tests: 3 files, 23 tests passed. No CI waited on.
There was a problem hiding this comment.
Apex Sentinel: 5 new findings on fcaf681. The verdict and the full list are in the summary comment.
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Verdict: needs-changes.
Reviewed 3b60492d1bee0d55b337dd03d803513689a970cc..fcaf681234b83c55556bd1a49236d75ca3dcf765 and the affected allocation, teardown, capture, SDK, and plugin-close paths. These are five actionable findings; the bot dispositions below also identify inherited/nonblocking issues.
-
P1 — The remote DELETE is still unbounded.
packages/provider-limrun/src/runtime.ts:553,658-660. After taking a live owner, disposal awaitsendInstance -> deleteInstance -> iosInstances/androidInstances.deletewithoutawaitLimrunWithinBudget. A DELETE that returns headers but never finishes its response body can therefore leave release, shutdown, and every joining allocation unresolved indefinitely: the pinned SDK clears its fetch timer before parsing that body (@limrun/api/src/client.ts:624). Neither the 45-second local budget nor the 15-second capture budget is reached. Give the DELETE an end-to-end deadline; reportcleanup: 'uncertain', retain the owner for retry, and observe its eventual settlement. An uncertain delete must continue to refuse release rather than hand the lease back. -
P1 — A create answering after the disposal budget has no cleanup continuation.
packages/provider-limrun/src/runtime.ts:543-546,578-590. Start allocate, take it with release/shutdown while its POST is pending, and let 120 seconds pass before the POST names an instance. Disposal settlesuncertainwith no ID and stops running. If the POST subsequently returns X and session construction succeeds,onRentrecords X, butgoLiveskips the releasing owner and the allocation merely reads the already-settled disposal and throws. Nothing calls DELETE for X unless somebody explicitly invokes another teardown. This is particularly material during daemon shutdown, when no next in-process retry is promised. Install an observed late-create continuation that disposes its eventual instance/session. In the related known-ID/slow-handshake case, deletion can close the owner before the eventual session exists, and that late session also needs local cleanup. -
P2 — Failure after constructing a session drops its local cleanup handle.
packages/provider-limrun/src/runtime.ts:439-449,544,624. An uploaded Android app rents X, constructs its client, opens ADB forresolvePreinstalledAppId, then fails because the installed application ID cannot be resolved unambiguously. The catch now returns only{ status: 'failed', error }. Disposal finds X throughrentedInstanceIdand deletes it, but derives no session from the failed outcome, soreleaseOwnerResourcesreturns without disconnecting the instance client, retiring ADB, disconnecting its host endpoint, or closing its tunnel. The round-1 implementation carried its constructed session in the failed outcome. Retain the session on the owner as soon as it exists, or carry it in that failed outcome, and dispose both its remote identity and local resources. -
P2 — A valid joiner inherits another caller's profile-validation refusal.
packages/provider-limrun/src/runtime.ts:319,331-338,425. Call allocate(L,{ flags: { providerOsVersion: '15' } }) and immediately allocate(L) without that unsupported flag. The first publishes an owner before its validator runs; the second validates its own valid context and joins that owner's creation. The shared creation then fails with the first caller'sINVALID_ARGS, and both callers throw that same error despite the second having passed validation. In round 1 the invalid caller was rejected before creating shared work. Keep validation caller-local and synchronous before publishing/joining shared creation, or explicitly distinguish a refused caller from a failed provider create so another caller can allocate using its own validated context. -
P2 — The cleanup-only ADB serial is available to every retained caller.
packages/provider-limrun/src/android.ts:175-179,272-279. On a session with an existing endpoint, cleanup sets retired, recordsadbTeardownSerial, and awaits reverse-list/removal. A retained interactor issuingadbProvider.exec(['shell', ...])during that wait reaches the sameensurePersistentAndroidAdbSerialand returns the teardown serial before checking retirement. Thus ordinary commands still dispatch into a taken session; an attached instance makes this observable without a racing remote DELETE terminating it. Give reverse cleanup a separate private executor/explicit serial argument, and make the public executor reject retirement before any cached-serial route.
Round-1 P1 disposition, with concrete traces:
- 1, publication before validation await: resolved for a new owner.
allocatehas no await from its absent-owner lookup tobeginAllocation -> leases.set(323-328,397). The first create import/validation is inside that published owner (416-425); an immediate release synchronously finds and takes it (755-760). This avoids the provider-webdriver trap of simply moving an await ahead of publication. Finding 4 is the separate shared-validation follow-on. - 2, pending capture startup: resolved for the reported race.
captures.startpublishes{ phase: 'starting', opening }beforeopenCapture(capture-lifecycle.ts:65-83); duplicate callers join that promise (51-54).endremoves it from the owner, waits within its budget, and owns a late result (100-112). A releasing owner cannot start a new tunnel becauseliveSessionOfreturns none. The final taken-state check follows capture startup (runtime.ts:353-356). - 3, rented identity lost on construction failure: resolved for remote identity. Both POST helpers invoke
onRent(instanceId)immediately after the response and before endpoint checks/session construction (session-allocation.ts:97-101,144-148); helper-local deletes are gone. Disposal reads that ID even on failure; unconfirmed deletes retain the owner. Finding 3 is the new loss of an already-constructed local session. - 4, ambiguous-create SDK retries: resolved. Both Android and iOS POSTs pass
LIMRUN_SINGLE_ATTEMPT = { maxRetries: 0 }. The new tests use the real SDK and a server that drops the connection after reading the POST, and verify exactly one POST per platform. - 5, unbounded disposal: partially resolved. Creation wait, pending capture open, local teardown, and capture-record join have budgets; local/capture work starts alongside deletion. Findings 1 and 2 prevent accepting the overall claim that every teardown wait is bounded and every late resource is cleaned.
- 6, concurrent tunnel startup after retirement: resolved for reopening/late publication. Android cleanup sets
adbTunnelRetiredbefore its first await (161-169), public startup checks retirement beforeadbSerial(279-280), and a tunnel already opening publishes its endpoint then refuses a retired session so its waiting cleanup can close it (291-302). Finding 5 is the separate overly broad teardown exemption.
Other requested follow-on checks:
awaitLimrunWithinBudgetcallstimer.unref?.()and clears the timer in finally. Its race observes late work rejection; the extra catch also observes the original promise. Explicit DELETE failures become uncertain and refuse release; a DELETE that never settles currently bypasses that helper altogether (finding 1).- The helper does propagate an early rejection; its return union models expiry, not rejection. The pinned SDK's
getTunnelStatusis async (instance-client.ts:1913), and its rejected promise is caught inreadLastDialFailure. This does not substantiate the bots' proposed synchronous-status-throw crash scenario. The detached capture continuation would deserve rejection/finally hardening if a concrete rejecting capture-close implementation is introduced. - Shutdown calls each owner's capture
endbefore disconnect/delete;endremovesowner.capturesynchronously. Settled artifact reads therefore use records, not a disconnected live session. The plugin registry attemptsruntime.shutdownand thendisposeindependently (packages/plugins/src/registry.ts:331-333). I found no normal shutdown artifact re-query after plugin dispose; the runner-finalization step only stops runners. Budget-expired capture completion can finish later by design, so that continuation must remain self-contained. - All three new lazy modules are behind the relevant publication boundary:
lease-release.tsloads aftertake;lifecycle-budget.tsloads afterowner.state = releasing;capture-lifecycle.tsloads before capture start, butstartchecks current liveness and publishes pending capture state synchronously before opening. None adds an await inside new-owner publication or take. The existing-owner branch still awaits caller validation before looking the owner up again; it must not be used as proof that validation is isolated from shared creation (finding 4).
Open bot review threads from GraphQL reviewThreads(first:100) at this head: 38 unresolved, no next page. This inventory includes outdated open threads, each evaluated against current code. 13 threads identify real issues (1 blocking, 12 nonblocking, including duplicate diagnostics); 25 are not current defects. No thread replies or resolutions were posted.
| Thread | Verdict on this head |
|---|---|
| r4435549981 | Not a current defect: take now handles disposal rejection and settles its shared promise. |
| r4435549987 | Not a current defect: pending capture ownership and joined startup replace the reported late assignment. |
| r4435549993 | Not a defect: a stricter discriminated outcome is a design preference; no invalid producer/observable failure is identified. |
| r4435549999 | Real defect, nonblocking/inherited: overlapping HAR reads/releases can still truncate/rewrite one path concurrently; atomic or serialized writes would address it. |
| r4435550004 | Not the reported defect now: retirement is synchronous and precedes the ordinary serial fast path; finding 5 concerns the new teardown exemption. |
| r4435550011 | Real defect, nonblocking/inherited: concurrent ends can prune while all records' owners remain held, then close owners without another prune, retaining more than 32 records. |
| r4435550021 | Real defect, nonblocking test robustness: an assertion before releasing the create gate sends finally into the 120-second shutdown wait instead of prompt failure cleanup. |
| r4435550032 | Real defect, nonblocking diagnostic: the revised “Reopen the lease” hint still fails while uncertain deletion holds its releasing owner; it should direct release retry. |
| r4435550038 | Not a behavioral defect: the same comment immediately defines attached as never owned for deletion; wording can be tightened. |
| r4435571153 | Not a current defect: the new rejection branch reports uncertain and resolves disposal instead of stranding it. |
| r4435571159 | Not a current defect: the taken-state check now follows capture startup and pending startup has an owner. |
| r4435571165 | Not a regression: merge-base f7879cf2 counted live captures in networkCaptures.size too (runtime.ts:436-438). |
| r4435571170 | Real limitation, nonblocking/inherited: all IDs are retained but only the first failure cause is exposed; round 1 already noted this. |
| r4435571179 | Not a defect: take retries a settled uncertain owner; shutDown blocks allocation, not release or repeated shutdown. |
| r4435571188 | Not a defect: importing device.ts does not evaluate its importer network-har.ts; the old comment is also gone. |
| r4435571193 | Not a demonstrated defect: concurrent teardown still joins once per owner; a possible rate limit does not prove excess instance deletion. |
| r4435571196 | Not a defect: awaiting writeCaptureRecord precedes tunnel close, exactly as the comment says. |
| r4436141712 | Real defect, nonblocking diagnostic: an unknown-instance timeout substitutes a lease ID into “delete instance”; only known IDs should get that instruction. |
| r4436141717 | Not a demonstrated production defect: rejection propagates, but current create outcomes fulfill, local failure is mapped, and SDK status rejection is caught; the proposed capture failure producer is not established. |
| r4436141721 | Real defect, nonblocking/inherited: completed traffic arriving during the final asynchronous HAR write can be omitted before close; snapshot-before-close ordering is unchanged from round 1. |
| r4436141724 | Not a demonstrated defect with this SDK: its status method is async and its rejection is caught; no reachable rejecting endCapture operation is supplied. |
| r4436141729 | Not a functional defect: the test comment is stale, while the assertion correctly requires synchronous retirement. |
| r4436141734 | Real defect, nonblocking test robustness: clearing state.tunnelGate does not resolve the promise already awaited by a tunnel, delaying failure cleanup through the budgets. |
| r4436155369 | Real defect, nonblocking diagnostic: a timed-out unnamed create produces “could not delete 0 instances” and an empty delete hint despite uncertain work. |
| r4436155384 | Not a separate defect: under 32 held captures the late record displaces an older released one; at 32 held captures its eviction follows the existing combined cap. |
| r4436155389 | Not a demonstrated defect: its asserted synchronous getTunnelStatus throw contradicts the pinned SDK's async implementation; same disposition as r4436141724. |
| r4436155397 | Not a defect: releasing an attached lease releases this runtime's handles without deleting an instance it does not own; returning its identity does not assert deletion. |
| r4436155401 | Not the claimed defect: registry cleanup catches shutdown rejection, and capture end clears the live capture before disconnect, leaving record-only artifact reads. |
| r4436155404 | Not a defect: goLive refuses releasing state; a successful goLive has a ready session/ID, so the claimed unresolved-create/no-ID timer combination is impossible. |
| r4436155407 | Real defect, blocking: constructed-session loss on later app-resolution failure skips client/ADB cleanup (finding 3). |
| r4436155412 | Not reachable as claimed: capture starts only after a ready/live session, so an unrented or unanswered create cannot already hold a starting/capturing capture. |
| r4436155414 | Not a defect: valid comment formatting; cosmetic style only. |
| r4436155419 | Real defect, nonblocking documentation: the field comment promises HAR carriage on refusal, but refusal builders omit it; listing the settled record remains available. |
| r4436201433 | Real defect, nonblocking: a prior capture's delayed end/write can overwrite a newer capture's lease-keyed HAR/settled record after reallocation; use capture generations or distinct paths. |
| r4436201435 | Not a defect: uncertain owners are retained, so the proposed reallocation cannot replace them; confirmed owners are removed and are not retried, and a new attached session constructs a new client. |
| r4436201437 | Not a defect: start returns early only for phase starting; unavailable falls through and opens a new capture on the live owner. |
| r4436201440 | Not a demonstrated defect with the pinned SDK: failures() uses an async status method whose rejection is caught; same unproven failure producer as r4436141717. |
| r4436201444 | Real defect, nonblocking diagnostic: duplicate of r4436155369; unnamed uncertain work is incorrectly counted as zero instances with an empty delete hint. |
Validation: pnpm install --frozen-lockfile && pnpm build succeeded. Ran focused tests once: pnpm exec vitest run --project unit-core packages/provider-limrun/src/runtime.test.ts packages/provider-limrun/src/android.test.ts packages/provider-limrun/src/session-allocation.test.ts src/__tests__/limrun-runtime.test.ts → 47 passed, 4 files. No additional suite, base experiment, device run, or CI wait. Source and Git status remained clean.
Scope/size: this round changes 8 production files (+782/-342; net +440) and 3 test files (+533/-13; net +520), with no docs changes. Head's Bundle Size job succeeded; size report gives +6.4 kB unpacked and +2.2 kB download against its base. The extracted lifecycle modules group related capture/release/budget work, but discarding the session while adding a separate rented-ID field creates the incomplete resource owner in finding 3; retaining the actual resource handle is the smaller correction. This is an independent review, not a CI or merge-readiness receipt.
|
Thanks for the update. At fcaf681 the earlier hang is fixed: take() now settles when a disposal rejects, and the capture start/publish race and the retired ADB tunnel check are fixed too. Two problems remain, and the Coverage job fails. I read the code and did not run the tests or a live Limrun session. disposeOwner starts the capture end and the local teardown (https://github.com/callstack/agent-device/blob/fcaf681/packages/provider-limrun/src/runtime.ts#L551-L552) and attaches no handler until after the instance DELETE finishes (L553). If disconnect, the ADB tunnel close, the capture close or createForLease drops the session when resolvePreinstalledAppId fails (https://github.com/callstack/agent-device/blob/fcaf681/packages/provider-limrun/src/runtime.ts#L443-L449). The failed outcome no longer carries it, so disposeOwner gets no session and only the instance DELETE runs. Not blocking, take or leave: the take() rejection fallback (runtime.ts:521-528) has no regression test, because the shared stub The Coverage job fails only in "Check coverage-based CRAP of touched functions". It points at resolveAppReference (runtime.ts:795, CRAP 30.0, cyclomatic 5, 0% coverage). This PR changed that body and the delta added no test that reaches it, so the failure is related. The other 18 checks pass. Before merge, the two fixes above and a test for resolveAppReference must land. Is the new surface needed? The delta adds three modules, three budget constants, the 'starting' capture phase with its publish guard, deletedInstanceId, adbTeardownSerial and the disposeUnrented/endInstance split, for about 927 production lines. Both problems above are the same class: a resource exists before the owner holds it, or a promise exists before anyone observes it. Would one ledger on the owner be smaller? Each resource registers where it is created (instance id at rent, session when built, keep-alive, capture start) with its own dispose. Disposal then deletes the instance and runs one bounded Promise.allSettled over the ledger, turning each rejection into a warning that never changes Open inline threads that still apply: #4141 (comment) (session dropped, the second problem above), #4141 (comment) (rejected work reports uncertain), #4141 (comment) and #4141 (comment) (late continuation), #4141 (comment) (failures() rejection), all part of the first problem. Also #4141 (comment) (flat disposal outcome type) and 15 lower-priority threads also still apply. These do not apply at fcaf681, so you can resolve them: #4141 (comment) (disposal now settles), #4141 (comment) (start publish guard), #4141 (comment) (retire before first await), #4141 (comment) (disposal settles), #4141 (comment) (phase read after start), #4141 (comment) (same counting as base), #4141 (comment) (unconfirmed delete keeps owner, as Revyl), #4141 (comment) (import is type-only), #4141 (comment) (concurrent deletes match base), #4141 (comment) (record then close matches comment), #4141 (comment) (eviction order), #4141 (comment) (attached release matches base), #4141 (comment) (shutdown throw already accepted), #4141 (comment) (goLive only while allocating), #4141 (comment) (end() bounds its own wait), #4141 (comment) (retried take hits the same retired session), #4141 (comment) (allocate retries unavailable). |
Five ways a lease could outlive its caller's answer are closed here, each behind a test that fails without its fix. The remote delete now has a deadline of its own: the SDK clears a request's timer before it reads the response body, so a DELETE whose body never arrives would otherwise hold the release, the daemon close and every joining allocation open forever. It reports `uncertain`, keeps the lease held, and stays observed, so its late confirmation is recorded. A create that answers after its disposal stopped waiting is no longer unowned: the disposal watches it and ends the instance it rents and the session built on it, rather than leaving both running with no route left to end them. A session is recorded on its owner in the step that answers for it, so an allocation that fails after building one still has its instance connection and host ADB endpoint released by whoever ends the lease. A create that refuses the caller who started it for a profile flag this provider reads no field for answers for that caller alone. The lease rented nothing, so a joiner whose own flags are valid makes that create its own instead of inheriting another caller's refusal. The serial an ADB teardown is ending belongs to that teardown alone: it dispatches through an executor of its own, and retirement is checked before any cached serial, so a retained interactor cannot take commands on a device being disconnected. The disposal itself moved to `lease-release.ts`, beside the refusals that report what it could not stop, and asks only for one instance delete and one capture end.
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
Apex Sentinel: 13 new findings on c91a2b9. The verdict and the full list are in the summary comment.
|
The late reporting of A disposal can leave a rejected promise with no handler, and one rejection closes the daemon. In Not blocking, and you can take or leave these: The delta grows production code from about 927 to 1204 lines, adding Open inline threads that still apply: #4141 (comment) (rejected release escapes), #4141 (comment) (second DELETE), #4141 (comment) (joiner flag validation), #4141 (comment) (session-wide ADB executor), #4141 (comment) (budget wait rejects) and #4141 (comment) (capture continuation unhandled). Ten lower-priority threads also still apply (reporting, doc and comment nits, and repeats of the points above). These do not apply and can be resolved: #4141 (comment) (the 120 s budget predates the delta and shutdown is bounded by the daemon), #4141 (comment) (base also refused before any capture step), #4141 (comment) (an unrented owner has no capture to end), #4141 (comment) (joiner wait unchanged since fcaf681), #4141 (comment) (the earlier finding was retracted), #4141 (comment) ( I read the code at c91a2b9 and did not run the tests or a live Limrun session. The live run in the PR body was made at 3b60492, and no live run covers the new ADB cleanup dispatch or the disposal paths. A live run after the fix should reach a Limrun release with the delete gated and a failing local teardown, and should print the warning |
Six ways a lease could be torn down wrongly, or a caller handed a session it was never entitled to, each behind a test that fails without its fix. A joiner with profile flags this provider reads no field for is now refused by the create it joins, not handed that create's session: validation runs before the join for every lease this runtime already holds, while a new lease keeps its validation as the first step of its own create so publication stays free of any await. A late end joins the delete already in flight. The owner records the request it has with Limrun, so the continuation of a create that answered after its disposal gave up waits on that request instead of sending a second one for the same instance. The route an ADB teardown ends its serial through is scoped to that teardown. The reverse manager dispatches through it, and a caller command that reaches the same route while the teardown is mid-request carries no marker: it is refused by the session's closed public route, so nothing gets mapped behind the teardown's one listing. A disposal reports every step it stopped waiting for. A rejected local teardown is reported rather than thrown; an expired capture record contributes its own failure; and a daemon close names why each lease it could not end is still held, not only the first. Two disposal routes awaited a capture end with no deadline at all, and a repeat allocation joined a stranger's destination-tunnel handshake unbounded; all three now answer within a budget. Both continuations that outlive every budget carry rejection handlers, so work this runtime deliberately stopped waiting for cannot reach the process as an unhandled rejection.
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
Apex Sentinel: 11 new findings on e5ebf76. The verdict and the full list are in the summary comment.
|
Thanks for the update. At e5ebf76 the two late continuations now have rejection branches, the joiner is validated before it joins, the expired capture record is reported, and a repeat delete joins the pending one. Two problems remain from the earlier review (#4141 (comment)). I read the code only. I did not run the tests or a live Limrun session. The unhandled-rejection windows are still open, and the new code adds one more with the same shape. The Android reverse cleanup and the release, disposal and capture-record paths changed after the only live Limrun run in the PR body (3b60492). After the fix above, please run a live Limrun Android session at the final head with the worktree CLI: allocate, add one Could the design be smaller? In Not blocking, and fine to take or leave: the new comment at runtime.ts:388 says the caller's own flags were answered before it got there, but the first caller's flags are checked only inside Checks: Smoke Tests is still running and has not failed. Those lanes drive local emulators and simulators and do not reach the Limrun provider, and the diff touches only On the open inline threads: the P2 thread on the budget wait (#4141 (comment)) still applies and is covered by the first finding. Eleven lower-priority threads still apply; they are small comment, doc, test-assertion and error-detail points. The following do not apply at e5ebf76 and can be resolved. Fixed at the head: r4436526037, r4436526045, r4436526052, r4436526062, r4436141724, r4435571170, r4436526081 and r4436155389 (all under #4141 (comment)_). Not applicable: r4436526072 (shutdown is now awaited in both cases), r4436547315 (the 120 s create budget predates this PR and the daemon bounds owner shutdown), r4436547321 (the expired record is reported and the capture still closes its tunnel), r4436547324 and r4436547332 (the continuations have rejection branches), r4436547327 (base also refused flags before any capture), r4436547337 (the author retracted it), r4436547339 (local teardown is memoized and repeat deletes join), r4436547342 (all failures are joined into the cause), r4436547345 (the capture lifecycle is reached only by dynamic import and the field is type-only on purpose), r4436547347 (the Android narrowing is correct), r4436547352 (the tests drive the real SDK and assert one POST), r4436794202 (holding a lease whose instance is still billed is the intended rule), r4436794206 (DELETE is idempotent), r4436794223 (the keyed lock runs queued callers without the marker), r4436794235 (a refused delete is an answer and retrying it is the documented rule), r4436794243 ( |
The instance delete joins the create under the same request contract: one attempt at the SDK, because a lost answer is ambiguous and this runtime retries deletes at its own budgets, not the SDK's. A 404 is Limrun answering that the instance is already gone — exactly what the delete was after — so it confirms the end instead of holding the lease for a retry that can only fail again. The recovery route that ends a lease from its labels keeps the same contract. Every refusal that says an instance is not confirmed deleted now carries the delete's own error as its cause, where a cause renders, beside the text in details; a create that both failed and could not be undone reports both reasons, not only the delete's. The disposal outcome records the delete's cause so each route can raise it without re-deriving anything. A replacement creation is observed the moment it is published, so the owner's promise that a creation cannot reach the process unobserved holds by construction on every path that installs one. Docs and shape: the teardown-scope marker and the reverse route keep their own comments, the capture lifecycle's paragraph sits on its class, shutdown's snapshot names its binding, and the close's report lists every lease it could not end with each of their reasons.
There was a problem hiding this comment.
All reported issues were addressed across 11 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
Apex Sentinel: 5 new findings on 4ae7368. The verdict and the full list are in the summary comment.
|
Thanks for the update. At 4ae7368 most of the earlier items are fixed, but one release-path defect and one missing live run remain. CI is green (19 checks, 0 failing), but the device and replay lanes do not reach the Limrun provider, so they do not cover either item below. There are no conflicts. I read the code only. I did not run the tests, a live Limrun session, or a reproduction of the rejection. The defect from the earlier review is unchanged. The Android reverse cleanup, the release and disposal paths, the capture record path, and the instance delete route with its 404 handling all changed after the only live Limrun run (3b60492), and no newer run is posted. Please run a live Limrun Android session at the final head with the worktree CLI: allocate, add one Not blocking, and you can take or leave it: add one Could the rejected variant also let several per-site guards go? With it, the Of the open inline threads, these still apply: labelled-recovery delete options untested (P2), hint ignores uncertain cleanup (P2), loop bypasses delete helper (P2), rejecting capture end, rejection half of unobserved capture and no rejected variant (the last three are covered by the first finding). Seven lower-priority threads still apply as non-blocking. One thread does not apply and you can resolve it: spurious capture warning, since the warning is accurate while the late tunnel open is still outstanding. The remaining threads are fixed at this head or benign and you can resolve them. The next step is to add the rejected variant, wrap the capture and local promises where they start, and add the |
…t starts it A bounded wait now answers for both of a work item's outcomes: a capture record or a delete that refuses is an outcome in its own right, not a failure for this route to re-raise, and it never reaches the process as an unhandled rejection — inside the budget or after it. The two promises a disposal starts beside the instance delete are observed the moment they are started, so a session disconnect or a tunnel close that throws during a gated delete has its observer already beside it. The daemon treats one loose rejection as fatal, and a disposal has other people's sessions. A release whose delete Limrun confirmed still answers for the instance it ended: a refused capture record reports as a teardown warning naming the record, not as the release's failure. The disposal's outcome keeps its shape from the delete alone — no capture or local-teardown result can change what was confirmed gone. Tests: two `createLimrunRuntime` cases with a gated delete and a throwing disconnect, and with a throwing capture close, each with a process-level unhandledRejection spy. The alias case now drives the platform module's binding, through `resolveOpenTarget`, and asserts the name stops answering once the lease is released, so the exported runtime type keeps the surface it had before this PR.
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
Apex Sentinel: 9 new findings on 385fd27. The verdict and the full list are in the summary comment.
…budgets, read path The label-based recovery route ends instances through the same delete helper every other route uses, so a 404 confirms there exactly as it does everywhere else, and a delete the helper could not confirm refuses with the typed unconfirmed-delete error — naming the instance left in question beside every id the labels carried, and which of them are already gone. The daemon's expiry sweep is that refusal's retry. A disposal's join for a capture record now outlasts the destination tunnel's own open budget it contains, so a handshake that answers at the end of its stated budget is no longer reported as a refused record. An artifacts read answers at once for a capture still opening its tunnel: the read is not the route that owns that handshake, and waiting it bought an immediate answer nothing to wait twenty seconds for. A take-refusal names what the disposal actually confirmed: an uncertain delete is said as unconfirmed, not as an instance that was ended. The release-lease hint keeps the same honesty. Tests: recovery route cases (confirm, 404, refusal with both ids), the budget ordering, the read path's immediate answer, and the take-refusal's two hints; typed delete mocks retire the cast assertions, and one pins the single-attempt options argument.
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Takeover update at Addressed the 20 open threads and added dispositions to 10 previously resolved threads that had no reply. The test rename fixes the R84 test-colocation failure from run 41347322478 without a guard exception. Release/capture rejection handling remains covered by gated DELETE + throwing disconnect/capture-close tests; the observer now yields a macrotask before checking. New tests fail on the old recovery deadline, omitted third ID, pending artifacts, late-owner cleanup, record pruning, and late-capture rejection. The late capture also now closes if reporting fails. The requested live run found an additional defect: SDK disconnect closed ADB before reverse cleanup. Validation: 69 focused tests passed (six files). Eager closure measured 30; a static profile-validation import adds two modules, so lazy caller validation is retained. Live verification at Before release, A transparent ADB observer forwarded every command to the real executable and read the reverse list immediately before the daemon's own disconnect. The production cleanup issued removal, and the subsequent list was empty (exit 0): [
{
"args": [
"-s",
"127.0.0.1:63817",
"reverse",
"--list"
]
},
{
"args": [
"-s",
"127.0.0.1:63817",
"reverse",
"--remove",
"tcp:18441"
]
},
{
"args": [
"disconnect",
"127.0.0.1:63817"
],
"afterReverseCleanup": "\n",
"checkExit": 0
}
]Release A (no warnings): {
"success": true,
"data": {
"connected": false,
"session": "pr4141-final-a",
"released": false,
"provider": {
"limrunInstanceId": "android_euna_01m6cfsztafbv9carm14ws602e"
}
}
}
Session B answered on the same daemon after A released: {
"success": true,
"data": {
"platform": "android",
"package": "com.android.settings",
"activity": "com.android.settings.Settings"
}
}
B was then disconnected. Daemon stop: {
"success": true,
"data": {
"stopped": true,
"mode": "graceful",
"cleanupConfidence": "known",
"claimsReleased": [],
"claimsOrphaned": [],
"claimsSuperseded": [],
"claimsUnattributable": [],
"providerReleases": {
"status": "completed",
"released": [],
"pending": []
},
"warnings": [],
"clean": false
}
}Leak check from inside packages/provider-limrun used @limrun/api Android and iOS list iterators (all pages): 512 records, all six instances created in the three takeover runs found terminated, zero still running. No other instance was modified. The three fresh state directories remain preserved; no live state directory was deleted. Review dispositions: r4436526037, r4436526045, r4436526052, r4436526062, r4436526081, r4436975777, r4436975784, r4436975786, r4436975790, r4436975793, r4436992317, r4436992321, r4436992322, r4436992324, r4436992329, r4437146080, r4437146085, r4437146086, r4437155045, r4437155068, r4437155088, r4437155098, r4437155104, r4437155108, r4437155111, r4437155120, r4437155132, r4437238682, r4437238686, r4437238692. Head |
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Follow-up push at
Validation: 69 tests passed in six files, the isolated deadline regression passed, full-repository format, lint, typecheck, and build passed. Eager closure remains 30 modules. Repeated the live run at this head with the worktree CLI and fresh state Before release: Actual cleanup commands, observed transparently before forwarding to real ADB: [
{
"args": [
"-s",
"127.0.0.1:58613",
"reverse",
"--list"
]
},
{
"args": [
"-s",
"127.0.0.1:58613",
"reverse",
"--remove",
"tcp:18441"
]
},
{
"args": [
"disconnect",
"127.0.0.1:58613"
],
"afterReverseCleanup": "\n",
"checkExit": 0
}
]The mapping was removed, then Release A (no warnings): {
"success": true,
"data": {
"connected": false,
"session": "pr4141-f051-a",
"released": false,
"provider": {
"limrunInstanceId": "android_euna_01m6cgpda9fbvacvt6bzb8z08d"
}
}
}The idempotent B answered on the same daemon after A's release: {
"success": true,
"data": {
"platform": "android",
"package": "com.android.settings",
"activity": "com.android.settings.Settings"
}
}A's serial is absent from B also ran with network capture; Leak check from inside packages/provider-limrun used @limrun/api Android/iOS iterators across 514 records: all eight takeover instances were found terminated, zero still running. All four temporary daemons stopped; state/evidence directories remain preserved. Head |
|
At f051db5 the code looks good, and the earlier findings (#4141 (comment)) are fixed. I read the code only and ran no tests or live sessions, so the live run, the check:affected receipt and the closure count of 30 rest on your report. Smoke Tests is still running and is the only check not passing. Those jobs drive local simulators and emulators and never construct the Limrun provider, and this diff touches only packages/provider-limrun/src and two facade tests, so I do not expect it to matter here. There are no conflicts. Before merge, Smoke Tests needs to finish green and the design question below needs an answer. Not blocking, take or leave: (1) in https://github.com/callstack/agent-device/blob/f051db5/packages/provider-limrun/src/lease-release.ts#L211, endWhatTheCreateNamed frees the lease only when Could the freeing logic in endWhatTheCreateNamed and the ownerHoldsNothing seam go away? When take() reports 'uncertain' because the create had not answered, the runtime already chains Of the open inline threads, I checked each one against this head and only these still need action from you: none. These are fixed at this head and can be resolved: #4141 (comment), #4141 (comment), #4141 (comment), #4141 (comment), #4141 (comment), #4141 (comment), #4141 (comment), #4141 (comment), #4141 (comment), #4141 (comment), #4141 (comment), #4141 (comment), #4141 (comment), #4141 (comment), #4141 (comment), #4141 (comment), #4141 (comment), #4141 (comment), #4141 (comment), #4141 (comment), #4141 (comment), #4141 (comment), #4141 (comment). These are resolved as intended: #4141 (comment) (404 stays idempotent, per your reply), #4141 (comment) (android.ts documents the context requirement), #4141 (comment) (lazy validation kept on purpose, per your reply), #4141 (comment) (the race is unreachable). |
Revyl kept its own lease bookkeeping: one start per lease, joins bounded by each caller's budget, stop-then-forget release with one shared stop, a shutdown drain, and expired-lease recovery by the lease's own id. That bookkeeping moves to `@agent-device/capture-kit/provider-lease-table`, and Revyl now supplies only start, stop, find-by-lease and its errors. The table bounds every caller by the earlier of its deadline and the provider's start timeout, and stops a ready answer that arrives after the starting caller's deadline or cancellation. The provider runtime instance fingerprint moves next to `providerRuntimeOwner` as `providerRuntimeInstance`; Limrun's digest is unchanged, Revyl's widens to the shared 32-byte length. Limrun's lease ownership stays as is until #4141 lands, so it moves onto the table in a follow-up. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Revyl kept its own lease bookkeeping: one start per lease, joins bounded by each caller's budget, stop-then-forget release with one shared stop, a shutdown drain, and expired-lease recovery by the lease's own id. That bookkeeping moves to `@agent-device/capture-kit/provider-lease-table`, and Revyl now supplies only start, stop, find-by-lease and its errors. The table bounds every caller by the earlier of its deadline and the provider's start timeout, and stops a ready answer that arrives after the starting caller's deadline or cancellation. The provider runtime instance fingerprint moves next to `providerRuntimeOwner` as `providerRuntimeInstance`; Limrun's digest is unchanged, Revyl's widens to the shared 32-byte length. Limrun's lease ownership stays as is until #4141 lands, so it moves onto the table in a follow-up. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Revyl kept its own lease bookkeeping: one start per lease, joins bounded by each caller's budget, stop-then-forget release with one shared stop, a shutdown drain, and expired-lease recovery by the lease's own id. That bookkeeping moves to `@agent-device/capture-kit/provider-lease-table`, and Revyl now supplies only start, stop, find-by-lease and its errors. The table bounds every caller by the earlier of its deadline and the provider's start timeout, and stops a ready answer that arrives after the starting caller's deadline or cancellation. The provider runtime instance fingerprint moves next to `providerRuntimeOwner` as `providerRuntimeInstance`; Limrun's digest is unchanged, Revyl's widens to the shared 32-byte length. Limrun's lease ownership stays as is until #4141 lands, so it moves onto the table in a follow-up. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Revyl kept its own lease bookkeeping: one start per lease, joins bounded by each caller's budget, stop-then-forget release with one shared stop, a shutdown drain, and expired-lease recovery by the lease's own id. That bookkeeping moves to `@agent-device/capture-kit/provider-lease-table`, and Revyl now supplies only start, stop, find-by-lease and its errors. The table bounds every caller by the earlier of its deadline and the provider's start timeout, and stops a ready answer that arrives after the starting caller's deadline or cancellation. The provider runtime instance fingerprint moves next to `providerRuntimeOwner` as `providerRuntimeInstance`; Limrun's digest is unchanged, Revyl's widens to the shared 32-byte length. Limrun's lease ownership stays as is until #4141 lands, so it moves onto the table in a follow-up. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Implemented the coordinator's decision for the maintainer review. Head: Design answer: adopted. The new runtime regression holds creation beyond the first release's budget, then makes disconnect throw. A joining allocation waits for the normal re-take, the joined release reports the confirmed instance plus the actual local failure, and the next allocation creates a new instance. Existing late-create tests remain green. The non-blocking items are done too:
Fallow initially caught cognitive complexity in artifact formatting. Close-warning formatting is now one common step after artifact construction; no guard threshold, baseline or allowlist changed. Four regressions failed at The review's listed inline threads were already resolved, and the fresh audit confirms all 97/97 have replies and are resolved. Repeated the short live run at this head with A's reverse mapping before release: Release A returned its instance ID without warnings: {
"success": true,
"data": {
"connected": false,
"session": "pr4141-maintainer-publish-a",
"released": false,
"provider": {
"limrunInstanceId": "android_euna_01m6dkrf3afbvbs1x7zm0w0c6e"
}
}
}Cleanup issued [
{
"args": [
"-s",
"127.0.0.1:56580",
"reverse",
"tcp:18441",
"tcp:18441"
]
},
{
"args": [
"-s",
"127.0.0.1:56580",
"reverse",
"--list"
]
},
{
"args": [
"-s",
"127.0.0.1:56580",
"reverse",
"--list"
]
},
{
"args": [
"-s",
"127.0.0.1:56580",
"reverse",
"--remove",
"tcp:18441"
]
},
{
"args": [
"disconnect",
"127.0.0.1:56580"
],
"afterReverseCleanup": "\n",
"checkExit": 0
}
]The serial was absent after release, and the SDK confirmed the instance terminated. B still answered on the same daemon: {
"success": true,
"data": {
"platform": "android",
"package": "com.android.settings",
"activity": "com.android.settings.Settings"
}
}B's HAR was ready with zero failures, and release retained the HAR. B then disconnected; daemon stop returned The SDK leak check ran from packages/provider-limrun using all Android/iOS list pages: 523 records, all 16 takeover instances found terminated, 0 still running. This includes all eight instances created in this review round. Every temporary daemon is stopped, and state/evidence directories are retained. Head |
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Head Confirmed disposal during pending creation is safe: the sole rent is already deleted, and memoized cleanup disconnects the late session; a new runtime test proves one rent/delete/disconnect. Corrected the comments, retained simultaneous capture write/close failures, and added the requested rejection watcher. The double-failure regression failed before the fix; 22 focused tests pass. Exact-head affected gate passed ( |
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Apex Sentinel could not review commit Apex Sentinel hit an internal error and could not finish this review. This is a problem on Sentinel's side, not with the change; if it keeps happening, tell whoever runs Sentinel for this repository. Push a new commit or comment |
|
The code looks good at acd4c10: the earlier review at f051db5 left evidence pending, and that evidence is now satisfied. Not blocking, and you can take or leave it: The three P3 threads are fixed at this commit and already resolved: the late-create test now asserts silent rejections (#4141 (comment)), the ownership doc scopes the re-take to uncertain outcomes (#4141 (comment)), and a write rejection now throws the write error with the close failure appended (#4141 (comment)). I read the code paths and did not run tests or a live device. The late re-take needs a create that outlives No conflicts. Smoke Tests, Repo Guards & Compatibility, Coverage and Apex Sentinel are still running with no failure so far. Repo Guards & Compatibility and Coverage run the provider-limrun unit tests, so they must finish green before merge. |
thymikee
added
the
ready-for-human
) * refactor(providers): share one lease table across cloud providers Revyl kept its own lease bookkeeping: one start per lease, joins bounded by each caller's budget, stop-then-forget release with one shared stop, a shutdown drain, and expired-lease recovery by the lease's own id. That bookkeeping moves to `@agent-device/capture-kit/provider-lease-table`, and Revyl now supplies only start, stop, find-by-lease and its errors. The table bounds every caller by the earlier of its deadline and the provider's start timeout, and stops a ready answer that arrives after the starting caller's deadline or cancellation. The provider runtime instance fingerprint moves next to `providerRuntimeOwner` as `providerRuntimeInstance`; Limrun's digest is unchanged, Revyl's widens to the shared 32-byte length. Limrun's lease ownership stays as is until #4141 lands, so it moves onto the table in a follow-up. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(providers): move the lease table to provision-kit Renting a cloud device prepares it for execution, which ADR 0019 gives to provision-kit; capture-kit owns capture, snapshot and recording. The table, its tests and its export move unchanged, and Revyl depends on provision-kit instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(providers): fence lease teardown and keep uncertain rentals owed Review of the shared lease table found routes that could forget or hand out a billed device: - Release, recovery, a failed start and shutdown now end a lease through one teardown that they all join. It fences the lease in the same step, so an allocation that arrives or is still waiting is refused with `releasing` instead of handed a device that is being stopped. - A recovery of a lease this process holds nothing for publishes an entry before its lookup, so concurrent routes share one lookup and one stop, and a failed stop keeps the handle it found. - A start that rejects and that find-by-lease cannot name stays owed: the lease is refused and kept pending until a teardown finds it. - A start can publish its rental before it answers, and a teardown waits for an answer for at most the start timeout before it stops that rental. An optional stop timeout bounds each wait for a stop, and the next teardown joins the stop still in flight. - A ready device goes to any caller still waiting; it is stopped only when every caller gave up. `gaveUp` reports the caller's own budget. - Joinability is an `allocate` option instead of a `requestOf` read, and join clears its timer once the caller is answered. Both providers pin their runtime-instance digest through their own wrapper, and ADR 0007 and ADR 0019 describe the table as built. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * build(provider-revyl): reference provision-kit from the TypeScript project The lease table moved to provision-kit, and the layering suite requires each package's project references to match its manifest dependencies. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(provision-kit): split the lease table's start and teardown steps Fallow's cognitive-complexity bar flagged runStart and runTeardown. Each step now has its own method (call the start, record its answer, name a failure, end a failed start; name a rental, finish a teardown), with no change in behavior. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(providers): hand a ready device only to callers still within budget Second review round on the shared lease table: - A ready answer goes to a caller only while that caller's own signal and deadline still hold, so a caller whose deadline passed before its timer ran is not handed a device; nobody waiting means it is stopped. - The session a start rented is recorded even when nobody takes it, so a release that ended the start reports the device (Revyl: `revylRunId`). - A caller whose budget is already spent sends no start. - Shutdown repeats its drain until it has ended every entry, including one a recovery or release opened while it ran. - Revyl carries its platform in the lease request, names its cap `REVYL_ALLOCATION_TIMEOUT_MS` with a margin over start plus readiness, and builds its driver with `createRevylLeaseDriver`. - ADR 0007 records the inventory and interactor effect of the fence, the `provider-lease-releasing` reason and Revyl's in-process digest change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(providers): name each lease refusal reason and scope the digest warning ADR 0007 names `provider-lease-releasing` for a lease that a release, recovery or failed start is ending and `runtime-shutting-down` after shutdown began, and says shutdown re-drains leases a recovery opens. The digest comment covers only derived instances and the inputs that change them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(revyl): record only Revyl API calls in the fixture backend The fixture listens on an ephemeral localhost port that any process on the host can reach, and a stray `GET /` from one made the exact-path assertion in the lease-start test fail under host load. Requests outside `/api/` are answered 404 and not recorded as the runtime's calls. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(revyl): keep a shared start running when its first caller hangs up The Revyl driver passed the starting caller's signal into the start, so that caller's cancellation ended readiness for every caller that joined it. The start now gets only the caller's deadline; the table stops the device once it is ready if no caller still wants it. A lease without a Revyl platform is refused with a typed error, the fixture ignores only a bare `GET /` probe, and the table documents which waits it bounds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(revyl): record a fixture request only when it carries the API key Every runtime request carries the key, so a request without one is another host process probing the fixture's ephemeral port. It is answered 404 and not recorded; any runtime request is recorded at any path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(providers): run a shared start for every waiter, and type the principal Coordinator follow-up on the Apex Sentinel findings at d4151ce: - The start no longer sees any single caller's signal or deadline. It runs under the provider's start timeout, and the table aborts the start's own signal only when no caller still waits. A first caller that hangs up or runs out of time ends only its own wait; a joiner still waiting gets the device. Revyl passes that signal to its start. - `providerRuntimeInstance(provider, principal)` takes an ordered list of named facts. An unset fact is the same account as an absent one; an empty or repeated name, or a principal with no value, is refused. The serialization is unchanged, so Limrun's and Revyl's pinned digests stay byte-identical. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(provision-kit): keep every named rental owed, and check budgets at delivery Codex's final check at fc94661 reproduced three table bugs: - A rental published before a stop of an earlier one confirmed could be stopped and let the entry go while the earlier stop later failed. The entry now keeps every rental it was told of, stops them one at a time in the order named, and forgets the lease only once all are confirmed. - A start that answered between a teardown deciding `pending` and the teardown being cleared joined that finished teardown, so nothing stopped its device. A late answer or publication now runs a teardown after the one in flight instead of joining it. - A caller whose deadline passed before its timer ran, or that reused a ready session after hanging up, was still handed the session. Each caller's own signal and deadline are checked at delivery; the session stays for callers within budget. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(provision-kit): fence an abandoned start, and harden the principal - Abandoning a start nobody waits for now also fences the lease and runs its teardown, so a caller that arrives later is refused instead of joining a start the provider is giving up; once its rental is stopped, a new allocation starts afresh. - `join` listens on the caller's signal directly, so a long-lived caller signal keeps no link to the entry after the wait. - `providerRuntimeInstance` builds its facts on a null-prototype object, refuses integer-like field names that JSON would reorder, and states its synchronous scrypt cost. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * chore(providers): name Revyl's readiness margin and correct the scrypt cost The allocation cap's 10 s margin is a named constant with its reason, the scrypt note gives the real 16 MiB (64 MiB is only `maxmem`), and a table test settles instead of sleeping 40 ms. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(provision-kit): report every rental a teardown leaves rented A teardown returned at the first rental whose stop failed, so a second rental of the same lease was neither stopped nor named in the cleanup report. A stop that settled unconfirmed no longer holds later rentals back; only a stop still in flight does, so stops never overtake one another. Each teardown tries each rental once and reports every one that stays rented. Only one follow-up teardown is queued per entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(provision-kit): report rentals that wait behind a stop still in flight When a stop was still in flight, the rentals named after it were left untried, as intended, but also left out of the cleanup report. The pending report now names every rental still rented, with the ones waiting behind the in-flight stop given that as their cause. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * chore(providers): name the scrypt parameters and record what an unauthenticated probe is - scrypt's block size and parallelism are named beside its cost and key length, under one comment saying all four feed the durable digest. - `RentalRecord.stopping` documents that it is the fence that keeps a later rental's stop from overtaking an in-flight one. - The Revyl fixture ignores only a request with neither the API key nor a path under Revyl's API, so a runtime call that drops the key is still recorded. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * refactor(provision-kit): drive each lease through one explicit phase Each lease entry now has a generation and one phase, `starting`, `ready` or `ending`, replacing the flags that encoded it before. Only `starting` → `ready` and any phase → `ending` happen, each in one synchronous step. After every await, the table checks that the entry is still its lease's current generation and in the expected phase before it hands out a session or forgets the lease, so a stale start answer only ever stops its device. Shutdown is a synchronous fence: an allocation, or a recovery of a lease with no entry, is refused with `shuttingDown` once it begins, so the drain ends every entry in one pass. ADR 0007 states the fence, and the Revyl troubleshooting guide names `provider-lease-releasing`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(provision-kit): sweep seeded interleavings of the lease table 400 seeded runs issue allocations, aborts, releases, recoveries, one shutdown and time passing in bursts, and answer every start and stop in a seeded order. Each run checks that no session is handed out after its lease ended or its device stopped, that no device the table took on is left running or unreported when shutdown settles, that at most one stop of a device is in flight, and that a failed stop is retried until the final drain confirms it. A failing seed names itself for replay. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test(provision-kit): name the interleaving sweep for its module's aspect R84 colocation reads a test named <module>-<aspect>.test.ts as its module's; a dotted aspect names no module. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(provision-kit): look for new rentals before a teardown forgets its lease The seeded sweep, extended to starts that publish their device early, found a teardown that awaited its stops, found none owed, and then forgot the lease even though the start had recorded a rental meanwhile: shutdown resolved with that device still rented (seed 92). The teardown now re-checks synchronously for an unnamed or unstopped rental before it forgets the lease, and loops if one appeared. The sweep now publishes some devices before their start answers, does not rent a published device twice, and accepts a shutdown report that names a still-unnamed rental by its lease. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(revyl): retry a released open with open, not connect `connect` saves the profile and rents nothing; `open` allocates the new lease that rents a fresh device. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(revyl): point a released open at open, and state the 370 s ceiling The `provider-lease-releasing` hint now names `open`, as the guide does, since `connect` rents nothing. The guide states that an `open` with no deadline stops waiting after 370 s, and Revyl's instance wrapper keeps the reason its principal is digested. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * docs(adr): say that a release only joins a lease entry that already exists Shutdown's drain needs one pass because nothing registers an entry once it begins: allocation and entryless recovery are refused, and a release only joins an existing entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(providers): stop a released Revyl start without waiting out readiness A release, recovery or shutdown that took over a lease mid-start waited for the start to finish readiness before it could stop the device, up to Revyl's whole allocation timeout. The table now aborts the start's own signal as soon as a teardown takes the lease, since no caller can receive that device any more, and reports the lease's end, not the aborted start, to callers still waiting. Revyl publishes its session through `control.rented` the moment `start_device` answers with it, the first moment Revyl holds it, and a release of a session that never became ready still reports its run id. New test: a release during a stalled readiness stops the device at once. The two table tests the seeded sweep fully covers are folded into it to keep the change within its budget, and Revyl's lease tests share one refusal helper to stay within the file's line cap. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(providers): let teardown alone abort a start, and pin readiness aborts Teardown is now the one place that aborts a start's signal: when a release, recovery, shutdown, failed start or the last waiter leaving ends a lease whose start has not answered. `stillWanted` is false for an ending entry, so nothing can count a fenced lease's waiters, and the last-waiter path only ends the entry. Two Revyl regressions pin that the abort reaches readiness: a sole caller that hangs up during readiness has its session stopped before the delayed health answer, and a release that lands while the first caller still waits stops the session and refuses that caller before health answers. Both fail if the teardown abort, or Revyl's `signal: control.signal`, is removed. The table test those cover is folded in to keep the change within its budget. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(provision-kit): stop a late rental even when a driver's error factory throws When a late start answer lands on an ending lease, the table builds the waiters' error with the driver's `releasing()` before it runs the follow-up teardown that stops the late rental. A driver whose factory threw skipped that teardown and left the device rented. The failure is now built inside a guard, and the follow-up teardown always runs. Tests: a start that fails 50 ms in answers its caller at once, stops what it rented and frees the lease for a new allocation; and a driver whose `releasing()` throws still has a late answer's device stopped. The table test that the seeded sweep's stop-overlap check covers is folded in, and the table's type docs are condensed to stay within the change's budget. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(provision-kit): name the lease's end in every refusal a caller gets A caller that gave up in `join()` while a release, recovery or drain was ending its lease got its own `gaveUp` reason (`request_canceled` or `provider-not-ready`) instead of `provider-lease-releasing`, and lost `runtime-shutting-down` during shutdown. One private `refusal(lease, entry, fallback)` now decides every refusal: shutdown first, then a lease that is ending or replaced, else the site's own reason. Allocate, delivery, the start's failure and join's give-up all go through it. A start that failed on its own still reports its own error. Join builds its refusal before the caller leaves, so a caller's own departure is never read as someone else's release, and a throwing driver factory still settles the caller. Regression (from #4169): a caller that gives up while a release is in flight is told the lease is being released, and exactly one `/stop` is sent. It fails at dce5c56 with `request_canceled`. The table notes that it does not bound `findByLease`, and the failed-start test schedules its rejection on the next tick instead of asserting a wall-clock bound. Test comments that restated their names are trimmed to keep the change within its budget. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Summary
Fix Limrun lease ownership and release races (SM-4). One owner is published before the first await and carries every resource.
take()is the sole freeing route; uncertain pending creation schedules normal disposal after settlement, using idempotent deletion and memoized local cleanup.A confirmed disposal can precede creation settlement safely: its only rented instance is already deleted, and observed local cleanup awaits the eventual session. A new runtime test proves one rent, one delete, and one late disconnect.
Capture write failures now retain a simultaneous close failure and preserve the original error as cause. The late-create/local-failure test also observes unhandled rejections.
Scope: 19 files; this round changes five (three production, two tests), with no resource-disposal behavior change.
Validation
Head
acd4c1026c6a623afaef3f50fb90be618a48879b; this round's base9043233d01a3b1bb28797124c615bb50ecc40176.pnpm check:affected --run --head HEAD: passed at head; receiptrun-mRc75q,exactCommit=true,checkoutChanged=false; 312 related tests pass and 16,794 unit tests pass (one skipped).9043233d: Settings, reverse cleanup, second-session isolation, HAR, disconnect, daemon stop; all 16 owned instances terminated, zero running. This diagnostics-only round requires no repeat live run.CI at new head is queued/running; hosted lanes remain authoritative. No merge or label changes.