Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Greptile SummaryThis PR introduces a new Key changes:
Issues found:
Confidence Score: 3/5Not safe to merge as-is — the commented-out subscribe message may cause the monitor to be permanently in false-alert mode, and the race condition in sendFatalAlert can spam Slack on first trigger. Two P1 issues are present: (1) the subscribe message is commented out, which may prevent events from ever being received and cause constant false alerts that spam the Slack channel, and (2) the async race condition in sendFatalAlert allows duplicate concurrent Slack messages. Both need to be resolved before this service runs in production. monitoring/src/main.ts requires the most attention — specifically the commented-out subscribe block and the sendFatalAlert race condition. Important Files Changed
Sequence DiagramsequenceDiagram
participant M as WebSocketMonitor
participant WS as Backend WebSocket
participant HI as setInterval (2s)
participant SL as Slack API
M->>WS: new WebSocket(backendUrl)
WS-->>M: open event
M->>M: scheduleConnectionRefresh()
Note over M: (subscribe message commented out)
loop Every 2 seconds
HI->>M: checkHealth()
alt silenceDuration > SILENCE_THRESHOLD
M->>M: inFatalState = true
M->>M: sendFatalAlert() [no await]
M->>SL: chat.postMessage (fatal alert)
SL-->>M: ok
M->>M: lastNotification = now
else silenceDuration <= threshold & was in fatal state
M->>M: inFatalState = false
M->>M: sendRecoveryAlert() [no await]
M->>SL: chat.postMessage (recovery)
end
end
WS-->>M: message event
M->>M: lastMessageTime = Date.now()
WS-->>M: close event
M->>M: scheduleReconnect() (exp. backoff)
M->>WS: new WebSocket(backendUrl)
M->>M: shutdown(SIGINT/SIGTERM)
M->>SL: chat.postMessage (offline alert)
M->>M: process.exit(0)
Reviews (1): Last reviewed commit: "reset last msg time on reconnect" | Re-trigger Greptile |
| private async sendFatalAlert(silenceDuration: number) { | ||
| const now = Date.now() | ||
|
|
||
| // Check cooldown | ||
| if ( | ||
| this.lastNotification > 0 && | ||
| now - this.lastNotification < COOLDOWN_MS | ||
| ) { | ||
| console.log( | ||
| `Skipping alert (cooldown: ${((now - this.lastNotification) / 1000).toFixed(0)}s / ${(COOLDOWN_MS / 1000).toFixed(0)}s)`, | ||
| ) | ||
| return | ||
| } |
There was a problem hiding this comment.
Race condition allows duplicate Slack alerts
sendFatalAlert is an async function but is called without await from checkHealth (which runs every 2 seconds via setInterval). The cooldown guard only prevents duplicate alerts after this.lastNotification has been set — but lastNotification is only updated after the Slack API call completes (line 310). During the initial alert window when lastNotification === 0, the condition this.lastNotification > 0 && ... is false, so every health-check tick (every 2 s) that fires before the first Slack response will pass the guard and dispatch a separate Slack message.
To fix this, set this.lastNotification = now before the await call so subsequent concurrent invocations see it immediately:
| private async sendFatalAlert(silenceDuration: number) { | |
| const now = Date.now() | |
| // Check cooldown | |
| if ( | |
| this.lastNotification > 0 && | |
| now - this.lastNotification < COOLDOWN_MS | |
| ) { | |
| console.log( | |
| `Skipping alert (cooldown: ${((now - this.lastNotification) / 1000).toFixed(0)}s / ${(COOLDOWN_MS / 1000).toFixed(0)}s)`, | |
| ) | |
| return | |
| } | |
| private async sendFatalAlert(silenceDuration: number) { | |
| const now = Date.now() | |
| // Check cooldown | |
| if ( | |
| this.lastNotification > 0 && | |
| now - this.lastNotification < COOLDOWN_MS | |
| ) { | |
| console.log( | |
| `Skipping alert (cooldown: ${((now - this.lastNotification) / 1000).toFixed(0)}s / ${(COOLDOWN_MS / 1000).toFixed(0)}s)`, | |
| ) | |
| return | |
| } | |
| // Lock immediately to prevent concurrent calls from bypassing the cooldown | |
| this.lastNotification = now |
| // Send subscribe message (must be sent within 10 seconds) | ||
| //const subscribeMessage = { | ||
| //type: 'subscribe', | ||
| //event_filters: [ | ||
| //{'event_name': 'BlockStart'} | ||
| //] | ||
| //} | ||
| //this.ws?.send(JSON.stringify(subscribeMessage)) | ||
| //console.log('Sent subscribe message') |
There was a problem hiding this comment.
Commented-out subscribe message may cause constant false alerts
The subscribe message that registers this client to receive events is entirely commented out. The inline comment itself says "must be sent within 10 seconds," which suggests the server requires it. If the WebSocket server only sends events to subscribed clients, this monitor will never receive any messages and will immediately and permanently report a "FATAL STATE: No messages" alert after the 10-second SILENCE_THRESHOLD_MS.
If the subscription is intentionally disabled (e.g., the server pushes events to all connections automatically), the dead code block should be removed to avoid confusion. If it is still needed, it should be re-enabled before this service goes live.
| # SILENCE_THRESHOLD_MS=10000 # Default: 10 seconds | ||
| # COOLDOWN_MS=300000 # Default: 5 minutes |
There was a problem hiding this comment.
COOLDOWN_MS default documented incorrectly + missing CONNECTION_MAX_LIFETIME_MIN
Two documentation gaps in the env example:
-
The comment says the default for
COOLDOWN_MSis 5 minutes (300000 ms), but the source code sets it to300_000 * 3 = 900_000 ms(15 minutes). Operators relying on this file to understand defaults will be misled. -
The
CONNECTION_MAX_LIFETIME_MINvariable (used on line 14 ofmain.ts, defaulting to 5 minutes) is not listed here at all. It should be documented so it can be tuned.
Suggested addition:
| # SILENCE_THRESHOLD_MS=10000 # Default: 10 seconds | |
| # COOLDOWN_MS=300000 # Default: 5 minutes | |
| # Optional: Override default thresholds | |
| # SILENCE_THRESHOLD_MS=10000 # Default: 10 seconds | |
| # COOLDOWN_MS=900000 # Default: 15 minutes | |
| # CONNECTION_MAX_LIFETIME_MIN=5 # Default: 5 minutes |
| try { | ||
| await this.slackClient.chat.postMessage({ | ||
| channel: this.config.slackChannel, | ||
| text: '🟢 *Monode Monitor Service Online*', | ||
| blocks: [], | ||
| }) | ||
|
|
||
| console.log('Slack online alert sent') |
There was a problem hiding this comment.
sendOnlineAlert sends an empty blocks array
Every other alert method (sendOfflineAlert, sendFatalAlert) fills the blocks array with properly formatted sections. sendOnlineAlert passes blocks: [], which means Slack will only display the plain-text text field with no rich formatting. This is inconsistent with the other alerts. Consider either populating the blocks or removing the blocks key entirely to rely solely on the text field.
Motivation:
Explain here the context, and why you're making that change. What is the problem you're trying to solve.
Modifications:
Describe the modifications you've done.
Result:
After your change, what will change.