[PATCH] fix incorrect 304's responses when cache is unwritable

[Colm MacCarthaigh]

I finally developed some time to look into this. mod_cache doesn't
behave very nicely when the cache area fills. Of course administators
should make sure it doesn't fill in the first place, but nevertheless a
few people have hit this bug (me included) and I think mod_cache should
handle the problem gracefully.

Anyway, the problem occurs when the cache is unwritable, and mod_cache
needs to revalidate a cached entity. cache_select_url handles this by
rewriting headers_in to become a conditional request. However the code
in cache_save_filter which turns the request back into its original
(possibly unconditional) format is itself conditional on store_headers()
working. 

The patch I've attached should be reasonably self-documenting, any
questions - just ask. 

-- 
Colm MacCárthaigh                        Public Key: [EMAIL PROTECTED]

Index: mod_cache.c
===================================================================
--- mod_cache.c (revision 230608)
+++ mod_cache.c (working copy)
@@ -666,7 +666,13 @@
         ap_cache_accept_headers(cache->handle, r, 1);
     }
 
-    /* Write away header information to cache. */
+    /* Write away header information to cache. It is possible that we are
+     * trying to update headers for an entity which has already been cached.
+     * 
+     * This may fail, due to an unwritable cache area. E.g. filesystem full,
+     * permissions problems or a read-only (re)mount. This must be handled 
+     * later. 
+     */
     rv = cache->provider->store_headers(cache->handle, r, info);
 
     /* Did we just update the cached headers on a revalidated response?
@@ -675,7 +681,7 @@
      * the same way as with a regular response, but conditions are now checked
      * against the cached or merged response headers.
      */
-    if (rv == APR_SUCCESS && cache->stale_handle) {
+    if (cache->stale_handle) {
         apr_bucket_brigade *bb;
         apr_bucket *bkt;
         int status;
@@ -699,12 +705,42 @@
         }
 
         cache->block_response = 1;
+
+        /* Before returning we need to handle the possible case of an
+         * unwritable cache. Rather than leaving the entity in the cache
+         * and having it constantly re-validated, now that we have recalled 
+         * the body it is safe to try and remove the url from the cache.
+         */
+        if (rv != APR_SUCCESS) {
+            ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, r->server,
+                         "cache: updating headers with store_headers failed. "
+                         "Removing cached url.");
+    
+            if (cache->provider->remove_url(url) != OK) {
+                /* Probably a mod_disk_cache cache area has been (re)mounted 
+                 * read-only, or that there is a permissions problem. 
+                 *
+                 * XXX: right now mod_disk_cache's remove_url doesn't do
+                 * anything and always returns OK. Once it does, this codepath 
+                 * will make more sense. 
+                 */
+                ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, r->server,
+                     "cache: attempt to remove url from cache unsuccessful.");
+            }
+        }
+
         return ap_pass_brigade(f->next, bb);
     }
+  
+    if(rv != APR_SUCCESS) {
+        ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, r->server,
+                     "cache: store_headers failed");
+        ap_remove_output_filter(f);
 
-    if (rv == APR_SUCCESS) {
-        rv = cache->provider->store_body(cache->handle, r, in);
+        return ap_pass_brigade(f->next, in);
     }
+
+    rv = cache->provider->store_body(cache->handle, r, in);
     if (rv != APR_SUCCESS) {
         ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, r->server,
                      "cache: store_body failed");