Hopefully fixed lockup bug.

5 files changed, 233 insertions, 116 deletions

diff --git a/bragi.py b/bragi.py
index 9072561..b5008d4 100755
--- a/bragi.py
+++ b/bragi.py
@@ -547,8 +547,7 @@ class MumbleBot:
                     elif self.on_interrupting or len(raw_music) < self.pcm_buffer_size:
                         self.mumble.sound_output.add_sound(
                             audioop.mul(self._fadeout(raw_music, self.stereo, fadein=False), 2, self.volume_helper.real_volume))
-                        self.thread.kill()
-                        self.thread = None
+                        self._cleanup_ffmpeg_process()
                         time.sleep(0.1)
                         self.on_interrupting = False
                 else:
@@ -557,7 +556,7 @@ class MumbleBot:
                 time.sleep(0.1)
 
             if not self.is_pause and not raw_music:
-                self.thread = None
+                self._cleanup_ffmpeg_process()
                 # bot is not paused, but ffmpeg thread has gone.
                 # indicate that last song has finished, or the bot just resumed from pause, or something is wrong.
                 if self.read_pcm_size < self.pcm_buffer_size \
@@ -703,12 +702,14 @@ class MumbleBot:
     def clear(self):
         # Kill the ffmpeg thread and empty the playlist
         self.interrupt()
+        self._cleanup_ffmpeg_process()  # Ensure proper cleanup
         var.playlist.clear()
         self.wait_for_ready = False
         self.log.info("bot: music stopped. playlist trashed.")
 
     def stop(self):
         self.interrupt()
+        self._cleanup_ffmpeg_process()  # Ensure proper cleanup
         self.is_pause = True
         if len(var.playlist) > 0:
             self.wait_for_ready = True
@@ -716,6 +717,34 @@ class MumbleBot:
             self.wait_for_ready = False
         self.log.info("bot: music stopped.")
 
+    def _cleanup_ffmpeg_process(self):
+        """Properly cleanup FFmpeg process and associated resources"""
+        if self.thread:
+            try:
+                # Check if process is already terminated to avoid double cleanup
+                if self.thread.poll() is None:  # Process is still running
+                    self.thread.terminate()  # Send SIGTERM first
+                    try:
+                        self.thread.wait(timeout=2)  # Wait up to 2 seconds for graceful exit
+                    except sp.TimeoutExpired:
+                        self.log.warning("bot: FFmpeg process didn't terminate gracefully, killing forcefully")
+                        self.thread.kill()  # Force kill if it doesn't terminate
+                        self.thread.wait()  # Wait for process to be reaped
+                else:
+                    # Process already terminated, just wait to reap it
+                    self.thread.wait()
+            except Exception as e:
+                self.log.error(f"bot: Error cleaning up FFmpeg process: {e}")
+            finally:
+                # Close stderr pipe if it exists
+                if self.thread_stderr:
+                    try:
+                        self.thread_stderr.close()
+                        self.thread_stderr = None
+                    except Exception as e:
+                        self.log.error(f"bot: Error closing stderr pipe: {e}")
+                self.thread = None
+
     def interrupt(self):
         # Kill the ffmpeg thread
         if self.thread:
@@ -728,6 +757,7 @@ class MumbleBot:
     def pause(self):
         # Kill the ffmpeg thread
         self.interrupt()
+        self._cleanup_ffmpeg_process()  # Ensure proper cleanup
         self.is_pause = True
         self.song_start_at = -1
         if len(var.playlist) > 0:
diff --git a/database.py b/database.py
index ab64932..5138920 100644
--- a/database.py
+++ b/database.py
@@ -208,10 +208,14 @@ class SettingsDatabase:
 
     def get(self, section, option, **kwargs):
         conn = sqlite3.connect(self.db_path)
-        cursor = conn.cursor()
-        result = cursor.execute("SELECT value FROM botamusique WHERE section=? AND option=?",
-                                (section, option)).fetchall()
-        conn.close()
+        try:
+            cursor = conn.cursor()
+            result = cursor.execute("SELECT value FROM botamusique WHERE section=? AND option=?",
+                                    (section, option)).fetchall()
+        finally:
+            if 'cursor' in locals():
+                cursor.close()
+            conn.close()
 
         if len(result) > 0:
             return result[0][0]
@@ -232,18 +236,26 @@ class SettingsDatabase:
 
     def set(self, section, option, value):
         conn = sqlite3.connect(self.db_path)
-        cursor = conn.cursor()
-        cursor.execute("INSERT OR REPLACE INTO botamusique (section, option, value) "
-                       "VALUES (?, ?, ?)", (section, option, value))
-        conn.commit()
-        conn.close()
+        try:
+            cursor = conn.cursor()
+            cursor.execute("INSERT OR REPLACE INTO botamusique (section, option, value) "
+                           "VALUES (?, ?, ?)", (section, option, value))
+            conn.commit()
+        finally:
+            if 'cursor' in locals():
+                cursor.close()
+            conn.close()
 
     def has_option(self, section, option):
         conn = sqlite3.connect(self.db_path)
-        cursor = conn.cursor()
-        result = cursor.execute("SELECT value FROM botamusique WHERE section=? AND option=?",
-                                (section, option)).fetchall()
-        conn.close()
+        try:
+            cursor = conn.cursor()
+            result = cursor.execute("SELECT value FROM botamusique WHERE section=? AND option=?",
+                                    (section, option)).fetchall()
+        finally:
+            if 'cursor' in locals():
+                cursor.close()
+            conn.close()
         if len(result) > 0:
             return True
         else:
@@ -251,23 +263,35 @@ class SettingsDatabase:
 
     def remove_option(self, section, option):
         conn = sqlite3.connect(self.db_path)
-        cursor = conn.cursor()
-        cursor.execute("DELETE FROM botamusique WHERE section=? AND option=?", (section, option))
-        conn.commit()
-        conn.close()
+        try:
+            cursor = conn.cursor()
+            cursor.execute("DELETE FROM botamusique WHERE section=? AND option=?", (section, option))
+            conn.commit()
+        finally:
+            if 'cursor' in locals():
+                cursor.close()
+            conn.close()
 
     def remove_section(self, section):
         conn = sqlite3.connect(self.db_path)
-        cursor = conn.cursor()
-        cursor.execute("DELETE FROM botamusique WHERE section=?", (section,))
-        conn.commit()
-        conn.close()
+        try:
+            cursor = conn.cursor()
+            cursor.execute("DELETE FROM botamusique WHERE section=?", (section,))
+            conn.commit()
+        finally:
+            if 'cursor' in locals():
+                cursor.close()
+            conn.close()
 
     def items(self, section):
         conn = sqlite3.connect(self.db_path)
-        cursor = conn.cursor()
-        results = cursor.execute("SELECT option, value FROM botamusique WHERE section=?", (section,)).fetchall()
-        conn.close()
+        try:
+            cursor = conn.cursor()
+            results = cursor.execute("SELECT option, value FROM botamusique WHERE section=?", (section,)).fetchall()
+        finally:
+            if 'cursor' in locals():
+                cursor.close()
+            conn.close()
 
         if len(results) > 0:
             return list(map(lambda v: (v[0], v[1]), results))
@@ -276,9 +300,13 @@ class SettingsDatabase:
 
     def drop_table(self):
         conn = sqlite3.connect(self.db_path)
-        cursor = conn.cursor()
-        cursor.execute("DROP TABLE botamusique")
-        conn.close()
+        try:
+            cursor = conn.cursor()
+            cursor.execute("DROP TABLE botamusique")
+        finally:
+            if 'cursor' in locals():
+                cursor.close()
+            conn.close()
 
 
 class MusicDatabase:
@@ -287,66 +315,79 @@ class MusicDatabase:
 
     def insert_music(self, music_dict, _conn=None):
         conn = sqlite3.connect(self.db_path) if _conn is None else _conn
-        cursor = conn.cursor()
-
-        id = music_dict['id']
-        title = music_dict['title']
-        type = music_dict['type']
-        path = music_dict['path'] if 'path' in music_dict else ''
-        keywords = music_dict['keywords']
-
-        tags_list = list(dict.fromkeys(music_dict['tags']))
-        tags = ''
-        if tags_list:
-            tags = ",".join(tags_list) + ","
-
-        del music_dict['id']
-        del music_dict['title']
-        del music_dict['type']
-        del music_dict['tags']
-        if 'path' in music_dict:
-            del music_dict['path']
-        del music_dict['keywords']
-
-        existed = cursor.execute("SELECT 1 FROM music WHERE id=?", (id,)).fetchall()
-        if len(existed) == 0:
-            cursor.execute(
-                "INSERT INTO music (id, type, title, metadata, tags, path, keywords) VALUES (?, ?, ?, ?, ?, ?, ?)",
-                (id,
-                 type,
-                 title,
-                 json.dumps(music_dict),
-                 tags,
-                 path,
-                 keywords))
-        else:
-            cursor.execute("UPDATE music SET type=:type, title=:title, metadata=:metadata, tags=:tags, "
-                           "path=:path, keywords=:keywords WHERE id=:id",
-                           {'id': id,
-                            'type': type,
-                            'title': title,
-                            'metadata': json.dumps(music_dict),
-                            'tags': tags,
-                            'path': path,
-                            'keywords': keywords})
-
+        cursor = None
+        try:
+            cursor = conn.cursor()
+
+            id = music_dict['id']
+            title = music_dict['title']
+            type = music_dict['type']
+            path = music_dict['path'] if 'path' in music_dict else ''
+            keywords = music_dict['keywords']
+
+            tags_list = list(dict.fromkeys(music_dict['tags']))
+            tags = ''
+            if tags_list:
+                tags = ",".join(tags_list) + ","
+
+            del music_dict['id']
+            del music_dict['title']
+            del music_dict['type']
+            del music_dict['tags']
+            if 'path' in music_dict:
+                del music_dict['path']
+            del music_dict['keywords']
+
+            existed = cursor.execute("SELECT 1 FROM music WHERE id=?", (id,)).fetchall()
+            if len(existed) == 0:
+                cursor.execute(
+                    "INSERT INTO music (id, type, title, metadata, tags, path, keywords) VALUES (?, ?, ?, ?, ?, ?, ?)",
+                    (id,
+                     type,
+                     title,
+                     json.dumps(music_dict),
+                     tags,
+                     path,
+                     keywords))
+            else:
+                cursor.execute("UPDATE music SET type=:type, title=:title, metadata=:metadata, tags=:tags, "
+                               "path=:path, keywords=:keywords WHERE id=:id",
+                               {'id': id,
+                                'type': type,
+                                'title': title,
+                                'metadata': json.dumps(music_dict),
+                                'tags': tags,
+                                'path': path,
+                                'keywords': keywords})
+
+        finally:
+            if cursor:
+                cursor.close()
         if not _conn:
             conn.commit()
             conn.close()
 
     def query_music_ids(self, condition: Condition):
         conn = sqlite3.connect(self.db_path)
-        cursor = conn.cursor()
-        results = cursor.execute("SELECT id FROM music WHERE id != 'info' AND %s" %
-                                 condition.sql(conn), condition.filler).fetchall()
-        conn.close()
+        try:
+            cursor = conn.cursor()
+            results = cursor.execute("SELECT id FROM music WHERE id != 'info' AND %s" %
+                                     condition.sql(conn), condition.filler).fetchall()
+        finally:
+            if 'cursor' in locals():
+                cursor.close()
+            conn.close()
         return list(map(lambda i: i[0], results))
 
     def query_all_paths(self):
         conn = sqlite3.connect(self.db_path)
-        cursor = conn.cursor()
-        results = cursor.execute("SELECT path FROM music WHERE id != 'info' AND type = 'file'").fetchall()
-        conn.close()
+        try:
+            cursor = conn.cursor()
+            results = cursor.execute("SELECT path FROM music WHERE id != 'info' AND type = 'file'").fetchall()
+        finally:
+            if 'cursor' in locals():
+                cursor.close()
+            conn.close()
         paths = []
         for result in results:
             if result and result[0]:
@@ -356,25 +397,33 @@ class MusicDatabase:
 
     def query_all_tags(self):
         conn = sqlite3.connect(self.db_path)
-        cursor = conn.cursor()
-        results = cursor.execute("SELECT tags FROM music WHERE id != 'info'").fetchall()
-        tags = []
-        for result in results:
-            for tag in result[0].strip(",").split(","):
-                if tag and tag not in tags:
-                    tags.append(tag)
-        conn.close()
+        try:
+            cursor = conn.cursor()
+            results = cursor.execute("SELECT tags FROM music WHERE id != 'info'").fetchall()
+            tags = []
+            for result in results:
+                for tag in result[0].strip(",").split(","):
+                    if tag and tag not in tags:
+                        tags.append(tag)
+        finally:
+            if 'cursor' in locals():
+                cursor.close()
+            conn.close()
         return tags
 
     def query_music_count(self, condition: Condition):
         filler = condition.filler
 
         conn = sqlite3.connect(self.db_path)
-        condition_str = condition.sql(conn)
-        cursor = conn.cursor()
-        results = cursor.execute("SELECT COUNT(*) FROM music "
-                                 "WHERE id != 'info' AND %s" % condition_str, filler).fetchall()
-        conn.close()
+        try:
+            condition_str = condition.sql(conn)
+            cursor = conn.cursor()
+            results = cursor.execute("SELECT COUNT(*) FROM music "
+                                     "WHERE id != 'info' AND %s" % condition_str, filler).fetchall()
+        finally:
+            if 'cursor' in locals():
+                cursor.close()
+            conn.close()
 
         return results[0][0]
 
@@ -382,10 +431,15 @@ class MusicDatabase:
         filler = condition.filler
 
         conn = sqlite3.connect(self.db_path) if _conn is None else _conn
-        condition_str = condition.sql(conn)
-        cursor = conn.cursor()
-        results = cursor.execute("SELECT id, type, title, metadata, tags, path, keywords FROM music "
-                                 "WHERE id != 'info' AND %s" % condition_str, filler).fetchall()
+        cursor = None
+        try:
+            condition_str = condition.sql(conn)
+            cursor = conn.cursor()
+            results = cursor.execute("SELECT id, type, title, metadata, tags, path, keywords FROM music "
+                                     "WHERE id != 'info' AND %s" % condition_str, filler).fetchall()
+        finally:
+            if cursor:
+                cursor.close()
         if not _conn:
             conn.close()
 
@@ -393,9 +447,14 @@ class MusicDatabase:
 
     def _query_music_by_plain_sql_cond(self, sql_cond, _conn=None):
         conn = sqlite3.connect(self.db_path) if _conn is None else _conn
-        cursor = conn.cursor()
-        results = cursor.execute("SELECT id, type, title, metadata, tags, path, keywords FROM music "
-                                 "WHERE id != 'info' AND %s" % sql_cond).fetchall()
+        cursor = None
+        try:
+            cursor = conn.cursor()
+            results = cursor.execute("SELECT id, type, title, metadata, tags, path, keywords FROM music "
+                                     "WHERE id != 'info' AND %s" % sql_cond).fetchall()
+        finally:
+            if cursor:
+                cursor.close()
         if not _conn:
             conn.close()
 
diff --git a/pymumble_py3/callbacks.py b/pymumble_py3/callbacks.py
index b46e883..895eef1 100644
--- a/pymumble_py3/callbacks.py
+++ b/pymumble_py3/callbacks.py
@@ -81,6 +81,7 @@ class CallBacks(dict):
             for func in self[callback]:
                 if callback is PYMUMBLE_CLBK_TEXTMESSAGERECEIVED:
                     thr = threading.Thread(target=func, args=pos_parameters)
+                    thr.daemon = True  # Fix critical memory leak - ensure thread cleanup
                     thr.start()
                 else:
                     func(*pos_parameters)
diff --git a/pymumble_py3/soundoutput.py b/pymumble_py3/soundoutput.py
index 678a71d..ea750a2 100644
--- a/pymumble_py3/soundoutput.py
+++ b/pymumble_py3/soundoutput.py
@@ -161,14 +161,27 @@ class SoundOutput:
         samples = int(self.encoder_framesize * PYMUMBLE_SAMPLERATE * 2 * self.channels)  # number of samples in an encoder frame
 
         self.lock.acquire()
-        if len(self.pcm) and len(self.pcm[-1]) < samples:
-            initial_offset = samples - len(self.pcm[-1])
-            self.pcm[-1] += pcm[:initial_offset]
-        else:
-            initial_offset = 0
-        for i in range(initial_offset, len(pcm), samples):
-            self.pcm.append(pcm[i:i + samples])
-        self.lock.release()
+        try:
+            # Prevent unbounded buffer growth - limit to 10 seconds of audio
+            max_buffer_seconds = 10.0
+            max_buffer_frames = int(max_buffer_seconds / self.encoder_framesize)
+            
+            # If buffer is too full, drop oldest frames to prevent memory leak
+            if len(self.pcm) > max_buffer_frames:
+                frames_to_drop = len(self.pcm) - max_buffer_frames
+                self.pcm = self.pcm[frames_to_drop:]
+                if hasattr(self, 'Log'):
+                    self.Log.warning(f"Audio buffer overflow, dropped {frames_to_drop} frames")
+            
+            if len(self.pcm) and len(self.pcm[-1]) < samples:
+                initial_offset = samples - len(self.pcm[-1])
+                self.pcm[-1] += pcm[:initial_offset]
+            else:
+                initial_offset = 0
+            for i in range(initial_offset, len(pcm), samples):
+                self.pcm.append(pcm[i:i + samples])
+        finally:
+            self.lock.release()
 
     def clear_buffer(self):
         self.lock.acquire()
diff --git a/util.py b/util.py
index cd4845a..1916ce0 100644
--- a/util.py
+++ b/util.py
@@ -312,15 +312,29 @@ def get_media_duration(path):
     command = ("ffprobe", "-v", "quiet", "-show_entries", "format=duration",
                "-of", "default=noprint_wrappers=1:nokey=1", path)
     process = sp.Popen(command, stdout=sp.PIPE, stderr=sp.PIPE)
-    stdout, stderr = process.communicate()
-
     try:
-        if not stderr:
-            return float(stdout)
-        else:
+        stdout, stderr = process.communicate(timeout=10)  # Add timeout to prevent hanging
+        
+        try:
+            if not stderr:
+                return float(stdout)
+            else:
+                return 0
+        except ValueError:
             return 0
-    except ValueError:
+    except sp.TimeoutExpired:
+        process.kill()
+        process.wait()
         return 0
+    finally:
+        # Ensure process is properly cleaned up
+        if process.poll() is None:
+            process.terminate()
+            try:
+                process.wait(timeout=2)
+            except sp.TimeoutExpired:
+                process.kill()
+                process.wait()
 
 
 def parse_time(human):
@@ -405,9 +419,9 @@ def get_snapshot_version():
     ver = "unknown"
     if os.path.exists(os.path.join(root_dir, ".git")):
         try:
-            ret = subprocess.check_output(["git", "describe", "--tags"]).strip()
+            ret = subprocess.check_output(["git", "describe", "--tags"], timeout=5).strip()
             ver = ret.decode("utf-8")
-        except (FileNotFoundError, subprocess.CalledProcessError):
+        except (FileNotFoundError, subprocess.CalledProcessError, subprocess.TimeoutExpired):
             try:
                 with open(os.path.join(root_dir, ".git/refs/heads/master")) as f:
                     ver = "g" + f.read()[:7]