fix(mediator-transport): ack inbound TSP frames, after the consumer has finished - #11
Merged
Conversation
…as finished
The single-socket TSP demux added in 0.6.0 routed a `-E` frame to `onTspFrame`
and returned — before `_dispatchFrame`, which is the method that acks. The
assumption behind that was that TSP delivery is fire-and-forget. It is not.
The mediator does not treat TSP specially. `handle_inbound_tsp` stores a Direct
message
reusing the protocol-neutral store path that DIDComm direct delivery uses
base64url-encoded, and the live-stream path fetches with `DoNotDelete` because
redelivery is a notification re-cover, not an ack. The client deletes what
it has processed via the normal message-pickup path
So TSP obeys the same delete-to-ack contract as DIDComm, and nothing on this
side was ever deleting. Every TSP message a client received stayed queued at
the mediator and was redelivered on every reconnect, indefinitely.
Request/reply masked it completely. The consumer's waiter takes the first
delivery and every redelivery afterwards is discarded as a straggler with no
outstanding request, so the visible behaviour is correct while the inbox grows
without bound and reconnect traffic scales with everything the client has ever
received. Neither surfaces as an error anywhere, which is why this survived a
live-validated cutover.
`_dispatchTspFrame` applies the ordering `_dispatchFrame` already applies to
DIDComm — hand off, **await**, then ack — reusing `sha256(text)` as the
queue-id. That computation is correct unchanged: the mediator stores the qb64
text form, which is the form it delivers, so the digest is over the same bytes
on both sides. `_seen` now covers TSP, so a redelivery after a lost or racing
ack is re-acked without being dispatched twice.
Two deliberate differences from the DIDComm path:
- **No `isQueued` sender check.** That check exists so the mediator's own
status/problem-report frames are not acked — acking one provokes another
status, in an endless loop. The mediator speaks DIDComm JSON to us and
never emits a `-E` frame of its own, and this transport is key-blind for
TSP so it could not read a sender to check. Every `-E` frame is a queued
message.
- **A throwing consumer withholds the ack.** `_deliver` swallows an
`onMessage` throw and acks regardless; here a throw means the consumer did
not persist, so the message is redelivered rather than deleted. The DIDComm
path's swallow-then-ack is older behaviour and narrowing it would change
delivery for every existing listener — a separate change, not a drive-by.
`onTspFrame` may now return a promise and is awaited. A synchronous handler is
unaffected; an asynchronous one — which is what R1.6 requires of an MV3 host —
completes before the mediator is told to delete its only other copy. A client
built without `onTspFrame` no longer acks TSP frames at all: nothing has
handled them, so they stay queued.
Four tests; three of them fail against the previous implementation, on the
ack ordering, the withheld ack after a throw, and the re-ack-without-redispatch
of a redelivery.
Signed-off-by: Glenn Gore <glenn.g@affinidi.com>
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.
TSP delivery is not fire-and-forget
The single-socket TSP demux added in 0.6.0 routes a
-Eframe toonTspFrameand returns:That return is before the ack because TSP was assumed to be fire-and-forget. It isn't. The mediator does not treat TSP specially —
handle_inbound_tspstores a Direct messagebase64url-encoded, and the live-stream path fetches with
DoNotDeletebecauseSo TSP obeys the same delete-to-ack contract as DIDComm, and nothing on this side was ever deleting.
What that has been doing
Every TSP message a client received has stayed queued at the mediator, and is redelivered on every reconnect, indefinitely.
Request/reply masks it completely: the consumer's waiter takes the first delivery, and every redelivery afterwards is discarded as a straggler with no outstanding request. So the visible behaviour is correct while the inbox grows without bound and reconnect traffic scales with everything the client has ever received. Neither shows up as an error anywhere, which is how it survived a live-validated cutover.
It also means R1.6 does not currently hold for TSP at all — there is no ack to order against — which blocks using TSP for anything a consumer must durably persist before acknowledging.
The fix
_dispatchTspFrameapplies the ordering_dispatchFramealready applies to DIDComm: hand off, await, then ack, reusingsha256(text)as the queue-id.That computation is correct unchanged — the mediator stores the qb64 text form, which is the form it delivers, so the digest is over the same bytes on both sides.
_seennow covers TSP, so a redelivery after a lost or racing ack is re-acked without being dispatched twice.onTspFramemay now return a promise and is awaited. A synchronous handler is unaffected; an asynchronous one — which is what R1.6 requires of an MV3 host — finishes before the mediator is told to delete its only other copy.Two deliberate differences from the DIDComm path
Both documented at the method, because neither is obvious:
No
isQueuedsender check. That check exists so the mediator's own status/problem-report frames are not acked — acking one provokes another status, in an endless ~300ms loop. The mediator speaks DIDComm JSON to us and never emits a-Eframe of its own, and this transport is key-blind for TSP so it could not read a sender to check anyway. Every-Eframe is a queued message.A throwing consumer withholds the ack.
_deliverswallows anonMessagethrow and acks regardless; here a throw means the consumer did not persist, so the message is redelivered rather than deleted.That asymmetry is deliberate but worth naming: the DIDComm path's swallow-then-ack is a real hole — a listener that throws mid-persist loses the message permanently. Narrowing it would change delivery for every existing listener (a routinely-throwing one would start looping on redelivery), so it belongs in its own change rather than as a drive-by here.
A client built without
onTspFrameno longer acks TSP frames at all: nothing has handled them, so they stay queued. That is a behaviour change, and the intended one.Tests
Four added. Three fail against the previous implementation:
The fourth (
a TSP frame with no consumer is not acked either) passes on both — it pins a property that was already true and must stay true.216 tests pass;
npm run build:typesis clean.Version
Minor bump to 0.7.0. A synchronous
onTspFramekeeps working, so this is not breaking, but the ack is new outbound traffic and the no-handler case changes behaviour — both worth a minor rather than a patch.Noticed, not fixed
CHANGELOG.mdhas duplicated0.6.2and0.6.1sections (now at lines 67/91 and 117/141), apparently from the 0.6.x backfill in bbc22ce. I inserted the 0.7.0 entry above them rather than fold a docs cleanup into a functional change — worth a one-line follow-up.Downstream
pnm-browser-pluginneeds this to route unsolicited inbound TSP (consent and step-up pushes) into its persist-before-ack path; its^0.6.2floor moves to^0.7.0once this is published.