Skip to content

Treat concurrent default-group creation as success - #6668

Open
lorenzozanee wants to merge 1 commit into
stacklok:mainfrom
lorenzozanee:restore/pr-6457
Open

lorenzozanee wants to merge 1 commit into
stacklok:mainfrom
lorenzozanee:restore/pr-6457

Conversation

@lorenzozanee

Copy link
Copy Markdown
Contributor

Summary

  • Starting several thv processes concurrently could fail with failed to ensure default group exists: group already exists: default when the processes raced the Exists-then-Create window in the default-group startup migration. This is reproducible in the core E2E shard, which runs Ginkgo with PROCS=4.
  • The migration now treats groups.ErrGroupAlreadyExists as success: when another process created the default group first, the desired end state already holds. Explicit group create calls (CLI and the API 409 response) still report the conflict.
  • --help, -h and help are now classified as informational commands, so printing help no longer triggers migrations or the container runtime check.
  • CheckTHVBinaryAvailable now includes stderr in its error so startup failures are diagnosable instead of being masked as thv binary not available.

Fixes #6359

Type of change

  • Bug fix

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)
  • Manual testing (describe below)

New Test_EnsureDefaultGroupExistsConcurrent spawns 8 concurrent default-group migrations against a fresh shared store and requires all of them to succeed (it fails on main with group already exists: default). Manually verified: 10 rounds of 4 concurrent thv processes against a fresh shared XDG state directory all exit 0 (previously 6/10 rounds had at least one process fail), and thv --help on a fresh directory exits 0 without creating the default group or requiring a container runtime.

Does this introduce a user-facing change?

Yes. Concurrent thv startups that previously failed with group already exists: default now succeed, and thv --help no longer requires a container runtime or triggers migrations.

Supersedes #6457 (原PR分支已删,按原提交重建)

@codecov

codecov Bot commented Sep 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 79.08%. Comparing base (f3dfc9e) to head (61e6de7).

Files with missing lines Patch % Lines
pkg/migration/migration.go 80.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6668   +/-   ##
=======================================
  Coverage   79.08%   79.08%           
=======================================
  Files         785      785           
  Lines       78381    78385    +4     
=======================================
+ Hits        61989    61993    +4     
  Misses      16387    16387           
  Partials        5        5           

☔ View full report in Codecov by Harness.
📢 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.

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.

Handle concurrent default-group creation during startup

1 participant