Skip to content

fix: launchWorkers cancelling other tenants' bundle workers - #427

Merged
srenatus merged 1 commit into
open-policy-agent:mainfrom
yi-chen-roger:ychen/fix-multitenant-worker-retire
Sep 24, 2026
Merged

srenatus merged 1 commit into
open-policy-agent:mainfrom
yi-chen-roger:ychen/fix-multitenant-worker-retire

Conversation

@yi-chen-roger

@yi-chen-roger yi-chen-roger commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #422

Changes

  • Gather every tenant's bundles/sources/stacks first, then build one combined active-bundle set across all tenants before retiring any worker, instead of retiring against a single tenant's set while iterating tenants (which cancelled other tenants' still-valid workers as soon as a second tenant was onboarded).
  • Stop clobbering s.failures on each tenant loop iteration so failures from earlier tenants aren't dropped.
  • Extract collectTenantWorkloads and retireStaleWorkers out of launchWorkers so each step has a name.
  • Add TestLaunchWorkers_MultiTenantDoesNotCancelOtherTenants, which reproduces the bug against the old code and passes with the fix.

Testing

  • go test ./pkg/service/... — all pass (one pre-existing, unrelated TestService/config_with_builtins failure in this sandbox because httptest can't bind a local port here; reproduces identically on main without this change).
  • go vet ./pkg/service/..., gofmt -l, golangci-lint run ./pkg/service/... — clean.

@yi-chen-roger
yi-chen-roger force-pushed the ychen/fix-multitenant-worker-retire branch from 9d2fbea to a9a16c7 Compare September 22, 2026 17:06
@yi-chen-roger yi-chen-roger changed the title service: don't cancel other tenants' workers when onboarding a new tenant fix: launchWorkers cancelling other tenants' bundle workers Sep 22, 2026
@yi-chen-roger
yi-chen-roger marked this pull request as ready for review September 22, 2026 19:15
@yi-chen-roger

Copy link
Copy Markdown
Contributor Author

@srenatus do you know who the Go Lint fails with message Error: The action 'Download dependencies' has timed out after 15 minutes.
how can I fix it in that case?

@srenatus srenatus 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.

I've rerun the lint test, it passed now.

Please have a look:

  1. The new test hangs for 30s about half the time. pkg/service/launchworkers_test.go:105 waits for allWorkersDone(), but a worker only checks configurationChanged() at the top of Execute. If it finished its build before the test called UpdateConfig, the pool has it scheduled 30s out (worker.go:23), so the test blocks that long. 2 of 5 -count=5 runs took 30.02s. Adding .WithSingleShot(true) to the test's New() fixes it — workers die() after one iteration, and die() doesn't close changed, so the configurationChanged() assertion still holds. A bounded wait instead of the unbounded loop would also be nice.

  2. failures is keyed by bare bundle name, not tenant_name. Now that the map spans all tenants (service.go:458, :509), two tenants with a same-named bundle collide. Same for s.report.Bundles[w.bundleConfig.Name] at service.go:298. Pre-existing and strictly less broken than before (the old code threw away all but the last tenant's failures), so fine as a follow-up issue rather than a blocker.

One behavior change worth being aware of: on a listing error the whole round is now skipped, so no tenant makes progress instead of the ones before the failure. In single-shot mode that means Run exits 0 with an empty report. It's the right call for the retire logic (partial activeBundles is exactly what caused the bug), just noting it.

@yi-chen-roger
yi-chen-roger force-pushed the ychen/fix-multitenant-worker-retire branch from 874d9c7 to 4835438 Compare September 23, 2026 19:08
@yi-chen-roger

Copy link
Copy Markdown
Contributor Author

I've rerun the lint test, it passed now.

Please have a look:

  1. The new test hangs for 30s about half the time. pkg/service/launchworkers_test.go:105 waits for allWorkersDone(), but a worker only checks configurationChanged() at the top of Execute. If it finished its build before the test called UpdateConfig, the pool has it scheduled 30s out (worker.go:23), so the test blocks that long. 2 of 5 -count=5 runs took 30.02s. Adding .WithSingleShot(true) to the test's New() fixes it — workers die() after one iteration, and die() doesn't close changed, so the configurationChanged() assertion still holds. A bounded wait instead of the unbounded loop would also be nice.
  2. failures is keyed by bare bundle name, not tenant_name. Now that the map spans all tenants (service.go:458, :509), two tenants with a same-named bundle collide. Same for s.report.Bundles[w.bundleConfig.Name] at service.go:298. Pre-existing and strictly less broken than before (the old code threw away all but the last tenant's failures), so fine as a follow-up issue rather than a blocker.
  1. Fixed — added .WithSingleShot(true) to the test's Service so a worker die()s right after its one iteration
  2. Agreed this is pre-existing and not a regression from this change

Fixes open-policy-agent#422

## Changes

- Gather every tenant's bundles/sources/stacks first, then build one combined active-bundle set across all tenants before retiring any worker, instead of retiring against a single tenant's set while iterating tenants (which cancelled other tenants' still-valid workers as soon as a second tenant was onboarded).
- Stop clobbering `s.failures` on each tenant loop iteration so failures from earlier tenants aren't dropped.
- Extract `collectTenantWorkloads` and `retireStaleWorkers` out of `launchWorkers` so each step has a name.
- Add `TestLaunchWorkers_MultiTenantDoesNotCancelOtherTenants`, which reproduces the bug against the old code and passes with the fix. Run with `WithSingleShot(true)` so a worker dies right after its one iteration instead of being rescheduled ~30s out, and the test's wait for shutdown is bounded instead of unbounded.

## Testing

- `go test ./pkg/service/... -run TestLaunchWorkers_MultiTenantDoesNotCancelOtherTenants -count=10` — all pass, ~0.02s each.
- `go test ./pkg/service/...` — all pass (one pre-existing, unrelated `TestService/config_with_builtins` failure in this sandbox because `httptest` can't bind a local port here; reproduces identically on `main` without this change).
- `go vet ./pkg/service/...`, `gofmt -l`, `golangci-lint run ./pkg/service/...` — clean.

Signed-off-by: Yi Chen <yi.chen.roger@gmail.com>
@yi-chen-roger
yi-chen-roger force-pushed the ychen/fix-multitenant-worker-retire branch from 4835438 to 72c8308 Compare September 23, 2026 20:59
@srenatus
srenatus merged commit 9e3b3e3 into open-policy-agent:main Sep 24, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Onboarding a tenant silently stops the previously-onboarded one building

2 participants