Skip to content

MT#55283 add rollback NG message - #2159

Draft
danieldonoghue wants to merge 4 commits into
sipwise:masterfrom
danieldonoghue:rollback
Draft

MT#55283 add rollback NG message#2159
danieldonoghue wants to merge 4 commits into
sipwise:masterfrom
danieldonoghue:rollback

Conversation

@danieldonoghue

@danieldonoghue danieldonoghue commented Aug 21, 2026

Copy link
Copy Markdown

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. If multiple offers
arrive before an answer or rollback, RTPengine preserves the original snapshot
and pending generation so rollback still returns to the last committed state.

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

A restore returns "rolled-back": 1 and the last committed generation. No
outstanding checkpoint and a generation mismatch return "rolled-back": 0;
for a checkpoint-enabled dialogue the response also reports the unchanged
committed generation. This 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. Tracked offer and answer responses expose
generation so a client can match a failure to its pending exchange. Capability
discovery uses the existing supports / supported mechanism.

What is restored

Rollback restores addresses and ports, endpoints, codecs and payload mappings,
RTP profile and transport, media directions, SDES parameters and keys, ICE
credentials, and DTLS configuration.

It rebuilds sink handlers from the persistent subscription graph rather than
snapshotting it, so an independent subscribe request issued while an offer was
pending is not undone.

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 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 as one optional, versioned checkpoint-data
field in the Redis call record so they survive takeover; invalid or unsupported
checkpoint payloads are discarded atomically while the call itself is restored
without rollback capability.

Compatibility is additive. Older instances ignore checkpoint-data and newer
instances treat its absence or an unsupported version as no checkpoint, so
mixed-version takeover safely degrades to rollback being a no-op. The checkpoint
contains SDES keys, as existing Redis call state already does, so Redis remains
part of the same trusted security boundary.

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 the
checkpoint module.

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; generation and
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 uses the established
preload fake listener, with no external Redis server, and covers native and
JSON records, checkpoint payload version/content, round-trip and
second-instance takeover, rollback after takeover, and type-invalid payloads
degrading to a restored call without checkpoint state.

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.

Add an opt-in checkpoint for calls using the track-state offer flag.
The checkpoint records the last committed media state so a rejected
renegotiation can be rolled back without deleting the call.

Take the snapshot under the call write lock before applying the offer.
Applying it can immediately replace media parameters and restart ICE or
DTLS, so a later snapshot would no longer represent endpoint-agreed
state. A successful answer commits the generation and consumes the
snapshot.

Preserve the first pending snapshot and generation across consecutive
offers so glare, retransmissions, and re-offers still roll back to the
last committed state.
Store pending rollback state in an optional, versioned checkpoint-data
field in the existing Redis call record. This lets a restored instance
rebind the snapshot to the call graph and retain rollback capability
across takeover.

The field is additive so older instances can ignore it and newer
instances treat its absence as no checkpoint. Rebinding is atomic:
unresolvable snapshot objects never produce a partial or potentially
incorrect restore.
Cover DTLS reinitialisation, active media subscriptions, pending-
checkpoint deletion, repeated rollback cycles, protocol responses, and
checkpoint statistics.

ICE and DTLS reconverge instead of rewinding live state. Applying the
rejected offer has already discarded the previous ICE graph and DTLS
association, and the OpenSSL state cannot be serialized. Restoring the
accepted credentials and configuration lets authenticated ICE checks
and a new DTLS handshake rebuild working state.

Rollback rebuilds sink handlers from the persistent subscription graph
rather than snapshotting it. Verify that a subscribe request remains
connected to the restored media and receives packets both before and
after rollback.

Deletion coverage ensures pending snapshots and their references are
released with the call. Keep query comparisons focused on negotiated
state by excluding offer-refreshed liveness timestamps; traffic-bearing
coverage asserts the subscription graph, endpoint, and media delivery
directly.
Move checkpoint escaping to the heap because the payload is unbounded
and cannot safely use the VLA in JSON_SET_SIMPLE_LEN. A 31-media call
produced roughly 76 kB of checkpoint data, growing by about 2.5 kB per
medium; around 270 media would exhaust the default 2 MB thread stack
after worst-case escaping.

Validate checkpoint JSON member and array element types before reading
them. Treat malformed or unsupported checkpoint data as loss of
rollback capability rather than loss of the restored call, and omit the
invalid payload from subsequent Redis updates.

Remove obsolete real-Redis test scaffolding and document the restored
socket-binding and ICE locking assumptions.
@danieldonoghue
danieldonoghue marked this pull request as ready for review August 21, 2026 09:42
@rfuchs

rfuchs commented Aug 21, 2026

Copy link
Copy Markdown
Member

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

@danieldonoghue

Copy link
Copy Markdown
Author

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

Copy link
Copy Markdown
Author

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.

@danieldonoghue
danieldonoghue marked this pull request as draft August 21, 2026 13:58
@rfuchs

rfuchs commented Aug 21, 2026

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Author

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.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants