refactor(#2949): the build-log row carries its node id
`QueueInner` was `{ sched, node_rt }`, where `node_rt` held exactly one
datum per node: the `build_logs` row id. It existed because a `hive_jobq`
node payload is immutable after insert while the log row is created when
the build starts — so the link could not ride the node.
Invert it: the log row names its node (`build_logs.node_id`, one migration
in the existing `schema_versions` framework). Same single-home property,
in the direction the type system allows.
`QueueInner` is now just the scheduler. That is the point: the queue holds
no per-node side map, so nothing has to be locked alongside the graph.
Deleted as a consequence, each surfaced by dead-code analysis after the
edit above rather than predicted:
- `NodeRuntime`, `node_rt`, `set_build_log_id`, and `build_log_id_of`
(which linear-scanned the map to match a wire `u64` against opaque
`NodeId`s). The lookup is an indexed query now.
- `struct Ctx`, entirely. It carried `coord` + `dag_id` + `node_id` into
the executors so the build-log callback could reach the queue; without
the callback, `coord`/`dag_id` were never read and `node_id` was already
on the `Claim` both executors receive.
- `QueueInner::node_running`, which existed only for `set_build_log_id`'s
"only while running" guard.
- The `Fn(i64)` callbacks on `prebuild_toplevel` / `swap_update` /
`priv_run_inner`, replaced by a `node_id: Option<u64>` passed down. The
id travels one way now instead of being registered back.
`meta.rs`'s `nix_logged` passes `None` deliberately: its callers reach it
from outside the queue as well as inside, and nothing reads the link for
them yet.
`id_for_node` takes `MAX(id)` rather than assuming uniqueness — a retried
node opens a second row and the panel wants the current attempt. The test
moved to where the behaviour lives and covers that, plus survival across
completion and non-collision with node-less rows.
This commit is contained in:
atlas
mara
parent
82ef06f445
commit
77cc7bea6b
7 changed files with 150 additions and 162 deletions
|
|
@ -13,6 +13,8 @@ use serde::Serialize;
|
|||
use tokio::sync::broadcast;
|
||||
use utoipa::ToSchema;
|
||||
|
||||
use crate::db::Migration;
|
||||
|
||||
/// Process-singleton handle, set once at coordinator startup. Lets
|
||||
/// the `lifecycle` module's `run` / `prebuild_toplevel` access the
|
||||
/// writer without threading an `Arc<BuildLogs>` through every
|
||||
|
|
@ -65,6 +67,27 @@ CREATE INDEX IF NOT EXISTS idx_build_logs_status_finished
|
|||
WHERE finished_at IS NOT NULL;
|
||||
";
|
||||
|
||||
/// Ordered schema migrations tracked in `schema_versions` (key `"build_logs"`).
|
||||
///
|
||||
/// v1 makes the log row carry its node, replacing the host-side
|
||||
/// `NodeId -> build_log_id` map the job queue used to hold. The link has a
|
||||
/// single home again, and the direction is the one the type system allows:
|
||||
/// a `hive_jobq` node payload is immutable after insert, but the log row is
|
||||
/// written when the build starts and can name the node it belongs to.
|
||||
///
|
||||
/// Legacy rows keep `node_id IS NULL` — they predate the column and no node
|
||||
/// still exists to link them to, so the dashboard's by-node lookup simply
|
||||
/// misses them (the by-agent listing, which is how they're reached, is
|
||||
/// unaffected).
|
||||
const MIGRATIONS: &[Migration] = &[Migration {
|
||||
sql: "BEGIN;
|
||||
ALTER TABLE build_logs ADD COLUMN node_id INTEGER;
|
||||
CREATE INDEX IF NOT EXISTS idx_build_logs_node
|
||||
ON build_logs (node_id) WHERE node_id IS NOT NULL;
|
||||
COMMIT;",
|
||||
adds_column: Some(("build_logs", "node_id")),
|
||||
}];
|
||||
|
||||
/// Status of a finished build attempt. Stored as the literal string in
|
||||
/// the `status` column; `NULL` while the attempt is still in progress.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize)]
|
||||
|
|
@ -152,6 +175,7 @@ impl BuildLogs {
|
|||
let conn = crate::db::open(&path, "build_logs")?;
|
||||
conn.execute_batch(SCHEMA)
|
||||
.context("apply build_logs schema")?;
|
||||
crate::db::apply_versioned_migrations(&conn, "build_logs", MIGRATIONS)?;
|
||||
let (notify_tx, _) = broadcast::channel(NOTIFY_CAP);
|
||||
Ok(Self {
|
||||
conn: Mutex::new(conn),
|
||||
|
|
@ -169,17 +193,49 @@ impl BuildLogs {
|
|||
/// Open a row for a new build attempt. Returns the assigned id
|
||||
/// — the caller threads it through `append_stdout` / `append_stderr`
|
||||
/// while the child runs and into `finish` once it exits.
|
||||
pub fn start(&self, agent: &str, kind: &str, cmdline: &str) -> Result<i64> {
|
||||
///
|
||||
/// `node_id` is the queue node this build belongs to, when there is one.
|
||||
/// It is `None` for builds that run outside the job queue; those are
|
||||
/// reachable by agent + time, just not by node.
|
||||
pub fn start(
|
||||
&self,
|
||||
agent: &str,
|
||||
kind: &str,
|
||||
cmdline: &str,
|
||||
node_id: Option<u64>,
|
||||
) -> Result<i64> {
|
||||
let now = Utc::now().timestamp();
|
||||
let node_id = node_id.and_then(|n| i64::try_from(n).ok());
|
||||
let conn = self.conn.lock().unwrap();
|
||||
conn.execute(
|
||||
"INSERT INTO build_logs (agent, kind, cmdline, started_at) VALUES (?1, ?2, ?3, ?4)",
|
||||
params![agent, kind, cmdline, now],
|
||||
"INSERT INTO build_logs (agent, kind, cmdline, started_at, node_id) \
|
||||
VALUES (?1, ?2, ?3, ?4, ?5)",
|
||||
params![agent, kind, cmdline, now, node_id],
|
||||
)
|
||||
.context("insert build_logs row")?;
|
||||
Ok(conn.last_insert_rowid())
|
||||
}
|
||||
|
||||
/// The most recent build-log row for `node_id`, if any. Replaces the job
|
||||
/// queue's in-memory `NodeId -> build_log_id` side map: the link lives in
|
||||
/// the row itself now, so it survives a restart and needs no lock held
|
||||
/// alongside the scheduler's.
|
||||
///
|
||||
/// `MAX(id)` rather than a uniqueness assumption — a node that is retried
|
||||
/// opens a second row, and the newest is the one the panel should show.
|
||||
#[must_use]
|
||||
pub fn id_for_node(&self, node_id: u64) -> Option<i64> {
|
||||
let node_id = i64::try_from(node_id).ok()?;
|
||||
let conn = self.conn.lock().unwrap();
|
||||
conn.query_row(
|
||||
"SELECT MAX(id) FROM build_logs WHERE node_id = ?1",
|
||||
params![node_id],
|
||||
|row| row.get::<_, Option<i64>>(0),
|
||||
)
|
||||
.ok()
|
||||
.flatten()
|
||||
}
|
||||
|
||||
/// Append a single stdout line. Best-effort: errors are logged
|
||||
/// but never returned to the caller, so a transient sqlite blip
|
||||
/// never tears down a rebuild's stdout pump.
|
||||
|
|
@ -453,7 +509,7 @@ mod tests {
|
|||
fn start_appends_finish_flow() {
|
||||
let (_d, db) = tmpdb();
|
||||
let id = db
|
||||
.start("alice", "prebuild", "nix build foo")
|
||||
.start("alice", "prebuild", "nix build foo", None)
|
||||
.expect("start");
|
||||
db.append_stdout(id, "building '/nix/store/abc.drv'");
|
||||
db.append_stderr(id, "error: line 12");
|
||||
|
|
@ -482,9 +538,9 @@ mod tests {
|
|||
// assert id-ordering (autoincrement) is the tiebreaker — list
|
||||
// sorts by started_at DESC but the ORDER BY still produces the
|
||||
// last-inserted row first when timestamps match.
|
||||
let id_a1 = db.start("alice", "run", "cmd one").expect("start");
|
||||
let _id_b = db.start("bob", "run", "cmd two").expect("start");
|
||||
let id_a2 = db.start("alice", "run", "cmd three").expect("start");
|
||||
let id_a1 = db.start("alice", "run", "cmd one", None).expect("start");
|
||||
let _id_b = db.start("bob", "run", "cmd two", None).expect("start");
|
||||
let id_a2 = db.start("alice", "run", "cmd three", None).expect("start");
|
||||
db.finish(id_a1, BuildStatus::Ok);
|
||||
|
||||
let alice_rows = db.list_recent_for_agent("alice", 10).expect("list");
|
||||
|
|
@ -515,10 +571,12 @@ mod tests {
|
|||
#[test]
|
||||
fn vacuum_drops_old_finished_only_per_status() {
|
||||
let (_d, db) = tmpdb();
|
||||
let id_fresh_fail = db.start("alice", "run", "fresh fail").expect("start");
|
||||
let id_old_fail = db.start("alice", "run", "old fail").expect("start");
|
||||
let id_old_ok = db.start("alice", "run", "old ok").expect("start");
|
||||
let id_running = db.start("alice", "run", "still running").expect("start");
|
||||
let id_fresh_fail = db.start("alice", "run", "fresh fail", None).expect("start");
|
||||
let id_old_fail = db.start("alice", "run", "old fail", None).expect("start");
|
||||
let id_old_ok = db.start("alice", "run", "old ok", None).expect("start");
|
||||
let id_running = db
|
||||
.start("alice", "run", "still running", None)
|
||||
.expect("start");
|
||||
db.finish(id_fresh_fail, BuildStatus::Fail);
|
||||
db.finish(id_old_fail, BuildStatus::Fail);
|
||||
db.finish(id_old_ok, BuildStatus::Ok);
|
||||
|
|
@ -550,6 +608,31 @@ mod tests {
|
|||
assert!(db.get_full(id_running).unwrap().is_some());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn node_link_survives_completion_and_newest_wins() {
|
||||
// The queue used to hold this link in an in-memory side map, which
|
||||
// meant it died with the process and needed the queue lock to read.
|
||||
// On the row it outlives both the node's completion and a restart.
|
||||
let (_d, db) = tmpdb();
|
||||
let first = db.start("alice", "swap", "cmd", Some(7)).expect("start");
|
||||
db.finish(first, BuildStatus::Fail);
|
||||
assert_eq!(
|
||||
db.id_for_node(7),
|
||||
Some(first),
|
||||
"link survives the build finishing"
|
||||
);
|
||||
|
||||
// A retried node opens a second row; the panel wants the current
|
||||
// attempt, not the first one.
|
||||
let retry = db.start("alice", "swap", "cmd", Some(7)).expect("start");
|
||||
assert_eq!(db.id_for_node(7), Some(retry), "newest attempt wins");
|
||||
|
||||
// Builds that run outside the queue carry no node and are found by
|
||||
// agent + time instead — they must not collide with node lookups.
|
||||
db.start("alice", "run", "no node", None).expect("start");
|
||||
assert_eq!(db.id_for_node(999), None, "unknown node → no row");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn append_after_finish_still_appends() {
|
||||
// Defensive: if a child's stdout pump fires one last line
|
||||
|
|
@ -557,7 +640,7 @@ mod tests {
|
|||
// append should land on the row (status already set, but the
|
||||
// log stays consistent with what happened).
|
||||
let (_d, db) = tmpdb();
|
||||
let id = db.start("alice", "run", "cmd").expect("start");
|
||||
let id = db.start("alice", "run", "cmd", None).expect("start");
|
||||
db.finish(id, BuildStatus::Ok);
|
||||
db.append_stdout(id, "post-finish trailing line");
|
||||
let full = db.get_full(id).expect("get").expect("Some");
|
||||
|
|
|
|||
Loading…
Reference in a new issue