codeant-ai-for-open-source[bot] commented on code in PR #42033:
URL: https://github.com/apache/superset/pull/42033#discussion_r3600768719
##########
superset-frontend/src/middleware/asyncEvent.ts:
##########
@@ -151,26 +191,22 @@ const setLastId = (asyncEvent: AsyncEvent) => {
export const processEvents = async (events: AsyncEvent[]) => {
events.forEach((asyncEvent: AsyncEvent) => {
const jobId = asyncEvent.job_id;
- const listener = listenersByJobId.get(jobId);
- // `jobId` originates from server/WebSocket payloads, so the listener is
- // resolved exclusively through a Map (never plain-object property access,
- // which would expose the prototype chain), and we confirm the retrieved
- // value is a registered function before dispatching the event to it.
- if (typeof listener === 'function') {
+ const listener = listenersByJobId[jobId];
+ if (listener) {
listener(asyncEvent);
- retriesByJobId.delete(jobId);
+ delete retriesByJobId[jobId];
} else {
// handle race condition where event is received
// before listener is registered
- const retries = (retriesByJobId.get(jobId) ?? 0) + 1;
- retriesByJobId.set(jobId, retries);
+ if (!retriesByJobId[jobId]) retriesByJobId[jobId] = 0;
+ retriesByJobId[jobId] += 1;
- if (retries <= MAX_RETRIES) {
+ if (retriesByJobId[jobId] <= MAX_RETRIES) {
setTimeout(() => {
processEvents([asyncEvent]);
- }, RETRY_DELAY * retries);
+ }, RETRY_DELAY * retriesByJobId[jobId]);
} else {
- retriesByJobId.delete(jobId);
+ delete retriesByJobId[jobId];
logging.warn('listener not found for job_id', asyncEvent.job_id);
}
Review Comment:
**Suggestion:** The retry path drops an event permanently after
`MAX_RETRIES`, but there is no fallback to deliver that terminal event when the
listener is registered later. This can leave the corresponding async chart
request hanging forever (no resolve/reject) if the event arrives before
listener registration and registration is delayed (for example in heavy UI work
or background-tab throttling). Keep unmatched terminal events in a per-job
buffer and flush them from `addListener`, or explicitly fail pending jobs when
retries are exhausted. [race condition]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
❌ Charts using async queries can hang indefinitely awaiting events.
❌ Native filter controls may never load default async values.
⚠️ AsyncEvent tests don't cover listener-missing race drop behavior.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Start the Superset frontend with the GlobalAsyncQueries feature flag
enabled so the
async event middleware `init` runs
(superset-frontend/src/middleware/asyncEvent.ts:34-60,
with `init();` at line 62 starting polling via `loadEventsFromApi` or
WebSocket via
`wsConnect`).
2. Trigger a chart-data request that is handled by `exploreJSON` in
superset-frontend/src/components/Chart/chartAction.ts:734-942 (e.g., loading
a chart or
dashboard), which eventually calls `handleChartDataResponse` (lines 700-752)
and, when the
HTTP response status is 202, invokes `waitForAsyncData(...)` (lines 36-45 in
the excerpt
at 680-759) with the async event payload.
3. In the async middleware, `waitForAsyncData`
(superset-frontend/src/middleware/asyncEvent.ts:107-171) calls
`addListener(jobId,
listener)` (lines 78-80, 171) to register a listener in `listenersByJobId`.
If an async
event for that job (status DONE or ERROR) is delivered by
`loadEventsFromApi` (lines
226-246) before the listener is registered (due to network/UI delay or
background tab
throttling), `loadEventsFromApi` still calls `fetchEvents(eventArgs)` and
then
`processEvents(events)` (line 234).
4. Inside `processEvents`
(superset-frontend/src/middleware/asyncEvent.ts:191-215), when
`listenersByJobId[jobId]` is undefined the else branch (lines 199-211) runs:
it increments
`retriesByJobId[jobId]` (lines 201-202), schedules
`processEvents([asyncEvent])` via
`setTimeout` (lines 204-207), and after more than `MAX_RETRIES` (line 59)
deletes
`retriesByJobId[jobId]` and logs a warning (lines 208-210). The event is
dropped
permanently while `setLastId(asyncEvent)` (line 213) advances
`lastReceivedEventId`. When
`waitForAsyncData` is eventually called for that job from ChartAction.ts
(lines 36-45),
FilterValue.tsx (lines 25-37), or FiltersConfigForm.tsx (lines 24-34), no
further events
for that job ever reach the listener, so the Promise returned by
`waitForAsyncData` never
resolves or rejects and the associated chart or filter stays stuck in a
loading state.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=89e07b5049e54b86b3b355834bed1022&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=89e07b5049e54b86b3b355834bed1022&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/middleware/asyncEvent.ts
**Line:** 204:211
**Comment:**
*Race Condition: The retry path drops an event permanently after
`MAX_RETRIES`, but there is no fallback to deliver that terminal event when the
listener is registered later. This can leave the corresponding async chart
request hanging forever (no resolve/reject) if the event arrives before
listener registration and registration is delayed (for example in heavy UI work
or background-tab throttling). Keep unmatched terminal events in a per-job
buffer and flush them from `addListener`, or explicitly fail pending jobs when
retries are exhausted.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42033&comment_hash=22d750aaaa676f780db6c92a1044197ec5cef5b0b3483a599ad961401fd9ce3d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42033&comment_hash=22d750aaaa676f780db6c92a1044197ec5cef5b0b3483a599ad961401fd9ce3d&reaction=dislike'>👎</a>
##########
superset-frontend/src/middleware/asyncEvent.ts:
##########
@@ -190,7 +226,7 @@ const getPollingDelay = () => {
const loadEventsFromApi = async () => {
const generation = pollingGeneration;
const eventArgs = lastReceivedEventId ? { last_id: lastReceivedEventId } :
{};
- if (listenersByJobId.size) {
+ if (Object.keys(listenersByJobId).length) {
Review Comment:
**Suggestion:** Conditionally skipping `fetchEvents` when there are no
active listeners prevents `lastReceivedEventId` from advancing during idle
periods, so the next active request can trigger a large backlog fetch and
replay of stale events. That creates avoidable latency spikes and extra timer
churn before current jobs are processed. Continue polling to advance
`lastReceivedEventId` (even without listeners), and only gate listener
dispatching. [performance]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ First async chart after idle can incur backlog delay.
⚠️ Async polling performs extra retries for stale unhandled events.
⚠️ Potential spikes in network and timer work on resume.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. On frontend startup, the async middleware `init`
(superset-frontend/src/middleware/asyncEvent.ts:34-60) is executed (line
62), loading
`lastReceivedEventId` from localStorage (lines 48-52) and starting polling
via
`loadEventsFromApi` when `transport === TRANSPORT_POLLING` (lines 54-56).
2. While there are no active async chart jobs, `listenersByJobId` remains
empty because
the only place that registers listeners is `waitForAsyncData`
(asyncEvent.ts:107-171),
which is invoked from chart and native filter code when an HTTP 202
chart-data response is
received (ChartAction.ts:17-45 at 680-759, FilterValue.tsx:18-37 at 270-329,
FiltersConfigForm.tsx:15-35 at 520-579).
3. During this idle period, any async events emitted on the server-side for
this user are
not fetched by the client because `loadEventsFromApi`
(asyncEvent.ts:226-246) wraps the
call to `fetchEvents(eventArgs)` and subsequent `processEvents(events)`
inside `if
(Object.keys(listenersByJobId).length) { ... }` (lines 229-240); with zero
listeners, the
function returns after the guard and `lastReceivedEventId` (used in
`eventArgs` at line
228) never advances.
4. When a new chart-data request later receives a 202 and registers a
listener via
`waitForAsyncData`, the next invocation of `loadEventsFromApi` finally calls
`fetchEvents(eventArgs)` with a stale `lastReceivedEventId`. The backend
returns the
backlog of all events since that cursor, and `processEvents(events)`
(asyncEvent.ts:191-215) iterates them: events for jobs without listeners go
through the
retry loop (lines 199-211) before being dropped, and only then are current
job events
processed. This creates an avoidable latency spike for the first active
async chart and
extra `setTimeout` churn as stale events are retried and logged before the
latest job’s
data is delivered.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2878e2de98094754ae607a2ecc2b70cf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=2878e2de98094754ae607a2ecc2b70cf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/middleware/asyncEvent.ts
**Line:** 229:240
**Comment:**
*Performance: Conditionally skipping `fetchEvents` when there are no
active listeners prevents `lastReceivedEventId` from advancing during idle
periods, so the next active request can trigger a large backlog fetch and
replay of stale events. That creates avoidable latency spikes and extra timer
churn before current jobs are processed. Continue polling to advance
`lastReceivedEventId` (even without listeners), and only gate listener
dispatching.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42033&comment_hash=95ad4554b5c8b527282b2fa9dbaa5c94836cead9eaf2b4874775ebe3a916c4e6&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42033&comment_hash=95ad4554b5c8b527282b2fa9dbaa5c94836cead9eaf2b4874775ebe3a916c4e6&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
