From 9ba263ce30e7b6311f138a871edcded0de4935b3 Mon Sep 17 00:00:00 2001
From: Adrian Niculescu <15037449+adrian-niculescu@users.noreply.github.com>
Date: Tue, 2 Dec 2025 23:46:16 +0200
Subject: [PATCH] fix: resolve timer bugs causing lost timers and resource
 leaks

  Fixed five bugs in Timers.cpp:
  - Inverted EAGAIN retry condition causing timer loss when pipe buffer full
  - File descriptor leak (fd_[1] never closed in Destroy)
  - deletedTimers_ accumulation for immediate timers cleared before callback
  - Data race on stopped flag (now atomic)
  - Non-EAGAIN write errors incorrectly treated as success

  Additional improvements:
  - Clean up deletedTimers_ when timer cancelled during EAGAIN retry
  - Guard isBufferFull reset to only occur on actual write success
---
 test-app/runtime/src/main/cpp/Timers.cpp | 23 ++++++++++++++++++-----
 test-app/runtime/src/main/cpp/Timers.h   |  2 +-
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/test-app/runtime/src/main/cpp/Timers.cpp b/test-app/runtime/src/main/cpp/Timers.cpp
index 31d417866..694ed14c2 100644
--- a/test-app/runtime/src/main/cpp/Timers.cpp
+++ b/test-app/runtime/src/main/cpp/Timers.cpp
@@ -89,11 +89,14 @@ void Timers::addTask(std::shared_ptr<TimerTask> task) {
     bool needsScheduling = true;
     if (!isBufferFull.load() && task->dueTime_ <= now) {
         auto result = write(fd_[1], &task->id_, sizeof(int));
-        if (result != -1 || errno != EAGAIN) {
+        if (result != -1) {
+            // Wrote successfully to the pipe; no need to schedule in sortedTimers_
             needsScheduling = false;
-        } else {
+        } else if (errno == EAGAIN) {
+            // Pipe is full; mark buffer as full and fall back to sortedTimers_
             isBufferFull = true;
         }
+        // For other errors: keep needsScheduling = true to avoid dropping timer
     }
     if (needsScheduling) {
         {
@@ -156,11 +159,16 @@ void Timers::threadLoop() {
                 auto result = write(fd_[1], &timer->id, sizeof(int));
                 if (result == -1 && errno == EAGAIN) {
                     isBufferFull = true;
-                    while (!stopped && deletedTimers_.find(timer->id) != deletedTimers_.end() &&
+                    while (!stopped && deletedTimers_.find(timer->id) == deletedTimers_.end() &&
                            write(fd_[1], &timer->id, sizeof(int)) == -1 && errno == EAGAIN) {
                         bufferFull.wait(lk);
                     }
-                } else if (isBufferFull.load() &&
+                    // If the timer was cleared while we were waiting for buffer space,
+                    // clean up deletedTimers_ to avoid leaking the timer ID.
+                    if (deletedTimers_.find(timer->id) != deletedTimers_.end()) {
+                        deletedTimers_.erase(timer->id);
+                    }
+                } else if (result != -1 && isBufferFull.load() &&
                            (sortedTimers_.empty() || sortedTimers_.at(0)->dueTime > now)) {
                     // we had a successful write and the next timer is not due
                     // mark the buffer as free to re-enable the setTimeout with 0 optimization
@@ -189,6 +197,7 @@ void Timers::Destroy() {
     auto mainLooper = Runtime::GetMainLooper();
     ALooper_removeFd(mainLooper, fd_[0]);
     close(fd_[0]);
+    close(fd_[1]);
     timerMap_.clear();
     ALooper_release(looper_);
 }
@@ -319,6 +328,10 @@ int Timers::PumpTimerLoopCallback(int fd, int events, void *data) {
         }
 
 
+    } else {
+        // Timer was cleared before callback ran - clean up deletedTimers_
+        std::lock_guard<std::mutex> lock(thiz->mutex);
+        thiz->deletedTimers_.erase(timerId);
     }
     thiz->bufferFull.notify_one();
     return 1;
@@ -332,4 +345,4 @@ void Timers::InitStatic(v8::Isolate* isolate, v8::Local<v8::ObjectTemplate> glob
 
 };
 
-NODE_BINDING_PER_ISOLATE_INIT_OBJ(timers, tns::Timers::InitStatic);
\ No newline at end of file
+NODE_BINDING_PER_ISOLATE_INIT_OBJ(timers, tns::Timers::InitStatic);
diff --git a/test-app/runtime/src/main/cpp/Timers.h b/test-app/runtime/src/main/cpp/Timers.h
index 35fd9485d..317776748 100644
--- a/test-app/runtime/src/main/cpp/Timers.h
+++ b/test-app/runtime/src/main/cpp/Timers.h
@@ -128,7 +128,7 @@ namespace tns {
         std::condition_variable bufferFull;
         std::mutex mutex;
         std::thread watcher_;
-        bool stopped = false;
+        std::atomic<bool> stopped = false;
     };
 
 }