Address code review feedback: use exception hierarchy, report total time in seconds, periodic stats updates

Co-authored-by: bbockelm <1093447+bbockelm@users.noreply.github.com>

Author: Copilot

src/scitokens.h

@@ -335,7 +335,7 @@ int scitoken_config_get_str(const char *key, char **output, char **err_msg);
  * - successful_validations: count of successful token validations
  * - unsuccessful_validations: count of failed token validations
  * - expired_tokens: count of expired tokens encountered
- * - average_validation_time_ms: average validation time in milliseconds
+ * - total_validation_time_s: total validation time in seconds
  * - failed_issuer_lookups: count of failed issuer lookups (limited to prevent DDoS)
  *
  * The returned string must be freed by the caller using free().

src/scitokens_internal.cpp

@@ -652,35 +652,41 @@ SciToken::deserialize_continue(std::unique_ptr<SciTokenAsyncStatus> status) {
 std::unique_ptr<AsyncStatus>
 Validator::get_public_keys_from_web(const std::string &issuer,
                                     unsigned timeout) {
-    std::string openid_metadata, oauth_metadata;
-    get_metadata_endpoint(issuer, openid_metadata, oauth_metadata);
-
-    std::unique_ptr<AsyncStatus> status(new AsyncStatus());
-    status->m_oauth_metadata_url = oauth_metadata;
-    status->m_cget.reset(new internal::SimpleCurlGet(1024 * 1024, timeout));
-    auto cget_status = status->m_cget->perform_start(openid_metadata);
-    status->m_continue_fetch = true;
-    if (!cget_status.m_done) {
-        return status;
-    }
-    return get_public_keys_from_web_continue(std::move(status));
+    try {
+        std::string openid_metadata, oauth_metadata;
+        get_metadata_endpoint(issuer, openid_metadata, oauth_metadata);
+
+        std::unique_ptr<AsyncStatus> status(new AsyncStatus());
+        status->m_oauth_metadata_url = oauth_metadata;
+        status->m_cget.reset(new internal::SimpleCurlGet(1024 * 1024, timeout));
+        auto cget_status = status->m_cget->perform_start(openid_metadata);
+        status->m_continue_fetch = true;
+        if (!cget_status.m_done) {
+            return status;
+        }
+        return get_public_keys_from_web_continue(std::move(status));
+    } catch (const CurlException &e) {
+        // Rethrow CURL errors during issuer key fetch as IssuerLookupException
+        throw IssuerLookupException(e.what());
+    }
 }
 
 std::unique_ptr<AsyncStatus> Validator::get_public_keys_from_web_continue(
     std::unique_ptr<AsyncStatus> status) {
-    char *buffer;
-    size_t len;
+    try {
+        char *buffer;
+        size_t len;
 
-    switch (status->m_state) {
+        switch (status->m_state) {
 
-    case AsyncStatus::DOWNLOAD_METADATA: {
-        auto cget_status = status->m_cget->perform_continue();
-        if (!cget_status.m_done) {
-            return std::move(status);
-        }
+        case AsyncStatus::DOWNLOAD_METADATA: {
+            auto cget_status = status->m_cget->perform_continue();
+            if (!cget_status.m_done) {
+                return std::move(status);
+            }
         if (cget_status.m_status_code != 200) {
             if (status->m_oauth_fallback) {
-                throw CurlException("Failed to retrieve metadata provider "
+                throw IssuerLookupException("Failed to retrieve metadata provider "
                                     "information for issuer.");
             } else {
                 status->m_oauth_fallback = true;
@@ -729,7 +735,7 @@ std::unique_ptr<AsyncStatus> Validator::get_public_keys_from_web_continue(
             return std::move(status);
         }
         if (cget_status.m_status_code != 200) {
-            throw CurlException("Failed to retrieve the issuer's key set");
+            throw IssuerLookupException("Failed to retrieve the issuer's key set");
         }
 
         status->m_cget->get_data(buffer, len);
@@ -762,6 +768,14 @@ std::unique_ptr<AsyncStatus> Validator::get_public_keys_from_web_continue(
 
     } // Switch
     return std::move(status);
+    } catch (const CurlException &e) {
+        // Rethrow CURL errors during issuer key fetch as IssuerLookupException
+        // (unless it's already an IssuerLookupException)
+        if (dynamic_cast<const IssuerLookupException*>(&e)) {
+            throw;
+        }
+        throw IssuerLookupException(e.what());
+    }
 }
 
 std::string Validator::get_jwks(const std::string &issuer) {

src/scitokens_internal.h

@@ -121,8 +121,7 @@ struct IssuerStats {
     std::atomic<uint64_t> successful_validations{0};
     std::atomic<uint64_t> unsuccessful_validations{0};
     std::atomic<uint64_t> expired_tokens{0};
-    std::atomic<uint64_t> total_time_ns{0}; // Total time in nanoseconds
-    std::atomic<uint64_t> validation_count{0}; // For computing average
+    std::atomic<double> total_time_s{0.0}; // Total time in seconds
 };
 
 /**
@@ -134,10 +133,13 @@ class MonitoringStats {
   public:
     static MonitoringStats &instance();
 
+    // Get a reference to issuer stats for periodic updates
+    IssuerStats* get_issuer_stats(const std::string &issuer);
+    
     void record_validation_success(const std::string &issuer,
-                                    uint64_t duration_ns);
+                                    double duration_s);
     void record_validation_failure(const std::string &issuer,
-                                    uint64_t duration_ns);
+                                    double duration_s);
     void record_expired_token(const std::string &issuer);
     void record_failed_issuer_lookup(const std::string &issuer);
 
@@ -180,6 +182,17 @@ class CurlException : public std::runtime_error {
     explicit CurlException(const std::string &msg) : std::runtime_error(msg) {}
 };
 
+class IssuerLookupException : public CurlException {
+  public:
+    explicit IssuerLookupException(const std::string &msg) : CurlException(msg) {}
+};
+
+class TokenExpiredException : public JWTVerificationException {
+  public:
+    explicit TokenExpiredException(const std::string &msg)
+        : JWTVerificationException(msg) {}
+};
+
 class MissingIssuerException : public std::runtime_error {
   public:
     MissingIssuerException()
@@ -473,21 +486,39 @@ class Validator {
     void verify(const SciToken &scitoken, time_t expiry_time) {
         std::string issuer = "";
         auto start_time = std::chrono::steady_clock::now();
-        bool has_issuer = false;
+        internal::IssuerStats* stats_ptr = nullptr;
 
         try {
-            // Try to extract issuer for monitoring
-            if (scitoken.m_decoded && scitoken.m_decoded->has_payload_claim("iss")) {
-                issuer = scitoken.m_decoded->get_issuer();
-                has_issuer = true;
+            auto result = verify_async(scitoken);
+            
+            // Extract issuer from the result's JWT string after decoding starts
+            const jwt::decoded_jwt<jwt::traits::kazuho_picojson> *jwt_decoded =
+                scitoken.m_decoded.get();
+            if (jwt_decoded && jwt_decoded->has_payload_claim("iss")) {
+                issuer = jwt_decoded->get_issuer();
+                stats_ptr = internal::MonitoringStats::instance().get_issuer_stats(issuer);
             }
 
-            auto result = verify_async(scitoken);
             while (!result->m_done) {
                 auto timeout_val = result->get_timeout_val(expiry_time);
+                // Limit select to 50ms for periodic updates
+                if (timeout_val.tv_sec > 0 || timeout_val.tv_usec > 50000) {
+                    timeout_val.tv_sec = 0;
+                    timeout_val.tv_usec = 50000;
+                }
+                
                 select(result->get_max_fd() + 1, result->get_read_fd_set(),
                        result->get_write_fd_set(), result->get_exc_fd_set(),
                        &timeout_val);
+                       
+                // Update elapsed time periodically
+                if (stats_ptr) {
+                    auto current_time = std::chrono::steady_clock::now();
+                    auto duration = std::chrono::duration_cast<std::chrono::duration<double>>(
+                        current_time - start_time);
+                    stats_ptr->total_time_s = duration.count();
+                }
+                
                 if (time(NULL) >= expiry_time) {
                     throw CurlException("Timeout when loading the OIDC metadata.");
                 }
@@ -496,17 +527,20 @@ class Validator {
             }
 
             // Record successful validation
-            if (has_issuer) {
+            if (!issuer.empty()) {
                 auto end_time = std::chrono::steady_clock::now();
-                auto duration = std::chrono::duration_cast<std::chrono::nanoseconds>(
+                auto duration = std::chrono::duration_cast<std::chrono::duration<double>>(
                     end_time - start_time);
                 internal::MonitoringStats::instance().record_validation_success(
                     issuer, duration.count());
             }
         } catch (const std::exception &e) {
             // Record failure if we have an issuer
-            if (has_issuer) {
-                record_validation_error(issuer, e, start_time);
+            if (!issuer.empty()) {
+                auto end_time = std::chrono::steady_clock::now();
+                auto duration = std::chrono::duration_cast<std::chrono::duration<double>>(
+                    end_time - start_time);
+                record_validation_error(issuer, e, duration.count());
             }
             throw;
         }
@@ -515,13 +549,11 @@ class Validator {
     void verify(const jwt::decoded_jwt<jwt::traits::kazuho_picojson> &jwt) {
         std::string issuer = "";
         auto start_time = std::chrono::steady_clock::now();
-        bool has_issuer = false;
 
         try {
             // Try to extract issuer for monitoring
             if (jwt.has_payload_claim("iss")) {
                 issuer = jwt.get_issuer();
-                has_issuer = true;
             }
 
             auto result = verify_async(jwt);
@@ -530,17 +562,20 @@ class Validator {
             }
 
             // Record successful validation
-            if (has_issuer) {
+            if (!issuer.empty()) {
                 auto end_time = std::chrono::steady_clock::now();
-                auto duration = std::chrono::duration_cast<std::chrono::nanoseconds>(
+                auto duration = std::chrono::duration_cast<std::chrono::duration<double>>(
                     end_time - start_time);
                 internal::MonitoringStats::instance().record_validation_success(
                     issuer, duration.count());
             }
         } catch (const std::exception &e) {
             // Record failure if we have an issuer
-            if (has_issuer) {
-                record_validation_error(issuer, e, start_time);
+            if (!issuer.empty()) {
+                auto end_time = std::chrono::steady_clock::now();
+                auto duration = std::chrono::duration_cast<std::chrono::duration<double>>(
+                    end_time - start_time);
+                record_validation_error(issuer, e, duration.count());
             }
             throw;
         }
@@ -652,7 +687,17 @@ class Validator {
 
         const jwt::decoded_jwt<jwt::traits::kazuho_picojson> jwt(
             status->m_jwt_string);
-        verifier.verify(jwt);
+        try {
+            verifier.verify(jwt);
+        } catch (const std::exception &e) {
+            // Check if this is an expiration error from jwt-cpp
+            std::string error_msg = e.what();
+            if (error_msg.find("exp") != std::string::npos ||
+                error_msg.find("expir") != std::string::npos) {
+                throw TokenExpiredException(error_msg);
+            }
+            throw;
+        }
 
         bool must_verify_everything = true;
         if (jwt.has_payload_claim("ver")) {
@@ -927,29 +972,18 @@ class Validator {
      */
     void record_validation_error(const std::string &issuer,
                                   const std::exception &e,
-                                  const std::chrono::steady_clock::time_point &start_time) {
-        auto end_time = std::chrono::steady_clock::now();
-        auto duration = std::chrono::duration_cast<std::chrono::nanoseconds>(
-            end_time - start_time);
-        
-        std::string error_msg = e.what();
-        
-        // Check if this is a failed issuer lookup (network/DNS errors)
-        if (error_msg.find("resolve") != std::string::npos ||
-            error_msg.find("host") != std::string::npos ||
-            error_msg.find("network") != std::string::npos ||
-            error_msg.find("Failed to retrieve") != std::string::npos) {
+                                  double duration_s) {
+        // Check exception type instead of string introspection
+        if (dynamic_cast<const IssuerLookupException*>(&e)) {
             internal::MonitoringStats::instance().record_failed_issuer_lookup(issuer);
         }
 
-        // Check if this is an expiration error
-        if (error_msg.find("exp") != std::string::npos ||
-            error_msg.find("expir") != std::string::npos) {
+        if (dynamic_cast<const TokenExpiredException*>(&e)) {
             internal::MonitoringStats::instance().record_expired_token(issuer);
         }
 
         internal::MonitoringStats::instance().record_validation_failure(
-            issuer, duration.count());
+            issuer, duration_s);
     }
 
     bool m_validate_all_claims{true};

src/scitokens_monitoring.cpp

@@ -16,22 +16,25 @@ MonitoringStats &MonitoringStats::instance() {
     return instance;
 }
 
+IssuerStats* MonitoringStats::get_issuer_stats(const std::string &issuer) {
+    std::lock_guard<std::mutex> lock(m_mutex);
+    return &m_issuer_stats[issuer];
+}
+
 void MonitoringStats::record_validation_success(const std::string &issuer,
-                                                 uint64_t duration_ns) {
+                                                 double duration_s) {
     std::lock_guard<std::mutex> lock(m_mutex);
     auto &stats = m_issuer_stats[issuer];
     stats.successful_validations++;
-    stats.total_time_ns += duration_ns;
-    stats.validation_count++;
+    stats.total_time_s = stats.total_time_s.load() + duration_s;
 }
 
 void MonitoringStats::record_validation_failure(const std::string &issuer,
-                                                 uint64_t duration_ns) {
+                                                 double duration_s) {
     std::lock_guard<std::mutex> lock(m_mutex);
     auto &stats = m_issuer_stats[issuer];
     stats.unsuccessful_validations++;
-    stats.total_time_ns += duration_ns;
-    stats.validation_count++;
+    stats.total_time_s = stats.total_time_s.load() + duration_s;
 }
 
 void MonitoringStats::record_expired_token(const std::string &issuer) {
@@ -112,17 +115,8 @@ std::string MonitoringStats::get_json() const {
                 stats.unsuccessful_validations.load()));
         issuer_obj["expired_tokens"] =
             picojson::value(static_cast<double>(stats.expired_tokens.load()));
-
-        uint64_t validation_count = stats.validation_count.load();
-        if (validation_count > 0) {
-            uint64_t total_time_ns = stats.total_time_ns.load();
-            double avg_time_ms =
-                static_cast<double>(total_time_ns) / validation_count / 1e6;
-            issuer_obj["average_validation_time_ms"] =
-                picojson::value(avg_time_ms);
-        } else {
-            issuer_obj["average_validation_time_ms"] = picojson::value(0.0);
-        }
+        issuer_obj["total_validation_time_s"] =
+            picojson::value(stats.total_time_s.load());
 
         std::string sanitized_issuer = sanitize_issuer_for_json(issuer);
         issuers_obj[sanitized_issuer] = picojson::value(issuer_obj);