Skip to content

Fix: prevent global environment pollution in multi-site ls check #2174

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.

Sign up for GitHub

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

Open
Dana-Johnson wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix: prevent global environment pollution in multi-site ls check #2174

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions lib/instance.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,20 +202,26 @@ class Instance {
if (!this._cliConfig.has('running')) {
const currentEnvironment = this.system.development;

const envIsRunning = async (environment) => {
const envIsRunning = async (environment) => {
if (!Config.exists(path.join(this.dir, `config.${environment}.json`))) {
return false;
}

// 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;
Comment on lines +210 to 222

@coderabbitai coderabbitai Bot May 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

};

const production = await envIsRunning('production');
if (production) {
return true;
Expand Down Expand Up @@ -386,7 +392,7 @@ class Instance {
dir: this.dir.replace(os.homedir(), '~'),
running: true,
version: this.version,
mode: this.system.environment,
mode: this._cliConfig.get('running') || this.system.environment,
url: this.config.get('url'),
port: this.config.get('server.port'),
process: this.process.name
Comment on lines +395 to 398

@coderabbitai coderabbitai Bot May 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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().

Expand Down