Rebase fork onto upstream rpush v9.2.0 - #11
Closed
rstojano wants to merge 33 commits into
Closed
Conversation
I sort of just guessed at this method of getting more advanced data into the APNS body and it worked, figured it'd be better if it was explicit. Co-authored-by: Ben Langfeld <blangfeld@powerhrg.com>
Missed in rpush@4c98e17
Still supporting some upstream unsupported versions for now to make it easier for users of rpush to upgrade. https://endoflife.date/rails https://endoflife.date/ruby
This was shut down by Google in August 2024 and replaced by FCM (supported in RPush 8.0.0).
Closes rpush#703 ### Background on the change FSM messages should be consistent with: https://firebase.google.com/docs/reference/fcm/rest/v1/projects.messages#Message These messages support a "apns" key: https://firebase.google.com/docs/reference/fcm/rest/v1/projects.messages#ApnsConfig Which in its turn has a "payload" key, described as: > APNs payload as a JSON object, including both aps dictionary and custom payload. See [Payload Key Reference](https://developer.apple.com/documentation/usernotifications/setting_up_a_remote_notification_server/generating_a_remote_notification). If present, it overrides [google.firebase.fcm.v1.Notification.title](https://firebase.google.com/docs/reference/fcm/rest/v1/projects.messages#Notification.FIELDS.title) and [google.firebase.fcm.v1.Notification.body](https://firebase.google.com/docs/reference/fcm/rest/v1/projects.messages#Notification.FIELDS.body). The Payload Key Reference contains information on supported keys, including a "badge": > The number to display in a badge on your app’s icon. Specify 0 to remove the current badge, if any.
Had some struggles setting up FCM in my repository with the given documentation, so I'm suggesting some extra steps people might look over on their initial tests. From the documentation it's not immediately clear that `data` is not where the title and body of a notification go. Leaving `notification` blank causes rpush to return a success, but nothing will actually arrive on the device. The project ID also might have changed labels overtime, as setting the "project ID" as my `firebase_project_id` instead of the "project number" also prevents notifications from showing up.
Only difference was in the comment (possibly typo?) Co-authored-by: Ben Langfeld <blangfeld@powerhrg.com>
Fixes rpush#692 This should help release the gem on GitHub. When release, you can dispatch the release workflow on GitHub Actions. Choose "Release" workflow, and run it. The workflow will: 1. Build and push the gem with the version defined in `rpush/version.rb`. 2. Tag the repository with `v<version>`. e.g., `v9.2.0`. 3. GitHub will create a release with release notes. Because the release workflow uses an action called `rubygems/release-gem`, to publish the gem via GitHub, the gem owners must configure the ["Trusted publishing"](https://guides.rubygems.org/trusted-publishing/). You must [add GitHub as a trusted platform](https://guides.rubygems.org/trusted-publishing/adding-a-publisher/). ------ Other changes: 1. I also bumped the Ruby in the development environment to 3.2 with gems including `pg`, `mysql2` to resolve a few build problems on my development environment. 2. I also use Gemfile for development dependencies as Rubocop suggests. 3. I updated `tests.yml` to run the latest Ubuntu. This can be further improved to use GitHub repo's ruleset to set certain or all tests must pass for PR to be able to merge. Happy to help on this area too. --------- Co-authored-by: Ben Langfeld <blangfeld@powerhrg.com>
… structured push logging (#7) * Retry APNs (apnsp8) frames dropped by a mid-flight connection reset When the APNs HTTP/2 connection is reset while a batch is in flight, net-http2 tears down the stream set and delivers the error to the client's on(:error) callback; #join then returns normally. The in-flight notifications never receive an on(:close), so they are marked neither delivered, failed, nor retryable, and the batch discards them silently on completion -- no retry, no failure record. An idle apnsp8 socket (e.g. a low-volume Live Activity app) that APNs resets loses the next frame this way. Fix, localized to the apnsp8 transport plus a read-only Batch helper: - Batch#unresolved returns notifications with no terminal outcome. - Apnsp8::Delivery#perform reconciles after #join: any unresolved notification is re-queued (retryable) instead of dropped. No-op on the normal path where every stream reported a result. - handle_response treats an absent status code (stream closed before APNs answered) as a transport failure -> retry, not a permanent failure. - Errno::ECONNRESET added to the retryable rescue (matching apns2), in case a reset ever surfaces synchronously rather than via on(:error). Every existing APNs status outcome (200/410/400/429/500/503) is unchanged; only the "no verdict from APNs" cases move from {silent drop, permanent fail} to {retry}. Specs: Batch#unresolved, plus apnsp8 reconciliation and no-status handling. * Structured push-pipeline logging (apnsp8 transport + Loggable helper) Add Loggable#log_push_event, which emits logfmt-style key=value lines so Datadog indexes each field and the lines join to the nexus push logs on rpush_notification_id. Give every apnsp8 notification a discoverable outcome: - delivery: delivered / failed / retrying (with the device token truncated), and a retryable APNs response is logged as a retry, not a failure - app_runner: enqueued per notification (both the batch and non-batch paths), and startup_failed when an app cannot start (bad cert) - dispatcher: connection_error on a socket-level client error, so a mid-flight reset is findable Logging only; no delivery behavior changes. * Quote the app name in structured push logs The app name bypassed push_event_value, so a name containing whitespace, =, or " produced an unparseable logfmt line and broke Datadog field indexing. Route it through the same quoter as every other field. * Log the error message on connection_error, and keep push logs single-line Review follow-up: the connection_error line logged only the exception class, so two same-class socket errors with different messages were indistinguishable in a Datadog query. Include the message. Also fold newlines in structured field values so a multi-line message cannot split the one-line record.
Apns2::Delivery has the same latent silent-drop pattern PR #7 fixed for Apnsp8, called out there as a deliberate follow-up: a dropped HTTP/2 connection tears down its in-flight streams via net-http2's on(:error) callback rather than raising into #perform, so those notifications never receive an on(:close) and are marked neither delivered, failed, nor retryable -- silently discarded when the batch completes. This is the transport a cert-based app (e.g. currypizzahouse_ios) uses, observed in production as a connection that goes completely silent for hours -- no sends, no errors logged -- then resumes on its own with no restart. Reused a single push message's rpush_notifications rows confirm the same request succeeding on one attempt and silently vanishing (no delivered/failed/retryable outcome) on another, minutes apart, against the same two device tokens. Fix, mirroring Apnsp8::Delivery#perform and reusing Batch#unresolved (already added by #7, transport-agnostic): - Apns2::Delivery#perform reconciles after #join: any unresolved notification is re-queued (retryable) instead of dropped. - handle_response treats an absent status code (stream closed before APNs answered) as a transport failure -> retry, not a permanent failure (previously this fell through to the `else` branch and was marked permanently *failed* -- worse than Apnsp8's pre-#7 silent drop). - Also fixes the untested per-notification SSLError rescue named in #7's "Follow-ups" section: preparing a request could raise before the notification ever got a stream, and the old code just logged and moved on, leaving it with no outcome at all. Now retried via the same path. Every existing APNs status outcome (200/410/400/429/500/503) is unchanged; only the "no verdict from APNs" cases move from {silent drop, permanent fail} to {retry} -- strictly safer, matching #7's regression-safety argument for Apnsp8. Also brings structured push-event logging (#7) to this transport for parity: delivered/failed/retrying events on Delivery, and the dispatcher's connection_error now includes the error message (not just its class) -- addressing the one open review comment on #7 before it repeats here. Tests mirror #7's apnsp8 coverage: the reconnection sweep, no-status handling, the SSLError-at-prepare-time path, and the logging format cases. Stacked on rstojano/apnsp8-retry-dropped-frames (#7) to reuse Batch#unresolved and Loggable#log_push_event without redefining them; rebase onto master once #7 merges. Co-authored-by: Robert Stojanovski <robert@thanx.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 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 |
Resolve conflicts using the ours strategy. master (74f7afc) is the pre-rebase fork state; every change on it is the same custom work this branch already contains, replayed on top of upstream rpush v9.2.0. The only unique change on master is "Allow Rails 5.1", intentionally dropped as obsolete under v9.2.0's activesupport >= 6.0 constraint.
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
9 participants
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.
Summary
Rebases the Thanx fork onto upstream
rpushv9.2.0. The fork previously sat on a pre-8.0.0master snapshot (version7.1.0, fork point 2024-09-02). This replays our 7 custom commits on top of the latest stable upstream release.Upstream changes pulled in
content-availableAPNs notifications. Drops Ruby 2.4–2.6 and Rails 5.2.Rebase details
ECONNRESETretry).dc5137f "Allow Rails 5.1"— obsolete, contradicts v9.2.0'sactivesupport >= 6.0.Testing
configuration.rbModis redis delegation (untouched file, environment issue).Action items before merge
Gemfile.lockfor the target Ruby/Rails; upstream's committed lock pins Rails7.0.2.2.