Repository navigation
test(integration): wait for live fan-out in the NATS e2e test - #788
Merged
Merged
Conversation
TestNATSBackend_EndToEnd opened its live streams and ingested at once. A stream answers ": connected" before its hub registration, and the hub bridge reads the history stream from now on, so an event ingested before either is in place is never delivered. The test now ingests probe rows until both streams have received one. Closes #768 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wjv2nu841pxTMdUWSAPmDH
Member
Author
|
@coderabbitai review |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details🧰 Additional context used📚 Code guidelines (1)📓 Path-based instructions (2)Source excerpt: **WH001 applies to every tracked Markdown file, with no carve-out** — `AGENTS.md`, `CHANGELOG.md`, `.github/` CI docs and `.claude/` agent prompts included.📄 CodeRabbit inference engine (AGENTS.md) Files:
Source excerpt: **Editors see WH001 in `.md` only.**📄 CodeRabbit inference engine (AGENTS.md) Files:
🪛 LanguageToolCHANGELOG.md[style] ~117-~117: This sentence is over 40 words long. Consider splitting it up, as shorter sentences make the text easier to read. (TOO_LONG_SENTENCE) 🔇 Additional comments (5)
📝 SummarySummary by CodeRabbit
WalkthroughThe NATS end-to-end integration test now waits for both live streams to receive probe events before ingesting the rows under test. New helpers read matching SSE events and retry probe ingestion for up to 30 seconds. ChangesNATS integration test stream readiness
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other · Severity of issue fixed: Low Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change only makes the NATS end-to-end test wait for both live streams before ingesting rows. It does not alter production behavior, so merge risk is minimal. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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. Comment |
github-actions
Bot
added
go
|
Contributor
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
EricAndrechek
marked this pull request as ready for review
October 9, 2026 15:33
EricAndrechek
added a commit
that referenced
this pull request
Oct 9, 2026
Brings in #793 (the squash of this branch's parent, identical tree) and #788. This PR's own diff against main is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wjv2nu841pxTMdUWSAPmDH
4 of 9 tasks
EricAndrechek
added a commit
that referenced
this pull request
Oct 9, 2026
) Repeated pushes to a PR reused no Go build objects. Measured from CI logs (2026-10-06 to 10-08), that only matters on a PR that changes `go.mod`/`go.sum`, so only that case is cached, and only for the two long-pole jobs. - **Measured, typical PR.** The integration suite's `app` part compiles in 5.8–8.5 s from `main`'s entry, against 2.3–3.2 s from a same-tree one. e2e's 4–7 s cover build is hidden behind its SDK build. A PR-scoped cache would buy about 4–5 s per push, which is inside the `app` part's run-to-run spread, so these PRs still save nothing. - **Measured, dependency-changing PR (#730, #742, #753).** Every push recompiles the dependency delta from `main`'s generation: 35–50 s in the integration suite and a 37–49 s cover build in e2e, against about 3 s and 4 s from the PR's own entry. That is about 30–45 s per push on the critical path. Of 129 PR runs since 2026-09-25, 34 came from 9 such PRs, and 25 of those were second or later pushes. - **Classifier.** `scripts/classify-paths.sh` also emits `go-deps`: any `go.mod`/`go.sum` in the change set, in any module, which are the files the cache keys hash. `classify-changes.sh` passes it through only for a cleanly classified pull request and sets it false everywhere it fails closed. The `changes` job exposes it as an output. - **setup-env.** A new `go-cache-pr-save` input, which the `app` part and e2e set only when `go-deps` is true. So a PR that changes no dependency never saves, even while `main` holds no entry for its key: just after a bump lands on `main`, after a failed `main` run, or for a newly opted-in job. With the input set, the job looks up the exact key first, from the PR's scope or `main`'s, so a PR whose dependency change `main` already has hits `main`'s entry and saves nothing. Only on a miss does it restore the newest entry of its flavor (the PR's own, then `main`'s) and save the exact key into the PR's scope. The key is per dependency state, so a PR saves once per dependency change. Merge-queue refs and branches stay restore-only, and nothing is added to `ci.yml` beyond the two inputs. - **Pruning.** `pr-cache-cleanup.yml` gains a `workflow_run` job that runs after every PR's CI run. It runs the new `scripts/ci/prune-pr-build-cache.sh` from the default branch with `actions: write` and deletes every PR-scoped `gobuild-v3` entry that `main` also holds or that a newer entry of the same flavor supersedes. It deduplicates the paged listing before grouping, since a row returned twice would otherwise delete the PR's newest entry, and a fixture test (`make test-prune-pr-build-cache`, a new `make verify` leaf) pins the selection and the failure handling. No PR code holds the write token. The job needs no PR number, so it covers fork PRs too. A PR holds at most about 0.48 GiB of build cache (double that for the minutes before a prune). - **Budget.** The README sizes this for four such PRs open at once, the most open at once in the last two weeks. The worst case at rest is about 11.7 GiB, about 1.7 GiB over the cap, and eviction by last access takes it from `main`'s superseded generation, which nothing reads (inferred). At two such PRs everything fits. `main`'s live entries stay safe up to six: 3.9 + N × (0.5 CodeQL + 0.48 build) ≤ 10 GiB, assuming the superseded generation is the size of the live one. The six build flavors together are 1.02 GiB on `main`; the issue's "about 1.3 GiB per suffix" was the total for all six of a PR's flavors. ## Test plan This branch's diff matches #783's (same 13 files, +450/−67), which CodeRabbit approved; the only line differences are the verify-leaf list and the `make verify` table row, which differ because the base did. Re-run here: the pre-commit `make verify` (which runs both fixture suites below) and a full `make ci`. Items marked "measured on #783" were not repeated; the CI items are observable once this PR targets `main`. - [x] `make lint-gha` (actionlint, which also checks the new input and the `go-deps` output) and `make lint-sh` clean; `make verify` green in the pre-commit hook, including the new leaf. - [x] `make test-classify-paths`: new `go-deps` cases (root and nested modules, look-alike names, mixed with docs, empty, large input); a mutant that matches only the root `go.mod`/`go.sum` fails it (mutant measured on #783). `classify-changes.sh` against a stub `gh`: `go-deps=true` only for a pull request whose file list holds a `go.sum`; false on push, merge group, dispatch and a failed listing. - [ ] (measured on #783) The setup-env step conditions, evaluated for push to `main`, `pull_request` with and without the input, `merge_group` and `workflow_dispatch`: exactly one restore path runs, and only a PR miss with the input set is save-capable. - [ ] (measured on #783) `prune-pr-build-cache.sh --dry-run` against the live cache list selects 6 entries (1058 MiB): copies of `main`'s keys under #731, #754 and #756. It keeps #753's own entries (one per flavor, not on `main`) and never touches `main` or merge-queue scopes. - [x] `make test-prune-pr-build-cache`: deletes copies of `main`'s key and version and keeps a same key with another version; deletes superseded entries and the older of a same-second pair; keeps `-integration-` apart from `-integration-app-`; ignores other families and `main`/merge-queue scopes; keeps a PR's only entry listed twice. Through a stub `gh`: a 404 reads as already gone, a 403 fails after the rest are attempted, and a failed listing fails. Mutants caught (measured on #783): no early dedup (the pre-fix script), key-only match, merged flavors, no prefix filter, `main` scope included, listing failure ignored, 404 treated as an error, 403 swallowed. - [ ] This PR's own run, which changes no dependency: `Detect changes` logs `go-deps=false`, and every Go job, the `app` part and e2e included, uses the plain restore step, with no `Cache saved` for `gobuild-v3` in any post step. - [ ] After merge, the first `PR cache cleanup` run triggered by a PR's CI (`Prune PR build caches`): passes and deletes the entries the dry run listed, if they are still there. - [ ] The first PR that changes `go.mod`/`go.sum` (the next Dependabot go-deps PR): `Detect changes` logs `go-deps=true`; the first push restores `main`'s generation through restore-keys and saves `gobuild-v3-Linux-go-integration-app-<its hash>` and `-e2e-cov-<its hash>` under `refs/pull/<n>/merge`; the second push exact-hits them, its integration compile is about 3 s and e2e reaches its containers about 20 s after `make` starts; the prune leaves one entry per flavor; the close cleanup deletes them. A fork PR's save is unverified: if GitHub refuses it, the job only logs a warning. - [x] `make ci` passes on this branch, after merging `main` (#793 and #788). ## Related Issues Closes #781. Refs #535 (two generations on `main` still leave thin headroom). Part of #740. Supersedes #783. Now based on `main`: its parent, the `-trimpath` PR #793, has merged. It does not itself use `-trimpath` or `make ci-remote`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Wjv2nu841pxTMdUWSAPmDH --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stops
TestNATSBackend_EndToEndfrom failing now and then withno event with "\"a1\"" within 30s(#768). Test-only.mq_nats_test.go:214); A's had already received the event. The test opened both streams and ingested at once. A stream answers: connectedbefore its hub registration, and each process's hub bridge reads the history stream "from now on" (DeliverNewPolicy), so an event ingested before either is in place is never delivered to that stream; waiting longer cannot help. Under-race -parallel 8on four cores the window can open.awaitLiveFanOutingests probe rows until both streams have received one, then the test ingests the rows it checks. It does not raise the 30 s budget. A probe that needed a retry is logged./v1/streamstill sends: connectedbefore registering with the hub. A client that needs the events between connecting and registering usessince; whether: connectedshould wait for registration is a product question, left open.Test plan
go vet -tags integration ./tests/integration, and the pre-commitmake verify, green.-race -parallel 8, six iterations beside the other NATS, ingest, DLQ and boot tests and eight busy loops: 5 of 5 completed iterations passed (12.1 to 17.4 s). The flake did not reproduce.Related Issues
Closes #768. Refs #784 (the product gap the probes work around). Part of #740.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Wjv2nu841pxTMdUWSAPmDH