Skip to content

test(events): synchronize saturated pool cancellation - #6852

Open
chenrui333 wants to merge 1 commit into
mainfrom
test/saturated-pool-cancellation
Open

test(events): synchronize saturated pool cancellation#6852
chenrui333 wants to merge 1 commit into
mainfrom
test/saturated-pool-cancellation

Conversation

@chenrui333

Copy link
Copy Markdown
Member

What

Synchronize the saturated-pool cancellation test using worker events and synctest.

Why

The test must cancel while workers occupy the pool, without relying on scheduler timing.

Testing

100 race-enabled repetitions and the full events package, including -race, pass.

Copilot AI lite review requested due to automatic review settings September 7, 2026 06:45
@github-actions github-actions Bot added go Pull requests that update Go code size/s labels Sep 7, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The updated test can leak/hang under synctest if a require.* assertion fails before unblocking worker goroutines; it should close the release channel via t.Cleanup (with sync.Once) to ensure cleanup on early exits.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates the server/events test suite to make the “saturated worker pool cancellation” scenario deterministic by synchronizing goroutine progress using worker events and testing/synctest, rather than relying on scheduler timing.

Changes:

  • Reworks TestRunProjectCmdsParallel_CancelledWhileExceedingPoolSize to block the first two workers explicitly and trigger cancellation while the submitter is blocked in wg.Add().
  • Uses synctest.Wait() to ensure the test reaches the intended blocked state before cancelling and unblocking.
File summaries
File Description
server/events/project_command_pool_executor_test.go Makes the saturated-pool cancellation test deterministic using synctest and worker synchronization channels.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread server/events/project_command_pool_executor_test.go
@github-code-quality

github-code-quality Bot commented Sep 7, 2026

Copy link
Copy Markdown

Code Coverage Overview

Languages: Go

Go / code-coverage/go

The overall line coverage in commit c3d7b48 in the test/saturated-pool-... branch is 73%. Line coverage data for the main branch is not yet available.

Show a line coverage summary of the most covered files.
File main test/saturated-pool-... c3d7b48 +/-
server/events/v...ithub/client.go 88%
server/core/ter...aform_client.go 88%
server/controll...i_controller.go 83%
server/events/p...mand_builder.go 83%
server/events/v...itlab/client.go 80%
server/events/p...mmand_runner.go 76%
server/events/event_parser.go 76%
server/events/working_dir.go 73%
server/server.go 70%
server/controll...s_controller.go 64%

Updated September 07, 2026 23:55 UTC

@chenrui333
chenrui333 force-pushed the test/saturated-pool-cancellation branch 2 times, most recently from 769bb12 to 575581b Compare September 7, 2026 09:16
Block active workers and wait for the submission loop to exhaust the pool before cancelling. Verify which projects finish and which are cancelled without wall-clock scheduling assumptions.

Assisted-by: OpenAI
Signed-off-by: Rui Chen <rui@chenrui.dev>
@chenrui333
chenrui333 force-pushed the test/saturated-pool-cancellation branch from 575581b to c3d7b48 Compare September 7, 2026 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

go Pull requests that update Go code size/s

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants