🐛 Surface analyzer error during server startup - #1461
Conversation
Signed-off-by: Alejandro Brugarolas <abrugaro@redhat.com>
📝 WalkthroughWalkthroughThe pull request adds a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vscode/core/src/client/analyzerClient.ts (1)
163-170:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEarly-abort retry check is bypassed after process handlers clear
this.analyzerRpcServer.
exit/close/errorsetthis.analyzerRpcServer = null, but the retry loop aborts only when the object exists andexitCode !== null. After a fast failure, retries can still continue unnecessarily.💡 Proposed fix
- if (this.analyzerRpcServer && this.analyzerRpcServer.exitCode !== null) { + if ( + !this.analyzerRpcServer || + this.analyzerRpcServer.exitCode !== null || + this.analyzerRpcServer.signalCode !== null + ) { throw new Error( - `Analyzer process exited with code ${this.analyzerRpcServer.exitCode} before the pipe became available.`, + "Analyzer process exited before the pipe became available.", ); }Also applies to: 171-175, 271-277
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vscode/core/src/client/analyzerClient.ts` around lines 163 - 170, The issue is that the process event handlers for "exit", "close", and "error" (visible in the diff and mentioned in lines 271-277) set this.analyzerRpcServer to null, but the retry loop's abort condition checks if this.analyzerRpcServer exists, causing retries to continue unnecessarily after fast failures. To fix this, introduce a separate flag (such as this.analyzerServerStopped or similar) that is set to true in all three event handlers before clearing this.analyzerRpcServer, and then modify the retry loop's abort condition to check this flag in addition to (or instead of) checking if this.analyzerRpcServer exists. This ensures the retry mechanism recognizes the server has stopped even after the reference is cleared.
🧹 Nitpick comments (1)
vscode/core/src/client/analyzerClient.ts (1)
354-357: ⚡ Quick winAvoid warning-level spam from normal stderr progress output.
With
-progress-output stderrenabled, non-error progress payloads can be logged as warnings. Consider filtering/parsing progress lines (debug/info) and reservingwarnfor non-progress stderr content.Also applies to: 372-376
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vscode/core/src/client/analyzerClient.ts` around lines 354 - 357, The progress output configuration in the analyzerClient.ts file with `-progress-output stderr` and `-progress-format json` is causing non-error progress payloads to be logged as warnings, creating unnecessary warning spam. You need to parse the stderr output and differentiate between progress lines and actual error content. For JSON-formatted progress payloads received on stderr, log them at debug or info level instead of warn level, and reserve the warn log level only for actual non-progress stderr content that represents genuine errors or warnings. Update the stderr processing logic to examine the content and route it to the appropriate log level based on whether it is a progress update or actual error information.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@vscode/core/src/client/analyzerLogTailer.ts`:
- Around line 54-72: In the poll() method, the condition checking if currentSize
is less than or equal to this.lastSize currently returns early without handling
log file rotation. When a log file is truncated or rotated, currentSize will be
smaller than this.lastSize, causing subsequent content to be skipped. Modify the
logic to detect when currentSize is less than this.lastSize (indicating
rotation/truncation) and reset this.lastSize to 0 in that case, allowing the
file to be re-read from the beginning and capturing any startup errors in the
rotated file.
---
Outside diff comments:
In `@vscode/core/src/client/analyzerClient.ts`:
- Around line 163-170: The issue is that the process event handlers for "exit",
"close", and "error" (visible in the diff and mentioned in lines 271-277) set
this.analyzerRpcServer to null, but the retry loop's abort condition checks if
this.analyzerRpcServer exists, causing retries to continue unnecessarily after
fast failures. To fix this, introduce a separate flag (such as
this.analyzerServerStopped or similar) that is set to true in all three event
handlers before clearing this.analyzerRpcServer, and then modify the retry
loop's abort condition to check this flag in addition to (or instead of)
checking if this.analyzerRpcServer exists. This ensures the retry mechanism
recognizes the server has stopped even after the reference is cleared.
---
Nitpick comments:
In `@vscode/core/src/client/analyzerClient.ts`:
- Around line 354-357: The progress output configuration in the
analyzerClient.ts file with `-progress-output stderr` and `-progress-format
json` is causing non-error progress payloads to be logged as warnings, creating
unnecessary warning spam. You need to parse the stderr output and differentiate
between progress lines and actual error content. For JSON-formatted progress
payloads received on stderr, log them at debug or info level instead of warn
level, and reserve the warn log level only for actual non-progress stderr
content that represents genuine errors or warnings. Update the stderr processing
logic to examine the content and route it to the appropriate log level based on
whether it is a progress update or actual error information.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 58d81926-913d-40fd-a910-14bda49e37c5
📒 Files selected for processing (3)
changes/unreleased/surface-analyzer-errors-to-output-channel.yamlvscode/core/src/client/analyzerClient.tsvscode/core/src/client/analyzerLogTailer.ts
djzager
left a comment
djzager
left a comment
There was a problem hiding this comment.
Nice approach — tailing the log file directly is the right call since errors go to analyzer.log, not stderr. Three things to address.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vscode/core/src/client/analyzerClient.ts (1)
183-188: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClean up the tailer on pre-connection startup failures.
AnalyzerLogTaileris started before the first awaited failure point, butstart()has no cleanup path ifgetSocket()throws while the analyzer process is still alive. In that case the poller keeps running and the server state stays"starting". Wrap the startup handshake in atry/catchso failed connection setup stops the tailer, marksstartFailed, and tears down the orphaned process.Proposed fix
const analyzerLogPath = path.join(paths().serverLogs.fsPath, "analyzer.log"); this.logTailer = new AnalyzerLogTailer(analyzerLogPath, this.logger); this.logTailer.start(); - const socket: Socket = await this.getSocket(pipeName); + let socket: Socket; + try { + socket = await this.getSocket(pipeName); + } catch (err) { + this.stopLogTailer(); + this.fireServerStateChange("startFailed"); + if (this.analyzerRpcServer && this.analyzerRpcServer.exitCode === null) { + this.analyzerRpcServer.kill(); + } + throw err; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vscode/core/src/client/analyzerClient.ts` around lines 183 - 188, The startup handshake in analyzerClient.ts leaves AnalyzerLogTailer running if getSocket() fails before connection is established. Wrap the tailer start and socket acquisition flow in a try/catch around AnalyzerLogTailer.start and getSocket in the analyzer client startup path, and on failure stop/clean up the tailer, set startFailed, and tear down the orphaned analyzer process so the server does not remain in "starting".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@vscode/core/src/client/analyzerClient.ts`:
- Around line 372-382: The stderr handler in analyzerClient’s
analyzerRpcServer.on("data") callback is classifying an entire chunk as one
message, so multiple newline-delimited lines can be mislogged together. Update
the listener to split the incoming data into individual lines before applying
the stage check and logging, and then process each line independently so
progress JSON and real errors are classified correctly.
---
Outside diff comments:
In `@vscode/core/src/client/analyzerClient.ts`:
- Around line 183-188: The startup handshake in analyzerClient.ts leaves
AnalyzerLogTailer running if getSocket() fails before connection is established.
Wrap the tailer start and socket acquisition flow in a try/catch around
AnalyzerLogTailer.start and getSocket in the analyzer client startup path, and
on failure stop/clean up the tailer, set startFailed, and tear down the orphaned
analyzer process so the server does not remain in "starting".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b1bda5b4-31ca-4025-aa24-bad9ccb51235
📒 Files selected for processing (2)
vscode/core/src/client/analyzerClient.tsvscode/core/src/client/analyzerLogTailer.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- vscode/core/src/client/analyzerLogTailer.ts
djzager
left a comment
djzager
left a comment
There was a problem hiding this comment.
All three comments addressed: lastSize starts at 0, stderr filters progress JSON to debug, early-abort handles null server ref. Thanks.
Signed-off-by: Alejandro Brugarolas <abrugaro@redhat.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
vscode/core/src/client/analyzerClient.ts (1)
184-189: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClean up the tailer on startup-path failures.
Line 186 starts a new
AnalyzerLogTailer, but if Line 189 throws before the process exits or emits"started", none of the current cleanup paths run. That leaves the poller alive against a failed startup and can strand the client in a startup state while the analyzer keeps retrying in the background.Suggested fix
- this.logTailer = new AnalyzerLogTailer(analyzerLogPath, this.logger); - this.logTailer.start(); - - const socket: Socket = await this.getSocket(pipeName); + this.logTailer = new AnalyzerLogTailer(analyzerLogPath, this.logger); + this.logTailer.start(); + + let socket: Socket; + try { + socket = await this.getSocket(pipeName); + } catch (err) { + this.stopLogTailer(); + this.fireServerStateChange("startFailed"); + if (this.analyzerRpcServer && this.analyzerRpcServer.exitCode === null) { + this.analyzerRpcServer.kill(); + } + throw err; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vscode/core/src/client/analyzerClient.ts` around lines 184 - 189, The startup path in AnalyzerClient leaves AnalyzerLogTailer running if getSocket(pipeName) fails before the client reaches a started state. Update the startup flow in analyzerClient’s constructor/startup method to wrap the tailer creation and socket acquisition so any failure explicitly stops and clears this.logTailer before the error is propagated. Use the existing AnalyzerLogTailer and getSocket symbols to locate the cleanup point, and make sure the tailer is also cleaned up on all startup-path exceptions, not just the current exit or "started" paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@vscode/core/src/client/analyzerClient.ts`:
- Around line 184-189: The startup path in AnalyzerClient leaves
AnalyzerLogTailer running if getSocket(pipeName) fails before the client reaches
a started state. Update the startup flow in analyzerClient’s constructor/startup
method to wrap the tailer creation and socket acquisition so any failure
explicitly stops and clears this.logTailer before the error is propagated. Use
the existing AnalyzerLogTailer and getSocket symbols to locate the cleanup
point, and make sure the tailer is also cleaned up on all startup-path
exceptions, not just the current exit or "started" paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1c82d4d0-2547-48cf-a693-7fd8a5bbd1c3
📒 Files selected for processing (1)
vscode/core/src/client/analyzerClient.ts
|
Failed to cherry-pick this PR to branch release-0.4. View failed action |
2 participants
Resolves #1422
Surface analyzer log errors to core extension's output during server status.
When the analyzer encounters errors during startup, these errors are only written to the analyzer.log file and never surfaced to the user. The extension's start button remains in a spinner state for 5 minutes with no indication of what went wrong, forcing users to manually locate and inspect the log file.
This:
Did not surface analyzer errors because they are written to the log file, not to stderr.
Before
After
Summary by CodeRabbit