feat: add extension-to-BrowserManager finalize ack

[vringar]

Summary

Adds a bidirectional channel on the existing extension control socket so the
WebExtension can send responses back to BrowserManager, and uses it to
replace FinalizeCommand's fixed pre-Finalize sleep with an explicit
FinalizeAck from the extension.

This is the extension→BrowserManager back-channel discussed as option (b)
under #1172 / #1173: a general-purpose reply path a follow-up can reuse to
route JS instrumentation failures (and other extension-side signals) directly
to BrowserManager.

Note on history: this revision is a substantial rewrite of the original
WIP. The review below explains what changed and why; the earlier
implementation should not be used as a reference.

Background

The control socket (Initialize / Finalize, Python → extension) was
send-only. Per-visit teardown worked like this:

tab_restart_browser(webdriver)   # close the visited tab
time.sleep(self.sleep)           # fixed 5s grace (FinalizeCommand(sleep=5))
extension_socket.send(Finalize)  # extension clears visit_id

The sleep exists because closing the tab does not immediately stop data
collection — events captured just before teardown are still in flight
(content-script→background messaging, late webRequest callbacks). The sleep
runs before Finalize, so visit_id stays set in the extension during it
and those stragglers are still attributed to the correct visit. After the
sleep, Finalize clears visit_id.

Two problems: it always costs the full fixed interval, and BrowserManager
gets no confirmation that the extension actually finished.

Why the first cut of this PR was wrong

The initial implementation sent Finalize immediately (no grace) and had
the extension clear visit_id and ack right away. That ack only confirmed the
control-socket round trip — it did not wait for stragglers, and by
removing the grace it made any event arriving after Finalize get tagged
visit_id = -1. For a measurement framework that is a worse regression than
the slowness it set out to fix. The redesign below keeps the grace's
guarantee.

Design

The grace period moves into the extension, where the stragglers actually
are. On Finalize, the extension keeps visit_id set, waits out a grace
period, then flushes meta_information, clears the visit, and only then
replies FinalizeAck. BrowserManager passes the grace duration in the
Finalize message and blocks on the ack.

This is strictly better than the old Python-side sleep: the wait now happens
inside the JS event loop that drains the in-flight events, instead of in an
idle Python time.sleep() in a different process. The ack is now meaningful —
it means "this visit is fully finalized and handed off to storage" — which is
exactly the signal the #1173 follow-up needs. Behaviour for real crawls is
unchanged (same grace, same attribution); the test suite uses a short grace.

Ack-timeout handling

Because a visit is now considered done only once its FinalizeAck arrives, the
timeout path carries real meaning: if the extension does not reply within the
grace + FINALIZE_ACK_MARGIN window, the visit's data may be incomplete.
Rather than silently proceeding (which would leave an incomplete visit
indistinguishable from a clean one in the data), the timeout is now surfaced as
a distinct, non-fatal outcome:

• FinalizeCommand raises FinalizeAckTimeout when the deadline is hit.
• BrowserManager maps that to a dedicated FINALIZE_INCOMPLETE command
  status and, on it, marks the visit unsuccessful
  (finalize_visit_id(success=False)) and increments failure_count — but
  does not set restart_required. A merely-slow drain is not a crash and
  shouldn't pay the browser-restart cost.
• The increment still feeds the existing failure-limit safety, so a sustained
  run of ack timeouts trips the normal ExceedCommandFailureLimit abort rather
  than being ignored.

This keeps the crawl moving on a one-off slow finalize while ensuring an
unacknowledged visit is recorded as unsuccessful and counted, not silently
passed. The crawl_history row for the FinalizeCommand records the
finalize_incomplete status, so the timeout is auditable after the fact.

The command-level timeout for FinalizeCommand is derived from the same
constant as the internal blocking bound and is kept strictly greater than it,
so the manager watchdog cannot kill a browser that is healthily waiting out its
own ack window.

Changes

Privileged socket API (Extension/bundled/privileged/sockets/{api.js,schema.json})

• Track accepted connections in a connectionMap (connectionId → streams).
• sockets.sendResponse(connectionId, data, json) writes a framed reply on
  the originating connection.
• sockets.onDataReceived also passes the connectionId to listeners.
• The reply output stream is opened blocking (OPEN_BLOCKING), and is
  closed on disconnect rather than leaked.
• sendData and sendResponse now share one writeFramedMessage() helper.

Extension plumbing (Extension/src/{socket.ts,loggingdb.ts,types/browser.d.ts})

• DataReceiver.onDataReceived builds a connection-bound respond() closure
  and passes it to the callback.
• On Finalize, loggingdb waits the BrowserManager-supplied grace (during
  which visit_id stays valid), then sends meta_information, clears the
  visit, and replies FinalizeAck.

Python side (openwpm/socket_interface.py, openwpm/commands/browser_commands.py, openwpm/browser_manager.py)

• ClientSocket.receive(timeout) reads a framed reply. It buffers bytes on
  the instance, so a mid-frame timeout cannot desync the protocol, and it
  restores any socket timeout that was set before the call.
• FinalizeCommand sends the grace duration, then waits for a FinalizeAck
  whose action and visit_id match the current visit, with a bounded
  fallback timeout. On timeout it raises FinalizeAckTimeout instead of
  logging and proceeding.
• BrowserManager catches FinalizeAckTimeout and emits a new
  FINALIZE_INCOMPLETE status, which the command loop handles by marking the
  visit unsuccessful and incrementing failure_count without requiring a
  browser restart (see Ack-timeout handling above).

Tests (test/test_socket_interface.py, test/test_finalize_ack.py)

• New pyonly unit tests for ClientSocket.receive framing: single frame,
  two frames in one read, mid-frame timeout without desync, timeout
  restoration, peer-close, and byte-trickled reassembly.
• New test/test_finalize_ack.py (pyonly): FinalizeCommand sends the grace
  and visit_id; a matching FinalizeAck returns normally; a timeout raises
  FinalizeAckTimeout (and does so after roughly the deadline); a
  non-matching control message is discarded before a subsequent valid ack is
  accepted.

How the Copilot review comments were addressed

Copilot comment	Resolution
receive() resets socket timeout to None, clobbering any prior timeout	receive() now captures gettimeout() and restores it in finally. Test: test_receive_restores_prior_socket_timeout.
A timeout mid-frame desyncs the framing stream	ClientSocket now keeps an internal _recv_buffer; partial reads and over-reads are preserved across calls. Test: test_partial_frame_timeout_does_not_desync.
FinalizeCommand catches all exceptions and logs them as a timeout	Split into socket.timeout (the timeout path, which now raises FinalizeAckTimeout so the visit is marked unsuccessful) vs. (RuntimeError, ValueError, struct.error) (logged with logger.exception).
FinalizeCommand doesn't validate the ack matches the current visit	It now loops until it sees action == "FinalizeAck" and visit_id == self.visit_id, discarding (and logging) anything else, bounded by a shared deadline.
FinalizeAck sent before late records are flushed / visit_id cleared too early	Core of the redesign: the grace period moved into the extension, visit_id stays valid through it, and the ack is sent only after meta_information is handed to storage.
Closed connection leaks its output stream	On disconnect the paired output stream is closed before the connectionMap entry is dropped.
sendResponse frames by data.length (string length, not bytes)	See "Known limitation" below.

Critical review against Mozilla socket practices

I reviewed the privileged socket code against Mozilla's nsITransport /
nsISocketTransport contract (via Searchfox) and against OpenWPM's own
established socket patterns.

• Reply stream blocking mode. The first cut opened the reply stream with
  transport.openOutputStream(0, 0, 0). Per
  nsITransport.idl,
  flags 0 yields a non-blocking stream whose write() can throw
  NS_BASE_STREAM_WOULD_BLOCK — which the surrounding try/catch would
  silently swallow, dropping the ack. Correctly handling a non-blocking output
  stream means nsIAsyncOutputStream + asyncWait and a write queue. Since
  the existing outbound stream in connect() already uses
  openOutputStream(OPEN_BLOCKING, …) and the ack is a tiny payload to a
  localhost peer that is actively reading, a blocking write returns
  effectively instantly. The reply stream now uses OPEN_BLOCKING, matching
  the existing pattern; the value (1 << 0) is confirmed by nsITransport.idl.
• Bidirectional use of one accepted transport. Opening both an input and
  an output stream from the single transport handed to onSocketAccepted is
  the supported pattern for full-duplex use; the input stream stays
  non-blocking (required for asyncWait) and the two streams are independent.
• Stream lifetime. nsIServerSocket accepted connections own their
  streams; nothing closes them implicitly. The disconnect path now closes the
  output stream (the input stream is already closed by the peer at that
  point), preventing a per-visit-failure descriptor leak.
• Framing consistency. OpenWPM's existing sendData and the Python
  socket_interface both frame with a 4-byte big-endian length + 1-byte
  serialization tag. sendResponse reuses that exact format (now via a shared
  writeFramedMessage helper) so the Python receive() and send() stay
  symmetric.

Known limitation (documented, not silently shipped)

writeFramedMessage derives the length prefix from data.length, a UTF-16
code-unit count, which equals the byte count only for ASCII. This is the
pre-existing framing already used by sendData. It is correct for every
message that travels this socket today — storage records (ASCII-escaped JSON)
and the control messages Initialize / Finalize / FinalizeAck (string
action + integer visit_id + boolean) are all ASCII. Making the framing
UTF-8-safe is worthwhile, but it must change sendData and sendResponse
together to keep the two senders consistent, so it is intentionally left
as a separate follow-up rather than introducing a divergence here. The
writeFramedMessage doc comment records this.

Sources referenced

• netwerk/base/nsITransport.idl (Searchfox, mozilla-central) — confirmed
  OPEN_BLOCKING = 1<<0 / OPEN_UNBUFFERED = 1<<1 and that flags 0 produces
  a non-blocking stream. This is what drove switching the reply stream to
  OPEN_BLOCKING and rejecting the original openOutputStream(0,0,0).
• OpenWPM's existing socket code (api.js connect() / sendData,
  socket_interface.py send / _handle_conn / _parse) — used as the
  reference for stream flags, the wire format, and the serialization tags.
  This is why sendResponse mirrors sendData (shared helper, blocking
  stream) instead of introducing a parallel convention, and why the
  byte-length question is treated as a cross-cutting follow-up.
• git log on browser_commands.py — traced the time.sleep grace to
  e338bb2 ("Command refactoring", Command refactoring #750) and FinalizeCommand(sleep=5) in
  command_sequence.py. Establishing that the grace keeps visit_id valid for
  in-flight events is what exposed the original PR's attribution regression and
  shaped the "move the grace into the extension" design.
• Issues [BUG FIX] the extension doesn't instrument all the APIs in js_instrumentaion_collections #1171 / fix the aborting issue of js instrument #1172 / fix(js-instrument): propagate instrumentation failures to BrowserManager #1173 — confirmed the intended downstream
  consumer of this back-channel, keeping sendResponse / respond() generic
  rather than FinalizeAck-specific.

Follow-up

• Replace remaining fixed-timeout teardown drains with confirmations (follow-up to finalize-ack #1174) #1220 — the finalize ack confirms the extension→BrowserManager handoff;
  two other teardown points (the log-socket shutdown sleep(3) in mp_logger
  and the storage-controller completion-queue poll) still use fixed timeouts
  and are tracked there for the same drain-and-confirm treatment.

Test plan

• pytest -m pyonly test/test_socket_interface.py — 6 new pyonly framing tests pass.
• pytest -m pyonly test/test_finalize_ack.py — 5 pyonly tests pass (grace/visit_id message, ack returns normally, timeout raises FinalizeAckTimeout, deadline timing, stale-message discard).
• pytest test/test_simple_commands.py::test_get_site_visits_table_valid
  and ::test_get_http_tables_valid — full visit cycle incl. the
  FinalizeAck round trip, pass (headless + xvfb).
• pytest test/test_js_instrument.py -k "not failure" — 6 tests pass;
  exercises async JS events, the most likely stragglers, attributed correctly.
• cd Extension && npm run build && npm run lint — clean.
• pre-commit run — clean on changed Python files.

[Copilot]

Pull request overview

Adds an extension → BrowserManager reply path on the existing extension control socket, and uses it to make FinalizeCommand wait for an explicit FinalizeAck instead of sleeping a fixed interval.

Changes:

• Add framed response receiving on the Python ClientSocket and wait for an extension ack in FinalizeCommand.
• Extend the privileged WebExtension sockets API to track accepted connections, include connectionId in onDataReceived, and add sendResponse(...).
• Plumb a per-message respond(...) callback through Extension/src/socket.ts and send FinalizeAck from loggingdb.ts.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
openwpm/socket_interface.py	Adds ClientSocket.receive() for framed replies from the extension.
openwpm/commands/browser_commands.py	Updates FinalizeCommand to wait for a FinalizeAck with a bounded timeout.
Extension/src/types/browser.d.ts	Updates TS types for connectionId and sendResponse.
Extension/src/socket.ts	Passes a bound respond function to per-socket listeners.
Extension/src/loggingdb.ts	Replies with FinalizeAck on Finalize.
Extension/bundled/privileged/sockets/schema.json	Extends schema for sendResponse and adds connectionId to the event.
Extension/bundled/privileged/sockets/api.js	Implements connection tracking and sendResponse in the privileged sockets API.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov]

Codecov Report

❌ Patch coverage is 73.52941% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.40%. Comparing base (25a7827) to head (c7f760f).

Files with missing lines	Patch %	Lines
openwpm/browser_manager.py	22.22%	14 Missing ⚠️
openwpm/commands/browser_commands.py	86.95%	3 Missing ⚠️
openwpm/socket_interface.py	95.65%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1174      +/-   ##
==========================================
+ Coverage   62.36%   62.40%   +0.04%     
==========================================
  Files          40       40              
  Lines        3930     3993      +63     
==========================================
+ Hits         2451     2492      +41     
- Misses       1479     1501      +22     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.