[RAPTOR-19727] Don't suggest logs command that won't work - #852
[RAPTOR-19727] Don't suggest logs command that won't work#852taras-pokornyy wants to merge 2 commits into
Conversation
|
🎫 Jira: |
|
/approve-smoke-tests |
|
🔐 Fork PR smoke tests triggered by @taras-pokornyy What happens next:
|
|
🔐 Fork smoke tests started by maintainer ⏳ Security scans passed. Running smoke tests... Commit: |
|
✅ All smoke tests passed! (Fork PR) ✅ Security Scan: success |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 5c756c6. Configure here.
| // logsAvailable is true. | ||
| func BuildFailureMessage(artifactID, buildID, status string, logsAvailable bool) error { | ||
| if !logsAvailable { | ||
| return fmt.Errorf("build %s ended with status %s; no logs were captured for this build", buildID, status) |
There was a problem hiding this comment.
Fetch errors claimed as missing logs
Medium Severity
BuildFailureMessage states that no logs were captured whenever logsAvailable is false, but callers treat a failed logs fetch the same as a successful empty result. BuildLogsAvailable returns false on any error, and create/get infer availability from LogTail after BuildSummaryFor swallows fetch failures. A 5xx, 403, or transient outage then tells the user logs never existed, so they will not retry dr artifact build logs even though the stream may have just printed lines or the command would succeed moments later.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 5c756c6. Configure here.
wojtekwdr
left a comment
wojtekwdr
left a comment
There was a problem hiding this comment.
LGTM in general, one medium finding
| // Precheck log availability before suggesting the logs command -- | ||
| // this is the only call site with no already-fetched log info, so | ||
| // it's the one place that needs its own extra fetch. | ||
| return built.ID, workload.BuildFailureMessage(artifactID, built.ID, built.Status, hasLogsFn(artifactID, built.ID)) |
There was a problem hiding this comment.
[medium] Build log ingestion trails the builder by 20-40s per the buildLogLagAllowance comment, and this fires right after tail.Finish(), which is a single catch-up poll. A build that fails fast gets told no logs were captured, then the lines land a second later. Could the tail track whether it ever emitted a line instead?
| return fmt.Errorf("build %s ended with status %s; no logs were captured for this build", buildID, status) | ||
| } | ||
|
|
||
| return fmt.Errorf("build %s ended with status %s; see 'dr artifact build logs %s %s'", buildID, status, artifactID, buildID) |
There was a problem hiding this comment.
[low] question: does this hint hold up for a build that only logged at DEBUG? dr artifact build logs defaults to --level info and filters, but both availability checks count unfiltered entries, so that build gets the hint and then prints "No logs found."
| fmt.Errorf("build %s ended with status %s", id, workload.BuildStatusFailed) | ||
| } | ||
| // Logs exist for this build, so the failure message should point at them. | ||
| f.hasLogs = func(string, string) bool { return true } |
There was a problem hiding this comment.
[low] Both tests that reach this path force hasLogs true, so nothing asserts the "no logs were captured" wording through buildImage. Hardcoding true at build.go:487 would still pass the suite.
| // build. Used to decide whether it's worth pointing the user at | ||
| // 'dr artifact build logs' -- suggesting a command that 404s or prints | ||
| // nothing is worse than not suggesting anything. | ||
| func BuildLogsAvailable(artifactID, buildID string) bool { |
There was a problem hiding this comment.
[P2] BuildLogsAvailable drains every log page to answer a boolean
BuildLogsAvailable calls GetArtifactBuildLogs, which hardcodes maxEntries = 0 (build.go:337). In drainLogPages, maxEntries <= 0 means "drain every page" — it follows every next link to the end with no early exit. For a failed build with a verbose Docker log stream, that's potentially dozens of HTTP round-trips just to evaluate len(entries) > 0.
The trigger is this PR's only "extra precheck fetch" call site: buildImage in internal/workload/up/build.go calls hasLogsFn on every failed dr workload up, right after BuildLogTail already fetched (much of) the same data during the wait.
The fix is cheap because the seam already exists: call fetchArtifactBuildLogs(artifactID, buildID, 1, "", "", "") directly — limit=1 short-circuits after the first page — or give GetArtifactBuildLogs a limit parameter. (Related to but distinct from the ingestion-lag concern raised in the other thread: even with correct timing, this check is far more expensive than it needs to be.)
| func BuildLogsAvailable(artifactID, buildID string) bool { | |
| func BuildLogsAvailable(artifactID, buildID string) bool { | |
| // limit=1: we only need to know whether at least one entry exists; | |
| // maxEntries=0 would drain every page of a verbose build's log stream. | |
| otel, err := fetchArtifactBuildLogs(artifactID, buildID, 1, "", "", "build logs") | |
| return err == nil && len(otel) > 0 | |
| } |
| } | ||
|
|
||
| if len(entries) == 0 { | ||
| fmt.Fprintln(os.Stderr, "No logs found.") |
There was a problem hiding this comment.
[P3] "No logs found." is wrong for logs filtered out by the default level
Verified end-to-end: cmd/artifact/build/logs/cmd.go calls GetArtifactBuildLogs (unfiltered), then FilterLogsByLevel(entries, "info") (the flag's default), then RenderBuildLogs. A DEBUG-only build therefore delivers an empty slice here, and this now prints No logs found. to stderr — factually wrong, the logs exist. Before this PR, empty text output printed nothing, so this message is newly introduced for the filtered-empty case, and it nudges the user away from the actual remedy (--level debug).
Render-side sibling of the unfiltered-availability comment on build.go: since filtering happens in the caller, RenderBuildLogs can't distinguish "no logs" from "all filtered out". Options: pass the pre-filter count into the render call, or have the cmd emit a level-aware message like No logs at this level (try --level debug).
3 participants
RATIONALE
RAPTOR-19727: when a build fails or is
cancelled before the builder produces any output, the CLI unconditionally told the user to run
dr artifact build logs <artifact> <build>— a command that either 404s or succeeds whileprinting nothing, since there's genuinely no log data to show. The suggestion was built purely off
the build's terminal status, with no check that logs actually existed, and was duplicated
independently across three call sites (with inconsistent wording, and one missing the artifact ID
entirely, so the command it "helpfully" printed couldn't even be copy-pasted as-is).
dr artifact build logsitself also had no real empty case: text mode printed nothing (a silentno-op) and
-o jsonprinted the literal stringnullinstead of[].CHANGES
workload.BuildLogsAvailable(artifactID, buildID)andworkload.BuildFailureMessage(artifactID, buildID, status, logsAvailable)as the single sharedway to decide whether a logs hint is worth showing and to word it — replacing three independent,
drifted copies of the same message.
WaitForBuildno longer guesses at a logs suggestion (it doesn't have the artifact ID cheaply);it returns a plain status error and lets callers that already have log info build the final
message via
BuildFailureMessage.cmd/artifact/build/getandcmd/artifact/build/createnow reuse the log tailBuildSummaryForalready fetches to decide whether to show the hint, at no extra API cost;
dr workload up'sbuildImageis the one call site with no pre-fetched log info, so it does the one extra precheckfetch.
RenderBuildLogsnow matchesRenderWorkloadLogs's established empty-output convention: JSON isalways an array (never
null), and text mode prints"No logs found."to stderr so stdout staysclean for piping/grepping.
mainto pick up the concurrent OTEL-based build-log streaming rewrite(
internal/workload/build_logs.go, live--waitlog tailing inartifact build create) andresolved the two real conflicts (
internal/workload/build.go,cmd/artifact/build/create/cmd.go)by layering this change's log-availability precheck on top of
main's new streamingimplementation. Updated two tests that asserted the old raw-HTTP-404 behavior of
GetArtifactBuildLogs, since logs are now read from the OTEL stream, where "no logs for thisbuild" is a normal empty page rather than a 404.
PR Automation
Comment-Commands: Trigger CI by commenting on the PR:
/trigger-smoke-testor/trigger-test-smoke- Run smoke tests/trigger-install-testor/trigger-test-install- Run installation testsLabels: Apply labels to trigger workflows:
run-smoke-testsorgo- Run smoke tests on demand (only works for non-forked PRs)Important
For Forked PRs: The
run-smoke-testslabel won't work. A required Smoke Tests check will block merge until a maintainer acts:/approve-smoke-teststo run smoke tests (results will set the check)/skip-smoke-teststo bypass the check without running testsPlease comment requesting a maintainer review if you need smoke tests to run.
Note
Low Risk
CLI error messaging and log rendering only; no changes to auth, deploy logic, or API contracts beyond clearer exit errors.
Overview
Failed or cancelled builds no longer always tell users to run
dr artifact build logswhen there is nothing to show.BuildLogsAvailableandBuildFailureMessagecentralize that decision: suggestdr artifact build logs <artifact> <build>only when logs exist, otherwise say no logs were captured.WaitForBuildnow returns a plain terminal-status error (no logs hint).dr artifact build getandcreate --waitbuild the user-facing error viaBuildFailureMessage, usingsummary.LogTailfromBuildSummaryForso there is no extra fetch.dr workload upis the one path that still callsBuildLogsAvailablebefore wording the failure.RenderBuildLogsmatches workload logs behavior: JSON always emits[](notnull); empty text mode prints "No logs found." on stderr so stdout stays pipe-safe.Reviewed by Cursor Bugbot for commit 5c756c6. Configure here.