Add benchmark workflow and fix nats-core protocol benches

[caspervonb]

Adds a bench workflow that runs the nats-core benches on a PR's base commit and its head on the same runner, then compares head against the same-runner baseline and fails past a regression threshold. Same-machine comparison is the only way these microbenchmark numbers mean anything.

Also fixes bench_protocol.py, which passed str to encode_pub/encode_hpub after those moved to bytes subjects/replies — without it the workflow would have nothing green to run.

[synadia-claude-reviewer]

Claude finished @caspervonb's task in 2m 31s —— View job

---

Code Review

• Gather context and read PR changes
• Review .github/workflows/bench.yml
• Review nats-core/benches/bench_protocol.py
• Post inline comments on specific issues
• Post final review summary

---

Summary

bench_protocol.py — The str → bytes fixes are correct. encode_pub and encode_hpub both declare subject: bytes and reply: bytes | None, so passing byte literals is right. encode_sub still receives str subjects, which also matches its signature (subject: str) — no issue there.

bench.yml — The structure is sound: same-machine base/head comparison using runner.temp as ephemeral storage ensures --benchmark-compare=0001 always refers to the just-saved baseline. The fail-fast: false per-OS matrix, if: always() artifact upload, and the workflow_dispatch fallback to HEAD^ are all well thought out.

One minor security pattern to address: two run: steps embed ${{ steps.refs.outputs.base/head }} directly in the shell script. In practice these values are 40-char hex SHAs and carry no injection risk, but GitHub's hardening guide recommends routing them through env vars instead. Left inline suggestions on both steps.

[synadia-claude-reviewer]

Using ${{ steps.refs.outputs.base }} directly in a run: script is an expression-injection pattern that GitHub's security hardening guide warns against. The value here is always a 40-hex-char SHA so there's no real risk, but the safer idiom passes it through an env var:

Suggested change

	--benchmark-save=base \
	- name: Check out base
	env:
	BASE_SHA: ${{ steps.refs.outputs.base }}
	run: git checkout --force --detach "$BASE_SHA"
	working-directory: .

[synadia-claude-reviewer]

Same expression-injection pattern as the Check out base step — same fix applies:

Suggested change

	- name: Check out head
	env:
	HEAD_SHA: ${{ steps.refs.outputs.head }}
	run: git checkout --force --detach "$HEAD_SHA"
	working-directory: .

[caspervonb]

Review comments addressed, PTAL @claude

[synadia-claude-reviewer]

Claude finished @caspervonb's task in 58s —— View job

---

Review

• Gather context and read PR changes
• Verify the two inline suggestions were applied
• Review bench_client.py (new file, +99 lines)
• Post final review

---

Both inline suggestions have been applied correctly:

bench.yml lines 82–86 — "Check out base" now passes the SHA through an env var:

env:
    BASE_SHA: ${{ steps.refs.outputs.base }}
run: git checkout --force --detach "$BASE_SHA"

bench.yml lines 100–104 — "Check out head" likewise:

env:
    HEAD_SHA: ${{ steps.refs.outputs.head }}
run: git checkout --force --detach "$HEAD_SHA"

Both match the suggested idiom exactly. ✅

bench_client.py (new) looks solid:

• test_bench_publish and test_bench_request_throughput use benchmark.pedantic(setup=..., teardown=...) — each round gets a fresh server/client, which is correct for isolating system-call noise.
• test_bench_request_latency deliberately sets up once and repeats just the round-trip — this is the right pattern for per-request latency distribution rather than aggregate throughput.
• The semaphore-bounded gather in test_bench_request_throughput (THROUGHPUT_INFLIGHT = 100) prevents the event loop from starving under its own backlog. Good call.
• _stop_responder cancels and awaits the background task before closing the client — no leaked tasks.

No issues remaining. LGTM. 🟢
|