Fix sequence number race when allowing writes during external SST ingestion

[gengliqi]

Ref tikv/tikv#19891 (comment)

When allow_write=true, external SST ingestion may assign sequence numbers
concurrently with foreground writes. This can cause duplicate sequence numbers
or make the global sequence-number state move backwards.

This change briefly blocks writers to reserve enough sequence numbers before
running the ingestion jobs. Writers are resumed immediately after the
reservation, so SST processing and MANIFEST updates remain outside the
write-blocking period.

The existing behavior for allow_write=false is unchanged.

This also fixes an enable_multi_batch_write edge case where writers that fail
to write WAL still publish their allocated sequence numbers but were not counted
as pending. They are now included in pending-write accounting, preventing
ingestion from reserving sequence numbers before their sequence publication
completes.

Summary by CodeRabbit

• Bug Fixes

  • Improved external SST file ingestion while writes occur concurrently.
  • Ensured ingestion waits for pending writes before applying imported data.
  • Preserved consistent sequence numbering and prevented conflicts with new writes.
  • Maintained snapshot isolation so earlier snapshots do not expose newly ingested files.

• Documentation

  • Clarified concurrent-write requirements and snapshot behavior for external file ingestion.

• Tests

  • Added coverage for concurrent writes and multi-file ingestion.
  • Verified write ordering, final values, sequence assignments, and snapshot visibility.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

External SST ingestion now reserves sequence numbers before running ingestion jobs when foreground writes are allowed. Jobs receive the reserved sequence boundary. Tests cover concurrent writes, pending writers, sequence ordering, reads, and snapshot visibility.

Changes

External SST sequence reservation

Layer / File(s)	Summary
Ingestion sequence contract db/external_sst_file_ingestion_job.h, db/external_sst_file_ingestion_job.cc, include/rocksdb/options.h	ExternalSstFileIngestionJob::Run now accepts last_seqno. The allow_write documentation defines concurrent-write and snapshot behavior.
Write reservation and manifest updates db/db_impl/db_impl.cc	When writes are allowed, DBImpl stops both write queues, drains pending writes, reserves a shared sequence range, and passes the boundary to ingestion jobs. Manifest application conditionally updates sequence counters.
Concurrent ingestion validation db/external_sst_file_test.cc	Tests cover multi-file ingestion during concurrent writes and ingestion waiting for a pending writer. They verify sequence ordering, reads, and snapshot visibility.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant DBImpl
  participant WriteQueues
  participant VersionSet
  participant ExternalSstFileIngestionJob
  DBImpl->>WriteQueues: Block both write queues
  DBImpl->>WriteQueues: Wait for pending writes
  DBImpl->>VersionSet: Reserve sequence numbers
  DBImpl->>ExternalSstFileIngestionJob: Run(last_seqno)
  ExternalSstFileIngestionJob-->>DBImpl: Return consumed sequence count
  DBImpl->>VersionSet: Apply manifest and conditionally update counters

Loading

Suggested reviewers: overvenus, connor1996, lykxsassinator, hhwyt

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% 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
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: fixing sequence number races when writes are allowed during external SST ingestion.

✨ 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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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 `@db/db_impl/db_impl.cc`:
- Around line 5951-5964: The ingestion reservation currently publishes reserved
sequence numbers before the MANIFEST write completes. In the IngestExternalFiles
flow, keep SetLastPublishedSequence unchanged during reservation and advance it
only after LogAndApply succeeds, while preserving SetLastAllocatedSequence and
SetLastSequence updates and ensuring failure does not publish the reserved
range.

🪄 Autofix

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 Plus

Run ID: 42a6d5ce-d7f7-42c8-b402-ca1b2764d0e1

📥 Commits

Reviewing files that changed from the base of the PR and between 044e8b9 and e3426d3.

📒 Files selected for processing (4)

• db/db_impl/db_impl.cc
• db/external_sst_file_ingestion_job.cc
• db/external_sst_file_ingestion_job.h
• db/external_sst_file_test.cc

[Copilot]

Pull request overview

This PR addresses a race in external SST ingestion when allow_write=true, where ingestion and foreground writes could concurrently assign sequence numbers, potentially causing duplicates or regressions in global seqno state. It introduces a short write-stall to reserve a safe seqno range before running ingestion jobs, and updates ingestion job interfaces/tests accordingly.

Changes:

• Briefly blocks writers to reserve enough sequence numbers prior to ingestion when allow_write=true.
• Refactors ExternalSstFileIngestionJob::Run to accept a last_seqno value from the caller instead of reading it internally.
• Expands/adjusts tests to validate assigned seqnos and writer interaction during allow-write ingestion.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
db/external_sst_file_test.cc	Updates/extends tests for allow-write ingestion sequencing and writer interactions.
db/external_sst_file_ingestion_job.h	Changes ingestion job API to pass last_seqno into Run().
db/external_sst_file_ingestion_job.cc	Uses passed-in last_seqno and exposes assigned seqno via sync point callback.
db/db_impl/db_impl.cc	Adds seqno reservation logic under a brief write stop and passes last_seqno into ingestion jobs.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[gengliqi]

/retest

[Connor1996]

The description is not clear. How about snapshot consistency is not promised, because xxxx.

[gengliqi]

Added.

[Connor1996]

rest LGTM

[Connor1996]

add assert for ConsumedSequenceNumbersCount vs reserved_last_seqno

[gengliqi]

Added this assert after ExternalSstFileIngestionJob::Run.

[hhwyt]

Deep Review

#435 — Fix sequence number race when allowing writes during external SST ingestion
Author: gengliqi · Base: 8.10.tikv · Reviewed at local squashed commit 81c2da9fd (was 3d9a6b9ce, 4 commits, byte-identical tree, not pushed)
Ref: tikv/tikv#19891

Problem Summary

PR #18096 added allow_write to IngestExternalFileOptions so that SST ingestion no longer stops writes. The cost is that ingestion now runs concurrently with foreground writes, while the code that assigns sequence numbers still assumes it is the only writer. Under allow_write = true this produces two separate defects.

Defect 1 — duplicate sequence numbers (silent). ExternalSstFileIngestionJob::Run() read versions_->LastSequence() as its numbering base. The comment above that read said "It is safe to use this instead of LastAllocatedSequence since we are the only active writer". That is false under allow_write. LastSequence() is the published watermark, and with multi-batch write the allocation watermark (WriteThread::last_sequence_, a shadow cursor) can already be ahead. Ingestion then hands a file a sequence number that a concurrent foreground write also uses. Two different pieces of data end up on the same sequence number, and nothing reports it.

Defect 2 — sequence number regression (leads to an upper-layer panic). After LogAndApply, IngestExternalFiles published the sequence with a read-modify-write: read LastSequence(), add consumed_seqno_count, store back. A foreground write that lands between the read and the store gets overwritten with the older value, so the DB-global sequence moves backwards. Once it does, later snapshots stop seeing already-committed writes. MVCC then shows a committed Write and a live Lock on the same key, CheckTxnStatus pushes min_commit_ts and writes the lock back, Raft replicates that contradictory state, and minutes later rollback_lock panics on every replica in turn — a crash loop (tikv/tikv#19891, severity/critical, affects 7.5 / 8.1 / 8.5).

Defect 2 has a second path that an atomic RMW alone cannot fix. Multi-batch write publishes with commit_sequence->store(commit_lsn, release) (db/write_thread.cc:887), an unconditional store rather than a max. Any in-flight writer holding a smaller commit_lsn wipes out whatever ingestion raised the sequence to. That is why the fix cannot live on the ingestion side alone: ingestion must read the sequence at a moment when no writer holds an unpublished number.

The current mitigation on the TiKV side is to hard-code ENABLE_INGEST_ALLOW_WRITE to false. This PR fixes the underlying race so allow_write can be turned back on.

Solution Walkthrough

The idea is not to remove the write stall but to shrink it. The old stall covered the memtable flush and the MANIFEST write plus fsync in LogAndApply. The new one covers only the sequence reservation: a few atomic stores plus one wait for in-flight writes to land in the memtable.

① The reservation window — db/db_impl/db_impl.cc:5931-5968

if (allow_write) {
  // Briefly stop writes while reserving sequence numbers for ingestion.
  write_thread_.EnterUnbatched(&w, &mutex_);
  if (two_write_queues_) {
    nonmem_write_thread_.EnterUnbatched(&nonmem_w, &mutex_);
  }
  WaitForPendingWrites();
}

Both steps are required, and the second one is the load-bearing one.

• EnterUnbatched (:5933) blocks new allocations.
• WaitForPendingWrites() (:5937) drains in-flight publications. For multi-batch write that means waiting for pending_memtable_writes_ to reach zero (db/db_impl/db_impl.h:2050-2053). MultiBatchWriteCommit publishes the sequence first and decrements the counter second (db/db_impl/db_impl_write.cc:182-183), so a zero count means every allocated sequence has been published.

The first step alone is not enough. Multi-batch write releases the write thread at ExitAsBatchGroupLeader (db_impl_write.cc:376), but the sequence is not published until MultiBatchWriteCommit (:394). In between, versions_->LastSequence() lags the real allocation watermark. The wait also prevents a second problem: raising last_sequence_ past in-flight writes would make those writes temporarily invisible. A snapshot taken at that moment would miss them and then see them once they land.

The code reuses w and nonmem_w, declared at :5854-5855 for the !allow_write branch. Enter and exit are paired per branch and the branches are mutually exclusive:

branch	enter	exit
!allow_write	:5858-5861	:6072-6077
allow_write	:5933-5936	:5960-5963

Both exits release nonmem_write_thread_ before write_thread_, mirroring the enter order. No double Enter, no double Exit.

② Taking the base — db_impl.cc:5940

SequenceNumber last_seqno = versions_->LastSequence();

This line sits outside the if (allow_write) guard, so both paths share it. For !allow_write it hoists a read that used to live inside Run(). This is equivalent: writes are stopped, Run() itself does not advance LastSequence (it only records into consumed_seqno_count_), and RegisterRange() does not either. Every job used to read the same value anyway.

③ Computing the reservation — db_impl.cc:5942-5955

// Each file consumes at most one sequence number. Jobs for different
// column families share the same sequence range, so reserve the maximum
// file count. Unused sequence numbers are harmless gaps.
SequenceNumber reserved_seqno_count = 0;
for (size_t i = 0; i != num_cfs; ++i) {
  reserved_seqno_count =
      std::max(reserved_seqno_count,
               static_cast<SequenceNumber>(
                   ingestion_jobs[i].files_to_ingest().size()));
}
assert(reserved_seqno_count > 0);

It takes the max, not the sum, and that is correct. It is not a conservative estimate — it is what the semantics require. All jobs share one last_seqno base, so the first file of CF A and the first file of CF B get the same sequence number. That is exactly what atomic multi-CF ingestion means. The old code computed consumed_seqno_count as a max too (:6022-6028), so the behavior matches.

The upper bound holds. In Run(), each file consumes at most one sequence number (assigned_seqno = last_seqno + 1, then last_seqno = assigned_seqno; ++consumed_seqno_count_, guarded by assert(assigned_seqno == last_seqno + 1) at external_sst_file_ingestion_job.cc:440-443), and only when force_global_seqno is set or the file overlaps existing data.

assert(reserved_seqno_count > 0) is safe. IngestExternalFiles rejects empty external_files at :5761, and Prepare() rejects an empty files_to_ingest_ at db/external_sst_file_ingestion_job.cc:68-69. Both checks run before this point.

④ Publishing the reservation — db_impl.cc:5954-5958

const SequenceNumber reserved_last_seqno = last_seqno + reserved_seqno_count;
versions_->SetLastAllocatedSequence(reserved_last_seqno);
versions_->SetLastPublishedSequence(reserved_last_seqno);
versions_->SetLastSequence(reserved_last_seqno);

The order is correct and it has to be this order. SetLastSequence carries assert(!db_options_->two_write_queues || s <= last_allocated_sequence_) (db/version_set.h:1348), so the allocated value must be set first. This matches the old code at :6031-6035. See F3 for a pre-existing hazard in the same three lines.

ExitUnbatched follows immediately (:5960-5963), which ends the window. Run(), LogAndApply, and InstallSuperVersion all run concurrently with foreground writes from here on.

⑤ Passing the base into Run — db/external_sst_file_ingestion_job.cc:375, .h:135

Status ExternalSstFileIngestionJob::Run(SequenceNumber last_seqno) {

The stale read inside the function is gone, and so is the comment above it. Removing that comment is right — "we are the only active writer" was the source of the defect. The // REQUIRES: line above the function was removed as well (see F4).

⑥ The old publication moves under !allow_write — db_impl.cc:6013-6036

The whole block, including the comment explaining why publication belongs after LogAndApply, moves verbatim into if (!allow_write). The allow_write path no longer publishes a second time. The sequence is published exactly once, in ④.

⑦ New comment above LogAndApply — db_impl.cc:6006-6009

// With allow_write, a concurrent flush may persist a higher last sequence
// before this ingestion edit is applied. LogAndApplyHelper raises this edit's
// last sequence as needed to keep VersionEdit::last_sequence values
// non-decreasing in the MANIFEST.

This comment is accurate. VersionSet::LogAndApplyHelper (db/version_set.cc:5871) raises max_last_sequence monotonically at :5891-5894, starting from descriptor_last_sequence_ (:5211), with an assert at :5648. The MANIFEST sequence cannot go backwards because ingestion used an older value.

Findings (ordered by severity)

F1 [Critical] WaitForPendingWrites() has no test, and it is the line that fixes the defect for TiKV's default configuration
db/db_impl/db_impl.cc:5937 · db/external_sst_file_test.cc:2452, :2488

The shipped code is correct. What is critical is that the correctness is unprotected: delete WaitForPendingWrites() and every existing test still passes. Removing that line reintroduces tikv/tikv#19891, a severity/critical production crash loop.

AllowWriteIngestWaitsForPendingWriter (test.cc:2452) is the one discriminating regression test in the PR. Before the fix, ingestion does not wait for the writer, so assigned_seqno == last_seqno + 2 fails. But its first lines set enable_multi_batch_write = false (:2488).

The reason is structural, not carelessness. The test hangs the writer on DBImpl::WriteImpl:BeforeLeaderEnters at db/db_impl/db_impl_write.cc:667, while the multi-batch dispatch returns at :560-564. With multi-batch enabled, that sync point is unreachable.

The problem is that on the plain WriteImpl path, the leader publishes at SetLastSequence (:919) before releasing at ExitAsBatchGroupLeader (:922). There, EnterUnbatched alone is sufficient and WaitForPendingWrites() is a no-op. Only multi-batch leaves an "allocated but not published" window, between ExitAsBatchGroupLeader (:376) and MultiBatchWriteCommit (:394). That window is the entire reason the line exists. And TiKV runs with enable_multi_batch_write = true by default (!enable_pipelined_write && !enable_unordered_write, both false by default).

There is a ready-made sync point in the right place:

db_impl_write.cc:278  allocate   UpdateLastSequence(LastSequence()) + 1
db_impl_write.cc:376  ExitAsBatchGroupLeader     <- write thread released, LastSequence not moved yet
db_impl_write.cc:382  TEST_SYNC_POINT("DBImpl::WriteImpl:CommitAfterWriteWAL")   <-- park here
db_impl_write.cc:394  MultiBatchWriteCommit -> publish

Park a writer at :382. It has allocated its sequence, released the write thread, and not yet published — exactly the state WaitForPendingWrites() handles. Then run an allow_write ingestion and assert the file's sequence number is strictly greater than the parked writer's. Acceptance criterion: commenting out WaitForPendingWrites() must make the new test fail.

unordered_write is the second configuration in which WaitForPendingWrites() does real work (db/db_impl/db_impl.h:2042-2046 returns early only when both unordered_write and enable_multi_batch_write are false). It has its own path through WriteImplWALOnly with kDoPublishLastSeq (db_impl_write.cc:1354-1361). No test covers allow_write under unordered_write either. TiKV does not use it, so this is lower priority than the multi-batch gap, but it belongs in the same test plan.

F2 [High] The new snapshot-instability window is documented nowhere
db/db_impl/db_impl.cc:5956-5958 (new publication point) vs :6014-6021 (the old comment, now under !allow_write)

The comment that was preserved says:

It is necessary to update last seqno here since LogAndApply releases mutex when persisting MANIFEST file, and the snapshots taken during that period will not be stable if VersionSet last seqno is updated before LogAndApply.

The allow_write path now does exactly what that comment warns about. The sequence is published at :5958, but the files are not visible until LogAndApply installs the Version at :6010. A snapshot created in between has a sequence number that already covers the ingested files, yet reads return nothing. After LogAndApply, that same snapshot does see them. This breaks snapshot immutability. It is a library-level behavior change, not just an internal detail.

For TiKV's add-peer path this is acceptable. The range is owned exclusively by the peer applying the snapshot and serves no reads during the window. But:

• IngestExternalFileOptions::allow_write (include/rocksdb/options.h:2103-2105) says nothing about it.
• The new comment at :5965-5966 is about the reservation not being rollback-able, which is a different topic.
• The only related assertion in the tests checks that an older snapshot cannot see the ingested data (db/external_sst_file_test.cc:2441-2445). That assertion is correct, but it leaves the impression that snapshot semantics are unchanged.

Recommendation: state the precondition in the allow_write doc comment at options.h:2105 — the caller must guarantee that nobody reads the ingested range during ingestion — and add a comment above :5956 explaining why publishing before LogAndApply is acceptable here. This is the line between a known trade-off and an undiscovered bug, and it needs to be written down.

F3 [High] Under two_write_queues with seq_per_batch, the reservation can lower the allocation cursor (pre-existing)
db/db_impl/db_impl.cc:5956 · db/version_set.h:1360

SetLastAllocatedSequence(last_seqno + reserved_seqno_count) assumes last_allocated_sequence_ never leads LastSequence(). That assumption breaks for WritePrepared and WriteUnprepared transactions:

• With two_write_queues_ && disable_memtable, WAL-only Prepare batches go to WriteImplWALOnly on the nonmem thread with kDontPublishLastSeq (db_impl_write.cc:522-529).
• assign_order = seq_per_batch_ ? kDoAssignOrder : kDontAssignOrder on the same lines. With seq_per_batch_ set, seq_inc = total_batch_cnt (:1279-1288), so FetchAddLastAllocatedSequence really advances the allocation cursor.
• kDontPublishLastSeq means last_sequence_ is not advanced by that path (:1354-1357), and pending_memtable_writes_ is not incremented either (:1359-1361, conditional on unordered_write). So WaitForPendingWrites() does not drain this gap, and EnterUnbatched on the nonmem thread only stops new ones.
• Steady state under WritePrepared is therefore last_allocated_sequence_ > last_sequence_, by the number of prepared-but-uncommitted batches.

If that lead exceeds reserved_seqno_count, then reserved_last_seqno < last_allocated_sequence_. In debug builds the assert at version_set.h:1360 fires. In release builds the assert is compiled out and the allocation cursor moves backwards, so prepared data and future writes can be handed the same sequence numbers. The release behavior is worse than the crash.

Scope and priority:

• This is pre-existing. The !allow_write path at :6031-6035 does the same three calls with the same base, so the hazard predates this PR. This PR widens exposure slightly, because the !allow_write block is guarded by if (consumed_seqno_count > 0) while the new path always runs when allow_write is set.
• Not reachable for TiKV. TiKV enables neither two_write_queues nor seq_per_batch. Plain two_write_queues without seq_per_batch is also safe: assign_order is then kDontAssignOrder, seq_inc stays 0, and the allocation cursor does not move.

If this PR is intended for upstream rather than only for TiKV, use std::max(versions_->LastAllocatedSequence(), last_seqno + reserved_seqno_count), or take the base from LastAllocatedSequence() in the first place. At minimum, state the assumption in a comment so the next reader does not have to rediscover it.

F4 [Medium] Run() lost its calling contract, and the contract just became harder
db/external_sst_file_ingestion_job.cc:375 · db/external_sst_file_ingestion_job.h:135

Removed:

// REQUIRES: we have become the only writer by entering both write_thread_ and
// nonmem_write_thread_

The header at .h:134-135 now says only // REQUIRES: Mutex held.

The contract did not disappear. It forked:

• !allow_write: the caller must still be the only writer.
• allow_write: the caller must have reserved at least files_to_ingest_.size() sequence numbers, and last_seqno must be the start of that reservation, not the current LastSequence().

The new parameter SequenceNumber last_seqno does not explain itself. From the signature alone you cannot tell whether to pass the current value or a reserved base. Passing the wrong one compiles cleanly, warns about nothing, and silently reintroduces duplicate sequence numbers. Add two lines on the declaration describing what last_seqno means and what each path requires.

F5 [Medium] Nothing asserts that consumption stays within the reservation
db/db_impl/db_impl.cc:5943 (the premise, stated in a comment) · :5972 (the Run call)

"Each file consumes at most one sequence number" is the foundation of the whole reservation calculation, and right now it exists only as a comment. consumed_seqno_count_ is still maintained inside Run() (external_sst_file_ingestion_job.cc:440-444), and the allow_write path no longer reads it. One line turns the premise into a runtime invariant:

assert(ingestion_jobs[i].ConsumedSequenceNumbersCount() <=
       static_cast<int>(reserved_seqno_count));

(reserved_seqno_count needs to be hoisted out of the if (allow_write) block, or kept debug-only.)

If AssignLevelAndSeqnoForIngestedFile ever starts consuming more than one sequence per file, today's code silently returns to handing out duplicate sequence numbers — the exact defect this PR fixes.

F6 [Medium] The multi-CF path has no allow_write coverage at all
db/db_impl/db_impl.cc:5946-5952

Taking the max instead of the sum is the decision in this PR that most needs explaining, and it is correct and consistent with the old code. But both allow_write tests go through the single-CF db_->IngestExternalFile(...). The multi-CF IngestExternalFiles combined with allow_write is not exercised anywhere.

Worth adding: two column families with unequal file counts (say 1 and 3), asserting that the first file of each CF gets the same sequence number and that the reservation is 3.

F7 [Low] No test for the failure path
db/db_impl/db_impl.cc:5965-5966

// The reservation cannot be rolled back if ingestion fails because a
// foreground write may have already consumed a later sequence number.

The conclusion is right and the consequence is harmless. An unpersisted gap disappears on restart, since LastSequence is recovered from the WAL and the MANIFEST, and a completed ingestion is covered by VersionEdit::last_sequence.

Partial failure across multiple column families is also safe, and I traced it rather than assuming: Run() returns non-OK only through early returns at external_sst_file_ingestion_job.cc:384, :387, :426, :446, and :451, all of which precede CreateEquivalentFileIngestingCompactions() at :488. A failed job therefore leaves file_ingesting_compactions_ empty, so UnregisterRange() (db_impl.cc:6040-6042) iterates an empty vector. Jobs the loop never reached are empty for the same reason, and RegisterRange() (:5976) only runs after a successful Run(). No double-unregister and no dangling compaction pointers.

What is missing is a test. Injecting one Run() or LogAndApply failure and then continuing to read and write is cheap, and it would pin down both the single-CF and the job-2-of-3 case.

F8 [Low] WriteDuringIngest is a descriptive test, not a regression test
db/external_sst_file_test.cc:2394-2451

It depends on DBImpl::IngestExternalFiles:AfterReserveSeqno (db_impl.cc:5967), a sync point this PR adds. It cannot run against the pre-fix code, so it cannot show that the old behavior was broken. It also injects its concurrent write after ExitUnbatched, so it covers "a write after the reservation" rather than "a write already in flight during the reservation", which is the actual defect shape (F1 covers that half).

This is not a defect, it is a division of labor. The test's value is pinning down the new behavior (assigned_seqnos == {last+1, last+2}, GetLatestSequenceNumber() == last+3). It should just not be presented as regression evidence in the PR description.

F9 [Nit] The allow_write condition is tested five times in one function
db_impl.cc:5856 / :5931 / :5942 / :6013 / :6072

IngestExternalFiles was already very long, and the same condition is now tested in five places — if (allow_write) at :5931 and :5942, if (!allow_write) at :5856, :6013, and :6072. :5931 and :5942 are separated by a single line that reads last_seqno. Merging :5931-5968 into one if (allow_write) { ... } — either by duplicating the last_seqno read or by declaring it earlier and assigning in both branches — would make the boundary of the reservation window visible at a glance. Readability only; correctness is unaffected.

Costs and Negative Impacts

• Correctness: The fix addresses both defects. Duplicate numbering is solved by numbering from the reserved range (⑤). Regression is solved by taking the base when no writer holds an unpublished sequence, and publishing exactly once (①④⑥). I checked Enter/Exit pairing, the shared base across jobs, the reachability of assert(reserved_seqno_count > 0), the ordering of the three Set* calls against their asserts, the equivalence of the !allow_write path, and multi-CF partial failure. All hold. The remaining correctness risk sits in F2 (undocumented semantic change), F3 (pre-existing allocation-cursor hazard, not reachable for TiKV), and F5 (unasserted premise).

• Security: No impact. No input parsing, permissions, or crypto involved.

• Compatibility:

  • Behavior change for library users: allow_write = true now gives up snapshot atomicity for ingestion (F2). This is not a wire or on-disk format change, but it is a semantic change, and the option's documentation does not mention it.
  • API: ExternalSstFileIngestionJob::Run changed signature, but the class lives in a private header under db/. Not public API.
  • On-disk / MANIFEST: unchanged. LogAndApplyHelper's monotonic raise keeps last_sequence in the MANIFEST non-decreasing.
  • !allow_write path: behavior identical. Only the read point of last_seqno moved, and that is equivalent under stopped writes.

• Robustness:

  • A failed ingestion leaves a sequence gap. Harmless; the recovery path and the multi-CF partial-failure path both check out (F7).
  • One side effect worth recording: after a failed ingestion, LastSequence has advanced with no matching data, so it will be lower after a restart. TiKV's snapshot-sequence tripwire in engine_rocks: detect snapshot sequence regression tikv#19927 is scoped per live DB instance and rebuilds on reopen, so it will not misfire. The two changes line up.
  • The #ifndef NDEBUG NeedsFlush recheck inside Run() (external_sst_file_ingestion_job.cc:378-388) now runs after the write thread is released. That was already true before this PR, since allow_write never entered EnterUnbatched. Not a new risk, but it still rests on the external guarantee that the ingested range does not overlap concurrent writes. If that guarantee is ever violated, a debug build turns it into a Status::TryAgain from Run() rather than silent corruption — useful, but only in debug builds.

• Cognitive Load: Moderate to high. The write stall moved from one contiguous section to a small block in the middle of the function. A reader has to understand what EnterUnbatched blocks, what WaitForPendingWrites drains, and why neither alone is sufficient. Right now that line carries only "Briefly stop writes while reserving sequence numbers", which does not say what it adds over EnterUnbatched. Combined with F1 (no test protects it) and F9 (the condition is scattered), this is the line most likely to be optimized away later. Add a WHY comment at :5937.

• CPU: Against allow_write = false, the new window is a strict subset of the old one. The memtable flush, Run(), and LogAndApply (including the MANIFEST fsync) all move outside it, so this is strictly better. Compared to the current defective allow_write = true behavior, this is a regression: zero stall becomes a short stall. Inside the window, ingestion waits for in-flight writers to finish inserting their batches into the memtable. That wait contains no I/O — the WAL is written before ExitAsBatchGroupLeader — so it is tens to hundreds of microseconds, bounded by the largest in-flight write batch. The apply-wait P99 numbers from #18096 should not be reused; re-measure, and emit WaitForPendingWrites duration as its own histogram. It is the only cost item with real variance.

• Memory: No additional allocation.

• Log Volume: No new logging.

Engineering Rules Check

This PR lives in tikv/rocksdb (C++), so the Engineering Rules in cloud-storage-engine/AGENTS.md do not apply directly. Checked against the portable parts:

• Comments explain WHY: mostly met. :5943-5945 and :6006-6009 both explain reasoning rather than restating code, and both are accurate. The gap is :5937 — the single line that most needs a WHY has no comment (see Cognitive Load, F1).
• No stale comments: met, and worth calling out. The PR deletes the "we are the only active writer" comment inside Run(), which was the origin of the defect.
• Contracts are written down: not met, see F4.
• Minimal change, no drive-by refactors: met. The !allow_write path is preserved verbatim.
• Naming: reserved_seqno_count and reserved_last_seqno are clear. Run(SequenceNumber last_seqno) is not self-explanatory (see F4); base_seqno or reserved_start_seqno would read better.
• Deliberate asserts state their invariant: assert(reserved_seqno_count > 0) (:5953) has no comment. It holds because of the checks at :5761 and in Prepare(). One line would say so.

Questions and Assumptions

• Assumption (confirmed with the requester): TiKV's caller guarantees that the ingested range does not overlap concurrent writes, and that nobody reads that range during ingestion. F2's trade-off rests entirely on the second half. This report does not re-verify the precondition, but it does insist that it be written into the allow_write documentation.
• Not verified dynamically: F3 is derived from reading the write paths, not from running a WritePrepared workload with ingestion. The code path is clear, but nobody has observed the assert fire. Anyone taking this upstream should reproduce it before or alongside the fix.
• No local build and no test run (per the no-local-build convention). All conclusions come from reading the source. Line numbers refer to the local squashed commit 81c2da9fd.
• Prepare() timing: assumed files_to_ingest_ is fully populated and stable by the time :5951 reads it. Confirmed by code path, not by dynamic testing.

Suggested Tests / Validation

In order of value:

1. In-flight writer under multi-batch write (required, addresses F1). Park a writer at DBImpl::WriteImpl:CommitAfterWriteWAL (db_impl_write.cc:382), where it has allocated a sequence, released the write thread, and not yet published. Set enable_multi_batch_write = true, run an allow_write ingestion, and assert the file's sequence number is strictly greater than the writer's. Acceptance criterion: commenting out WaitForPendingWrites() must make this test fail.

2. Multi-CF with allow_write (addresses F6). Two column families with 1 and 3 files. Assert that the first file of each CF gets the same sequence number, that the reservation is 3, and that GetLatestSequenceNumber() advances by 3.

3. assert(consumed <= reserved) (addresses F5). Less a test than turning the premise into a runtime invariant. Costs one line.

4. Failure injection (addresses F7). Fail Run() after the reservation — for a single CF and for job 2 of 3 — then assert the DB still reads and writes normally and a later ingestion succeeds.

5. Pin the snapshot window explicitly (addresses F2, optional). Take a snapshot inside the AfterReserveSeqno callback, read through it after ingestion returns, and assert the behavior that actually happens. The point is to leave a record of the semantic change in the code rather than letting the next person rediscover it. If this test is skipped, the documentation change in F2 becomes mandatory.

6. unordered_write with allow_write (addresses the second half of F1). The other configuration where WaitForPendingWrites() does real work. Lower priority than item 1 because TiKV does not use it.

7. Stress validation. Run gengliqi's reproduction from txn: panic "txn record found but not expected" when a committed write record and a lock coexist on the same key at the same start_ts tikv#19891 (a 10 ms sleep injected after LastSequence() to widen the window) over a long workload and confirm no regression appears. Note that the snapshot-sequence tripwire in engine_rocks: detect snapshot sequence regression tikv#19927 only catches regressions, not duplicates, so items 1 and 3 are still needed to cover both defects.

8. Performance regression. Re-measure the apply-wait P99 from #18096, and emit WaitForPendingWrites duration as its own histogram. It is the only cost item with real variance, and its tail depends on the largest in-flight write batch.

[Connor1996]

rest LGTM

[Connor1996]

It would have some issues with two_write_queues

How about narrowing down the scope

if (allow_write && two_write_queues_) {
  return Status::NotSupported(
      "allow_write is incompatible with two_write_queues");
}

[Connor1996]

Oh, it also has the issues for !allow_write. Forget about it.

[ti-chi-bot]

[LGTM Timeline notifier]

Timeline:

• 2026-08-10 09:00:27.542513234 +0000 UTC m=+3037013.578608291: ☑️ agreed by hhwyt.
• 2026-08-10 23:37:14.961782489 +0000 UTC m=+3089620.997877545: ☑️ agreed by Connor1996.

[gengliqi]

/cherry-pick 6.29.tikv

[ti-chi-bot]

@gengliqi: new pull request created to branch 6.29.tikv: #437.
But this PR has conflicts, please resolve them!

Details

In response to this:

/cherry-pick 6.29.tikv

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Connor1996, hhwyt, overvenus

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [Connor1996,overvenus]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment