Skip to content

fix: replace emptyAppOptions with tmpAppOptions #110

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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
AdriaCarrera merged 1 commit into from
Feb 23, 2026
Merged

fix: replace emptyAppOptions with tmpAppOptions #110

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions cmd/exrpd/cmd/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,16 @@ import (
"github.com/xrplevm/node/v10/app"
)

type emptyAppOptions struct{}

func (ao emptyAppOptions) Get(_ string) interface{} { return nil }
type tmpAppOptions struct{}

func (ao tmpAppOptions) Get(searchFlag string) interface{} {
switch searchFlag {
case flags.FlagHome:
return tempDir(app.DefaultNodeHome)
default:
return nil
}
Comment on lines +53 to +61
Copy link

@coderabbitai coderabbitai bot Feb 23, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Stabilize FlagHome resolution to avoid multiple temp dirs.

Get(flags.FlagHome) may be called multiple times; returning a new temp dir each time can cause inconsistent home paths and temp-dir leaks. Cache the directory and switch to a pointer receiver.

♻️ Suggested memoization with `sync.Once` 
 import (
 	"io"
 	"os"
 	"path/filepath"
+	"sync"
@@
-type tmpAppOptions struct{}
+type tmpAppOptions struct {
+	once sync.Once
+	home string
+}
 
-func (ao tmpAppOptions) Get(searchFlag string) interface{} {
+func (ao *tmpAppOptions) Get(searchFlag string) interface{} {
 	switch searchFlag {
 	case flags.FlagHome:
-		return tempDir(app.DefaultNodeHome)
+		ao.once.Do(func() {
+			ao.home = tempDir(app.DefaultNodeHome)
+		})
+		return ao.home
 	default:
 		return nil
 	}
 }
-		tmpAppOptions{},
+		&tmpAppOptions{},

Also applies to: 74-74

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@cmd/exrpd/cmd/root.go` around lines 53 - 61, The tmpAppOptions.Get
implementation returns a new tempDir each call which causes inconsistent home
paths and leaks; change tmpAppOptions to hold a cached home string and a
sync.Once (e.g., fields cachedHome string and init sync.Once), switch Get to a
pointer receiver func (ao *tmpAppOptions) Get(...), and initialize cachedHome
once by calling tempDir(app.DefaultNodeHome) inside the Once.Do, then return
cachedHome for flags.FlagHome; ensure other callers use the pointer type so the
memoized value is reused.

}

// NewRootCmd creates a new root command for a Cosmos SDK application
func NewRootCmd() (*cobra.Command, sdktestutil.TestEncodingConfig) {
Expand All @@ -64,7 +71,7 @@ func NewRootCmd() (*cobra.Command, sdktestutil.TestEncodingConfig) {
dbm.NewMemDB(),
nil, true, nil,
0,
emptyAppOptions{},
tmpAppOptions{},
)
encodingConfig := sdktestutil.TestEncodingConfig{
InterfaceRegistry: tempApp.InterfaceRegistry(),
Expand Down Expand Up @@ -424,3 +431,13 @@ func getChainIDFromOpts(appOpts servertypes.AppOptions) (chainID string, err err

return
}

func tempDir(defaultHome string) string {
dir, err := os.MkdirTemp("", ".exrpd-tmp")
if err != nil {
dir = defaultHome
}
defer os.RemoveAll(dir)

return dir
Comment on lines +435 to +442
Copy link

@coderabbitai coderabbitai bot Feb 23, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical: tempDir deletes the returned directory (and may delete the real home dir).

defer os.RemoveAll(dir) runs before the caller can use dir, and if MkdirTemp fails it deletes defaultHome. This is a data-loss and correctness blocker.

🛡️ Minimal safe fix (avoid deleting on return) 
 func tempDir(defaultHome string) string {
 	dir, err := os.MkdirTemp("", ".exrpd-tmp")
 	if err != nil {
-		dir = defaultHome
+		return defaultHome
 	}
-	defer os.RemoveAll(dir)
-
 	return dir
 }

Please move cleanup to a controlled lifecycle hook (e.g., process shutdown) instead of inside tempDir.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func tempDir(defaultHome string) string {
dir, err := os.MkdirTemp("", ".exrpd-tmp")
if err != nil {
dir = defaultHome
}
defer os.RemoveAll(dir)
return dir
func tempDir(defaultHome string) string {
dir, err := os.MkdirTemp("", ".exrpd-tmp")
if err != nil {
return defaultHome
}
return dir
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@cmd/exrpd/cmd/root.go` around lines 435 - 442, The tempDir function currently
defers os.RemoveAll(dir) which will delete the directory before callers can use
it and will remove defaultHome when MkdirTemp fails; remove the defer from
tempDir (and avoid calling RemoveAll on defaultHome) and instead return the
chosen path, then register a controlled cleanup at process shutdown (e.g., in
your main shutdown/signal handler) that only removes the temporary directory
created by os.MkdirTemp; specifically update tempDir, its use sites, and add
cleanup logic that tracks whether MkdirTemp succeeded (create a variable/flag
for the temp path from tempDir and only call os.RemoveAll on that tracked
tempDir during shutdown).

}
Loading