diff --git a/js/net/src/auth.test.ts b/js/net/src/auth.test.ts index 96ab771a01b..c947339655a 100644 --- a/js/net/src/auth.test.ts +++ b/js/net/src/auth.test.ts @@ -134,6 +134,99 @@ describe.each([Lite.ALPN_07_WIP, Ietf.ALPN.DRAFT_17, Ietf.ALPN.DRAFT_22])("%s", second.server.abort(); }); + test("a shared handle presents a token whose stream is lost again on the same session", async () => { + const clock = new Manual(); + const installed = clock.install(); + const warned = spyOn(console, "warn").mockImplementation(() => {}); + // The server's side of each AUTH stream carrying our token, in arrival order. + const Wire = protocol === Lite.ALPN_07_WIP ? LiteAuthWire : IetfAuthWire; + const original = Wire.prototype.accept; + const streams: Stream[] = []; + const accepted = spyOn(Wire.prototype, "accept").mockImplementation(async function ( + this: InstanceType, + stream: Stream, + ) { + const token = await original.call(this, stream); + if (new TextDecoder().decode(token) === "token") streams.push(stream); + return token; + }); + const handle = new Handle(); + const { client, server } = await connect({ publish: new OriginProducer(), protocol, auth: handle }); + answer(server.auth.requests(), (token) => (token ? grant(["a"], []) : grant([], []))); + try { + const token = await handle.add("token"); + await waitFor(token.grant, (g) => g?.publish.equals(patterns("a")) === true); + let ended = false; + void token.closed.then(() => { + ended = true; + }); + + streams[0]?.abort(new StreamError(StreamCode.Internal, { message: "lost" })); + await waitFor(token.grant, (g) => g === undefined); + await clock.advance(Nano.fromMilli(Milli(999))); + for (let i = 0; i < 10; i++) await nextTask(); + expect(streams.length).toBe(1); + await clock.advance(Nano.fromMilli(Milli(1))); + await waitFor(token.grant, (g) => g?.publish.equals(patterns("a")) === true); + expect(streams.length).toBe(2); + expect(ended).toBe(false); + } finally { + accepted.mockRestore(); + warned.mockRestore(); + client.abort(); + server.abort(); + installed.close(); + } + }); + + test("a token whose peer resets only its sending half ends that stream before presenting again", async () => { + const clock = new Manual(); + const installed = clock.install(); + const warned = spyOn(console, "warn").mockImplementation(() => {}); + const Wire = protocol === Lite.ALPN_07_WIP ? LiteAuthWire : IetfAuthWire; + const original = Wire.prototype.accept; + const streams: Stream[] = []; + const accepted = spyOn(Wire.prototype, "accept").mockImplementation(async function ( + this: InstanceType, + stream: Stream, + ) { + const token = await original.call(this, stream); + if (new TextDecoder().decode(token) === "token") streams.push(stream); + return token; + }); + const handle = new Handle(); + const { client, server } = await connect({ publish: new OriginProducer(), protocol, auth: handle }); + answer(server.auth.requests(), (token) => (token ? grant(["a"], []) : grant([], []))); + try { + const token = await handle.add("token"); + await waitFor(token.grant, (g) => g?.publish.equals(patterns("a")) === true); + const first = streams[0]; + if (!first) throw new Error("the token was never presented"); + let abandoned: unknown; + first.reader.closed.then( + () => { + abandoned = "finished"; + }, + (err: unknown) => { + abandoned = err; + }, + ); + + first.writer.reset(new StreamError(StreamCode.Internal, { message: "lost" })); + await waitFor(token.grant, (g) => g === undefined); + await clock.advance(Nano.fromMilli(Milli(1000))); + await waitFor(token.grant, (g) => g?.publish.equals(patterns("a")) === true); + expect(streams.length).toBe(2); + expect(abandoned).toBeInstanceOf(Error); + } finally { + accepted.mockRestore(); + warned.mockRestore(); + client.abort(); + server.abort(); + installed.close(); + } + }); + test("a handle backs one active session", async () => { const handle = new Handle(); const first = await connect({ publish: new OriginProducer(), protocol, auth: handle }); @@ -693,8 +786,9 @@ test("lite-07 presents a transiently refused token again after a backoff", async } }); -// The peer replied to the first presentation, so it knows AUTH: a reset is a lost stream. -test("lite-07 keeps a shared token whose retry stream is reset", async () => { +// The peer replied to the first presentation, so it knows AUTH: a reset is a lost stream, +// presented again after the doubled backoff. +test("lite-07 presents a shared token again when its retry stream is reset", async () => { const clock = new Manual(); const installed = clock.install(); const warned = spyOn(console, "warn").mockImplementation(() => {}); @@ -726,6 +820,10 @@ test("lite-07 keeps a shared token whose retry stream is reset", async () => { await settles(() => logged("auth token ended") > 0); for (let i = 0; i < 10; i++) await nextTask(); expect(rejected).toBeUndefined(); + + await clock.advance(Nano.fromMilli(Milli(2000))); + await settles(() => logged("auth token ended") > 1); + expect(rejected).toBeUndefined(); } finally { read?.mockRestore(); client.abort(); diff --git a/js/net/src/auth_session.ts b/js/net/src/auth_session.ts index 351f5d57347..3ddcae27b63 100644 --- a/js/net/src/auth_session.ts +++ b/js/net/src/auth_session.ts @@ -67,6 +67,12 @@ function transient(err: Error | null): boolean { ); } +// Whether a token's stream ending with `result` is the peer's verdict on it: a refusal that will +// not pass, Unsupported, or a clean close after a grant. Anything else left the token undecided. +function isVerdict(refused: boolean, granted: boolean, result: Error | null): boolean { + return (refused && !transient(result)) || result instanceof Unsupported || (result === null && granted); +} + /** A grant as the wire carries it: the expiry is milliseconds from now. @internal */ export interface WireGrant { publish: Path.Patterns; @@ -341,7 +347,8 @@ export class Handle { * Present another token on every session of this handle, resolving once a peer answers it. * * With no session live it waits for the next one. A refusal that may pass, such as an - * unreachable auth server, is presented again after a backoff on moq-lite. Rejects with + * unreachable auth server, is presented again after a backoff on moq-lite, and so is a token + * whose stream the peer drops without a verdict. Rejects with * {@link Unsupported} when a session neither carries AUTH nor lets a request carry a token * (moq-lite without AUTH), with the peer's refusal as an `Error.Session`, or with the * handle's end. On moq-transport without AUTH it resolves at once with no grant: only a @@ -858,7 +865,7 @@ export class AuthSession { if ("grant" in reply) { const expires = reply.grant.expires === undefined ? undefined : wall() + reply.grant.expires; presented.grant = { publish: reply.grant.publish, subscribe: reply.grant.subscribe, expires }; - answered = granted = true; + answered = granted = replied = true; delay = undefined; this.#replied = true; this.#answered(presented); @@ -879,6 +886,9 @@ export class AuthSession { break; } } catch (err: unknown) { + // End both halves: a peer that reset only its own leaves ours open, and a retry that + // abandoned it would hold the stream's credit until the session ends. + presented.stream?.abort(cancel()); closed = transportClosed(err); if (presented.withdrawn || presented.ended) { result = null; @@ -895,10 +905,21 @@ export class AuthSession { } presented.stream = undefined; - // A refusal that may pass, such as an unreachable auth server: present the token - // again after a backoff while it is still wanted here. Only moq-lite's AUTH_ERROR - // carries such a code. - if (!refused || !transient(result) || this.#closed || presented.withdrawn) break; + // Present the token again after a backoff while it is still wanted on an active + // session: after a refusal that may pass, such as an unreachable auth server (only + // moq-lite's AUTH_ERROR carries such a code), or, on a shared handle, after its stream + // was lost without a verdict, so the session never runs on without it. A handle of the + // session's own ends a lost token instead, which tells the application. + if ( + isVerdict(refused, granted, result) || + closed || + !this.active || + presented.withdrawn || + presented.ended || + result instanceof ProtocolViolation || + (!refused && this.#state.own) + ) + break; presented.grant = undefined; granted = false; // Unanswered again, as on its first presentation: a request or publication the token @@ -934,7 +955,8 @@ export class AuthSession { if (owner && this.#byToken.get(owner) === presented) this.#byToken.delete(owner); this.#ended.update((n) => n + 1); // Only the peer's verdict ends one of the handle's tokens on every session: a refusal, - // Unsupported, or a clean close after a grant. Anything else leaves it for the next. + // Unsupported, or a clean close after a grant. Anything else leaves it for the next + // session, since an active one presents it again instead. // A draining session's verdict is stale while an active session still presents the // token: that session's own answer decides, even mid-retry with no grant right now. const superseded = @@ -942,12 +964,12 @@ export class AuthSession { owner !== undefined && this.#state.sessions.some((session) => session !== this && session.active && session.presents(owner)); // A handle of the session's own has no next session to present it, so any end is final. - const verdict = + const settled = !this.#closed && !presented.withdrawn && !superseded && - (this.#state.own || refused || result instanceof Unsupported || (result === null && granted)); - if (owner && verdict) this.#state.verdict(owner, result); + (this.#state.own || isVerdict(refused, granted, result)); + if (owner && settled) this.#state.verdict(owner, result); // Closed before the union shrinks, so the scope never reads it as a revocation. if (closed) this.close(); this.#recompute(); diff --git a/quest/m1/auth/README.md b/quest/m1/auth/README.md index 2dc74db2535..65bb8c2ee58 100644 --- a/quest/m1/auth/README.md +++ b/quest/m1/auth/README.md @@ -148,7 +148,6 @@ because `main` assigned 0x3A to NOT_FETCHABLE first. AUTH streams: a session starts on the token its SETUP or URL carried and its AUTH streams add the rest - [Binding auth polish](/quest/m1/auth/bindings-polish.md) - OBS reconnects with the presented token, Swift grant sequences -- [Re-present a lost token](/quest/m1/auth/token-represent.md) - a token whose AUTH stream ended without a verdict is presented again on the live session ## Related diff --git a/quest/m1/auth/token-represent.md b/quest/m1/auth/token-represent.md deleted file mode 100644 index 59a465634aa..00000000000 --- a/quest/m1/auth/token-represent.md +++ /dev/null @@ -1,17 +0,0 @@ -# [M] A lost AUTH stream re-presents its token - -## Goal - -When a session's AUTH stream ends without a verdict, the shared auth handle -presents its token again on the same session, with backoff, on lite and -moq-transport. The session never runs on without a token it was given. - -## Plan - -Since [moq-dev/moq#6109](https://github.com/moq-dev/moq/pull/6109), a token -whose AUTH stream is lost waits for the next session. The moq-transport -presenter has no retry loop at all. Retry on the live session with the -transient-refusal backoff (1s doubling to 30s), stop on a verdict or session -end, and test both wires with mocked time. - -Public API: none expected. Wire: none. diff --git a/rs/moq-net/src/auth.rs b/rs/moq-net/src/auth.rs index 92bab3af116..bf182107ff2 100644 --- a/rs/moq-net/src/auth.rs +++ b/rs/moq-net/src/auth.rs @@ -159,6 +159,16 @@ fn transient(err: &Error) -> bool { ) } +/// Whether a token's stream ending with `err` is the peer's verdict on it: a refusal that +/// will not pass, [`Error::Unsupported`], or a clean close after a grant. Anything else +/// left the token undecided. +fn verdict(slot: &Slot, err: &Error) -> bool { + (slot.refused && !transient(err)) + || matches!(err, Error::Unsupported) + // The peer ended the grant without revoking it. + || (matches!(err, Error::Cancel) && slot.grant.is_some()) +} + /// The shared state behind one [`Handle`]: the tokens it presents, and every session /// it backs. #[derive(Default)] @@ -524,7 +534,8 @@ impl Handle { /// /// The grant joins the union while any clone of the returned [`Auth`] lives. With no /// session live it waits for the next one. A refusal that may pass, such as an - /// unreachable auth server, is presented again after a backoff. On moq-transport + /// unreachable auth server, is presented again after a backoff, and so is a token whose + /// stream the peer drops without a verdict. On moq-transport /// without AUTH there is no answer to wait for: it resolves at once with /// [`Answer::Requests`] and a grant that stays unknown, and only a request that names it /// carries the token. @@ -960,8 +971,7 @@ impl Link { /// Record an AUTH_ERROR for the token: a reply that grants nothing, so a refused /// setup token leaves an empty union rather than an unknown (unrestricted) one. - /// The driver then calls [`retry`](Self::retry), and [`ended`](Self::ended) unless it - /// presents the token again. + /// The driver then calls [`retry`](Self::retry), as on every end of the token's stream. pub(crate) fn refused(&self, id: u64) { self.with(|link| { if let Some(slot) = link.slots.get_mut(&id) { @@ -971,15 +981,27 @@ impl Link { }); } - /// The delay before presenting a refused token again, when the refusal `err` may pass - /// and the token is still wanted here; `None` when the driver should end it instead. + /// The delay before presenting a token again once its stream ended with `err`, or `None` + /// when the driver should end it instead. + /// + /// A token still wanted on an active session is presented again when the peer refused it + /// for a reason that may pass, or, on a shared handle, when its stream was lost without a + /// verdict, so the session never runs on without it. A session's own handle ends a lost + /// token instead, which tells the application. Both back off from [`RETRY_INITIAL`], + /// doubling until a grant resets it. pub(crate) fn retry(&self, id: u64, err: &Error) -> Option { - self.with(|link| { - if link.closed.is_some() || !transient(err) { + self.with_state(|state, link| { + let bound = state.bound; + let link = attached(&mut state.links, link); + if !link.active() { return None; } let slot = link.slots.get_mut(&id)?; - if !slot.refused || slot.withdrawn || slot.ended.is_some() { + // A protocol violation ends the session anyway. + if slot.withdrawn || slot.ended.is_some() || verdict(slot, err) || matches!(err, Error::ProtocolViolation) { + return None; + } + if bound && !slot.refused { return None; } slot.refused = false; @@ -1018,8 +1040,9 @@ impl Link { }) } - /// End the token on this session: refused or revoked by the peer, withdrawn by us, or - /// its stream lost. A token that was never answered records `err` as its answer. The + /// End the token on this session, once [`retry`](Self::retry) declined to present it + /// again: refused or revoked by the peer, withdrawn by us, or its stream lost while the + /// session drains. A token that was never answered records `err` as its answer. The /// peer's verdict ends one of the handle's tokens on every session; anything else /// leaves it for the next session to present, except on a handle of the session's own. pub(crate) fn ended(&self, id: u64, err: Error) { @@ -1040,16 +1063,12 @@ impl Link { let Some(slot) = link.slots.get_mut(&id) else { return; }; - let verdict = !slot.setup + let settled = !slot.setup && !superseded && link.closed.is_none() && !slot.withdrawn && slot.ended.is_none() - && (bound - || slot.refused - || matches!(err, Error::Unsupported) - // The peer ended the grant without revoking it. - || (matches!(err, Error::Cancel) && slot.grant.is_some())); + && (bound || verdict(slot, &err)); slot.grant = None; slot.answered.get_or_insert(Err(err.clone())); slot.ended.get_or_insert(err.clone()); @@ -1060,7 +1079,7 @@ impl Link { // Even with the union unchanged, a request carrying this token must drop it. link.epoch += 1; link.recompute(); - if verdict { + if settled { state.end_token(id, err); } }); @@ -2797,7 +2816,9 @@ mod tests { let link = Link::new(true); let auth = link.present(Bytes::from_static(b"jwt"), false).unwrap(); let id = opened(&link); - link.ended(id, Error::Stream(crate::StreamError::Internal)); + let lost = Error::Stream(crate::StreamError::Internal); + assert_eq!(link.retry(id, &lost), None, "the application hears of a lost token"); + link.ended(id, lost); assert!(matches!( auth.poll_answered(&kio::Waiter::noop()), Poll::Ready(Err(Error::Stream(_))) @@ -2907,6 +2928,65 @@ mod tests { )); } + /// A shared handle's token whose stream is lost without a verdict is presented again on the + /// same session, doubling its backoff until a grant resets it. A request keeps carrying it + /// meanwhile, and a request it might cover waits for the retry's answer. + #[test] + fn a_lost_stream_is_presented_again() { + let handle = Handle::new(); + let link = Link::attach(&handle, true, true).unwrap(); + let auth = handle.present(Bytes::from_static(b"jwt")).unwrap(); + let id = opened(&link); + let lost = Error::Stream(crate::StreamError::Internal); + + link.granted(id, Grant::all()); + assert_eq!(link.retry(id, &lost), Some(RETRY_INITIAL)); + assert_eq!(auth.grant().peek(), None, "the lost stream's grant is gone"); + assert_eq!(link.token(&auth).as_deref(), Some(&b"jwt"[..])); + let mut gate = Gate::new(link.clone(), crate::Path::new("room").to_owned(), Direction::Publish); + assert!(gate.poll_ready(&kio::Waiter::noop()).is_pending()); + + assert_eq!(link.reopen(id).unwrap().as_ref(), b"jwt"); + assert_eq!(link.retry(id, &lost), Some(RETRY_INITIAL * 2)); + link.reopen(id).unwrap(); + link.granted(id, Grant::all()); + assert!(gate.poll_ready(&kio::Waiter::noop()).is_ready()); + assert_eq!(link.retry(id, &lost), Some(RETRY_INITIAL), "a grant resets the backoff"); + assert!(Selector::Token(id).ended(&handle.state.read()).is_none()); + + // A clean close after a grant is the peer's verdict, not a lost stream. + link.reopen(id).unwrap(); + link.granted(id, Grant::all()); + assert_eq!(link.retry(id, &Error::Cancel), None); + link.ended(id, Error::Cancel); + assert!(matches!( + Selector::Token(id).ended(&handle.state.read()), + Some(Error::Cancel) + )); + } + + /// A draining session presents nothing again, and a transient refusal there is no verdict: + /// the token waits for the successor. + #[test] + fn a_draining_session_leaves_an_undecided_token() { + let handle = Handle::new(); + let draining = Link::attach(&handle, true, false).unwrap(); + let _auth = handle.present(Bytes::from_static(b"jwt")).unwrap(); + let id = opened(&draining); + let (goaway, _protocol) = crate::goaway::Handle::new(false); + goaway.producer().send(crate::goaway::Goaway::new()).unwrap(); + draining.drain_with(std::sync::Arc::new(goaway)); + + let busy = Error::Session(SessionError::TooManyRequests); + draining.refused(id); + assert_eq!(draining.retry(id, &busy), None); + draining.ended(id, busy); + assert!(Selector::Token(id).ended(&handle.state.read()).is_none()); + + let successor = Link::attach(&handle, true, false).unwrap(); + assert_eq!(opened(&successor), id, "the successor presents it"); + } + /// A refusal that may pass is presented again after a doubling backoff, and settles nothing. #[test] fn a_transient_refusal_backs_off() { diff --git a/rs/moq-net/src/ietf/auth.rs b/rs/moq-net/src/ietf/auth.rs index a1fc89f857c..7f142f9adc0 100644 --- a/rs/moq-net/src/ietf/auth.rs +++ b/rs/moq-net/src/ietf/auth.rs @@ -268,24 +268,45 @@ pub(super) async fn run_present( }; let violation = violation.clone(); tasks.push(async move { - // After a GOAWAY, sent or received, the peer must not see new requests. AUTH_ERROR - // carries no code that may pass, so unlike moq-lite a refusal is never presented again. - let err = match present.handle.active() { - true => present.run(token).await, - false => Error::GoingAway, - }; - let err = crate::auth::protocol_error(err); - if matches!(err, Error::ProtocolViolation) { - violation.lock().get_or_insert(err.clone()); - } + let mut token = Ok(token); + let mut answered = false; + loop { + // After a GOAWAY, sent or received, the peer must not see new requests. + let err = match token { + Ok(token) if present.handle.active() => present.run(token, &mut answered).await, + Ok(_) => Error::GoingAway, + Err(err) => err, + }; + let err = match err { + // The peer negotiated the extension, but a reset before any reply still tells + // us nothing about the token: it earns nothing here, as on moq-lite. + Error::Stream(_) if !answered => Error::Unsupported, + err => crate::auth::protocol_error(err), + }; + // AUTH_ERROR carries no code that may pass, so unlike moq-lite only a lost + // request is presented again. + if let Some(delay) = present.handle.retry(present.id, &err) { + tracing::debug!(%err, ?delay, "auth token presenting again"); + token = present.reopen(delay).await; + continue; + } + if matches!(err, Error::ProtocolViolation) { + violation.lock().get_or_insert(err.clone()); + } - match &err { - Error::Cancel | Error::Unsupported | Error::Transport(_) | Error::IdleTimeout | Error::Session(_) => { - tracing::debug!(%err, "auth token ended") + match &err { + Error::Cancel + | Error::Unsupported + | Error::Transport(_) + | Error::IdleTimeout + | Error::Session(_) => { + tracing::debug!(%err, "auth token ended") + } + err => tracing::warn!(%err, "auth token ended"), } - err => tracing::warn!(%err, "auth token ended"), + present.handle.ended(present.id, err); + return; } - present.handle.ended(present.id, err); }); } std::future::pending().await @@ -305,9 +326,25 @@ struct Present { #[cfg(feature = "async")] impl Present { + /// Wait out a retry's `delay`, then take the token to present again. + async fn reopen(&self, delay: Duration) -> Result { + let mut deadline = crate::time::Deadline::after(&self.runtime, delay); + kio::wait(|waiter| { + if self.handle.poll_withdrawn(self.id, waiter).is_ready() { + return Poll::Ready(Err(Error::Cancel)); + } + std::task::ready!(deadline.poll(waiter)); + Poll::Ready(self.handle.reopen(self.id)) + }) + .await + } + /// Send the token, then track its grant until either side ends it, resolving with why. - #[cfg(feature = "async")] - async fn run(&self, token: Bytes) -> Error { + /// + /// `answered` records that the peer replied to the token, on this request or an + /// earlier one, so a request it closes or resets before replying is lost rather than + /// unsupported. + async fn run(&self, token: Bytes, answered: &mut bool) -> Error { let mut stream = match Stream::open_request(&mut self.session.clone(), self.version).await { Ok(stream) => stream, Err(err) => return err, @@ -321,7 +358,6 @@ impl Present { return err; } - let mut answered = false; let mut messages = super::request_stream::Messages::new(self.session.clone(), self.control.client()); loop { enum Next { @@ -345,7 +381,7 @@ impl Present { Next::Withdrawn => return Error::Cancel, Next::Reply(Ok(Some(super::request_stream::Framed(id, body)))) => (id, body), // The peer ended the grant without revoking it. - Next::Reply(Ok(None)) if answered => return Error::Cancel, + Next::Reply(Ok(None)) if *answered => return Error::Cancel, // A peer that closes before answering could not tell us anything. Next::Reply(Ok(None)) => return Error::Unsupported, Next::Reply(Err(err)) => return err, @@ -364,7 +400,7 @@ impl Present { Some(None) => return Error::ProtocolViolation, expires => expires.flatten(), }; - answered = true; + *answered = true; self.handle.granted( self.id, Grant { @@ -386,6 +422,7 @@ impl Present { if !matches!(err, Error::Unsupported | Error::GoingAway) { self.handle.refused(self.id); } + *answered = true; return err; } _ => return Error::UnexpectedMessage, @@ -604,7 +641,10 @@ mod tests { version, id: 0, }; - assert!(matches!(present.run(Bytes::new()).await, Error::NoStreamCredit)); + assert!(matches!( + present.run(Bytes::new(), &mut false).await, + Error::NoStreamCredit + )); assert_eq!( control.poll_next_request_id(&kio::Waiter::noop()), Poll::Ready(RequestId(first)) diff --git a/rs/moq-net/src/ietf/session.rs b/rs/moq-net/src/ietf/session.rs index 80f37892270..2875c771bc5 100644 --- a/rs/moq-net/src/ietf/session.rs +++ b/rs/moq-net/src/ietf/session.rs @@ -1777,6 +1777,113 @@ mod tests { } } + /// A granted token whose Auth request the peer resets is presented again on the same + /// session, backing off while each attempt is lost before a reply: at 0s, 1s, and 3s. + #[moq_clock::test] + async fn a_lost_auth_request_is_presented_again() { + let version = Version::Draft17; + let mut grant = Vec::new(); + let mut encoder = crate::coding::Encoder::new(&mut grant, version.into()); + encoder.varint(auth::AuthOk::ID).unwrap(); + encoder.u16(3); + grant.extend([0, 0, 0]); + let session = crate::lite::test_transport::ScriptedSession::per_stream(vec![grant.clone(), grant]); + session.close(crate::lite::test_transport::Close::ResetStream( + crate::ietf::error::to_stream_code(&crate::StreamError::Internal, version), + )); + let log = session.log.clone(); + let handle = crate::auth::Handle::new(); + let (driver, _) = start(Config { + runtime: crate::time::sim(), + stats: Default::default(), + session: crate::driver::Transport::new(session.clone()), + setup: None, + request_id_max: None, + limits: Default::default(), + client: true, + publish: None, + subscribe: None, + peer_hop: None, + cost: None, + version, + path: None, + authority: None, + peer_setup_stream: None, + peer_declared: Some(peer::Peer { + auth: true, + ..Default::default() + }), + auth: crate::auth::Link::attach(&handle, true, true).unwrap(), + early_unis: Vec::new(), + implementation: None, + peer_implementation: Default::default(), + extensions: Default::default(), + }) + .unwrap(); + let token = handle.present(bytes::Bytes::from_static(b"token")).unwrap(); + + let res = moq_clock::timeout(std::time::Duration::from_secs(5), driver).await; + assert!(res.is_err(), "the session ended: {res:?}"); + assert_eq!(occurrences(&log, b"token"), 3); + assert!( + futures::FutureExt::now_or_never(token.closed()).is_none(), + "a lost request ended the token" + ); + } + + /// A peer that negotiated MoQ Auth but resets a token's request before any reply gave it + /// nothing: the token fails as unsupported rather than being presented again. + #[moq_clock::test] + async fn an_auth_request_reset_before_its_reply_is_unsupported() { + let version = Version::Draft17; + let mut grant = Vec::new(); + let mut encoder = crate::coding::Encoder::new(&mut grant, version.into()); + encoder.varint(auth::AuthOk::ID).unwrap(); + encoder.u16(3); + grant.extend([0, 0, 0]); + // The setup token is granted; ours is reset before any reply. + let session = crate::lite::test_transport::ScriptedSession::per_stream(vec![grant, Vec::new()]); + session.close(crate::lite::test_transport::Close::ResetStream( + crate::ietf::error::to_stream_code(&crate::StreamError::Internal, version), + )); + let log = session.log.clone(); + let handle = crate::auth::Handle::new(); + let (driver, _) = start(Config { + runtime: crate::time::sim(), + stats: Default::default(), + session: crate::driver::Transport::new(session.clone()), + setup: None, + request_id_max: None, + limits: Default::default(), + client: true, + publish: None, + subscribe: None, + peer_hop: None, + cost: None, + version, + path: None, + authority: None, + peer_setup_stream: None, + peer_declared: Some(peer::Peer { + auth: true, + ..Default::default() + }), + auth: crate::auth::Link::attach(&handle, true, true).unwrap(), + early_unis: Vec::new(), + implementation: None, + peer_implementation: Default::default(), + extensions: Default::default(), + }) + .unwrap(); + let token = handle.present(bytes::Bytes::from_static(b"token")).unwrap(); + + let res = moq_clock::timeout(std::time::Duration::from_secs(5), driver).await; + assert!(res.is_err(), "the session ended: {res:?}"); + assert_eq!(occurrences(&log, b"token"), 1, "presented again"); + let err = futures::FutureExt::now_or_never(token.closed()).expect("the token is still open"); + assert!(matches!(err, Error::Unsupported), "{err:?}"); + } + #[moq_clock::test] async fn malformed_presenter_auth_closes_the_session() { for version in [Version::Draft17, Version::Draft22] { diff --git a/rs/moq-net/src/lite/session.rs b/rs/moq-net/src/lite/session.rs index c82da5fe430..77b6c2baf04 100644 --- a/rs/moq-net/src/lite/session.rs +++ b/rs/moq-net/src/lite/session.rs @@ -572,9 +572,11 @@ enum PresentState { stream: Stream, answered: bool, }, - /// Refused for a reason that may pass: present it again once the deadline fires. + /// Refused for a reason that may pass, or its stream lost: present it again once the + /// deadline fires. Retry { deadline: crate::time::Deadline, + answered: bool, }, Done, } @@ -583,34 +585,46 @@ impl kio::Task for PresentToken { type Output = (); fn poll(&mut self, waiter: &kio::Waiter) -> Poll<()> { - let err = ready!(self.poll_present(waiter)); - let answered = matches!( - self.state, - PresentState::Open { answered: true, .. } | PresentState::Run { answered: true, .. } - ); - if let PresentState::Run { stream, .. } = std::mem::replace(&mut self.state, PresentState::Done) { - stream.writer.abort(&Error::Cancel); - } - let err = match err { - // A peer that predates AUTH resets a stream type it does not know, and one - // that takes no tokens in band refuses the same way: either way, before any - // reply, this token earns nothing here. - Error::Stream(_) if !answered => Error::Unsupported, - // A malformed reply, truncated ones included, is the peer breaking the session - // protocol, not refusing a token. - err => crate::auth::protocol_error(err), - }; - match &err { - Error::Cancel | Error::Unsupported | Error::Transport(_) | Error::IdleTimeout | Error::Session(_) => { - tracing::debug!(%err, "auth token ended") + loop { + let err = ready!(self.poll_present(waiter)); + let answered = matches!( + self.state, + PresentState::Open { answered: true, .. } + | PresentState::Run { answered: true, .. } + | PresentState::Retry { answered: true, .. } + ); + if let PresentState::Run { stream, .. } = std::mem::replace(&mut self.state, PresentState::Done) { + stream.writer.abort(&Error::Cancel); } - err => tracing::warn!(%err, "auth token ended"), - } - if let Error::ProtocolViolation = err { - self.violation.lock().get_or_insert(err.clone()); + let err = match err { + // A peer that predates AUTH resets a stream type it does not know, and one + // that takes no tokens in band refuses the same way: either way, before any + // reply, this token earns nothing here. + Error::Stream(_) if !answered => Error::Unsupported, + // A malformed reply, truncated ones included, is the peer breaking the session + // protocol, not refusing a token. + err => crate::auth::protocol_error(err), + }; + if let Some(delay) = self.handle.retry(self.id, &err) { + tracing::debug!(%err, ?delay, "auth token presenting again"); + self.state = PresentState::Retry { + deadline: crate::time::Deadline::after(&self.runtime, delay), + answered, + }; + continue; + } + match &err { + Error::Cancel | Error::Unsupported | Error::Transport(_) | Error::IdleTimeout | Error::Session(_) => { + tracing::debug!(%err, "auth token ended") + } + err => tracing::warn!(%err, "auth token ended"), + } + if let Error::ProtocolViolation = err { + self.violation.lock().get_or_insert(err.clone()); + } + self.handle.ended(self.id, err); + return Poll::Ready(()); } - self.handle.ended(self.id, err); - Poll::Ready(()) } } @@ -690,29 +704,21 @@ impl PresentToken { self.handle.refused(self.id); } *answered = true; - let Some(delay) = self.handle.retry(self.id, &err) else { - return Poll::Ready(err); - }; - tracing::debug!(?delay, "auth token refused transiently; presenting it again"); - let retry = PresentState::Retry { - deadline: crate::time::Deadline::after(&self.runtime, delay), - }; - if let PresentState::Run { stream, .. } = std::mem::replace(&mut self.state, retry) { - stream.writer.abort(&Error::Cancel); - } + return Poll::Ready(err); } } } - PresentState::Retry { deadline } => { + PresentState::Retry { deadline, answered } => { if self.handle.poll_withdrawn(self.id, waiter).is_ready() { return Poll::Ready(Error::Cancel); } ready!(deadline.poll(waiter)); + let answered = *answered; let token = match self.handle.reopen(self.id) { Ok(token) => token, Err(err) => return Poll::Ready(err), }; - self.state = PresentState::Open { token, answered: true }; + self.state = PresentState::Open { token, answered }; } PresentState::Done => return Poll::Ready(Error::Cancel), } @@ -1003,11 +1009,8 @@ mod tests { assert_eq!(log.closes()[0].0, SessionError::ProtocolViolation.to_code()); } - /// A retry's stream that ends before its reply is lost, not a peer without AUTH: the - /// peer already answered the token, so a shared handle keeps it for the next session. - #[moq_clock::test] - async fn a_lost_retry_keeps_a_shared_token() { - let version = Version::Lite07; + /// An AUTH_OK granting nothing, forever. + fn empty_grant(version: Version) -> Vec { let mut grant = Vec::new(); let mut w = crate::coding::Encoder::new(&mut grant, version.into()); w.varint(0).unwrap(); @@ -1016,17 +1019,12 @@ mod tests { w.varint(0).unwrap(); } w.fill(prefix).unwrap(); - let mut busy = Vec::new(); - let mut w = crate::coding::Encoder::new(&mut busy, version.into()); - w.varint(1).unwrap(); - let prefix = w.prefix_varint(); - w.varint(SessionError::TooManyRequests.to_code().into()).unwrap(); - w.bytes(b"busy").unwrap(); - w.fill(prefix).unwrap(); + grant + } - // The setup token is granted, ours is refused transiently, and its retry's stream ends. - let transport = ScriptedSession::per_stream_eof(vec![grant, busy, Vec::new()]); - let log = transport.log.clone(); + /// Run a client session on a shared handle over `transport` for five simulated seconds, + /// presenting one token, and return it after checking the session still runs. + async fn present_on_shared_handle(transport: ScriptedSession) -> crate::auth::Auth { let handle = crate::auth::Handle::new(); let start = start(Config { runtime: crate::time::sim(), @@ -1038,7 +1036,7 @@ mod tests { publish: None, subscribe: None, peer_hop: None, - version, + version: Version::Lite07, our_setup: Setup::default(), peer_setup: None, auth: crate::auth::Link::attach(&handle, true, false).unwrap(), @@ -1053,7 +1051,50 @@ mod tests { ) .await; assert!(res.is_err(), "the session ended: {res:?}"); - assert_eq!(log.bi_opens(), 3, "the token was not presented again"); + token + } + + /// A granted token whose stream the peer resets is presented again on the same session, + /// backing off while each attempt is lost before a reply: at 0s, 1s, and 3s. + #[moq_clock::test] + async fn a_lost_stream_is_presented_again() { + let grant = empty_grant(Version::Lite07); + let transport = ScriptedSession::per_stream(vec![grant.clone(), grant]); + transport.close(crate::lite::test_transport::Close::ResetStream( + crate::StreamError::Internal.to_code(), + )); + let log = transport.log.clone(); + let token = present_on_shared_handle(transport).await; + + let writes = log.writes.lock().unwrap().clone(); + assert_eq!(writes.windows(5).filter(|window| *window == b"token").count(), 3); + assert!( + futures::FutureExt::now_or_never(token.closed()).is_none(), + "a lost stream ended the token" + ); + } + + /// A retry's stream that ends before its reply is lost, not a peer without AUTH: the + /// peer already answered the token, so it is presented again, backing off from the + /// refusal's delay: at 0s, 1s, and 3s. + #[moq_clock::test] + async fn a_lost_retry_is_presented_again() { + let version = Version::Lite07; + let mut busy = Vec::new(); + let mut w = crate::coding::Encoder::new(&mut busy, version.into()); + w.varint(1).unwrap(); + let prefix = w.prefix_varint(); + w.varint(SessionError::TooManyRequests.to_code().into()).unwrap(); + w.bytes(b"busy").unwrap(); + w.fill(prefix).unwrap(); + + // The setup token is granted and then closed, which settles it. Ours is refused + // transiently, and every retry's stream ends without a reply. + let transport = ScriptedSession::per_stream_eof(vec![empty_grant(version), busy]); + let log = transport.log.clone(); + let token = present_on_shared_handle(transport).await; + + assert_eq!(log.bi_opens(), 4, "the setup token once, ours three times"); assert!( futures::FutureExt::now_or_never(token.closed()).is_none(), "a lost retry ended the token" diff --git a/rs/moq-net/src/lite/test_transport.rs b/rs/moq-net/src/lite/test_transport.rs index 72d05e3ed83..d46cfdbb22d 100644 --- a/rs/moq-net/src/lite/test_transport.rs +++ b/rs/moq-net/src/lite/test_transport.rs @@ -23,6 +23,8 @@ use crate::transport::poll; #[derive(Debug, Clone, Default)] pub struct SinkError { code: u32, + /// The peer reset this one stream with this code, rather than failing the session. + stream: Option, } impl std::fmt::Display for SinkError { @@ -35,7 +37,14 @@ impl std::error::Error for SinkError {} impl crate::transport::Error for SinkError { fn session_error(&self) -> Option<(u32, String)> { - Some((self.code, "closed".to_string())) + match self.stream { + Some(_) => None, + None => Some((self.code, "closed".to_string())), + } + } + + fn stream_error(&self) -> Option { + self.stream } } @@ -95,6 +104,7 @@ impl Log { }) .map(|closed| SinkError { code: closed.expect("closed"), + stream: None, }) } @@ -893,7 +903,10 @@ pub struct Script { #[derive(Clone, Copy, Debug)] pub enum Close { Fin, + /// Fail the read as a session close would. Reset, + /// Reset just this stream with a code, as a peer that gave up on one stream would. + ResetStream(u32), } impl poll::Chunks for Script { @@ -912,6 +925,10 @@ impl poll::Chunks for Script { true => match *self.close.lock().unwrap() { Some(Close::Fin) => Poll::Ready(Ok(None)), Some(Close::Reset) => Poll::Ready(Err(SinkError::default())), + Some(Close::ResetStream(code)) => Poll::Ready(Err(SinkError { + stream: Some(code), + ..Default::default() + })), None => self.log.poll_closed(self.park.hold(cx)).map(Err), }, false => Poll::Ready(Ok(Some(chunk))),