Skip to content

Clean up gateway lifecycle processes - #304

Closed
brookerslyn wants to merge 2 commits into
Phineas:mainfrom
brookerslyn:hmmm
Closed

brookerslyn wants to merge 2 commits into
Phineas:mainfrom
brookerslyn:hmmm

Conversation

@brookerslyn

Copy link
Copy Markdown
Contributor

discordbot was starting the gateway with start_link and also monitoring it, which meant an abnormal gateway exit could kill the bot before the existing :DOWN handler got a chance to restart/resume it

for example, if the discord websocket drops and the gateway exits with something like {:remote, :closed}, the existing monitor/restart path could get skipped because discordbot was still linked to that process

this fix makes that path actually work by trapping exits and ignoring stale lifecycle messages from old gateway pids

it also cleans up the heartbeat/sequence tracker when the gateway closes, and makes heartbeat stop when its socket owner dies, so reconnects don't leave old heartbeat processes hanging around

tested locally with a small lifecycle repro to confirm old heartbeat processes no longer stay alive after gateway exit

forgot to add docs to the code so i had to make a separate commit for it, sorry dustin, no clean history

@pxseu

pxseu commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

I get what you're trying to solve here but this is pretty clearly heavily AI assisted, the defensive programming gives it away. There are three overlapping mechanisms for each problem: link + monitor + trap_exit in DiscordBot, and cleanup in close_gateway + cleanup in websocket_terminate + a socket monitor in Heartbeat. Some of these layers never even run. In websocket_client 1.6.1 the handler close paths go through handle_response({close, ...}) -> {stop, :normal} and websocket_terminate is never called there, and a stale :DOWN from an old gateway pid can't happen because a monitor only fires once.

The issues are real but manually managing process lifetimes from callbacks with GenServer.stop and Process.alive? checks is not how supervisors and GenServers are meant to be used. Teardown is the job of links and the supervision tree. I removed exactly this kind of code before in ccf795c ("remove linked manual kills") and that was on purpose. The current design is intentional: when the connection is broken (wrong gateway, no hello, stale heartbeat) we kill the socket, the client process dies and DiscordBot restarts it. On abnormal exits the links take the heartbeat and seq agent down with the gateway and everything comes back through the tree.

The one valid thing here is that the intentional close paths (op reconnect / invalid session / stale heartbeat) exit with :normal, which links don't propagate, so the heartbeat and seq agent from that cycle stay alive. But the fix for that is to stop exiting :normal on those paths, not to add cleanup around it. Have the client exit({:shutdown, reason}) instead of returning {:close, ...} (the lib pins handler-returned closes to :normal, so it takes an explicit exit). Then the links tear down the heartbeat and seq agent, the bot goes down the same way it already does for remote closes, and the supervisor restarts everything through one single path. No monitor, no :DOWN handler, no trap_exit, no cleanup helpers. We lose nothing on the wire either: the lib doesn't even prepend a status code to its close frames (encode_frame just masks the payload as-is), so the current "clean" closes aren't valid RFC 6455 closes anyway, and Discord explicitly wants a non-1000 close or a plain TCP drop when the session should be resumable.

Happy to look at a version that goes that direction. As-is I don't want lifecycle micromanagement merged back into the callbacks.

@brookerslyn

Copy link
Copy Markdown
Contributor Author

regarding the AI-assisted part: no, this wasn't AI-written. i used the websocket client source and some docs on the web, but that's it.

that said, the point about the lifecycle layering is fair. my last PR got left unmerged because the fix was too narrow, so i overcorrected here trying to make sure reconnect paths didn't leave anything behind.

the leak/stale-process issue is what i was trying to address, but i can see why trap_exit + manual cleanup + extra monitoring is more lifecycle micromanagement than this codebase wants

i guess i'll correct the pr and drop the callback level cleanup then, thanks for the feedback

@brookerslyn brookerslyn closed this Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants