Skip to content

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

Merged
thymikee merged 4 commits into
callstack:mainfrom
mvanhorn:fix/3350-tenant-isolation-lease-error
Oct 18, 2026
Merged

thymikee merged 4 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.

Keeps the tenant isolation message as "tenant isolation requires lease
id." and moves the remediation into the AppError hint, "Allocate a lease
with leases.allocate and pass its leaseId.", the field normalizeError
already keeps. The request-admission tests now assert the message and
the hint separately.
@mvanhorn

Copy link
Copy Markdown
Contributor Author

Thanks, the hint field is the better place for it. 2da53a0 keeps the message as tenant isolation requires lease id. and moves the pointer into the AppError hint: Allocate a lease with leases.allocate and pass its leaseId. The request-admission tests now assert the message and the hint separately.

The branch also has current main merged in (c2244c0). On 2da53a0, lint, typecheck, the request-admission tests, the Repo Guards gates (layering, gate-manifest, fallow, production-exports, daemon-wire-compat and the rest), build and pnpm test:smoke pass locally. A full local pnpm test:unit run had 4 failures outside this change, in daemon-client-ensure-drain, macos-helper-package-contents and the fuzz harness. The iOS and Android emulator jobs did not run here.

@thymikee

Copy link
Copy Markdown
Member

Looks good at 2da53a0. The pointer to leases.allocate is now the AppError hint, normalizeError carries it to the CLI, MCP and client surfaces, and the tests assert the message and the hint separately. The limrun.md example sets retainOnClose: true and explains it. All 15 checks pass with main merged in, so the earlier stale-base failures are gone. There are no conflicts. I ran no tests myself and relied on CI.

Not blocking, take or leave: the PR body still says the message itself changed, but the message is unchanged and the pointer is in the hint, so a one-line body fix would help. The router-level test in request-router-apps-admission.test.ts asserts only response.error.message, so a check on response.error.hint would cover the production route.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Oct 18, 2026
@thymikee
thymikee merged commit f2e60e9 into callstack:main Oct 18, 2026
15 checks passed
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.

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

2 participants