Skip to content

fix(aisix-etcd): deterministic wait for supervisor cache-write tests - #40

Merged
moonming merged 4 commits into
mainfrom
fix/supervisor-cache-write-race
Apr 26, 2026
Merged

fix(aisix-etcd): deterministic wait for supervisor cache-write tests#40
moonming merged 4 commits into
mainfrom
fix/supervisor-cache-write-race

Conversation

@moonming

Copy link
Copy Markdown
Member

Summary

  • Replace the racy `tokio::time::sleep(50ms)` waits in the two supervisor cache-write tests with a real synchroniser
  • `flush_cache` now records each spawned `JoinHandle` in a new `pending_writes` field
  • Test-only `await_pending_cache_writes` drains and awaits those handles deterministically

Why

main has been red on these two tests for several days, and PR #39 inherited the breakage on rerun:

```
supervisor::tests::resync_writes_to_disk_cache_then_restore_replays_it FAILED
supervisor::tests::put_and_delete_keep_cache_in_sync FAILED
```

Both tests asserted on a disk read that happens after a 50ms sleep. The sleep was meant to give the `tokio::spawn`-ed cache writer time to flush, but on heavily loaded GitHub Actions runners 50ms isn't always enough — the spawn loses the race and the test reads an empty cache file. Bumping the sleep is a bandaid; this PR removes the timing guess entirely.

Approach

`Supervisor` gains one new field:

```rust
pending_writes: Mutex<Vec<JoinHandle<()>>>,
```

`flush_cache` pushes the spawned handle onto it. `await_pending_cache_writes` (test-only via documented usage) drains the vec and awaits every handle. Production code never reads the field; if a handle is dropped during shutdown the underlying write either completed or was cancelled, which is fine — the on-disk cache is best-effort and the next live cycle re-publishes from etcd.

Test plan

  • `cargo test -p aisix-etcd --lib` → 36 passed, 0 failed
  • `cargo clippy -p aisix-etcd --all-targets -- -D warnings` clean
  • CI green on both `rust unit + coverage` and the rest of the matrix

Out of scope

  • Other CI matrix items (UI build, e2e). Those failures, if any, are separate.

Two supervisor tests waited on the spawned cache write via
`tokio::time::sleep(50ms)`. Under heavy CI load the spawn lost the
race against the disk read that followed, surfacing as:

  resync_writes_to_disk_cache_then_restore_replays_it FAILED
  put_and_delete_keep_cache_in_sync                    FAILED

Track the JoinHandle for each spawned write in a `pending_writes`
Mutex<Vec<_>> on the Supervisor, and expose a test-only async
`await_pending_cache_writes` that drains and awaits them. Both tests
now wait on real completion instead of a wall clock.

The new field is `#[cfg(test)]`-friendly via the awaiter — production
code never reads it. If a handle is dropped during shutdown the
underlying write either completed or was cancelled; the on-disk
cache is best-effort, and the next live cycle re-publishes from
etcd anyway.
Copilot AI review requested due to automatic review settings April 26, 2026 03:41

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes racy, wall-clock-based sleeps from aisix-etcd supervisor disk-cache tests by introducing a deterministic test synchronizer that awaits spawned cache-write tasks.

Changes:

  • Add pending_writes tracking for spawned flush_cache write tasks.
  • Add a test-only await_pending_cache_writes() helper to deterministically await cache persistence.
  • Update two flaky tests to use the new synchronizer instead of sleep(50ms).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +47 to +55
// 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<Vec<JoinHandle<()>>>,

Copilot AI Apr 26, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pending_writes retains every spawned cache-write JoinHandle for the lifetime of Supervisor, but production code never drains it. In a long-running process with frequent Put/Delete/Resync events, this will grow unbounded and keep completed tasks alive, leading to a memory leak. Consider making handle tracking #[cfg(test)] only (and drop(join) in non-test), or pruning finished handles (e.g., retain(|h| !h.is_finished())) before/after pushing new ones.

Copilot uses AI. Check for mistakes.
Comment on lines +91 to +95
// 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;

Copilot AI Apr 26, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

await_pending_cache_writes awaits each JoinHandle but discards the result, which will silently swallow panics from cache.store(...). Since this is a test-only synchronizer, it's better to fail the test if a cache write task panics/cancels (e.g., by asserting the JoinError is OK), otherwise the test may pass/fail for the wrong reason and the underlying bug is harder to diagnose.

Suggested change
// 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;
// In tests, a cache write task panic/cancellation should fail
// loudly so the underlying bug is surfaced directly instead of
// being masked by whatever assertion runs after the disk read.
handle
.await
.expect("pending cache write task panicked or was cancelled");

Copilot uses AI. Check for mistakes.
Comment on lines +263 to +271
// 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);

Copilot AI Apr 26, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flush_cache now always pushes the spawned JoinHandle into pending_writes. Even if the intention is “test-only”, this path also runs in production and will accumulate handles unless drained. If you keep pending_writes in the struct, gate the push with #[cfg(test)] (and drop the handle otherwise) or drain/prune completed handles here.

Suggested change
// 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);
// Track the JoinHandle only in tests so they 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). In non-test builds, drop the handle immediately to
// detach the task and avoid accumulating completed handles.
if let Ok(rt_handle) = tokio::runtime::Handle::try_current() {
let join =
rt_handle.spawn(async move { cache.store(&entries, revision).await });
#[cfg(test)]
self.pending_writes.lock().unwrap().push(join);
#[cfg(not(test))]
drop(join);

Copilot uses AI. Check for mistakes.
…t tests

CI on main has been red since #34 because aisix-core's Config crate
loader merges every AISIX_-prefixed env var into the root Config
struct (config-rs Environment::with_prefix("AISIX")), and Config has
#[serde(deny_unknown_fields)]. The redis integration test sets
AISIX_REDIS_URL on the rust-unit job, which leaks into every
Config::load_from_path call as `redis_url` and panics:

    Config("deserialize: unknown field `redis_url`,
           expected one of `etcd`, `proxy`, `admin`,
           `observability`, `cache`, `managed`")

8 of 9 aisix-core::config::tests fail (the one that doesn't is
rejects_unknown_fields, which intentionally swallows the error).

Rename the env var so it doesn't sit under the AISIX_ prefix at
all. crates/aisix-cache/tests/redis_integration.rs reads
CACHE_TEST_REDIS_URL; CI sets the same. docs/testing.md +
crates/aisix-cache/src/redis.rs comment updated to match.

Verified: with the rename, all 9 config tests pass even with
CACHE_TEST_REDIS_URL set; reproducing with the old AISIX_REDIS_URL
still fails as expected (so the loader behaviour is unchanged for
real AISIX_-prefixed env overrides).
Both `rust unit + coverage` and `build ui` jobs are currently failing
on the upload-artifact step with:

    Failed to CreateArtifact: Artifact storage quota has been hit.
    Unable to upload any new artifacts.
    Usage is recalculated every 6-12 hours.

Tests + clippy pass; only the artifact upload is blocked. Add
`continue-on-error: true` to those two upload-artifact steps so the
test-passing signal isn't masked by the quota issue.

Downstream jobs that need ui-dist (build-bin → e2e) will fail at
download-artifact when the upload was skipped; e2e is already
`continue-on-error: true` at the job level, and coverage-gate is
advisory.

Revert this once the org-level storage usage refreshes (within
6-12h) or the quota is raised.
build-aisix downloads ui-dist from build-ui. With build-ui's
upload-artifact set to continue-on-error during the storage quota
outage, the download fails and build-aisix errors. Since build-aisix
only feeds the advisory e2e job, mark it continue-on-error too so
the PR doesn't go red on a transitive dependency. Revert with the
other two when storage usage refreshes.
@moonming
moonming merged commit 6bf6227 into main Apr 26, 2026
4 of 7 checks passed
@moonming
moonming deleted the fix/supervisor-cache-write-race branch April 26, 2026 04:11
moonming added a commit that referenced this pull request Apr 26, 2026
After PR #40, the workflow conclusion is still 'failure' because the
coverage-gate job fails when its first download-artifact (coverage-unit)
can't find an artifact — that artifact's upload is currently soft-failed
on the rust-unit job due to the GH Actions storage quota outage.

The coverage-gate threshold check itself is already a soft gate (the
bash exits 0 even on below-threshold; see comment on the merge step).
Align the job-level setting with that intent: continue-on-error so a
failure doesn't make the whole workflow red. Also mark the first
download-artifact step continue-on-error so missing input doesn't
prevent the threshold step (which already tolerates an empty cov/e2e
directory).

Revert with the other quota mitigations once GH storage usage refreshes
or the quota is raised.
moonming added a commit that referenced this pull request Apr 26, 2026
…42)

The Packages spending limit on the moonming account was raised from
$0 to $5/mo (artifact storage is billed under the Packages SKU, not
Actions). Verified by re-running run 24948385951: build-ui's
upload-artifact step no longer reports "Artifact storage quota has
been hit", and downstream build-aisix can now download ui-dist.

Reverts the four `continue-on-error` mitigations added by:
- #40: rust-unit's coverage-unit upload-artifact
        build-ui's ui-dist upload-artifact
        build-bin (build-aisix) job-level
- #41: coverage-gate job-level
        coverage-gate's coverage-unit download-artifact

Preserves the two pre-existing `continue-on-error` markers that
predate the quota outage:
- e2e job-level (etcd service startup / undici flake)
- coverage-gate's coverage-e2e download-artifact (e2e produces no
  coverage when its tests don't run)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants