Only increment stats when the worker acknowledged the test by ChrisBr · Pull Request #373 · Shopify/ci-queue

ChrisBr · 2026-02-08T20:47:11Z

Only increment error stats when the worker acknowledged the test otherwise we end up with an incorrect counter.

ruby/lib/minitest/queue/runner.rb

ruby/lib/ci/queue/redis/build_record.rb

ruby/lib/minitest/queue/build_status_recorder.rb

…place - Record stats only when worker acknowledges; duplicate acks do not increment - Redis: record_stats_delta (HINCRBY); record_success returns true when ack'd or replaced - Stat correction when success replaces failure; real assertion count (test.assertions) in delta - Test helper: Requeue before Skip when both set; test_aggregation and integration expectations updated - Remove [stats] debug logging from Redis BuildRecord; test_redis_reporter assertions = 8

thadcraft-shopify · 2026-02-25T02:19:13Z

@kangze-jia How do we test this in an actual pipeline run?

ruby/lib/ci/queue/redis/build_record.rb

kangze-jia · 2026-02-25T03:18:17Z

@kangze-jia How do we test this in an actual pipeline run?

Good question.

Here are my thoughts:

Since we’re not changing the error-reporting path (the “FAILED TESTS SUMMARY:” section), we can use it as a baseline to validate the log stats against the error report output. Ideally we should add an alarm, but we can start with a manual check first.
For each agent, verify whether it had any test failures (manual search). If it did, check whether the failures were successfully retried: If the retry succeeds, the log stats should be zero. If the retry still fails, the log stats should reflect that failure.

This will require some manual sampling across a few Buildkite builds though.

thadcraft-shopify · 2026-02-25T14:36:20Z

@kangze-jia How do we test this in an actual pipeline run?

Good question.

Here are my thoughts:

Since we’re not changing the error-reporting path (the “FAILED TESTS SUMMARY:” section), we can use it as a baseline to validate the log stats against the error report output. Ideally we should add an alarm, but we can start with a manual check first.

For each agent, verify whether it had any test failures (manual search). If it did, check whether the failures were successfully retried: If the retry succeeds, the log stats should be zero. If the retry still fails, the log stats should reflect that failure.

This will require some manual sampling across a few Buildkite builds though.

I think I am trying to understand how we get these code changes into a test pipeline before we merge this

kangze-jia · 2026-02-25T15:44:33Z

@kangze-jia How do we test this in an actual pipeline run?

Good question.
Here are my thoughts:

Since we’re not changing the error-reporting path (the “FAILED TESTS SUMMARY:” section), we can use it as a baseline to validate the log stats against the error report output. Ideally we should add an alarm, but we can start with a manual check first.

For each agent, verify whether it had any test failures (manual search). If it did, check whether the failures were successfully retried: If the retry succeeds, the log stats should be zero. If the retry still fails, the log stats should reflect that failure.

This will require some manual sampling across a few Buildkite builds though.

I think I am trying to understand how we get these code changes into a test pipeline before we merge this

Got it.

I created a branch (trigger-ci-status-test) which hit my personal ci-queue branch by updating Gemfile.lock file (https://app.graphite.com/github/pr/shop/world/419165/Add-no-op-comment-to-trigger-CI-selective-tests%3B-include-Gemfile-changes) and scheduled a job to run that branch: https://buildkite.com/shopify/world-shopify-selective-tests/builds?branch=trigger-ci-status-test&page=7

I checked the log stats which look good to me.

ChrisBr commented Feb 8, 2026

View reviewed changes

ruby/lib/minitest/queue/runner.rb Outdated Show resolved Hide resolved

ChrisBr mentioned this pull request Feb 8, 2026

Only increment counts when we acknowledge #372

Closed

ChrisBr requested a review from thadcraft-shopify February 8, 2026 21:03

ChrisBr force-pushed the cbruckmayer/only-increment-on-ack-v2 branch from a9c8024 to 14cf9e8 Compare February 9, 2026 10:37

thadcraft-shopify approved these changes Feb 9, 2026

View reviewed changes

kangze-jia reviewed Feb 9, 2026

View reviewed changes

ruby/lib/ci/queue/redis/build_record.rb Outdated Show resolved Hide resolved

kangze-jia reviewed Feb 9, 2026

View reviewed changes

ruby/lib/ci/queue/redis/build_record.rb Show resolved Hide resolved