MT#55283 add rollback NG message

[danieldonoghue]

This adds an opt-in rollback message to the RTPengine NG protocol. It lets a
signalling application undo an SDP offer that RTPengine has applied but the
remote endpoint subsequently rejects, without deleting the established call.

The change addresses the case discussed on the mailing list in RTP source port
change after RE-INVITE followed by 488: a rejected renegotiation can
leave RTPengine using media parameters that neither endpoint accepted. The
design proposal that preceded this work sets out the problem, the
alternatives considered, and why handling it purely client-side is not
sufficient.

Protocol

A client enables checkpointing with track-state on an offer. RTPengine
snapshots the affected dialogue before applying the offer. A successful answer
commits the exchange and discards the snapshot; rollback restores and consumes
it. Calls that do not opt in allocate no checkpoint. Each side of a dialogue holds
at most one outstanding checkpoint: offers arriving before an exchange completes belong to
the same uncommitted exchange and keep the existing snapshot, so a rollback
returns to the last completed offer/answer rather than to an intermediate one.

{
  "command": "rollback",
  "call-id": "...",
  "from-tag": "...",
  "to-tag": "...",
  "via-branch": "..."
}

A successful response contains rolled-back, set to 1 if a pending checkpoint
was restored or 0 if there was none outstanding. Repeating a
rollback is therefore safe and returns rolled-back: 0, which makes repeated or
delayed failure handling safe. Dialogue and optional via-branch matching
prevent one fork from consuming another fork's checkpoint.

track-state names the opt-in behaviour and track state is accepted for
consistency with existing flag forms. Capability discovery uses the existing
supports / supported mechanism, so a proxy that needs to know whether it must
go on tracking state itself can ask.

Implementation

Each monologue holds a checkpoint: a snapshot in the call record format the Redis
encoder already produces, so there is one definition of what a call's state looks
like, and state added to the call graph is captured without a second encoder
having to be kept in step. A snapshot covers only the monologue it belongs to,
and only the entries the decoder reads.

Decoding a snapshot needs the same field readers the Redis restore path uses, so
those readers are shared rather than duplicated, which is why daemon/redis.c is
the largest file in the diff. json_restore_call() reads the same fields, in the
same order, with the same strict-return behaviour it always has.

Applying a snapshot copies negotiated state back onto the live objects, matched by
unique id. The objects are never recreated, so local sockets and ports survive,
which is the point of the exercise.

What is restored

Rollback restores addresses and ports, endpoints and endpoint maps, codecs and
payload mappings including offered payload types, RTP profile and transport,
media directions, SDES parameters and keys, ICE credentials and candidates, DTLS
configuration, and endpoint learning state.

State that the rejected offer introduced is removed as well as overwritten. An
offer that upgraded a media to DTLS-SRTP, for example, leaves behind no TLS ID,
fingerprint or SRTP context once it has been rolled back. Fields the encoder
writes only when set are absent from a snapshot taken before they existed, and
that absence is treated as "clear this" rather than "leave it alone".

Sink handlers are rebuilt from the persistent subscription graph rather than
snapshotted, so an independent subscribe request issued while an offer was
pending is not undone.

Where a call has been forked, the offering side is shared between the branches.
Its checkpoint is taken once, before the first uncommitted offer, so rolling back
one branch does not disturb what rolling back another has already restored.

Sockets and endpoint maps allocated for a rejected offer are not released by a
rollback. The media is returned to the sockets it was using, and the surplus is
reclaimed with the call.

ICE and DTLS

ICE and DTLS reconverge rather than being rewound. Applying a changed offer has
already reset the ICE candidate, pair, nomination, and timer state and shut down
the live OpenSSL DTLS association before rollback is requested. Restoring the
accepted ICE credentials and candidates lets connectivity checks rebuild the ICE
state; restoring the DTLS fingerprint, TLS ID, role, and setup permits a fresh
handshake. The live OpenSSL association cannot be copied or serialized, so DTLS
media pauses while that handshake completes. This is also the existing limitation
of Redis takeover.

For non-DTLS media, restoring a changed remote endpoint follows the existing
endpoint-change path through call_stream_crypto_reset(), which resets the
crypto context and extended sequence state together; a zero extended sequence
also suppresses transcoding ROC restoration. DTLS media skip that reset and
instead follow the re-handshake path above, so this post-rollback behaviour is
observably different between SDES and DTLS calls.

Redis

Outstanding checkpoints are stored in the Redis call record as a
checkpoint-<monologue id> dict holding the pending flag and the snapshot, so
they survive takeover. Checkpoint data is auxiliary: one that cannot be read
discards the checkpoints for that call, which is restored without rollback
capability.

Compatibility is additive. Older instances ignore the checkpoint keys, and their
absence is read as no checkpoint rather than as an error, so mixed-version
takeover degrades safely to rollback being a no-op. The snapshot contains SDES
keys, as existing Redis call state already does, so Redis remains part of the
same trusted security boundary.

A snapshot carries negotiated state the call record does not otherwise persist:
ICE credentials and candidates, TLS ID, the preferred fingerprint hash, endpoint
maps, offered payload types, and endpoint learning state. That state is written
into snapshots only, not into every call record, so a call that does not opt in
serialises to the same size it did before. Wiring it into json_restore_call()
would let ICE survive a takeover, which it currently cannot, but that would mean
carrying it in every record and is a separate change on its own merits.

Cost

Snapshotting is O(medias × streams) inside the existing call write lock on the
offer path and is paid only by opted-in calls. The copy must remain under that
lock so it is atomic with both the protected media state and application of the
offer. Rollback also needs to stop media created by the rejected offer, so the
existing media_stop() helper is exposed as call_media_stop() for use by
snapshot application.

Tests

A rollback daemon suite and a fake-Redis takeover suite run as direct
prerequisites of the default daemon-tests target. The rollback suite covers
restoration of media, transport, SDES, ICE, and DTLS state; no-op semantics;
consecutive unanswered offers; fork and via-branch isolation; active
subscription reconstruction and media flow before and after rollback; deletion;
and repeated cycles.

The Redis suite needs no external Redis server; it serves Redis from a forked
child, because the daemon writes the call record from the poller thread as well
as after a signalling request, and a single-process fake eventually leaves a
write unanswered and blocks the daemon in redis_consume(). It covers native and
JSON records, round-trip and second-instance takeover, rollback after takeover,
and unreadable checkpoint data degrading to a restored call without checkpoint
state.

Its assertions are made against the call record rather than against query,
because query exposes only a subset of a media's state, so a rollback that
stopped restoring the rest would leave every query-based assertion green.

The pre-existing Redis tests read the record length as however many digits it
takes, and across as many reads as it needs, rather than assuming four digits
and a single read.

One unrelated line in t/Makefile changes with them: the preload shims no longer
inherit the sanitizer options from CFLAGS. In a sanitizer build those options
linked the sanitizer runtime into the shims, which the harness loads into the
test scripts' own interpreter, and that crashed the daemon tests on aarch64
during library initialisation. Everything else in CFLAGS, the Debian hardening
flags included, still applies to them. Happy to split this out if you would
rather it came separately.

Notes

The commits currently carry no Change-Id trailers; I can regenerate them with
the project's standard Gerrit commit-msg hook if that is preferred.

This was developed with AI assistance. I have reviewed the design, the code and
the tests myself, and I will carry the change through review and address anything
raised.

[rfuchs]

This is all obviously AI generated and honestly looks like a maintenance nightmare

[danieldonoghue]

This is all obviously AI generated and honestly looks like a maintenance nightmare

Hi @rfuchs

yes I did use AI to generate this but I was very careful to monitor the AI closely and give it specific directions at every stage.

I tried to keep the scope of the actual change as small as possible, keeping the actual rollback implementation in its own file, which necessitated promoting some other existing functions to internal apis in order to prevent reinventing the wheel for some of the code - particularly around Redis.

is there something specific you're referring to with regard to maintenance? I'll be happy to take another look.

[danieldonoghue]

The sanitizer job is failing in the new Redis test. It doesnt fail for me when I run it locally but im still trying to see if I can reproduce it.

[rfuchs]

I tried to keep the scope of the actual change as small as possible, keeping the actual rollback implementation in its own file, which necessitated promoting some other existing functions to internal apis in order to prevent reinventing the wheel for some of the code - particularly around Redis.

Incidentally this is my primary concern here. This adds a ~2000 line wall of code, with some of it appearing very much redundant at first glance, and maintenance of all of that will ultimately fall on my shoulders. A bunch of new structures which seem to mirror what already exists elsewhere, plus all the required boilerplate code to deal with them, which also already exists in a slightly different shape elsewhere.

AI is great at generating new code but doesn't like reusing what's already there, and when the time comes it will be my burden to refactor the mess into actually maintainable code.

[danieldonoghue]

Incidentally this is my primary concern here. This adds a ~2000 line wall of code, with some of it appearing very much redundant at first glance, and maintenance of all of that will ultimately fall on my shoulders. A bunch of new structures which seem to mirror what already exists elsewhere, plus all the required boilerplate code to deal with them, which also already exists in a slightly different shape elsewhere.

AI is great at generating new code but doesn't like reusing what's already there, and when the time comes it will be my burden to refactor the mess into actually maintainable code.

That's fair and I did raise similar concern during development but didn't push hard enough. I'll rework this so there's only 1 representation instead of parallel sets; this should remove a chunk of the diff.

I also managed to track down the sanitiser issue as a race in the new redis test that assumes a fixed command sequence per request whilst the poller thread also writes on ICE activity. it only occurs for me if I run on x86_64 instead of arm64 - I'll fix that, too.

its the weekend now so I'll pick this up again on Monday morning.

[rfuchs]

A couple of thoughts about the overall implementation:

• The generation counter. Is that actually useful or needed? AFAICT the only(?) use case for this is a pending offer, which if rejected would cause a rollback to the previous state. I can't envision a scenario that would require tracking of multiple pending states?
• The supports=rollback entry. Ok, we can advertise the feature being supported, but does that actually serve a meaningful purpose? What difference in behaviour (either on the SIP proxy side or on rtpengine side) would the presence of that flag effect? Especially since tracking of a pending state must be explicitly enabled with a flag anyway?

[danieldonoghue]

A couple of thoughts about the overall implementation:

• The generation counter. Is that actually useful or needed? AFAICT the only(?) use case for this is a pending offer, which if rejected would cause a rollback to the previous state. I can't envision a scenario that would require tracking of multiple pending states?
• The supports=rollback entry. Ok, we can advertise the feature being supported, but does that actually serve a meaningful purpose? What difference in behaviour (either on the SIP proxy side or on rtpengine side) would the presence of that flag effect? Especially since tracking of a pending state must be explicitly enabled with a flag anyway?

Thanks for taking the time to review the work.

regarding the generation counter, it's a holdover from my original design which had the client holding state, where a generation was needed to tie a late answer (in ACK) back to the right pending offer to stop a retransmitted ACK or late failure from acting on obsolete state. consecutive offers collapse into the same uncommitted exchange but the only thing left stopping a delayed rollback from landing on a later exchange, as you already noted, is the counter but im not sure that earns a protocol field. Do you have any further thoughts? perhaps I should simply drop it.

the suppoted=roolback change is one I think I'd like to keep though, if you're ok with it. my thinking here was to be able to support the previously discussed replay capability in the kamailio rtpengine module if the rtpengine doesnt support rollback. Fallback has to be armed at offer time so it needs the offer and answer SDP to be recorded before the failure arrives; if the older rtpengine silently ignores the track-state directive, there's no other way to establish that tracking is active vs the flag being ignored.

[rfuchs]

regarding the generation counter, it's a holdover from my original design which had the client holding state, where a generation was needed to tie a late answer (in ACK) back to the right pending offer to stop a retransmitted ACK or late failure from acting on obsolete state. consecutive offers collapse into the same uncommitted exchange but the only thing left stopping a delayed rollback from landing on a later exchange, as you already noted, is the counter but im not sure that earns a protocol field. Do you have any further thoughts? perhaps I should simply drop it.

I'm still not sure I follow. Is the scenario of concern something like offer1, answer1, offer2, retransmit of answer1 or similar?

If so, then I think that should rather be handled in the SIP proxy? So that it just doesn't engage rtpengine on an outdated message?

Alternatively, if this would put too much burden on the SIP proxy (which would be understandable - I guess Kamailio still needs to relay the ACK with an appropriate SDP, which it needs to obtain from somewhere if it's stateless), we can add support to detect such cases, sure, but then I think it should be rather something that more generically applies to all offer/answer exchanges, and isn't tied to any rollback mechanism.

the suppoted=roolback change is one I think I'd like to keep though, if you're ok with it. my thinking here was to be able to support the previously discussed replay capability in the kamailio rtpengine module if the rtpengine doesnt support rollback. Fallback has to be armed at offer time so it needs the offer and answer SDP to be recorded before the failure arrives; if the older rtpengine silently ignores the track-state directive, there's no other way to establish that tracking is active vs the flag being ignored.

Ok, that makes sense, at least for rtpengine advertising its capability back to Kamailio. Not so much for the other direction 😄 but no matter, it doesn't hurt.

[danieldonoghue]

I've removed the generation counter now. per your comments and having looked at all the possible uses cases, it only guards against states compliant SIP can't reach. I agree that stale message detection belongs in the proxy, if it's necessary or in something generic across all offer/answer exchanges.

I've now refactored the change considerably. The mirrored structures are gone, with the snapshot now being held in the call record format the redis encoder already produces. To aid with this, I pulled some of the field readers into helper functions that could be called both for redis and snapshotting.

The sanitiser failure experienced in the tests should now be fixed. It was caused by the new redis tests assuming a fixed command sequence per request while the polling thread also writes on ICE activity. The mock server now runs in a forked child that always answers. the asn-checks now pass for both x86_64 and aarch64.

In order to get asn checks to pass successfully in aarch64, I had to change the preload shims so that they no longer inherit from CFLAGS. In a sanitiser build, those flags linked the sanitiser runtime into the shims, which the test harness then loads into the test scripts' own interpreter, causing the daemon tests to crash on aarch64 during library initialisation. not sure whether this is the right fix, or the right place to make the fix?

One other point worth noting: the call record now persists ICE credentials and candidates, tls-id, the preferred fingerprint hash, endpoint maps and offered payload types which are read by the snapshot decoder only. If they were also wired into json_restore_call(), that would let ICE survive a takeover. I haven't dont that as it felt outside the scope of this PR but thought I'd raise it in case that would be useful?

[rfuchs]

I've now refactored the change considerably. The mirrored structures are gone, with the snapshot now being held in the call record format the redis encoder already produces. To aid with this, I pulled some of the field readers into helper functions that could be called both for redis and snapshotting.

Ok, interesting approach. I'll have a more detailed review later. Since the most recent commit undoes large parts of the previous ones you should probably squash them.

The sanitiser failure experienced in the tests should now be fixed. It was caused by the new redis tests assuming a fixed command sequence per request while the polling thread also writes on ICE activity. The mock server now runs in a forked child that always answers. the asn-checks now pass for both x86_64 and aarch64.

Github seems to disagree 😄

Could be something specific that only triggers in its specific (Ubuntu) environment.

In order to get asn checks to pass successfully in aarch64, I had to change the preload shims so that they no longer inherit from CFLAGS. In a sanitiser build, those flags linked the sanitiser runtime into the shims, which the test harness then loads into the test scripts' own interpreter, causing the daemon tests to crash on aarch64 during library initialisation. not sure whether this is the right fix, or the right place to make the fix?

I think Debian CI might have an issue with this, as it insists that the Debian hardening flags are used for all built objects, including the preload .so, and they're passed in CFLAGS. Might have to separate out the variables that keep the asan flags and the hardening flags?

One other point worth noting: the call record now persists ICE credentials and candidates, tls-id, the preferred fingerprint hash, endpoint maps and offered payload types which are read by the snapshot decoder only. If they were also wired into json_restore_call(), that would let ICE survive a takeover. I haven't dont that as it felt outside the scope of this PR but thought I'd raise it in case that would be useful?

That would probably be useful, yes. (ICE is mostly used in WebRTC, and those calls can't really be restored because of DTLS, but could be useful to have it anyway.)

[danieldonoghue]

I've squashed the commits now, as requested.

regarding the preload shims, I've changed it to just filter out sanitizer options now and keep everything else.

on the CI, the failure in the unit tests was caused by the new fields I added getting close to, and in some cases even exceeding, the expected record size of 9999 bytes. I've changed those tests to read the length as however many digits it takes, and read the record across as many reads as it needs. hopefully they pass this time :)

I've left the ICE field wiring for now. happy to do it in a separate pr but there is a cost to it as each record grows by around 1-2kb for each call which, if its not being used, is not insignificant.

[rfuchs]

A few questions, and then some comments/suggestions, but these could be left for later.

I do wonder if the object to contain the snapshot state for a call_monologue could be just another call_monologue, i.e. just make a copy before modifying it, and then copy back when restoring the state. Might be less effort than going through a serialised string...

[rfuchs]

I think call_get_monologue already guarantees that the returned monologue has the tag that was given to it, doesn't it?

[danieldonoghue]

it does for the from monologue, so I can drop that check but the to monologue can come from the via branch table rather than call_get_monologue(), and that requires additional checks to make sure the branch actually carries the to-tag we were given. There's a test for this behaviour.

[danieldonoghue]

I do wonder if the object to contain the snapshot state for a call_monologue could be just another call_monologue, i.e. just make a copy before modifying it, and then copy back when restoring the state. Might be less effort than going through a serialised string...

i took a closer look at that. the trouble here is that a monologue doesn't hold much of the state a rollback restores; it's nearly all in the medias below it and the streams below those; i.e., codecs, crypto, endpoints, ICE. Copying the struct would copy pointers so it would need a deep clone all the way down, and some of that, like stream_fds, which are refcounted and shared, or DTLS associations, which are OpenSSL objects, would be difficult to clone.

copying back would then have the same problem. sockets and ports have to survive the rollback so the live objects can't be readily replaced - it would still require field by field copying, as is done now. it would also need a new line each time a field is added, where the decoder picks those up on its own.

I will continue working on the other points raised.

[danieldonoghue]

I've now addressed the remaining issues from the last round of comments. in doing so I noted a couple of things to flag:

1. the record format for checkpoints has changed. its a checkout-<monologue id> dict per monologue now, rather than num_checkpoints and checkpoint-<n>. Absent keys still mean no checkpoint, allowing takeover between versions to degrade rollback to a no-op, as before.
2. adding rollback to NG_COMMANDS inserts a new column in the middle of the CLI control statistics table, between Answer and Delete. This shouldnt affect graphite but it could affect 3rd party scrapers and probably warrants something in release notes, should this PR get merged. I could also move the column to the end of the table but there's no garuntee that that would make existing parsers happy and would affect the logical grouping of the columns.

quite a few commits since the last review; let me know if you'd prefer that I squash them, or all the commits.

[rfuchs]

Ok, if no further changes are made, I will merge this in the coming days.

I think some other methods that rely on an offer/answer mechanism (publish/subscribe/create) could also benefit from this in the future.

It could even be useful for the existing active/active mechanism, which currently completely tears down the entire call and rebuilds it from scratch for every update from Redis. With some changes it might be possible to update the running states much more surgically.

[rfuchs]

I still feel like there should be an existing function which already does this sufficiently, without having to double check the returned objects. call_get_dialogue() perhaps? But it's a minor point, if tests are covering this, it can be addressed later.

[rfuchs]

I haven't checked, but I think all instances of this being used already have some sort of loop over the respective monologues, so the freeing of the snapshots could be rolled into those. But also a minor point that could be left for later.