fix(sandbox): prevent state patch loss during block processing#15536
Closed
r-near wants to merge 4 commits into
Closed
fix(sandbox): prevent state patch loss during block processing#15536r-near wants to merge 4 commits into
r-near wants to merge 4 commits into
Conversation
sandbox_patch_state sometimes permanently drops patches. The patched account never appears, even on retry. This affects ~30% of first-patch-after-startup calls and occasionally hits mid-lifecycle patches too. Three interrelated bugs: 1. pending_state_patch.take() eagerly drains the patch queue when a block starts processing. If that block fails preprocessing (orphan, missing chunks, optimistic deferral), the patch is gone. Fix: clone a backup before preprocessing and restore it on any error. 2. patch_state_in_progress() checks is_empty(), which returns true immediately after take() — long before the patch is committed to state. The RPC returns "done" while the patch is still in-flight. Fix: generation counter (sandbox_patch_generation vs sandbox_patch_committed_gen) that only advances after DB commit. 3. At startup, block 1 has no new chunks. The runtime's apply() early-returns for old chunks and silently drops the state_patch. Fix: skip take() when the block has no new chunks, and only assign the patch to shards that actually have a new chunk.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #15536 +/- ##
==========================================
- Coverage 69.27% 69.25% -0.03%
==========================================
Files 937 937
Lines 210882 211094 +212
Branches 210882 211094 +212
==========================================
+ Hits 146093 146196 +103
- Misses 58983 59091 +108
- Partials 5806 5807 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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: 41d243287b
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes sandbox patch_state reliability by ensuring state patches aren’t dropped during block processing and by making the sandbox RPC status reflect DB-committed completion rather than “queue drained”.
Changes:
- Avoid draining
pending_state_patchfor blocks with no new chunks (startup race), and only pass patches to shards with a new chunk. - Restore drained patches back into
pending_state_patchwhen block preprocessing fails (orphan/missing chunks/optimistic deferral). - Add generation-based tracking (
sandbox_patch_generation/sandbox_patch_committed_gen) sopatch_state_in_progress()stays true until the patch’s block is committed.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
chain/chain/src/chain.rs |
Implements patch backup/restore on preprocess errors, avoids patch take on no-new-chunk blocks, and adds generation-based “in progress until commit” tracking. |
chain/chain/src/block_processing_utils.rs |
Extends BlockPreprocessInfo to carry the sandbox patch generation through preprocessing into postprocessing/commit. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Early-return in patch_state() when patch is empty to avoid bumping the generation counter with nothing to commit (would cause patch_state_in_progress() to stay true forever) - Update doc comment on BlockPreprocessInfo::sandbox_patch_generation to reflect actual usage
The generation counter fields and sandbox-specific logic paths (patch backup/restore, generation tracking, is_new_chunk guard) are only meaningful when the sandbox feature is enabled. Without cfg-gating, these code paths are compiled but unreachable on non-sandbox builds, causing codecov patch coverage failures since CI runs without the sandbox feature.
Contributor
|
I'll take a look soon... |
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Apr 16, 2026
## Background `sandbox_patch_state` sometimes permanently loses patches — the patched account never appears, even on retry. This was reported in the [Zulip thread](https://near.zulipchat.com/#narrow/channel/308695-nearone.2Fprivate/topic/Sandbox.20patch_state.20issues) and reproduced with [near-sandbox-rs#51](near/near-sandbox-rs#51). Three interrelated bugs: - **Patch lost on block failure**: `pending_state_patch.take()` drains the queue at the start of block processing. If preprocessing fails (orphan, missing chunks, optimistic deferral), the patch is gone forever. - **RPC returns too early**: `patch_state_in_progress()` checks `is_empty()`, which is true right after `take()` — before the patch is actually committed to state. - **Startup race**: Block 1 after genesis has no new chunks. The runtime's `apply()` early-returns for old chunks and silently drops the `state_patch`. ## What changed Introduces `SandboxPatchTracker` — a type that encapsulates the pending patch, a generation counter, and a committed-generation counter. Like `SandboxStatePatch`, it uses the ZST pattern: real struct on sandbox builds, zero-sized no-op on non-sandbox builds. This replaces the three separate fields on `Chain` (`pending_state_patch`, `sandbox_patch_generation`, `sandbox_patch_committed_gen`) with a single `sandbox_patches: SandboxPatchTracker`. The key design change is **taking the patch late** — at point of use inside the per-shard loop of `apply_chunks_preprocessing`, instead of at the top of `start_process_block_impl`. This eliminates the need for backup/clone/restore on error paths, since any error before the take leaves the patch in the tracker. The generation counter (`in_progress()` checks `generation != committed_gen`, not `is_empty()`) ensures the RPC doesn't return prematurely despite the late take. This removes the `state_patch` parameter from `preprocess_block` and `apply_chunks_preprocessing`, and results in zero `#[cfg(feature = "sandbox")]` blocks in `chain.rs`. ## Related - #15536 — alternative fix by @r-near that addresses the same three bugs with a backup/restore approach and 22 cfg-gates. This PR takes a different approach (take-late + tracker) to achieve the same result with less code deviation. - #14893 — fixed a different patch loss path (`clear()` in `postprocess_ready_block`)
Contributor
Author
|
Closing in favor of #15534 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
sandbox_patch_statesometimes permanently loses patches — the patched account never appears, even on retry. This has been a known pain point in near-workspaces-rs and near-sandbox-rs (both work around it by sending the patch twice).Three interrelated bugs:
pending_state_patch.take()drains the queue at the start of block processing. If preprocessing fails (orphan, missing chunks, optimistic deferral), the patch is gone forever.patch_state_in_progress()checksis_empty(), which is true right aftertake()— before the patch is actually committed to state. The RPC reports "done" while the patch is still in-flight.apply()early-returns for old chunks and silently drops thestate_patchpassed to it.Fix
sandbox_patch_generation/sandbox_patch_committed_gen) that only advances afterchain_update.commit(), so the RPC actually waits for the data to hit the DBtake()when the block has no new chunks; defense-in-depth check inapply_chunks_preprocessingonly assigns the patch to shards with a new chunkTesting
Tested with near-sandbox-rs#51 regression test (3 sandboxes × 50 patch+query cycles). Before this fix, ~6% failure rate. After: 0 failures across 3000+ iterations.
Also tested with a targeted startup-race diagnostic (30 sequential sandbox starts, single patch each). Before: ~23% permanent loss. After: 0.
Related
clear()inpostprocess_ready_block)