feat: add extension-to-BrowserManager finalize ack#1131
feat: add extension-to-BrowserManager finalize ack#1131vringar wants to merge 1 commit intorefactor/event-driven-completionfrom
Conversation
c644753 to
c362b94
Compare
b896b91 to
6d496a1
Compare
There was a problem hiding this comment.
Pull request overview
This PR replaces a fixed sleep in the FinalizeCommand with a bidirectional acknowledgment from the WebExtension. Instead of sleeping for a fixed duration, Python now waits for the extension to send a FinalizeAck response after processing the Finalize message, improving reliability and reducing unnecessary wait time.
Changes:
- Added bidirectional socket communication support by implementing response functionality in the privileged sockets API
- Extension now sends
FinalizeAckafter processing Finalize messages - Python's
FinalizeCommandwaits for acknowledgment with configurable timeout instead of using fixed sleep
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| openwpm/socket_interface.py | Added receive() and _receive_bytes() methods to read acknowledgment messages from the socket |
| openwpm/commands/browser_commands.py | Replaced fixed sleep with receive() call to wait for finalize acknowledgment with timeout fallback |
| Extension/src/types/browser.d.ts | Added TypeScript type definitions for ConnectionId and sendResponse API |
| Extension/src/socket.ts | Modified to pass connectionId and create respond function for sending responses |
| Extension/src/loggingdb.ts | Added logic to send FinalizeAck response after processing Finalize message |
| Extension/bundled/privileged/sockets/schema.json | Added schema definition for sendResponse function and connectionId parameter |
| Extension/bundled/privileged/sockets/api.js | Implemented bidirectional socket support with output streams and connection tracking |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| data += chunk | ||
| return data | ||
|
|
||
| def receive(self, timeout: float = 5.0) -> Any: |
There was a problem hiding this comment.
The default timeout of 5.0 seconds in receive() is inconsistent with how it's called in FinalizeCommand, where the timeout is explicitly passed based on testing mode. Consider removing the default value to make timeout specification mandatory and prevent accidental misuse with an unintended default.
| def receive(self, timeout: float = 5.0) -> Any: | |
| def receive(self, timeout: float) -> Any: |
| self.visit_id, | ||
| ack, | ||
| ) | ||
| except Exception: |
There was a problem hiding this comment.
Catching bare Exception is too broad and will mask unexpected errors like KeyboardInterrupt or programming errors. Catch specific exceptions like socket.timeout or RuntimeError that are expected from the receive() operation.
| // Abnormal close, let's log the error. | ||
| console.error(e); | ||
| } | ||
| gManager.connectionMap.delete(connectionId); |
There was a problem hiding this comment.
Connection cleanup only happens when the socket closes abnormally. Normal connection closure (when available bytes becomes 0) should also delete the connectionId from the map to prevent memory leaks from accumulating stale connections.
Summary
Replaces the fixed sleep in
FinalizeCommandwith a bidirectional ack from the WebExtension. When the extension finishes processing a Finalize message (forwarding to StorageController, clearing visitID), it sends aFinalizeAckresponse back on the same TCP connection. Python reads this ack instead of sleeping.outputStreamon accepted connections, addsendResponse()API, passconnectionIdthroughonDataReceivedFinalizeAckafter processing Finalize inloggingdb.tsreceive()method toClientSocketinsocket_interface.pyFinalizeCommandwaits for ack with timeout fallback (0.5s test / 5s prod)Stacked on #1128.