diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ac810eb5..fd771837 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -51,7 +51,14 @@ jobs: env: # Picked up by crates/aisix-cache/tests/redis_integration.rs. # The tests no-op when this is unset (local dev), so absence is safe. - AISIX_REDIS_URL: redis://127.0.0.1:6379 + # + # MUST NOT start with the `AISIX_` prefix — Config::load_from_path + # merges every AISIX_* env var into the root Config (via + # config-rs Environment::with_prefix("AISIX")), and Config has + # `#[serde(deny_unknown_fields)]`. An AISIX_REDIS_URL leaks into + # every Config::load_from_path test as `redis_url` and breaks + # the entire `aisix-core::config::tests` module. + CACHE_TEST_REDIS_URL: redis://127.0.0.1:6379 steps: - uses: actions/checkout@v4 - uses: dtolnay/rust-toolchain@stable @@ -66,6 +73,12 @@ jobs: - name: unit tests with coverage run: cargo llvm-cov --workspace --all-features --lcov --output-path lcov-unit.info - uses: actions/upload-artifact@v4 + # Soft until the org-level Actions storage quota refreshes (every + # 6-12h per GH). Tests passing is the gating signal here; the + # coverage artifact only feeds the advisory coverage-gate job. + # Remove this `continue-on-error` when storage usage has been + # raised or has fallen below quota. + continue-on-error: true with: name: coverage-unit path: lcov-unit.info @@ -92,6 +105,12 @@ jobs: working-directory: ui run: pnpm build - uses: actions/upload-artifact@v4 + # Soft until the org-level Actions storage quota refreshes. The + # downstream build-bin / e2e jobs depend on this artifact; if + # the upload fails their download-artifact will fail and they + # will be skipped (e2e is already continue-on-error at the job + # level; coverage-gate is advisory). Remove when quota is OK. + continue-on-error: true with: name: ui-dist path: crates/aisix-admin/ui-dist @@ -101,6 +120,11 @@ jobs: name: build aisix (instrumented) needs: build-ui runs-on: ubuntu-latest + # Soft while the artifact storage quota issue persists. This job + # only feeds the advisory e2e job; if its download-artifact for + # ui-dist fails because build-ui couldn't upload, don't fail the + # whole PR. Revert once quota is OK. + continue-on-error: true env: RUSTFLAGS: "-C instrument-coverage" LLVM_PROFILE_FILE: "coverage/aisix-%p-%m.profraw" diff --git a/crates/aisix-cache/src/redis.rs b/crates/aisix-cache/src/redis.rs index a823abf5..271349b3 100644 --- a/crates/aisix-cache/src/redis.rs +++ b/crates/aisix-cache/src/redis.rs @@ -148,7 +148,7 @@ mod tests { } // The full integration path (real Redis round-trip) lives in - // `tests/redis_integration.rs` and is opt-in via the `AISIX_REDIS_URL` + // `tests/redis_integration.rs` and is opt-in via the `CACHE_TEST_REDIS_URL` // env var so the unit-test job stays hermetic. CI runs Redis as a // service so the integration test exercises the happy path. } diff --git a/crates/aisix-cache/tests/redis_integration.rs b/crates/aisix-cache/tests/redis_integration.rs index aae06328..9b5b70a3 100644 --- a/crates/aisix-cache/tests/redis_integration.rs +++ b/crates/aisix-cache/tests/redis_integration.rs @@ -1,6 +1,6 @@ //! End-to-end Redis tests against a live Redis instance. //! -//! Runs only when `AISIX_REDIS_URL` is set (e.g. on CI which spins +//! Runs only when `CACHE_TEST_REDIS_URL` is set (e.g. on CI which spins //! `redis:7-alpine` as a service). The unit test module in //! `src/redis.rs` handles hermetic checks; this file proves the //! request → upstream → cache round-trip actually round-trips. @@ -13,7 +13,7 @@ use aisix_cache::{Cache, RedisCache}; use aisix_gateway::{ChatMessage, ChatResponse, FinishReason, UsageStats}; fn redis_url() -> Option { - std::env::var("AISIX_REDIS_URL").ok() + std::env::var("CACHE_TEST_REDIS_URL").ok() } fn sample(content: &str) -> ChatResponse { @@ -29,7 +29,7 @@ fn sample(content: &str) -> ChatResponse { #[tokio::test] async fn put_then_get_round_trips_against_real_redis() { let Some(url) = redis_url() else { - eprintln!("skipping: AISIX_REDIS_URL not set"); + eprintln!("skipping: CACHE_TEST_REDIS_URL not set"); return; }; @@ -48,7 +48,7 @@ async fn put_then_get_round_trips_against_real_redis() { #[tokio::test] async fn ttl_eviction_drops_entry_after_window() { let Some(url) = redis_url() else { - eprintln!("skipping: AISIX_REDIS_URL not set"); + eprintln!("skipping: CACHE_TEST_REDIS_URL not set"); return; }; @@ -70,7 +70,7 @@ async fn ttl_eviction_drops_entry_after_window() { #[tokio::test] async fn missing_key_returns_none() { let Some(url) = redis_url() else { - eprintln!("skipping: AISIX_REDIS_URL not set"); + eprintln!("skipping: CACHE_TEST_REDIS_URL not set"); return; }; diff --git a/crates/aisix-etcd/src/supervisor.rs b/crates/aisix-etcd/src/supervisor.rs index c42ac886..f58b5ab3 100644 --- a/crates/aisix-etcd/src/supervisor.rs +++ b/crates/aisix-etcd/src/supervisor.rs @@ -22,6 +22,7 @@ use std::collections::HashMap; use std::sync::Arc; use std::sync::Mutex; use std::time::Duration; +use tokio::task::JoinHandle; use crate::backoff::ExpBackoff; use crate::key; @@ -42,6 +43,16 @@ pub struct Supervisor { state: Mutex>, revision: Mutex, cache: SnapshotCache, + + // JoinHandles for in-flight `flush_cache` writes. Tests use + // [`Self::await_pending_cache_writes`] to deterministically wait + // for these without relying on a wall-clock sleep, which proved + // flaky on slow CI runners. Production code does not read this + // field; if a handle is dropped (e.g. during shutdown), the + // underlying write either completed or was cancelled — either + // way the on-disk cache is best-effort and the next live cycle + // re-publishes from etcd. + pending_writes: Mutex>>, } impl Supervisor

{ @@ -63,6 +74,25 @@ impl Supervisor

{ state: Mutex::new(HashMap::new()), revision: Mutex::new(0), cache, + pending_writes: Mutex::new(Vec::new()), + } + } + + /// Drain the JoinHandles for any in-flight cache writes spawned + /// by [`Self::flush_cache`] and await them. Test-only synchroniser: + /// production code never needs to block on disk persistence. + #[cfg(test)] + pub async fn await_pending_cache_writes(&self) { + let handles: Vec> = { + let mut pending = self.pending_writes.lock().unwrap(); + std::mem::take(&mut *pending) + }; + for handle in handles { + // Failures here are not test failures — a write that + // panicked is its own bug surfaced separately. We only + // need the await to deterministically order against the + // disk read that follows. + let _ = handle.await; } } @@ -230,8 +260,14 @@ impl Supervisor

{ let cache = self.cache.clone(); // Spawn the actual write so the apply path stays sync. If we // aren't inside a runtime (cache::disabled() tests), just skip. - if let Ok(handle) = tokio::runtime::Handle::try_current() { - handle.spawn(async move { cache.store(&entries, revision).await }); + // Track the JoinHandle so tests can deterministically await + // the write via [`Self::await_pending_cache_writes`] instead + // of leaning on `tokio::time::sleep`, which under CI load + // raced the spawn (~50ms wasn't enough on heavily loaded + // GitHub Actions runners). + if let Ok(rt_handle) = tokio::runtime::Handle::try_current() { + let join = rt_handle.spawn(async move { cache.store(&entries, revision).await }); + self.pending_writes.lock().unwrap().push(join); } } @@ -524,10 +560,10 @@ mod tests { )); let sup = Supervisor::with_cache(provider, "/aisix", SnapshotCache::new(&cache_path)); sup.load_once().await.unwrap(); - // Yield so the spawned cache write has a chance to complete - // before we drop the supervisor. - tokio::task::yield_now().await; - tokio::time::sleep(std::time::Duration::from_millis(50)).await; + // Deterministically wait for the spawned cache write to + // complete before we drop the supervisor. Replaces an + // earlier 50ms sleep that flaked on slow CI runners. + sup.await_pending_cache_writes().await; } // Second lifecycle: provider returns nothing, but restore_from_cache @@ -557,15 +593,15 @@ mod tests { sup.apply_put(&entry("/aisix/models/m-1", VALID_MODEL, 5)); sup.apply_put(&entry("/aisix/models/m-2", VALID_MODEL, 6)); - // Yield so the spawned cache writes have a chance to complete. - tokio::time::sleep(std::time::Duration::from_millis(50)).await; + // Wait for both spawned cache writes to flush before reading. + sup.await_pending_cache_writes().await; let cache = SnapshotCache::new(&cache_path); let (entries, _) = cache.load().expect("cache file present"); assert_eq!(entries.len(), 2); sup.apply_delete("/aisix/models/m-1"); - tokio::time::sleep(std::time::Duration::from_millis(50)).await; + sup.await_pending_cache_writes().await; let (entries, _) = cache.load().expect("cache file present"); assert_eq!(entries.len(), 1); diff --git a/docs/testing.md b/docs/testing.md index ef8292a3..90a94af4 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -71,14 +71,14 @@ Used when a test needs a real external service. Current integration suites: - `crates/aisix-cache/tests/redis_integration.rs` — runs against a - real Redis. Requires `AISIX_REDIS_URL`; tests no-op silently if the + real Redis. Requires `CACHE_TEST_REDIS_URL`; tests no-op silently if the env var is unset, so local `cargo test` stays hermetic. CI exposes the env vars these tests need: | Test | Service | Env var | |---|---|---| -| `redis_integration` | `redis:7-alpine` | `AISIX_REDIS_URL=redis://127.0.0.1:6379` | +| `redis_integration` | `redis:7-alpine` | `CACHE_TEST_REDIS_URL=redis://127.0.0.1:6379` | Future suites that need etcd, an OTLP collector, or a Langfuse mock will follow the same pattern: a service container in CI, a no-op @@ -226,7 +226,7 @@ Job descriptions: | Job | Purpose | |---|---| | `lint` | `cargo fmt --check`, `cargo clippy -D warnings`, `pnpm typecheck` | -| `rust-unit` | `cargo llvm-cov --workspace --all-features` → uploads `lcov-unit.info`. Spins a `redis:7-alpine` service so `AISIX_REDIS_URL` integration tests can run | +| `rust-unit` | `cargo llvm-cov --workspace --all-features` → uploads `lcov-unit.info`. Spins a `redis:7-alpine` service so `CACHE_TEST_REDIS_URL` integration tests can run | | `build-ui` | `pnpm build` in `ui/` → uploads `ui-dist` artifact for the next job | | `build-bin` | `cargo build` of `aisix-server` with `RUSTFLAGS=-C instrument-coverage` → uploads the binary artifact for the e2e job | | `e2e` | Spins etcd + redis services, downloads the binary artifact, runs Vitest. Currently `continue-on-error: true` while the harness stabilises across CI runners |