Preserve cached configuration when refresh fails

Build replacement appenders and thresholds before installing them and
advancing the local version. Release partial replacements and old caches
outside the configuration lock.  In order to make the `ensureFreshness`
version check as cheap as posible and only incur overhead when we
actually need to update the local cache, isolate the refresh logic in a
separate scope to prevent initialization and cleanup of temporary
variables when the version has not changed

Add regression coverage for repeated clone failures, cache preservation,
cleanup, and retry without another configuration change. Update
documentation to reflect the remaining freshness issue.

src/namespaced_logging.nim

@@ -285,15 +285,6 @@ proc shutdownThreadedConsoleLogging() =
 proc shutdownThreadedFileLogging() =
   shutdownWriter(fileLogging)
 
-
-proc ensureFreshness*(ls: var LogService) =
-  ## A shared handle has no cached configuration. Retained for callers that
-  ## previously refreshed LogService values; only local services need refresh.
-  if ls.shared == nil:
-    raise newException(ValueError,
-      "LogService not properly initialized. Use initLogService")
-
-
 proc ensureFreshness*(ls: ThreadLocalLogService) =
   ## Check our thread-local version of the logging service configuration
   ## against the global log service definition. This uses an atomic version
@@ -304,22 +295,41 @@ proc ensureFreshness*(ls: ThreadLocalLogService) =
       "LogService not properly initialized. Use initLogService to obtain a" &
       "properly initialized LogService object")
 
-  # Keep a stable borrowed pointer while inspecting configuration. In
-  # particular, do not iterate a seq through a function-returned expression:
-  # that can create an owning temporary and register its shared appender refs
-  # with this thread's ORC cycle collector even while the lock is held.
+  # ORC's cycle collector maintains some thread-local bookkeeping state.
+  # Because of this, we have to be careful here while making a copy of the
+  # shared logging configuration state to avoid behavior that would result in
+  # temporary variables with ownership semantics (specifically the shared
+  # appender references in the seq).
+  #
+  # We will keep a stable borrowed pointer while inspecting configuration and
+  # will create temporary variables to own the data being copied that can be
+  # cleaned up in this proc's scope.
   let state = ls.serviceHandle.shared
   if ls.configVersion == state.configVersion.load(): return
 
-  withLock state.lock:
-    ls.configVersion = state.configVersion.load
+  block:
+    # This block is a performance optimization. By putting these variables and
+    # operations in their own scope they won't get initialized or cleaned up
+    # unless ls.configVersion != state.configVersion.load().
 
-    ls.appenders = @[]
-    for a in state.appenders: ls.appenders.add(clone(a))
+    # We want the following vars to be in the outer scope (not in the lock) so
+    # that their cleanup happens after the lock is released.
+    var newAppenders: seq[LogAppender]
+    var newThresholds: TableRef[string, Level]
 
-    ls.thresholds = newTable[string, Level]()
-    for ns, t in pairs(state.thresholds): ls.thresholds[ns] = t
+    withLock state.lock:
+      let version = state.configVersion.load()
 
+      for a in state.appenders: newAppenders.add(clone(a))
+      newThresholds = newTable[string, Level]()
+      for ns, t in pairs(state.thresholds): newThresholds[ns] = t
+
+      # Record the version update only after every copy succeeds; a failure
+      # leaves the old version in place so the next freshness check retries the
+      # refresh.
+      swap(ls.appenders, newAppenders)
+      swap(ls.thresholds, newThresholds)
+      ls.configVersion = version
 
 proc initSharedLogServiceState(
     rootLevel = lvlAll,