Repository navigation
fix(provider-webdriver): allocate one WebDriver session per lease - #4130
Conversation
Two concurrent lease_allocate calls for the same lease id each rented a billed cloud session: the reuse check ran after an await, so neither call could see the other's session, and only the last one published was ever releasable. A release or shutdown that landed while a create was in flight also found nothing to dispose. Publish the lease's entry before the first await so a duplicate allocation joins the in-flight create instead of starting its own. Model the entry as a discriminated union: allocating until the hub answers, then ready. Release and shutdown take the entry out of the map in one synchronous step, so whoever holds it disposes the session exactly once, including a session that finishes renting after the lease is already gone.
|
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.
No blocking findings.
I reviewed merge-base f7879cf2d266ee1dd476d0342b2819bdeceb647b through head 0f58d95ea0a7ed853978fa47c16348dcd7e75bd7. The allocating/ready entry gives each lease one published create promise; release and shutdown synchronously take ownership of that entry, then await and dispose the eventual session once. This also handles a joined allocation after release and a new allocation for the same lease while the old create is pending: the old taker disposes its session, and the old allocation cannot return a session no longer held by its entry.
The new regression tests exercise the original duplicate-create and in-flight release/shutdown failures; each would fail on the base implementation. Refusal classification uses AppError details.reason, consistent with ADR 0010. The union is a reasonable shape because it makes the two states explicit; no simpler representation improves ownership clarity. Changes stay in the owning session manager and its adjacent tests.
![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 2 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.
Needs changes. Reviewed exactly f7879cf2d266ee1dd476d0342b2819bdeceb647b..0f58d95ea0a7ed853978fa47c16348dcd7e75bd7.
-
[P1] Keep a releasing allocation tracked until disposal finishes —
packages/provider-webdriver/src/runtime-session.ts:141. Interleaving: allocation A is held in preparation orPOST /session; release removes A's entry and waits; allocation B for the same lease now sees no entry and starts a second create. When both succeed, two billed sessions have been created for overlapping allocations of one lease. A scratch reproduction returnedcreated=[session-1, session-2], with only session-1 deleted by that release and session-2 remaining live until a later shutdown. The same removal also meansA -> release -> shutdownmakes shutdown resolve with the create and cleanup still pending: the entry is absent from its snapshot. Fix: keep an owner-local releasing entry with a shared disposal promise, refuse allocation during that lifetime, and have shutdown join that promise. Remove it by identity only after disposal settles. Add regressions for replacement allocation before the old create settles and shutdown joining an in-flight release. -
[P2] Revalidate ready-session reuse after its new await —
packages/provider-webdriver/src/runtime-session.ts:114-118. Interleaving: an initial allocation finishes; a repeated allocation captures the ready entry and suspends inrejectRefusedProfileFields; release or shutdown removes and closes that session; the repeated allocation resumes and returnsheartbeat(lease), which is nowundefined. The allocation resolves successfully although no session remains, bypassing the typed refusals used by the allocating branch. This is a new race: the base performed validation before looking up the live session and had no await between reuse lookup and heartbeat. The scratch reproduction confirmed a resolvedundefinedafter one successful DELETE. Fix: after validation, require the same ready entry/session to remain current and throw the corresponding typed release/shutdown refusal otherwise. Cover both teardown paths. -
[P2] Do not report disposal success without its outcome —
packages/provider-webdriver/src/runtime-session.ts:127-129,:500-502. Interleaving: allocation is pending; shutdown takes its entry; create succeeds; DELETE fails. The allocation nevertheless throws withdisposedSession: trueand the hintNothing to clean up: agent-device stopped the session it was creating.Shutdown discardscloseSession's warnings, so the caller gets neither the DELETE failure nor the remote session identity needed for manual cleanup. The scratch reproduction confirmed this exact error after a rejected DELETE. Even a slow successful DELETE can still be pending when this success claim is emitted. The release refusal makes the same premature claim and advises another billed allocation. Fix: share/await the disposal outcome, preserve the allocation refusal as primary, attach failed-cleanup evidence and session identifiers as structured secondary diagnostics per ADR 0010, and choose a hint that reflects confirmed versus uncertain cleanup. Keep the existing failed-DELETE sessionId retention and no-create-retry policy.
Validation: pnpm install --frozen-lockfile && pnpm build passed. Ran the package unit suite once: pnpm exec vitest run --project unit-core packages/provider-webdriver — 27 files / 311 tests passed. Separate scratch probes imported the unmodified runtime and reproduced the findings. A 480-schedule action/settlement matrix covering successful and rejected creates found no duplicate or missing disposal attempts when every earlier release and final shutdown were awaited; it does not resolve shutdown returning before a release finishes. No CI wait. Source checkout is clean.
The three added regressions do detect the original base defects by source comparison: the base can create twice, release finds no pending session, and shutdown snapshots no pending session. I did not rerun them on the base. They omit the interleavings above. The production diff stays in the owning session manager. COMMAND_FAILED plus details.reason fits ADR 0010 and survives normalizeError; no text matching or new global error-code registration is needed. The owner-local union is appropriate, but it currently omits the releasing lifetime. A single lease entry holding the creation and shared disposal promises could also express this without a separate framework; the hand-built deferred and extensive narrating comments are simplifiable, not additional blockers. Intentional stale-owner retention, failed-DELETE client identity, and unretried ambiguous creates remain unchanged.
|
Reviewed 0f58d95. I found three defects in the session lifecycle. Checks are still running (Smoke Tests and Coverage; Coverage runs the new allocation tests, so it overlaps this diff). There are no conflicts. Before merge, each lease entry must stay in the map until its disposal settles, and the live-hub evidence below is still missing. Shutdown can finish while a release is still running. Release deletes the entry and then awaits the creation at L142. Shutdown snapshots the map, no longer sees that entry, and resolves at once, while the create, DELETE and provider cleanup are still pending. If the process exits then, a billed hub session stays alive until the provider reaps it. The rule: an entry stays in the map until its disposal settles, and allocate, release and shutdown all see it. Please add a releasing state with one shared disposal promise and the reason that started it (release or shutdown). Release swaps the entry to releasing synchronously and deletes it by identity after closeSession settles. Shutdown snapshots every entry, including releasing ones, and joins their disposal. Allocate refuses while an entry is releasing. This matches the Revyl rule at packages/provider-revyl/src/runtime.ts:150-160 and :436. Please add regressions for release then same-lease allocate, and release then shutdown, while the create is held. A repeat allocation can resolve successfully with no session. It reads a ready entry at L109 and suspends in the profile-field refusal at L117, which awaits a dynamic import. A release or shutdown can take and close the session in that gap. On resume, heartbeat at L118 returns undefined, so lease_allocate answers ok with no provider data for a session that was just deleted. The caller then drives a device that no longer exists and gets no typed refusal. The base had no await between the lookup and heartbeat, so this window is new. The rule: run the profile-field refusal once, before the entry lookup, for every caller, so the lookup and publish stay synchronous after that single await, as in Revyl allocate at packages/provider-revyl/src/runtime.ts:257-277. Then the refusals at L117, L123 and L251 can go. A refused caller can be told cleanup succeeded when it did not. When a release or shutdown has taken the entry, allocate throws as soon as the creation resolves, while the disposal may still be pending or may fail. Even so, the refusal sets disposedSession: true with the hint "Nothing to clean up" (L500-502), and the release variant says the session is already stopped (L478). Shutdown also drops closeSession warnings, because Promise.allSettled at L195 ignores the close result. If DELETE /session fails, the caller holds a structured claim of cleanup and no session id to stop by hand. The daemon reports released only when the provider returned no warnings (packages/daemon/src/sessions/handlers/lease.ts:317-319). The rule: a refusal may claim cleanup only after the disposal it names has settled with no warnings. Please await the entry's shared disposal promise before throwing, set disposedSession from its outcome, attach the session ids and warnings on failure, and take the reason from the entry rather than the global shutDown flag. The open inline threads still apply: the release-before-creation delete (P1), the reason from global flag (P2), and the refusal inside shared creation (P2). The three fixes above resolve all of them. No thread can be resolved yet. Could the whole fix stay in WebDriverSessionManager by copying the Revyl rule exactly? That means one profile-field await before the lookup, one entry type with allocating, ready and releasing{reason, disposal} states, allocate refusing on a releasing entry with that entry's reason after awaiting its disposal, and shutdown joining every disposal. I think that fixes the three defects and the three threads in about the same number of lines. It also makes the later move onto the shared lease table in #4102 a pure move. Nothing needs to change first for this fix. Not blocking, take or leave: add a test where the first POST /session returns 500, the first allocate rejects, and a second allocate of the same lease creates session-1 (nothing asserts the identity-based clear at L232-236 today); consider linking #4102 in the PR body, since this copy of Revyl's entry logic uses a weaker delete-before-disposal rule; and optionally bound a joining caller by its own signal and deadline, as Revyl does with revylCallerBudget, instead of the starter's deadline at L324-326. For live validation, the PR body reports a BrowserStack run (created=1 deleted=1) but no receipt, and that run bypassed the daemon lease_allocate route and the prepareSession upload path. After the fixes, please re-run against the real hub through createCloudWebDriverRuntime and attach the log. It should cover (a) concurrent allocate, (b) release while the create is in flight and then a same-lease allocate before the release settles, and (c) release in flight and then shutdown. For each case, the hub's created and deleted session-id lists should match with zero live sessions at the end, and the typed reason each refused caller received should be shown. I read the code and did not run the tests or these interleavings. I also did not confirm that the process exits right after runtime shutdown resolves, which the billing impact in the first finding assumes. |
…ends A release took a lease out of the manager's map and then waited on the hub, so a fresh allocation in that window saw a free lease and rented a second billed session the release never saw, and a shutdown that arrived there answered with the first session still live. A repeated allocation validated its flags after reading the live session, so a teardown landing inside that await left the caller told the lease was held by a session that no longer existed. A shutdown whose DELETE was refused reported a clean close, and the allocation refused for that session was told nothing needed cleaning up. A lease entry now keeps owning its session while it is being ended: the take and the one disposal it starts are recorded in the same synchronous step, the entry leaves the map only once that disposal settles, and a second release or a shutdown joins it. An allocation reports the disposal it waited on, naming the session and whether its end is confirmed, pending, or failed; a shutdown that could not stop a session now fails the way a plugin cleanup failure is reported at close. Every allocation is refused for its own refused profile flags before it joins or publishes anything.
|
Thanks for the update. At 8b2bdb0 the three round-1 code issues are fixed: a racing allocate can no longer start a second create, the refusal reason comes from the entry's own take, and a refused joiner can no longer poison the create it joins. One piece of live evidence is still missing. All 19 checks pass at this commit, and the unit lanes run the changed allocation tests. Before merge, please attach the live run described below. The round-1 review asked for live case (b): a release issued while the real Not blocking, and fine to take or leave: Open threads: these still apply, all non-blocking: the test polling budget, artifact listing in shutdown, and merged shutdown warnings. Two lower-priority threads on the teardown refusal wording also still apply. These do not apply and can be resolved: repeat-allocate envelope is the same answer heartbeat returns, behind the same guard; disposal rejection handler is moot because every awaited step in One limit on what I checked: I did not run the new tests. I judged the claim that 6 failed at 0f58d95 plausible from reading only. |
Round 2 — review #5726962994 addressedNew head: Each finding got a regression test first; all six new cases were recorded failing at the reviewed head ( P1 —
|
| head | scenario | POST /session |
DELETE |
racing allocate |
|---|---|---|---|---|
0f58d95e (reviewed) |
allocate during in-flight release | 2 | 2 | resolved with a second real session |
8b2bdb05f5 (this push) |
allocate during in-flight release | 1 | 1 | rejected DEVICE_IN_USE / provider_session_release_in_progress, cleanup: pending |
At the new head the hub's DELETE was still held when the racing allocate, the repeat allocate and the shutdown were all issued; pendingBeforeAnswer { release: true, shutdown: true, deleted: 0 } — neither the release nor shutdown answered while the session was live, and after the delete was released created=1 deleted=1. Concurrent-allocate on one lease: created=1 deleted=1 at both heads (that half was already covered by round 1).
P2 — runtime-session.ts:118 ready-session reuse can resolve successfully after teardown
Accepted, fixed structurally rather than by adding a post-validation check (matching the reviewer's own note). allocate now runs rejectRefusedProfileFields(req) first, before the map lookup and before any entry is published or joined; nothing after the lookup awaits, so the ready-session answer and the session it describes are the same synchronous step and a teardown cannot land between them. createSessionForLease no longer validates flags — every allocation is refused for its own flags before it bills anything, and a caller that joins someone else's create is no longer refused for the other caller's flags (it is refused for its own, before it joins).
Tests (all failed at the reviewed head, where the repeat allocation resolved session-2 / a stale success):
a repeated allocation cannot answer that a released lease is alive— the hub holds the release'sDELETE, so the release is provably still ending the session at the moment the repeat allocation answers; it is refusedprovider_session_release_in_progresswithcleanup: pending, and bills nothing.a repeated allocation cannot answer that a shut down lease is alive— same shape onshutdown.an allocation refused for its own flags leaves the create it joined alone— covers the flags move: the joined create still resolves with its session, the refused caller getsINVALID_ARGSfor its own flags, one create total.
P2 — runtime-session.ts:129 shutdown claims successful disposal when DELETE fails
Accepted. Disposal now reports cleanup: 'confirmed' | 'uncertain' | 'none', derived from the provider's answer rather than assumed, and it is threaded to both surfaces:
- the refused allocation carries
cleanuppluscleanupWarnings(socleanup: 'uncertain'when theDELETEwas refused, never'none'); shutdown()now throwsCOMMAND_FAILED/details.reason: 'provider_session_cleanup_incomplete'naming every session id it could not end (providerSessionIds,cleanup: 'uncertain',cleanupWarnings) instead of resolving. The plugin registry captures a throwingruntime.shutdown()intoplugins.failed, which is what the daemon's shutdown report and theprovider_plugin_cleanup_faileddiagnostic read, so the failure reaches an operator rather than being dropped. No retry or extra delete is attempted — the create-retry policy is untouched, and the livecreated/deletedcounts above are unchanged by this.
Test a session shutdown could not stop fails the shutdown and the refused allocation failed at the reviewed head: shutdown resolved as clean and the refused allocation was told cleanup: 'none' while the hub had refused the DELETE.
Validation at 8b2bdb05f5
pnpm check:affected --run --head in the foreground, status: passed, exit 0, exactCommit: true at this head — format, lint, typecheck, layering, fallow, build, vitest-related (304 tests) and unit (16 757 tests) all passed; device/replay/web-smoke ids skipped as not affected. Receipt: /Users/thymikee/.t3/worktrees/agent-device/t3-c4bef626/.tmp/validation/run-9dCIop/receipt.json. Focused: pnpm exec vitest run --project unit-core packages/provider-webdriver → 27 files, 317 tests passed; daemon session and http projects also green. Scratch harnesses deleted, working tree clean.
Honest note: provider shutdown has a 5 s budget (DAEMON_PLUGIN_SHUTDOWN_TIMEOUT_MS) while a held POST /session can run longer than that. A create disposed after that budget still gets disposed and its delete still goes out, but the daemon close reports the plugin as pending rather than failed. That budget is owned by the daemon, outside this PR's scope.
Two windows stayed open on the reviewed head. An allocation validated its profile fields before it published the lease's entry, so a release issued while that check was pending found no entry, answered that the lease held nothing, and the create that resumed billed a session no route was left to stop. Cloud leases without a device key are not serialized by the daemon's lease registry, so both calls can be in flight at once. The entry is now published before the allocation's first await, and the publishing caller's own flags are refused by the create it stands on, as its first step, so a call that cannot be honoured still bills nothing. A caller that joined an existing entry is still refused for its own flags, and its live session answer is still read with no await after the lookup. Ending a session also waited on the plugin's artifact lookup, which has no bound of its own. A custom listArtifacts that never answered kept the lease DEVICE_IN_USE for every later caller and was awaited by daemon shutdown. Ownership now ends once the remote DELETE and the provider's cleanup settle; the artifact report is started at that point and joined only by the release that carries it. Both provider callbacks the manager awaits are bounded by providerCallbackTimeoutMs, defaulting to this package's one-round-trip budget, and a cleanup that outlives it is reported as PROVIDER_CLEANUP_FAILED with the session's outcome uncertain rather than holding the lease. Also: a disposal that could not be observed at all now reports itself instead of hanging every waiter; the reuse answer and heartbeat share one envelope builder; and a shutdown's hint names the shutdown rather than a release.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
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.
Needs changes. Reviewed 0f58d95ea0a7ed853978fa47c16348dcd7e75bd7..8b2bdb05f5cd37d18d91932c27027784fbe2f65b, including daemon lease callers and plugin shutdown handling.
-
[P1] Track allocation admission before the first await —
packages/provider-webdriver/src/runtime-session.ts:144-151. Moving validation before publication opens a new release window: callallocate(lease), then release whilerejectRefusedProfileFields()is pending.release()sees no entry and resolves; validation then resumes and creates a billed session for the released lease. This regresses the reviewed head, which published the allocating entry before awaiting validation. It is reachable through daemon lease callers for cloud leases withoutdeviceKey, whereLeaseRegistry.runDeviceMutation()does not serialize them. A scratch probe through the actualhandleLeaseCommandsandLeaseRegistry, with the unmodified runtime and a fake WebDriver hub, returnedrelease={ok:true,data:{released:true}},allocationOk=true,sessionId=session-1,activeLeases=0, and hub events[POST]: the remote session survives a successful release with no active daemon lease. Fix: make validation synchronous before lookup/publication, or track pending admission so release joins or invalidates it. Preserve per-caller validation and the ready-session path's lack of a post-lookup await. Add a regression that releases immediately after allocation starts, before provider preparation begins. -
[P2] Bound callbacks that now retain the releasing entry —
packages/provider-webdriver/src/runtime-session.ts:373-377.take()only removes the entry and resolves its shared disposal afterdisposeTakenEntry()finishes, but providercleanup()andlistArtifacts()have no manager-enforced bound. These callbacks are supported by the exported WebDriver plugin runtime options. If a plugin's artifact lookup never settles, even after successful DELETE and provider cleanup, every later allocation of that lease returnsDEVICE_IN_USEforever, repeated release never completes, and shutdown joins the same stuck disposal. A probe holding onlylistArtifactsconfirmed[POST, DELETE]with release and shutdown still pending and allocation refused withcleanup: pending; answering that callback lets both finish. Shutdown now also performs artifact lookup, whereas the reviewed head's shutdown did not. The bundled BrowserStack and AWS implementations have their own I/O bounds (15 s artifact HTTP and 30 s host commands), but custom callbacks have none; the daemon's 5 s plugin-close budget reports pending and does not settle this runtime disposal. Fix: bound provider callbacks and keep optional artifact collection from indefinitely controlling lease ownership or shutdown completion after resource cleanup has settled. Cover a hanging artifact callback separately from a held DELETE.
Round-1 dispositions:
- P1 release ownership: fixed after an entry exists: a take is recorded synchronously, mid-release allocation is refused, repeated release/shutdown join one disposal, and map removal is identity-checked after settlement. Finding 1 above concerns the newly introduced window before entry publication.
- P2 ready reuse: the stale ready-entry answer is fixed: validation precedes lookup and ready reuse returns without another await. Each joining caller validates its own flags.
- P2 failed DELETE on shutdown: the runtime now reports
cleanup: uncertainand throwsCOMMAND_FAILED/provider_session_cleanup_incompletewith session ids. RegistrycleanUp()catches shutdown errors, still runs plugin dispose, and closes registrations concurrently; the daemon records failed plugins and continues finalization. Existing report projection retains code/message/hint (including ids in the message), rather than the full cleanup details.
DEVICE_IN_USE / provider_session_release_in_progress correctly describes contention and normalizes to retriable: true; daemon allocation does not automatically retry it, and a refused reused allocation leaves the original lease alone. The other new refusals use existing COMMAND_FAILED with details.reason, consistent with ADR 0010; no production consumer text-matches these messages. Stale device ownership remains retained through release and cleared at shutdown; failed DELETE still leaves the client's sessionId intact. No create retry was added.
Validation: provider unit suite run once — pnpm exec vitest run --project unit-core packages/provider-webdriver: 27 files, 317 tests passed. Scratch probes imported unchanged source; no source edits, commits, or pushes. Reviewed checkout remains clean. No CI wait.
Apex SentinelVerdict: Since the last review: 6 new · 1 still open · 4 no longer raised 7 finding(s): 3 minor, 4 nit. Verdict: approve. reached max rounds (1). 🟡 Minor (3)
🔵 Nit (4)
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: 5 new findings on 8b2bdb0. The verdict and the full list are in the summary comment.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Get a Slack DM when cubic finishes reviewing your PRs. Connect Slack
View guided diff | Turn on auto-fix | Re-trigger cubic
Round-3 dispositions — head
|
| Where | Disposition |
|---|---|
runtime-session.ts:153 — reuse answer drops the envelope heartbeat() shares |
Accepted. liveLeaseEnvelope() is now the single builder behind both the heartbeat answer and the reuse answer of allocate, so lease_allocate and lease_heartbeat cannot drift for recordProviderSession. |
runtime-session.ts:338 — disposal .then had no rejection handler |
Accepted. disposeTakenEntry is handled on both branches. The rejection branch settles cleanup: 'uncertain' with PROVIDER_CLEANUP_FAILED, resolves the disposal, and still removes the entry, so a disposal this manager lost sight of is reported rather than hanging release, shutdown and every later allocation of that lease. |
runtime-session.ts:487 — an in-flight create is invisible to readers |
Declined, deliberately unchanged. While the create is in flight there is no session id, endpoint or device to hand out: findSessionForDevice, getInteractor and listCloudArtifacts have nothing valid to return, and making heartbeat answer for an unsettled create would make the daemon recordProviderSession for a session that may never exist and point callers at releasing a session that does not exist. The invariant this PR owns is that no billed session can outlive a route able to end it — which the admission entry above now guarantees — and the reviewer notes this window matches the predecessor. Widening reader visibility is a separate behaviour change, and the minimal-change instruction for this round rules it out. |
runtime-session.ts:592 — refusalForTeardownInProgress duplicates the cleanup/hint logic |
Partly accepted. Both branches now read cleanup and providerSessionId from the take exactly once, so those two facts cannot drift. The two builders stay separate on purpose: the refusals differ in error code (COMMAND_FAILED vs DEVICE_IN_USE, ADR 0010), reason, message and hint audience — the in-progress caller created nothing and is told about the lease, while the teardown caller's own session died and needs teardownHint. Collapsing them means passing code, reason, message and hint as four mode arguments to one builder to share two lines. |
runtime-session.ts:640 — teardownHint names "that release" on a shutdown |
Accepted, with your patch: teardownHint(provider, by, cleanup) names the route that is actually running. |
Validation
pnpm check:affected --run --head 7490b82b00, foreground: status: passed, exit 0, exactCommit: true. Format, lint, typecheck, layering, fallow, build, vitest-related (304 tests) and unit (16 760 passed, 1 skipped) all passed; device, replay and web-smoke ids skipped as not affected. Receipt: .tmp/validation/run-Ud5g4z/receipt.json. Focused: pnpm exec vitest run --project unit-core packages/provider-webdriver → 27 files, 320 passed; --project provider-integration on cloud-webdriver-runtime.test.ts + cloud-webdriver-provider-adapters.test.ts → 15 passed, so the release envelope still carries ready cloudArtifacts. No user-facing help or docs change: no documented flag, command or output changed.
Carried forward from round 2, unchanged: plugin close has a 5 s budget (DAEMON_PLUGIN_SHUTDOWN_TIMEOUT_MS) while a held create may run longer, so a create disposed after that budget reads as a pending plugin in the shutdown report.
|
Coordinator check of round 3 ( Both round-2 findings are resolved:
Non-blocking notes, for follow-up only:
No blocking findings. |
|
Most of the earlier findings (#4130 (comment)) are fixed at 7490b82, and one still needs a live run. All 18 checks pass, and there are no conflicts. The earlier review asked for a live run of the main race: a release or shutdown while the real hub's Not blocking, take or leave: (1) the Would it be simpler to drop the public On the open inline threads: #4130 (comment) (unbounded failure cleanup) still applies, and it is not blocking because the bundled BrowserStack and AWS callbacks have their own I/O bounds, but the doc at runtime.ts:106-111 overclaims. #4130 (comment) (waitFor yield cap) and #4130 (comment) (warnings keyed by code) still apply and are not blocking. #4130 (comment) (duplicate uncertain hint) still applies; it is lower priority. These do not apply any more and can be resolved: #4130 (comment) (heartbeat and reuse answers now return I did not run the new or changed tests; the fail-first at 8b2bdb0 and the pass at head are judged from reading the code. The ordering in note (2) is reasoned from the code and not reproduced. I did not re-trace the daemon lease handlers or plugin close, which are unchanged since the earlier review, and I did not inspect the round-2 live logs (the PR body gives counts only). Before merge, please attach the two live real-hub runs at 7490b82 with equal created and deleted session-id lists, zero live sessions, and the typed refusals with cleanup confirmed. |
… warnings per session Two review threads on the reviewed head. The new provider-callback budget covered a session's disposal but not the hand-back a create owes when the hub refuses it. That cleanup is the plugin's own code too, and no session exists yet for a release or a shutdown to end, so a custom cleanup that never answered held the allocation, the lease behind it, and daemon shutdown forever. It now runs through the same bounded providerCallback. A close that could not stop several sessions collapsed their warnings into one map keyed by code, so two sessions refused by the provider were reported with one reason. Each warning now travels as its own entry with the session id it is about, so every session in the shutdown error stays diagnosable. The allocation tests stopped polling the clock for a step to happen: waiting for a create to reach the provider, for the hub to end a session, and for an artifact lookup to start are all arrival signals now, each resolved in the step that reaches it, with one watchdog that only fires when a step never comes. A busy CI worker can no longer fail a test whose allocation is still progressing.
There was a problem hiding this comment.
Apex Sentinel: 6 new findings on 8621648. The verdict and the full list are in the summary comment.
Round-4 dispositions — head
|
| Thread | Where | Disposition |
|---|---|---|
discussion_r4434966109 |
runtime-session-allocation.test.ts — test polls 200 scheduler yields for prepareSession |
Fixed (test-only). Replaced the bounded poll with an arrival notification: startProviderPreparation keeps a waiter list and resolves each waiter in the same step that registers a create. The two other clock-polling waits this file had became signals too — hub.deletedArrived('session-1') and startSignal() / awaitSignal(...) for the artifact lookup — and untilSettled's wall-clock loop is gone. One unref'd watchdog (ARRIVAL_BUDGET_MS = 2_000) remains and only fires when a step never arrives at all, so a slow or instrumented worker can no longer fail a test whose allocation is still progressing. |
discussion_r4434966117 |
runtime-session.ts — shutdownLeftSessionsRunning collapsed warnings by code |
Fixed. cleanupWarnings on the shutdown error is now one entry per warning, each carrying its providerSessionId, code and message, so two sessions the provider refused to end report two refusals. A refusal for a single session keeps the code-keyed shape (one session's warnings already carry distinct codes), so details.cleanupWarnings for refusalForTeardown is unchanged. New regression a shutdown that could not stop two sessions names both refusals: fails at 7490b82b with warnings.map is not a function (the field was a map), passes here. |
discussion_r4435167374 |
runtime.ts / createSessionWithPreparedCleanup — cubic P1: prepared.cleanup on a failed create was not bounded |
Fixed. The failure hand-back now runs through the same bounded providerCallback as the disposal's cleanup. This was the one cleanup the allocation itself must not wait on forever: no session exists yet, so neither release nor shutdown can take it over, and a hanging plugin cleanup held the allocation, the lease behind it, and daemon shutdown. The create's own refusal is still what the caller gets, with the timed-out hand-back in details.cleanupError; providerCallbackTimeoutMs's doc comment now names both routes. New regression a create the hub refused still answers when its provider cleanup hangs: times out at 5 s at 7490b82b (the allocation never answered), passes here, where the next allocation reaches the hub again instead of a contention refusal and shutdown() resolves. |
Fail-first record, same command (pnpm exec vitest run --project unit-core packages/provider-webdriver/src/runtime-session-allocation.test.ts): at 7490b82b with the final test text → Tests 2 failed | 13 passed (15); at 86216483fb → Tests 15 passed (15). The thread-1 item is test flakiness, not a behaviour break, so it carries no fail-first test; the arrival signal replaced the poll outright.
Validation: pnpm check:affected --run --head 86216483fb, foreground — status: passed, exit 0, exactCommit: true; format, lint, typecheck, layering, fallow, build, vitest-related (304) and unit (16 762 passed, 1 skipped) all green; device/replay/web-smoke ids skipped as not affected. Receipt: .tmp/validation/run-H59C6j/receipt.json. Focused: --project unit-core packages/provider-webdriver → 27 files, 323 passed; --project unit-core packages/plugins packages/daemon/src/server/http packages/provider-webdriver src/provider-webdriver.test.ts → 70 files, 618 passed (the shutdown report's cleanupWarnings shape has no consumer outside this package and its tests). No user-facing help or docs change: no documented flag, command or output changed. PR body Summary and Validation updated to this head.
|
Thanks for the update. At 8621648 the code looks right, and the earlier review's code findings are fixed. One piece of live evidence is still missing. CI is green: 19 checks pass, and your check:affected receipt at this head also passed. There are no conflicts. The previous review asked for two live real-hub runs: a release, and then a shutdown, issued while BrowserStack's POST /session is still pending and the allocation waits on Not blocking, and fine to take or leave: the rewritten On the open inline threads: three are fixed at this head and can be resolved: the create-failure cleanup timeout (#4130 (comment)), the waiter-list wait (#4130 (comment)), and the per-session shutdown warnings (#4130 (comment)). Six more need no change, with a one-line reason each. The shared during-allocation reason after shutdown stays distinguishable through cleanup 'none' and its own hint (#4130 (comment)). The shutdown rejection is reported as plugins.failed while other plugins still close (#4130 (comment)). The I did not run the changed tests; fail-first at 7490b82 and pass at head are judged from reading the code. I did not inspect the earlier live logs or the validation receipts, because the PR body gives only counts. Before merge, please attach the two live real-hub runs at 8621648 described above. |
|
Review follow-up for the 2026-11-01 12:34 maintainer comment. New head:
Validation at the new head: Complete live log at 8621648{"at":"2026-11-01T21:11:15.847Z","event":"run start","head":"86216483fb980ab3d9c6fe6ddd5a64b6cfa60041","by":"release","leaseId":"sm3-4130-release-1793567475847","device":"Samsung Galaxy S21","osVersion":"11","route":"createCloudWebDriverRuntime.leaseLifecycle","preparation":"no-app capabilities; no app upload","transport":"native fetch to real BrowserStack; no held responses or fake hub"}
{"at":"2026-11-01T21:11:15.953Z","event":"POST /session sent; response pending","by":"release"}
{"at":"2026-11-01T21:11:16.114Z","event":"teardown invoked","by":"release","postPending":true}
{"at":"2026-11-01T21:11:16.115Z","event":"same-lease allocate invoked","by":"release","postPending":true,"teardownSettled":false}
{"at":"2026-11-01T21:11:16.115Z","event":"racing allocate outcome","by":"release","status":"rejected","code":"DEVICE_IN_USE","details":{"reason":"provider_session_release_in_progress","provider":"browserstack","leaseId":"sm3-4130-release-1793567475847","runId":"pr4130-86216483fb","cleanup":"pending","hint":"Wait for this lease to finish releasing, then allocate it again."},"postPending":true,"teardownSettled":false}
{"at":"2026-11-01T21:11:37.078Z","event":"POST /session answered","by":"release","status":200,"providerSessionId":"942bd37d58a28bb67efc75b73f15d7569e60896c"}
{"at":"2026-11-01T21:11:37.080Z","event":"DELETE /session sent","by":"release","providerSessionId":"942bd37d58a28bb67efc75b73f15d7569e60896c"}
{"at":"2026-11-01T21:11:37.277Z","event":"DELETE /session answered","by":"release","status":200}
{"at":"2026-11-01T21:11:37.278Z","event":"first allocate outcome","by":"release","status":"rejected","code":"COMMAND_FAILED","details":{"reason":"provider_session_released_during_allocation","provider":"browserstack","leaseId":"sm3-4130-release-1793567475847","runId":"pr4130-86216483fb","providerSessionId":"942bd37d58a28bb67efc75b73f15d7569e60896c","cleanup":"confirmed","hint":"Allocate the lease again; agent-device stopped the session this allocation created."}}
{"at":"2026-11-01T21:11:37.278Z","event":"teardown outcome","by":"release","status":"resolved","value":{"provider":"browserstack","providerSessionId":"942bd37d58a28bb67efc75b73f15d7569e60896c"}}
{"at":"2026-11-01T21:11:37.313Z","event":"post-delete hub probe","by":"release","providerSessionId":"942bd37d58a28bb67efc75b73f15d7569e60896c","status":404}
{"at":"2026-11-01T21:11:37.313Z","event":"run PASS","head":"86216483fb980ab3d9c6fe6ddd5a64b6cfa60041","by":"release","created":["942bd37d58a28bb67efc75b73f15d7569e60896c"],"deleted":["942bd37d58a28bb67efc75b73f15d7569e60896c"],"live":[],"liveCount":0}
{"at":"2026-11-01T21:11:37.313Z","event":"final owned-session accounting","by":"release","created":["942bd37d58a28bb67efc75b73f15d7569e60896c"],"deleted":["942bd37d58a28bb67efc75b73f15d7569e60896c"],"live":[]}
{"at":"2026-11-01T21:11:37.313Z","event":"run start","head":"86216483fb980ab3d9c6fe6ddd5a64b6cfa60041","by":"shutdown","leaseId":"sm3-4130-shutdown-1793567497313","device":"Samsung Galaxy S21","osVersion":"11","route":"createCloudWebDriverRuntime.leaseLifecycle","preparation":"no-app capabilities; no app upload","transport":"native fetch to real BrowserStack; no held responses or fake hub"}
{"at":"2026-11-01T21:11:37.314Z","event":"POST /session sent; response pending","by":"shutdown"}
{"at":"2026-11-01T21:11:37.414Z","event":"teardown invoked","by":"shutdown","postPending":true}
{"at":"2026-11-01T21:11:37.420Z","event":"same-lease allocate invoked","by":"shutdown","postPending":true,"teardownSettled":false}
{"at":"2026-11-01T21:11:37.420Z","event":"racing allocate outcome","by":"shutdown","status":"rejected","code":"COMMAND_FAILED","details":{"reason":"provider_session_shutdown_during_allocation","provider":"browserstack","leaseId":"sm3-4130-shutdown-1793567497313","runId":"pr4130-86216483fb","cleanup":"pending","hint":"agent-device is still stopping the session this allocation created. Wait for that shutdown to finish before allocating the lease again."},"postPending":true,"teardownSettled":false}
{"at":"2026-11-01T21:11:58.970Z","event":"POST /session answered","by":"shutdown","status":200,"providerSessionId":"9037ba0b8cdd9a77879f19eb9839e46f61c89f1b"}
{"at":"2026-11-01T21:11:58.971Z","event":"DELETE /session sent","by":"shutdown","providerSessionId":"9037ba0b8cdd9a77879f19eb9839e46f61c89f1b"}
{"at":"2026-11-01T21:11:59.108Z","event":"DELETE /session answered","by":"shutdown","status":200}
{"at":"2026-11-01T21:11:59.108Z","event":"first allocate outcome","by":"shutdown","status":"rejected","code":"COMMAND_FAILED","details":{"reason":"provider_session_shutdown_during_allocation","provider":"browserstack","leaseId":"sm3-4130-shutdown-1793567497313","runId":"pr4130-86216483fb","providerSessionId":"9037ba0b8cdd9a77879f19eb9839e46f61c89f1b","cleanup":"confirmed","hint":"Allocate the lease again; agent-device stopped the session this allocation created."}}
{"at":"2026-11-01T21:11:59.108Z","event":"teardown outcome","by":"shutdown","status":"resolved"}
{"at":"2026-11-01T21:11:59.144Z","event":"post-delete hub probe","by":"shutdown","providerSessionId":"9037ba0b8cdd9a77879f19eb9839e46f61c89f1b","status":404}
{"at":"2026-11-01T21:11:59.144Z","event":"run PASS","head":"86216483fb980ab3d9c6fe6ddd5a64b6cfa60041","by":"shutdown","created":["9037ba0b8cdd9a77879f19eb9839e46f61c89f1b"],"deleted":["9037ba0b8cdd9a77879f19eb9839e46f61c89f1b"],"live":[],"liveCount":0}
{"at":"2026-11-01T21:11:59.144Z","event":"final owned-session accounting","by":"shutdown","created":["9037ba0b8cdd9a77879f19eb9839e46f61c89f1b"],"deleted":["9037ba0b8cdd9a77879f19eb9839e46f61c89f1b"],"live":[]}Complete exact-head validation receipt{
"version": 1,
"cwd": "/Users/thymikee/.t3/worktrees/agent-device/t3-c4bef626",
"startedAt": "2026-11-01T21:12:53.309Z",
"status": "passed",
"exitCode": 0,
"refs": {
"base": "origin/main",
"head": "e094faebb3",
"resolvedBase": "343b7fcf41e2514038ad52d07aec51169e0a750e",
"resolvedHead": "e094faebb3a6899a89b95410647fa9331ad33fa7",
"mergeBase": "f7879cf2d266ee1dd476d0342b2819bdeceb647b"
},
"checkoutChanged": false,
"exactCommit": true,
"results": [
{
"id": "format",
"status": "passed",
"command": [
"pnpm",
"run",
"format:check"
],
"logPath": "/Users/thymikee/.t3/worktrees/agent-device/t3-c4bef626/.tmp/validation/run-NzjuKc/0.log",
"exitCode": 0,
"durationMs": 1140,
"testReportPaths": [],
"tests": []
},
{
"id": "lint",
"status": "passed",
"command": [
"pnpm",
"run",
"lint"
],
"logPath": "/Users/thymikee/.t3/worktrees/agent-device/t3-c4bef626/.tmp/validation/run-NzjuKc/1.log",
"exitCode": 0,
"durationMs": 3486,
"testReportPaths": [],
"tests": []
},
{
"id": "typecheck",
"status": "passed",
"command": [
"pnpm",
"run",
"typecheck"
],
"logPath": "/Users/thymikee/.t3/worktrees/agent-device/t3-c4bef626/.tmp/validation/run-NzjuKc/2.log",
"exitCode": 0,
"durationMs": 1904,
"testReportPaths": [],
"tests": []
},
{
"id": "layering",
"status": "passed",
"command": [
"pnpm",
"run",
"check:layering"
],
"logPath": "/Users/thymikee/.t3/worktrees/agent-device/t3-c4bef626/.tmp/validation/run-NzjuKc/3.log",
"exitCode": 0,
"durationMs": 33227,
"testReportPaths": [],
"tests": []
},
{
"id": "fallow",
"status": "passed",
"command": [
"pnpm",
"run",
"check:fallow",
"--base",
"343b7fcf41e2514038ad52d07aec51169e0a750e"
],
"logPath": "/Users/thymikee/.t3/worktrees/agent-device/t3-c4bef626/.tmp/validation/run-NzjuKc/4.log",
"exitCode": 0,
"durationMs": 13680,
"testReportPaths": [],
"tests": []
},
{
"id": "build",
"status": "passed",
"command": [
"pnpm",
"run",
"build"
],
"logPath": "/Users/thymikee/.t3/worktrees/agent-device/t3-c4bef626/.tmp/validation/run-NzjuKc/5.log",
"exitCode": 0,
"durationMs": 10134,
"testReportPaths": [],
"tests": []
},
{
"id": "vitest-related",
"status": "passed",
"command": [
"pnpm",
"exec",
"vitest",
"related",
"--run",
"--passWithNoTests",
"--project=!unit-core",
"--project=!fuzz-worker",
"packages/provider-webdriver/src/profile-fields.fixtures.ts",
"packages/provider-webdriver/src/runtime-session-allocation.test.ts",
"packages/provider-webdriver/src/runtime-session.ts",
"packages/provider-webdriver/src/runtime.ts"
],
"logPath": "/Users/thymikee/.t3/worktrees/agent-device/t3-c4bef626/.tmp/validation/run-NzjuKc/6.log",
"exitCode": 0,
"durationMs": 56467,
"testReportPaths": [
"/Users/thymikee/.t3/worktrees/agent-device/t3-c4bef626/.tmp/validation/run-NzjuKc/6.tests.28070.json"
],
"tests": [
{
"passed": 304,
"failed": 0,
"skipped": 0,
"pending": 0,
"failedModules": 0,
"unhandledErrors": 0,
"failures": [],
"omittedFailures": 0
}
]
},
{
"id": "unit",
"status": "passed",
"command": [
"pnpm",
"run",
"check:unit"
],
"logPath": "/Users/thymikee/.t3/worktrees/agent-device/t3-c4bef626/.tmp/validation/run-NzjuKc/7.log",
"exitCode": 0,
"durationMs": 254131,
"testReportPaths": [
"/Users/thymikee/.t3/worktrees/agent-device/t3-c4bef626/.tmp/validation/run-NzjuKc/7.tests.36047.json"
],
"tests": [
{
"passed": 16762,
"failed": 0,
"skipped": 1,
"pending": 0,
"failedModules": 0,
"unhandledErrors": 0,
"failures": [],
"omittedFailures": 0
}
]
},
{
"id": "web-smoke",
"status": "skipped",
"reason": "GitHub-authoritative (jobs: Apple / Smoke Tests, Integration Tests, Lint, Typecheck & Package, Perf Nightly / Android Command Perf, Perf Nightly / iOS Command Perf)"
},
{
"id": "replay-ios",
"status": "skipped",
"reason": "parked, workflow_dispatch only (Replay Manual / iOS Replay Suite)"
},
{
"id": "replay-ios-device",
"status": "skipped",
"reason": "parked, workflow_dispatch only (Replay Manual / iOS Replay Suite)"
},
{
"id": "replay-macos",
"status": "skipped",
"reason": "GitHub-authoritative (jobs: Apple / Smoke Tests)"
},
{
"id": "replay-linux",
"status": "skipped",
"reason": "GitHub-authoritative (jobs: Linux / Smoke Tests)"
},
{
"id": "linux-command-evidence",
"status": "skipped",
"reason": "GitHub-authoritative (jobs: Linux / Smoke Tests)"
},
{
"id": "replay-android",
"status": "skipped",
"reason": "parked, workflow_dispatch only (Replay Manual / Android Full Emulator Suite)"
}
],
"initial": {
"head": "e094faebb3a6899a89b95410647fa9331ad33fa7",
"status": "",
"fingerprint": "e292d347bb696042f040fc1394e4e87be89d052e96cfcbcd6a4964d2b2ca3190"
},
"changedFiles": [
"packages/provider-webdriver/src/profile-fields.fixtures.ts",
"packages/provider-webdriver/src/runtime-session-allocation.test.ts",
"packages/provider-webdriver/src/runtime-session.ts",
"packages/provider-webdriver/src/runtime.ts"
],
"plan": {
"failOpen": false,
"checks": [
"format",
"lint",
"typecheck",
"layering",
"fallow",
"build",
"vitest-related",
"unit",
"web-smoke",
"replay-ios",
"replay-ios-device",
"replay-macos",
"replay-linux",
"linux-command-evidence",
"replay-android"
],
"reasons": [
{
"check": "format",
"path": "packages/provider-webdriver/src/profile-fields.fixtures.ts",
"rule": "gate:format",
"detail": "oxfmt covers packages/"
},
{
"check": "lint",
"path": "packages/provider-webdriver/src/profile-fields.fixtures.ts",
"rule": "gate:lint",
"detail": "oxlint covers packages/"
},
{
"check": "typecheck",
"path": "packages/provider-webdriver/src/profile-fields.fixtures.ts",
"rule": "gate:typecheck",
"detail": "tsc includes packages/"
},
{
"check": "fallow",
"path": "packages/provider-webdriver/src/profile-fields.fixtures.ts",
"rule": "gate:fallow",
"detail": "fallow audits changed TypeScript for dead code"
},
{
"check": "layering",
"path": "packages/provider-webdriver/src/profile-fields.fixtures.ts",
"rule": "package-src",
"detail": "layering R11 guards workspace package boundaries"
},
{
"check": "vitest-related",
"path": "packages/provider-webdriver/src/profile-fields.fixtures.ts",
"rule": "vitest:related",
"detail": "Vitest resolves affected tests through its static module graph"
},
{
"check": "build",
"path": "packages/provider-webdriver/src/profile-fields.fixtures.ts",
"rule": "package-src",
"detail": "package source is bundled into the published artifact"
},
{
"check": "unit",
"path": "packages/provider-webdriver/src/profile-fields.fixtures.ts",
"rule": "own:unit-text-gates",
"detail": "unit-suite gates read production sources as text, which the Vitest module graph cannot trace"
},
{
"check": "replay-ios",
"path": "packages/provider-webdriver/src/profile-fields.fixtures.ts",
"rule": "own:device-lane:shared",
"detail": "shared runtime surface: every device lane drives the CLI and daemon through it"
},
{
"check": "replay-ios-device",
"path": "packages/provider-webdriver/src/profile-fields.fixtures.ts",
"rule": "own:device-lane:shared",
"detail": "shared runtime surface: every device lane drives the CLI and daemon through it"
},
{
"check": "replay-macos",
"path": "packages/provider-webdriver/src/profile-fields.fixtures.ts",
"rule": "own:device-lane:shared",
"detail": "shared runtime surface: every device lane drives the CLI and daemon through it"
},
{
"check": "replay-android",
"path": "packages/provider-webdriver/src/profile-fields.fixtures.ts",
"rule": "own:device-lane:shared",
"detail": "shared runtime surface: every device lane drives the CLI and daemon through it"
},
{
"check": "replay-linux",
"path": "packages/provider-webdriver/src/profile-fields.fixtures.ts",
"rule": "own:device-lane:shared",
"detail": "shared runtime surface: every device lane drives the CLI and daemon through it"
},
{
"check": "linux-command-evidence",
"path": "packages/provider-webdriver/src/profile-fields.fixtures.ts",
"rule": "own:device-lane:shared",
"detail": "shared runtime surface: every device lane drives the CLI and daemon through it"
},
{
"check": "web-smoke",
"path": "packages/provider-webdriver/src/profile-fields.fixtures.ts",
"rule": "own:device-lane:shared",
"detail": "shared runtime surface: every device lane drives the CLI and daemon through it"
},
{
"check": "format",
"path": "packages/provider-webdriver/src/runtime-session-allocation.test.ts",
"rule": "gate:format",
"detail": "oxfmt covers packages/"
},
{
"check": "lint",
"path": "packages/provider-webdriver/src/runtime-session-allocation.test.ts",
"rule": "gate:lint",
"detail": "oxlint covers packages/"
},
{
"check": "typecheck",
"path": "packages/provider-webdriver/src/runtime-session-allocation.test.ts",
"rule": "gate:typecheck",
"detail": "tsc includes packages/"
},
{
"check": "fallow",
"path": "packages/provider-webdriver/src/runtime-session-allocation.test.ts",
"rule": "gate:fallow",
"detail": "fallow audits changed TypeScript for dead code"
},
{
"check": "layering",
"path": "packages/provider-webdriver/src/runtime-session-allocation.test.ts",
"rule": "package-src",
"detail": "layering R11 guards workspace package boundaries"
},
{
"check": "vitest-related",
"path": "packages/provider-webdriver/src/runtime-session-allocation.test.ts",
"rule": "vitest:related",
"detail": "Vitest resolves affected tests through its static module graph"
},
{
"check": "format",
"path": "packages/provider-webdriver/src/runtime-session.ts",
"rule": "gate:format",
"detail": "oxfmt covers packages/"
},
{
"check": "lint",
"path": "packages/provider-webdriver/src/runtime-session.ts",
"rule": "gate:lint",
"detail": "oxlint covers packages/"
},
{
"check": "typecheck",
"path": "packages/provider-webdriver/src/runtime-session.ts",
"rule": "gate:typecheck",
"detail": "tsc includes packages/"
},
{
"check": "fallow",
"path": "packages/provider-webdriver/src/runtime-session.ts",
"rule": "gate:fallow",
"detail": "fallow audits changed TypeScript for dead code"
},
{
"check": "layering",
"path": "packages/provider-webdriver/src/runtime-session.ts",
"rule": "package-src",
"detail": "layering R11 guards workspace package boundaries"
},
{
"check": "vitest-related",
"path": "packages/provider-webdriver/src/runtime-session.ts",
"rule": "vitest:related",
"detail": "Vitest resolves affected tests through its static module graph"
},
{
"check": "build",
"path": "packages/provider-webdriver/src/runtime-session.ts",
"rule": "package-src",
"detail": "package source is bundled into the published artifact"
},
{
"check": "unit",
"path": "packages/provider-webdriver/src/runtime-session.ts",
"rule": "own:unit-text-gates",
"detail": "unit-suite gates read production sources as text, which the Vitest module graph cannot trace"
},
{
"check": "replay-ios",
"path": "packages/provider-webdriver/src/runtime-session.ts",
"rule": "own:device-lane:shared",
"detail": "shared runtime surface: every device lane drives the CLI and daemon through it"
},
{
"check": "replay-ios-device",
"path": "packages/provider-webdriver/src/runtime-session.ts",
"rule": "own:device-lane:shared",
"detail": "shared runtime surface: every device lane drives the CLI and daemon through it"
},
{
"check": "replay-macos",
"path": "packages/provider-webdriver/src/runtime-session.ts",
"rule": "own:device-lane:shared",
"detail": "shared runtime surface: every device lane drives the CLI and daemon through it"
},
{
"check": "replay-android",
"path": "packages/provider-webdriver/src/runtime-session.ts",
"rule": "own:device-lane:shared",
"detail": "shared runtime surface: every device lane drives the CLI and daemon through it"
},
{
"check": "replay-linux",
"path": "packages/provider-webdriver/src/runtime-session.ts",
"rule": "own:device-lane:shared",
"detail": "shared runtime surface: every device lane drives the CLI and daemon through it"
},
{
"check": "linux-command-evidence",
"path": "packages/provider-webdriver/src/runtime-session.ts",
"rule": "own:device-lane:shared",
"detail": "shared runtime surface: every device lane drives the CLI and daemon through it"
},
{
"check": "web-smoke",
"path": "packages/provider-webdriver/src/runtime-session.ts",
"rule": "own:device-lane:shared",
"detail": "shared runtime surface: every device lane drives the CLI and daemon through it"
},
{
"check": "format",
"path": "packages/provider-webdriver/src/runtime.ts",
"rule": "gate:format",
"detail": "oxfmt covers packages/"
},
{
"check": "lint",
"path": "packages/provider-webdriver/src/runtime.ts",
"rule": "gate:lint",
"detail": "oxlint covers packages/"
},
{
"check": "typecheck",
"path": "packages/provider-webdriver/src/runtime.ts",
"rule": "gate:typecheck",
"detail": "tsc includes packages/"
},
{
"check": "fallow",
"path": "packages/provider-webdriver/src/runtime.ts",
"rule": "gate:fallow",
"detail": "fallow audits changed TypeScript for dead code"
},
{
"check": "layering",
"path": "packages/provider-webdriver/src/runtime.ts",
"rule": "package-src",
"detail": "layering R11 guards workspace package boundaries"
},
{
"check": "vitest-related",
"path": "packages/provider-webdriver/src/runtime.ts",
"rule": "vitest:related",
"detail": "Vitest resolves affected tests through its static module graph"
},
{
"check": "build",
"path": "packages/provider-webdriver/src/runtime.ts",
"rule": "package-src",
"detail": "package source is bundled into the published artifact"
},
{
"check": "unit",
"path": "packages/provider-webdriver/src/runtime.ts",
"rule": "own:unit-text-gates",
"detail": "unit-suite gates read production sources as text, which the Vitest module graph cannot trace"
},
{
"check": "replay-ios",
"path": "packages/provider-webdriver/src/runtime.ts",
"rule": "own:device-lane:shared",
"detail": "shared runtime surface: every device lane drives the CLI and daemon through it"
},
{
"check": "replay-ios-device",
"path": "packages/provider-webdriver/src/runtime.ts",
"rule": "own:device-lane:shared",
"detail": "shared runtime surface: every device lane drives the CLI and daemon through it"
},
{
"check": "replay-macos",
"path": "packages/provider-webdriver/src/runtime.ts",
"rule": "own:device-lane:shared",
"detail": "shared runtime surface: every device lane drives the CLI and daemon through it"
},
{
"check": "replay-android",
"path": "packages/provider-webdriver/src/runtime.ts",
"rule": "own:device-lane:shared",
"detail": "shared runtime surface: every device lane drives the CLI and daemon through it"
},
{
"check": "replay-linux",
"path": "packages/provider-webdriver/src/runtime.ts",
"rule": "own:device-lane:shared",
"detail": "shared runtime surface: every device lane drives the CLI and daemon through it"
},
{
"check": "linux-command-evidence",
"path": "packages/provider-webdriver/src/runtime.ts",
"rule": "own:device-lane:shared",
"detail": "shared runtime surface: every device lane drives the CLI and daemon through it"
},
{
"check": "web-smoke",
"path": "packages/provider-webdriver/src/runtime.ts",
"rule": "own:device-lane:shared",
"detail": "shared runtime surface: every device lane drives the CLI and daemon through it"
}
],
"failOpenReasons": [],
"docsOnlyPaths": []
},
"final": {
"head": "e094faebb3a6899a89b95410647fa9331ad33fa7",
"status": "",
"fingerprint": "e292d347bb696042f040fc1394e4e87be89d052e96cfcbcd6a4964d2b2ca3190"
},
"finishedAt": "2026-11-01T22:22:21.378Z"
} |
|
I reviewed e094fae and found no code problems. The earlier review at 8621648 asked for live evidence on the facade route, and the author-supplied runs now cover it, so that request is met. The delta since 8621648 changes comments only in runtime-session.ts and runtime.ts. The live runs are author logs from a harness I could not rerun, and I did not query BrowserStack to confirm both session ids are ended. They exercise createCloudWebDriverRuntime with no-app Android capabilities, not daemon dispatch or the app-upload prepareSession path. They ran at 8621648, which is fine here because the delta is comments only. I did not run the tests myself. On the open inline threads: the failed-create cleanup thread (#4130 (comment)) still applies, and two lower-priority threads still apply. These threads do not apply and can be resolved: #4130 (comment) (the post-shutdown refusal carries cleanup 'none' and its own restart hint), #4130 (comment) (a shutdown() rejection lands in plugins.failed and other plugins still close), #4130 (comment) (the owned-id add runs before shutdown's clear), #4130 (comment) ('uncertain' after a failed hand-back is deliberate), #4130 (comment) (the duplicate 30_000 value is the same and deferred), #4130 (comment) (each cleanupWarnings shape is lossless for its case), and #4130 (comment) (the docs now match release()). Smoke Tests, Repo Guards & Compatibility and Coverage are still running. All 19 checks were green at 8621648, and no code on their routes changed since. Once those three finish green at e094fae, nothing else stands in the way of merge. |
thymikee
added
the
ready-for-human
Summary
Concurrent allocations of one lease could rent two billed WebDriver sessions, and teardown during creation could orphan a session. The manager now publishes one lease entry synchronously, shares its creation and disposal, refuses allocations during teardown, and reports cleanup outcomes with session identities.
Provider callback budgets cover prepared-session cleanup on disposal and failed creation, plus teardown
listArtifacts.prepareSessionremains unbounded; release and shutdown wait for preparation. Release awaits artifacts even when it joins a shutdown take; freeing the lease slot andshutdown()await disposal only.Node option:
createCloudWebDriverRuntime({ ...options, providerCallbackTimeoutMs: 5000 }).Four files in
packages/provider-webdriver; no scope expansion. SM-3; related #4102.Validation
Tested head
e094faebb3a6899a89b95410647fa9331ad33fa7; basef7879cf2d266ee1dd476d0342b2819bdeceb647b.pnpm formatandpnpm check:affected --run --head e094faebb3a6899a89b95410647fa9331ad33fa7passed (exactCommit: true); receipt.tmp/validation/run-NzjuKc/receipt.json.Both real BrowserStack races passed at
86216483fb: release and shutdown during unanswered POST /session, followed by same-lease allocation. Each created/deleted the same session ID, left zero owned sessions live, and returned the expected typed refusals. Full logs and validation receipt: review response.This follow-up changes comments only; runtime behavior is unchanged from the live-tested head. Existing fail-first results remain in earlier review responses. GitHub owns full coverage, provider integration, and device lanes. Limitation: no-app Android sessions; no app-upload path was exercised. Independent review remains with the coordinator.