-
Notifications
You must be signed in to change notification settings - Fork 43
add go sidecar build #1622
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
add go sidecar build #1622
Conversation
| process.on("exit", cleanup); | ||
| process.on("SIGINT", cleanup); | ||
| process.on("SIGTERM", cleanup); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Medium
src/process-manager.ts:264 Custom SIGINT/SIGTERM handlers suppress Node.js default termination. Consider calling process.exit() after the cleanup to ensure the process actually terminates.
| process.on("exit", cleanup); | |
| process.on("SIGINT", cleanup); | |
| process.on("SIGTERM", cleanup); | |
| process.on("exit", cleanup); | |
| process.on("SIGINT", () => { | |
| cleanup(); | |
| process.exit(130); | |
| }); | |
| process.on("SIGTERM", () => { | |
| cleanup(); | |
| process.exit(143); | |
| }); |
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file npm/gateway/src/process-manager.ts around lines 264-266:
Custom `SIGINT`/`SIGTERM` handlers suppress Node.js default termination. Consider calling `process.exit()` after the cleanup to ensure the process actually terminates.
| ); | ||
| } | ||
|
|
||
| function gracefulShutdown(child: ChildProcess): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Medium
src/process-manager.ts:204 Suggestion: make stop() idempotent (track shutdown state) so repeated calls don’t add timers/listeners, and cancel any pending auto‑restart timer to prevent a restart after stopping.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file npm/gateway/src/process-manager.ts around line 204:
Suggestion: make `stop()` idempotent (track shutdown state) so repeated calls don’t add timers/listeners, and cancel any pending auto‑restart timer to prevent a restart after stopping.
| socket.once("error", () => { | ||
| socket.close(); | ||
| // Port is in use (gateway is listening) — this is what we want | ||
| resolve(true); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Medium
src/process-manager.ts:172 Calling socket.close() in the error handler throws ERR_SERVER_NOT_RUNNING since the server never started. Consider removing that call.
| socket.once("error", () => { | |
| socket.close(); | |
| // Port is in use (gateway is listening) — this is what we want | |
| resolve(true); | |
| }); | |
| socket.once("error", () => { | |
| // Port is in use (gateway is listening) — this is what we want | |
| resolve(true); | |
| }); |
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file npm/gateway/src/process-manager.ts around lines 172-176:
Calling `socket.close()` in the error handler throws `ERR_SERVER_NOT_RUNNING` since the server never started. Consider removing that call.
| node -e " | ||
| const fs = require('fs'); | ||
| const pkg = JSON.parse(fs.readFileSync('${PKG_JSON}', 'utf8')); | ||
| pkg.version = '${VERSION}'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 Critical
npm/sync-versions.sh:31 Command injection vulnerability: VERSION is interpolated directly into JavaScript strings without escaping. A malicious version like 1.0.0'; exec('cmd'); ' would execute arbitrary code. Consider passing VERSION via an environment variable and reading it with process.env.VERSION instead of string interpolation.
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file npm/sync-versions.sh around line 31:
Command injection vulnerability: `VERSION` is interpolated directly into JavaScript strings without escaping. A malicious version like `1.0.0'; exec('cmd'); '` would execute arbitrary code. Consider passing `VERSION` via an environment variable and reading it with `process.env.VERSION` instead of string interpolation.
| const earlyExitPromise = new Promise<never>((_, reject) => { | ||
| child.on("exit", (code, signal) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 High
src/process-manager.ts:30 Missing error event handler on spawned child process. If the binary fails to spawn (permissions, invalid path), Node.js crashes. Consider adding child.on('error', ...) and rejecting earlyExitPromise from it.
- const earlyExitPromise = new Promise<never>((_, reject) => {
- child.on("exit", (code, signal) => {
+ const earlyExitPromise = new Promise<never>((_, reject) => {
+ child.on("error", (err) => {
+ reject(new Error(`Gateway failed to spawn: ${err.message}`));
+ });
+ child.on("exit", (code, signal) => {🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file npm/gateway/src/process-manager.ts around lines 30-31:
Missing `error` event handler on spawned child process. If the binary fails to spawn (permissions, invalid path), Node.js crashes. Consider adding `child.on('error', ...)` and rejecting `earlyExitPromise` from it.
|
|
||
| function setupLogForwarding(child: ChildProcess): void { | ||
| child.stdout?.on("data", (data: Buffer) => { | ||
| const lines = data.toString().split("\n").filter(Boolean); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟢 Low
src/process-manager.ts:109 Stream data events can split lines across chunks, causing JSON.parse to fail on partial JSON. Consider buffering incomplete lines between chunks and only processing complete lines (those ending with a newline).
🚀 Want me to fix this? Reply ex: "fix it for me".
🤖 Prompt for AI
In file npm/gateway/src/process-manager.ts around line 109:
Stream `data` events can split lines across chunks, causing `JSON.parse` to fail on partial JSON. Consider buffering incomplete lines between chunks and only processing complete lines (those ending with a newline).
| XMTPD_API_PORT: String(port), | ||
| XMTPD_API_ENABLE: "true", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it need the API? Thats a lot of computation for nothing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for now just testing, will clean it up later.
|
|
||
| on: | ||
| push: | ||
| branches: [main, dev] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is no dev branch
|
@mchenani this particular implementation of the gateway does not support any extensions or JWT token auth. Does that meet your requirements? |
Thanks for the review, tbh I'm not sure if I'm aware of the requirements there, will discuss them with @cameronvoell to make sure I have the full picture how we want to tackle that. |
No description provided.