From e46bfe88c1c74b7dc43e61275c3f4ac37b2f7295 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Sat, 13 Jun 2026 10:58:31 -0500 Subject: [PATCH 01/12] Document DiagsTypes.hh routines --- include/tscore/DiagsTypes.h | 581 +++++++++++++++++++++++++++++++++--- src/tscore/Diags.cc | 17 -- 2 files changed, 536 insertions(+), 62 deletions(-) diff --git a/include/tscore/DiagsTypes.h b/include/tscore/DiagsTypes.h index 1b3713160b5..e2bdf0cba07 100644 --- a/include/tscore/DiagsTypes.h +++ b/include/tscore/DiagsTypes.h @@ -45,14 +45,36 @@ #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; @@ -60,96 +82,276 @@ struct DiagsModeOutput { 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 the exit path for the terminal + * level: DL_Fatal and DL_Alert invoke ink_fatal_va; DL_Emergency invokes + * ink_emergency_va (a distinct, stronger exit path). + * @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). + * + * @note DEFECT: The backing store _enabled[2] is a plain int, not + * std::atomic. Concurrent reads from emission threads while a + * reconfiguration thread writes is a C++ data race (undefined behavior). + * The intended semantics are relaxed-atomic load/store; until the type + * is fixed, callers should treat reads as best-effort. + */ 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 DEFECT: _enabled[2] is a plain int. Concurrent reads + * while a write is in progress are a C++ data race. Intended semantics: + * relaxed atomic load. See DiagsConfigState class contract. + */ 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. 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. + */ 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). 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. 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 + * 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 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_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. + */ 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 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 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) { 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 DEFECT: reads DiagsConfigState::_enabled, a plain int, + * without a lock. Intended semantics: relaxed atomic load. See + * DiagsConfigState class 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 C string naming the tag to check, or nullptr. + * @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. A null tag matches unconditionally (delegates + * to tag_activated, which returns true for nullptr). + * @pre None (null tag is safe). + * @post No state change. + * @errors None. + * @thread-safety The tag_activated sub-call is safe: tag-table reads are + * serialized by tag_table_lock. The on(mode) sub-call reads _enabled + * without a lock; see DiagsConfigState::enabled(). + */ bool on(const char *tag, DiagsTagType mode = DiagsTagType_Debug) const { @@ -160,10 +362,33 @@ 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 C string to match, or nullptr. + * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action. + * @return True if tag matches the compiled regex for mode. If tag is + * nullptr, returns true unconditionally (null tag matches everything). + * @pre A Diags instance is active. + * @post No state change. + * @errors None. + * @thread-safety Safe to call concurrently. Reads are serialized by + * the internal tag_table_lock. + */ bool tag_activated(const char *tag, DiagsTagType mode = DiagsTagType_Debug) const; + /** + * @brief DebugInterface override: test whether a debug tag is active. + * + * @param[in] tag C string naming the debug tag, or nullptr. + * @return True if tag is active in DiagsTagType_Debug mode. A null tag + * returns true unconditionally (see tag_activated). + * @pre None (null tag is safe). + * @post No state change. + * @errors None. + * @thread-safety Same as tag_activated. + */ bool debug_tag_activated(const char *tag) const override { @@ -174,12 +399,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 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. + * 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. + */ void print(const char *tag, DiagsLevel level, const SourceLocation *loc, const char *fmt, ...) const TS_PRINTFLIKE(5, 6) { @@ -189,9 +423,37 @@ class Diags : public DebugInterface va_end(ap); } + /** + * @brief va_list form of print(). + * + * @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. + * @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 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 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(). + * @thread-safety Same as print(). The on() call reads _enabled without a + * lock; see DiagsConfigState::enabled(). + */ void log(const char *tag, DiagsLevel level, const SourceLocation *loc, const char *fmt, ...) const TS_PRINTFLIKE(5, 6) { @@ -203,6 +465,21 @@ class Diags : public DebugInterface } } + /** + * @brief va_list form of log(). + * + * @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 fmt is non-null; ap is initialized. + * @post Same as log(). + * @errors Same as print(). + * @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) { @@ -211,6 +488,18 @@ class Diags : public DebugInterface } } + /** + * @brief Emit a message at the given level unconditionally. + * + * @param[in] level DiagsLevel for routing and terminal handling. + * @param[in] loc Source location, or nullptr. + * @param[in] fmt Non-null printf-format string. + * @pre fmt is non-null; arguments match its conversions. + * @post Message emitted to all enabled sinks. For terminal levels the + * process exits — does not return. + * @errors Same as print(). + * @thread-safety Safe to call concurrently from any thread. + */ void error(DiagsLevel level, const SourceLocation *loc, const char *fmt, ...) const TS_PRINTFLIKE(4, 5) { @@ -220,26 +509,228 @@ 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 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; + /** + * @brief Enable tags matching the given PCRE2 pattern for the given mode. + * + * @param[in] taglist PCRE2 regex string, or nullptr. If nullptr, this call + * is a no-op and the previous pattern (if any) is unchanged. + * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action. + * @pre taglist is null or a valid PCRE2 pattern string. The regex engine is + * PCRE2, not POSIX ERE; PCRE2-specific syntax (lookaheads, named groups, + * etc.) is accepted. + * @post If taglist is non-null: the new pattern unconditionally replaces the + * previous one. If the pattern fails to compile, the new (empty) Regex + * object still replaces the previous one — no tags will match until a + * valid pattern is installed. For DiagsTagType_Debug, if this is the + * process-global Diags instance, all registered DbgCtl objects are + * updated immediately to reflect the new pattern via DbgCtl::update. + * @errors Invalid regex is silently accepted; compile failure is not + * signaled. The previous pattern is NOT retained on compile failure. + * @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 Open the given BaseLogFile for use as a diagnostic log destination. + * + * Does NOT assign diags_log; the caller is responsible for that assignment + * on success. On failure, blf is deleted before returning. + * + * @param[in] blf Pointer to a constructed but not-yet-opened BaseLogFile, + * or nullptr (null is a no-op that returns true). When non-null, ownership + * transfers unconditionally: on success the caller must assign blf to + * diags_log; on failure blf has been deleted. + * @return True if blf was opened successfully (or blf is nullptr); false if + * the open failed (blf is deleted before returning false). + * @pre blf is null or points to a BaseLogFile that has not yet been opened. + * @post On true return: blf (if non-null) is open; caller must assign it to + * diags_log. On false return: blf has been deleted; diags_log is unchanged. + * @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 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); + + /** + * @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 fields are updated in the calling thread. + * @errors None. + * @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); + + /** + * @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 fields are updated in the calling thread. + * @errors None. + * @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); + + /** + * @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 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 + * 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 before it returns. + * @errors Open failures are not directly signaled via the return value; + * 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 *. + * @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 Roll diags.log if the current rolling policy 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 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 Roll stdout_log and stderr_log if the current rolling policy + * condition is met. + * + * @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 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 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 + * failure paths. + */ bool set_std_output(StdStream stream, const char *file); - const char *base_debug_tags; // internal copy of default debug tags - const char *base_action_tags; // internal copy of default action tags + /// Initial debug tag regex string; copied at construction. May be nullptr. + const char *base_debug_tags; + + /// Initial action tag regex string; copied at construction. May be nullptr. + const char *base_action_tags; + /// Optional IP address for per-connection debug override. When set, + /// on(mode) returns true for connections whose remote IP matches this. IpAddr debug_client_ip; private: diff --git a/src/tscore/Diags.cc b/src/tscore/Diags.cc index 6fe5b79cf8b..d003d2978e6 100644 --- a/src/tscore/Diags.cc +++ b/src/tscore/Diags.cc @@ -350,17 +350,6 @@ Diags::tag_activated(const char *tag, DiagsTagType mode) const return regex ? regex->exec(tag, RE_ANCHORED) : false; } -////////////////////////////////////////////////////////////////////////////// -// -// void Diags::activate_taglist(char * taglist, DiagsTagType mode) -// -// This routine adds all tags in the vertical-bar-separated taglist -// to the tag table of type . Each addition is done under a lock. -// If an individual tag is already set, that tag is ignored. If -// is nullptr, this routine exits immediately. -// -////////////////////////////////////////////////////////////////////////////// - void Diags::activate_taglist(const char *taglist, DiagsTagType mode) { @@ -714,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) { From e7a6727873b5e81c80b2a8e648fb3ec3e52bb2fa Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Sat, 13 Jun 2026 11:15:06 -0500 Subject: [PATCH 02/12] Address Copilot review --- include/tscore/DiagsTypes.h | 42 ++++++++++++++++++++----------------- src/tscore/Diags.cc | 23 ++++++++------------ 2 files changed, 32 insertions(+), 33 deletions(-) diff --git a/include/tscore/DiagsTypes.h b/include/tscore/DiagsTypes.h index e2bdf0cba07..b4f759387a6 100644 --- a/include/tscore/DiagsTypes.h +++ b/include/tscore/DiagsTypes.h @@ -199,11 +199,13 @@ class DiagsConfigState * * @thread-safety After construction, the instance is safe to use concurrently * 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. + * mutation methods (activate_taglist, deactivate_all) acquire tag_table_lock + * internally. Log-pointer swap methods (reseat_diagslog, should_roll_*, + * set_std_output) acquire tag_table_lock only for their pointer swap step; + * file operations run outside the lock. setup_diagslog() acquires no lock at + * all; callers must serialize it. 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 { @@ -223,10 +225,12 @@ class Diags : public DebugInterface * files, or -1 for the system default. * @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. + * @post magic == DIAGS_MAGIC; tag regex strings are stored; activated tag + * tables are empty until activate_taglist() is called (typically via + * reconfigure_diags()); diags_log is set and open if _diags_log is + * non-null and openable, otherwise diags_log is null. * @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. + * reported via log_log_error and result in diags_log being null. * @thread-safety Single-threaded construction expected. After construction * the instance may be installed via DiagsPtr::set and used concurrently. */ @@ -343,14 +347,14 @@ class Diags : public DebugInterface * @param[in] tag C string naming the tag to check, or nullptr. * @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. A null tag matches unconditionally (delegates - * to tag_activated, which returns true for nullptr). + * the configured tag regex. A null tag matches unconditionally. * @pre None (null tag is safe). * @post No state change. * @errors None. - * @thread-safety The tag_activated sub-call is safe: tag-table reads are - * serialized by tag_table_lock. The on(mode) sub-call reads _enabled - * without a lock; see DiagsConfigState::enabled(). + * @thread-safety The tag_activated sub-call is safe: the active regex + * pointer is snapshotted under tag_table_lock, with regex execution + * outside the lock. The on(mode) sub-call reads _enabled without a lock; + * see DiagsConfigState::enabled(). */ bool on(const char *tag, DiagsTagType mode = DiagsTagType_Debug) const @@ -369,12 +373,12 @@ class Diags : public DebugInterface * @param[in] tag C string to match, or nullptr. * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action. * @return True if tag matches the compiled regex for mode. If tag is - * nullptr, returns true unconditionally (null tag matches everything). - * @pre A Diags instance is active. + * nullptr, returns true unconditionally. + * @pre None (null tag is safe). * @post No state change. * @errors None. - * @thread-safety Safe to call concurrently. Reads are serialized by - * the internal tag_table_lock. + * @thread-safety Safe to call concurrently. The active regex pointer is + * snapshotted under tag_table_lock; regex execution runs outside the lock. */ bool tag_activated(const char *tag, DiagsTagType mode = DiagsTagType_Debug) const; @@ -383,7 +387,7 @@ class Diags : public DebugInterface * * @param[in] tag C string naming the debug tag, or nullptr. * @return True if tag is active in DiagsTagType_Debug mode. A null tag - * returns true unconditionally (see tag_activated). + * returns true unconditionally. * @pre None (null tag is safe). * @post No state change. * @errors None. @@ -443,7 +447,7 @@ class Diags : public DebugInterface * @brief Emit a message only if the tag is active in DiagsTagType_Debug. * * @param[in] tag C string naming the debug tag, or nullptr (null matches - * unconditionally; see tag_activated). + * unconditionally). * @param[in] level DiagsLevel for routing. * @param[in] loc Source location, or nullptr. * @param[in] fmt Non-null printf-format string. diff --git a/src/tscore/Diags.cc b/src/tscore/Diags.cc index d003d2978e6..ad2e8540faa 100644 --- a/src/tscore/Diags.cc +++ b/src/tscore/Diags.cc @@ -88,19 +88,13 @@ tell_diags_regression_testing_is_on() ////////////////////////////////////////////////////////////////////////////// // -// Diags::Diags(char *bdt, char *bat) +// Diags::Diags(prefix_string, bdt, bat, _diags_log, dl_perm, ol_perm) // -// This is the constructor for the Diags class. The constructor takes -// two strings called the "base debug tags" (bdt) and the -// "base action tags" (bat). These represent debug/action overrides, -// to override the records.yaml values. They current come from -// command-line options. -// -// If bdt is not nullptr, and not "", it overrides records.yaml settings. -// If bat is not nullptr, and not "", it overrides records.yaml settings. -// -// When the constructor is done, records.yaml callbacks will be set, -// the initial values read, and the Diags instance will be ready to use. +// Constructor for the Diags class. bdt and bat are the "base debug +// tags" and "base action tags" — command-line overrides for the +// records.yaml tag settings. Non-null, non-empty values are stored +// for later use by activate_taglist(); activated tag tables are left +// empty until an external reconfigure call populates them. // ////////////////////////////////////////////////////////////////////////////// @@ -320,8 +314,9 @@ Diags::print_va(const char *debug_tag, DiagsLevel diags_level, const SourceLocat // // This routine inquires if a particular in the tag table of // type is activated, returning true if it is, false if it -// isn't. If is nullptr, true is returned. The call uses a lock -// to get atomic access to the tag tables. +// isn't. If is nullptr, true is returned. The active regex +// pointer is snapshotted under tag_table_lock; regex execution runs +// outside the lock. // ////////////////////////////////////////////////////////////////////////////// From f773791cdb27f3155474b423ed31802d0ae54f57 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Sat, 13 Jun 2026 11:46:18 -0500 Subject: [PATCH 03/12] Address Copilot comments --- include/tscore/DiagsTypes.h | 61 ++++++++++++++++++++----------------- src/tscore/Diags.cc | 6 ++++ 2 files changed, 39 insertions(+), 28 deletions(-) diff --git a/include/tscore/DiagsTypes.h b/include/tscore/DiagsTypes.h index b4f759387a6..987ff4e044c 100644 --- a/include/tscore/DiagsTypes.h +++ b/include/tscore/DiagsTypes.h @@ -85,8 +85,8 @@ struct DiagsModeOutput { /** * @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. + * STDOUT targets the standard output stream; STDERR targets the standard + * error stream. */ enum StdStream { STDOUT = 0, STDERR }; @@ -135,16 +135,17 @@ using DiagsCleanupFunc = void (*)(); * 1 — enabled for all callers, * 2 — enabled only when ContFlags::DEBUG_OVERRIDE is set on the * current Continuation (per-connection debug override). - * - * @note DEFECT: The backing store _enabled[2] is a plain int, not - * std::atomic. Concurrent reads from emission threads while a - * reconfiguration thread writes is a C++ data race (undefined behavior). - * The intended semantics are relaxed-atomic load/store; until the type - * is fixed, callers should treat reads as best-effort. */ +// DEFECT: The backing store _enabled[2] is a plain int, not std::atomic. +// Concurrent reads from emission threads while a reconfiguration thread writes +// is a C++ data race (undefined behavior). The intended semantics are +// relaxed-atomic load/store; until the type is fixed, callers should treat +// reads as best-effort. class DiagsConfigState { public: + // DEFECT: _enabled[2] is a plain int; concurrent reads while a write is in + // progress are a C++ data race. See the DEFECT comment above the class. /** * @brief Return the current enable state for the given tag type. * @@ -153,9 +154,7 @@ class DiagsConfigState * @pre None. * @post No state change. * @errors None. - * @thread-safety DEFECT: _enabled[2] is a plain int. Concurrent reads - * while a write is in progress are a C++ data race. Intended semantics: - * relaxed atomic load. See DiagsConfigState class contract. + * @thread-safety Not thread-safe; see class-level DEFECT note. */ static int enabled(DiagsTagType dtt) @@ -168,11 +167,12 @@ 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. 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. + * @pre new_value is in [0, 2]; values outside this range produce + * unspecified behavior. + * @post enabled(dtt) returns new_value on all subsequent calls. When + * dtt == DiagsTagType_Debug and the value changes, 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. @@ -307,10 +307,12 @@ class Diags : public DebugInterface * @pre None. * @post No state change. * @errors None. - * @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. + * @thread-safety Not thread-safe: debug_client_ip is a plain (non-atomic) + * struct that may be written by the config-update callback on another + * thread. */ + // DEFECT: debug_client_ip is read here without synchronization while the + // config-update callback (update_debug_client_ip) may write it concurrently. bool test_override_ip(IpEndpoint const &test_ip) { @@ -553,11 +555,11 @@ class Diags : public DebugInterface * PCRE2, not POSIX ERE; PCRE2-specific syntax (lookaheads, named groups, * etc.) is accepted. * @post If taglist is non-null: the new pattern unconditionally replaces the - * previous one. If the pattern fails to compile, the new (empty) Regex - * object still replaces the previous one — no tags will match until a - * valid pattern is installed. For DiagsTagType_Debug, if this is the - * process-global Diags instance, all registered DbgCtl objects are - * updated immediately to reflect the new pattern via DbgCtl::update. + * previous one. If the pattern fails to compile, the previous pattern is + * cleared and no tags will match until a valid pattern is installed. + * For DiagsTagType_Debug, if this is the process-global Diags instance, + * all registered debug controls are updated immediately to reflect the + * new pattern. * @errors Invalid regex is silently accepted; compile failure is not * signaled. The previous pattern is NOT retained on compile failure. * @thread-safety Acquires tag_table_lock. Safe to call concurrently with @@ -717,9 +719,10 @@ class Diags : public DebugInterface * 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 stream pointer is set to nullptr — the previous - * binding is NOT preserved. The previous BaseLogFile is NOT freed - * (leaked on the failure path). + * On file-open failure: the stream pointer is set to nullptr — the + * previous binding is NOT preserved. The previous BaseLogFile is NOT + * freed (leaked on this path). + * On empty-string return: no state is modified. * @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 @@ -734,7 +737,9 @@ class Diags : public DebugInterface const char *base_action_tags; /// Optional IP address for per-connection debug override. When set, - /// on(mode) returns true for connections whose remote IP matches this. + /// connections whose remote IP matches this address have + /// ContFlags::DEBUG_OVERRIDE set, which causes on(mode) to return true + /// only when the global debug state is in DEBUG_OVERRIDE mode (value 2). IpAddr debug_client_ip; private: diff --git a/src/tscore/Diags.cc b/src/tscore/Diags.cc index ad2e8540faa..a3bbbf4757b 100644 --- a/src/tscore/Diags.cc +++ b/src/tscore/Diags.cc @@ -57,6 +57,12 @@ using namespace swoc::literals; void DiagsConfigState::enabled(DiagsTagType dtt, int new_value) { + // NOTE: proxy.config.diags.debug.enabled is validated as [0-3] in + // RecordsConfig.cc, so new_value may be 3 even though the API contract + // documents [0, 2]. Value 3 is not range-checked here; it reaches + // _enabled[dtt] and DbgCtl::_config_mode unchanged. In Diags::on(), + // the test (config.enabled(mode) & 1) makes value 3 behave identically + // to value 1 (always-enabled). if (_enabled[dtt] == new_value) { return; } From b4363bbb8b7698dc3e312c8794382df09b11ad04 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Sat, 13 Jun 2026 12:02:35 -0500 Subject: [PATCH 04/12] Address Copilot review --- include/tscore/DiagsTypes.h | 219 ++++++++++++++++++++++-------------- 1 file changed, 136 insertions(+), 83 deletions(-) diff --git a/include/tscore/DiagsTypes.h b/include/tscore/DiagsTypes.h index 987ff4e044c..1e9b65308ca 100644 --- a/include/tscore/DiagsTypes.h +++ b/include/tscore/DiagsTypes.h @@ -71,7 +71,8 @@ enum DiagsTagType { DiagsTagType_Debug = 0, DiagsTagType_Action = 1 }; * 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 + * @par Thread safety + * Carries no internal synchronization. Callers reading or * modifying Diags::config.outputs[] must respect the concurrency contract * of the containing DiagsConfigState. */ @@ -121,7 +122,8 @@ enum RollingEnabledValues { NO_ROLLING = 0, ROLL_ON_TIME, ROLL_ON_SIZE, ROLL_ON_ * @post On return, the process continues into the exit path for the terminal * level: DL_Fatal and DL_Alert invoke ink_fatal_va; DL_Emergency invokes * ink_emergency_va (a distinct, stronger exit path). - * @thread-safety Only one callback is installed per Diags instance. + * @par Thread safety + * Only one callback is installed per Diags instance. * The callback is invoked at most once per process. */ using DiagsCleanupFunc = void (*)(); @@ -134,27 +136,25 @@ using DiagsCleanupFunc = void (*)(); * 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). + * current Continuation (per-connection debug override), + * 3 — treated identically to 1 (always-enabled). */ -// DEFECT: The backing store _enabled[2] is a plain int, not std::atomic. -// Concurrent reads from emission threads while a reconfiguration thread writes -// is a C++ data race (undefined behavior). The intended semantics are -// relaxed-atomic load/store; until the type is fixed, callers should treat -// reads as best-effort. class DiagsConfigState { public: - // DEFECT: _enabled[2] is a plain int; concurrent reads while a write is in - // progress are a C++ data race. See the DEFECT comment above the class. /** * @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). + * @return 0 (disabled), 1 (enabled), 2 (DEBUG_OVERRIDE mode), or 3 + * (always-enabled, same effect as 1). * @pre None. * @post No state change. - * @errors None. - * @thread-safety Not thread-safe; see class-level DEFECT note. + * @par Errors + * None. + * @par Thread safety + * Not thread-safe. Reads _enabled without synchronization; + * concurrent writes from a reconfiguration thread are a data race. */ static int enabled(DiagsTagType dtt) @@ -166,15 +166,18 @@ class DiagsConfigState * @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]; values outside this range produce + * @param[in] new_value 0, 1, 2, or 3 (see class contract); 3 behaves + * identically to 1. + * @pre new_value is in [0, 3]; values outside this range produce * unspecified behavior. * @post enabled(dtt) returns new_value on all subsequent calls. When * dtt == DiagsTagType_Debug and the value changes, 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 + * @par Errors + * None. + * @par Thread safety + * Must be called with external serialization against * concurrent enabled(dtt) reads. */ static void enabled(DiagsTagType dtt, int new_value); @@ -197,15 +200,19 @@ class DiagsConfigState * 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). Tag-table - * mutation methods (activate_taglist, deactivate_all) acquire tag_table_lock - * internally. Log-pointer swap methods (reseat_diagslog, should_roll_*, - * set_std_output) acquire tag_table_lock only for their pointer swap step; - * file operations run outside the lock. setup_diagslog() acquires no lock at - * all; callers must serialize it. Rolling-policy configuration methods - * (config_roll_diagslog, config_roll_outputlog) and dump() do NOT acquire any - * lock; see per-method @thread-safety for details. + * @par Thread safety + * Concurrent emission calls (print, log, error) are serialized + * against each other via tag_table_lock. Reconfiguration writes to + * config.outputs (e.g., via DiagsConfig::reconfigure_diags) do not acquire + * tag_table_lock, so concurrent emission and reconfiguration are a data race + * on config.outputs. Tag-table mutation methods (activate_taglist, + * deactivate_all) acquire tag_table_lock internally. Log-pointer swap methods + * (reseat_diagslog, should_roll_*, set_std_output) acquire tag_table_lock + * only for their pointer swap step; file operations run outside the lock. + * setup_diagslog() acquires no lock; callers must serialize it. + * Rolling-policy configuration methods (config_roll_diagslog, + * config_roll_outputlog) and dump() do not acquire any lock; see the + * per-method @par Thread safety paragraphs below for details. */ class Diags : public DebugInterface { @@ -229,9 +236,11 @@ class Diags : public DebugInterface * tables are empty until activate_taglist() is called (typically via * reconfigure_diags()); diags_log is set and open if _diags_log is * non-null and openable, otherwise diags_log is null. - * @errors Allocation failures abort via ink_abort. Log-open failures are + * @par Errors + * Allocation failures abort via ink_abort. Log-open failures are * reported via log_log_error and result in diags_log being null. - * @thread-safety Single-threaded construction expected. After construction + * @par 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, @@ -242,8 +251,10 @@ class Diags : public DebugInterface * * @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 + * @par Errors + * None. + * @par Thread safety + * Single-threaded destruction. The caller must ensure no * other thread holds or will acquire a reference to this instance. */ virtual ~Diags(); @@ -286,8 +297,10 @@ class Diags : public DebugInterface * 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 + * @par Errors + * None. + * @par Thread safety + * Safe to call from any thread; reads a thread-local * Continuation flag. */ bool @@ -306,13 +319,13 @@ class Diags : public DebugInterface * debug client IP; false otherwise or if no debug client IP is set. * @pre None. * @post No state change. - * @errors None. - * @thread-safety Not thread-safe: debug_client_ip is a plain (non-atomic) - * struct that may be written by the config-update callback on another - * thread. + * @par Errors + * None. + * @par Thread safety + * Not thread-safe. Reads debug_client_ip without + * synchronization; concurrent writes from the config-update callback are + * a data race. */ - // DEFECT: debug_client_ip is read here without synchronization while the - // config-update callback (update_debug_client_ip) may write it concurrently. bool test_override_ip(IpEndpoint const &test_ip) { @@ -324,15 +337,17 @@ class Diags : public DebugInterface * * @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. + * @return True if the tag type is unconditionally enabled (value 1 or 3), + * 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 DEFECT: reads DiagsConfigState::_enabled, a plain int, - * without a lock. Intended semantics: relaxed atomic load. See - * DiagsConfigState class contract. + * @par Errors + * None. + * @par Thread safety + * Not thread-safe. Reads DiagsConfigState::_enabled without + * synchronization; concurrent writes from a reconfiguration thread are a + * data race. * @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. @@ -352,8 +367,10 @@ class Diags : public DebugInterface * the configured tag regex. A null tag matches unconditionally. * @pre None (null tag is safe). * @post No state change. - * @errors None. - * @thread-safety The tag_activated sub-call is safe: the active regex + * @par Errors + * None. + * @par Thread safety + * The tag_activated sub-call is safe: the active regex * pointer is snapshotted under tag_table_lock, with regex execution * outside the lock. The on(mode) sub-call reads _enabled without a lock; * see DiagsConfigState::enabled(). @@ -378,8 +395,10 @@ class Diags : public DebugInterface * nullptr, returns true unconditionally. * @pre None (null tag is safe). * @post No state change. - * @errors None. - * @thread-safety Safe to call concurrently. The active regex pointer is + * @par Errors + * None. + * @par Thread safety + * Safe to call concurrently. The active regex pointer is * snapshotted under tag_table_lock; regex execution runs outside the lock. */ bool tag_activated(const char *tag, DiagsTagType mode = DiagsTagType_Debug) const; @@ -392,8 +411,10 @@ class Diags : public DebugInterface * returns true unconditionally. * @pre None (null tag is safe). * @post No state change. - * @errors None. - * @thread-safety Same as tag_activated. + * @par Errors + * None. + * @par Thread safety + * Same as tag_activated. */ bool debug_tag_activated(const char *tag) const override @@ -417,8 +438,10 @@ class Diags : public DebugInterface * @post Message emitted to all sinks enabled for level in config.outputs. * 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. + * @par Errors + * I/O errors during emission are absorbed; output may be lost. + * @par Thread safety + * Safe to call concurrently from any thread. */ void print(const char *tag, DiagsLevel level, const SourceLocation *loc, const char *fmt, ...) const TS_PRINTFLIKE(5, 6) @@ -440,8 +463,10 @@ class Diags : public DebugInterface * 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(). + * @par Errors + * Same as print(). + * @par Thread safety + * Same as print(). */ void print_va(const char *tag, DiagsLevel level, const SourceLocation *loc, const char *fmt, va_list ap) const override; @@ -456,8 +481,10 @@ class Diags : public DebugInterface * @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(). - * @thread-safety Same as print(). The on() call reads _enabled without a + * @par Errors + * Same as print(). + * @par Thread safety + * Same as print(). The on() call reads _enabled without a * lock; see DiagsConfigState::enabled(). */ void @@ -482,8 +509,10 @@ class Diags : public DebugInterface * without va_end + va_start. * @pre fmt is non-null; ap is initialized. * @post Same as log(). - * @errors Same as print(). - * @thread-safety Same as log(). The on() call reads _enabled without a + * @par Errors + * Same as print(). + * @par Thread safety + * Same as log(). The on() call reads _enabled without a * lock; see DiagsConfigState::enabled(). */ void @@ -503,8 +532,10 @@ class Diags : public DebugInterface * @pre fmt is non-null; arguments match its conversions. * @post Message emitted to all enabled sinks. For terminal levels the * process exits — does not return. - * @errors Same as print(). - * @thread-safety Safe to call concurrently from any thread. + * @par Errors + * Same as print(). + * @par Thread safety + * Safe to call concurrently from any thread. */ void error(DiagsLevel level, const SourceLocation *loc, const char *fmt, ...) const TS_PRINTFLIKE(4, 5) @@ -526,8 +557,10 @@ class Diags : public DebugInterface * @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. + * @par Errors + * None signaled; I/O errors absorbed. + * @par Thread safety + * Safe to call concurrently. */ virtual void error_va(DiagsLevel level, const SourceLocation *loc, const char *fmt, va_list ap) const; @@ -537,8 +570,10 @@ class Diags : public DebugInterface * @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 Not thread-safe. Reads config fields (_enabled, + * @par Errors + * I/O errors on fp are not signaled. + * @par 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. @@ -560,9 +595,11 @@ class Diags : public DebugInterface * For DiagsTagType_Debug, if this is the process-global Diags instance, * all registered debug controls are updated immediately to reflect the * new pattern. - * @errors Invalid regex is silently accepted; compile failure is not + * @par Errors + * Invalid regex is silently accepted; compile failure is not * signaled. The previous pattern is NOT retained on compile failure. - * @thread-safety Acquires tag_table_lock. Safe to call concurrently with + * @par 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); @@ -573,8 +610,10 @@ class Diags : public DebugInterface * @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 + * @par Errors + * None. + * @par 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); @@ -594,9 +633,11 @@ class Diags : public DebugInterface * @pre blf is null or points to a BaseLogFile that has not yet been opened. * @post On true return: blf (if non-null) is open; caller must assign it to * diags_log. On false return: blf has been deleted; diags_log is unchanged. - * @errors Open failures cause a false return and an internal log_log_error + * @par 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 + * @par Thread safety + * Caller MUST serialize with all other reconfiguration * 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 @@ -613,8 +654,10 @@ class Diags : public DebugInterface * policies). * @pre None. * @post Rolling policy fields are updated in the calling thread. - * @errors None. - * @thread-safety No lock is acquired. The caller must ensure no concurrent + * @par Errors + * None. + * @par 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); @@ -627,8 +670,10 @@ class Diags : public DebugInterface * @param[in] rs Rolling size threshold in bytes. * @pre None. * @post Rolling policy fields are updated in the calling thread. - * @errors None. - * @thread-safety No lock is acquired. The caller must ensure no concurrent + * @par Errors + * None. + * @par 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); @@ -660,9 +705,11 @@ class Diags : public DebugInterface * subsequent emission goes to the new file. * On failure: diags_log is unchanged; the newly allocated BaseLogFile * is deleted before it returns. - * @errors Open failures are not directly signaled via the return value; + * @par Errors + * Open failures are not directly signaled via the return value; * failure is reported to the internal diagnostic trace log only. - * @thread-safety The swap in step 4 is performed under tag_table_lock. + * @par 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 @@ -683,9 +730,11 @@ class Diags : public DebugInterface * 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 + * @par 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 + * @par 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()). @@ -701,8 +750,10 @@ class Diags : public DebugInterface * @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(). + * @par Errors + * None signaled. fstat failures silently suppress the roll. + * @par Thread safety + * Same as should_roll_diagslog(); see config_roll_outputlog(). */ bool should_roll_outputlog(); @@ -723,9 +774,11 @@ class Diags : public DebugInterface * previous binding is NOT preserved. The previous BaseLogFile is NOT * freed (leaked on this path). * On empty-string return: no state is modified. - * @errors File-open failures cause a false return and internal + * @par 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 + * @par Thread safety + * Acquires tag_table_lock internally on both success and * failure paths. */ bool set_std_output(StdStream stream, const char *file); From ce6945840597a4c227a0943d334fe040f3c30504 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Sat, 13 Jun 2026 12:15:34 -0500 Subject: [PATCH 05/12] Address Copilot review --- include/tscore/DiagsTypes.h | 58 ++++++++++++++++++++++--------------- src/tscore/Diags.cc | 11 ++++--- 2 files changed, 39 insertions(+), 30 deletions(-) diff --git a/include/tscore/DiagsTypes.h b/include/tscore/DiagsTypes.h index 1e9b65308ca..fd90e1a92f7 100644 --- a/include/tscore/DiagsTypes.h +++ b/include/tscore/DiagsTypes.h @@ -59,8 +59,8 @@ * 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. + * @note Numeric values are used as array indices into the enable-state + * array and Diags::activated_tags. Do not renumber. */ enum DiagsTagType { DiagsTagType_Debug = 0, DiagsTagType_Action = 1 }; @@ -129,15 +129,24 @@ enum RollingEnabledValues { NO_ROLLING = 0, ROLL_ON_TIME, ROLL_ON_SIZE, ROLL_ON_ using DiagsCleanupFunc = void (*)(); /** - * @brief Process-global enable state for the two tag tables. + * @brief Bundles the two orthogonal pieces of Diags output configuration: + * the process-global per-tag enable state and the per-level output routing. * - * Exposes two static methods to read and write the per-tag enable state. - * The state for each DiagsTagType is one of: + * The enable 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), * 3 — treated identically to 1 (always-enabled). + * + * The output routing is stored in @c outputs[], one entry per DiagsLevel. + * + * @par Thread safety + * The enable state is process-global (static storage). Reads via + * enabled() and writes via enabled(dtt, value) carry no internal + * synchronization; callers must provide external serialization when a + * write may race a read. The @c outputs[] array carries no internal + * synchronization either; the same serialization requirement applies. */ class DiagsConfigState { @@ -153,7 +162,7 @@ class DiagsConfigState * @par Errors * None. * @par Thread safety - * Not thread-safe. Reads _enabled without synchronization; + * Not thread-safe. The read carries no synchronization; * concurrent writes from a reconfiguration thread are a data race. */ static int @@ -171,9 +180,9 @@ class DiagsConfigState * @pre new_value is in [0, 3]; values outside this range produce * unspecified behavior. * @post enabled(dtt) returns new_value on all subsequent calls. When - * dtt == DiagsTagType_Debug and the value changes, DbgCtl::_config_mode - * is also updated via a relaxed atomic store so that DbgCtl-based checks - * remain in sync without acquiring a lock. + * dtt == DiagsTagType_Debug and the value changes, the DbgCtl fast-path + * enable state is updated via a relaxed atomic store so that DbgCtl-based + * checks remain in sync without acquiring a lock. * @par Errors * None. * @par Thread safety @@ -201,11 +210,13 @@ class DiagsConfigState * via DiagsPtr. * * @par Thread safety - * Concurrent emission calls (print, log, error) are serialized - * against each other via tag_table_lock. Reconfiguration writes to - * config.outputs (e.g., via DiagsConfig::reconfigure_diags) do not acquire - * tag_table_lock, so concurrent emission and reconfiguration are a data race - * on config.outputs. Tag-table mutation methods (activate_taglist, + * Concurrent emission calls (print, log, error) hold tag_table_lock for + * diagslog, stdout, and stderr output. On non-FreeBSD platforms the lock is + * released before the syslog() call, so concurrent syslog output is not + * serialized against other emission calls. Reconfiguration writes to + * config.outputs do not acquire tag_table_lock, so concurrent emission + * and reconfiguration are a data race on config.outputs. Tag-table + * mutation methods (activate_taglist, * deactivate_all) acquire tag_table_lock internally. Log-pointer swap methods * (reseat_diagslog, should_roll_*, set_std_output) acquire tag_table_lock * only for their pointer swap step; file operations run outside the lock. @@ -345,9 +356,8 @@ class Diags : public DebugInterface * @par Errors * None. * @par Thread safety - * Not thread-safe. Reads DiagsConfigState::_enabled without - * synchronization; concurrent writes from a reconfiguration thread are a - * data race. + * Not thread-safe. The enable-state read carries no synchronization; + * concurrent writes from a reconfiguration thread are a data race. * @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. @@ -372,8 +382,8 @@ class Diags : public DebugInterface * @par Thread safety * The tag_activated sub-call is safe: the active regex * pointer is snapshotted under tag_table_lock, with regex execution - * outside the lock. The on(mode) sub-call reads _enabled without a lock; - * see DiagsConfigState::enabled(). + * outside the lock. The on(mode) sub-call carries no synchronization on + * the enable-state read; see DiagsConfigState::enabled(). */ bool on(const char *tag, DiagsTagType mode = DiagsTagType_Debug) const @@ -484,8 +494,8 @@ class Diags : public DebugInterface * @par Errors * Same as print(). * @par Thread safety - * Same as print(). The on() call reads _enabled without a - * lock; see DiagsConfigState::enabled(). + * Same as print(). The on() call carries no synchronization on + * the enable-state read; see DiagsConfigState::enabled(). */ void log(const char *tag, DiagsLevel level, const SourceLocation *loc, const char *fmt, ...) const TS_PRINTFLIKE(5, 6) @@ -512,8 +522,8 @@ class Diags : public DebugInterface * @par Errors * Same as print(). * @par Thread safety - * Same as log(). The on() call reads _enabled without a - * lock; see DiagsConfigState::enabled(). + * Same as log(). The on() call carries no synchronization on + * the enable-state read; see DiagsConfigState::enabled(). */ void log_va(const char *tag, DiagsLevel level, const SourceLocation *loc, const char *fmt, va_list ap) @@ -573,7 +583,7 @@ class Diags : public DebugInterface * @par Errors * I/O errors on fp are not signaled. * @par Thread safety - * Not thread-safe. Reads config fields (_enabled, + * Not thread-safe. Reads config fields (enable state, * outputs, base_debug_tags, base_action_tags) without holding * tag_table_lock. Concurrent reconfiguration may produce * interleaved or inconsistent output. diff --git a/src/tscore/Diags.cc b/src/tscore/Diags.cc index a3bbbf4757b..ca266f30c3b 100644 --- a/src/tscore/Diags.cc +++ b/src/tscore/Diags.cc @@ -57,12 +57,11 @@ using namespace swoc::literals; void DiagsConfigState::enabled(DiagsTagType dtt, int new_value) { - // NOTE: proxy.config.diags.debug.enabled is validated as [0-3] in - // RecordsConfig.cc, so new_value may be 3 even though the API contract - // documents [0, 2]. Value 3 is not range-checked here; it reaches - // _enabled[dtt] and DbgCtl::_config_mode unchanged. In Diags::on(), - // the test (config.enabled(mode) & 1) makes value 3 behave identically - // to value 1 (always-enabled). + // proxy.config.diags.debug.enabled is validated as [0-3] in RecordsConfig.cc. + // Value 3 is not range-checked here; it reaches _enabled[dtt] and + // DbgCtl::_config_mode unchanged. In Diags::on(), the test + // (config.enabled(mode) & 1) makes value 3 behave identically to value 1 + // (always-enabled). if (_enabled[dtt] == new_value) { return; } From 06fba7fdb52a3fbc3ebb9c23a607b8cb23f622fb Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Sat, 13 Jun 2026 12:20:08 -0500 Subject: [PATCH 06/12] Remove range comment in Diags.cc --- src/tscore/Diags.cc | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/tscore/Diags.cc b/src/tscore/Diags.cc index ca266f30c3b..ad2e8540faa 100644 --- a/src/tscore/Diags.cc +++ b/src/tscore/Diags.cc @@ -57,11 +57,6 @@ using namespace swoc::literals; void DiagsConfigState::enabled(DiagsTagType dtt, int new_value) { - // proxy.config.diags.debug.enabled is validated as [0-3] in RecordsConfig.cc. - // Value 3 is not range-checked here; it reaches _enabled[dtt] and - // DbgCtl::_config_mode unchanged. In Diags::on(), the test - // (config.enabled(mode) & 1) makes value 3 behave identically to value 1 - // (always-enabled). if (_enabled[dtt] == new_value) { return; } From 98009b09cebb798dfe503490c37526e88cc8f771 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Sat, 13 Jun 2026 12:46:26 -0500 Subject: [PATCH 07/12] Address Copilot review --- include/tscore/DiagsTypes.h | 47 ++++++++++++++++++++----------------- src/tscore/Diags.cc | 18 +++----------- 2 files changed, 29 insertions(+), 36 deletions(-) diff --git a/include/tscore/DiagsTypes.h b/include/tscore/DiagsTypes.h index fd90e1a92f7..636a9e4ca2b 100644 --- a/include/tscore/DiagsTypes.h +++ b/include/tscore/DiagsTypes.h @@ -248,8 +248,9 @@ class Diags : public DebugInterface * reconfigure_diags()); diags_log is set and open if _diags_log is * non-null and openable, otherwise diags_log is null. * @par Errors - * Allocation failures abort via ink_abort. Log-open failures are - * reported via log_log_error and result in diags_log being null. + * Allocation failures abort via ink_abort. Log-open failures result in + * diags_log being null and are reported via log_log_error only when + * BASELOGFILE_DEBUG_MODE is enabled (off by default). * @par Thread safety * Single-threaded construction expected. After construction * the instance may be installed via DiagsPtr::set and used concurrently. @@ -446,6 +447,8 @@ class Diags : public DebugInterface * @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. + * stderr is also written when regression_testing_on is true, even if + * config.outputs[level].to_stderr is false. * Does not handle terminal levels; the process does not exit. * Use error_va() if terminal-level exit behavior is required. * @par Errors @@ -644,8 +647,9 @@ class Diags : public DebugInterface * @post On true return: blf (if non-null) is open; caller must assign it to * diags_log. On false return: blf has been deleted; diags_log is unchanged. * @par Errors - * Open failures cause a false return and an internal log_log_error - * diagnostic at LL_Error. + * Open failures cause a false return. A log_log_error diagnostic at + * LL_Error is emitted only when BASELOGFILE_DEBUG_MODE is enabled + * (off by default). * @par Thread safety * Caller MUST serialize with all other reconfiguration * methods. reseat_diagslog() acquires tag_table_lock only for the @@ -705,7 +709,8 @@ class Diags : public DebugInterface * @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. + * observable only via log_log_trace, which is compiled out unless + * BASELOGFILE_DEBUG_MODE is enabled (off by default). * @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 @@ -716,16 +721,16 @@ class Diags : public DebugInterface * On failure: diags_log is unchanged; the newly allocated BaseLogFile * is deleted before it returns. * @par Errors - * Open failures are not directly signaled via the return value; - * failure is reported to the internal diagnostic trace log only. + * Open failures are not signaled via the return value. They are reported + * via log_log_trace only when BASELOGFILE_DEBUG_MODE is enabled (off + * by default); in normal builds the failure is completely silent. * @par 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. + * initialization guard causes an early false return. */ bool reseat_diagslog(); @@ -771,25 +776,25 @@ class Diags : public DebugInterface * @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 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. + * @param[in] file Non-null filesystem path. 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). + * @pre file must not be null. * @post On success: the new file is open and bound as the named stream; * the previous BaseLogFile is deleted. - * On file-open failure: the stream pointer is set to nullptr — the - * previous binding is NOT preserved. The previous BaseLogFile is NOT - * freed (leaked on this path). + * On file-open failure: the stream pointer is set to nullptr and the + * previous BaseLogFile is not freed. * On empty-string return: no state is modified. * @par Errors - * File-open failures cause a false return and internal - * log_log_error messages; the named stream is left unbound (nullptr). + * File-open failures cause a false return; the named stream is left + * unbound (nullptr). Internal log_log_error messages are emitted only + * when BASELOGFILE_DEBUG_MODE is enabled (off by default). * @par Thread safety - * Acquires tag_table_lock internally on both success and - * failure paths. + * Acquires tag_table_lock for the pointer update on success and on + * file-open failure. The empty-string early return does not acquire + * the lock. */ bool set_std_output(StdStream stream, const char *file); diff --git a/src/tscore/Diags.cc b/src/tscore/Diags.cc index ad2e8540faa..e15d2667f34 100644 --- a/src/tscore/Diags.cc +++ b/src/tscore/Diags.cc @@ -217,6 +217,7 @@ Diags::~Diags() // void print(...) // void log_va(...) // void log(...) +// void error_va(...) // ////////////////////////////////////////////////////////////////////////////// @@ -467,19 +468,6 @@ Diags::config_roll_outputlog(RollingEnabledValues re, int ri, int rs) outputlog_rolling_size = rs; } -/* - * Update diags_log to use the underlying file on disk. - * - * This function will replace the current BaseLogFile object with a new one, as - * each BaseLogFile object logically represents one file on disk. It can be - * used when we want to re-open the log file if the initial one was moved. - * - * Note that, however, cross process race conditions may still exist, - * especially with the metafile, and further work with flock() for fcntl() may - * still need to be done. - * - * Returns true if the log was reseated, false otherwise. - */ bool Diags::reseat_diagslog() { @@ -728,7 +716,7 @@ Diags::set_std_output(StdStream stream, const char *file) log_log_error("[Warning]: %s is currently not bound to anything\n", target_stream); delete new_log; lock(); - *current = nullptr; + *current = nullptr; // old_log is not freed here unlock(); return false; } @@ -737,7 +725,7 @@ Diags::set_std_output(StdStream stream, const char *file) log_log_error("[Warning]: %s is currently not bound to anything\n", target_stream); delete new_log; lock(); - *current = nullptr; + *current = nullptr; // old_log is not freed here unlock(); return false; } From e307237cc04d9df053b0f5685c91556fd813a731 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Sat, 13 Jun 2026 13:11:12 -0500 Subject: [PATCH 08/12] Address Copilot review --- include/tscore/DiagsTypes.h | 92 +++++++++++++++++++++++-------------- src/tscore/Diags.cc | 6 --- 2 files changed, 57 insertions(+), 41 deletions(-) diff --git a/include/tscore/DiagsTypes.h b/include/tscore/DiagsTypes.h index 636a9e4ca2b..ce82e34a7ac 100644 --- a/include/tscore/DiagsTypes.h +++ b/include/tscore/DiagsTypes.h @@ -596,21 +596,21 @@ class Diags : public DebugInterface /** * @brief Enable tags matching the given PCRE2 pattern for the given mode. * - * @param[in] taglist PCRE2 regex string, or nullptr. If nullptr, this call - * is a no-op and the previous pattern (if any) is unchanged. + * @param[in] taglist PCRE2 regex string, or nullptr. If nullptr, the stored + * pattern is unchanged. * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action. * @pre taglist is null or a valid PCRE2 pattern string. The regex engine is - * PCRE2, not POSIX ERE; PCRE2-specific syntax (lookaheads, named groups, - * etc.) is accepted. - * @post If taglist is non-null: the new pattern unconditionally replaces the - * previous one. If the pattern fails to compile, the previous pattern is - * cleared and no tags will match until a valid pattern is installed. + * PCRE2, not POSIX ERE. + * @post If taglist is non-null, the new pattern unconditionally replaces the + * previous one; if the pattern fails to compile, the stored pattern becomes + * empty and no tags will match until a valid pattern is installed. * For DiagsTagType_Debug, if this is the process-global Diags instance, - * all registered debug controls are updated immediately to reflect the - * new pattern. + * DbgCtl::update() is called to resync all registered debug controls + * against the current pattern — this occurs regardless of whether taglist + * is null. * @par Errors - * Invalid regex is silently accepted; compile failure is not - * signaled. The previous pattern is NOT retained on compile failure. + * Compile failures are not signaled to the caller; the stored pattern + * becomes empty rather than retaining the previous one. * @par Thread safety * Acquires tag_table_lock. Safe to call concurrently with * emission, but serialized with other reconfiguration methods. @@ -708,9 +708,7 @@ class Diags : public DebugInterface * * @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 log_log_trace, which is compiled out unless - * BASELOGFILE_DEBUG_MODE is enabled (off by default). + * reopen failures are not reflected in the return value. * @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 @@ -722,8 +720,8 @@ class Diags : public DebugInterface * is deleted before it returns. * @par Errors * Open failures are not signaled via the return value. They are reported - * via log_log_trace only when BASELOGFILE_DEBUG_MODE is enabled (off - * by default); in normal builds the failure is completely silent. + * at LL_Error only when BASELOGFILE_DEBUG_MODE is enabled (off by + * default); in normal builds the failure is completely silent. * @par Thread safety * The swap in step 4 is performed under tag_table_lock. * Concurrent emission either observes the pre-swap or post-swap log; @@ -737,17 +735,24 @@ class Diags : public DebugInterface /** * @brief Roll diags.log if the current rolling policy 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 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. + * Under a size-based policy, the current file's size is compared to the + * configured threshold. Under a time-based policy, the elapsed time since + * the last roll is compared to the configured interval. Combined policies + * evaluate both conditions independently. When a condition is met the + * current file is flushed, renamed, and a fresh file is opened at the same + * path. + * + * @return True if the underlying file was renamed; false otherwise. True is + * returned even when reopening the fresh file at the original path fails. + * @pre None. A null or uninitialized diags_log is a safe no-op that + * returns false. + * @post On a successful roll the file at the configured path is a fresh, + * empty file and emission is directed to it. If renaming succeeded but + * reopening did not, the configured path is left absent and emission + * continues to the renamed file. With no roll, all state is unchanged. * @par Errors - * None signaled. fstat and reopen failures silently suppress the - * replacement; the rolled state of the original file is not reversed. + * None signaled. fstat (size-based policy only) and reopen failures + * silently suppress replacement; a completed rename is never reversed. * @par Thread safety * tag_table_lock is acquired only during the BaseLogFile * pointer swap. The rolling-condition checks and file operations execute @@ -757,18 +762,35 @@ class Diags : public DebugInterface bool should_roll_diagslog(); /** - * @brief Roll stdout_log and stderr_log if the current rolling policy - * condition is met. + * @brief Roll the standard-output redirection file if the current rolling + * policy condition is met. + * + * Under a size-based policy, the current file's size is compared to the + * configured threshold. Under a time-based policy, the elapsed time since + * the last roll is compared to the configured interval. Combined policies + * evaluate both conditions independently. When a condition is met both + * stdout_log and stderr_log are flushed, the underlying file is renamed, + * and a fresh file is opened at the same path; the process's stdout (and + * stderr, when redirected to the same path) are rebound to the new file. + * The time-based path also advances the last-roll timestamp. * - * @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. + * @return True if the underlying file was renamed; false otherwise. + * @pre stdout_log and stderr_log are non-null; violation aborts. + * When a roll occurs, stdout_log and stderr_log must refer to the same + * filesystem path; violation aborts. + * @post On a successful roll the file at the configured path is a fresh, + * empty file and the process's stdout and stderr (when sharing the path) + * are bound to it. With no roll, all state is unchanged. An + * uninitialized stdout_log returns false without inspecting policy. * @par Errors - * None signaled. fstat failures silently suppress the roll. + * None signaled. fstat (size-based policy only) and roll failures silently + * suppress replacement. An fstat failure short-circuits the function and + * skips any time-based check. * @par Thread safety - * Same as should_roll_diagslog(); see config_roll_outputlog(). + * tag_table_lock is acquired only during the BaseLogFile pointer swap and + * the dup2() rebinding of the standard stream. The rolling-condition + * checks and file operations execute without a lock; the caller must + * ensure no concurrent reconfiguration (see config_roll_outputlog()). */ bool should_roll_outputlog(); diff --git a/src/tscore/Diags.cc b/src/tscore/Diags.cc index e15d2667f34..5ae11392c16 100644 --- a/src/tscore/Diags.cc +++ b/src/tscore/Diags.cc @@ -432,12 +432,6 @@ Diags::error_va(DiagsLevel level, const SourceLocation *loc, const char *format_ } } -/* - * Sets up and error handles the given BaseLogFile object to work - * with this instance of Diags. - * - * Returns true on success, false otherwise - */ bool Diags::setup_diagslog(BaseLogFile *blf) { From 9cea2642b14bc086ce1247181a0fc921ba1ab41e Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Sat, 13 Jun 2026 13:19:57 -0500 Subject: [PATCH 09/12] Address Copilot review --- include/tscore/DiagsTypes.h | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/include/tscore/DiagsTypes.h b/include/tscore/DiagsTypes.h index ce82e34a7ac..87327b6ae49 100644 --- a/include/tscore/DiagsTypes.h +++ b/include/tscore/DiagsTypes.h @@ -599,8 +599,9 @@ class Diags : public DebugInterface * @param[in] taglist PCRE2 regex string, or nullptr. If nullptr, the stored * pattern is unchanged. * @param[in] mode DiagsTagType_Debug or DiagsTagType_Action. - * @pre taglist is null or a valid PCRE2 pattern string. The regex engine is - * PCRE2, not POSIX ERE. + * @pre None (null taglist and invalid PCRE2 patterns are both safe; see + * @post for the invalid-pattern case). The regex engine is PCRE2, not + * POSIX ERE. * @post If taglist is non-null, the new pattern unconditionally replaces the * previous one; if the pattern fails to compile, the stored pattern becomes * empty and no tags will match until a valid pattern is installed. @@ -662,14 +663,17 @@ class Diags : public DebugInterface /** * @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). + * @param[in] re Rolling policy (see RollingEnabledValues). NO_ROLLING + * disables rolling. + * @param[in] ri Rolling interval in seconds, consulted under time-based + * policies. A value of -1 disables the time-based threshold. + * @param[in] rs Rolling size threshold in megabytes, consulted under + * size-based policies. A value of -1 disables the size-based threshold. * @pre None. - * @post Rolling policy fields are updated in the calling thread. + * @post The three rolling policy fields are overwritten with the supplied + * values. No other state is modified. * @par Errors - * None. + * None. Inputs are stored verbatim without validation. * @par Thread safety * No lock is acquired. The caller must ensure no concurrent * calls to should_roll_diagslog() occur during reconfiguration. @@ -679,13 +683,17 @@ class Diags : public DebugInterface /** * @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. + * @param[in] re Rolling policy (see RollingEnabledValues). NO_ROLLING + * disables rolling. + * @param[in] ri Rolling interval in seconds, consulted under time-based + * policies. A value of -1 disables the time-based threshold. + * @param[in] rs Rolling size threshold in megabytes, consulted under + * size-based policies. A value of -1 disables the size-based threshold. * @pre None. - * @post Rolling policy fields are updated in the calling thread. + * @post The three rolling policy fields are overwritten with the supplied + * values. No other state is modified. * @par Errors - * None. + * None. Inputs are stored verbatim without validation. * @par Thread safety * No lock is acquired. The caller must ensure no concurrent * calls to should_roll_outputlog() occur during reconfiguration. From 3974095a5d7570ccbc57a8bbd547ceb7a1d5a196 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Sat, 13 Jun 2026 13:33:31 -0500 Subject: [PATCH 10/12] Address Copilot review --- include/tscore/DiagsTypes.h | 36 ++++++++++++++++++++---------------- src/tscore/Diags.cc | 3 +++ 2 files changed, 23 insertions(+), 16 deletions(-) diff --git a/include/tscore/DiagsTypes.h b/include/tscore/DiagsTypes.h index 87327b6ae49..d98bcf5c1de 100644 --- a/include/tscore/DiagsTypes.h +++ b/include/tscore/DiagsTypes.h @@ -114,17 +114,21 @@ enum RollingEnabledValues { NO_ROLLING = 0, ROLL_ON_TIME, ROLL_ON_SIZE, ROLL_ON_ /** * @brief Cleanup callback invoked before process termination on a terminal - * DiagsLevel (Fatal, Alert, Emergency). + * DiagsLevel (DL_Fatal, DL_Alert, DL_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 the exit path for the terminal - * level: DL_Fatal and DL_Alert invoke ink_fatal_va; DL_Emergency invokes - * ink_emergency_va (a distinct, stronger exit path). + * @pre Runs synchronously on the thread that emitted the terminal message, + * before the process exits. MUST NOT throw. MUST be async-signal-safe if + * installed in a process that may emit terminal-level messages from a + * signal handler. MUST tolerate being invoked more than once per process + * (for example, from concurrent terminal emissions on different threads, + * or from a terminal emission inside the callback itself). + * @post On return, the process is terminated; control does not return to + * the emitter. DL_Emergency exits via the unrecoverable path; DL_Fatal + * and DL_Alert exit via the recoverable path. * @par Thread safety - * Only one callback is installed per Diags instance. - * The callback is invoked at most once per process. + * No serialization is provided. The callback may run concurrently on + * multiple threads if multiple terminal emissions race; the callback + * itself is responsible for any required synchronization or idempotency. */ using DiagsCleanupFunc = void (*)(); @@ -543,12 +547,11 @@ class Diags : public DebugInterface * @param[in] loc Source location, or nullptr. * @param[in] fmt Non-null printf-format string. * @pre fmt is non-null; arguments match its conversions. - * @post Message emitted to all enabled sinks. For terminal levels the - * process exits — does not return. + * @post Same as error_va(). * @par Errors - * Same as print(). + * Same as error_va(). * @par Thread safety - * Safe to call concurrently from any thread. + * Same as error_va(). */ void error(DiagsLevel level, const SourceLocation *loc, const char *fmt, ...) const TS_PRINTFLIKE(4, 5) @@ -568,12 +571,13 @@ class Diags : public DebugInterface * @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. + * @post Message emitted to all sinks enabled for level. For terminal + * levels (DL_Fatal and above), invokes cleanup_func (if set) and then + * terminates the process; does not return. * @par Errors * None signaled; I/O errors absorbed. * @par Thread safety - * Safe to call concurrently. + * Safe to call concurrently from any thread. */ virtual void error_va(DiagsLevel level, const SourceLocation *loc, const char *fmt, va_list ap) const; diff --git a/src/tscore/Diags.cc b/src/tscore/Diags.cc index 5ae11392c16..030b057d3a9 100644 --- a/src/tscore/Diags.cc +++ b/src/tscore/Diags.cc @@ -418,6 +418,9 @@ Diags::error_va(DiagsLevel level, const SourceLocation *loc, const char *format_ va_list ap2; va_copy(ap2, ap); + // No re-entry guard: if two threads emit a terminal-level message + // concurrently, or if cleanup_func itself emits one, cleanup_func may + // be invoked more than once. The callback must tolerate this. if (cleanup_func) { cleanup_func(); } From 3088f788b9e5b9d400011ab71c1cd871cb714187 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Sat, 13 Jun 2026 13:54:26 -0500 Subject: [PATCH 11/12] Address Copilot review --- include/tscore/DiagsTypes.h | 53 ++++++++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 16 deletions(-) diff --git a/include/tscore/DiagsTypes.h b/include/tscore/DiagsTypes.h index d98bcf5c1de..ad5977e3c7d 100644 --- a/include/tscore/DiagsTypes.h +++ b/include/tscore/DiagsTypes.h @@ -222,9 +222,11 @@ class DiagsConfigState * and reconfiguration are a data race on config.outputs. Tag-table * mutation methods (activate_taglist, * deactivate_all) acquire tag_table_lock internally. Log-pointer swap methods - * (reseat_diagslog, should_roll_*, set_std_output) acquire tag_table_lock - * only for their pointer swap step; file operations run outside the lock. - * setup_diagslog() acquires no lock; callers must serialize it. + * (reseat_diagslog, should_roll_diagslog) acquire tag_table_lock only for + * their pointer swap step; file operations run outside the lock. + * set_std_output (and should_roll_outputlog through it) additionally holds + * the lock across the dup2() rebinding of the standard stream descriptor on + * success. setup_diagslog() acquires no lock; callers must serialize it. * Rolling-policy configuration methods (config_roll_diagslog, * config_roll_outputlog) and dump() do not acquire any lock; see the * per-method @par Thread safety paragraphs below for details. @@ -450,15 +452,23 @@ class Diags : public DebugInterface * @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. + * @pre level is in [0, DiagsLevel_Count); out-of-range values terminate the + * process via release assertion. * @post Message emitted to all sinks enabled for level in config.outputs. - * stderr is also written when regression_testing_on is true, even if + * When regression-testing mode has been enabled (see + * tell_diags_regression_testing_is_on()), stderr is also written even if * config.outputs[level].to_stderr is false. * Does not handle terminal levels; the process does not exit. * Use error_va() if terminal-level exit behavior is required. * @par Errors * I/O errors during emission are absorbed; output may be lost. * @par Thread safety - * Safe to call concurrently from any thread. + * Safe to call concurrently with other emission calls (print, log, error); + * tag_table_lock serializes the diagslog, stdout, and stderr writes. The + * read of config.outputs[level] is not synchronized against + * reconfigure_diags(), which writes config without acquiring the lock; + * concurrent reconfiguration is a data race on config.outputs (see the + * class-level Thread safety paragraph). */ void print(const char *tag, DiagsLevel level, const SourceLocation *loc, const char *fmt, ...) const TS_PRINTFLIKE(5, 6) @@ -479,6 +489,8 @@ class Diags : public DebugInterface * @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. + * @pre level is in [0, DiagsLevel_Count); out-of-range values terminate the + * process via release assertion. * @post Same as print(). * @par Errors * Same as print(). @@ -786,18 +798,26 @@ class Diags : public DebugInterface * stderr, when redirected to the same path) are rebound to the new file. * The time-based path also advances the last-roll timestamp. * - * @return True if the underlying file was renamed; false otherwise. - * @pre stdout_log and stderr_log are non-null; violation aborts. - * When a roll occurs, stdout_log and stderr_log must refer to the same - * filesystem path; violation aborts. + * @return True if the underlying file was renamed; false otherwise. True is + * returned even when reopening the fresh file at the original path fails. + * @pre stdout_log and stderr_log are non-null. When a roll occurs, + * stdout_log and stderr_log must refer to the same filesystem path. Both + * conditions are checked with ink_assert(): violations abort in debug + * builds and are undefined behavior (typically a null-pointer crash) in + * release builds. * @post On a successful roll the file at the configured path is a fresh, * empty file and the process's stdout and stderr (when sharing the path) - * are bound to it. With no roll, all state is unchanged. An + * are bound to it. If renaming succeeded but reopening did not, stdout_log + * (and stderr_log when sharing the path) are set to null and the configured + * path is left absent; the standard-stream descriptors retain their prior + * binding to the renamed file. With no roll, all state is unchanged. An * uninitialized stdout_log returns false without inspecting policy. * @par Errors - * None signaled. fstat (size-based policy only) and roll failures silently - * suppress replacement. An fstat failure short-circuits the function and - * skips any time-based check. + * None signaled via the return value. fstat (size-based policy only), + * rename, and reopen failures are reported at LL_Error only when + * BASELOGFILE_DEBUG_MODE is enabled (off by default); in normal builds + * they are silent. An fstat failure short-circuits the function and skips + * any time-based check. * @par Thread safety * tag_table_lock is acquired only during the BaseLogFile pointer swap and * the dup2() rebinding of the standard stream. The rolling-condition @@ -826,9 +846,10 @@ class Diags : public DebugInterface * unbound (nullptr). Internal log_log_error messages are emitted only * when BASELOGFILE_DEBUG_MODE is enabled (off by default). * @par Thread safety - * Acquires tag_table_lock for the pointer update on success and on - * file-open failure. The empty-string early return does not acquire - * the lock. + * On success, tag_table_lock is held across both the pointer update and + * the dup2() rebinding of the standard-stream file descriptor; on + * file-open failure it is held only for the pointer update. The + * empty-string early return does not acquire the lock. */ bool set_std_output(StdStream stream, const char *file); From 7f63160654c7de4167bc5fe349847f53e99f2db7 Mon Sep 17 00:00:00 2001 From: Josiah VanderZee Date: Sat, 13 Jun 2026 14:05:47 -0500 Subject: [PATCH 12/12] Address Copilot review --- include/tscore/DiagsTypes.h | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/include/tscore/DiagsTypes.h b/include/tscore/DiagsTypes.h index ad5977e3c7d..5f5b8ab9da3 100644 --- a/include/tscore/DiagsTypes.h +++ b/include/tscore/DiagsTypes.h @@ -117,11 +117,14 @@ enum RollingEnabledValues { NO_ROLLING = 0, ROLL_ON_TIME, ROLL_ON_SIZE, ROLL_ON_ * DiagsLevel (DL_Fatal, DL_Alert, DL_Emergency). * * @pre Runs synchronously on the thread that emitted the terminal message, - * before the process exits. MUST NOT throw. MUST be async-signal-safe if - * installed in a process that may emit terminal-level messages from a - * signal handler. MUST tolerate being invoked more than once per process - * (for example, from concurrent terminal emissions on different threads, - * or from a terminal emission inside the callback itself). + * before the process exits. MUST NOT throw. MUST tolerate being invoked + * more than once per process (for example, from concurrent terminal + * emissions on different threads, or from a terminal emission inside the + * callback itself). + * @note Terminal-level emission is not async-signal-safe, so the callback + * MUST NOT be reached from a signal handler. Code paths that may emit at + * terminal level from signal context are bugs in the emitter, not a + * constraint this callback is expected to satisfy. * @post On return, the process is terminated; control does not return to * the emitter. DL_Emergency exits via the unrecoverable path; DL_Fatal * and DL_Alert exit via the recoverable path.