Adding handshake websocket by marcosfrenkel · Pull Request #114 · PublicQuantumNetwork/pqn-node

marcosfrenkel · 2025-09-04T23:31:42Z

Adding a new routes file called "coordination" in charge of handling the coordination between nodes. At the moment this is what it does:

reset_coordination_state: New call that the frontend should call when loading the main page. Makes sure all the following and leading state are reset and clean for a new interaction (if frontend is in the main page, it is not following or leading anyone).
collect_follower: Leader node call to make it look for a follower in a separate node. Not much use for this right now, but that is how it would like. Mostly have been used for development.
follow_requested: follower node called by a leader requesting that it follows it. Checks if the frontend is connected and ready to accept a follow request (this can only happen in the main page right now). This is done by a flag (state.client_listening_for_follower_requests) that changes when the websocket gets connected or not. If the client is ready to accept a follow, a second flag (state.following_requested = True) changes and an event gets triggered that makes the websocket send the message to the front end.
follow_requested_alert: Websocket that detects when the frontend connects, this changes the flag ``state.client_listening_for_follower_requeststoTrue` to indicate that the client is ready to follow. Sends a message to the client when a follow request comes, receives the response from the client and detects when the websocket gets disconnected and changes the flag back to `False`

Benjamin-Nussbaum

Change the global state object to be an injected dependency with Depends. Maybe we can think about better endpoint locations/naming/etc, but generally it seems ok.

Benjamin-Nussbaum · 2025-09-11T16:04:45Z

+    # Leader's state
+    leading: bool = False
+    followers_address: str | None = None
+
+    # Follower's state
+    following: bool = False
+    # Other node requested this node to follow it.
+    following_requested: bool = False
+    # User's response to the follow request. None if no response yet, True if accepted, False if rejected.
+    following_requested_user_response: bool | None = None
+    # The address of the leader this node is following. None if not following anyone.
+    leaders_address: str | None = None
+    leaders_name: str | None = None


Remove "or none" fields. If we want to account content differences in requests/responses, we can add extra models.

Suggested change

# Leader's state

leading: bool = False

followers_address: str | None = None

# Follower's state

following: bool = False

# Other node requested this node to follow it.

following_requested: bool = False

# User's response to the follow request. None if no response yet, True if accepted, False if rejected.

following_requested_user_response: bool | None = None

# The address of the leader this node is following. None if not following anyone.

leaders_address: str | None = None

leaders_name: str | None = None

# Leader's state

leading: bool = False

followers_address: str = ""

# Follower's state

following: bool = False

# Other node requested this node to follow it.

following_requested: bool = False

# User's response to the follow request. None if no response yet, True if accepted, False if rejected.

following_requested_user_response: bool = False

# The address of the leader this node is following. None if not following anyone.

leaders_address: str = ""

leaders_name: str = ""

marcosfrenkel · 2025-09-11T18:15:57Z

I moved all the coordination flags into its own model, and made it a dep. I only left None for following_requested_user_response Since None indicate no reply from the user, True indicates accept and False means reject the request. Could you give it another look @Benjamin-Nussbaum

Benjamin-Nussbaum

A few comments, otherwise looks fine modulo general cleanup.

Benjamin-Nussbaum · 2025-09-11T20:14:54Z

+    # Check if the client is ready to accept a follower request and that node is not already following someone.
+    if coord.client_listening_for_follower_requests and not coord.following:
+        coord.following_requested = True
+        coord.leaders_name = leaders_name
+        coord.leaders_address = leaders_address
+        # Trigger the state change to get the websocket to send question to user
+        ask_user_for_follow_event.set()
+
+        logger.debug("Asking user to accept follow request from %s (%s)", leaders_name, leaders_address)
+        await user_replied_event.wait()  # Wait for a state change event to see if user accepted
+        user_replied_event.clear()  # Reset the event for the next change
+        if coord.following_requested_user_response:
+            logger.debug("Follow request from %s accepted.", leaders_address)
+            coord.following = True
+            coord.leaders_name = leaders_name
+            coord.leaders_address = leaders_address
+            ask_user_for_follow_event.set()
+            return True
+
+        logger.debug("Follow request from %s rejected.", leaders_address)
+        # Clean up the state if user rejected
+        coord.leaders_address = ""
+        coord.leaders_name = ""
+        coord.following_requested = False
+
+    else:
+        logger.info(
+            "Request rejected because %s",
+            (
+                "client is not listening for requests"
+                if coord.client_listening_for_follower_requests
+                else "this node is already following someone"
+            ),
+        )


I suggest inverting the logic here to reduce the indent level and keep the error handling close to the condition.

Benjamin-Nussbaum · 2025-09-11T20:26:08Z

+async def get_coordination_state() -> AsyncGenerator[CoordinationState, None]:
+    yield state.coordination_state


Some suggestions:

Put this function in the same module as the class definition

doesn't need to be a generator

probably doesn't need to be async

rather than a whole separate dependency for a sub-component of the overall state (potentially opening the door for additional complexity via more separate things for other sub-components), just depend on the overall state object, and use the component - getting the whole thing isn't that much more overhead, and it keeps the style consistent across endpoints.

Benjamin-Nussbaum · 2025-09-11T20:29:26Z

    qkd_request_bit_list: list[int] = []


 state = NodeState()


This state object should be returned in a get_state() function in this file as mentioned in my other comment.

check it isn't imported directly for use in any endpoints

Benjamin-Nussbaum · 2025-09-12T16:27:42Z

+    ret = await http_client.post(f"http://{address}/coordination/follow_requested?leaders_name={settings.node_name}")
+    if ret.status_code != status.HTTP_200_OK:
+        raise HTTPException(status_code=ret.status_code, detail=ret.text)
+
+    if ret.json() is True:
+        coord.leading = True
+        coord.followers_address = address
+        logger.info("Successfully collected follower")
+        return True
+    if ret.json() is False:
+        logger.info("Follower rejected follow request")
+        return False


Also, this section could be clearer if follow_requested returned a structured response rather than a raw bool. (either a small dict[str,bool] with a meaningful key, or a more robust ResponseModel

marcosfrenkel · 2025-09-17T18:27:19Z

@Benjamin-Nussbaum, I implemented all the changes let me know if anything else needs changing. I would hold on the merge though since. I wasn't able to test the code properly

Benjamin-Nussbaum

Looks fine - we can discuss later about other possible changes

Benjamin-Nussbaum · 2025-09-24T22:02:23Z

+                    try:
+                        await websocket.send_text(
+                            f"Do you want to accept a connection from {state.leaders_name} ({state.leaders_address})?"
+                        )
+                    except RuntimeError as e:
+                        if "websocket.close" in str(e) or "response already completed" in str(e):
+                            logger.debug("WebSocket already closed, cannot send message")
+                            break
+                        raise


Multiple nested try blocks adds confusion. Rather than just trying to send text and handling an exception if the socket is closed, I suggest checking if the socket is closed before attempting to send.

marcosfrenkel · 2025-11-20T20:50:38Z

New changes incoming in different PR

marcosfrenkel force-pushed the basic_websocket branch from 7659cb6 to d88a0e2 Compare September 4, 2025 23:31

Benjamin-Nussbaum requested changes Sep 11, 2025

View reviewed changes

Benjamin-Nussbaum reviewed Sep 12, 2025

View reviewed changes

Benjamin-Nussbaum previously approved these changes Sep 17, 2025

View reviewed changes

marcosfrenkel dismissed Benjamin-Nussbaum’s stale review via 34281a3 September 24, 2025 21:50

Benjamin-Nussbaum requested changes Sep 24, 2025

View reviewed changes

marcosfrenkel added 9 commits October 15, 2025 18:02

First version of working websockets

96de00a

Implements basic state handling, as well as client disconnects detection

aa14812

cleans up code and deals with ruff and mypy

d782a75

Moved all coordination state into its own model and dep

09a0616

Implemented feedback to coordination.py

413f199

Removed CoordinationState object

8131525

State is now used through a dep

08cf41c

mypy and ruff fixes

b4bfac8

Minor bugfix

64d8884

marcosfrenkel force-pushed the basic_websocket branch from 34281a3 to 64d8884 Compare October 15, 2025 23:24

marcosfrenkel added 2 commits October 15, 2025 18:31

ruff mypy checks

26d73c4

Checking for websocket connection before sending message

cd72572

marcosfrenkel closed this Nov 20, 2025

marcosfrenkel deleted the basic_websocket branch December 15, 2025 19:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding handshake websocket#114

Adding handshake websocket#114
marcosfrenkel wants to merge 11 commits into
PublicQuantumNetwork:masterfrom
marcosfrenkel:basic_websocket

marcosfrenkel commented Sep 4, 2025

Uh oh!

Benjamin-Nussbaum left a comment

Uh oh!

Benjamin-Nussbaum Sep 11, 2025

Uh oh!

marcosfrenkel commented Sep 11, 2025

Uh oh!

Benjamin-Nussbaum left a comment

Uh oh!

Benjamin-Nussbaum Sep 11, 2025

Uh oh!

Benjamin-Nussbaum Sep 11, 2025

Uh oh!

Benjamin-Nussbaum Sep 11, 2025

Uh oh!

Benjamin-Nussbaum Sep 12, 2025

Uh oh!

marcosfrenkel commented Sep 17, 2025

Uh oh!

Benjamin-Nussbaum left a comment

Uh oh!

Benjamin-Nussbaum Sep 24, 2025

Uh oh!

marcosfrenkel commented Nov 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		async def get_coordination_state() -> AsyncGenerator[CoordinationState, None]:
		yield state.coordination_state

Conversation

marcosfrenkel commented Sep 4, 2025

Uh oh!

Benjamin-Nussbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Benjamin-Nussbaum Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

marcosfrenkel commented Sep 11, 2025

Uh oh!

Benjamin-Nussbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Benjamin-Nussbaum Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

Benjamin-Nussbaum Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

Benjamin-Nussbaum Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

Benjamin-Nussbaum Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

marcosfrenkel commented Sep 17, 2025

Uh oh!

Benjamin-Nussbaum left a comment

Choose a reason for hiding this comment

Uh oh!

Benjamin-Nussbaum Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

marcosfrenkel commented Nov 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants