Skip to content

fix: propagate failures in bench-ci and bench-sync#8

Merged
maxence2997 merged 4 commits into
mainfrom
fix/bench-makefile-failures
May 3, 2026
Merged

fix: propagate failures in bench-ci and bench-sync#8
maxence2997 merged 4 commits into
mainfrom
fix/bench-makefile-failures

Conversation

@maxence2997
Copy link
Copy Markdown
Owner

Summary

`make bench-ci` and `make bench-sync` could exit 0 even when the underlying
`go test` or `benchsync` command failed, because:

  1. `go test ... | tee bench.txt` returns `tee`'s exit code (0 unless `tee`
    itself fails). A failed bench would still report green.
  2. `bench-sync`'s `;` chain ran `rm -f` last, so its exit code (0) overwrote
    any earlier failure exit from `bench-ci` or `go run ./cmd/benchsync`.

Both bugs were caught by GitHub Copilot during a review of three downstream
PRs in another repo (wspulse) that had ported this Makefile pattern. After
the wspulse fixes landed, CI immediately surfaced a real goroutine leak that
had been silently passing under the broken target. Fixing here prevents
future ports from inheriting the same trap.

Related issues

(none)

Changes

  • Added `SHELL := /bin/bash` so `set -o pipefail` and `trap` are available
    in recipes. All existing recipes are POSIX-compatible so this is safe.
  • `bench-ci`: `set -o pipefail` so `tee` no longer masks `go test`
    failures.
  • `bench-sync`: `set -e` plus `trap 'rm -f $tmpfile' EXIT` so the temp
    file is always cleaned up but a real failure in `bench-ci` or
    `go run ./cmd/benchsync` exits non-zero.

Checklist

Required

  • `make check` passes (`fmt` → `lint` → `test`)
  • Each commit represents exactly one logical change
  • Commit messages follow the format in `commit-message-instructions.md`
  • No unrelated code reformatting in this PR

1.tee swallowed go test exit code → set pipefail
2.; chain hid bench-sync errors → set -e + trap cleanup
Copilot AI review requested due to automatic review settings May 3, 2026 09:50
@github-actions github-actions Bot added the fix label May 3, 2026
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes two failure-propagation bugs in the repository’s benchmark automation so benchmark-related Make targets correctly return non-zero when the underlying benchmark run or sync step fails. That aligns the Makefile behavior with the rest of the project’s developer workflow and prevents benchmark CI from silently reporting success.

Changes:

  • Set SHELL := /bin/bash so the benchmark recipes can rely on pipefail and trap.
  • Updated bench-ci to use set -o pipefail, ensuring go test failures are not masked by tee.
  • Updated bench-sync to use set -e plus an EXIT trap so failures propagate while temporary files are still cleaned up.

1.> /dev/null hid 60s of bench output → unsilence
2.No phase markers → echo headers around the steps
1.Hardcoded /bin/bash broke on Nix → use PATH lookup
Copilot AI review requested due to automatic review settings May 3, 2026 11:00
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

Comment thread Makefile Outdated
1.Global SHELL forced bash on every recipe → target-scoped
2.POSIX recipes now usable in /bin/sh-only environments
@maxence2997 maxence2997 merged commit 996e394 into main May 3, 2026
2 checks passed
@maxence2997 maxence2997 deleted the fix/bench-makefile-failures branch May 3, 2026 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants