tests: add goleak leak detection and advisory race-detector CI (CON-179) - #4721
Open
prakhargarg105 wants to merge 10 commits into
Open
tests: add goleak leak detection and advisory race-detector CI (CON-179)#4721prakhargarg105 wants to merge 10 commits into
prakhargarg105 wants to merge 10 commits into
Conversation
… packages
Wire goleak.VerifyTestMain into internal/impl/{protobuf,sql,postgresql,mysql}
— the packages that shipped leak bugs (9caed49, 07c39cd) plus the two
flagship CDC connector packages — with narrow, commented per-package ignore
lists, each verified green on a clean baseline (unit x3, -race x2,
postgres/mysql integration suites under Docker).
Two real test-side leaks surfaced and fixed rather than ignored:
- protobuf: runMockBSRServer used bare http.Serve with no shutdown, leaking
the accept loop and connection goroutines; now an http.Server closed via
t.Cleanup.
- postgresql: TestIntegrationPostgresSnapshotAckBarrier's consumer waited on
a context benthos never cancels (runConsumerFunc passes Background), and
run 1 was never stopped because Stream.Run returns on ctx cancel without
tearing the stream down; now selects on the test-owned context and stops
the crashed stream explicitly.
Also documents the "adding goleak to a package" recipe in the tester agent
guide, per the R2 acceptance criteria.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…r packages Add race_test.yml: on every PR it runs go test -race against only the internal/impl/<component> packages the PR touches, using the same change-detection as integration_test.yml to bound the 2-10x race-detector cost. Failures surface as warning annotations and a step-summary table, not a red check — unless the package is listed in .github/race-blocking-packages.txt, the promotion list for packages that have earned a green -race baseline. The list starts empty so the gate can never fail on a pre-existing race a PR didn't touch. Also reference task test:unit-race in the CONTRIBUTING §6 pre-PR checklist, per the R3 acceptance criteria. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…nores Review on #4721 flagged that the goleak ignores in main_test.go were masking a real production leak: multiModuleWatcher created each prototransform.SchemaWatcher with context.Background() and no stop path, so every BSR-backed protobuf processor leaked a poll goroutine per module on each construction. The original justification ("no stop hook") was wrong — SchemaWatcher exposes Stop(). - multiModuleWatcher gains close(), which stops every watcher. - protobufProc.Close calls it, giving the poll loops a shutdown path. - BSR tests now Close their processors via t.Cleanup. - All three goleak ignores (prototransform poll loop and the two open-ended net/http persistConn entries) are removed; goleak now guards against regressions of this leak instead of hiding it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Follow-up review on #4721: wiring close() into protobufProc.Close fixed the happy path, but watcher poll goroutines still leaked when construction failed after watchers were started: - newProtobuf: newMultiModuleWatcher succeeds, then strToProtobufBSROperator fails (e.g. message not resolvable in any BSR module) — the framework never gets a processor to Close. - newMultiModuleWatcher: an error on a later module dropped already-running watchers for earlier modules. Both paths now stop the accumulated watchers before returning the error. Adds TestProtobufBSRConstructionFailureStopsWatchers covering the unresolvable-message path; the package's goleak TestMain is what verifies the watchers actually stop (removing the newProtobuf close() call makes goleak fail with the prototransform poll goroutine). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ak review Two more leak paths from review on #4721: protobuf: prototransform.NewSchemaWatcher starts its poll loop immediately, so when AwaitReady failed (unreachable BSR URL, rejected API key) newSchemaWatcher returned without Stop() and the watcher never reached bsrClients — no cleanup path could touch it. Stop the watcher before returning the error. postgres_cdc: the snapshot->stream handoff parked a goroutine on snapshotAckWG.Wait(); a soft stop with unacked snapshot batches abandoned it for the life of the process (WaitGroup.Wait cannot be interrupted). Replace the WaitGroup with an atomic pending counter plus a buffered drain-notification channel so the handoff waits directly on drain-or-soft-stop with no helper goroutine at all. Semantics are unchanged: block until acks drain or soft stop, and only promote the replication slot when actually drained. The processStream.func3 goleak ignore is removed from the postgresql TestMain; TestIntegrationPostgresSnapshotAckBarrier (which triggers exactly the previously-leaking path) passes under the stricter check. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…paths Review on #4721 noted two of the leak fixes had no test exercising them, so the package goleak gate could not catch a regression: - TestProtobufBSRPartialModuleFailureStopsWatchers: two-module config where the first module's watcher starts against the mock BSR server and the second fails module parsing, covering the deferred close() guard in newMultiModuleWatcher. - TestProtobufBSRAwaitReadyFailureStopsWatcher: unreachable BSR endpoint, covering the watcher.Stop() in newSchemaWatcher's AwaitReady error branch. watcherTimeout becomes a var so the test can shorten it to 250ms — prototransform retries until the constructor context dies, so with the const the failure always took the full 10s. Both tests verified negatively: removing either fix makes the package run fail with the corresponding leaked prototransform goroutine. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…up-module watcher orphaning From a high-effort review pass over this branch: postgres_cdc: after the snapshot handoff was abandoned on soft stop, the trailing counter re-check could still call MarkSnapshotAcknowledged when teardown nacks drained the counter (reachable with auto_replay_nacks: false, where nacks hit the raw ackFn), promoting the replication slot over rows never durably delivered and skipping the snapshot on restart. Mark is now gated on a drain observed via the drain channel, never the soft-stop exit. Also restore the WaitGroup's lost double-settle safety net: snapshotAckDone panics if the counter goes negative instead of silently disabling the drain signal forever. protobuf: two bsr entries naming the same module silently overwrote the first watcher in bsrClients, leaving its running poll goroutine unreachable by close(). Duplicate modules are now a config error (the construction-failure guard stops the earlier watchers); covered by TestProtobufBSRDuplicateModuleStopsWatchers, negatively verified against the goleak gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review findings against the new advisory race job: - Classify failures by grepping output for 'WARNING: DATA RACE': only an actual race in a blocking-listed package can exit 1. Build errors, flakes, and per-package timeouts are advisory warnings even for blocking packages (the ordinary unit-test workflow gates those). - Write each package's summary row incrementally and cap the run at MAX_PACKAGES=8 with a logged advisory truncation, with job timeout-minutes raised above the composed worst case, so a job-level kill can never turn a wide PR red without a race. - Pass github.base_ref via env and quote it (git ref names may contain shell metacharacters), and split merge-base/diff into checked statements under pipefail so git failures fail the step loudly instead of silently scoping to zero packages. integration_test.yml carries the same two inherited flaws; fixing it (or extracting a shared composite action) is left as a follow-up to keep this PR from touching working CI. - test:unit-race now accepts package paths (task test:unit-race -- ./internal/impl/foo/) and mirrors the CI flags (-count=1 -race -shuffle=on -timeout 10m) so the documented promotion baseline is actually executable and immune to the test cache; CONTRIBUTING.md shows the scoped invocation. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review on #4721: tr '\n' ' ' turned the blank-input case into a lone space, which survives command substitution, so the 'filters != empty' guards never skipped — every docs-only PR still ran checkout with fetch-depth 0, installed Go, and posted an empty results table. Join the package list with xargs instead, which emits an empty string on blank input (verified locally for both the empty and real-diff cases). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…napshot barrier Follow-up review on #4721: gating MarkSnapshotAcknowledged on the drain channel still conflated acks with nacks — ackFn discarded its error, so with auto_replay_nacks: false a downstream that permanently rejects the final snapshot batch drained the counter, fired the drain, and promoted the slot over rows never durably delivered (deterministically, no soft stop needed). The soft-stop-vs-drain select race had the same hole. ackFn now records a sticky snapshotAckFailed flag before the counter decrement, and the handoff promotes only on a clean drain; a nack drain logs and soft-stops so the stream restarts and re-runs the snapshot, matching the neighboring flush-failure paths. The flag resets on Connect (a fresh snapshot re-reads the rows); the pending counter deliberately does not (late cross-epoch settles still decrement it). Adds TestIntegrationPostgresSnapshotNackBarrier: auto_replay_nacks false, the whole snapshot in one batch, consumer nacks it — asserts the slot is never promoted and the snapshot re-runs on restart. Negatively verified: with the fix reverted the test fails with the slot promoted. Co-Authored-By: Claude Fable 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.
Part of CON-179 (test hardening). This PR adds two complementary checks: goroutine-leak detection inside the test suites of selected connector packages, and a race-detector CI job that starts out advisory and gains teeth package by package.
1. Goroutine-leak detection (goleak)
Adds
go.uber.org/goleakand wires it into a package-levelTestMainfor four packages:internal/impl/mysql,internal/impl/postgresql,internal/impl/protobuf, andinternal/impl/sql. After the last test in the package finishes, goleak fails the run if any goroutine started during the tests is still alive.How it runs:
TestMainis Go's standard per-package entry point, so this needs no CI wiring. It fires on everygo testof these packages, which means every PR (via the existingtest.ymlunit suite) and every nightly integration run. A leak is a hard test failure.Ignore policy: each
TestMaincarries only narrow, individually commented ignores for goroutines we cannot stop from test code — after review, the sole remaining example is the license service's hourly expiry-metric loop started byInjectTestService. Broad ignores are deliberately avoided; each entry names the library and the reason. (Earlier revisions ignored the prototransformSchemaWatcherpoll loop andnet/httppersistConn goroutines; review showed those masked real component leaks, which are now fixed instead — see below.)Real leaks fixed along the way:
protobuf(BSR mode): every processor construction started oneprototransform.SchemaWatcherpoll loop per module with no stop path —protobufProc.Closewas a no-op — so stream restarts/config reloads accumulated goroutines for the life of the process. Watchers are now stopped fromCloseand on every construction-failure path (operator error after watchers start, partial multi-module failure,AwaitReadyfailure), and duplicatebsrmodule entries are rejected instead of silently orphaning the first module's watcher. Each path has a regression test guarded by goleak.postgres_cdc: the snapshot→stream handoff parked a goroutine onsnapshotAckWG.Wait(); a soft stop with unacked snapshot batches abandoned it forever. The WaitGroup is replaced by an atomic pending counter plus a drain-notification channel, so the handoff waits directly on drain-or-soft-stop with no helper goroutine, and the former ignore for it is removed.MarkSnapshotAcknowledgedfires only when every snapshot batch settled via ack: a sticky nack flag (reachable withauto_replay_nacks: false, where raw nacks hit the ack function) leaves the slot unpromoted and restarts the stream so the snapshot re-runs, covered by a nack-barrier integration test; a double-settle now panics like the WaitGroup used to instead of silently disabling the drain signal.TestIntegrationPostgresSnapshotAckBarrierblocked a consumer goroutine on a context that benthos never cancels (consumer funcs receivecontext.Background()), which leaked the goroutine and the unacked stream behind it past the end of the test. The test now uses its own cancellable context for the simulated crash and explicitly stops the crashed stream withStopWithin. The protobuf mock BSR server also gets a properShutdownon cleanup.All four packages were verified leak-clean on repeated local runs before the check was enabled, so this cannot fail a PR on a pre-existing condition.
2. Advisory race-detector CI job
New workflow
.github/workflows/race_test.yml, plus a promotion list at.github/race-blocking-packages.txt.How it runs: on every PR the job diffs against the merge-base and collects the
internal/impl/<pkg>directories the PR touched (same auto-scoping asintegration_test.yml, which keeps the 2-10x race-detector runtime cost bounded). If no connector package changed, the job skips entirely. Otherwise it runsgo test -count=1 -race -shuffle=on -timeout 10mper touched package and writes a pass/fail table to the job summary. Aworkflow_dispatchtrigger allows manual runs against any package by name.Advisory-first design: only an actual data race (
WARNING: DATA RACEin the test output) in a package listed inrace-blocking-packages.txtfails the check. Everything else — races in unlisted packages, and build errors/flakes/timeouts anywhere (the ordinary unit-test workflow gates those) — produces a warning annotation and the check stays green. Summary rows are written incrementally and the job caps itself at 8 packages per run (excess logged as an advisory skip) so a job-level timeout kill can never turn a wide PR red without a race. That file starts empty, so on day one nothing can go red from this job. A package is promoted to blocking only after it holds a green-racebaseline (repeated clean scoped runs plus a green advisory run in CI). The invariant this preserves: a blocking gate must never be able to fail a PR on a pre-existing race the PR did not introduce.Supporting changes
CONTRIBUTING.md+taskfiles/test.yml:task test:unit-racenow accepts package paths (task test:unit-race -- ./internal/impl/<component>/...) and mirrors the CI job's flags (-count=1 -race -shuffle=on), so the documented promotion baseline is executable and immune to the test cache; contributors are asked to run it scoped to packages they touch, with a pointer to the blocking-list semantics..claude/agents/tester.md: recipe for wiring goleak into a new package, including the ignore policy and the requirement to verify against a clean baseline before merging.go.mod: addsgo.uber.org/goleak v1.3.0(test-only dependency).🤖 Generated with Claude Code