fix(remote): renew the lease while an artifact upload runs (#2946) - #2990
Conversation
Size Report
Startup median (7 runs, lower is better):
|
![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 6 files
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 20 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
The core fix does not close the gap #2946 describes. Each beat is sent with the lease_heartbeat timeout policy (90s, https://github.com/callstack/agent-device/blob/410711c/src/daemon-client/daemon-client.ts#L535), and beats never overlap: the next one is only armed after the previous one settles (https://github.com/callstack/agent-device/blob/410711c/src/daemon-client/daemon-client.ts#L401). If one beat stalls on a half-open connection or a stuck daemon, no successor is armed for up to 90s, and the lease's 60s window lapses while the upload keeps running. On the success path, No test exercises sendToDaemon, which is the route users actually hit; if line 70's call were reverted to the pre-fix signal, the suite would stay green (https://github.com/callstack/agent-device/blob/410711c/src/daemon-client/daemon-client.ts#L70). runProtectedLeaseWork, buildUploadLeaseHeartbeat, and uploadArtifact are each tested in isolation, and the comment at line 524 saying the cadence is too slow for an end-to-end test looks stale now that the first beat fires at t=0. Can we add a loopback test that drives an install through sendToDaemon against a fake remote daemon: hold the upload response until a lease.heartbeat RPC with the lease id arrives and confirm it precedes the install RPC, then a second case where the heartbeat answers UNAUTHORIZED/LEASE_NOT_FOUND and confirm sendToDaemon rejects with that reason, the upload socket is destroyed, and no install RPC goes out? That test would also let the stale comment be removed. Not blocking: proxy leases allocated without ttlMs now keep the 60s registry default instead of the old 5-minute extension, which only ADR 0007 records while remote-proxy.md still promises five minutes for CLI connect proxy, so the docs could use one clarifying sentence; LOST_LEASE_BEAT_REASONS and isInactiveLeaseError diverge and the set includes LEASE_SESSION_MISMATCH, which admission-exempt heartbeats can never actually receive; daemon-client.ts has grown to 561 lines with four exports that exist only for tests, which could move to their own module alongside its tests; the JSDoc and ADR 0007 claim a missing lease is caught "before any bytes move," but hashing, preflight, and the start of the stream can run before the first beat answers; and after a beat's terminal error races the task, the code doesn't check the abort signal again once artifact preparation finishes, so an optional signal check there would tighten the abort window; all of these can be taken or left. Is there a smaller shape than a dedicated lease-renewal module here? One general beat loop that enforces "budget ends before the window does" could live in one place and be shared with the similar setInterval-plus-in-flight loop in src/cli/commands/takeover.ts, instead of adding more test-only exports to daemon-client.ts — worth doing now or later? I did not run the changed remote-install upload path live; that still needs |
410711c to
612909b
Compare
|
|
This is a follow-up on the earlier review (410711c, #2990 (comment)): d9b7af7 still has a blocking finding. The fix makes one number do two jobs. In Not blocking: the lease-beat tests using a heartbeat fake that ignores Coverage failing looks caused by this PR: the eager-closure log shows a new static edge, Before this can merge: budget each beat from the lease window rather than the 1s cadence floor and add the slow-beat test through |
The extraction that moved the beat out of daemon-client.ts put it in cli.ts's eager closure, which every command pays for, and took it from 295 modules to 296 (#2990's coverage failure). The beat is only ever useful to one kind of request: one heading to a remote daemon that names a lease. Both guards are cheap and local — the lease scope comes off the request the caller already built — so the module now loads behind them and the closure is back to 295. Also folds the beat's thin wrappers away from the entry surface, which is what the four test-only exports in daemon-client.ts had become: createLeaseRenewalBeat and leaseScopeForHeartbeat existed only for tests, the scope precedence they pinned is already covered where it lives in contracts, and the one invariant that wasn't covered anywhere — a beat per request id, so a timed-out beat cannot cancel its successor — is now pinned over the real transport, where a shared id is actually observable.
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
|
Both blockers addressed, plus the 15:35 follow-up. Budget from the window, not the cadence (15:35). You were right that one number did two jobs, and the failure is worse than I read: the 1s floor budget meant every beat on a slow link timed out, never raised the cadence, and logged a diagnostic pair forever. Cadence and budget are separate now — armed every third of the window, allowed the whole window to answer — and Tested through Lazy load. Live run, both scenarios, local daemon over its HTTP baseUrl with a loopback proxy that throttles uploads and rewrites the upload origin the daemon advertises (it names its own port, which would otherwise bypass the slow link). Lease What the live run does not show: the install succeeding. The Android artifact is the one that uploads, and no Android device is attached here, so the request ends at Taken from the non-blocking list: the docs sentence, Left: the post-prepare abort check. On |
|
Reviewed at 4c74a43. The concerns from the earlier pass at d9b7af7 are addressed: cadence and per-beat budget are now separate, so a slow first answer sets the cadence instead of timing out, and the eager-closure edge is gone. I found nothing else in the code that should block this. Coverage is red only on the new Next step: give Not blocking: could |
thymikee
added
the
ready-for-human
|
Reviewed at 58a6110. The delta since 4c74a43 looks correct: the lease-beat reorder keeps the beat lifecycle intact, and the new provider-integration test covers it. No code issues. Smoke Tests was still running at review time. This PR only touches the beat module on a leased remote install upload, which the smoke route does not exercise, so I do not expect an overlap. I did not run the integration test locally; the timing reading comes from the code. Not blocking: the |
|
Fixed in
Both non-blocking notes taken, in
On CI: the iOS smoke job also drew |
|
Reviewed c10e3a8. The lease-renewal fix still looks correct. This delta only changes ADR 0007 and the upload lease-beat integration test, so no production code changed since the clean review at 58a6110, and the ADR text matches the shipped timeout ceiling. I did not run the changed integration test on a loaded runner, so I can't confirm the new CI is green. |
…ready carries `heartbeatLease` resolved an absent `ttlMs` through the registry's default resolver, so a caller that heartbeats without repeating its allocation TTL silently shortened the lease to the daemon default. Every renewal that is not asked to change the window now renews for the window the lease is living on, which is the rule `refreshProtectedLease` already applied to protected work; that arithmetic moves to `leaseOwnTtlMs` beside the other lease-scope rules so both callers read one definition. The caller that heartbeats without a TTL is not asking for the default, it is asking for the same lease to keep going. An admitted request does exactly that, and this is half of why a long upload expired the lease paying for its own device (#2946).
…ler's side A remote install uploaded the artifact from the caller before the install request was admitted, so nothing renewed the lease while the bytes moved: the daemon protects a lease while admitted work runs on it (#2509, ADR 0007), and an upload is the mirror image of that. A 449 MB APK that took 1m47s to upload against the one-minute default TTL expired the lease paying for the device it was uploading to, and the install then failed `Lease is not active` (#2946). `sendToDaemon` now brackets the upload phase with beats over the same transport the command uses. A beat names the command's lease scope, its own request id, and nothing else: the scope is what the daemon needs, and reusing the install request would send the upload's own payload once per beat. Each beat gets a fresh id because a beat that times out is canceled under its own. Only a remote daemon uploads and only a remote daemon holds a billed device, so a local command and a command that names no lease get no timer at all, and the interval starts rather than fires immediately, so an install that beats never sends no extra request. A beat that finds the lease gone ends the upload with that lease error rather than finishing bytes to a device nobody owns; a beat that fails for any other reason is reported through diagnostics and survived, since a later beat covers one lost request. ADR 0007 gains the rule this closes: what protects work that happens before admission.
Admission named its own proxy default on every request, so a client that rented a device for longer than that default lost the window on the next command — the lease-side twin of the heartbeat bug in #2946, which the client fix could not reach because the shortening happened at admission. The window a lease carries is the one its client named when it allocated; only a request naming its own window changes it now, which retires DEFAULT_PROXY_LEASE_TTL_MS with no producer left.
The upload beat waited a fixed twenty seconds for its first renewal, so a lease admitted with a shorter window lapsed while a perfectly valid upload ran — the #2946 symptom one seam earlier. A beat answers with the window it just renewed, so the phase now beats immediately and then a third of that window after each beat lands: a beat slower than the cadence delays its successor instead of replacing the schedule or silencing every beat behind it, and a floor keeps a pathological window from becoming a request loop. A beat refused for a missing or mismatched owner scope says this request can never renew the lease, so retrying it only spent the upload against a lease that had stopped renewing. Those reasons now end the phase beside the lost-lease ones, and the phase hands its task an abort signal the upload chain honours: preflight, finalize, the direct PUT, and the legacy stream all run under the caller's cancellation combined with their own timeout, and a canceled upload stops asking the daemon for a fresh ticket. node:http's own request signal is what stops bytes already piped at a device nobody holds.
Every moved declaration is a client-side optional parameter — the request bytes an older daemon parses are unchanged, and an aborted request is the client disconnect it already handles — so each gets its own digest-keyed compatibleChanges entry rather than a protocol bump. AbortSignal joins TS_GLOBALS: a lib global with no declaration site to digest, like URL.
…inal The beat's scope and window are fixed where it is built, so a daemon that refuses them — a ttl outside [minLeaseTtlMs, maxLeaseTtlMs], an unusable lease id — refuses every successor identically. Such a refusal carries no reason to key on, so the code is the signal: end the phase the way a lost lease does instead of spending the upload against a lease that stopped renewing on the first beat.
…on test The byte counts are what prove the stream stopped early; waiting for the server to see the socket close added a second promise that only loopback timing could settle, and under a loaded coverage lane it never did inside the test timeout. A smaller payload keeps the same pause-and-abort scenario off the CPU.
The contract on the upload signal is that an aborted upload rejects with the signal's own reason. The preflight leg honoured it and the stream leg did not: node:http reports an aborted request as a transport error, and the shared handler wrapped that into COMMAND_FAILED. That is not just the wrong message — a wrapped cancellation is indistinguishable from a broken transport, so the direct-upload retry policy is one refactor away from re-preflighting for a fresh ticket after the caller asked for none. Finalize wrapped the same way. Both tests now pin the rejection instead of accepting anything, and the admission test pins heartbeatAt so the window assertion cannot pass on a lease that was never renewed.
finalizeDirectUpload and streamFileToHttpRequestAttempt changed shape; both edits are client-side only — the bytes a protocol-2 daemon parses for an uncancelled request are unchanged, and a canceled request is the disconnect it already handles.
…real route A beat was sent under the lease_heartbeat command policy's 90s timeout and its successor was armed only after it settled, so one stalled round trip stopped the renewals for up to 90s while the lease's 60s window lapsed underneath the upload and the install RPC waited behind it. Each beat now carries a budget no longer than the cadence it started on and arms its successor at beat start, so a beat that never settles is abandoned on schedule; nothing awaits an outstanding beat when the phase settles. The beat's cadence is gone with it: the loop beats at the registry's floor until a beat names the window, and an answer that names no window keeps the cadence rather than slowing down on the absence of evidence. The lease-lost reasons move to contracts as one taxonomy shared with the connection runtime, and LEASE_SESSION_MISMATCH leaves the terminal set: only request admission raises it and lease_heartbeat is admission-exempt. Two suites back it. The beat loop is pinned against fake timers for the stalled beat, its budget, and an abandoned beat that answers too late to matter. remote-upload-lease-beat.test.ts drives an install through sendToDaemon against a fake remote daemon, so the wiring between the loop, the beat it is handed, and the upload is covered, not just each of them alone: the first holds the upload until a second beat names the lease and asserts a renewed artifact lands on a live lease before the install RPC; the second answers a beat with LEASE_NOT_FOUND and asserts the artifact is destroyed mid-flight, is not left to drain once the daemon stops applying backpressure, and no install RPC goes out. Killing either half of that wiring fails them: dropping the heartbeat shows no beat, and dropping the signal hands the daemon a drained artifact.
runProtectedLeaseWork, the beat that keeps a remote lease alive across a client-side phase, and the request it sends were three of daemon-client.ts's four exports and existed there only so their tests could reach them. They move to daemon-client-lease-beat.ts, which leaves the transport entry module 287 lines and knowing nothing about cadence, budgets, or which lease reasons are terminal. The test moves with them and is renamed to match. The beat family is a pure move: no behaviour change, and the beat's transport is still the send it is handed, so the module stays free of transport decisions.
…re request R78 names measured edges, never a directory, so the extracted module owes its own entry: a type-only import of the same DaemonRequest vocabulary the four sibling client modules already record.
…sent ttlMs keeps ADR 0007 and the loop's JSDoc both claimed a missing lease is caught before any bytes move. Hashing, the preflight, and the start of the stream can all run while the opening beat is still outstanding, so the claim was stronger than the behaviour: the first beat buys learning the loss during the upload, not before it. The same section described the successor as armed a third of a window after the beat lands, which the budget change just superseded; it now says the successor is armed at beat start and the budget is capped at the cadence, and the survived- failure paragraph says that promise covers a beat abandoned at its budget, not only one that fails fast — the distinction the review found in the old shape. remote-proxy.md promises five minutes of proxy lease inactivity for CLI connect. That holds because `open` names the window; a lease allocated over the RPC with no ttlMs keeps the daemon's one-minute default, so the sentence now says which one a reader gets.
…me is causal The lost-lease case waited on whichever of two events reached the fake daemon first, with a grace window deciding an inconclusive answer. A client blocked in a kernel write cannot observe its own cancellation until the peer drains, so that window had to be longer than the drain — timing the test had to win rather than a fact it could state. The fake daemon now holds the artifact until the test releases it, and only releases it after sendToDaemon has already rejected. An upload nobody canceled has nothing left that could stop it by then and drains; one that was destroyed cannot. The success case reads the delivered byte count instead of racing the same oracle. Killing either half of the wiring still fails: dropping the heartbeat leaves no beat and both cases time out, dropping the signal hands the daemon a drained artifact.
…that needs them collapse createLeaseRenewalBeat and leaseScopeForHeartbeat into buildUploadLeaseHeartbeat, which was their only caller, and let the beat tests read the scope through the contracts helper the factory itself uses. the per-beat request id is now pinned over the real transport, where a shared id is observable, instead of against an injected send.
…dence Budgeting a beat at the cadence it started on made one number do two jobs. Until a beat answers, the cadence sits at the 1s floor, so `sendRequest` got a 1s budget, destroyed the request, and emitted a `daemon_request_timeout`. That answer is non-terminal, so the cadence never left the floor: on any link where a round trip takes over a second — a tunneled proxy, a fresh TLS connect per beat because every timeout tore the socket down, an uplink the upload was saturating — no beat ever got a window in which to answer, and the lease could lapse on exactly the slow uploads #2946 exists to protect, with a diagnostic pair every second. Cadence and budget are two numbers now. A beat is armed every third of the window and allowed the whole window to answer in, so a slow first answer sets the cadence instead of being cut off before it can say anything. The window the loop assumes before the first answer is the registry's five-second minimum, which is now shared with the client as `MIN_LEASE_WINDOW_MS` rather than assumed twice: planning against a longer window than the daemon would ever grant is the same mistake pointed the other way. The slow-link case is covered through `sendToDaemon`, which no loopback unit test travels: the fake daemon answers a beat after 3s — longer than the assumed cadence, shorter than the assumed window, so a beat budgeted at the cadence is cut off and one budgeted at the window is not. It asserts the beat stops arriving once the window is known and counts no `daemon_request_timeout` inside the upload, in a diagnostics scope because outside one every count reads zero for the wrong reason. Verified against the previous beat loop: five arrivals there, none of them answered in time.
The extraction that moved the beat out of daemon-client.ts put it in cli.ts's eager closure, which every command pays for, and took it from 295 modules to 296 (#2990's coverage failure). The beat is only ever useful to one kind of request: one heading to a remote daemon that names a lease. Both guards are cheap and local — the lease scope comes off the request the caller already built — so the module now loads behind them and the closure is back to 295. Also folds the beat's thin wrappers away from the entry surface, which is what the four test-only exports in daemon-client.ts had become: createLeaseRenewalBeat and leaseScopeForHeartbeat existed only for tests, the scope precedence they pinned is already covered where it lives in contracts, and the one invariant that wasn't covered anywhere — a beat per request id, so a timed-out beat cannot cancel its successor — is now pinned over the real transport, where a shared id is actually observable.
…o timeout The provider-integration project sets no `testTimeout`, so the scenario ran under Vitest's 5s default and the Coverage job's full-suite run — heavier than a lane-local one — tripped it: the slow-link case spends 4.5s watching a real lease window, and that body has to sit past the assumed cadence so the opening beat's late answer can move the loop off it. Under the documented parallel import tail the worst case approached the default, and CI drew first. Uses the project's own PARALLEL_PROVIDER_SCENARIO_TIMEOUT_MS, which exists for scenarios that build a real in-process daemon path and get timed out by setup contention rather than by their own work. The observation window is untouched: shortening it would cut the margin between the cadence-budgeted beat (three arrivals, each timed out) and the window-budgeted one (the answer lands), which is the only thing this test reads.
Two non-blocking notes from the 4c74a43 pass. SLOW_BEAT_MS only discriminates the window-budgeted beat from the cadence-budgeted one inside a narrow band: slower than the pre-window cadence (1666ms) so a beat held to that cadence is cut off before the answer lands, and fast enough to answer before that cadence arms a third beat (3332ms). At 3000ms the late answer had ~332ms of margin to the buggy third arrival — enough to flake on a loaded runner. 2500ms is the center of the band; verified by mutation, budgeting the beat at its cadence now trips "beats seen: 3". Moved the leaseWindowFromHeartbeatResponse doc off leaseBeatIntervalMs and onto the function it describes, and reordered the renewed-window guard to match the undefined guard above it. No behavior change.
…on its cause The ADR told slow links they get the whole window as a beat's answer deadline. The implementation takes the shorter of the window and the heartbeat's request policy, so a ten-minute lease caps at ninety seconds and the paragraph promised a budget the code never grants. Documented rather than removed: the daemon renews when it handles the request, before it answers, so a beat abandoned at the cap still extended the lease when the request landed, and when it didn't a saturated link is not served by a socket held for the rest of a multi-minute window. Shorter windows are unchanged. The slow-link case released its withheld artifact through a module-level global assigned inside the current test's upload handler. A client regression that stops the artifact from ever arriving left the previous test's closure in place, so the call was a no-op and the test hung on `await running` until the lane timeout instead of naming the cause. It now calls the fake daemon's own resumeUpload, which throws when no artifact reached that daemon. Verified by pointing the install at a path that is never uploaded: all three cases fail on that message rather than timing out.
…l clock The slow-link case sampled `beatArrivals.length` at a fixed 4.5s and asserted 2. Both directions of that number depended on event-loop timing: a stall over the margin between the first answer landing and the assumed cadence arming a third beat read as three beats, and a stall long enough to keep the second beat out of the sample read as one. Neither says anything about the loop under test. The count now waits for the two beats it expects, then keeps watching through a quiet window of two assumed cadences and returns what arrived. A loop that budgets a beat at its cadence is still arriving throughout that window and is counted, not awaited; a loop that moved to the window it just renewed is silent until ten seconds in. A runner stall delays the read instead of moving a beat across it. Verified by mutation both ways: budgeting the beat at its interval reports 4 beats, and arming the opening beat late fails with "only 0 of 2 beats arrived" rather than hanging to the lane timeout. The assumed cadence is derived from MIN_LEASE_WINDOW_MS instead of restated, so it cannot drift from what the loop actually assumes.
c10e3a8 to
ba9dad1
Compare
1 participant
Summary
A remote
installuploaded its artifact before the install request was admitted, so nothing renewed the lease while bytes moved: a 449 MB APK at 1m47s against a 60s TTL expired the lease paying for the device (#2946).sendToDaemonnow beats that lease during the upload.heartbeatLeasewith nottlMsrenewed to the registry default, shortening any lease allocated above it. It now renews for the window the lease carries. Admission did the same with a proxy default of its own, unreachable from the client; it is retired.Local and lease-less requests get no timer. Closes #2946
Gross diff ~1,600 is over the 1,000-line budget: the admission and cancellation halves cannot split without shipping the shortening bug.
Validation
Commit
410711c2a:pnpm check:affected --run, format, lint, typecheck, fallow, and wire-compat passed; new assertions are mutation-verified. CI green. No live provider run: needs a remote daemon and a >60s upload.agent-browser-lifecycle.test.tsfails pre-existing atbce6f526d.