feat(notifications): plan push handling as data - #1441
Merged
Merged
Conversation
MetadataBuilder declares `infix fun String.to(value: Any)`. Passing the nullable title and body resolved to kotlin.to instead, which builds a Pair and discards it, so neither field ever reached the log — including when the push carried a body. A device trace confirms it: a push sent with push_notification_body=sanity-silent-001 logged onMessageReceived | actions=0, silent=true, bucket=active, latency_ms=-329 with no body= at all. It compiles clean, so nothing flagged it. Replace both with non-null values. Body content stays out: TraceType.Process is forwarded to breadcrumb sinks, and message text does not belong in Bugsnag. has_body carries what the spike needs, and spike_seq gives the bucket matrix a per-push correlation id so a dropped push and a late one are distinguishable.
planPushHandling branched once per rule, so the shared event taxonomy would have grown a branch per event class. The category dimension is now a map from NotificationCategory to the actions it implies, and adding an event class is an entry in it. The key becomes the new field once flipcash.push.v1.Payload carries one; the shape does not change with it. Navigation stays a `when` because each arm reads the chat id or mint off the trigger it matched, so its actions cannot be written down ahead of time. The two sources are concatenated and deduplicated, which replaces the explicit `RefreshFeed !in actions` guard: a contact-join push that also names a chat asks for a feed refresh from both sides and gets one. Two characterization tests went in first and stayed green across the change — the deduplicated contact-join-plus-chat ordering, and a category with no entry planning no sync of its own.
… the outcome FeatureFlagController.get is suspend over DataStore, and the call site wrapped it in runBlocking on the FCM dispatch thread. Every push paid that read, including the visible ones, which are all of production traffic today — and a visible push posts its notification and plans its sync whatever the flag says. Passing the read as a lambda keeps the rule in planPushHandling, which is the only place that knows a title means the flag is irrelevant. A test asserts the lambda is never invoked for a titled push, so the property is pinned rather than implied by reading the branch. Also rewords the spike_seq comment, since the correlation key lands with the harness rather than staying on the spike branch.
scripts/spike/run-bucket-matrix.sh drives one cell of the standby-bucket matrix: put the app in a bucket, optionally force or wait out Doze, send N high-priority pushes at a fixed interval with a sequence number in each, and capture logcat plus the power state around them. parse-bucket-log.py turns the capture into per-push delivery latency and the actions the planner chose; standby-bucket-probe.sh reads the bucket without running a cell. It lands with the feature rather than being deleted with the branch because the next delivery question needs the same instrument, and because the results document cites these scripts by path for how each number was produced. Raw captures stay out of the repo: a full device logcat carries unrelated personal content, so docs/spikes/raw/ is gitignored and only the summarised results are committed. maestro/spike_login.yaml gets the device back to a logged-in state after the data wipe a bucket reset needs.
139 of 139 high-priority pushes were delivered across every bucket including restricted, with no priority downgrades, so bucket is not the delivery risk the design assumed. What the matrix did surface is a CPU one: three of the runs ended in ActivityManager killing the process for exceeding 2% CPU over a 300 s window while cached, and regressing the kills against the log volume inside their own windows puts one push at roughly 6.7 s of CPU. The document is the evidence for both halves of the push-preload design, and it cites scripts/spike/ for how each figure was produced, so it lands in the same change as the harness and the planner it describes.
github-actions
Bot
added
type: feature
The 6.7 s per push in the results document comes from regressing two kills against how many lines the app logged inside their own 300 s windows. That infers the quantity the killer reads; it does not read it. ActivityManager takes utime+stime out of /proc through ProcessCpuTracker and compares the delta against 2% of the window, so sampling those two fields around a push measures what actually decides the kill. The script brackets each push with a /proc sample, waits out the ~90 s burst the captures show, and starts with one push-free window of the same length for the idle term. Budget arithmetic needs both: 2% of 300 s is 6000 ms, and idle CPU spends some of it before any push arrives. It does not use deep Doze. The kill is gated on the process being cached with the framework seeing battery, which `dumpsys battery unplug` plus a screen-off, adj>=900 process gives without the Wi-Fi adb drops that ended two earlier cells. A pid change across a sample is reported rather than averaged in: a process killed partway through work it had not finished is not a reading of what that work costs.
The 2% killer is a 6000 ms allowance per 300 s window, so how many pushes fit is that budget minus idle, divided by the cost of one push. The regression fit both terms to log volume. Sampling /proc/<pid>/stat once a minute for 906 s with the app cached at adj 910, screen off and the phone unplugged, the counters do not move once: 0 ticks, against a 10 ms tick. The process is frozen — cgroup.freeze reads 1 — so it is not sleeping cheaply, it is not running. That is a different quantity from the regression's ~0.5 ms per idle second, which was fitted across windows with pushes arriving every 180 s and so attributed each burst's thaw and timer catch-up to the gap it sat in. With nothing arriving the floor is zero, which leaves the budget with one term. At the 6.7 s per push the regression gives, that is 0.90 pushes per five minutes and a break-even cadence of 335 s: the app cannot absorb one push per window. A preload has to get a push under 6000 ms to survive one, and under 1500 ms for the four per five minutes a chat preload implies. The per-push term is still the regression's figure and still an upper bound — includeRpcBodies was on for the capture build. The section says what would settle it and names the variant to re-measure on.
0.5.0 carries flipcash2-protobuf-api#92, which adds `messaging.v1.Message message = 3` to `push.v1.ChatMetadata`. That field is what lets a chat push carry its own message body instead of naming a chat to fetch. `ocp-client-protocol` is unchanged.
`push.v1.ChatMetadata.message` arrived in flipcash2-client-protocol 0.5.0, so a chat push can now carry the message itself rather than only naming the chat it belongs to. `asPayload` maps it onto `PushChatMetadata.message` behind the proto's `hasMessage()` presence check, and `applyPushedMessage` writes it through the same `ChatMessageDataSource.upsert` the event stream uses. That upsert already drops a copy whose `event_sequence` is not newer than the stored row, which is what makes re-delivery of the same push harmless and lets a later `loadMessages` converge on the same transcript. Two things it deliberately does not do: - advance the event-log cursor. A push carries one message, not a page, so seating the cursor at its sequence would let a catch-up resume from a frontier it never fetched and skip what it missed in between. - rewind the feed row. `last_message_id` and `last_activity_epoch_ms` only move forward, guarded by the new `getLastMessageId` read. The field is optional and the message body is size-limited, so a chat push will still sometimes arrive without one. Nothing here replaces the fetch path.
github-actions
Bot
added
area: network
A push at a named chat planned `RefreshFeed` then `LoadMessages`, and `LoadMessages` is a `GetMessages` round trip on the wake path. Splitting one push-woken burst by trace phase puts roughly half its lines under wake and stream setup and roughly half under the sync it triggered (docs/spikes/2026-09-08-standby-bucket-results.md), so both are material against the 2%-over-five-minutes CPU ceiling and the split between them is unmeasured. This removes one of the two round trips the sync half makes. When the payload inlines the message, `syncForNavigation` now plans `PushAction.ApplyMessage` in its place, which is a local write. When it does not, the plan is the `LoadMessages` it has always been. The fallback is not defensive: `chat_metadata.message` is optional in the proto and the body is size-limited, so the server will omit it. `RefreshFeed` stays. Unread counts come from the feed, not from the message row, so dropping it would change what the user sees rather than only what the push costs. Also corrects the table's comment: `flipcash.push.v1.Payload.category` is the event taxonomy and no separate event field is coming, so the map is already keyed on the taxonomy. Adding an event class stays an entry rather than a branch.
… a finding The regression section and the option-B bullet both reported ~0.5 ms per idle second as a measured cost, ~200 lines before the section that refutes it. A direct `/proc` read of a cached process shows zero ticks over 906 s, because `cgroup.freeze` is 1 — the residual was burst edges attributed to the gap beside them, not an idle rate. Both now say what survived measurement and point forward to the reading. Also makes the break-even cadence read 335 s in both places; it was 336 s here and 335 s in the budget table, the same division at different precision.
Silent preload was gated on `PushSilentSync`, default off, so a data-only push planned nothing on a default build. `planPushHandling` now plans the same sync work either way and a title only adds `PostNotification` on top, which also takes the blocking DataStore read off the FCM dispatch thread — the reason the flag was passed as a `() -> Boolean` rather than read as a value. The two tests asserting that a titleless push planned nothing go with it, replaced by their inverse plus the invariant that a title changes the notification and not the sync plan. The spike doc's flag paragraphs move to past tense: its numbers were measured with the flag on, and a re-run now needs no flag setup.
bmc08gt
force-pushed
the
feat/push-silent-sync
branch
from
September 10, 2026 20:10
5fac584 to
0a8796a
Compare
bmc08gt
changed the title
`applyPushedMessage` was argued in review comments and asserted nowhere. Seven tests now pin the three properties the planner cannot see: the message reaches the same `ChatMessageDataSource.upsert` a fetched page uses, the event-log cursor is left where it was, and the feed row only moves forward. The cursor case is the one worth having. A push carries one message rather than a page, so seating `updateLatestEventSequence` at its sequence would let a later catch-up resume from a frontier it never fetched — a silent gap in the transcript, not a visible failure. Checked against a mutant: dropping the forward-only guard, emptying the upsert list and adding the cursor write fails four of the seven, and the three that survive are the ones those mutations do not reach.
The same message reaches `ChatMessageDao.upsert` from a fetched page, the event stream and now a push that carried it, with no ordering between them. The guard is what makes arrival order stop mattering, and it had no test of its own. Eight cases against real Room rather than a mock DAO, because the behaviour under test is what SQLite stores after a REPLACE. Three of them are edges the comparison makes rather than the guard's headline: a copy at the same sequence writes through, since the comparison is strict; a copy at sequence 0 bypasses the guard entirely and can overwrite a stamped row; and a dropped write leaves `pending_client_id_hex` on the row it lost to, which is what keeps an optimistic message matchable to the server's echo. Checked against a mutant: deleting the guard fails the two cases that assert a drop and leaves the other six passing, which is the right split — the rest describe behaviour the guard is not responsible for.
messaging.v1 tells clients to ignore a copy whose event_sequence is at or below the version they hold. `ChatMessageDao.upsert` drops only a strictly older copy, and the difference is load-bearing: `confirmPendingMessage` stamps the server's event_sequence onto the optimistic row while leaving the content written locally, so the server's canonical copy of that message arrives at a sequence equal to the one already stored. Treating equal as "ignore" would pin the optimistic content. Also records that the sequence-0 passthrough is not reachable from the server. `event_sequence` carries `(validate.rules).uint64.gte = 1`, and every Message the server emits — GetMessages, the send echo, the event stream, and the message inlined in a push — is built by the same `Message.ToProto()` off a store-assigned sequence, so only a locally built row can arrive unstamped. Comments only.
measure() returns immediately once the process is dead, so after a kill the loop filled the rest of the file with `dead_before` rows in a couple of seconds. A run that died on push 4 of 8 still printed eight windows and finished early without saying why. Check for the pid before each window and break, naming the window the cell stopped at. Also drops the stale note about the feature flag, which no longer exists, and records why the window filter gates on the process being cached at the start of a window rather than at both ends: handling a push is itself what promotes the process to the previous-app slot, so requiring both ends discards every push window by construction.
The 6.7 s per push in this document was inferred from log volume. Bracketing /proc/<pid>/stat around each send puts it at ~2.2 s across twelve windows in three cells, with 87% of a burst inside its first two seconds. The arithmetic that follows from 6000/2200 is wrong anyway, and the cells say so: 240 s spacing killed the app twice while 120 s survived ten pushes and 420 s survived five. The kill record reconciles it. AMS charged 6230 ms where the three measured windows sum to 6210, so the instrument was right; what was wrong was the window. Those three sends are 240 s apart and AMS calls the interval between them 300043, because the window is 300 s of uptimeMillis(), which does not advance across suspend. A cadence that never lets the process freeze also never lets the device suspend, which is why the tightest spacing is as safe as the widest. Records the resulting rule: two pushes per 300 s of uptime, which the cells bracket at one push per 420 s of wall clock, with less margin than the division suggests. Also corrects the idle table, which gave adj 905 as the kill band. The kills here were at 700 and 900; AMS checks setProcState >= PROCESS_STATE_HOME, so the promotion a push earns does not put the process out of reach.
#1436 fixed the MetadataBuilder.to resolution bug this branch also fixes, and both landed on the same trace call in onMessageReceived. Took this branch's side of the conflict: planPushHandling decides whether a push does anything, so the `if (title == null) return` #1436 left in place is what this PR removes. Kept #1436's reason for the derived fields. Both branches arrived at `silent` and `has_body` rather than the values, but its comment says why — TraceType. Process reaches breadcrumb sinks, so push text would go to Bugsnag — where this branch's comment described the resolution bug instead. That comment is now wrong as well as redundant: #1436 widened the parameter to Any?, so a nullable argument no longer silently resolves to kotlin.to.
ChatUpdate.new_messages is deprecated in favour of the sequenced, gap-detectable events field — the proto states new messages now arrive as events, and the backend sends new_messages empty. The fallback branch in applyUpdate was therefore unreachable. Drop newMessages from the ChatUpdate domain model, its protobuf mapper, and the EventStreamingController trace, which now reports the events count. ChatCoordinatorEventsTest's two fallback cases covered behaviour that no longer exists, so they go. ReceivedEventTest, ReceivedCounterTest and ChatCoordinatorEagerBalanceTest build their fixtures on events instead — they cover receipts, counters and eager balance rather than the removed field. Their event sequences run contiguously from 1 so the gap detector stays out of the way.
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.
Push handling on
code/cashis a chain ofifs insideonMessageReceived, and a data-only push returns before any of it runs. The cross-platform preload design needs both to change: one event taxonomy, both platforms reacting to the same events, only the wake mechanism differing. This is the Android seed for that, moved off the spike branch it was measured on so it stops drifting fromcode/cash.What lands
PushAction— the sealed set of things a push can ask the app to do:RefreshFeed,LoadMessages,ApplyMessage,UpdateTokens,SyncContacts,PostNotification.planPushHandling— a pure function from title, body and decoded payload to a list ofPushAction. No Android types, no coroutines, no injection, so every rule about push handling is unit-testable.onMessageReceiveddecodes, plans, then executes.sentTime, the app's own standby bucket, and FCM'spriorityandoriginalPriority.What changes for a received push
A data-only push now does the sync its payload implies instead of returning early. That is the behaviour change in this PR: on
code/casha titleless push is dropped whatever it carries. The sync half of the plan is the same either way, and a title only addsPostNotificationon top of it.This shipped behind
PushSilentSyncwhile the matrix was measured and the flag is gone from this branch, so there is no default-off path to fall back to.A visible push plans the actions the
ifchain performed, with one deliberate difference: a contact-join push that also names a chat calledchatCoordinator.refreshFeed()twice and now calls it once.The
onMessageReceivedtrace records the size of the plan alongside the delivery fields, so a push that decoded but planned nothing is visible asactions= 0 rather than as an absent trace.The switch is a table
Adding an event class should be a data change.
syncByCategorymapsNotificationCategoryto the actions it implies, and adding a class is an entry in it.flipcash.push.v1.Payload.categoryis the event taxonomy — there is no separate event field coming — so the table is keyed on the taxonomy already.Navigation stays a
when, because each arm reads the chat id or mint off the trigger it matched and its actions cannot be written down ahead of time. The two lists are concatenated and deduplicated, which is what replaces the explicitRefreshFeed !in actionsguard.A chat push can carry its own message
flipcash2-client-protocol 0.5.0 adds
messaging.v1.Message message = 3topush.v1.ChatMetadata(flipcash2-protobuf-api#92). When the payload inlines the message, the plan isRefreshFeedthenApplyMessage— a local write through the sameChatMessageDataSource.upsertthe event stream uses — in place of theLoadMessagesGetMessagesround trip. The DAO's upsert already drops a copy whoseevent_sequenceis not newer than the stored row, so a re-delivered push and a laterloadMessagesconverge on the same transcript.The field is optional and the body is size-limited, so the server will sometimes omit it and the plan falls back to
LoadMessages.applyPushedMessagedoes not advance the event-log cursor: a push carries one message, not a page, so seating the cursor at its sequence would let a catch-up resume from a frontier it never fetched. The feed row'slast_message_idandlast_activity_epoch_msonly move forward.RefreshFeedstays, because unread counts come from the feed rather than the message row.Harness and results
scripts/spike/anddocs/spikes/2026-09-08-standby-bucket-results.mdland with the code rather than being deleted with the branch. The document cites the scripts by path for how each figure was produced, so splitting them leaves the results referencing an instrument absent from its tree — and that instrument is what the open measurement below still needs.Raw captures stay out of the repo: a full device logcat carries unrelated personal content, so
docs/spikes/raw/is gitignored and only the summarised results are committed.The delivery result is that bucket is not the risk the design assumed — 139 of 139 high-priority pushes delivered across every bucket including
restricted, with no priority downgrades. The matrix surfaced a CPU risk instead: three runs ended withActivityManagerkilling the process for exceeding 2% CPU over a 300 s window while cached.Sizing the preload against that ceiling
2% of a 300 s window is 6000 ms of CPU. A cached process that receives nothing spends none of it — sampled at 60 s over 906 s, utime + stime did not move a single tick, because the process is frozen at
cgroup.freeze= 1. The whole budget is available to push handling.scripts/spike/measure-push-cpu.shhas now been run against the device's own FCM token. Bracketing/proc/<pid>/stataround each send puts one push at ~2.2 s, not the ~6700 ms the regression inferred from log volume: twelve windows across three cells, 1810–2680 ms, with a separate 2 s profile agreeing at 2030 ms for a single burst. 87% of that lands in the first two seconds. Bodies are still on — this is adebuginstall — so 2.2 s remains an upper bound on production, just a much tighter one.Dividing gives 2.7 pushes per five minutes, and the device disagrees: 240 s spacing killed the app in both attempts while 120 s survived ten pushes and 420 s survived five. The kill record says why. AMS charged 6230 ms where the three measured windows sum to 6210, so the instrument is right; the window is not what it looked like. Those three sends are 240 s apart, and AMS calls the interval between them
300043— the window is 300 s ofuptimeMillis(), which does not advance across suspend. A cadence that never lets the process freeze also never lets the device suspend, which is why the tightest spacing is as safe as the widest.The rule that falls out is two pushes per 300 s of uptime, which these cells bracket at one push per 420 s of wall clock. A chat preload driven by message arrival exceeds that on any active conversation, so it needs either server-side coalescing or a cheaper handler — and with 87% of the burst in the sync RPC fan-out, that is a fan-out problem rather than a timer one.
Removing the
GetMessagesround trip from a chat push cuts one of the two round trips in that fan-out. The size of that cut is unmeasured.