Discard run reductions computed before the trigger filter - #13
Conversation
Reached production. `sdr-e-base-repo-setup`'s `Lint` -- the PR-only workflow the trigger filter was written to drop -- came back as `conclusion="failure"` and put GitHubCIFailingDefaultBranch into pending. Worse than before the filter existed, because it previously showed as the harmless `stale`. `fetch_runs` caches a *reduction*, keyed by a fingerprint of each workflow's path, name, and triggers. Teaching `reduce_runs` to drop workflows with no default-branch state changed what it produces without changing any of those, so the key was unchanged, the persisted file's version still matched, and every entry still deserialised cleanly. GitHub answered 304 and the exporter replayed a pre-filter reduction containing the run it would now discard. `record_runs` then published it, and since such a workflow is no longer masked as stale, published it as an outright failure. Both existing safeguards missed it because both key on decodability: the version check and the per-entry decode fallback catch a projection whose *shape* changed, and this one's shape was byte-identical while its meaning was not. `CACHE_FORMAT_VERSION` is bumped to 2 and its documentation now says to bump it for a change in meaning as well as in shape, with a version history, since that distinction is what made this invisible. The filter is also enforced in `record_runs`, not only in `reduce_runs`. Reaching that point means the reduction did not come from the current code, and a cached projection is the way that happens. Checking at publication too makes the rule hold regardless of a reduction's provenance, which is the difference between a stale cache costing one sweep of accuracy and it publishing a fossil failure that pages. Note that clearing the cache is sufficient to recover a running deployment; verified that the released binary filters the workflow correctly on a cold cache, and that the pre-filter binary is what writes the poisoned entry. The publication guard is mutation-tested: removed, the new test fails on the exact production symptom. Its companion asserts a cadenced workflow's ancient failure is still published, masked as stale, so the guard stays surgical.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe exporter excludes cached workflow runs and last-success entries without default-branch state. Cadenced failures remain published as stale. The persisted cache format version increases to 2 for the semantic reduction change. ChangesWorkflow run cache handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@github-ci-exporter/src/collector.rs`:
- Around line 600-614: Apply the same DefaultBranchSignal::None filter used for
runs.latest to the loop publishing runs.last_success, so PR-only workflows are
excluded before emitting workflow_last_success_timestamp. Update the PR-only
replay fixture to include a last_success entry and verify that no metric for the
Lint workflow is emitted.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b7560ab9-5d08-47c0-9569-ecd135613d19
📒 Files selected for processing (2)
github-ci-exporter/src/collector.rsgithub-ci-exporter/src/github/client.rs
There was a problem hiding this comment.
Pull request overview
Updates the exporter’s ETag cache semantics and run publication logic to prevent stale cached reductions from reintroducing PR-only workflows (and their potentially “pageable” failures) after trigger-based filtering was introduced.
Changes:
- Bumps
CACHE_FORMAT_VERSIONto invalidate previously persisted projections whose meaning changed without a shape change. - Enforces
DefaultBranchSignal::Nonefiltering at publication time (record_runs) to guard against replayed cached reductions. - Adds regression tests ensuring a replayed reduction cannot resurrect a PR-only workflow while still publishing cadenced workflows as
stale.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| github-ci-exporter/src/github/client.rs | Bumps cache format version and documents semantic-versioning requirements for cached projections. |
| github-ci-exporter/src/collector.rs | Drops DefaultBranchSignal::None runs at publication and adds tests covering stale-cache replay scenarios. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Review findings, all three valid. The publication guard covered `runs.latest` but not `runs.last_success`, which a separate loop publishes. A replayed pre-filter reduction populates both, so a PR-only workflow could still emit `workflow_last_success_timestamp` -- asserting that it last passed on the default branch, which is precisely the claim being retracted. No alert or dashboard reads that series today, so the practical effect was nil, but the guard's whole premise is that the rule holds regardless of a reduction's provenance, and it did not. The signal lookup is now a single closure used by both loops, so the two cannot drift apart again. The replay fixture carries a `last_success` entry, without which the new path was untested; confirmed by mutation that removing only that guard fails the test. The PR-only assertion now checks line-wise that no sample mentions the workflow at all, rather than searching the whole document for `conclusion="failure"`, which would have been brittle against any unrelated series carrying that label. The cadenced companion gained an assertion that its last-success timestamp is still published, so the new gate cannot silently over-reach. Version-history wording now names `DefaultBranchSignal::None` rather than "no default-branch trigger", matching the type it describes.
|
All three review findings were valid and are fixed in
The fixture gained a Brittle assertion (Copilot): correct. Version-history wording (Copilot): correct. It now names 138 tests, all CI green. |
Follow-up to #12, which regressed in production. Found on deploy to the monitoring host.
Symptom
sdr-enthusiasts/sdr-e-base-repo-setup'sLint— the PR-only workflow #12's trigger filter was written specifically to drop — came back asconclusion="failure"and droveGitHubCIFailingDefaultBranchinto pending. That is worse than before the filter existed, when the same fossil run showed as the harmless, non-alertingstale.Cause
fetch_runscaches a reduction, not the raw response, keyed by a fingerprint of each workflow's path, name, and triggers.#12 taught
reduce_runsto drop workflows with no default-branch state. That changed what the reduction contains without changing the path, the name, or the triggers — so:CACHE_FORMAT_VERSIONstill matched, so the file was accepted304, and the exporter replayed a pre-filter reduction still containing the fossil runrecord_runspublished it — and because such a workflow is no longer masked as stale, published it as an outrightfailureBoth existing safeguards key on decodability. They catch a projection whose shape changed; this one's shape was byte-identical while its meaning was not. That gap is the actual defect.
Fix
CACHE_FORMAT_VERSION→ 2. The cause. Its documentation now states that a change in a projection's meaning requires a bump just as a change in shape does, and carries a version history, since that distinction is precisely what made this invisible.The filter is enforced at publication as well as at reduction. Reaching
record_runswith such a run means the reduction did not come from the current code, and a replayed cache entry is how that happens. Checking there too makes the rule hold regardless of a reduction's provenance — the difference between a stale cache costing one sweep of accuracy and it publishing a fossil failure that pages.Verification
Reproduced end-to-end with three binaries against the live API, which is how the mechanism was pinned down:
Lintreported ascbaeda6)stale— the fossil, maskedbec6c39, deployed)bec6c39, deployed)failure— the regressionThat third row is what the monitoring host hit: correct code, poisoned cache.
A useful consequence, since the second row shows the released binary is fine on a cold cache: a running deployment recovers by deleting
etags.jsonand restarting, at the cost of one uncached sweep. No redeploy needed. The version bump is what stops it recurring on the next semantic change to a projection.The publication guard is mutation-tested. With it removed,
a_replayed_reduction_cannot_resurrect_a_pr_only_workflowfails on the exact production symptom. Its companion,a_replayed_reduction_still_publishes_a_cadenced_workflow, asserts a cadenced workflow's ancient failure is still published and masked as stale, so the guard stays surgical rather than dropping runs wholesale.138 tests,
cargo xtask cigreen.Summary by CodeRabbit