fix(execution): treat an undetermined lease as a fallback, not a denial - #7228
Merged
Conversation
The distributed owner lease is a cross-process fairness check, not a correctness lock. A round trip that did not answer before its deadline was reported as a hard failure and rejected the execution, even though the per-process pool and the per-owner active/queued limits still bound the work. - Fall back to the local limits when the lease is undetermined. Only `limit_exceeded` denies an execution, since it is an actual answer. - Rename that outcome from `unavailable` to `undetermined` so the absence of an answer is not read as a negative one, and log it at warn. - Make the round-trip deadline configurable and raise its default. This deadline and the client's `commandTimeout` are both plain timers, so a value near normal event-loop latency misreads a scheduling pause as an unreachable dependency. - Skip the release round trip when no lease was ever registered.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
Greptile SummaryThe PR changes an undetermined distributed execution lease from a denial into a fallback to process-local limits.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported negative-deadline bypass is guarded by the positive-value check, and the late-acquisition path now issues release through the same ordered Redis client.
|
| Filename | Overview |
|---|---|
| apps/sim/lib/core/config/env.ts | Adds the configurable Redis lease deadline environment variable with a 1000 ms default. |
| apps/sim/lib/execution/isolated-vm.ts | Treats unresolved lease acquisition as a local-limit fallback, validates non-positive deadlines, and always attempts release for potentially late acquisitions. |
| apps/sim/lib/execution/isolated-vm.test.ts | Expands scheduler coverage for fallback behavior, explicit limit rejection, deadline validation, and late lease release. |
Reviews (3): Last reviewed commit: "test(execution): stop the lease deadline..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Addresses review findings on the fallback path. - Always release the lease. The deadline abandons the local wait but cannot cancel the script, so a late completion still registers the lease id; leaving it unreleased kept it counted against the owner for the whole TTL and denied later executions that did have capacity. The id is unique per execution, so removing one that was never registered is a no-op. - Treat a non-positive configured deadline as unconfigured. A timer of zero or less fires immediately, which would leave every acquisition undetermined and silently drop cross-replica enforcement. - Cover both with tests: a lease that completes after the deadline is still released, and a non-positive deadline still lets a real answer land.
![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 3 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
Collaborator
Author
- Add `IVM_LEASE_REDIS_DEADLINE_MS` to the harness env reset. It was absent, so a test that overrode it left the value in the module-scoped mock env for every later test in the file, quietly changing their fallback timing. - Drop the duplicate over-limit test and fold its extra assertion into the existing one; the two had identical setup and covered the same path.
Collaborator
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
limit_exceededstill denies, since it is an actual answer rather than the absence of one.unavailabletoundeterminedso a missing answer is not read as a negative one, and dropped its log level to warn.IVM_LEASE_REDIS_DEADLINE_MSand raised the default from 200ms to 1000ms. This deadline and the client'scommandTimeoutare both plain timers, so a value close to normal event-loop latency misreads a scheduling pause (a GC pause, say) as an unreachable dependency.Type of Change
Testing
vitest run lib/execution/isolated-vm.test.ts— 14 passing, including three new cases covering the fallback (no client, evaluation error, deadline exceeded) and one asserting the over-limit path still denies. Each new test was verified to fail against the previous behavior.bun run type-check,bun run lint, andbun run check:audits(39 audits) all pass.Checklist