NotificationsYou must be signed in to change notification settings
Fork1.1k
Star11.6k

fix: ensure embedded-postgres state is wiped between retries#20809

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

zedkipp merged 1 commit intomainfromzedkipp/testserver-retry-state

Nov 21, 2025

Merged

fix: ensure embedded-postgres state is wiped between retries#20809

zedkipp merged 1 commit intomainfromzedkipp/testserver-retry-state

Nov 21, 2025

Conversation

Copy link

Contributor

zedkipp commentedNov 17, 2025•
edited
Loading

Retries were previously added when starting embedded postgres to mitigate port allocation conflicts (we can't use an ephemeral port for tests). Retries alone seemingly did not fix the test flakes. A new failure mode appeared on the retries: timing out connecting to the database.

When a port discovery error occurrs, embedded-postgres does not create the database. If the data directory exists on the next attempt, embedded-postgres will assume the database has already been created. This seems to cause the timeout error. Wipe all state between retries to ensure attempts execute the same logic that creates the database.

#658

This isn't easily testable or reproducible. That said, I tested this on a Linux machine by removing thetesting.Testing() call that enables the retries, hard-coding the initial postgres port and usingnc to force a port conflict.

github-actionsbot assignedzedkipp

Nov 17, 2025

Copy link

Member

Emyrk commentedNov 17, 2025

Could this accidentally delete real postgres data?

Copy link

ContributorAuthor

zedkipp commentedNov 17, 2025

@Emyrk I don't think so because the retries are only attempted when running tests.

coder/cli/server.go

Lines 2148 to 2149 ina272843

	_,err=cfg.PostgresPort().Read()
	retryPortDiscovery:=errors.Is(err,os.ErrNotExist)&&testing.Testing()

zedkipp marked this pull request as ready for review

November 17, 2025 20:56

zedkipp requested review fromEmyrk andhugodutka

November 17, 2025 23:46

Copy link

Contributor

cstyan commentedNov 20, 2025

To Steven's question/your answer, I still think it would be a good idea to leave apotential footgun comment on the assignment of the value forretryPortDiscovery to make it clear that if the assignment logic is changed to not includetesting.Testing() then we should also update the conditional check on the new block you've added.

Alternatively we could preemptively guard against that case by checking fortesting.Testing() on your block as well, or maybe slightly more involved but refactor so that the retry function is definable on the server struct and define the server within tests to have a retry function that does the filesystem cleaning.

I think the former (comment or add the additional check fortesting.Testing()) is enough for now to see if that resolves the flakiness.

fix: ensure embedded-postgres state is wiped between retries

44948bc

Retries were previously added when starting embedded postgres to mitigate portallocation conflicts (we can't use an ephemeral port). Retries alone seeminglydid not fix the test flakes. A new failure mode appeared on the retries: timingout connecting to the database.When a port discovery error occurrs, embedded-postgres does not create thedatabase. If the data directory exists on the next attempt, embedded-postgreswill assume the database has already been created. This seems to cause thetimeout error. Wipe all state between retries to ensure attempts execute thesame logic that creates the database.

zedkipp force-pushed thezedkipp/testserver-retry-state branch from9e8ff44 to44948bcCompare

November 20, 2025 23:54

Copy link

ContributorAuthor

zedkipp commentedNov 20, 2025

Yeah, that's a really good point. I updated the comments to better document the intent and potential footgun to future folks that may change this code.