fix(socket): guard against missing operator during store patch#2962
fix(socket): guard against missing operator during store patch#2962grolu wants to merge 4 commits into
Conversation
Defer socket events when the store is uninitialized (list is null) and log an error when the operator cannot be created for an already-initialized store, distinguishing transient startup timing from permanent misconfiguration.
grolu
requested review from
holgerkoser,
klocke-io and
petersutter
as code owners
gardener-prow
Bot
added
area/quality
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
gardener-prow
Bot
added
size/S
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe handler treats an uninitialized list store as an explicit error, adds restore/flush helpers to re-queue buffered socket events, gates flushing on store initialization and document visibility, and wires a watcher to flush when the store becomes ready. Tests validate queuing, requeue-on-reset, immediate sync when initialized, and operator error behavior. ChangesSocket event handler refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
gardener-prow
Bot
added
cla: no
Socket events arriving before the initial list fetch now queue until the store transitions out of its null/initial state. Events are restored (not dropped) when the store resets mid-synchronize. Operator failures surface as errors instead of being masked as transient initialization delays.
gardener-prow
Bot
added
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/src/composables/useSocketEventHandler.js`:
- Around line 119-132: Before awaiting socketStore.synchronize(pluralName, uids)
capture the store's current initialization epoch/token (e.g., call
isInitialized(store) or read an initEpoch property) and store it in a local
variable; after the await and before applying patches/using items, re-check the
store's current epoch and only proceed if it matches the captured epoch,
otherwise call restoreEvents(events) and return. Update the logic around
socketStore.synchronize, isInitialized(store), restoreEvents(events), and any
code that applies items to the store to use this epoch comparison so transient
reset→reinit cycles won’t patch stale results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06e75b63-ed22-45bc-a476-e3772877bce7
📒 Files selected for processing (2)
frontend/__tests__/composables/useSocketEventHandler.spec.jsfrontend/src/composables/useSocketEventHandler.js
| items = await socketStore.synchronize(pluralName, uids) | ||
| } catch (err) { | ||
| if (isTooManyRequestsError(err)) { | ||
| logger.info('Skipped synchronization of modified %s: %s', pluralName, err.message) | ||
| } else { | ||
| logger.error('Failed to synchronize modified %s: %s', pluralName, err.message) | ||
| } | ||
| // Synchronization failed -> Rollback events | ||
| restoreEvents(events) | ||
| return | ||
| } | ||
| if (!isInitialized(store)) { | ||
| restoreEvents(events) | ||
| return |
There was a problem hiding this comment.
Track store resets across the async gap.
This only checks isInitialized(store) after socketStore.synchronize(...) resolves. If the store briefly resets to its initial state and reinitializes before the await finishes, this branch will still patch stale results into the new store state instead of requeueing them. Please capture a reset/init epoch before the await and compare it again before applying the patch, so transient reset→reinit cycles are treated the same as the slower reset case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/composables/useSocketEventHandler.js` around lines 119 - 132,
Before awaiting socketStore.synchronize(pluralName, uids) capture the store's
current initialization epoch/token (e.g., call isInitialized(store) or read an
initEpoch property) and store it in a local variable; after the await and before
applying patches/using items, re-check the store's current epoch and only
proceed if it matches the captured epoch, otherwise call restoreEvents(events)
and return. Update the logic around socketStore.synchronize,
isInitialized(store), restoreEvents(events), and any code that applies items to
the store to use this epoch comparison so transient reset→reinit cycles won’t
patch stale results.
There was a problem hiding this comment.
Isn't it the case that if the store resets during an active synchronize(), the connection was lost.. which triggers a disconnect/reconnect cycle that initiates a fresh synchronize with the new store state anyway? The stale patch would never survive because the entire sync restarts.
Wouldn't adding an epoch counter introduce complexity for a race condition that already self-heals through the existing reconnect mechanism?
There was a problem hiding this comment.
@petersutter, that's a well-reasoned architectural argument. You're right that in practice:
- A store reset during an in-flight
synchronize()is driven by a connection loss. - The disconnect/reconnect cycle triggers a fresh full synchronization, which overwrites any stale patch that may have briefly landed on the reinitialized store.
- The existing post-
awaitisInitialized(store)check already covers the simpler case where the store is still in reset state whensynchronize()resolves.
Given the self-healing guarantee from the reconnect mechanism, adding an epoch counter would be extra complexity for a race window that the system architecture already handles. I'll withdraw this concern.
(´･ω･`)ﾉ
✏️ Learnings added
Learnt from: petersutter
Repo: gardener/dashboard PR: 2962
File: frontend/src/composables/useSocketEventHandler.js:119-132
Timestamp: 2026-05-13T16:56:42.824Z
Learning: In `frontend/src/composables/useSocketEventHandler.js` (gardener/dashboard), a store reset during an in-flight `socketStore.synchronize()` is always tied to a connection loss and a subsequent disconnect/reconnect cycle. The reconnect cycle triggers a fresh full synchronization that overwrites any stale patch, so the post-await `isInitialized(store)` check is sufficient; adding an epoch counter for the transient reset→reinit race is unnecessary complexity.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
2 participants
/area quality
/kind bug
What this PR does / why we need it:
Guards the socket event handler against a missing operator when the
store has not been initialized yet. Events arriving before store
initialization are deferred and requeued. If the operator cannot be
created for an already-initialized store (permanent misconfiguration),
an error is logged instead of silently swallowing events.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
The default operator returns undefined when
state.listis not anarray. Stores initialize
listasnulland set it to an array afterfetching. The guard distinguishes these two cases to avoid hiding real
failures behind a debug log.
Release note:
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests