Skip to content

ci(nix): print the get-sources duration on every attempt, not just failures - #436

Merged
EtienneLescot merged 1 commit into
mainfrom
claude/get-sources-durations
Aug 21, 2026
Merged

ci(nix): print the get-sources duration on every attempt, not just failures#436
EtienneLescot merged 1 commit into
mainfrom
claude/get-sources-durations

Conversation

@EtienneLescot

@EtienneLescot EtienneLescot commented Aug 21, 2026

Copy link
Copy Markdown
Collaborator

What this does

The get-sources duration added in #429 exists to compare the sources path against record's. The first run carrying it printed only record's — the grep sat inside a failure branch, so the successful side, which is the interesting half, went into a scratch file and stayed there. The durations had to be reconstructed from log timestamps, which is the guessing the instrumentation was meant to replace.

Both greps move ahead of the branching. On the record side that matters more than on sources: a record that works is the one measurement this path has never produced.

The sources capture also folds into 2>&1, like record and export already do. The probe at the top of the step established that this host's xvfb-run merges stderr into stdout, so the separate .err file could never hold anything.

What the reconstruction already shows

Worth recording, because it settles a question left open in #429 — whether the 30s bound in the shared handler is too tight for a slow runner.

It is not. Enumeration on this host is bimodal, not slow:

attempt 1  succeeded   [milestone +491ms]
attempt 2  succeeded   [milestone +377ms]
attempt 3  failed      Desktop source enumeration did not return within 20000ms
attempt 4  succeeded   [milestone +387ms]
attempt 5  failed      Desktop source enumeration did not return within 20000ms

Successes land under half a second including Electron start. Failures do not return at all and are cut by whichever bound applies — 20s in the sources runner, 30s in the shared handler. There is no middle, so there is no threshold to tune, and raising the bound would buy nothing.

The asymmetry that motivated the measurement holds and got stronger: record has taken the non-returning path 6 times out of 6 across two runs, against roughly 5 in 10 for sources. At a 40% per-attempt rate, six in a row is under 0.5%, so something about the record path looks specific rather than unlucky. This PR does not attempt that; it makes the next run able to say so with a number.

Verified

Workflow YAML parses; all four embedded run blocks pass bash -n. No remaining reference to the removed .err file. No code changes, so no test or typecheck impact.

Not verified: the durations this is meant to print. That takes a run on main.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved diagnostic reporting for source and recording smoke tests.
    • Combined standard output and error output into a single test log for easier troubleshooting.
    • Ensured diagnostic milestones are reported for every test attempt, including successful runs.

…ilures

The measurement added in #429 was there to compare the sources path against
record's, and the first run carrying it printed only record's: the grep sat
inside a failure branch, so the successful side -- the interesting half -- went
into a scratch file and stayed there. The durations had to be reconstructed from
log timestamps instead, which is exactly the guessing the instrumentation was
meant to replace.

Move both greps ahead of the branching, so a success reports its duration too. On
the record side that matters more than on sources: a record that works is the one
measurement this path has never produced.

Also fold the sources capture into 2>&1 like record and export. The probe at the
top of the step established that this host's xvfb-run merges stderr into stdout,
so the separate .err file could never hold anything, and every reader that went
looking for output in it found an empty file and drew the wrong conclusion.

What the reconstruction already shows, for whoever reads this next: enumeration
here is bimodal, not slow. Successes land at 377-491ms including Electron start;
failures do not return at all and are cut by whichever bound applies -- 20s in the
sources runner, 30s in the shared handler. There is no middle, so no threshold to
tune, which is worth knowing before anyone tries.

Co-Authored-By: Claude <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 3d5484ee-84a6-408f-9bfa-12563af919b6

📥 Commits

Reviewing files that changed from the base of the PR and between b1fc616 and 2bd9db5.

📒 Files selected for processing (1)
  • .github/workflows/nix-build.yml

Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

The Nix build workflow now captures combined output for sources attempts and reports the get-sources diagnostic for every attempt. The record loop reports the same diagnostic before failure handling.

Changes

Nix build diagnostic reporting

Layer / File(s) Summary
Diagnostic capture and reporting
.github/workflows/nix-build.yml
The sources check merges stderr into its stdout capture and always reports the get-sources diagnostic. The record loop reports the diagnostic before checking the result.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to 2bd9d

This change makes CI source-fetch durations visible on successful and failed attempts without changing product code or build behavior; no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary CI workflow change: printing get-sources duration on every attempt.
Description check ✅ Passed The description provides a detailed change summary, testing results, scope, and verification limitations, despite not using all template headings.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.)
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/get-sources-durations

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@EtienneLescot
EtienneLescot merged commit 1db259b into main Aug 21, 2026
18 checks passed
@EtienneLescot
EtienneLescot deleted the claude/get-sources-durations branch August 21, 2026 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant