-
Notifications
You must be signed in to change notification settings - Fork 24
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.
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #132 +/- ##
==========================================
+ Coverage 63.48% 63.57% +0.09%
==========================================
Files 212 212
Lines 22345 22348 +3
==========================================
+ Hits 14185 14208 +23
+ Misses 7078 7057 -21
- Partials 1082 1083 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zimeg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 A few of the thoughts happening with some of these changes for the most kind reviewers!
| b, err := c.get(ctx, url, token, "") | ||
| if err != nil { | ||
| return ActivityResult{}, errHTTPRequestFailed.WithRootCause(err) | ||
| return ActivityResult{}, slackerror.New(slackerror.ErrHTTPRequestFailed).WithRootCause(err) |
There was a problem hiding this comment.
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
| if err != nil { | ||
| return err | ||
| } | ||
| return nil |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
| cm.API.AssertNumberOfCalls(t, "Activity", 1) | ||
| }, | ||
| }, | ||
| "should return nil if TailArg is set and activity request fails while polling": { |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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!
mwbrooks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Nice improvement @zimeg!
🧪 During testing, I noticed that a few times I'd receive a connection reset by peer error when re-connecting the wifi network. However, it doesn't always happen.
🎥 Below is a video for anyone who wants to see the original error that this PR fixes:
2025-06-23-run-disconnect.mov
| if err != nil { | ||
| return err | ||
| } | ||
| return nil |
There was a problem hiding this comment.
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!
| cm.API.AssertNumberOfCalls(t, "Activity", 1) | ||
| }, | ||
| }, | ||
| "should return nil if TailArg is set and activity request fails while polling": { |
There was a problem hiding this comment.
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!
|
@mwbrooks Thanks so much for testing this! 🎁
Woah this might be happening for The retries of
The before video was also forgotten in these hacks - thank you for sharing this too 📸 I'll merge this now to find out if I can keep a connection ongoing these next multiple hours and will report back 🫡 |
|
🔍 A CLI error has since appeared for me after running the app for a while and repeats every few seconds but without exiting which I think is expected: I'm wondering if an expired token for |
Summary
This PR avoids exiting on an error when streaming the activity logs for the
runcommand to workaround or fix #128! 👾Preview
The following video shows a disconnected and reconnected internet connection and a healing socket connection:
0:07: The app has started and connected0:19: The internet connection is turned off0:52: A new web socket attempts to connect1:15: Activity logs begin to fail retries - New1:37: The internet connection is turned on1:40: A single socket mode connection is created with streaming logsdisconnect.mov
Notes
Requirements