Skip to content

Fix port replacement on creating new site from backup #944

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

Merged
nightnei merged 11 commits into from
Feb 19, 2025
Merged

Fix port replacement on creating new site from backup #944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
b1e27c9
Fix port replacement on creating new site from backup
nightnei Feb 14, 2025
664e920
remove console
nightnei Feb 14, 2025
3488884
chore port
nightnei Feb 14, 2025
40ecb84
revert
nightnei Feb 14, 2025
acc93cb
test: fix
nightnei Feb 17, 2025
6fc2074
test: fix
nightnei Feb 17, 2025
bb017a5
test: fix
nightnei Feb 17, 2025
da18d9d
redund redundant starting server
nightnei Feb 17, 2025
00db122
fix: starting server
nightnei Feb 17, 2025
f6ee07c
change default port
nightnei Feb 18, 2025
c8b8a6a
chore: remove unused code
nightnei Feb 18, 2025
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/components/tests/assistant-code-block.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const selectedSite: SiteDetails = {
path: '/test-site',
phpVersion: '8.0',
adminPassword: btoa( 'test-password' ),
port: 9999,
};

( useSiteDetails as jest.Mock ).mockReturnValue( {
Expand Down
1 change: 1 addition & 0 deletions src/components/tests/content-tab-import-export.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const selectedSite: SiteDetails = {
path: '/test-site',
phpVersion: '8.0',
adminPassword: btoa( 'test-password' ),
port: 9999,
};

beforeEach( () => {
Expand Down
1 change: 1 addition & 0 deletions src/hooks/tests/use-import-export.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ const selectedSite: SiteDetails = {
path: '/test-site',
phpVersion: '8.0',
adminPassword: btoa( 'test-password' ),
port: 9999,
};

const wrapper = ( { children }: { children: React.ReactNode } ) => (
Expand Down
2 changes: 2 additions & 0 deletions src/hooks/tests/use-update-demo-site.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ describe( 'useUpdateDemoSite', () => {
phpVersion: '8.0',
id: '54321',
path: '/path/to/site',
port: 9999,
};
const clientReqPost = jest.fn().mockResolvedValue( {
data: 'success',
Expand Down Expand Up @@ -160,6 +161,7 @@ describe( 'useUpdateDemoSite', () => {
phpVersion: '8.0',
id: '09876',
path: '/path/to/site2',
port: 9999,
};

// Mock different response times for each site
Expand Down
5 changes: 4 additions & 1 deletion src/hooks/use-add-site.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,11 @@ export function useAddSite() {
isNewSite: true,
} );
clearImportState( newSite.id );
} else {
// when we import file we start the server automatically, so doesn't make sense to run it again, to avoid unexpected behaviour
await startServer( newSite.id );
}
await startServer( newSite.id );

@nightnei nightnei Feb 17, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

getIpcApi().showNotification( {
title: newSite.name,
body: __( 'Your new site is up and running' ),
Expand Down
1 change: 1 addition & 0 deletions src/hooks/use-site-details.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ export function SiteDetailsProvider( { children }: SiteDetailsProviderProps ) {
id: tempSiteId,
name: siteName || path,
path,
port: -1, // Set a temporary port
running: false,
isAddingSite: true,
phpVersion: '',
Expand Down
4 changes: 4 additions & 0 deletions src/ipc-handlers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import { StoredToken } from 'src/lib/oauth';
import * as oauthClient from 'src/lib/oauth';
import { createPassword } from 'src/lib/passwords';
import { phpGetThemeDetails } from 'src/lib/php-get-theme-details';
import { portFinder } from 'src/lib/port-finder';
import { shellOpenExternalWrapper } from 'src/lib/shell-open-external-wrapper';
import { sortSites } from 'src/lib/sort-sites';
import { installSqliteIntegration, keepSqliteIntegrationUpdated } from 'src/lib/sqlite-versions';
Expand Down Expand Up @@ -161,11 +162,14 @@ export async function createSite(
}
}

const port = await portFinder.getOpenPort();

@nightnei nightnei Feb 14, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@sejas sejas Feb 18, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 );

@nightnei nightnei Feb 18, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about it, but:

  1. 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 :)
  2. 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?

@nightnei nightnei Feb 18, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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:

  1. We created website and set some port for urls inside DB
  2. Then we removed site and created a new one with the same DB
  3. 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.

@sejas sejas Feb 18, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@nightnei nightnei Feb 18, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 :)

@nightnei nightnei Feb 18, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@sejas sejas Feb 18, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@sejas sejas Feb 18, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 );


const details = {
id: crypto.randomUUID(),
name: siteName || nodePath.basename( path ),
path,
adminPassword: createPassword(),
port,
running: false,
phpVersion: DEFAULT_PHP_VERSION,
} as const;
Expand Down
2 changes: 1 addition & 1 deletion src/ipc-types.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ interface StoppedSiteDetails {
id: string;
name: string;
path: string;
port?: number;
port: number;
phpVersion: string;
adminPassword?: string;
themeDetails?: {
Expand Down
1 change: 1 addition & 0 deletions src/lib/import-export/tests/export/exporters/default-exporter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ platformTestSuite( 'DefaultExporter', ( { normalize } ) => {
id: '123',
name: '123',
path: normalize( '/path/to/site' ),
port: 9999,
phpVersion: '7.4',
},
backupFile: normalize( '/path/to/backup.tar.gz' ),
Expand Down
1 change: 1 addition & 0 deletions src/lib/import-export/tests/export/exporters/sql-exporter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ platformTestSuite( 'SqlExporter', ( { normalize } ) => {
id: '123',
name: '123',
path: '/path/to/site',
port: 9999,
phpVersion: '7.4',
},
backupFile: normalize( '/path/to/backup.sql' ),
Expand Down
1 change: 1 addition & 0 deletions src/lib/import-export/tests/import/import-manager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ describe( 'importManager', () => {
id: '123',
name: 'Site Name',
path: '/path/to/site',
port: 9999,
phpVersion: '7.4',
running: false,
};
Expand Down
1 change: 1 addition & 0 deletions src/modules/preview-site/components/tests/preview-action-buttons-menu.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ describe( 'PreviewActionButtonsMenu Rename', () => {
name: 'Test Site',
path: '/test/path',
phpVersion: '8.0',
port: 9999,
running: false,
};
beforeEach( () => {
Expand Down
1 change: 1 addition & 0 deletions src/modules/preview-site/components/tests/preview-site-row.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ describe( 'PreviewSiteRow', () => {
name: 'Test',
path: '/test/path',
phpVersion: '8.2',
port: 9999,
running: false,
};

Expand Down
7 changes: 3 additions & 4 deletions src/site-server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,16 +78,15 @@ export class SiteServer {
if ( this.details.running || this.server ) {
return;
}
const port = await portFinder.getOpenPort( this.details.port );
portFinder.addUnavailablePort( this.details.port );

const options = await getWpNowConfig( {
path: this.details.path,
port,
port: this.details.port,
adminPassword: decodePassword( this.details.adminPassword ?? '' ),
siteTitle: this.details.name,
php: this.details.phpVersion,
} );
const absoluteUrl = `http://localhost:${ port }`;
const absoluteUrl = `http://localhost:${ this.details.port }`;
options.absoluteUrl = absoluteUrl;
options.siteLanguage = await getPreferredSiteLanguage( options.wordPressVersion );

Expand Down
7 changes: 7 additions & 0 deletions src/tests/ipc-handlers.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@ jest.mock( 'src/main-window' );
jest.mock( '@sentry/electron/main' );
jest.mock( 'src/lib/import-export/import/import-manager' );

jest.mock( 'src/lib/port-finder', () => ( {
portFinder: {
getOpenPort: jest.fn().mockResolvedValue( 9999 ),
},
} ) );

( SiteServer.create as jest.Mock ).mockImplementation( ( details ) => ( {
start: jest.fn(),
details,
Expand Down Expand Up @@ -65,6 +71,7 @@ describe( 'createSite', () => {
name: 'Test',
path: '/test',
phpVersion: '8.2',
port: 9999,
running: false,
} );
} );
Expand Down