Repository navigation
feat(bson-parsing): terminate worker if execution exceeds 5sec COMPASS-11147 - #893
Merged
Merged
Conversation
Anemy
reviewed
Sep 21, 2026
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); |
mabaasit
force-pushed
the
timout-long-running-worker
branch
2 times, most recently
from
September 28, 2026 12:20
b96a2f9 to
0abaf9e
Compare
mabaasit
force-pushed
the
timout-long-running-worker
branch
from
September 28, 2026 12:33
0abaf9e to
e27980e
Compare
mabaasit
force-pushed
the
timout-long-running-worker
branch
from
September 28, 2026 12:38
e27980e to
b211fce
Compare
mabaasit
marked this pull request as ready for review
September 28, 2026 13:37
mabaasit
force-pushed
the
timout-long-running-worker
branch
from
September 28, 2026 15:28
3de38eb to
1a46cde
Compare
mabaasit
force-pushed
the
timout-long-running-worker
branch
2 times, most recently
from
September 29, 2026 08:10
14a71f4 to
697c416
Compare
mabaasit
force-pushed
the
timout-long-running-worker
branch
from
September 29, 2026 08:35
697c416 to
fcd4d37
Compare
mabaasit
force-pushed
the
timout-long-running-worker
branch
from
October 5, 2026 13:00
fcd4d37 to
b654b01
Compare
mabaasit
force-pushed
the
timout-long-running-worker
branch
from
October 5, 2026 15:47
a932a6b to
72dbb6f
Compare
mabaasit
force-pushed
the
timout-long-running-worker
branch
from
October 5, 2026 19:44
72dbb6f to
15f62f1
Compare
mabaasit
force-pushed
the
timout-long-running-worker
branch
from
October 5, 2026 20:30
15f62f1 to
1daa2a9
Compare
mabaasit
force-pushed
the
timout-long-running-worker
branch
2 times, most recently
from
October 9, 2026 23:03
be00ce4 to
a2f74b5
Compare
mabaasit
commented
Oct 12, 2026
| import type { WorkerResponse } from './worker-types.js'; | ||
|
|
||
| /** Default execution timeout for worker requests */ | ||
| const DEFAULT_EXECUTION_TIMEOUT_MS = 120_000; |
Collaborator
Author
There was a problem hiding this comment.
(project sync note) We reduce this number to 5secs
nbbeeken
changed the title
mabaasit
force-pushed
the
timout-long-running-worker
branch
from
October 12, 2026 21:18
a2f74b5 to
407bc73
Compare
|
coverage: 79.657%. remained the same — timout-long-running-worker into restrict-worker-scope |
|
coverage: 79.97% (+0.01%) from 79.958% — timout-long-running-worker into restrict-worker-scope |
Anemy
approved these changes
Oct 13, 2026
mabaasit
force-pushed
the
timout-long-running-worker
branch
from
October 14, 2026 09:52
407bc73 to
1ac9267
Compare
mabaasit
force-pushed
the
timout-long-running-worker
branch
from
October 15, 2026 10:54
1ac9267 to
578ec8d
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 💬