fix(daemon): let admitted work keep the lease it is working on - #2517
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
The new in-flight registry leaks entries under lease churn at f6f7b05. lease-in-flight-work.ts:60 removes a completed pass from its set but leaves the empty map entry; explicit lease release then removes the lease before expiry can call isDeferred to clean that entry. Each completed, explicitly released lease leaves another permanent key in the daemon. Delete empty entries when releasing work and cover the normal lease-release path. Current checks pass and there are no conflicts; this layer also depends on #2516. |
f6f7b05 to
b0cdafc
Compare
|
Fixed in Covered at both ends: releasing the last pass leaves no key recorded, |
b0cdafc to
5fe9166
Compare
5fe9166 to
f00fe88
Compare
|
The lease cleanup fix is clean on f00fe88: releasing the last pass removes the empty map entry, and forgetting a lease disarms retained passes. The reported regression tests cover those cases. This layer still depends on the documentation correction in #2516. Current CI also contains cancelled Smoke Tests and Bundle Size runs, so it is not fully green. |
A lease renewed only at admission, so a command slower than its own inactivity TTL expired the lease that was paying for the device it was using. Expiry then tore the provider session down underneath a client still waiting for that same command's result, and every later command on the session reported a lease that was no longer active. The session's own work was the thing that killed it. Admitted work now preserves its lease the way a human-control hold does: while the request is still wanted it defers expiry, and finishing while still wanted renews the lease for its existing TTL from the moment the work ended. Work whose client hung up preserves nothing — it neither defers expiry past that cancellation nor renews the lease when it finally lands — so a handler that ignores its cancellation cannot hold a rented device open. Found while investigating #2509. Not its reported mechanism: a cloud WebDriver connection profile asks for a ten-minute lease, so a one-minute hang cannot starve it. This reaches leases on the daemon's one-minute default.
…hem empty A completed pass emptied its set but left the key behind, and only the expiry sweep removed keys — which never reads a released lease again. Every connect-and-close that ran a command on a leased device left another permanent entry in the daemon. Releasing the last pass now removes its lease's entry, and releasing the lease drops its claims outright and disarms the passes still running on them, so work that outlives its own lease renews nothing.
Four comments told the report's story as though it were this mechanism. A cloud WebDriver connection profile asks for a ten-minute lease, so the reported one-minute hang cannot have expired anything. The invariant stands on its own; where it came from and which leases it reaches belong in ADR 0007 and the commit, not in each test.
f00fe88 to
25c563d
Compare
|
The dependency is closed: #2516 carries the bounded paragraph at On the cancelled runs — those are this stack's own force-pushes cancelling their predecessors, not |
|
No actionable findings on 25c563d. This is a byte-identical rebase of the reviewed lease changes, and #2516 has corrected its documentation. The reported focused regressions still apply. Current CI is running; the cancelled predecessor runs do not establish a failure on this head. Merge readiness awaits current checks and the prerequisite. |
thymikee
added
the
ready-for-human
|
1 participant
Summary
A lease renewed only at admission, so a command slower than its own inactivity TTL expired the lease
that was paying for the device it was using. Expiry then tore the provider session down underneath a
client still waiting for that same command's result, and every later command reported a lease that was
no longer active.
Admitted work now preserves its lease the way a human-control hold does: while the request is still
wanted it defers expiry, and finishing while still wanted renews the lease for its existing TTL from
the moment the work ended. Work whose client hung up preserves nothing — it neither defers expiry past
that cancellation nor renews the lease when it lands. ADR 0007 records the rule.
Found while investigating #2509, and not its mechanism: a cloud WebDriver profile asks for a
ten-minute lease, so a one-minute hang cannot starve it. This reaches the daemon's one-minute default.
Depends on #2516 through shared fixtures.
Validation
pnpm check:affected --runpassed at25c563d02(all runnable checks).npx vitest run src/daemon/__tests__/{lease-registry,lease-in-flight-work,request-lease-work,request-execution-scope}.test.ts— 67 passed. Red without the layer:
an admitted request that outlives the lease TTL keeps its lease and session,work nobody waited for renews nothing once its lease fell due.Review round: a completed pass emptied its set but left the map key, and only the expiry sweep
removed keys — which never reads a released lease again, so every connect-and-close leaked one.
Releasing the last pass now removes the entry, and releasing a lease drops its claims and disarms the
passes still running on them. All three new tests fail at the previous head and pass here.
Live check that set this scope: on AWS Device Farm a 66-second command kept its lease on the pre-fix
build, which is what ruled this layer out as the snapshot -i hangs on AWS Device Farm Android when the screen plays looping video, then the whole session freezes #2509 cause.
Remaining risk: a handler that ignores cancellation while holding the session lock now keeps the
lease open instead of expiring it mid-command.
Second round:
25c563d02is comment-only — it restates the invariant without borrowing snapshot -i hangs on AWS Device Farm Android when the screen plays looping video, then the whole session freezes #2509's cause, which a ten-minute cloud WebDriver lease rules out for that report.