Skip to content
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.

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

fix(daemon): make status harmless. #2987

Merged
merged 1 commit into from
Dec 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 12 additions & 4 deletions cli/internal/daemon/connector/connector.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@ import (

var (
// ErrFailedToStart is returned when the daemon process cannot be started
ErrFailedToStart = errors.New("daemon could not be started")
errVersionMismatch = errors.New("daemon version does not match client version")
ErrFailedToStart = errors.New("daemon could not be started")
// ErrVersionMismatch is returned when the daemon process was spawned by a different version than the connecting client
ErrVersionMismatch = errors.New("daemon version does not match client version")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now shared.

errConnectionFailure = errors.New("could not connect to daemon")
// ErrTooManyAttempts is returned when the client fails to connect too many times
ErrTooManyAttempts = errors.New("reached maximum number of attempts contacting daemon")
Expand All @@ -38,6 +39,7 @@ var (
type Opts struct {
ServerTimeout time.Duration
DontStart bool // if true, don't attempt to start the daemon
DontKill bool // if true, don't attempt to kill the daemon
}

// Client represents a connection to the daemon process
Expand Down Expand Up @@ -237,13 +239,19 @@ func (c *Connector) connectInternal(ctx context.Context) (*Client, error) {
if err := c.sendHello(ctx, client); err == nil {
// We connected and negotiated a version, we're all set
return client, nil
} else if errors.Is(err, errVersionMismatch) {
} else if errors.Is(err, ErrVersionMismatch) {
// We don't want to knock down a perfectly fine daemon in a status check.
if c.Opts.DontKill {
return nil, err
}

// We now know we aren't going to return this client,
// but killLiveServer still needs it to send the Shutdown request.
// killLiveServer will close the client when it is done with it.
if err := c.killLiveServer(ctx, client, serverPid); err != nil {
return nil, err
}
// Loops back around and tries again.
} else if errors.Is(err, errConnectionFailure) {
// close the client, see if we can kill the stale daemon
_ = client.Close()
Expand Down Expand Up @@ -321,7 +329,7 @@ func (c *Connector) sendHello(ctx context.Context, client turbodprotocol.TurbodC
case codes.OK:
return nil
case codes.FailedPrecondition:
return errVersionMismatch
return ErrVersionMismatch
case codes.Unavailable:
return errConnectionFailure
default:
Expand Down
4 changes: 2 additions & 2 deletions cli/internal/daemon/connector/connector_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -241,8 +241,8 @@ func TestKillLiveServer(t *testing.T) {
ClientConn: conn,
}
err = c.sendHello(ctx, client)
if !errors.Is(err, errVersionMismatch) {
t.Errorf("sendHello error got %v, want %v", err, errVersionMismatch)
if !errors.Is(err, ErrVersionMismatch) {
t.Errorf("sendHello error got %v, want %v", err, ErrVersionMismatch)
}
err = c.killLiveServer(ctx, client, 99999)
assert.NilError(t, err, "killLiveServer")
Expand Down
6 changes: 1 addition & 5 deletions cli/internal/daemon/daemon.go
Original file line number Diff line number Diff line change
Expand Up @@ -306,9 +306,5 @@ func GetClient(ctx context.Context, repoRoot turbopath.AbsoluteSystemPath, logge
LogPath: logPath,
TurboVersion: turboVersion,
}
client, err := c.Connect(ctx)
if err != nil {
return nil, err
}
return client, nil
return c.Connect(ctx)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is 1:1. at no place can you get both a client and an error.

}
26 changes: 17 additions & 9 deletions cli/internal/daemon/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,10 @@ package daemon
import (
"context"
"encoding/json"
"errors"
"fmt"
"time"

"github.com/pkg/errors"
"github.com/vercel/turbo/cli/internal/cmdutil"
"github.com/vercel/turbo/cli/internal/daemon/connector"
"github.com/vercel/turbo/cli/internal/daemonclient"
Expand Down Expand Up @@ -34,6 +34,9 @@ func (l *lifecycle) status(ctx context.Context, outputJSON bool) error {
// If the daemon is not running, the status is that it's not running.
// We don't want to start it just to check the status.
DontStart: true,
// If the daemon is a different version simply report that.
// Don't attempt to kill the existing daemon.
DontKill: true,
})
if err != nil {
return l.reportStatusError(err, outputJSON)
Expand All @@ -60,22 +63,27 @@ func (l *lifecycle) status(ctx context.Context, outputJSON bool) error {
}

func (l *lifecycle) reportStatusError(err error, outputJSON bool) error {
var msg string
// Determine the unwrapped error message that we want to render.
var toRender error
if errors.Is(err, connector.ErrDaemonNotRunning) {
msg = "the daemon is not running"
toRender = connector.ErrDaemonNotRunning
} else if errors.Is(err, connector.ErrVersionMismatch) {
toRender = connector.ErrVersionMismatch
} else {
msg = err.Error()
toRender = err
}

Comment on lines +66 to +75
Copy link
Contributor Author

Choose a reason for hiding this comment

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

err is possibly wrapped as a ConnectionError at this point which means we want to reduce the amount of content we render when we're in a known error state that doesn't need so much context.

Instead of magic-stringing it like previously, we just let the error speak for itself.

// Spit it out as plain text or JSON.
if outputJSON {
rendered, err := json.MarshalIndent(map[string]string{
"error": msg,
rendered, jsonErr := json.MarshalIndent(map[string]string{
"error": toRender.Error(),
}, "", " ")
if err != nil {
return err
if jsonErr != nil {
return jsonErr
}
l.base.UI.Output(string(rendered))
} else {
l.base.UI.Output(fmt.Sprintf("Failed to contact daemon: %v", msg))
l.base.UI.Output(fmt.Sprintf("Failed to contact daemon: %v", toRender.Error()))
}
return nil
}