[WIP, AIT-569] Investigation of push registration failures - #2204
Draft
lawrence-forooghian wants to merge 35 commits into
Draft
[WIP, AIT-569] Investigation of push registration failures#2204lawrence-forooghian wants to merge 35 commits into
lawrence-forooghian wants to merge 35 commits into
Conversation
Document two possible approaches for handling legacy data where the deviceIdentityToken may not match the current device id: Direction A: validate the token with a GET, then re-register if rejected. Currently written into the spec (RSH3i/RSH3j). The spec has a TODO in RSH3j2a for a possible improvement: using a PATCH with the deviceSecret to preserve the registration rather than discarding it. Direction B: skip validation, discard the token, and go through the normal registration flow. The POST is an upsert on the server (confirmed in realtime code) so the existing registration is preserved. Simpler but relies on undocumented server behaviour. Also adds context on how much we care about preserving the registration — devices that have been through the Keychain bug already have orphaned registrations, but many devices may never have been affected. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add analysis of RSH3a2a — the existing "validation" mechanism in the spec. It appears to be unreachable through normal state machine transitions (all paths into NotActivated clear the token), and its failure path loops on 401 errors. Add direction C: hook into RSH3a2a by keeping the token and starting in NotActivated. Would fix the token-mismatch loop for all cases (not just legacy migration) by modifying the sync failure path to discard the token on 401 and fall through to fresh registration. But loses context about why the 401 happened, making it impossible to distinguish legacy migration recovery from other auth failures. Note that directions B and C are not yet written into the spec (only direction A is specced). Update recommendation to reflect that we haven't reached a decision between the three. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Background research on which push activation states are persistent vs non-persistent across ably-cocoa, ably-java, and ably-js. Traces the history of these decisions, the motivation (ably-java#546), the #966 bug caused by non-persistent states, and the unmerged attempt to fix it (0e92186). Out of scope for the current push registration failure investigation but captured for future reference, particularly for informing ably-swift's push implementation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add comprehensive inventory of everything ably-cocoa persists, with storage mechanisms and consequences of loss for each item - Note Apple's APNS token caching guidance and how RSH8i addresses it (see specification#25) - Add direction C (hook into RSH3a2a) as an alternative approach - Add analysis of RSH3a2a's purpose, reachability, and whether we can use it for recovery - Clarify that directions B and C are not yet written into the spec - Update direction A/B comparison to be more balanced Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add section clarifying two implicit assumptions in the spec: 1. LocalDevice is treated as a single atomic blob — the spec doesn't anticipate split storage (Keychain + NSUserDefaults). RSH8a2 acknowledges this but we haven't proposed how ably-cocoa would achieve atomicity going forward. 2. RSH3h1's failure recovery is a safety net, not a routine code path. If it fires frequently (due to sometimes-unavailable storage), orphaned registrations accumulate with no cleanup mechanism. Paddy's comment on #1109 confirms always-available storage was the intended approach. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Discuss whether the spec's atomic unit should be just the (id, secret, token) tuple (as proposed in RSH8a2) or the entire set of persisted data including state machine state. The (id, secret, token) tuple is the only group where atomicity is critical for correctness. Other items (clientId, APNS tokens, state machine state) are self-correcting or have fallbacks. But the state machine state is logically part of the same unit, and storing everything as one blob in ably-cocoa would be simplest. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Covers: - Test app requirements (display device state, debug tampering) - How to create the stale token state directly (debug button) - How to reproduce the actual Keychain failure (reboot + silent push before unlock) - Migration testing (old SDK → new SDK, with and without stale token, with and without Keychain availability) - Verification criteria for all cases These test plans have not been verified and are initial proposals to ensure we have a testing strategy. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Create two Ably instances: mainAbly (the client under test) and eventLoggingAbly (for publishing events to the LocalDeviceStorageBugTest-events channel). Wire up a custom ARTLog handler on mainAbly that publishes all log messages to the events channel via eventLoggingAbly. Also add a Secrets.example.swift template and .gitignore to keep API keys out of source control, and a README describing the app. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Set up PushKit to register for VoIP push tokens, publishing them to the events channel as voipToken events. Add a minimal CallKit handler (CXProvider/CXProviderDelegate) to satisfy the iOS requirement that VoIP pushes must report an incoming call. Add a shell script (send-voip-push.sh) that fetches the latest VoIP token from the events channel via the Ably CLI and sends a push to APNs. Also configure the Xcode project with the required capabilities: push notification entitlement, VoIP background mode, and usage description. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Introduce a Codable Event enum that unifies all events published to the events channel, with documented associated-value structs for each case. Custom Codable conformance avoids the _0 wrapper that Swift's default enum synthesis produces. Add an ARTRealtimeChannel extension in a separate file for publishing Event values directly. Update all call sites and the send-voip-push script to use the new event names and JSON structure. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add buttons to activate push and subscribe to a push channel, with result display. Each action publishes attempt/result event pairs to the events channel, linked by an attempt ID and tagged with a reason (currently userTappedButton, extensible to automatic triggers). Add CodableErrorInfo to capture the full ARTErrorInfo in event payloads (code, statusCode, message, reason, href, requestId, cause). Add an AppDelegate to forward APNs device tokens to ARTPush, as required by the Ably push activation flow. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add a settings section with toggles for auto-activating push and auto-subscribing to the push channel on app launch. When both are enabled, activation runs first and subscription follows on success. Settings are stored in a JSON file with FileProtectionType.none so they remain readable before first unlock — necessary because a VoIP push can launch the app while the device is still locked. Add appLaunch as an ActionReason to distinguish automatic actions from user-initiated ones. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add a setup step to enable message persistence on the events channel, since the default 2-minute retention is too short for the send script to find the VoIP token. Also increase the history limit in send-voip-push.sh from 100 to 1000. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Generate a UUID on first launch and persist it to an unprotected file (installation-id.txt). Include it in every event alongside appLaunchID, so events can be correlated across launches of the same installation. The ID does not survive reinstallation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Publish an appLaunched event before any other setup, capturing whether protected data was available at launch time. This is key for identifying launches that occur before first unlock (e.g. from a VoIP push). Also observe protectedDataDidBecomeAvailable and protectedDataWillBecomeUnavailable notifications, publishing a protectedDataAvailability event on each subsequent change. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add CodableLocalDevice (and CodablePushDetails, CodableIdentityTokenDetails) to capture the full state of ARTLocalDevice. Include it in the pushActivateResult event so that changes to device details (e.g. ID, secret) can be detected when the SDK is unable to load persisted data — as happens when the app is launched before first unlock. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use appInstallation-<UUID> as the clientId so that multiple device registrations from the same installation are easy to identify — this is the failure mode under investigation where the device ID gets unnecessarily recreated. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Read the file protection attribute of the NSUserDefaults plist at startup and include it in the appLaunched event. This tells us whether the file that ARTLocalDeviceStorage uses to persist device details is accessible before first unlock. The plist path (Library/Preferences/<bundle-id>.plist) is an implementation detail of NSUserDefaults. The file may not exist on a fresh install before any defaults have been written. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Include a full dump of UserDefaults.standard in both the appLaunched and pushActivateResult events, so that we can compare before and after to see whether the SDK wrote new values during activation — even if the file was previously unavailable (before first unlock). Non-JSON-serialisable values (e.g. Data) are sanitised to string representations. The dump is inlined as a dictionary in the Ably message payload rather than a JSON string. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Forthcoming work on this branch needs a test-only Ably API that is only exposed through the `Ably.Private` submodule, so import it here. Importing `Ably.Private` exposes additional designated initialisers on `ARTLog` (notably `-initCapturingOutput:` and `-initCapturingOutput:historyLines:`, declared in `ARTLog+Private.h`), and that has a knock-on effect on the `EventLoggingLogHandler: ARTLog` subclass defined in this file. Without any action, the app now fatally errors at launch with "Use of unimplemented initializer 'init(capturingOutput:)'". Work around it by routing `super.init(...)` through the terminal 3-arg initialiser rather than `-init`, which avoids the self-dispatch that triggers the Swift-synthesised trap. The comment in the code flags that the reasoning is Claude's speculation and should be verified if it ever becomes load-bearing.
Use the private test-only options added in 571c79ae and 4015f8e6: - Set `disableLocalDevice = true` on the event-logging client so it doesn't become the owner of the shared `ARTLocalDevice` and the first client to access `rest.device_nosync` (the main client) ends up bound to the shared device instead, keeping its device-storage activity attributed to its own logger. - Set `logLocalDeviceStorageValues = true` on the main client so that storage read/write log lines include the persisted values rather than the `(retracted)` placeholder. This is what we're here to investigate.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
lawrence-forooghian
changed the base branch from
main
to
AIT-569-keychain-investigation-groundwork
April 17, 2026 19:40
github-actions
Bot
temporarily deployed
to
staging/pull/2204/markdown-api-reference
April 17, 2026 19:44
Inactive
lawrence-forooghian
changed the title
Point to the LocalDeviceStorageBugTest app as the current test approach. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
lawrence-forooghian
force-pushed
the
AIT-569-investigating-push-registration-failures
branch
from
April 17, 2026 20:16
c36916f to
d25a26a
Compare
github-actions
Bot
temporarily deployed
to
staging/pull/2204/markdown-api-reference
April 17, 2026 20:21
Inactive
Base automatically changed from
AIT-569-keychain-investigation-groundwork
to
main
May 15, 2026 17:10
The `ably channels history --json` command now emits newline-delimited JSON: a `fetching` status event, then the `result` object holding `messages`, then a `completed` status event. The jq filter ran `.messages[]` against every line, so the two status lines (which have no `messages` key) caused "Cannot iterate over null" errors. Selecting the `result` line first restores correct behaviour. Assumptions made (not independently verified against a live CLI run): - The format change was inferred from the failing output, not from observing the old format; I never saw the previous output. - The fix was checked against synthetic NDJSON matching the structure in the failing run, plus the user confirming the script now works. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lawrence-forooghian
added a commit
that referenced
this pull request
Jun 18, 2026
…tion (#2206) Replaces `NSUserDefaults` + keychain with a single property-list file (`Application Support/Ably/LocalDevice.plist`) written atomically and with `NSFileProtectionNone` so the data is readable regardless of device lock state. Adds a `performBatchUpdate:` method to `<ARTDeviceStorage>` that batches related writes into a single flush, so callers can group writes that must reach disk together. Existing UserDefaults + keychain entries are migrated on first init and are being kept just in case. If the legacy keychain is locked (`errSecInteractionNotAllowed`/`errSecNotAvailable`) it means the secret is unreachable, in which case the legacy device details and state machine are discarded. A bit more context, added by Lawrence to Marat's commit message: The overall context here is trying to fix an issue where the customer is reporting what seems to be a LocalDevice whose id and deviceSecret seem to have become out of sync, leading to an error of "Token deviceId does not match requested device operation" upon calling `channel.push.subscribeDevice()`; see Slack thread [1] which contains a discussion of how we've arrived at the work done in this commit and investigations done along the way. We don't know exactly how this is happening and have not been able to reproduce it (and thus although the fixes here are sensible ones they are still speculative in relation to this particular customer issue), but we have had a long-standing belief that we may have problems resulting from various characteristics of the way that ably-cocoa persists data and handles load failures: - all persisted data is stored in either keychain or UserDefaults, neither of which is guaranteed to be available before first unlock and thus may lead to undesired load failures (which as mentioned below were not necessarily being handled correctly) and device ID regenerations (the None protection level change above fixes this) - device's id and deviceIdentityToken are stored under different user defaults keys and there's nothing tying the stored deviceIdentityToken to that device id; so _maybe_ there's a possiblity of drift (the atomic storage mentioned above fixes this) - we weren't clearing the deviceIdentityToken upon failure to load the device id or secret, leading to a possibility of drift (this is addressed by new spec point RSH8a1 implemented here) - the push activation state machine may have stayed in its previous state upon failure to load LocalDevice data whereas it should become NotActivated (this is addressed by new spec points RSH8a1 and RSH3h implemented here; they replace the previous unimplemented RSH8j; thus resolves #1257) One thing _not_ done here: a mechanism to detect the case where we have a device that belives itself to be activated and has loaded a persisted out-of-sync deviceIdentityToken; that is, the changes we've made should mean that no new device ends up in this situation, but it may not help existing devices that already are (if such exist; again, we haven't been able to reproduce). This may be something we need to do if the customer's issue continues (see the aforementioned Slack thread). The unmerged investigation PR #2204 adds a test harness app which is useful for testing the before-first-unlock data inaccessibility scenario. [1] https://ably-real-time.slack.com/archives/CURL4U2FP/p1773945259481269 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-Authored-By: Lawrence Forooghian <lawrence@forooghian.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Note: This PR is based on top of #2203, which provides groundwork for the demo app contained in this PR.
This contains a WIP investigation of https://ably.atlassian.net/browse/AIT-569. I have not yet reproduced the customer's issue and am awaiting further information. However, the investigation document and corresponding specification changes do contain some things that we need to fix no matter what.
The two main things contained in this PR are:
Examples/LocalDeviceStorageBugTest), which allows us to explore how the SDK behaves under before-first-unlock data inaccessibility; see its README for information on how to use itThere are some corresponding specification changes that I've drafted; see ably/specification#450. Some of these may be valuable, some may not, because until I actually reproduce the issue some of the things it's trying to address may just be speculative and not actually in need of "fixing".