refactor(runtime)!: collapse the subprocess transport stack — single-attempt requests, merged pool, always-on framing#297
Conversation
github-actions
Bot
added
area:docs
|
Warning Review limit reached
Next review available in: 47 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe change removes chunking negotiation from subprocess transport, makes ChangesTransport framing and contracts
Runtime pooling and execution
Validation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
coderabbitai
Bot
added
area:codec
The baseline arm asserted supportsChunking === false, a state that no longer exists — framing is always on and the capability is always true. The comparison itself is unchanged: the high ceiling makes the 20 MiB response one frame, so the arm still measures the effectively single-frame path. (Slipped local gates: the perf suite only runs with TYWRAP_PERF_BUDGETS=1, which check:all leaves off.)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/pooled-transport.ts`:
- Around line 135-158: Strengthen constructor validation in the options
initialization around maxWorkers and maxConcurrentPerWorker: reject non-finite
maxWorkers values, including NaN, and require maxConcurrentPerWorker to be a
finite positive number. Throw BridgeExecutionError during construction for
invalid values so acquire() cannot enter a permanently queuing state, while
preserving valid defaults and existing minWorkers validation.
- Around line 202-228: Update doDispose() to iterate over a snapshot of
this.workers, created before awaiting worker disposal, so concurrent
removeWorker() mutations cannot skip workers or their errors. Continue clearing
the live workers array after processing and preserve the existing error
aggregation behavior.
In `@src/runtime/subprocess-transport.ts`:
- Around line 480-539: Bound the active-dispatch drain in reserveDispatch so a
hung request cannot block restart indefinitely or serialize all later
reservations behind dispatchMutex. Replace the unbounded dispatchDrainWaiters
wait with the transport’s established timeout mechanism, then proceed with a
forced restart after the timeout while preserving cancellation and
lifecycle-ending checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: df35038d-931f-49c9-b598-142226cd2a6d
📒 Files selected for processing (29)
docs/public/llms-full.txtdocs/reference/env-vars.mddocs/transport-capabilities.mddocs/transport-framing.mdruntime/frame_codec.pyruntime/python_bridge.pyruntime/tywrap_bridge_core.pysrc/index.tssrc/runtime/bounded-context.tssrc/runtime/node.tssrc/runtime/pooled-transport.tssrc/runtime/pyodide-bootstrap-core.generated.tssrc/runtime/rpc-client.tssrc/runtime/subprocess-transport.tssrc/runtime/transport-pool.tssrc/runtime/transport.tssrc/types/index.tstest/bounded-context.test.tstest/data-plane-perf.test.tstest/pooled-chunking.test.tstest/python/test_frame_codec.pytest/rpc-client.test.tstest/runtime_conformance.test.tstest/runtime_node.test.tstest/transport-chunking.test.tstest/transport-framing.test.tstest/transport-pool.test.tstest/transport-request-chunking.test.tstest/transport.test.ts
💤 Files with no reviewable changes (8)
- src/runtime/transport-pool.ts
- src/index.ts
- docs/reference/env-vars.md
- test/data-plane-perf.test.ts
- test/transport-framing.test.ts
- src/types/index.ts
- runtime/tywrap_bridge_core.py
- test/transport-request-chunking.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: os (windows-latest)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript strict mode and avoid
any; preferunknownwith type guards.
Files:
test/bounded-context.test.tssrc/runtime/pyodide-bootstrap-core.generated.tssrc/runtime/transport.tstest/runtime_node.test.tstest/runtime_conformance.test.tssrc/runtime/node.tssrc/runtime/bounded-context.tstest/rpc-client.test.tssrc/runtime/rpc-client.tstest/transport.test.tssrc/runtime/pooled-transport.tstest/pooled-chunking.test.tstest/transport-chunking.test.tssrc/runtime/subprocess-transport.tstest/transport-pool.test.ts
src/runtime/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Implement runtime bridges and transport code under
src/runtime/.Implement runtime bridge code under
src/runtime/.
Files:
src/runtime/pyodide-bootstrap-core.generated.tssrc/runtime/transport.tssrc/runtime/node.tssrc/runtime/bounded-context.tssrc/runtime/rpc-client.tssrc/runtime/pooled-transport.tssrc/runtime/subprocess-transport.ts
test/runtime_*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place corresponding runtime bridge tests in
test/runtime_*.test.ts.
test/runtime_*.test.ts: Place corresponding runtime bridge tests intest/runtime_*.test.ts.
Include tests for new runtime behavior, following existing runtime test patterns.
Files:
test/runtime_node.test.tstest/runtime_conformance.test.ts
🧠 Learnings (6)
📚 Learning: 2026-01-19T21:48:27.823Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/runtime_bridge_fixtures.test.ts:59-66
Timestamp: 2026-01-19T21:48:27.823Z
Learning: In fixture-based tests (e.g., test/runtime_bridge_fixtures.test.ts) and similar tests in the tywrap repository, prefer early returns when Python or fixture files are unavailable. Do not rely on Vitest dynamic skip APIs; a silent pass is intentional for environments lacking Python/fixtures. Treat missing fixtures as optional soft-guards and ensure the test remains non-disruptive in non-availability scenarios.
Applied to files:
test/bounded-context.test.tstest/runtime_node.test.tstest/runtime_conformance.test.tstest/rpc-client.test.tstest/transport.test.tstest/pooled-chunking.test.tstest/transport-chunking.test.tstest/transport-pool.test.ts
📚 Learning: 2026-01-20T01:34:07.064Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 136
File: src/runtime/node.ts:444-458
Timestamp: 2026-01-20T01:34:07.064Z
Learning: When reviewing promise-based polling patterns (e.g., recursive setTimeout in waitForAvailableWorker functions), ensure that any recursive timer is tracked and cleared on timeout or promise resolution to prevent timer leaks. Do not rely on no-op checks after settlement; verify clearTimeout is invoked in all paths and consider using an explicit cancellation (e.g., AbortController) or a guard to cancel pending timers when the promise settles.
Applied to files:
test/bounded-context.test.tssrc/runtime/pyodide-bootstrap-core.generated.tssrc/runtime/transport.tstest/runtime_node.test.tstest/runtime_conformance.test.tssrc/runtime/node.tssrc/runtime/bounded-context.tstest/rpc-client.test.tssrc/runtime/rpc-client.tstest/transport.test.tssrc/runtime/pooled-transport.tstest/pooled-chunking.test.tstest/transport-chunking.test.tssrc/runtime/subprocess-transport.tstest/transport-pool.test.ts
📚 Learning: 2026-01-20T18:37:05.670Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: src/runtime/process-io.ts:37-40
Timestamp: 2026-01-20T18:37:05.670Z
Learning: In the tywrap repo, ESLint is used for linting (not Biome). Do not flag regex literals that contain control characters (e.g., \u001b, \u0000-\u001F) as lint errors, since the current ESLint configuration allows them. When reviewing TypeScript files (e.g., src/**/*.ts), rely on ESLint results and avoid raising issues about these specific control-character regex literals unless there is a competing config change or a policy requiring explicit escaping.
Applied to files:
test/bounded-context.test.tssrc/runtime/pyodide-bootstrap-core.generated.tssrc/runtime/transport.tstest/runtime_node.test.tstest/runtime_conformance.test.tssrc/runtime/node.tssrc/runtime/bounded-context.tstest/rpc-client.test.tssrc/runtime/rpc-client.tstest/transport.test.tssrc/runtime/pooled-transport.tstest/pooled-chunking.test.tstest/transport-chunking.test.tssrc/runtime/subprocess-transport.tstest/transport-pool.test.ts
📚 Learning: 2026-01-20T16:00:49.829Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 152
File: docs/adr/002-bridge-protocol.md:203-211
Timestamp: 2026-01-20T16:00:49.829Z
Learning: In the BridgeProtocol implementation (tywrap), reject Map and Set explicitly before serialization (e.g., in safeStringify or the serialization entrypoint) with a clear error like "Bridge protocol does not support Map/Set values". Do not rely on post-hoc checks of non-string keys at the point of JSON.stringify, since Maps/Sets cannot round-trip. This should be enforced at the boundary where data is prepared for serialization to ensure deterministic errors and prevent invalid data from propagating.
Applied to files:
test/bounded-context.test.tssrc/runtime/pyodide-bootstrap-core.generated.tssrc/runtime/transport.tstest/runtime_node.test.tstest/runtime_conformance.test.tssrc/runtime/node.tssrc/runtime/bounded-context.tstest/rpc-client.test.tssrc/runtime/rpc-client.tstest/transport.test.tssrc/runtime/pooled-transport.tstest/pooled-chunking.test.tstest/transport-chunking.test.tssrc/runtime/subprocess-transport.tstest/transport-pool.test.ts
📚 Learning: 2026-01-19T21:14:29.869Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:375-385
Timestamp: 2026-01-19T21:14:29.869Z
Learning: In the runtime env-var parsing (e.g., in src/runtime/bridge-core.ts and similar modules), adopt a tolerant, best-effort policy: numeric env values should parse by taking the leading numeric portion (e.g., TYWRAP_CODEC_MAX_BYTES=1024abc -> 1024). Only reject clearly invalid values (non-numeric start or <= 0). This reduces surprising failures from minor typos. Add tests to cover partial numeric prefixes, and ensure downstream logic documents the trusted range; consider a small helper to extract a positive integer from a string with a safe fallback.
Applied to files:
src/runtime/pyodide-bootstrap-core.generated.tssrc/runtime/transport.tssrc/runtime/node.tssrc/runtime/bounded-context.tssrc/runtime/rpc-client.tssrc/runtime/pooled-transport.tssrc/runtime/subprocess-transport.ts
📚 Learning: 2026-01-19T21:14:35.390Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:35.390Z
Learning: In src/runtime/bridge-core.ts and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (such as runtime, shape, or error-payload checks) inside tight loops unless the protocol boundary is untrusted or there is a concrete bug report. The Python bridge protocol is controlled via tests, so these checks add unnecessary branching overhead without meaningful benefit. Apply this guidance to other hot-path runtime loops in src/runtime/**/*.ts, and re-enable additional validations only when a documented risk or failure scenario is identified. Ensure tests cover protocol validation where applicable.
Applied to files:
src/runtime/pyodide-bootstrap-core.generated.tssrc/runtime/transport.tssrc/runtime/node.tssrc/runtime/bounded-context.tssrc/runtime/rpc-client.tssrc/runtime/pooled-transport.tssrc/runtime/subprocess-transport.ts
🪛 Ruff (0.15.20)
runtime/python_bridge.py
[warning] 235-235: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 237-237: Avoid specifying long messages outside the exception class
(TRY003)
[warning] 403-403: Missing return type annotation for private function _accept_request_frame
(ANN202)
[warning] 415-415: Missing return type annotation for private function _try_parse_frame_line
(ANN202)
🔇 Additional comments (25)
src/runtime/bounded-context.ts (1)
5-5: LGTM!Also applies to: 53-59, 378-378, 398-407, 476-477
test/bounded-context.test.ts (1)
582-607: LGTM!test/transport-chunking.test.ts (1)
9-9: LGTM!Also applies to: 116-116, 218-221
test/transport-pool.test.ts (1)
2-13: LGTM!Also applies to: 26-26, 77-79, 92-106, 115-115, 124-124, 128-128, 135-135, 143-143, 153-153, 176-176, 199-199, 220-220, 234-234, 244-244, 261-261, 278-278, 296-296, 319-319, 351-351, 375-375, 398-398, 407-407, 428-428, 438-438, 455-455, 475-475, 503-503, 522-522, 543-543, 553-553, 578-578, 603-603, 642-642, 670-670, 692-692, 702-702, 729-729, 761-761, 789-789, 802-802, 828-828, 859-859, 882-882, 909-909, 919-919, 939-939, 956-956, 974-974, 991-991, 1007-1007, 1016-1016, 1031-1031, 1063-1063, 1073-1073, 1103-1103, 1115-1115, 1137-1137, 1164-1164, 1180-1180, 1205-1205, 1242-1242, 1279-1279, 1308-1308, 1318-1318, 1350-1350, 1362-1362
test/transport.test.ts (1)
73-73: LGTM!Also applies to: 434-439, 478-632, 1535-1535, 1554-1554, 1600-1600
test/pooled-chunking.test.ts (1)
7-14: LGTM!Also applies to: 33-33, 119-120, 138-139, 154-155, 186-198, 277-281, 293-293
test/python/test_frame_codec.py (1)
469-479: LGTM!Also applies to: 480-495
test/rpc-client.test.ts (1)
7-7: LGTM!Also applies to: 22-25, 626-627, 648-648, 679-679, 707-707, 736-736, 766-766, 787-787, 828-828, 864-864, 905-905, 978-978, 1023-1023
test/runtime_conformance.test.ts (1)
1324-1325: LGTM!Also applies to: 1346-1349
test/runtime_node.test.ts (1)
1358-1363: LGTM!Also applies to: 1372-1372
docs/public/llms-full.txt (1)
2387-2390: LGTM!Also applies to: 2678-2691, 2712-2739, 2751-2773, 2801-2808, 2836-2838, 2860-2874, 2884-2886, 2905-2915, 2928-2976, 3016-3017, 3392-3393
runtime/frame_codec.py (1)
411-414: LGTM!runtime/python_bridge.py (2)
227-241: LGTM!Also applies to: 277-313, 350-391, 395-432, 500-536
48-48: 🗄️ Data Integrity & IntegrationNo dangling
FRAME_PROTOCOL_IDorTRANSPORT_INFOreferences remain.runtime/python_bridge.pyno longer contains those symbols or anytransport_infocall sites.> Likely an incorrect or invalid review comment.src/runtime/pooled-transport.ts (1)
160-179: LGTM!Also applies to: 230-337, 339-383, 385-565
src/runtime/node.ts (1)
26-26: 📐 Maintainability & Code QualityNo remaining references to the removed option or module path.
No remaining references to the removedenableChunkingoption or the oldtransport-pool.jspath.src/runtime/pyodide-bootstrap-core.generated.ts (1)
12-12: 🗄️ Data Integrity & IntegrationGenerated bootstrap matches the source exactly.
> Likely an incorrect or invalid review comment.docs/transport-capabilities.md (1)
5-100: LGTM!docs/transport-framing.md (1)
9-225: LGTM!Docs consistently describe the always-on framing behavior and match the implementation in
subprocess-transport.ts(fixedframeBytes/maxLineLengthceiling, eagerReassembler, reject+restart on framing failure).src/runtime/transport.ts (1)
203-247: LGTM!Doc comments accurately reflect
SubprocessTransport.capabilities()(supportsChunking: true,maxFrameBytes: this.maxLineLength).src/runtime/rpc-client.ts (2)
60-196: LGTM!Removal of the transport-negotiation validation block is consistent with the always-on-framing model, and is backed by
test/transport-framing.test.tsconfirminginfo.transportis now expected to beundefined.
289-329: LGTM!JSDoc updates correctly drop "retry" language to match the single-attempt execution model.
src/runtime/subprocess-transport.ts (3)
541-565: LGTM!
isLifecycleEndingguards, reassembler reset on restart/dispose, and the always-on frame routing inhandleResponseLine/handleResponseFrameare correctly wired.Also applies to: 742-775, 846-940
599-617: LGTM!
doDisposecorrectly rejects all pending requests (releasing any held dispatch reservations viaresponse.finally) before fencing onwithDispatchMutex, which prevents the dispose path itself from deadlocking against a stuck restart.Also applies to: 336-478
648-657: 🗄️ Data Integrity & IntegrationNo action needed —
runtime/python_bridge.pyalready accepts--tywrap-max-frame-bytesand reads the following integer value.
| // Validate required options | ||
| if (typeof options.createTransport !== 'function') { | ||
| throw new BridgeExecutionError('createTransport must be a function'); | ||
| } | ||
| const maxWorkers = options.maxWorkers ?? 1; | ||
| if (typeof maxWorkers !== 'number' || maxWorkers < 1) { | ||
| throw new BridgeExecutionError('maxWorkers must be a positive number'); | ||
| } | ||
|
|
||
| this.poolOptions = { | ||
| const minWorkers = options.minWorkers ?? 0; | ||
| if (minWorkers > maxWorkers) { | ||
| throw new BridgeExecutionError('minWorkers cannot exceed maxWorkers'); | ||
| } | ||
|
|
||
| this.options = { | ||
| createTransport: options.createTransport, | ||
| maxWorkers: options.maxWorkers ?? 1, | ||
| minWorkers: options.minWorkers ?? 0, | ||
| maxWorkers, | ||
| minWorkers, | ||
| queueTimeoutMs: options.queueTimeoutMs ?? 30000, | ||
| maxConcurrentPerWorker: options.maxConcurrentPerWorker ?? 1, | ||
| onWorkerReady: options.onWorkerReady, | ||
| onReplacementWorkerReady: options.onReplacementWorkerReady, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Strengthen constructor validation to actually fail fast.
typeof maxWorkers !== 'number' || maxWorkers < 1 lets NaN through (since NaN < 1 is false), which then permanently disables worker creation (every acquire() queues and times out) rather than throwing at construction — defeating the "fail fast" intent. There's also no validation on maxConcurrentPerWorker: 0 or a negative value makes findAvailableWorker() never succeed once maxWorkers is reached, causing every subsequent request to queue until timeout instead of failing immediately.
🛡️ Proposed fix
const maxWorkers = options.maxWorkers ?? 1;
- if (typeof maxWorkers !== 'number' || maxWorkers < 1) {
+ if (!Number.isFinite(maxWorkers) || maxWorkers < 1) {
throw new BridgeExecutionError('maxWorkers must be a positive number');
}
const minWorkers = options.minWorkers ?? 0;
if (minWorkers > maxWorkers) {
throw new BridgeExecutionError('minWorkers cannot exceed maxWorkers');
}
+
+ const maxConcurrentPerWorker = options.maxConcurrentPerWorker ?? 1;
+ if (!Number.isFinite(maxConcurrentPerWorker) || maxConcurrentPerWorker < 1) {
+ throw new BridgeExecutionError('maxConcurrentPerWorker must be a positive number');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Validate required options | |
| if (typeof options.createTransport !== 'function') { | |
| throw new BridgeExecutionError('createTransport must be a function'); | |
| } | |
| const maxWorkers = options.maxWorkers ?? 1; | |
| if (typeof maxWorkers !== 'number' || maxWorkers < 1) { | |
| throw new BridgeExecutionError('maxWorkers must be a positive number'); | |
| } | |
| this.poolOptions = { | |
| const minWorkers = options.minWorkers ?? 0; | |
| if (minWorkers > maxWorkers) { | |
| throw new BridgeExecutionError('minWorkers cannot exceed maxWorkers'); | |
| } | |
| this.options = { | |
| createTransport: options.createTransport, | |
| maxWorkers: options.maxWorkers ?? 1, | |
| minWorkers: options.minWorkers ?? 0, | |
| maxWorkers, | |
| minWorkers, | |
| queueTimeoutMs: options.queueTimeoutMs ?? 30000, | |
| maxConcurrentPerWorker: options.maxConcurrentPerWorker ?? 1, | |
| onWorkerReady: options.onWorkerReady, | |
| onReplacementWorkerReady: options.onReplacementWorkerReady, | |
| }; | |
| } | |
| // Validate required options | |
| if (typeof options.createTransport !== 'function') { | |
| throw new BridgeExecutionError('createTransport must be a function'); | |
| } | |
| const maxWorkers = options.maxWorkers ?? 1; | |
| if (!Number.isFinite(maxWorkers) || maxWorkers < 1) { | |
| throw new BridgeExecutionError('maxWorkers must be a positive number'); | |
| } | |
| const minWorkers = options.minWorkers ?? 0; | |
| if (minWorkers > maxWorkers) { | |
| throw new BridgeExecutionError('minWorkers cannot exceed maxWorkers'); | |
| } | |
| const maxConcurrentPerWorker = options.maxConcurrentPerWorker ?? 1; | |
| if (!Number.isFinite(maxConcurrentPerWorker) || maxConcurrentPerWorker < 1) { | |
| throw new BridgeExecutionError('maxConcurrentPerWorker must be a positive number'); | |
| } | |
| this.options = { | |
| createTransport: options.createTransport, | |
| maxWorkers, | |
| minWorkers, | |
| queueTimeoutMs: options.queueTimeoutMs ?? 30000, | |
| maxConcurrentPerWorker: options.maxConcurrentPerWorker ?? 1, | |
| onWorkerReady: options.onWorkerReady, | |
| onReplacementWorkerReady: options.onReplacementWorkerReady, | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/pooled-transport.ts` around lines 135 - 158, Strengthen
constructor validation in the options initialization around maxWorkers and
maxConcurrentPerWorker: reject non-finite maxWorkers values, including NaN, and
require maxConcurrentPerWorker to be a finite positive number. Throw
BridgeExecutionError during construction for invalid values so acquire() cannot
enter a permanently queuing state, while preserving valid defaults and existing
minWorkers validation.
| protected async doDispose(): Promise<void> { | ||
| if (this.pool) { | ||
| await this.pool.dispose(); | ||
| this.pool = undefined; | ||
| // Reject all waiters in the queue | ||
| for (const waiter of this.waitQueue) { | ||
| clearTimeout(waiter.timer); | ||
| waiter.reject(new BridgeExecutionError('Pool disposed')); | ||
| } | ||
| this.waitQueue.length = 0; | ||
|
|
||
| // Dispose all workers | ||
| const errors: Error[] = []; | ||
| for (const worker of this.workers) { | ||
| try { | ||
| await worker.transport.dispose(); | ||
| } catch (e) { | ||
| errors.push(e instanceof Error ? e : new Error(String(e))); | ||
| } | ||
| } | ||
| this.workers.length = 0; | ||
|
|
||
| // Report errors if any | ||
| if (errors.length === 1) { | ||
| throw errors[0]; | ||
| } | ||
| if (errors.length > 1) { | ||
| throw new AggregateError(errors, 'Multiple errors during worker disposal'); | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'async dispose\(' -A 20 src/runtime/bounded-context.tsRepository: bbopen/tywrap
Length of output: 810
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,320p' src/runtime/pooled-transport.tsRepository: bbopen/tywrap
Length of output: 10262
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/runtime/pooled-transport.ts --view expandedRepository: bbopen/tywrap
Length of output: 2403
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'removeWorker|publishAvailableWorker|isShuttingDown|withWorker|dispose\(' src/runtime/pooled-transport.ts -A 30 -B 10Repository: bbopen/tywrap
Length of output: 14495
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure first, then inspect the specific lifecycle methods.
ast-grep outline src/runtime/pooled-transport.ts --view expanded
printf '\n--- relevant matches ---\n'
rg -n 'removeWorker|publishAvailableWorker|isShuttingDown|withWorker|dispose\(' src/runtime/pooled-transport.ts -A 30 -B 10Repository: bbopen/tywrap
Length of output: 16773
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether transport disposal is documented as idempotent or if pooled disposal
# already guards against double-dispose elsewhere.
rg -n 'interface Transport|dispose\(\): Promise<void>|idempotent|already disposed' src/runtime -A 20 -B 10Repository: bbopen/tywrap
Length of output: 17246
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
# Probe array-iteration behavior with splice during an awaited disposal-like loop.
import subprocess, textwrap, json, tempfile, os, sys
js = r"""
async function test() {
const workers = [{id:'A'}, {id:'B'}, {id:'C'}];
const seen = [];
let removed = false;
async function disposeWorker(w) {
seen.push('start:' + w.id);
if (!removed && w.id === 'A') {
removed = true;
// remove middle worker during the await gap
workers.splice(1, 1);
seen.push('spliced');
}
await Promise.resolve();
seen.push('end:' + w.id);
}
for (const worker of workers) {
await disposeWorker(worker);
}
console.log(JSON.stringify({seen, remaining: workers.map(w => w.id)}));
}
test();
"""
out = subprocess.check_output(["node", "-e", js], text=True)
print(out)
PYRepository: bbopen/tywrap
Length of output: 231
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- transport interface / dispose docs ---'
rg -n 'interface Transport|dispose\(\): Promise<void>|idempotent|already disposed' src/runtime -A 20 -B 10
printf '\n%s\n' '--- iteration probe ---'
node - <<'JS'
async function main() {
const workers = [{id: 'A'}, {id: 'B'}, {id: 'C'}];
const seen = [];
let removed = false;
async function disposeWorker(worker) {
seen.push(`start:${worker.id}`);
if (!removed && worker.id === 'A') {
removed = true;
workers.splice(1, 1); // remove B while A is "disposing"
seen.push('spliced');
}
await Promise.resolve();
seen.push(`end:${worker.id}`);
}
for (const worker of workers) {
await disposeWorker(worker);
}
console.log(JSON.stringify({ seen, remaining: workers.map(w => w.id) }));
}
main();
JSRepository: bbopen/tywrap
Length of output: 17393
Iterate over a snapshot of this.workers in doDispose(). A concurrent removeWorker() can splice the live array during the await, which can skip a worker and omit its dispose error from this pass.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/pooled-transport.ts` around lines 202 - 228, Update doDispose()
to iterate over a snapshot of this.workers, created before awaiting worker
disposal, so concurrent removeWorker() mutations cannot skip workers or their
errors. Continue clearing the live workers array after processing and preserve
the existing error aggregation behavior.
| /** | ||
| * Atomically reserve the current process generation for one request. A | ||
| * threshold restart waits for every request reserved on the old generation | ||
| * to settle before killing it; later callers remain queued behind this lock. | ||
| */ | ||
| private async withDispatchMutex<T>(operation: () => Promise<T>): Promise<T> { | ||
| let unlock!: () => void; | ||
| const previous = this.dispatchMutex; | ||
| this.dispatchMutex = new Promise<void>(resolve => { | ||
| unlock = resolve; | ||
| }); | ||
| await previous; | ||
| try { | ||
| return await operation(); | ||
| } finally { | ||
| unlock(); | ||
| } | ||
| } | ||
|
|
||
| private async reserveDispatch(isLive: () => boolean): Promise<boolean> { | ||
| return this.withDispatchMutex(async () => { | ||
| // Cancellation may win while this request waits behind an earlier | ||
| // generation. It must consume neither a request slot nor a restart. | ||
| if (!isLive()) { | ||
| return false; | ||
| } | ||
| if (this.isLifecycleEnding()) { | ||
| throw new BridgeDisposedError('Transport has been disposed'); | ||
| } | ||
| if ( | ||
| this.needsRestart || | ||
| (this.restartAfterRequests > 0 && this.requestCount >= this.restartAfterRequests) | ||
| ) { | ||
| if (this.activeDispatches > 0) { | ||
| await new Promise<void>(resolve => this.dispatchDrainWaiters.push(resolve)); | ||
| } | ||
| // The owner or transport can settle while waiting for the old | ||
| // generation to drain. Do not restart for work that no longer exists. | ||
| if (!isLive()) { | ||
| return false; | ||
| } | ||
| if (this.isLifecycleEnding()) { | ||
| throw new BridgeDisposedError('Transport has been disposed'); | ||
| } | ||
| await this.restartProcess(); | ||
| } | ||
| this.activeDispatches++; | ||
| this.requestCount++; | ||
| return true; | ||
| }); | ||
| } | ||
|
|
||
| private releaseDispatch(): void { | ||
| this.activeDispatches = Math.max(0, this.activeDispatches - 1); | ||
| if (this.activeDispatches === 0) { | ||
| for (const resolve of this.dispatchDrainWaiters.splice(0)) { | ||
| resolve(); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the referenced area and related symbols.
FILE="src/runtime/subprocess-transport.ts"
wc -l "$FILE"
echo '--- relevant symbols ---'
rg -n "dispatchMutex|dispatchDrainWaiters|releaseOwnedReservation|restartAfterRequests|needsRestart|timeoutMs|abort|dispose|restartProcess|send\(" "$FILE"
echo '--- lines 1-260 ---'
sed -n '1,260p' "$FILE"
echo '--- lines 260-620 ---'
sed -n '260,620p' "$FILE"Repository: bbopen/tywrap
Length of output: 26520
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focused inspection of the transport implementation and nearby helpers.
FILE="src/runtime/subprocess-transport.ts"
echo '--- outline ---'
ast-grep outline "$FILE" --view expanded || true
echo '--- targeted slices ---'
sed -n '1,220p' "$FILE"
echo '---'
sed -n '220,420p' "$FILE"
echo '---'
sed -n '420,620p' "$FILE"Repository: bbopen/tywrap
Length of output: 27183
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/runtime/subprocess-transport.ts"
echo '--- restartProcess and related state transitions ---'
sed -n '720,790p' "$FILE"
echo '--- write path reservation/release around send completion ---'
sed -n '1110,1265p' "$FILE"
echo '--- process exit/error handling and release paths ---'
sed -n '980,1065p' "$FILE"Repository: bbopen/tywrap
Length of output: 11688
Bound restart drain so one hung request can’t stall the transport. A request with no timeout or abort can hold its reservation forever; once a later send() hits needsRestart or restartAfterRequests, it waits on dispatchDrainWaiters while still holding dispatchMutex, and every later reservation queues behind that chain. Add a bounded drain timeout or forced-restart fallback.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/runtime/subprocess-transport.ts` around lines 480 - 539, Bound the
active-dispatch drain in reserveDispatch so a hung request cannot block restart
indefinitely or serialize all later reservations behind dispatchMutex. Replace
the unbounded dispatchDrainWaiters wait with the transport’s established timeout
mechanism, then proceed with a forced restart after the timeout while preserving
cancellation and lifecycle-ending checks.
1 participant
Summary
DisposableBaselifecycle-only and remove retry/backoff behavior.TransportPoolintoPooledTransport, removing a pass-through layer.process generation before replacement.
tywrap-frame/1framing unconditional for packaged subprocess bridges.What broke and why it cannot now
The retry loop reused one stamped request ID across attempts, so a late response
could satisfy the wrong attempt. Every RPC now gets exactly one transport send;
timeouts and errors surface unchanged and no retry ID can remain in flight.
Threshold restarts previously raced with concurrent sends. Dispatch now reserves
a process generation under a mutex, and restart waits for that generation's
active reservations to settle before killing it. A timed-out queued reservation
cannot release another request or leak its reservation.
Python request framing is serialized. A new frame burst or complete request now
drops any abandoned partial burst, preventing timeout/abort reassembly leaks.
Because the bridge uses a blocking stdin loop, an abandoned partial buffer may
remain while fully idle; the next request or process restart frees it.
BREAKING CHANGE
Requests are no longer retried by
DisposableBase; retry-related executionoptions are removed and each request makes one attempt.
enableChunkingandTYWRAP_TRANSPORT_CHUNKING,TYWRAP_TRANSPORT_FRAME_PROTOCOL, andTYWRAP_TRANSPORT_MAX_FRAME_BYTESareremoved. Packaged subprocess framing is always enabled and
maxLineLengthisthe per-frame wire ceiling; codec
maxPayloadBytesremains the logical payloadand reassembly ceiling.
Pool-limit validation is intentionally fail-fast at
NodeBridgeconstruction;invalid values such as
maxProcesses: 0now throw synchronously instead ofwaiting for asynchronous initialization.