fix(cubeproxy): invalidate deleted sandbox route cache

[zyl1121]

Motivation

CubeProxy should stop using stale local routes after a sandbox is deleted and expose a clear response when Redis confirms that no route metadata exists.

Scanning the entire shared cache during invalidation makes lifecycle operations proportional to the global cache size and blocks the Nginx worker performing the scan. After invalidation, a confirmed route miss and a Redis lookup failure also both return HTTP 503, so clients cannot distinguish missing metadata from a temporary metadata-store failure.

What Changed

• Replace shared-cache scans with bounded invalidation of meta_cached and the fixed route metadata mirrored from Redis.
• Invalidate meta_cached first so new data-plane requests reload route metadata instead of immediately using stale per-port entries.
• Reuse the bounded helper for lifecycle deletion and explicit backend-cache invalidation without enumerating dynamic per-port keys.
• Return HTTP 404 when both supported Redis route lookups complete successfully and neither contains route metadata.
• Preserve HTTP 503 when any Redis route lookup fails and no valid route is found.
• Log confirmed route misses at warning level and add focused regression coverage.

Validation

• make -C CubeProxy test passed with the target OpenResty LuaJIT runtime.
• Regression coverage confirms that invalidation never calls get_keys(), removes the cache-hit sentinel before the fixed route keys, leaves dynamic per-port entries to TTL cleanup, and does not affect other sandboxes.
• Route lookup coverage confirms HTTP 404 for authoritative misses and HTTP 503 for Redis failures and mixed miss/error outcomes.

Scope

This PR keeps cache invalidation bounded to known per-sandbox keys. It intentionally does not scan or eagerly delete dynamic per-port entries; those entries cannot form an immediate cache hit after meta_cached is removed and remain subject to their existing TTL.

This PR does not add negative caching, change the lifecycle event protocol, or modify Redis route publication ordering. Requests for nonexistent sandbox IDs continue to perform the same Redis lookups as before.

Note: E2B Compatibility

Official E2B's data-plane proxy returns HTTP 502 when the requested sandbox cannot be found, while this PR currently returns HTTP 404 for a confirmed missing sandbox route.

Maintainer confirmation is needed on whether CubeProxy should keep HTTP 404 or use HTTP 502 for closer E2B data-plane compatibility. Redis lookup failures remain HTTP 503 in either case.

[cubesandboxbot]

A confirmed-miss 404 is an expected client outcome (request for a non-existent or already-deleted sandbox ID), but this path logs at ngx.ERR. On a busy dataplane, requests for arbitrary/unknown sandbox IDs will fill the error log and can trip error-log alerting, which is normally reserved for actual failures. Also, the "LEVEL_WARN||" prefix contradicts the ngx.ERR level used here — the two should agree. Suggest ngx.WARN (matching the prefix) or a rate-limited/deduped debug log for this case.

[cubesandboxbot]

Review: fix(cubeproxy): invalidate deleted sandbox route cache (#1252)

AI-generated review — reviewed against the master base tree; the PR head was not checked out.

Summary

The PR replaces the shared-cache scan in backend_cache.delete_sandbox with bounded invalidation of the fixed per-sandbox route keys (meta_cached + the Redis-mirrored fields), and changes a confirmed Redis route miss from HTTP 503 to HTTP 404. It adds focused regression coverage. The diff was not truncated.

Overall the change is well-scoped and internally consistent. I traced the lookup/invalidation paths against the base tree and verified:

• invalidate_sandbox deletes meta_cached first, so a subsequent data-plane request takes the miss path and reloads from Redis instead of serving the stale per-port entries — the core fix works for the sequential (non-concurrent) case.
• The fixed suffix list matches the documented sandbox:proxy hash fields in docs/dev/redis-key-spec.md (HostIP, SandboxIP, CreatedAt, AllowPublicTraffic, TrafficAccessToken, MaskRequestHost). Dynamic {port} mirror keys ({sid}:{port}:backend_*, and the {sid}:{port} metadata mirror) are never read on the cache-hit path, so leaving them to TTL is sound.
• The 404/503 split in load_sandbox_proxy_metadata is correct: only when both supported Redis keys answer authoritatively empty is a miss reported as 404; any transport failure on any key with no valid route stays 503. Valid data from any key still wins over an error on the other key.
• The new test file is consistent with the implementation (I hand-traced the mock's retry counts: 6 calls for a 2-key full failure, 4 for each mixed miss/error case), and the existing sandbox_backend_cache_test.lua is unaffected. Makefile + test_lua_syntax.sh wiring is correct.

Findings below are ordered by importance. Three are posted as inline comments on the PR.

Findings

1. Data-plane contract change: confirmed miss now returns 404 (was 503). This is the largest user-visible change and is called out in the PR body. The previous "get redis nil" path returned 503 with an ERR-level log, indistinguishable from a Redis outage. Now a nonexistent sandbox is 404. Anyone relying on 503 for "sandbox gone" (or treating 503 as retryable) will see different behavior, and the E2B-502 compatibility question raised in the PR body is effectively decided by this code as 404. Recommend confirming the status choice (404 vs 502) before merge, and checking the SDK/CubeAPI status handling across the repo.

   • Inline comment on sandbox_backend.lua:175

2. No negative caching + WARN log per miss = Redis and log amplification. Every request to a deleted/unknown sandbox re-runs the two-key Redis lookup (up to 6 calls when Redis errors) and now emits a WARN-level log. Under client retry loops or ID scanning this is both Redis load and log noise. Since a confirmed miss is an expected outcome, WARN is a loud level for it; consider DEBUG/INFO, or a short-TTL negative cache to bound the repeated lookups (the PR explicitly scopes negative caching out, so this is a tradeoff to consciously accept).

   • Inline comment on sandbox_backend.lua:175

3. Invalidation is best-effort under concurrency (pre-existing race). The scope note states dynamic per-port entries "cannot form an immediate cache hit after meta_cached is removed," but an in-flight cache-hit request that observed the stale entries before invalidation will re-write meta_cached and the per-port keys after the invalidation loop runs, re-arming the stale route for a fresh TTL. Not a regression (the old get_keys scan had the same window), but the guarantee is sequential-only; worth documenting/accepting explicitly.

   • Inline comment on backend_cache.lua:42

4. Admin response count semantics changed. backend_cache_deleted / deleted used to count every per-sandbox entry removed, including dynamic per-port keys; it now counts only the fixed keys that existed. A sandbox whose only remaining entries are dynamic will report 0 even though stale entries persist until TTL. If any caller infers cleanup completeness from this field, the meaning changed silently.

   • Inline comment on admin_phase.lua:112

5. Minor test-coverage gaps. The new sandbox_route_lookup_test.lua never exercises the mock's "recreated" branch (dead code), doesn't assert that the 404 path does not write meta_cached (i.e., no cache poisoning on the miss path — though the code is correct here), and doesn't cover the gRPC error mapping. The backend_cache_test.lua rewrite is good (it pins the "never calls get_keys" guarantee), but a test asserting meta_cached is removed before the fixed keys only verifies ordering within the loop, not against a concurrent re-arm.

Verdict

No blocking correctness bug found in the changed code; the diff is internally consistent and the tests match the implementation. The main items for the author are the status-code contract decision (404 vs E2B-502), the WARN-log/Redis amplification for repeated misses, and explicitly accepting the concurrent re-arm race. Worth a maintainer pass before merge.

[chenhengqi]

Thanks, we do have to invalidate route cache for use-case like cross-node pause/resume.

[chenhengqi]

For clients, these responses incorrectly suggest an infrastructure or backend failure even when CubeProxy has confirmed that no route exists.

I am curious about the motivation here.

• A client can check sandbox existence via control plane API.
• There are security concerns like every request of non-existence sandbox now hit Redis.

[zyl1121]

I am curious about the motivation here. A client can check sandbox existence via control plane API.

For example, an existing sandbox may be removed asynchronously when an agent operation outlives the configured auto-kill timeout. The client may still hold the sandbox’s data-plane endpoint and continue sending command, file, or service requests without checking the control plane before each request.

The problem today is that CubeProxy may return 504 while a stale local route still exists, and 503 after the route-cache TTL expires. Both look like retryable infrastructure failures, even though the sandbox no longer exists.

Once CubeProxy receives the lifecycle delete notification, invalidating the stale route and returning 404 makes that terminal state explicit.

There are security concerns like every request of non-existence sandbox now hit Redis.

On the security concern: nonexistent sandbox requests already query the Redis route keys today and return 503. This change only changes the response for a confirmed miss from 503 to 404, negative caching or rate limiting for arbitrary IDs would be a separate concern.

[chenhengqi]

I don't understand why a tombstone is needed. Deleting the HostIP key already invalidates cache.

[zyl1121]

Cache hits use the per-port backend_ip / backend_port entries plus meta_cached, so I think the key invalidation is probably meta_cached rather than HostIP, and deleting meta_cached does handle the normal case.

The issue is that deleting meta_cached only invalidates the cache at that moment. A concurrent cache-hit request may have already read the stale route and can refresh meta_cached and the per-port entries after deletion.

The tombstone is used to prevent that stale route from becoming valid again. It keeps those dynamic entries unusable until their TTL expires, without scanning the shared dict to find and delete every per-port key, which would be expensive.

[chenhengqi]

I consider this is kind of overdesigned.

As you said, A concurrent cache-hit request may have already read the stale route will cause a 504 instead of 404. I think the tombstone does not handle this well. That is to say, there is always a race window.

[zyl1121]

The race I’m trying to cover is: a request reads the cached route, the delete handler clears the cache, and that request then writes the stale route back. The tombstone cannot change that in-flight request, but it prevents subsequent requests from using the refreshed stale entry.

The chance of this race is very low. If you consider it not worth guarding against, I can remove the tombstone and keep the simpler fixed-key invalidation.

[chenhengqi]

Up to you. I tend to keep the diff minimal.

[zyl1121]

Makes sense. I’ll keep the change minimal and remove the tombstone. The delete handler will clear meta_cached together with the known fixed route-cache keys.

Separately, for a confirmed route miss / sandbox-not-found case, would you prefer aligning with E2B’s 502, or using 404 for explicit not-found semantics?

[chenhengqi]

For status code, I have no preference myself. But if we reset stale connections from CubeVS, the status code would be 502. See #992.

[cubesandboxbot]

The PR body describes a fixed-TTL per-sandbox tombstone ("POST /admin/meta/delete now writes a fixed-TTL per-sandbox tombstone before clearing known route-cache keys"; "while the tombstone exists, route resolution bypasses local cache and queries Redis"; "requests neither extend nor clear the tombstone"), but this diff implements only cache-key deletion — no tombstone is written anywhere, and nothing in resolve_backend consults one.

That leaves a re-arm window: if a request lands after this delete but while Redis still holds the sandbox's route, resolve_backend (sandbox_backend.lua:179–195) re-populates sid:meta_cached and the fixed keys. The surviving per-port entries (sid:<port>:backend_ip / sid:<port>:backend_port, left to TTL here) then become valid cache hits again for other ports until their TTLs expire — reproducing the exact stale-route → 504 symptom the PR motivation says it eliminates. The tombstone design described in the body (outliving the max route-cache TTL, not cleared by a successful lookup) would prevent this; the implemented code does not.

Either the tombstone logic is missing from this diff, or the PR body needs to be rewritten to describe the "delete fixed keys + meta_cached sentinel" approach actually implemented (including its one-request re-arm caveat).

[cubesandboxbot]

Two things worth confirming before merge:

1. gRPC ingress behavior change not mentioned in the PR. On the 9090 plaintext gRPC server ($cube_ingress_protocol=grpc), utils:respond_not_found() maps 404 → grpc-status 5 (NOT_FOUND), where a missing sandbox previously returned 503 → grpc-status 14 (UNAVAILABLE). That is a client-visible protocol change for native gRPC clients (nginx.conf:428 maps only 502/503/504 to UNAVAILABLE, so the new 404 lands on @grpc_lua_error with status 5). Probably the more correct semantic, but it should be called out.

2. Log noise / clarity. This WARN fires on every confirmed miss, including the sandbox-creation race the PR deliberately treats as authoritative — under ID-scanning traffic this is a per-request WARN. Also, since the err branch above calls respond_unavailable() (which exits), this could be elseif not metadata then for clarity. No functional issue — the pattern matches the existing respond_not_found() calls below.

[cubesandboxbot]

make test contract change: this block makes the target hard-fail (exit 1) when neither docker (with a pullable CUBE_PROXY_BASE_IMAGE) nor a host luajit is available — previously make test only ran the two bash scripts. Environments that ran make test without docker/luajit will now break. Also note the pre-existing standalone tests/sandbox_backend_cache_test.lua exercises the same resolve_backend cache/Redis paths but is not added to LUA_TESTS, so its coverage still isn't wired into make test while the two new tests are.

[chenhengqi]

@zyl1121 Sorry for the delay. Please rebase and resolve conflict.

[zyl1121]

Sorry for the delay. Please rebase and resolve conflict.

Rebased and resolved the conflict. I also removed the delete-side cache invalidation from this PR, since #1322 now covers it through the shared backend-cache helper.

The PR currently returns a 404 when no sandbox route exists, while Redis lookup failures remain 503. I’m considering changing a confirmed route miss to a 502, which would align with E2B and with the expected behavior when CubeVS resets a stale upstream connection, as discussed in #992. Which behavior would you prefer?

[chenhengqi]

@zyl1121 Do you have AI Agent update this PR automatically? It seems like it refer to #1322 which clean route cache in a bad way(Scan the entire cache in a loop).

[zyl1121]

Do you have AI Agent update this PR automatically? It seems like it refer to #1322 which clean route cache in a bad way(Scan the entire cache in a loop).

No, it wasn’t an automatic update. While rebasing, I saw that the delete-side invalidation was already in the merged code, so I checked the discussion in #1322.

That discussion initially pointed to reusing the scoped invalidation from this PR, but #1322 ultimately merged with backend_cache.delete_sandbox() handling the deletion path. I thought that means the merged implementation had superseded the overlapping logic here, so I removed it during the rebase and noted that change in my rebase update.

Looks like I misread the intent of that merge. Since the helper is now also shared with the cross-node pause/resume path, should I update the shared helper itself to use scoped invalidation, or only restore it for the delete path here?

[chenhengqi]

I think we can have a single handle_meta_delete() for both sandbox deletion and pause/resume(cross-node or not). What do you think?

[cubesandboxbot]

This new branch changes the data-plane contract for a confirmed route miss from HTTP 503 to HTTP 404, and it's worth confirming two consequences before merge:

1. Contract change / E2B 502 question. Previously an authoritative miss fell through with err set ("get redis nil") → respond_unavailable() → 503, indistinguishable from a Redis failure. Now it's 404. Any caller (SDK / CubeAPI / CLM) that matched on 503 for "sandbox gone" or that retries on 503 will behave differently. The PR body raises the E2B-502 compatibility question — this code path now bakes 404 in as the definitive contract, so the maintainer decision should land before merge.

2. WARN log + Redis amplification on every miss. Because negative caching is intentionally skipped, every request to a deleted/unknown sandbox re-runs the 2-key Redis lookup and emits a WARN-level log line with the request ID. Under retry loops or ID scanning that is both Redis load amplification and log noise. An expected miss is arguably not a WARN condition — consider DEBUG/INFO, or a short-TTL negative cache to bound the repeated lookups.

[cubesandboxbot]

The "bounded invalidation makes stale per-port entries unable to form an immediate cache hit" guarantee (and the scope note in the PR) holds only in the sequential case. A request that already observed meta_cached + the per-port backend_ip/backend_port entries just before this loop runs will, on the cache-hit path, re-write meta_cached, the per-port keys, and the optional fields (sandbox_backend.lua:167-172) after this loop deletes them — re-arming the stale route for a fresh TTL.

This race is pre-existing (the old get_keys scan had the same window), so it's not a regression, but it's worth documenting that invalidate_sandbox is best-effort under concurrency, or accepting the residual stale window explicitly.

[cubesandboxbot]

Semantics of the response count changed silently. delete_sandbox previously returned the number of every per-sandbox entry removed (fixed fields + dynamic per-port backend_* keys); invalidate_sandbox returns only the count of fixed keys that existed. For a sandbox whose only remaining cache entries are dynamic per-port keys, both /admin/meta/delete (backend_cache_deleted) and /admin/backend_cache/delete (deleted) will now report 0 even though stale entries remain until TTL. If any caller uses this field to confirm cleanup completeness, the meaning has changed — worth noting in the API docs or renaming the field to reflect what it now counts.

[zyl1121]

I think we can have a single handle_meta_delete() for both sandbox deletion and pause/resume(cross-node or not). What do you think?

Yes, that makes sense. I’ve updated it that way: both request paths now dispatch to the same handle_meta_delete(), and handle_backend_cache_delete() has been removed. The route-cache invalidation remains in the shared backend_cache.invalidate_sandbox() helper.

[chenhengqi]

LGTM. Thanks.