Skip to content

fix: 'run' exiting on disconnected api errors when streaming activity logs #132

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
zimeg merged 4 commits into from
Jun 25, 2025
Merged

fix: 'run' exiting on disconnected api errors when streaming activity logs #132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2f593d0
fix: avoid exiting on error when streaming activity logs
zimeg Jun 19, 2025
0d6e830
test: mock an example error code when creating a new error
zimeg Jun 19, 2025
77a3d7d
test: make the expected error states and test names match setup
zimeg Jun 19, 2025
583bea7
Merge branch 'main' into zimeg-fix-activity-tail-disconnect
mwbrooks Jun 23, 2025
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
4 changes: 2 additions & 2 deletions internal/api/activity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,13 +112,13 @@ func (c *Client) Activity(ctx context.Context, token string, activityRequest typ

b, err := c.get(ctx, url, token, "")
if err != nil {
return ActivityResult{}, errHTTPRequestFailed.WithRootCause(err)
return ActivityResult{}, slackerror.New(slackerror.ErrHTTPRequestFailed).WithRootCause(err)
Copy link
Member Author

@zimeg zimeg Jun 19, 2025

Choose a reason for hiding this comment

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

📣 We create a new error here to avoid appending different repeated root causes to this same error:

HTTP request failed (http_request_failed)
   Get "https://slack.com/api/apps.activities.list?app_id=A0923PDU39B&limit=100&min_log_level=info&min_date_created=1750312064888018": dial tcp: lookup slack.com: no such host

   Get "https://slack.com/api/apps.activities.list?app_id=A0923PDU39B&limit=100&min_log_level=info&min_date_created=1750312064888018": dial tcp: lookup slack.com: no such host

   Get "https://slack.com/api/apps.activities.list?app_id=A0923PDU39B&limit=100&min_log_level=info&min_date_created=1750312064888018": dial tcp: lookup slack.com: no such host

   Get "https://slack.com/api/apps.activities.list?app_id=A0923PDU39B&limit=100&min_log_level=info&min_date_created=1750312064888018": dial tcp: lookup slack.com: no such host

}

resp := activityResponse{}
err = goutils.JSONUnmarshal(b, &resp)
if err != nil {
return ActivityResult{}, errHTTPResponseInvalid.WithRootCause(err).AddAPIMethod(appActivityMethod)
return ActivityResult{}, slackerror.New(slackerror.ErrHTTPResponseInvalid).WithRootCause(err).AddAPIMethod(appActivityMethod)
}

if !resp.Ok {
Expand Down
13 changes: 11 additions & 2 deletions internal/api/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,18 @@ type responseMetadata struct {
}

var (
errInvalidArguments = slackerror.New(slackerror.ErrInvalidArguments)
// FIXME: errInvalidArguments should be returned as a new error from within
// the API method call to avoid changing values for this error pointer across
// API calls.
errInvalidArguments = slackerror.New(slackerror.ErrInvalidArguments)
// FIXME: errHTTPResponseInvalid should be returned as a new error right after
// the API .JSONUnmarshal function call to avoid changing values for this error
// pointer across API calls.
errHTTPResponseInvalid = slackerror.New(slackerror.ErrHTTPResponseInvalid)
errHTTPRequestFailed = slackerror.New(slackerror.ErrHTTPRequestFailed)
// FIXME: errHTTPRequestFailed should be returned as a new error right after
// the API .get call methods to avoid changing values for this error pointer
// across API calls.
errHTTPRequestFailed = slackerror.New(slackerror.ErrHTTPRequestFailed)
)

// NewClient accepts an httpClient to facilitate making http requests to Slack.
Expand Down
11 changes: 7 additions & 4 deletions internal/pkg/platform/activity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,12 @@ func Activity(
}

latestCreatedTimestamp, _, err := printLatestActivity(ctx, clients, token, activityRequest, token)
if err != nil {
return err
}

// Return an error from the activity API if the logs are not being tailed
if !args.TailArg {
if err != nil {
return err
}
return nil
Comment on lines +94 to 97
Copy link
Member Author

@zimeg zimeg Jun 19, 2025

Choose a reason for hiding this comment

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

🔍 Small preference to make the nil return more clear instead of returning err in both cases, even if it too is nil.

Copy link
Member

@mwbrooks mwbrooks Jun 20, 2025

Choose a reason for hiding this comment

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

Thanks, I prefer this clarify as well!

}

Expand All @@ -108,9 +109,11 @@ func Activity(
case <-ticker.C:
// Try to grab new logs using the last logs timestamp
activityRequest.MinimumDateCreated = latestCreatedTimestamp + 1

// Avoid exit on error from the activity API when tailing logs
newLatestCreatedTimestamp, count, err := printLatestActivity(ctx, clients, token, activityRequest, token)
if err != nil {
return slackerror.New(slackerror.ErrStreamingActivityLogs).WithRootCause(err)
clients.IO.PrintDebug(ctx, "%s\n", err)
}

if count > 0 {
Expand Down
89 changes: 82 additions & 7 deletions internal/pkg/platform/activity_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,28 +110,80 @@ func TestPlatformActivity_StreamingLogs(t *testing.T) {
},
"should return error if session validation fails": {
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) context.Context {
cm.API.On("ValidateSession", mock.Anything, mock.Anything).Return(api.AuthSession{}, slackerror.New("boomsies"))
cm.API.On("ValidateSession", mock.Anything, mock.Anything).Return(api.AuthSession{}, slackerror.New("mock_broken_validation"))
return ctx
},
ExpectedError: slackerror.New("boomsies"),
ExpectedError: slackerror.New("mock_broken_validation"),
},
"should return error if activity request fails": {
"should return error from Activity API request if TailArg is not set": {
Args: types.ActivityArgs{
TailArg: false,
},
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) context.Context {
cm.API.On("Activity", mock.Anything, mock.Anything, mock.Anything).Return(api.ActivityResult{}, slackerror.New("explosions"))
cm.API.On("Activity", mock.Anything, mock.Anything, mock.Anything).Return(api.ActivityResult{}, slackerror.New("mock_broken_logs"))
return ctx
},
ExpectedError: slackerror.New("explosions"),
ExpectedError: slackerror.New("mock_broken_logs"),
ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {
cm.API.AssertNumberOfCalls(t, "Activity", 1)
},
},
"should return nil and invoke Activity API only once if TailArg is not set": {
Args: types.ActivityArgs{
TailArg: false,
TailArg: false,
IdleTimeoutM: 1,
PollingIntervalMS: 20, // poll activity every 20 ms
},
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) context.Context {
cm.API.On("Activity", mock.Anything, mock.Anything, mock.Anything).Return(api.ActivityResult{}, nil)
cm.API.On("Activity", mock.Anything, mock.Anything, mock.Anything).
Return(api.ActivityResult{
Activities: []api.Activity{
{
Level: types.WARN,
ComponentID: "a123",
TraceID: "tr123",
},
{
Level: types.ERROR,
ComponentID: "a456",
TraceID: "tr456",
},
},
}, nil)
ctx, cancel := context.WithCancel(ctx)
go func() {
time.Sleep(time.Millisecond * 50) // cancel activity in 50 ms
cancel()
}()
return ctx
},
ExpectedError: nil,
ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {
// with the above tail argument not included we call the method once.
cm.API.AssertNumberOfCalls(t, "Activity", 1)
assert.Contains(t, cm.GetStdoutOutput(), "[warn] [a123] (Trace=tr123)")
assert.Contains(t, cm.GetStdoutOutput(), "[error] [a456] (Trace=tr456)")
},
},
"should return nil and invoke Activity API twice if TailArg is set while polling": {
Args: types.ActivityArgs{
TailArg: true,
IdleTimeoutM: 0,
PollingIntervalMS: 20, // poll activity every 20 ms
},
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) context.Context {
cm.API.On("Activity", mock.Anything, mock.Anything, mock.Anything).Return(api.ActivityResult{}, nil)
ctx, cancel := context.WithCancel(ctx)
go func() {
time.Sleep(time.Millisecond * 50) // cancel activity in 50 ms
cancel()
}()
return ctx
},
ExpectedError: nil,
ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {
// with the above polling/canceling setup, expectation is activity called two times.
cm.API.AssertNumberOfCalls(t, "Activity", 2)
},
},
"should return nil if TailArg is set and context is canceled": {
Expand All @@ -148,11 +200,33 @@ func TestPlatformActivity_StreamingLogs(t *testing.T) {
}()
return ctx
},
ExpectedError: nil,
ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {
// with the above polling/canceling setup, expectation is activity called only once.
cm.API.AssertNumberOfCalls(t, "Activity", 1)
},
},
"should return nil if TailArg is set and activity request fails while polling": {
Copy link
Member Author

@zimeg zimeg Jun 19, 2025

Choose a reason for hiding this comment

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

🧪 The added nil values for ExpectedError above are included to make these cases more explicit instead of preferring default behavior.

For this test I think it's useful, but I'm of course open to reverting this!

Copy link
Member

@mwbrooks mwbrooks Jun 20, 2025

Choose a reason for hiding this comment

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

No need to revert, I like this as well!

Args: types.ActivityArgs{
TailArg: true,
IdleTimeoutM: 1,
PollingIntervalMS: 20, // poll activity every 20 ms
},
Setup: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) context.Context {
cm.API.On("Activity", mock.Anything, mock.Anything, mock.Anything).Return(api.ActivityResult{}, slackerror.New("mock_broken_logs"))
ctx, cancel := context.WithCancel(ctx)
go func() {
time.Sleep(time.Millisecond * 50) // cancel activity in 50 ms
cancel()
}()
return ctx
},
ExpectedError: nil,
ExpectedAsserts: func(t *testing.T, ctx context.Context, cm *shared.ClientsMock) {
// with the above polling/canceling setup, expectation is activity called three times.
cm.API.AssertNumberOfCalls(t, "Activity", 3)
},
},
} {
t.Run(name, func(t *testing.T) {
// Create mocks
Expand All @@ -172,6 +246,7 @@ func TestPlatformActivity_StreamingLogs(t *testing.T) {

err := Activity(ctxMock, clients, &logger.Logger{}, tt.Args)
if tt.ExpectedError != nil {
require.Error(t, err)
assert.Contains(t, err.Error(), tt.ExpectedError.Error(), err)
} else {
require.NoError(t, err)
Expand Down
Loading