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---
This commit is contained in:
Vendored
+8
-1
@@ -58,10 +58,17 @@ func Open(path string) (*Cache, error) {
|
||||
if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {
|
||||
return nil, fmt.Errorf("create cache db dir: %w", err)
|
||||
}
|
||||
db, err := sql.Open("sqlite", path+"?_pragma=journal_mode(WAL)")
|
||||
// REQ-156 / P07 T1: busy_timeout(5000) so concurrent cache opens
|
||||
// (e.g. two `orca node list` invocations racing on the same shell)
|
||||
// wait up to 5s for the writer instead of failing immediately with
|
||||
// SQLITE_BUSY. SetMaxOpenConns(1) serializes the connections so the
|
||||
// busy_timeout is rarely needed but keeps the cache durable under
|
||||
// contention.
|
||||
db, err := sql.Open("sqlite", path+"?_pragma=journal_mode(WAL)&_pragma=busy_timeout(5000)")
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("open cache sqlite: %w", err)
|
||||
}
|
||||
db.SetMaxOpenConns(1)
|
||||
if err := db.Ping(); err != nil {
|
||||
_ = db.Close()
|
||||
return nil, fmt.Errorf("ping cache sqlite: %w", err)
|
||||
|
||||
+8
-25
@@ -175,32 +175,15 @@ func lockACL() (func(), error) {
|
||||
return security.Flock(paths.ACLPath() + ".lock")
|
||||
}
|
||||
|
||||
// writeAtomicFile writes data to a temp file in dir(path) and renames
|
||||
// it into place, matching the security.WriteAtomic pattern (P02 keeps
|
||||
// a local copy to avoid importing internal/security into the CLI).
|
||||
// writeAtomicFile writes data atomically (REQ-156, P07 T9).
|
||||
// Previously a local copy of the temp+chmod+rename pattern (P02 kept a
|
||||
// local copy to avoid importing internal/security); it lacked fsync,
|
||||
// so a crash between write and rename could promote a partially-durable
|
||||
// file. Now a thin wrapper around the canonical security.WriteAtomic
|
||||
// (temp + chmod + fsync + rename) so all CLI atomic writes share one
|
||||
// fsync-correct implementation.
|
||||
func writeAtomicFile(path string, data []byte, mode os.FileMode) error {
|
||||
dir := filepath.Dir(path)
|
||||
tmp, err := os.CreateTemp(dir, ".acl-tmp-*")
|
||||
if err != nil {
|
||||
return fmt.Errorf("create temp: %w", err)
|
||||
}
|
||||
tmpName := tmp.Name()
|
||||
defer func() { _ = os.Remove(tmpName) }()
|
||||
if _, err := tmp.Write(data); err != nil {
|
||||
_ = tmp.Close()
|
||||
return fmt.Errorf("write temp: %w", err)
|
||||
}
|
||||
if err := tmp.Chmod(mode); err != nil {
|
||||
_ = tmp.Close()
|
||||
return fmt.Errorf("chmod temp: %w", err)
|
||||
}
|
||||
if err := tmp.Close(); err != nil {
|
||||
return fmt.Errorf("close temp: %w", err)
|
||||
}
|
||||
if err := os.Rename(tmpName, path); err != nil {
|
||||
return fmt.Errorf("rename temp: %w", err)
|
||||
}
|
||||
return nil
|
||||
return security.WriteAtomic(path, mode, data)
|
||||
}
|
||||
|
||||
var aclGrantCmd = &cobra.Command{
|
||||
|
||||
@@ -12,6 +12,8 @@ package cli
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"time"
|
||||
|
||||
"github.com/spf13/cobra"
|
||||
@@ -29,6 +31,31 @@ var (
|
||||
restoreDryRun bool
|
||||
)
|
||||
|
||||
// acquireBackupLock atomically creates an exclusive lock file at
|
||||
// paths.ClusterDir()/backup.lock (REQ-156, P07 T4). Returns a release
|
||||
// function that MUST be deferred (it removes the lock file). If the
|
||||
// lock file already exists, returns an error "backup already in
|
||||
// progress" — preventing two concurrent `orca backup` invocations
|
||||
// from racing on the same ORCA_HOME (two tarballs being written from
|
||||
// the same source tree could produce inconsistent archives). O_CREATE
|
||||
// |O_EXCL is atomic under POSIX.
|
||||
func acquireBackupLock() (func(), error) {
|
||||
lockPath := filepath.Join(paths.ClusterDir(), "backup.lock")
|
||||
if err := os.MkdirAll(filepath.Dir(lockPath), 0o755); err != nil {
|
||||
return nil, fmt.Errorf("create cluster dir for backup lock: %w", err)
|
||||
}
|
||||
f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600)
|
||||
if err != nil {
|
||||
if os.IsExist(err) {
|
||||
return nil, fmt.Errorf("backup already in progress (lock file %s exists; remove it if stale)", lockPath)
|
||||
}
|
||||
return nil, fmt.Errorf("acquire backup lock: %w", err)
|
||||
}
|
||||
_, _ = f.WriteString(fmt.Sprintf("pid=%d started=%s\n", os.Getpid(), time.Now().UTC().Format(time.RFC3339)))
|
||||
_ = f.Close()
|
||||
return func() { _ = os.Remove(lockPath) }, nil
|
||||
}
|
||||
|
||||
var backupCmd = &cobra.Command{
|
||||
Use: "backup",
|
||||
Short: "Create a signed tar.gz backup of ORCA_HOME",
|
||||
@@ -44,6 +71,14 @@ written to --out; the hex-encoded signature to --out + ".sig".`,
|
||||
if err != nil {
|
||||
return fmt.Errorf("load master key: %w", err)
|
||||
}
|
||||
// REQ-156 / P07 T4: acquire an exclusive backup lock so two
|
||||
// concurrent `orca backup` invocations don't race on the same
|
||||
// ORCA_HOME (producing interleaved / inconsistent archives).
|
||||
backupRelease, err := acquireBackupLock()
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
defer backupRelease()
|
||||
out := backupOutPath
|
||||
if out == "" {
|
||||
ts := time.Now().UTC().Format("20060102-150405")
|
||||
|
||||
@@ -101,6 +101,27 @@ func cachePutList(class, key string, list any, ttl time.Duration) {
|
||||
cachePopulate(class, key, val, ttl)
|
||||
}
|
||||
|
||||
// cacheInvalidate drops all entries for the given cache class
|
||||
// (REQ-156, P07 T5). It is called after write operations (node
|
||||
// join/leave, ns create/delete, job run/stop) so the very next read
|
||||
// does not surface a stale cached list. Errors are logged but never
|
||||
// returned — a failed invalidation must not break the write command
|
||||
// (the cache entry will simply expire at its TTL).
|
||||
func cacheInvalidate(class string) {
|
||||
if !cacheAvailable() {
|
||||
return
|
||||
}
|
||||
c, err := cache.Open(paths.CacheDB())
|
||||
if err != nil {
|
||||
slog.Warn("cache: open failed during invalidate", "class", class, "err", err)
|
||||
return
|
||||
}
|
||||
defer c.Close()
|
||||
if err := c.Invalidate(class); err != nil {
|
||||
slog.Warn("cache: invalidate failed", "class", class, "err", err)
|
||||
}
|
||||
}
|
||||
|
||||
// Per-class TTLs (P00-T2).
|
||||
const (
|
||||
cacheNodeTTL = 30 * time.Second
|
||||
|
||||
@@ -0,0 +1,422 @@
|
||||
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")
|
||||
}
|
||||
}
|
||||
@@ -123,6 +123,9 @@ var jobRunCmd = &cobra.Command{
|
||||
return derr
|
||||
}
|
||||
res.unitPaths = unitPaths
|
||||
// REQ-156 / P07 T5: invalidate the jobs cache (the
|
||||
// dispatch decision records a local job entry).
|
||||
cacheInvalidate(cacheJobClass)
|
||||
if jsonOutput {
|
||||
return printJSON(map[string]any{
|
||||
"status": "deployed",
|
||||
@@ -149,6 +152,10 @@ var jobRunCmd = &cobra.Command{
|
||||
}
|
||||
runErr := exec.Run(ctx, job, workloadToTaskSpecs(spec))
|
||||
logDispatch(res, runErr)
|
||||
// REQ-156 / P07 T5: invalidate the jobs cache so the next
|
||||
// `orca job list` reflects the just-run (or just-failed)
|
||||
// job instead of a stale cached list.
|
||||
cacheInvalidate(cacheJobClass)
|
||||
if runErr != nil {
|
||||
if jsonOutput {
|
||||
_ = printJSON(map[string]any{"id": job.ID, "status": "failed", "error": runErr.Error()})
|
||||
@@ -316,6 +323,9 @@ var jobStopCmd = &cobra.Command{
|
||||
if err := repo.UpdateStatus(ctx, id, model.JobStatusStopped, 130); err != nil {
|
||||
return err
|
||||
}
|
||||
// REQ-156 / P07 T5: invalidate the jobs cache so the next
|
||||
// `orca job list` reflects the just-stopped job.
|
||||
cacheInvalidate(cacheJobClass)
|
||||
if jsonOutput {
|
||||
return printJSON(map[string]any{"id": id, "status": "stopped", "previous_status": job.Status})
|
||||
}
|
||||
|
||||
@@ -140,6 +140,10 @@ func joinLocal(cmd *cobra.Command) error {
|
||||
if err := registry.Join(ctx, node); err != nil {
|
||||
return err
|
||||
}
|
||||
// REQ-156 / P07 T5: invalidate the nodes cache so the next
|
||||
// `orca node list` does not surface a stale list missing the
|
||||
// just-joined node.
|
||||
cacheInvalidate(cacheNodeClass)
|
||||
if jsonOutput {
|
||||
return printJSON(node)
|
||||
}
|
||||
@@ -203,6 +207,8 @@ func joinProxmox(cmd *cobra.Command) error {
|
||||
if err := registry.Join(regCtx, node); err != nil {
|
||||
return fmt.Errorf("register proxmox node: %w", err)
|
||||
}
|
||||
// REQ-156 / P07 T5: invalidate the nodes cache.
|
||||
cacheInvalidate(cacheNodeClass)
|
||||
if jsonOutput {
|
||||
return printJSON(node)
|
||||
}
|
||||
@@ -236,6 +242,9 @@ var nodeLeaveCmd = &cobra.Command{
|
||||
if err := registry.Leave(ctx, id); err != nil {
|
||||
return err
|
||||
}
|
||||
// REQ-156 / P07 T5: invalidate the nodes cache so the next
|
||||
// `orca node list` does not surface the just-left node.
|
||||
cacheInvalidate(cacheNodeClass)
|
||||
if jsonOutput {
|
||||
return printJSON(map[string]string{"id": id, "state": "left"})
|
||||
}
|
||||
|
||||
+22
-3
@@ -24,6 +24,7 @@ import (
|
||||
|
||||
"git.cloudinit.dev/coreci/orca/internal/ns"
|
||||
"git.cloudinit.dev/coreci/orca/internal/paths"
|
||||
"git.cloudinit.dev/coreci/orca/internal/security"
|
||||
)
|
||||
|
||||
var nsCmd = &cobra.Command{
|
||||
@@ -154,9 +155,13 @@ repeated to declare inheritance; _defaults is always appended last.`,
|
||||
// Explicit _defaults listing is allowed (de-duped silently).
|
||||
}
|
||||
body := renderNSMd(name, parents, nsCreateInheritsEnv, nsCreateInheritsSecret)
|
||||
if err := os.WriteFile(paths.NSMd(name), []byte(body), 0o644); err != nil {
|
||||
if err := writeNSMdAtomic(paths.NSMd(name), body); err != nil {
|
||||
return fmt.Errorf("write ns.md: %w", err)
|
||||
}
|
||||
// REQ-156 / P07 T5: invalidate the namespaces cache so the
|
||||
// next `orca ns list` does not surface a stale list missing
|
||||
// the just-created namespace.
|
||||
cacheInvalidate(cacheNamespaceClass)
|
||||
if jsonOutput {
|
||||
return printJSON(map[string]any{
|
||||
"name": name,
|
||||
@@ -200,6 +205,10 @@ cannot be deleted.`,
|
||||
if err := os.RemoveAll(nsDir); err != nil {
|
||||
return fmt.Errorf("delete %s: %w", nsDir, err)
|
||||
}
|
||||
// REQ-156 / P07 T5: invalidate the namespaces cache so the
|
||||
// next `orca ns list` does not surface the just-deleted
|
||||
// namespace.
|
||||
cacheInvalidate(cacheNamespaceClass)
|
||||
if jsonOutput {
|
||||
return printJSON(map[string]string{"name": name, "deleted": nsDir})
|
||||
}
|
||||
@@ -349,7 +358,7 @@ _defaults is always appended last (D-185).`,
|
||||
}
|
||||
|
||||
body := renderNSMdFull(cfg, nsBody)
|
||||
if err := os.WriteFile(nsMd, []byte(body), 0o644); err != nil {
|
||||
if err := writeNSMdAtomic(nsMd, body); err != nil {
|
||||
return fmt.Errorf("write %s: %w", nsMd, err)
|
||||
}
|
||||
if jsonOutput {
|
||||
@@ -398,7 +407,7 @@ across the inheritance chain by the resolver.`,
|
||||
cfg.Constraints = append(cfg.Constraints, constraint)
|
||||
|
||||
body := renderNSMdFull(cfg, nsBody)
|
||||
if err := os.WriteFile(nsMd, []byte(body), 0o644); err != nil {
|
||||
if err := writeNSMdAtomic(nsMd, body); err != nil {
|
||||
return fmt.Errorf("write %s: %w", nsMd, err)
|
||||
}
|
||||
if jsonOutput {
|
||||
@@ -472,6 +481,16 @@ func renderNSMd(name string, parents []string, inheritsEnv, inheritsSecrets bool
|
||||
return b.String()
|
||||
}
|
||||
|
||||
// writeNSMdAtomic writes the ns.md frontmatter for a namespace
|
||||
// atomically (REQ-156, P07 T7). Uses security.WriteAtomic (temp +
|
||||
// chmod + fsync + rename) so a crash mid-write does not leave a
|
||||
// truncated ns.md that the inheritance resolver would fail to parse.
|
||||
// The file mode is 0644 (ns.md is not secret - it contains
|
||||
// frontmatter only).
|
||||
func writeNSMdAtomic(path, body string) error {
|
||||
return security.WriteAtomic(path, 0o644, []byte(body))
|
||||
}
|
||||
|
||||
// dirNonEmpty returns an error wrapping the offending entry if dir
|
||||
// contains any entries.
|
||||
func dirNonEmpty(dir string) error {
|
||||
|
||||
@@ -406,11 +406,16 @@ func findNamespaceDirs(targetDir string) []string {
|
||||
// read-only. It uses the same driver as the rest of the codebase
|
||||
// (modernc.org/sqlite via store.Open, but with a read-only pragma).
|
||||
func dbOpenable(path string) error {
|
||||
dsn := "file:" + path + "?mode=ro&_pragma=journal_mode(WAL)"
|
||||
// REQ-156 / P07 T1: busy_timeout(5000) so the read-only open
|
||||
// used by post-restore verification does not fail with SQLITE_BUSY
|
||||
// when another connection holds the writer. SetMaxOpenConns(1)
|
||||
// serializes the (read-only) connections.
|
||||
dsn := "file:" + path + "?mode=ro&_pragma=journal_mode(WAL)&_pragma=busy_timeout(5000)"
|
||||
db, err := sql.Open("sqlite", dsn)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
db.SetMaxOpenConns(1)
|
||||
defer db.Close()
|
||||
if err := db.Ping(); err != nil {
|
||||
return err
|
||||
|
||||
@@ -20,6 +20,7 @@ import (
|
||||
"git.cloudinit.dev/coreci/orca/internal/engine"
|
||||
"git.cloudinit.dev/coreci/orca/internal/model"
|
||||
"git.cloudinit.dev/coreci/orca/internal/paths"
|
||||
"git.cloudinit.dev/coreci/orca/internal/security"
|
||||
"git.cloudinit.dev/coreci/orca/internal/store"
|
||||
)
|
||||
|
||||
@@ -246,10 +247,14 @@ func rotateSSHKeys(ctx context.Context, transport driftTransport, nodes []*model
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("generate new ssh key: %w", err)
|
||||
}
|
||||
if err := os.WriteFile(keyPath, newPriv, 0o600); err != nil {
|
||||
// REQ-156 / P07 T8/T9: write the new SSH keypair atomically so a
|
||||
// crash mid-write does not leave a truncated key (which would
|
||||
// break all peer SSH until manually regenerated). security.WriteAtomic
|
||||
// does temp + chmod + fsync + rename.
|
||||
if err := security.WriteAtomic(keyPath, 0o600, newPriv); err != nil {
|
||||
return nil, fmt.Errorf("write new ssh key: %w", err)
|
||||
}
|
||||
if err := os.WriteFile(pubPath, newPub, 0o644); err != nil {
|
||||
if err := security.WriteAtomic(pubPath, 0o644, newPub); err != nil {
|
||||
return nil, fmt.Errorf("write new ssh pub: %w", err)
|
||||
}
|
||||
|
||||
@@ -318,7 +323,11 @@ func writeCurrentLead(ctx context.Context, name string) error {
|
||||
return err
|
||||
}
|
||||
leadPath := filepath.Join(dir, "lead")
|
||||
return os.WriteFile(leadPath, []byte(name), 0o644)
|
||||
// REQ-156 / P07 T8: write atomically (temp + fsync + rename) so
|
||||
// a crash mid-write does not leave a truncated cluster/lead file
|
||||
// (which would cause the next rotate-lead to mis-compare the
|
||||
// current lead and potentially no-op or re-rotate).
|
||||
return security.WriteAtomic(leadPath, 0o644, []byte(name))
|
||||
}
|
||||
|
||||
func trimSpace(s string) string {
|
||||
|
||||
+55
-2
@@ -29,6 +29,7 @@ import (
|
||||
|
||||
"git.cloudinit.dev/coreci/orca/internal/paths"
|
||||
"git.cloudinit.dev/coreci/orca/internal/secrets"
|
||||
"git.cloudinit.dev/coreci/orca/internal/security"
|
||||
)
|
||||
|
||||
var secretsCmd = &cobra.Command{
|
||||
@@ -93,6 +94,23 @@ func saveNSSecrets(namespace string, nsKey []byte, lines []string) error {
|
||||
return nil
|
||||
}
|
||||
|
||||
// lockNSSecrets acquires an exclusive advisory lock on the namespace's
|
||||
// .env.secrets file (REQ-156, P07 T2). The lock file is
|
||||
// paths.NSSecrets(ns) + ".lock". Returns a release function that MUST
|
||||
// be deferred. Used by set/rotate/delete/rotate-master to prevent
|
||||
// concurrent read-modify-write races: two operators running
|
||||
// `orca secrets set` simultaneously against the same namespace would
|
||||
// otherwise each load-then-save and the second write would clobber the
|
||||
// first (losing a key). The flock is advisory; the parent dir is
|
||||
// created first so Flock's O_CREATE does not fail on a missing dir.
|
||||
func lockNSSecrets(namespace string) (func(), error) {
|
||||
secPath := paths.NSSecrets(namespace)
|
||||
if err := os.MkdirAll(filepath.Dir(secPath), 0o755); err != nil {
|
||||
return nil, fmt.Errorf("create ns dir for lock: %w", err)
|
||||
}
|
||||
return security.Flock(secPath + ".lock")
|
||||
}
|
||||
|
||||
// parseKV splits a "KEY=value" argument. The value may contain '='.
|
||||
func parseKV(arg string) (key, value string, err error) {
|
||||
idx := strings.IndexByte(arg, '=')
|
||||
@@ -136,6 +154,14 @@ is appended. The .env.secrets file is rewritten atomically.`,
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
// REQ-156 / P07 T2: flock around load+save so concurrent
|
||||
// `orca secrets set` on the same namespace don't clobber
|
||||
// each other (the second write would lose the first's key).
|
||||
release, err := lockNSSecrets(ns)
|
||||
if err != nil {
|
||||
return fmt.Errorf("acquire secrets lock: %w", err)
|
||||
}
|
||||
defer release()
|
||||
nsKey, lines, err := loadMasterAndNSSecrets(ns)
|
||||
if err != nil {
|
||||
return err
|
||||
@@ -230,6 +256,13 @@ old ciphertext copies. The .env.secrets file is rewritten atomically.`,
|
||||
RunE: func(cmd *cobra.Command, args []string) error {
|
||||
ns := args[0]
|
||||
key := args[1]
|
||||
// REQ-156 / P07 T2: flock around load+save (re-encryption is a
|
||||
// read-modify-write of the whole .env.secrets file).
|
||||
release, err := lockNSSecrets(ns)
|
||||
if err != nil {
|
||||
return fmt.Errorf("acquire secrets lock: %w", err)
|
||||
}
|
||||
defer release()
|
||||
nsKey, lines, err := loadMasterAndNSSecrets(ns)
|
||||
if err != nil {
|
||||
return err
|
||||
@@ -263,6 +296,13 @@ var secretsDeleteCmd = &cobra.Command{
|
||||
RunE: func(cmd *cobra.Command, args []string) error {
|
||||
ns := args[0]
|
||||
key := args[1]
|
||||
// REQ-156 / P07 T2: flock around load+save (delete rewrites
|
||||
// the whole file).
|
||||
release, err := lockNSSecrets(ns)
|
||||
if err != nil {
|
||||
return fmt.Errorf("acquire secrets lock: %w", err)
|
||||
}
|
||||
defer release()
|
||||
nsKey, lines, err := loadMasterAndNSSecrets(ns)
|
||||
if err != nil {
|
||||
return err
|
||||
@@ -341,11 +381,20 @@ automatic rollback to the old key on any failure (C-30).`,
|
||||
// Re-encrypt each namespace. On any failure, rollback.
|
||||
rolled := make(map[string][]string) // ns -> old encrypted (for rollback)
|
||||
for _, ns := range namespaces {
|
||||
_, lines, err := loadMasterAndNSSecrets(ns)
|
||||
// REQ-156 / P07 T2: lock each namespace while we re-encrypt
|
||||
// it so a concurrent `secrets set` cannot interleave a write
|
||||
// under the OLD key after we have already rotated.
|
||||
release, err := lockNSSecrets(ns)
|
||||
if err != nil {
|
||||
rollbackRotation(rolled, oldKey)
|
||||
return fmt.Errorf("acquire secrets lock for ns %s: %w", ns, err)
|
||||
}
|
||||
_, lines, loadErr := loadMasterAndNSSecrets(ns)
|
||||
if loadErr != nil {
|
||||
release()
|
||||
// Rollback already-processed namespaces.
|
||||
rollbackRotation(rolled, oldKey)
|
||||
return fmt.Errorf("load secrets for ns %s: %w", ns, err)
|
||||
return fmt.Errorf("load secrets for ns %s: %w", ns, loadErr)
|
||||
}
|
||||
// Save the old encrypted content for rollback.
|
||||
secPath := paths.NSSecrets(ns)
|
||||
@@ -355,18 +404,22 @@ automatic rollback to the old key on any failure (C-30).`,
|
||||
// Re-encrypt under the new key.
|
||||
newNSKey, err := secrets.DeriveNamespaceKey(newKey, ns)
|
||||
if err != nil {
|
||||
release()
|
||||
rollbackRotation(rolled, oldKey)
|
||||
return fmt.Errorf("derive new ns key for %s: %w", ns, err)
|
||||
}
|
||||
enc, err := secrets.EncryptEnvFile(newNSKey, lines)
|
||||
if err != nil {
|
||||
release()
|
||||
rollbackRotation(rolled, oldKey)
|
||||
return fmt.Errorf("re-encrypt ns %s: %w", ns, err)
|
||||
}
|
||||
if err := writeAtomicFile(secPath, []byte(enc), 0o600); err != nil {
|
||||
release()
|
||||
rollbackRotation(rolled, oldKey)
|
||||
return fmt.Errorf("write ns %s: %w", ns, err)
|
||||
}
|
||||
release()
|
||||
}
|
||||
|
||||
// Save the new master key.
|
||||
|
||||
+46
-1
@@ -94,6 +94,33 @@ func init() {
|
||||
rootCmd.AddCommand(upgradeCmd)
|
||||
}
|
||||
|
||||
// acquireUpgradeLock atomically creates an exclusive lock file at
|
||||
// paths.ClusterDir()/upgrade.lock (REQ-156, P07 T3). Returns a release
|
||||
// function that MUST be deferred (it removes the lock file). If the
|
||||
// lock file already exists, returns an error "upgrade already in
|
||||
// progress" — preventing two concurrent `orca upgrade` invocations
|
||||
// from racing on the same cluster state (cutover, install.sh, peer
|
||||
// user creation). O_CREATE|O_EXCL is atomic under POSIX: only one of
|
||||
// two racing callers succeeds; the other gets EEXIST.
|
||||
func acquireUpgradeLock() (func(), error) {
|
||||
lockPath := filepath.Join(paths.ClusterDir(), "upgrade.lock")
|
||||
if err := os.MkdirAll(filepath.Dir(lockPath), 0o755); err != nil {
|
||||
return nil, fmt.Errorf("create cluster dir for upgrade lock: %w", err)
|
||||
}
|
||||
f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600)
|
||||
if err != nil {
|
||||
if os.IsExist(err) {
|
||||
return nil, fmt.Errorf("upgrade already in progress (lock file %s exists; remove it if stale)", lockPath)
|
||||
}
|
||||
return nil, fmt.Errorf("acquire upgrade lock: %w", err)
|
||||
}
|
||||
// Write the current PID + timestamp for diagnostics (best-effort;
|
||||
// a stale lock from a crashed process is the operator's signal).
|
||||
_, _ = f.WriteString(fmt.Sprintf("pid=%d started=%s\n", os.Getpid(), time.Now().UTC().Format(time.RFC3339)))
|
||||
_ = f.Close()
|
||||
return func() { _ = os.Remove(lockPath) }, nil
|
||||
}
|
||||
|
||||
// UpgradeResult is the JSON-serializable summary of an upgrade run.
|
||||
type UpgradeResult struct {
|
||||
TargetVersion string `json:"target_version"`
|
||||
@@ -134,8 +161,26 @@ func runUpgrade(cmd *cobra.Command, out interface{ Write([]byte) (int, error) })
|
||||
return nil
|
||||
}
|
||||
|
||||
// REQ-156 / P07 T3: v0.8 layout detection is read-only and MUST
|
||||
// run BEFORE the upgrade lock is acquired — the lock creates the
|
||||
// cluster/ dir (for the lock file), and Detectv08 treats the
|
||||
// presence of a cluster/ dir as "already v0.11" (no migration
|
||||
// needed). Detecting first avoids a false negative that would
|
||||
// skip the migration on a genuine v0.8 layout.
|
||||
home := paths.Root()
|
||||
if migration.Detectv08(home) {
|
||||
needV08Migration := migration.Detectv08(home)
|
||||
|
||||
// Acquire an exclusive upgrade lock for the rest of the run so
|
||||
// two concurrent `orca upgrade` invocations cannot race on the
|
||||
// cutover / install.sh / peer user creation. The lock is released
|
||||
// on return (including error paths).
|
||||
upgradeRelease, err := acquireUpgradeLock()
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
defer upgradeRelease()
|
||||
|
||||
if needV08Migration {
|
||||
if !jsonOutput {
|
||||
fmt.Fprintf(out, "• v0.8 layout detected; running data migration first\n")
|
||||
}
|
||||
|
||||
@@ -98,24 +98,39 @@ type TaskSpec struct {
|
||||
}
|
||||
|
||||
func (e *Executor) Run(ctx context.Context, job *model.Job, specs []TaskSpec) error {
|
||||
e.mu.Lock()
|
||||
defer e.mu.Unlock()
|
||||
// REQ-156 / P07 T6: the mutex previously guarded the ENTIRE job
|
||||
// (insert + status transitions + task execution + wait). That
|
||||
// serialized unrelated jobs against each other and held the lock
|
||||
// across long-running child processes, blocking concurrent
|
||||
// Submit/Status/Run callers. The mutex is now scoped ONLY to the
|
||||
// DB inserts/updates (the part that must be serialized against
|
||||
// the single-writer SQLite connection pool — see store.Open
|
||||
// SetMaxOpenConns(1)). The task goroutines spawned below do not
|
||||
// hold e.mu; they share the per-job failure counter via a local
|
||||
// sync.Mutex.
|
||||
|
||||
// Insert the job first so tasks can reference it via foreign key.
|
||||
// Insert the job + flip to Running under the lock (serializes
|
||||
// the DB writes; the underlying SQLite busy_timeout(5000) +
|
||||
// SetMaxOpenConns(1) handles contention).
|
||||
e.mu.Lock()
|
||||
if err := e.jobs.Insert(ctx, job); err != nil {
|
||||
e.mu.Unlock()
|
||||
return err
|
||||
}
|
||||
if err := e.jobs.UpdateStatus(ctx, job.ID, model.JobStatusRunning, 0); err != nil {
|
||||
e.mu.Unlock()
|
||||
return err
|
||||
}
|
||||
e.mu.Unlock()
|
||||
|
||||
// Task execution runs WITHOUT e.mu — concurrent jobs (and
|
||||
// concurrent Submit/Status callers) are no longer blocked by a
|
||||
// long-running child process.
|
||||
var (
|
||||
wg sync.WaitGroup
|
||||
failedCount int
|
||||
exitCode int
|
||||
mu sync.Mutex
|
||||
)
|
||||
|
||||
for _, ts := range specs {
|
||||
wg.Add(1)
|
||||
go func(ts TaskSpec) {
|
||||
@@ -133,14 +148,16 @@ func (e *Executor) Run(ctx context.Context, job *model.Job, specs []TaskSpec) er
|
||||
}
|
||||
wg.Wait()
|
||||
|
||||
// Final status transition under the lock (the DB write is the
|
||||
// only thing that needs serialization).
|
||||
e.mu.Lock()
|
||||
defer e.mu.Unlock()
|
||||
if failedCount > 0 {
|
||||
exitCode = 1
|
||||
if err := e.jobs.UpdateStatus(ctx, job.ID, model.JobStatusFailed, exitCode); err != nil {
|
||||
if err := e.jobs.UpdateStatus(ctx, job.ID, model.JobStatusFailed, 1); err != nil {
|
||||
return err
|
||||
}
|
||||
return fmt.Errorf("%d/%d tasks failed", failedCount, len(specs))
|
||||
}
|
||||
|
||||
if err := e.jobs.UpdateStatus(ctx, job.ID, model.JobStatusComplete, 0); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
@@ -29,6 +29,8 @@ import (
|
||||
|
||||
"github.com/coreos/go-oidc/v3/oidc"
|
||||
"golang.org/x/oauth2"
|
||||
|
||||
"git.cloudinit.dev/coreci/orca/internal/security"
|
||||
)
|
||||
|
||||
// OIDCConfig holds the OIDC client configuration. It is loaded from
|
||||
@@ -113,7 +115,13 @@ func SaveCredentials(c *Credentials) error {
|
||||
if err != nil {
|
||||
return fmt.Errorf("oidc: marshal: %w", err)
|
||||
}
|
||||
return writeAtomic0600(path, data)
|
||||
// REQ-156 / P07 T9: use the canonical security.WriteAtomic (temp
|
||||
// + chmod + fsync + rename) instead of the local writeAtomic0600
|
||||
// (which did temp + chmod + rename with NO fsync - a crash before
|
||||
// rename could leave a partially-written tmp file that rename
|
||||
// would then promote, or the rename could land before the data
|
||||
// reached durable storage).
|
||||
return security.WriteAtomic(path, 0o600, data)
|
||||
}
|
||||
|
||||
// ClearCredentials removes the stored credentials (logout).
|
||||
@@ -128,16 +136,6 @@ func ClearCredentials() error {
|
||||
return nil
|
||||
}
|
||||
|
||||
// writeAtomic0600 writes data to path atomically at mode 0600
|
||||
// (temp + chmod + rename).
|
||||
func writeAtomic0600(path string, data []byte) error {
|
||||
tmp := path + ".tmp"
|
||||
if err := os.WriteFile(tmp, data, 0o600); err != nil {
|
||||
return fmt.Errorf("oidc: write tmp: %w", err)
|
||||
}
|
||||
return os.Rename(tmp, path)
|
||||
}
|
||||
|
||||
// OIDCClient wraps the OIDC provider + oauth2 config for the auth flow.
|
||||
type OIDCClient struct {
|
||||
provider *oidc.Provider
|
||||
|
||||
@@ -22,6 +22,11 @@ func Open(path string) (*sql.DB, error) {
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("open sqlite: %w", err)
|
||||
}
|
||||
// REQ-156 / P07 T1: SQLite is a single-writer database. Cap the
|
||||
// connection pool at 1 so concurrent goroutines serialize on the
|
||||
// busy_timeout(5000) above instead of racing for the WAL writer
|
||||
// lock and surfacing spurious SQLITE_BUSY errors to callers.
|
||||
db.SetMaxOpenConns(1)
|
||||
if err := db.Ping(); err != nil {
|
||||
_ = db.Close()
|
||||
return nil, fmt.Errorf("ping sqlite: %w", err)
|
||||
|
||||
@@ -15,6 +15,7 @@ import (
|
||||
"fmt"
|
||||
"net/http"
|
||||
"strings"
|
||||
"sync"
|
||||
"time"
|
||||
|
||||
"github.com/go-webauthn/webauthn/protocol"
|
||||
@@ -80,26 +81,54 @@ type RegistrationSession struct {
|
||||
CreatedAt time.Time
|
||||
}
|
||||
|
||||
// sessionStore holds in-flight sessions (registration + login). In
|
||||
// production this would be a Redis/shared cache; for the bundled
|
||||
// single-lead Dex, an in-memory map with TTL is sufficient.
|
||||
// sessionStore holds in-flight sessions (registration). In production
|
||||
// this would be a Redis/shared cache; for the bundled single-lead
|
||||
// Dex, an in-memory map with TTL is sufficient.
|
||||
//
|
||||
// REQ-156 / P07 T10: the session maps are accessed from HTTP handler
|
||||
// goroutines (one goroutine per request) and were previously plain
|
||||
// maps with no synchronization. Concurrent BeginRegistration calls
|
||||
// for the same username would race on map writes (detected by go
|
||||
// test -race in T11). A sync.Mutex now guards all access.
|
||||
type sessionStore struct {
|
||||
mu sync.Mutex
|
||||
sessions map[string]*RegistrationSession
|
||||
}
|
||||
|
||||
// regSessions is the global in-flight registration session store.
|
||||
var regSessions = &sessionStore{sessions: make(map[string]*RegistrationSession)}
|
||||
|
||||
// loginSessionStore holds in-flight login sessions (T10). Same
|
||||
// mutex pattern as sessionStore.
|
||||
type loginSessionStore struct {
|
||||
mu sync.Mutex
|
||||
sessions map[string]*LoginSession
|
||||
}
|
||||
|
||||
// loginSessions is the global in-flight login session store.
|
||||
var loginSessions = &loginSessionStore{sessions: make(map[string]*LoginSession)}
|
||||
|
||||
// sessionTTL is the max time a registration/login session is valid.
|
||||
const sessionTTL = 5 * time.Minute
|
||||
|
||||
// cleanSessions removes expired sessions.
|
||||
// cleanSessions removes expired registration + login sessions.
|
||||
// Called under each store's lock by the Begin* handlers.
|
||||
func cleanSessions() {
|
||||
now := time.Now()
|
||||
for id, s := range regSessions.sessions {
|
||||
if now.Sub(s.CreatedAt) > sessionTTL {
|
||||
regSessions.mu.Lock()
|
||||
for id, sess := range regSessions.sessions {
|
||||
if now.Sub(sess.CreatedAt) > sessionTTL {
|
||||
delete(regSessions.sessions, id)
|
||||
}
|
||||
}
|
||||
regSessions.mu.Unlock()
|
||||
loginSessions.mu.Lock()
|
||||
for id, sess := range loginSessions.sessions {
|
||||
if now.Sub(sess.CreatedAt) > sessionTTL {
|
||||
delete(loginSessions.sessions, id)
|
||||
}
|
||||
}
|
||||
loginSessions.mu.Unlock()
|
||||
}
|
||||
|
||||
// requireAuth checks the request for an authenticated session. When
|
||||
@@ -165,11 +194,13 @@ func (c *Connector) BeginRegistration(w http.ResponseWriter, r *http.Request) {
|
||||
return
|
||||
}
|
||||
sessionID := base64.RawURLEncoding.EncodeToString(userID)
|
||||
regSessions.mu.Lock()
|
||||
regSessions.sessions[sessionID] = &RegistrationSession{
|
||||
UserID: username,
|
||||
Challenge: session,
|
||||
CreatedAt: time.Now(),
|
||||
}
|
||||
regSessions.mu.Unlock()
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
json.NewEncoder(w).Encode(options)
|
||||
}
|
||||
@@ -188,16 +219,20 @@ func (c *Connector) FinishRegistration(w http.ResponseWriter, r *http.Request) {
|
||||
return
|
||||
}
|
||||
sessionID := base64.RawURLEncoding.EncodeToString([]byte(username))
|
||||
regSessions.mu.Lock()
|
||||
session, ok := regSessions.sessions[sessionID]
|
||||
if !ok {
|
||||
regSessions.mu.Unlock()
|
||||
http.Error(w, "no registration session; call /register first", http.StatusBadRequest)
|
||||
return
|
||||
}
|
||||
if time.Since(session.CreatedAt) > sessionTTL {
|
||||
delete(regSessions.sessions, sessionID)
|
||||
regSessions.mu.Unlock()
|
||||
http.Error(w, "session expired", http.StatusBadRequest)
|
||||
return
|
||||
}
|
||||
regSessions.mu.Unlock()
|
||||
parsed, err := protocol.ParseCredentialCreationResponseBody(r.Body)
|
||||
if err != nil {
|
||||
http.Error(w, fmt.Sprintf("parse attestation: %v", err), http.StatusBadRequest)
|
||||
@@ -221,7 +256,9 @@ func (c *Connector) FinishRegistration(w http.ResponseWriter, r *http.Request) {
|
||||
http.Error(w, fmt.Sprintf("store credential: %v", err), http.StatusInternalServerError)
|
||||
return
|
||||
}
|
||||
regSessions.mu.Lock()
|
||||
delete(regSessions.sessions, sessionID)
|
||||
regSessions.mu.Unlock()
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
json.NewEncoder(w).Encode(map[string]string{"status": "registered", "user_id": username})
|
||||
}
|
||||
@@ -233,8 +270,6 @@ type LoginSession struct {
|
||||
CreatedAt time.Time
|
||||
}
|
||||
|
||||
var loginSessions = map[string]*LoginSession{}
|
||||
|
||||
// BeginLogin starts the WebAuthn login ceremony.
|
||||
// GET /orca/webauthn/login?username=<name>
|
||||
func (c *Connector) BeginLogin(w http.ResponseWriter, r *http.Request) {
|
||||
@@ -259,11 +294,13 @@ func (c *Connector) BeginLogin(w http.ResponseWriter, r *http.Request) {
|
||||
http.Error(w, fmt.Sprintf("begin login: %v", err), http.StatusInternalServerError)
|
||||
return
|
||||
}
|
||||
loginSessions[username] = &LoginSession{
|
||||
loginSessions.mu.Lock()
|
||||
loginSessions.sessions[username] = &LoginSession{
|
||||
UserID: username,
|
||||
Challenge: session,
|
||||
CreatedAt: time.Now(),
|
||||
}
|
||||
loginSessions.mu.Unlock()
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
json.NewEncoder(w).Encode(options)
|
||||
}
|
||||
@@ -276,16 +313,20 @@ func (c *Connector) FinishLogin(w http.ResponseWriter, r *http.Request) {
|
||||
http.Error(w, "username required", http.StatusBadRequest)
|
||||
return
|
||||
}
|
||||
session, ok := loginSessions[username]
|
||||
loginSessions.mu.Lock()
|
||||
session, ok := loginSessions.sessions[username]
|
||||
if !ok {
|
||||
loginSessions.mu.Unlock()
|
||||
http.Error(w, "no login session; call /login first", http.StatusBadRequest)
|
||||
return
|
||||
}
|
||||
if time.Since(session.CreatedAt) > sessionTTL {
|
||||
delete(loginSessions, username)
|
||||
delete(loginSessions.sessions, username)
|
||||
loginSessions.mu.Unlock()
|
||||
http.Error(w, "session expired", http.StatusBadRequest)
|
||||
return
|
||||
}
|
||||
loginSessions.mu.Unlock()
|
||||
existing, _ := c.store.GetCredential(username)
|
||||
if existing == nil {
|
||||
http.Error(w, "user not registered", http.StatusNotFound)
|
||||
@@ -307,7 +348,9 @@ func (c *Connector) FinishLogin(w http.ResponseWriter, r *http.Request) {
|
||||
return
|
||||
}
|
||||
_ = c.store.UpdateSignCount(username, cred.Authenticator.SignCount)
|
||||
delete(loginSessions, username)
|
||||
loginSessions.mu.Lock()
|
||||
delete(loginSessions.sessions, username)
|
||||
loginSessions.mu.Unlock()
|
||||
// The OIDC sub is the username (the connector maps credential ID
|
||||
// to sub). Dex uses this to issue the ID token.
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
|
||||
@@ -0,0 +1,179 @@
|
||||
package webauthn
|
||||
|
||||
// connector_concurrency_test.go covers REQ-156 / P07 T10: the
|
||||
// WebAuthn session maps (regSessions, loginSessions) are accessed
|
||||
// from HTTP handler goroutines (one goroutine per request) and were
|
||||
// previously plain maps with no synchronization. Concurrent
|
||||
// BeginRegistration calls for the same username would race on map
|
||||
// writes (detected by `go test -race`). T10 added a sync.Mutex to
|
||||
// each store; this test exercises the fix under the race detector.
|
||||
//
|
||||
// Run with: go test -race ./internal/webauthn/
|
||||
|
||||
import (
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"sync"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// TestBeginRegistrationConcurrentNoPanic fires many concurrent
|
||||
// BeginRegistration requests (all authenticated, all for the SAME
|
||||
// username so they hit the SAME session map entry) and asserts the
|
||||
// handler does not panic and does not race on the shared
|
||||
// regSessions.sessions map. Without the T10 mutex this test panics
|
||||
// under -race with "concurrent map writes".
|
||||
func TestBeginRegistrationConcurrentNoPanic(t *testing.T) {
|
||||
dbPath := t.TempDir() + "/webauthn-conc.db"
|
||||
store, err := NewStore(dbPath)
|
||||
if err != nil {
|
||||
t.Fatalf("NewStore: %v", err)
|
||||
}
|
||||
defer store.Close()
|
||||
c, err := NewConnectorWithAuth(store, "test.cluster", "https://test.cluster",
|
||||
func(r *http.Request) (bool, string, error) { return true, "admin", nil })
|
||||
if err != nil {
|
||||
t.Fatalf("NewConnector: %v", err)
|
||||
}
|
||||
mux := c.Routes()
|
||||
|
||||
const n = 25
|
||||
var wg sync.WaitGroup
|
||||
wg.Add(n)
|
||||
panicCh := make(chan interface{}, n)
|
||||
for i := 0; i < n; i++ {
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
select {
|
||||
case panicCh <- r:
|
||||
default:
|
||||
}
|
||||
}
|
||||
}()
|
||||
req := httptest.NewRequest("GET", "/orca/webauthn/register?username=admin", nil)
|
||||
rec := httptest.NewRecorder()
|
||||
// BeginRegistration writes to regSessions.sessions[sessionID]
|
||||
// under the mutex; concurrent writers for the same key
|
||||
// must not panic or race.
|
||||
mux.ServeHTTP(rec, req)
|
||||
}()
|
||||
}
|
||||
wg.Wait()
|
||||
close(panicCh)
|
||||
if p, ok := <-panicCh; ok {
|
||||
t.Fatalf("BeginRegistration panicked under concurrency: %v", p)
|
||||
}
|
||||
}
|
||||
|
||||
// TestBeginLoginConcurrentNoPanic is the login-session variant. It
|
||||
// pre-registers a credential so BeginLogin finds the user, then fires
|
||||
// concurrent BeginLogin calls for the same username. The login
|
||||
// session map writes must be mutex-guarded (T10).
|
||||
func TestBeginLoginConcurrentNoPanic(t *testing.T) {
|
||||
dbPath := t.TempDir() + "/webauthn-conc-login.db"
|
||||
store, err := NewStore(dbPath)
|
||||
if err != nil {
|
||||
t.Fatalf("NewStore: %v", err)
|
||||
}
|
||||
defer store.Close()
|
||||
// Pre-seed a credential so BeginLogin does not 404.
|
||||
if err := store.PutCredential(&Credential{
|
||||
UserID: "loginuser",
|
||||
CredentialID: []byte("cred-id-bytes"),
|
||||
PublicKey: []byte("pub-key-bytes"),
|
||||
}); err != nil {
|
||||
t.Fatalf("PutCredential: %v", err)
|
||||
}
|
||||
c, err := NewConnectorWithAuth(store, "test.cluster", "https://test.cluster",
|
||||
func(r *http.Request) (bool, string, error) { return true, "loginuser", nil })
|
||||
if err != nil {
|
||||
t.Fatalf("NewConnector: %v", err)
|
||||
}
|
||||
mux := c.Routes()
|
||||
|
||||
const n = 25
|
||||
var wg sync.WaitGroup
|
||||
wg.Add(n)
|
||||
panicCh := make(chan interface{}, n)
|
||||
for i := 0; i < n; i++ {
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
select {
|
||||
case panicCh <- r:
|
||||
default:
|
||||
}
|
||||
}
|
||||
}()
|
||||
req := httptest.NewRequest("GET", "/orca/webauthn/login?username=loginuser", nil)
|
||||
rec := httptest.NewRecorder()
|
||||
mux.ServeHTTP(rec, req)
|
||||
}()
|
||||
}
|
||||
wg.Wait()
|
||||
close(panicCh)
|
||||
if p, ok := <-panicCh; ok {
|
||||
t.Fatalf("BeginLogin panicked under concurrency: %v", p)
|
||||
}
|
||||
}
|
||||
|
||||
// TestCleanSessionsConcurrentNoPanic exercises the cleanSessions
|
||||
// helper which iterates + deletes from BOTH session maps. Without
|
||||
// the T10 mutexes, concurrent cleanSessions + BeginRegistration
|
||||
// would race. We drive cleanSessions from multiple goroutines while
|
||||
// also doing BeginRegistration writes.
|
||||
func TestCleanSessionsConcurrentNoPanic(t *testing.T) {
|
||||
dbPath := t.TempDir() + "/webauthn-clean.db"
|
||||
store, err := NewStore(dbPath)
|
||||
if err != nil {
|
||||
t.Fatalf("NewStore: %v", err)
|
||||
}
|
||||
defer store.Close()
|
||||
c, err := NewConnectorWithAuth(store, "test.cluster", "https://test.cluster",
|
||||
func(r *http.Request) (bool, string, error) { return true, "admin", nil })
|
||||
if err != nil {
|
||||
t.Fatalf("NewConnector: %v", err)
|
||||
}
|
||||
mux := c.Routes()
|
||||
|
||||
const n = 15
|
||||
var wg sync.WaitGroup
|
||||
wg.Add(n * 2)
|
||||
panicCh := make(chan interface{}, n*2)
|
||||
for i := 0; i < n; i++ {
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
select {
|
||||
case panicCh <- r:
|
||||
default:
|
||||
}
|
||||
}
|
||||
}()
|
||||
cleanSessions()
|
||||
}()
|
||||
go func() {
|
||||
defer wg.Done()
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
select {
|
||||
case panicCh <- r:
|
||||
default:
|
||||
}
|
||||
}
|
||||
}()
|
||||
req := httptest.NewRequest("GET", "/orca/webauthn/register?username=admin", nil)
|
||||
rec := httptest.NewRecorder()
|
||||
mux.ServeHTTP(rec, req)
|
||||
}()
|
||||
}
|
||||
wg.Wait()
|
||||
close(panicCh)
|
||||
if p, ok := <-panicCh; ok {
|
||||
t.Fatalf("cleanSessions/BeginRegistration panicked under concurrency: %v", p)
|
||||
}
|
||||
}
|
||||
@@ -46,11 +46,15 @@ func NewStore(dbPath string) (*Store, error) {
|
||||
if err := os.MkdirAll(filepath.Dir(dbPath), 0o700); err != nil {
|
||||
return nil, fmt.Errorf("webauthn: mkdir: %w", err)
|
||||
}
|
||||
dsn := fmt.Sprintf("file:%s?_pragma=journal_mode(WAL)", dbPath)
|
||||
// REQ-156 / P07 T1: busy_timeout(5000) so concurrent webauthn
|
||||
// DB opens wait up to 5s for the writer instead of failing with
|
||||
// SQLITE_BUSY. SetMaxOpenConns(1) serializes the connections.
|
||||
dsn := fmt.Sprintf("file:%s?_pragma=journal_mode(WAL)&_pragma=busy_timeout(5000)", dbPath)
|
||||
db, err := sql.Open("sqlite", dsn)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("webauthn: open db: %w", err)
|
||||
}
|
||||
db.SetMaxOpenConns(1)
|
||||
if err := db.Ping(); err != nil {
|
||||
db.Close()
|
||||
return nil, fmt.Errorf("webauthn: ping: %w", err)
|
||||
|
||||
Reference in New Issue
Block a user