Address Copilot reviews

Author: JosiahWI

include/tscore/Diags.h

@@ -60,35 +60,39 @@
 /**
  * @brief Holder for the process-global Diags pointer.
  *
- * Provides write access via set() (single-threaded initialization) and
- * read access via the free function diags(). After initialization the
- * pointer is read-only and may be read concurrently by any number of threads.
+ * Provides write access via set() and read access via the free function
+ * diags(). The pointer is initially null. set() may be called more than
+ * once to replace the active instance (e.g., to upgrade from a bootstrap
+ * configuration to a fully records-backed one).
  *
- * @thread-safety _diags_ptr is written during single-threaded initialization
- *   before any thread calls diags(). After initialization it is read-only;
- *   concurrent reads are safe. Calling set() after initialization is
- *   undefined behavior.
+ * @thread-safety _diags_ptr is a plain (non-atomic) pointer. The first
+ *   call to set() must complete before any thread calls diags(). A second
+ *   call while other threads are actively calling diags() is a data race
+ *   on _diags_ptr; the behavior is formally undefined. In practice the
+ *   replacement window is narrow, but callers that cache the result of
+ *   diags() across a replacement may observe the previous instance.
  */
 class DiagsPtr
 {
 public:
   friend Diags *diags();
 
   /**
-   * @brief Install the process-global Diags instance.
+   * @brief Install or replace the process-global Diags instance.
    *
    * @param[in] new_ptr Pointer to a fully constructed Diags instance whose
    *   lifetime exceeds all subsequent diags() callers. Ownership is NOT
    *   transferred; the caller remains responsible for the object.
-   * @pre new_ptr is non-null; no thread has called diags() since process
-   *   start (single-threaded initialization).
+   * @pre new_ptr is non-null.
    * @post diags() returns new_ptr on all subsequent calls. Additionally,
    *   DebugInterface::set_instance(new_ptr) is called, registering the Diags
    *   instance as the active DebugInterface so that DbgCtl-based debug output
    *   (Dbg / DbgPrint) is routed correctly.
    * @errors None signaled. A null new_ptr causes subsequent diags() calls
    *   to return nullptr — a precondition violation.
-   * @thread-safety Single-threaded initialization only. See class contract.
+   * @thread-safety The first call must occur before other threads call
+   *   diags(). Subsequent calls while threads are active are a data race
+   *   on _diags_ptr; see class contract.
    */
   static void set(Diags *new_ptr);
 

include/tscore/DiagsTypes.h

@@ -169,7 +169,10 @@ class DiagsConfigState
    * @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.
+   * @post enabled(dtt) returns new_value on all subsequent calls. As a side
+   *   effect, when dtt == DiagsTagType_Debug, DbgCtl::_config_mode is also
+   *   updated via a relaxed atomic store so that DbgCtl-based checks remain
+   *   in sync without acquiring a lock.
    * @errors None.
    * @thread-safety Must be called with external serialization against
    *   concurrent enabled(dtt) reads.
@@ -195,19 +198,21 @@ class DiagsConfigState
  * 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.
+ *   from any number of threads for emission (print, log, error). Tag-table
+ *   mutation methods (activate_taglist, deactivate_all) and log-pointer
+ *   swap methods (setup_diagslog, reseat_diagslog, should_roll_*,
+ *   set_std_output) acquire tag_table_lock internally. Rolling-policy
+ *   configuration methods (config_roll_diagslog, config_roll_outputlog) and
+ *   dump() do NOT acquire any lock; see per-method @thread-safety 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] prefix_string Tag prefix prepended to all debug output. Must
+   *   be non-empty.
    * @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
@@ -216,10 +221,11 @@ class Diags : public DebugInterface
    *   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().
+   * @pre prefix_string is non-empty; 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
+   * @errors Allocation failures abort via ink_abort. 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.
@@ -290,13 +296,16 @@ class Diags : public DebugInterface
    * @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.
+   * @param[in] test_ip Endpoint whose IP address is compared against the
+   *   configured debug client IP. The port is ignored.
+   * @return True if the IP address of @a test_ip equals the configured
+   *   debug client IP; false otherwise or if no debug client IP is set.
    * @pre None.
    * @post No state change.
    * @errors None.
-   * @thread-safety Safe to call concurrently; debug_client_ip is written
-   *   only during single-threaded reconfiguration.
+   * @thread-safety DEFECT: reads debug_client_ip, a plain (non-atomic)
+   *   struct, while the live config-update callback may write it on another
+   *   thread. Intended semantics: relaxed atomic read.
    */
   bool
   test_override_ip(IpEndpoint const &test_ip)
@@ -393,15 +402,15 @@ class Diags : public DebugInterface
   /**
    * @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.
-   * @param[in] level DiagsLevel for routing and terminal-level handling.
+   * @param[in] tag C string label included in output, or nullptr to omit the
+   *   tag prefix. Not checked against the tag regex.
+   * @param[in] level DiagsLevel for sink routing.
    * @param[in] loc Source location of the call site, or nullptr.
    * @param[in] fmt Non-null printf-format string.
    * @pre fmt is non-null and arguments match its conversions.
    * @post Message emitted to all sinks enabled for level in config.outputs.
-   *   For terminal levels (DiagsLevel_IsTerminal), the process exits — this
-   *   call does not return.
+   *   Does not handle terminal levels; the process does not exit.
+   *   Use error_va() if terminal-level exit behavior is required.
    * @errors I/O errors during emission are absorbed; output may be lost.
    * @thread-safety Safe to call concurrently from any thread.
    */
@@ -417,7 +426,7 @@ class Diags : public DebugInterface
   /**
    * @brief va_list form of print().
    *
-   * @param[in] tag Non-null tag label.
+   * @param[in] tag Tag label, or nullptr to omit the tag prefix.
    * @param[in] level DiagsLevel for routing.
    * @param[in] loc Source location, or nullptr.
    * @param[in] fmt Non-null printf-format string.
@@ -433,11 +442,12 @@ class Diags : public DebugInterface
   /**
    * @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] tag C string naming the debug tag, or nullptr (null matches
+   *   unconditionally; see tag_activated).
    * @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.
+   * @pre fmt is non-null; arguments match fmt's conversions.
    * @post If on(tag) is true, message is emitted identically to print().
    *   If on(tag) is false, no output is produced.
    * @errors Same as print().
@@ -458,16 +468,17 @@ class Diags : public DebugInterface
   /**
    * @brief va_list form of log().
    *
-   * @param[in] tag Non-null tag name.
+   * @param[in] tag Tag name, or nullptr (null matches unconditionally).
    * @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.
+   * @pre fmt is non-null; ap is initialized.
    * @post Same as log().
    * @errors Same as print().
-   * @thread-safety Same as log().
+   * @thread-safety Same as log(). The on() call reads _enabled without a
+   *   lock; see DiagsConfigState::enabled().
    */
   void
   log_va(const char *tag, DiagsLevel level, const SourceLocation *loc, const char *fmt, va_list ap)
@@ -521,8 +532,10 @@ class Diags : public DebugInterface
    * @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.
+   * @thread-safety Not thread-safe. Reads config fields (_enabled,
+   *   outputs, base_debug_tags, base_action_tags) without holding
+   *   tag_table_lock. Concurrent reconfiguration may produce
+   *   interleaved or inconsistent output.
    */
   void dump(FILE *fp = stdout) const;
 
@@ -578,8 +591,10 @@ class Diags : public DebugInterface
    * @errors Open failures cause a false return and an internal log_log_error
    *   diagnostic at LL_Error.
    * @thread-safety Caller MUST serialize with all other reconfiguration
-   *   methods. reseat_diagslog() acquires tag_table_lock around its call;
-   *   direct callers must provide equivalent serialization.
+   *   methods. reseat_diagslog() acquires tag_table_lock only for the
+   *   pointer swap that follows this call; setup_diagslog() itself is not
+   *   called under the lock. Direct callers must provide equivalent
+   *   serialization for the pointer swap.
    */
   bool setup_diagslog(BaseLogFile *blf);
 
@@ -591,10 +606,10 @@ class Diags : public DebugInterface
    * @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.
+   * @post Rolling policy fields are updated in the calling thread.
    * @errors None.
-   * @thread-safety Acquires tag_table_lock.
+   * @thread-safety No lock is acquired. The caller must ensure no concurrent
+   *   calls to should_roll_diagslog() occur during reconfiguration.
    */
   void config_roll_diagslog(RollingEnabledValues re, int ri, int rs);
 
@@ -605,9 +620,10 @@ class Diags : public DebugInterface
    * @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.
+   * @post Rolling policy fields are updated in the calling thread.
    * @errors None.
-   * @thread-safety Acquires tag_table_lock.
+   * @thread-safety No lock is acquired. The caller must ensure no concurrent
+   *   calls to should_roll_outputlog() occur during reconfiguration.
    */
   void config_roll_outputlog(RollingEnabledValues re, int ri, int rs);
 
@@ -625,11 +641,10 @@ class Diags : public DebugInterface
    *     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; actual failure is observable only via the
-   *   internal log_log_error message written to BaseLogFile's error channel.
+   * @return False if diags_log is null or not yet initialized (safe no-op).
+   *   True in all other cases, including when the internal reopen fails —
+   *   reopen failures are not reflected in the return value and are
+   *   observable only via the internal diagnostic trace log.
    * @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
@@ -638,9 +653,9 @@ class Diags : public DebugInterface
    *   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.
+   *   is deleted before it returns.
    * @errors Open failures are not directly signaled via the return value;
-   *   failure is observable only via an internal log_log_error message.
+   *   failure is reported to the internal diagnostic trace log only.
    * @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 *.
@@ -652,42 +667,55 @@ class Diags : public DebugInterface
   bool reseat_diagslog();
 
   /**
-   * @brief Test whether diags.log is due for a roll under its current policy.
+   * @brief Roll diags.log if the current rolling policy condition is met.
    *
-   * @return True if the configured rolling condition is met.
+   * @return True if the underlying file was renamed (rolled); false if no
+   *   roll condition was triggered, or if fstat failed. Note: true is
+   *   returned even when the subsequent reopen of the new log file fails.
    * @pre None.
-   * @post No state change; rolling is not performed.
-   * @errors None.
-   * @thread-safety Acquires tag_table_lock.
+   * @post If the rolling condition is met: the current diags.log is flushed,
+   *   rolled (renamed), and replaced by a new BaseLogFile at the same path.
+   *   If not: no state change. fstat failure causes an early false return
+   *   without rolling.
+   * @errors None signaled. fstat and reopen failures silently suppress the
+   *   replacement; the rolled state of the original file is not reversed.
+   * @thread-safety tag_table_lock is acquired only during the BaseLogFile
+   *   pointer swap. The rolling-condition checks and file operations execute
+   *   without a lock; the caller must ensure no concurrent reconfiguration
+   *   (see config_roll_diagslog()).
    */
   bool should_roll_diagslog();
 
   /**
-   * @brief Test whether the output log is due for a roll under its current
-   *   policy.
+   * @brief Roll stdout_log and stderr_log if the current rolling policy
+   *   condition is met.
    *
-   * @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.
+   * @return True if any output log was rolled; false otherwise.
+   * @pre stdout_log and stderr_log are non-null.
+   * @post If the rolling condition is met: affected logs are flushed, rolled,
+   *   and replaced by new BaseLogFile instances at the same paths.
+   *   If not: no state change. fstat failure causes an early false return.
+   * @errors None signaled. fstat failures silently suppress the roll.
+   * @thread-safety Same as should_roll_diagslog(); see config_roll_outputlog().
    */
   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, non-empty path string. Passed verbatim to the
-   *   OS open call; symlinks are re-resolved at each call. An empty string
-   *   causes an immediate false return without modifying state.
-   * @return True on success; false if the file could not be opened or the
-   *   resulting FILE * is null.
-   * @pre A Diags instance is active; file is non-null and non-empty.
+   * @param[in] file Non-null path string. Passed verbatim to the OS open
+   *   call; symlinks are re-resolved at each call. An empty string causes an
+   *   immediate false return without modifying any state.
+   * @return True on success; false if file is empty, the file could not be
+   *   opened, or the resulting FILE * is null.
+   * @pre A Diags instance is active; file is non-null (passing null is UB —
+   *   it is passed directly to strcmp without a null guard).
    * @post On success: the new file is open and bound as the named stream;
    *   the previous BaseLogFile is deleted.
-   *   On failure: the previous BaseLogFile is deleted and the stream pointer
-   *   is set to nullptr — the previous binding is NOT preserved.
+   *   On failure: the stream pointer is set to nullptr — the previous
+   *   binding is NOT preserved. The previous BaseLogFile is NOT freed
+   *   (leaked on the failure path).
    * @errors File-open failures cause a false return and internal
    *   log_log_error messages; the named stream is left unbound (nullptr).
    * @thread-safety Acquires tag_table_lock internally on both success and

include/tscore/LogMessage.h

@@ -167,16 +167,22 @@ class LogMessage : public Throttler
    * @brief Emit a tagged message at the given level using Diags::print,
    *   subject to throttling.
    *
+   * Unlike message(), this method delegates to Diags::print_va, which does
+   * not perform terminal-level exit. The process will not exit regardless of
+   * level; use message() if terminal-level exit behavior is required.
+   *
    * @param[in] tag Non-null tag string (included in output, not checked
    *   against the tag regex).
    * @param[in] level DiagsLevel for routing.
    * @param[in] loc Call-site source location.
    * @param[in] fmt Non-null printf-format string.
    * @pre tag and fmt are non-null.
-   * @post Same as message() except throttling uses the debug throttling
-   *   interval (_default_debug_throttling_interval) rather than the log
-   *   throttling interval (_default_log_throttling_interval).
-   * @errors None signaled.
+   * @post If not throttled: message emitted to all sinks enabled for level.
+   *   The process does not exit, even for terminal levels.
+   *   If throttled: message silently dropped.
+   *   Throttling uses the debug throttling interval
+   *   (_default_debug_throttling_interval).
+   * @errors None signaled; I/O errors absorbed.
    * @thread-safety Safe to call concurrently.
    */
   void print(const char *tag, DiagsLevel level, SourceLocation const &loc, const char *fmt, ...) TS_PRINTFLIKE(5, 6);

src/tscore/Diags.cc

@@ -703,12 +703,6 @@ Diags::should_roll_outputlog()
   return ret_val;
 }
 
-/*
- * Sets up a BaseLogFile for the specified file. Then it binds the specified standard steam
- * to the aforementioned BaseLogFile.
- *
- * Returns true on successful binding and setup, false otherwise
- */
 bool
 Diags::set_std_output(StdStream stream, const char *file)
 {