fix(realtime): catch up missed board events after rejoin - #3320
Chris0Jeky wants to merge 3 commits into
Conversation
|
@codex review Please review exact head The first implementation head Run: https://github.com/Chris0Jeky/Taskdeck/actions/runs/35591987360 ; JUnit artifact |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cc235f6fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Events lost while disconnected are not replayed by a new subscription. | ||
| // Reuse the mutation coordinator so an older pending read retains one | ||
| // follow-up instead of swallowing the catch-up or starting parallel reads. | ||
| startMutationRefresh(boardId) |
There was a problem hiding this comment.
When a 30-second fallback tick has already started fetchBoard as JoinBoard acknowledges, this call does not guarantee a post-rejoin read. In the production path, boardCrudStore.fetchBoard returns the existing same-board background promise (boardCrudStore.ts:375-395), so a request whose snapshot predates a disconnected-period mutation absorbs the catch-up without queuing a successor; that mutation was not delivered over SignalR and can remain absent until another event or manual reload. Coordinate fallback reads with this recovery slot, or otherwise retain one read that starts after the acknowledged rejoin.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 072d252b0e258fd4a28443f547c6ea3e34ee49bb. The store really does reuse an active same-board background promise, so fallback ticks now enter the same controller-owned active/pending refresh slot as mutation and recovery reads. A catch-up waits for the older fallback to settle and then starts one successor. The formal deferred-read regression now runs for both mutation and fallback sources. A real-controller/synthetic-transport supplementary probe reproduced this failure on 3cc235f and passes on the repair; exact-head Vitest/CI is pending. No store contract or shared active PR path changed.
| } | ||
| return | ||
| } | ||
| if (!isCurrentRequest() || hubConnection.state !== HubConnectionState.Connected) return |
There was a problem hiding this comment.
When switchBoard(B) is called while this callback is awaiting JoinBoard(A), the generation becomes stale and this return abandons the only recovery catch-up. The queued B transition eventually joins B and stops fallback polling, while the existing interval is still bound to A and ignores ticks once requestedBoardId is B. Because BoardView.vue:350-356 finishes loading B before calling switchBoard, a B mutation occurring during the delayed join is newer than the displayed payload, is not delivered over SignalR, and can remain missing indefinitely; the recovery obligation should transfer to the latest queued board rather than disappear with A's generation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 072d252b0e258fd4a28443f547c6ea3e34ee49bb. Recovery now remains pending until the latest request's acknowledged join schedules catch-up. switchBoard transfers pending recovery and polling to its requested board; stale A completion cannot consume B's obligation. The switch/stop regression now positively asserts one B catch-up on switch and no abandoned-A read, while stop discards recovery. A new failed-B-rejoin case proves polling remains bound to B. The two review schedules failed before the repair and pass in the supplementary real-controller transport probe (4/4 including original controls); exact-head hosted verification remains pending.
|
@codex review Please re-review head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 072d252b0e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| stopFallbackPolling() | ||
| if (recoveryPending) { | ||
| recoveryPending = false | ||
| startBoardRefresh(boardId) |
There was a problem hiding this comment.
The new shared slot covers only refreshes started by this controller. A card archive/restore can concurrently start a background read directly through refreshBoardAfterLifecycleChange (CardModal.vue:145 and CardModal.vue:422), while boardCrudStore.fetchBoard returns an active same-board background promise instead of queuing a successor (boardCrudStore.ts:375-395). If that request's snapshot predates a mutation missed during the disconnect, this recovery call merely joins the old promise after polling and recovery have been retired, so no post-rejoin read remains. This is fresh evidence beyond the earlier fallback-read thread because the new slot still excludes these direct background-read producers; queue recovery at the store boundary or otherwise force one read after the active request.
Useful? React with 👍 / 👎.
| if (isCurrentRequest() && hubConnection.state === HubConnectionState.Connected) { | ||
| stopFallbackPolling() | ||
| if (recoveryPending) { | ||
| recoveryPending = false |
There was a problem hiding this comment.
When the post-rejoin HTTP read transiently fails, this clears the recovery obligation after fallback polling has already stopped. In production, the adapter in BoardView.vue:132-152 discards boardStore.fetchBoard's boolean result, and a background failure resolves false rather than rejecting in boardCrudStore.ts:539-555, so the controller treats the failed catch-up as completed. If no later mutation arrives, changes missed during the disconnection remain absent indefinitely; preserve recovery or schedule a bounded retry until the catch-up reports that it committed.
Useful? React with 👍 / 👎.
1 participant
Closes #3319.
Repair
No dependency, workflow, backend or store changes. The separate server-side JoinBoard investigation #1521 remains open.
Exact current head
072d252b0e258fd4a28443f547c6ea3e34ee49bb, based on main307c3b8b50bec1cb0bfaea3e570a942bcb1d4451.Four files: the controller, ten new recovery cases, and two existing reconnect assertions updated to require exactly one catch-up while independently proving periodic polling stops. No assertions are skipped or weakened to allow missing recovery.
Evidence and review history
6fb7f2d...: normal Ubuntu frontend lint/typecheck/build/PWA validation passed; all eight then-new recovery cases passed. Full JUnit recorded 7,158 tests, one failure, zero errors. The sole failure was an older resilience test asserting no reconnect read. Run: https://github.com/Chris0Jeky/Taskdeck/actions/runs/35591987360 . That outdated expectation was corrected in3cc235f...without changing production behavior.3cc235f...found two valid P2 gaps: an in-flight fallback could swallow catch-up, and navigation during rejoin could discard recovery. Both were reproduced before correction and are addressed by current head. The supplementary probe now passes 4/4; ten formal recovery tests include both schedules and failed recovery transfer. Inline threads contain detailed replies.Local npm registry DNS failed (EAI_AGAIN), and installed Node 22 is below the repository's Node 24 contract. Locked-dependency runtime qualification therefore uses normal hosted Actions, without engine/dependency/workflow bypasses.
Handoff
Keep draft until current-head frontend/full CI and the repeat review are inspected. Recheck current main and changed paths before continuing. Do not merge, alter release gates or infer that successful unit checks qualify real-server SignalR behavior. This PR does not own board/session-store invalidation (#3305/#3306/#3324).