V0.21.5.rc - temp dir cache bug resolution, ASA-2025-004 patch #273
Conversation
📝 WalkthroughWalkthroughThis pull request removes the Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant RootCmd as NewRootCmd
participant TempApp as tempApp
participant TempDir as TemporaryDirectory
participant DataDir as "Data Directory"
App->>RootCmd: Start
RootCmd->>TempApp: Defer closure
TempApp-->>RootCmd: Return (ok/error)
RootCmd->>TempDir: Check if not default
RootCmd->>TempDir: Remove directory if condition met
RootCmd->>DataDir: Check existence and emptiness
DataDir-->>RootCmd: Return status
RootCmd->>DataDir: Remove if empty
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| if err := tempApp.Close(); err != nil { | ||
| panic(err) | ||
| } | ||
| if tempDir != bitsong.DefaultNodeHome { |
There was a problem hiding this comment.
I think that the user must manually delete the /tmp folder
There was a problem hiding this comment.
a temp file is created during each invocation of the cli root process.
![coderabbitai[bot]](/~https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cmd/bitsongd/cmd/root.go (3)
80-83: Avoid panicking when closing the application.
Panic might be too disruptive for a CLI application. Consider handling the error more gracefully, for instance by logging a warning or returning an error instead.Example adjustment:
defer func() { - if err := tempApp.Close(); err != nil { - panic(err) - } + if closeErr := tempApp.Close(); closeErr != nil { + fmt.Fprintf(os.Stderr, "Warning: error closing tempApp: %v\n", closeErr) + }
84-86: Handle the os.RemoveAll error.
Currently, the removal error is silently ignored. If removal fails unexpectedly, logging or returning an error could facilitate debugging.Example improvement:
if tempDir != bitsong.DefaultNodeHome { - os.RemoveAll(tempDir) + if err := os.RemoveAll(tempDir); err != nil { + fmt.Fprintf(os.Stderr, "Warning: failed to remove tempDir: %v\n", err) + } }
95-107: Guard against panics during directory cleanup.
Panic is called on any error, which might hamper UX. Logging or error propagation would allow CLI commands to fail gracefully.Example adjustment:
dirEntries, err := os.ReadDir(dataDir) if err != nil { - panic(err) + fmt.Fprintf(os.Stderr, "Warning: could not read directory %s: %v\n", dataDir, err) + return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
app/app.go(0 hunks)app/keepers/keepers.go(0 hunks)cmd/bitsongd/cmd/root.go(1 hunks)go.mod(1 hunks)
💤 Files with no reviewable changes (2)
- app/app.go
- app/keepers/keepers.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: e2e-tests (e2e-pfm)
- GitHub Check: e2e-tests (e2e-basic)
🔇 Additional comments (4)
go.mod (2)
27-27: Dependency Version Update for IBC Hooks ModuleThe version for
github.com/cosmos/ibc-apps/modules/ibc-hooks/v8has been updated to a newer pre-release (v8.0.0-20250226172931-56b9c5e4400a). Verify that this upgrade introduces no breaking changes and that all dependent functionalities (especially those related to inter-blockchain communication) remain compatible.
30-30: Dependency Version Update for IBC-GoThe dependency
github.com/cosmos/ibc-go/v8was updated from versionv8.5.3tov8.6.0, reflecting an upgrade to a stable release. Please confirm that this update has been tested against the application's IBC-related workflows and that no API changes adversely affect your code.cmd/bitsongd/cmd/root.go (2)
78-79: No concerns here.
These lines only introduce comments explaining the cleanup logic, and they are clear and informative.
88-94: Verify assumptions when building the data directory path.
Relying onos.Getwd()at runtime for cleanup can introduce edge cases if the working directory changes. Consider storing the initial working directory or allowing a configurable path for more robust handling.
resolves issue nodes experience causing recursive tempdir creation , as well as patch for ASA-2025-004
Summary by CodeRabbit
cosmossdk.io/mathandgithub.com/cosmos/ibc-go/v8.