Skip to content

RuntimeReview.md

Latest commit

 

History

History
155 lines (78 loc) · 12.8 KB

RuntimeReview.md

File metadata and controls

155 lines (78 loc) · 12.8 KB

Code Review: BrowserX Runtime

Summary

The BrowserX Runtime is a well-architected orchestration layer with strong state-machine design, thorough config validation, and a clean plugin system using topological sort for activation ordering. However, several critical gaps exist: event listeners are wired but never dispatched in core classes, plugin context stubs silently discard contributions, dynamic plugin loading has path-traversal and remote-code-execution risks, and a significant portion of the test suite contains vacuous assertions that provide zero coverage value.

Findings

Critical

  • BrowserPool.eventListeners populated but never dispatched (resources/BrowserPool.ts:71,518-530) — addEventListener/removeEventListener are implemented but there is no emitEvent call anywhere. Subscribers (e.g., MCP ServiceInitializer) will never receive events for acquire, release, close, or error lifecycle transitions.

  • UnifiedMetricsCollector.eventListeners populated but never dispatched (metrics/UnifiedMetricsCollector.ts:61,472-484) — Identical issue. Listeners registered via addEventListener are stored but no event is ever emitted.

  • Unrestricted dynamic import of arbitrary URLs (plugins/PluginLoader.ts:48,310) — import(moduleUrl) accepts any path including http:///https:// URLs with no allowlist, signature verification, or hash pinning. A compromised config file could execute arbitrary remote code.

  • Path traversal in plugin manifest loading (plugins/PluginLoader.ts:146-148) — manifest.main from untrusted JSON is used to construct a file path without containment checks. "main": "../../../../../../etc/passwd" would be imported without restriction.

  • registerInitStep/registerShutdownStep silently discard contributions (plugins/PluginContext.ts:252-262) — Both methods log a debug message but never store or wire the step into any pipeline. Plugin authors calling these get silent no-ops.

  • forceStep does not cancel the underlying async operation (lifecycle/ShutdownSequence.ts:332-371) — On timeout, forceStep marks the component STOPPED but the original step.execute() promise keeps running, creating races with subsequent shutdown steps.

  • Prometheus label values not sanitized (metrics/UnifiedMetricsCollector.ts:448-452) — Label values containing " or \ produce malformed Prometheus output and could inject arbitrary text into the metrics stream.

  • Multiple test files contain vacuous assertions — SignalHandler.test.ts (6 tests assert nothing behavioral), EventCoordinator.test.ts:215 (no-op listener test), BrowserXRuntime.test.ts:200 (event listeners never fired), HealthChecker.test.ts:302 (stale-cache test body is empty), stress test lifecycle-stress.test.ts:493 (errorCount >= 0 always true). These provide false confidence with zero coverage.

High

  • validateConfig never called during construction or start() (BrowserXRuntime.ts:80-126) — ConfigValidator is well-implemented but never invoked. Invalid configs pass silently to runtime failures.

  • getStats() calls browserPool.getStats() twice (BrowserXRuntime.ts:683-686) — Redundant duplicate call in the same synchronous method.

  • getStats() hardcodes zeros for query/connection/page stats (BrowserXRuntime.ts:688-700) — Returns 0 for metrics that could be fetched from live _queryEngine and _proxyRuntime.

  • DAG built twice per start (plugins/PluginManager.ts:425-479) — getActivationOrder() and detectCycles() each build the full DAG independently. Should build once and share.

  • ShutdownSequence.getExecutionOrder() relies on registration order (lifecycle/ShutdownSequence.ts:128-131) — Simply reverses this.steps slice rather than computing reverse topological sort of the dependency graph. Incorrect if steps registered out of dependency order.

  • Signal handler double-shutdown race (signals/SignalHandler.ts:146-149, BrowserXRuntime.ts:310-316) — Both SIGINT and SIGTERM trigger shutdown(). Near-simultaneous signals race on shutdownSequence.execute() which throws if called while running.

  • wireDevToolsDomain/unwireDevToolsDomain are no-op stubs (plugins/PluginManager.ts:715-731) — Only emit console.debug. Plugins registering DevTools domains have no actual effect.

  • _priority parameter silently ignored on middleware methods (plugins/PluginContext.ts:104,129) — Both addRequestMiddleware and addResponseMiddleware accept priority but never use it. Plugins get incorrect ordering with no warning.

  • emitPluginEvent swallows all listener errors (plugins/PluginManager.ts:761) — Bare catch {} discards all exceptions silently.

  • Timeout detection via string matching (lifecycle/ShutdownSequence.ts:293) — err.message.includes("timed out") is fragile; any user error containing that phrase triggers force-shutdown incorrectly.

  • No test for start() failure → ERROR state path — Happy-path lifecycle is tested but no test verifies runtime behavior when a component fails during initialization.

  • InitializationSequence circular dependency test not awaited (tests/unit/InitializationSequence.test.ts:255) — assertRejects() without await may pass silently on broken implementations.

Medium

  • LifecycleManager.transition doesn't record previous state in history (lifecycle/LifecycleManager.ts:98-108) — previousState is captured but never stored, making root-cause analysis harder.

  • LifecycleManager.reset() doesn't clear component registrations (lifecycle/LifecycleManager.ts:252-264) — Re-calling registerComponent after reset() throws "already registered".

  • Shallow freeze on plugin config (plugins/PluginContext.ts:88) — Object.freeze only freezes top-level; nested objects remain mutable.

  • Metrics server exposes no authentication (metrics/UnifiedMetricsCollector.ts:144-179) — config.host can be set to 0.0.0.0 with no auth or IP allowlisting.

  • Health check staleness window hardcoded to 10s (metrics/HealthChecker.ts:167-170) — Ignores configurable healthCheckInterval, using 10000 ms always.

  • BrowserPool.generateInstanceId uses Date.now() (resources/BrowserPool.ts:511-513) — Millisecond-resolution timestamps can collide under load. crypto.randomUUID() alone is sufficient.

  • exportJSON stuffs histogram data into labels (metrics/UnifiedMetricsCollector.ts:383-389) — Breaks the type contract of Record<string, string>.

  • mergeConfig shallow-merges proxy.gateways (config/RuntimeConfig.ts:451-452) — Inconsistent with deep-merge of query.cache and query.sandbox.

  • InitializationSequence.registerStep auto-registers unknown components (lifecycle/InitializationSequence.ts:101-103) — Bypasses deliberate registration in BrowserXRuntime.registerComponents().

  • traceCyclePath algorithm incorrect for complex graphs (plugins/PluginManager.ts:485-523) — Path construction mixes nodes; fallback papers over it. Should use GraphX CycleError data instead.

  • Plugin ID collision resolved by load order (plugins/PluginManager.ts:205-209) — Non-deterministic; should throw or provide explicit resolution.

  • findPluginConfig filename-stem heuristic is fragile (plugins/PluginManager.ts:782-786) — No guarantee plugin IDs match filenames.

  • closeInstance not awaited in release() (resources/BrowserPool.ts:255-257) — Errors in async cleanup silently lost.

  • LifecycleManager.stateHistory uses shift() at cap (lifecycle/LifecycleManager.ts:106-108) — O(n) per push. Should use ring buffer.

  • No test covers plugin activation timeout — Critical production scenario (hanging plugin stalls startup) with zero coverage.

  • Stress test asserts maxConcurrent >= 1 (tests/stress/lifecycle-stress.test.ts:209) — Always true; should assert > 1 to verify parallelism.

  • ConfigValidator test swallows assertion failures (tests/unit/ConfigValidator.test.ts:445) — Uses try/catch instead of assertThrows.

Low

  • Event emitter pattern duplicated ~8 times — addEventListener/removeEventListener/emitEvent copy-pasted across every class (~160 lines). Should extract EventEmitter<T> mixin.

  • console.error used for informational logs (BrowserXRuntime.ts:493,528,561) — "Starting BrowserX Runtime..." written to stderr. Should use logger with logLevel.

  • ComponentId open string union under-constrained (types.ts:31) — No enforcement of "plugin:namespace:name" convention.

  • previousState variable unused (lifecycle/LifecycleManager.ts:98) — Captured but never referenced after assignment.

  • maxConcurrency config defined but never used (lifecycle/InitializationSequence.ts:52,64) — Config declares maxConcurrency: 3 but execution is strictly sequential.

  • README documents runtime.initialize() but code uses runtime.start() (README.md:28) — Stale API in documentation.

  • Duplicate mock helpers across test files — createMockPlugin, createTestPluginManager etc. copy-pasted. Should extract shared tests/helpers/mocks.ts.

  • console.log in integration tests (tests/event-loop-verification.test.ts:52,154,197) — Clutters CI output.

  • PluginContextImpl exported from plugins/mod.ts — Leaks implementation class; only PluginContext interface should be public.

  • getQueryEngine()/getProxyRuntime() return unknown (plugins/types.ts:197-200) — Every caller must unsafely cast.

Strengths

  • Declarative state-machine with explicit transition table — LifecycleManager defines all valid transitions in STATE_TRANSITIONS, making illegal transitions immediately detectable with clear error messages. Far safer than ad-hoc conditionals.

  • Timeout-per-step with global budget in shutdown — ShutdownSequence tracks both per-step and total-timeout deadlines using Math.min(step.timeout, remaining). Production-grade shutdown orchestration.

  • Promise-based waiter queue in BrowserPool — The waiters array with resolve/reject/timer tuples eliminates polling and correctly hands instances to waiters on release().

  • Thorough ConfigValidator — Checks ranges, cross-field invariants (e.g., minInstances > maxInstances, drainTimeout > timeout), and separates errors from warnings with actionable messages.

  • GraphX DAG for plugin activation — Using topological sort for plugin activation ordering correctly handles dependency chains and cycles, replacing error-prone manual ordering.

  • Disposable pattern throughout plugin system — Every contribution returns a Disposable registered with the plugin registry. Cleanup is automatic and predictable without manual tracking.

  • Dynamic imports for workspace boundaries — await import(...) for cross-workspace dependencies (@browserx/proxy-engine, @browserx/browser) keeps module boundaries clean and enables lazy initialization.

  • Composable health check handlers — HealthChecker.createBooleanHandler and createSimpleHandler allow declarative health registration without coupling to HealthChecker internals.

  • Excellent dependency-ordering tests — plugin-manager-graphx.test.ts covers diamond dependencies, 3-node cycles, linear chains, missing deps, and reverse-deactivation invariant.

  • Genuine end-to-end plugin integration test — plugins.test.ts exercises four contribution types simultaneously, verifies wiring, executes functions, deactivates, and confirms complete cleanup.

Recommendations

  1. Implement emitEvent in BrowserPool and UnifiedMetricsCollector — These are the most impactful fixes. Wire event dispatch at lifecycle boundaries (acquire, release, close, error, metric change).

  2. Implement registerInitStep/registerShutdownStep and wireDevToolsDomain in PluginContext/PluginManager — These are documented APIs that silently no-op. Per project rules, called-but-missing means implement.

  3. Add path containment check in PluginLoader — Verify mainPath resolves inside pluginDir. Gate remote URL imports behind explicit config flag.

  4. Call validateConfig in BrowserXRuntime.start() — The validator exists and is thorough; it just needs to be wired in.

  5. Fix vacuous test assertions — Prioritize SignalHandler tests, EventCoordinator listener test, HealthChecker stale-cache test, and stress test assertions. These currently provide false confidence.

  6. Extract shared EventEmitter<T> mixin — Eliminate ~160 lines of duplicated event listener boilerplate across 8+ classes.

  7. Sanitize Prometheus label values — Escape " and \ in formatPrometheusLabels per the exposition format spec.

  8. Add AbortSignal support to shutdown steps — Allow forceStep to genuinely cancel timed-out operations rather than leaving orphaned promises.