feat(config): serve last-known-good values for rejected keys across resync and restart - #874
Conversation
…esync and restart A watch put that fails validation already leaves the previous good value serving, but the retention ended at the next full resync: the snapshot was rebuilt from accepted rows only, so a key whose latest etcd bytes are rejected silently vanished — an api_key would 401 identically to no such key, days after the write that broke it (issue #871, the pre- existing cliff PR #872 deliberately left in place). The supervisor now pins the serving bytes as the key's last known good the moment a replacement is rejected, and re-injects the pinned value on every resync (xDS-NACK style). Retention ends exactly when the etcd key loads cleanly again or is deleted — including a key observed absent from a resynced full read — so a pin can never outlive its key. The pinned values ride in the snapshot cache (additive stale section, same format version) so retention and the staleness clock survive restarts. Accounting picks the honest option on both axes: config_hash covers the bytes each key actually serves (pinned bytes for stale keys, nothing for rejected keys with no last good), so it neither claims the rejected bytes applied nor that the row stopped serving; resource_counts keeps counting served rows. Staleness is reported every cycle: rejected[] on /status/config gains serving_stale_since plus a recomputed serving_stale_age_seconds, the heartbeat rejected_resources entries gain stale_serving_since_unix_secs, and a per-kind aisix_config_stale_served_resources gauge lands next to the existing partially-compatible gauge with the same label-zeroing discipline. The pinned bytes rebuild through the normal loader on injection, so a last good value that is itself partially compatible keeps reporting its ignored fields next to the rejection for the same key. The stale map is deliberately uncapped and independent of the 256-cap rejection buffer: overflow there truncates a report, never takes a served row offline. managed.snapshot_cache_path becomes optional: omitted keeps today's defaults (managed on at /var/lib/aisix/config_cache.json, self-hosted etcd off), an explicit path now enables the cache in self-hosted etcd mode too, and an empty string still disables it everywhere.
…ss clock Real-etcd coverage for the issue scenario: a seeded model serves a real chat request, a schema-invalid replacement lands in etcd, and the old value keeps serving through the rejected watch put, a full process restart (snapshot-cache replay plus a live load_all resync against the still-rejected bytes), and dies only when the etcd key is deleted. The staleness surface is asserted at every step: serving_stale_since and serving_stale_age_seconds on rejected[], and the per-kind aisix_config_stale_served_resources gauge including its zeroing. The restart leg caught a real gap: a rejected watch put neither mirrored the rejected bytes into the observed-state map nor flushed the cache, so a restart inside that window rebuilt a cache shape with no pin and the staleness clock reset at boot. A rejected put now records its bytes in the state map (source_hash reflects the observed etcd state immediately, matching what a resync would report) and flushes, so a restart in any window restores the same rejected-bytes + pinned-value shape. Pinned by a new unit test asserting stale-since continuity across a restart with no intervening resync. Harness: spawnApp gains etcdPrefix and snapshotCachePath overrides and apps gain stop() (terminate without cleanup) so restart scenarios can hand etcd state and the cache file to a successor process.
|
Warning Review limit reached
Next review available in: 40 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change preserves last-known-good etcd values after rejected updates. It persists stale-serving state across restart, updates status and metrics, adds heartbeat timestamps, and introduces mode-aware snapshot cache path resolution. ChangesStale configuration retention
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant Etcd
participant Supervisor
participant SnapshotCache
participant StatusMetrics
Etcd->>Supervisor: rejected resource update
Supervisor->>Supervisor: capture last-known-good value
Supervisor->>SnapshotCache: persist stale entry
Supervisor->>StatusMetrics: publish stale timestamp and kind count
SnapshotCache-->>Supervisor: restore stale entry after restart
Supervisor->>StatusMetrics: serve retained value and recompute status
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
crates/aisix-etcd/src/supervisor.rs (2)
2015-2059: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the "pinned value no longer parses" branch.
apply_resyncdrops retention and logs ERROR when a pinned last-known-good value fails to rebuild (Lines 910-922). That branch takes a served row offline, and no test covers it. It is the only new branch inapply_resyncwithout coverage.The restart shape here is the natural place to pin it: write a cache file whose
stalevalue is invalid for this build, callrestore_from_cache, then assert the snapshot is empty and the pin is gone.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-etcd/src/supervisor.rs` around lines 2015 - 2059, Add coverage for the pinned-value parse failure in the restart test around rejected_update_keeps_last_good_serving_across_restart: seed the cache with a stale/pinned value that is invalid for the current build, call restore_from_cache, and assert the restored snapshot is empty and the rejected pin/retention is removed. Keep the existing valid-cache restart assertions unchanged.
854-932: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the stale reconciliation block from
apply_resync.
apply_resyncnow performs six distinct steps: build, stale reconciliation, pin re-validation, snapshot merge, publish, and cache flush. The function is about 90 lines. The reconciliation block at Lines 854-932 is self-contained: it consumesstats,entries, and the pre-resync state, and produces the entries to inject.Extract it into a private helper, for example
fn reconcile_stale(&self, stats: &mut BuildStats, entries: &[RawEntry], snap: &AisixSnapshot).apply_resyncthen reads as build → reconcile → publish. The coding guidelines ask for functions under 20-30 lines and one responsibility per function.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-etcd/src/supervisor.rs` around lines 854 - 932, Extract the stale-serving reconciliation logic from apply_resync into a private helper such as reconcile_stale, keeping the existing rejected-key filtering, previous-state capture, stale retention updates, and injected-entry construction together. Pass the required stats, entries, and snapshot inputs, return the entries to inject, and leave pin re-validation, snapshot merging, publishing, and cache flushing in apply_resync so its flow reads build → reconcile → publish.Source: Coding guidelines
crates/aisix-etcd/src/snapshot_cache.rs (1)
37-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving
StaleServingout ofsupervisor.
snapshot_cachenow importsStaleServingfromsupervisor, whilesupervisorimportsSnapshotCachefromsnapshot_cache. The two modules depend on each other. Rust accepts this inside one crate, so nothing breaks.
StaleServingis a persisted wire type built fromRawEntry. Defining it next toRawEntryinprovider, or insnapshot_cacheitself, makes the dependency one-directional.supervisorwould then import both types from the same direction.Also applies to: 96-103
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-etcd/src/snapshot_cache.rs` at line 37, Move the persisted wire type StaleServing out of supervisor and colocate it with RawEntry in provider or snapshot_cache, then update all imports and references, including the definitions around the additionally noted lines, so supervisor and snapshot_cache no longer depend on each other.Source: Coding guidelines
tests/e2e/src/cases/config-last-known-good-e2e.test.ts (1)
114-252: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the retention lifecycle test self-contained.
The restart test requires the rejected model and
staleSinceBeforeRestartfrom the preceding test. The deletion test also requires that retained model. A focused run or an execution-order change can therefore fail without its precondition or delete a non-stale row.Put the three stages in one lifecycle test, or create and reject a fresh model in each test.
As per coding guidelines, “Avoid explicit dependencies between tests and hidden execution order assumptions.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/src/cases/config-last-known-good-e2e.test.ts` around lines 114 - 252, The retention lifecycle tests currently depend on state created by earlier tests and their execution order. Refactor the tests around the rejected model—especially the tests covering restart and deletion—so each test establishes its own model and rejection preconditions, or combine the three stages into one self-contained lifecycle test; ensure the restart stage initializes staleSinceBeforeRestart before use and the deletion stage only removes a model created by that lifecycle.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aisix-server/src/main.rs`:
- Around line 578-586: Move snapshot-cache restoration in the startup flow
before the first required etcd connection established by
EtcdConfigProvider::connect, so Supervisor::restore_from_cache() can recover
configuration before etcd-dependent initialization. Preserve the existing
managed/self-hosted path selection and disabled behavior, and explicitly handle
an unavailable etcd connection when offline startup is supported.
In `@tests/e2e/src/harness/app.ts`:
- Around line 89-103: Update the app harness startup-failure cleanup around
spawnApp and cleanup to distinguish a reused etcdPrefix from the successor’s
temporary directory: when a prefix is supplied, never clean the shared prefix on
successor failure, and clean only successor-owned temporary resources. Require
every previously stopped app to call exit() after the successor exits, then add
a regression case that forces successor startup failure and verifies the
original etcd keys remain.
---
Nitpick comments:
In `@crates/aisix-etcd/src/snapshot_cache.rs`:
- Line 37: Move the persisted wire type StaleServing out of supervisor and
colocate it with RawEntry in provider or snapshot_cache, then update all imports
and references, including the definitions around the additionally noted lines,
so supervisor and snapshot_cache no longer depend on each other.
In `@crates/aisix-etcd/src/supervisor.rs`:
- Around line 2015-2059: Add coverage for the pinned-value parse failure in the
restart test around rejected_update_keeps_last_good_serving_across_restart: seed
the cache with a stale/pinned value that is invalid for the current build, call
restore_from_cache, and assert the restored snapshot is empty and the rejected
pin/retention is removed. Keep the existing valid-cache restart assertions
unchanged.
- Around line 854-932: Extract the stale-serving reconciliation logic from
apply_resync into a private helper such as reconcile_stale, keeping the existing
rejected-key filtering, previous-state capture, stale retention updates, and
injected-entry construction together. Pass the required stats, entries, and
snapshot inputs, return the entries to inject, and leave pin re-validation,
snapshot merging, publishing, and cache flushing in apply_resync so its flow
reads build → reconcile → publish.
In `@tests/e2e/src/cases/config-last-known-good-e2e.test.ts`:
- Around line 114-252: The retention lifecycle tests currently depend on state
created by earlier tests and their execution order. Refactor the tests around
the rejected model—especially the tests covering restart and deletion—so each
test establishes its own model and rejection preconditions, or combine the three
stages into one self-contained lifecycle test; ensure the restart stage
initializes staleSinceBeforeRestart before use and the deletion stage only
removes a model created by that lifecycle.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6da1fdd1-c80d-4b95-b753-de7c45bf01e9
📒 Files selected for processing (12)
crates/aisix-admin/src/lib.rscrates/aisix-core/src/config.rscrates/aisix-core/src/config_status.rscrates/aisix-core/src/filesource/status.rscrates/aisix-etcd/src/loader.rscrates/aisix-etcd/src/snapshot_cache.rscrates/aisix-etcd/src/supervisor.rscrates/aisix-obs/src/metrics.rscrates/aisix-server/src/heartbeat.rscrates/aisix-server/src/main.rstests/e2e/src/cases/config-last-known-good-e2e.test.tstests/e2e/src/harness/app.ts
| // Snapshot cache: persist to disk so the DP can serve traffic | ||
| // from the last-known config across CP outages and restarts. | ||
| // Disabled outside managed mode and when the operator clears the | ||
| // path explicitly. | ||
| let snapshot_cache = | ||
| if cfg.managed.is_managed() && !cfg.managed.snapshot_cache_path.is_empty() { | ||
| SnapshotCache::new(&cfg.managed.snapshot_cache_path) | ||
| } else { | ||
| SnapshotCache::disabled() | ||
| }; | ||
| // Managed mode defaults to /var/lib/aisix/config_cache.json; | ||
| // self-hosted etcd mode enables it only when the operator sets | ||
| // a path explicitly; "" disables it in either mode. | ||
| let snapshot_cache = match cfg.managed.effective_snapshot_cache_path() { | ||
| Some(path) => SnapshotCache::new(path), | ||
| None => SnapshotCache::disabled(), | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect whether provider construction awaits etcd_client::Client::connect.
rg -n -C 16 'EtcdConfigProvider|async fn connect|pub async fn connect|Client::connect' crates/aisix-etcd/src
# Confirm the startup ordering in the server.
rg -n -C 12 'EtcdConfigProvider::connect|effective_snapshot_cache_path|restore_from_cache' crates/aisix-server/src/main.rsRepository: api7/aisix
Length of output: 2028
🌐 Web query:
aisix EtcdConfigProvider connect Client::connect
💡 Result:
In the context of the AISIX AI Gateway codebase, EtcdConfigProvider::connect_client is an internal asynchronous function responsible for establishing a connection to an etcd cluster [1][2]. This method utilizes the etcd_client::Client::connect library method to initiate the gRPC connection [1][2]. Key operational details of connect_client include: 1. Connection Logic: It configures connection parameters, such as timeouts and credentials (user/password) using etcd_client::ConnectOptions [1][2]. 2. TLS/mTLS Support: The function dynamically detects whether the provided etcd endpoints use the https:// scheme [1][2]. If HTTPS is detected, it configures an etcd_client::OpenSslClientConfig to handle secure connections, including loading CA certificates and client certificate/key pairs for mTLS [1][3][4]. 3. Connectivity Probing: After establishing the connection, the function performs a connectivity check to verify the etcd server is responsive. As of recent updates, it uses a client.get("probe", None).await call for this purpose, as standard maintenance status calls were found to be incompatible with certain control plane implementations [2]. This provider is configured via the standard AISIX startup configuration (config.yaml), where users define endpoints, prefix, and optional tls settings for mTLS [5][6][7]. The implementation ensures that TLS configurations, if provided, are eagerly validated and parsed before connection attempts [4].
Citations:
- 1: fc1bc22
- 2: 559e790
- 3: P0-7: Real OpenTelemetry tracing (currently scaffold-only) #49
- 4: P1-3: JWT auth + OIDC (Entra ID / Google Workspace / Okta) for proxy clients #53
- 5: https://docs.api7.ai/ai-gateway/reference/configuration-files
- 6: https://docs.api7.ai/ai-gateway/deployment/tls-and-mtls
- 7: https://docs.api7.ai/ai-gateway/deployment/startup-configuration
🌐 Web query:
crates/aisix-etcd/src EtcdConfigProvider connect
💡 Result:
In the AISIX AI Gateway codebase, EtcdConfigProvider::connect is a method located in crates/aisix-etcd/src/etcd_provider.rs (or within the EtcdConfigProvider implementation block) that handles establishing a gRPC connection to an etcd cluster [1][2]. Key characteristics of this connection process include: 1. Protocol and Client: It utilizes the etcd-client crate (which is built on tonic for gRPC) to establish the connection [1][2]. 2. Connection Probing: Instead of using Maintenance.Status (which can return a 404 error when connecting to specific management proxies like the API7 dp-manager), the method performs a connectivity probe using client.get("probe", None) [2]. 3. TLS Support: The provider supports TLS/mTLS configurations, which are parsed from the gateway's startup configuration (e.g., config.yaml) [3][4][5]. If an https:// scheme is detected in the etcd endpoints, the provider configures an OpenSSL-based TLS stack [2][5]. This implementation includes handling for custom certificate chains, mTLS client certificates, and optional verification overrides (e.g., insecure_skip_verify) [4][2][5]. 4. Context: EtcdConfigProvider is central to how the gateway synchronizes dynamic resources (like models, API keys, and guardrails) from etcd [1][3][6]. Once connected, the supervisor spawns a long-running task to manage the etcd watch stream for real-time configuration updates [2][7].
Citations:
- 1: test(server): guard dp-manager REST clients against h2 ALPN (#535, #536) #539
- 2: 559e790
- 3: https://docs.api7.ai/ai-gateway/reference/configuration-files
- 4: fc1bc22
- 5: P1-3: JWT auth + OIDC (Entra ID / Google Workspace / Okta) for proxy clients #53
- 6: https://github.com/nxtreaming/aisix
- 7: https://github.com/api7/ai-gateway/blob/main/crates/aisix-etcd/src/supervisor.rs
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git clone --depth 1 "https://github.com/api7/aisix.git" /tmp/aisix > /tmp/clone.log 2>&1 || { cat /tmp/clone.log; exit 1; }
printf '\n--- etcd provider relevant section ---\n'
sed -n '1,240p' /tmp/aisix/crates/aisix-etcd/src/etcd_provider.rs
printf '\n--- server main startup section ---\n'
sed -n '530,610p' /tmp/aisix/crates/aisix-server/src/main.rs
printf '\n--- snapshot cache definitions/usages ---\n'
rg -n -C 6 'struct SnapshotCache|impl SnapshotCache|restore_from_cache|effective_snapshot_cache_path|SnapshotCache::new|SnapshotCache::disabled' /tmp/aisix/cratesRepository: api7/aisix
Length of output: 1999
🌐 Web query:
site:github.com/api7/aisix crates/aisix-etcd/src/etcd_provider.rs async fn connect EtcdConfigProvider
💡 Result:
In the api7/aisix repository, the async function connect_client (located within crates/aisix-etcd/src/etcd_provider.rs as part of the EtcdConfigProvider implementation) is responsible for establishing a connection to an etcd server [1][2]. Key characteristics of this function as of August 2026 include: 1. Connection Logic: It builds a connection using the etcd-client crate, configuring timeouts and optional authentication (user/password) based on the provided configuration [1][2]. 2. TLS Support: The function supports TLS (via OpenSSL) when an https:// scheme is detected in the etcd host configuration [3][4]. It enables mTLS by configuring the etcd-client with ca_cert_pem, client_cert_pem_and_key, or insecure_skip_verify options depending on the settings provided [1][2]. The implementation specifically uses the openssl-tls backend to facilitate custom verification logic, such as allowing specific hostname mismatches [4]. 3. Connectivity Probe: Instead of using Maintenance.Status, the function uses a KV.Get request (specifically fetching a probe key) as a connectivity probe, as this is compatible with the project's gRPC-based management layers [4]. 4. Evolution: The function has undergone significant refinements, including updates to how TLS configurations are parsed and applied to improve robustness and idiomatic Rust usage (e.g., transitioning from ad-hoc parsing to dedicated structs like EtcdTlsConfig) [5]. Top results: [1][3][2][4][5]
Citations:
- 1: main@{1day}...main
- 2: fc1bc22
- 3: P0-7: Real OpenTelemetry tracing (currently scaffold-only) #49
- 4: 559e790
- 5: P1-3: JWT auth + OIDC (Entra ID / Google Workspace / Okta) for proxy clients #53
🌐 Web query:
site:github.com/api7/aisix crates/aisix-server/src/main.rs Supervisor::restore_from_cache EtcdConfigProvider::connect
💡 Result:
In the AISIX project (api7/aisix), the EtcdConfigProvider::connect method—often called within the context of the Supervisor's initialization—is responsible for establishing a connection to an etcd cluster [1][2]. As of recent development (April 2026), this process involves the following key steps: 1. Configuration Validation and Client Setup: The provider validates the etcd connection configuration [1][3]. It specifically checks for HTTPS schemes in the provided endpoints to determine if TLS should be enabled [1][2]. 2. TLS and mTLS Handling: If TLS is required, the provider uses the OpenSSL backend (via etcd_client) to configure the connection [1][2][4]. It handles CA certificates, client certificates, and private keys (for mTLS), and provides options for insecure verification [1][5]. 3. Connectivity Probe: After connecting the client, the provider performs a connectivity check [2]. Instead of using Maintenance.Status (which may return a 404 error depending on the etcd deployment), it executes a KV.Get operation (using a probe key like probe) to verify that the connection is active and functional [2]. 4. Retry Logic: When initializing the supervisor (often in Supervisor::restore_from_cache or similar routines), the provider utilizes a retry mechanism (e.g., using ConstantBuilder) to handle transient connection failures during the initial connection attempt [1][3]. The Supervisor subsequently uses this established provider to maintain watch streams and synchronize configuration state from etcd [2][6].
Citations:
- 1: fc1bc22
- 2: 559e790
- 3: main@{1day}...main
- 4: P0-7: Real OpenTelemetry tracing (currently scaffold-only) #49
- 5: P1-3: JWT auth + OIDC (Entra ID / Google Workspace / Okta) for proxy clients #53
- 6: https://github.com/api7/aisix
Restore the snapshot cache before awaiting the first required etcd connections.
EtcdConfigProvider::connect establishes the etcd gRPC client, so the current startup path can fail before Supervisor::restore_from_cache() can recover configuration from disk. If etcd is required for run, restore the snapshot cache before that connection. If etcd may be unreachable for offline startup, handle that failure path intentionally.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/aisix-server/src/main.rs` around lines 578 - 586, Move snapshot-cache
restoration in the startup flow before the first required etcd connection
established by EtcdConfigProvider::connect, so Supervisor::restore_from_cache()
can recover configuration before etcd-dependent initialization. Preserve the
existing managed/self-hosted path selection and disabled behavior, and
explicitly handle an unavailable etcd connection when offline startup is
supported.
| /** | ||
| * Reuse a fixed etcd prefix instead of generating a fresh one. For | ||
| * restart scenarios: `stop()` the first app (keeps etcd data), then | ||
| * spawn a second one with the same prefix so it loads the survivor | ||
| * state. The LAST app spawned on the prefix should `exit()` to clean | ||
| * it up. | ||
| */ | ||
| etcdPrefix?: string; | ||
| /** | ||
| * `managed.snapshot_cache_path` — enables the on-disk snapshot cache | ||
| * (#871) without managed mode. Point two sequential apps (same | ||
| * `etcdPrefix`) at one path to exercise cache-restored restarts. | ||
| * The caller owns the file's lifecycle. | ||
| */ | ||
| snapshotCachePath?: string; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve a reused etcd prefix when a successor fails to start.
etcdPrefix can identify state owned by a stopped app. The startup-failure path calls cleanup with that same prefix before spawnApp retries. This deletes retained keys and invalidates the restart scenario.
Define ownership explicitly. If a prefix is reused, a failed successor must clean only its own temporary directory. It must not clean the shared etcd prefix. Also require exit() for every stopped app after the successor exits, because a successor cannot remove the predecessor's temporary directory. Add a regression case that forces a successor startup failure and verifies that the original keys remain.
As per coding guidelines, “State assumptions explicitly” and “ask for clarification when requirements are unclear.”
Also applies to: 130-136, 211-211
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn, type ChildProcess } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/e2e/src/harness/app.ts` around lines 89 - 103, Update the app harness
startup-failure cleanup around spawnApp and cleanup to distinguish a reused
etcdPrefix from the successor’s temporary directory: when a prefix is supplied,
never clean the shared prefix on successor failure, and clean only
successor-owned temporary resources. Require every previously stopped app to
call exit() after the successor exits, then add a regression case that forces
successor startup failure and verifies the original etcd keys remain.
Source: Coding guidelines
… delete path Independent audit finding (MEDIUM) on the last-known-good change: a rejected put now mirrors its bytes into the observed-state map even when the row never served, so a subsequent watch delete takes the not-present early return in apply_delete — which cleared the rejection and the pin but left the key's bytes in the state map and never re-flushed the cache. The deleted key then haunted source_hash until the next resync and its document (secret-bearing for provider keys) persisted in the cache file; a cache-restored boot during an etcd outage resurrected the rejection for a key deleted long ago. The state-map removal now runs on both delete branches, and the not-present branch syncs status and flushes the cache whenever it actually removed something. Pinned by a test asserting source_hash returns to its pre-put value the moment the key is deleted. Also from the audit: merge_snapshot and snapshot_has exhaustively destructure AisixSnapshot so adding a resource kind without wiring these paths becomes a compile error instead of a silently unpinnable kind, and the snapshot_cache_path docs state that a bare key (YAML null) reads as omitted.
Independent audit (repo guideline 8)A fresh, zero-context audit agent reviewed this PR against the six contract claims in the description (correctness, reliability, security, sensitive-info leakage, breaking changes, e2e coverage), re-running the unit suites and the e2e against a live etcd. Findings and disposition: MEDIUM — deleted never-serving key leaked into observed state (FIXED in 480dbca). Because a rejected put now mirrors its bytes into the observed-state map, a watch delete for a key that never served took the not-present early return in LOW — kind-dispatch drift guard (FIXED in 480dbca). LOW — LOW — unordered spawned cache flushes (JUSTIFIED, not fixed). All other angles came back clean: the capture invariant held against constructed adversarial sequences, single-lock discipline verified, the cache's |
1 participant
Part 2 of #871 (the delivery plan's "PR 2 — RED last-known-good across resync/restart"). PR 1 (lenient parse + tri-state reporting) landed as #872.
Problem
A watch put that fails validation already leaves the previous good value serving — but only until the next full resync or restart, which rebuild the snapshot from accepted rows only. A key whose latest etcd bytes are rejected then vanishes: an
api_keystops authenticating with a 401 byte-identical to "no such key", days after and decoupled from the write that broke it. This is the step-3 "cliff" in #871, present for every resource kind.Design
Retention (xDS-NACK style). The supervisor pins the serving bytes as the key's last known good the moment a replacement is rejected (
capture_last_goodon the rejected-put path; the same capture runs insideapply_resyncfor keys first observed rejected there). On every resync the pinned bytes re-build through the normal loader and merge into the fresh snapshot, so the typed value and every derived signal stay consistent with what actually serves. A pinned value the running build can no longer parse (a DP downgrade) drops its retention with an ERROR — the same contract as any RED row.Retention ends exactly when the etcd key stops existing or heals. A successful put for the key, a watch delete, or a resync that no longer carries the key (deletion observed via full re-read after reconnect/compaction) all remove the pin — no zombie config can outlive its key. Pinned values and their staleness clocks ride in the snapshot cache (additive
stalesection, format version unchanged: old DPs ignore it, new DPs default it), so retention survives restarts.Design decision 1 — what
config_hash/resource_countssay about a rejected-but-serving-stale row.config_hashcovers the bytes each key actually serves: observed etcd bytes for accepted keys, the pinned last-known-good bytes for stale-serving keys, and nothing for a rejected key with no last good. The hash therefore never claims the rejected bytes applied (a CP diffing its expected hash sees divergence) and never claims the row stopped serving (two DPs — one serving a pin, one that never had the value — report different hashes). The substitution is decided by the stale map, not the capped rejection buffer, so buffer overflow cannot flip a served key's hash contribution.resource_countscounts served rows, pins included — they are serving — with the stale detail carried separately (rejected[]per-row + a per-kind gauge), andstatestaysdegradedwhile any rejection exists. Pre-existing boundary, unchanged in scope: a rejected key with no pin that has been evicted from the 256-cap rejection buffer folds its observed bytes back intoconfig_hash; this predates this PR and now applies uniformly to put- and resync-observed rejections.Design decision 2 — coexistence with PR 1's YELLOW retention and the 256-cap rejection buffer. The same key can simultaneously be "new value rejected (RED)" and "serving old value that is itself YELLOW". Both signals coexist and describe different bytes:
rejected[]describes the observed etcd value,partially_compatible[]describes the served pin — on injection the pin re-runsserde_ignored, so its ignored-field paths are regenerated from the same bytes that serve (pinned bystale_last_good_that_was_yellow_keeps_its_partial_compat_signal). The stale map is a third, deliberately uncapped store, independent of both the 256-cap RED buffer and the 1024-cap YELLOW buffer: evicting there truncates a report; evicting a pin would take a served resource offline. Its size is bounded by rows that ever loaded successfully — a subset of the (uncapped) served snapshot, one extra copy of the raw bytes per rejected row.Staleness is reported every cycle, on the PR 1 surfaces.
GET /status/configrejected[]entries gainserving_stale_since(RFC3339) andserving_stale_age_seconds(recomputed per read); absent both when nothing serves for the row, so consumers can tell "serving stale" from "actually down". Heartbeatrejected_resources[]entries gainstale_serving_since_unix_secs(omitted when absent; the CP heartbeat handler tolerates unknown fields — verified in #872). Metrics gainaisix_config_stale_served_resources{kind}with the same stale-label zeroing as the PR 1 gauge. Thesinceinstant persists in the cache, so the age is continuous across restarts instead of resetting at boot.Behavior change: a rejected live put now enters the observed-state map. Previously the bytes only reached
source_hashat the next resync; nowsource_hashreflects the observed etcd state immediately and — the load-bearing part — a restart inside the rejected-put window restores the same rejected-bytes + pinned-value cache shape a post-resync restart would, keeping the staleness clock continuous. The e2e restart leg caught this gap live (the clock reset by 2s across the restart before the fix).Config:
managed.snapshot_cache_pathbecomes optional. Omitted keeps today's defaults exactly (managed on at/var/lib/aisix/config_cache.json, self-hosted etcd off); an explicit path now enables the cache in self-hosted etcd mode too (previously silently ignored there — anyone who set it wanted it);""still disables everywhere. This is DP-localconfig.yaml, not a CP-managed resource, so nocp-admin.yamlcounterpart applies.Testing
Reproducing tests were written first and failed on the pre-PR code at the resync cliff (row count 1 → 0), then flipped green:
config_hashequals the served-bytes hash and diverges fromsource_hash, pin persists for an immediate restart with a continuous clock, cache format round-trips the stale section and still loads legacy files.serving_stale_since/age reported and the gauge at 1 → full process restart on the same prefix + cache (a sentinel write proves the live load_all/resync completed against the still-rejected bytes) → chat still 200,serving_stale_sincebyte-identical across the restart → etcd delete → chat 404,rejected[]empty, gauge zeroed.Prior art
The issue's prior-art table (#871) surveys mainstream proxies/gateways and config-delivery systems: the dominant pattern for hard-invalid dynamic config is NACK-with-last-valid-config-retained (the xDS ACK/NACK model), and per-key last-known-good retention at the data plane appears in the surveyed gateway with hybrid CP/DP deployment and in the row-strict gateway of the same family. This PR lands that shape: whole-row rejection stays (RED is still RED), but rejection of an update no longer takes down the running value, and the divergence is continuously observable.
Out of scope / follow-ups
Closes #871.
Summary by CodeRabbit
New Features
Bug Fixes