fix(pjsua): honour the retry interval when it is below the jitter

[laconicman]

Fixes #5318.

schedule_reregistration() randomises the retry delay by reg_retry_random_interval (10 s by default). For a retry interval below that value, the else branch cleared delay.sec before using it in the modulo, so delay.sec * 1000 was always 0 and the delay was a uniform 0 to reg_retry_random_interval whatever the interval was. An interval of 1 and one of 9 behaved alike, and the mean was 5 s for every value below 10 but 10 s at 10. This was pointed out in the review of #5306, where it became reachable from pjsua through --rereg-delay.

The delay.sec * 1000 term came with #1802 (e9268d2), but delay.sec was cleared on the line before, so it has been 0 ever since. The behaviour is the one the code has had since 2011 (#1375), not a regression.

This computes the delay before clearing delay.sec, so for an interval below reg_retry_random_interval it is uniform in 0 to the interval plus reg_retry_random_interval. That is the whole documented range, "interval ± reg_retry_random_interval" with the negative part cut off at 0. It is continuous with the >= branch (at an interval equal to reg_retry_random_interval both give 0 to twice that), it grows with the interval, and it keeps the default first retry (interval 0) at 0–10 s. I did not clamp the documented range at 0 instead, because that puts a point mass at 0: with the defaults, about half of all retries at an interval of 0 or 1 would be immediate.

The documentation cannot be followed literally below the jitter, where the lower end would be negative, so the pjsua.h and pjsua2 comments now say what range applies there.

A second commit, from review: where long is 32-bit, an interval too large for a long makes delay.sec negative, and in this branch that made the divisor 0. It is now clamped to 0 there, so such a value keeps the old 0 to reg_retry_random_interval delay. The same commit corrects the comments that said the first retry is immediate (it waits reg_first_retry_interval), points the reg_retry_interval and reg_first_retry_interval docs at reg_retry_random_interval for the range, and uses the pjsua2 field names in AccountRegConfig. The docs side is pjsip/pjproject_docs#70.

With the default jitter of 10 s, the delay window becomes:

Retry interval	Before	After
0 (the default reg_first_retry_interval)	0–10 s	0–10 s
1	0–10 s	0–11 s
5	0–10 s	0–15 s
9	0–10 s	0–19 s
10 or more	interval ± 10 s	unchanged

This is a behaviour change for anyone who sets reg_retry_interval or reg_first_retry_interval to 1–9 s: the average delay grows from 5 s to (interval + 10) / 2 s, so a 2 s first retry goes from 0–10 s to 0–12 s. It is what the documentation says for such an interval, and I found nothing in the repository that depends on the old window.

Testing

pjsua with #5306 applied, so that --rereg-delay reaches the account, against a closed TCP port, --rereg-delay=9, 155 s, reading the "Scheduling re-registration retry for acc 1 in N seconds" lines after the first retry (the app's own first retry is 60 s):

Build	Retries	Longest	Values (s)
before	25	9	6 2 0 1 4 0 7 2 8 4 7 4 9 2 5 9 6 1 3 0 1 1 0 3 8
after	11	16	9 5 2 0 13 4 9 6 10 16 15

The logged seconds are truncated, and the sample is small. The windows in the table above are the exact ones: I computed the minimum, maximum and mean over a million draws for intervals 0, 1, 2, 5, 9, 10, 11, 15 and 100, and the "after" delay stays inside the documented range for every one of them. make builds without new warnings, and tests/pjsua scripts-run/100_simple.py passes.

I can't build with a 32-bit long here. With long emulated as 32-bit under UBSan, the arithmetic divides by zero for (unsigned)-10 before the clamp and gives the old 0–10 s after it, and the new divisor overflows only for a reg_retry_random_interval at which the other branch's rand_ms * 2 already does (above about 12 days).

There is no regression test, and I would rather say so. The delay is computed inline in a static function that takes a live account, so testing it means extracting the calculation into a small helper first. Happy to do that if you would like one.

[sauwming]

The divisor now depends on delay.sec, which is a signed long copied from the unsigned reg_retry_interval / reg_first_retry_interval. Where long is 32-bit (Windows, 32-bit Linux), a huge value, e.g. (unsigned)-10, makes delay.sec negative. It then takes this branch, delay.sec * 1000 + rand_ms can become 0, and the % crashes with SIGFPE. Values like 0x80000000 also overflow delay.sec * 1000. Before this change the divisor was always rand_ms, so this is new.

It's an odd config, but the guard is cheap. Could you clamp delay.sec to [0, reg_retry_random_interval) before using it here, e.g. by testing delay.sec < 0 too, or by doing the comparison in unsigned?

[laconicman]

You're right that this is new: the divisor used to be rand_ms alone. Fixed in 0e1ad28 by clamping a negative delay.sec to 0 in that branch, so such a value gets the same 0 to reg_retry_random_interval delay as before this PR. I didn't do the comparison in unsigned: those values would then take the other branch and come out negative, and since the timer adds the delay to the current time, every retry would fire at once. I can't build with a 32-bit long here, so I checked the arithmetic with long emulated as 32-bit under UBSan: division by zero before the fix, the old 0–10 s after it. With the clamp the divisor stays below 2 * rand_ms, so it can only overflow for a reg_retry_random_interval at which the other branch's rand_ms * 2 already does (above about 12 days); I've left that pre-existing limit alone.

[sauwming]

Two nearby comments no longer match the behaviour (both lines are outside the diff, so I'm commenting here):

• L5524 /* Randomize interval by +/- reg_retry_random_interval, if configured */: the else branch now uses 0 to interval + jitter, not +/- jitter. Please mention that the lower bound is clamped at 0.
• L5519 The first attempt will be done immediately. (and the same statement in the function header at L5471): the first retry actually waits reg_first_retry_interval plus jitter. That predates this PR, but the new header docs now contradict it, so it'd be good to fix it here too.

[laconicman]

Both fixed in 0e1ad28. The randomisation comment now says the lower bound is clamped at 0 and gives the resulting range for an interval below reg_retry_random_interval. The function header and the in-body comment now say that, until a registration succeeds again, the first retry waits reg_first_retry_interval and later ones reg_retry_interval, since attempt_cnt is only reset on a 2xx.

[sauwming]

Thanks for documenting the new window here. The reg_retry_interval (L5147) and reg_first_retry_interval (L5161) docs still say the interval is "randomized slightly by some seconds", though, and with this change a 1 s interval can become anything from 0 to 11 s. Could you update those two to point to the clamped window described here?

[laconicman]

Done in 0e1ad28: both now say the interval is randomized by reg_retry_random_interval and point there for the resulting range, rather than repeating it. The pjsua2 retryIntervalSec and firstRetryIntervalSec docs got the same change, and pjsip/pjproject_docs#70 adds reg_retry_random_interval to the retry settings listed in the server affinity guide.

[sauwming]

Minor, PJSUA2 naming: "this value" here, and the retryIntervalSec doc at L118, refer to the C field reg_retry_random_interval. In this struct the field is randomRetryIntervalSec, so please use that name (e.g. \a randomRetryIntervalSec).

[laconicman]

Done in 0e1ad28, together with the other C names in the same three field docs: retryIntervalSec and firstRetryIntervalSec now refer to randomRetryIntervalSec, and randomRetryIntervalSec refers to retryIntervalSec and firstRetryIntervalSec.