network: terminate live CDP connections on shutdown

[navidemad]

What

lightpanda serve ignored a single SIGTERM while a CDP connection was live — the process only exited if the socket was closed before the signal, or after a third signal. A conventional one-shot graceful stop (kill -TERM <pid> then waitpid) hung after the client had driven a session.

Closes #2510.

Root cause

On shutdown the sighandler runs Network.stop (sets shutdown, lets the run loop exit) before Server.shutdown. A live CDP worker parks in curl_multi_poll and is woken only by the Network thread via dropCdp -> handles.wakeup(). Once the run loop exits with links still live, nothing can wake those workers, so Server.deinit()'s while (active_threads > 0) loop spins forever.

flowchart TD
    A1[SIGTERM] --> A2["Network.stop: shutdown = true"]
    A2 --> A3{"run loop: shutdown and running_handles == 0"}
    A3 -->|break| A4["Network thread exits<br/>(live CDP links abandoned)"]
    A4 --> A5["Server.shutdown: conn.shutdown()"]
    A5 --> A6["worker still parked in curl_multi_poll<br/>(nothing left to wake it)"]
    A6 --> A7["Server.deinit: while active_threads &gt; 0<br/>spins forever"]

Loading

Change

Network.run, on observing shutdown, now calls a new shutdownCdpLinks() that drops every still-live CDP link via the existing peer-EOF path (dropCdp(notify=true) — pushes .disconnect into the worker's inbox and wakes it out of curl_multi_poll). The worker's cdp.tick() then returns false and it exits before the loop breaks. The helper is idempotent (a no-op once the links are drained).

flowchart TD
    B1[SIGTERM] --> B2["Network.stop: shutdown = true"]
    B2 --> B3["run loop: shutdownCdpLinks()<br/>dropCdp(notify=true) per live link"]
    B3 --> B4["worker woken: .disconnect in inbox<br/>+ curl_multi_wakeup"]
    B4 --> B5["cdp.tick() returns false<br/>worker exits, active_threads -&gt; 0"]
    B3 --> B6[run loop breaks, Network exits]
    B5 --> B7["Server.deinit returns cleanly"]

Loading

Test plan

• zig build test — 717/717 pass.
• End-to-end (ReleaseFast build): the reproducer below — a raw CDP client that opens a WebSocket and sends one SIGTERM with the connection still open — now exits cleanly; before the fix it stayed alive 12s+ until SIGKILL. Closing the socket before SIGTERM already worked and still does.

No in-process unit test: the shutdown path is multi-threaded (sighandler thread → Network run loop → per-connection worker), and the without-fix failure mode is a Server.deinit() spin rather than an assertion. The link's lifetime is guarded by unregisterCdp's synchronous handoff on the Network thread, so dropping links from any other thread (e.g. a test) risks a use-after-free on cdp_poll_snapshot. A race-free test would need a dedicated Server/Network instance plus a watchdog — happy to add that if you'd like it in the suite.

Reproducer

Self-contained (Lightpanda + Python stdlib, no extra deps), telemetry disabled to show this is independent of #2509:

import socket, base64, os, sys, subprocess, time, signal

BIN = sys.argv[1]
print("binary:", subprocess.run([BIN, "version"], capture_output=True, text=True).stdout.strip())

def ws_connect(port):
    s = socket.create_connection(("127.0.0.1", port), timeout=5)
    key = base64.b64encode(os.urandom(16)).decode()
    s.sendall(("GET / HTTP/1.1\r\nHost: x\r\nUpgrade: websocket\r\nConnection: Upgrade\r\n"
               "Sec-WebSocket-Key: %s\r\nSec-WebSocket-Version: 13\r\n\r\n" % key).encode())
    buf = b""
    while b"\r\n\r\n" not in buf:
        buf += s.recv(1)
    assert buf.startswith(b"HTTP/1.1 101"), buf[:40]
    return s

def run(label, close_before_sigterm, port):
    env = dict(os.environ); env["LIGHTPANDA_DISABLE_TELEMETRY"] = "true"
    p = subprocess.Popen([BIN, "serve", "--host", "127.0.0.1", "--port", str(port), "--log_level", "error"],
                         stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, env=env, start_new_session=True)
    time.sleep(2)
    s = ws_connect(port)               # one CDP WebSocket; no commands sent
    if close_before_sigterm:
        s.close(); time.sleep(0.3)
    p.send_signal(signal.SIGTERM)      # exactly ONE SIGTERM
    deadline = time.time() + 12
    while time.time() < deadline:
        if p.poll() is not None:
            print("[%s] exited on SIGTERM (ok)" % label); break
        time.sleep(0.25)
    else:
        print("[%s] *** SIGTERM IGNORED — still alive 12s after one SIGTERM ***" % label)
        os.killpg(os.getpgid(p.pid), signal.SIGKILL)
    try: s.close()
    except OSError: pass
    try: p.wait(timeout=3)
    except Exception:
        try: os.killpg(os.getpgid(p.pid), signal.SIGKILL)
        except Exception: pass

run("WS closed before SIGTERM", True, 39920)
run("WS still open at SIGTERM", False, 39921)

Before this PR the second case prints SIGTERM IGNORED; after it, both print exited on SIGTERM (ok).

[karlseguin]

@krichprollsch mind reviewing my changes? They are somewhat independent but serve the same purpose. The original commit looks good, but, for me, it wasn't enough to cleanly stop under all circumstances. Only the two sets of changes combined were enough.

[navidemad]

Thanks @karlseguin — good catch, I'd missed the syncRequest path. Agreed the two changes are complementary, not redundant. Writing out the split for @krichprollsch's review:

• My commit handles the idle worker — parked at the top-level perform/poll. shutdownCdpLinks → dropCdp(notify=true) pushes a .disconnect and wakes it, so drainInbox returns error.ClientDisconnected and the worker exits.
• Your commit handles the busy worker — mid-syncRequest. The .disconnect is drained inside the .sync_wait loop and error.ClientDisconnected propagates out of syncRequest, but the JS caller treats it as a failed fetch rather than a fatal disconnect. The worker resumes, re-parks with the link already dropped and nothing left to wake it. The terminated latch makes that one-shot error sticky so the next tick/syncRequest exits cleanly.

So: mine delivers + wakes, yours makes the signal survive a swallowed error. Both needed. (Your half also fixes this outside shutdown — a client disconnecting mid-synchronous-fetch would leak its worker thread the same way, and on a single-connection server wedge the next connection.)

I checked the latch's safety since it's never cleared: terminated is only touched on the worker thread (the Network thread only touches inbox.queue under the mutex, so no atomic needed), and it can't poison the next pooled connection because cdp.init → browser.init → http_client.init rebuilds the inbox fresh (.inbox = .{}).

One real issue in Capture disconnect/close in Worker: the early return in syncRequest (HttpClient.zig:543) returns before self.request(r, null), which takes ownership of req.headers and frees the curl_slist on every path. Callers (Frame.zig external stylesheet, ScriptManager blocking scripts, worker importScripts) rely on request() to free them — so after a latched disconnect, any nested fetch leaks its header list. I pushed a one-line fix mirroring request()'s own failure paths:

if (self.inbox.terminated) {
    req.headers.deinit();
    return error.ClientDisconnected;
}

Tests: I pushed a tick-latch regression test (fails against the pre-latch HttpClient.zig and passes with it) and a behavioral syncRequest-after-disconnect test. The header leak above isn't directly assertable — curl_slist is C-allocated through the tracking allocator, outside the per-test leak check — so that one's verified by review. Full suite green (719/719).

One thing, take or leave: the now-unconditional cdp_unregister.broadcast() — is that closing a specific missed-wakeup race, or defensive? Harmless either way; a one-line comment naming the race would help future readers if there is one.

@krichprollsch over to you for the combined diff.

[krichprollsch]

One non-blocking comment

[krichprollsch]

maybe it's silly and I see we do this way on other loops, but why not directly having it = node.next;?

[navidemad]

Not silly at all. The direct form isn't safe here because this loop removes nodes while it iterates, so reading node.next after dropCdp breaks two ways:

1. dropCdp calls self.cdp_links.remove(&link.node) (Network.zig:535). Zig's DoublyLinkedList.remove doesn't clear the removed node's own next (it just relinks the neighbors), so node.next still points at the old successor and we'd keep walking a node that's already out of the list.
2. With notify=true, dropCdp also wakes the worker, which can then exit and free the CdpLink that node lives inside. By the time the iteration resumes, node itself may be gone, so it's a real use-after-free and not just a stale read.

Grabbing next before the dropCdp call avoids both. The loops that advance with : (it = node.next) directly (like prepareCdpPollFds, Network.zig:571) only read the list and never drop nodes, so they're fine. This one matches the processCdpEvents first pass (Network.zig:601), which removes nodes for the same reason.

[karlseguin]

Yes, but you can grab next BEFORE the drop INTO it

while (it) |node| {
    it = node.next;
    const link: *CdpLink = @fieldParentPtr("node", node);
    if (link.state == .live) {
        self.dropCdp(link, null, true);
    }
}