Skip to content

/go/{cmd,libraries} check for valid dbs, close opened dbs on error#10690

Merged
coffeegoddd merged 1 commit intomainfrom
db/fix-panic
Mar 16, 2026
Merged

/go/{cmd,libraries} check for valid dbs, close opened dbs on error#10690
coffeegoddd merged 1 commit intomainfrom
db/fix-panic

Conversation

@coffeegoddd
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds proper cleanup of DoltDB resources in the multi-repo environment setup. It tracks opened environments during MultiEnvForDirectory, closes them on error, and adds a Close() method to MultiRepoEnv for orderly shutdown. It also adds a defensive validity check in newDatabase before attempting to use the database.

Changes:

  • Track opened DoltEnvs during MultiEnvForDirectory and close their DoltDBs if the function fails partway through.
  • Add a Close() method on MultiRepoEnv to close all held DoltDB instances.
  • Add a validity check in newDatabase to return a clear error if the DoltEnv is not valid.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
go/libraries/doltcore/env/multi_repo_env.go Track opened envs for cleanup on error; add Close() method
go/cmd/dolt/commands/engine/utils.go Add validity check in newDatabase
go/libraries/doltcore/env/multi_repo_env_test.go Add test for MultiRepoEnv.Close()

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 56 to +59
dbdata := dEnv.DbData(ctx)
if !dEnv.Valid() {
return sqle.Database{}, fmt.Errorf("failed to load database %q: %w", name, errors.Join(dEnv.CfgLoadErr, dEnv.DBLoadError))
}
@coffeegoddd
Copy link
Copy Markdown
Contributor Author

goes with dolthub/driver#190 to fix #10687

@coffeegoddd coffeegoddd requested a review from reltuk March 16, 2026 18:29
@coffeegoddd
Copy link
Copy Markdown
Contributor Author

@coffeegoddd DOLT

read_tests from_latency to_latency percent_change
covering_index_scan 0.55 0.57 3.64
groupby_scan 9.91 9.91 0.0
index_join 1.82 1.82 0.0
index_join_scan 1.39 1.39 0.0
index_scan 21.89 21.89 0.0
oltp_point_select 0.27 0.27 0.0
oltp_read_only 5.28 5.28 0.0
select_random_points 0.53 0.53 0.0
select_random_ranges 0.55 0.55 0.0
table_scan 22.28 21.89 -1.75
types_table_scan 66.84 65.65 -1.78
write_tests from_latency to_latency percent_change
oltp_delete_insert 6.32 6.32 0.0
oltp_insert 3.13 3.13 0.0
oltp_read_write 11.24 11.24 0.0
oltp_update_index 3.19 3.19 0.0
oltp_update_non_index 3.13 3.13 0.0
oltp_write_only 5.99 5.99 0.0
types_delete_insert 6.79 6.79 0.0

@coffeegoddd
Copy link
Copy Markdown
Contributor Author

@coffeegoddd DOLT

comparing_percentages
100.000000 to 100.000000
version result total
c0f7ec8 ok 5937471
version total_tests
c0f7ec8 5937471
correctness_percentage
100.0

Copy link
Copy Markdown
Contributor

@reltuk reltuk left a comment

Choose a reason for hiding this comment

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

LGTM!

@coffeegoddd
Copy link
Copy Markdown
Contributor Author

@coffeegoddd DOLT

test_name from_latency_p95 to_latency_p95 percent_change
tpcc-scale-factor-1 61.08 61.08 0.0
test_name from_server_name from_server_version from_tps to_server_name to_server_version to_tps percent_change
tpcc-scale-factor-1 dolt 1bd70a3 37.87 dolt c0f7ec8 38.21 0.9

@coffeegoddd coffeegoddd merged commit 24e549c into main Mar 16, 2026
44 of 50 checks passed
@coffeegoddd coffeegoddd deleted the db/fix-panic branch March 16, 2026 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants