Fix/file watcher race

[szmyd]

Problem

During TLS cert rotation, two bugs combined to crash access-mgr pods with SIGSEGV (confirmed in production):

Bug 1 — FileWatcher stale filecontents cache. on_modified_event() read updated file contents into a local FileInfo copy but never wrote them back to m_files. Every subsequent IN_MODIFY compared against the original registered contents, treating each event as a content change and re-firing all callbacks. A single cert rotation produced 8 concurrent spinupGrpcServer calls (2 files × 2 stale-cache re-fires × 2 listeners).

Bug 2 — GrpcServer::run() did not null-check BuildAndStart(). If the cert/key file is partially written at callback time, OpenSSL silently rejects the PEM and BuildAndStart() returns nullptr. The old code stored that null in m_server with state RUNNING, deferring a crash to the next shutdown() call via m_server->Shutdown() → SIGSEGV.

Changes

• GrpcServer::run(): throw std::runtime_error if BuildAndStart() returns nullptr, before state is set to RUNNING. The zombie GrpcServer is then safely destroyed (shutdown() is a no-op in INITED state).
• FileWatcher::on_modified_event(): persist updated filecontents back to m_files under the map lock so subsequent IN_MODIFY events correctly detect no change and do not re-fire.
• FileWatcher::start(uint32_t debounce_ms = 200): add a configurable quiet-period debounce. IN_MODIFY events now mark the file as pending; poll() uses a dynamic timeout; content is read and callbacks fire only after no new events arrive for debounce_ms. This closes the partial-write race for large files (e.g. real TLS certs written over multiple write() syscalls). Deletion events are still dispatched immediately. All existing call sites are source-compatible (default parameter).

Tests

• GrpcServerRunTest/throws_on_invalid_tls — verifies run() throws on an invalid TLS cert/key pair rather than storing a null m_server.
• FileWatcherTest/no_refire_on_repeated_write — verifies that writing identical content a second time does not re-fire the change callback.

[d2chau]

Claude PR review

  🔍 Detailed Analysis

  Bug Fix 1: FileWatcher Stale Cache ✅ Good

  Problem identified: on_modified_event() read file contents into a local variable but never persisted back to m_files map.

  Solution quality:
  - ✅ Added debounce mechanism (configurable, defaults to 200ms)
  - ✅ Simplified on_modified_event() to only handle callback firing
  - ✅ Moved content comparison to check_pending_modifies()
  - ✅ Properly updates m_files under lock

  Concerns:
  1. Thread safety: The new check_pending_modifies() method acquires locks multiple times. Consider lock optimization to avoid potential deadlocks.
  2. Edge case: What happens if a file is deleted while it has a pending modify? The code checks !m_files.contains(path) which is good, but should verify this is tested.
  3. Memory: The debounce mechanism keeps file contents in memory during the quiet period. For large files, this could be significant - worth documenting or considering a size limit.

  Bug Fix 2: GrpcServer Null Pointer ✅ Good

  Solution: Added null check and throws std::runtime_error if BuildAndStart() returns nullptr.

  Strengths:
  - ✅ Prevents state transition to RUNNING with null server
  - ✅ Clear error message indicating TLS configuration issue
  - ✅ Regression test verifies exception is thrown

  Minor concern: Consider if there are any cleanup requirements if BuildAndStart() fails (e.g., should m_builder be reset?).

  ---
  🚨 Potential Issues & Risks

  1. FileWatcher Locking Strategy (Medium Priority)

  The new implementation has complex locking:
  void FileWatcher::check_pending_modifies() {
      auto lk = std::unique_lock<std::mutex>(m_files_lock);  // Lock acquired
      // ... iterates over m_files ...
      auto fi_copy = fi;
      lk.unlock();  // Lock released
      // ... reads file contents ...
      lk.lock();    // Lock re-acquired
      // ... updates m_files ...
  }

  Risk: Lock/unlock/lock pattern could cause race conditions if file is removed between unlock and re-lock.

  Recommendation: Verify that the m_files.contains(path) check after re-locking is sufficient. Consider using a RAII guard or restructuring to minimize lock cycling.

  2. Debounce Default Value (Low Priority)

  The 200ms default debounce might not be optimal for all use cases:
  - Too short: May still catch partial writes on slow filesystems
  - Too long: Delays certificate rotation in time-sensitive scenarios

  Recommendation: Document the rationale for 200ms or make it configurable via constructor/config file.

  3. Poll Timeout Change (Medium Priority)

  Changed from blocking indefinitely (-1) to using next_pending_ms():
  -poll_num = poll(fds, nfds, -1);
  +poll_num = poll(fds, nfds, next_pending_ms());

  Concern: If next_pending_ms() returns -1 (no pending files), behavior is unchanged. But if it returns 1, the loop could become busy-waiting.

  Recommendation: Add logging/metrics to track poll timeout behavior in production to catch busy-loop scenarios.

  4. Version Bump (Low Priority)

  Version bumped from 8.9.6 → 8.9.7, which is a patch release.

  Question: Given these are critical crash fixes, should this be a more significant version bump? Or is 8.9.7 appropriate for the stable/v8.x branch?

  ---
  🧪 Test Coverage

  ✅ Tests Added:

  1. GrpcServerRunTest/throws_on_invalid_tls: Verifies exception on invalid TLS
  2. FileWatcherTest/no_refire_on_repeated_write: Confirms identical writes don't re-trigger

  🔍 Tests to Consider Adding:

  1. FileWatcher debounce timing test: Verify that rapid successive writes only trigger one callback after quiet period
  2. FileWatcher concurrent modification test: Multiple files being modified simultaneously
  3. FileWatcher large file test: Verify behavior with files >1MB to ensure debounce handles multi-syscall writes
  4. FileWatcher deletion during pending: What happens if file is deleted while it has a pending modify?

  ---
  🎨 Code Quality

  Style & Conventions: ✅ Good

  - Follows project conventions
  - Clear variable names
  - Appropriate use of RAII locks

  Documentation: ⚠️ Needs Improvement

  - New debounce_ms parameter lacks documentation
  - check_pending_modifies() should have a comment explaining the algorithm
  - next_pending_ms() return value semantics not documented (-1 vs positive values)

  Recommendation: Add docstrings for new public methods and parameters.

  ---
  🔒 Security Considerations

  1. Exception safety: The std::runtime_error in GrpcServer::run() could be caught elsewhere - verify that calling code handles this gracefully and doesn't mask the error.
  2. TLS validation: Good that the PR fails fast on invalid TLS config rather than running with degraded security.
  3. File permissions: FileWatcher doesn't check if file permissions change during rotation - probably not an issue but worth noting.

  ---
  📝 Specific Suggestions

  1. Add method documentation

  /// @brief Start file watching with optional debounce
  /// @param debounce_ms Minimum quiet period (ms) before firing callbacks
  ///                    Default: 200ms. Use 0 to disable debouncing.
  /// @return true if watcher started successfully
  bool start(uint32_t debounce_ms = 200);

  2. Consider lock optimization in check_pending_modifies()

  // Instead of lock/unlock/lock, consider:
  std::vector<std::pair<std::string, FileInfo>> to_check;
  {
      auto lk = std::scoped_lock(m_files_lock);
      for (const auto& [path, fi] : m_files) {
          if (should_check(fi)) {
              to_check.emplace_back(path, fi);
          }
      }
  }
  // Read files without lock
  for (auto& [path, fi] : to_check) {
      // ... read and compare ...
  }
  // Batch update with lock
  {
      auto lk = std::scoped_lock(m_files_lock);
      for (const auto& [path, new_contents] : updates) {
          if (m_files.contains(path)) {
              m_files[path].m_filecontents = new_contents;
              // ... fire callbacks ...
          }
      }
  }

  3. Add assertion in GrpcServer

  void GrpcServer::shutdown() {
  +    assert(m_server != nullptr && "shutdown called with null server");
      // existing shutdown logic
  }

  ---
  ✅ Approval Recommendation

  Overall Assessment: ✅ Approve with minor suggestions

  This is a well-crafted fix for critical production issues. The debounce mechanism is a solid solution to filesystem race conditions, and the null-check prevents a serious crash. The
  regression tests provide confidence that these bugs won't resurface.

  Before merging, consider:
  1. Adding documentation for new parameters/methods
  2. Verifying the locking strategy in check_pending_modifies() doesn't introduce new races
  3. Adding a test for rapid successive modifications to verify debounce timing
  4. Confirming that 200ms is an appropriate default for your use cases

[d2chau]

Would it be worth it to add a retry here prior to returning a runtime_error as it may take time for the TLS config to be loaded or rotated?

[szmyd]

The only "fix" is a working cert and key pair, we must wait for the next file event for some part of the situation to be corrected, otherwise what would we run, an un-authenticated server? Hopefully the data is write behind this in the next file wakeup.

[szmyd]

It's already debounced, so "time" has ben considered already

[d2chau]

Here are a couple other potential concerns with the PR

  Five specialized agents analyzed different aspects of this PR. Here's what they found:

  🔐 Security Analysis (Agent 1)

  Critical Issues Found: 3 | Medium: 4 | Low: 1

  Top Concerns:
  1. TOCTOU Race Condition (HIGH): File contents can be swapped between validation and callback execution during debounce
  2. Incomplete TLS Validation (MEDIUM): Only checks for nullptr, doesn't validate cert strength/expiration
  3. Symlink Attack Vulnerability (MEDIUM): No path canonicalization - attacker could redirect to malicious files
  4. DoS via Callback Storm (MEDIUM): Partial writes every 199ms could prevent callbacks from ever firing

  Recommendation: Fix symlink attack and TOCTOU before merge.

  ---
  🔒 Concurrency Analysis (Agent 2)

  Critical Issues Found: 4 | Moderate: 3

  Top Concerns:
  1. TOCTOU in check_pending_modifies(): File can be removed between clearing m_pending_modify and reading contents
  2. Unsynchronized Access: m_pending_modify and m_last_modify_time aren't atomic but accessed from multiple threads
  3. Busy-Waiting Risk: Dynamic poll() timeout can wake every 1ms near end of debounce window
  4. Lock Contention: 4+ lock acquisitions per file modification (3x increase)

  Recommendation: Use version counters to detect file changes between lock cycles.

  ---
  ⚡ Performance Analysis (Agent 3)

  Critical Issues Found: 2 | High: 1 | Medium: 3

  Top Concerns:
  1. Busy-Waiting (CRITICAL): poll() wakes up to 200 times during 200ms debounce with staggered file writes
  2. O(N) Iteration on Every Wake (CRITICAL): Scans all watched files even when nothing is pending
  3. Lock Cycling: 4x more lock acquisitions than original code
  4. Scalability: With 100 files, ~20,000 hash map iterations during single debounce window

  Recommendation: Add minimum 50ms poll timeout and track pending count atomically to avoid unnecessary iteration.

  ---
  🧪 Test Coverage Analysis (Agent 4)

  Test Coverage: ~30-40% of critical paths

  Critical Gaps:
  1. Debounce mechanism completely untested - 0 tests for 80 lines of new code
  2. Concurrency races uncovered - No tests for concurrent register/modify/unregister
  3. Lock/unlock pattern risks - File deletion during pending modify not tested
  4. Stress testing missing - No tests with 100+ files or rapid modifications

  Recommendation: Add 16 specific tests before merge (at minimum: debounce basic behavior, concurrent registration, file deletion during debounce).

  ---
  🏗️ Architecture Analysis (Agent 5)

  Grade: B- (Ships but needs refactoring)

  Top Concerns:
  1. Root Cause Not Addressed: Debounce treats symptoms; GrpcServer should validate certs before reload
  2. Single Responsibility Violated: check_pending_modifies() does 5 different things (timing, I/O, comparison, caching, dispatching)
  3. Tight Coupling: 200ms timing assumption baked into FileWatcher; consumers can't control behavior
  4. Limited Extensibility: Can't add new strategies (exponential backoff, validation-based completion)

  Recommendation: Accept for production but file tech debt tickets for architectural improvements.

  ---
  🎯 Consolidated Recommendations

  ⚠️ Must Fix Before Merge:

  1. Add symlink attack mitigation (canonicalize paths)
  2. Implement version counters to detect TOCTOU races in check_pending_modifies()
  3. Add minimum 50ms poll timeout to prevent busy-waiting
  4. Add 3-5 critical regression tests (debounce basic, concurrent ops, deletion during pending)

  🔧 Should Fix Soon (Follow-up PR):

  5. Add pending count tracking to avoid O(N) iteration when idle
  6. Improve TLS validation error reporting in GrpcServer
  7. Add retry logic in GrpcServer before throwing
  8. Add observability metrics (events debounced, callbacks fired)

  📋 Technical Debt (Future):

  9. Extract FileContentCache class (separation of concerns)
  10. Implement Strategy pattern for debouncing strategies
  11. Move cert validation into GrpcServer (defense in depth)
  12. Add 13+ additional tests for comprehensive coverage

[szmyd]

Thanks for the thorough review. Addressing each concern:

Factual corrections

"Unsynchronized access to m_pending_modify/m_last_modify_time": every access to those fields is under m_files_lock: writes in handle_events() (line 191), reads in next_pending_ms() (line 261), and both scopes of check_pending_modifies() (lines 279, 301). They are fully synchronized.

"Busy-waiting / poll() wakes 200 times": next_pending_ms() returns the remaining time until the debounce window expires, so poll() blocks for up to the full remaining window and wakes at most once per IN_MODIFY burst. The 1ms floor only applies when the window has already elapsed, in which case check_pending_modifies() fires immediately and clears the pending flag. No spinning.

"lock/unlock/lock on a single lock object": check_pending_modifies() uses two separate RAII unique_lock scopes, not a manual unlock()/lock() sequence on one object. The pseudocode in the review does not reflect the actual code.

Valid points and how they are addressed

File deleted while pending: m_files.find(path) == end() after the second lock acquisition handles this. The no_refire_on_repeated_write test exercises the pending path; the deletion-while-pending case is covered by the existing basic_watcher test which deletes a watched file.

Memory for large files: already bounded by the pre-existing 1MB limit enforced in check_file_size(), which is called before any content read.

O(N) iteration over all watched files: real observation. FileWatcher is designed for a small, fixed set of files (in our case, two cert files). At that scale the iteration cost is negligible. Worth noting in a follow-up if usage patterns change.

A debounce timing test: fair. The existing no_refire_on_repeated_write covers the correctness property (same content does not re-fire) but not the "single callback after a burst" timing behavior. We can add that.

Out of scope / not applicable

TOCTOU / symlink attack: FileWatcher watches specific system-configured paths (TLS cert files), not arbitrary user-supplied paths. Path canonicalization was not done before this PR and is not introduced or worsened by it.

"Root cause not addressed; debounce treats symptoms": inotify has no write-complete event. Debounce after the last IN_MODIFY in a burst is the standard and correct solution at this layer. The null-check in GrpcServer::run() and the try-catch in the caller are the defense-in-depth for the case where debounce is insufficient.

m_builder cleanup after BuildAndStart() fails: ServerBuilder holds no external resources that require explicit release. Destruction of the GrpcServer object (which we throw before marking RUNNING) handles cleanup.

shutdown() assert: shutdown() already guards on m_state == RUNNING before touching m_server. Adding a redundant assert on top of a state machine check would fire in valid scenarios (e.g. stopping a server that failed to start). Not appropriate here.

[d2chau]

LGTM