diff --git a/.kiro/specs/component-failover-adoption/tasks.md b/.kiro/specs/component-failover-adoption/tasks.md index 827c23c3..dda6ffa1 100644 --- a/.kiro/specs/component-failover-adoption/tasks.md +++ b/.kiro/specs/component-failover-adoption/tasks.md @@ -117,7 +117,7 @@ - _Requirements: 2.4, 2.5_ - _Depends: 4.3_ -- [ ] 4.5 Add apiserver's backend-health circuit breaker +- [x] 4.5 Add apiserver's backend-health circuit breaker - Add a degraded-until state that lets request handlers fail fast instead of blocking on the full retry budget when the backend is known-degraded, clearing on either a successful rejoin or its own timeout - Wire it into the shared backend-call helper: check before every call, mark degraded when a call's outcome indicates retry-budget exhaustion, mark healthy on a successful rejoin from either the write path or the idle-period path - Keep this state's own timeout independent from the pending-rejoin flag — the circuit breaker may reset optimistically on its timer, but that must never suppress the pending-rejoin flag's own retry-until-actually-joined behavior @@ -173,4 +173,5 @@ - Task 4.3: rejoin flag-setter (`go_backend_rejoin_handler`), its C shim, `extern` declaration, and `xcecli_set_reconnect_handler()` registration call all live together in `clib.go`'s `xce_backend_init()` — confirms design.md's "cgo symbol resolution is per-file" constraint (see `wire_dispatch_support.go`'s own documented rationale): a `C.` reference only resolves against an `extern` declared in THAT file's own cgo preamble, even though the Go `//export` mechanism makes the function callable package-wide — splitting the shim into a separate `reconnect.go` preamble while the extern lived in `clib.go` (or vice versa) would silently fail to link. Registration placed strictly AFTER `xcecli_start()`'s success check (task 2.3's ordering lesson ported to Go: `xcecli_start()` unconditionally wipes `rejoin_cb` to NULL as part of its own init). `needsRejoin` is a proper `sync/atomic` flag, not a plain bool — apiserver serves concurrent HTTP requests (unlike worker's/scheduler's single-threaded daemons), so this is a real correctness requirement here, not just style. `checkAndPerformRejoin()` wired into all 5 `gCtx.ctx`-based backend-call wrappers (SubmitJob/ControlJob/ControlQueue/ControlHost/FetchFile) — note `xcecli_fetchfile()` itself isn't `XCECLI_FAILOVER_CALL`-wrapped in libxce (never triggers the C-side rejoin invoke), so wrapping it here is safe but less load-bearing than the other four; that's a libxce gap, out of this task's `ApiserverBackendBridge` boundary to fix. Reviewer found (non-blocking, correctness-safe): concurrent goroutines can each observe `needsRejoin==1` after a failover and each call the real `xcecli_join()`, serialized by the C library's own per-ctx lock (no corruption, no incorrect flag state) but each can block up to the ~100s retry budget on a redundant join — a latency/traffic concern, not a Requirement-2 violation; a CAS-based single-flight guard is a reasonable fast-follow, not required. Scope reminder for task 4.4/4.5: `ApiserverBackendHealth`/`IsDegraded()`/`MarkHealthy()` deliberately do not exist yet — `checkAndPerformRejoin()` is the POST-call half only; task 4.5 adds a pre-call check to the same helper, not a new one. - Task 4.4: thin adapter, not a second rejoin mechanism — `runHeartbeat()`'s new `onFailureToSuccess`/`onSuccessTick` callback params are wired directly to task 4.3's real `setNeedsRejoin`/`checkAndPerformRejoin` (`reconnect.go`) from `StartHeartbeat()`, zero duplicated flag-check logic added in `heartbeat.go` itself. Necessary because `xcecli_heartbeat()` is `XCECLI_FF_CALL`-wrapped (`client_internal.h`), a deliberately non-retrying fire-and-forget path that does its own best-effort one-shot reconnect but NEVER calls `xcecli_invoke_rejoin()` (that only fires from `xcecli_retry_next`, the write-call retry path) — so nothing sets `needsRejoin` on a pure idle-period recovery unless the heartbeat loop does it itself. `rc >= 0` success classification for `xcecli_heartbeat` confirmed against `xceapi.c`/`request.c`: it only ever returns 0 or a negative errno, never a positive value, so this is equivalent to (not just consistent with) the `rc<0`-only-failure convention used elsewhere. `onSuccessTick` fires unconditionally on every successful tick (no pending-check duplicated in `heartbeat.go`) — safe because `checkAndPerformRejoin()` is itself a cheap single-atomic-load no-op when nothing is pending. `prevFailed` (transition-tracking state) is correctly a plain non-atomic local, not `sync/atomic` — verified `runHeartbeat()` has exactly one call site (`StartHeartbeat()`, called once from `cmd/main.go`), one goroutine for the process lifetime; contrast with `needsRejoin` itself, which stays atomic since concurrent HTTP-handler goroutines reach it via the write-path wrappers. Known, accepted, already-documented (task 4.3) tradeoff carried forward: a rejoin attempt from the heartbeat goroutine shares `ac->failover.lock` with concurrent HTTP-handler backend calls, so one can block the other for up to the ~1-2 minute retry budget — not a new issue this task introduces, same mechanism task 4.3 already reviewed and accepted, task 4.4 just adds one more caller of it. - Task 3.4: pure test-coverage task, zero production changes to `scheduler.c`/`driver.c` (correct outcome — task 3.2 already implemented the retry/shutdown/rejoin behavior correctly; 3.4 proves it holds at multi-decision QUEUE granularity through the real drain loop, which task 3.2's own tests never exercised, all 6 of which call `sched_publish_dispatch()`/`sched_publish_pend_reason()` directly on one decision). New `test_scheduler_decision_redelivery.c` links real `scheduler_srcs` with `-Wl,--wrap=xcecli_jobdispatch` (task 3.2's established technique) and drives the real `xce_sched_driver_on_decision()` (driver.c) over 3 queued, distinct-jobid decisions. Two load-bearing tests, both RED→GREEN-verified against the real code by two independent reviewer rounds (not just the implementer's claim): ordering (decision 1 stuck-then-succeeds fully completes before decision 2's call ever fires) and bounded shutdown latency (decisions queued behind one that triggers `core.stop` mid-retry get ZERO publish calls — the concrete, fast, deterministic proof that `sched_publish_retry_loop()`'s check-`stop`-before-each-attempt placement is correct, verified by reviewers moving the check to bottom-of-loop and confirming only this one test catches it). Test-fixture gotcha for any future test building a bare `xce_sched_core_t` via `xce_sched_core_init()` alone (not full `sched_init()`): do NOT call `xce_sched_core_fini()` on it — `fini()` calls `dataq_fini()` on `pend_updates[0/1]`, which only `sched_init()` (not `xce_sched_core_init()`) ever `dataq_init()`s, so `fini()` on a bare core SEGVs on a NULL dataq; omit the `fini()` call entirely, matching `test_core_events.c`'s established "fixtures aren't torn down" convention for this exact minimal-fixture pattern. +- Task 4.5: subtle design question resolved and worth remembering for task 4.6/5.x — `callBackend()`'s (`clib.go`) degraded fast-path deliberately skips `checkAndPerformRejoin()` ENTIRELY (not just the underlying `C.xcecli_*` call), meaning write-path requests contribute NOTHING to rejoin-retry cadence while `IsDegraded()` is true; only task 4.4's heartbeat goroutine keeps retrying during that window. Reviewer independently confirmed this is correct, not a violation of "must never suppress the pending-rejoin flag's own retry-until-actually-joined behavior": that constraint is one-directional (the circuit breaker's own optimistic TTL-based state must never be READ BY the rejoin mechanism to gate it — confirmed `checkAndPerformRejoin()` never references `IsDegraded()` anywhere), not a requirement that every possible call site attempt a rejoin check regardless of cost. The alternative (calling `checkAndPerformRejoin()`, which can itself block ~100s inside `xcecli_join()`'s own retry budget, on every fast-failed request) would directly violate THIS task's own fail-fast acceptance criterion — the two constraints (fail-fast + never-suppress-retry) are jointly satisfiable only because `heartbeat.go`'s goroutine runs unconditionally for the process lifetime, independent of circuit-breaker state, and `backendDegradedTTL` (10s, chosen well below the ~100s `XCECLI_FAILOVER_CALL` budget) is shorter than the heartbeat interval (default 30s) so write traffic resumes contributing once the TTL clears anyway. `callBackend()` is a genuine refactor consolidating what tasks 4.3/4.4 left as duplicated inline `rc := C.xcecli_*(...); checkAndPerformRejoin()` pairs across 5 wrapper functions into one shared helper — `checkAndPerformRejoin()` now appears exactly once in `clib.go`, inside `callBackend`, matching design.md's explicit "not copy-pasted per wrapper" instruction. `E_XCE_NOT_READY`-triggered `MarkDegraded()` uses a plain-Go `notReadyRC` mirror var (`= -int(C.E_XCE_NOT_READY)`, computed live via cgo, not hardcoded) since `_test.go` files can't reference `C.*` symbols directly (task 4.1's established Go-toolchain restriction) — keep this pattern for any future test needing a C errno constant. - Task 3.3: fixed a real production gap found along the way — `sched_init()`'s two all-candidates-unreachable/join-failure log lines logged only a candidate COUNT, never host:port names (same gap worker's task 2.3 fixed for the worker). Added `sched_format_candidate_list()`, threading `sched->cluster_cfg` through as a parameter rather than a new file-scope global (unlike worker's `g_cluster_cfg`) since `test_scheduler_connect.c` links `scheduler_srcs` as separate objects, not `#include`, so the `static` helper isn't directly callable from tests — coverage requires a real `dup2()`-based stdout capture around a live `sched_init()` call hitting the failure path, not a white-box direct call (see `capture_stdout_during_sched_init()` in `test_scheduler_connect.c` — reusable pattern for any future scheduler.c log-content assertion). Round-1 review caught this diagnostic fix shipping with zero test coverage (proven by reverting the 59-line diff and rebuilding: all existing tests still passed) — a reminder that "the fix works, I verified it manually/via live logs" is not the same as a committed regression test; always add the assertion, don't just observe the correct behavior once. The worst-case-occupancy backpressure test (`test_scheduler_backpressure.c`) only proves Req 3 AC2 (buffering/block/resume-with-no-loss) — it does not exercise task 3.2's `-E_XCE_NOT_READY` retry path, so don't cite it for Req 7 coverage (that's task 3.4's job). E2E kill-primary/reconnect and all-unreachable-at-startup were validated manually against a real 3-node `/xce-deploy-ha` cluster rather than committed as an automated test, following worker's task 2.3 precedent — this repo has no automated live-multi-process-deploy E2E harness for any C daemon (the only one, `test_e2e_live_ha_push.py`, is Python/apiserver-only); reviewer judged this acceptable both rounds. Manual validation confirmed join, primary-death survival, and lazy socket-orphaning-on-idle (matches design), but could not observe a live rejoin firing against real sockets — blocked by bug-010 (`xcecd` ASAN "already joined thread" abort under `unshare -r`, needed to bypass the worker daemon's root-only check in this sandbox; unconfirmed whether it's a genuine bug or a userns artifact, never tried under real root). The exact rejoin-after-publish-success code path already has solid white-box mock coverage from task 3.2's `test_rejoin_composition_retries_across_publish_calls`, so this residual gap is narrowly "proven against mocks, not yet against a live socket" — worth closing opportunistically if bug-010 ever gets resolved, not a blocker for this spec. diff --git a/src/frontend/apiserver/backend/clib.go b/src/frontend/apiserver/backend/clib.go index adcda94f..62e3a806 100644 --- a/src/frontend/apiserver/backend/clib.go +++ b/src/frontend/apiserver/backend/clib.go @@ -13,6 +13,7 @@ package backend #include "controller.capnp.h" #include "val_helpers.h" #include "xce/cluster.h" +#include "xce/errnox.h" // backend_ctx_t wraps xcecli_ctx_t for use as the C handler's arg. typedef struct { @@ -78,9 +79,11 @@ import ( "apiserver/models" logger "apiserver/utils" "context" + "errors" "fmt" "strings" "sync" + "time" "unsafe" "go.uber.org/zap" @@ -534,6 +537,85 @@ func handleNotifyData(nd *C.xce_notify_data_t) C.int { return 0 } +// ErrBackendUnavailable is returned immediately by callBackend (and +// therefore by every wrapper below) when ApiserverBackendHealth reports the +// backend connection degraded (Req 4.1) -- a clear, distinct error a caller +// can recognize as "backend down, retrying" rather than an unrelated +// internal failure, without ever invoking the underlying blocking +// C.xcecli_* call. +var ErrBackendUnavailable = errors.New( + "apiserver: backend connection unavailable (circuit breaker open); try again shortly") + +// backendDegradedTTL is how long callBackend marks the backend degraded +// (ApiserverBackendHealth.MarkDegraded) after a call's own outcome +// indicates its retry budget was exhausted, before request handlers resume +// probing it directly again -- the half-open half of the circuit breaker +// (design.md's ApiserverBackendHealth). Chosen well below xcecli's own +// full per-call retry budget (XCECLI_FAILOVER_CALL: max_retries=20, +// backoff capped at 5000ms -- roughly 1-2 minutes total, +// client_failover.c:218,220): 10s is short enough that a backend which +// recovers quickly gets re-probed promptly, while still long enough that +// every request arriving during a genuine outage fails fast instead of +// each independently re-discovering the same ~100s exhausted budget one +// blocking call at a time. +const backendDegradedTTL = 10 * time.Second + +// notReadyRC exposes -E_XCE_NOT_READY as a plain Go int, for +// health_rejoin_test.go (a //go:build cgo test file that cannot `import "C"` +// itself, golang/go#13938) to script a callBackend closure returning this +// exact retry-budget-exhaustion value without hardcoding the raw errno +// number -- same plain-Go-signature test-seam rationale as performRejoinFn +// (reconnect.go) and currentClientConfigNMasters() (above). +var notReadyRC = -int(C.E_XCE_NOT_READY) + +// callBackend is the single shared pre-call/post-call helper every +// gCtx.ctx-based backend-call wrapper below (SubmitJob, ControlJob, +// ControlQueue, ControlHost, FetchFile) calls around its own single +// C.xcecli_* invocation (component-failover-adoption task 4.5, boundary: +// ApiserverBackendBridge/ApiserverBackendHealth) -- implemented once here, +// not copy-pasted per wrapper. fn performs exactly one C.xcecli_* call and +// returns its raw rc as a plain Go int (not C.int) -- the same +// plain-Go-signature test seam pattern as reconnect.go's performRejoinFn, +// letting a //go:build cgo test file (which can never `import "C"` itself, +// golang/go#13938) drive this helper directly with a scripted closure, with +// no live backend connection or even an initialized gCtx required. +// +// Pre-call (Req 4.1): if IsDegraded(), fn is never invoked at all -- the +// wrapped call (and its own up-to-~100s xcecli retry budget) is skipped +// entirely, returning ErrBackendUnavailable immediately. checkAndPerformRejoin +// is deliberately NOT called on this path either: it can itself block on +// xcecli_join's own up-to-~100s retry budget, which would defeat the whole +// point of failing fast here (Req 4.1's bounded, sub-second response). The +// rejoin-retry cadence during this window keeps running regardless, driven +// by heartbeat.go's own unconditional per-tick call to checkAndPerformRejoin +// (task 4.4) -- which never reads IsDegraded() -- so the pending-rejoin +// flag's retry-until-actually-joined behavior is never suppressed by this +// short-circuit (see health.go's own doc comment for why these two states +// must stay independent). +// +// Post-call (only reached when not degraded): the existing rejoin check +// (checkAndPerformRejoin, task 4.3/4.4, unchanged) always runs, regardless +// of fn's own outcome -- a pending rejoin is orthogonal to whether this +// particular call happened to succeed. If fn's rc is exactly +// -E_XCE_NOT_READY (this call's own retry budget exhausted), MarkDegraded +// opens the circuit for subsequent calls. +func callBackend(name string, fn func() int) (rc int, err error) { + if IsDegraded() { + return 0, ErrBackendUnavailable + } + + rc = fn() + checkAndPerformRejoin() + + if rc == notReadyRC { + MarkDegraded(backendDegradedTTL) + } + if rc < 0 { + return rc, fmt.Errorf("%s failed: %d", name, rc) + } + return rc, nil +} + // SubmitJob sends a job submission to the backend via xcecli_jobsubmit. // arraySize=1 for normal jobs, >1 for array jobs. // The store is updated only when the backend pushes the resulting OnJobCreated @@ -545,10 +627,11 @@ func SubmitJob(job *models.JobInfo, arraySize int) error { js.array_size = C.uint64_t(arraySize) var outJobID C.uint64_t - rc := C.xcecli_jobsubmit(gCtx.ctx, js, &outJobID) - checkAndPerformRejoin() - if rc < 0 { - return fmt.Errorf("xcecli_jobsubmit failed: %d", int(rc)) + _, err := callBackend("xcecli_jobsubmit", func() int { + return int(C.xcecli_jobsubmit(gCtx.ctx, js, &outJobID)) + }) + if err != nil { + return err } if outJobID != 0 { job.Unique_JobID = int64(outJobID) @@ -581,12 +664,10 @@ func ControlJob(jobid uint64, action int, content string) error { C.jobctrl_set_ckptoption(&ctrl, &ckpt) } - rc := C.xcecli_jobctrl(gCtx.ctx, &ctrl) - checkAndPerformRejoin() - if rc < 0 { - return fmt.Errorf("xcecli_jobctrl failed: %d", int(rc)) - } - return nil + _, err := callBackend("xcecli_jobctrl", func() int { + return int(C.xcecli_jobctrl(gCtx.ctx, &ctrl)) + }) + return err } // ControlQueue sends a queue open/close/activate/deactivate action to the @@ -600,12 +681,10 @@ func ControlQueue(queuename string, action int) error { queue: cQueue, } - rc := C.xcecli_queuectrl(gCtx.ctx, &ctrl) - checkAndPerformRejoin() - if rc < 0 { - return fmt.Errorf("xcecli_queuectrl failed: %d", int(rc)) - } - return nil + _, err := callBackend("xcecli_queuectrl", func() int { + return int(C.xcecli_queuectrl(gCtx.ctx, &ctrl)) + }) + return err } // ControlHost sends a host open/close action to the backend via xcecli_hostctrl. @@ -618,12 +697,10 @@ func ControlHost(hostname string, action int) error { hostname: cHost, } - rc := C.xcecli_hostctrl(gCtx.ctx, &ctrl) - checkAndPerformRejoin() - if rc < 0 { - return fmt.Errorf("xcecli_hostctrl failed: %d", int(rc)) - } - return nil + _, err := callBackend("xcecli_hostctrl", func() int { + return int(C.xcecli_hostctrl(gCtx.ctx, &ctrl)) + }) + return err } // signalNumber maps a job action and optional signal name to a POSIX signal @@ -660,10 +737,11 @@ func FetchFile(jobid uint64, filepath string, offset uint64, maxBytes uint32) (* } var reply *C.xce_fetchfile_reply_t - rc := C.xcecli_fetchfile(gCtx.ctx, &msg, &reply) - checkAndPerformRejoin() - if rc < 0 { - return nil, fmt.Errorf("xcecli_fetchfile failed: %d", int(rc)) + _, err := callBackend("xcecli_fetchfile", func() int { + return int(C.xcecli_fetchfile(gCtx.ctx, &msg, &reply)) + }) + if err != nil { + return nil, err } if reply == nil { return nil, fmt.Errorf("xcecli_fetchfile returned nil reply") diff --git a/src/frontend/apiserver/backend/health.go b/src/frontend/apiserver/backend/health.go new file mode 100644 index 00000000..b0ff8bfe --- /dev/null +++ b/src/frontend/apiserver/backend/health.go @@ -0,0 +1,80 @@ +package backend + +import ( + "sync/atomic" + "time" +) + +// health.go implements ApiserverBackendHealth (component-failover-adoption +// task 4.5, boundary: ApiserverBackendHealth, Req 4.1-4.2): a small, +// independent circuit breaker that lets apiserver's HTTP request handlers +// fail fast with a clear "backend unavailable" error instead of blocking on +// the client library's own retry budget (XCECLI_FAILOVER_CALL: +// max_retries=20, backoff capped at 5000ms -- roughly 1-2 minutes total, +// client_failover.c:218,220) when the backend connection is known-degraded. +// clib.go's shared callBackend helper is the only caller of IsDegraded()/ +// MarkDegraded() (Req 4.1); calls that never touch gCtx (e.g. GenerateJobID) +// don't route through callBackend and are therefore unaffected (Req 4.2). +// +// State model: a single atomic "degraded-until" unix-nano timestamp +// (int64 via sync/atomic), the same pattern heartbeat.go's +// heartbeatIntervalSecs already uses for a lock-free, concurrency-safe +// package-level value. IsDegraded() is true while time.Now() is before that +// timestamp -- process-local, in-memory only, reset on restart (matches the +// rest of apiserver's in-memory state model). +// +// Deliberately no build tag: pure Go, no CGO involved in this file at all -- +// it compiles and behaves identically under CGO_ENABLED=0 and =1, so this +// state and its tests need neither cgo nor a live backend connection. +// +// Deliberately independent of reconnect.go's needsRejoin flag (Req 2 AC5, +// a design-validation third-pass correction). This state's TTL-based +// auto-clear exists purely so a request handler eventually probes the +// backend again even without an explicit MarkHealthy() call -- a +// legitimate circuit-breaker half-open pattern. It must NEVER be read by, +// or used to gate, the rejoin-retry logic in reconnect.go/heartbeat.go: +// needsRejoin tracks "have we actually re-joined the cluster yet" and +// clears only on a genuine xcecli_join() success, with no timeout of its +// own. If this state's TTL expired before an actual rejoin succeeded, +// gating the rejoin retry on "still degraded" would silently stop retrying +// the join even though apiserver is still not actually a cluster member -- +// exactly the class of bug an earlier draft of the heartbeat-idle-path fix +// (task 4.4) introduced and design validation's third pass corrected. See +// reconnect.go's checkAndPerformRejoin, which this file's MarkHealthy() is +// called FROM (at the moment a pending rejoin's xcecli_join() call +// succeeds) but which never calls back into IsDegraded() itself. + +// degradedUntilNanos holds the unix-nano timestamp before which IsDegraded() +// reports true. Its zero value (0, i.e. the Unix epoch) means "never marked +// degraded" -- correctly reports healthy at process start, since time.Now() +// is always after the epoch. +var degradedUntilNanos int64 + +// IsDegraded reports whether the backend connection is currently considered +// degraded: true while time.Now() is before the timestamp MarkDegraded most +// recently set. Called by clib.go's shared callBackend helper as the +// pre-call fail-fast check before every gCtx.ctx-based backend call +// (Req 4.1). +func IsDegraded() bool { + return time.Now().UnixNano() < atomic.LoadInt64(°radedUntilNanos) +} + +// MarkDegraded opens the circuit for ttl: IsDegraded() reports true until +// ttl elapses or MarkHealthy() is called first, whichever happens sooner +// (a half-open circuit breaker -- the TTL alone lets a healthy backend get +// probed again even if the explicit-clear signal never fires). Called by +// clib.go's callBackend when a backend call's own outcome indicates its +// retry budget was exhausted (-E_XCE_NOT_READY). +func MarkDegraded(ttl time.Duration) { + atomic.StoreInt64(°radedUntilNanos, time.Now().Add(ttl).UnixNano()) +} + +// MarkHealthy clears the degraded state immediately, regardless of any +// pending TTL. Called by reconnect.go's checkAndPerformRejoin at the exact +// point a pending rejoin's xcecli_join() call succeeds -- a single call +// site reached from both the write-path (task 4.3) and the idle-period +// heartbeat path (task 4.4), since both already funnel through +// checkAndPerformRejoin. +func MarkHealthy() { + atomic.StoreInt64(°radedUntilNanos, 0) +} diff --git a/src/frontend/apiserver/backend/health_rejoin_test.go b/src/frontend/apiserver/backend/health_rejoin_test.go new file mode 100644 index 00000000..0f71fce3 --- /dev/null +++ b/src/frontend/apiserver/backend/health_rejoin_test.go @@ -0,0 +1,283 @@ +//go:build cgo + +package backend + +import ( + "strings" + "sync/atomic" + "testing" + "time" +) + +// health_rejoin_test.go exercises two things task 4.5 adds +// (component-failover-adoption, boundary: ApiserverBackendBridge/ +// ApiserverBackendHealth, Req 4.1-4.2): +// +// 1. clib.go's callBackend shared helper: the pre-call IsDegraded() +// fail-fast check and the post-call retry-budget-exhaustion -> +// MarkDegraded() transition. +// 2. The independence guarantee between health.go's degraded-until state +// and reconnect.go's needsRejoin/checkAndPerformRejoin -- the task's +// own critical constraint (design.md's "Deliberately independent of +// needs_rejoin" correction): IsDegraded() must never gate whether a +// rejoin gets retried. +// +// Kept //go:build cgo since callBackend/checkAndPerformRejoin/needsRejoin +// only exist in the CGO build -- this file itself does not `import "C"` (a +// _test.go file can never do that directly, golang/go#13938): callBackend's +// own signature (func(string, func() int) (int, error)) is plain Go, the +// same test seam pattern reconnect.go's performRejoinFn already +// established, so this file can drive it directly with a scripted closure +// -- no live backend connection or initialized gCtx required at all. + +// ════════════════════════════════════════════════════════════════════════ +// Req 4.1 -- callBackend's pre-call fail-fast check +// ════════════════════════════════════════════════════════════════════════ + +func TestCallBackend_Degraded_SkipsUnderlyingCallAndReturnsUnavailable(t *testing.T) { + resetHealthForTest() + defer resetHealthForTest() + MarkDegraded(1 * time.Minute) + + calls := 0 + rc, err := callBackend("xcecli_test", func() int { + calls++ + return 0 + }) + + if calls != 0 { + t.Errorf("expected the underlying call to be skipped entirely while "+ + "degraded, but it was invoked %d time(s)", calls) + } + if err != ErrBackendUnavailable { + t.Errorf("expected ErrBackendUnavailable, got %v", err) + } + if rc != 0 { + t.Errorf("expected rc=0 on the fail-fast path, got %d", rc) + } +} + +func TestCallBackend_Healthy_InvokesUnderlyingCallAndReturnsItsResult(t *testing.T) { + resetHealthForTest() + defer resetHealthForTest() + + calls := 0 + rc, err := callBackend("xcecli_test", func() int { + calls++ + return 7 + }) + + if calls != 1 { + t.Errorf("expected exactly 1 underlying call while healthy, got %d", calls) + } + if err != nil { + t.Errorf("expected no error for a non-negative rc, got %v", err) + } + if rc != 7 { + t.Errorf("expected callBackend to return the underlying rc (7), got %d", rc) + } +} + +func TestCallBackend_NegativeRc_ReturnsErrorNamingTheCall(t *testing.T) { + resetHealthForTest() + defer resetHealthForTest() + + _, err := callBackend("xcecli_test", func() int { + return -5 + }) + + if err == nil { + t.Fatal("expected an error for a negative rc") + } + if !strings.Contains(err.Error(), "xcecli_test") || !strings.Contains(err.Error(), "-5") { + t.Errorf("expected error to name the call and rc, got %q", err.Error()) + } +} + +// ════════════════════════════════════════════════════════════════════════ +// Req 4.1 -- retry-budget exhaustion (-E_XCE_NOT_READY) marks degraded +// ════════════════════════════════════════════════════════════════════════ + +func TestCallBackend_RetryBudgetExhausted_MarksDegraded(t *testing.T) { + resetHealthForTest() + defer resetHealthForTest() + + _, err := callBackend("xcecli_test", func() int { + return notReadyRC + }) + + if err == nil { + t.Fatal("expected an error for -E_XCE_NOT_READY") + } + if !IsDegraded() { + t.Error("expected IsDegraded() to be true after a call returns -E_XCE_NOT_READY") + } +} + +func TestCallBackend_OtherNegativeRc_DoesNotMarkDegraded(t *testing.T) { + resetHealthForTest() + defer resetHealthForTest() + + // A generic failure (e.g. -EINVAL) is not the same signal as + // retry-budget exhaustion -- only -E_XCE_NOT_READY should open the + // circuit. + _, err := callBackend("xcecli_test", func() int { + return -22 // -EINVAL, unrelated to retry-budget exhaustion + }) + + if err == nil { + t.Fatal("expected an error for a negative rc") + } + if IsDegraded() { + t.Error("expected IsDegraded() to remain false for a non--E_XCE_NOT_READY failure") + } +} + +// ════════════════════════════════════════════════════════════════════════ +// Structural regression guard: all wrapper functions route through the +// single shared callBackend helper, not a duplicated per-wrapper copy. +// ════════════════════════════════════════════════════════════════════════ + +func TestClibSource_BackendCallWrappersCallSharedCallBackendHelper(t *testing.T) { + clibSrc := readSourceForTest(t, "clib.go") + + count := strings.Count(clibSrc, "callBackend(") + // 5 real call sites (SubmitJob/ControlJob/ControlQueue/ControlHost/ + // FetchFile) + 1 for callBackend's own func declaration line. + if count < 6 { + t.Errorf("expected at least 6 occurrences of callBackend( in clib.go "+ + "(the function declaration plus all 5 wrapper call sites), got %d -- "+ + "the shared pre/post-call helper must not be duplicated or skipped "+ + "per wrapper", count) + } +} + +// ════════════════════════════════════════════════════════════════════════ +// Critical constraint -- IsDegraded() and the rejoin-retry mechanism +// (needsRejoin/checkAndPerformRejoin) are genuinely independent state +// machines. This is the design's own explicitly-called-out regression +// guard: an earlier draft of task 4.4's fix gated the heartbeat-idle-path +// rejoin retry on "still degraded," which would let this state's TTL +// silently stop retrying a join that never actually succeeded. +// ════════════════════════════════════════════════════════════════════════ + +func TestCheckAndPerformRejoin_FiresAndSucceedsWhileIsDegradedIsTrue(t *testing.T) { + captureLog(t) + resetNeedsRejoinForTest() + resetHealthForTest() + defer resetHealthForTest() + + // Force the circuit breaker open -- if checkAndPerformRejoin were + // (incorrectly) gated on IsDegraded(), this would suppress the retry. + MarkDegraded(1 * time.Minute) + if !IsDegraded() { + t.Fatal("setup: expected IsDegraded() to be true") + } + + atomic.StoreInt32(&needsRejoin, 1) + orig := performRejoinFn + defer func() { performRejoinFn = orig }() + + called := false + performRejoinFn = func() int { + called = true + return 0 // success + } + + checkAndPerformRejoin() + + if !called { + t.Error("expected checkAndPerformRejoin to invoke performRejoinFn even " + + "while IsDegraded() is true -- the rejoin-retry mechanism must be " + + "completely independent of the circuit-breaker's degraded state") + } + if atomic.LoadInt32(&needsRejoin) != 0 { + t.Error("expected needsRejoin to clear on a successful rejoin, " + + "regardless of IsDegraded()") + } +} + +func TestCheckAndPerformRejoin_SuccessfulRejoin_MarksHealthy(t *testing.T) { + captureLog(t) + resetNeedsRejoinForTest() + resetHealthForTest() + defer resetHealthForTest() + + // Start degraded (as would be realistic: a prior call exhausted its + // retry budget, which is presumably WHY a rejoin is now pending). + MarkDegraded(1 * time.Minute) + + atomic.StoreInt32(&needsRejoin, 1) + orig := performRejoinFn + defer func() { performRejoinFn = orig }() + performRejoinFn = func() int { return 0 } + + checkAndPerformRejoin() + + if IsDegraded() { + t.Error("expected a successful rejoin (via checkAndPerformRejoin) to " + + "call MarkHealthy() and clear the degraded state") + } +} + +func TestMarkDegraded_TTLExpiry_DoesNotTriggerOrSuppressRejoin(t *testing.T) { + resetNeedsRejoinForTest() + resetHealthForTest() + defer resetHealthForTest() + + orig := performRejoinFn + defer func() { performRejoinFn = orig }() + called := false + performRejoinFn = func() int { + called = true + return 0 + } + + // Mark degraded with a short TTL and let it expire, with NO rejoin + // pending (needsRejoin stays 0 throughout) and callAndPerformRejoin + // never invoked. + MarkDegraded(10 * time.Millisecond) + time.Sleep(40 * time.Millisecond) + + if IsDegraded() { + t.Fatal("setup: expected the TTL to have expired") + } + if called { + t.Error("expected the degraded-state TTL expiring on its own to never " + + "call performRejoinFn -- these two state machines must stay independent") + } + if atomic.LoadInt32(&needsRejoin) != 0 { + t.Error("expected needsRejoin to remain unset -- the TTL expiring has " + + "nothing to do with whether a rejoin is pending") + } + + // Also confirm the reverse direction directly through checkAndPerformRejoin: + // with needsRejoin genuinely 0 (nothing pending) and now-healthy state, + // checkAndPerformRejoin is a no-op, as it always was pre-task-4.5. + checkAndPerformRejoin() + if called { + t.Error("expected checkAndPerformRejoin to remain a no-op when " + + "needsRejoin is unset, TTL expiry or not") + } +} + +// ════════════════════════════════════════════════════════════════════════ +// Req 4.2 -- calls that don't touch gCtx are genuinely unaffected by the +// degraded state (a quick confirmation, not new test infrastructure). +// ════════════════════════════════════════════════════════════════════════ + +func TestGenerateJobID_UnaffectedByDegradedState(t *testing.T) { + resetHealthForTest() + defer resetHealthForTest() + MarkDegraded(1 * time.Minute) + + id, err := GenerateJobID(1, 1, 1) + + if err != nil { + t.Errorf("expected GenerateJobID to succeed while degraded (it never "+ + "touches gCtx / callBackend), got error: %v", err) + } + if id == 0 { + t.Error("expected a non-zero generated job ID") + } +} diff --git a/src/frontend/apiserver/backend/health_test.go b/src/frontend/apiserver/backend/health_test.go new file mode 100644 index 00000000..8576a254 --- /dev/null +++ b/src/frontend/apiserver/backend/health_test.go @@ -0,0 +1,144 @@ +package backend + +import ( + "sync" + "sync/atomic" + "testing" + "time" +) + +// health_test.go exercises health.go's degraded/healthy state machine +// (component-failover-adoption task 4.5, boundary: ApiserverBackendHealth, +// Req 4.1-4.2). Deliberately no build tag, matching health.go itself: pure +// Go, no CGO -- runs under both CGO_ENABLED=0 and =1, so this file is the +// one this package's tests can run without any backend/nix CGO setup at +// all (see the task's own validation command: `CGO_ENABLED=0 go test +// ./backend/...`). + +// resetHealthForTest restores degradedUntilNanos to its zero value (never +// degraded) between tests, mirroring reconnect_test.go's +// resetNeedsRejoinForTest for the sibling needsRejoin state. +func resetHealthForTest() { + atomic.StoreInt64(°radedUntilNanos, 0) +} + +// ════════════════════════════════════════════════════════════════════════ +// Req 4.1 -- basic degraded/healthy transitions +// ════════════════════════════════════════════════════════════════════════ + +func TestIsDegraded_FreshState_ReportsHealthy(t *testing.T) { + resetHealthForTest() + + if IsDegraded() { + t.Error("expected IsDegraded() to report false before MarkDegraded is ever called") + } +} + +func TestMarkDegraded_ThenIsDegraded_ReportsTrue(t *testing.T) { + resetHealthForTest() + + MarkDegraded(1 * time.Minute) + + if !IsDegraded() { + t.Error("expected IsDegraded() to report true immediately after MarkDegraded") + } +} + +// ════════════════════════════════════════════════════════════════════════ +// design.md's own stated validation requirement: "unit tests cover +// degraded->healthy transitions via both the TTL expiry and explicit +// MarkHealthy()" +// ════════════════════════════════════════════════════════════════════════ + +func TestMarkHealthy_ClearsDegradedStateExplicitly(t *testing.T) { + resetHealthForTest() + MarkDegraded(1 * time.Minute) + if !IsDegraded() { + t.Fatal("setup: expected degraded immediately after MarkDegraded") + } + + MarkHealthy() + + if IsDegraded() { + t.Error("expected IsDegraded() to report false immediately after MarkHealthy()") + } +} + +func TestMarkDegraded_TTLExpiry_ClearsAutomaticallyWithoutMarkHealthy(t *testing.T) { + resetHealthForTest() + + MarkDegraded(20 * time.Millisecond) + if !IsDegraded() { + t.Fatal("setup: expected degraded immediately after MarkDegraded") + } + + time.Sleep(60 * time.Millisecond) + + if IsDegraded() { + t.Error("expected IsDegraded() to report false once the TTL has elapsed, " + + "without any explicit MarkHealthy() call -- the half-open circuit " + + "breaker's own timeout must clear it") + } +} + +func TestMarkDegraded_ExtendsAnExistingDegradedWindow(t *testing.T) { + resetHealthForTest() + + MarkDegraded(20 * time.Millisecond) + time.Sleep(10 * time.Millisecond) + // A second call (e.g. a second call's own retry-budget exhaustion) + // resets the window from now, not additively -- confirms MarkDegraded + // always sets an absolute deadline, not a relative extension bug. + MarkDegraded(20 * time.Millisecond) + time.Sleep(10 * time.Millisecond) + + if !IsDegraded() { + t.Error("expected still degraded 10ms into a freshly-reset 20ms window") + } +} + +// ════════════════════════════════════════════════════════════════════════ +// Concurrent-request safety (a go test -race concern, though -race itself +// is known not to work in this ASAN-linked CGO environment -- see +// tasks.md's task 4.2 Implementation Note. This proves race-freedom by +// construction: every access goes through sync/atomic, so this test's +// value is in exercising heavy concurrent read/write without a panic or +// data corruption, not in relying on -race actually running clean here). +// ════════════════════════════════════════════════════════════════════════ + +func TestHealthState_ConcurrentReadersAndWriters_NoDataCorruption(t *testing.T) { + resetHealthForTest() + + const goroutines = 50 + const iterations = 200 + + var wg sync.WaitGroup + wg.Add(goroutines * 3) + + for i := 0; i < goroutines; i++ { + go func() { + defer wg.Done() + for j := 0; j < iterations; j++ { + _ = IsDegraded() + } + }() + go func() { + defer wg.Done() + for j := 0; j < iterations; j++ { + MarkDegraded(time.Millisecond) + } + }() + go func() { + defer wg.Done() + for j := 0; j < iterations; j++ { + MarkHealthy() + } + }() + } + + wg.Wait() + // No assertion on the final state (inherently racy which goroutine's + // write lands last) -- reaching here without a panic/deadlock, under + // concurrent atomic reads and writes from 150 goroutines, is the proof + // this test exists for. +} diff --git a/src/frontend/apiserver/backend/reconnect.go b/src/frontend/apiserver/backend/reconnect.go index e529ea2f..ea52fe3c 100644 --- a/src/frontend/apiserver/backend/reconnect.go +++ b/src/frontend/apiserver/backend/reconnect.go @@ -129,10 +129,19 @@ var performRejoinFn = performPendingRejoin // independent of whether this particular call happened to succeed). // // This is deliberately only the POST-call half of what design.md describes -// as the eventual shared helper. Task 4.5 (ApiserverBackendHealth / -// IsDegraded()) will add a PRE-call fail-fast check for the same wrapper -// call sites -- that circuit-breaker state does not exist yet, so there is -// nothing to check before the call in this task. +// as the eventual shared helper -- the PRE-call fail-fast check +// (IsDegraded(), health.go) lives in clib.go's callBackend (task 4.5), not +// here: checkAndPerformRejoin() itself never reads IsDegraded() and must +// not (see health.go's own doc comment for why the two states must stay +// independent -- gating this function's retry on "still degraded" would +// let the circuit breaker's TTL silently stop retrying a join that never +// actually succeeded). +// +// On success, also calls health.go's MarkHealthy() (task 4.5, Req 4.1/4.2): +// this is the single call site both the write-path (task 4.3, via each +// backend-call wrapper's post-call check) and the idle-period heartbeat +// path (task 4.4, via runHeartbeat's onSuccessTick) funnel through, so one +// MarkHealthy() call here covers a successful rejoin from either path. func checkAndPerformRejoin() { if atomic.LoadInt32(&needsRejoin) == 0 { return @@ -141,6 +150,7 @@ func checkAndPerformRejoin() { rc := performRejoinFn() if rc >= 0 { atomic.StoreInt32(&needsRejoin, 0) + MarkHealthy() logger.Info("checkAndPerformRejoin: rejoin after transparent reconnect succeeded") return } diff --git a/src/frontend/apiserver/backend/reconnect_test.go b/src/frontend/apiserver/backend/reconnect_test.go index 0fbc5b11..7d713029 100644 --- a/src/frontend/apiserver/backend/reconnect_test.go +++ b/src/frontend/apiserver/backend/reconnect_test.go @@ -274,19 +274,32 @@ func TestClibSource_RejoinShimExternAndRegistrationLiveInClibGo(t *testing.T) { } // ════════════════════════════════════════════════════════════════════════ -// Shared helper wraps every backend call: structural check that clib.go's -// wrapper functions actually route through checkAndPerformRejoin, not a -// duplicated per-wrapper copy of its logic. +// Shared helper wraps every backend call: structural check that +// checkAndPerformRejoin is invoked from exactly one place in clib.go. +// +// Superseded by task 4.5's consolidation (component-failover-adoption, +// boundary: ApiserverBackendBridge/ApiserverBackendHealth): task 4.3 +// originally wired checkAndPerformRejoin() into each of +// SubmitJob/ControlJob/ControlQueue/ControlHost directly (>=4 literal call +// sites, this test's original assertion). Task 4.5 folds that post-call +// check into the single shared callBackend helper (also housing the new +// pre-call IsDegraded() check), so checkAndPerformRejoin() now appears +// exactly ONCE in clib.go's source -- inside callBackend -- with every +// wrapper routing through callBackend instead +// (TestClibSource_BackendCallWrappersCallSharedCallBackendHelper, +// health_rejoin_test.go, is this test's direct successor for "wrappers +// don't duplicate the shared logic"). This test now only guards against a +// regression back to per-wrapper duplication of the rejoin check itself. // ════════════════════════════════════════════════════════════════════════ func TestClibSource_BackendCallWrappersCallSharedRejoinHelper(t *testing.T) { clibSrc := readSourceForTest(t, "clib.go") count := strings.Count(clibSrc, "checkAndPerformRejoin()") - if count < 4 { - t.Errorf("expected at least 4 call sites of checkAndPerformRejoin() "+ - "in clib.go (SubmitJob/ControlJob/ControlQueue/ControlHost at "+ - "minimum), got %d -- the shared post-call helper must not be "+ - "duplicated or skipped per wrapper", count) + if count != 1 { + t.Errorf("expected exactly 1 call site of checkAndPerformRejoin() in "+ + "clib.go (inside the shared callBackend helper, task 4.5), got %d "+ + "-- either it has been duplicated back out to individual wrappers, "+ + "or callBackend itself no longer calls it", count) } }