sdk: a stale lease error from the local LLM worker kills the whole run, though the kernel already retried the step - #562
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The five worker-lease regression files were not in tsconfig.tests.json, whose `include` names its files rather than globbing, so `npm run typecheck:tests` never saw them. Add them; the evidence directory's supplemental config existed only to cover that gap and goes with them. Also record what the live-kernel failures in this directory's transcripts actually were. They are not the branch: /home/daytona/package.json sits above the checkout declaring "type": "commonjs", which turns off Node's module-syntax detection for testdata/preflight's extensionless ESM agent-CLI fixtures. Each fixture then exits 0 having written nothing and every agent step it drives fails its execution gate. Declaring that one directory ESM restores the condition a GitHub runner has and the whole suite passes on this branch unchanged; the transcript carries the reproduction and the passing run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
onWorkerFailure downgrades a lease loss to a warning only when it can name the attempt the kernel now owns. Every emitter passes the dispatch today; this keeps a future one from turning a lease loss into a TypeError. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
7ea8000 to
2dfb951
Compare
|
Rebased onto main, which carries #558 and 2.0.30; the rebase was clean. Reviewed the source changes, and added one commit so Focused regressions run locally on the rebased head: The factory's full-suite failures are the sandbox's (bubblewrap loopback, Bun version); it showed the same failures on the base commit. Marking ready so the review bots run. @codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2dfb951950
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| mutation('filter','packages/sdk/src/worker-lease.ts', | ||
| 'if (!isLeaseLost(error))', 'if (true)', | ||
| 'npx vitest run tests/worker-lease-lost.test.ts') |
There was a problem hiding this comment.
Update the mutation script for the final predicate
Running the documented python3 evidence/worker-lease-lost/mutate.py command at this commit fails immediately because worker-lease.ts now contains if (!isLeaseLost(error) || dispatch === undefined), so this exact needle is absent. The script also truncates mutations.txt before reaching the assertion, and the recorded restored hashes no longer match the final source. Update the mutations and recapture both mutant and restored outputs against the final head before retaining the mutation-verification claim.
AGENTS.md reference: AGENTS.md:L93-L96
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cebdc0a. mutate.py now matches the fail-closed predicate (if (!isLeaseLost(error) || dispatch === undefined)) and no longer hard-codes the sandbox PATH. I reran it on the rebased head: all seven mutations reproduce, with mutant exit 1 and restored exit 0 each (mutations.txt, with the restored SHA-256 of each file).
| `python3 evidence/worker-lease-lost/baseline.py` replaces only changed SDK source | ||
| files with their original bytes from `f6ece41`, rebuilds, and runs the unchanged | ||
| live-kernel suite. It restores the implementation byte-for-byte in `finally`, | ||
| asserts equality, and rebuilds it. The script uses the same PATH and | ||
| RELAYFLOWD_BIN as the final full suite. |
There was a problem hiding this comment.
Compare failures against the actual rebased parent
On this rebased change the parent is 2e2043f, but the baseline restores source from f6ece41; those revisions differ in two files this PR modifies (cli/run.ts and llm-worker.ts, by 58 lines), and the captured transcript identifies SDK 2.0.29 rather than the rebased 2.0.30. Consequently this comparison cannot establish that the full-suite failures also occur on the reviewed base; rerun it against the actual parent or narrow the claim.
AGENTS.md reference: AGENTS.md:L83-L86
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cebdc0a. baseline.py now uses the rebased parent, git merge-base HEAD origin/main (BASE=<rev> overrides), and records it in each transcript. It takes RELAYFLOWD_BIN from the caller. I reran it: base 2e2043f gives Tests 31 passed (31), and the live-kernel suite at this head also passes 31/31 (live-kernel-head.txt). The old sandbox comparison is removed. The README explains that the sandbox's eight failures came from a stray /home/daytona/package.json, not from either revision.
mutate.py matches the fail-closed predicate; all seven mutations reproduce (mutant exit 1, restored exit 0). baseline.py compares against the rebased parent instead of a hard-coded commit and takes RELAYFLOWD_BIN from the caller: the live-kernel suite passes 31/31 at base 2e2043f and at this head. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Prevent stale local worker leases from aborting runs the kernel has already retried or completed.
run_terminalis dropped only forstep.heartbeat,step.complete, andstep.wait;lease_conflictis also dropped..cause.Regression coverage includes expired dispatch → attempt 2 success for both worker types, actual kernel success followed by completion/heartbeat refusal, terminal refusals, direct-run/resume failure handling, the sweep grace and its bound, and non-lease fatal errors. The concurrency issue that creates stale dispatches remains outside this change.
Mutation evidence is captured in the evidence index. Seven checks each captured a failing mutation and passing byte-for-byte restoration; the ledger records the exact substitutions and restored hashes. The fatal-policy check removes the retained fatal callback as a negative control.
Literal command and captured output for the core restored regressions:
Full output: worker regressions,
real-kernel regressions.
Every other mutation command and both outputs are linked in the evidence index.
The new regression files also received a separate TypeScript check because the
existing test configuration enumerates selected files:
The full SDK suite is not green. The final command was:
Captured final output:
Full output includes successful
SDK typecheck, build, and the repository's test typecheck, followed by the test
failures. Remaining failures:
configure loopback in this environment (
Failed RTM_NEWADDR: Operation not permitted).The unhandled error is also from the hosted sandbox.
1.3.6, while itsprerequisite asserts
1.4.0.identification and agent-output/verification assertions.
Validation is therefore limited to the passing focused regressions and mutation
checks; this report does not claim a green full suite. No kernel or workflow
files were changed.
The unchanged live-kernel suite was also run after restoring the original SDK
source from
f6ece41and rebuilding:Baseline output and
failed-name comparison
show the same eight failures. The implementation was restored byte-for-byte
and rebuilt successfully.
The comparison used the same Rust PATH and RELAYFLOWD_BIN as the final suite.
Checks
The checks fail on the base commit too, so these failures were not introduced by this change: they come from the repository itself or from the environment the checks ran in. This pull request is a draft until someone looks.
What ran (.relayflow/check.sh)
Output on this branch (last 80 lines)
Output on the base commit (last 80 lines)
What the repair agent found
Check repair notes
.relayflow/check.shwas failing on this branch. Three distinct causes, none ofthem a defect in the branch's change. Two were missing setup and are fixed in
.relayflow/check.sh(not committed). One is a kernel facility this containerdoes not have and is left alone.
Fixed: bun was not pinned to the version CI uses (setup)
.github/workflows/cloud-runtime-artifact.ymlpinssetup-bun@v2tobun-version: "1.4.0".check.shonly installed bun when none was present, sothis machine's preinstalled bun 1.3.6 was used instead.
The pin is load-bearing, not cosmetic. Under bun 1.3.6 a mutated
process.env.PATHis not handed tospawnSync, sopackages/schema/tests/parity.test.tscould not shadow the machine's realclaudewith the stub it writes, andstep-memory.flow.yamlfailed preflighton the host CLI's unauthenticated
auth status:That aborted
check.shat the schema step, so the SDK suite never ran at all.packages/sdk/tests/authored-node-runtime.test.ts:18asserts the same versiondirectly and was failing for the same reason.
check.shnow installsbun-v1.4.0whenever the machine's bun is anythingelse. With it, schema parity is
77 pass, 0 fail.Fixed: a package.json above the checkout disabled Node ESM detection (setup)
testdata/preflight's agent-CLI fixtures are extensionless files containing anESM
import. Node runs them only because module-syntax detection applies whenno package.json governs the file. A GitHub runner checks out under
/home/runner/workwith no package.json above it.This machine has
/home/daytona/package.jsondeclaring"type": "commonjs"ABOVE the checkout, which turns detection off. Each fixture is then parsed as
CommonJS, its module body never runs, and it exits 0 having written nothing:
Every agent step driven by one then fails its execution gate with
output: null— the seventests/live-kernel.test.tsfailures. They are notnew:
evidence/worker-lease-lost/baseline-comparison.txtrecords the same setfailing on the original SDK source at
f6ece41.check.shnow writes{"type":"module"}intotestdata/preflight/package.jsonbefore the run and removes it on exit, restoring the CI condition without
leaving anything in the tree. With it,
tests/live-kernel.test.tsis30 passed | 1 skipped. Full reproduction and captured output:evidence/worker-lease-lost/live-kernel-module-type.txt.A durable alternative, for a human to decide: commit
testdata/preflight/package.jsonas{"type":"module"}. It is a no-op in CIand makes the fixtures' module kind explicit rather than inferred. Not done
here — it is outside this branch's scope.
Not fixed: bubblewrap cannot create a user namespace here (outside my control)
check.shalready probes for this and warns. The sysctl that would relax it,kernel.apparmor_restrict_unprivileged_userns, is read-only in this container:22 tests in three files need the real sandbox and fail for want of that kernel
facility, not for a defect:
tests/hosted-extension-isolation.test.ts— 13 failedtests/hosted-extension-protocol.test.ts— 8 failedtests/babysitter-native-extension.test.ts— 1 failedWhere the run ends up
With both setup fixes, the last full
sh .relayflow/check.sh:The 22 are exactly the bubblewrap set above. Every other step passes: kernel
build and tests, surface build, schema regeneration and parity, SDK typecheck,
build, test typecheck and vitest. The branch's own regression files all pass:
check.shstops at the first failing step, so the steps after the SDK suitewere never reached in that run. They were run by hand afterwards, with the same
commands and the same PATH, and all pass:
flows checkreported
"ok":truefortestdata/hello-deterministic.flow.yamlops/preswarm-check/lens-parity-check.sh—PASSops/preswarm-check/lens-cli-parity-check.sh—PASSscripts/surface-package-gate.sh— exit 0(
PACKED_RUNTIME_OK,PACKED_RUNTIME_REFUSAL_OK,PACKED_TYPESCRIPT_OK)The review gate self-tests skip here exactly as
check.shsays they will:ruby is not installed, and they parse YAML with Psych.
Fixes #560
Summary by cubic
Prevents a stale local worker lease error from aborting a run the kernel has already retried or completed.
Bug Fixes
WorkerLeaseLostErrorand drops journal refusals only forlease_conflictorrun_terminalonstep.heartbeat,step.complete, andstep.wait.tsconfig.tests.json.The mutation and baseline evidence under
evidence/worker-lease-lost/was regenerated on the rebased head: all seven mutations reproduce a failing test,baseline.pycompares against the rebase's parent instead of a hard-coded commit, and the live-kernel suite passes at both base.Written for commit cebdc0a. Summary will update on new commits.
Note
Medium Risk
Changes how local workers react to lease-related journal errors on critical run/resume paths; incorrect classification could either abort good runs or mask real failures, though mutation tests and targeted regressions narrow that surface.
Overview
Fixes a bug where stale local worker lease errors (expired dispatches, post-retry
lease_conflict/run_terminalrefusals) closed the journal client and killed the whole run, even when the kernel had already retried or completed the step.The SDK now classifies lease loss via
WorkerLeaseLostErrorand journal refusals tagged withlease_conflictorrun_terminalonstep.heartbeat,step.complete, andstep.wait.onWorkerFailurekeeps workers attached for those cases: it emits a stderr warning with run/step/attempt context instead of calling fatal close. Other worker errors (including bad lease deadlines) still fail closed with the original error identity—no message or.causeheuristics.CLI paths wire the same filter for direct run, authored resume LLM, HN monitor, and communication workers.
flows runalso adds a 5s post-expiry grace (LEASE_SWEEP_GRACE_MS) while polling, aligned with the Rust CLI, so the executor can wait for kernel reconciliation.Regression coverage adds focused vitest suites (unit, live-kernel injection, sweep, direct/resume failure reporting) plus an
evidence/worker-lease-lost/bundle: mutation scripts, baseline comparison againstf6ece41, and captured test transcripts documenting passing regressions and environment-specific full-suite failures (bubblewrap, Bun pin, preflight ESM).Reviewed by Cursor Bugbot for commit 2dfb951. Bugbot is set up for automated code reviews on this repo. Configure here.