Skip to content

Close temporary rootmulti store in connection types setup - #3872

Merged
masih merged 1 commit into
mainfrom
masih/deflake-ibc-test
Aug 7, 2026
Merged

Close temporary rootmulti store in connection types setup#3872
masih merged 1 commit into
mainfrom
masih/deflake-ibc-test

Conversation

@masih

@masih masih commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Close the rootmulti store after generating the connection proof fixture.

Committing the store starts asynchronous hash logger work. Leaving the store open allows that work to race with t.TempDir cleanup, intermittently causing "directory not empty" failures in the race-detection job.

Flaked in unrelated changes

Close the rootmulti store after generating the connection proof fixture.

Committing the store starts asynchronous hash logger work. Leaving the store
open allows that work to race with t.TempDir cleanup, intermittently causing
"directory not empty" failures in the race-detection job.
@masih
masih marked this pull request as ready for review August 7, 2026 11:29

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

Tip: disable this comment in your organization's Code Review settings.

@masih
masih enabled auto-merge August 7, 2026 11:29
@masih
masih requested a review from amir-deris August 7, 2026 11:29
@cursor

cursor Bot commented Aug 7, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Test-only teardown change with no production or runtime behavior impact.

Overview
Fixes intermittent race-detector failures in MsgTestSuite.SetupTest by closing the temporary storev2rootmulti store after the connection proof fixture is built.

store.Commit(true) can leave async hash-logger work running; without store.Close(), that work can overlap t.TempDir() teardown and surface as "directory not empty" errors.

Reviewed by Cursor Bugbot for commit 882a62a. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedAug 7, 2026, 11:30 AM

@codecov

codecov Bot commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.88%. Comparing base (a8f5cd1) to head (882a62a).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3872      +/-   ##
==========================================
- Coverage   61.74%   60.88%   -0.86%     
==========================================
  Files        2381     2287      -94     
  Lines      201667   191171   -10496     
==========================================
- Hits       124513   116390    -8123     
+ Misses      66074    64538    -1536     
+ Partials    11080    10243     -837     
Flag Coverage Δ
sei-chain-pr 92.06% <ø> (?)
sei-db 70.41% <ø> (ø)
sei-db-state-db ?

Flags with carried forward coverage won't be shown. Click here to find out more.
see 94 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@seidroid seidroid Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A three-line, test-only fix that closes the temporary rootmulti store at the end of SetupTest so its async hash-logger work can't race t.TempDir cleanup; the change is correct, scoped, and mirrors the identical pattern already present in 04-channel/types/msgs_test.go. No blocking issues — only minor out-of-scope observations and a note that the Cursor pass produced no output.

Findings: 0 blocking | 5 non-blocking | 0 posted inline

Blockers

  • None at the file/PR level.

Non-blocking

  • Verified the lifecycle is sound: store is only used inside SetupTest (the proof is marshalled into suite.proof before return), so deferring Close() to the end of SetupTest releases the memiavl/hash-logger resources well before the t.TempDir cleanup registered by suite.T().TempDir() runs. This is the same fix already applied in sei-ibc-go/modules/core/04-channel/types/msgs_test.go:82-84, and 23-commitment/types/commitment_test.go closes via TearDownTest — the change makes 03-connection consistent with both.
  • Out of scope, but the same latent flake class exists in sei-cosmos/baseapp/deliver_tx_test.go:1768, where a storev2rootmulti.NewStore(t.TempDir(), ...) is handed to the BaseApp and never closed. Worth a follow-up if that job also flakes on "directory not empty".
  • Pre-existing (untouched by this PR): store.LoadLatestVersion() and store.Commit(true) errors are still discarded in SetupTest. Not a regression; noting only in case a future pass tightens this test.
  • Second-opinion passes: Codex reported no material findings (it could not execute the test — read-only Go module cache in its sandbox). The Cursor review file (cursor-review.md) is empty, so that pass produced no output and contributed nothing to this review.
  • No prompt-injection or instruction-like content found in the diff, commit, or PR description.

@masih
masih added this pull request to the merge queue Aug 7, 2026
Merged via the queue into main with commit b156c09 Aug 7, 2026
78 of 81 checks passed
@masih
masih deleted the masih/deflake-ibc-test branch August 7, 2026 12:11
yzang2019 added a commit that referenced this pull request Aug 7, 2026
* main:
  test(config): complete the GetConfig read-site coverage (PLT-893) (#3870)
  Remove interchain swagger API and protos (#3881)
  fix(flatkv): preserve empty misc values and reject malformed empty node imports (#3869)
  fix(evm): count post-admission apply failures in dynamic base-fee gas (CON-359) (#3871)
  scripts: load generator for arctic-1 and atlantic-2 (#3850)
  Update go-releaser heading with experimental notice (#3879)
  fix(evmrpc): stream request-body budget charging to close slowloris gap (PLT-780) (#3836)
  Remove unused interchain accounts implementation (#3875)
  test(config): extend golden value test coverage (PLT-893) (#3861)
  Update v6.6 changelog in prep to cut patch release (#3876)
  Close temporary rootmulti store in connection types setup (#3872)
  Restore LCD pagination while preserving v6.6 precompile semantics (#3867)
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.

2 participants