fix(ci-log): address review — prose tags, correct module doc, typed miss
- Remove the two issue-number tags from Rust comments (tracker-tag lint).
- Correct the module doc: the durable download is keyed by the per-repo
run NUMBER + attempt segment (matches the code + persisted_logs doc),
not a global id — the earlier 'global id' wording was stale.
- Replace the fragile e.to_string().contains("out of range") branch with
a typed StreamerMiss enum (StepOutOfRange vs Unavailable), so a usage
error can never silently fall through to the persisted-log path if a
message string changes.
This commit is contained in:
parent
d340c1773a
commit
e816cf4d72
1 changed files with 56 additions and 37 deletions
|
|
@ -14,15 +14,17 @@
|
||||||
//! shows and `pr-status` surfaces as a CI context `target_url`).
|
//! shows and `pr-status` surfaces as a CI context `target_url`).
|
||||||
//! 2. **Durable persisted-log download** — when the streamer is pruned the
|
//! 2. **Durable persisted-log download** — when the streamer is pruned the
|
||||||
//! step-log files still exist on disk (the web UI reads them fine after
|
//! step-log files still exist on disk (the web UI reads them fine after
|
||||||
//! the streamer 500s). We fetch them via the web download route
|
//! the streamer 500s). We fetch them via the web download route the run
|
||||||
//! `<base>/<owner>/<repo>/actions/runs/<run-id>/jobs/<job>/logs`, keyed
|
//! page's "view raw logs" link uses,
|
||||||
//! by the run's GLOBAL id (mirrors `artifact-get`: the REST runs list
|
//! `<base>/<owner>/<repo>/actions/runs/<run>/jobs/<job>/attempt/<n>/logs`,
|
||||||
//! translates run number → global id). This is a flat whole-job log with
|
//! keyed by the per-repo run NUMBER (same value `--run` takes) with the
|
||||||
//! no per-step framing, so `--step` is not honored on this path.
|
//! attempt segment. This is a flat whole-job log with no per-step framing,
|
||||||
|
//! so `--step` is not honored on this path.
|
||||||
//!
|
//!
|
||||||
//! Result: live + recent runs get the rich streamed view; older / quickly-
|
//! Result: live + recent runs get the rich streamed view; older / quickly-
|
||||||
//! pruned runs (the #1781 case argus + atlas both hit) still print via the
|
//! pruned runs (where `act_runner` has already pruned the live task record)
|
||||||
//! durable fallback instead of a bare "logs garbage-collected" error.
|
//! still print via the durable fallback instead of a bare
|
||||||
|
//! "logs garbage-collected" error.
|
||||||
|
|
||||||
use anyhow::{Result, bail};
|
use anyhow::{Result, bail};
|
||||||
use clap::Args as ClapArgs;
|
use clap::Args as ClapArgs;
|
||||||
|
|
@ -74,29 +76,46 @@ fn step_name(step: Option<&Value>) -> String {
|
||||||
.to_owned()
|
.to_owned()
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Try the live run-view streamer. Returns `Ok(true)` when it printed (or
|
/// Why the live streamer didn't produce logs, so `run` can tell a real usage
|
||||||
/// emitted JSON for) actual log lines, `Ok(false)` when the streamer is
|
/// error (which must surface) from a pruned/empty run (which falls back to the
|
||||||
/// reachable but has no lines (pruned task), and `Err` on a transport / HTTP
|
/// durable persisted-log download). Avoids brittle string-matching on the
|
||||||
/// failure (e.g. the `500 "resource does not exist"` a pruned run returns).
|
/// error message to make that distinction.
|
||||||
/// Both the `Ok(false)` and `Err` cases tell the caller to fall back to the
|
enum StreamerMiss {
|
||||||
/// durable persisted-log download.
|
/// A requested `--step` index is past the job's step count — a usage
|
||||||
fn streamer_logs(client: &Client, path: &str, args: &Args) -> Result<bool> {
|
/// error; `run` surfaces it instead of masking it with the fallback.
|
||||||
|
StepOutOfRange(String),
|
||||||
|
/// The streamer is reachable but has no log lines, OR a transport / HTTP
|
||||||
|
/// failure (e.g. the `500 "resource does not exist"` a pruned run
|
||||||
|
/// returns). Either way `run` falls back to the durable download.
|
||||||
|
Unavailable,
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Try the live run-view streamer. `Ok(())` means it printed (or emitted JSON
|
||||||
|
/// for) actual log lines. `Err(StreamerMiss::StepOutOfRange)` is a usage error
|
||||||
|
/// to surface; `Err(StreamerMiss::Unavailable)` tells the caller to fall back
|
||||||
|
/// to the durable persisted-log download.
|
||||||
|
fn streamer_logs(client: &Client, path: &str, args: &Args) -> Result<(), StreamerMiss> {
|
||||||
// 1. Discover the job's steps (state is returned regardless of cursors).
|
// 1. Discover the job's steps (state is returned regardless of cursors).
|
||||||
let view = client.post_json_web(path, &json!({ "logCursors": [] }))?;
|
// A transport error / pruned-task 500 surfaces here → Unavailable.
|
||||||
|
let view = client
|
||||||
|
.post_json_web(path, &json!({ "logCursors": [] }))
|
||||||
|
.map_err(|_| StreamerMiss::Unavailable)?;
|
||||||
let steps = steps_of(&view);
|
let steps = steps_of(&view);
|
||||||
if steps.is_empty() {
|
if steps.is_empty() {
|
||||||
return Ok(false);
|
return Err(StreamerMiss::Unavailable);
|
||||||
}
|
}
|
||||||
|
|
||||||
// 2. Request the chosen step(s) from cursor 0, fully expanded.
|
// 2. Request the chosen step(s) from cursor 0, fully expanded.
|
||||||
let want: Vec<usize> = match args.step {
|
let want: Vec<usize> = match args.step {
|
||||||
Some(s) if s >= steps.len() => bail!(
|
Some(s) if s >= steps.len() => {
|
||||||
"step {s} out of range — run #{} job {} has {} step(s) (0..{})",
|
return Err(StreamerMiss::StepOutOfRange(format!(
|
||||||
args.run,
|
"step {s} out of range — run #{} job {} has {} step(s) (0..{})",
|
||||||
args.job,
|
args.run,
|
||||||
steps.len(),
|
args.job,
|
||||||
steps.len() - 1
|
steps.len(),
|
||||||
),
|
steps.len() - 1
|
||||||
|
)));
|
||||||
|
}
|
||||||
Some(s) => vec![s],
|
Some(s) => vec![s],
|
||||||
None => (0..steps.len()).collect(),
|
None => (0..steps.len()).collect(),
|
||||||
};
|
};
|
||||||
|
|
@ -104,7 +123,9 @@ fn streamer_logs(client: &Client, path: &str, args: &Args) -> Result<bool> {
|
||||||
.iter()
|
.iter()
|
||||||
.map(|&s| json!({ "step": s, "cursor": 0, "expanded": true }))
|
.map(|&s| json!({ "step": s, "cursor": 0, "expanded": true }))
|
||||||
.collect();
|
.collect();
|
||||||
let resp = client.post_json_web(path, &json!({ "logCursors": cursors }))?;
|
let resp = client
|
||||||
|
.post_json_web(path, &json!({ "logCursors": cursors }))
|
||||||
|
.map_err(|_| StreamerMiss::Unavailable)?;
|
||||||
|
|
||||||
let steps_log = resp
|
let steps_log = resp
|
||||||
.get("logs")
|
.get("logs")
|
||||||
|
|
@ -113,12 +134,12 @@ fn streamer_logs(client: &Client, path: &str, args: &Args) -> Result<bool> {
|
||||||
.cloned()
|
.cloned()
|
||||||
.unwrap_or_default();
|
.unwrap_or_default();
|
||||||
if steps_log.is_empty() {
|
if steps_log.is_empty() {
|
||||||
return Ok(false);
|
return Err(StreamerMiss::Unavailable);
|
||||||
}
|
}
|
||||||
|
|
||||||
if client.json_mode() {
|
if client.json_mode() {
|
||||||
crate::verbs::print_json(&resp)?;
|
crate::verbs::print_json(&resp).map_err(|_| StreamerMiss::Unavailable)?;
|
||||||
return Ok(true);
|
return Ok(());
|
||||||
}
|
}
|
||||||
|
|
||||||
// Human output: a header per step, then its lines in order.
|
// Human output: a header per step, then its lines in order.
|
||||||
|
|
@ -138,7 +159,7 @@ fn streamer_logs(client: &Client, path: &str, args: &Args) -> Result<bool> {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
Ok(true)
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Durable fallback: download the persisted whole-job log via the web route
|
/// Durable fallback: download the persisted whole-job log via the web route
|
||||||
|
|
@ -189,20 +210,18 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
|
||||||
let stream_path = format!("/{repo}/actions/runs/{}/jobs/{}", args.run, args.job);
|
let stream_path = format!("/{repo}/actions/runs/{}/jobs/{}", args.run, args.job);
|
||||||
|
|
||||||
// 1. Live run-view streamer first — rich per-step framing for in-progress
|
// 1. Live run-view streamer first — rich per-step framing for in-progress
|
||||||
// and recently-finished runs. A pruned run errors or returns no lines;
|
// and recently-finished runs.
|
||||||
// both cases fall through to the durable download below. (An
|
|
||||||
// out-of-range `--step` is a real usage error and propagates.)
|
|
||||||
match streamer_logs(client, &stream_path, &args) {
|
match streamer_logs(client, &stream_path, &args) {
|
||||||
Ok(true) => return Ok(()),
|
Ok(()) => return Ok(()),
|
||||||
// An out-of-range `--step` is a real usage error, not a pruned run —
|
// An out-of-range `--step` is a real usage error, not a pruned run —
|
||||||
// surface it instead of masking it with the fallback.
|
// surface it instead of masking it with the fallback.
|
||||||
Err(e) if e.to_string().contains("out of range") => return Err(e),
|
Err(StreamerMiss::StepOutOfRange(msg)) => bail!(msg),
|
||||||
// Pruned task (Err) or reachable-but-no-lines (Ok(false)) → fall back.
|
// Pruned task or reachable-but-no-lines → fall through to the durable
|
||||||
Ok(false) | Err(_) => {}
|
// download (survives the act_runner task prune that 500s the streamer).
|
||||||
|
Err(StreamerMiss::Unavailable) => {}
|
||||||
}
|
}
|
||||||
|
|
||||||
// 2. Durable persisted-log download (survives the act_runner task prune
|
// 2. Durable persisted-log download (the pruned-run case this verb fixes).
|
||||||
// that 500s the streamer — the #1781 case).
|
|
||||||
persisted_logs(client, repo, &args)
|
persisted_logs(client, repo, &args)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue