Skip to content

feat(backend): record server_disconnect metric when a server peer is left mid-sync - #303

Draft
gmaclennan wants to merge 2 commits into
mainfrom
feat/sync-server-disconnect-events
Draft

gmaclennan wants to merge 2 commits into
mainfrom
feat/sync-server-disconnect-events

Conversation

@gmaclennan

@gmaclennan gmaclennan commented Nov 5, 2026 •

Copy link
Copy Markdown
Member

We have no telemetry showing whether a phone leaves the cloud archive while it still wants blocks from the phone. The investigation found that leaving the Exchange screen tears down the server websocket on blur regardless of progress, and that Stop (or the 30 s autostop) stops serving a still-connected server, so the cloud is left holding partial originals. Nothing in Sentry today tells us how often that happens in the field.

This adds one count metric, comapeo.sync.server_disconnect, emitted from the existing sync observer in backend/lib/sync-observer.js. The watcher wraps $sync.stop() and $sync.disconnectServers() on the project's sync API and treats any data-sync-off edge that neither call produced as the autostop, so reason=autostop covers both the 30 s timer and the full stop that fires when the app is backgrounded. It fires once per data-sync session while a server peer is connected, with reason (disconnect, stop, autostop), data_enabled (was data sync on when the call came in) and, at the usage tier only, server_want_bucket (blocks the server still wanted from us: 0, 1-10, 11-100, 101-1000, 1000+). Server peers are the members whose deviceType is selfHostedServer, looked up through $member.getMany() and cached in the watcher. No peer or project identifiers are attached; the bucket helper and the metric wrapper live in backend/lib/metrics.js beside the existing ones. The tests cover each reason, the usage-off case, a local-only exchange recording nothing, and the server leaving and reconnecting re-arming the event.

Two things for reviewers. The want bucket is usage-gated like peers_bucket, so at the diagnostics tier we only learn that a server was left and whether data sync was on; if the bucket should be always-on, it is a one-line change. And the "once per session" rule resets when the server peer disappears or data sync starts again, which means Stop followed by leaving the screen records a single stop event rather than two.

Related changes. digidem/comapeo-core#1353 and digidem/comapeo-mobile#2213 remove the two ways of leaving the server with a partial upload that this metric counts. The metric is still worth having to see how often it happens on phones that have not updated. Once the mobile change ships, the reason=disconnect count should fall to near zero; if it does not, something else is closing the socket.

@gmaclennan gmaclennan left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure of the benefit of this if we're shipping the other fixes, and it adds quite a lot of code to maintain. Isn't the metric that's valuable to collect is when a connection is disconnected before sync completes? Does that need a change in core to capture? And then we can also capture this metric on the server side also.

@gmaclennan

Copy link
Copy Markdown
Member Author

Agreed, the number worth having is "a connection closed while sync with that peer was unfinished", and this PR only approximates it for servers by watching sync state from the outside. Core is the only place that knows both facts at the moment they happen: #handlePeerDisconnect in SyncApi has the peer's remote state (want and wanted per namespace) just before it deletes it. A small core change could emit an event there, say peer-disconnect with the device id, whether the peer was a server, and the outstanding want and wanted counts. comapeo-core-react-native then forwards that to Sentry in a few lines, and the cloud can report the same event from its side, so we see both ends of the same disconnect and can tell phone-initiated from network-initiated ones.

I would close this PR in favour of that. If you agree I will open the core PR, with the two one-line consumers to follow once it lands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature (changelog)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant