Skip to content

fix: only clean up worker resources this process created - #277

Open
hamodywe wants to merge 1 commit into
supabase:masterfrom
hamodywe:fix/276-worker-exit-cleanup
Open

fix: only clean up worker resources this process created#277
hamodywe wants to merge 1 commit into
supabase:masterfrom
hamodywe:fix/276-worker-exit-cleanup

Conversation

@hamodywe

Copy link
Copy Markdown

Fixes #276.

The problem

net_on_exit() is registered at worker.c:251, before BackgroundWorkerInitializeConnection() at 258 and long before event_monitor() / curl_multi_init() at 270/276 — but it closed worker_state->epfd and freed worker_state->curl_mhandle unconditionally.

Both fields live in WorkerState (core.h:17-18), which is shared memory, so they outlive the process that created them. A worker terminated during startup — its database dropped, datallowconn = false, or a pg_terminate_backend() racing the 1 s restart — therefore ran the cleanup on an fd number and a heap pointer belonging to the previous worker. The double free is a SIGSEGV, and the postmaster answers a background worker crash by restarting the entire cluster into recovery.

One consequence beyond the report: ev_monitor_close() also does close(timerfd), and timerfd is static int timerfd = 0 (event.c:13). In a process that never created one it is still 0, so the same path closes fd 0.

The change

Track how far this process got through startup, and unwind only that far.

The stage is process-local rather than another WorkerState field, which matters for a case the report doesn't need but which exists anyway: a worker SIGKILLed without ever running the callback leaves the shared fields populated, so resetting them to sentinels on exit would still leave the next worker trusting stale values. A fresh process starts at NONE regardless of how the last one died.

The shared fields are still reset after a real cleanup, so they never hold a freed pointer.

Verification

I could not build or run this — pg_net's dev environment is Nix-based and this is a Windows machine, so CI is the real check. What I could do was model the two exit paths standalone, driving the same sequence (worker A starts fully and exits; worker B restarts and dies during startup) against the old and new net_on_exit:

BEFORE - cleanup runs unconditionally
   worker A owned epfd=4 timerfd=3
   worker B closed 2 fd(s): 4 0
   worker B freed A's curl handle: YES - double free, this is the SIGSEGV

AFTER  - cleanup guarded by worker_init_stage
   worker A owned epfd=4 timerfd=3
   worker B closed 0 fd(s):
   worker B freed A's curl handle: no

That exercises the control flow and the shared-state lifetime, not the real curl or epoll calls — it is evidence about the ordering bug, not a substitute for the suite.

No test is included. The reporter measured the race at 6 crashes in 16 CI runs, so a test built on that timing would be flaky. A deterministic one looks possible — point pg_net.database_name at a database that does not exist, reload, and terminate the worker so the restart always fails inside BackgroundWorkerInitializeConnection — but I would be guessing at whether it survives your harness, since I cannot run it. Happy to add it if you want that shape, or if you would rather it went in test_worker_behavior.py next to test_worker_will_not_block_drop_database (which, as the issue notes, drops foo and so never terminates the worker connected to postgres).

AI disclosure: this change was prepared with the assistance of Claude Code. I reviewed the change and the analysis before opening, and I stand behind them as my own.

net_on_exit() is registered before BackgroundWorkerInitializeConnection(),
but it closed worker_state->epfd and freed worker_state->curl_mhandle
unconditionally. Both fields live in shared memory, so they outlive the
process that created them: a worker terminated during startup - its database
dropped, datallowconn false, or a pg_terminate_backend racing the restart -
ran the cleanup on an fd number and a heap pointer belonging to the previous
worker. The double free is a SIGSEGV, which the postmaster treats as a
backend crash and answers by restarting the whole cluster.

ev_monitor_close() also closes the file-scope `timerfd`, which is 0 in a
process that never created one, so the same path closes fd 0.

Track how far this process got and unwind only that far. The stage lives in
process-local storage, so it is NONE in a fresh worker even when a previous
one was killed without running the callback at all.

Fixes supabase#276

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant