Copilot commented on code in PR #13262:
URL: https://github.com/apache/trafficserver/pull/13262#discussion_r3404732789


##########
include/tscore/DiagsTypes.h:
##########
@@ -160,10 +343,31 @@ class Diags : public DebugInterface
   // low-level tag inquiry functions //
   /////////////////////////////////////
 
-  // This does regex match against the tag.
-  //
+  /**
+   * @brief Test whether a tag string matches the active regex for the given
+   *   tag type.
+   *
+   * @param[in] tag Non-null C string to match.
+   * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action.
+   * @return True if tag matches the compiled regex for mode.
+   * @pre tag is non-null; a Diags instance is active.
+   * @post No state change.

Review Comment:
   tag_activated() is documented as requiring a non-null tag, but the 
implementation returns true when tag == nullptr. The docs should describe the 
nullptr behavior (and drop the non-null precondition).



##########
include/tscore/DiagsTypes.h:
##########
@@ -45,111 +45,294 @@
 #include "tscore/ink_mutex.h"
 #include "tsutil/Regex.h"
 
+/// Sentinel value stored in Diags::magic to detect heap corruption or
+/// use-after-free of a Diags instance. Diags::magic == DIAGS_MAGIC for
+/// any properly constructed, living instance.
 #define DIAGS_MAGIC 0x12345678
-#define BYTES_IN_MB 1000000
 
-enum DiagsTagType {
-  DiagsTagType_Debug  = 0, // do not renumber --- used as array index
-  DiagsTagType_Action = 1
-};
+/// Bytes-per-megabyte (decimal, not binary). Used by configuration
+/// parsing to convert MB-valued knobs into byte counts.
+#define BYTES_IN_MB 1000000
 
+/**
+ * @brief Selects which of the two tag tables a Diags operation addresses:
+ *   debug tags (controlling Dbg/Diag emission) or action tags (controlling
+ *   is_action_tag_set conditional code paths).
+ *
+ * @note Numeric values are used as array indices into
+ *   DiagsConfigState::_enabled and Diags::activated_tags. Do not renumber.
+ */
+enum DiagsTagType { DiagsTagType_Debug = 0, DiagsTagType_Action = 1 };
+
+/**
+ * @brief Per-DiagsLevel output destination configuration.
+ *
+ * Each boolean controls whether messages at the owning level are emitted
+ * to the corresponding sink. Multiple sinks may be set simultaneously;
+ * the message is emitted to all enabled sinks in unspecified order.
+ *
+ * @thread-safety Carries no internal synchronization. Callers reading or
+ *   modifying Diags::config.outputs[] must respect the concurrency contract
+ *   of the containing DiagsConfigState.
+ */
 struct DiagsModeOutput {
   bool to_stdout;
   bool to_stderr;
   bool to_syslog;
   bool to_diagslog;
 };
 
+/**
+ * @brief Identifies which standard stream Diags::set_std_output reseats.
+ *
+ * STDOUT (=0) targets the standard output stream; STDERR targets the
+ * standard error stream. Values are used as array indices; do not renumber.
+ */
 enum StdStream { STDOUT = 0, STDERR };
 
+/**
+ * @brief Selects the rolling policy for a managed log file.
+ *
+ * - NO_ROLLING             — never roll; file grows without bound.
+ * - ROLL_ON_TIME           — roll when the configured time interval elapses.
+ * - ROLL_ON_SIZE           — roll when the configured size threshold is 
exceeded.
+ * - ROLL_ON_TIME_OR_SIZE   — roll when either condition is satisfied.
+ * - INVALID_ROLLING_VALUE  — sentinel; any code path receiving this value MUST
+ *   treat it as NO_ROLLING. MUST be the largest enumerator so that range 
checks
+ *   can detect out-of-range configuration values.
+ */
 enum RollingEnabledValues { NO_ROLLING = 0, ROLL_ON_TIME, ROLL_ON_SIZE, 
ROLL_ON_TIME_OR_SIZE, INVALID_ROLLING_VALUE };
 
+/// Count of valid DiagsLevel values; equal to DL_Undefined and used to
+/// size DiagsConfigState::outputs[].
 #define DiagsLevel_Count DL_Undefined
 
+/// True if the given DiagsLevel causes process termination after emission.
+/// Terminal levels are DL_Fatal and above (excluding DL_Undefined).
 #define DiagsLevel_IsTerminal(_l) (((_l) >= DL_Fatal) && ((_l) < DL_Undefined))
 
-// Cleanup Function Prototype - Called before ink_fatal to
-//   cleanup process state
+/**
+ * @brief Cleanup callback invoked before process termination on a terminal
+ *   DiagsLevel (Fatal, Alert, Emergency).
+ *
+ * @pre The callback runs on the thread that emitted the terminal message.
+ *   It MUST NOT throw. It MUST be async-signal-safe if terminal-level
+ *   emission can occur from a signal handler.
+ * @post On return the process continues into fatal-exit logic.
+ * @thread-safety Only one callback is installed per Diags instance.
+ *   The callback is invoked at most once per process.
+ */
 using DiagsCleanupFunc = void (*)();
 
+/**
+ * @brief Process-global enable state for the two tag tables.
+ *
+ * Exposes two static methods to read and write the per-tag enable state.
+ * The state for each DiagsTagType is one of:
+ *   0 — disabled (no emission),
+ *   1 — enabled for all callers,
+ *   2 — enabled only when ContFlags::DEBUG_OVERRIDE is set on the
+ *       current Continuation (per-connection debug override).
+ *
+ */
 class DiagsConfigState
 {
 public:
+  /**
+   * @brief Return the current enable state for the given tag type.
+   *
+   * @param[in] dtt DiagsTagType_Debug or DiagsTagType_Action.
+   * @return 0 (disabled), 1 (enabled), or 2 (DEBUG_OVERRIDE mode).
+   * @pre None.
+   * @post No state change.
+   * @errors None.
+   * @thread-safety Intended semantics: relaxed atomic load, safe to call
+   *   concurrently.

Review Comment:
   The doc claims enabled() is a "relaxed atomic load" and safe to call 
concurrently, but _enabled is a plain static int array (non-atomic). This 
documentation should not promise atomic/thread-safe semantics that the 
implementation does not provide.



##########
include/tscore/DiagsTypes.h:
##########
@@ -45,111 +45,294 @@
 #include "tscore/ink_mutex.h"
 #include "tsutil/Regex.h"
 
+/// Sentinel value stored in Diags::magic to detect heap corruption or
+/// use-after-free of a Diags instance. Diags::magic == DIAGS_MAGIC for
+/// any properly constructed, living instance.
 #define DIAGS_MAGIC 0x12345678
-#define BYTES_IN_MB 1000000
 
-enum DiagsTagType {
-  DiagsTagType_Debug  = 0, // do not renumber --- used as array index
-  DiagsTagType_Action = 1
-};
+/// Bytes-per-megabyte (decimal, not binary). Used by configuration
+/// parsing to convert MB-valued knobs into byte counts.
+#define BYTES_IN_MB 1000000
 
+/**
+ * @brief Selects which of the two tag tables a Diags operation addresses:
+ *   debug tags (controlling Dbg/Diag emission) or action tags (controlling
+ *   is_action_tag_set conditional code paths).
+ *
+ * @note Numeric values are used as array indices into
+ *   DiagsConfigState::_enabled and Diags::activated_tags. Do not renumber.
+ */
+enum DiagsTagType { DiagsTagType_Debug = 0, DiagsTagType_Action = 1 };
+
+/**
+ * @brief Per-DiagsLevel output destination configuration.
+ *
+ * Each boolean controls whether messages at the owning level are emitted
+ * to the corresponding sink. Multiple sinks may be set simultaneously;
+ * the message is emitted to all enabled sinks in unspecified order.
+ *
+ * @thread-safety Carries no internal synchronization. Callers reading or
+ *   modifying Diags::config.outputs[] must respect the concurrency contract
+ *   of the containing DiagsConfigState.
+ */
 struct DiagsModeOutput {
   bool to_stdout;
   bool to_stderr;
   bool to_syslog;
   bool to_diagslog;
 };
 
+/**
+ * @brief Identifies which standard stream Diags::set_std_output reseats.
+ *
+ * STDOUT (=0) targets the standard output stream; STDERR targets the
+ * standard error stream. Values are used as array indices; do not renumber.
+ */
 enum StdStream { STDOUT = 0, STDERR };
 
+/**
+ * @brief Selects the rolling policy for a managed log file.
+ *
+ * - NO_ROLLING             — never roll; file grows without bound.
+ * - ROLL_ON_TIME           — roll when the configured time interval elapses.
+ * - ROLL_ON_SIZE           — roll when the configured size threshold is 
exceeded.
+ * - ROLL_ON_TIME_OR_SIZE   — roll when either condition is satisfied.
+ * - INVALID_ROLLING_VALUE  — sentinel; any code path receiving this value MUST
+ *   treat it as NO_ROLLING. MUST be the largest enumerator so that range 
checks
+ *   can detect out-of-range configuration values.
+ */
 enum RollingEnabledValues { NO_ROLLING = 0, ROLL_ON_TIME, ROLL_ON_SIZE, 
ROLL_ON_TIME_OR_SIZE, INVALID_ROLLING_VALUE };
 
+/// Count of valid DiagsLevel values; equal to DL_Undefined and used to
+/// size DiagsConfigState::outputs[].
 #define DiagsLevel_Count DL_Undefined
 
+/// True if the given DiagsLevel causes process termination after emission.
+/// Terminal levels are DL_Fatal and above (excluding DL_Undefined).
 #define DiagsLevel_IsTerminal(_l) (((_l) >= DL_Fatal) && ((_l) < DL_Undefined))
 
-// Cleanup Function Prototype - Called before ink_fatal to
-//   cleanup process state
+/**
+ * @brief Cleanup callback invoked before process termination on a terminal
+ *   DiagsLevel (Fatal, Alert, Emergency).
+ *
+ * @pre The callback runs on the thread that emitted the terminal message.
+ *   It MUST NOT throw. It MUST be async-signal-safe if terminal-level
+ *   emission can occur from a signal handler.
+ * @post On return the process continues into fatal-exit logic.
+ * @thread-safety Only one callback is installed per Diags instance.
+ *   The callback is invoked at most once per process.
+ */
 using DiagsCleanupFunc = void (*)();
 
+/**
+ * @brief Process-global enable state for the two tag tables.
+ *
+ * Exposes two static methods to read and write the per-tag enable state.
+ * The state for each DiagsTagType is one of:
+ *   0 — disabled (no emission),
+ *   1 — enabled for all callers,
+ *   2 — enabled only when ContFlags::DEBUG_OVERRIDE is set on the
+ *       current Continuation (per-connection debug override).
+ *
+ */
 class DiagsConfigState
 {
 public:
+  /**
+   * @brief Return the current enable state for the given tag type.
+   *
+   * @param[in] dtt DiagsTagType_Debug or DiagsTagType_Action.
+   * @return 0 (disabled), 1 (enabled), or 2 (DEBUG_OVERRIDE mode).
+   * @pre None.
+   * @post No state change.
+   * @errors None.
+   * @thread-safety Intended semantics: relaxed atomic load, safe to call
+   *   concurrently.
+   */
   static int
   enabled(DiagsTagType dtt)
   {
     return _enabled[dtt];
   }
 
+  /**
+   * @brief Set the enable state for the given tag type.
+   *
+   * @param[in] dtt DiagsTagType_Debug or DiagsTagType_Action.
+   * @param[in] new_value 0, 1, or 2 (see class contract).
+   * @pre new_value is in [0, 2].
+   * @post enabled(dtt) returns new_value on all subsequent calls.
+   * @errors None.
+   * @thread-safety Must be called with external serialization against
+   *   concurrent enabled(dtt) reads.
+   */
   static void enabled(DiagsTagType dtt, int new_value);
 
-  DiagsModeOutput outputs[DiagsLevel_Count]; // where each level prints
+  /// Per-level output routing. Each element controls where messages at
+  /// the corresponding DiagsLevel are sent. See DiagsModeOutput.
+  DiagsModeOutput outputs[DiagsLevel_Count];
 
 private:
   static int _enabled[2]; // one debug, one action
 };
 
-//////////////////////////////////////////////////////////////////////////////
-//
-//      class Diags
-//
-//      The Diags class is used for global configuration of the run-time
-//      diagnostics system.  This class provides the following services:
-//
-//      * run-time notices, debugging, warnings, errors
-//      * debugging tags to selectively enable & disable diagnostics
-//      * action tags to selectively enable & disable code paths
-//      * configurable output to stdout, stderr, syslog, error logs
-//      * interface to supporting on-the-fly reconfiguration
-//
-//////////////////////////////////////////////////////////////////////////////
-
+/**
+ * @brief Active diagnostic emission and tag-table state for the process.
+ *
+ * Owns the diags.log, stdout, and stderr BaseLogFile handles, the per-level
+ * output routing configuration, the regex tables for debug and action tags,
+ * and the rolling policy for the diagnostic and standard-output logs.
+ * All diagnostic emission macros (Status, Note, Warning, Error, Fatal, Alert,
+ * Emergency, Diag) route through the process-global Diags instance installed
+ * via DiagsPtr.
+ *
+ * @thread-safety After construction, the instance is safe to use concurrently
+ *   from any number of threads for emission (print, log, error). 
Reconfiguration
+ *   methods (activate_taglist, deactivate_all, setup_diagslog, config_roll_*,
+ *   reseat_diagslog, should_roll_*, set_std_output) acquire an internal mutex
+ *   and are mutually exclusive with each other and with tag-table reads. See
+ *   per-method @thread-safety blocks for details.
+ */
 class Diags : public DebugInterface
 {
 public:
+  /**
+   * @brief Construct a Diags instance with the given initial configuration.
+   *
+   * @param[in] prefix_string Tag prefix prepended to all debug output.
+   * @param[in] base_debug_tags Initial debug tag regex, or nullptr for none.
+   * @param[in] base_action_tags Initial action tag regex, or nullptr for none.
+   * @param[in] _diags_log BaseLogFile for diags.log output, or nullptr to
+   *   suppress file logging. Ownership transfers to this Diags instance.
+   * @param[in] diags_log_perm File permission bits for diags.log, or -1 for
+   *   the system default (LOGFILE_DEFAULT_PERMS).
+   * @param[in] output_log_perm File permission bits for stdout/stderr log
+   *   files, or -1 for the system default.
+   * @pre None; safe to construct before any thread reads via diags().
+   * @post magic == DIAGS_MAGIC; tag tables are initialized from the given
+   *   regexes; diags_log is open if _diags_log is non-null and openable.
+   * @errors Allocation failures abort via ink_fatal. Log-open failures are
+   *   reported via log_log_error and result in a null m_fp for the log.
+   * @thread-safety Single-threaded construction expected. After construction
+   *   the instance may be installed via DiagsPtr::set and used concurrently.
+   */
   Diags(std::string_view prefix_string, const char *base_debug_tags, const 
char *base_action_tags, BaseLogFile *_diags_log,
         int diags_log_perm = -1, int output_log_perm = -1);
+
+  /**
+   * @brief Destroy the Diags instance, closing all owned log files.
+   *
+   * @pre No thread is currently executing a method on this instance.
+   * @post All owned BaseLogFile handles are deleted (closing their FILE *).
+   * @errors None.
+   * @thread-safety Single-threaded destruction. The caller must ensure no
+   *   other thread holds or will acquire a reference to this instance.
+   */
   virtual ~Diags();
 
+  /// Diagnostic log file. Owned by this instance. Read-only from outside
+  /// the tscore module; mutation via any path other than setup_diagslog /
+  /// reseat_diagslog is undefined behavior.
   BaseLogFile *diags_log;
+
+  /// Standard-output redirection file. Owned by this instance.
+  /// Read-only from outside the tscore module.
   BaseLogFile *stdout_log;
+
+  /// Standard-error redirection file. Owned by this instance.
+  /// Read-only from outside the tscore module.
   BaseLogFile *stderr_log;
 
+  /// Sentinel. Always DIAGS_MAGIC for a live, properly constructed instance.
   const unsigned int magic;
-  DiagsConfigState   config;
-  DiagsShowLocation  show_location;
-  DiagsCleanupFunc   cleanup_func;
+
+  /// Per-level output routing and tag-enable state. See DiagsConfigState.
+  DiagsConfigState config;
+
+  /// Controls whether source location is appended to emitted messages.
+  DiagsShowLocation show_location;
+
+  /// Optional cleanup callback invoked before terminal-level process exit.
+  /// May be nullptr (no cleanup). See DiagsCleanupFunc.
+  DiagsCleanupFunc cleanup_func;
 
   ///////////////////////////
   // conditional debugging //
   ///////////////////////////
 
+  /**
+   * @brief Return the per-connection DEBUG_OVERRIDE flag for the current
+   *   Continuation.
+   *
+   * @return True if ContFlags::DEBUG_OVERRIDE is set on the currently
+   *   executing Continuation; false otherwise or if no Continuation is active.
+   * @pre None.
+   * @post No state change.
+   * @errors None.
+   * @thread-safety Safe to call from any thread; reads a thread-local
+   *   Continuation flag.
+   */
   bool
   get_override() const override
   {
     return get_cont_flag(ContFlags::DEBUG_OVERRIDE);
   }
 
+  /**
+   * @brief Test whether the given IP endpoint matches the configured debug
+   *   client IP.
+   *
+   * @param[in] test_ip Endpoint to compare against debug_client_ip.
+   * @return True if test_ip equals debug_client_ip.
+   * @pre None.
+   * @post No state change.
+   * @errors None.
+   * @thread-safety Safe to call concurrently; debug_client_ip is written
+   *   only during single-threaded reconfiguration.
+   */
   bool
   test_override_ip(IpEndpoint const &test_ip)
   {
     return this->debug_client_ip == test_ip;
   }
 
-  // It seems to make a big difference to performance (due to the caching of 
the enabled flag) to call this
-  // function first before doing anything else for debug output.  This 
includes entering blocks with static
-  // DbgCtl instances, or other static variables with non-const 
initialization.  Static variables with
-  // non-const initialization inside a function have a hidden flag that is 
checked every time the containing
-  // block is entered, to see if the variable has been initialized or not.
-  //
+  /**
+   * @brief Test whether emission for the given tag mode is globally enabled.
+   *
+   * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action; defaults to
+   *   DiagsTagType_Debug.
+   * @return True if the tag type is enabled (value 1), OR if it is in
+   *   DEBUG_OVERRIDE mode (value 2) and the current Continuation has the
+   *   override flag set.
+   * @pre None.
+   * @post No state change.
+   * @errors None.
+   * @thread-safety Intended semantics: relaxed atomic load, safe to call
+   *   concurrently. See DiagsConfigState contract.
+   * @note This method is on the hot path of every Dbg/Diag macro. Call it
+   *   before any block-local static with non-const initialization to avoid
+   *   the hidden initialization-check overhead when diagnostics are off.
+   */
   bool
   on(DiagsTagType mode = DiagsTagType_Debug) const
   {
     return (config.enabled(mode) & 1) || (config.enabled(mode) == 2 && 
this->get_override());
   }
 
-  // Returns true if tag is enabled for mode.
-  //
+  /**
+   * @brief Test whether the given tag is active for the given mode.
+   *
+   * @param[in] tag Non-null C string naming the tag to check.
+   * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action.
+   * @return True if the tag mode is globally enabled AND the tag matches
+   *   the configured tag regex.
+   * @pre tag is non-null.
+   * @post No state change.

Review Comment:
   The `@pre` for on(tag, ...) requires tag be non-null, but the implementation 
allows nullptr and treats it as always-active. The precondition should match 
actual behavior.



##########
include/tscore/DiagsTypes.h:
##########
@@ -45,111 +45,294 @@
 #include "tscore/ink_mutex.h"
 #include "tsutil/Regex.h"
 
+/// Sentinel value stored in Diags::magic to detect heap corruption or
+/// use-after-free of a Diags instance. Diags::magic == DIAGS_MAGIC for
+/// any properly constructed, living instance.
 #define DIAGS_MAGIC 0x12345678
-#define BYTES_IN_MB 1000000
 
-enum DiagsTagType {
-  DiagsTagType_Debug  = 0, // do not renumber --- used as array index
-  DiagsTagType_Action = 1
-};
+/// Bytes-per-megabyte (decimal, not binary). Used by configuration
+/// parsing to convert MB-valued knobs into byte counts.
+#define BYTES_IN_MB 1000000
 
+/**
+ * @brief Selects which of the two tag tables a Diags operation addresses:
+ *   debug tags (controlling Dbg/Diag emission) or action tags (controlling
+ *   is_action_tag_set conditional code paths).
+ *
+ * @note Numeric values are used as array indices into
+ *   DiagsConfigState::_enabled and Diags::activated_tags. Do not renumber.
+ */
+enum DiagsTagType { DiagsTagType_Debug = 0, DiagsTagType_Action = 1 };
+
+/**
+ * @brief Per-DiagsLevel output destination configuration.
+ *
+ * Each boolean controls whether messages at the owning level are emitted
+ * to the corresponding sink. Multiple sinks may be set simultaneously;
+ * the message is emitted to all enabled sinks in unspecified order.
+ *
+ * @thread-safety Carries no internal synchronization. Callers reading or
+ *   modifying Diags::config.outputs[] must respect the concurrency contract
+ *   of the containing DiagsConfigState.
+ */
 struct DiagsModeOutput {
   bool to_stdout;
   bool to_stderr;
   bool to_syslog;
   bool to_diagslog;
 };
 
+/**
+ * @brief Identifies which standard stream Diags::set_std_output reseats.
+ *
+ * STDOUT (=0) targets the standard output stream; STDERR targets the
+ * standard error stream. Values are used as array indices; do not renumber.
+ */
 enum StdStream { STDOUT = 0, STDERR };
 
+/**
+ * @brief Selects the rolling policy for a managed log file.
+ *
+ * - NO_ROLLING             — never roll; file grows without bound.
+ * - ROLL_ON_TIME           — roll when the configured time interval elapses.
+ * - ROLL_ON_SIZE           — roll when the configured size threshold is 
exceeded.
+ * - ROLL_ON_TIME_OR_SIZE   — roll when either condition is satisfied.
+ * - INVALID_ROLLING_VALUE  — sentinel; any code path receiving this value MUST
+ *   treat it as NO_ROLLING. MUST be the largest enumerator so that range 
checks
+ *   can detect out-of-range configuration values.
+ */
 enum RollingEnabledValues { NO_ROLLING = 0, ROLL_ON_TIME, ROLL_ON_SIZE, 
ROLL_ON_TIME_OR_SIZE, INVALID_ROLLING_VALUE };
 
+/// Count of valid DiagsLevel values; equal to DL_Undefined and used to
+/// size DiagsConfigState::outputs[].
 #define DiagsLevel_Count DL_Undefined
 
+/// True if the given DiagsLevel causes process termination after emission.
+/// Terminal levels are DL_Fatal and above (excluding DL_Undefined).
 #define DiagsLevel_IsTerminal(_l) (((_l) >= DL_Fatal) && ((_l) < DL_Undefined))
 
-// Cleanup Function Prototype - Called before ink_fatal to
-//   cleanup process state
+/**
+ * @brief Cleanup callback invoked before process termination on a terminal
+ *   DiagsLevel (Fatal, Alert, Emergency).
+ *
+ * @pre The callback runs on the thread that emitted the terminal message.
+ *   It MUST NOT throw. It MUST be async-signal-safe if terminal-level
+ *   emission can occur from a signal handler.
+ * @post On return the process continues into fatal-exit logic.
+ * @thread-safety Only one callback is installed per Diags instance.
+ *   The callback is invoked at most once per process.
+ */
 using DiagsCleanupFunc = void (*)();
 
+/**
+ * @brief Process-global enable state for the two tag tables.
+ *
+ * Exposes two static methods to read and write the per-tag enable state.
+ * The state for each DiagsTagType is one of:
+ *   0 — disabled (no emission),
+ *   1 — enabled for all callers,
+ *   2 — enabled only when ContFlags::DEBUG_OVERRIDE is set on the
+ *       current Continuation (per-connection debug override).
+ *
+ */
 class DiagsConfigState
 {
 public:
+  /**
+   * @brief Return the current enable state for the given tag type.
+   *
+   * @param[in] dtt DiagsTagType_Debug or DiagsTagType_Action.
+   * @return 0 (disabled), 1 (enabled), or 2 (DEBUG_OVERRIDE mode).
+   * @pre None.
+   * @post No state change.
+   * @errors None.
+   * @thread-safety Intended semantics: relaxed atomic load, safe to call
+   *   concurrently.
+   */
   static int
   enabled(DiagsTagType dtt)
   {
     return _enabled[dtt];
   }
 
+  /**
+   * @brief Set the enable state for the given tag type.
+   *
+   * @param[in] dtt DiagsTagType_Debug or DiagsTagType_Action.
+   * @param[in] new_value 0, 1, or 2 (see class contract).
+   * @pre new_value is in [0, 2].
+   * @post enabled(dtt) returns new_value on all subsequent calls.
+   * @errors None.
+   * @thread-safety Must be called with external serialization against
+   *   concurrent enabled(dtt) reads.
+   */
   static void enabled(DiagsTagType dtt, int new_value);
 
-  DiagsModeOutput outputs[DiagsLevel_Count]; // where each level prints
+  /// Per-level output routing. Each element controls where messages at
+  /// the corresponding DiagsLevel are sent. See DiagsModeOutput.
+  DiagsModeOutput outputs[DiagsLevel_Count];
 
 private:
   static int _enabled[2]; // one debug, one action
 };
 
-//////////////////////////////////////////////////////////////////////////////
-//
-//      class Diags
-//
-//      The Diags class is used for global configuration of the run-time
-//      diagnostics system.  This class provides the following services:
-//
-//      * run-time notices, debugging, warnings, errors
-//      * debugging tags to selectively enable & disable diagnostics
-//      * action tags to selectively enable & disable code paths
-//      * configurable output to stdout, stderr, syslog, error logs
-//      * interface to supporting on-the-fly reconfiguration
-//
-//////////////////////////////////////////////////////////////////////////////
-
+/**
+ * @brief Active diagnostic emission and tag-table state for the process.
+ *
+ * Owns the diags.log, stdout, and stderr BaseLogFile handles, the per-level
+ * output routing configuration, the regex tables for debug and action tags,
+ * and the rolling policy for the diagnostic and standard-output logs.
+ * All diagnostic emission macros (Status, Note, Warning, Error, Fatal, Alert,
+ * Emergency, Diag) route through the process-global Diags instance installed
+ * via DiagsPtr.
+ *
+ * @thread-safety After construction, the instance is safe to use concurrently
+ *   from any number of threads for emission (print, log, error). 
Reconfiguration
+ *   methods (activate_taglist, deactivate_all, setup_diagslog, config_roll_*,
+ *   reseat_diagslog, should_roll_*, set_std_output) acquire an internal mutex
+ *   and are mutually exclusive with each other and with tag-table reads. See
+ *   per-method @thread-safety blocks for details.
+ */
 class Diags : public DebugInterface
 {
 public:
+  /**
+   * @brief Construct a Diags instance with the given initial configuration.
+   *
+   * @param[in] prefix_string Tag prefix prepended to all debug output.
+   * @param[in] base_debug_tags Initial debug tag regex, or nullptr for none.
+   * @param[in] base_action_tags Initial action tag regex, or nullptr for none.
+   * @param[in] _diags_log BaseLogFile for diags.log output, or nullptr to
+   *   suppress file logging. Ownership transfers to this Diags instance.
+   * @param[in] diags_log_perm File permission bits for diags.log, or -1 for
+   *   the system default (LOGFILE_DEFAULT_PERMS).
+   * @param[in] output_log_perm File permission bits for stdout/stderr log
+   *   files, or -1 for the system default.
+   * @pre None; safe to construct before any thread reads via diags().
+   * @post magic == DIAGS_MAGIC; tag tables are initialized from the given
+   *   regexes; diags_log is open if _diags_log is non-null and openable.
+   * @errors Allocation failures abort via ink_fatal. Log-open failures are
+   *   reported via log_log_error and result in a null m_fp for the log.
+   * @thread-safety Single-threaded construction expected. After construction
+   *   the instance may be installed via DiagsPtr::set and used concurrently.
+   */
   Diags(std::string_view prefix_string, const char *base_debug_tags, const 
char *base_action_tags, BaseLogFile *_diags_log,
         int diags_log_perm = -1, int output_log_perm = -1);
+
+  /**
+   * @brief Destroy the Diags instance, closing all owned log files.
+   *
+   * @pre No thread is currently executing a method on this instance.
+   * @post All owned BaseLogFile handles are deleted (closing their FILE *).
+   * @errors None.
+   * @thread-safety Single-threaded destruction. The caller must ensure no
+   *   other thread holds or will acquire a reference to this instance.
+   */
   virtual ~Diags();
 
+  /// Diagnostic log file. Owned by this instance. Read-only from outside
+  /// the tscore module; mutation via any path other than setup_diagslog /
+  /// reseat_diagslog is undefined behavior.
   BaseLogFile *diags_log;
+
+  /// Standard-output redirection file. Owned by this instance.
+  /// Read-only from outside the tscore module.
   BaseLogFile *stdout_log;
+
+  /// Standard-error redirection file. Owned by this instance.
+  /// Read-only from outside the tscore module.
   BaseLogFile *stderr_log;
 
+  /// Sentinel. Always DIAGS_MAGIC for a live, properly constructed instance.
   const unsigned int magic;
-  DiagsConfigState   config;
-  DiagsShowLocation  show_location;
-  DiagsCleanupFunc   cleanup_func;
+
+  /// Per-level output routing and tag-enable state. See DiagsConfigState.
+  DiagsConfigState config;
+
+  /// Controls whether source location is appended to emitted messages.
+  DiagsShowLocation show_location;
+
+  /// Optional cleanup callback invoked before terminal-level process exit.
+  /// May be nullptr (no cleanup). See DiagsCleanupFunc.
+  DiagsCleanupFunc cleanup_func;
 
   ///////////////////////////
   // conditional debugging //
   ///////////////////////////
 
+  /**
+   * @brief Return the per-connection DEBUG_OVERRIDE flag for the current
+   *   Continuation.
+   *
+   * @return True if ContFlags::DEBUG_OVERRIDE is set on the currently
+   *   executing Continuation; false otherwise or if no Continuation is active.
+   * @pre None.
+   * @post No state change.
+   * @errors None.
+   * @thread-safety Safe to call from any thread; reads a thread-local
+   *   Continuation flag.
+   */
   bool
   get_override() const override
   {
     return get_cont_flag(ContFlags::DEBUG_OVERRIDE);
   }
 
+  /**
+   * @brief Test whether the given IP endpoint matches the configured debug
+   *   client IP.
+   *
+   * @param[in] test_ip Endpoint to compare against debug_client_ip.
+   * @return True if test_ip equals debug_client_ip.
+   * @pre None.
+   * @post No state change.
+   * @errors None.
+   * @thread-safety Safe to call concurrently; debug_client_ip is written
+   *   only during single-threaded reconfiguration.
+   */
   bool
   test_override_ip(IpEndpoint const &test_ip)
   {
     return this->debug_client_ip == test_ip;
   }
 
-  // It seems to make a big difference to performance (due to the caching of 
the enabled flag) to call this
-  // function first before doing anything else for debug output.  This 
includes entering blocks with static
-  // DbgCtl instances, or other static variables with non-const 
initialization.  Static variables with
-  // non-const initialization inside a function have a hidden flag that is 
checked every time the containing
-  // block is entered, to see if the variable has been initialized or not.
-  //
+  /**
+   * @brief Test whether emission for the given tag mode is globally enabled.
+   *
+   * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action; defaults to
+   *   DiagsTagType_Debug.
+   * @return True if the tag type is enabled (value 1), OR if it is in
+   *   DEBUG_OVERRIDE mode (value 2) and the current Continuation has the
+   *   override flag set.
+   * @pre None.
+   * @post No state change.
+   * @errors None.
+   * @thread-safety Intended semantics: relaxed atomic load, safe to call
+   *   concurrently. See DiagsConfigState contract.

Review Comment:
   The on() fast-path docs also claim a relaxed atomic load / concurrent 
safety, but it calls DiagsConfigState::enabled() which reads a non-atomic int. 
The comment should reflect that reads are only safe when not racing a 
concurrent write.



##########
include/tscore/DiagsTypes.h:
##########
@@ -220,26 +486,201 @@ class Diags : public DebugInterface
     va_end(ap);
   }
 
+  /**
+   * @brief va_list form of error().
+   *
+   * @param[in] level DiagsLevel for routing and terminal handling.
+   * @param[in] loc Source location, or nullptr.
+   * @param[in] fmt Non-null printf-format string.
+   * @param[in] ap Initialized va_list. Consumed; caller MUST NOT reuse
+   *   without va_end + va_start.
+   * @pre fmt is non-null; ap is initialized.
+   * @post Same as error(). For terminal levels, invokes cleanup_func (if
+   *   set) then exits the process; does not return.
+   * @errors None signaled; I/O errors absorbed.
+   * @thread-safety Safe to call concurrently.
+   */
   virtual void error_va(DiagsLevel level, const SourceLocation *loc, const 
char *fmt, va_list ap) const;
 
+  /**
+   * @brief Print the current Diags configuration to fp.
+   *
+   * @param[in] fp Destination FILE *; defaults to stdout.
+   * @pre fp is a valid open stream.
+   * @post Configuration summary written to fp.
+   * @errors I/O errors on fp are not signaled.
+   * @thread-safety Safe to call concurrently; acquires tag_table_lock
+   *   internally for the tag-table portion of output.
+   */
   void dump(FILE *fp = stdout) const;
 
+  /**
+   * @brief Enable tags matching the given regex pattern for the given mode.
+   *
+   * @param[in] taglist Non-null regex string. Replaces the previous pattern.
+   * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action.
+   * @pre taglist is non-null and contains a valid POSIX extended regex.
+   * @post Tags matching taglist are active for mode; previous pattern is
+   *   discarded.
+   * @errors Invalid regex is silently ignored; previous pattern is retained.

Review Comment:
   activate_taglist() says an invalid regex is ignored and the previous pattern 
is retained, but the implementation always installs the newly constructed Regex 
even if Regex::compile() fails (which clears the compiled code and results in a 
regex that never matches). The `@errors` contract should match actual behavior.



##########
include/tscore/DiagsTypes.h:
##########
@@ -203,6 +443,20 @@ class Diags : public DebugInterface
     }
   }
 
+  /**
+   * @brief va_list form of log().
+   *
+   * @param[in] tag Non-null tag name.

Review Comment:
   log_va() documentation requires tag be non-null, but nullptr is allowed and 
is treated as "no tag filtering" (global enable check only).



##########
include/tscore/DiagsTypes.h:
##########
@@ -220,26 +486,201 @@ class Diags : public DebugInterface
     va_end(ap);
   }
 
+  /**
+   * @brief va_list form of error().
+   *
+   * @param[in] level DiagsLevel for routing and terminal handling.
+   * @param[in] loc Source location, or nullptr.
+   * @param[in] fmt Non-null printf-format string.
+   * @param[in] ap Initialized va_list. Consumed; caller MUST NOT reuse
+   *   without va_end + va_start.
+   * @pre fmt is non-null; ap is initialized.
+   * @post Same as error(). For terminal levels, invokes cleanup_func (if
+   *   set) then exits the process; does not return.
+   * @errors None signaled; I/O errors absorbed.
+   * @thread-safety Safe to call concurrently.
+   */
   virtual void error_va(DiagsLevel level, const SourceLocation *loc, const 
char *fmt, va_list ap) const;
 
+  /**
+   * @brief Print the current Diags configuration to fp.
+   *
+   * @param[in] fp Destination FILE *; defaults to stdout.
+   * @pre fp is a valid open stream.
+   * @post Configuration summary written to fp.
+   * @errors I/O errors on fp are not signaled.
+   * @thread-safety Safe to call concurrently; acquires tag_table_lock
+   *   internally for the tag-table portion of output.

Review Comment:
   dump() documentation says it acquires tag_table_lock for tag-table output, 
but dump() doesn't read the tag tables (activated_tags) and does not take the 
lock in the implementation. Adjust the thread-safety note to match reality.



##########
include/tscore/DiagsTypes.h:
##########
@@ -220,26 +486,201 @@ class Diags : public DebugInterface
     va_end(ap);
   }
 
+  /**
+   * @brief va_list form of error().
+   *
+   * @param[in] level DiagsLevel for routing and terminal handling.
+   * @param[in] loc Source location, or nullptr.
+   * @param[in] fmt Non-null printf-format string.
+   * @param[in] ap Initialized va_list. Consumed; caller MUST NOT reuse
+   *   without va_end + va_start.
+   * @pre fmt is non-null; ap is initialized.
+   * @post Same as error(). For terminal levels, invokes cleanup_func (if
+   *   set) then exits the process; does not return.
+   * @errors None signaled; I/O errors absorbed.
+   * @thread-safety Safe to call concurrently.
+   */
   virtual void error_va(DiagsLevel level, const SourceLocation *loc, const 
char *fmt, va_list ap) const;
 
+  /**
+   * @brief Print the current Diags configuration to fp.
+   *
+   * @param[in] fp Destination FILE *; defaults to stdout.
+   * @pre fp is a valid open stream.
+   * @post Configuration summary written to fp.
+   * @errors I/O errors on fp are not signaled.
+   * @thread-safety Safe to call concurrently; acquires tag_table_lock
+   *   internally for the tag-table portion of output.
+   */
   void dump(FILE *fp = stdout) const;
 
+  /**
+   * @brief Enable tags matching the given regex pattern for the given mode.
+   *
+   * @param[in] taglist Non-null regex string. Replaces the previous pattern.
+   * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action.
+   * @pre taglist is non-null and contains a valid POSIX extended regex.
+   * @post Tags matching taglist are active for mode; previous pattern is
+   *   discarded.
+   * @errors Invalid regex is silently ignored; previous pattern is retained.
+   * @thread-safety Acquires tag_table_lock. Safe to call concurrently with
+   *   emission, but serialized with other reconfiguration methods.
+   */
   void activate_taglist(const char *taglist, DiagsTagType mode = 
DiagsTagType_Debug);
 
+  /**
+   * @brief Disable all tags for the given mode.
+   *
+   * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action.
+   * @pre None.
+   * @post No tag matches for mode; activated_tags[mode] is null.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock. Safe to call concurrently with
+   *   emission, but serialized with other reconfiguration methods.
+   */
   void deactivate_all(DiagsTagType mode = DiagsTagType_Debug);
 
+  /**
+   * @brief Install the given BaseLogFile as the active diagnostic log,
+   *   taking ownership unconditionally.
+   *
+   * @param[in] blf Non-null pointer to a constructed but not-yet-opened
+   *   BaseLogFile. Ownership always transfers to this function: on success
+   *   the Diags instance manages blf; on failure blf is deleted by this
+   *   function before returning.
+   * @return True if blf was opened and installed; false if open failed
+   *   (blf is deleted before returning false).
+   * @pre blf is non-null; blf->is_init() is false.
+   * @post On success: blf is open and diags_log points to it.
+   *   On failure: blf has been deleted; diags_log is unchanged.

Review Comment:
   setup_diagslog() is documented as "opened and installed" and claims 
diags_log points to the new BaseLogFile on success, but the implementation only 
opens the file and leaves installing/swapping diags_log to the caller (e.g., 
ctor/reseat/roll). Please correct the return/postcondition text.



##########
include/tscore/DiagsTypes.h:
##########
@@ -220,26 +486,201 @@ class Diags : public DebugInterface
     va_end(ap);
   }
 
+  /**
+   * @brief va_list form of error().
+   *
+   * @param[in] level DiagsLevel for routing and terminal handling.
+   * @param[in] loc Source location, or nullptr.
+   * @param[in] fmt Non-null printf-format string.
+   * @param[in] ap Initialized va_list. Consumed; caller MUST NOT reuse
+   *   without va_end + va_start.
+   * @pre fmt is non-null; ap is initialized.
+   * @post Same as error(). For terminal levels, invokes cleanup_func (if
+   *   set) then exits the process; does not return.
+   * @errors None signaled; I/O errors absorbed.
+   * @thread-safety Safe to call concurrently.
+   */
   virtual void error_va(DiagsLevel level, const SourceLocation *loc, const 
char *fmt, va_list ap) const;
 
+  /**
+   * @brief Print the current Diags configuration to fp.
+   *
+   * @param[in] fp Destination FILE *; defaults to stdout.
+   * @pre fp is a valid open stream.
+   * @post Configuration summary written to fp.
+   * @errors I/O errors on fp are not signaled.
+   * @thread-safety Safe to call concurrently; acquires tag_table_lock
+   *   internally for the tag-table portion of output.
+   */
   void dump(FILE *fp = stdout) const;
 
+  /**
+   * @brief Enable tags matching the given regex pattern for the given mode.
+   *
+   * @param[in] taglist Non-null regex string. Replaces the previous pattern.
+   * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action.
+   * @pre taglist is non-null and contains a valid POSIX extended regex.
+   * @post Tags matching taglist are active for mode; previous pattern is
+   *   discarded.
+   * @errors Invalid regex is silently ignored; previous pattern is retained.
+   * @thread-safety Acquires tag_table_lock. Safe to call concurrently with
+   *   emission, but serialized with other reconfiguration methods.
+   */
   void activate_taglist(const char *taglist, DiagsTagType mode = 
DiagsTagType_Debug);
 
+  /**
+   * @brief Disable all tags for the given mode.
+   *
+   * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action.
+   * @pre None.
+   * @post No tag matches for mode; activated_tags[mode] is null.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock. Safe to call concurrently with
+   *   emission, but serialized with other reconfiguration methods.
+   */
   void deactivate_all(DiagsTagType mode = DiagsTagType_Debug);
 
+  /**
+   * @brief Install the given BaseLogFile as the active diagnostic log,
+   *   taking ownership unconditionally.
+   *
+   * @param[in] blf Non-null pointer to a constructed but not-yet-opened
+   *   BaseLogFile. Ownership always transfers to this function: on success
+   *   the Diags instance manages blf; on failure blf is deleted by this
+   *   function before returning.
+   * @return True if blf was opened and installed; false if open failed
+   *   (blf is deleted before returning false).
+   * @pre blf is non-null; blf->is_init() is false.
+   * @post On success: blf is open and diags_log points to it.
+   *   On failure: blf has been deleted; diags_log is unchanged.
+   * @errors Open failures cause a false return and an internal log_log
+   *   diagnostic at LL_Error.
+   * @thread-safety Caller MUST serialize with all other reconfiguration
+   *   methods. The swap step inside reseat_diagslog() acquires
+   *   tag_table_lock; direct callers of setup_diagslog() must provide
+   *   equivalent serialization.
+   */
   bool setup_diagslog(BaseLogFile *blf);
+
+  /**
+   * @brief Configure the rolling policy for diags.log.
+   *
+   * @param[in] re Rolling policy (see RollingEnabledValues).
+   * @param[in] ri Rolling interval in seconds (used by time-based policies).
+   * @param[in] rs Rolling size threshold in bytes (used by size-based
+   *   policies).
+   * @pre None.
+   * @post Rolling policy for diags.log is updated; effective on the next
+   *   should_roll_diagslog() call.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock.
+   */
   void config_roll_diagslog(RollingEnabledValues re, int ri, int rs);
+
+  /**
+   * @brief Configure the rolling policy for the output log (traffic.out).
+   *
+   * @param[in] re Rolling policy.
+   * @param[in] ri Rolling interval in seconds.
+   * @param[in] rs Rolling size threshold in bytes.
+   * @pre None.
+   * @post Rolling policy for the output log is updated.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock.
+   */
   void config_roll_outputlog(RollingEnabledValues re, int ri, int rs);
+
+  /**
+   * @brief Close the current diags.log and reopen it at the configured path.
+   *
+   * Intended for use after an external log rotation tool has renamed or
+   * removed the active diags.log. The implementation:
+   *  1. Flushes the current file's buffered output.
+   *  2. Captures the configured filename from the active BaseLogFile.
+   *  3. Constructs a new BaseLogFile at that filename. The filename is
+   *     passed to the OS verbatim — symlinks are re-resolved at each call.
+   *  4. On success: atomically swaps the new file into place under
+   *     tag_table_lock and destroys the previous BaseLogFile (closing its
+   *     FILE *).
+   *  5. On failure: leaves the previous file in place.
+   *
+   * @return True if diags_log was successfully reseated; false if the
+   *   existing log was not initialized (the pre-init early-return path).
+   *   Note: the current implementation returns true even when
+   *   setup_diagslog() fails; callers MUST inspect the emitted Note() to
+   *   determine actual success.
+   * @pre A Diags instance is active and diags_log is initialized
+   *   (is_init() == true). If this precondition is not met, returns false
+   *   without performing a reopen — this is the safe no-op path for calls
+   *   that arrive before the diagnostics subsystem is initialized.
+   * @post On success: diags_log is a fresh BaseLogFile at the configured
+   *   path; the previous BaseLogFile (and its FILE *) is destroyed; all
+   *   subsequent emission goes to the new file.
+   *   On failure: diags_log is unchanged; the newly allocated BaseLogFile
+   *   is deleted by setup_diagslog() before it returns.
+   * @errors Open failures are not directly signaled via the return value;
+   *   failure is observable via the emitted Note().

Review Comment:
   reseat_diagslog() docs say failure is observable via an emitted Note(), but 
the current implementation does not emit a Note() on open failure; 
setup_diagslog() logs via log_log_error() and reseat_diagslog() still returns 
true. The `@errors` text should match the actual signaling mechanism.



##########
include/tscore/DiagsTypes.h:
##########
@@ -220,26 +486,201 @@ class Diags : public DebugInterface
     va_end(ap);
   }
 
+  /**
+   * @brief va_list form of error().
+   *
+   * @param[in] level DiagsLevel for routing and terminal handling.
+   * @param[in] loc Source location, or nullptr.
+   * @param[in] fmt Non-null printf-format string.
+   * @param[in] ap Initialized va_list. Consumed; caller MUST NOT reuse
+   *   without va_end + va_start.
+   * @pre fmt is non-null; ap is initialized.
+   * @post Same as error(). For terminal levels, invokes cleanup_func (if
+   *   set) then exits the process; does not return.
+   * @errors None signaled; I/O errors absorbed.
+   * @thread-safety Safe to call concurrently.
+   */
   virtual void error_va(DiagsLevel level, const SourceLocation *loc, const 
char *fmt, va_list ap) const;
 
+  /**
+   * @brief Print the current Diags configuration to fp.
+   *
+   * @param[in] fp Destination FILE *; defaults to stdout.
+   * @pre fp is a valid open stream.
+   * @post Configuration summary written to fp.
+   * @errors I/O errors on fp are not signaled.
+   * @thread-safety Safe to call concurrently; acquires tag_table_lock
+   *   internally for the tag-table portion of output.
+   */
   void dump(FILE *fp = stdout) const;
 
+  /**
+   * @brief Enable tags matching the given regex pattern for the given mode.
+   *
+   * @param[in] taglist Non-null regex string. Replaces the previous pattern.
+   * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action.
+   * @pre taglist is non-null and contains a valid POSIX extended regex.
+   * @post Tags matching taglist are active for mode; previous pattern is
+   *   discarded.
+   * @errors Invalid regex is silently ignored; previous pattern is retained.
+   * @thread-safety Acquires tag_table_lock. Safe to call concurrently with
+   *   emission, but serialized with other reconfiguration methods.
+   */
   void activate_taglist(const char *taglist, DiagsTagType mode = 
DiagsTagType_Debug);
 
+  /**
+   * @brief Disable all tags for the given mode.
+   *
+   * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action.
+   * @pre None.
+   * @post No tag matches for mode; activated_tags[mode] is null.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock. Safe to call concurrently with
+   *   emission, but serialized with other reconfiguration methods.
+   */
   void deactivate_all(DiagsTagType mode = DiagsTagType_Debug);
 
+  /**
+   * @brief Install the given BaseLogFile as the active diagnostic log,
+   *   taking ownership unconditionally.
+   *
+   * @param[in] blf Non-null pointer to a constructed but not-yet-opened
+   *   BaseLogFile. Ownership always transfers to this function: on success
+   *   the Diags instance manages blf; on failure blf is deleted by this
+   *   function before returning.
+   * @return True if blf was opened and installed; false if open failed
+   *   (blf is deleted before returning false).
+   * @pre blf is non-null; blf->is_init() is false.
+   * @post On success: blf is open and diags_log points to it.
+   *   On failure: blf has been deleted; diags_log is unchanged.
+   * @errors Open failures cause a false return and an internal log_log
+   *   diagnostic at LL_Error.
+   * @thread-safety Caller MUST serialize with all other reconfiguration
+   *   methods. The swap step inside reseat_diagslog() acquires
+   *   tag_table_lock; direct callers of setup_diagslog() must provide
+   *   equivalent serialization.
+   */
   bool setup_diagslog(BaseLogFile *blf);
+
+  /**
+   * @brief Configure the rolling policy for diags.log.
+   *
+   * @param[in] re Rolling policy (see RollingEnabledValues).
+   * @param[in] ri Rolling interval in seconds (used by time-based policies).
+   * @param[in] rs Rolling size threshold in bytes (used by size-based
+   *   policies).
+   * @pre None.
+   * @post Rolling policy for diags.log is updated; effective on the next
+   *   should_roll_diagslog() call.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock.
+   */
   void config_roll_diagslog(RollingEnabledValues re, int ri, int rs);
+
+  /**
+   * @brief Configure the rolling policy for the output log (traffic.out).
+   *
+   * @param[in] re Rolling policy.
+   * @param[in] ri Rolling interval in seconds.
+   * @param[in] rs Rolling size threshold in bytes.
+   * @pre None.
+   * @post Rolling policy for the output log is updated.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock.
+   */
   void config_roll_outputlog(RollingEnabledValues re, int ri, int rs);
+
+  /**
+   * @brief Close the current diags.log and reopen it at the configured path.
+   *
+   * Intended for use after an external log rotation tool has renamed or
+   * removed the active diags.log. The implementation:
+   *  1. Flushes the current file's buffered output.
+   *  2. Captures the configured filename from the active BaseLogFile.
+   *  3. Constructs a new BaseLogFile at that filename. The filename is
+   *     passed to the OS verbatim — symlinks are re-resolved at each call.
+   *  4. On success: atomically swaps the new file into place under
+   *     tag_table_lock and destroys the previous BaseLogFile (closing its
+   *     FILE *).
+   *  5. On failure: leaves the previous file in place.
+   *
+   * @return True if diags_log was successfully reseated; false if the
+   *   existing log was not initialized (the pre-init early-return path).
+   *   Note: the current implementation returns true even when
+   *   setup_diagslog() fails; callers MUST inspect the emitted Note() to
+   *   determine actual success.
+   * @pre A Diags instance is active and diags_log is initialized
+   *   (is_init() == true). If this precondition is not met, returns false
+   *   without performing a reopen — this is the safe no-op path for calls
+   *   that arrive before the diagnostics subsystem is initialized.
+   * @post On success: diags_log is a fresh BaseLogFile at the configured
+   *   path; the previous BaseLogFile (and its FILE *) is destroyed; all
+   *   subsequent emission goes to the new file.
+   *   On failure: diags_log is unchanged; the newly allocated BaseLogFile
+   *   is deleted by setup_diagslog() before it returns.
+   * @errors Open failures are not directly signaled via the return value;
+   *   failure is observable via the emitted Note().
+   * @thread-safety The swap in step 4 is performed under tag_table_lock.
+   *   Concurrent emission either observes the pre-swap or post-swap log;
+   *   no message is lost or written to a destroyed FILE *.
+   * @note When diagnostic output is disabled or redirected to a non-file
+   *   sink (e.g., syslog), diags_log is null or uninitialized and the
+   *   initialization guard causes an early false return. SIGUSR2 is a
+   *   safe no-op in that configuration.
+   */
   bool reseat_diagslog();
+
+  /**
+   * @brief Test whether diags.log is due for a roll under its current policy.
+   *
+   * @return True if the configured rolling condition is met.
+   * @pre None.
+   * @post No state change; rolling is not performed.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock.

Review Comment:
   should_roll_diagslog() docs claim it acquires tag_table_lock, but the 
implementation only takes the lock when swapping diags_log after a roll (and 
does not otherwise lock while evaluating roll conditions). Please update the 
thread-safety note.



##########
include/tscore/DiagsTypes.h:
##########
@@ -174,12 +378,21 @@ class Diags : public DebugInterface
   // raw printing interfaces //
   /////////////////////////////
 
-  ///////////////////////////////////////////////////////////////////////
-  // user diagnostic output interfaces --- enabled on or off based     //
-  // on the value of the enable flag, and the state of the debug tags. //
-  ///////////////////////////////////////////////////////////////////////
-
-  /// Print the log message without respect to whether the tag is enabled.
+  /**
+   * @brief Emit a message unconditionally, regardless of tag state.
+   *
+   * @param[in] tag Non-null C string label; included in output but not checked
+   *   against the tag regex.

Review Comment:
   print() documents tag as non-null, but Diags::print_va() treats the debug 
tag as optional (nullptr means no tag label is printed). Allow nullptr in the 
doc to match the implementation.



##########
include/tscore/DiagsTypes.h:
##########
@@ -220,26 +486,201 @@ class Diags : public DebugInterface
     va_end(ap);
   }
 
+  /**
+   * @brief va_list form of error().
+   *
+   * @param[in] level DiagsLevel for routing and terminal handling.
+   * @param[in] loc Source location, or nullptr.
+   * @param[in] fmt Non-null printf-format string.
+   * @param[in] ap Initialized va_list. Consumed; caller MUST NOT reuse
+   *   without va_end + va_start.
+   * @pre fmt is non-null; ap is initialized.
+   * @post Same as error(). For terminal levels, invokes cleanup_func (if
+   *   set) then exits the process; does not return.
+   * @errors None signaled; I/O errors absorbed.
+   * @thread-safety Safe to call concurrently.
+   */
   virtual void error_va(DiagsLevel level, const SourceLocation *loc, const 
char *fmt, va_list ap) const;
 
+  /**
+   * @brief Print the current Diags configuration to fp.
+   *
+   * @param[in] fp Destination FILE *; defaults to stdout.
+   * @pre fp is a valid open stream.
+   * @post Configuration summary written to fp.
+   * @errors I/O errors on fp are not signaled.
+   * @thread-safety Safe to call concurrently; acquires tag_table_lock
+   *   internally for the tag-table portion of output.
+   */
   void dump(FILE *fp = stdout) const;
 
+  /**
+   * @brief Enable tags matching the given regex pattern for the given mode.
+   *
+   * @param[in] taglist Non-null regex string. Replaces the previous pattern.
+   * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action.
+   * @pre taglist is non-null and contains a valid POSIX extended regex.
+   * @post Tags matching taglist are active for mode; previous pattern is
+   *   discarded.
+   * @errors Invalid regex is silently ignored; previous pattern is retained.
+   * @thread-safety Acquires tag_table_lock. Safe to call concurrently with
+   *   emission, but serialized with other reconfiguration methods.
+   */
   void activate_taglist(const char *taglist, DiagsTagType mode = 
DiagsTagType_Debug);
 
+  /**
+   * @brief Disable all tags for the given mode.
+   *
+   * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action.
+   * @pre None.
+   * @post No tag matches for mode; activated_tags[mode] is null.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock. Safe to call concurrently with
+   *   emission, but serialized with other reconfiguration methods.
+   */
   void deactivate_all(DiagsTagType mode = DiagsTagType_Debug);
 
+  /**
+   * @brief Install the given BaseLogFile as the active diagnostic log,
+   *   taking ownership unconditionally.
+   *
+   * @param[in] blf Non-null pointer to a constructed but not-yet-opened
+   *   BaseLogFile. Ownership always transfers to this function: on success
+   *   the Diags instance manages blf; on failure blf is deleted by this
+   *   function before returning.
+   * @return True if blf was opened and installed; false if open failed
+   *   (blf is deleted before returning false).
+   * @pre blf is non-null; blf->is_init() is false.
+   * @post On success: blf is open and diags_log points to it.
+   *   On failure: blf has been deleted; diags_log is unchanged.
+   * @errors Open failures cause a false return and an internal log_log
+   *   diagnostic at LL_Error.
+   * @thread-safety Caller MUST serialize with all other reconfiguration
+   *   methods. The swap step inside reseat_diagslog() acquires
+   *   tag_table_lock; direct callers of setup_diagslog() must provide
+   *   equivalent serialization.
+   */
   bool setup_diagslog(BaseLogFile *blf);
+
+  /**
+   * @brief Configure the rolling policy for diags.log.
+   *
+   * @param[in] re Rolling policy (see RollingEnabledValues).
+   * @param[in] ri Rolling interval in seconds (used by time-based policies).
+   * @param[in] rs Rolling size threshold in bytes (used by size-based
+   *   policies).
+   * @pre None.
+   * @post Rolling policy for diags.log is updated; effective on the next
+   *   should_roll_diagslog() call.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock.
+   */
   void config_roll_diagslog(RollingEnabledValues re, int ri, int rs);
+
+  /**
+   * @brief Configure the rolling policy for the output log (traffic.out).
+   *
+   * @param[in] re Rolling policy.
+   * @param[in] ri Rolling interval in seconds.
+   * @param[in] rs Rolling size threshold in bytes.
+   * @pre None.
+   * @post Rolling policy for the output log is updated.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock.
+   */
   void config_roll_outputlog(RollingEnabledValues re, int ri, int rs);
+
+  /**
+   * @brief Close the current diags.log and reopen it at the configured path.
+   *
+   * Intended for use after an external log rotation tool has renamed or
+   * removed the active diags.log. The implementation:
+   *  1. Flushes the current file's buffered output.
+   *  2. Captures the configured filename from the active BaseLogFile.
+   *  3. Constructs a new BaseLogFile at that filename. The filename is
+   *     passed to the OS verbatim — symlinks are re-resolved at each call.
+   *  4. On success: atomically swaps the new file into place under
+   *     tag_table_lock and destroys the previous BaseLogFile (closing its
+   *     FILE *).
+   *  5. On failure: leaves the previous file in place.
+   *
+   * @return True if diags_log was successfully reseated; false if the
+   *   existing log was not initialized (the pre-init early-return path).
+   *   Note: the current implementation returns true even when
+   *   setup_diagslog() fails; callers MUST inspect the emitted Note() to
+   *   determine actual success.
+   * @pre A Diags instance is active and diags_log is initialized
+   *   (is_init() == true). If this precondition is not met, returns false
+   *   without performing a reopen — this is the safe no-op path for calls
+   *   that arrive before the diagnostics subsystem is initialized.
+   * @post On success: diags_log is a fresh BaseLogFile at the configured
+   *   path; the previous BaseLogFile (and its FILE *) is destroyed; all
+   *   subsequent emission goes to the new file.
+   *   On failure: diags_log is unchanged; the newly allocated BaseLogFile
+   *   is deleted by setup_diagslog() before it returns.
+   * @errors Open failures are not directly signaled via the return value;
+   *   failure is observable via the emitted Note().
+   * @thread-safety The swap in step 4 is performed under tag_table_lock.
+   *   Concurrent emission either observes the pre-swap or post-swap log;
+   *   no message is lost or written to a destroyed FILE *.
+   * @note When diagnostic output is disabled or redirected to a non-file
+   *   sink (e.g., syslog), diags_log is null or uninitialized and the
+   *   initialization guard causes an early false return. SIGUSR2 is a
+   *   safe no-op in that configuration.
+   */
   bool reseat_diagslog();
+
+  /**
+   * @brief Test whether diags.log is due for a roll under its current policy.
+   *
+   * @return True if the configured rolling condition is met.
+   * @pre None.
+   * @post No state change; rolling is not performed.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock.
+   */
   bool should_roll_diagslog();
+
+  /**
+   * @brief Test whether the output log is due for a roll under its current
+   *   policy.
+   *
+   * @return True if the configured rolling condition is met.
+   * @pre None.
+   * @post No state change; rolling is not performed.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock.

Review Comment:
   should_roll_outputlog() docs claim it acquires tag_table_lock, but the 
implementation does not lock while checking file size/time (it only takes the 
lock inside set_std_output() during rebinding). Please make the thread-safety 
statement precise.



##########
include/tscore/DiagsTypes.h:
##########
@@ -189,9 +402,36 @@ class Diags : public DebugInterface
     va_end(ap);
   }
 
+  /**
+   * @brief va_list form of print().
+   *
+   * @param[in] tag Non-null tag label.
+   * @param[in] level DiagsLevel for routing.
+   * @param[in] loc Source location, or nullptr.
+   * @param[in] fmt Non-null printf-format string.
+   * @param[in] ap Initialized va_list whose types match fmt. Consumed by this
+   *   call; the caller MUST NOT reuse ap without va_end + va_start.
+   * @pre fmt is non-null; ap is initialized.
+   * @post Same as print().
+   * @errors Same as print().
+   * @thread-safety Same as print().
+   */
   void print_va(const char *tag, DiagsLevel level, const SourceLocation *loc, 
const char *fmt, va_list ap) const override;
 
-  /// Print the log message only if tag is enabled.
+  /**
+   * @brief Emit a message only if the tag is active in DiagsTagType_Debug.
+   *
+   * @param[in] tag Non-null C string naming the debug tag.
+   * @param[in] level DiagsLevel for routing.
+   * @param[in] loc Source location, or nullptr.
+   * @param[in] fmt Non-null printf-format string.
+   * @pre tag and fmt are non-null; arguments match fmt's conversions.

Review Comment:
   log() documentation requires tag be non-null, but the implementation allows 
nullptr (on(nullptr) becomes the global enable check because 
tag_activated(nullptr) returns true). Update the doc and precondition to 
reflect this behavior.



##########
include/tscore/DiagsTypes.h:
##########
@@ -220,26 +486,201 @@ class Diags : public DebugInterface
     va_end(ap);
   }
 
+  /**
+   * @brief va_list form of error().
+   *
+   * @param[in] level DiagsLevel for routing and terminal handling.
+   * @param[in] loc Source location, or nullptr.
+   * @param[in] fmt Non-null printf-format string.
+   * @param[in] ap Initialized va_list. Consumed; caller MUST NOT reuse
+   *   without va_end + va_start.
+   * @pre fmt is non-null; ap is initialized.
+   * @post Same as error(). For terminal levels, invokes cleanup_func (if
+   *   set) then exits the process; does not return.
+   * @errors None signaled; I/O errors absorbed.
+   * @thread-safety Safe to call concurrently.
+   */
   virtual void error_va(DiagsLevel level, const SourceLocation *loc, const 
char *fmt, va_list ap) const;
 
+  /**
+   * @brief Print the current Diags configuration to fp.
+   *
+   * @param[in] fp Destination FILE *; defaults to stdout.
+   * @pre fp is a valid open stream.
+   * @post Configuration summary written to fp.
+   * @errors I/O errors on fp are not signaled.
+   * @thread-safety Safe to call concurrently; acquires tag_table_lock
+   *   internally for the tag-table portion of output.
+   */
   void dump(FILE *fp = stdout) const;
 
+  /**
+   * @brief Enable tags matching the given regex pattern for the given mode.
+   *
+   * @param[in] taglist Non-null regex string. Replaces the previous pattern.
+   * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action.
+   * @pre taglist is non-null and contains a valid POSIX extended regex.
+   * @post Tags matching taglist are active for mode; previous pattern is
+   *   discarded.
+   * @errors Invalid regex is silently ignored; previous pattern is retained.
+   * @thread-safety Acquires tag_table_lock. Safe to call concurrently with
+   *   emission, but serialized with other reconfiguration methods.
+   */
   void activate_taglist(const char *taglist, DiagsTagType mode = 
DiagsTagType_Debug);
 
+  /**
+   * @brief Disable all tags for the given mode.
+   *
+   * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action.
+   * @pre None.
+   * @post No tag matches for mode; activated_tags[mode] is null.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock. Safe to call concurrently with
+   *   emission, but serialized with other reconfiguration methods.
+   */
   void deactivate_all(DiagsTagType mode = DiagsTagType_Debug);
 
+  /**
+   * @brief Install the given BaseLogFile as the active diagnostic log,
+   *   taking ownership unconditionally.
+   *
+   * @param[in] blf Non-null pointer to a constructed but not-yet-opened
+   *   BaseLogFile. Ownership always transfers to this function: on success
+   *   the Diags instance manages blf; on failure blf is deleted by this
+   *   function before returning.
+   * @return True if blf was opened and installed; false if open failed
+   *   (blf is deleted before returning false).
+   * @pre blf is non-null; blf->is_init() is false.
+   * @post On success: blf is open and diags_log points to it.
+   *   On failure: blf has been deleted; diags_log is unchanged.
+   * @errors Open failures cause a false return and an internal log_log
+   *   diagnostic at LL_Error.
+   * @thread-safety Caller MUST serialize with all other reconfiguration
+   *   methods. The swap step inside reseat_diagslog() acquires
+   *   tag_table_lock; direct callers of setup_diagslog() must provide
+   *   equivalent serialization.
+   */
   bool setup_diagslog(BaseLogFile *blf);
+
+  /**
+   * @brief Configure the rolling policy for diags.log.
+   *
+   * @param[in] re Rolling policy (see RollingEnabledValues).
+   * @param[in] ri Rolling interval in seconds (used by time-based policies).
+   * @param[in] rs Rolling size threshold in bytes (used by size-based
+   *   policies).
+   * @pre None.
+   * @post Rolling policy for diags.log is updated; effective on the next
+   *   should_roll_diagslog() call.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock.

Review Comment:
   config_roll_diagslog() is documented as acquiring tag_table_lock, but the 
implementation just assigns member variables without locking. The thread-safety 
note should reflect that there's no internal synchronization.



##########
include/tscore/DiagsTypes.h:
##########
@@ -220,26 +486,201 @@ class Diags : public DebugInterface
     va_end(ap);
   }
 
+  /**
+   * @brief va_list form of error().
+   *
+   * @param[in] level DiagsLevel for routing and terminal handling.
+   * @param[in] loc Source location, or nullptr.
+   * @param[in] fmt Non-null printf-format string.
+   * @param[in] ap Initialized va_list. Consumed; caller MUST NOT reuse
+   *   without va_end + va_start.
+   * @pre fmt is non-null; ap is initialized.
+   * @post Same as error(). For terminal levels, invokes cleanup_func (if
+   *   set) then exits the process; does not return.
+   * @errors None signaled; I/O errors absorbed.
+   * @thread-safety Safe to call concurrently.
+   */
   virtual void error_va(DiagsLevel level, const SourceLocation *loc, const 
char *fmt, va_list ap) const;
 
+  /**
+   * @brief Print the current Diags configuration to fp.
+   *
+   * @param[in] fp Destination FILE *; defaults to stdout.
+   * @pre fp is a valid open stream.
+   * @post Configuration summary written to fp.
+   * @errors I/O errors on fp are not signaled.
+   * @thread-safety Safe to call concurrently; acquires tag_table_lock
+   *   internally for the tag-table portion of output.
+   */
   void dump(FILE *fp = stdout) const;
 
+  /**
+   * @brief Enable tags matching the given regex pattern for the given mode.
+   *
+   * @param[in] taglist Non-null regex string. Replaces the previous pattern.
+   * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action.
+   * @pre taglist is non-null and contains a valid POSIX extended regex.
+   * @post Tags matching taglist are active for mode; previous pattern is
+   *   discarded.
+   * @errors Invalid regex is silently ignored; previous pattern is retained.
+   * @thread-safety Acquires tag_table_lock. Safe to call concurrently with
+   *   emission, but serialized with other reconfiguration methods.
+   */
   void activate_taglist(const char *taglist, DiagsTagType mode = 
DiagsTagType_Debug);
 
+  /**
+   * @brief Disable all tags for the given mode.
+   *
+   * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action.
+   * @pre None.
+   * @post No tag matches for mode; activated_tags[mode] is null.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock. Safe to call concurrently with
+   *   emission, but serialized with other reconfiguration methods.
+   */
   void deactivate_all(DiagsTagType mode = DiagsTagType_Debug);
 
+  /**
+   * @brief Install the given BaseLogFile as the active diagnostic log,
+   *   taking ownership unconditionally.
+   *
+   * @param[in] blf Non-null pointer to a constructed but not-yet-opened
+   *   BaseLogFile. Ownership always transfers to this function: on success
+   *   the Diags instance manages blf; on failure blf is deleted by this
+   *   function before returning.
+   * @return True if blf was opened and installed; false if open failed
+   *   (blf is deleted before returning false).
+   * @pre blf is non-null; blf->is_init() is false.
+   * @post On success: blf is open and diags_log points to it.
+   *   On failure: blf has been deleted; diags_log is unchanged.
+   * @errors Open failures cause a false return and an internal log_log
+   *   diagnostic at LL_Error.
+   * @thread-safety Caller MUST serialize with all other reconfiguration
+   *   methods. The swap step inside reseat_diagslog() acquires
+   *   tag_table_lock; direct callers of setup_diagslog() must provide
+   *   equivalent serialization.
+   */
   bool setup_diagslog(BaseLogFile *blf);
+
+  /**
+   * @brief Configure the rolling policy for diags.log.
+   *
+   * @param[in] re Rolling policy (see RollingEnabledValues).
+   * @param[in] ri Rolling interval in seconds (used by time-based policies).
+   * @param[in] rs Rolling size threshold in bytes (used by size-based
+   *   policies).
+   * @pre None.
+   * @post Rolling policy for diags.log is updated; effective on the next
+   *   should_roll_diagslog() call.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock.
+   */
   void config_roll_diagslog(RollingEnabledValues re, int ri, int rs);
+
+  /**
+   * @brief Configure the rolling policy for the output log (traffic.out).
+   *
+   * @param[in] re Rolling policy.
+   * @param[in] ri Rolling interval in seconds.
+   * @param[in] rs Rolling size threshold in bytes.
+   * @pre None.
+   * @post Rolling policy for the output log is updated.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock.

Review Comment:
   config_roll_outputlog() is documented as acquiring tag_table_lock, but the 
implementation does not lock. Please update the thread-safety note to avoid 
claiming internal locking.



##########
include/tscore/DiagsTypes.h:
##########
@@ -45,111 +45,294 @@
 #include "tscore/ink_mutex.h"
 #include "tsutil/Regex.h"
 
+/// Sentinel value stored in Diags::magic to detect heap corruption or
+/// use-after-free of a Diags instance. Diags::magic == DIAGS_MAGIC for
+/// any properly constructed, living instance.
 #define DIAGS_MAGIC 0x12345678
-#define BYTES_IN_MB 1000000
 
-enum DiagsTagType {
-  DiagsTagType_Debug  = 0, // do not renumber --- used as array index
-  DiagsTagType_Action = 1
-};
+/// Bytes-per-megabyte (decimal, not binary). Used by configuration
+/// parsing to convert MB-valued knobs into byte counts.
+#define BYTES_IN_MB 1000000
 
+/**
+ * @brief Selects which of the two tag tables a Diags operation addresses:
+ *   debug tags (controlling Dbg/Diag emission) or action tags (controlling
+ *   is_action_tag_set conditional code paths).
+ *
+ * @note Numeric values are used as array indices into
+ *   DiagsConfigState::_enabled and Diags::activated_tags. Do not renumber.
+ */
+enum DiagsTagType { DiagsTagType_Debug = 0, DiagsTagType_Action = 1 };
+
+/**
+ * @brief Per-DiagsLevel output destination configuration.
+ *
+ * Each boolean controls whether messages at the owning level are emitted
+ * to the corresponding sink. Multiple sinks may be set simultaneously;
+ * the message is emitted to all enabled sinks in unspecified order.
+ *
+ * @thread-safety Carries no internal synchronization. Callers reading or
+ *   modifying Diags::config.outputs[] must respect the concurrency contract
+ *   of the containing DiagsConfigState.
+ */
 struct DiagsModeOutput {
   bool to_stdout;
   bool to_stderr;
   bool to_syslog;
   bool to_diagslog;
 };
 
+/**
+ * @brief Identifies which standard stream Diags::set_std_output reseats.
+ *
+ * STDOUT (=0) targets the standard output stream; STDERR targets the
+ * standard error stream. Values are used as array indices; do not renumber.
+ */
 enum StdStream { STDOUT = 0, STDERR };
 
+/**
+ * @brief Selects the rolling policy for a managed log file.
+ *
+ * - NO_ROLLING             — never roll; file grows without bound.
+ * - ROLL_ON_TIME           — roll when the configured time interval elapses.
+ * - ROLL_ON_SIZE           — roll when the configured size threshold is 
exceeded.
+ * - ROLL_ON_TIME_OR_SIZE   — roll when either condition is satisfied.
+ * - INVALID_ROLLING_VALUE  — sentinel; any code path receiving this value MUST
+ *   treat it as NO_ROLLING. MUST be the largest enumerator so that range 
checks
+ *   can detect out-of-range configuration values.
+ */
 enum RollingEnabledValues { NO_ROLLING = 0, ROLL_ON_TIME, ROLL_ON_SIZE, 
ROLL_ON_TIME_OR_SIZE, INVALID_ROLLING_VALUE };
 
+/// Count of valid DiagsLevel values; equal to DL_Undefined and used to
+/// size DiagsConfigState::outputs[].
 #define DiagsLevel_Count DL_Undefined
 
+/// True if the given DiagsLevel causes process termination after emission.
+/// Terminal levels are DL_Fatal and above (excluding DL_Undefined).
 #define DiagsLevel_IsTerminal(_l) (((_l) >= DL_Fatal) && ((_l) < DL_Undefined))
 
-// Cleanup Function Prototype - Called before ink_fatal to
-//   cleanup process state
+/**
+ * @brief Cleanup callback invoked before process termination on a terminal
+ *   DiagsLevel (Fatal, Alert, Emergency).
+ *
+ * @pre The callback runs on the thread that emitted the terminal message.
+ *   It MUST NOT throw. It MUST be async-signal-safe if terminal-level
+ *   emission can occur from a signal handler.
+ * @post On return the process continues into fatal-exit logic.
+ * @thread-safety Only one callback is installed per Diags instance.
+ *   The callback is invoked at most once per process.
+ */
 using DiagsCleanupFunc = void (*)();
 
+/**
+ * @brief Process-global enable state for the two tag tables.
+ *
+ * Exposes two static methods to read and write the per-tag enable state.
+ * The state for each DiagsTagType is one of:
+ *   0 — disabled (no emission),
+ *   1 — enabled for all callers,
+ *   2 — enabled only when ContFlags::DEBUG_OVERRIDE is set on the
+ *       current Continuation (per-connection debug override).
+ *
+ */
 class DiagsConfigState
 {
 public:
+  /**
+   * @brief Return the current enable state for the given tag type.
+   *
+   * @param[in] dtt DiagsTagType_Debug or DiagsTagType_Action.
+   * @return 0 (disabled), 1 (enabled), or 2 (DEBUG_OVERRIDE mode).
+   * @pre None.
+   * @post No state change.
+   * @errors None.
+   * @thread-safety Intended semantics: relaxed atomic load, safe to call
+   *   concurrently.
+   */
   static int
   enabled(DiagsTagType dtt)
   {
     return _enabled[dtt];
   }
 
+  /**
+   * @brief Set the enable state for the given tag type.
+   *
+   * @param[in] dtt DiagsTagType_Debug or DiagsTagType_Action.
+   * @param[in] new_value 0, 1, or 2 (see class contract).
+   * @pre new_value is in [0, 2].
+   * @post enabled(dtt) returns new_value on all subsequent calls.
+   * @errors None.
+   * @thread-safety Must be called with external serialization against
+   *   concurrent enabled(dtt) reads.
+   */
   static void enabled(DiagsTagType dtt, int new_value);
 
-  DiagsModeOutput outputs[DiagsLevel_Count]; // where each level prints
+  /// Per-level output routing. Each element controls where messages at
+  /// the corresponding DiagsLevel are sent. See DiagsModeOutput.
+  DiagsModeOutput outputs[DiagsLevel_Count];
 
 private:
   static int _enabled[2]; // one debug, one action
 };
 
-//////////////////////////////////////////////////////////////////////////////
-//
-//      class Diags
-//
-//      The Diags class is used for global configuration of the run-time
-//      diagnostics system.  This class provides the following services:
-//
-//      * run-time notices, debugging, warnings, errors
-//      * debugging tags to selectively enable & disable diagnostics
-//      * action tags to selectively enable & disable code paths
-//      * configurable output to stdout, stderr, syslog, error logs
-//      * interface to supporting on-the-fly reconfiguration
-//
-//////////////////////////////////////////////////////////////////////////////
-
+/**
+ * @brief Active diagnostic emission and tag-table state for the process.
+ *
+ * Owns the diags.log, stdout, and stderr BaseLogFile handles, the per-level
+ * output routing configuration, the regex tables for debug and action tags,
+ * and the rolling policy for the diagnostic and standard-output logs.
+ * All diagnostic emission macros (Status, Note, Warning, Error, Fatal, Alert,
+ * Emergency, Diag) route through the process-global Diags instance installed
+ * via DiagsPtr.
+ *
+ * @thread-safety After construction, the instance is safe to use concurrently
+ *   from any number of threads for emission (print, log, error). 
Reconfiguration
+ *   methods (activate_taglist, deactivate_all, setup_diagslog, config_roll_*,
+ *   reseat_diagslog, should_roll_*, set_std_output) acquire an internal mutex
+ *   and are mutually exclusive with each other and with tag-table reads. See
+ *   per-method @thread-safety blocks for details.
+ */
 class Diags : public DebugInterface
 {
 public:
+  /**
+   * @brief Construct a Diags instance with the given initial configuration.
+   *
+   * @param[in] prefix_string Tag prefix prepended to all debug output.
+   * @param[in] base_debug_tags Initial debug tag regex, or nullptr for none.
+   * @param[in] base_action_tags Initial action tag regex, or nullptr for none.
+   * @param[in] _diags_log BaseLogFile for diags.log output, or nullptr to
+   *   suppress file logging. Ownership transfers to this Diags instance.
+   * @param[in] diags_log_perm File permission bits for diags.log, or -1 for
+   *   the system default (LOGFILE_DEFAULT_PERMS).
+   * @param[in] output_log_perm File permission bits for stdout/stderr log
+   *   files, or -1 for the system default.
+   * @pre None; safe to construct before any thread reads via diags().
+   * @post magic == DIAGS_MAGIC; tag tables are initialized from the given
+   *   regexes; diags_log is open if _diags_log is non-null and openable.
+   * @errors Allocation failures abort via ink_fatal. Log-open failures are
+   *   reported via log_log_error and result in a null m_fp for the log.
+   * @thread-safety Single-threaded construction expected. After construction
+   *   the instance may be installed via DiagsPtr::set and used concurrently.
+   */
   Diags(std::string_view prefix_string, const char *base_debug_tags, const 
char *base_action_tags, BaseLogFile *_diags_log,
         int diags_log_perm = -1, int output_log_perm = -1);
+
+  /**
+   * @brief Destroy the Diags instance, closing all owned log files.
+   *
+   * @pre No thread is currently executing a method on this instance.
+   * @post All owned BaseLogFile handles are deleted (closing their FILE *).
+   * @errors None.
+   * @thread-safety Single-threaded destruction. The caller must ensure no
+   *   other thread holds or will acquire a reference to this instance.
+   */
   virtual ~Diags();
 
+  /// Diagnostic log file. Owned by this instance. Read-only from outside
+  /// the tscore module; mutation via any path other than setup_diagslog /
+  /// reseat_diagslog is undefined behavior.
   BaseLogFile *diags_log;
+
+  /// Standard-output redirection file. Owned by this instance.
+  /// Read-only from outside the tscore module.
   BaseLogFile *stdout_log;
+
+  /// Standard-error redirection file. Owned by this instance.
+  /// Read-only from outside the tscore module.
   BaseLogFile *stderr_log;
 
+  /// Sentinel. Always DIAGS_MAGIC for a live, properly constructed instance.
   const unsigned int magic;
-  DiagsConfigState   config;
-  DiagsShowLocation  show_location;
-  DiagsCleanupFunc   cleanup_func;
+
+  /// Per-level output routing and tag-enable state. See DiagsConfigState.
+  DiagsConfigState config;
+
+  /// Controls whether source location is appended to emitted messages.
+  DiagsShowLocation show_location;
+
+  /// Optional cleanup callback invoked before terminal-level process exit.
+  /// May be nullptr (no cleanup). See DiagsCleanupFunc.
+  DiagsCleanupFunc cleanup_func;
 
   ///////////////////////////
   // conditional debugging //
   ///////////////////////////
 
+  /**
+   * @brief Return the per-connection DEBUG_OVERRIDE flag for the current
+   *   Continuation.
+   *
+   * @return True if ContFlags::DEBUG_OVERRIDE is set on the currently
+   *   executing Continuation; false otherwise or if no Continuation is active.
+   * @pre None.
+   * @post No state change.
+   * @errors None.
+   * @thread-safety Safe to call from any thread; reads a thread-local
+   *   Continuation flag.
+   */
   bool
   get_override() const override
   {
     return get_cont_flag(ContFlags::DEBUG_OVERRIDE);
   }
 
+  /**
+   * @brief Test whether the given IP endpoint matches the configured debug
+   *   client IP.
+   *
+   * @param[in] test_ip Endpoint to compare against debug_client_ip.
+   * @return True if test_ip equals debug_client_ip.
+   * @pre None.
+   * @post No state change.
+   * @errors None.
+   * @thread-safety Safe to call concurrently; debug_client_ip is written
+   *   only during single-threaded reconfiguration.

Review Comment:
   test_override_ip() is documented as safe concurrently because 
debug_client_ip is written only during single-threaded reconfiguration, but 
debug_client_ip is updated at runtime via the 
proxy.config.diags.debug.client_ip records callback. The thread-safety note 
should not assume single-threaded writes.



##########
include/tscore/DiagsTypes.h:
##########
@@ -45,111 +45,294 @@
 #include "tscore/ink_mutex.h"
 #include "tsutil/Regex.h"
 
+/// Sentinel value stored in Diags::magic to detect heap corruption or
+/// use-after-free of a Diags instance. Diags::magic == DIAGS_MAGIC for
+/// any properly constructed, living instance.
 #define DIAGS_MAGIC 0x12345678
-#define BYTES_IN_MB 1000000
 
-enum DiagsTagType {
-  DiagsTagType_Debug  = 0, // do not renumber --- used as array index
-  DiagsTagType_Action = 1
-};
+/// Bytes-per-megabyte (decimal, not binary). Used by configuration
+/// parsing to convert MB-valued knobs into byte counts.
+#define BYTES_IN_MB 1000000
 
+/**
+ * @brief Selects which of the two tag tables a Diags operation addresses:
+ *   debug tags (controlling Dbg/Diag emission) or action tags (controlling
+ *   is_action_tag_set conditional code paths).
+ *
+ * @note Numeric values are used as array indices into
+ *   DiagsConfigState::_enabled and Diags::activated_tags. Do not renumber.
+ */
+enum DiagsTagType { DiagsTagType_Debug = 0, DiagsTagType_Action = 1 };
+
+/**
+ * @brief Per-DiagsLevel output destination configuration.
+ *
+ * Each boolean controls whether messages at the owning level are emitted
+ * to the corresponding sink. Multiple sinks may be set simultaneously;
+ * the message is emitted to all enabled sinks in unspecified order.
+ *
+ * @thread-safety Carries no internal synchronization. Callers reading or
+ *   modifying Diags::config.outputs[] must respect the concurrency contract
+ *   of the containing DiagsConfigState.
+ */
 struct DiagsModeOutput {
   bool to_stdout;
   bool to_stderr;
   bool to_syslog;
   bool to_diagslog;
 };
 
+/**
+ * @brief Identifies which standard stream Diags::set_std_output reseats.
+ *
+ * STDOUT (=0) targets the standard output stream; STDERR targets the
+ * standard error stream. Values are used as array indices; do not renumber.
+ */
 enum StdStream { STDOUT = 0, STDERR };
 
+/**
+ * @brief Selects the rolling policy for a managed log file.
+ *
+ * - NO_ROLLING             — never roll; file grows without bound.
+ * - ROLL_ON_TIME           — roll when the configured time interval elapses.
+ * - ROLL_ON_SIZE           — roll when the configured size threshold is 
exceeded.
+ * - ROLL_ON_TIME_OR_SIZE   — roll when either condition is satisfied.
+ * - INVALID_ROLLING_VALUE  — sentinel; any code path receiving this value MUST
+ *   treat it as NO_ROLLING. MUST be the largest enumerator so that range 
checks
+ *   can detect out-of-range configuration values.
+ */
 enum RollingEnabledValues { NO_ROLLING = 0, ROLL_ON_TIME, ROLL_ON_SIZE, 
ROLL_ON_TIME_OR_SIZE, INVALID_ROLLING_VALUE };
 
+/// Count of valid DiagsLevel values; equal to DL_Undefined and used to
+/// size DiagsConfigState::outputs[].
 #define DiagsLevel_Count DL_Undefined
 
+/// True if the given DiagsLevel causes process termination after emission.
+/// Terminal levels are DL_Fatal and above (excluding DL_Undefined).
 #define DiagsLevel_IsTerminal(_l) (((_l) >= DL_Fatal) && ((_l) < DL_Undefined))
 
-// Cleanup Function Prototype - Called before ink_fatal to
-//   cleanup process state
+/**
+ * @brief Cleanup callback invoked before process termination on a terminal
+ *   DiagsLevel (Fatal, Alert, Emergency).
+ *
+ * @pre The callback runs on the thread that emitted the terminal message.
+ *   It MUST NOT throw. It MUST be async-signal-safe if terminal-level
+ *   emission can occur from a signal handler.
+ * @post On return the process continues into fatal-exit logic.
+ * @thread-safety Only one callback is installed per Diags instance.
+ *   The callback is invoked at most once per process.
+ */
 using DiagsCleanupFunc = void (*)();
 
+/**
+ * @brief Process-global enable state for the two tag tables.
+ *
+ * Exposes two static methods to read and write the per-tag enable state.
+ * The state for each DiagsTagType is one of:
+ *   0 — disabled (no emission),
+ *   1 — enabled for all callers,
+ *   2 — enabled only when ContFlags::DEBUG_OVERRIDE is set on the
+ *       current Continuation (per-connection debug override).
+ *
+ */
 class DiagsConfigState
 {
 public:
+  /**
+   * @brief Return the current enable state for the given tag type.
+   *
+   * @param[in] dtt DiagsTagType_Debug or DiagsTagType_Action.
+   * @return 0 (disabled), 1 (enabled), or 2 (DEBUG_OVERRIDE mode).
+   * @pre None.
+   * @post No state change.
+   * @errors None.
+   * @thread-safety Intended semantics: relaxed atomic load, safe to call
+   *   concurrently.
+   */
   static int
   enabled(DiagsTagType dtt)
   {
     return _enabled[dtt];
   }
 
+  /**
+   * @brief Set the enable state for the given tag type.
+   *
+   * @param[in] dtt DiagsTagType_Debug or DiagsTagType_Action.
+   * @param[in] new_value 0, 1, or 2 (see class contract).
+   * @pre new_value is in [0, 2].
+   * @post enabled(dtt) returns new_value on all subsequent calls.
+   * @errors None.
+   * @thread-safety Must be called with external serialization against
+   *   concurrent enabled(dtt) reads.
+   */
   static void enabled(DiagsTagType dtt, int new_value);
 
-  DiagsModeOutput outputs[DiagsLevel_Count]; // where each level prints
+  /// Per-level output routing. Each element controls where messages at
+  /// the corresponding DiagsLevel are sent. See DiagsModeOutput.
+  DiagsModeOutput outputs[DiagsLevel_Count];
 
 private:
   static int _enabled[2]; // one debug, one action
 };
 
-//////////////////////////////////////////////////////////////////////////////
-//
-//      class Diags
-//
-//      The Diags class is used for global configuration of the run-time
-//      diagnostics system.  This class provides the following services:
-//
-//      * run-time notices, debugging, warnings, errors
-//      * debugging tags to selectively enable & disable diagnostics
-//      * action tags to selectively enable & disable code paths
-//      * configurable output to stdout, stderr, syslog, error logs
-//      * interface to supporting on-the-fly reconfiguration
-//
-//////////////////////////////////////////////////////////////////////////////
-
+/**
+ * @brief Active diagnostic emission and tag-table state for the process.
+ *
+ * Owns the diags.log, stdout, and stderr BaseLogFile handles, the per-level
+ * output routing configuration, the regex tables for debug and action tags,
+ * and the rolling policy for the diagnostic and standard-output logs.
+ * All diagnostic emission macros (Status, Note, Warning, Error, Fatal, Alert,
+ * Emergency, Diag) route through the process-global Diags instance installed
+ * via DiagsPtr.
+ *
+ * @thread-safety After construction, the instance is safe to use concurrently
+ *   from any number of threads for emission (print, log, error). 
Reconfiguration
+ *   methods (activate_taglist, deactivate_all, setup_diagslog, config_roll_*,
+ *   reseat_diagslog, should_roll_*, set_std_output) acquire an internal mutex
+ *   and are mutually exclusive with each other and with tag-table reads. See
+ *   per-method @thread-safety blocks for details.
+ */
 class Diags : public DebugInterface
 {
 public:
+  /**
+   * @brief Construct a Diags instance with the given initial configuration.
+   *
+   * @param[in] prefix_string Tag prefix prepended to all debug output.
+   * @param[in] base_debug_tags Initial debug tag regex, or nullptr for none.
+   * @param[in] base_action_tags Initial action tag regex, or nullptr for none.
+   * @param[in] _diags_log BaseLogFile for diags.log output, or nullptr to
+   *   suppress file logging. Ownership transfers to this Diags instance.
+   * @param[in] diags_log_perm File permission bits for diags.log, or -1 for
+   *   the system default (LOGFILE_DEFAULT_PERMS).
+   * @param[in] output_log_perm File permission bits for stdout/stderr log
+   *   files, or -1 for the system default.
+   * @pre None; safe to construct before any thread reads via diags().
+   * @post magic == DIAGS_MAGIC; tag tables are initialized from the given
+   *   regexes; diags_log is open if _diags_log is non-null and openable.
+   * @errors Allocation failures abort via ink_fatal. Log-open failures are
+   *   reported via log_log_error and result in a null m_fp for the log.
+   * @thread-safety Single-threaded construction expected. After construction
+   *   the instance may be installed via DiagsPtr::set and used concurrently.
+   */
   Diags(std::string_view prefix_string, const char *base_debug_tags, const 
char *base_action_tags, BaseLogFile *_diags_log,
         int diags_log_perm = -1, int output_log_perm = -1);
+
+  /**
+   * @brief Destroy the Diags instance, closing all owned log files.
+   *
+   * @pre No thread is currently executing a method on this instance.
+   * @post All owned BaseLogFile handles are deleted (closing their FILE *).
+   * @errors None.
+   * @thread-safety Single-threaded destruction. The caller must ensure no
+   *   other thread holds or will acquire a reference to this instance.
+   */
   virtual ~Diags();
 
+  /// Diagnostic log file. Owned by this instance. Read-only from outside
+  /// the tscore module; mutation via any path other than setup_diagslog /
+  /// reseat_diagslog is undefined behavior.
   BaseLogFile *diags_log;
+
+  /// Standard-output redirection file. Owned by this instance.
+  /// Read-only from outside the tscore module.
   BaseLogFile *stdout_log;
+
+  /// Standard-error redirection file. Owned by this instance.
+  /// Read-only from outside the tscore module.
   BaseLogFile *stderr_log;
 
+  /// Sentinel. Always DIAGS_MAGIC for a live, properly constructed instance.
   const unsigned int magic;
-  DiagsConfigState   config;
-  DiagsShowLocation  show_location;
-  DiagsCleanupFunc   cleanup_func;
+
+  /// Per-level output routing and tag-enable state. See DiagsConfigState.
+  DiagsConfigState config;
+
+  /// Controls whether source location is appended to emitted messages.
+  DiagsShowLocation show_location;
+
+  /// Optional cleanup callback invoked before terminal-level process exit.
+  /// May be nullptr (no cleanup). See DiagsCleanupFunc.
+  DiagsCleanupFunc cleanup_func;
 
   ///////////////////////////
   // conditional debugging //
   ///////////////////////////
 
+  /**
+   * @brief Return the per-connection DEBUG_OVERRIDE flag for the current
+   *   Continuation.
+   *
+   * @return True if ContFlags::DEBUG_OVERRIDE is set on the currently
+   *   executing Continuation; false otherwise or if no Continuation is active.
+   * @pre None.
+   * @post No state change.
+   * @errors None.
+   * @thread-safety Safe to call from any thread; reads a thread-local
+   *   Continuation flag.
+   */
   bool
   get_override() const override
   {
     return get_cont_flag(ContFlags::DEBUG_OVERRIDE);
   }
 
+  /**
+   * @brief Test whether the given IP endpoint matches the configured debug
+   *   client IP.
+   *
+   * @param[in] test_ip Endpoint to compare against debug_client_ip.
+   * @return True if test_ip equals debug_client_ip.
+   * @pre None.
+   * @post No state change.
+   * @errors None.
+   * @thread-safety Safe to call concurrently; debug_client_ip is written
+   *   only during single-threaded reconfiguration.
+   */
   bool
   test_override_ip(IpEndpoint const &test_ip)
   {
     return this->debug_client_ip == test_ip;
   }
 
-  // It seems to make a big difference to performance (due to the caching of 
the enabled flag) to call this
-  // function first before doing anything else for debug output.  This 
includes entering blocks with static
-  // DbgCtl instances, or other static variables with non-const 
initialization.  Static variables with
-  // non-const initialization inside a function have a hidden flag that is 
checked every time the containing
-  // block is entered, to see if the variable has been initialized or not.
-  //
+  /**
+   * @brief Test whether emission for the given tag mode is globally enabled.
+   *
+   * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action; defaults to
+   *   DiagsTagType_Debug.
+   * @return True if the tag type is enabled (value 1), OR if it is in
+   *   DEBUG_OVERRIDE mode (value 2) and the current Continuation has the
+   *   override flag set.
+   * @pre None.
+   * @post No state change.
+   * @errors None.
+   * @thread-safety Intended semantics: relaxed atomic load, safe to call
+   *   concurrently. See DiagsConfigState contract.
+   * @note This method is on the hot path of every Dbg/Diag macro. Call it
+   *   before any block-local static with non-const initialization to avoid
+   *   the hidden initialization-check overhead when diagnostics are off.
+   */
   bool
   on(DiagsTagType mode = DiagsTagType_Debug) const
   {
     return (config.enabled(mode) & 1) || (config.enabled(mode) == 2 && 
this->get_override());
   }
 
-  // Returns true if tag is enabled for mode.
-  //
+  /**
+   * @brief Test whether the given tag is active for the given mode.
+   *
+   * @param[in] tag Non-null C string naming the tag to check.

Review Comment:
   The documentation for on(tag, ...) says tag must be non-null, but 
tag_activated(nullptr, ...) explicitly returns true (so on(nullptr) is 
equivalent to the global enable check). Please update the `@param` description 
accordingly.



##########
include/tscore/DiagsTypes.h:
##########
@@ -203,6 +443,20 @@ class Diags : public DebugInterface
     }
   }
 
+  /**
+   * @brief va_list form of log().
+   *
+   * @param[in] tag Non-null tag name.
+   * @param[in] level DiagsLevel for routing.
+   * @param[in] loc Source location, or nullptr.
+   * @param[in] fmt Non-null printf-format string.
+   * @param[in] ap Initialized va_list. Consumed; caller MUST NOT reuse
+   *   without va_end + va_start.
+   * @pre tag and fmt are non-null; ap is initialized.
+   * @post Same as log().

Review Comment:
   log_va() precondition currently requires tag be non-null, but the 
implementation allows nullptr. The `@pre` should only require fmt/ap validity.



##########
include/tscore/DiagsTypes.h:
##########
@@ -220,26 +486,201 @@ class Diags : public DebugInterface
     va_end(ap);
   }
 
+  /**
+   * @brief va_list form of error().
+   *
+   * @param[in] level DiagsLevel for routing and terminal handling.
+   * @param[in] loc Source location, or nullptr.
+   * @param[in] fmt Non-null printf-format string.
+   * @param[in] ap Initialized va_list. Consumed; caller MUST NOT reuse
+   *   without va_end + va_start.
+   * @pre fmt is non-null; ap is initialized.
+   * @post Same as error(). For terminal levels, invokes cleanup_func (if
+   *   set) then exits the process; does not return.
+   * @errors None signaled; I/O errors absorbed.
+   * @thread-safety Safe to call concurrently.
+   */
   virtual void error_va(DiagsLevel level, const SourceLocation *loc, const 
char *fmt, va_list ap) const;
 
+  /**
+   * @brief Print the current Diags configuration to fp.
+   *
+   * @param[in] fp Destination FILE *; defaults to stdout.
+   * @pre fp is a valid open stream.
+   * @post Configuration summary written to fp.
+   * @errors I/O errors on fp are not signaled.
+   * @thread-safety Safe to call concurrently; acquires tag_table_lock
+   *   internally for the tag-table portion of output.
+   */
   void dump(FILE *fp = stdout) const;
 
+  /**
+   * @brief Enable tags matching the given regex pattern for the given mode.
+   *
+   * @param[in] taglist Non-null regex string. Replaces the previous pattern.
+   * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action.
+   * @pre taglist is non-null and contains a valid POSIX extended regex.
+   * @post Tags matching taglist are active for mode; previous pattern is
+   *   discarded.
+   * @errors Invalid regex is silently ignored; previous pattern is retained.
+   * @thread-safety Acquires tag_table_lock. Safe to call concurrently with
+   *   emission, but serialized with other reconfiguration methods.
+   */
   void activate_taglist(const char *taglist, DiagsTagType mode = 
DiagsTagType_Debug);
 
+  /**
+   * @brief Disable all tags for the given mode.
+   *
+   * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action.
+   * @pre None.
+   * @post No tag matches for mode; activated_tags[mode] is null.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock. Safe to call concurrently with
+   *   emission, but serialized with other reconfiguration methods.
+   */
   void deactivate_all(DiagsTagType mode = DiagsTagType_Debug);
 
+  /**
+   * @brief Install the given BaseLogFile as the active diagnostic log,
+   *   taking ownership unconditionally.
+   *
+   * @param[in] blf Non-null pointer to a constructed but not-yet-opened
+   *   BaseLogFile. Ownership always transfers to this function: on success
+   *   the Diags instance manages blf; on failure blf is deleted by this
+   *   function before returning.
+   * @return True if blf was opened and installed; false if open failed
+   *   (blf is deleted before returning false).
+   * @pre blf is non-null; blf->is_init() is false.
+   * @post On success: blf is open and diags_log points to it.
+   *   On failure: blf has been deleted; diags_log is unchanged.
+   * @errors Open failures cause a false return and an internal log_log
+   *   diagnostic at LL_Error.
+   * @thread-safety Caller MUST serialize with all other reconfiguration
+   *   methods. The swap step inside reseat_diagslog() acquires
+   *   tag_table_lock; direct callers of setup_diagslog() must provide
+   *   equivalent serialization.
+   */
   bool setup_diagslog(BaseLogFile *blf);
+
+  /**
+   * @brief Configure the rolling policy for diags.log.
+   *
+   * @param[in] re Rolling policy (see RollingEnabledValues).
+   * @param[in] ri Rolling interval in seconds (used by time-based policies).
+   * @param[in] rs Rolling size threshold in bytes (used by size-based
+   *   policies).
+   * @pre None.
+   * @post Rolling policy for diags.log is updated; effective on the next
+   *   should_roll_diagslog() call.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock.
+   */
   void config_roll_diagslog(RollingEnabledValues re, int ri, int rs);
+
+  /**
+   * @brief Configure the rolling policy for the output log (traffic.out).
+   *
+   * @param[in] re Rolling policy.
+   * @param[in] ri Rolling interval in seconds.
+   * @param[in] rs Rolling size threshold in bytes.
+   * @pre None.
+   * @post Rolling policy for the output log is updated.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock.
+   */
   void config_roll_outputlog(RollingEnabledValues re, int ri, int rs);
+
+  /**
+   * @brief Close the current diags.log and reopen it at the configured path.
+   *
+   * Intended for use after an external log rotation tool has renamed or
+   * removed the active diags.log. The implementation:
+   *  1. Flushes the current file's buffered output.
+   *  2. Captures the configured filename from the active BaseLogFile.
+   *  3. Constructs a new BaseLogFile at that filename. The filename is
+   *     passed to the OS verbatim — symlinks are re-resolved at each call.
+   *  4. On success: atomically swaps the new file into place under
+   *     tag_table_lock and destroys the previous BaseLogFile (closing its
+   *     FILE *).
+   *  5. On failure: leaves the previous file in place.
+   *
+   * @return True if diags_log was successfully reseated; false if the
+   *   existing log was not initialized (the pre-init early-return path).
+   *   Note: the current implementation returns true even when
+   *   setup_diagslog() fails; callers MUST inspect the emitted Note() to
+   *   determine actual success.
+   * @pre A Diags instance is active and diags_log is initialized
+   *   (is_init() == true). If this precondition is not met, returns false
+   *   without performing a reopen — this is the safe no-op path for calls
+   *   that arrive before the diagnostics subsystem is initialized.
+   * @post On success: diags_log is a fresh BaseLogFile at the configured
+   *   path; the previous BaseLogFile (and its FILE *) is destroyed; all
+   *   subsequent emission goes to the new file.
+   *   On failure: diags_log is unchanged; the newly allocated BaseLogFile
+   *   is deleted by setup_diagslog() before it returns.
+   * @errors Open failures are not directly signaled via the return value;
+   *   failure is observable via the emitted Note().
+   * @thread-safety The swap in step 4 is performed under tag_table_lock.
+   *   Concurrent emission either observes the pre-swap or post-swap log;
+   *   no message is lost or written to a destroyed FILE *.
+   * @note When diagnostic output is disabled or redirected to a non-file
+   *   sink (e.g., syslog), diags_log is null or uninitialized and the
+   *   initialization guard causes an early false return. SIGUSR2 is a
+   *   safe no-op in that configuration.
+   */
   bool reseat_diagslog();
+
+  /**
+   * @brief Test whether diags.log is due for a roll under its current policy.
+   *
+   * @return True if the configured rolling condition is met.
+   * @pre None.
+   * @post No state change; rolling is not performed.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock.
+   */
   bool should_roll_diagslog();
+
+  /**
+   * @brief Test whether the output log is due for a roll under its current
+   *   policy.
+   *
+   * @return True if the configured rolling condition is met.
+   * @pre None.
+   * @post No state change; rolling is not performed.
+   * @errors None.
+   * @thread-safety Acquires tag_table_lock.
+   */
   bool should_roll_outputlog();
 
+  /**
+   * @brief Reseat the named standard stream to a file at the given path.
+   *
+   * @param[in] stream STDOUT or STDERR (see StdStream).
+   * @param[in] file Non-null path. Passed verbatim to the OS open call;
+   *   symlinks are re-resolved at each call. The special values "stdout"
+   *   and "stderr" select the corresponding standard streams directly.
+   * @return True on success; false if the file could not be opened.
+   * @pre A Diags instance is active; file is non-null.
+   * @post On success, subsequent emission to the named stream is written
+   *   to the new file; the previous binding is closed. On failure the
+   *   previous binding is preserved.
+   * @errors File-open failures cause a false return.
+   * @thread-safety Caller MUST serialize with concurrent emission to the
+   *   named stream. No internal lock is acquired by this method.

Review Comment:
   set_std_output() docs claim no internal lock is acquired and callers must 
serialize with concurrent emission, but the implementation does acquire 
tag_table_lock while swapping stdout_log/stderr_log and calling 
rebind_std_stream(). The thread-safety contract should reflect the actual 
locking behavior.



##########
include/tscore/DiagsTypes.h:
##########
@@ -45,111 +45,294 @@
 #include "tscore/ink_mutex.h"
 #include "tsutil/Regex.h"
 
+/// Sentinel value stored in Diags::magic to detect heap corruption or
+/// use-after-free of a Diags instance. Diags::magic == DIAGS_MAGIC for
+/// any properly constructed, living instance.
 #define DIAGS_MAGIC 0x12345678
-#define BYTES_IN_MB 1000000
 
-enum DiagsTagType {
-  DiagsTagType_Debug  = 0, // do not renumber --- used as array index
-  DiagsTagType_Action = 1
-};
+/// Bytes-per-megabyte (decimal, not binary). Used by configuration
+/// parsing to convert MB-valued knobs into byte counts.
+#define BYTES_IN_MB 1000000
 
+/**
+ * @brief Selects which of the two tag tables a Diags operation addresses:
+ *   debug tags (controlling Dbg/Diag emission) or action tags (controlling
+ *   is_action_tag_set conditional code paths).
+ *
+ * @note Numeric values are used as array indices into
+ *   DiagsConfigState::_enabled and Diags::activated_tags. Do not renumber.
+ */
+enum DiagsTagType { DiagsTagType_Debug = 0, DiagsTagType_Action = 1 };
+
+/**
+ * @brief Per-DiagsLevel output destination configuration.
+ *
+ * Each boolean controls whether messages at the owning level are emitted
+ * to the corresponding sink. Multiple sinks may be set simultaneously;
+ * the message is emitted to all enabled sinks in unspecified order.
+ *
+ * @thread-safety Carries no internal synchronization. Callers reading or
+ *   modifying Diags::config.outputs[] must respect the concurrency contract
+ *   of the containing DiagsConfigState.
+ */
 struct DiagsModeOutput {
   bool to_stdout;
   bool to_stderr;
   bool to_syslog;
   bool to_diagslog;
 };
 
+/**
+ * @brief Identifies which standard stream Diags::set_std_output reseats.
+ *
+ * STDOUT (=0) targets the standard output stream; STDERR targets the
+ * standard error stream. Values are used as array indices; do not renumber.
+ */
 enum StdStream { STDOUT = 0, STDERR };
 
+/**
+ * @brief Selects the rolling policy for a managed log file.
+ *
+ * - NO_ROLLING             — never roll; file grows without bound.
+ * - ROLL_ON_TIME           — roll when the configured time interval elapses.
+ * - ROLL_ON_SIZE           — roll when the configured size threshold is 
exceeded.
+ * - ROLL_ON_TIME_OR_SIZE   — roll when either condition is satisfied.
+ * - INVALID_ROLLING_VALUE  — sentinel; any code path receiving this value MUST
+ *   treat it as NO_ROLLING. MUST be the largest enumerator so that range 
checks
+ *   can detect out-of-range configuration values.
+ */
 enum RollingEnabledValues { NO_ROLLING = 0, ROLL_ON_TIME, ROLL_ON_SIZE, 
ROLL_ON_TIME_OR_SIZE, INVALID_ROLLING_VALUE };
 
+/// Count of valid DiagsLevel values; equal to DL_Undefined and used to
+/// size DiagsConfigState::outputs[].
 #define DiagsLevel_Count DL_Undefined
 
+/// True if the given DiagsLevel causes process termination after emission.
+/// Terminal levels are DL_Fatal and above (excluding DL_Undefined).
 #define DiagsLevel_IsTerminal(_l) (((_l) >= DL_Fatal) && ((_l) < DL_Undefined))
 
-// Cleanup Function Prototype - Called before ink_fatal to
-//   cleanup process state
+/**
+ * @brief Cleanup callback invoked before process termination on a terminal
+ *   DiagsLevel (Fatal, Alert, Emergency).
+ *
+ * @pre The callback runs on the thread that emitted the terminal message.
+ *   It MUST NOT throw. It MUST be async-signal-safe if terminal-level
+ *   emission can occur from a signal handler.
+ * @post On return the process continues into fatal-exit logic.
+ * @thread-safety Only one callback is installed per Diags instance.
+ *   The callback is invoked at most once per process.
+ */
 using DiagsCleanupFunc = void (*)();
 
+/**
+ * @brief Process-global enable state for the two tag tables.
+ *
+ * Exposes two static methods to read and write the per-tag enable state.
+ * The state for each DiagsTagType is one of:
+ *   0 — disabled (no emission),
+ *   1 — enabled for all callers,
+ *   2 — enabled only when ContFlags::DEBUG_OVERRIDE is set on the
+ *       current Continuation (per-connection debug override).
+ *
+ */
 class DiagsConfigState
 {
 public:
+  /**
+   * @brief Return the current enable state for the given tag type.
+   *
+   * @param[in] dtt DiagsTagType_Debug or DiagsTagType_Action.
+   * @return 0 (disabled), 1 (enabled), or 2 (DEBUG_OVERRIDE mode).
+   * @pre None.
+   * @post No state change.
+   * @errors None.
+   * @thread-safety Intended semantics: relaxed atomic load, safe to call
+   *   concurrently.
+   */
   static int
   enabled(DiagsTagType dtt)
   {
     return _enabled[dtt];
   }
 
+  /**
+   * @brief Set the enable state for the given tag type.
+   *
+   * @param[in] dtt DiagsTagType_Debug or DiagsTagType_Action.
+   * @param[in] new_value 0, 1, or 2 (see class contract).
+   * @pre new_value is in [0, 2].
+   * @post enabled(dtt) returns new_value on all subsequent calls.
+   * @errors None.
+   * @thread-safety Must be called with external serialization against
+   *   concurrent enabled(dtt) reads.
+   */
   static void enabled(DiagsTagType dtt, int new_value);
 
-  DiagsModeOutput outputs[DiagsLevel_Count]; // where each level prints
+  /// Per-level output routing. Each element controls where messages at
+  /// the corresponding DiagsLevel are sent. See DiagsModeOutput.
+  DiagsModeOutput outputs[DiagsLevel_Count];
 
 private:
   static int _enabled[2]; // one debug, one action
 };
 
-//////////////////////////////////////////////////////////////////////////////
-//
-//      class Diags
-//
-//      The Diags class is used for global configuration of the run-time
-//      diagnostics system.  This class provides the following services:
-//
-//      * run-time notices, debugging, warnings, errors
-//      * debugging tags to selectively enable & disable diagnostics
-//      * action tags to selectively enable & disable code paths
-//      * configurable output to stdout, stderr, syslog, error logs
-//      * interface to supporting on-the-fly reconfiguration
-//
-//////////////////////////////////////////////////////////////////////////////
-
+/**
+ * @brief Active diagnostic emission and tag-table state for the process.
+ *
+ * Owns the diags.log, stdout, and stderr BaseLogFile handles, the per-level
+ * output routing configuration, the regex tables for debug and action tags,
+ * and the rolling policy for the diagnostic and standard-output logs.
+ * All diagnostic emission macros (Status, Note, Warning, Error, Fatal, Alert,
+ * Emergency, Diag) route through the process-global Diags instance installed
+ * via DiagsPtr.
+ *
+ * @thread-safety After construction, the instance is safe to use concurrently
+ *   from any number of threads for emission (print, log, error). 
Reconfiguration
+ *   methods (activate_taglist, deactivate_all, setup_diagslog, config_roll_*,
+ *   reseat_diagslog, should_roll_*, set_std_output) acquire an internal mutex
+ *   and are mutually exclusive with each other and with tag-table reads. See
+ *   per-method @thread-safety blocks for details.

Review Comment:
   The class-level `@thread-safety` block says all reconfiguration methods 
acquire an internal mutex and are mutually exclusive with tag-table reads, but 
several listed methods (e.g., setup_diagslog(), config_roll_*(), 
should_roll_*(), dump()) do not lock in the current implementation. The 
high-level contract should not overstate internal serialization.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to