add polling for action status #587
Conversation
WalkthroughAdds a polling loop after action invocation that repeatedly calls Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/tasks/local/action_invoker.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: go-test (1.25.2, windows-latest)
pkg/tasks/local/action_invoker.go
Outdated
| l.Info("ActionInvoke response", zap.Any("resp", resp)) | ||
|
|
||
| if resp.GetStatus() == v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED { | ||
| if status == v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED { | ||
| return fmt.Errorf("action invoke failed: %v", resp.GetResponse()) | ||
| } |
There was a problem hiding this comment.
Error message uses stale response data.
After polling, status reflects the latest state, but line 81 uses resp.GetResponse() from the initial InvokeAction call. If the action failed during execution, the failure details would be in the final polled response, not the original one.
Consider capturing the final response from polling:
+ var finalResp *v2.InvokeActionResponse = resp
for status == v2.BatonActionStatus_BATON_ACTION_STATUS_PENDING {
// ... polling logic ...
+ // Update finalResp if GetActionStatus returns response data
}
- l.Info("ActionInvoke response", zap.Any("resp", resp))
+ l.Info("ActionInvoke response", zap.Any("resp", finalResp))
if status == v2.BatonActionStatus_BATON_ACTION_STATUS_FAILED {
- return fmt.Errorf("action invoke failed: %v", resp.GetResponse())
+ return fmt.Errorf("action invoke failed: %v", finalResp.GetResponse())
}Note: Verify that GetActionStatusResponse provides the response data needed for the error message; if it uses a different type, adjust accordingly.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/tasks/local/action_invoker.go around lines 78 to 82, the error log on
failure uses resp.GetResponse() from the initial InvokeAction call which may be
stale; update the code to use the final polled response returned by the last
GetActionStatusResponse (or whatever variable holds the final status response)
when constructing the error message so failure details reflect the terminal
state — if the polled response type differs, extract the response payload or
error field from that type and include it in the fmt.Errorf call instead of
resp.GetResponse().
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkg/tasks/local/action_invoker.go (2)
67-67: Consider whether 1-second polling interval is appropriate.The ticker uses a 1-second interval, which is 10× slower than the 100ms suggested in previous review comments. This could delay completion detection by up to 1 second per poll. If actions typically complete quickly, a shorter interval (e.g., 250-500ms) might provide better responsiveness.
70-84: Good improvements, but consider adding a timeout or max retry limit.The polling loop now correctly handles context cancellation and GetActionStatus errors, addressing the major concerns from previous reviews. However, if an action remains in PENDING/RUNNING indefinitely and the caller didn't provide a timeout context, this loop will spin forever.
Consider adding a safeguard:
+ const maxPollAttempts = 300 // 5 minutes at 1-second intervals + attempts := 0 + ticker := time.NewTicker(1 * time.Second) defer ticker.Stop() for status == v2.BatonActionStatus_BATON_ACTION_STATUS_PENDING || status == v2.BatonActionStatus_BATON_ACTION_STATUS_RUNNING { + if attempts >= maxPollAttempts { + return fmt.Errorf("action polling timeout: exceeded %d attempts", maxPollAttempts) + } + attempts++ + select { case <-ctx.Done(): return ctx.Err()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/tasks/local/action_invoker.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: go-test (1.25.2, ubuntu-latest)
- GitHub Check: go-test (1.25.2, windows-latest)
🔇 Additional comments (2)
pkg/tasks/local/action_invoker.go (2)
64-65: LGTM!Good initialization of both status and response for subsequent polling updates.
86-89: LGTM!Correctly uses
finalRespfrom the polling loop for both logging and error reporting, ensuring failure details reflect the terminal state. This addresses the previous concern about stale response data.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.