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 |
…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.
410711c to
612909b
Compare
|
…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.
|
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 |
This branch has not been deployed
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.