Address review comments from bryancall

- CacheHosting.cc: restrict suffix skip loops to uppercase KMGT to
  match ink_atoi64's uppercase-only handling
- RemapConfig.cc: use swoc::TextView to validate @Volume= segments
  (rejects empty, zero, out-of-range, trailing comma)
- ReverseProxy.h/cc, HttpSM.cc: use std::atomic<UrlRewrite*> for
  rewrite_table to make atomic access explicit
- cache_volume_features.replay.yaml: remove misleading @Volume=99
  remap entry and its corresponding session test

Author: zwoop

include/proxy/ReverseProxy.h

@@ -32,6 +32,8 @@
 
 #pragma once
 
+#include <atomic>
+
 #include "records/RecProcess.h"
 
 #include "tscore/ink_defs.h"
@@ -45,7 +47,7 @@
 class url_mapping;
 struct host_hdr_info;
 
-extern UrlRewrite *rewrite_table;
+extern std::atomic<UrlRewrite *> rewrite_table;
 
 // API Functions
 int init_reverse_proxy();

src/iocore/cache/CacheHosting.cc

@@ -754,7 +754,7 @@ ConfigVolumes::BuildListFromString(char *config_file_path, char *file_buf)
           err = "Invalid avg_obj_size value (must be >= 0)";
           break;
         }
-        while (*tmp && (ParseRules::is_digit(*tmp) || strchr("kmgtKMGT", *tmp))) {
+        while (*tmp && (ParseRules::is_digit(*tmp) || strchr("KMGT", *tmp))) {
           tmp++;
         }
       } else if (strcasecmp(tmp, "fragment_size") == 0) { // match fragment_size
@@ -769,7 +769,7 @@ ConfigVolumes::BuildListFromString(char *config_file_path, char *file_buf)
           err = "Invalid fragment_size value (must be >= 0)";
           break;
         }
-        while (*tmp && (ParseRules::is_digit(*tmp) || strchr("kmgtKMGT", *tmp))) {
+        while (*tmp && (ParseRules::is_digit(*tmp) || strchr("KMGT", *tmp))) {
           tmp++;
         }
       } else if (strcasecmp(tmp, "ramcache") == 0) { // match ramcache
@@ -797,7 +797,7 @@ ConfigVolumes::BuildListFromString(char *config_file_path, char *file_buf)
           break;
         }
         // Note: ram_cache_size=0 disables RAM cache for this volume, same as ramcache=false
-        while (*tmp && (ParseRules::is_digit(*tmp) || strchr("kmgtKMGT", *tmp))) {
+        while (*tmp && (ParseRules::is_digit(*tmp) || strchr("KMGT", *tmp))) {
           tmp++;
         }
       } else if (strcasecmp(tmp, "ram_cache_cutoff") == 0) { // match ram_cache_cutoff
@@ -812,7 +812,7 @@ ConfigVolumes::BuildListFromString(char *config_file_path, char *file_buf)
           err = "Invalid ram_cache_cutoff value (must be >= 0)";
           break;
         }
-        while (*tmp && (ParseRules::is_digit(*tmp) || strchr("kmgtKMGT", *tmp))) {
+        while (*tmp && (ParseRules::is_digit(*tmp) || strchr("KMGT", *tmp))) {
           tmp++;
         }
       }

src/proxy/ReverseProxy.cc

@@ -50,7 +50,7 @@ DbgCtl dbg_ctl_url_rewrite{"url_rewrite"};
 } // end anonymous namespace
 
 // Global Ptrs
-UrlRewrite                       *rewrite_table       = nullptr;
+std::atomic<UrlRewrite *>         rewrite_table       = nullptr;
 thread_local PluginThreadContext *pluginThreadContext = nullptr;
 
 // Tokens for the Callback function
@@ -67,13 +67,13 @@ thread_local PluginThreadContext *pluginThreadContext = nullptr;
 int
 init_reverse_proxy()
 {
-  ink_assert(rewrite_table == nullptr);
+  ink_assert(rewrite_table.load() == nullptr);
   reconfig_mutex = new_ProxyMutex();
-  rewrite_table  = new UrlRewrite();
+  rewrite_table.store(new UrlRewrite());
 
-  rewrite_table->acquire();
+  rewrite_table.load()->acquire();
   Note("%s loading ...", ts::filename::REMAP);
-  if (!rewrite_table->load()) {
+  if (!rewrite_table.load()->load()) {
     Emergency("%s failed to load", ts::filename::REMAP);
   } else {
     Note("%s finished loading", ts::filename::REMAP);
@@ -147,7 +147,7 @@ reloadUrlRewrite()
     newTable->acquire();
 
     // Swap configurations
-    oldTable = ink_atomic_swap(&rewrite_table, newTable);
+    oldTable = rewrite_table.exchange(newTable);
 
     ink_assert(oldTable != nullptr);
 
@@ -206,7 +206,7 @@ init_store_volume_host_records(UrlRewrite::MappingsStore &store)
 void
 init_remap_volume_host_records()
 {
-  UrlRewrite *table = rewrite_table;
+  UrlRewrite *table = rewrite_table.load(std::memory_order_acquire);
 
   if (!table) {
     return;
@@ -233,7 +233,7 @@ url_rewrite_CB(const char * /* name ATS_UNUSED */, RecDataT /* data_type ATS_UNU
 
   switch (my_token) {
   case REVERSE_CHANGED:
-    rewrite_table->SetReverseFlag(data.rec_int);
+    rewrite_table.load()->SetReverseFlag(data.rec_int);
     break;
 
   case TSNAME_CHANGED:

src/proxy/http/HttpSM.cc

@@ -332,7 +332,7 @@ HttpSM::init(bool from_early_data)
 
   t_state.http_config_param = HttpConfig::acquire();
   // Acquire a lease on the global remap / rewrite table (stupid global name ...)
-  m_remap = rewrite_table->acquire();
+  m_remap = rewrite_table.load()->acquire();
 
   // Simply point to the global config for the time being, no need to copy this
   // entire struct if nothing is going to change it.

src/proxy/http/remap/RemapConfig.cc

@@ -1228,12 +1228,31 @@ remap_parse_config_bti(const char *path, BUILD_TABLE_INFO *bti)
           goto MAP_ERROR;
         }
 
-        for (const char *p = volume_str; *p; p++) {
-          if (*p != ',' && (*p < '0' || *p > '9')) {
-            snprintf(errStrBuf, sizeof(errStrBuf), "Invalid character '%c' in @volume=%s at line %d", *p, volume_str, cln + 1);
+        {
+          swoc::TextView vol_list{volume_str};
+
+          if (vol_list.back() == ',') {
+            snprintf(errStrBuf, sizeof(errStrBuf), "Invalid @volume=%s at line %d (trailing comma)", volume_str, cln + 1);
             errStr = errStrBuf;
             goto MAP_ERROR;
           }
+          while (!vol_list.empty()) {
+            swoc::TextView span;
+            swoc::TextView token{vol_list.take_prefix_at(',')};
+            auto           n = swoc::svtoi(token, &span);
+
+            if (span.size() != token.size() || token.empty()) {
+              snprintf(errStrBuf, sizeof(errStrBuf), "Invalid @volume=%s at line %d (expected comma-separated numbers 1-255)",
+                       volume_str, cln + 1);
+              errStr = errStrBuf;
+              goto MAP_ERROR;
+            } else if (n < 1 || n > 255) {
+              snprintf(errStrBuf, sizeof(errStrBuf), "Volume number %jd out of range (1-255) in @volume=%s at line %d", n,
+                       volume_str, cln + 1);
+              errStr = errStrBuf;
+              goto MAP_ERROR;
+            }
+          }
         }
 
         // Check if cache is ready (will be true during config reload, possibly false during initial startup)

tests/gold_tests/cache/cache_volume_features.replay.yaml

@@ -78,12 +78,6 @@ autest:
         options:
           - "@volume=2"
 
-      # Invalid volume selection (should fallback)
-      - from: "http://invalid.example.com/"
-        to: "http://backend.ex:{SERVER_HTTP_PORT}/"
-        options:
-          - "@volume=99"
-
 # Proxy verifier test sessions
 sessions:
   # Test 1: Volume selection via @volume=1
@@ -144,36 +138,7 @@ sessions:
             fields:
               - [X-Volume-Test, { value: "2", as: equal }]
 
-  # Test 3: Invalid volume selection (should fallback to default)
-  - transactions:
-      - client-request:
-          method: "GET"
-          version: "1.1"
-          url: "/test_invalid"
-          headers:
-            fields:
-              - [Host, invalid.example.com]
-              - [X-Test-ID, "invalid-volume-test"]
-              - [uuid, 3]
-
-        server-response:
-          status: 200
-          reason: OK
-          headers:
-            fields:
-              - [Content-Type, "text/plain"]
-              - [Cache-Control, "max-age=3600"]
-              - [X-Volume-Test, "fallback"]
-          content:
-            data: "Content for invalid volume (fallback)"
-
-        proxy-response:
-          status: 200
-          headers:
-            fields:
-              - [X-Volume-Test, { value: "fallback", as: equal }]
-
-  # Test 4: Default volume selection (no @volume=)
+  # Test 3: Default volume selection (no @volume=)
   - transactions:
       - client-request:
           method: "GET"
@@ -202,7 +167,7 @@ sessions:
             fields:
               - [X-Volume-Test, { value: "default", as: equal }]
 
-  # Test 5: Cache hit test (verify caching works)
+  # Test 4: Cache hit test (verify caching works)
   - transactions:
       - client-request:
           method: "GET"
@@ -231,7 +196,7 @@ sessions:
             fields:
               - [X-Origin-Response, { value: "yes", as: equal }]
 
-  # Test 6: Second request for cached content (should be cache hit)
+  # Test 5: Second request for cached content (should be cache hit)
   - transactions:
       - client-request:
           method: "GET"

tests/gold_tests/cache/cache_volume_features.test.py

@@ -20,5 +20,6 @@
 - @volume= directive in remap.config for volume selection
 - Integration between both features
 '''
+# TODO: hosting.config + @volume= priority interaction test requires ats_replay.test.ext to support volume_config/hosting_config.
 
 Test.ATSReplayTest(replay_file="cache_volume_features.replay.yaml")