[PATCH v2 0/3] prserv: add --status, and make --start notice a dead server

[PATCH v2 0/3] prserv: add --status, and make --start notice a dead server

[Babanpreet Singh]

v1 of --status failed bitbake-selftest on the autobuilder: test_2 found
a stale pidfile for the server test_1 had just started. The daemon dies
somewhere between writing its pidfile and serving, and --start cannot
tell, because it returns once the intermediate fork exits. The old
start/stop pair never noticed a dead server since --stop treats a stale
pidfile as success. Every autobuilder run that carried v1 failed the
same way (26 runs on 20 workers, on Richard's and Mathieu's
master-next) while it passes here. Binding 8585 before --start gives
the identical symptom, but the workers' prserv.log is not published so
I do not know what kills it there.

1/3 ScriptTests picks a free port and keeps one database directory
    for the class instead of 8585 and a directory per test.
2/3 --start waits until the daemon has bound and opened its database,
    and exits 1 with the daemon's error and the logfile path if it
    fails first. The daemon removes its pidfile on failure. New test:
    --start on a busy port fails and leaves no pidfile.
3/3 --status, as in v1, with the tests on the class port.

If the daemon still dies on the autobuilder, test_1 fails with the
reason in the log rather than test_2 with a stale pidfile.

Tested on top of 046a90b0e: prserv.tests 10/10 at the tip and at each
intermediate commit, ScriptTests 20 runs in a row clean, and the rest of
bitbake-selftest module by module with BB_SKIP_NETTESTS=yes, all OK
except hashserv.tests, which hangs on this host with or without these
patches.

Babanpreet Singh (3):
  prserv/tests: run ScriptTests on a free port with one database
  prserv: make --start fail when the server does not come up
  prserv: add --status to list running servers

 bin/bitbake-prserv  |  7 +++++
 lib/prserv/serv.py  | 76 ++++++++++++++++++++++++++++++++++++++++-----
 lib/prserv/tests.py | 54 +++++++++++++++++++++++++++-----
 3 files changed, 122 insertions(+), 15 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] prserv/tests: run ScriptTests on a free port with one database

[Babanpreet Singh]

ScriptTests starts bitbake-prserv on the default port (8585) and gives
each test its own temporary directory for the database. Anything
already listening on 8585 on the test machine stops the daemon from
binding.

Pick a free port in setUpClass and keep one database directory for the
whole class, which is what the oe-selftest prservice test already does
for its port.

AI-Generated: Uses Claude (claude-fable-5-1)
Signed-off-by: Babanpreet Singh <bbnpreetsingh@gmail.com>
---
 lib/prserv/tests.py | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/lib/prserv/tests.py b/lib/prserv/tests.py
index df0c00300..f3b2aae45 100644
--- a/lib/prserv/tests.py
+++ b/lib/prserv/tests.py
@@ -369,20 +369,28 @@ class PRUpstreamTests(PRTestSetup, unittest.TestCase):
 
 class ScriptTests(unittest.TestCase):
 
-    def setUp(self):
-
-        self.temp_dir = tempfile.TemporaryDirectory(prefix='bb-prserv')
-        self.addCleanup(self.temp_dir.cleanup)
-        self.dbfile = os.path.join(self.temp_dir.name, "prtest.sqlite3")
+    @classmethod
+    def setUpClass(cls):
+        cls.temp_dir = tempfile.TemporaryDirectory(prefix='bb-prserv')
+        cls.dbfile = os.path.join(cls.temp_dir.name, "prtest.sqlite3")
+        # The server test_1 starts runs until test_2 stops it, so use a
+        # port nothing else on the machine is listening on
+        with socket.socket() as s:
+            s.bind(("0.0.0.0", 0))
+            cls.port = str(s.getsockname()[1])
+
+    @classmethod
+    def tearDownClass(cls):
+        cls.temp_dir.cleanup()
 
     def test_1_start_bitbake_prserv(self):
         try:
-            subprocess.check_call([BIN_DIR / "bitbake-prserv", "--start", "-f", self.dbfile])
+            subprocess.check_call([BIN_DIR / "bitbake-prserv", "--start", "-f", self.dbfile, "--port", self.port])
         except subprocess.CalledProcessError as e:
             self.fail("Failed to start bitbake-prserv: %s" % e.returncode)
 
     def test_2_stop_bitbake_prserv(self):
         try:
-            subprocess.check_call([BIN_DIR / "bitbake-prserv", "--stop"])
+            subprocess.check_call([BIN_DIR / "bitbake-prserv", "--stop", "--port", self.port])
         except subprocess.CalledProcessError as e:
             self.fail("Failed to stop bitbake-prserv: %s" % e.returncode)
-- 
2.43.0

[PATCH v2 2/3] prserv: make --start fail when the server does not come up

[Babanpreet Singh]

Make `prserv --start` fail when the server can’t start, and clean up stale pidfiles on failure.

AI-Generated: Uses Claude (claude-fable-5-1)
Signed-off-by: Babanpreet Singh <bbnpreetsingh@gmail.com>
---
 lib/prserv/serv.py  | 53 +++++++++++++++++++++++++++++++++++++++------
 lib/prserv/tests.py | 14 ++++++++++++
 2 files changed, 60 insertions(+), 7 deletions(-)

diff --git a/lib/prserv/serv.py b/lib/prserv/serv.py
index e17588630..6aba1c3e0 100644
--- a/lib/prserv/serv.py
+++ b/lib/prserv/serv.py
@@ -205,12 +205,13 @@ class PRServerClient(bb.asyncrpc.AsyncServerConnection):
         return {"readonly": self.server.read_only}
 
 class PRServer(bb.asyncrpc.AsyncServer):
-    def __init__(self, dbfile, read_only=False, upstream=None):
+    def __init__(self, dbfile, read_only=False, upstream=None, on_ready=None):
         super().__init__(logger)
         self.dbfile = dbfile
         self.table = None
         self.read_only = read_only
         self.upstream = upstream
+        self.on_ready = on_ready
 
     def accept_client(self, socket):
         return PRServerClient(socket, self)
@@ -226,6 +227,9 @@ class PRServer(bb.asyncrpc.AsyncServer):
         if self.upstream is not None:
             self.logger.info("And upstream PRServer: %s " % (self.upstream))
 
+        if self.on_ready is not None:
+            self.on_ready()
+
         return tasks
 
     async def stop(self):
@@ -253,16 +257,27 @@ class PRServSingleton(object):
 def run_as_daemon(func, pidfile, logfile):
     """
     See Advanced Programming in the UNIX, Sec 13.3
+
+    func runs in the daemon and is passed a callable to invoke once it is
+    ready to serve. Return the daemon's pid when that happens; raise
+    RuntimeError with the daemon's error if it fails first.
     """
+    readfd, writefd = os.pipe()
     try:
         pid = os.fork()
         if pid > 0:
+            os.close(writefd)
             os.waitpid(pid, 0)
+            with os.fdopen(readfd) as f:
+                report = f.read()
+            if report != "ready":
+                raise RuntimeError(report or "daemon exited before it was ready")
             #parent return instead of exit to give control
             return pid
     except OSError as e:
         raise Exception("%s [%d]" % (e.strerror, e.errno))
 
+    os.close(readfd)
     os.setsid()
     """
     fork again to make sure the daemon is not session leader,
@@ -316,9 +331,27 @@ def run_as_daemon(func, pidfile, logfile):
     with open(pidfile, "w") as pf:
         pf.write("%s\n" % pid)
 
-    func()
-    os.remove(pidfile)
-    os._exit(0)
+    def ready():
+        nonlocal writefd
+        os.write(writefd, b"ready")
+        os.close(writefd)
+        writefd = None
+
+    ret = 0
+    try:
+        func(ready)
+    except Exception as e:
+        import traceback
+        traceback.print_exc()
+        if writefd is not None:
+            os.write(writefd, str(e).encode())
+        ret = 1
+    try:
+        os.remove(pidfile)
+    except FileNotFoundError:
+        # --stop may have removed it already
+        pass
+    os._exit(ret)
 
 def start_daemon(dbfile, host, port, logfile, read_only=False, upstream=None):
     ip = socket.gethostbyname(host)
@@ -335,12 +368,18 @@ def start_daemon(dbfile, host, port, logfile, read_only=False, upstream=None):
         return 1
 
     dbfile = os.path.abspath(dbfile)
-    def daemon_main():
-        server = PRServer(dbfile, read_only=read_only, upstream=upstream)
+    logfile = os.path.abspath(logfile)
+    def daemon_main(ready):
+        server = PRServer(dbfile, read_only=read_only, upstream=upstream, on_ready=ready)
         server.start_tcp_server(ip, port)
         server.serve_forever()
 
-    run_as_daemon(daemon_main, pidfile, os.path.abspath(logfile))
+    try:
+        run_as_daemon(daemon_main, pidfile, logfile)
+    except RuntimeError as e:
+        sys.stderr.write("Failed to start PRServer on %s:%s: %s\nSee %s for details.\n"
+                         % (ip, port, e, logfile))
+        return 1
     return 0
 
 def stop_daemon(host, port):
diff --git a/lib/prserv/tests.py b/lib/prserv/tests.py
index f3b2aae45..f1e4f12c8 100644
--- a/lib/prserv/tests.py
+++ b/lib/prserv/tests.py
@@ -7,6 +7,7 @@
 
 from . import create_server, create_client, increase_revision, revision_greater, revision_smaller, _revision_greater_or_equal
 import prserv.db as db
+from prserv.serv import PIDPREFIX
 from bb.asyncrpc import InvokeError
 import logging
 import os
@@ -383,6 +384,19 @@ class ScriptTests(unittest.TestCase):
     def tearDownClass(cls):
         cls.temp_dir.cleanup()
 
+    def test_0_start_bitbake_prserv_port_in_use(self):
+        with socket.socket() as s:
+            s.bind(("0.0.0.0", 0))
+            s.listen()
+            port = str(s.getsockname()[1])
+            result = subprocess.run([BIN_DIR / "bitbake-prserv", "--start", "-f", self.dbfile, "--port", port],
+                                    capture_output=True, text=True)
+        self.assertNotEqual(result.returncode, 0,
+                            "--start reported success with the port in use")
+        self.assertIn("Failed to start PRServer", result.stderr)
+        self.assertFalse(os.path.exists(PIDPREFIX % ("0.0.0.0", port)),
+                         "pidfile left behind by a server that never came up")
+
     def test_1_start_bitbake_prserv(self):
         try:
             subprocess.check_call([BIN_DIR / "bitbake-prserv", "--start", "-f", self.dbfile, "--port", self.port])
-- 
2.43.0

[PATCH v2 3/3] prserv: add --status to list running servers

[Babanpreet Singh]

Add `prserv --status` to report running servers and stale pidfiles, with lifecycle tests for both running and stopped states.

[YOCTO #10865]

AI-Generated: Uses Claude (claude-fable-5-1)
Signed-off-by: Babanpreet Singh <bbnpreetsingh@gmail.com>
---
 bin/bitbake-prserv  |  7 +++++++
 lib/prserv/serv.py  | 23 +++++++++++++++++++++++
 lib/prserv/tests.py | 18 +++++++++++++++++-
 3 files changed, 47 insertions(+), 1 deletion(-)

diff --git a/bin/bitbake-prserv b/bin/bitbake-prserv
index e666fef06..5775f47fb 100755
--- a/bin/bitbake-prserv
+++ b/bin/bitbake-prserv
@@ -64,6 +64,11 @@ def main():
         action="store_true",
         help="stop daemon",
     )
+    parser.add_argument(
+        "--status",
+        action="store_true",
+        help="list PR servers running on this machine",
+    )
     parser.add_argument(
         "--host",
         help="ip address to bind",
@@ -102,6 +107,8 @@ def main():
         )
     elif args.stop:
         ret=prserv.serv.stop_daemon(args.host, args.port)
+    elif args.status:
+        ret=prserv.serv.status_daemon()
     else:
         ret=parser.print_help()
     return ret
diff --git a/lib/prserv/serv.py b/lib/prserv/serv.py
index 6aba1c3e0..a49faeb4a 100644
--- a/lib/prserv/serv.py
+++ b/lib/prserv/serv.py
@@ -427,6 +427,29 @@ def stop_daemon(host, port):
 
     return 0
 
+def status_daemon():
+    import glob
+    running = 0
+    for pidfile in sorted(glob.glob(PIDPREFIX % ("*", "*"))):
+        root, _ = os.path.splitext(os.path.basename(pidfile))
+        parts = root.split("_")
+        ip, port = "_".join(parts[1:-1]), parts[-1]
+        try:
+            with open(pidfile) as pf:
+                pid = int(pf.readline().strip())
+        except (IOError, ValueError):
+            sys.stderr.write("Ignoring unreadable pidfile %s\n" % pidfile)
+            continue
+        if is_running(pid):
+            print("PRServer running at %s:%s (pid %s)" % (ip, port, pid))
+            running += 1
+        else:
+            print("Stale pidfile %s (pid %s not running)" % (pidfile, pid))
+    if not running:
+        print("No PRServer running.")
+        return 1
+    return 0
+
 def is_running(pid):
     try:
         os.kill(pid, 0)
diff --git a/lib/prserv/tests.py b/lib/prserv/tests.py
index f1e4f12c8..8b78e2496 100644
--- a/lib/prserv/tests.py
+++ b/lib/prserv/tests.py
@@ -403,8 +403,24 @@ class ScriptTests(unittest.TestCase):
         except subprocess.CalledProcessError as e:
             self.fail("Failed to start bitbake-prserv: %s" % e.returncode)
 
-    def test_2_stop_bitbake_prserv(self):
+    def test_2_status_bitbake_prserv(self):
+        result = subprocess.run([BIN_DIR / "bitbake-prserv", "--status"],
+                                capture_output=True, text=True)
+        self.assertEqual(result.returncode, 0,
+                         "Expected a running PRServer: %s" % result.stdout)
+        self.assertIn(":%s (pid " % self.port, result.stdout)
+
+    def test_3_stop_bitbake_prserv(self):
         try:
             subprocess.check_call([BIN_DIR / "bitbake-prserv", "--stop", "--port", self.port])
         except subprocess.CalledProcessError as e:
             self.fail("Failed to stop bitbake-prserv: %s" % e.returncode)
+
+    def test_4_status_stopped_bitbake_prserv(self):
+        result = subprocess.run([BIN_DIR / "bitbake-prserv", "--status"],
+                                capture_output=True, text=True)
+        self.assertNotIn(":%s (pid " % self.port, result.stdout)
+        # Servers unrelated to this test may be running on the machine
+        if "PRServer running at" not in result.stdout:
+            self.assertEqual(result.returncode, 1,
+                             "Expected no running PRServer: %s" % result.stdout)
-- 
2.43.0