Repository navigation
Keep kelos logs -f open after empty streams - #1654
Conversation
github-actions
Bot
added
kind/bug
|
/kelos review |
|
🤖 Kelos Task Status Task |
|
🤖 Kelos Reviewer Agent @gjkim42 Review SummaryVerdict: APPROVE (sticky comment only) Findings Overview
FindingsNo qualifying findings. Key takeaways
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/cli/logs.go">
<violation number="1" location="internal/cli/logs.go:416">
P2: When the Pod has already failed but the Task status has not caught up, this branch treats the empty stream as transient and skips the `PodFailed` error below. Check `pod.Status.Phase == corev1.PodFailed` before iterating container statuses, or handle failed terminated containers explicitly, so `logs -f` reports the failure instead of retrying indefinitely.</violation>
<violation number="2" location="internal/cli/logs.go:416">
P2: When the agent container has terminated with a non-zero exit code, this branch returns retry=true (because `status.State.Terminated` is non-nil) without inspecting the exit code, and it returns before the Task/Pod failure checks. The CLI then reconnects on an empty stream every retryInterval instead of reporting the failure, until the Task phase happens to flip to Failed. Treat a non-zero `Terminated.ExitCode` as a failure so it yields the actionable error the PR/documented behavior promises, and add a test for a terminated-with-error container.</violation>
</file>
<file name="internal/cli/logs_test.go">
<violation number="1" location="internal/cli/logs_test.go:209">
P3: The new tests cover the main retry/terminal/failure paths but leave three branches of `shouldRetryEmptyTaskLogStream` unvalidated: the pod-level `PodFailed` message error, the `PodUnknown` error, and the transient `Waiting` reasons (PodInitializing/ContainerCreating) that should keep retrying. These are new decision branches in this PR; add a case for each so the empty-stream retry logic is fully verified.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| switch { | ||
| case status.State.Running != nil: | ||
| return true, nil | ||
| case status.State.Terminated != nil: |
There was a problem hiding this comment.
P2: When the Pod has already failed but the Task status has not caught up, this branch treats the empty stream as transient and skips the PodFailed error below. Check pod.Status.Phase == corev1.PodFailed before iterating container statuses, or handle failed terminated containers explicitly, so logs -f reports the failure instead of retrying indefinitely.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/cli/logs.go, line 416:
<comment>When the Pod has already failed but the Task status has not caught up, this branch treats the empty stream as transient and skips the `PodFailed` error below. Check `pod.Status.Phase == corev1.PodFailed` before iterating container statuses, or handle failed terminated containers explicitly, so `logs -f` reports the failure instead of retrying indefinitely.</comment>
<file context>
@@ -349,6 +384,71 @@ func openTaskLogStream(
+ switch {
+ case status.State.Running != nil:
+ return true, nil
+ case status.State.Terminated != nil:
+ return true, nil
+ case status.State.Waiting == nil:
</file context>
| case status.State.Terminated != nil: | ||
| return true, nil |
There was a problem hiding this comment.
P2: When the agent container has terminated with a non-zero exit code, this branch returns retry=true (because status.State.Terminated is non-nil) without inspecting the exit code, and it returns before the Task/Pod failure checks. The CLI then reconnects on an empty stream every retryInterval instead of reporting the failure, until the Task phase happens to flip to Failed. Treat a non-zero Terminated.ExitCode as a failure so it yields the actionable error the PR/documented behavior promises, and add a test for a terminated-with-error container.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/cli/logs.go, line 416:
<comment>When the agent container has terminated with a non-zero exit code, this branch returns retry=true (because `status.State.Terminated` is non-nil) without inspecting the exit code, and it returns before the Task/Pod failure checks. The CLI then reconnects on an empty stream every retryInterval instead of reporting the failure, until the Task phase happens to flip to Failed. Treat a non-zero `Terminated.ExitCode` as a failure so it yields the actionable error the PR/documented behavior promises, and add a test for a terminated-with-error container.</comment>
<file context>
@@ -349,6 +384,71 @@ func openTaskLogStream(
+ switch {
+ case status.State.Running != nil:
+ return true, nil
+ case status.State.Terminated != nil:
+ return true, nil
+ case status.State.Waiting == nil:
</file context>
| case status.State.Terminated != nil: | |
| return true, nil | |
| case status.State.Terminated != nil: | |
| if status.State.Terminated.ExitCode != 0 { | |
| return false, fmt.Errorf( | |
| "container %q in pod %q terminated with exit %d while following task %q logs", | |
| container, | |
| podName, | |
| status.State.Terminated.ExitCode, | |
| taskName, | |
| ) | |
| } | |
| return true, nil |
| } | ||
| } | ||
|
|
||
| func TestOpenTaskLogStreamRetriesEmptyStreamWhileTaskIsPending(t *testing.T) { |
There was a problem hiding this comment.
P3: The new tests cover the main retry/terminal/failure paths but leave three branches of shouldRetryEmptyTaskLogStream unvalidated: the pod-level PodFailed message error, the PodUnknown error, and the transient Waiting reasons (PodInitializing/ContainerCreating) that should keep retrying. These are new decision branches in this PR; add a case for each so the empty-stream retry logic is fully verified.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/cli/logs_test.go, line 209:
<comment>The new tests cover the main retry/terminal/failure paths but leave three branches of `shouldRetryEmptyTaskLogStream` unvalidated: the pod-level `PodFailed` message error, the `PodUnknown` error, and the transient `Waiting` reasons (PodInitializing/ContainerCreating) that should keep retrying. These are new decision branches in this PR; add a case for each so the empty-stream retry logic is fully verified.</comment>
<file context>
@@ -206,6 +206,237 @@ func TestOpenTaskLogStreamRetriesWhilePodIsStarting(t *testing.T) {
}
}
+func TestOpenTaskLogStreamRetriesEmptyStreamWhileTaskIsPending(t *testing.T) {
+ task := &kelos.Task{
+ ObjectMeta: metav1.ObjectMeta{Name: "task-1", Namespace: "default"},
</file context>
What type of PR is this?
/kind bug
What this PR does / why we need it:
Kubernetes can successfully open an agent log stream and immediately return EOF while the Task and Pod are still Pending. The CLI previously treated that empty stream as successful completion, so
kelos logs <task> -fcould exit before the Task produced any output.This change probes successful agent follow streams before returning them to the log parser. If a stream is empty, the CLI checks the current Task, Pod, and container state and reconnects while the Task remains active. Terminal Tasks end promptly, while failed Tasks and non-transient container startup failures return actionable errors. Non-empty streams continue through the existing parser without replaying output.
Which issue(s) this PR is related to:
Fixes #1653
Special notes for your reviewer:
The retry path applies only to empty agent follow streams. Init-container and non-follow log behavior remain unchanged.
Validation:
env -u CODEX_HOME -u CODEX_AUTH_JSON make testmake verifymake buildgit diff --checkThe E2E suite is left to PR CI as directed by the project guidance.
Does this PR introduce a user-facing change?
Summary by cubic
Keep
kelos logs -fopen when Kubernetes returns an empty agent log stream so the CLI does not exit before Task output appears. Previously an empty stream meant success; now the CLI reconnects while the Task remains active and returns actionable errors for terminal or non-transient startup failures.Reviewer notes
bufferedReadCloserto avoid replaying or dropping data when the stream is non-empty.shouldRetryEmptyTaskLogStream(...), which inspects Task, Pod, and container state; it retries for active states and fails with clear messages for TaskFailed, container waiting reasons (e.g., ImagePullBackOff), or PodFailed/Unknown.docs/reference.mdto describe the reconnection behavior and adds tests for retry, terminal, and failure paths.Written for commit 49b7b4a. Summary will update on new commits.