Block conflicting site operations by bcotrim · Pull Request #4406 · Automattic/studio

bcotrim · 2026-07-31T12:40:00Z

Related issues

STU-2220

How AI was used in this PR

Claude Code handled the implementation. I designed the solution, reviewed and iterated over the code, and manually tested.

Proposed Changes

Two Studio operations on the same site could run at once and corrupt each other. Starting a site while it was stopping left it down while the UI claimed it started, then looped retrying. Because the agent calls the CLI directly, guarding this in the UI alone wouldn't hold.

The CLI now records the in-flight operation on the site and refuses a conflicting one with a readable error — so the guard applies no matter who started the work: you, the agent, a terminal, or a second window. One at a time: start, stop, delete and settings changes each own the site's server process, and duplicate copies the whole site directory. The operation belongs to the process holding it, so a crash or force quit releases it and no site can be left stuck.

Sync is deliberately out of scope: export and push never stop the server, so blocking a start during one would be a regression, and import and pull hold the site far too long to freeze it wholesale. Narrowing those to just their local write window is a follow-up.

The operation reaches the UI as its own kind of event, deliberately not the general "site updated" one — that asserts whether the site is running, and consumers treat it as authoritative. The UI disables the actions it blocks and names what's running ("Saving settings…") instead of leaving controls that look live but swallow the click. Progress survives navigating away and back.

Testing Instructions

Start a slow settings change: studio config set --wp 6.4 --path <site> — it runs wp core update, so it holds the site for a while.
While it runs, try to start / stop / delete / duplicate / change settings on that site — from the sidebar, the site dropdown and the overview screen, and by asking the agent. Controls are disabled and read "Saving settings…"; the agent gets a readable refusal.
Navigate away from the site and back — it still shows as saving.
Let it finish — everything re-enables.
Repeat, but kill -9 the CLI mid-run. The site must not stay locked.
Confirm studio site list is never blocked, and that export / import / pull / push still run while the site is busy.

Verified in light and dark mode.

Pre-merge Checklist

Have you checked for TypeScript, React or other console errors?

🤖 Generated with Claude Code

wpmobilebot · 2026-07-31T12:55:36Z

📊 Performance Test Results

Comparing f3daa1b vs trunk

app-size

Metric	trunk	`f3daa1b`	Diff	Change
App Size (Mac)	1406.62 MB	1406.64 MB	+0.02 MB	⚪ 0.0%

site-editor

Metric	trunk	`f3daa1b`	Diff	Change
load	1047 ms	1047 ms	0 ms	⚪ 0.0%

site-startup

Metric	trunk	`f3daa1b`	Diff	Change
siteCreation	7551 ms	7526 ms	25 ms	⚪ 0.0%
siteStartup	2881 ms	2871 ms	10 ms	⚪ 0.0%

Results are median values from multiple test runs.

Legend: 🟢 Improvement (faster) | 🔴 Regression (slower) | ⚪ No change (<50ms diff)

…le-stopping

…le-stopping # Conflicts: # apps/ui/src/components/site-dropdown/main-view.tsx # apps/ui/src/components/site-list/index.tsx # apps/ui/src/components/site-overview-view/index.tsx

Removes the two SITE_EVENTS.UPDATED emits withSiteLock fires on acquire and release, and widens the metrics artifact glob so a failure uploads Playwright's error-context.md. The emits are the only behavioural difference this branch makes to the desktop: they triple the running-state broadcasts per site start/stop, because _events recomputes `running` for each one and the desktop treats that as authoritative. site-startup fails on iteration 3 of 5 every run, which fits accumulation. They are also the signal the UI uses to show "Exporting…", so if this turns the job green they get reinstated as a lease-specific event that carries no running verdict. Both changes revert before merge.

… into guard-site-start-while-stopping

Restores the UI signal the diagnostic commit removed, as its own event kind. Reusing SITE_EVENTS.UPDATED made every lease change assert whether the site was running. Both consumers treat that as authoritative: the main process overwrites SiteServer.details.running and runs the start/stop side effects off the transition, and the renderer merges the flag and clears its per-site loading state. So claiming a lease mid-stop reported the site as still running and cleared the loading flag before the stop had begun, which is what failed the site-startup performance metric on the third cycle. SITE_EVENTS.OPERATIONS_CHANGED carries only the lease set. Both consumers apply `operations` and leave running and loading alone.

fredrikekelund

Do I understand it correctly that we aren't really distinguishing between different types of locks? You say as much in your description: for example, I cannot start or stop a site while exporting it.

I can see how pairing all the different possible locks could easily become arduous, but not permitting users to start the site while an export is happening seems like a functional regression.

Maybe we should scale this back and lock only site start and stop operations? We know that users are running into problems with those specific operations (judging by the related issues in STU-2220), and it makes perfect sense that those operations should not conflict. Maybe add site deletion to that list, too.

It's not immediately clear to me that exporting/importing, pulling/pushing, and listing sites would cause similar conflicts. It's true that WP-CLI operations execute in-process for Playground sites, so stopping the site in the midst of an export could indeed mess with the export operation, but I still hesitate to introduce such a sweeping change to prevent that specific issue…

fredrikekelund · 2026-08-11T07:41:22Z

+		// Must override what `...site` carries, not just add to it: the stored
+		// array can hold leases from crashed processes, and reporting those
+		// would leave clients disabling the site forever. `undefined` drops the
+		// key on serialization, so an idle site reports no `operations` at all.


There's something about how Claude and Codex phrase comments that oftentimes just doesn't make things more understandable… I understand the point here after reading it a few times, but it's almost like this comment mystifies what's actually quite straightforward if I just read the code and see that we filter the operations array through getLiveSiteOperations() and return undefined if length === 0.

"reporting those would leave clients disabling the site forever" -> is this actually true? Is it Studio Code that reads the list of sites in this way?

fredrikekelund · 2026-08-11T08:06:56Z

We're mixing terms between "lock", "lease", and "operations" here.

I would argue that "lease" is the most ambiguous term. It'd be nice to standardize the other two.

bcotrim · 2026-08-11T09:48:12Z

Do I understand it correctly that we aren't really distinguishing between different types of locks? You say as much in your description: for example, I cannot start or stop a site while exporting it.

I can see how pairing all the different possible locks could easily become arduous, but not permitting users to start the site while an export is happening seems like a functional regression.

Maybe we should scale this back and lock only site start and stop operations? We know that users are running into problems with those specific operations (judging by the related issues in STU-2220), and it makes perfect sense that those operations should not conflict. Maybe add site deletion to that list, too.

It's not immediately clear to me that exporting/importing, pulling/pushing, and listing sites would cause similar conflicts. It's true that WP-CLI operations execute in-process for Playground sites, so stopping the site in the midst of an export could indeed mess with the export operation, but I still hesitate to introduce such a sweeping change to prevent that specific issue…

Agree we can drop export, it's the weakest case in the set, and blocking a start during one is a poor trade. It was in there because keepSqliteIntegrationUpdated runs on start, export, push and import, so export isn't read-only on the site even though the archive excludes those files. Same applies to push, so I'll drop that too unless you'd rather keep it.
Let me know if you agree and I'll remove it.

For the rest, I did explore action pairings, but I'd rather start strict than risk broken states. With the Agentic UI the agent and the user are both acting on a site, so concurrency goes from rare to routine.

Keep start/stop/delete/import/pull/duplicate/settings strict and relax specific pairs as we prove them safe?

PS: just to clarify from the first comment the site list command is never blocked.

…le-stopping # Conflicts: # apps/ui/src/components/site-list/index.tsx # apps/ui/src/components/site-overview-view/index.test.tsx # apps/ui/src/components/site-overview-view/index.tsx # apps/ui/src/hooks/use-site-management-actions.ts

…le-stopping # Conflicts: # apps/cli/commands/export.ts # apps/cli/commands/import.ts

…le-stopping # Conflicts: # apps/ui/src/components/site-dropdown/main-view.tsx

fredrikekelund · 2026-08-12T06:13:05Z

Keep start/stop/delete/import/pull/duplicate/settings strict and relax specific pairs as we prove them safe?

Let's think about this theoretically for a moment before landing this… We might not revisit this so soon after landing this PR, so better to go over the cases now.

Start, stop, and delete make sense.

Import and pull… I'd want to lock them against secondary import and pull operations (which would clearly break the site). Deleting a site while an import/pull is happening would obviously also mess things up. Stopping a site while import/pull is happening is harmless for native PHP but potentially harmful for Playground (because it might kill an ongoing WP-CLI command that's executing in-process). The thing that worries me the most here is that imports and pulls, especially, are slow. Holding a lock for 10 minutes seems very risky to me from a UX standpoint.

Duplicate seems harmless to me. It's just a copy+start operation. Am I missing something? The one potential risk is if a site is duplicated while an import/pull is happening, which could potentially leave the new site in a bad state.

Settings also seems mostly harmless because the writeFile() operation is atomic. It's true that if multiple clients change the site settings in quick succession, you might end up with different site settings than expected, but concurrency locks don't entirely protect from this, either (they do block if two studio set commands are fired at the exact same time, but not if there's a 15-second gap). It's possible that the site restart operation that happens at the end could interfere with regular start/stop operations. What's the risk here, as you see it, @bcotrim?

I'd be fine shipping start/stop/delete/settings, but I think we should clarify the need during import/pull/duplicate before landing this PR. Happy to jump on a huddle and discuss 👍

bcotrim · 2026-08-12T09:21:34Z

Let's think about this theoretically for a moment before landing this… We might not revisit this so soon after landing this PR, so better to go over the cases now.

Start, stop, and delete make sense.

Import and pull… I'd want to lock them against secondary import and pull operations (which would clearly break the site). Deleting a site while an import/pull is happening would obviously also mess things up. Stopping a site while import/pull is happening is harmless for native PHP but potentially harmful for Playground (because it might kill an ongoing WP-CLI command that's executing in-process). The thing that worries me the most here is that imports and pulls, especially, are slow. Holding a lock for 10 minutes seems very risky to me from a UX standpoint.

Let's think the other way around. If a user is doing an import/pull, what's the use case for working on the site while it's ongoing? Starting and stopping can have side effects, and the proposal is broad precisely because those are hard to enumerate — so we "freeze" the site as much as we can. The user or AI agent can still change files and records directly; I just want to prevent the invisible side-effects caused by Studio itself. As a user I'd rather wait 20–30 min for a pull than have a broken site for reasons I don't understand.

Duplicate seems harmless to me. It's just a copy+start operation. Am I missing something? The one potential risk is if a site is duplicated while an import/pull is happening, which could potentially leave the new site in a bad state.

Yes, that's the case I had in mind. It seems odd to allow a duplicate while another operation is adding or removing files. Worth noting it's free either way: no CLI command performs duplication, so it never holds anything — it's only the UI disabling the button.

Settings also seems mostly harmless because the writeFile() operation is atomic. It's true that if multiple clients change the site settings in quick succession, you might end up with different site settings than expected, but concurrency locks don't entirely protect from this, either (they do block if two studio set commands are fired at the exact same time, but not if there's a 15-second gap). It's possible that the site restart operation that happens at the end could interfere with regular start/stop operations. What's the risk here, as you see it, @bcotrim?

My motivation was the restart. Every setting except --name triggers one, so on a running site config set is a stop+start rather than just a config write. Updating the WordPress version runs a wp core update that takes a while and leaves things broken if interrupted. And mid-transition it's worse than a failure: wasRunning is checked once, so if the site is still starting we skip both the stop and the restart — the new settings land in config while the server keeps running with the old ones.

I'd be fine shipping start/stop/delete/settings, but I think we should clarify the need during import/pull/duplicate before landing this PR. Happy to jump on a huddle and discuss 👍

I think we should land this with start/stop/delete/settings (+duplicate, which costs nothing) and pull/import. In a follow-up we can explore not freezing the site during the remote part of the operation, but that needs care: if the lock is only taken for the import phase, a busy site at that moment would fail the pull and throw away 30 minutes of work. That seems complex enough to deserve its own PR — so let's start conservative and reduce the lock later.

Push and export I'll drop — neither ever stops the site, which is exactly why blocking a start during them was the regression you flagged.

fredrikekelund

@bcotrim and I huddled to continue our discussion in this PR. We concluded that a reasonable way of shipping this was to remove the locking functionality for pull and import operations (as those locks would be held for so long that it could have a detrimental UX impact).

I've tested the changes in this PR briefly and can confirm they work. They also interact nicely with the agentic UI 👍

Approving this with some suggestions (primarily around potential code simplifications in the UI) ahead of Bernardo's changes that will remove locking functionality for pulling and import commands.

fredrikekelund · 2026-08-12T12:42:51Z

+		site,
+		isStarting,
+		isStopping,
+		useSiteOperation( site )


Calling a React hook as an argument instead of assigning it to a variable has a weird smell, even if it doesn't matter from a functional perspective

fredrikekelund · 2026-08-12T12:45:52Z

+	const isLocalTransitioning = useIsSiteBusy( site );
+	const operation = useSiteOperation( site );


Two things:

Doesn't useSiteOperation tell us the exact same thing as useIsSiteBusy?

If they do tell us different things, can we rename isLocalTransitioning? Maybe isCliOperationInProgress instead

fredrikekelund · 2026-08-12T12:49:05Z

+export function getSiteStatusName( {
+	running,
+	starting,
+	stopping,
+	operation,
+}: {
+	running: boolean;
+	starting: boolean;
+	stopping: boolean;
+	operation: SiteOperationKind | null;
+} ): string {


Similarly, doesn't operation already tell us if the site is stopping or starting?

fredrikekelund · 2026-08-12T12:49:44Z

 function LocalServerControl( {
 	running,
 	starting,
 	stopping,
+	operation,
 	disabled,
 	onStart,
 	onStop,
 }: {
 	running: boolean;
 	starting: boolean;
 	stopping: boolean;
+	// A CLI operation (an agent export, another window's import). Blocks the toggle
+	// and names itself in the tooltip, so a dead control explains why.
+	operation: SiteOperationKind | null;
 	disabled: boolean;
 	onStart: () => void;
 	onStop: () => void;
 } ) {


Doesn't operation already tell us if a site is stopping or starting? Do we need the starting and stopping props also?

fredrikekelund · 2026-08-12T12:52:18Z

 	const startSite = useStartSite();
 	const stopSite = useStopSite();
-	const { status } = deriveSiteStatus( site, isStarting, isStopping );
-	const busy = isStarting || isStopping;
-	const statusName =
-		status === 'running'
-			? __( 'Running' )
-			: status === 'transitioning'
-			? isStopping
-				? __( 'Stopping' )
-				: __( 'Starting' )
-			: __( 'Stopped' );
+	const busy = useIsSiteBusy( site );
+	const operation = useSiteOperation( site );
+	const { status } = deriveSiteStatus( site, isStarting, isStopping, operation );
+	// The recorded operation wins: it names work this window didn't start (an
+	// agent import, another Studio window) that local start/stop state can't see.
+	const statusName = getSiteStatusName( {
+		running: site.running,
+		starting: isStarting,
+		stopping: isStopping,
+		operation,
+	} );


Same theme: doesn't operation already tell us if the site is starting or stopping?

fredrikekelund · 2026-08-12T13:00:33Z

+	// Identifies which entry to clear on release, so a process can't drop a
+	// different operation that happens to share its PID.
+	id: z.string(),


Just thought I'd note that this appears redundant for now. I'm fine with keeping it in, but for now, a single process always holds a single lock, AFAICT.

…le-stopping

Don't start a site while a stop for it is still in flight

a57383e

github-actions Bot assigned bcotrim Jul 31, 2026

bcotrim marked this pull request as draft July 31, 2026 13:22

bcotrim changed the title ~~Don't start a site while a stop for it is still in flight~~ Agentic UI: Don't start a site while a stop for it is still in flight Jul 31, 2026

bcotrim mentioned this pull request Aug 7, 2026

Agentic UI: Add Open in actions to the site overview #4469

Merged

1 task

bcotrim added 3 commits August 7, 2026 10:56

Merge remote-tracking branch 'origin/trunk' into guard-site-start-whi…

3e609c6

…le-stopping

Serialize concurrent site operations behind a CLI-owned lease

31b5e90

Keep site operation progress visible across navigation

4712457

bcotrim changed the title ~~Agentic UI: Don't start a site while a stop for it is still in flight~~ Block conflicting site operations with a per-site lease Aug 7, 2026

Share one source for site management action state

5d8d689

bcotrim changed the title ~~Block conflicting site operations with a per-site lease~~ Block conflicting site operations Aug 7, 2026

bcotrim added 5 commits August 7, 2026 18:31

Merge remote-tracking branch 'origin/trunk' into guard-site-start-whi…

7d47734

…le-stopping # Conflicts: # apps/ui/src/components/site-dropdown/main-view.tsx # apps/ui/src/components/site-list/index.tsx # apps/ui/src/components/site-overview-view/index.tsx

Address review: surface why an action was refused, bound stale leases

8ee29d3

Reclaim site operation leases written before the current boot

d1ca661

Key site operation leases on the owning process alone

f941973

Hold one site operation at a time

8a1357a

bcotrim requested review from fredrikekelund and youknowriad August 7, 2026 18:53

bcotrim marked this pull request as ready for review August 7, 2026 18:53

bcotrim and others added 6 commits August 7, 2026 19:54

trigger ci

faa7bfa

Replace nested ternaries in site status derivation with named helpers

0280c80

Merge branch 'trunk' into guard-site-start-while-stopping

0a26f98

Merge branch 'trunk' into guard-site-start-while-stopping

27f327f

Merge remote-tracking branch 'origin/guard-site-start-while-stopping'…

6d4bb0c

… into guard-site-start-while-stopping

bcotrim requested a review from a team as a code owner August 10, 2026 14:37

fredrikekelund reviewed Aug 11, 2026

View reviewed changes

bcotrim mentioned this pull request Aug 11, 2026

Agentic UI: Add backup import to the site overview #4500

Merged

1 task

bcotrim added 2 commits August 11, 2026 10:50

Standardize site operation terminology and clarify comments

eca8729

bcotrim requested a review from fredrikekelund August 11, 2026 10:06

bcotrim added 2 commits August 11, 2026 15:12

Merge remote-tracking branch 'origin/trunk' into guard-site-start-whi…

3419d61

…le-stopping # Conflicts: # apps/cli/commands/export.ts # apps/cli/commands/import.ts

Merge remote-tracking branch 'origin/trunk' into guard-site-start-whi…

9bf8423

…le-stopping # Conflicts: # apps/ui/src/components/site-dropdown/main-view.tsx

fredrikekelund approved these changes Aug 12, 2026

View reviewed changes

bcotrim added 2 commits August 12, 2026 18:47

Merge remote-tracking branch 'origin/trunk' into guard-site-start-whi…

16fb848

…le-stopping

Narrow the site operation guard and store one operation per site

f3daa1b

bcotrim enabled auto-merge (squash) August 12, 2026 22:56

bcotrim merged commit 6b77634 into trunk Aug 12, 2026
14 checks passed

bcotrim deleted the guard-site-start-while-stopping branch August 12, 2026 22:57

		const isLocalTransitioning = useIsSiteBusy( site );
		const operation = useSiteOperation( site );

Uh oh!

Conversation

bcotrim commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issues

How AI was used in this PR

Proposed Changes

Testing Instructions

Pre-merge Checklist

Uh oh!

wpmobilebot commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Performance Test Results

app-size

site-editor

site-startup

Uh oh!

fredrikekelund left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bcotrim commented Aug 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredrikekelund commented Aug 12, 2026

Uh oh!

bcotrim commented Aug 12, 2026

Uh oh!

fredrikekelund left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bcotrim commented Jul 31, 2026 •

edited

Loading

wpmobilebot commented Jul 31, 2026 •

edited

Loading

bcotrim commented Aug 11, 2026 •

edited

Loading