test(export): add multithread safety tests for system and disk driver by RUPESHKUMARPALLAI · Pull Request #4409 · open-policy-agent/gatekeeper

RUPESHKUMARPALLAI · 2026-02-26T14:35:49Z

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #4024

Special notes for your reviewer:

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Signed-off-by: RUPESHKUMARPALLAI <rkp13@iitbbs.ac.in>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

codecov-commenter · 2026-02-27T00:40:08Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 43.96%. Comparing base (3350319) to head (60c40e3).
⚠️ Report is 610 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (3350319) and HEAD (60c40e3). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (3350319) HEAD (60c40e3)

unittests 2 1

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #4409       +/-   ##
===========================================
- Coverage   54.49%   43.96%   -10.54%     
===========================================
  Files         134      282      +148     
  Lines       12329    20522     +8193     
===========================================
+ Hits         6719     9022     +2303     
- Misses       5116    10733     +5617     
- Partials      494      767      +273

Flag	Coverage Δ
unittests	`43.96% <ø> (-10.54%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

JaydipGabani

Thanks for picking this up.

JaydipGabani · 2026-02-27T20:32:16Z

Add a test that exercises Writer.CloseConnection concurrently with Writer.retryFailedConnections through the actual Writer

TestCloseConnectionConcurrentWithRetry added

JaydipGabani · 2026-02-27T20:35:02Z

+type concurrentTestDriver struct {
+	mu              sync.Mutex
+	publishCalls    int
+	createCalls     int
+	updateCalls     int
+	closeCalls      int
+	failPublish     bool
+	failCreate      bool
+	failUpdate      bool
+	failClose       bool
+}


Unused Counter Fields and Failure Flags

type concurrentTestDriver struct { mu sync.Mutex publishCalls int // incremented but NEVER read/asserted createCalls int // incremented but NEVER read/asserted updateCalls int // incremented but NEVER read/asserted closeCalls int // incremented but NEVER read/asserted failPublish bool // NEVER set to true failCreate bool // NEVER set to true failUpdate bool // NEVER set to true failClose bool // NEVER set to true }

Either:

Assert the counters after wg.Wait() (e.g., assert.Greater(t, testDrv.createCalls, 0)) to verify work actually happened

Enable failure flags in a second subtest to exercise error-path concurrency

Remove them if they're not needed — dead instrumentation suggests the test is incomplete

Removed not needed flags

JaydipGabani · 2026-02-27T20:37:31Z

+	return nil
+}
+
+func TestSystem_ConcurrentAccess(t *testing.T) {


assert testDrv.publishCalls > 0 after the test to confirm at least some publishers reached the driver. Or track success/failure counts separately

JaydipGabani · 2026-02-27T20:42:21Z

+			}()
+		}
+
+		wg.Wait()


The assertions RetryCount > 0 and NextRetryAt.After(now) pass because goroutine 1 already set them. A single-call test produces identical results. This test doesn't exercise concurrency at all.

What actually happens at runtime:

Step What runs Effect

Goroutine 1 acquires lock Retries all 5 connections; all fail. Sets RetryCount=1, NextRetryAt = now + ~30s Real work done

Goroutine 2 acquires lock Finds now.Before(failedConn.NextRetryAt) → true for all 5 → skips all No-op

Goroutines 3–8 Same as goroutine 2 All no-ops

updated to simulate through actual writer

… resolve comments Signed-off-by: RUPESHKUMARPALLAI <rkp13@iitbbs.ac.in>

JaydipGabani · 2026-03-03T01:25:02Z

Here is the fix - #4422 for data races surfaced by this PR in the CI.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Copilot · 2026-03-04T19:08:43Z

+		}
+
+		wg.Wait()
+


Because closeAndRemoveFilesWithRetry is forced to fail, CloseConnection will start backgroundCleanup() via cleanupOnce.Do(...). The test never stops that goroutine (e.g., by closing cleanupDone / marking cleanupStopped), which can leak goroutines across the package test run and introduce nondeterminism if the ticker fires. Consider explicitly stopping background cleanup during test teardown.

Suggested change

// Ensure background cleanup goroutine is stopped to avoid leaking

// goroutines across tests when CloseConnection starts backgroundCleanup.

cleanupStopped = true

@RUPESHKUMARPALLAI can you address this?

Copilot · 2026-03-04T19:08:43Z

+	// Ensure at least one publish reached the driver so we know publishers
+	// were able to observe a usable connection during concurrent access.
+	testDrv.mu.Lock()
+	publishCount := testDrv.publishCalls
+	testDrv.mu.Unlock()
+	if publishCount == 0 {
+		t.Fatalf("expected at least one successful Publish call, got %d", publishCount)
+	}


publishCount == 0 can be a flaky failure because publishers may all run before any UpsertConnection has installed the mapping (or only during windows when CloseConnection has removed it). Consider creating the connection once before starting publishers/closers, or use a start barrier/retry loop with a short timeout so at least one publish is guaranteed to observe an initialized connection.

@RUPESHKUMARPALLAI can you address this?

github-actions · 2026-06-19T00:50:12Z

This PR has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

RUPESHKUMARPALLAI requested a review from a team as a code owner February 26, 2026 14:35

Copilot AI review requested due to automatic review settings February 26, 2026 14:35

Copilot started reviewing on behalf of RUPESHKUMARPALLAI February 26, 2026 14:36 View session

Copilot AI reviewed Feb 26, 2026

test(export): add multithread safety tests for system and disk driver

60c40e3

Signed-off-by: RUPESHKUMARPALLAI <rkp13@iitbbs.ac.in>

RUPESHKUMARPALLAI force-pushed the add-multithread-export-tests-4024 branch from 8c36408 to 60c40e3 Compare February 26, 2026 14:42

RUPESHKUMARPALLAI mentioned this pull request Feb 26, 2026

Add tests for multi thread safety #4024

Open

JaydipGabani requested a review from Copilot February 26, 2026 21:38

Copilot started reviewing on behalf of JaydipGabani February 26, 2026 21:38 View session

Copilot AI reviewed Feb 26, 2026

View reviewed changes

JaydipGabani requested changes Feb 27, 2026

View reviewed changes

test(export): add multithread safety tests for system and disk driver…

b7c9774

… resolve comments Signed-off-by: RUPESHKUMARPALLAI <rkp13@iitbbs.ac.in>

RUPESHKUMARPALLAI requested a review from JaydipGabani February 27, 2026 22:04

Merge branch 'master' into add-multithread-export-tests-4024

dd1d671

Copilot AI review requested due to automatic review settings March 4, 2026 19:04

Copilot started reviewing on behalf of JaydipGabani March 4, 2026 19:04 View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

JaydipGabani mentioned this pull request Mar 9, 2026

fix: harden disk exporter concurrency #4422

Open

github-actions Bot added the stale label Jun 19, 2026

JaydipGabani removed the stale label Jun 19, 2026

Step	What runs	Effect
Goroutine 1 acquires lock	Retries all 5 connections; all fail. Sets RetryCount=1, NextRetryAt = now + ~30s	Real work done
Goroutine 2 acquires lock	Finds now.Before(failedConn.NextRetryAt) → true for all 5 → skips all	No-op
Goroutines 3–8	Same as goroutine 2	All no-ops

+		// Ensure background cleanup goroutine is stopped to avoid leaking
+		// goroutines across tests when CloseConnection starts backgroundCleanup.
+		cleanupStopped = true

Uh oh!

Conversation

RUPESHKUMARPALLAI commented Feb 26, 2026 • edited by JaydipGabani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

codecov-commenter commented Feb 27, 2026

Codecov Report

Uh oh!

JaydipGabani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JaydipGabani commented Mar 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RUPESHKUMARPALLAI commented Feb 26, 2026 •

edited by JaydipGabani

Loading