Conversation
Anemy
reviewed
Sep 21, 2026
| /** Close the worker after being idle for 30sec */ | ||
| const IDLE_TIMEOUT_MS = 30_000; | ||
| /** Default execution timeout for worker requests */ | ||
| const DEFAULT_EXECUTION_TIMEOUT_MS = 120_000; |
Member
There was a problem hiding this comment.
Should consumers be able to pass this timeout? Will 0 indicate no timeout?
Collaborator
Author
There was a problem hiding this comment.
That's a good idea, will implement this.
mabaasit
changed the title
mabaasit
marked this pull request as draft
September 22, 2026 08:58
mabaasit
force-pushed
the
timout-long-running-worker
branch
from
September 23, 2026 11:19
2bce04b to
887c77d
Compare
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Moderate worker-concurrency and test-environment cleanup issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (4)
What changed in this PR
Adds configurable two-minute execution timeouts for BSON parser workers, including worker termination and slow-worker tests.
Changes:
- Adds timeout handling and worker termination.
- Exposes timeout options through the parser API.
- Adds slow-worker fixtures, tests, and related dependency/lint configuration.
| File | Reviewed changes | Findings |
|---|---|---|
packages/shell-bson-parser/tsconfig-lint.json |
Excludes test fixtures from lint type checking. | No findings. |
packages/shell-bson-parser/test/fixtures/slow-worker.mjs |
Adds a deliberately slow worker fixture. | No findings. |
packages/shell-bson-parser/src/worker-client.ts |
Implements request timers and worker termination. | moderate (2 votes): global termination can target the wrong worker during concurrent initialization, leaving the timed-out worker active and leaking its blob URL. Initialization must be serialized or termination made instance-aware. nit (3 votes): correct the duplicated/incorrect verb in the comment. |
packages/shell-bson-parser/src/index.ts |
Exposes execution timeout options. | No findings. |
packages/shell-bson-parser/src/index.spec.ts |
Adds timeout and worker replacement tests. | moderate (3 votes, also line 233): restore or delete TEST_WORKER_SCRIPT_URL when initially unset. moderate (1 vote): the cleanup truthiness check leaks the override when initially undefined. nit (2 votes): correct the comment to say the request “times out.” |
packages/shell-bson-parser/package.json |
Adds the worker test dependency. | No findings. |
packages/shell-bson-parser/.eslintrc.cjs |
Excludes fixture files from ESLint. | No findings. |
package-lock.json |
Locks the new dependency. | No findings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+50
to
+52
| if (initialWorkerScriptUrl) { | ||
| process.env.TEST_WORKER_SCRIPT_URL = initialWorkerScriptUrl; | ||
| } |
Comment on lines
+120
to
+123
| terminateWorker( | ||
| new Error(`Worker execution timed out after ${executionTimeoutMs}ms`), | ||
| ); | ||
| }, executionTimeoutMs); |
| }); | ||
|
|
||
| it('spins up a fresh worker for the next call after a timeout kill', async function () { | ||
| await callWorker([1000]).catch(() => {}); // timeouts out |
| const promise = new Promise<T>((resolve, reject) => { | ||
| pending.set(id, { resolve, reject }); | ||
| const executionTimer = setTimeout(() => { | ||
| // Terminate the worker is this message is taking too long to execute, |
mabaasit
force-pushed
the
timout-long-running-worker
branch
from
September 23, 2026 14:48
887c77d to
b96a2f9
Compare
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.
Description
COMPASS-11147
Open Questions
Checklist
Stack created with GitHub Stacks CLI • Give Feedback 💬