Skip to content

fix(store): prevent batch race condition causing silent state loss in nested batches#1663

Open
atul-upadhyay-7 wants to merge 1 commit into
Karanjot786:mainfrom
atul-upadhyay-7:fix/store-batch-race-condition
Open

fix(store): prevent batch race condition causing silent state loss in nested batches#1663
atul-upadhyay-7 wants to merge 1 commit into
Karanjot786:mainfrom
atul-upadhyay-7:fix/store-batch-race-condition

Conversation

@atul-upadhyay-7

@atul-upadhyay-7 atul-upadhyay-7 commented Jun 19, 2026

Copy link
Copy Markdown

Summary

Fixes a critical race condition in @termuijs/store's batch() mechanism where nested batch calls could silently lose intermediate state mutations, and where a stale microtask could dispatch notifications for the wrong batch.

Closes #1658

Changes

1. Epoch guard for microtask dispatch

Added _batchEpoch counter that increments when an outermost batch starts. flushBatch() snapshots the epoch before scheduling the microtask. If a new batch starts before the microtask fires, the epoch will have changed and the stale microtask bails out without dispatching.

Before: Two consecutive batches could both dispatch, with the first microtask reading the second batch's entries.

After: The first microtask detects the epoch mismatch and skips dispatch.

2. mutate() updates rollback closure in existing batch entries

When mutate() found an existing batch entry, it only updated existing.nextState -- the commit and rollback closures were never updated. This meant:

  • The rollback restored to an intermediate state, not the true pre-batch state
  • The commit captured a stale nextState

After: Both commit and rollback closures are updated when an existing entry is found.

3. Cleanup: removed unused prevState destructuring in error path

The flushBatch(true) path destructured prevState but never used it.

Tests added

Test What it verifies
nested batch does not lose intermediate state Outer setState survives inner setState
consecutive batches do not interfere Two sequential batches both apply correctly
stale microtask from old batch does not dispatch Epoch guard prevents double-dispatch
mutate inside batch merges correctly with setState mutate() + setState() coalesce properly
multiple mutate calls inside batch coalesce Three mutate() calls produce one notification
batch rollback restores state before any updates including mutate Error in batch with mutate() rolls back completely

Verification

  • All 91 store tests pass
  • Typecheck passes across all packages
  • No breaking changes to public API

Summary by CodeRabbit

  • Bug Fixes

    • Improved handling and reliability of batch operations when nested or overlapping.
    • Enhanced error recovery with better rollback semantics for failed batch updates.
    • Fixed state consistency issues that could occur across overlapping batch cycles.
    • Optimized microtask scheduling to prevent stale state from being committed.
  • Tests

    • Extended test suite with comprehensive coverage for advanced batch scenarios.

… nested batches

Add epoch guard to batch microtask dispatch to prevent stale notifications
when a new batch starts before the previous microtask fires. Fix mutate()
inside batches to update both commit and rollback closures so error
recovery restores the correct pre-batch state.

Closes Karanjot786#1658
@github-actions github-actions Bot added type:bug +10 pts. Bug fix. type:testing +10 pts. Tests. needs-star PR author has not starred the repo. labels Jun 19, 2026
@github-actions

Copy link
Copy Markdown

Hi @atul-upadhyay-7 👋

Star this repo before your PR merges.

Why? GSSoC 2026 contributors who star get priority review and points credit. After you star, push any commit (or re-run this check). The needs-star label lifts automatically.

Thanks for your contribution to TermUI.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 Thanks for your first PR to TermUI, @atul-upadhyay-7.

Before your PR merges:

  1. Star the repo. Required. The star-check job blocks your merge otherwise.
  2. ✅ All checks green: build, test, typecheck.
  3. 🏷 PR title follows type: short description. Example: fix: handle empty list.
  4. 🔗 Link your closing issue in the description.

GSSoC 2026 points come from labels after merge:

  • gssoc:approved. +50 base points.
  • level:beginner / intermediate / advanced / critical. +20 / +35 / +55 / +80.
  • quality:clean / exceptional. x 1.2 / x 1.5.
  • type:*. Stackable bonus.

Your reviewer responds within 48 hours. Ping @Karanjot786 on Discord for urgent help.

🚀 Welcome to the cohort.

@coderabbitai

coderabbitai Bot commented Jun 19, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 98040acb-67b0-42a2-9660-f2f241378cf0

📥 Commits

Reviewing files that changed from the base of the PR and between 4b874c0 and 383ef93.

📒 Files selected for processing (2)
  • packages/store/src/store.test.ts
  • packages/store/src/store.ts

📝 Walkthrough

Walkthrough

_batchEpoch, a global counter, is added to store.ts and incremented on each outermost batch() entry. flushBatch now snapshots the epoch before scheduling a queueMicrotask; the microtask aborts commit and notification if the epoch advanced. Rollback in flushBatch and mutate's batch path are also corrected. Six new async tests validate these behaviors.

Changes

Batch epoch tracking and microtask staleness prevention

Layer / File(s) Summary
Epoch tracking and flushBatch rework
packages/store/src/store.ts
Adds _batchEpoch counter, increments it on outermost batch entry, reworks flushBatch to snapshot the epoch and bail out of the queued microtask if a newer batch has started, and fixes rollback in both flushBatch (iterating BatchEntry shapes) and mutate (restoring from existing.prevState).
New async batch tests
packages/store/src/store.test.ts
Adds six async batch test cases covering: nested batch state merge, sequential batch isolation, microtask staleness across overlapping batches, setState+mutate combination, multiple mutate coalescing into a single notification, and full rollback on thrown error.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

Possibly related PRs

  • Karanjot786/TermUI#1045: Both modify batch() transaction and rollback behavior in store.ts, with overlapping changes to commit/rollback handling and store.test.ts coverage for nested and rollback cases.
  • Karanjot786/TermUI#1322: Both update flushBatch async notification deferral via microtasks and adjust end-of-batch rollback handling, making the _batchEpoch bail-out directly related to that PR's async batch flushing approach.
  • Karanjot786/TermUI#1368: Both modify batch/flushBatch internals and commit/rollback handling for batched setState/mutate, with this PR's BatchEntry rollback fix being a direct continuation of that work.

Suggested labels

area:store, level:advanced, quality:clean

Suggested reviewers

  • Karanjot786

Poem

🐇 Hoppity-hop through the microtask queue,
No stale old epochs shall muddle the view!
Each batch gets a number, a fresh little stamp,
If a newer one comes, the old one gets damped.
Rollback restores what was broken before —
The store stays consistent, just like folklore! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main fix: preventing a batch race condition that causes silent state loss in nested batches, matching the core issue addressed in the PR.
Description check ✅ Passed The description provides a comprehensive summary of changes, explains the race condition fix with before/after context, lists all tests added, and includes verification details. However, the PR author did not fill in the required template sections (Related Issue, Which package(s), Type of Change, Checklist, and GSSoC fields), though essential information is present in the custom format.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-star PR author has not starred the repo. type:bug +10 pts. Bug fix. type:testing +10 pts. Tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] Store batch() race condition causes silent state loss in nested batches

1 participant