fix: run benchmark workflow after mkdocs to preserve data

[thomasht86]

Summary

• Change benchmark workflow trigger from push to workflow_run (triggered after mkdocs completes)
• Add condition to only run if mkdocs succeeded
• Fixes race condition where mkdocs --force push would wipe benchmark data from gh-pages

Root Cause

Both workflows triggered on push to main. MkDocs uses gh-deploy --force which wipes the entire gh-pages branch, deleting any benchmark data that was previously added.

Test plan

• Merge this PR
• Verify mkdocs workflow runs on merge
• Verify benchmark workflow triggers after mkdocs completes
• Check https://thomasht86.github.io/httpr/dev/bench/ shows accumulated data points

🤖 Generated with Claude Code

[claude]

Code Review: PR #49 - Fix benchmark workflow race condition

Summary

This PR successfully addresses the race condition between the mkdocs and benchmark workflows where mkdocs gh-deploy --force was wiping benchmark data from the gh-pages branch.

---

✅ Strengths

1. Correct Problem Identification

The root cause analysis is accurate - both workflows triggering on push to main creates a race condition where mkdocs's force push wipes the gh-pages branch.

2. Appropriate Solution

Using workflow_run is the right approach here. This ensures sequential execution: mkdocs runs first, then benchmark runs after completion.

3. Proper Condition Handling

The condition github.event_name == 'workflow_dispatch' || github.event.workflow_run.conclusion == 'success' is well-thought-out:

• Preserves manual trigger capability via workflow_dispatch
• Only runs after successful mkdocs completion
• Prevents wasted CI resources on failed mkdocs runs

---

🔍 Issues & Suggestions

1. ⚠️ Branch Filter May Not Work as Expected

Location: .github/workflows/benchmark.yml:8-9

The branches filter under workflow_run doesn't filter the triggering workflow's branch - it filters which branches this workflow file is active on. According to GitHub Actions docs, workflow_run doesn't support branch filtering for the trigger.

Current code:

workflow_run:
  workflows: ["mkdocs"]
  types:
    - completed
  branches:
    - main

Issue: If mkdocs runs on a branch other than main (though your mkdocs.yml specifies master/main), this won't prevent the benchmark from running.

Recommendation: Add an explicit branch check in the job condition:

if: ${{ (github.event_name == 'workflow_dispatch' || github.event.workflow_run.conclusion == 'success') && (github.event_name == 'workflow_dispatch' || github.event.workflow_run.head_branch == 'main') }}

Or accept that this is fine since mkdocs only runs on main/master anyway.

2. 📝 Minor: Branch Mismatch in mkdocs.yml

Location: .github/workflows/mkdocs.yml:5-6

The mkdocs workflow triggers on both master and main, but the benchmark workflow only references main. This is likely fine (assuming main is the primary branch), but could be clarified.

Recommendation: Consider if mkdocs should also only trigger on main for consistency.

---

🧪 Testing Considerations

Missing: Integration Test

The test plan relies entirely on manual verification post-merge. Consider:

• Can you test this in a fork or test branch first?
• GitHub Actions supports re-running workflows - you could verify the trigger behavior before merging

Test Plan Validation

Your test plan is comprehensive:

• ✅ Verifies mkdocs runs
• ✅ Verifies benchmark triggers after mkdocs
• ✅ Validates data accumulation

---

🔒 Security & Performance

Security

✅ No security concerns. The workflow:

• Uses the same permissions as before
• Only adds a dependency between workflows
• Condition properly validates workflow success

Performance

✅ Improvement: The benchmark workflow will no longer run unnecessarily when mkdocs fails, saving CI minutes.

⚠️ Consideration: Sequential execution means longer total time from push to benchmark completion (mkdocs must complete first). This is acceptable and necessary for the fix.

---

📊 Code Quality

Style & Conventions

✅ Follows GitHub Actions best practices
✅ Clear, descriptive workflow changes
✅ Good PR description with root cause analysis

Best Practices

✅ Preserves workflow_dispatch for manual testing
✅ Uses proper workflow_run conditions
✅ Maintains existing permissions structure

---

🎯 Final Recommendation

Approve with minor suggestion: This PR solves the stated problem correctly. The only potential improvement is clarifying the branch filtering behavior (issue #1 above), but this is not a blocker since mkdocs only runs on main/master anyway.

Before Merging:

1. ✅ Verify mkdocs workflow name is exactly "mkdocs" (it is)
2. ✅ Confirm you're okay with sequential execution (necessary trade-off)
3. ⚠️ Consider testing the trigger in a fork first (optional but recommended)

After Merging:

Follow your test plan and monitor the first few runs to ensure:

• Benchmark only runs after mkdocs completes
• Data accumulates correctly at https://thomasht86.github.io/httpr/dev/bench/

---

Overall Assessment: ✅ LGTM - Well-reasoned fix with appropriate solution. The sequential workflow execution is the correct approach for this race condition.