fix(ci): fall back to Observer when live WorkflowRun logs are empty - #733
Open
artemiyokulov wants to merge 2 commits into
Open
fix(ci): fall back to Observer when live WorkflowRun logs are empty#733artemiyokulov wants to merge 2 commits into
artemiyokulov wants to merge 2 commits into
Conversation
hasLiveObservability stays true while the Argo Workflow CR exists, but live /logs reads pods. After OnWorkflowSuccess podGC, pods are gone and the CI plugin returned [] forever. Align fetchWorkflowRunLogs with GenericWorkflowService: for terminal runs, fall back to Observer when live logs are empty (skip for incremental sinceSeconds polls). Fixes #732 Related: openchoreo/openchoreo#4403 Signed-off-by: Artemy Okulov <artemy.okulov@x5.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
artemiyokulov
requested review from
Mirage20,
kaviththiranga,
sameerajayasoma and
stefinie123
as code owners
August 4, 2026 10:30
|
Changeset detected — the following file(s) will be released with this PR: |
📝 WalkthroughWalkthrough
ChangesWorkflow log fallback
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (1)
plugins/openchoreo-ci-backend/src/services/WorkflowService.ts (1)
614-624: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared 29-day window constant.
The same cap, comment, and computation exist in
fetchWorkflowRunEventsat Lines 748-753. Move29 * 24 * 60 * 60 * 1000to a module-level constant and reference it from both methods. This keeps the two Observer queries aligned when the limit changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/openchoreo-ci-backend/src/services/WorkflowService.ts` around lines 614 - 624, Extract the shared 29-day millisecond value into a module-level constant, preserving the existing explanatory comment, and update both fetchWorkflowRunEvents and the current time-window calculation to reference it instead of duplicating the literal. Keep the existing requested-duration and Math.min behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/openchoreo-ci-backend/src/services/WorkflowService.ts`:
- Around line 552-577: Update isWorkflowRunTerminal to treat a workflow run as
terminal whenever the API data has completedAt set, regardless of the derived
Ready reason. Preserve the existing terminal-status handling for runs without
completedAt, so statuses such as Succeeded, Failed, and Cancelled remain
terminal.
---
Nitpick comments:
In `@plugins/openchoreo-ci-backend/src/services/WorkflowService.ts`:
- Around line 614-624: Extract the shared 29-day millisecond value into a
module-level constant, preserving the existing explanatory comment, and update
both fetchWorkflowRunEvents and the current time-window calculation to reference
it instead of duplicating the literal. Keep the existing requested-duration and
Math.min behavior unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d9d37a3-389f-493f-8e2c-2f53b86fb05f
📒 Files selected for processing (1)
plugins/openchoreo-ci-backend/src/services/WorkflowService.ts
Comment on lines
+552
to
+577
| private async isWorkflowRunTerminal( | ||
| namespaceName: string, | ||
| runName: string, | ||
| token?: string, | ||
| ): Promise<boolean> { | ||
| const terminal = new Set([ | ||
| 'Succeeded', | ||
| 'Failed', | ||
| 'Completed', | ||
| 'Error', | ||
| 'Cancelled', | ||
| ]); | ||
| try { | ||
| const client = createOpenChoreoApiClient({ | ||
| baseUrl: this.baseUrl, | ||
| token, | ||
| logger: this.logger, | ||
| }); | ||
| const { data, error, response } = await client.GET( | ||
| '/api/v1/namespaces/{namespaceName}/workflowruns/{runName}', | ||
| { params: { path: { namespaceName, runName } } }, | ||
| ); | ||
| if (error || !response.ok || !data) { | ||
| return false; | ||
| } | ||
| return terminal.has(deriveWorkflowRunStatus(data)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find all WorkflowRun Ready-condition reasons and task phases used in the repo.
set -uo pipefail
# Locate other terminal-status checks to compare the accepted value sets.
rg -nP -C4 "'Succeeded'|\"Succeeded\"" --type=ts -g '!**/node_modules/**' | head -100
# Locate reason/phase constants and unions.
rg -nP -C3 "(reason|phase)\s*[:=]\s*['\"]" --type=ts -g '!**/node_modules/**' | head -80
# Show every caller/definition of deriveWorkflowRunStatus.
rg -nP -C6 '\bderiveWorkflowRunStatus\b' --type=ts -g '!**/node_modules/**'Repository: openchoreo/backstage-plugins
Length of output: 166
🏁 Script executed:
#!/bin/bash
set -u
echo "Repo files around workflow service:"
fd -a 'WorkflowService.ts' . || true
echo
echo "Git status/stat:"
git status --short || true
git diff --stat || true
echo
echo "Search deriveWorkflowRunStatus in TS files:"
rg -n -C 8 '\bderiveWorkflowRunStatus\b' . --glob '*.ts' || true
echo
echo "Search terminal status strings in TS files:"
rg -n -C 4 "Succeeded|Failed|Completed|Error|Cancelled|DeadlineExceeded|Terminated|Timeout|Running|Pending" . --glob '*.ts' || trueRepository: openchoreo/backstage-plugins
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -u
echo "Focus WorkspaceService.ts file:"
wc -l plugins/openchoreo-ci-backend/src/services/WorkflowService.ts
sed -n '1,90p' plugins/openchoreo-ci-runtime/src/services/WorkflowService.ts 2>/dev/null || true
sed -n '1,90p' plugins/openchoreo-ci-backend/src/services/WorkflowService.ts
sed -n '520,610p' plugins/openchoreo-ci-backend/src/services/WorkflowService.ts
echo
echo "targeted deriveWorkflowRunStatus occurrences:"
rg -n -C 8 '\bderiveWorkflowRunStatus\b' plugins packages --glob '*.ts' | sed -n '1,220p'
echo
echo "WorkflowRun status type definitions:"
rg -n -C 8 "WorkflowRun|workflowruns|completedAt|readyCondition" packages --glob '*.ts' plugins --glob '*.ts' | sed -n '1,260p'
echo
echo "terminal status occurrences outside generated:"
rg -n -C 4 "Succeeded|Failed|Completed|Error|Cancelled|DeadlineExceeded|Terminated|Timeout|Running|Pending" plugins packages --glob '*.ts' --glob '!**/packages/openchoreo-client-node/src/generated/**/*.ts' | sed -n '1,240p'Repository: openchoreo/backstage-plugins
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -u
echo "WorkflowService surrounding fetchWorkflowRunLogs:"
sed -n '410,590p' plugins/openchoreo-ci-backend/src/services/WorkflowService.ts
echo
echo "Programmatic behavior probe for deriveWorkflowRunStatus / isWorkflowRunTerminal terminal allowlist"
node - <<'JS'
function deriveWorkflowRunStatus(run) {
const readyCondition = (run.status?.conditions ?? []).find(
(c) => c.type === 'Ready',
);
const tasks = run.status?.tasks ?? [];
if (run.status?.completedAt) {
if (tasks.some((t) => t.phase === 'Failed' || t.phase === 'Error')) {
return 'Failed';
}
const reason = readyCondition?.reason;
if (reason && reason !== 'Running' && reason !== 'Pending') {
return reason;
}
return 'Succeeded';
}
if (readyCondition) {
return readyCondition.reason ||
(readyCondition.status === 'True' ? 'Succeeded' : 'Running');
}
if (tasks.some((t) => t.phase === 'Failed' || t.phase === 'Error')) {
return 'Failed';
}
if (tasks.every((t) => t.phase === 'Succeeded') && tasks.length > 0) {
return 'Succeeded';
}
if (tasks.some((t) => t.phase === 'Running')) {
return 'Running';
}
if (run.status?.startedAt) return 'Running';
return 'Pending';
}
const terminal = new Set([
'Succeeded',
'Failed',
'Completed',
'Error',
'Cancelled',
]);
for (const name of [
'DeadlineExceeded',
'Terminated',
'Timeout',
'Cancelled',
'Succeeded',
'Running',
'Pending',
]) {
const run = {
status: { completedAt: '2026-08-01T00:00:00Z', conditions: [{ type: 'Ready', reason: name }] },
};
const derived = deriveWorkflowRunStatus(run);
console.log(JSON.stringify({ reason: name, derived, terminal, isTerminal: terminal.has(derived) }));
}
JSRepository: openchoreo/backstage-plugins
Length of output: 6277
Treat completed workflow runs as terminal.
isWorkflowRunTerminal only checks a fixed set of status names, while deriveWorkflowRunStatus returns the Ready reason any time status.completedAt is set unless the reason is Running or Pending. Completed runs with reasons such as DeadlineExceeded, Terminated, or Timeout are seen as non-terminal, so fetchWorkflowRunLogs returns the empty live [] result instead of falling back to Observer. Use completedAt as the terminal signal here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/openchoreo-ci-backend/src/services/WorkflowService.ts` around lines
552 - 577, Update isWorkflowRunTerminal to treat a workflow run as terminal
whenever the API data has completedAt set, regardless of the derived Ready
reason. Preserve the existing terminal-status handling for runs without
completedAt, so statuses such as Succeeded, Failed, and Cancelled remain
terminal.
Signed-off-by: Artemy Okulov <artemy.okulov@x5.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
|
@artemiyokulov Thank you for your contribution. Can you please resolve the review comment from @coderabbitai? Thanks |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="openchoreo/backstage-plugins"
pr_number="733"
gh api "repos/${repo}/pulls/${pr_number}/comments?per_page=100" \
--paginate \
--jq '.[] | select(.user.login == "coderabbitai") | {
id,
path,
line,
original_line,
side,
body,
html_url,
created_at
}'
gh api graphql \
-F owner="openchoreo" \
-F name="backstage-plugins" \
-F number="$pr_number" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
path
line
comments(first: 20) {
nodes {
author { login }
body
url
}
}
}
}
}
}
}' \
--jq '.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false)
| select(any(.comments.nodes[]; .author.login == "coderabbitai"))
| {path, line, isResolved, comments: [.comments.nodes[] | select(.author.login == "coderabbitai")]}'Length of output: 7587
Please update Without this change, completed runs with Ready reasons such as You are interacting with an AI system. |
LakshanSS
reviewed
Aug 11, 2026
LakshanSS
left a comment
LakshanSS
left a comment
Contributor
There was a problem hiding this comment.
@artemiyokulov Kind reminder on this
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
podGC.strategy: OnWorkflowSuccess.hasLiveObservability=truebut live openchoreo-api logs are empty and the run is terminal, fall back to Observer (POST /api/v1/logs/query), matchingGenericWorkflowService.getWorkflowRunLogs.sinceSecondspolls while a step is still running.Fixes openchoreo/openchoreo#4404
Related: openchoreo/openchoreo#4403
Context
hasLiveObservabilityis true while the Argo Workflow CR exists. Live/logsreads pods. After success + podGC, pods are gone → live returns[]while the CR (and flag) remain. The CI backend previously returned that empty array; the workflows backend already had Observer fallback.Test plan
podGC: OnWorkflowSuccess, pods deleted, Workflow CR still presenthasLiveObservability=truestill)sinceSecondsdoes not fall back when live is emptyhasLiveObservability=falsestill use Observer-only pathMade with Cursor
Summary by CodeRabbit