Signal Handlers in the Wrong Thread: Reviewing an Agent's Graceful Shutdown PR

A pull request lands at 09:40 titled "add graceful shutdown". It touches three files and adds 212 lines. CI is green, and the agent's summary promises "no more dropped jobs".

The summary is the least reliable artifact in the patch. The diff is the evidence. This review sorts one shutdown patch into three piles: what to trust, what to revert, what to test.

Why shutdown patches attract confident agents

Graceful shutdown looks like a small feature. It is actually a concurrency contract. That contract covers signal handling, in-flight work, and exit codes.

Agents generate plausible shutdown code quickly. Signal registration, a drain loop, and a sleep show up in the right order. The real failure modes sit in details the summary never mentions.

Step 1 — Classify every hunk before running anything

The reviewer reads the diff first and the description second. Each hunk gets a bucket, and each bucket gets a different action.

Bucket	Typical hunks	Action
Trust	New unit tests, type hints, log strings	Merge after a read
Test	Signal registration, drain loop, exit codes, timeouts	Prove with a harness
Revert	Bare except, handler inside a thread, fixed sleep	Remove before merge

Most agent patches contain all three buckets. Treating the PR as one unit is how a swallowed exception reaches production.

Step 2 — Build a worker with observable state

The patch needs a target. A 30-line worker is enough to reproduce the contract. It uses only the standard library.

# worker.py — long-running job runner with cooperative cancellation
import signal
import sys
import time

STOP = False


def _handle(signum, frame):
    global STOP
    STOP = True


signal.signal(signal.SIGTERM, _handle)
signal.signal(signal.SIGINT, _handle)


def job(i: int, seconds: float = 1.0) -> str:
    deadline = time.monotonic() + seconds
    while time.monotonic() < deadline:
        if STOP:
            return f"job-{i}:aborted"
        time.sleep(0.01)
    return f"job-{i}:done"


for i in range(200):
    print(job(i), flush=True)
    if STOP:
        print(f"worker: stopped after {i}", flush=True)
        sys.exit(0)
print("worker: drained", flush=True)

The handler runs in the main thread and only flips a flag. That is the safe pattern. The flag becomes the cancellation point inside the loop.

Step 3 — Write the harness that proves the claim

The test spawns the worker, waits for real work, then sends SIGTERM mid-job. The assertion is about the exit path, not the log text.

# test_shutdown.py — proves the in-flight job resolves before exit
import signal
import subprocess
import sys
import time

proc = subprocess.Popen(
    [sys.executable, "worker.py"],
    stdout=subprocess.PIPE,
    text=True,
    bufsize=1,
)

first = proc.stdout.readline().strip()
assert first.endswith(":done"), first

time.sleep(0.3)                 # land inside the next job
proc.send_signal(signal.SIGTERM)

rest, _ = proc.communicate(timeout=10)   # TimeoutExpired means the bug is real
lines = [line for line in rest.splitlines() if line]

assert any(":aborted" in line or ":done" in line for line in lines), lines
assert proc.returncode == 0, proc.returncode
print("PASS: in-flight job resolved, exit code 0")

A patched worker that ignores SIGTERM hangs here. communicate(timeout=10) raises, and the review has its first hard fact.

Step 4 — Revert three hunks on sight

The patch contains three suspicious hunks. All three appear in real agent output, and all three are revertable without discussion.

# Hunk 4 — outcome swallowed
try:
    drain()
except Exception:
    pass          # revert: hides partial writes and exits 0 anyway

# Hunk 7 — handler registered from a worker thread
threading.Thread(
    target=lambda: signal.signal(signal.SIGTERM, _handle)
).start()
# revert: ValueError, signal registration is main-thread only

# Hunk 9 — constant exit latency
time.sleep(30)    # revert: replaces a drain bound with a fixed wait

The second hunk fails loudly and is therefore harmless in review. The first and third fail quietly, which is why they need explicit attention.

Step 5 — Ask for a bounded, idempotent drain

The fixed version tracks in-flight work and caps the wait. A second signal escalates instead of being ignored.

INFLIGHT: list[str] = []
STOP = False


def drain(deadline_s: float = 5.0) -> list[str]:
    end = time.monotonic() + deadline_s
    finished: list[str] = []
    while INFLIGHT and time.monotonic() < end:
        finished.append(INFLIGHT.pop())
        time.sleep(0.01)
    return finished


def _handle(signum, frame):
    global STOP
    if STOP:                     # second signal: stop cooperating
        raise SystemExit(1)
    STOP = True

Both properties are testable. Repeat SIGTERM twice in the harness and assert exit code 1. That test is short and worth keeping permanently.

Step 6 — Verify the test fails on the old code

A passing test proves nothing until it fails on the unpatched branch. This is the mutation check, and it takes one command sequence.

python test_shutdown.py          # on main: expect TimeoutExpired
git apply agent-shutdown.patch
python test_shutdown.py          # expect PASS, exit code 0
git checkout -b review/shutdown

If the test passes on main, it is not testing shutdown. The agent likely wrote an assertion about log text instead of exit behavior.

Step 7 — Use a hosted model as a second reader

Runtime behavior needs local execution. Static opinion does not. A hosted model reads the diff well and the process tree badly.

MonkeyCode offers free model access plus a free server option, per the project's supplied availability claims. The useful move is one narrow prompt, not a general review request.

Diff below. List every code path that can exit the process
without running the drain loop. Cite line numbers only.

Disclosure: This article was prepared as part of MonkeyCode's product outreach.

The free server option covers the harness run when the local machine cannot hold a long-lived process. Availability terms and allowances change, so confirm the current terms on the project's site before planning around them.

What this workflow does not cover

• Child processes. The harness checks one PID and says nothing about grandchildren.
• SIGKILL. No handler runs after SIGKILL, and no patch changes that.
• Orchestrator timeouts. Kubernetes preStop and termination grace periods need cluster-level tests.
• Partial writes. Draining a queue and committing a transaction are different problems.

Who should skip this approach

Skip the harness if the service holds no in-flight state. Stateless request handlers have nothing to drain, and the test adds maintenance for free.

Skip the hosted second reader if the diff contains regulated code. The classification step, revert rules, and mutation check still apply without any model.

Closing note

A shutdown patch is reviewed hunk by hunk, not summary by summary. The three piles are the whole method: trust the boring parts, revert the quiet failures, test the contract.

When a free server run reproduces a hang in under a minute, paste that failing output beside the diff. Evidence beats a confident PR description every time.