Skip to content

fix(resources): point tenant isolation lease errors to leases.allocate - #3635

Open
mvanhorn wants to merge 2 commits into
callstack:mainfrom
mvanhorn:fix/3350-tenant-isolation-lease-error
Open

mvanhorn wants to merge 2 commits into
callstack:mainfrom
mvanhorn:fix/3350-tenant-isolation-lease-error

Conversation

@mvanhorn

@mvanhorn mvanhorn commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Enabling tenant isolation without a lease id fails with an invalid-arguments error that only says a lease id is required. The Limrun guide also never showed how a provider should attach to an instance a harness already owns. normalizeLeaseAdmissionRequest throws INVALID_ARGS with the literal "tenant isolation requires lease id." when normalizeLeaseId returns nothing, and admission uses that check for close and for other commands. The Limrun page described passing an existing instance URL and token, then stopped short of allocating a lease and placing the returned id on the client scope.

The thrown message is now "tenant isolation requires lease id from leases.allocate.", and the admission tests match that wording. The Limrun page adds a provider that calls leases.allocate in acquire, returns lease.leaseId with sessionIsolation set to tenant, and calls leases.release with the same id. The note states that the daemon is started with the instance URL and token, and that neither call creates or deletes the instance.

Fixes #3350

Test plan

npx --no-install vitest run src/sessions/tests/request-admission.test.ts finished with all 15 tests passing.

View guided diff

The thrown message is now "tenant isolation requires lease id from
leases.allocate.", and the admission tests match that wording. The
Limrun page adds a provider that calls leases.allocate in acquire,
returns lease.leaseId with sessionIsolation set to tenant, and calls
leases.release with the same id. The note states that the daemon is
started with the instance URL and token, and that neither call creates
or deletes the instance.

Fixes callstack#3350

@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 3 files

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

View guided diff | Re-trigger cubic

Comment thread website/docs/docs/limrun.md
@thymikee

Copy link
Copy Markdown
Member

Reviewed e9b6074. The code change looks good: the new error text points to leases.allocate, and I found no blocking problem. No conflicts.

The failing check is the Android emulator smoke job (helperTransport 'instrumentation' vs expected 'persistent-session'). It looks unrelated, since this PR only changes the INVALID_ARGS message text, the admission test regexes and a docs page, and the smoke scenario does not reach that missing-leaseId path. I did not check whether it also fails on main. A second Smoke Tests job is still queued.

Not blocking: the limrun.md example (https://github.com/callstack/agent-device/blob/e9b6074/website/docs/docs/limrun.md#L116) allocates without retainOnClose: true while the provider releases the lease itself, so a session close in between would end the lease and the release would return released:false. Adding the flag with one sentence of explanation would fix it. Also, "leases.allocate" is an SDK name, so CLI-only users who hit this INVALID_ARGS path see a method name they cannot call. A typed hint could carry that pointer instead of the message text. You can take or leave both.

The open inline thread on the limrun.md example still applies (#3635 (comment)). It is the same retainOnClose gap as above, and it is a doc gap, not a production defect.

I did not run request-admission.test.ts, so the 15-pass claim is unverified. I checked the docs example against the type declarations and registry code, not against a running daemon. I also did not check whether a CHANGELOG entry is needed for the message change.

Before merge, optionally add retainOnClose: true to the example, then rerun the Android smoke job.

The provider example releases its lease itself, so allocate it with
retainOnClose: true. Without it, a session close before release ends the
lease and leases.release returns released: false. Also note that a
retained lease still expires after its ttlMs.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KFTE4BHbGYzgf8Uhzcppdh
@mvanhorn

Copy link
Copy Markdown
Contributor Author

Added retainOnClose: true to the limrun.md allocate example in d42f9b5, with a sentence on why: the provider releases the lease itself, so without it a session close before release would end the lease. Also confirmed the request-admission tests locally (15/15). The red Smoke Tests job is the Android emulator smoke test that has been flaky on main too, so it likely just needs a rerun.

@thymikee

Copy link
Copy Markdown
Member

The fixes since e9b6074 hold at d42f9b5: limrun.md now passes retainOnClose: true and explains the lifecycle, which matches lease-registry.ts:297. I did not re-check the one resolved inline thread on its own. I did not re-run the request-admission tests or lint. I did not compile or run the docs example, which is a docs-only change. I found no new code problems.

Smoke Tests fails in the iOS runner XCTest testSynthesizedReplacementLeavesAMaskedFieldUnverified. This PR changes one error string, test regexes and docs, so it does not touch that route, and the failure looks unrelated. The author's Android-flake explanation does not match the log. Repo Guards fails in check:affected lint after about 0.02s and prints no diagnostic, so I cannot tell the cause. The base is stale, and fallow flagged 882 changed files against the old base fc224bc, which a 3-file diff cannot cause. There are no conflicts.

Could you rebase onto current main and rerun CI? A clean rerun would show that the Repo Guards lint and the iOS Smoke failures come from the stale base or a flake.

A small question, not blocking. The pointer to leases.allocate is remediation guidance, and AppError already has a hint field for that (lease-registry-scope.ts:377 uses it). normalizeError keeps hint. Would it be better to keep the message as 'tenant isolation requires lease id.' and add { hint: 'Allocate a lease with leases.allocate and pass its leaseId.' }? The test could then assert on the hint and not on the message text.

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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Point the "tenant isolation requires lease id" error at leases.allocate, and document providers that attach to an existing Limrun instance

2 participants