Clear app executor timeout timers#1419
Open
morluto wants to merge 1 commit into
Open
Conversation
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
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.
Fixes #1413
Summary
Completed app tool invocations now clear their losing timeout timers instead of retaining them until the configured deadline.
The change applies to the in-process executor and the emitted workerd and dynamic Worker drivers.
What changed
try/finallyand clear the timer after the race settles.DRIVER_VERSIONfrom1to2so Worker Loader does not reuse an isolate containing the previous generated driver.executorpatch changeset.The configured timeout, timeout error message, and app tool result contract are unchanged.
Regression proof
The regression starts a child process that completes an app tool invocation immediately with a 30-second timeout.
Before the fix, the child printed
invocation completebut remained alive past the test's five-second process deadline because the losing timer was still active.After the fix, the child exits successfully within that deadline.
Validation
From
packages/plugins/apps:bun run test -- src/executor/app-tool-executor.test.ts src/executor/app-tool-executor.conformance.test.ts: 2 files and 19 tests passed.bunx vitest run --config vitest.worker.config.ts src/executor/dynamic-worker-app-tool-executor.worker.test.ts: 1 file and 1 test passed.bun run typecheck: passed with unrelated existing Effect suggestions.Additional validation:
bun run bootstrappassed.git diff --checkpassed.origin/mainreturned no actionable findings.Limitations
The process-liveness regression directly proves timer release in the in-process Node executor. The workerd conformance tests and dynamic Worker pool test prove that the changed emitted drivers still invoke tools successfully, but those runtimes do not expose pending timer handles for the same direct assertion.
This change does not cancel the underlying tool invocation after a timeout. It only releases the losing timeout when the invocation settles first.
Review order