feat: Plugin performance improvements#606
Conversation
… server - Added a memory pressure monitor to the PoolServer class, which triggers garbage collection and cache eviction based on heap usage thresholds. - Introduced a ContextPool and ScriptCache to optimize VM context and script management, reducing overhead during plugin execution. - Updated the WorkerPoolManager to handle memory pressure awareness and improve overall performance. - Enhanced error handling and logging for better observability during high memory usage scenarios.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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. WalkthroughThis pull request introduces a comprehensive plugin pool executor system for the relayer. It adds a persistent Node.js worker pool infrastructure to execute pre-compiled JavaScript plugins efficiently, replacing per-request spawning. The changes span Rust backend services (configuration, connection pooling, health monitoring, circuit breakers, inter-process protocols), TypeScript plugin infrastructure (compilation, sandboxing, worker management), and REST API extensions for plugin retrieval and updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Relayer as Relayer (Rust)
participant PoolMgr as PoolManager
participant PoolSrv as Pool Server (Node.js)
participant Executor as Sandbox Executor
participant Plugin as Plugin Code
Client->>Relayer: execute_plugin()
activate Relayer
Relayer->>PoolMgr: ensure_started()
activate PoolMgr
PoolMgr->>PoolSrv: spawn ts-node pool-server
activate PoolSrv
PoolSrv->>PoolSrv: initialize Piscina pool
PoolSrv-->>PoolMgr: readiness signal
deactivate PoolMgr
Relayer->>PoolMgr: execute_plugin(request)
activate PoolMgr
PoolMgr->>PoolSrv: send PoolRequest.Execute via socket
activate PoolSrv
PoolSrv->>Executor: worker.executeInSandbox(task)
activate Executor
Executor->>Executor: acquire VM context from pool
Executor->>Executor: acquire/cache compiled script
Executor->>Plugin: run handler(api, params)
activate Plugin
Plugin->>Plugin: execute user code
Plugin-->>Executor: return result + logs
deactivate Plugin
Executor->>Executor: cleanup context & sockets
Executor-->>PoolSrv: SandboxResult
deactivate Executor
PoolSrv-->>PoolMgr: PoolResponse.Execute
deactivate PoolSrv
PoolMgr-->>Relayer: ScriptResult
deactivate PoolMgr
Relayer-->>Client: response
deactivate Relayer
sequenceDiagram
participant Relayer as Relayer (Rust)
participant SharedSock as Shared Socket
participant Plugin as Plugin Context
participant PluginCode as Plugin API Call
Relayer->>SharedSock: ensure_shared_socket_started()
activate SharedSock
SharedSock->>SharedSock: create Unix socket listener
deactivate SharedSock
Relayer->>SharedSock: register_execution(execution_id)
activate SharedSock
SharedSock-->>Relayer: ExecutionGuard (RAII)
deactivate SharedSock
Plugin->>SharedSock: send Register message
activate SharedSock
SharedSock->>SharedSock: track execution context
deactivate SharedSock
Plugin->>PluginCode: make API call
activate PluginCode
PluginCode->>SharedSock: send ApiRequest message
activate SharedSock
SharedSock-->>PluginCode: send via relayer socket
PluginCode-->>SharedSock: receive ApiResponse
deactivate SharedSock
PluginCode-->>Plugin: result
deactivate PluginCode
Plugin->>SharedSock: send Trace message
activate SharedSock
SharedSock->>SharedSock: store trace in execution context
deactivate SharedSock
Note over Relayer,Plugin: ExecutionGuard dropped (implicit or explicit)
Relayer->>SharedSock: auto-unregister via Drop
activate SharedSock
SharedSock-->>SharedSock: cleanup execution context
deactivate SharedSock
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
zeljkoX
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
plugins/examples/example-rpc.ts (1)
21-26: Remove invalidparamsfrometh_blockNumberRPC call.The
eth_blockNumberJSON-RPC method does not accept any parameters. According to the Ethereum JSON-RPC specification, it requiresparams: []and simply returns the current block number. The'latest'tag is used with other methods likeeth_getBalanceoreth_call, but noteth_blockNumber.Proposed fix
const result = await relayer.rpc({ method: 'eth_blockNumber', id: 1, jsonrpc: '2.0', - params: ['latest'], + params: [], });docs/plugins/index.mdx (1)
186-191: JSON syntax error: missing comma.The JSON example is missing a comma after the
"config"object, which would cause a parse error if users copy this configuration.Suggested fix
"config": { "featureFlagExample": true - } + }, "forward_logs": falsesrc/repositories/plugin/plugin_in_memory.rs (1)
29-41: Clone withtry_locksilently drops data if lock is contended.The clone implementation falls back to an empty HashMap if the lock is held. This could cause data loss in concurrent scenarios. Consider documenting this behavior or using a blocking lock.
openapi.json (2)
1195-1216: Docs/spec mismatch:/callGET mentions path routing (/call/verify) but OpenAPI doesn’t define it.The description says clients can call
/api/v1/plugins/{plugin_id}/call/verify(Line 1196) but only/api/v1/plugins/{plugin_id}/callexists inpaths. Either:
- add an explicit path like
/api/v1/plugins/{plugin_id}/call/{route}(withrouteas a path parameter), or- remove the path-based example and document only the query parameter form (which the spec already supports via
route).
1315-1336: Same/callrouting doc mismatch for POST; also clarify “route” shape.Same issue as GET: description includes
/call/verifybut no matching path is defined (Line 1316). Also consider clarifying whetherrouteshould be"verify"vs"/verify"(your examples use?route=/verify), since clients will copy/paste this.
🤖 Fix all issues with AI agents
In @.gitignore:
- Around line 72-85: There are conflicting .gitignore rules for
plugins/lib/sandbox-executor.js: the explicit ignore entry
"plugins/lib/sandbox-executor.js" and the later negation
"!plugins/lib/sandbox-executor.js"; decide the intended behavior and remove the
redundant lines accordingly—if you want to keep the pre-built sandbox executor
in VCS, delete the ignore line "plugins/lib/sandbox-executor.js"; if you want to
ignore it, delete the negation line(s) "!plugins/lib/sandbox-executor.js" (and
the adjacent comment about keeping it).
In @build.rs:
- Around line 66-86: The build script currently only logs warnings when the
sandbox-executor build fails (inside the if needs_build block that runs
Command::new("pnpm").arg("run").arg("build")), which lets cargo continue with a
missing/stale artifact; change the match arms handling the Command output so
failures abort the cargo build: in the Ok(output) non-success branch replace the
println!("cargo:warning=...") with a panic! (or propagate an Err) that includes
the String::from_utf8_lossy(&output.stderr) message, and in the Err(e) branch
similarly panic! with e.to_string(), ensuring the build script stops on
sandbox-executor compilation failure.
In @Cargo.toml:
- Around line 68-69: The Cargo.toml currently pins async-channel to "2.3";
update the async-channel dependency to the newer minor series by changing
async-channel = "2.3" to async-channel = "2.5" so the project pulls
async-channel 2.5.x (ensuring latest fixes), while leaving crossbeam = { version
= "0.8" } unchanged; run cargo update / cargo build to verify compatibility.
In @openapi.json:
- Around line 4851-4925: ApiResponse_PluginModel currently inlines the plugin
schema under properties.data and declares error as only "string"; instead
replace properties.data with a $ref to the existing PluginModel schema (use
"$ref": "#/components/schemas/PluginModel") to avoid duplication and ensure any
new fields (e.g., forward_logs) are inherited, and make the error property
nullable by changing its schema to allow both string and null (e.g., type:
["string","null"]) so it matches the 200 examples.
In @plugins/lib/pool-server.ts:
- Around line 604-629: The default case declares a const named _exhaustive
within the switch which can be accessed by other cases; wrap the default case
body in its own block to limit scope. Update the switch handling in the method
that dispatches on message.type (the cases for 'execute' -> handleExecute and
'precompile' -> handlePrecompile) so the default branch reads: default: { const
_exhaustive: never = message; return { taskId, success: false, error: { message:
'Unknown work request type', code: 'UNKNOWN_TYPE' } }; } to ensure _exhaustive
is block-scoped and does not leak to other cases.
In @plugins/lib/sandbox-executor.ts:
- Around line 136-154: The two successive memory-pressure checks both run when
heapUsedRatio >= 0.85, causing evictOldest to be called twice and evict ~75%
instead of the intended ~50%; update the logic in sandbox-executor.ts to make
the second check exclusive (e.g., change the second if to else if using the same
heapUsedRatio variable) or capture the initial cache size into a local variable
and compute evictCount from that single baseline before calling evictOldest
once, referencing the existing heapUsedRatio, this.cache.size and evictOldest
symbols.
- Around line 63-76: The resetContext function currently swallows all errors
when running vm.runInContext, which can return polluted contexts to the pool;
update resetContext to detect failures and instead mark or replace the context
(e.g., create a fresh vm.createContext or call the pool’s allocator) so a failed
reset never returns a reused polluted ctx; specifically, catch the error from
vm.runInContext in resetContext, log or capture the error, and ensure the failed
ctx is removed/replaced from the pool rather than silently retained.
In @plugins/package.json:
- Around line 10-11: The build script ("build": "ts-node lib/build-executor.ts")
and postinstall rely on ts-node but it is not declared; add ts-node to
package.json devDependencies (e.g., add "ts-node": "<appropriate-semver>") so
the build step does not rely on transitive packages and remains stable, then run
your package manager to install dev deps (e.g., pnpm add -D ts-node) to ensure
ts-node is available during postinstall/build.
In @src/bootstrap/initialize_plugins.rs:
- Around line 94-99: The loop in initialize_plugins.rs duplicates path
normalization logic; replace the inline conditional that builds plugin_path from
plugin.path with a call to the existing helper
PluginService::<R>::resolve_plugin_path(&plugin.path) (or the module's
resolve_plugin_path if static) so the code uses the centralized implementation
used elsewhere (e.g., services/plugins::resolve_plugin_path) for plugins.items
handling; remove the redundant if/format logic and pass the resolved path
onward.
In @src/services/plugins/config.rs:
- Around line 407-411: The Default::default() implementation must be made pure
by removing the global side effect
std::env::remove_var("PLUGIN_MAX_CONCURRENCY"); — delete that call from the
Default impl and ensure defaults are constructed without touching environment
state; if clearing that env var was intended for tests, move the removal into
test setup (or a #[cfg(test)] helper) or into the from_env() path where
environment parsing occurs (e.g., in from_env()), keeping PLUGIN_MAX_CONCURRENCY
handling confined to from_env()/test utilities.
In @src/services/plugins/runner.rs:
- Around line 240-249: The code currently uses a hard-coded
Duration::from_secs(5) when awaiting guard.into_receiver().recv(); replace that
literal with the configured timeout by calling get_trace_timeout() (the same
function used by run_with_pool) to construct the Duration used in
tokio::time::timeout, e.g. build the Duration from get_trace_timeout() instead
of using 5 seconds so trace collection uses the same configured timeout across
modes (adjust unit conversion if get_trace_timeout() returns milliseconds vs
seconds).
- Around line 309-314: The ExecutionGuard returned by
shared_socket.register_execution(execution_id.clone()) is dropped immediately
after calling into_receiver(), allowing unregister to run and causing lost
traces; retain the guard through the entire execution by keeping it in scope
until trace collection completes (e.g., change traces_rx from Option<Receiver>
to Option<(ExecutionGuard, Receiver)> or store the guard in a separate variable
alongside traces_rx) so the guard isn’t dropped until after the trace collection
point where traces_rx is consumed.
In @src/services/plugins/shared_socket.rs:
- Around line 246-257: The background cleanup loop spawned in new() never
observes shutdown and will leak; modify new() to accept or create a shutdown
signal (e.g., a tokio::sync::watch::Receiver, oneshot::Receiver, or a
tokio::sync::Notify) and move a clone into the task, then replace the infinite
loop with a tokio::select! that awaits either interval.tick() or the shutdown
signal and breaks/returns when shutdown is received; ensure the task uses the
provided shutdown receiver (alongside executions_clone) so it exits cleanly when
the service is dropped.
- Around line 636-666: The legacy branch sets bound_execution_id from Request
without checking the execution registry; extract the candidate id from
request.http_request_id or request.request_id, verify it exists using the same
registry check used elsewhere (e.g., map.contains_key(&execution_id) or the
execution map variable), and only set bound_execution_id when the check passes;
if the id is missing/invalid, do not bind and treat the message as unauthorized
(log and skip/respond accordingly) before calling relayer_api.handle_request so
legacy requests are validated the same way as the new protocol.
- Around line 296-298: The method active_connection_count currently returns
connection_semaphore.available_permits(), which is the inverse of "active"
connections; fix by returning the actual active count instead (e.g., active =
total_permit_capacity - self.connection_semaphore.available_permits()) using the
configured max connections field (or store the initial permit count at semaphore
creation) and update active_connection_count to compute and return that value;
if you prefer not to change logic, rename active_connection_count to
available_connection_count and update all callers to reflect the new meaning
(referencing active_connection_count and connection_semaphore in your change).
🧹 Nitpick comments (46)
plugins/package.json (1)
16-23: Consider movingesbuildtodevDependencies.
esbuildis a build tool used only during the build step (viabuild-executor.ts). Build tools are typically listed underdevDependenciesrather thandependenciesto reduce production bundle size and avoid shipping unnecessary packages in deployments.
piscinais correctly placed as a runtime dependency since it's used for the worker pool at runtime.Suggested change
"dependencies": { "@openzeppelin/relayer-sdk": "^1.7.0", "@types/node": "^24.0.3", - "esbuild": "^0.24.0", "ethers": "^6.14.3", "ioredis": "^5.7.0", "piscina": "^4.7.0", "uuid": "^11.1.0" }, "devDependencies": { "@types/ioredis-mock": "^8.2.6", "@types/jest": "^29.5.12", "@types/uuid": "^10.0.0", + "esbuild": "^0.24.0", "husky": "^9.1.7",CLAUDE.md (1)
36-52: Add language specifier to code block.Per markdownlint (MD040), fenced code blocks should have a language specified. Since this is a directory structure, use
textorplaintext.Suggested fix
-``` +```text src/ ├── api/ # HTTP routes (Actix-web), OpenAPI specs, middlewareplugins/lib/build-executor.ts (2)
192-203: Atomic write function returns unused temp path.The
atomicWriteFilefunction writes to a temp file and renames it, but returns thetempPathafter the rename has completed. Since the temp file no longer exists afterrename, the return value is misleading and unused bywriteHashFileandsaveBuildCache.♻️ Suggested simplification
-function atomicWriteFile(targetPath: string, content: string): string { +function atomicWriteFile(targetPath: string, content: string): void { const tempPath = path.join( os.tmpdir(), `build-executor-${crypto.randomUUID()}.tmp` ); fs.writeFileSync(tempPath, content, 'utf-8'); fs.renameSync(tempPath, targetPath); - - return tempPath; }
340-350: Consider exiting with non-zero code on unhandled errors.The catch block calls
cleanup()andprocess.exit(1), which is correct. However, if the cleanup itself throws, the exit might not happen. Consider wrapping cleanup in a try-catch.♻️ Safer error handling
build().catch((err) => { const error = err as Error; console.error(`\n❌ Build failed: ${error.message}`); if (error.stack) { console.error('\nStack trace:'); console.error(error.stack.split('\n').slice(1, 5).join('\n')); } - cleanup(); + try { + cleanup(); + } catch { + // Ignore cleanup errors during failure path + } process.exit(1); });plugins/ARCHITECTURE.md (4)
7-37: Add language specifier to ASCII diagram code block.Per markdownlint, fenced code blocks should have a language specified. For ASCII diagrams, use
textorplaintext.📝 Add language specifier
-``` +```text ┌─────────────────────────────────────────────────────────────────────────┐
113-140: Add language specifier to request flow diagram.📝 Add language specifier
-``` +```text 1. HTTP POST /api/v1/plugins/{id}/call
183-189: Add language specifier to circuit breaker state diagram.📝 Add language specifier
-``` +```text CLOSED ──(>50% failure rate)──▶ OPEN
248-265: Add language specifier to module dependency graph.📝 Add language specifier
-``` +```text config.rsbuild.rs (1)
15-17: Consider adding more rerun-if-changed triggers.The build depends on more files than just
sandbox-executor.tsandpackage.json. Consider adding triggers for other files that affect the build output.📝 Additional rerun triggers
println!("cargo:rerun-if-changed=plugins/lib/sandbox-executor.ts"); println!("cargo:rerun-if-changed=plugins/package.json"); + println!("cargo:rerun-if-changed=plugins/pnpm-lock.yaml"); + println!("cargo:rerun-if-changed=plugins/tsconfig.json");plugins/lib/kv.ts (1)
283-298: Usequit()instead ofdisconnect()for graceful shutdown.The ioredis
disconnect()method forcefully closes the connection immediately without waiting for pending commands. For a proper graceful shutdown that allows in-flight operations to complete, usequit()instead, which instructs the server to close the connection after all pending replies are written.plugins/tests/lib/worker-pool.test.ts (2)
72-74:has()doesn't respect TTL, unlikeget().The
has()method returns true for keys regardless of whether they've expired, whileget()correctly returns null for expired entries. This inconsistency could lead to unexpected behavior if consumers check existence before retrieval.♻️ Suggested fix
has(key: string): boolean { - return this.cache.has(key); + const entry = this.cache.get(key); + if (!entry) return false; + if (Date.now() - entry.timestamp > this.ttlMs) { + this.delete(key); + return false; + } + return true; }
215-230: TTL test timing is reasonable but could be fragile under load.The test uses a very short TTL (~0.6ms) with a 10ms wait. While this works, consider using
jest.useFakeTimers()for more deterministic testing if you encounter flakiness in CI.plugins/tests/lib/plugin.test.ts (1)
373-380:fail()may not be defined in newer Jest versions.The
fail()function was removed from Jest globals in recent versions. Consider using an alternative pattern.♻️ Suggested fix
try { await sendPromise; - fail('Should have thrown'); + expect.fail('Should have thrown'); } catch (error: any) { expect(error.code).toBe('ESOCKETCLOSED'); expect(error.name).toBe('SocketClosedError'); }Or use the more idiomatic Jest pattern:
await expect(sendPromise).rejects.toMatchObject({ code: 'ESOCKETCLOSED', name: 'SocketClosedError', });src/services/plugins/protocol.rs (1)
280-305: Missing test case for "result" log level.The implementation handles "result" level (line 118), but the test doesn't cover it.
♻️ Suggested addition
let levels = vec![ ("log", LogLevel::Log), ("info", LogLevel::Info), ("warn", LogLevel::Warn), ("error", LogLevel::Error), ("debug", LogLevel::Debug), + ("result", LogLevel::Result), ("unknown", LogLevel::Log), ];src/repositories/plugin/plugin_in_memory.rs (1)
187-365: Missing tests for new compiled code cache methods.The test file doesn't include tests for the newly added
get_compiled_code,store_compiled_code,invalidate_compiled_code,invalidate_all_compiled_code,has_compiled_code, andget_source_hashmethods. Consider adding tests to validate these behaviors.Do you want me to generate test cases for these new methods?
src/constants/plugins.rs (1)
15-17: Clarify relationship between plugin timeout and pool execution timeout.
DEFAULT_PLUGIN_TIMEOUT_SECONDS(300s) andDEFAULT_POOL_EXECUTION_TIMEOUT_MS(30s) have a 10x difference. The comment on line 51 helps, but consider adding a note explaining when each is applied to prevent confusion.Also applies to: 50-52
plugins/lib/sandbox-executor.ts (1)
998-1007: Timeout timer not cleared when handler succeeds, causing minor resource leak.The
setTimeoutintimeoutPromisecontinues running even afterhandlerPromisewins the race. This delays garbage collection of the closure.♻️ Suggested fix
+ let handlerTimeoutId: NodeJS.Timeout | undefined; const timeoutPromise = new Promise<never>((_, reject) => { - setTimeout(() => { + handlerTimeoutId = setTimeout(() => { const error = new Error(`Plugin handler timed out after ${task.timeout}ms`); (error as any).code = 'ERR_HANDLER_TIMEOUT'; reject(error); }, task.timeout); }); - result = await Promise.race([handlerPromise, timeoutPromise]); + try { + result = await Promise.race([handlerPromise, timeoutPromise]); + } finally { + if (handlerTimeoutId) clearTimeout(handlerTimeoutId); + }plugins/tests/lib/sandbox-executor.test.ts (3)
1-3: Unused imports.The imports
vm,v8, andnetare not used in this test file. Consider removing them to keep the imports clean.-import * as vm from 'node:vm'; -import * as v8 from 'node:v8'; -import * as net from 'node:net'; +// No imports needed - tests use mock implementations
1262-1269: Unused variableheapRatioin test.The variable
heapRatiois declared but not used in the eviction calculation logic, making the test misleading about what triggers the eviction.it('should evict 50% at 85% heap usage', () => { const cacheSize = 100; - const heapRatio = 0.85; + // Test validates the eviction formula at 85% threshold const evictCount = Math.max(1, Math.ceil(cacheSize * 0.5)); expect(evictCount).toBe(50); });
819-827: Usingfail()which may not be available in all Jest configurations.Consider using
expect.fail()or restructuring the test to avoid relying onfail().Proposed fix
try { await pendingPromise; - fail('Should have rejected'); + expect(true).toBe(false); // Should have rejected } catch (error: any) { expect(error.code).toBe('ESOCKETCLOSED'); expect(error.name).toBe('SocketClosedError'); expect(error.message).toContain('connection lifetime exceeded'); }src/services/plugins/connection.rs (2)
99-106: BufReader created on every request is inefficient.Creating a new
BufReaderfor eachsend_requestcall adds overhead. Consider storing the buffered reader in the struct if the connection is reused.However, since
BufReader::newtakes&mut self.stream, storing it would require restructuring. The current approach is acceptable for correctness but worth noting for future optimization.
241-244:clear()silently drops connections without explicit cleanup.The
clear()method pops connections from the queue but doesn't explicitly close/drop theUnixStream. While Rust's drop semantics will close the streams, consider logging or adding a comment noting this behavior./// Clear all connections pub async fn clear(&self) { - while self.available.pop().is_some() {} + let mut cleared = 0; + while self.available.pop().is_some() { + cleared += 1; + } + if cleared > 0 { + tracing::debug!(cleared = cleared, "Cleared connections from pool"); + } }src/services/plugins/health.rs (1)
276-306: Duplicated time calculation logic.The
now_ms()calculation pattern is duplicated inopen_circuit()(lines 278-281) andmaybe_transition_to_half_open()(lines 288-291). Consider using the existingnow_ms()helper method.Proposed fix
- fn open_circuit(&self) { + pub fn open_circuit(&self) { self.set_state(CircuitState::Open); - let now = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap_or_default() - .as_millis() as u64; + let now = Self::now_ms(); self.opened_at_ms.store(now, Ordering::Relaxed); self.recovery_successes.store(0, Ordering::Relaxed); } - fn maybe_transition_to_half_open(&self) { + pub fn maybe_transition_to_half_open(&self) { let opened_at = self.opened_at_ms.load(Ordering::Relaxed); - let now = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap_or_default() - .as_millis() as u64; + let now = Self::now_ms();src/services/plugins/config.rs (1)
333-356: Usingassert!invalidate()will panic at runtime.Per coding guidelines: "Avoid unwrap; handle errors explicitly with Result." The
assert!macros will cause a panic if validation fails. Consider returning aResult<(), PluginError>instead and letting the caller decide how to handle invalid configuration.However, since this is configuration validation at startup, panicking may be intentional to fail fast. If this is the design intent, consider adding a comment explaining this choice.
plugins/lib/pool-server.ts (1)
334-334: Inconsistent import style:require('os')instead ofimport.Line 334 uses
require('os')while the file uses ES module imports elsewhere. Consider using the import at the top of the file for consistency.+import * as os from 'node:os'; // ... at line 334: - const cpuCount = require('os').cpus().length; + const cpuCount = os.cpus().length;plugins/tests/lib/pool-server.test.ts (2)
139-202: Tests exercise mock behavior rather than actual PoolServer.The execute message tests (lines 139-202) instantiate
WorkerPoolManagermocks directly instead of testing throughPoolServer. This validates the mock setup but doesn't test the actual message handling inPoolServer.handleExecute().Consider adding integration tests that send actual JSON messages through a socket to the
PoolServerfor more realistic coverage.
503-514: Cache eviction error test doesn't verify catch behavior.The test at lines 503-514 verifies that
clearCachethrows, but it doesn't test howPoolServer.evictCache()handles errors (which has a try-catch that logs and continues). Consider testing the actual error handling path.it('should handle cache eviction errors gracefully', () => { // This tests the mock throwing, but not PoolServer's error handling // Consider testing PoolServer.evictCache() directly });src/services/plugins/pool_executor.rs (4)
236-306: Queue worker shutdown may leave in-flight requests unprocessed.When shutdown is signaled, workers break immediately even if they were about to process a request from the channel. Requests already dequeued but not yet processed will have their
response_txdropped, causing the caller to receive a "Request queue processor closed" error.Consider draining pending requests or processing the current one before exiting:
tokio::select! { biased; _ = shutdown.notified() => { tracing::debug!(worker_id = worker_id, "Request queue worker received shutdown signal"); + // Drain remaining requests with graceful error + while let Ok(req) = rx_clone.try_recv() { + let _ = req.response_tx.send(Err(PluginError::PluginExecutionError( + "Plugin system shutting down".to_string() + ))); + } break; }
403-412: Silent fallback to empty string on JSON serialization failure.If
serde_json::to_string(&v)fails (which is rare but possible with certain edge cases), the result silently becomes an empty string. Consider logging this edge case:if v.is_string() { v.as_str().unwrap_or("").to_string() } else { - serde_json::to_string(&v).unwrap_or_default() + serde_json::to_string(&v).unwrap_or_else(|e| { + tracing::warn!(error = %e, "Failed to serialize plugin result"); + String::new() + }) }
788-811: Duplicated circuit breaker recording logic.The circuit breaker success/failure recording logic is duplicated between the fast path (lines 788-811) and slow path (lines 893-913). Consider extracting this into a helper method:
♻️ Suggested refactor
fn record_circuit_breaker_result( &self, result: &Result<ScriptResult, PluginError>, elapsed_ms: u32, ) { match result { Ok(_) => self.circuit_breaker.record_success(elapsed_ms), Err(e) => { if Self::is_dead_server_error(e) { self.circuit_breaker.record_failure(); tracing::warn!(error = %e, "Detected dead pool server error, triggering health check"); self.health_check_needed.store(true, Ordering::Relaxed); let pool = self.connection_pool.clone(); tokio::spawn(async move { pool.clear().await; }); } else { self.circuit_breaker.record_success(elapsed_ms); } } } }Also applies to: 893-913
1390-1401: Magic connection ID999999for shutdown request.The shutdown request uses a hardcoded connection ID
999999. Consider using a named constant for clarity:+const SHUTDOWN_CONNECTION_ID: u32 = 999999; + async fn send_shutdown_request( &self, timeout: std::time::Duration, ) -> Result<PoolResponse, PluginError> { // ... - let mut conn = match PoolConnection::new(&self.socket_path, 999999).await { + let mut conn = match PoolConnection::new(&self.socket_path, SHUTDOWN_CONNECTION_ID).await {src/main.rs (1)
203-214: Plugin pool shutdown may be skipped if server fails.If
try_join!orapp_server.await?returns an error, the?operator propagates immediately, skipping the graceful shutdown. Consider ensuring cleanup runs regardless:♻️ Suggested fix
- if let Some(metrics_server) = metrics_server_future { - futures::try_join!(app_server, metrics_server)?; - } else { - app_server.await?; - } + let server_result = if let Some(metrics_server) = metrics_server_future { + futures::try_join!(app_server, metrics_server) + } else { + app_server.await.map(|_| ((), ())) + }; // Graceful shutdown: cleanup plugin pool if it was started if pool_manager.is_some() { if let Err(e) = shutdown_plugin_pool().await { tracing::warn!(error = %e, "Failed to shutdown plugin pool gracefully"); } } + server_result?; + Ok(())src/models/plugin.rs (1)
82-87: Consider adding more validation error variants for future extensibility.The
PluginValidationErrorenum currently only hasInvalidTimeout. As the API evolves, you may want to validate other fields (e.g., config constraints). This is fine for now but consider whether a more generic variant or additional variants might be beneficial.src/bootstrap/initialize_plugins.rs (2)
76-137: Pagination limit may truncate large plugin sets.The hardcoded
per_page: 1000limit means if there are more than 1000 plugins, only the first 1000 will be precompiled. While 1000 is a reasonable default, consider either:
- Iterating through all pages until
plugins.items.is_empty()- Using a higher limit or making it configurable
- Documenting this limitation
♻️ Suggested fix to handle pagination properly
pub async fn precompile_plugins<PR: PluginRepositoryTrait>( plugin_repository: &PR, pool_manager: &PoolManager, ) -> eyre::Result<usize> { use crate::models::PaginationQuery; - let query = PaginationQuery { - page: 1, - per_page: 1000, - }; - - let plugins = plugin_repository - .list_paginated(query) - .await - .map_err(|e| eyre::eyre!("Failed to list plugins: {}", e))?; - let mut compiled_count = 0; + let mut page = 1; + let per_page = 100; + let mut total_plugins = 0; - for plugin in plugins.items { + loop { + let query = PaginationQuery { page, per_page }; + let plugins = plugin_repository + .list_paginated(query) + .await + .map_err(|e| eyre::eyre!("Failed to list plugins: {}", e))?; + + if page == 1 { + total_plugins = plugins.total; + } + + if plugins.items.is_empty() { + break; + } + + for plugin in plugins.items { // ... existing precompilation logic ... + } + + page += 1; } info!( compiled_count = compiled_count, - total_plugins = plugins.total, + total_plugins = total_plugins, "Plugin precompilation complete" ); Ok(compiled_count) }
153-168: Test coverage is minimal.The test only covers the "no plugins" scenario. Consider adding tests for:
- Successful pool initialization when plugins exist
- Precompilation success/failure scenarios
However, these may require more complex mocking of the PoolManager, which might be better suited for integration tests.
openapi.json (2)
4763-4767: Schema duplication risk:forward_logsadded in multiple places; prefer$reftoPluginModel.
forward_logswas added to:
ApiResponse_PaginatedResult_PluginModel.data.items[].forward_logs(Line 4763-4766)PluginModel.forward_logs(Line 7329-7332)To prevent future drift, consider referencing
PluginModelfrom the paginateditemsschema (and fromApiResponse_PluginModel.data), instead of repeating the same property sets in multiple schema blocks.Also applies to: 7329-7332
9702-9763:UpdatePluginRequest: avoid nullable booleans/timeout; reservenullfor “clear config”.The description says “all fields are optional” (Line 9704) and “config: null clears config” (Line 9718-9719), which is great. But allowing
nullfor booleans and integers (Line 9707-9710, 9725-9728, 9753-9756) adds confusing semantics for clients and makes validation weaker. I’d keepconfignullable, but make the other fields non-nullable and optional, and aligntimeout.minimumwith actual validation.src/services/plugins/runner.rs (3)
42-48: Environment variable parsing could be more robust.The function reads an environment variable on every call without caching. For high-frequency plugin executions, this could add unnecessary overhead.
Consider caching the result using
OnceLockor similar pattern, especially since this is called per-execution.♻️ Suggested improvement
+use std::sync::OnceLock; + +static USE_POOL: OnceLock<bool> = OnceLock::new(); + fn use_pool_executor() -> bool { - std::env::var("PLUGIN_USE_POOL") - .map(|v| v.eq_ignore_ascii_case("true") || v == "1") - .unwrap_or(true) // Pool mode is now the default + *USE_POOL.get_or_init(|| { + std::env::var("PLUGIN_USE_POOL") + .map(|v| v.eq_ignore_ascii_case("true") || v == "1") + .unwrap_or(true) + }) }
320-321: Silent fallback to string on JSON parse failure.If
script_paramsis invalid JSON, it silently falls back to wrapping the raw string as a JSON string value. This could mask configuration errors and lead to unexpected plugin behavior.Consider logging a warning when the parse fails so operators can diagnose misconfigured plugin calls.
♻️ Suggested improvement
let params: serde_json::Value = serde_json::from_str(&script_params) - .unwrap_or(serde_json::Value::String(script_params.clone())); + .unwrap_or_else(|e| { + tracing::warn!( + plugin_id = %plugin_id, + error = %e, + "Failed to parse script_params as JSON, using as string" + ); + serde_json::Value::String(script_params.clone()) + });
421-465: Test environment variable cleanup is not exception-safe.If the test panics between
set_varandremove_var, the environment variable will leak to subsequent tests, potentially affecting their behavior. Consider using a guard pattern ordefer!macro.♻️ Suggested pattern using scopeguard
// Using scopeguard crate or a simple struct with Drop struct EnvGuard { key: &'static str, } impl Drop for EnvGuard { fn drop(&mut self) { std::env::remove_var(self.key); } } // In test: std::env::set_var("PLUGIN_USE_POOL", "false"); let _guard = EnvGuard { key: "PLUGIN_USE_POOL" }; // ... rest of test // Variable is cleaned up automatically even on panicsrc/services/plugins/shared_socket.rs (4)
198-208: ExecutionGuard Drop spawns unbounded async task without tracking.The
Dropimplementation spawns a detached tokio task to perform cleanup. This has several implications:
- If many guards are dropped rapidly, this could spawn many concurrent tasks
- The cleanup may complete after the program has moved on, potentially causing issues during shutdown
- There's no backpressure mechanism
Consider using a bounded cleanup queue or synchronous cleanup where feasible.
347-349: Atomic ordering may not provide sufficient synchronization.Using
Ordering::Acquireonswapwithout a correspondingReleasestore elsewhere may not provide the intended memory ordering guarantees. For a simple "start once" flag,Ordering::SeqCstorOrdering::AcqRelwould be safer.♻️ Suggested fix
- if self.started.swap(true, Ordering::Acquire) { + if self.started.swap(true, Ordering::AcqRel) { return Ok(()); }
704-723: Double removal of socket file in initialization.The socket file is removed both in
SharedSocketService::new()(line 233) and inget_shared_socket_service()(line 709). This redundancy is harmless but indicates potential code smell.♻️ Remove redundant deletion
pub fn get_shared_socket_service() -> Result<Arc<SharedSocketService>, PluginError> { let socket_path = "/tmp/relayer-plugin-shared.sock"; let result = SHARED_SOCKET.get_or_init(|| { - // Remove existing socket file if it exists (from previous runs) - let _ = std::fs::remove_file(socket_path); - match SharedSocketService::new(socket_path) {
704-706: Hard-coded socket path reduces flexibility.The socket path
/tmp/relayer-plugin-shared.sockis hard-coded. This prevents running multiple instances on the same machine and makes testing more difficult.Consider making this configurable via the centralized config or environment variable.
src/api/routes/docs/plugin_docs.rs (1)
14-31: Documentation inconsistency between path examples.The documentation shows two ways to specify a route:
- Line 15:
?route=/verify(query parameter with leading slash)- Line 16:
/call/verify(path without leading slash after/call)The route query parameter example includes a leading slash (
/verify) but it's unclear if this is required or just convention. Consider clarifying whether the leading slash is expected/normalized.src/repositories/plugin/plugin_redis.rs (1)
449-456: Silent error swallowing in invalidate_all_compiled_code.Errors during deletion of individual compiled code entries are silently ignored with
let _ = .... While this allows the operation to continue, it could mask systemic issues.Consider at least logging warnings for failed deletions.
♻️ Suggested improvement
for plugin_id in &plugin_ids { let code_key = self.compiled_code_key(plugin_id); let hash_key = self.source_hash_key(plugin_id); - let _ = conn.del::<_, ()>(&code_key).await; - let _ = conn.del::<_, ()>(&hash_key).await; + if let Err(e) = conn.del::<_, ()>(&code_key).await { + warn!(plugin_id = %plugin_id, error = %e, "Failed to delete compiled code"); + } + if let Err(e) = conn.del::<_, ()>(&hash_key).await { + warn!(plugin_id = %plugin_id, error = %e, "Failed to delete source hash"); + } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockexamples/basic-example-plugin/test-plugin/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlplugins/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (45)
.gitignoreCLAUDE.mdCargo.tomlbuild.rsdocs/plugins/index.mdxexamples/basic-example-plugin/test-plugin/package.jsonopenapi.jsonplugins/ARCHITECTURE.mdplugins/examples/example-rpc.tsplugins/lib/build-executor.tsplugins/lib/compiler.tsplugins/lib/constants.tsplugins/lib/kv.tsplugins/lib/logger.tsplugins/lib/plugin.tsplugins/lib/pool-server.tsplugins/lib/sandbox-executor.tsplugins/lib/worker-pool.tsplugins/package.jsonplugins/tests/lib/plugin.test.tsplugins/tests/lib/pool-server.test.tsplugins/tests/lib/sandbox-executor.test.tsplugins/tests/lib/worker-pool.test.tssrc/api/controllers/plugin.rssrc/api/routes/docs/plugin_docs.rssrc/api/routes/plugin.rssrc/bootstrap/initialize_plugins.rssrc/bootstrap/mod.rssrc/constants/plugins.rssrc/main.rssrc/models/plugin.rssrc/openapi.rssrc/repositories/plugin/mod.rssrc/repositories/plugin/plugin_in_memory.rssrc/repositories/plugin/plugin_redis.rssrc/services/plugins/config.rssrc/services/plugins/connection.rssrc/services/plugins/health.rssrc/services/plugins/mod.rssrc/services/plugins/pool_executor.rssrc/services/plugins/protocol.rssrc/services/plugins/runner.rssrc/services/plugins/script_executor.rssrc/services/plugins/shared_socket.rssrc/services/plugins/socket.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.cursor/rules/rust_standards.mdc)
**/*.rs: Follow official Rust style guidelines using rustfmt (edition = 2021, max_width = 100)
Follow Rust naming conventions: snake_case for functions/variables, PascalCase for types, SCREAMING_SNAKE_CASE for constants and statics
Order imports alphabetically and group by: std, external crates, local crates
Include relevant doc comments (///) on public functions, structs, and modules. Use comments for 'why', not 'what'. Avoid redundant doc comments
Document lifetime parameters when they're not obvious
Avoid unsafe code unless absolutely necessary; justify its use in comments
Write idiomatic Rust: Prefer Result over panic, use ? operator for error propagation
Avoid unwrap; handle errors explicitly with Result and custom Error types for all async operations
Prefer header imports over function-level imports of dependencies
Prefer borrowing over cloning when possible
Use &str instead of &String for function parameters
Use &[T] instead of &Vec for function parameters
Avoid unnecessary .clone() calls
Use Vec::with_capacity() when size is known in advance
Use explicit lifetimes only when necessary; infer where possible
Always use Tokio runtime for async code. Await futures eagerly; avoid blocking calls in async contexts
Streams: Use futures::StreamExt for processing streams efficiently
Always use serde for JSON serialization/deserialization with #[derive(Serialize, Deserialize)]
Use type aliases for complex types to improve readability
Implement common traits (Debug, Clone, PartialEq) where appropriate, using derive macros when possible
Implement Display for user-facing types
Prefer defining traits when implementing services to make mocking and testing easier
For tests, prefer existing utils for creating mocks instead of duplicating code
Use assert_eq! and assert! macros appropriately
Keep tests minimal, deterministic, and fast
Use tracing for structured logging, e.g., tracing::info! for request and error logs
When optimizing, prefer clarity first, then performance
M...
Files:
src/models/plugin.rssrc/openapi.rssrc/main.rssrc/bootstrap/initialize_plugins.rsbuild.rssrc/services/plugins/runner.rssrc/api/routes/plugin.rssrc/services/plugins/shared_socket.rssrc/services/plugins/mod.rssrc/constants/plugins.rssrc/bootstrap/mod.rssrc/services/plugins/connection.rssrc/api/controllers/plugin.rssrc/services/plugins/config.rssrc/services/plugins/script_executor.rssrc/repositories/plugin/plugin_in_memory.rssrc/services/plugins/health.rssrc/services/plugins/protocol.rssrc/repositories/plugin/plugin_redis.rssrc/repositories/plugin/mod.rssrc/services/plugins/pool_executor.rssrc/api/routes/docs/plugin_docs.rs
🧠 Learnings (3)
📚 Learning: 2025-12-02T11:22:31.387Z
Learnt from: CR
Repo: OpenZeppelin/openzeppelin-relayer PR: 0
File: .cursor/rules/rust_standards.mdc:0-0
Timestamp: 2025-12-02T11:22:31.387Z
Learning: Applies to **/*.rs : Always use Tokio runtime for async code. Await futures eagerly; avoid blocking calls in async contexts
Applied to files:
src/services/plugins/runner.rssrc/services/plugins/mod.rs
📚 Learning: 2025-12-02T11:22:31.387Z
Learnt from: CR
Repo: OpenZeppelin/openzeppelin-relayer PR: 0
File: .cursor/rules/rust_standards.mdc:0-0
Timestamp: 2025-12-02T11:22:31.387Z
Learning: Applies to **/*.rs : Avoid unwrap; handle errors explicitly with Result and custom Error types for all async operations
Applied to files:
src/services/plugins/mod.rs
📚 Learning: 2025-12-02T11:22:31.387Z
Learnt from: CR
Repo: OpenZeppelin/openzeppelin-relayer PR: 0
File: .cursor/rules/rust_standards.mdc:0-0
Timestamp: 2025-12-02T11:22:31.387Z
Learning: Applies to **/*.rs : Use tracing for structured logging, e.g., tracing::info! for request and error logs
Applied to files:
src/services/plugins/mod.rs
🧬 Code graph analysis (18)
src/models/plugin.rs (2)
src/repositories/plugin/mod.rs (2)
update(54-54)update(120-125)src/repositories/plugin/plugin_redis.rs (1)
update(219-254)
src/openapi.rs (1)
src/api/routes/docs/plugin_docs.rs (2)
doc_get_plugin(360-360)doc_update_plugin(472-472)
plugins/lib/plugin.ts (1)
plugins/lib/constants.ts (1)
DEFAULT_SOCKET_REQUEST_TIMEOUT_MS(35-35)
src/main.rs (4)
src/bootstrap/initialize_app_state.rs (1)
initialize_app_state(110-131)src/bootstrap/initialize_plugins.rs (3)
initialize_plugin_pool(28-59)precompile_plugins(76-137)shutdown_plugin_pool(143-151)src/bootstrap/initialize_workers.rs (1)
initialize_workers(78-283)src/bootstrap/config_processor.rs (1)
process_config_file(335-384)
src/bootstrap/initialize_plugins.rs (1)
src/services/plugins/pool_executor.rs (2)
get_pool_manager(1414-1418)new(81-83)
plugins/lib/pool-server.ts (2)
plugins/lib/worker-pool.ts (1)
PluginExecutionRequest(61-86)plugins/lib/constants.ts (5)
DEFAULT_POOL_MIN_THREADS(17-17)DEFAULT_POOL_MAX_THREADS_FLOOR(20-20)DEFAULT_POOL_CONCURRENT_TASKS_PER_WORKER(23-23)DEFAULT_POOL_IDLE_TIMEOUT_MS(26-26)DEFAULT_POOL_SOCKET_BACKLOG(29-29)
plugins/tests/lib/plugin.test.ts (1)
plugins/lib/plugin.ts (1)
DefaultPluginAPI(410-594)
src/api/routes/plugin.rs (5)
src/api/controllers/plugin.rs (2)
get_plugin(181-203)update_plugin(220-254)src/repositories/plugin/plugin_in_memory.rs (3)
new(51-56)default(69-71)update(87-97)src/repositories/plugin/plugin_redis.rs (2)
new(27-41)update(219-254)src/models/api_response.rs (3)
new(37-45)error(57-65)success(47-55)src/models/plugin.rs (1)
default(137-149)
plugins/lib/sandbox-executor.ts (4)
plugins/lib/kv.ts (2)
key(139-142)DefaultPluginKVStore(102-299)plugins/lib/plugin.ts (4)
PluginHeaders(216-216)PluginAPI(195-210)Relayer(150-189)PluginContext(221-230)plugins/lib/constants.ts (1)
DEFAULT_SOCKET_REQUEST_TIMEOUT_MS(35-35)plugins/lib/compiler.ts (1)
wrapForVm(563-586)
src/constants/plugins.rs (1)
plugins/lib/constants.ts (6)
DEFAULT_POOL_MIN_THREADS(17-17)DEFAULT_POOL_MAX_THREADS_FLOOR(20-20)DEFAULT_POOL_CONCURRENT_TASKS_PER_WORKER(23-23)DEFAULT_POOL_IDLE_TIMEOUT_MS(26-26)DEFAULT_POOL_SOCKET_BACKLOG(29-29)DEFAULT_POOL_EXECUTION_TIMEOUT_MS(32-32)
src/services/plugins/connection.rs (1)
src/services/plugins/config.rs (1)
get_config(474-480)
plugins/lib/worker-pool.ts (3)
plugins/lib/sandbox-executor.ts (3)
LogEntry(347-350)SandboxTask(299-324)SandboxResult(329-345)plugins/lib/constants.ts (5)
DEFAULT_POOL_EXECUTION_TIMEOUT_MS(32-32)DEFAULT_POOL_MIN_THREADS(17-17)DEFAULT_POOL_MAX_THREADS_FLOOR(20-20)DEFAULT_POOL_CONCURRENT_TASKS_PER_WORKER(23-23)DEFAULT_POOL_IDLE_TIMEOUT_MS(26-26)plugins/lib/compiler.ts (1)
CompilationResult(38-45)
src/services/plugins/config.rs (2)
plugins/lib/constants.ts (5)
DEFAULT_POOL_CONCURRENT_TASKS_PER_WORKER(23-23)DEFAULT_POOL_IDLE_TIMEOUT_MS(26-26)DEFAULT_POOL_MAX_THREADS_FLOOR(20-20)DEFAULT_POOL_MIN_THREADS(17-17)DEFAULT_POOL_SOCKET_BACKLOG(29-29)src/services/plugins/shared_socket.rs (1)
new(231-268)
plugins/tests/lib/worker-pool.test.ts (5)
plugins/lib/kv.ts (1)
key(139-142)plugins/lib/sandbox-executor.ts (1)
size(183-185)plugins/lib/pool-server.ts (1)
onEmergency(112-114)plugins/lib/worker-pool.ts (1)
stats(399-405)src/models/rpc/json_rpc.rs (1)
result(112-119)
src/services/plugins/script_executor.rs (2)
plugins/lib/logger.ts (1)
LogLevel(1-1)src/api/routes/plugin.rs (1)
serde_json(97-97)
src/services/plugins/protocol.rs (1)
plugins/lib/logger.ts (1)
LogLevel(1-1)
src/repositories/plugin/mod.rs (2)
src/repositories/plugin/plugin_in_memory.rs (6)
get_compiled_code(142-145)store_compiled_code(147-162)invalidate_compiled_code(164-168)invalidate_all_compiled_code(170-174)has_compiled_code(176-179)get_source_hash(181-184)src/repositories/plugin/plugin_redis.rs (6)
get_compiled_code(377-389)store_compiled_code(391-416)invalidate_compiled_code(418-435)invalidate_all_compiled_code(437-459)has_compiled_code(461-471)get_source_hash(473-483)
src/services/plugins/pool_executor.rs (3)
src/services/plugins/config.rs (3)
get_config(474-480)s(204-204)s(220-220)src/services/plugins/health.rs (3)
new(19-30)new(170-180)from_error_str(118-124)src/services/plugins/connection.rs (2)
new(30-78)new(159-167)
🪛 Biome (2.1.2)
plugins/lib/pool-server.ts
[error] 611-611: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Safe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🪛 LanguageTool
CLAUDE.md
[grammar] ~75-~75: Ensure spelling is correct
Context: ...equested. - Document API endpoints with utoipa schemas and regenerate OpenAPI spec whe...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
plugins/ARCHITECTURE.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
113-113: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
183-183: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
248-248: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
CLAUDE.md
36-36: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
There was a problem hiding this comment.
Pull request overview
This draft PR introduces significant performance improvements to the plugin system by implementing pool-based execution with a persistent worker pool, replacing the per-request ts-node spawning approach.
Changes:
- Implemented a shared Unix socket service for bidirectional plugin communication, reducing file descriptor overhead
- Added pool-based plugin execution using Piscina worker pools with health monitoring, circuit breaker, and precompilation support
- Introduced plugin configuration management API endpoints (GET/PATCH) for runtime updates
Reviewed changes
Copilot reviewed 43 out of 48 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/services/plugins/shared_socket.rs | New shared socket service for unified bidirectional communication |
| src/services/plugins/pool_executor.rs | Pool manager with health monitoring and circuit breaker |
| src/services/plugins/runner.rs | Updated to support both pool and ts-node execution modes |
| src/services/plugins/protocol.rs | Protocol types for pool server communication |
| src/services/plugins/health.rs | Health monitoring and circuit breaker implementation |
| src/services/plugins/connection.rs | Connection pool for Unix socket reuse |
| src/services/plugins/config.rs | Centralized configuration with auto-derivation |
| src/repositories/plugin/plugin_redis.rs | Added compiled code caching methods |
| src/repositories/plugin/plugin_in_memory.rs | Added compiled code caching support |
| src/repositories/plugin/mod.rs | Updated trait with cache operations |
| src/models/plugin.rs | Added UpdatePluginRequest and validation |
| src/api/routes/plugin.rs | Added GET/PATCH endpoints and route resolution |
| src/api/controllers/plugin.rs | Added get_plugin and update_plugin controllers |
| src/main.rs | Added plugin pool initialization and shutdown |
| src/bootstrap/initialize_plugins.rs | New module for plugin pool initialization |
| src/constants/plugins.rs | Added comprehensive plugin configuration constants |
| plugins/tests/lib/worker-pool.test.ts | Tests for cache and memory monitor logic |
Files not reviewed (2)
- examples/basic-example-plugin/test-plugin/pnpm-lock.yaml: Language not supported
- plugins/pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (2)
src/services/plugins/config.rs:1
- Magic number 300 (5 minutes) should be extracted as a named constant for the stale execution cleanup threshold. This makes the timeout value discoverable and easier to configure if needed.
//! Plugin Configuration
src/services/plugins/pool_executor.rs:1
- Magic number 5 (MB per task) should be extracted as a named constant HEAP_MB_PER_CONCURRENT_TASK with documentation explaining this estimate is based on vm.createContext() memory usage.
//! Pool-based Plugin Executor
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }; | ||
|
|
||
| // Run with timeout | ||
| return withTimeout( |
There was a problem hiding this comment.
Are we sure that this actually works? Like if there is a timeout, esbuild stop the build?
There was a problem hiding this comment.
It does not actually stop build but just bails out of it at timeout and continues with other plugin initialisation logic.
Adding logic that dives into esbuild actually stop it is out of scope for this PR.
Eventually it will just complete.
30s should be safe value for timeout given that esbuild is performant.
- Add automatic recovery for transactions stuck in Sent status >5 minutes - Re-enqueue submit job if signed envelope exists, otherwise mark as Failed - Track recovery attempts via noop_count (max 3) to prevent infinite loops - Increase submit job retries from 1 to 3 (4 total attempts) - Only warn about missing hash for transactions that should have one Signed-off-by: Dylan Kilkenny <dylankilkenny95@gmail.com>
Move Sent status check before hash parsing as an early exit, matching the approach in PR #606. This simplifies the logic since: - Pending and Sent transactions don't have on-chain hashes yet - Only Submitted+ transactions need hash validation - If hash is missing for Submitted+, it's a database inconsistency Remove unused is_unsubmitted_transaction import.
* fix: Recover stuck Stellar Sent transactions - Add automatic recovery for transactions stuck in Sent status >5 minutes - Re-enqueue submit job if signed envelope exists, otherwise mark as Failed - Track recovery attempts via noop_count (max 3) to prevent infinite loops - Increase submit job retries from 1 to 3 (4 total attempts) - Only warn about missing hash for transactions that should have one Signed-off-by: Dylan Kilkenny <dylankilkenny95@gmail.com> * refactor: Simplify Stellar stuck Sent recovery logic - Replace 5-min threshold with 30s resend timeout + 30-min max lifetime - Revert WORKER_TRANSACTION_SUBMIT_RETRIES to 1 (affects all networks) - Reorder checks: expired → max lifetime (more specific status first) - Remove signed_envelope_xdr check from submit handler - Fix Preconditions::V2 handling in extract_time_bounds - Fix max_time==0 (unbounded) handling in validation - Extract valid_until from XDR time_bounds at request time - Fix u64 to i64 cast with try_from * refactor: Revert to is_final_state check Revert the strict Sent-only idempotency check back to the original is_final_state check. Keep the expiry check before submission. * test: Add max lifetime failsafe test Add test_stuck_sent_transaction_max_lifetime_marks_failed to verify that transactions stuck in Sent status for longer than 30 minutes are marked as Failed. This complements the existing tests for the re-enqueue and expired scenarios. Signed-off-by: Dylan Kilkenny <dylankilkenny95@gmail.com> * Fix/stellar stuck recovery improvements (#616) * fix: Stellar status improvements * chore: Handle submitted state, mark old txs as failed * fix: Update tests to use find_by_status_paginated Update tests in handle_pending_state_tests module to mock find_by_status_paginated instead of find_by_status to match the updated TransactionRepository trait from main branch. * refactor: Early exit for Sent in status_core Move Sent status check before hash parsing as an early exit, matching the approach in PR #606. This simplifies the logic since: - Pending and Sent transactions don't have on-chain hashes yet - Only Submitted+ transactions need hash validation - If hash is missing for Submitted+, it's a database inconsistency Remove unused is_unsubmitted_transaction import. * chore: Use sent_at timestamp for stellar status handler --------- Signed-off-by: Dylan Kilkenny <dylankilkenny95@gmail.com> Co-authored-by: Zeljko <zeljko89markovic@gmail.com>
|
Thanks for your reviews! I have reviewed feedback and applied most of them. If you find time please try to review this PR again so we can finalize it. Btw some parts are hard to test and hitting coverage is hard. Thanks! |
Summary
SDK PR: OpenZeppelin/openzeppelin-relayer-sdk#243
Testing Process
Checklist
Note
If you are using Relayer in your stack, consider adding your team or organization to our list of Relayer Users in the Wild!
Summary by CodeRabbit
Release Notes
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.