[AIT-767] uts: live objects ground work#1209
Open
ttypic wants to merge 11 commits into
Open
Conversation
- Added `MockHttpClient`, `MockHttpEngine`, and `MockWebSocketEngineFactory` to simulate network interactions. - Extended `DebugOptions` for customizable engine injection. - Updated `HttpCore` and `WebSocketTransport` to support mock engines in debug mode.
WalkthroughThe PR adds a Clock/timer abstraction and SystemClock, replaces direct system-time and java.util.Timer usage across runtime components (HTTP, transport, realtime, hosts, auth, connection), adds test-time FakeClock and comprehensive HTTP/WebSocket mocks plus test client factories and await helpers, and supplies UTS→Kotlin guidance and example recovery tests with Gradle/CI wiring for the new uts module. ChangesClock abstraction and time-source injection throughout SDK and tests
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ge handling - Added `MockWebSocket` and `MockEvent` to capture WebSocket connection attempts, messages, and error scenarios. - Added connection lifecycle events (`ConnectionEstablished`, `ConnectionRefused`, etc.) for enhanced testing. - Updated `MockWebSocketEngineFactory` and related components to support event tracking and simulation.
- Added `Clock` interface and concrete implementations (`SystemClock` and `FakeClock`) for unified time management. - Refactored classes (`Auth`, `Presence`, `Hosts`, `WebSocketTransport`, etc.) to use `Clock` instead of direct system calls. - Enabled mockable time-based operations for improved testability. - Updated `DebugOptions` to support custom clocks in debug mode.
…in tests - Introduced a new skill for converting UTS pseudocode specs into runnable Kotlin tests. - Included detailed translation rules for pseudocode to Kotlin, mock setup, and assertions. - Added file templates and steps for compilation, testing, and handling deviations. - Enhanced developer workflow for UTS test authoring.
…ssage decoding - Added support for inspecting outgoing WebSocket frames (`MessageFromClient` events) in `MockWebSocket`. - Enhanced `ClientOptionsBuilder` to set `useBinaryProtocol = false` by default for JSON text frame testing. - Updated `ConnectionRecoveryTest` to assert on outgoing ATTACH messages.
…onnections - Added `queryParams` field and `parseQueryString` utility for `PendingConnection` implementations. - Updated `MockWebSocketEngineFactory` and `MockHttpEngine` to parse and pass query parameters. - Enhanced connection recovery tests with assertions on query parameters.
ttypic
force-pushed
the
AIT-767/live-object-ground-work
branch
from
ec7e6c4 to
c8f95df
Compare
- Added KDoc comments to `FakeClock`, `MockHttpClient`, and `MockWebSocket` explaining their purpose and usage.
ttypic
force-pushed
the
AIT-767/live-object-ground-work
branch
from
c8f95df to
c9d79f2
Compare
paddybyers
reviewed
May 18, 2026
| You are translating a UTS pseudocode test spec file into a runnable Kotlin test in the `uts` module. Follow these steps in order. | ||
|
|
||
| --- | ||
|
|
Member
There was a problem hiding this comment.
Should there be a reference to https://github.com/ably/specification/blob/main/uts/docs/writing-derived-tests.md ?
paddybyers
reviewed
May 18, 2026
| ## Step 1 — Read the spec | ||
|
|
||
| Read the file at `$ARGUMENTS`. Identify: | ||
| - All test cases (each has an ID like `RTN4a`, `RSC1`, etc. and a description) |
Member
There was a problem hiding this comment.
There's a structured ID for each test, eg realtime/unit/RSA4c2/callback-error-connecting-disconnected-0 (eg https://github.com/ably/specification/blob/main/uts/realtime/unit/auth/auth_callback_errors_test.md#rsa4c2---authcallback-error-during-connecting-transitions-to-disconnected)
Contributor
Author
There was a problem hiding this comment.
Yeah it's weird, because later in the doc it uses structured ID, updated
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
liveobjects/src/main/kotlin/io/ably/lib/objects/ServerTime.kt (1)
16-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope server-time offset per adapter/clock instead of globally.
ServerTimekeeps a single globalserverTimeOffset, but Line 25/Line 35 now use an adapter-derived clock. If different adapters (or tests using different clocks) call this, the cached offset from one clock can be reused with another, yielding incorrect server times.Also applies to: 24-35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@liveobjects/src/main/kotlin/io/ably/lib/objects/ServerTime.kt` around lines 16 - 17, ServerTime currently caches a single global serverTimeOffset (private var serverTimeOffset) which must be scoped per adapter/clock; change the cache to map offsets keyed by the adapter/clock identity (e.g., Map<Clock, Long?> or Map<AdapterId, Long?>) and update all uses in ServerTime methods (the code paths around where serverTimeOffset is read/updated — lines ~24-35) to look up and set the offset using the current adapter/clock key instead of the global variable so each clock has its own cached offset.
🧹 Nitpick comments (2)
lib/src/main/java/io/ably/lib/rest/Auth.java (1)
926-927: ⚡ Quick winKeep nanoTime drift detection tied to system wall clock.
The
nanoTimeDeltacheck is meant to detect local wall-clock jumps. Basing it on injectedclock.currentTimeMillis()can trigger false drift when custom/fake clocks are used, causing avoidableably.time()refreshes.Suggested fix
- long currentNanoTimeDelta = clock.currentTimeMillis() - System.nanoTime()/(1000*1000); + long currentNanoTimeDelta = System.currentTimeMillis() - System.nanoTime()/(1000*1000); ... - this.nanoTimeDelta = clock.currentTimeMillis() - System.nanoTime()/(1000*1000); + this.nanoTimeDelta = System.currentTimeMillis() - System.nanoTime()/(1000*1000);Also applies to: 1056-1057
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/main/java/io/ably/lib/rest/Auth.java` around lines 926 - 927, The nanoTime drift detection wrongly uses the injected clock (clock.currentTimeMillis()) which allows fake/custom clocks to mask or create false wall-clock jumps; change the computation of currentNanoTimeDelta to use the JVM wall-clock directly (System.currentTimeMillis()) together with System.nanoTime()/ (1000*1000) so the drift check is tied to the real system clock; update both occurrences (the currentNanoTimeDelta calculation around currentTimeMillis/System.nanoTime and the similar logic at the later occurrence around lines 1056-1057) to reference System.currentTimeMillis() instead of clock.currentTimeMillis() and keep the rest of the logic unchanged.uts/build.gradle.kts (1)
24-24: ⚡ Quick winScope forced test reruns instead of disabling up-to-date checks globally.
Line 24 forces every test task to rerun on every build, which can noticeably slow local iteration and CI. Gate this behind a property (or remove it) so it’s opt-in.
Proposed diff
tasks.withType<Test>().configureEach { @@ - outputs.upToDateWhen { false } + if (project.hasProperty("forceTestRerun")) { + outputs.upToDateWhen { false } + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@uts/build.gradle.kts` at line 24, The build currently forces all tests to rerun by unconditionally setting outputs.upToDateWhen { false }; change this to be opt-in by gating it behind a project property (e.g., "forceTestRerun"). Locate the block that sets outputs.upToDateWhen { false } (the test task configuration) and wrap it so it only executes when project.findProperty("forceTestRerun") == "true" (or similar), or remove the line altogether; ensure to document the property in the repo README or CI docs so callers can opt in when needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/skills/uts-to-kotlin/SKILL.md:
- Around line 42-50: Two fenced code blocks in SKILL.md (the one listing
test/mock files and the ASCII "Test fails" diagram) lack a language tag; add
"text" to both opening triple-backtick fences so they read ```text to satisfy
MD040. Locate the fenced block containing the list of test/mock files and the
fenced block containing the ASCII decision diagram and update their opening
fences to include the language tag without changing the block contents.
In `@lib/src/main/java/io/ably/lib/http/HttpScheduler.java`:
- Around line 291-297: The timeout loop in HttpScheduler.get(...) can hang when
using an injected clock that doesn't advance with real time because
wait(remaining) blocks by wall-clock while remaining is recomputed from
clock.currentTimeMillis(); change the wait to use a bounded sleep interval so
the loop re-checks the injected clock frequently. Specifically, in
HttpScheduler.get use wait(Math.min(remaining, <smallMillis>)) (e.g. 100ms)
instead of wait(remaining), keep recalculating remaining = deadline -
clock.currentTimeMillis(), and preserve the existing isDone check and
spurious-wakeup handling so the loop exits correctly when done or timed out.
In `@lib/src/main/java/io/ably/lib/util/SystemClock.java`:
- Around line 19-31: Replace per-call java.util.Timer creation in
SystemClock.newTimer() with a shared ScheduledExecutorService: add a private
static final ScheduledExecutorService (created with
Executors.newScheduledThreadPool(1, daemon ThreadFactory) or a small pool) and
in the NamedTimer implementation use scheduler.schedule(...) to schedule the
TimerTask (wrap TimerTask in a Runnable that calls run and catches/logs
throwables), return a TimerInstance that cancels the underlying ScheduledFuture,
and implement cancel() to cancel outstanding futures as appropriate; update
references to NamedTimer.schedule, TimerInstance and cancel to use
ScheduledFuture cancellation semantics to avoid one-thread-per-timer and to
handle exceptions safely.
In
`@uts/src/test/kotlin/io/ably/lib/realtime/unit/connection/ConnectionRecoveryTest.kt`:
- Around line 304-306: The test currently only asserts msgSerial when
RUN_DEVIATIONS is set; add an else branch to assert the default-path behavior by
checking currentRecoveryKey.msgSerial equals the SDK's expected default value.
In the else branch (i.e., when System.getenv("RUN_DEVIATIONS") == null) add
assertEquals(<SDK_DEFAULT_MSG_SERIAL>, currentRecoveryKey.msgSerial), using the
explicit SDK constant or literal used elsewhere in the codebase (or the test's
initial recorded value) so the non-deviation path cannot vacuously pass;
reference currentRecoveryKey.msgSerial and the RUN_DEVIATIONS env check to
locate where to add this assertion.
In `@uts/src/test/kotlin/io/ably/lib/test/mock/DefaultPendingRequest.kt`:
- Around line 20-31: The respondWith method must preserve the provided headers
and serialize non-binary bodies as JSON instead of calling toString(); update
DefaultPendingRequest.respondWith to pass the supplied headers into
HttpResponse.builder().headers(...) and, when body is not a ByteArray, serialize
it to UTF-8 JSON bytes (e.g., via your project's JSON serializer) and set the
content-type to "application/json" on HttpBody; ensure ByteArray bodies still
bypass serialization and preserve their original bytes and any provided headers.
In `@uts/src/test/kotlin/io/ably/lib/test/mock/FakeClock.kt`:
- Around line 29-31: The advance() method mutates the timers map while iterating
(timers.values.forEach { ... }), which can throw ConcurrentModificationException
if a callback creates a new timer; change it to iterate over a snapshot of the
timer collection (e.g. make a list copy of timers.values) and call fireDue(time)
on that snapshot so callbacks can safely mutate the original timers map; update
the advance() implementation (and any similar iteration over timers) to use the
snapshot approach to avoid concurrent modification during Timer.fireDue calls.
In `@uts/src/test/kotlin/io/ably/lib/test/mock/MockHttpClient.kt`:
- Around line 35-39: The trySend() calls in the onConnection/onRequest lambdas
can fail silently when channels are recreated in reset(), causing
awaitConnectionAttempt()/awaitRequest() to timeout; update the
_pendingConnections.trySend(conn) and _pendingRequests.trySend(pending)
invocations so their Result is checked by calling getOrThrow() (e.g.,
_pendingConnections.trySend(conn).getOrThrow()) to surface failures instead of
dropping events; keep existing handler fallback logic and ensure any thrown
exceptions propagate so tests fail fast.
In `@uts/src/test/kotlin/io/ably/lib/test/mock/MockWebSocket.kt`:
- Around line 46-49: The events getter returns a snapshot from
Collections.synchronizedList without external synchronization, which can race
with concurrent writers; modify the events property (and any reset() that
iterates) to synchronize on _events when creating the snapshot (i.e. wrap the
_events.toList() call in synchronized(_events) { ... }) so iteration/read is
protected; reference the private val _events and the val events: List<MockEvent>
get() = ... and ensure any other callers that iterate _events do the same.
In `@uts/src/test/kotlin/io/ably/lib/Utils.kt`:
- Around line 29-35: The ConnectionStateListener added with
client.connection.on(listener) is only removed on cancellation; update the
success path to also remove it: when you call cont.resume(Unit) (both in the
listener callback and the immediate state-check), call
client.connection.off(listener) either before or immediately after resuming to
unregister the listener and avoid buildup. Modify the two occurrences (the
listener block where you check change.current == target and the immediate check
where you compare client.connection.state == target) to unregister the same
listener (listener) on successful resume, and keep the existing
cont.invokeOnCancellation { client.connection.off(listener) } to cover
cancellation.
---
Outside diff comments:
In `@liveobjects/src/main/kotlin/io/ably/lib/objects/ServerTime.kt`:
- Around line 16-17: ServerTime currently caches a single global
serverTimeOffset (private var serverTimeOffset) which must be scoped per
adapter/clock; change the cache to map offsets keyed by the adapter/clock
identity (e.g., Map<Clock, Long?> or Map<AdapterId, Long?>) and update all uses
in ServerTime methods (the code paths around where serverTimeOffset is
read/updated — lines ~24-35) to look up and set the offset using the current
adapter/clock key instead of the global variable so each clock has its own
cached offset.
---
Nitpick comments:
In `@lib/src/main/java/io/ably/lib/rest/Auth.java`:
- Around line 926-927: The nanoTime drift detection wrongly uses the injected
clock (clock.currentTimeMillis()) which allows fake/custom clocks to mask or
create false wall-clock jumps; change the computation of currentNanoTimeDelta to
use the JVM wall-clock directly (System.currentTimeMillis()) together with
System.nanoTime()/ (1000*1000) so the drift check is tied to the real system
clock; update both occurrences (the currentNanoTimeDelta calculation around
currentTimeMillis/System.nanoTime and the similar logic at the later occurrence
around lines 1056-1057) to reference System.currentTimeMillis() instead of
clock.currentTimeMillis() and keep the rest of the logic unchanged.
In `@uts/build.gradle.kts`:
- Line 24: The build currently forces all tests to rerun by unconditionally
setting outputs.upToDateWhen { false }; change this to be opt-in by gating it
behind a project property (e.g., "forceTestRerun"). Locate the block that sets
outputs.upToDateWhen { false } (the test task configuration) and wrap it so it
only executes when project.findProperty("forceTestRerun") == "true" (or
similar), or remove the line altogether; ensure to document the property in the
repo README or CI docs so callers can opt in when needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15dbe772-ae6e-4dca-810a-36af6ff01876
📒 Files selected for processing (37)
.claude/skills/uts-to-kotlin/SKILL.md.github/workflows/check.ymllib/src/main/java/io/ably/lib/debug/DebugOptions.javalib/src/main/java/io/ably/lib/http/HttpCore.javalib/src/main/java/io/ably/lib/http/HttpScheduler.javalib/src/main/java/io/ably/lib/realtime/ChannelBase.javalib/src/main/java/io/ably/lib/realtime/Presence.javalib/src/main/java/io/ably/lib/rest/Auth.javalib/src/main/java/io/ably/lib/transport/ConnectionManager.javalib/src/main/java/io/ably/lib/transport/Hosts.javalib/src/main/java/io/ably/lib/transport/WebSocketTransport.javalib/src/main/java/io/ably/lib/util/Clock.javalib/src/main/java/io/ably/lib/util/NamedTimer.javalib/src/main/java/io/ably/lib/util/SystemClock.javalib/src/main/java/io/ably/lib/util/TimerInstance.javaliveobjects/src/main/kotlin/io/ably/lib/objects/ServerTime.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/BaseRealtimeObject.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.ktliveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapManager.ktsettings.gradle.ktsuts/build.gradle.ktsuts/src/test/kotlin/io/ably/lib/ClientFactories.ktuts/src/test/kotlin/io/ably/lib/Utils.ktuts/src/test/kotlin/io/ably/lib/realtime/unit/connection/ConnectionRecoveryTest.ktuts/src/test/kotlin/io/ably/lib/test/mock/DefaultPendingConnection.ktuts/src/test/kotlin/io/ably/lib/test/mock/DefaultPendingRequest.ktuts/src/test/kotlin/io/ably/lib/test/mock/FakeClock.ktuts/src/test/kotlin/io/ably/lib/test/mock/MockEvent.ktuts/src/test/kotlin/io/ably/lib/test/mock/MockHttpClient.ktuts/src/test/kotlin/io/ably/lib/test/mock/MockHttpEngine.ktuts/src/test/kotlin/io/ably/lib/test/mock/MockWebSocket.ktuts/src/test/kotlin/io/ably/lib/test/mock/MockWebSocketEngineFactory.ktuts/src/test/kotlin/io/ably/lib/test/mock/PendingConnection.ktuts/src/test/kotlin/io/ably/lib/test/mock/PendingRequest.ktuts/src/test/kotlin/io/ably/lib/types/Utils.kt
Contributor
There was a problem hiding this comment.
Pull request overview
This PR lays groundwork for UTS-derived Kotlin tests and deterministic transport/time control by adding a Clock abstraction, mock HTTP/WebSocket transports, and a new uts Gradle module; it also updates parts of the SDK (core + liveobjects) to use injected time/engines via DebugOptions.
Changes:
- Introduces
Clock/NamedTimer/TimerInstance+SystemClock, and wires clock usage into transports, auth, scheduling, and liveobjects tombstoning/GC paths. - Adds mock WebSocket/HTTP implementations plus test helpers, and a new
utsmodule with initial UTS-derived connection recovery tests. - Adds CI wiring to run
:uts:testand includes a Claude skill doc for translating UTS specs to Kotlin tests.
Reviewed changes
Copilot reviewed 37 out of 37 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| uts/src/test/kotlin/io/ably/lib/Utils.kt | Adds coroutine helpers to await connection/channel states in tests. |
| uts/src/test/kotlin/io/ably/lib/types/Utils.kt | Adds a Kotlin DSL-style initializer for ConnectionDetails in tests. |
| uts/src/test/kotlin/io/ably/lib/test/mock/PendingRequest.kt | Defines HTTP pending request interface for mock engine. |
| uts/src/test/kotlin/io/ably/lib/test/mock/PendingConnection.kt | Defines connection-attempt interface and query parsing helper. |
| uts/src/test/kotlin/io/ably/lib/test/mock/MockWebSocketEngineFactory.kt | Adds a mock WebSocketEngineFactory/engine/client for tests. |
| uts/src/test/kotlin/io/ably/lib/test/mock/MockWebSocket.kt | Implements a higher-level mock WebSocket with callbacks/await APIs and event log. |
| uts/src/test/kotlin/io/ably/lib/test/mock/MockHttpEngine.kt | Implements a mock HttpEngine/HttpCall that tests must resolve. |
| uts/src/test/kotlin/io/ably/lib/test/mock/MockHttpClient.kt | Wraps mock HTTP engine with callback/await APIs and install helper. |
| uts/src/test/kotlin/io/ably/lib/test/mock/MockEvent.kt | Adds an event model for mock transport event recording. |
| uts/src/test/kotlin/io/ably/lib/test/mock/FakeClock.kt | Adds a virtual clock for deterministic timer advancement in tests. |
| uts/src/test/kotlin/io/ably/lib/test/mock/DefaultPendingRequest.kt | Implements PendingRequest response helpers for mock HTTP. |
| uts/src/test/kotlin/io/ably/lib/test/mock/DefaultPendingConnection.kt | Implements PendingConnection responses for mock WebSocket. |
| uts/src/test/kotlin/io/ably/lib/realtime/unit/connection/ConnectionRecoveryTest.kt | Adds initial UTS-derived tests for recovery key/recover behavior. |
| uts/src/test/kotlin/io/ably/lib/ClientFactories.kt | Adds TestRealtimeClient/TestRestClient builders and mock/fake-timer installation hooks. |
| uts/build.gradle.kts | Introduces the new uts test module configuration. |
| settings.gradle.kts | Includes the uts module in the build. |
| liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapManager.kt | Uses injected clock for tombstone timestamps when server timestamp missing. |
| liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/LiveMapEntry.kt | Makes GC eligibility time source injectable via Clock. |
| liveobjects/src/main/kotlin/io/ably/lib/objects/type/livemap/DefaultLiveMap.kt | Propagates clock into base object and uses it for GC decisions. |
| liveobjects/src/main/kotlin/io/ably/lib/objects/type/livecounter/DefaultLiveCounter.kt | Propagates clock into base object. |
| liveobjects/src/main/kotlin/io/ably/lib/objects/type/BaseRealtimeObject.kt | Adds Clock to base object lifecycle/GC timestamp computations. |
| liveobjects/src/main/kotlin/io/ably/lib/objects/ServerTime.kt | Uses injected clock to compute server time offset and derived server time. |
| lib/src/main/java/io/ably/lib/util/TimerInstance.java | Adds timer cancellation handle abstraction. |
| lib/src/main/java/io/ably/lib/util/SystemClock.java | Implements Clock using system time and java.util.Timer; provides clockFrom helper. |
| lib/src/main/java/io/ably/lib/util/NamedTimer.java | Adds named timer abstraction returning TimerInstance. |
| lib/src/main/java/io/ably/lib/util/Clock.java | Adds core clock interface used across SDK for time + timer creation. |
| lib/src/main/java/io/ably/lib/transport/WebSocketTransport.java | Allows injecting WebSocket engine factory and uses Clock + NamedTimer for activity timers. |
| lib/src/main/java/io/ably/lib/transport/Hosts.java | Uses injected Clock for preferred-host expiry calculations. |
| lib/src/main/java/io/ably/lib/transport/ConnectionManager.java | Uses injected Clock for stale/suspend calculations. |
| lib/src/main/java/io/ably/lib/rest/Auth.java | Uses injected Clock for timestamping and time-change detection. |
| lib/src/main/java/io/ably/lib/realtime/Presence.java | Uses channel clock for timestamps generated by presence sync logic. |
| lib/src/main/java/io/ably/lib/realtime/ChannelBase.java | Replaces Timer with NamedTimer and uses injected clock to create timers. |
| lib/src/main/java/io/ably/lib/http/HttpScheduler.java | Uses injected clock for timeout deadline calculations in Future.get(timeout). |
| lib/src/main/java/io/ably/lib/http/HttpCore.java | Allows injecting an HttpEngine via DebugOptions. |
| lib/src/main/java/io/ably/lib/debug/DebugOptions.java | Adds debug-time injection points: HttpEngine, WebSocketEngineFactory, and Clock. |
| .github/workflows/check.yml | Updates CI to run :uts:test. |
| .claude/skills/uts-to-kotlin/SKILL.md | Adds guidance/skill doc for translating UTS specs to Kotlin tests. |
Comments suppressed due to low confidence (1)
uts/src/test/kotlin/io/ably/lib/Utils.kt:54
awaitChannelStateregisters aChannelStateListenerbut does not unregister it when the target state is reached (only on cancellation). This can leak listeners across awaits and potentially affect subsequent tests. Ensure the listener is removed once the coroutine resumes successfully.
suspendCancellableCoroutine { cont ->
val listener = ChannelStateListener { change ->
if (change.current == target && cont.isActive) cont.resume(Unit)
}
channel.on(listener)
if (channel.state == target && cont.isActive) cont.resume(Unit)
cont.invokeOnCancellation { channel.off(listener) }
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .code(status) | ||
| .message("") | ||
| .body(HttpBody("application/json", bytes)) | ||
| .headers(emptyMap()) |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
lib/src/main/java/io/ably/lib/util/SystemClock.java (1)
18-31: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftEach
newTimer()call creates a dedicated daemon thread.Every
newTimer(name)invocation creates a newjava.util.Timerwith its own background thread. With multiple channels and transports, this scales thread count linearly and reduces reliability—if any timer task throws an unhandled exception, theTimer's thread terminates and subsequent schedules fail silently.Modern Java best practices recommend
ScheduledExecutorServiceoverTimerfor better resource management, exception handling, and scalability.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/main/java/io/ably/lib/util/SystemClock.java` around lines 18 - 31, The newTimer() implementation creates a dedicated java.util.Timer per call which spawns a thread per timer; replace this with a shared ScheduledExecutorService and schedule tasks as ScheduledFutures to avoid one-thread-per-timer and silent failures. Update newTimer() to use a singleton ScheduledExecutorService (created with a ThreadFactory that sets threads as daemon) and in the returned NamedTimer.schedule(...) submit the TimerTask as a Runnable that wraps task.run() in try/catch to log/handle exceptions, returning a TimerInstance that cancels the underlying ScheduledFuture (future.cancel(false)). Also update NamedTimer.cancel() to no-op or cancel any outstanding futures, and ensure the executor is properly created once (e.g., static final) and not shutdown on each timer creation; use the existing symbols newTimer, NamedTimer, TimerInstance, schedule and cancel to locate and change the code.uts/src/test/kotlin/io/ably/lib/test/mock/DefaultPendingRequest.kt (1)
20-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve response headers and serialize non-binary bodies as JSON.
respondWith(...)currently discards theheadersparameter (line 30) and usestoString()for non-ByteArray bodies (line 23), which produces invalid JSON and breaks consumers expecting proper JSON serialization.Proposed fix
+import io.ably.lib.util.Serialisation override fun respondWith(status: Int, body: Any, headers: Map<String, String>) { val bytes = when (body) { is ByteArray -> body - else -> body.toString().toByteArray(Charsets.UTF_8) + else -> Serialisation.gson.toJson(body).toByteArray(Charsets.UTF_8) } deferred.complete( HttpResponse.builder() .code(status) .message("") .body(HttpBody("application/json", bytes)) - .headers(emptyMap()) + .headers(headers.mapValues { (_, value) -> listOf(value) }) .build() ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@uts/src/test/kotlin/io/ably/lib/test/mock/DefaultPendingRequest.kt` around lines 20 - 33, In DefaultPendingRequest.respondWith, preserve the passed headers instead of using emptyMap() and serialize non-ByteArray bodies to JSON (not via toString()) before converting to bytes; update the HttpResponse builder to use the provided headers parameter and ensure HttpBody is constructed with UTF-8 bytes of JSON-serialized body for non-binary inputs, completing the deferred with that response.lib/src/main/java/io/ably/lib/http/HttpScheduler.java (1)
291-296:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftTimeout loop can still hang with non-advancing injected clocks.
The timeout calculation uses
clock.currentTimeMillis()to computedeadlineandremaining, but whenFakeClock(or any non-wall-clock implementation) is injected,clock.waitOn(this, remaining)may complete immediately (without advancing virtual time), causingremainingto never decrease. This can makeget(timeout, unit)hang indefinitely.Proposed fix
Use
System.nanoTime()for the timeout deadline to ensure it advances with real wall-clock time, regardless of the injected clock:public T get(long timeout, TimeUnit unit) throws InterruptedException, ExecutionException, TimeoutException { - long remaining = unit.toMillis(timeout), deadline = clock.currentTimeMillis() + remaining; + final long deadlineNanos = System.nanoTime() + unit.toNanos(timeout); synchronized(this) { - while(remaining > 0) { - clock.waitOn(this, remaining); - if(isDone) { break; } - remaining = deadline - clock.currentTimeMillis(); + while(!isDone) { + long remainingNanos = deadlineNanos - System.nanoTime(); + if (remainingNanos <= 0) { + break; + } + long remainingMillis = TimeUnit.NANOSECONDS.toMillis(remainingNanos); + clock.waitOn(this, Math.max(1, remainingMillis)); } if(!isDone) { throw new TimeoutException(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/main/java/io/ably/lib/http/HttpScheduler.java` around lines 291 - 296, Replace the wall-clock-dependent timeout math in HttpScheduler.get(timeout, unit): instead of using clock.currentTimeMillis() to compute deadline and remaining, compute a nano-based deadline with System.nanoTime() + unit.toNanos(timeout) and in the loop compute remainingNanos = deadline - System.nanoTime(); if remainingNanos <= 0 then break; convert remainingNanos to a millisecond value (e.g. Math.max(0, remainingNanos/1_000_000) or at least 1ms if you need non-zero waits) when calling clock.waitOn(this, remainingMillis). This ensures clock.waitOn/clock.currentTimeMillis (and injected FakeClock) cannot cause get(timeout, unit) to hang by preventing virtual-time from affecting the deadline calculation.uts/src/test/kotlin/io/ably/lib/test/mock/FakeClock.kt (1)
37-51:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent map mutation during timer firing.
Line 39 iterates
timers.valueswhile executingfireDue(time)callbacks. If a callback creates a new timer (viaclock.newTimer(...)), it mutatestimersand throwsConcurrentModificationException.Proposed fix
fun advance(ms: Long) { time += ms - timers.values.forEach { it.fireDue(time) } + timers.values.toList().forEach { it.fireDue(time) } val due = synchronized(waiters) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@uts/src/test/kotlin/io/ably/lib/test/mock/FakeClock.kt` around lines 37 - 51, FakeClock.advance currently iterates timers.values while calling each timer.fireDue(time), which allows callbacks (e.g., clock.newTimer) to mutate the timers map and cause ConcurrentModificationException; fix this by iterating over a stable snapshot of the timers collection (e.g., copy timers.values to a list) before calling fireDue so mutations during callbacks don't affect the iterator; update the loop in FakeClock.advance (the timers.values.forEach -> snapshot.forEach) while keeping the subsequent waiters handling and notifyAll logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@uts/src/test/kotlin/io/ably/lib/test/mock/FakeClock.kt`:
- Around line 28-34: The FakeClock.waitOn implementation currently calls (target
as Object).wait() without ensuring the caller holds target's monitor; update the
contract and implementation: add KDoc to Clock.waitOn stating callers MUST hold
the target's monitor (mention callers like HttpScheduler.get / AsyncRequest) and
enforce it in FakeClock.waitOn by checking Thread.holdsLock(target) and throwing
IllegalMonitorStateException with a clear message if not, then keep the existing
wait logic (synchronized(waiters) { waiters.add(Waiter(...)) } followed by
target.wait()) so the precondition is explicit and violations fail fast.
---
Duplicate comments:
In `@lib/src/main/java/io/ably/lib/http/HttpScheduler.java`:
- Around line 291-296: Replace the wall-clock-dependent timeout math in
HttpScheduler.get(timeout, unit): instead of using clock.currentTimeMillis() to
compute deadline and remaining, compute a nano-based deadline with
System.nanoTime() + unit.toNanos(timeout) and in the loop compute remainingNanos
= deadline - System.nanoTime(); if remainingNanos <= 0 then break; convert
remainingNanos to a millisecond value (e.g. Math.max(0,
remainingNanos/1_000_000) or at least 1ms if you need non-zero waits) when
calling clock.waitOn(this, remainingMillis). This ensures
clock.waitOn/clock.currentTimeMillis (and injected FakeClock) cannot cause
get(timeout, unit) to hang by preventing virtual-time from affecting the
deadline calculation.
In `@lib/src/main/java/io/ably/lib/util/SystemClock.java`:
- Around line 18-31: The newTimer() implementation creates a dedicated
java.util.Timer per call which spawns a thread per timer; replace this with a
shared ScheduledExecutorService and schedule tasks as ScheduledFutures to avoid
one-thread-per-timer and silent failures. Update newTimer() to use a singleton
ScheduledExecutorService (created with a ThreadFactory that sets threads as
daemon) and in the returned NamedTimer.schedule(...) submit the TimerTask as a
Runnable that wraps task.run() in try/catch to log/handle exceptions, returning
a TimerInstance that cancels the underlying ScheduledFuture
(future.cancel(false)). Also update NamedTimer.cancel() to no-op or cancel any
outstanding futures, and ensure the executor is properly created once (e.g.,
static final) and not shutdown on each timer creation; use the existing symbols
newTimer, NamedTimer, TimerInstance, schedule and cancel to locate and change
the code.
In `@uts/src/test/kotlin/io/ably/lib/test/mock/DefaultPendingRequest.kt`:
- Around line 20-33: In DefaultPendingRequest.respondWith, preserve the passed
headers instead of using emptyMap() and serialize non-ByteArray bodies to JSON
(not via toString()) before converting to bytes; update the HttpResponse builder
to use the provided headers parameter and ensure HttpBody is constructed with
UTF-8 bytes of JSON-serialized body for non-binary inputs, completing the
deferred with that response.
In `@uts/src/test/kotlin/io/ably/lib/test/mock/FakeClock.kt`:
- Around line 37-51: FakeClock.advance currently iterates timers.values while
calling each timer.fireDue(time), which allows callbacks (e.g., clock.newTimer)
to mutate the timers map and cause ConcurrentModificationException; fix this by
iterating over a stable snapshot of the timers collection (e.g., copy
timers.values to a list) before calling fireDue so mutations during callbacks
don't affect the iterator; update the loop in FakeClock.advance (the
timers.values.forEach -> snapshot.forEach) while keeping the subsequent waiters
handling and notifyAll logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 282b2ebb-747b-4b16-aab0-dee45f605dee
📒 Files selected for processing (7)
lib/src/main/java/io/ably/lib/http/HttpScheduler.javalib/src/main/java/io/ably/lib/transport/ConnectionManager.javalib/src/main/java/io/ably/lib/util/Clock.javalib/src/main/java/io/ably/lib/util/SystemClock.javauts/src/test/kotlin/io/ably/lib/test/mock/DefaultPendingRequest.ktuts/src/test/kotlin/io/ably/lib/test/mock/FakeClock.ktuts/src/test/kotlin/io/ably/lib/test/mock/PendingRequest.kt
Comment on lines
+28
to
+34
| @Suppress("PLATFORM_CLASS_MAPPED_TO_KOTLIN") | ||
| override fun waitOn(target: Any, timeout: Long) { | ||
| synchronized(waiters) { | ||
| waiters.add(Waiter(target as Object, time + timeout)) | ||
| } | ||
| (target as Object).wait() | ||
| } |
There was a problem hiding this comment.
waitOn violates Java monitor contract.
Line 33 calls (target as Object).wait() without holding the target's monitor. Java's Object.wait() requires the calling thread to own the object's monitor, or it throws IllegalMonitorStateException.
The caller (e.g., HttpScheduler.get()) holds the monitor of this (the AsyncRequest), not the monitor of target. Since FakeClock.waitOn is called with target = this (the AsyncRequest), the AsyncRequest's monitor is held by the caller's synchronized(this) block, so line 33 should succeed. However, the design is fragile because waitOn delegates to target.wait() assuming the caller already holds target's monitor, which is not enforced by the Clock interface contract.
Clarify the contract or add documentation
Add a precondition check or KDoc to Clock.waitOn stating that the caller MUST hold target's monitor before calling:
override fun waitOn(target: Any, timeout: Long) {
+ // Caller MUST hold target's monitor before calling this method
synchronized(waiters) {
waiters.add(Waiter(target as Object, time + timeout))
}
(target as Object).wait()
}Or enforce it with a check (though this adds overhead):
if (!Thread.holdsLock(target)) {
throw IllegalMonitorStateException("Caller must hold target's monitor")
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@uts/src/test/kotlin/io/ably/lib/test/mock/FakeClock.kt` around lines 28 - 34,
The FakeClock.waitOn implementation currently calls (target as Object).wait()
without ensuring the caller holds target's monitor; update the contract and
implementation: add KDoc to Clock.waitOn stating callers MUST hold the target's
monitor (mention callers like HttpScheduler.get / AsyncRequest) and enforce it
in FakeClock.waitOn by checking Thread.holdsLock(target) and throwing
IllegalMonitorStateException with a clear message if not, then keep the existing
wait logic (synchronized(waiters) { waiters.add(Waiter(...)) } followed by
target.wait()) so the precondition is explicit and violations fail fast.
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.
This PR includes:
Summary by CodeRabbit
New Features
Documentation
Tests / Chores