Files
orca/internal/cli/concurrency_test.go
T
Jon Chery 0358efe95b fix(P07): concurrency safety — SQLite, flock, cache, atomic writes (REQ-156)
- SQLite busy_timeout(5000) + SetMaxOpenConns(1) on all 4 DSNs
- secrets file flock (concurrent set on same ns no longer loses data)
- upgrade lock file (refuse concurrent orca upgrade)
- backup lock file (refuse concurrent backup)
- cache invalidation by writes (read-after-write consistency)
- Executor.Run mutex scope fix (hold only for DB inserts)
- ns create/inherit/set-constraint atomic writeNSMdAtomic
- writeCurrentLead + rotateSSHKeys atomic
- consolidate 3 writeAtomic impls onto security.WriteAtomic
- WebAuthn session stores guarded with sync.Mutex

Tests: concurrent secrets set, upgrade lock rejection, cache
read-after-write, WebAuthn session thread-safety (pass under -race).

---ci---
project: orca
phase: 7
milestone: v0.13
status: complete
requirements:
  covered: [156]
---/ci---
2026-08-10 12:27:05 +00:00

423 lines
14 KiB
Go

package cli
// concurrency_test.go covers the REQ-156 / P07 concurrency-safety
// fixes:
//
// - T11: concurrent `secrets set` on the same namespace preserves all
// keys (the flock serializes the read-modify-write so no key is
// lost to a clobbering second writer).
// - T12: a second `orca upgrade` invoked while the first is running
// is rejected with "upgrade already in progress".
// - T13: cache invalidation read-after-write - `node join` followed
// by an immediate `node list` (with a populated stale cache) shows
// the new node, not the stale cached list.
// - T14: (in internal/webauthn) concurrent BeginRegistration does
// not panic / race on the session map.
//
// These tests complement the per-fix unit tests in the relevant
// _test.go files; they specifically exercise the cross-cutting
// concurrency invariants the milestone hardens.
import (
"bytes"
"fmt"
"os"
"path/filepath"
"strings"
"sync"
"testing"
"time"
"git.cloudinit.dev/coreci/orca/internal/cache"
"git.cloudinit.dev/coreci/orca/internal/paths"
"git.cloudinit.dev/coreci/orca/internal/secrets"
)
// runCLI is a helper that resets root flags, wires a fresh output
// buffer, sets the given args, and runs rootCmd. Returns the captured
// output. The buffer must be wired AFTER resetRootFlags (which sets
// its own buffer).
func runCLI(t *testing.T, args ...string) (string, error) {
t.Helper()
resetRootFlags(t)
var buf bytes.Buffer
rootCmd.SetOut(&buf)
rootCmd.SetErr(&buf)
rootCmd.SetArgs(args)
err := rootCmd.Execute()
return buf.String(), err
}
// ---------------------------------------------------------------------------
// T11: concurrent secrets set preserves all keys
// ---------------------------------------------------------------------------
// TestSecretsConcurrentSetPreservesAllKeys runs 5 concurrent
// `orca secrets set` invocations against the SAME namespace, each
// setting a distinct key. Without the flock (P07 T2) the second writer
// would load-then-save and clobber the first, losing a key. With the
// flock all 5 keys must be present afterward.
//
// The cobra rootCmd is a package global and is NOT goroutine-safe
// (shared flag state), so we drive the secrets-set RunE body directly
// under real concurrency. This exercises the lockNSSecrets flock +
// loadMasterAndNSSecrets + saveNSSecrets path that the RunE uses.
func TestSecretsConcurrentSetPreservesAllKeys(t *testing.T) {
ns := "concsetns"
setupSecretsTestEnv(t, ns)
const n = 5
keys := make([]string, n)
for i := 0; i < n; i++ {
keys[i] = fmt.Sprintf("KEY_%d", i)
}
var wg sync.WaitGroup
errs := make([]error, n)
for i := 0; i < n; i++ {
wg.Add(1)
go func(idx int) {
defer wg.Done()
// Replicate the secretsSetCmd RunE body under real
// concurrency: lock -> load -> mutate -> save. The lock
// serializes the read-modify-write so concurrent sets do
// not clobber each other.
release, err := lockNSSecrets(ns)
if err != nil {
errs[idx] = fmt.Errorf("lock: %w", err)
return
}
defer release()
nsKey, lines, err := loadMasterAndNSSecrets(ns)
if err != nil {
errs[idx] = err
return
}
defer secrets.ZeroKey(nsKey)
key := keys[idx]
value := fmt.Sprintf("value_%d", idx)
newLine := key + "=" + value
j := findKeyIndex(lines, key)
if j >= 0 {
lines[j] = newLine
} else {
lines = append(lines, newLine)
}
errs[idx] = saveNSSecrets(ns, nsKey, lines)
}(i)
}
wg.Wait()
for i, err := range errs {
if err != nil {
t.Fatalf("goroutine %d: %v", i, err)
}
}
// All 5 keys must be present.
out, err := runCLI(t, "secrets", "list", ns)
if err != nil {
t.Fatalf("secrets list: %v", err)
}
for _, k := range keys {
if !strings.Contains(out, k) {
t.Errorf("key %q missing after concurrent set (flock did not serialize): %s", k, out)
}
}
}
// TestSecretsConcurrentSetViaCLI is the cobra-driven variant. cobra's
// rootCmd is not goroutine-safe (shared flag globals), so we serialize
// the Execute() calls. This still exercises the flock because the
// load+save happens inside RunE. Confirms the CLI path itself (with
// flock) does not lose keys under repeated serial sets.
func TestSecretsConcurrentSetViaCLI(t *testing.T) {
ns := "conccli"
setupSecretsTestEnv(t, ns)
const n = 5
for i := 0; i < n; i++ {
if _, err := runCLI(t, "secrets", "set", ns, fmt.Sprintf("K_%d=v_%d", i, i)); err != nil {
t.Fatalf("secrets set %d: %v", i, err)
}
}
out, err := runCLI(t, "secrets", "list", ns)
if err != nil {
t.Fatalf("secrets list: %v", err)
}
for i := 0; i < n; i++ {
k := fmt.Sprintf("K_%d", i)
if !strings.Contains(out, k) {
t.Errorf("key %q missing after serial CLI sets: %s", k, out)
}
}
}
// ---------------------------------------------------------------------------
// T12: concurrent upgrade rejection
// ---------------------------------------------------------------------------
// TestUpgradeConcurrentLockRejected verifies that a second upgrade
// invocation while the first holds the upgrade.lock is rejected with
// "upgrade already in progress".
func TestUpgradeConcurrentLockRejected(t *testing.T) {
setupUpgradeTest(t)
resetUpgradeFlags()
// Manually create the upgrade.lock as if a first upgrade is in
// progress (the lock file content is just diagnostic; its
// EXISTENCE is what blocks the second caller via O_CREATE|O_EXCL).
lockPath := filepath.Join(paths.ClusterDir(), "upgrade.lock")
if err := os.MkdirAll(filepath.Dir(lockPath), 0o755); err != nil {
t.Fatalf("mkdir cluster: %v", err)
}
if err := os.WriteFile(lockPath, []byte("pid=999 started=2026-01-01T00:00:00Z\n"), 0o600); err != nil {
t.Fatalf("write lock: %v", err)
}
defer os.Remove(lockPath)
// A dry-run upgrade must now be rejected because the lock exists.
_, err := runCLI(t, "upgrade", "--to", "v0.11.0", "--dry-run")
if err == nil {
t.Fatal("upgrade with stale lock should fail, got nil")
}
if !strings.Contains(err.Error(), "upgrade already in progress") {
t.Errorf("unexpected error: %v", err)
}
}
// TestUpgradeLockReleasedOnSuccess verifies the upgrade.lock is
// removed after a successful (dry-run) upgrade so a subsequent upgrade
// is not blocked by a stale lock.
func TestUpgradeLockReleasedOnSuccess(t *testing.T) {
setupUpgradeTest(t)
setupUpgradeTestWithMocks(t)
rootCmd.SetArgs([]string{"upgrade", "--to", "v0.11.0", "--dry-run"})
if err := rootCmd.Execute(); err != nil {
t.Fatalf("upgrade dry-run: %v", err)
}
lockPath := filepath.Join(paths.ClusterDir(), "upgrade.lock")
if _, err := os.Stat(lockPath); err == nil {
t.Errorf("upgrade.lock still exists after successful dry-run (not released): %s", lockPath)
}
}
// TestUpgradeLockReleasedOnError verifies the lock is released even
// when the upgrade fails mid-run (the defer in runUpgrade covers the
// error path).
func TestUpgradeLockReleasedOnError(t *testing.T) {
setupUpgradeTest(t)
setupUpgradeTestWithMocks(t)
// Force a failure: --to with a version that triggers a cutover
// whose verification fails. The runner reports :443 (cutover
// needed) and the http check returns 502 (verification fail).
runner := &mockUpgradeRunner{
outputs: map[string][]byte{
"ss -tlnp": []byte(":443"),
},
}
upgradeRunnerOverride = runner
httpClientOverride = func(url string) (int, error) { return 502, nil }
rootCmd.SetArgs([]string{"upgrade", "--to", "v0.11.0"})
_ = rootCmd.Execute() // expected to fail
lockPath := filepath.Join(paths.ClusterDir(), "upgrade.lock")
if _, err := os.Stat(lockPath); err == nil {
t.Errorf("upgrade.lock still exists after failed upgrade (not released on error): %s", lockPath)
}
}
// ---------------------------------------------------------------------------
// T13: cache invalidation read-after-write
// ---------------------------------------------------------------------------
// TestCacheInvalidationNodeJoinReadAfterWrite verifies that after
// `node join` invalidates the `nodes` cache class, an immediate
// `node list` (which would otherwise serve a STALE cached list) shows
// the just-joined node.
//
// Setup: populate the cache with a stale nodes list (missing the new
// node). Without T5's invalidation, the second `node list` would serve
// the stale list and the new node would be invisible until the TTL
// expired. With T5, the join invalidates the class and the list
// re-reads from the DB.
func TestCacheInvalidationNodeJoinReadAfterWrite(t *testing.T) {
_, cleanup := initTestEnv(t)
defer cleanup()
// Seed the cache with a stale nodes list (a sentinel node that
// does NOT exist in the DB). The TTL is long so it would be
// served on a subsequent list without invalidation.
c, err := cache.Open(paths.CacheDB())
if err != nil {
t.Fatalf("open cache: %v", err)
}
stale := `[{"id":"stale-id","name":"stale-node","address":"10.0.0.99:8443","state":"ready"}]`
if err := c.Set(cacheNodeClass, cacheListKey, []byte(stale), 10*time.Minute); err != nil {
t.Fatalf("set stale cache: %v", err)
}
c.Close()
// Confirm the stale entry is served by a fresh list (proving the
// cache is populated and would be hit).
staleOut, err := runCLI(t, "node", "list")
if err != nil {
t.Fatalf("stale node list: %v", err)
}
if !strings.Contains(staleOut, "stale-node") {
t.Fatalf("precondition: stale cache not served: %s", staleOut)
}
// Join a real node. T5 invalidates the `nodes` cache class.
if _, err := runCLI(t, "node", "join", "--name", "freshnode", "--addr", "10.0.0.42:8443"); err != nil {
t.Fatalf("node join: %v", err)
}
// Immediate list: the stale sentinel must be GONE (invalidated)
// and the real fresh node must be present (read from the DB).
out, err := runCLI(t, "node", "list")
if err != nil {
t.Fatalf("node list after join: %v", err)
}
if strings.Contains(out, "stale-node") {
t.Errorf("stale cache still served after join (invalidation missing): %s", out)
}
if !strings.Contains(out, "freshnode") {
t.Errorf("fresh node missing from list after join (cache not re-read): %s", out)
}
}
// TestCacheInvalidationNSCreateReadAfterWrite is the ns variant: a
// stale `namespaces` cache is invalidated by `ns create` so the next
// `ns list` shows the new namespace.
func TestCacheInvalidationNSCreateReadAfterWrite(t *testing.T) {
root := t.TempDir()
t.Setenv("ORCA_HOME", root)
writeDefaultsNS(t, root)
// Seed a stale namespaces cache containing only _defaults.
c, err := cache.Open(paths.CacheDB())
if err != nil {
t.Fatalf("open cache: %v", err)
}
stale := `[{"name":"_defaults","path":"` + filepath.Join(root, "_defaults") + `","default":true}]`
if err := c.Set(cacheNamespaceClass, cacheListKey, []byte(stale), 10*time.Minute); err != nil {
t.Fatalf("set stale: %v", err)
}
c.Close()
// Confirm stale served.
resetRootFlags(t)
resetNSFlags()
staleOut, err := runCLI(t, "ns", "list")
if err != nil {
t.Fatalf("stale ns list: %v", err)
}
if !strings.Contains(staleOut, "_defaults") {
t.Fatalf("precondition: stale ns cache not served: %s", staleOut)
}
// Create a new namespace. T5 invalidates the `namespaces` cache.
resetRootFlags(t)
resetNSFlags()
if _, err := runCLI(t, "ns", "create", "newns"); err != nil {
t.Fatalf("ns create: %v", err)
}
// Immediate list: must show the new namespace (read from disk,
// not the stale cache).
resetRootFlags(t)
resetNSFlags()
out, err := runCLI(t, "ns", "list")
if err != nil {
t.Fatalf("ns list after create: %v", err)
}
if !strings.Contains(out, "newns") {
t.Errorf("new namespace missing from list after create (cache not invalidated/re-read): %s", out)
}
}
// TestCacheInvalidationJobRunReadAfterWrite verifies `job run`
// invalidates the `jobs` cache so a stale cached job list is not
// served after a new job runs.
func TestCacheInvalidationJobRunReadAfterWrite(t *testing.T) {
_, cleanup := initTestEnv(t)
defer cleanup()
// Seed a stale jobs cache (a sentinel job that does not exist).
c, err := cache.Open(paths.CacheDB())
if err != nil {
t.Fatalf("open cache: %v", err)
}
stale := `[{"id":"stale-job","name":"stale","status":"complete","exit_code":0}]`
if err := c.Set(cacheJobClass, cacheListKey, []byte(stale), 10*time.Minute); err != nil {
t.Fatalf("set stale: %v", err)
}
c.Close()
// Confirm stale served.
staleOut, err := runCLI(t, "job", "list")
if err != nil {
t.Fatalf("stale job list: %v", err)
}
if !strings.Contains(staleOut, "stale") {
t.Fatalf("precondition: stale job cache not served: %s", staleOut)
}
// Write a job spec and run it. T5 invalidates the `jobs` cache.
specDir := t.TempDir()
specPath := filepath.Join(specDir, "job.md")
specBody := "---\n" +
"kind: Job\n" +
"name: cacheinv-job\n" +
"runtime:\n" +
" one_of: process\n" +
" command: /bin/true\n" +
"---\n# cacheinv\n\nRuns /bin/true.\n"
if err := os.WriteFile(specPath, []byte(specBody), 0o644); err != nil {
t.Fatalf("write spec: %v", err)
}
if _, err := runCLI(t, "job", "run", specPath); err != nil {
t.Fatalf("job run: %v", err)
}
// Immediate list: the stale sentinel must be gone; the real job
// must be present (read from the DB).
out, err := runCLI(t, "job", "list")
if err != nil {
t.Fatalf("job list after run: %v", err)
}
if strings.Contains(out, "stale-job") {
t.Errorf("stale job cache still served after run (invalidation missing): %s", out)
}
if !strings.Contains(out, "cacheinv-job") {
t.Errorf("new job missing from list after run (cache not re-read): %s", out)
}
}
// TestCacheInvalidateHelperDirectly is a small unit test for the
// cacheInvalidate helper itself: it confirms a populated class is
// empty after the helper runs.
func TestCacheInvalidateHelperDirectly(t *testing.T) {
_, cleanup := initTestEnv(t)
defer cleanup()
c, err := cache.Open(paths.CacheDB())
if err != nil {
t.Fatalf("open: %v", err)
}
if err := c.Set(cacheNodeClass, cacheListKey, []byte("x"), 0); err != nil {
t.Fatalf("set: %v", err)
}
c.Close()
cacheInvalidate(cacheNodeClass)
c2, err := cache.Open(paths.CacheDB())
if err != nil {
t.Fatalf("reopen: %v", err)
}
defer c2.Close()
if _, _, err := c2.Get(cacheNodeClass, cacheListKey); err == nil {
t.Errorf("nodes/list still present after cacheInvalidate")
}
}