You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "nic-6443 (via GitHub)" <gi...@apache.org> on 2026/10/20 09:02:35 UTC
[PR] fix(proxy-cache): serve a MISS after PURGE removes a disk cache entry [apisix]
nic-6443 opened a new pull request, #14055:
URL: https://github.com/apache/apisix/pull/14055
### Description
A disk-cache `PURGE` returns 200 and does delete the right cache file, but the next identical request still comes back as `HIT`. This affects both `proxy-cache` and `graphql-proxy-cache` (disk strategy).
The cause is the `open_file_cache max=1000 inactive=60;` that the generated `nginx.conf` sets at the http level. nginx's file cache opens cache files through `open_file_cache`, so after a `HIT` the worker keeps the file descriptor around. `PURGE` removes the file from disk, but the keys zone entry is still valid, so the next request reuses the cached descriptor of the already-deleted file and serves it as a `HIT` until `open_file_cache_valid` (60s by default) runs out.
The existing purge tests don't catch this because the test-nginx http config has no `open_file_cache`.
The fix turns `open_file_cache` off in the location that serves proxy-cache / graphql-proxy-cache (and mirrors it in `t/APISIX.pm`). That location only uses open_file_cache for the proxy cache files, so nothing else changes; a cache hit now costs one `open()` of the cache file.
The regression test is a CLI test (`t/cli/test_proxy_cache_purge.sh`) because it needs the real generated `nginx.conf`: MISS, HIT, PURGE, then expect MISS, for both plugins. It fails without the template change (`HIT` after purge) and passes with it.
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [x] I have verified that this change is backward compatible
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(proxy-cache): serve a MISS after PURGE removes a disk cache entry [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 commented on PR #14055:
URL: https://github.com/apache/apisix/pull/14055#issuecomment-6312382326
Closing this one. The benchmark in the description settles it: turning `open_file_cache` off makes every disk-cache HIT pay one extra `openat` + `fstat` + `close`, about 6 µs of worker CPU per request (roughly +15% CPU for a 1KB cached body, +8% for 64KB). Plain proxying and memory-strategy cache hits are unaffected, but disk-cache hits are exactly the path people pick proxy-cache for, and paying that on every hit to make PURGE take effect immediately isn't a good trade.
So the current behaviour stays: with the disk strategy, a PURGE returns 200 and deletes the file, but a worker that already has the file's descriptor cached keeps serving it as a `HIT` until `open_file_cache_valid` (60s by default) runs out. If you need the purged content gone sooner than that, plan for up to a minute of stale hits after a PURGE.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(proxy-cache): serve a MISS after PURGE removes a disk cache entry [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 closed pull request #14055: fix(proxy-cache): serve a MISS after PURGE removes a disk cache entry
URL: https://github.com/apache/apisix/pull/14055
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(proxy-cache): serve a MISS after PURGE removes a disk cache entry [apisix]
Posted by "nic-6443 (via GitHub)" <gi...@apache.org>.
nic-6443 commented on code in PR #14055:
URL: https://github.com/apache/apisix/pull/14055#discussion_r4323464122
##########
t/cli/test_proxy_cache_purge.sh:
##########
@@ -0,0 +1,139 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+. ./t/cli/common.sh
Review Comment:
errexit is already on: the script sources `t/cli/common.sh`, which starts with `set -ex`, so a failing bare `[ ... ]` exits the script non-zero (that is the convention the other `t/cli` scripts rely on too). Without the template fix the script does exit 1 here, so the assertions do fail. Leaving them as is.
##########
t/cli/test_proxy_cache_purge.sh:
##########
@@ -0,0 +1,139 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+. ./t/cli/common.sh
+
+# The http block of the generated nginx.conf enables open_file_cache, which the
+# test-nginx based cases don't have. A disk cache entry removed by PURGE must not
+# be served again from a file descriptor cached before the purge.
+
+echo '
+deployment:
+ admin:
+ admin_key_required: false
+ admin_key: ~
+nginx_config:
+ worker_processes: 1
+ http_configuration_snippet: |
+ server {
+ listen 1980;
+ location / {
+ return 200 "hello";
+ }
+ }
+' > conf/config.yaml
+
+rm -rf /tmp/disk_cache_one/*
+make run
+sleep 1
Review Comment:
Agreed, switched to the `wait_for_tcp 127.0.0.1 9180` helper from `common.sh`, which is what the other scripts use after `make run`, in 4d7bcf44f.
##########
t/cli/test_proxy_cache_purge.sh:
##########
@@ -0,0 +1,139 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+. ./t/cli/common.sh
+
+# The http block of the generated nginx.conf enables open_file_cache, which the
+# test-nginx based cases don't have. A disk cache entry removed by PURGE must not
+# be served again from a file descriptor cached before the purge.
+
+echo '
+deployment:
+ admin:
+ admin_key_required: false
+ admin_key: ~
+nginx_config:
+ worker_processes: 1
+ http_configuration_snippet: |
+ server {
+ listen 1980;
+ location / {
+ return 200 "hello";
+ }
+ }
+' > conf/config.yaml
+
+rm -rf /tmp/disk_cache_one/*
+make run
+sleep 1
+
+admin() {
+ curl -s -o /dev/null -w "%{http_code}" http://127.0.0.1:9180/apisix/admin$1 -X PUT -d "$2"
+}
Review Comment:
The Admin API decodes the body as JSON whatever the Content-Type is, and the existing `t/cli` scripts send admin requests the same way (only 2 of 26 set the header). Keeping it consistent with them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(proxy-cache): serve a MISS after PURGE removes a disk cache entry [apisix]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #14055:
URL: https://github.com/apache/apisix/pull/14055#discussion_r4323396001
##########
t/cli/test_proxy_cache_purge.sh:
##########
@@ -0,0 +1,139 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+. ./t/cli/common.sh
Review Comment:
This test relies on bare `[` assertions later in the script (e.g., `[ \"$(cache_status $url)\" = \"MISS\" ]`). Unless `./t/cli/common.sh` enables `errexit`, these assertions won’t fail the script, which can yield false positives. Make the failure behavior explicit in this script (e.g., enable errexit/pipefail near the top, or replace the bare assertions with `if` blocks that `exit 1` on mismatch).
##########
t/cli/test_proxy_cache_purge.sh:
##########
@@ -0,0 +1,139 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+. ./t/cli/common.sh
+
+# The http block of the generated nginx.conf enables open_file_cache, which the
+# test-nginx based cases don't have. A disk cache entry removed by PURGE must not
+# be served again from a file descriptor cached before the purge.
+
+echo '
+deployment:
+ admin:
+ admin_key_required: false
+ admin_key: ~
+nginx_config:
+ worker_processes: 1
+ http_configuration_snippet: |
+ server {
+ listen 1980;
+ location / {
+ return 200 "hello";
+ }
+ }
+' > conf/config.yaml
+
+rm -rf /tmp/disk_cache_one/*
+make run
+sleep 1
+
+admin() {
+ curl -s -o /dev/null -w "%{http_code}" http://127.0.0.1:9180/apisix/admin$1 -X PUT -d "$2"
+}
Review Comment:
The Admin API payload is JSON, but the request doesn’t set `Content-Type: application/json`. If the admin endpoint becomes stricter about content types, this test will start failing for reasons unrelated to the PURGE behavior. Consider adding the explicit JSON content-type header to the admin helper.
##########
t/cli/test_proxy_cache_purge.sh:
##########
@@ -0,0 +1,139 @@
+#!/usr/bin/env bash
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+. ./t/cli/common.sh
+
+# The http block of the generated nginx.conf enables open_file_cache, which the
+# test-nginx based cases don't have. A disk cache entry removed by PURGE must not
+# be served again from a file descriptor cached before the purge.
+
+echo '
+deployment:
+ admin:
+ admin_key_required: false
+ admin_key: ~
+nginx_config:
+ worker_processes: 1
+ http_configuration_snippet: |
+ server {
+ listen 1980;
+ location / {
+ return 200 "hello";
+ }
+ }
+' > conf/config.yaml
+
+rm -rf /tmp/disk_cache_one/*
+make run
+sleep 1
Review Comment:
Using a fixed `sleep 1` after `make run` can make the test flaky on slower CI machines (service may not be ready yet) or unnecessarily slow on fast machines. Prefer a readiness check (polling the Admin API or a health endpoint with a timeout) so the test deterministically waits until APISIX is accepting requests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org