Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
164 changes: 164 additions & 0 deletions src/runtime/bridge-protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,26 @@
* @see https://github.com/bbopen/tywrap/issues/149
*/

import type { BridgeInfo } from '../types/index.js';

import { BoundedContext, type ExecuteOptions } from './bounded-context.js';
import { BridgeProtocolError } from './errors.js';
import { SafeCodec, type CodecOptions } from './safe-codec.js';
import { TYWRAP_PROTOCOL_VERSION } from './protocol.js';
import { PROTOCOL_ID, type Transport, type ProtocolMessage } from './transport.js';

// =============================================================================
// TYPES
// =============================================================================

export interface GetBridgeInfoOptions {
/**
* If true, bypasses the cached info and queries the bridge again.
* This is useful when you want up-to-date instance counts or diagnostics.
*/
refresh?: boolean;
}

/**
* Configuration options for BridgeProtocol.
*/
Expand All @@ -38,6 +50,129 @@ export interface BridgeProtocolOptions {
defaultTimeoutMs?: number;
}

function validateBridgeInfoPayload(value: unknown): BridgeInfo {
if (!value || typeof value !== 'object' || Array.isArray(value)) {
const kind = value === null ? 'null' : Array.isArray(value) ? 'array' : typeof value;
throw new BridgeProtocolError(`Invalid bridge info payload: expected object, got ${kind}`);
}

interface BridgeInfoWire {
protocol?: unknown;
protocolVersion?: unknown;
bridge?: unknown;
pythonVersion?: unknown;
pid?: unknown;
codecFallback?: unknown;
arrowAvailable?: unknown;
scipyAvailable?: unknown;
torchAvailable?: unknown;
sklearnAvailable?: unknown;
instances?: unknown;
}

const formatValue = (val: unknown): string => {
try {
const serialized = JSON.stringify(val);
return serialized ?? String(val);
} catch {
return String(val);
}
};

const obj = value as BridgeInfoWire;

const protocol = obj.protocol;
if (protocol !== PROTOCOL_ID) {
throw new BridgeProtocolError(
`Invalid bridge info payload: protocol expected "${PROTOCOL_ID}", got ${formatValue(protocol)}`
);
}

const protocolVersion = obj.protocolVersion;
if (protocolVersion !== TYWRAP_PROTOCOL_VERSION) {
throw new BridgeProtocolError(
`Invalid bridge info payload: protocolVersion expected ${TYWRAP_PROTOCOL_VERSION}, got ${formatValue(protocolVersion)}`
);
}

const bridge = obj.bridge;
if (bridge !== 'python-subprocess') {
throw new BridgeProtocolError(
`Invalid bridge info payload: bridge expected "python-subprocess", got ${formatValue(bridge)}`
);
}

const pythonVersion = obj.pythonVersion;
if (typeof pythonVersion !== 'string' || pythonVersion.length === 0) {
throw new BridgeProtocolError(
`Invalid bridge info payload: pythonVersion expected non-empty string, got ${formatValue(pythonVersion)}`
);
}

const pid = obj.pid;
if (typeof pid !== 'number' || !Number.isInteger(pid) || pid <= 0) {
throw new BridgeProtocolError(
`Invalid bridge info payload: pid expected positive integer, got ${formatValue(pid)}`
);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

const codecFallback = obj.codecFallback;
if (codecFallback !== 'json' && codecFallback !== 'none') {
throw new BridgeProtocolError(
`Invalid bridge info payload: codecFallback expected "json" or "none", got ${formatValue(codecFallback)}`
);
}

const arrowAvailable = obj.arrowAvailable;
if (typeof arrowAvailable !== 'boolean') {
throw new BridgeProtocolError(
`Invalid bridge info payload: arrowAvailable expected boolean, got ${formatValue(arrowAvailable)}`
);
}

const scipyAvailable = obj.scipyAvailable;
if (typeof scipyAvailable !== 'boolean') {
throw new BridgeProtocolError(
`Invalid bridge info payload: scipyAvailable expected boolean, got ${formatValue(scipyAvailable)}`
);
}

const torchAvailable = obj.torchAvailable;
if (typeof torchAvailable !== 'boolean') {
throw new BridgeProtocolError(
`Invalid bridge info payload: torchAvailable expected boolean, got ${formatValue(torchAvailable)}`
);
}

const sklearnAvailable = obj.sklearnAvailable;
if (typeof sklearnAvailable !== 'boolean') {
throw new BridgeProtocolError(
`Invalid bridge info payload: sklearnAvailable expected boolean, got ${formatValue(sklearnAvailable)}`
);
}

const instances = obj.instances;
if (typeof instances !== 'number' || !Number.isInteger(instances) || instances < 0) {
throw new BridgeProtocolError(
`Invalid bridge info payload: instances expected non-negative integer, got ${formatValue(instances)}`
);
}
Comment on lines +154 to +159
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

instances validation allows fractional and negative numbers.

Same concern as pid — an instance count should be a non-negative integer. Number.isFinite permits -1 and 2.5.

🛡️ Proposed fix
   const instances = obj.instances;
-  if (typeof instances !== 'number' || !Number.isFinite(instances)) {
+  if (typeof instances !== 'number' || !Number.isInteger(instances) || instances < 0) {
     throw new BridgeProtocolError(
-      `Invalid bridge info payload: instances expected finite number, got ${formatValue(instances)}`
+      `Invalid bridge info payload: instances expected non-negative integer, got ${formatValue(instances)}`
     );
🤖 Prompt for AI Agents
In `@src/runtime/bridge-protocol.ts` around lines 154 - 159, The instances
validation currently allows negatives and fractions; update the check in the
bridge-protocol (the block that reads const instances = obj.instances) to
require a non-negative integer (use Number.isInteger(instances) && instances >=
0) instead of Number.isFinite, and update the thrown BridgeProtocolError message
to reflect "non-negative integer" while continuing to include
formatValue(instances) for context.


return {
protocol: PROTOCOL_ID,
protocolVersion: TYWRAP_PROTOCOL_VERSION,
bridge: 'python-subprocess',
pythonVersion,
pid,
codecFallback,
arrowAvailable,
scipyAvailable,
torchAvailable,
sklearnAvailable,
instances,
};
}
Comment on lines +53 to +174
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

🧩 Analysis chain

🏁 Script executed:

# First, find the BridgeInfo type definition
fd src/runtime/bridge-protocol.ts -x head -100 {} | rg -A 20 "type BridgeInfo|interface BridgeInfo"

Repository: bbopen/tywrap

Length of output: 39


🏁 Script executed:

# Search for HttpBridge and PyodideBridge classes
rg -l "class HttpBridge|class PyodideBridge" src/

Repository: bbopen/tywrap

Length of output: 99


🏁 Script executed:

# Check what bridges exist and their getBridgeInfo implementations
rg "getBridgeInfo" -A 5 src/runtime/

Repository: bbopen/tywrap

Length of output: 824


🏁 Script executed:

# Look at the file header and class-level docs mentioned at line 11
head -20 src/runtime/bridge-protocol.ts

Repository: bbopen/tywrap

Length of output: 811


🏁 Script executed:

# Check what HttpBridge returns for bridge identifier
cat -n src/runtime/http.ts | head -150

Repository: bbopen/tywrap

Length of output: 3227


🏁 Script executed:

# Check what PyodideBridge returns
cat -n src/runtime/pyodide.ts | head -150

Repository: bbopen/tywrap

Length of output: 3289


🏁 Script executed:

# Get the full BridgeInfo type definition
rg "type BridgeInfo|interface BridgeInfo" -A 30 src/types/

Repository: bbopen/tywrap

Length of output: 1337


🏁 Script executed:

# Check if there are transport-specific implementations that return different bridge values
rg "bridge.*http|bridge.*pyodide" -i src/runtime/

Repository: bbopen/tywrap

Length of output: 1462


🏁 Script executed:

# Check what each transport returns in its response handling
rg "BridgeInfo|bridge.*:" src/runtime/http-io.ts src/runtime/pyodide-io.ts | head -30

Repository: bbopen/tywrap

Length of output: 39


🏁 Script executed:

# Check if there are tests showing what getBridgeInfo returns for different bridges
fd test src/ | xargs rg "getBridgeInfo|BridgeInfo" | grep -E "http|pyodide" | head -20

Repository: bbopen/tywrap

Length of output: 39


🏁 Script executed:

# Look at the full BridgeInfo type definition to understand the design intent
cat -n src/types/index.ts | rg -A 15 "export interface BridgeInfo"

Repository: bbopen/tywrap

Length of output: 553


Consider whether BridgeInfo should support multiple bridge identifiers before extending the validator.

The hardcoded 'python-subprocess' check aligns with the BridgeInfo type definition, which explicitly declares bridge: 'python-subprocess' as a literal. HttpBridge and PyodideBridge inherit getBridgeInfo() from the base class and don't override the response format. If these bridges ever need to report different identifiers, the BridgeInfo type itself would need to change to a union type (e.g., bridge: 'python-subprocess' | 'python-http' | 'pyodide'). Extending the validator before updating the type contract could mask type safety issues. If supporting multiple identifiers is the intended design, start by updating the type definition.

Adding field discriminators to error messages would improve debugging:

♻️ Suggested error message improvement
-    throw new BridgeProtocolError('Invalid bridge info payload');
+    throw new BridgeProtocolError('Invalid bridge info payload: missing or invalid pythonVersion');

Similar discriminators for each field would aid debugging on this non-hot path.

🤖 Prompt for AI Agents
In `@src/runtime/bridge-protocol.ts` around lines 53 - 93, The validator
validateBridgeInfoPayload currently enforces a hardcoded bridge identifier
('python-subprocess') while the BridgeInfo type is a literal for that value; if
you intend to support multiple bridge identifiers you must first update the
BridgeInfo type to a union (e.g., bridge: 'python-subprocess' | 'python-http' |
'pyodide') and then relax the check in validateBridgeInfoPayload to accept that
union (use PROTOCOL_ID and TYWRAP_PROTOCOL_VERSION checks as-is); additionally
replace the repeated generic 'Invalid bridge info payload' throws with
field-discriminated error messages that include the failing field name and value
(e.g., include candidate.bridge, candidate.protocol, candidate.pythonVersion,
candidate.pid, candidate.codecFallback, etc.) to aid debugging while validating
each property.


// =============================================================================
// BRIDGE PROTOCOL BASE CLASS
// =============================================================================
Expand Down Expand Up @@ -80,6 +215,9 @@ export class BridgeProtocol extends BoundedContext {
/** Counter for generating unique request IDs */
private requestId = 0;

/** Cached bridge diagnostics info (populated by getBridgeInfo). */
private bridgeInfoCache?: BridgeInfo;
Comment thread
coderabbitai[bot] marked this conversation as resolved.

/**
* Create a new BridgeProtocol instance.
*
Expand Down Expand Up @@ -117,6 +255,7 @@ export class BridgeProtocol extends BoundedContext {
* but should not need to dispose the transport manually.
*/
protected async doDispose(): Promise<void> {
this.bridgeInfoCache = undefined;
// Transport is tracked and will be disposed by BoundedContext
// Subclasses can override to add additional cleanup
}
Expand Down Expand Up @@ -316,4 +455,29 @@ export class BridgeProtocol extends BoundedContext {
},
});
}

/**
* Fetch bridge diagnostics and feature availability.
*
* The Python bridge supports a `meta` method that returns protocol and environment info
* (including optional codec availability and current instance count).
*/
async getBridgeInfo(options: GetBridgeInfoOptions = {}): Promise<BridgeInfo> {
if (!options.refresh && this.bridgeInfoCache) {
return this.bridgeInfoCache;
}

const info = await this.sendMessage<BridgeInfo>(
{
method: 'meta',
params: {},
Comment on lines +470 to +473
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Aggregate bridge info across pooled workers

getBridgeInfo() sends a single meta request and caches that one response, but in Node pooled mode each request is executed by one worker (PooledTransport.send delegates to withWorker) and Python meta reports instances from that process-local map (len(instances) in runtime/python_bridge.py). After concurrent traffic has used multiple workers, this method can report a single worker’s pid/instances as if it were the whole bridge, which makes diagnostics and leak checks incorrect unless callers build their own aggregation.

Useful? React with 👍 / 👎.

},
{
validate: validateBridgeInfoPayload,
}
);

this.bridgeInfoCache = info;
return info;
}
}
33 changes: 32 additions & 1 deletion test/runtime_node.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { tmpdir } from 'node:os';
import { delimiter, join } from 'path';
import { NodeBridge } from '../src/runtime/node.js';
import { BridgeProtocolError } from '../src/runtime/errors.js';
import { TYWRAP_PROTOCOL_VERSION } from '../src/runtime/protocol.js';
import { getDefaultPythonPath, resolvePythonExecutable } from '../src/utils/python.js';
import { isNodejs, getVenvBinDir } from '../src/utils/runtime.js';

Expand Down Expand Up @@ -130,6 +131,36 @@ describeNodeOnly('Node.js Runtime Bridge', () => {
},
testTimeout
);

it(
'should report bridge info and track instance counts',
async () => {
const pythonAvailable = await isPythonAvailable();
if (!pythonAvailable || !isBridgeScriptAvailable()) return;

const info = await bridge.getBridgeInfo();
expect(info.protocol).toBe('tywrap/1');
expect(info.protocolVersion).toBe(TYWRAP_PROTOCOL_VERSION);
expect(info.bridge).toBe('python-subprocess');
expect(info.pythonVersion).toMatch(/^\d+\.\d+\.\d+$/);
expect(typeof info.scipyAvailable).toBe('boolean');
expect(typeof info.torchAvailable).toBe('boolean');
expect(typeof info.sklearnAvailable).toBe('boolean');

const cached = await bridge.getBridgeInfo();
expect(cached).toBe(info);

const before = info.instances;
const handle = await bridge.instantiate('collections', 'Counter', [[1, 2, 2]]);
const mid = await bridge.getBridgeInfo({ refresh: true });
expect(mid.instances).toBe(before + 1);

await bridge.disposeInstance(handle);
const after = await bridge.getBridgeInfo({ refresh: true });
expect(after.instances).toBe(before);
},
testTimeout
);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
});

describe('Stdlib Serialization', () => {
Expand Down Expand Up @@ -478,7 +509,7 @@ def get_path():

it.each(['__proto__', 'prototype', 'constructor'])(
'should reject dangerous environment override key %s',
(dangerousKey) => {
dangerousKey => {
const envOverrides = Object.create(null) as Record<string, string | undefined>;
Object.defineProperty(envOverrides, dangerousKey, {
value: 'blocked',
Expand Down