refactor: unify NodeBridge and OptimizedNodeBridge (ADR-0001) #136
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
This completes the bridge unification work outlined in ADR-0001. NodeBridge now supports both single-process (default) and multi-process pooling modes, replacing the separate OptimizedNodeBridge implementation. Key changes: - NodeBridge is now the unified implementation with optional pooling - Default behavior unchanged: single-process mode (minProcesses=1, maxProcesses=1) - Multi-process pooling enabled via minProcesses/maxProcesses options - OptimizedNodeBridge is now a deprecated re-export alias - ProcessPoolOptions is deprecated in favor of NodeBridgeOptions Technical improvements: - Single-process mode allows concurrent requests via BridgeCore multiplexing - Worker quarantine and replacement on timeout/error (fixes #101) - Consistent error types across all modes (BridgeDisposedError, BridgeTimeoutError) - Process pool scaling under load Tests: - Added 8 multi-worker adversarial tests for pool behavior - Updated existing tests to use unified imports - All 747+ tests pass Migration: No breaking changes. Existing code using NodeBridge or OptimizedNodeBridge continues to work. OptimizedNodeBridge users can migrate to NodeBridge with the same options. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughIntroduces a pooled multi-process NodeBridge: configurable worker pool (spawn/terminate/quarantine), per-worker stats, optional result caching, warmup, cleanup scheduler, public getStats/dispose, and re-exports OptimizedNodeBridge/ProcessPoolOptions as aliases to NodeBridge/NodeBridgeOptions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Bridge as NodeBridge<br/>(Pool Manager)
participant Selector as WorkerSelector
participant Pool as WorkerPool
participant Worker as PythonProcess
Client->>Bridge: executeRequest(payload)
Bridge->>Selector: selectOptimalWorker()
alt idle worker available
Selector-->>Bridge: chosen Worker
else under max capacity
Selector->>Pool: spawnProcess()
Pool->>Worker: start
Worker-->>Pool: ready
Pool-->>Bridge: new Worker
else all busy
Bridge->>Bridge: waitForAvailableWorker()
Bridge-->>Selector: worker freed
end
alt cache hit
Bridge-->>Client: cached result
else execute request
Bridge->>Worker: sendToWorker(payload)
Worker->>Worker: run python code
alt success
Worker-->>Bridge: result
Bridge->>Bridge: updateStats(), maybe cache
else timeout/crash
Bridge->>Bridge: quarantineWorker(Worker)
Bridge->>Pool: terminateWorker(Worker)
Pool->>Pool: spawn replacement
end
Bridge-->>Client: result/error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
coderabbitai
bot
added
area:runtime-node
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05b6e2f983
ℹ️ 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".
![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
🤖 Fix all issues with AI agents
In `@src/runtime/node.ts`:
- Around line 444-458: The waitForAvailableWorker implementation in
waitForAvailableWorker currently hardcodes a 5000ms internal timeout and rejects
with a generic Error; update it to use the provided timeoutMs (or fallback to
this.options.timeoutMs / a new poolWaitTimeout option) for the internal timer,
and reject with the BridgeTimeoutError type instead of Error. Locate
waitForAvailableWorker and its use of selectOptimalWorker, ensure clearTimeout
is called before resolving or rejecting, and construct BridgeTimeoutError with a
descriptive message that includes the timeout value so callers receive the
consistent error type used elsewhere.
- Around line 238-249: The current init() returns early when
this.processPool.length >= this.options.minProcesses which can mask prior
doInit() failures; update init() to only treat the pool-size check as sufficient
if a success flag is set (e.g., this.initialized or this.bridgeInfo is present)
and ensure initPromise is cleared on failure. Specifically: (1) in doInit() set
a success marker (this.initialized = true or set this.bridgeInfo) at the end of
a successful initialization; (2) in init() change the early-return condition to
require that marker in addition to processPool length; and (3) wrap the
this.initPromise = this.doInit() assignment so that if the promise rejects you
clear this.initPromise and do not set the success marker, then rethrow the
error; this ensures subsequent init() calls will retry rather than silently
succeed.
In `@test/adversarial_playground.test.ts`:
- Around line 646-676: The recovery assertions that verify cleanup and pool
functionality (the delay(200) and the subsequent callAdversarial(bridge, 'echo',
['after-timeout']) expect) should be moved into the finally block so they always
run; keep the slow/fast request setup and the timeout expectation inside the
try, and in finally perform the cleanup wait (delay(200)) and the pool recovery
check using callAdversarial(bridge, 'echo', ['after-timeout']) with its expect
to ensure the bridge (created via createPooledBridge) remains functional even if
earlier assertions fail.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
src/runtime/node.tssrc/runtime/optimized-node.tstest/adversarial_playground.test.tstest/optimized-node.test.tstest/performance-integration.test.skip.tstest/runtime_bridge_fixtures.test.tstest/runtime_node.test.ts
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/adversarial_playground.test.ts:339-360
Timestamp: 2026-01-19T21:00:30.825Z
Learning: In adversarial tests for the bridge (test/adversarial_playground.test.ts), recovery assertions should be kept in the finally block adjacent to cleanup to verify resilience under stress, even if that means post-timeout checks might mask the original error. The goal is to surface recovery failures as legitimate test failures.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
📚 Learning: 2026-01-19T21:00:30.825Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/adversarial_playground.test.ts:339-360
Timestamp: 2026-01-19T21:00:30.825Z
Learning: In adversarial tests for the bridge (test/adversarial_playground.test.ts), recovery assertions should be kept in the finally block adjacent to cleanup to verify resilience under stress, even if that means post-timeout checks might mask the original error. The goal is to surface recovery failures as legitimate test failures.
Applied to files:
test/runtime_node.test.tstest/runtime_bridge_fixtures.test.tstest/adversarial_playground.test.ts
📚 Learning: 2026-01-19T21:48:45.693Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
Applied to files:
test/runtime_node.test.tssrc/runtime/optimized-node.tstest/runtime_bridge_fixtures.test.tstest/optimized-node.test.tstest/adversarial_playground.test.tssrc/runtime/node.ts
📚 Learning: 2026-01-19T21:14:40.872Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Applied to files:
test/runtime_node.test.tstest/runtime_bridge_fixtures.test.tstest/adversarial_playground.test.ts
📚 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/runtime_node.test.tstest/runtime_bridge_fixtures.test.tstest/optimized-node.test.tstest/adversarial_playground.test.ts
📚 Learning: 2026-01-19T21:00:52.689Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/fixtures/out_of_order_bridge.py:29-48
Timestamp: 2026-01-19T21:00:52.689Z
Learning: In `test/fixtures/out_of_order_bridge.py`, the fixture intentionally leaves a pending request unanswered at EOF to simulate missing/out-of-order responses and validate bridge behavior when requests never complete; this is the exact failure mode being tested and must be preserved.
Applied to files:
test/runtime_node.test.tstest/runtime_bridge_fixtures.test.tstest/adversarial_playground.test.ts
📚 Learning: 2026-01-19T21:48:35.351Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/runtime_bridge_fixtures.test.ts:59-66
Timestamp: 2026-01-19T21:48:35.351Z
Learning: In test/runtime_bridge_fixtures.test.ts and similar fixture-based tests in the tywrap repository, early returns are preferred over explicit test skips when Python or fixture files are unavailable. These fixture tests are intentionally optional soft-guards, and the project does not currently rely on Vitest dynamic skip APIs. The silent pass behavior is deliberate for environments without Python/fixtures.
Applied to files:
test/performance-integration.test.skip.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/optimized-node.tssrc/runtime/node.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/optimized-node.tssrc/runtime/node.ts
📚 Learning: 2026-01-19T21:00:52.689Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/fixtures/out_of_order_bridge.py:29-48
Timestamp: 2026-01-19T21:00:52.689Z
Learning: Policy for adversarial test fixtures: adversarial fixtures must preserve the failure mode they exercise, even if that behavior would be "bad" in real code. When reviewing test fixtures with unusual patterns (e.g., missing error handling, intentional protocol violations, unanswered requests), ask for clarifying comments about the intent rather than suggesting normalization or fixes.
Applied to files:
test/runtime_bridge_fixtures.test.tstest/adversarial_playground.test.ts
🧬 Code graph analysis (3)
test/runtime_bridge_fixtures.test.ts (2)
src/runtime/node.ts (2)
NodeBridge(184-889)NodeBridge(895-895)src/runtime/optimized-node.ts (1)
NodeBridge(21-21)
test/adversarial_playground.test.ts (3)
src/runtime/node.ts (2)
NodeBridge(184-889)NodeBridge(895-895)src/runtime/optimized-node.ts (1)
NodeBridge(21-21)tywrap_ir/tywrap_ir/optimized_ir.py (1)
stats(126-138)
src/runtime/node.ts (4)
src/utils/logger.ts (3)
log(141-160)getComponentLogger(203-205)info(166-168)src/runtime/bridge-core.ts (3)
BridgeCore(59-346)RpcRequest(9-14)validateBridgeInfo(348-362)src/utils/codec.ts (2)
autoRegisterArrowDecoder(165-194)decodeValueAsync(481-483)src/utils/cache.ts (1)
globalCache(634-634)
🪛 GitHub Check: lint
src/runtime/node.ts
[failure] 525-525:
'env' is never reassigned. Use 'const' instead
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: bun (macos-latest)
- GitHub Check: os (windows-latest)
🔇 Additional comments (12)
src/runtime/optimized-node.ts (1)
2-21: Deprecation alias looks good.Clear migration guidance and a clean re-export keep backward compatibility intact.
test/runtime_node.test.ts (1)
290-292: Clarifying note aligns with new quarantine behavior.The comment accurately sets expectations for recovery after timeouts.
test/performance-integration.test.skip.ts (1)
16-18: Alias import aligns with unified bridge surface.Good update to keep tests consistent with the new public API.
test/optimized-node.test.ts (1)
5-7: Import alias matches the unified bridge export.Nice alignment with the new API surface.
src/runtime/node.ts (1)
525-528: The code at lines 525–528 already usesconst env, notlet env. The suggestion has either already been implemented or the review comment references outdated code.Likely an incorrect or invalid review comment.
test/runtime_bridge_fixtures.test.ts (7)
6-9: Import pattern is intentional and clear.The comment on line 7 explains the intent: since
OptimizedNodeBridgeis now an alias forNodeBridge, importingNodeBridge as OptimizedNodeBridgefrom the same module demonstrates this equivalence explicitly in the test file. This is a valid approach for parity testing.Note: Users migrating from
OptimizedNodeBridgecan also import fromsrc/runtime/optimized-node.jswhere the alias is re-exported, as shown in the relevant code snippets.
49-93: Well-structured data-driven tests for protocol error handling.The fixture array pattern with
descriptionfields makes test output readable, and iterating over fixtures ensures both bridges are tested identically. The early returns for missing Python/fixtures align with project conventions for soft-guards.
95-130: Good coverage for fragmented write reassembly.The working fixtures tests follow the same pattern as error fixtures and verify that fragmented JSON writes are correctly reassembled. The test structure is consistent and the expected value assertion is appropriate.
141-195: Thorough dispose behavior coverage.These tests comprehensively verify:
- Bridges work before disposal
BridgeDisposedErroris thrown for calls after dispose- Multiple
dispose()calls are safe (idempotent)This directly supports the PR objectives for consistent error types across modes.
197-265: Comprehensive getBridgeInfo parity verification.The tests verify both the structure contract (protocol, version, PID) and behavioral aspects (caching, refresh option). The
try/finallypattern ensures cleanup on test failure. Good coverage for API parity.
291-339: Good parity verification for basic functionality.The tests verify that both bridges produce identical results for simple and sequential calls. The explicit equality check on line 309 (
expect(nodeResult).toBe(optimizedResult)) reinforces parity beyond just asserting the expected value. Good test design.
267-289: Error pattern is correct and test structure is sound.The test expectation
/not found/correctly matches the actual error message thrown by both bridges:"Python bridge script not found at ${this.options.scriptPath}". Whencall()is invoked, it triggersinit()which validates the script path and throws aBridgeProtocolErrorif the file doesn't exist. The finally block safely disposes both bridges regardless of initialization state, sincedispose()handles uninitialized instances gracefully.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/node.ts (1)
215-232: Validate pool sizing and timeouts to avoid invalid configurations.Line 215-232: Invalid values (minProcesses < 1, maxProcesses < minProcesses, non‑positive timeouts) can lead to infinite waits or uncontrolled spawning. Add guardrails after resolving options.
🛠️ Suggested validation
this.options = { // Default to single-process mode for backward compatibility minProcesses: options.minProcesses ?? 1, maxProcesses: options.maxProcesses ?? 1, maxIdleTime: options.maxIdleTime ?? 300000, // 5 minutes maxRequestsPerProcess: options.maxRequestsPerProcess ?? 1000, pythonPath: options.pythonPath ?? venv?.pythonPath ?? getDefaultPythonPath(), scriptPath: resolvedScriptPath, virtualEnv, cwd, timeoutMs: options.timeoutMs ?? 30000, maxLineLength: options.maxLineLength, inheritProcessEnv: options.inheritProcessEnv ?? false, enableJsonFallback: options.enableJsonFallback ?? false, enableCache: options.enableCache ?? false, env: options.env ?? {}, warmupCommands: options.warmupCommands ?? [], }; + + if (this.options.minProcesses < 1) { + throw new RangeError('minProcesses must be >= 1'); + } + if (this.options.maxProcesses < this.options.minProcesses) { + throw new RangeError('maxProcesses must be >= minProcesses'); + } + if ( + this.options.maxIdleTime <= 0 || + this.options.maxRequestsPerProcess <= 0 || + this.options.timeoutMs <= 0 + ) { + throw new RangeError('maxIdleTime/maxRequestsPerProcess/timeoutMs must be > 0'); + }
🤖 Fix all issues with AI agents
In `@src/runtime/node.ts`:
- Around line 444-458: The polling in waitForAvailableWorker keeps scheduling
checkWorker after the timeout rejects; modify waitForAvailableWorker so the
timeout cancels further polls: introduce a local flag or store the poll timer id
(e.g., pollTimer) and when the timeout handler runs set timedOut = true or clear
the pollTimer, and ensure checkWorker returns immediately if timedOut (or
clears/resets pollTimer before scheduling the next setTimeout). Also clear any
outstanding pollTimer when a worker is found (in the branch that calls
resolvePromise) to prevent leaked timers; reference waitForAvailableWorker,
selectOptimalWorker, checkWorker, timeout/pollTimer and the timeout handler.
- Around line 743-759: The cleanup() logic must not rely on worker.busy because
in single-process multiplexing busy can be false while requests remain; change
the idleWorkers and overusedWorkers filters in cleanup() to require
worker.pendingRequests === 0 (or <= 0) in addition to the existing time/request
checks before calling terminateWorker(worker), and update any request
dispatch/response code to increment/decrement worker.pendingRequests (or add
that field to the worker object) so pendingRequests accurately tracks in-flight
calls; ensure terminateWorker and any related shutdown logic respect
pendingRequests and only tear down workers with zero pendingRequests.
♻️ Duplicate comments (1)
src/runtime/node.ts (1)
238-249: Init can report success after a failed doInit.Line 242 returns early based only on pool size, and initPromise is never cleared on failure. If doInit throws after spawning, later init() calls can silently skip initialization (e.g., bridgeInfo never set).
🛠️ Suggested fix
- if (this.processPool.length >= this.options.minProcesses) { - return; // Already initialized - } - if (this.initPromise) { - return this.initPromise; - } - this.initPromise = this.doInit(); - return this.initPromise; + if (this.initPromise) { + return this.initPromise; + } + if (this.processPool.length >= this.options.minProcesses && this.bridgeInfo) { + return; // Already initialized + } + this.initPromise = this.doInit().finally(() => { + this.initPromise = undefined; + }); + return this.initPromise;
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/runtime/node.ts
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-263
Timestamp: 2026-01-19T21:14:40.872Z
Learning: In `src/runtime/bridge-core.ts` and similar hot request/response loop implementations in the tywrap repository, avoid adding extra defensive validation (e.g., runtime shape checks on error payloads) in tight loops unless the protocol boundary is untrusted or there's a concrete bug report. The Python bridge protocol is controlled and validated via tests, so defensive checks would add unnecessary branching overhead without meaningful benefit.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/adversarial_playground.test.ts:339-360
Timestamp: 2026-01-19T21:00:30.825Z
Learning: In adversarial tests for the bridge (test/adversarial_playground.test.ts), recovery assertions should be kept in the finally block adjacent to cleanup to verify resilience under stress, even if that means post-timeout checks might mask the original error. The goal is to surface recovery failures as legitimate test failures.
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
📚 Learning: 2026-01-19T21:48:45.693Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: src/runtime/bridge-core.ts:260-298
Timestamp: 2026-01-19T21:48:45.693Z
Learning: In `src/runtime/bridge-core.ts`, keep `normalizeErrorPayload` to validate error payloads from the Python subprocess. The subprocess boundary is effectively untrusted, and normalizing error responses prevents `undefined: undefined` errors on malformed payloads. Error responses are not the hot path, so the small validation overhead is acceptable for the added resilience.
Applied to files:
src/runtime/node.ts
📚 Learning: 2026-01-19T21:00:30.825Z
Learnt from: bbopen
Repo: bbopen/tywrap PR: 127
File: test/adversarial_playground.test.ts:339-360
Timestamp: 2026-01-19T21:00:30.825Z
Learning: In adversarial tests for the bridge (test/adversarial_playground.test.ts), recovery assertions should be kept in the finally block adjacent to cleanup to verify resilience under stress, even if that means post-timeout checks might mask the original error. The goal is to surface recovery failures as legitimate test failures.
Applied to files:
src/runtime/node.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/node.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/node.ts
🔇 Additional comments (1)
src/runtime/node.ts (1)
413-438: Single-process multiplexing path preserves legacy concurrency.The single-process branch keeps request multiplexing intact while preventing quarantined/terminated workers from being reused. Looks good.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
This was referenced Jan 20, 2026
Closed
This was referenced Jan 20, 2026
8 tasks
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.
Summary
Changes
Core Implementation
src/runtime/node.tswith optional poolingOptimizedNodeBridgere-exportsNodeBridgefor backward compatibilityBehavior Improvements
BridgeDisposedError,BridgeTimeoutError)Tests
TYWRAP_ADVERSARIAL=1)Migration
No action required. Existing code works without changes.
For users of
OptimizedNodeBridge:Test plan
TYWRAP_ADVERSARIAL=1)🤖 Generated with Claude Code