fix: Address review of the check_logfile bookmark option

Four things the review of #1406 turned up, all in the new bookmark path.

An explicitly empty value (`bookmark=`) silently disabled bookmarking
even though the documentation says an empty value means `auto`. The REST
API renders a valueless parameter as a bare token so it never hit this,
but the client-query path passes `k=v` through verbatim and a check
written that way quietly went back to reporting the whole file on every
run. The option now uses a notifier instead of binding a variable, which
is what tells "given without a value" apart from "not given at all", and
maps the empty value to `auto`.

Positions moved as each file was read, so a check which failed on a
later file - a second `file=` which cannot be opened - returned an error
having already consumed the lines of the files before it, and nothing
ever reported them. Positions are now collected and applied only once
every file has been read.

Nothing bounded the stored state. A bookmark name comes from the caller
and an automatic one changes with the filter, so a host which generates
names grew nsclient.db without end. Positions are now held in a bounded
LRU map (1000 file/bookmark pairs); a position which ages out is dropped
from the live set and blanked in the store on the next shutdown, and its
file is simply read in full the next time that name appears. The
automatic name also carries a hash of the filter, warning and critical
expressions rather than the expressions themselves, so a filter of
arbitrary length and content no longer ends up in the persisted key.

Finally the documentation now covers what a bookmark costs: a line is
consumed when the check runs rather than when its result is submitted,
positions are saved on a clean shutdown so a crash re-reports the
backlog, a file whose last line has no terminator never reports that
line until something appends to it, and `${total}` counts the lines
examined rather than the lines in the file.

Assisted-by: Claude Code:claude-opus-5
Signed-off-by: Michael Medin <michael@medin.name>

Author: mickem

docs/samples/CheckLogFile_check_logfile_desc.md

@@ -13,8 +13,9 @@ The name (`app-errors` above) identifies the stored position. Two checks that
 use the same name over the same file share one position — the first one to run
 consumes the lines. Use distinct names when two checks look for different
 things in the same file, or let the name be derived automatically by passing
-`bookmark` with no value (or `bookmark=auto`), which builds it from the file
-name plus the `filter`, `warning` and `critical` expressions:
+`bookmark` with no value (`bookmark`, `bookmark=` and `bookmark=auto` all mean
+the same thing), which builds it from the file name plus a hash of the
+`filter`, `warning` and `critical` expressions:
 
 ```
 check_logfile file=/var/log/app.log "filter=column1 like 'ERROR'" "warning=count > 0" bookmark
@@ -25,7 +26,7 @@ tracked on its own. They are written to `${data-path}/nsclient.db` when
 NSClient++ shuts down and restored on start, so a restart does not re-report
 everything.
 
-Because an automatic name embeds the expressions, editing the filter starts a
+Because an automatic name covers the expressions, editing the filter starts a
 new position (and the file is read in full once more) and leaves the old one
 behind in the store. Prefer an explicit name for checks whose filter changes
 often, or which are generated with varying arguments.
@@ -46,13 +47,46 @@ Behaviour worth knowing:
   under a fixed name), the file is read from the beginning again. Rotation to a
   *different* name is not followed: point the check at the name that keeps
   receiving new lines.
+* **A failing check moves nothing.** If any of the files cannot be read the
+  check returns an error and every position stays where it was, so the lines
+  which were read on the way are reported by the next successful check instead
+  of vanishing with the error.
 * **A quiet check is OK, not UNKNOWN.** When nothing new arrived the result is
   the empty state (`%(status): Nothing found`), which is OK by default; use
   `empty-state=` to change it.
 * **Checks without `bookmark` are unaffected.** They neither read nor advance
   any stored position, so an ad-hoc full scan can be run at any time without
   disturbing a bookmarked check.
 
+Before you switch a check over to `bookmark`, know what you are trading a
+re-reported line for:
+
+* **A line is consumed when the check runs, not when its result arrives.** With
+  a bookmark the position moves as soon as the lines have been read. If the
+  result is submitted passively (NSCA, NRDP, …) and that submission fails, the
+  lines it described are not reported again by the next check. An actively
+  polled check is not exposed to this: the position moves as the poller
+  receives the answer.
+* **Positions are saved when NSClient++ shuts down.** A crash, a killed
+  service or a power loss therefore rewinds every bookmark to where it was when
+  the service last stopped cleanly, and the lines written since are reported
+  again. They are never lost, only repeated.
+* **A last line without its terminator is never reported on its own.** This is
+  the flip side of holding back half-written lines: a file which is written in
+  one go and does not end with `line-split` keeps its final line unreported
+  until something appends to the file. Without a bookmark that line is reported
+  on every check as before.
+* **`${total}` counts what the check looked at.** With a bookmark that is the
+  lines added since the previous run — not the number of lines in the file.
+  `${count}` is, as always, how many of them matched the filter.
+* **The number of remembered positions is capped** (at 1000 file/bookmark
+  pairs, which no ordinary configuration comes close to). If a host generates
+  bookmark names — a script which puts a timestamp in the name, or automatic
+  names for a filter which keeps changing — the least recently used positions
+  are dropped, and the files they tracked are read in full the next time that
+  name shows up. A dropped position is also cleared from `nsclient.db` on the
+  next shutdown, so the store does not grow forever.
+
 Real-time monitoring (`/settings/logfile/real-time/checks`) is the other way to
 get each line reported once; it pushes results as lines are written instead of
 being polled. `bookmark` is the polled equivalent and needs no configuration on

docs/samples/CheckLogFile_check_logfile_samples.md

@@ -52,6 +52,9 @@ check_logfile file=/var/log/app.log "filter=column1 like 'ERROR'" "warning=count
 3/5 (ERROR failed to connect to db, ERROR failed to connect to db, ERROR disk full)|'count'=3;0;0
 ```
 
+`bookmark=` (an empty value) and `bookmark=auto` are spelled differently by
+different transports but mean exactly this.
+
 **Watch several files with one check**
 
 Each file keeps its own position; the counts are aggregated. A fresh bookmark

modules/CheckLogFile/CheckLogFile.cpp

@@ -15,6 +15,7 @@
 #include <parsers/filter/cli_helper.hpp>
 #include <parsers/filter/modern_filter.hpp>
 #include <utility>
+#include <vector>
 
 #include "bookmark_state.hpp"
 #include "file_reader.hpp"
@@ -58,9 +59,13 @@ bool CheckLogFile::loadModuleEx(std::string alias, NSCAPI::moduleLoadMode mode)
   thread_->filters_.add_samples(settings.get_settings());
 
   // Restore the bookmark positions from the previous run so a restart does not
-  // re-report every line of every bookmarked log file.
+  // re-report every line of every bookmarked log file. An empty value is a
+  // position which was retired on an earlier shutdown; it is remembered as a
+  // key so it is not written again, but it is not a position.
   nscapi::core_helper core(get_core(), get_id());
   for (const nscapi::core_helper::storage_map::value_type &e : core.get_storage_strings(bookmark_context)) {
+    if (e.second.empty()) continue;
+    persisted_keys_.insert(e.first);
     bookmarks_.add(e.first, e.second);
   }
 
@@ -73,9 +78,17 @@ bool CheckLogFile::unloadModule() {
   if (thread_ && !thread_->stop()) NSC_LOG_ERROR_STD("Failed to stop thread");
 
   nscapi::core_helper core(get_core(), get_id());
-  for (const check_logfile::bookmarks::map_type::value_type &v : bookmarks_.get_copy()) {
+  const check_logfile::bookmarks::map_type live = bookmarks_.get_copy();
+  for (const check_logfile::bookmarks::map_type::value_type &v : live) {
     core.put_storage(bookmark_context, v.first, v.second, false, false);
   }
+  // A position which aged out of the (bounded) live set would otherwise keep
+  // its row - and the file name and expression hash in its key - in
+  // nsclient.db for good. The storage API has no delete, so blank the value:
+  // load skips empty entries, which is the same as not having one.
+  for (const std::string &key : persisted_keys_) {
+    if (live.find(key) == live.end()) core.put_storage(bookmark_context, key, "", false, false);
+  }
   return true;
 }
 
@@ -189,17 +202,23 @@ void CheckLogFile::check_logfile(const PB::Commands::QueryRequestMessage::Reques
 			"Notice that specifying multiple files will create an aggregate set it will not check each file individually.\n"
 			"In other words if one file contains an error the entire check will result in error or if you check the count it is the global count which is used.")
 		("files", po::value<std::string>(&files_string), "A comma separated list of files to scan (same as file except a list)")
-		("bookmark", po::value<std::string>(&bookmark)->implicit_value("auto"),
+		// Present-but-empty (`bookmark=`, which is how several transports render a
+		// valueless argument) has to mean the same as the bare flag, or a check
+		// written that way would silently stop being incremental. A notifier is
+		// what makes the two tellable apart from "not given at all": with no
+		// default_value it only runs when the option is actually supplied.
+		("bookmark", po::value<std::string>()->implicit_value("auto")->notifier([&bookmark](const std::string &value) { bookmark = value.empty() ? "auto" : value; }),
 			"Only scan lines added since the last check with the same bookmark name.\n"
 			"NSClient++ remembers, per file and per bookmark, how far it read last time and resumes from there, "
 			"so a line is reported once instead of on every check. The first check of a file reads it in full; "
 			"a file which is truncated, rotated or replaced is detected (via its size and a fingerprint of its "
 			"first bytes) and read from the beginning again. A trailing line which is not yet terminated by "
 			"line-split is held back until it is complete, so half-written lines are never reported twice.\n"
 			"If you set this to auto (or leave the value empty) the bookmark name is derived from the file name "
-			"together with your filter, warning and critical expressions, which keeps unrelated checks of the "
-			"same file from consuming each other's lines. Use an explicit name to share (or separate) positions "
-			"deliberately. Positions are persisted across restarts.")
+			"together with a hash of your filter, warning and critical expressions, which keeps unrelated checks "
+			"of the same file from consuming each other's lines. Use an explicit name to share (or separate) "
+			"positions deliberately. Positions are persisted when NSClient++ shuts down and restored on start; "
+			"the newest ones are kept if more than a thousand accumulate.")
 		("max-lines", po::value<std::size_t>(&max_lines)->default_value(0),
 			"Only examine the newest <N> lines of each file (0, the default, means every line).\n"
 			"The limit is applied per file, after any bookmark: with a bookmark the check still only sees lines "
@@ -255,13 +274,23 @@ void CheckLogFile::check_logfile(const PB::Commands::QueryRequestMessage::Reques
 
   // An "auto" bookmark is derived from the file plus the expressions which
   // decide what is interesting, so two different checks over the same file do
-  // not steal each other's lines (the same rule CheckEventLog uses).
+  // not steal each other's lines (the same rule CheckEventLog uses). The
+  // expressions go in as a hash rather than verbatim: the name ends up in the
+  // persisted key, where a filter of arbitrary length (and content) has no
+  // business being.
   std::string auto_suffix;
   if (bookmark == "auto") {
-    auto_suffix = "],filter[" + str::utils::joinEx(filter_helper.data.filter_string, ",") + "],warn[" +
-                  str::utils::joinEx(filter_helper.data.warn_string, ",") + "],crit[" + str::utils::joinEx(filter_helper.data.crit_string, ",") + "]";
+    const std::string expressions = "filter[" + str::utils::joinEx(filter_helper.data.filter_string, ",") + "],warn[" +
+                                    str::utils::joinEx(filter_helper.data.warn_string, ",") + "],crit[" +
+                                    str::utils::joinEx(filter_helper.data.crit_string, ",") + "]";
+    auto_suffix = "],expr[" + check_logfile::bookmark::to_hex(check_logfile::bookmark::fnv1a(expressions.data(), expressions.size())) + "]";
   }
 
+  // Positions are moved only once every file has been read: a check which ends
+  // in an error reports nothing, and lines it consumed on the way would be
+  // lost with no way to get them back.
+  std::vector<std::pair<std::string, std::string> > pending;
+
   for (const std::string &filename : file_list) {
     std::ifstream file(filename.c_str(), std::ios::in | std::ios::binary);
     if (!file.is_open()) {
@@ -324,7 +353,10 @@ void CheckLogFile::check_logfile(const PB::Commands::QueryRequestMessage::Reques
     }
 
     const check_logfile::bookmark::position next(decision.offset + consumed, head.size(), check_logfile::bookmark::fnv1a(head.data(), head.size()));
-    bookmarks_.add(key, check_logfile::bookmark::format(next));
+    pending.push_back(std::make_pair(key, check_logfile::bookmark::format(next)));
+  }
+  for (const std::pair<std::string, std::string> &p : pending) {
+    bookmarks_.add(p.first, p.second);
   }
   filter_helper.post_process(filter);
 }

modules/CheckLogFile/CheckLogFile.h

@@ -5,6 +5,7 @@
 
 #include <nscapi/nscapi_plugin_impl.hpp>
 #include <nscapi/protobuf/command.hpp>
+#include <set>
 
 #include "bookmarks.hpp"
 
@@ -13,6 +14,11 @@ class CheckLogFile : public nscapi::impl::simple_plugin {
  private:
   std::shared_ptr<real_time_thread> thread_;
   check_logfile::bookmarks bookmarks_;
+  // Bookmark keys which were read from the core storage on load. A key which
+  // is no longer live when we shut down is blanked out there, so a position
+  // that has aged out (or whose filter was edited) does not keep its row in
+  // nsclient.db forever.
+  std::set<std::string> persisted_keys_;
 
  public:
   CheckLogFile() {}

modules/CheckLogFile/bookmark_state.hpp

@@ -6,6 +6,7 @@
 #include <cstddef>
 #include <cstdint>
 #include <cstdlib>
+#include <map>
 #include <string>
 #include <vector>
 
@@ -31,6 +32,18 @@ inline std::uint64_t fnv1a(const char *data, std::size_t len) {
   return hash;
 }
 
+// 16 lowercase hex digits, used to name a bookmark after the expressions it
+// belongs to without spelling them out in the key.
+inline std::string to_hex(std::uint64_t value) {
+  static const char digits[] = "0123456789abcdef";
+  std::string ret(16, '0');
+  for (std::size_t i = 0; i < 16; i++) {
+    ret[15 - i] = digits[value & 0xf];
+    value >>= 4;
+  }
+  return ret;
+}
+
 // Where a previous check stopped reading a given file.
 //
 // `offset` is the end of the last COMPLETE record consumed, not the size of
@@ -142,5 +155,79 @@ inline resume_decision compute_resume(const position &prev, std::uint64_t cur_si
   return d;
 }
 
+// Upper bound on the number of positions which are remembered (and hence
+// persisted to nsclient.db).
+//
+// A bookmark name comes from whoever runs the query, and an automatic name
+// changes whenever the filter does, so nothing stops a caller from creating a
+// new name on every check. Without a cap that grows the stored state - and the
+// file it is saved to - without bound. 1000 positions is far more than a real
+// configuration uses (it is per bookmark AND file) while staying small enough
+// to be irrelevant on disk.
+const std::size_t max_positions = 1000;
+
+// A bounded, least-recently-used map of serialized positions.
+//
+// Not thread safe on its own - `check_logfile::bookmarks` wraps it in a lock.
+// Kept separate from that wrapper so the eviction rules can be unit tested
+// without a running module.
+class store {
+ public:
+  typedef std::map<std::string, std::string> map_type;
+
+  explicit store(std::size_t max_entries = max_positions) : max_entries_(max_entries == 0 ? 1 : max_entries), clock_(0) {}
+
+  void put(const std::string &key, const std::string &value) {
+    entry &e = entries_[key];
+    e.value = value;
+    e.used = ++clock_;
+    trim();
+  }
+
+  // The stored value, or an empty string when the key is unknown. Counts as a
+  // use: a bookmark which is checked but has nothing new to report must not
+  // age out before one which is merely written to.
+  std::string get(const std::string &key) {
+    const impl_type::iterator it = entries_.find(key);
+    if (it == entries_.end()) return "";
+    it->second.used = ++clock_;
+    return it->second.value;
+  }
+
+  map_type snapshot() const {
+    map_type ret;
+    for (const impl_type::value_type &v : entries_) {
+      ret[v.first] = v.second.value;
+    }
+    return ret;
+  }
+
+  std::size_t size() const { return entries_.size(); }
+
+ private:
+  struct entry {
+    std::string value;
+    // Monotonic use counter; the lowest one is the next to go.
+    std::uint64_t used;
+    entry() : used(0) {}
+  };
+  typedef std::map<std::string, entry> impl_type;
+
+  // Linear, but it only runs when the cap is exceeded and the cap is small.
+  void trim() {
+    while (entries_.size() > max_entries_) {
+      impl_type::iterator oldest = entries_.begin();
+      for (impl_type::iterator it = entries_.begin(); it != entries_.end(); ++it) {
+        if (it->second.used < oldest->second.used) oldest = it;
+      }
+      entries_.erase(oldest);
+    }
+  }
+
+  std::size_t max_entries_;
+  std::uint64_t clock_;
+  impl_type entries_;
+};
+
 }  // namespace bookmark
 }  // namespace check_logfile