From 208dfd759272aae5fe192d8c36c1bdc6cf2fc515 Mon Sep 17 00:00:00 2001 From: krishrathi1 Date: Sun, 9 Aug 2026 15:06:41 +0530 Subject: [PATCH] fix(tests): drive announce admission from an injectable clock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `SimulatedMeshTests.publicMessageRelaysAcrossLineTopologyWithinTTLBudget` flakes in CI on PRs that never touch BLE or mesh code: (mesh.deliveredFrameCount - baseline → 16) <= 12 The harness advances only simulated time, but the announce-admission decision in `sendAnnounceNow` read wall-clock `Date()`. On a loaded runner the 0.15 s forced-minimum window elapses *between* announces the simulation places at a single instant, so announce-backs and afterglows that should have been throttled are admitted instead and fan out to every neighbour — 12 frames becomes 16. The bound was measuring machine speed, not relay behaviour. Give `BLEService` an injectable `dateProvider`, defaulted to `Date.init` the way `BLEIncomingFileStore` and the other services here already do, and read it at the admission gate. `BLEEngineManualScheduler` exposes the counter `advance(by:)` already moves as a `Date`, and `SimulatedMesh` hands that in, so the throttle window now moves with `advanceTime` and only with `advanceTime`. Production behaviour is unchanged: the default provider is `Date.init`, so every non-test caller keeps the wall clock it had. Adds `announceAdmissionFollowsSimulatedTimeNotWallClock`, which pins both halves on a lone node (nothing answers it, so every emitted announce is one its own throttle admitted): a second forced announce at the same simulated instant is refused, and one issued after the window has elapsed in simulated time is admitted. Also corrects the comments in the harness and in `_test_resetAnnounceThrottle` that described the window as wall-clock and unreachable from scheduler time. The reset stays: it is still how a test gets a second announce without advancing time and releasing other scheduled work. Fixes #1630 Fixes #1621 --- bitchat/Services/BLE/BLEService.swift | 23 +++++++--- .../Mocks/BLEEngineManualScheduler.swift | 11 +++++ bitchatTests/Simulation/SimulatedMesh.swift | 22 ++++++---- .../Simulation/SimulatedMeshTests.swift | 44 ++++++++++++++++++- 4 files changed, 83 insertions(+), 17 deletions(-) diff --git a/bitchat/Services/BLE/BLEService.swift b/bitchat/Services/BLE/BLEService.swift index 5f9b97af..e4fc6c1d 100644 --- a/bitchat/Services/BLE/BLEService.swift +++ b/bitchat/Services/BLE/BLEService.swift @@ -327,7 +327,14 @@ final class BLEService: NSObject { // Simple announce throttling private let announceThrottle = BLEAnnounceThrottle() - + /// Clock backing announce admission. Injectable so a test harness that + /// advances only simulated time can move the throttle window too; + /// production passes `Date.init`. Without this the admission decision + /// reads the runner's wall clock while the harness's clock stands still, + /// which makes the simulated-mesh storm bound a measure of machine speed + /// rather than relay behaviour. + private let dateProvider: () -> Date + // Application state tracking (thread-safe) #if os(iOS) var isAppActive: Bool = true // Assume active initially @@ -509,9 +516,11 @@ final class BLEService: NSObject { startSuspendedForPanicRecovery: Bool = false, noiseResponderHandshakeTimeout: TimeInterval = NoiseSecurityConstants.ordinaryResponderHandshakeTimeout, - engineScheduler: BLEEngineScheduling = BLEEngineDispatchScheduler() + engineScheduler: BLEEngineScheduling = BLEEngineDispatchScheduler(), + dateProvider: @escaping () -> Date = Date.init ) { self.engineScheduler = engineScheduler + self.dateProvider = dateProvider self.keychain = keychain self.idBridge = idBridge self.incomingFileStore = incomingFileStore @@ -2757,7 +2766,7 @@ final class BLEService: NSObject { // after this announce was scheduled but before it runs. guard !isPanicSuspended else { return } // Throttle announces to prevent flooding - if !announceThrottle.shouldSend(force: forceSend, now: Date()) { + if !announceThrottle.shouldSend(force: forceSend, now: dateProvider()) { return } @@ -3097,9 +3106,11 @@ extension BLEService { onEngine { sendAnnounceNow(forceSend: true) } } - /// Clears the announce throttle's wall-clock debt — the simulator's - /// stand-in for "enough real time has passed", since scheduler time - /// cannot move the throttle's Date-based window. Deliberately NOT + /// Clears the announce throttle's debt so an announce issued at the + /// same instant as the previous one is admitted. With an injected + /// clock the simulator can also just advance time past the window; + /// this stays for the cases that want a second announce *without* + /// moving time (and so releasing scheduled work). Deliberately NOT /// part of `_test_forceAnnounce`: the panic-rotation mesh test relies /// on the production panic path performing its own reset, and a /// blanket reset here would mask that regression. diff --git a/bitchatTests/Mocks/BLEEngineManualScheduler.swift b/bitchatTests/Mocks/BLEEngineManualScheduler.swift index cfe4d712..86844241 100644 --- a/bitchatTests/Mocks/BLEEngineManualScheduler.swift +++ b/bitchatTests/Mocks/BLEEngineManualScheduler.swift @@ -24,6 +24,17 @@ final class BLEEngineManualScheduler: BLEEngineScheduling, @unchecked Sendable { lock.withLock { pending.count } } + /// The same counter `advance(by:)` moves, exposed as a `Date` so + /// production code whose decisions are windowed on elapsed time (the + /// announce throttle) can be driven by test time instead of the + /// runner's speed. Anchored at a plausible wall-clock instant rather + /// than the epoch so any timestamp derived from it stays sane. + var currentDate: Date { + lock.withLock { Self.epoch.addingTimeInterval(now) } + } + + private static let epoch = Date(timeIntervalSince1970: 1_780_000_000) + /// Advances the clock, releasing due work in deadline order. /// Cancellation keeps its production semantics: dispatch skips a /// cancelled `DispatchWorkItem` at execution. diff --git a/bitchatTests/Simulation/SimulatedMesh.swift b/bitchatTests/Simulation/SimulatedMesh.swift index c4bf6312..2dc5186e 100644 --- a/bitchatTests/Simulation/SimulatedMesh.swift +++ b/bitchatTests/Simulation/SimulatedMesh.swift @@ -77,7 +77,11 @@ final class SimulatedMesh { idBridge: idBridge, identityManager: identityManager, initializeBluetoothManagers: false, - engineScheduler: scheduler + engineScheduler: scheduler, + // Announce admission reads this, so the throttle window moves + // with `advanceTime` instead of with however long the runner + // took to get here. + dateProvider: { scheduler.currentDate } ) let index = nodes.count let node = Node(service: service, scheduler: scheduler) @@ -91,9 +95,8 @@ final class SimulatedMesh { // The tap must be live before `setNickname` below: setNickname // force-announces asynchronously on the engine, and if that slot // ran in the gap before a later tap install, the announce was - // emitted invisibly while still stamping the wall-clock announce - // throttle — swallowing `announceAll`'s forced announce on a - // starved runner (the CI flake this ordering fixes). + // emitted invisibly while still stamping the announce throttle — + // swallowing `announceAll`'s forced announce. service._test_onOutboundPacket = { [weak self] packet in // Runs on the sender's engine; only buffer here — delivering // inline would nest one engine inside another. @@ -210,11 +213,12 @@ final class SimulatedMesh { /// Full discovery round: every node announces, traffic settles. /// - /// Resets each node's announce throttle first: the throttle window is - /// wall-clock, so any announce that already ran (setNickname's, in - /// `addNode`) would otherwise swallow this forced one whenever the two - /// land within the forced minimum interval — which is always, on any - /// runner. `forceAnnounce(from:)` deliberately does NOT reset — the + /// Resets each node's announce throttle first: `setNickname`'s announce + /// in `addNode` lands at the same simulated instant as this one, well + /// inside the forced minimum interval, and would otherwise swallow it. + /// (Advancing time would clear the window too, but it would also + /// release scheduled work that callers of `announceAll` have not asked + /// for yet.) `forceAnnounce(from:)` deliberately does NOT reset — the /// panic-rotation tests pin the production reset behavior through it. func announceAll() { for node in nodes { diff --git a/bitchatTests/Simulation/SimulatedMeshTests.swift b/bitchatTests/Simulation/SimulatedMeshTests.swift index 512165d8..3b6a6f7e 100644 --- a/bitchatTests/Simulation/SimulatedMeshTests.swift +++ b/bitchatTests/Simulation/SimulatedMeshTests.swift @@ -99,6 +99,45 @@ struct SimulatedMeshTests { #expect(mesh.deliveredFrameCount(ofType: .message) - baseline <= 8) } + /// Guards the storm bound above. Announce admission used to consult the + /// wall clock while the harness advanced only simulated time, so on a + /// loaded runner the 0.15 s forced-minimum window elapsed *between* + /// announces that the simulation places at one instant. The extra + /// announces that let through fanned out to every neighbour and pushed + /// `deliveredFrameCount` past the bound — the test measured machine + /// speed, not relay behaviour. Admission must move with simulated time + /// and only with simulated time. + /// + /// A lone node keeps this exact: nothing answers it, so every emitted + /// announce is one its own throttle admitted. + @Test + func announceAdmissionFollowsSimulatedTimeNotWallClock() { + let mesh = SimulatedMesh() + _ = mesh.addNode(nickname: "alice") + mesh.announceAll() + + func announceCount() -> Int { + mesh.emittedPackets(from: 0) + .filter { $0.type == MessageType.announce.rawValue } + .count + } + + // Same simulated instant: refused, however many real milliseconds + // the runner burned between `announceAll` and here. + let admitted = announceCount() + mesh.forceAnnounce(from: 0) + #expect(announceCount() == admitted) + + // Simulated time is the only thing that opens the window. The first + // advance drains any work already scheduled, so the second crosses + // the forced minimum interval with nothing else due. + mesh.advanceTime(by: 5) + mesh.advanceTime(by: TransportConfig.bleForceAnnounceMinIntervalSeconds + 0.01) + let beforeAdmitted = announceCount() + mesh.forceAnnounce(from: 0) + #expect(announceCount() == beforeAdmitted + 1) + } + @Test func duplicateFloodIsDeliveredOnce() async { let mesh = SimulatedMesh() @@ -154,7 +193,8 @@ struct SimulatedMeshTests { // A fresh announce over the (re-established) link binds and // reconnects — the same heal path a real reconnection drives. - // (The announce throttle runs on wall clock; model elapsed time.) + // (Clear the throttle rather than advance time: this announce must + // land without releasing any other scheduled work.) b.service._test_resetAnnounceThrottle() mesh.forceAnnounce(from: 1) mesh.settleUntil { @@ -312,7 +352,7 @@ struct SimulatedMeshTests { // Containment: further announces (and the rebind cooldown) leave // the healed binding alone — no flip-flop back to the dead ID. - // (Reset the wall-clock announce throttles so these actually send.) + // (Reset the announce throttles so these actually send.) b.service._test_resetAnnounceThrottle() mesh.forceAnnounce(from: 1) a.service._test_resetAnnounceThrottle()