Fix port replacement on creating new site from backup #944

nightnei · 2025-02-14T17:35:23Z

Related issues

Fixes Search and replace doesn't work correctly during the import #939

Proposed Changes

If we create a new site with the help of backup archive, then it works under the hood as - we create a new website and then import backup to created website. During importing, we need to replace website URL with local URL (e.g. localhost:8001), but we set up port only during running a website, so it ends up with localhost:undefined.
With proposed changes in this PR we are chisong port for website during creation of it, instead of during first running.

Testing Instructions

Apply the next diff:

diff --git a/src/lib/import-export/import/importers/importer.ts b/src/lib/import-export/import/importers/importer.ts
index b82ec5d..f7a0ff0 100644
--- a/src/lib/import-export/import/importers/importer.ts
+++ b/src/lib/import-export/import/importers/importer.ts
@@ -102,6 +102,7 @@ abstract class BaseImporter extends EventEmitter implements Importer {
                const oldUrlVariants = [ `http://${ urlWithoutProtocol }`, `https://${ urlWithoutProtocol }` ];
 
                for ( const urlToReplace of oldUrlVariants ) {
+                       console.info( `Replacing URL: ${ urlToReplace } -> ${ studioUrl }` );
                        const { stderr, exitCode } = await server.executeWpCliCommand(
                                `search-replace '${ urlToReplace }' '${ studioUrl }'`,
                                { skipPluginsAndThemes: true }

Open Studio
Create a new website with teh help of backup
Assert that in terminal logs (from the diff above) you see a correct port, instead of undefined

nightnei · 2025-02-14T18:20:27Z

src/ipc-handlers.ts

 		}
 	}

+	const port = await portFinder.getOpenPort();


We are running website immidiatelly after creating it, so it's tiotale safe to define the port during setting up website. And I would say, there are even benefit of such approach, since we configure everything for our site in advance and have complete data for it and I don't see any benefit in postponning choosing port for it.

Now that the port is assigned in site creation, should we remove tha code from SiteServer.start ?

studio/src/site-server.ts

Lines 81 to 82 in 00db122

const port = await portFinder.getOpenPort( this.details.port );

portFinder.addUnavailablePort( this.details.port );

~~Maybe we can also remove the code related to release port:~~

studio/src/site-server.ts

Line 74 in 00db122

portFinder.releasePort( this.details.port );

I was thinking about it, but:

What in 1 months the users starts their website again, but another tool uses already this port too. Then in this case, portFinder.getOpenPort( this.details.port ); fixes the issue and will pick up the next available port. I thought that it way intentionally added. If not intentionally, then anyway we should keep it to cover such case :)

Actually, reading the code, I have assumption that we have another bug, connected to the point above. And it's edge case, but I was going to emulate case from the previous point and check how Studio handles it, since I think that we should do some thing like replacUrl in DB if port is busy during starting site and if await portFinder.getOpenPort( this.details.port ); selected another one port. So in this case we need to update DB and update appdata-v1.json (but this point is already hanled inside start function, so we need to updater just DB).

But this my assumption is connected, but another topic, so in this PR I want to fix described issues and later cover that edge case as a separate PR, to not mix up everything.

WDYT about my thoughts?

@sejas btw, I have also assumption that this issue is related to what I am mentioning above.

I will investigate it, but off the top of my head, the issue here is:

We created website and set some port for urls inside DB

Then we removed site and created a new one with the same DB

As a result, data in the DB has old port, but we assigned a new one in appdata-v1.json during creating a site

What I want to say is - maybe, during starting server, we should always take port from const port = await portFinder.getOpenPort( this.details.port ); and make updated to DB? So that, I suppose, we will fix this issue and also we will fix edge case mentioned by me in the previous message.

Thinking about it more, I believe that we should do complex fix, like I proposed above or something similar, to keep Studio more stable.

My concern is that in this PR we are adding another portFinder.getOpenPort call and now we are setting the port to the studio site in two places, and it's not clear to me which one actually is setting the final port. That's why I was suggesting to address it in this PR.

I understand the concern about a port being already busy, but currently the WordPress site will redirect to that port producing an unexpected behaviour. So I don't see harm on removing that code.

We could create an issue to consider that edge case and "replacing" the url+port in the database and appdata. We don't need to try to fix that in this PR. Maybe the issue is an edge case that it's not worth implementing it and we could document it somewhere, or wait to see if any user reports it. There are other solutions, like displaying an alert if the port is busy.

I see, ok, let's go this way. I will create issues and I removed this:

const port = await portFinder.getOpenPort( this.details.port ); portFinder.addUnavailablePort( this.details.port );

But I think it's nice to keep portFinder.releasePort( this.details.port );, however it's not critical. The reason is - if we remove a site (e.g. :8002) and creating a new site, then willt be created 8003, instead of 8002 (which is actually free, and there is no sense to skip it).
WDYT, should we remove releasePort? I don't have strong preference, both options are ok - I like removing code but also I like when ports are used sequentially instead of jumping, so there are pros/cons in both cases :)

I checked my assumption and it indeed fails and we don't show any error in UI, just endless "Starting..." copy.
Created an issue.

But I think it's nice to keep portFinder.releasePort( this.details.port );, however it's not critical. The reason is - if we remove a site (e.g. :8002) and creating a new site, then willt be created 8003, instead of 8002 (which is actually free, and there is no sense to skip it).
WDYT, should we remove releasePort? I don't have strong preference, both options are ok - I like removing code but also I like when ports are used sequentially instead of jumping, so there are pros/cons in both cases :)

I didn't see the code or tested this case but I also think the port assignment should be sequential. Maybe we should release the port when the site is removed ? I thought that if it's not in appData then it could be free to use.

Ok!, It seems the releaseport is already in delete and not in stop as I thought. It's good to leave it 👌

studio/src/site-server.ts

Line 74 in 9b31ef9

portFinder.releasePort( this.details.port );

nightnei · 2025-02-17T17:15:32Z

src/hooks/use-add-site.ts

 						} );
 						clearImportState( newSite.id );
 					}
-					await startServer( newSite.id );


It's redundant, since we are starting server in the previous step above https://github.com/Automattic/studio/blob/trunk/src/hooks/use-import-export.tsx#L134

wojtekn

Works as expected.

wojtekn · 2025-02-18T08:47:40Z

src/hooks/use-site-details.tsx

 						id: tempSiteId,
 						name: siteName || path,
 						path,
+						port: 9999, // Set a temporary port


Why we need to set temporary port?

It's used as a placeholder to comply with the types.
This site placeholder is added so that Studio can display the new site in the sidebar while it's being created. The site information will be replaced once the site is created when the call to const data = await getIpcApi().createSite(path, siteName); resolves.

We could split getIpcApi().createSite into two functions: one that retrieves the siteId and new port, add the site to the state in the react, and then pass those variables as parameters to the createSite function.

If we decide to use the temporary port, we could use a more different port like -1 that could help us discover any issues during site creation.

@sejas Updated to -1, makes sense, good idea 👍

sejas

Looks great. I confirm I was able to import a new studio site from a backup and the site started correctly with a new port following the next sequence.

I left a comment suggesting to remove the portFinder code from the start and stop functions.

My reasoning is that now that we don't override the WordPress siteurl, if we start a site in a port different to what's saved in sitedetails, then it will redirect to that URL producing unexpected behaviours.

sejas · 2025-02-18T11:01:32Z

src/hooks/use-site-details.tsx

 						id: tempSiteId,
 						name: siteName || path,
 						path,
+						port: 9999, // Set a temporary port


It's used as a placeholder to comply with the types.
This site placeholder is added so that Studio can display the new site in the sidebar while it's being created. The site information will be replaced once the site is created when the call to const data = await getIpcApi().createSite(path, siteName); resolves.

We could split getIpcApi().createSite into two functions: one that retrieves the siteId and new port, add the site to the state in the react, and then pass those variables as parameters to the createSite function.

If we decide to use the temporary port, we could use a more different port like -1 that could help us discover any issues during site creation.

sejas · 2025-02-18T11:09:55Z

src/ipc-handlers.ts

 		}
 	}

+	const port = await portFinder.getOpenPort();


Now that the port is assigned in site creation, should we remove tha code from SiteServer.start ?

studio/src/site-server.ts

Lines 81 to 82 in 00db122

const port = await portFinder.getOpenPort( this.details.port );

portFinder.addUnavailablePort( this.details.port );

~~Maybe we can also remove the code related to release port:~~

studio/src/site-server.ts

Line 74 in 00db122

portFinder.releasePort( this.details.port );

sejas

Thanks for the changes! I tested it and worked as expected 👍
Thanks for creating the other issue.

nightnei added 4 commits February 14, 2025 17:34

Fix port replacement on creating new site from backup

b1e27c9

remove console

664e920

chore port

3488884

revert

40ecb84

nightnei commented Feb 14, 2025

View reviewed changes

nightnei added 4 commits February 17, 2025 11:45

test: fix

acc93cb

test: fix

6fc2074

test: fix

bb017a5

redund redundant starting server

da18d9d

nightnei commented Feb 17, 2025

View reviewed changes

nightnei linked an issue Feb 17, 2025 that may be closed by this pull request

Search and replace doesn't work correctly during the import #939

Closed

nightnei marked this pull request as ready for review February 17, 2025 17:23

fix: starting server

00db122

wojtekn reviewed Feb 18, 2025

View reviewed changes

wojtekn requested a review from a team February 18, 2025 08:53

sejas reviewed Feb 18, 2025

View reviewed changes

nightnei added 2 commits February 18, 2025 12:31

change default port

f6ee07c

chore: remove unused code

c8b8a6a

sejas approved these changes Feb 18, 2025

View reviewed changes

nightnei merged commit abd53d2 into trunk Feb 19, 2025
7 checks passed

nightnei deleted the fix/portRaplacementOnCreatingNewSiteFromBackup branch February 19, 2025 12:07

nightnei mentioned this pull request Feb 20, 2025

Update urls in DB if site is being created from existing folder #962

Merged

	const port = await portFinder.getOpenPort( this.details.port );
	portFinder.addUnavailablePort( this.details.port );

Fix port replacement on creating new site from backup #944

Fix port replacement on creating new site from backup #944

Uh oh!

Conversation

nightnei commented Feb 14, 2025 • edited by wojtekn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related issues

Proposed Changes

Testing Instructions

Uh oh!

nightnei Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sejas Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nightnei Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sejas Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nightnei Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

nightnei Feb 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wojtekn 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!

sejas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sejas Feb 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sejas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

nightnei commented Feb 14, 2025 •

edited by wojtekn

Loading

nightnei Feb 14, 2025 •

edited

Loading

sejas Feb 18, 2025 •

edited

Loading

nightnei Feb 18, 2025 •

edited

Loading

sejas Feb 18, 2025 •

edited

Loading

nightnei Feb 18, 2025 •

edited

Loading

nightnei Feb 17, 2025 •

edited

Loading

sejas Feb 18, 2025 •

edited

Loading

sejas left a comment •

edited

Loading