Repository navigation
WHOOP 5/MG: let the offload probe supersede the handshake, or it never runs - #1743
Merged
Merged
Conversation
Caught on the 22:21 field capture, which is the first one taken with any of this in mind, and it says the probe as merged would essentially never have run. shouldProbeUnbondedOffload requires a link carrying no CLIENT_HELLO, and that gate is right: a subscribe refusal on a link the bond watchdog is about to bounce is unattributable, which is the ambiguity this whole thread has been paying for. But the probe is only SCHEDULED from the two no-hello branches, and on this strap neither is reached. Across 41 captures the suppression latch fired in three, all on 26 Aug, and never since. The explicit-bond deferral yields after its first connect by design (#1642), so every link after that writes a hello. In the 22:21 capture the pull-to-refresh reaches the offload and is turned away by exactly this gate - "deferred, connect handshake not done yet, helloWrittenThisLink=true" - twice in three seconds. The fix is not to loosen attributability, which is what makes the answer worth having. It is that these are mutually exclusive experiments: "does the hello work" and "is the hello needed at all" cannot both be asked of one link, because asking the first is what destroys the second's link. Turning the switch on is choosing the second question, so it now supersedes the handshake for that connect - the explicit pairing request as well as the hello - placed ahead of both, since the deferral branch returns early and a probe scheduled after it never runs. Pressing Connect still gets the handshake, exactly as it overrides the suppression latch. An explicit request for one experiment must not be answered with another. The line says the hello is absent BY CHOICE, because an absent hello reads identically to one that failed silently, and it names the explicit-bond switch when that is also on: a pairing in flight makes a refusal unattributable, so the probe declines to latch it and the capture is weaker for nothing. Also adds versionCode to the strap-log header. Build identity has been ambiguous three times in two days - twice it cost a detour reasoning about which binary a capture came from, and the 22:21 log needed the absence of probe lines to date it. One field ends that. Staging 390 / iOS 271. 4893 tests, lintVital, doc lint and i18n clean.
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.
Follow-up to #1742, caught on the first field capture taken with any of it in mind. The probe as merged would essentially never have run.
The gap
shouldProbeUnbondedOffloadrequires a link carrying noCLIENT_HELLO, and that gate is right — a subscribe refusal on a link the bond watchdog is about to bounce is unattributable, which is the ambiguity this whole thread has been paying for since June.But the probe is only scheduled from the two no-hello branches, and on this strap neither is reached in practice:
In the 22:21 capture the pull-to-refresh reaches the offload and is turned away by exactly this gate, twice in three seconds:
helloWrittenThisLink=trueon every one of those links. The probe would have waited behind a state the app almost never enters.The fix
Not to loosen attributability — that is what makes the answer worth having.
These are mutually exclusive experiments. "Does the hello work" and "is the hello needed at all" cannot both be asked of one link, because asking the first is what destroys the second's link. Turning the switch on is choosing the second question, so it now supersedes the handshake for that connect — the explicit pairing request as well as the hello.
Placed ahead of both, not beside the hello decision: the deferral branch returns early, so a probe scheduled after it never runs. That ordering is the bug, restated.
What still wins
Pressing Connect still gets the handshake, exactly as it overrides the suppression latch today. An explicit request for one experiment must never be answered with a different one.
Narrow otherwise: 5/MG only, never on a strap that has already bonded (that one reaches the offload the proven way), default off.
Log honesty
The new line says the hello is absent by choice — an absent hello reads identically to one that failed silently, which is precisely what made #1635 unreadable for eleven weeks.
It also names the explicit-bond switch when that is on too. The two do not conflict fatally, but a pairing in flight makes a subscribe refusal unattributable to the strap, so the probe declines to latch it and the capture is weaker for no gain. Better to say so than to let someone run both and get a mushy answer.
Also:
versionCodein the strap-log headerBuild identity has been ambiguous three times in two days. Twice it cost a detour reasoning about which binary a capture came from, and dating the 22:21 log depended on noticing that probe lines were absent. One field ends that.
Verification
4893tests, 0 failureslintVitalFullRelease, doc-comment lint, i18n--cicleanRelated to #1635 and #1742.