Skip to content

fix: improve rollapp start handling #1430

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

Open
anhductn2001 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: improve rollapp start handling #1430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1d7eec6
fix: improve rollapp start handling
anhductn2001 Dec 12, 2025
3d78d23
add unit test
anhductn2001 Jan 9, 2026
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
77 changes: 37 additions & 40 deletions cmd/rollapp/start/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ import (
var (
DaLcEndpoint string
DaLogPath string

execCmdFollowFunc = bash.ExecCmdFollow
)

func Cmd() *cobra.Command {
Expand Down Expand Up @@ -120,8 +122,8 @@ Consider using 'services' if you want to run a 'systemd'(unix) or 'launchd'(mac)
go healthagent.Start(home, rollappConfig.HealthAgent, rollerLogger)
}

done := make(chan error, 1)
// nolint: errcheck
var promptResponses map[string]string
if rollappConfig.KeyringBackend == consts.SupportedKeyringBackends.OS {
pswFileName, err := filesystem.GetOsKeyringPswFileName(
consts.Executables.RollappEVM,
Expand All @@ -138,58 +140,53 @@ Consider using 'services' if you want to run a 'systemd'(unix) or 'launchd'(mac)
return
}

pr := map[string]string{
promptResponses = map[string]string{
"Enter keyring passphrase": psw,
"Re-enter keyring passphrase": psw,
}
}

ctx, cancel := context.WithCancel(cmd.Context())
defer cancel()
go func() {
err := bash.ExecCmdFollow(
done,
ctx,
startRollappCmd,
pr,
)

done <- err
}()
} else {
ctx, cancel := context.WithCancel(cmd.Context())
defer cancel()

go func() {
err := bash.ExecCmdFollow(
done,
ctx,
startRollappCmd,
nil, // No need for printOutput since we configured output above
)

done <- err
}()
select {
case err := <-done:
if err != nil {
pterm.Error.Println("rollapp's process returned an error: ", err)
os.Exit(1)
}
case <-ctx.Done():
pterm.Error.Println("context cancelled, terminating command")
return
}
ctx, cancel := context.WithCancel(cmd.Context())
defer cancel()

err = runRollappCommand(ctx, startRollappCmd, promptResponses)
if err != nil {
pterm.Error.Println(err)
os.Exit(1)
}

select {}
},
}
cmd.Flags().String("log-level", "debug", "pass the log level to the rollapp")

return cmd
}

func runRollappCommand(
ctx context.Context,
cmd *exec.Cmd,
promptResponses map[string]string,
) error {
signalChan := make(chan error, 1)
execDone := make(chan error, 1)

go func() {
err := execCmdFollowFunc(signalChan, ctx, cmd, promptResponses)
execDone <- err
}()

select {
case err := <-signalChan:
if err != nil {
return fmt.Errorf("rollapp process received signal: %w", err)
}
return nil
case err := <-execDone:
return err
case <-ctx.Done():
return fmt.Errorf("context cancelled: %w", ctx.Err())
}
Comment on lines +177 to +187
Copy link

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

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

Logic error in channel handling: The function receives from signalChan (which is the done channel passed to execCmdFollowFunc) and execDone (which receives the return value from execCmdFollowFunc). However, when a signal is received, bash.ExecCmdFollow sends to signalChan and may still return an error. The current select will only read one value, potentially missing errors. Additionally, if signalChan receives nil (successful signal handling), the function returns nil without waiting for the execCmdFollowFunc to complete, which could lead to resource leaks.

Copilot uses AI. Check for mistakes.
}

func PrintOutput(
rlpCfg roller.RollappConfig,
withBalance,
Expand Down
77 changes: 77 additions & 0 deletions cmd/rollapp/start/start_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package start

import (
"context"
"errors"
"os/exec"
"testing"
"time"

"github.com/dymensionxyz/roller/utils/bash"
)

func TestRunRollappCommandSuccess(t *testing.T) {
t.Cleanup(func() { execCmdFollowFunc = bashExecCmdFollowWrapper })
execCmdFollowFunc = func(done chan<- error, ctx context.Context, cmd *exec.Cmd, _ map[string]string) error {
done <- nil
return nil
}
Comment on lines +15 to +18
Copy link

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

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

Test may be flaky: The mock function sends nil to the done channel and immediately returns nil. This creates a race condition where either signalChan or execDone could be selected first in the runRollappCommand select statement. The test expects success, but if execDone is selected first (receiving the return value of nil), the signalChan value would never be read, potentially causing goroutine leaks in production code. Consider adding a small delay or using a more deterministic approach to test the intended behavior.

Copilot uses AI. Check for mistakes.

ctx, cancel := context.WithTimeout(context.Background(), time.Second)
defer cancel()

if err := runRollappCommand(ctx, exec.Command("echo"), nil); err != nil {
t.Fatalf("expected no error, got %v", err)
}
Comment on lines +20 to +25
Copy link

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

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

Inconsistent indentation: These lines use spaces for indentation while the rest of the file uses tabs. Go convention is to use tabs for indentation.

Copilot uses AI. Check for mistakes.
}

func TestRunRollappCommandReturnsProcessError(t *testing.T) {
t.Cleanup(func() { execCmdFollowFunc = bashExecCmdFollowWrapper })
execCmdFollowFunc = func(done chan<- error, ctx context.Context, cmd *exec.Cmd, _ map[string]string) error {
return errors.New("boom")
}

ctx, cancel := context.WithTimeout(context.Background(), time.Second)
defer cancel()

err := runRollappCommand(ctx, exec.Command("echo"), nil)
if err == nil {
t.Fatalf("expected error, got nil")
}
Comment on lines +37 to +40
Copy link

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

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

Incomplete error handling test: The test only verifies that an error is returned, but doesn't verify that it's the correct error (matching "boom"). Consider using error message assertion to ensure the error is properly propagated.

Copilot uses AI. Check for mistakes.
}

func TestRunRollappCommandSignals(t *testing.T) {
t.Cleanup(func() { execCmdFollowFunc = bashExecCmdFollowWrapper })
execCmdFollowFunc = func(done chan<- error, ctx context.Context, cmd *exec.Cmd, _ map[string]string) error {
done <- errors.New("SIGTERM")
return nil
}

ctx, cancel := context.WithTimeout(context.Background(), time.Second)
defer cancel()

if err := runRollappCommand(ctx, exec.Command("echo"), nil); err == nil {
t.Fatalf("expected signal error")
}
Comment on lines +46 to +55
Copy link

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

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

Inconsistent indentation: These lines use spaces for indentation while the rest of the file uses tabs. Go convention is to use tabs for indentation.

Copilot uses AI. Check for mistakes.
Comment on lines +43 to +55
Copy link

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

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

Test may not properly verify signal handling: The mock sends an error to the done channel and returns nil, but in the actual bash.ExecCmdFollow implementation, when a signal is received, it sends to doneChan and then continues to execute cmd.Wait() which may also return an error. The test should verify the actual error message or type to ensure the signal error is being handled correctly, not just any error.

Copilot uses AI. Check for mistakes.
}

func TestRunRollappCommandContextCancelled(t *testing.T) {
t.Cleanup(func() { execCmdFollowFunc = bashExecCmdFollowWrapper })
execCmdFollowFunc = func(done chan<- error, ctx context.Context, cmd *exec.Cmd, _ map[string]string) error {
select {

Check failure on line 61 in cmd/rollapp/start/start_test.go

View workflow job for this annotation

GitHub Actions / lint 

S1000: should use a simple channel send/receive instead of `select` with a single case (gosimple)
case <-ctx.Done():
return ctx.Err()
}
}

ctx, cancel := context.WithCancel(context.Background())
cancel()

if err := runRollappCommand(ctx, exec.Command("echo"), nil); err == nil {
t.Fatalf("expected context error")
}
}

func bashExecCmdFollowWrapper(done chan<- error, ctx context.Context, cmd *exec.Cmd, prompts map[string]string) error {
return bash.ExecCmdFollow(done, ctx, cmd, prompts)
}
Comment on lines +59 to +77
Copy link

Copilot AI Jan 10, 2026

Choose a reason for hiding this comment

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

Inconsistent indentation: These lines use spaces for indentation while the rest of the file uses tabs. Go convention is to use tabs for indentation.

Copilot uses AI. Check for mistakes.
Loading