Fix: prevent global environment pollution in multi-site ls check#2174
Open
Dana-Johnson wants to merge 1 commit into
Open
Fix: prevent global environment pollution in multi-site ls check#2174Dana-Johnson wants to merge 1 commit into
Dana-Johnson wants to merge 1 commit into
Conversation
WalkthroughThis PR modifies environment state tracking within the Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 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 `@lib/instance.js`:
- Around line 210-222: The environment is not restored if
this.process.isRunning(this.dir) throws in envIsRunning; refactor env switch to:
capture originalEnv, set the desired environment, declare let running = false,
then call running = await this.process.isRunning(this.dir) inside a try block
(and inside the try update this._cliConfig.set('running', environment).save()
when running is truthy), and in a finally block always call
this.system.setEnvironment(originalEnv === 'development') to restore global
state before returning running.
- Around line 395-398: The summary() method currently mixes environments: it
sets mode from this._cliConfig.get('running') || this.system.environment but
then reads url/port/process from the mutable global config, which can cause
inconsistent results under concurrent "ghost ls" calls; fix by capturing the
tracked environment into a local variable (e.g. const trackedEnv =
this._cliConfig.get('running') || this.system.environment) immediately before
reading the other fields, and then use that trackedEnv to obtain
environment-scoped values for url, port and process (either via an
environment-specific config accessor or by reloading the config for trackedEnv)
so all returned fields (mode, url, port, process) are derived from the same
environment inside summary().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+210
to
222
| // 1. Save original environment state | ||
| const originalEnv = this.system.environment; | ||
|
|
||
| this.system.setEnvironment(environment === 'development'); | ||
| const running = await this.process.isRunning(this.dir); | ||
| if (running) { | ||
| this._cliConfig.set('running', environment).save(); | ||
| } | ||
|
|
||
| // 2. Restore original environment state | ||
| this.system.setEnvironment(originalEnv === 'development'); | ||
|
|
||
| return running; |
There was a problem hiding this comment.
Ensure environment restoration on exceptions in envIsRunning.
At Line 214, if this.process.isRunning(this.dir) throws, Line 220 never runs, leaving global environment mutated. Wrap the switch/restore block in try/finally.
Proposed fix
- // 1. Save original environment state
- const originalEnv = this.system.environment;
-
- this.system.setEnvironment(environment === 'development');
- const running = await this.process.isRunning(this.dir);
- if (running) {
- this._cliConfig.set('running', environment).save();
- }
-
- // 2. Restore original environment state
- this.system.setEnvironment(originalEnv === 'development');
-
- return running;
+ const originalEnv = this.system.environment;
+ this.system.setEnvironment(environment === 'development');
+ try {
+ const running = await this.process.isRunning(this.dir);
+ if (running) {
+ this._cliConfig.set('running', environment).save();
+ }
+ return running;
+ } finally {
+ this.system.setEnvironment(originalEnv === 'development');
+ }🤖 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 `@lib/instance.js` around lines 210 - 222, The environment is not restored if
this.process.isRunning(this.dir) throws in envIsRunning; refactor env switch to:
capture originalEnv, set the desired environment, declare let running = false,
then call running = await this.process.isRunning(this.dir) inside a try block
(and inside the try update this._cliConfig.set('running', environment).save()
when running is truthy), and in a finally block always call
this.system.setEnvironment(originalEnv === 'development') to restore global
state before returning running.
Comment on lines
+395
to
398
| mode: this._cliConfig.get('running') || this.system.environment, | ||
| url: this.config.get('url'), | ||
| port: this.config.get('server.port'), | ||
| process: this.process.name |
There was a problem hiding this comment.
Keep summary() environment-consistent for all returned fields.
Line 395 fixes mode, but url/port/process still depend on mutable global environment and can mismatch under concurrent ghost ls calls. Re-load the tracked running environment immediately before reading those fields.
Proposed fix
if (!running) {
return {
name: this.name,
dir: this.dir.replace(os.homedir(), '~'),
version: this.version,
running: false
};
}
+ this.loadRunningEnvironment();
+
return {
name: this.name,
dir: this.dir.replace(os.homedir(), '~'),
running: true,
version: this.version,
mode: this._cliConfig.get('running') || this.system.environment,
url: this.config.get('url'),
port: this.config.get('server.port'),
process: this.process.name
};🤖 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 `@lib/instance.js` around lines 395 - 398, The summary() method currently mixes
environments: it sets mode from this._cliConfig.get('running') ||
this.system.environment but then reads url/port/process from the mutable global
config, which can cause inconsistent results under concurrent "ghost ls" calls;
fix by capturing the tracked environment into a local variable (e.g. const
trackedEnv = this._cliConfig.get('running') || this.system.environment)
immediately before reading the other fields, and then use that trackedEnv to
obtain environment-scoped values for url, port and process (either via an
environment-specific config accessor or by reloading the config for trackedEnv)
so all returned fields (mode, url, port, process) are derived from the same
environment inside summary().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Issue:
In environments managing multiple Ghost instances, the
ghost lscommand misreports the environment mode of running production instances if a preceding instance in the registry is stopped.Root Cause Analysis:
isRunning()check for a stopped site, theenvIsRunninghelper is called to probe for a development process. This callsthis.system.setEnvironment(true), which globally mutates the singletonSystemobject without restoring it.ghost lsevaluates all instances concurrently usingPromise.all(), a stopped site can temporarily flip the globalthis.system.environmentvariable in the exact microsecond a running site evaluates itssummary().The Fix:
envIsRunninginlib/instance.jsto save and restore the global environment state before and after the probe.summary()inlib/instance.jsto prioritize reading the local instance configuration (this._cliConfig.get('running')) rather than relying exclusively on the thread-unsafe globalthis.system.environment.