feat(inbox): migrate the inbox socket from legacy to iwpv=v2 - #46
Closed
mikemilla wants to merge 1 commit into
Closed
feat(inbox): migrate the inbox socket from legacy to iwpv=v2#46mikemilla wants to merge 1 commit into
mikemilla wants to merge 1 commit into
Conversation
This SDK sent no `iwpv` query parameter at all, which the server treats as the
`legacy` protocol — a different envelope and a different frame shape from the one
GraphQL agrees with. That is the root cause of the standing
`// TODO: Make this cleaner. Need Riley's Changes.` next to `clickTrackingId`: on
`legacy` the socket nests tracking ids under `data` while the GraphQL read returns
them at the root, so the property had to check `data["trackingIds"]` first and fall
back. This is those changes.
`iwpv=v2` publishes the canonical message — the same object GraphQL returns, with
`trackingIds` at the root and no nested duplicate. This is not a version bump;
legacy and v2 are different protocols, so all four pieces move together:
- URL: `cid` and `iwpv` are now sent. The IWP handler rejects a connection missing
either, and `cid` is appended on the client-key path too, which previously
returned before any params were added. A client id is generated when the caller
never set `connectionId`, so that path yields a valid connection rather than a
rejected one.
- Subscribe: IWP envelope with `tid` and `action` at the root, options under
`data`. The `version` parameter is kept for source compatibility but no longer
sent — the server hardcodes `clientVersion` for every IWP subscription, so
passing it implied a negotiation that does not happen.
- Heartbeat: `keepAlive` is replaced by the IWP `ping`. `keepAlive` is not a valid
IWP action and carries no `tid`, so under v2 it would fail envelope validation
and return an error frame every interval. The client now also answers server
pings, echoing the `tid` — this is mandatory, not optional: the server
terminates a connection that fails to pong within its timeout.
- Frame parsing: `{"event":"message","iwpv":"v2","data":{…}}` decodes the message
from `data` instead of the frame root. Control frames are checked first, because
a `ping` has no `event` and would otherwise fall through to a failed message
decode and surface as a spurious error every heartbeat.
`clickTrackingId` now reads the root-level value only. Removing the nested branch
is safe *because* of the protocol move and not before it — on `legacy` the nested
copy is the only one present, so dropping the fallback alone would have broken
click tracking outright.
A mismatched `iwpv` on an incoming frame is logged. The server downgrades an
unrecognized version to legacy rather than rejecting it, so without that log a
downgrade would stay invisible until tracking silently stopped.
Requires the server-side v2 support (trycourier/services#1527) to be deployed
first, for the same reason.
Tests: 11 new JVM unit tests — protocol version and `cid` on both auth paths,
generated client id, root-level tracking id read, nested-only copy correctly
ignored, canonical frame decoding, and ping/ack control-frame decoding. 11 tests /
0 failures via `:android:testDebugUnitTest`; the full unit suite stays green.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Collaborator
Author
|
Parking this PR during a PR-cleanup pass. The branch is kept (not deleted) and the work is tracked in C-19800 (cycle 367). Reopening restores everything — branch, commits, and this thread. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket: C-19800
Important
Do not merge before trycourier/services#1527 is deployed. The server downgrades an unrecognized
iwpvto the legacy protocol rather than rejecting the connection, so shipping this first would silently move clients onto the legacy shape with no error.This is "Riley's changes"
InboxMessage.clickTrackingIdhas carried// TODO: Make this cleaner. Need Riley's Changes.for a long time. This is that change.The reason it existed: this SDK sent no
iwpvquery parameter at all, which the server treats as thelegacyprotocol. Onlegacythe socket nests tracking ids underdatawhile the GraphQL read returns them at the root — so the property had to check both places and guess:Legacy → v2 is a protocol change, not a version bump
All four pieces have to move together:
legacy)iwpv=v2)?auth=…only+ cidand+ iwpv— the IWP handler rejects a connection missing either{action, data}, notidtid+actionat root{action:"keepAlive"}ping, and answers server pings{"event":"message","iwpv":"v2","data":{…}}Details worth flagging in review:
cidon the client-key path.buildUrlpreviously returned before any params were appended on that branch. A client id is also generated when the caller never setconnectionId, so that path yields a valid connection rather than a rejected one.tidit's waiting on. A v2 client that ignores pings gets dropped mid-session.keepAlivehad to go. It isn't a valid IWP action and carries notid, so under v2 it would fail envelope validation and return an error frame every interval. It's now an IWPping.pinghas noevent, so if it fell through to the message decode it would surface as a spurious error every heartbeat.version:onsendSubscribeis retained but no longer sent — the server hardcodesclientVersionfor every IWP subscription, so passing it implied a negotiation that doesn't happen. Kept in the signature for source compatibility.The fallback removal is safe because of the protocol move
clickTrackingIdnow reads the root-level value only. Dropping the nested branch on its own would have broken click tracking outright, since onlegacythe nested copy is the only one present. The two changes are not separable.A mismatched
iwpvon an incoming frame is logged — without it, a silent downgrade stays invisible until tracking quietly stops.Tests
11 new JVM unit tests via
:android:testDebugUnitTest— 11 tests, 0 failures, full unit suite still green:cidpresent on both auth paths; generated client id whenconnectionIdis unset; JWT auth preserveddata, incl.trackingIdsand the promoted-key-freedatapingandackdecode as control framesNot verified
Superseded — dev is healthy again (inbox-dev
UPDATE_COMPLETE, ws-api task def rev 52, 1/1 running). The earlier blocker was not what I first diagnosed: the workingDRAGONFLY_API_KEYlives in 1Password, and the/courier/dev/dragonfly/*entries in SSM/Secrets Manager describe a different Dragonfly instance than dev dials, which is why they returnedWRONGPASSrather than being merely stale. The C-19789trackingIdslift is now confirmed on a live dev socket end to end.🤖 Generated with Claude Code