Skip to content

fix(lease): return a retried allocation's own lease and refuse an unbound renewed lease - #3619

Open
thymikee wants to merge 6 commits into
mainfrom
fix/3487-review-followups
Open

thymikee wants to merge 6 commits into
mainfrom
fix/3487-review-followups

Conversation

@thymikee

@thymikee thymikee commented Oct 14, 2026 •

Copy link
Copy Markdown
Member

Follow-up to #3487, which merged before these review threads were answered.

  • A retried lease allocation that names its own lease id now gets that lease back before the daemon picks a device. Before, the retry failed with DEVICE_NOT_FOUND when the leased device had left the inventory, or with LEASE_ID_IN_USE when the spec now matched another device. The registry's new renewRequestedLease refreshes the live lease when tenant, run, backend, provider and client match; the lease id is the retry key because one run can hold several leases.
  • open on a renewed lease that names no device now fails with UNAUTHORIZED / LEASE_SCOPE_MISMATCH and never releases that lease. Only a lease supplied from outside can lack a device key; before this fix open ran on a locally picked device that lease did not bind. This matches the behavior before feat(proxy): allocate by device spec and bind the device the lease names #3487.
  • capture-kit validates envelope device targets with the kernel's isDeviceTarget instead of a private copy.
  • The lease handler test helper asserts that a lease came back.

Docs: remote-proxy.md and client-api.md describe the retry behavior and the refused keyless lease.

Checks: build, typecheck, lint, layering, fallow, wire-compat (protocol unchanged), the eager-closure budgets, and the lease handler, registry, lease-scope, remote-connection and capture-kit envelope tests. Each new test fails without its fix.

View guided diff Turn on auto-fix

thymikee and others added 3 commits October 14, 2026 18:36
…device

An allocation that repeats the leaseId of its live lease, for the same
tenant, run, backend, provider, and client, now gets that lease back
before a proxy daemon picks a device. A retry after a lost reply no
longer fails when the leased device left the inventory, and no longer
binds another device when the spec resolves differently. The registry
owns the rule in renewRequestedLease, and allocateLease applies it too.

The by-spec test helper now asserts that the reply carries a lease.

Co-Authored-By: Claude <noreply@anthropic.com>
…nning unbound

Every lease the CLI allocates names its device: a picking daemon binds
the key, and a keyless lease from an older daemon is released and
reallocated by the locally picked key. A renewed keyless lease was
therefore supplied from outside, so open now refuses it with
LEASE_SCOPE_MISMATCH and leaves it active, as the heartbeat refused it
before by-spec allocation, rather than running on a device the lease
does not bind. The heartbeat test fixture echoes the deviceKey the
request names, as the daemon does.

Co-Authored-By: Claude <noreply@anthropic.com>
…el guard

Co-Authored-By: Claude <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 10 files

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread packages/daemon/src/resources/lease-registry.ts Outdated
Comment thread website/docs/docs/remote-proxy.md Outdated
Comment thread packages/daemon/src/resources/__tests__/lease-registry.test.ts
Comment thread website/docs/docs/client-api.md Outdated
@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.25 MB 5.25 MB +1.0 kB
Package (unpacked) 5.25 MB 5.25 MB +1.0 kB
Package (download) 1.58 MB 1.58 MB +288 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 28.1 ms 28.4 ms +0.3 ms
CLI --help 77.7 ms 79.9 ms +2.1 ms

@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1

QR code for preview link

🚀 View preview at
https://callstack.github.io/agent-device/pr-preview/pr-3619/

Built to branch gh-pages at 2026-10-14 18:41 UTC.
Preview will be ready when the GitHub Pages deployment is complete.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 4 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

Comment thread website/docs/docs/remote-proxy.md
@thymikee

Copy link
Copy Markdown
Member Author

I found no new blocking problem in the code at 1067575, but the PR is not ready to merge. The Smoke Tests jobs are still queued or in progress and none has failed. Smoke runs device commands without a proxy lease, so it does not exercise the lease registry, the lease handler or the CLI proxy binding that this diff changes. There are no conflicts.

Not blocking, and you can take or leave these: refreshRequestedLease renews with the request's ttlMs and never applies hostTtlMsByLeaseId, so a tenant that retries allocate with a host-pinned lease id and a larger ttlMs can extend the lease past the host cap. refreshExistingRunBinding has the same gap, so a rule like "every renewal route applies the host TTL cap" would cover both, heartbeatLease and this path. Also, the isDeviceTarget dedupe in durable-resource-envelope.ts is correct but unrelated to leases, so it could be its own PR. Finally, allocateOrRenewLease probes the registry twice for one rule, which is fine as a pre-pick probe.

The merge step that remains is the open Cubic P1 thread on the issued-id fallback (#3619 (comment)). Please enforce one rule: an allocate that names an already-issued leaseId returns exactly that lease or fails with LEASE_ID_IN_USE, and never returns another lease. To do that, check issuedLeaseIds before refreshExistingRunBinding, then add a registry test with two leases on one run. The four Cubic P2 threads are fixed at this head (#3619 (comment), #3619 (comment), #3619 (comment), #3619 (comment)), so you can resolve them.

I did not run the new tests. I judged that they would fail on the old code by reading the pre-change paths. I also did not check the claim that refusing a keyless renewed lease "matches the behavior before #3487". #3487 shipped in v0.21.28, so this refusal changes behavior for released users. The docs are updated. Please check whether CHANGELOG.md needs an entry, because #2922 removed it and #3593 re-added it.

@thymikee

Copy link
Copy Markdown
Member Author

Both points are fixed in a1033ad.

  • An allocate that names an already-issued leaseId returns exactly that lease or fails with LEASE_ID_IN_USE. The issued-id check now runs before the run-binding reuse, so it never returns another lease of the same run. A registry test with two leases on one run and client covers it.
  • The host TTL cap now lives in the registry's single renewal function, so heartbeat, the retry by id and the run-binding reuse all keep a host-pinned lease inside the host's window. A test retries a host-pinned lease with a larger ttlMs and checks the expiry. Both new tests fail without the fix.

I left the isDeviceTarget dedupe in this PR: it came from a review thread on #3487 and is three lines. Checks: typecheck, lint, format, and 145 daemon session and remote-connection test files (967 tests).

@thymikee

Copy link
Copy Markdown
Member Author

The code at a436b0a looks good to me, and it fixes the problems from the earlier review (#3619 (comment)). A retried allocation now returns its own lease. A renewed lease without a binding is refused. I did not run the new registry tests myself. I read the old code path, and the new tests should fail on it. I did not check whether MIN_LEASE_WINDOW_MS is at or below the 10_000 ttl that the host-window test passes to putHostLease. You report that the test passes, so I am taking that as given.

The five cubic-dev-ai threads all look fixed at this head, so you can resolve them. The first is the issued-id check before refreshExistingRunBinding (https://github.com/callstack/agent-device/blob/a436b0a/packages/daemon/src/resources/lease-registry.ts#L85-L92), covered by the two-leases-one-run test: #3619 (comment). The second is the retry without ttlMs renewing with leaseOwnTtlMs: #3619 (comment). The third is the remote-proxy.md retry scope and the several-leases-per-run note: #3619 (comment). The fourth is the clock advance and the 91_000 expiresAt assertion in the lease-registry test: #3619 (comment). The fifth is the client-api.md scope-conditional retry text, which the last review confirmed and this delta does not touch: #3619 (comment).

The two Smoke Tests failures look unrelated to this change. On Android, agent-device wait text Automation lab failed with wait_capture_stalled in a local --serial session with no proxy lease, and the smoke test has no lease, proxy or tenant reference. On iOS, the runner test testSynthesizedReplacementLeavesAMaskedFieldUnverified failed on a masked-field bullet count in Swift code this PR does not touch. Please re-run both jobs. Once they pass, the checks are green and the PR can merge. There are no conflicts. Separately, please decide whether CHANGELOG.md needs an entry for refusing a keyless renewed lease. That is outside this delta and does not hold up the merge.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 14, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant