perf(runs): don't block action writes on the NOTIFY pump - #7787
perf(runs): don't block action writes on the NOTIFY pump#7787stantheman0128 wants to merge 1 commit into
Conversation
notifyActionUpdate and notifyRunUpdate did a blocking send on a 256-slot channel from inside write RPCs, right after the row was already committed. Once the pump fell behind, a best-effort wakeup turned into user-visible write latency, and a cancelled request context discarded the notification outright, leaving other watchers on a stale phase. Replace the FIFO channels with a mutex-guarded pending set per notification kind plus a capacity-1 signal channel. The writer inserts a key and nudges the pump without ever blocking; the pump swaps the sets out and emits them. Because a payload is an identity and every listener re-reads state from the database when woken, repeated updates for one action collapse into a single delivery, and memory is bounded by distinct pending actions rather than by update volume. A failed pg_notify merges its keys back into the pending set and retries with backoff instead of dropping them, so a transient database problem costs a delay rather than a lost wakeup. Two limits keep that retry from becoming a new failure mode. A payload only spends its retry budget when the connection was healthy and it failed anyway, which is what happens to a payload Postgres will never accept, such as one of 8000 bytes or more; after enough of those attempts it is dropped with an error. And a drain that delivered anything resets the backoff, so one bad payload cannot slow every other watcher down. A lost connection aborts the current drain so the remaining payloads do not each trigger their own reconnect attempt. Fixes flyteorg#7757 Signed-off-by: stantheman0128 <stanshih888@gmail.com>
|
This PR was flagged by our automated quality checks. If you're a genuine We appreciate your contribution and apologize if this is a false positive! |
There was a problem hiding this comment.
Pull request overview
This PR removes write-path latency caused by blocking NOTIFY wakeups in the runs-service by replacing the buffered FIFO notification channels with a coalescing “pending set + single nudge channel” design, ensuring write RPCs never wait on the notify pump and that wakeups aren’t dropped due to request-context cancellation.
Changes:
- Replace
actionNotifyCh/runNotifyChwithpendingActions/pendingRunssets guarded bynotifyMu, plus a capacity-1pendingChsignal to wake the pump without blocking writers. - Rework
runNotifyLoopto drain the pending sets, retry undelivered payloads with bounded backoff, and drop permanently-undeliverable payloads afternotifyRetryLimit. - Add/adjust tests to assert non-blocking writer behavior, coalescing semantics, retry behavior, and end-to-end delivery via a real Postgres listener.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| runs/repository/impl/action.go | Replaces blocking notify-channel sends with a coalescing pending-set + non-blocking wake mechanism and adds retry/backoff logic in the pump. |
| runs/repository/impl/action_test.go | Adds and updates tests to cover non-blocking behavior, coalescing, retries, poison payload drop, and end-to-end delivery. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Genuine contributor — happy to trim the PR description if maintainers prefer a shorter body. The automated flag looks driven by description length, fork rate, and the username heuristic on stantheman0128 rather than the code change itself. Fixes #7757; tests and CI are green. |
Could you try to test it in the debbox? I added a make target for installing grafana dashboard in the devbox Lines 53 to 54 in fd2a82a |
|
Thanks, that target is exactly what I was missing. I will bring the devbox up with the Grafana stack and report the before and after for Will follow up here with the numbers. |
|
Following up on the devbox + Grafana request. Setup
This branch (commit
|
| Stat | UpdateActionStatus |
|---|---|
| samples | 57,630 |
| p50 | 9.7 ms |
| p95 | 149.3 ms |
| p99 | 1205.8 ms |
Main baseline
I imported origin/main (41e562c57) flyte-binary into the same devbox image slot and tried to rerun the identical swarm for a paired measurement. On a freshly recreated devbox volume I hit repeated rustfs-svc.flyte:9000 connection refused errors during run submit (inputs upload), so I do not have a clean main histogram from this session to put beside the branch numbers.
For main comparison I am still using the table from #7757 (same workload class, dev cluster): p95 3085 ms at ~101.8 rps on UpdateActionStatus.
Takeaway
On the branch side, the devbox run moved p95 from the issue's ~3s main figure down to ~150 ms under swarm load, which matches the non-blocking notify intent. Happy to rerun the main side on request once devbox storage is stable on my box, or if you have a CI devbox run from devbox-functional-tests with metrics exported.
Commands and raw query output from the branch run are in the PR branch worktree notes if useful.
Tracking issue
Closes #7757
Why are the changes needed?
notifyActionUpdateandnotifyRunUpdatedid a blocking send on a 256-slot channel from inside write RPCs, right after the row was already committed. When the pump falls behind, the buffer fills and the send blocks in the RPC handler, so a best-effort wakeup becomes user-visible write latency. The issue measures that as a p95 of 3085 ms onUpdateActionStatuswhile the database itself sat idle.There is a second, quieter problem in the same six lines. The old
selecthad a<-ctx.Done()arm, so a client that disconnected mid-request discarded a notification that other watchers still needed.What changes were proposed in this pull request?
The FIFO channels are replaced with a coalescing pending set per notification kind, plus one capacity-1 signal channel shared by both, as the issue proposes.
ctx.Done()and on anypg_notifyfailureThe writer takes the lock, inserts the payload as a key, unlocks, and does a non-blocking send on
pendingCh. It cannot block and it cannot discard a key. A nudge that is already buffered needs no second one: the sets are the queue, the channel only wakes the pump.Dropping a nudge is safe because of the ordering. The insert always happens before the signal attempt, and the pump always swaps the sets out after it has consumed a token. If the send finds the buffer full, a token was outstanding at that instant, so the pump has not yet woken for it; when it does, its swap happens after our insert and picks the key up. If the send succeeds, the same argument applies to the token we just put there. Either way a key cannot sit in the set with no pending wakeup.
The pump swaps both sets out under the lock and emits them. Payloads it could not deliver are merged back into the pending set and retried with backoff, so a transient database problem costs a delay rather than a lost wakeup. Re-adding a key is idempotent, so a retry cannot duplicate work, and updates that arrived meanwhile merge into the same entry.
Retrying forever is its own failure mode
An earlier version of this patch retried unconditionally, which is what the issue asks for. That turns out to have a sharp edge, and I only found it because I went looking for a payload that fails for a reason other than a connection blip.
Postgres rejects a
pg_notifypayload of 8000 bytes or more, permanently. Measured against the package's own test database:isConnErrorcorrectly says that is not a connection problem, so the connection stays up and the payload fails the same way on every attempt. With an unconditional retry the key never leaves the pending set, the success branch never runs, and the backoff pins at its 5 s ceiling. One such payload adds seconds of wakeup latency to every other watcher for the life of the process. That is worse than the old behaviour, which logged once and dropped it.RunIdentifier.Namehas no length validation (identifier.pb.validate.gosays as much), and the root-actionnotifyRunUpdateinUpdateActionPhaseis not gated onrowsAffected, so a caller can queue such a payload without a row existing. Reachable, not theoretical.Two limits keep the retry honest:
A payload only spends its retry budget when the connection was healthy and it failed anyway, because that is the case where the payload itself is what Postgres rejected. Connection-level failures cost nothing, so a genuine outage still retries for as long as it lasts. After
notifyRetryLimitpayload-specific failures the key is dropped with an error log.A drain that delivered anything resets the backoff. Otherwise one failing payload would slow every healthy notification behind it. The effect is measurable: the test covering this drops from 16.4 s to 1.0 s once the reset is in, because the poison key burns its budget at the minimum interval while ordinary traffic keeps flowing.
Two smaller notes
A lost connection aborts the current drain.
execNotifyreconnects whenconnis nil, so without an early exit a dead connection would make every remaining payload in a large batch trigger its own reconnect attempt, which is a storm precisely when the database is already unhappy. The undelivered payloads stay pending and the backoff handles recovery instead.runNotifyLoopnow takes a context rather than exiting when a channel is closed. Closing a channel that writers send on would panic the write path, and the loop still needs a stop condition the tests can drive. Production passescontext.Background(), so the running behaviour is unchanged.Level of fix
Fixed at the notify layer inside
action.go, which owns both writers and the pump, rather than at either call site. I enumerated the class of the same defect shape acrossruns/:updates <- run/updates <- actionin the fourWatch*methods (action.go:707,:760,:826,:884) block, but each runs in its own stream-serving goroutine. Blocking there is backpressure against a slow client and is correct. Not the same defect.ch <- notif.ExtrainprocessNotificationsis already non-blocking with adefaultthat drops. That is the subscriber side, and feat: add Prometheus counter for dropped subscriber updates #7643 is already open to instrument those drops. Left alone.dedupeQueue.pushinruns/service/abort_reconciler.go:70-88is the same defect and this PR does not fix it. It has the same shape, a blocking send into a 1000-slot buffer guarded only by<-ctx.Done(), andAbortRuncalls it atrun_service.go:530after the row is already marked ABORTED at line 521. So a backed-up reconciler blocks that RPC too, and a disconnecting client drops the abort task and deletes its dedup key, so nothing re-pushes it. I left it alone because fixing it means touching a different subsystem and its tests in a PR scoped to the files this issue names. Happy to open a follow-up issue, or to fold it in here if you would rather see them land together.So the write path has two members of this class and this PR fixes the notify one. I considered lifting a shared coalescing-pending-set type that both could use. The cost is a new internal package plus a rewrite of the reconciler's tests, which puts two subsystems in one review, so I would rather do it as a follow-up once the shape here has been reviewed.
Batching the pump's round trips (#7756) composes with this cleanly. The pump already hands
emita whole set, so batching becomes a change insideemitrather than a change to the structure. I have not touched instrumentation, since #7758 covers it.How was this patch tested?
The package brings up an embedded PostgreSQL on port 15432 by itself, so the delivery tests below issue real
pg_notifystatements and read them back through the real listener. The blocking, coalescing, and retry tests are white box and touch no database.Red then green. Both new blocking tests fail on unmodified
mainat2ad38dca6:and pass after the change, along with the rest of the new coverage:
Mapping tests to the outcomes listed in the issue:
TestNotifyActionUpdate_DoesNotBlockOnStalledPump,TestNotifyRunUpdate_DoesNotBlockOnStalledPumpTestUpdateActionPhase_CompletesWithStalledPumpTestNotifyPump_ConcurrentWritersDeliverEveryActionpg_notifyretries rather than droppingTestRunNotifyLoop_RetriesUndeliveredPayloadsTestNotifyPump_CoalescesRepeatsOnTheWire(on the wire),TestNotifyActionUpdate_CoalescesRepeats(in the set)TestNotifyActionUpdate_KeepsWakeupAfterContextCancelTestWatchActionUpdates_DeliversPhaseChangeTestRunNotifyLoop_DropsPayloadPostgresWillNeverAcceptTwo of these are worth a note.
TestNotifyPump_CoalescesRepeatsOnTheWirequeues all 500 updates for one action before the pump starts, so the whole burst is guaranteed to be one drain. It then asserts one delivery arrives and that a second never does. That makes the collapse observable on the wire rather than by asserting that a Go map deduplicates.TestRunNotifyLoop_DropsPayloadPostgresWillNeverAcceptwas the one that nearly fooled me. Its first version polled the pending set for the poison key's absence, and it passed even with the retry limit raised to 100000, because the pump briefly holds a taken batch outside the pending set and a 20 ms poll lands in that window eventually. It now requires 25 consecutive absent readings. Mutation checked both ways: withnotifyRetryLimitraised to 100000 it fails after 15 s, and at 10 it passes in 1 s.Race detector, since this is a concurrency change:
Full tree without the race detector:
For a baseline I ran
./runs/repository/impl/...on a clean checkout of2ad38dca6before touching anything:ok ... 25.838s. There were no pre-existing failures in this package to work around, so every red result quoted above came from the change itself.Labels
Check all the applicable boxes
What was not tested
The 100,000-action benchmark from
flyteorg/benchmarkthat produced the p95 numbers in the issue. I have no dev cluster to runswarm.pyagainst, so the latency improvement is argued from the structure and from the concurrency test rather than re-measured end to end. If someone with a cluster reruns it, the number to watch isrpc_server_duration_millisecondsforUpdateActionStatus.A real connection loss or failover.
TestRunNotifyLoop_RetriesUndeliveredPayloadsuses a nil connection with no database to reconnect to, which exercises the same branch, but it is not the same as pulling the plug on a live Postgres mid-drain.The pending set still has no upper bound, which is what the issue proposes and I did not change. During a long outage it is bounded by the number of distinct actions being updated rather than by anything the process controls, whereas the old 256-slot buffer at least applied backpressure. At the issue's own figure of 65,000 live actions that is a few MB, so I left it alone, but if you would rather have a cap plus a dropped-notification counter I am happy to add it, and #7758 looks like the natural home for the counter.
I did not check whether
InternalRunService.UpdateActionStatusis reachable by anything other than the in-cluster operator, so I cannot say whether the oversized-payload path above is only an operational footgun or something a caller could aim. The retry limit bounds the damage either way.Related PRs
This change was developed with AI assistance from Claude. I reviewed and verified it before submitting.