[Flyte 7757] Prevent NOTIFY from blocking action writes by 0yukali0 · Pull Request #7965 · flyteorg/flyte

0yukali0 · 2026-09-02T23:39:43Z

Tracking issue

Why are the changes needed?

Action and run updates could block writes when the notification queue was full, or be lost when notification delivery failed.
This issue is addressed by PR-7787.

What changes were proposed in this pull request?

Replaced fixed notification queues with deduplicated pending sets.
Made update writes return immediately without waiting for notification delivery.
Retried failed notifications instead of dropping them.
Merged repeated updates for the same action or run.
Added tests for stalled queues, retries, concurrent updates, and end-to-end delivery.

How was this patch tested?

go test ./runs/repository/impl

Labels

fixed: For any bug fixed.

Setup process

A stable environment to run the performance have 24 core and 64G is better.
Or building a k3s cluster with multiple machines which provides 18core and 38G.
Updating k3s setting

# git clone flyte
make devbox-build && make devbox-run && make devbox-monitoring

# ssh postgres container
 k3s_node="$(kubectl get nodes -o jsonpath='{.items[0].metadata.name}')"

  kubectl debug "node/${k3s_node}" -it \
    --image=nicolaka/netshoot --profile=sysadmin -- bash
    
 tc qdisc replace dev cni0 root handle 1: prio bands 3
  tc qdisc replace dev cni0 parent 1:3 handle 30: netem delay 15ms
  tc filter replace dev cni0 protocol ip parent 1: \
    prio 1 u32 match ip protocol 6 0xff \
    match ip sport 5432 0xffff flowid 1:3

  tc -s qdisc show dev cni0
  exit


# git clone flyte-benchmark
uv run scripts/v2/swarm.py --k 25 --n 2000

Screenshots

Check all the applicable boxes

I updated the documentation accordingly.
All new and existing tests passed.
All commits are signed-off.

Related PRs

Stack

If you do use git town to manage PR Stacks, the stack relevant to this PR
will show below. Otherwise, you can ignore this section.

Docs link

0yukali0 · 2026-09-03T18:20:13Z

experiment of this pr with 15ms delay setting on Postgres.

result based on main branch

RPC	rate	p50	p95	mean
InternalRunService.UpdateActionStatus	7.65/s	21.7 ms	10s	12.5 ms
InternalRunService.RecordActionEvents	3.87/s	3.04s	10s	5.99 ms
ActionsService.Enqueue	276/s		736ms	20.2ms

result from this branch.

RPC	rate	p50	p95	mean
InternalRunService.UpdateActionStatus	31.8/s	21.7 ms	99.7 ms	31.8 ms
InternalRunService.RecordActionEvents	22/s	36.1 ms	123 ms	41 ms
ActionsService.Enqueue	1.23/s		3.92s	35.4ms

popojk · 2026-09-04T04:12:29Z

-	runNotifyCh    chan string
+	notifyMu       sync.Mutex
+	pendingActions map[string]struct{}
+	pendingRuns    map[string]struct{}


Why do we change the data structure to map here? I think we will not be able to guarantee FIFO with a map

Using a map alone does not guarantee FIFO ordering.
I'll add a slice to preserve the order when sending batch notifications.

popojk · 2026-09-04T04:19:54Z

-	repo.actionNotifyCh = make(chan string, 256)
-	repo.runNotifyCh = make(chan string, 256)
+	repo.pendingActions = make(map[string]struct{})
+	repo.pendingRuns = make(map[string]struct{})


I think we should still set a boundary for the notification buffer. In case where db shut down, the buffer will keep accumulating the notifications to a point where the run service OOM. I think we can try to find out a buffer size that could pass our pressure test(e.g. 500k actions). WDYT?

Good catch.
I think that set a boundary to prevent from OOM is a great idea.
When exceeding the boundary, LRU strategy remove the old notification.

kumare3 · 2026-09-04T06:24:40Z

This is pretty good

0yukali0 · 2026-09-05T01:13:13Z

Result from this branch. (heap and FIFO try it best) (Deprecated)

RPC	rate	p50	p95	mean
InternalRunService.UpdateActionStatus	33.6/s	21.1 ms	93.1 ms	127ms
InternalRunService.RecordActionEvents	21.2/s	36.1 ms	99.4 ms	22-6 ms
ActionsService.Enqueue	1.75/s		3.49s	38-11ms

I will set 50k as the boundary

0yukali0 · 2026-09-08T17:22:26Z

Result from this branch. FIFO and deduplicate

RPC	rate	p50	p95	mean
InternalRunService.UpdateActionStatus	29.1/s	20.9 ms	91.7ms	289ms
InternalRunService.RecordActionEvents	38.3/s	37.1 ms	93.4 ms	24.1 ms
ActionsService.Enqueue	0.6-1.36/s		1.91s	45-7ms

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> Signed-off-by: Yuteng Chen <a08h0283@gmail.com>

Signed-off-by: Yuteng Chen <a08h0283@gmail.com>

popojk

Looks really good. I just have a small comment. Then we are good to go. Thanks!

popojk · 2026-09-11T04:17:37Z

+	notificationBufferLimit     = 65_000
+	pendingNotificationCapacity = 256
+	notifyRetryMinBackoff       = 50 * time.Millisecond
+	notifyRetryMaxBackoff       = 5 * time.Second


Can we make these settings configurable in the config file while at the same time keeping the default settings? In this way it is easier for perf test.

github-actions Bot added the flyte2 label Sep 2, 2026

0yukali0 changed the title ~~Flyte 7757~~ [Flyte 7757] Prevent NOTIFY from blocking action writes Sep 2, 2026

0yukali0 marked this pull request as ready for review September 4, 2026 00:20

popojk reviewed Sep 4, 2026

View reviewed changes

0yukali0 marked this pull request as draft September 5, 2026 01:03

0yukali0 marked this pull request as ready for review September 5, 2026 07:54

0yukali0 marked this pull request as draft September 8, 2026 05:25

0yukali0 force-pushed the flyte-7757 branch from ec708f1 to 90fb5f2 Compare September 8, 2026 17:27

stantheman0128 and others added 7 commits September 9, 2026 01:32

keep pending notifications until delivery succeeds

08ad675

Signed-off-by: Yuteng Chen <a08h0283@gmail.com>

Implement LRU and heap to prevent from OOM

ddbd756

Signed-off-by: Yuteng Chen <a08h0283@gmail.com>

Limit pending notifications to 50k

f517b67

Signed-off-by: Yuteng Chen <a08h0283@gmail.com>

Fix pending notification assertions

a79db83

Signed-off-by: Yuteng Chen <a08h0283@gmail.com>

Simple solution: slice for FIFO and LRU

64b2d16

Signed-off-by: Yuteng Chen <a08h0283@gmail.com>

merge tests and adopting table-driven

b69da2e

Signed-off-by: Yuteng Chen <a08h0283@gmail.com>

0yukali0 force-pushed the flyte-7757 branch from 90fb5f2 to b69da2e Compare September 8, 2026 17:50

0yukali0 marked this pull request as ready for review September 8, 2026 17:51

popojk reviewed Sep 11, 2026

View reviewed changes

Navigation Menu

Uh oh!

[Flyte 7757] Prevent NOTIFY from blocking action writes - #7965

[Flyte 7757] Prevent NOTIFY from blocking action writes#7965
0yukali0 wants to merge 7 commits into
flyteorg:mainfrom
0yukali0:flyte-7757

0yukali0 commented Sep 2, 2026 •

edited

Loading

Uh oh!

0yukali0 commented Sep 3, 2026 •

edited

Loading

Uh oh!

popojk Sep 4, 2026

Uh oh!

0yukali0 Sep 4, 2026

Uh oh!

popojk Sep 4, 2026

Uh oh!

0yukali0 Sep 4, 2026

Uh oh!

kumare3 commented Sep 4, 2026

Uh oh!

0yukali0 commented Sep 5, 2026 •

edited

Loading

Uh oh!

0yukali0 commented Sep 8, 2026 •

edited

Loading

Uh oh!

popojk left a comment

Uh oh!

popojk Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

0yukali0 commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Labels

Setup process

Screenshots

Check all the applicable boxes

Related PRs

Stack

Docs link

Uh oh!

0yukali0 commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

result based on main branch

result from this branch.

Uh oh!

popojk Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

0yukali0 Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

popojk Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

0yukali0 Sep 4, 2026

Choose a reason for hiding this comment

Uh oh!

kumare3 commented Sep 4, 2026

Uh oh!

0yukali0 commented Sep 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Result from this branch. (heap and FIFO try it best) (Deprecated)

Uh oh!

0yukali0 commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Result from this branch. FIFO and deduplicate

Uh oh!

popojk left a comment

Choose a reason for hiding this comment

Uh oh!

popojk Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

0yukali0 commented Sep 2, 2026 •

edited

Loading

0yukali0 commented Sep 3, 2026 •

edited

Loading

0yukali0 commented Sep 5, 2026 •

edited

Loading

0yukali0 commented Sep 8, 2026 •

edited

Loading