NotificationsYou must be signed in to change notification settings
Fork920
Star10.1k

chore: close db properly in early exit paths in ConnectToPostgres#18448

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

hugodutka merged 1 commit intomainfromhugodutka/connect-to-postgres-close-db

Jun 20, 2025

Merged

chore: close db properly in early exit paths in ConnectToPostgres#18448

hugodutka merged 1 commit intomainfromhugodutka/connect-to-postgres-close-db

Jun 20, 2025

+5 −8

Conversation

Copy link

Contributor

hugodutka commentedJun 19, 2025•
edited
Loading

There were some code paths where if we exited early from the function the postgres connection would never get cleaned up.

This is the mechanism that cleans up the db - it requires the err variable to be not nil:

coder/cli/server.go

Lines 2319 to 2328 in118bf98

	deferfunc() {
	iferr==nil {
	return
	}
	ifsqlDB!=nil {
	_=sqlDB.Close()
	sqlDB=nil
	}
	logger.Error(ctx,"connect to postgres failed",slog.Error(err))
	}()

github-actionsbot assignedhugodutka

Jun 19, 2025

hugodutka changed the title~~chore: close db properly on false version.Next in ConnectToPostgres~~chore: close db properly in early exit paths in ConnectToPostgres

Jun 19, 2025

hugodutka force-pushed thehugodutka/connect-to-postgres-close-db branch from471bd6d toe9f833bCompare

June 19, 2025 09:46

hugodutka marked this pull request as ready for review

June 19, 2025 10:36

hugodutka requested a review fromEmyrk

June 19, 2025 10:37

Emyrk reviewed

Jun 20, 2025

View reviewed changes

cli/server.go Outdated

Comment on lines 2366 to 2367

		err=xerrors.Errorf("no rows returned for version select: %w",version.Err())
		returnnil,err

Copy link

Member

EmyrkJun 20, 2025

Choose a reason for hiding this comment

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

Can we just name this error something else? LikedbError?

I say that because a few lines up isversion, err := sqlDB.QueryContext(ctx, "SHOW server_version_num;"). Does that not shadow theerr variable and break this assignment to the correcterr here?

Copy link

ContributorAuthor

hugodutkaJun 20, 2025•
edited
Loading

Choose a reason for hiding this comment

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

Based onthe language spec, this statement assigns to the correcterr:

Unlike regular variable declarations, a short variable declaration may redeclare variables provided they were originally declared earlier in the same block (or the parameter lists if the block is the function body) [...]Redeclaration does not introduce a new variable; it just assigns a new value to the original.

Either way, it's non-obvious and if somebody modified this function in the future, it's likely they would forget about the err assignment/close db logic. I added adbNeedsClosing variable and refactored thedefer statement to use it instead oferr. This way, the db will always be closed unless a code path explicitly opts out of it.