Skip to content

fix: pass ErrorInfo status code before Ably code in client-side errors - #1248

Open
RaphaelFakhri wants to merge 1 commit into
ably:mainfrom
RaphaelFakhri:fix/errorinfo-status-code-order
Open

RaphaelFakhri wants to merge 1 commit into
ably:mainfrom
RaphaelFakhri:fix/errorinfo-status-code-order

Conversation

@RaphaelFakhri

@RaphaelFakhri RaphaelFakhri commented Sep 29, 2026 •

Copy link
Copy Markdown

Fixes #793

Description

ErrorInfo(String message, int statusCode, int code) takes the HTTP status code before the Ably error code. Several client-side errors passed the arguments the other way round, for example new ErrorInfo("...", 40000, 400). The resulting error reported code = 400 and statusCode = 40000.

This change swaps the arguments so these errors report code = 40000 and statusCode = 400 (and code = 50000, statusCode = 500 for the two server-style errors). The affected errors are:

  • ConnectionManager.ping when the connection is not connected, and the heartbeat timeout
  • Hosts option validation (fallbackHosts, fallbackHostsUseDefault, environment)
  • HttpCore proxy configuration, HttpAuth header parsing
  • Auth null key, AblyRealtime.channels.get reattach options, ChannelBase untilAttach validation
  • ErrorInfo.fromThrowable
  • Android: Platform, Push, ActivationContext, ActivationStateMachine

Testing

  • HostsTest.hosts_invalid_options_error_codes asserts code 40000 and statusCode 400 for invalid host options.
  • ConnectionManagerPingErrorTest asserts the same for ping on a client that is not connected.
  • Both tests fail without the change (2 failed of 17) and pass with it (17 passed).

Run them with:

./gradlew :java:test --tests io.ably.lib.transport.HostsTest --tests io.ably.lib.transport.ConnectionManagerPingErrorTest

Summary by CodeRabbit

  • Bug Fixes
    • Corrected HTTP status and error code values reported for invalid configuration, authentication, channel, connection, push, and unexpected exception errors.
    • Error responses now consistently distinguish HTTP status codes from Ably error codes.
  • Tests
    • Added coverage to verify the corrected status and error codes for connection and host configuration errors.

@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: ed8d89d6-6c7d-436f-ae3d-a8dc6420da4b

📥 Commits

Reviewing files that changed from the base of the PR and between b515ef7 and 18d4692.

📒 Files selected for processing (14)
  • android/src/main/java/io/ably/lib/platform/Platform.java
  • android/src/main/java/io/ably/lib/push/ActivationContext.java
  • android/src/main/java/io/ably/lib/push/ActivationStateMachine.java
  • android/src/main/java/io/ably/lib/push/Push.java
  • lib/src/main/java/io/ably/lib/http/HttpAuth.java
  • lib/src/main/java/io/ably/lib/http/HttpCore.java
  • lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java
  • lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
  • lib/src/main/java/io/ably/lib/rest/Auth.java
  • lib/src/main/java/io/ably/lib/transport/ConnectionManager.java
  • lib/src/main/java/io/ably/lib/transport/Hosts.java
  • lib/src/main/java/io/ably/lib/types/ErrorInfo.java
  • lib/src/test/java/io/ably/lib/transport/ConnectionManagerPingErrorTest.java
  • lib/src/test/java/io/ably/lib/transport/HostsTest.java

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


Walkthrough

The changes correct error code and HTTP status code assignments in Android, HTTP, authentication, realtime, transport, and error conversion paths. Tests check the values reported by ping and host validation errors.

Changes

ErrorInfo assignments

Layer / File(s) Summary
Android platform and push errors
android/src/main/java/io/ably/lib/platform/Platform.java, android/src/main/java/io/ably/lib/push/*
Platform and push error paths now use corrected error code and HTTP status code assignments.
HTTP, authentication, and realtime validation errors
lib/src/main/java/io/ably/lib/http/*, lib/src/main/java/io/ably/lib/realtime/*, lib/src/main/java/io/ably/lib/rest/Auth.java, lib/src/main/java/io/ably/lib/transport/Hosts.java, lib/src/test/java/io/ably/lib/transport/HostsTest.java
HTTP, authentication, realtime, and host configuration errors now use corrected code and status assignments. The host test checks the reported values.
Transport errors and throwable conversion
lib/src/main/java/io/ably/lib/transport/ConnectionManager.java, lib/src/main/java/io/ably/lib/types/ErrorInfo.java, lib/src/test/java/io/ably/lib/transport/ConnectionManagerPingErrorTest.java
Ping, heartbeat-timeout, and unexpected-exception errors now use corrected code and status assignments. A ping test checks the reported values.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: ttypic

Merge Risk: ⚪ Minimal · up to 18d46

The corrected error values are ready to merge after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 18d46

The correction changes the error codes applications receive, but the examined authentication, connection, and push-registration paths retain their existing acceptance and failure behavior. No introduced security issue was established. Application-specific handling of the corrected values remains unknown.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The independently affected scope is SDK callers receiving these local errors across core and Android. The examined edits do not add an entrypoint or expand the conditions under which those errors occur; downstream application policies were not available.

Trust Boundaries and Controls

  • observed — Malformed authentication headers still take the existing rejection path, before an authentication challenge is accepted. The edit changes the returned error fields, not credential generation or the retry condition.
  • observed — The malformed device-registration response still fails activation before a device identity token is stored. Client-side evidence does not establish whether a server registration created before that response is rolled back.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 14 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: correcting the argument order for status codes and Ably error codes in client-side ErrorInfo constructions.
Linked Issues check ✅ Passed Issue [#793] requires the ping API to report the Ably error code and HTTP status code in the correct fields. ConnectionManager now passes the arguments in status-code, then Ably-code order. `Connect…
Out of Scope Changes check ✅ Passed The additional changes correct the same ErrorInfo argument-order defect in client-side HTTP, host, channel, authentication, Android, and throwable error paths. The added host and ping tests verify c…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

❤️ Share

A rabbit checks the codes in place,
And sets each status with care and grace.
The ping replies, the tests agree,
The hosts report consistently.
Then hops away, pleased as can be.

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

ErrorInfo code and status code are mixed up in ping API

1 participant