Skip to main content

max / makenotwork

Handle the discarded results in the build runner, monitor, and tier prices A failed build-log append left a build looking clean. Appends go through a wrapper that counts what it could not store, and both terminal status updates carry the count, so a lossy log is visible on the build row. Remote-dir cleanup and temp-file removal name what leaked instead of dropping the result. monitor tallies per-recipient status-notification failures, so an alert that reached nobody no longer looks like one that reached everyone. install_global warns on a second call rather than ignoring it silently.
Co-Authored-By
Claude Opus 5 (1M context) <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-07-27 23:29 UTC
Signed with PGP, not checked
Commit: ca3b20fc75085999c564b95da2e45f8db4d450e1
Parent: 2f80949
3 files changed, +126 insertions, -20 deletions
M server/src/build_runner.rs +95 -17
@@ -5,6 +5,7 @@
5 5 //! the appropriate build host, clones, builds, signs, and uploads artifacts.
6 6
7 7 use std::sync::Arc;
8 + use std::sync::atomic::{AtomicUsize, Ordering};
8 9 use std::time::Duration;
9 10
10 11 use axum::extract::FromRef;
@@ -64,11 +65,24 @@
64 65 /// in `run_ssh_command` reaps the ssh process if the timeout fires.
65 66 async fn cleanup_remote_dir(host: &str, build_dir: &str) {
66 67 let cmd = format!("rm -rf {}", shell_escape(build_dir));
67 - let _ = tokio::time::timeout(
68 + // Genuinely nothing to do on failure: the build must not be held up over a
69 + // leftover directory, and a retry on a host that just timed out would hold
70 + // the slot longer. It still gets named, because the host is now carrying a
71 + // build dir nobody will remove and that is an operator's problem.
72 + match tokio::time::timeout(
68 73 Duration::from_secs(SSH_CLEANUP_TIMEOUT_SECS),
69 74 Box::pin(run_ssh_command(host, &cmd)),
70 75 )
71 - .await;
76 + .await
77 + {
78 + Ok(Ok(_)) => {}
79 + Ok(Err(e)) => {
80 + tracing::warn!(host, build_dir, error = %e, "remote build dir cleanup failed");
81 + }
82 + Err(_) => {
83 + tracing::warn!(host, build_dir, "remote build dir cleanup timed out");
84 + }
85 + }
72 86 }
73 87
74 88 /// Post-receive hook script template.
@@ -229,6 +243,9 @@
229 243 let mut artifact_keys: Vec<(String, String, String, String)> = Vec::new(); // (target_os, arch, s3_key, signature)
230 244 let mut failed_count: usize = 0;
231 245 let mut first_error: Option<String> = None;
246 + // Shared across the per-host tasks, so a log line dropped inside any target
247 + // still reaches the final status message.
248 + let log_drops = Arc::new(AtomicUsize::new(0));
232 249
233 250 // Resolve each target to its build host up front. Synchronous failures (bad
234 251 // target format, no host configured) are tallied here; resolvable targets are
@@ -239,7 +256,7 @@
239 256 for target_str in &config.targets {
240 257 let Some((target_os, arch)): Option<(&str, &str)> = target_str.split_once('/') else {
241 258 let msg = format!("invalid target format: {target_str}\n");
242 - let _ = append_log_bounded(ctx, build.id, &msg).await;
259 + append_log(ctx, build.id, &msg, &log_drops).await;
243 260 failed_count += 1;
244 261 if first_error.is_none() {
245 262 first_error = Some(format!("invalid target format: {target_str}"));
@@ -250,7 +267,7 @@
250 267 let Some(host) = build_host_for_target(&ctx.config, target_os) else {
251 268 let msg = format!("no build host for {target_os}, skipping {target_str}\n");
252 269 tracing::warn!("{}", msg.trim());
253 - let _ = append_log_bounded(ctx, build.id, &msg).await;
270 + append_log(ctx, build.id, &msg, &log_drops).await;
254 271 failed_count += 1;
255 272 if first_error.is_none() {
256 273 first_error = Some(format!("no build host for {target_os}"));
@@ -274,12 +291,13 @@
274 291 let ctx = ctx.clone();
275 292 let build = build.clone();
276 293 let config = config.clone();
294 + let log_drops = Arc::clone(&log_drops);
277 295 set.spawn(async move {
278 296 let mut oks: Vec<TargetArtifact> = Vec::new();
279 297 let mut errs: Vec<TargetError> = Vec::new();
280 298 for (target_os, arch) in &targets {
281 299 match Box::pin(execute_target(
282 - &ctx, &build, &config, &host, target_os, arch,
300 + &ctx, &build, &config, &host, target_os, arch, &log_drops,
283 301 ))
284 302 .await
285 303 {
@@ -314,7 +332,7 @@
314 332 for (target_str, e) in errs {
315 333 let msg = format!("target {target_str} failed: {e}\n");
316 334 tracing::error!("{}", msg.trim());
317 - let _ = append_log_bounded(ctx, build.id, &msg).await;
335 + append_log(ctx, build.id, &msg, &log_drops).await;
318 336 failed_count += 1;
319 337 if first_error.is_none() {
320 338 first_error = Some(e);
@@ -323,8 +341,11 @@
323 341 }
324 342
325 343 if artifact_keys.is_empty() || failed_count > 0 {
326 - let err_msg =
344 + let mut err_msg =
327 345 build_failure_message(artifact_keys.len(), failed_count, first_error.as_deref());
346 + if let Some(note) = incomplete_log_note(&log_drops) {
347 + err_msg.push_str(&note);
348 + }
328 349 if let Err(e) =
329 350 db::builds::update_build_status(&ctx.db, build.id, BuildStatus::Failed, Some(&err_msg))
330 351 .await
@@ -433,9 +454,13 @@
433 454 tracing::error!(build_id = %build.id, release_id = %release.id, error = ?e, "failed to link build to release");
434 455 }
435 456
436 - // All targets succeeded (partial failures return early above)
457 + // All targets succeeded (partial failures return early above). The build is
458 + // still a success if log lines were lost, but the row says so rather than
459 + // presenting a short log as the whole story.
460 + let note = incomplete_log_note(&log_drops);
437 461 if let Err(e) =
438 - db::builds::update_build_status(&ctx.db, build.id, BuildStatus::Succeeded, None).await
462 + db::builds::update_build_status(&ctx.db, build.id, BuildStatus::Succeeded, note.as_deref())
463 + .await
439 464 {
440 465 tracing::error!(build_id = %build.id, error = ?e, "failed to mark build as succeeded");
441 466 }
@@ -456,6 +481,7 @@
456 481 host: &str,
457 482 target_os: &str,
458 483 arch: &str,
484 + log_drops: &AtomicUsize,
459 485 ) -> std::result::Result<(String, String), String> {
460 486 let target = format!("{target_os}/{arch}");
461 487 let rust_triple =
@@ -516,7 +542,7 @@
516 542 );
517 543
518 544 let log_msg = format!("[{target}] building on {host}...\n");
519 - let _ = append_log_bounded(ctx, build.id, &log_msg).await;
545 + append_log(ctx, build.id, &log_msg, log_drops).await;
520 546
521 547 // Execute via SSH with timeout
522 548 let ssh_result = tokio::time::timeout(
@@ -538,7 +564,13 @@
538 564 }
539 565 };
540 566
541 - let _ = append_log_bounded(ctx, build.id, &format!("[{target}] {}\n", output.trim())).await;
567 + append_log(
568 + ctx,
569 + build.id,
570 + &format!("[{target}] {}\n", output.trim()),
571 + log_drops,
572 + )
573 + .await;
542 574
543 575 // SCP artifact back and upload to S3
544 576 let s3_key = crate::storage::S3Client::generate_ota_artifact_key(
@@ -570,7 +602,7 @@
570 602 // doesn't accumulate orphaned .sig temp files (the main temp is removed
571 603 // unconditionally further down, but on this early return it was never
572 604 // created).
573 - let _ = tokio::fs::remove_file(&local_sig_tmp).await;
605 + remove_temp_file(&local_sig_tmp).await;
574 606 return Err(format!("SCP download failed: {e}"));
575 607 }
576 608
@@ -579,7 +611,7 @@
579 611 let sig = tokio::fs::read_to_string(&local_sig_tmp)
580 612 .await
581 613 .unwrap_or_default();
582 - let _ = tokio::fs::remove_file(&local_sig_tmp).await;
614 + remove_temp_file(&local_sig_tmp).await;
583 615 sig
584 616 } else {
585 617 String::new()
@@ -607,18 +639,24 @@
607 639
608 640 // Always remove the local temp file, even if the upload failed, leaving
609 641 // it on disk fills the build runner's tmp directory across retries.
610 - let _ = tokio::fs::remove_file(&local_tmp).await;
642 + remove_temp_file(&local_tmp).await;
611 643
612 644 upload_result?;
613 645
614 646 if signature.is_empty() {
615 - let _ =
616 - append_log_bounded(ctx, build.id, &format!("[{target}] uploaded to {s3_key}\n")).await;
647 + append_log(
648 + ctx,
649 + build.id,
650 + &format!("[{target}] uploaded to {s3_key}\n"),
651 + log_drops,
652 + )
653 + .await;
617 654 } else {
618 - let _ = append_log_bounded(
655 + append_log(
619 656 ctx,
620 657 build.id,
621 658 &format!("[{target}] uploaded to {s3_key} (signed)\n"),
659 + log_drops,
622 660 )
623 661 .await;
624 662 }
@@ -777,6 +815,46 @@
777 815 ///
778 816 /// Probes `octet_length(log)` instead of fetching the whole row (the log
779 817 /// column tops out at 5 MiB and is read on every line append).
818 + /// Remove a build temp file, naming it if it could not be removed.
819 + ///
820 + /// The caller is either bailing out or done with the file, so there is nothing
821 + /// to propagate to; what matters is that a temp file left behind is visible,
822 + /// since these accumulate across retries and fill the runner's tmp directory.
823 + /// A missing file is the expected case on the error paths and is not a failure.
824 + async fn remove_temp_file(path: &str) {
825 + match tokio::fs::remove_file(path).await {
826 + Ok(()) => {}
827 + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {}
828 + Err(e) => tracing::warn!(path, error = %e, "failed to remove build temp file"),
829 + }
830 + }
831 +
832 + /// A one-line note for the build row when log lines were lost, or `None` when
833 + /// the log is whole.
834 + fn incomplete_log_note(drops: &AtomicUsize) -> Option<String> {
835 + match drops.load(Ordering::Relaxed) {
836 + 0 => None,
837 + n => Some(format!(
838 + " (build log incomplete: {n} line(s) could not be stored)"
839 + )),
840 + }
841 + }
842 +
843 + /// Append a build-log line, counting the line against `drops` if it could not
844 + /// be stored.
845 + ///
846 + /// Nothing useful can be done at the call site (the line is already produced
847 + /// and the build is mid-flight), but a build whose log silently lost lines must
848 + /// not finish looking clean: `run_build` reads the counter and says so on the
849 + /// build row. That is the difference between "the build printed nothing here"
850 + /// and "we failed to write down what it printed".
851 + async fn append_log(ctx: &BuildCtx, build_id: db::BuildId, line: &str, drops: &AtomicUsize) {
852 + if let Err(e) = append_log_bounded(ctx, build_id, line).await {
853 + drops.fetch_add(1, Ordering::Relaxed);
854 + tracing::error!(build_id = %build_id, error = ?e, "build log append failed; build log is incomplete");
855 + }
856 + }
857 +
780 858 async fn append_log_bounded(
781 859 ctx: &BuildCtx,
782 860 build_id: db::BuildId,
M server/src/monitor.rs +19 -2
@@ -307,6 +307,12 @@
307 307 count = subscribers.len(),
308 308 "sending status notifications to opted-in users"
309 309 );
310 + // One recipient's send failing must not stop the
311 + // fan-out, but a status alert that reached
312 + // nobody cannot look the same as one that
313 + // reached everyone: tally the failures and say
314 + // how many of the intended recipients missed it.
315 + let mut send_failures = 0usize;
310 316 for (i, sub) in subscribers.iter().enumerate() {
311 317 if i > 0 && i % 50 == 0 {
312 318 tokio::time::sleep(std::time::Duration::from_secs(1)).await;
@@ -318,7 +324,7 @@
318 324 &sub.id.to_string(),
319 325 &signing_secret,
320 326 );
321 - let _ = email_client
327 + if let Err(e) = email_client
322 328 .send_status_notification(
323 329 &sub.email,
324 330 sub.display_name.as_deref(),
@@ -326,7 +332,18 @@
326 332 &prev_status,
327 333 &unsub_url,
328 334 )
329 - .await;
335 + .await
336 + {
337 + send_failures += 1;
338 + tracing::warn!(user_id = %sub.id, error = ?e, "status notification send failed");
339 + }
340 + }
341 + if send_failures > 0 {
342 + tracing::error!(
343 + failed = send_failures,
344 + total = subscribers.len(),
345 + "status notifications did not reach every opted-in user"
346 + );
330 347 }
331 348 }
332 349 Err(e) => {
M server/src/tier_prices.rs +12 -1
@@ -144,7 +144,15 @@
144 144 /// calls are ignored (OnceLock semantics); production installs exactly
145 145 /// once.
146 146 pub fn install_global(self) {
147 - let _ = GLOBAL.set(self);
147 + if GLOBAL.set(self).is_err() {
148 + // Not fatal (the first install is the live one and prices are read
149 + // from it either way), but in production this is called exactly
150 + // once, so a second call means two config sources exist and the
151 + // second one is being ignored.
152 + tracing::warn!(
153 + "TierPrices::install_global called more than once; keeping the first install"
154 + );
155 + }
148 156 }
149 157
150 158 /// Read the installed global. Panics if `install_global` hasn't been
@@ -171,6 +179,9 @@
171 179 let a = Assumptions::load("docs/business/assumptions.toml")
172 180 .expect("test setup: load canonical assumptions.toml");
173 181 let tp = TierPrices::from_assumptions(&a);
182 + // Ignored deliberately: concurrent tests race to install here and the
183 + // values are identical, so losing the race is the documented outcome
184 + // rather than a failure.
174 185 let _ = GLOBAL.set(tp);
175 186 }
176 187 }