修复事务队列，正常插入到队尾 by yy782 · Pull Request #2 · yy782/ImDragonfly

yy782 · 2026-07-25T08:24:53Z

No description provided.

qodo-code-review · 2026-07-25T08:26:05Z

PR Summary by Qodo

Fix TxQueue ordering/free-list growth and unify GTest/CTest unit_tests target

🐞 Bug fix 🧪 Tests ⚙️ Configuration changes 🕐 40+ Minutes

AI Description

• Fix TxQueue free-list growth and ensure transactions append in txid order.
• Update transaction scheduling to use new TxQueue Push/Pop APIs.
• Consolidate tests into unit_tests with CTest runner and add TxQueue concurrency tests.

Diagram

graph TD
  TX["Transaction"] --> ES["EngineShard"] --> Q["TxQueue"] --> FL["Free list"]
  CTEST("CTest runner") --> UT("unit_tests") --> TX
  UT --> Q

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep O(1) size_ counter (fix correctness only)

➕ Avoids O(n) traversal in TxQueue::Size()
➕ Keeps existing performance expectations if Size() is used in hot paths
➖ Requires careful updates for Pop(Iterator) and edge cases to avoid drift
➖ More state to maintain during future queue logic changes

2. Use an intrusive linked list + separate node pool

➕ Clearer separation between list ordering and allocation policy
➕ Iterator stability can be made explicit
➖ More refactor surface area across transaction scheduling
➖ Harder to keep contiguous storage/cache locality vs vector pool

3. Replace with std::list / std::deque

➕ Simplifies free-list and Grow() logic
➕ Standard container semantics reduce custom allocator bugs
➖ Higher allocation overhead; worse locality
➖ Harder to integrate with pmr allocator strategy consistently

Recommendation: The PR’s approach (vector-backed node pool + explicit free list + txid-ordered insertion) is reasonable for a high-throughput queue and directly fixes the Grow/free-list bug and the LIFO-like insertion behavior. However, re-computing Size() by traversal is a potential performance footgun if Size() is called frequently outside tests—consider restoring a maintained size_ counter (with thorough tests around Pop(Iterator)) if Size() is needed in production paths.

Files changed (12) +499 / -278

Enhancement (1) +22 / -32

tx_queue.hppTxQueue API change: Insert/Remove -> Push/Pop + pmr vector +22/-32

TxQueue API change: Insert/Remove -> Push/Pop + pmr vector

• Updates the public interface to Push/Pop and changes empty semantics to rely on head_ == kEnd. Switches the backing storage to a PMR polymorphic allocator vector and removes explicit used/size_ bookkeeping from Node/state (Size becomes computed).

src/detail/tx_queue.hpp

Bug fix (3) +100 / -73

tx_queue.cppRewrite TxQueue insertion/removal and fix Grow free-list head +79/-57

Rewrite TxQueue insertion/removal and fix Grow free-list head

• Fixes the Grow() path to correctly initialize and publish the free list head (free_head_). Replaces the old Insert/Remove API with Push/Pop, inserting nodes in txid order (typically FIFO for monotonic txid) and returning nodes to the free list on Pop(). Adds nullptr-safe Front/Back and makes Size() computed by traversal.

src/detail/tx_queue.cpp

engine_shard.cppPollExecution uses nullptr-safe Front loop +8/-7

PollExecution uses nullptr-safe Front loop

• Updates the shard execution loop to repeatedly fetch txq_.Front() until it returns nullptr, matching the new TxQueue Front() contract. Preserves the early break when a transaction cannot conclude.

src/sharding/engine_shard.cpp

transaction.cppSchedule queue operations updated to Push/Pop +13/-9

Schedule queue operations updated to Push/Pop

• Replaces TxQueue Insert/Remove calls with Push/Pop in scheduling and multi-shard unlock callbacks. Under UNIT_TESTS, marks transactions as having a result when CollectedResult is invoked to support test synchronization.

src/transaction_layer/transaction.cpp

Tests (4) +353 / -131

transaction.hppAdd UNIT_TESTS hooks for txid/result observation +25/-7

Add UNIT_TESTS hooks for txid/result observation
• Adds UNIT_TESTS-only helpers to set txid, check completion, and fetch the collected result without blocking. Introduces a UNIT_TESTS-only HasRes_ flag used by tests to detect completion.
src/transaction_layer/transaction.hpp

db_table_perf.cppConvert db_table_perf into a GTest benchmark test +25/-116

Convert db_table_perf into a GTest benchmark test

• Wraps the performance benchmark into a TEST() so it runs under the unified unit_tests binary. Removes Redis dict comparison code and keeps DashTable vs std::unordered_map measurements.

test/db_table_perf.cpp

db_table_test.cppRely on gtest_main; remove custom main and namespace wrapper +1/-8

Rely on gtest_main; remove custom main and namespace wrapper
• Removes the custom main() and switches to using gtest_main provided by the unified test target. Simplifies namespace usage to match the consolidated build.
test/db_table_test.cpp

tx_queue_test.cppAdd TxQueue unit tests including multi-shard concurrency scenario +302/-0

Add TxQueue unit tests including multi-shard concurrency scenario

• Adds coverage for empty queue behavior, FIFO ordering, iterator-based removal from head/middle/tail, and push/pop alternation. Includes a heavier MultiConcurrent test that executes concurrent MSET operations across shards and verifies results via MGET, relying on UNIT_TESTS transaction completion hooks.

test/tx_queue_test.cpp

Other (4) +24 / -42

CMakeLists.txtStop building legacy redis test subdirectory +0/-1

Stop building legacy redis test subdirectory

• Removes inclusion of test/redis from the top-level test build. This aligns the repository with the new unified unit_tests target and eliminates building vendored Redis dict sources.

CMakeLists.txt

build.shDefault build targets now include unit_tests +3/-3

Default build targets now include unit_tests

• Updates help text and default TARGETS from the previous per-test binary list to building unit_tests. This makes the standard build flow compile the consolidated test executable by default.

scripts/build.sh

run_unit_tests.shRun tests via CTest from a build directory +9/-9

Run tests via CTest from a build directory

• Switches from executing a single test binary path to invoking ctest against a build directory. Improves compatibility with gtest_discover_tests and provides uniform output-on-failure behavior.

scripts/run_unit_tests.sh

CMakeLists.txtConsolidate all tests into unit_tests (GTest + CTest) +12/-29

Consolidate all tests into unit_tests (GTest + CTest)

• Replaces multiple per-file executables with a single unit_tests target including db_table_test, tx_queue_test, and db_table_perf. Links gtest_main, defines UNIT_TESTS for test-only hooks, and registers tests via gtest_discover_tests.

test/CMakeLists.txt

qodo-code-review · 2026-07-25T08:30:54Z

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. ~~Shard modulo off-by-one~~ ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New

Description

Shard() 使用 hash % (shard_set_size - 1) 会导致最后一个 shard 永远不会被选择，并且当 shard_set_size==1 时出现除零未定义行为。依赖
Shard(key, shard_set->size()) 的路由（如 watch key 注册）会因此把负载和数据路由到错误的 shard 集合上。

Code

src/detail/common.hpp[42]
+  return hash % (shard_set_size - 1);

Evidence

Shard() 的返回值被上游直接当成 shard_id 使用（传入 shard_set->size()），因此结果必须落在 [0, size)；现在用 size-1 会丢掉最后一个 shard，且
size==1 会除零。

src/detail/common.hpp[17-43]
src/detail/conn_context.hpp[92-99]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`dfly::Shard()` currently returns `hash % (shard_set_size - 1)`, which (1) excludes the last shard (range becomes `[0, N-2]`), and (2) is UB when `shard_set_size == 1` due to division by zero.

### Issue Context
Call sites pass `shard_set->size()` and expect a shard id in `[0, size)`.

### Fix Focus Areas
- src/detail/common.hpp[17-43]

### Suggested fix
- Change to `hash % shard_set_size` (after validating `shard_set_size > 0`).
- If you need the bitmask fast-path, keep `hash & (shard_set_size - 1)` but **only** when `shard_set_size` is a power of two; otherwise fall back to modulo by `shard_set_size`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. ~~TryAdd task loss hang~~ ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New

Description

EngineShardSet::Add 改为调用队列的 TryAdd 并只用 assert 检查成功；在 NDEBUG 下 TryAdd 失败会静默返回 false，而调用方（如
Transaction::IterateActiveShards）通常忽略返回值并继续等待计数器。若发生一次入队失败，对应 shard 的回调不会执行，协程可能永久等待导致事务卡死。

Code

src/sharding/engine_shard_set.hpp[R25-27]

+    bool success = shards_[sid]->GetQueue()->TryAdd(std::forward<F>(f));
+    assert(success);
+    return success;

Evidence
EngineShardSet::Add 现在可能失败（TryAdd），但 IterateActiveShards 不检查返回值且会等待 counter；只要有一次任务没入队，counter 不会被
Dec，Wait 将无法结束。
src/sharding/engine_shard_set.hpp[22-28]
src/transaction_layer/transaction.hpp[220-245]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`EngineShardSet::Add` now uses non-blocking `TryAdd` and relies on `assert(success)`. With assertions disabled, enqueue failure can be silently ignored by callers, breaking completion invariants (e.g., counters that assume each shard task will run).

### Issue Context
`Transaction::IterateActiveShards` enqueues tasks to active shards and then waits on a counter; it does not check the boolean return from `shard_set->Add`.

### Fix Focus Areas
- src/sharding/engine_shard_set.hpp[22-28]
- src/transaction_layer/transaction.hpp[220-245]

### Suggested fix
- Prefer a guaranteed-enqueue API here (restore previous `AsyncAdd`/blocking enqueue), OR
- If `TryAdd` must be kept, implement a retry/backoff until success (or propagate failure and make **all** callers handle it before waiting).
- Ensure `IterateActiveShards` cannot enter `Wait()` unless all required shard tasks were actually enqueued.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. ~~Release asserts on empty~~ ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New

Description

DbSlice::Release 现在对 lock_args.fps 为空直接 assert，但 DbSlice::Acquire 对空 fps 明确返回 true。对某个 shard 没有
key 的事务 slice，ScheduleInShard 仍会把 KEYLOCK_ACQUIRED 置位并最终调用
UnlockMultiShardCb→Release，从而在断言开启的构建中直接崩溃。

Code

src/sharding/db_slice.cpp[R331-341]

+void DbSlice::Release(IntentLock::Mode mode, const KeyLockArgs& lock_args
+#ifdef UNIT_TESTS
+                      ,
+                      int id
+#endif
+) {
+
+  // #ifdef UNIT_TESTS
+  //   LOG(INFO)<< " 事务: " << id << " 释放锁 "<<" shard:" <<  shard_id_;
+  // #endif
+  assert(!lock_args.fps.empty());

Evidence
同一套 KeyLockArgs：Acquire 对空 fps 返回 true；Release 却 assert 非空。事务路径中 Acquire 成功会设置 KEYLOCK_ACQUIRED，并在
FinishHop 解锁时调用 Release，因此空 key slice 会触发该断言。
src/sharding/db_slice.cpp[303-311]
src/sharding/db_slice.cpp[331-345]
src/transaction_layer/transaction.cpp[260-279]
src/transaction_layer/transaction.cpp[320-327]
src/transaction_layer/transaction.cpp[359-375]
src/transaction_layer/transaction.cpp[340-347]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`DbSlice::Release` now asserts `!lock_args.fps.empty()`, but `DbSlice::Acquire` allows empty fps and returns `true`. This creates a contract mismatch and can abort in assertion-enabled builds when unlocking a slice with no keys.

### Issue Context
Transactions mark `KEYLOCK_ACQUIRED` when `LockMultiShardCb()` returns true; `Acquire()` returns true for empty fps, so the unlock path can be reached with an empty fps list.

### Fix Focus Areas
- src/sharding/db_slice.cpp[303-311]
- src/sharding/db_slice.cpp[331-345]

### Suggested fix
- Restore the previous empty-check in `Release`:
 - `if (lock_args.fps.empty()) return;`
- Optionally also avoid setting `KEYLOCK_ACQUIRED` for empty-fps slices so the system state reflects reality.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

View more (2)

4. ~~Rollback releases wrong lock~~ ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New

Description

DbSlice::Acquire 在加锁失败时回滚释放 fps[0..i]（包含第 i 个未成功 Acquire 的 fp），会对 LockTable 中已有的同 fp 锁执行
Release。由于 LockTable 不按事务区分所有权，这会把其他事务持有的锁错误地释放/降级，破坏隔离性。

Code

src/sharding/db_slice.cpp[R323-325]

+    for (int j = 0; j <= i; j++) {
+      lt.Release(lock_args.fps[j], mode);
    }

Evidence

Acquire 失败点的 fp 没有被当前事务成功 Acquire；但 LockTable::Release 只按 fp 找锁并对 IntentLock 做
Release，没有“属于哪个事务”的校验，因此会影响其他事务的锁状态。

src/sharding/db_slice.cpp[313-326]
src/sharding/db_table.hpp[29-45]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`DbSlice::Acquire` rollback loop currently releases fingerprints up to and including the failed index (`j <= i`). Since `LockTable::Release` is not transaction-owned, releasing `fps[i]` can decrement/release a lock held by a different transaction.

### Issue Context
`i` is the index where `lt.Acquire(fps[i], mode)` failed, so only `[0, i-1]` are guaranteed to have been acquired by the current transaction.

### Fix Focus Areas
- src/sharding/db_slice.cpp[313-326]

### Suggested fix
- Change rollback to release only successfully acquired fingerprints, e.g.:
 - `for (int j = 0; j < i; ++j) lt.Release(lock_args.fps[j], mode);`
 - or restore the previous reverse loop `for (; i > 0; --i) Release(fps[i-1], ...)`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

5. ~~Double-pop corrupts queue~~ ✓ Resolved 🐞 Bug ≡ Correctness

Description

TxQueue::Pop 在未验证迭代器仍属于队列的情况下直接摘链并加入空闲链表，重复 Pop 同一 Iterator 会破坏队列/空闲链表并导致崩溃或死循环。Transaction 在多处
Pop(pq_pos_[sid]) 后不清空 pq_pos_，且重试清理路径会再次 Pop 旧 iterator，使该损坏路径可达。

Code

src/detail/tx_queue.cpp[R65-84]

+void TxQueue::Pop(Iterator it) {
+  if (it == kEnd) {
    return;
  }
-
-  // 从链表中移除
  if (vec_[it].prev != kEnd) {
-    vec_[vec_[it].prev].next = vec_[it].next;
+      vec_[vec_[it].prev].next = vec_[it].next;
  } else {
-    head_ = vec_[it].next;  // 移除的是头节点
+      head_ = vec_[it].next;  
  }

  if (vec_[it].next != kEnd) {
-    vec_[vec_[it].next].prev = vec_[it].prev;
+      vec_[vec_[it].next].prev = vec_[it].prev;
  } else {
-    tail_ = vec_[it].prev;  // 移除的是尾节点
+      tail_ = vec_[it].prev;  
+  }
+  vec_[it].next = free_head_;
+  vec_[it].prev = kEnd;
+  vec_[it].trans = nullptr;
+  free_head_ = it;
+}

Evidence
TxQueue::Pop 仅判断 it == kEnd，随后无条件修改前后指针并把节点接回 free_head_；一旦对已释放节点再次 Pop，会把 free-list
节点当作队列节点处理并破坏结构。Transaction 多处调用 Pop(pq_pos_[sid])，但从未将 pq_pos_[sid] 置回 kEnd，在重试清理/FinishHop
等路径中可能重复 Pop 同一 iterator。
src/detail/tx_queue.cpp[65-84]
src/transaction_layer/transaction.cpp[176-215]
src/transaction_layer/transaction.cpp[240-270]
src/transaction_layer/transaction.cpp[291-319]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`TxQueue::Pop(Iterator)` currently assumes `it` is always a live node inside the active queue. If the same iterator is popped twice (or popped after it was already removed during a retry cleanup), it will treat a free-list node as a queue node and corrupt `head_`/`tail_` and the free list.

This is now reachable because `Transaction` keeps `pq_pos_[sid]` unchanged after removing the queue node, and the retry cleanup path calls `Pop(pq_pos_[sid])` again.

## Issue Context
- `TxQueue` no longer has a `used` flag / membership check.
- `Transaction::ScheduleInternal` and `Transaction::FinishHop` remove from the queue but never reset `pq_pos_[sid]` to `TxQueue::kEnd`.

## Fix Focus Areas
- src/detail/tx_queue.cpp[65-84]
- src/transaction_layer/transaction.cpp[176-215]
- src/transaction_layer/transaction.cpp[240-270]
- src/transaction_layer/transaction.cpp[291-319]

### What to change
1) Make `TxQueue::Pop` safe/idempotent:
  - Add an `in_use`/`used` boolean back into `Node`, or
  - Use a sentinel invariant (e.g., `trans==nullptr` means free) and check it before unlinking.
  - If node is already free/not in queue, `Pop` should return without mutating queue/free list.

2) Reset `pq_pos_[sid]` immediately after a successful pop in **all** paths:
  - After `e->txq()->Pop(pq_pos_[sid]);` set `pq_pos_[sid] = TxQueue::kEnd;`.
  - In the retry cleanup lambda, only pop when `pq_pos_[sid] != TxQueue::kEnd`, then reset it.

3) (Optional but recommended) Add debug assertions to detect double-pop early in debug builds.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

6. Port check false-negative 🐞 Bug ☼ Reliability ⭐ New

Description

多个脚本在 set -euo pipefail 下使用 ss -tlnp | grep 做端口占用检测，但当环境缺少 ss 或者 ss -p
因权限看不到进程信息时，检测会误判“端口空闲”。随后启动服务/测试会在 bind 或连接阶段以更难定位的方式失败（EADDRINUSE/连接到旧进程）。

Code

scripts/run_unit_tests.sh[R11-16]

+# ── 检查 6379 端口占用 ────────────────────────────────────
+if ss -tlnp | grep -q ":6379 "; then
+    echo "ERROR: Port 6379 is already in use."
+    echo "Please free port 6379 before running tests."
+    exit 1
+fi

Evidence
这些脚本都无条件调用 ss -tlnp 并基于其输出决定是否退出；当该命令不可用或输出不可见时，端口检测逻辑会失效并把错误推迟到后续步骤。
scripts/run_unit_tests.sh[9-16]
scripts/run_benchmark.sh[9-20]
scripts/run_integration_tests.sh[9-21]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Scripts depend on `ss -tlnp` for port-in-use detection. On systems without `ss` (e.g. some macOS/minimal containers) or where `-p` output is restricted, the check can become a false-negative and later steps fail with `EADDRINUSE` or connect to an unexpected process.

### Issue Context
The port check is used in benchmark/integration/unit-test scripts.

### Fix Focus Areas
- scripts/run_unit_tests.sh[11-16]
- scripts/run_benchmark.sh[15-20]
- scripts/run_integration_tests.sh[16-21]

### Suggested fix
- Guard with `command -v ss >/dev/null` and fall back to another method (`lsof -iTCP:$PORT -sTCP:LISTEN`, `netstat`, or a small Python bind test).
- Consider using `ss -tln` (no `-p`) to avoid permission-related visibility issues.
- If no checker is available, print a warning and skip the pre-check (do not silently assume the port is free).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

7. ~~Benchmark in unit_tests~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

db_table_perf.cpp 作为 GTest 用例被编进 unit_tests 并通过 gtest_discover_tests 注册到 CTest，运行包含 10k/100k/1M 元素的
insert/find/erase 基准循环。默认执行会显著拖慢甚至超时 CI 的单测阶段。

Code

test/CMakeLists.txt[R3-8]

+# 单元测试可执行文件（统一入口，包含所有 *_test.cpp 和 *_perf.cpp）
+add_executable(unit_tests
+    db_table_test.cpp
+    tx_queue_test.cpp
+    db_table_perf.cpp
)
-target_include_directories(db_table_test PRIVATE
-    ${CMAKE_SOURCE_DIR}
-    ${CMAKE_SOURCE_DIR}/net
-)
-target_compile_options(db_table_test PRIVATE ${CXX_WARNING_OPTIONS})
-gtest_discover_tests(db_table_test)
-
-target_include_directories(redis_dict PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/redis)
-# 对于 redis_dict，我们使用宽松模式编译，因为它是从C代码移植过来的
-target_compile_options(redis_dict PRIVATE
-    -fpermissive
-    -Wno-pedantic
-    -Wno-shadow
-    -Wno-error
-)
-
-add_executable(db_table_perf db_table_perf.cpp)
-target_link_libraries(db_table_perf PRIVATE

Evidence
CMake 把 db_table_perf.cpp 编入 unit_tests 且使用 gtest_discover_tests 注册，因此 `TEST(DbTablePerf,
Benchmark)` 会在 ctest 中默认执行；该测试固定运行到 1,000,000 元素规模的循环。
test/CMakeLists.txt[3-22]
test/db_table_perf.cpp[75-180]
test/db_table_perf.cpp[205-214]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A large performance benchmark (`DbTablePerf.Benchmark`) is now part of the default `unit_tests` binary and therefore runs under `ctest`. This can make CI slow/flaky due to long runtime.

## Issue Context
`unit_tests` is registered via `gtest_discover_tests(unit_tests)`, so every TEST in the binary becomes part of CTest.

## Fix Focus Areas
- test/CMakeLists.txt[3-22]
- test/db_table_perf.cpp[205-214]

### What to change
- Move `db_table_perf.cpp` into a separate executable target (e.g., `db_table_bench`) not registered in CTest by default, OR
- Rename the test to `DISABLED_Benchmark` (GTest convention) and/or guard it behind an env var/flag, OR
- Exclude perf tests from `gtest_discover_tests` using labels/regex if supported.

Goal: keep unit tests fast and deterministic; run benchmarks only on demand.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

8. ~~Fixed port test server~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

TxQueueTest 在每个用例的 SetUp 中启动 RedisServer 并硬编码绑定 6379 端口，端口被占用时测试将失败且无法并行运行。TearDown 里每次都 sleep(1)
会明显拉长整套测试用时。

Code

test/tx_queue_test.cpp[R19-31]

+  void SetUp() override {
+    loop_ = new EventLoop();
+    server_ = new RedisServer(6379, loop_, shardNum);
+    server_->Start();
+    q_ = new TxQueue();
+  }
+  void TearDown() override {
+    server_->Stop();
+    sleep(1);
+    delete server_;
+    delete loop_;
+    delete q_;
+  }

Evidence

测试夹具中明确创建 RedisServer(6379, ...) 并在 TearDown 无条件 sleep(1)，这直接引入端口冲突风险与固定额外耗时。

test/tx_queue_test.cpp[19-31]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new `TxQueueTest` fixture starts a real `RedisServer` on a fixed port (6379) for every test case and then sleeps for 1 second during teardown. This makes the suite flaky (port conflicts) and slow.

## Issue Context
Unit tests should be isolated and parallel-friendly; fixed ports and unconditional sleeps are common sources of CI flakes.

## Fix Focus Areas
- test/tx_queue_test.cpp[19-31]

### What to change
- Bind to an ephemeral port (port 0) or find a free port at runtime, and expose the chosen port if needed.
- Replace `sleep(1)` with a deterministic shutdown/join mechanism (wait for server thread/loop to stop).
- If the test only needs globals initialized (CIs/shard_set/namespaces), consider factoring out a lightweight init helper that doesn't require binding a TCP port.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

Previous review results

Review updated until commit aa1dbf2

Results up to commit 11cb2da ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. ~~Double-pop corrupts queue~~ ✓ Resolved 🐞 Bug ≡ Correctness

Description

TxQueue::Pop 在未验证迭代器仍属于队列的情况下直接摘链并加入空闲链表，重复 Pop 同一 Iterator 会破坏队列/空闲链表并导致崩溃或死循环。Transaction 在多处
Pop(pq_pos_[sid]) 后不清空 pq_pos_，且重试清理路径会再次 Pop 旧 iterator，使该损坏路径可达。

Code

src/detail/tx_queue.cpp[R65-84]

+void TxQueue::Pop(Iterator it) {
+  if (it == kEnd) {
    return;
  }
-
-  // 从链表中移除
  if (vec_[it].prev != kEnd) {
-    vec_[vec_[it].prev].next = vec_[it].next;
+      vec_[vec_[it].prev].next = vec_[it].next;
  } else {
-    head_ = vec_[it].next;  // 移除的是头节点
+      head_ = vec_[it].next;  
  }

  if (vec_[it].next != kEnd) {
-    vec_[vec_[it].next].prev = vec_[it].prev;
+      vec_[vec_[it].next].prev = vec_[it].prev;
  } else {
-    tail_ = vec_[it].prev;  // 移除的是尾节点
+      tail_ = vec_[it].prev;  
+  }
+  vec_[it].next = free_head_;
+  vec_[it].prev = kEnd;
+  vec_[it].trans = nullptr;
+  free_head_ = it;
+}

Evidence
TxQueue::Pop 仅判断 it == kEnd，随后无条件修改前后指针并把节点接回 free_head_；一旦对已释放节点再次 Pop，会把 free-list
节点当作队列节点处理并破坏结构。Transaction 多处调用 Pop(pq_pos_[sid])，但从未将 pq_pos_[sid] 置回 kEnd，在重试清理/FinishHop
等路径中可能重复 Pop 同一 iterator。
src/detail/tx_queue.cpp[65-84]
src/transaction_layer/transaction.cpp[176-215]
src/transaction_layer/transaction.cpp[240-270]
src/transaction_layer/transaction.cpp[291-319]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`TxQueue::Pop(Iterator)` currently assumes `it` is always a live node inside the active queue. If the same iterator is popped twice (or popped after it was already removed during a retry cleanup), it will treat a free-list node as a queue node and corrupt `head_`/`tail_` and the free list.

This is now reachable because `Transaction` keeps `pq_pos_[sid]` unchanged after removing the queue node, and the retry cleanup path calls `Pop(pq_pos_[sid])` again.

## Issue Context
- `TxQueue` no longer has a `used` flag / membership check.
- `Transaction::ScheduleInternal` and `Transaction::FinishHop` remove from the queue but never reset `pq_pos_[sid]` to `TxQueue::kEnd`.

## Fix Focus Areas
- src/detail/tx_queue.cpp[65-84]
- src/transaction_layer/transaction.cpp[176-215]
- src/transaction_layer/transaction.cpp[240-270]
- src/transaction_layer/transaction.cpp[291-319]

### What to change
1) Make `TxQueue::Pop` safe/idempotent:
  - Add an `in_use`/`used` boolean back into `Node`, or
  - Use a sentinel invariant (e.g., `trans==nullptr` means free) and check it before unlinking.
  - If node is already free/not in queue, `Pop` should return without mutating queue/free list.

2) Reset `pq_pos_[sid]` immediately after a successful pop in **all** paths:
  - After `e->txq()->Pop(pq_pos_[sid]);` set `pq_pos_[sid] = TxQueue::kEnd;`.
  - In the retry cleanup lambda, only pop when `pq_pos_[sid] != TxQueue::kEnd`, then reset it.

3) (Optional but recommended) Add debug assertions to detect double-pop early in debug builds.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. ~~Fixed port test server~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

TxQueueTest 在每个用例的 SetUp 中启动 RedisServer 并硬编码绑定 6379 端口，端口被占用时测试将失败且无法并行运行。TearDown 里每次都 sleep(1)
会明显拉长整套测试用时。

Code

test/tx_queue_test.cpp[R19-31]

+  void SetUp() override {
+    loop_ = new EventLoop();
+    server_ = new RedisServer(6379, loop_, shardNum);
+    server_->Start();
+    q_ = new TxQueue();
+  }
+  void TearDown() override {
+    server_->Stop();
+    sleep(1);
+    delete server_;
+    delete loop_;
+    delete q_;
+  }

Evidence

测试夹具中明确创建 RedisServer(6379, ...) 并在 TearDown 无条件 sleep(1)，这直接引入端口冲突风险与固定额外耗时。

test/tx_queue_test.cpp[19-31]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new `TxQueueTest` fixture starts a real `RedisServer` on a fixed port (6379) for every test case and then sleeps for 1 second during teardown. This makes the suite flaky (port conflicts) and slow.

## Issue Context
Unit tests should be isolated and parallel-friendly; fixed ports and unconditional sleeps are common sources of CI flakes.

## Fix Focus Areas
- test/tx_queue_test.cpp[19-31]

### What to change
- Bind to an ephemeral port (port 0) or find a free port at runtime, and expose the chosen port if needed.
- Replace `sleep(1)` with a deterministic shutdown/join mechanism (wait for server thread/loop to stop).
- If the test only needs globals initialized (CIs/shard_set/namespaces), consider factoring out a lightweight init helper that doesn't require binding a TCP port.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. ~~Benchmark in unit_tests~~ ✓ Resolved 🐞 Bug ☼ Reliability

Description

db_table_perf.cpp 作为 GTest 用例被编进 unit_tests 并通过 gtest_discover_tests 注册到 CTest，运行包含 10k/100k/1M 元素的
insert/find/erase 基准循环。默认执行会显著拖慢甚至超时 CI 的单测阶段。

Code

test/CMakeLists.txt[R3-8]

+# 单元测试可执行文件（统一入口，包含所有 *_test.cpp 和 *_perf.cpp）
+add_executable(unit_tests
+    db_table_test.cpp
+    tx_queue_test.cpp
+    db_table_perf.cpp
)
-target_include_directories(db_table_test PRIVATE
-    ${CMAKE_SOURCE_DIR}
-    ${CMAKE_SOURCE_DIR}/net
-)
-target_compile_options(db_table_test PRIVATE ${CXX_WARNING_OPTIONS})
-gtest_discover_tests(db_table_test)
-
-target_include_directories(redis_dict PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/redis)
-# 对于 redis_dict，我们使用宽松模式编译，因为它是从C代码移植过来的
-target_compile_options(redis_dict PRIVATE
-    -fpermissive
-    -Wno-pedantic
-    -Wno-shadow
-    -Wno-error
-)
-
-add_executable(db_table_perf db_table_perf.cpp)
-target_link_libraries(db_table_perf PRIVATE

Evidence
CMake 把 db_table_perf.cpp 编入 unit_tests 且使用 gtest_discover_tests 注册，因此 `TEST(DbTablePerf,
Benchmark)` 会在 ctest 中默认执行；该测试固定运行到 1,000,000 元素规模的循环。
test/CMakeLists.txt[3-22]
test/db_table_perf.cpp[75-180]
test/db_table_perf.cpp[205-214]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A large performance benchmark (`DbTablePerf.Benchmark`) is now part of the default `unit_tests` binary and therefore runs under `ctest`. This can make CI slow/flaky due to long runtime.

## Issue Context
`unit_tests` is registered via `gtest_discover_tests(unit_tests)`, so every TEST in the binary becomes part of CTest.

## Fix Focus Areas
- test/CMakeLists.txt[3-22]
- test/db_table_perf.cpp[205-214]

### What to change
- Move `db_table_perf.cpp` into a separate executable target (e.g., `db_table_bench`) not registered in CTest by default, OR
- Rename the test to `DISABLED_Benchmark` (GTest convention) and/or guard it behind an env var/flag, OR
- Exclude perf tests from `gtest_discover_tests` using labels/regex if supported.

Goal: keep unit tests fast and deterministic; run benchmarks only on demand.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

yy782 · 2026-07-26T09:38:09Z

/review

qodo-code-review · 2026-07-26T09:43:13Z

+# ── 检查 6379 端口占用 ────────────────────────────────────
+if ss -tlnp | grep -q ":6379 "; then
+    echo "ERROR: Port 6379 is already in use."
+    echo "Please free port 6379 before running tests."
+    exit 1
+fi


5. Port check false-negative 🐞 Bug ☼ Reliability

多个脚本在 set -euo pipefail 下使用 ss -tlnp | grep 做端口占用检测，但当环境缺少 ss 或者 ss -p 因权限看不到进程信息时，检测会误判“端口空闲”。随后启动服务/测试会在 bind 或连接阶段以更难定位的方式失败（EADDRINUSE/连接到旧进程）。

Agent Prompt

### Issue description Scripts depend on `ss -tlnp` for port-in-use detection. On systems without `ss` (e.g. some macOS/minimal containers) or where `-p` output is restricted, the check can become a false-negative and later steps fail with `EADDRINUSE` or connect to an unexpected process. ### Issue Context The port check is used in benchmark/integration/unit-test scripts. ### Fix Focus Areas - scripts/run_unit_tests.sh[11-16] - scripts/run_benchmark.sh[15-20] - scripts/run_integration_tests.sh[16-21] ### Suggested fix - Guard with `command -v ss >/dev/null` and fall back to another method (`lsof -iTCP:$PORT -sTCP:LISTEN`, `netstat`, or a small Python bind test). - Consider using `ss -tln` (no `-p`) to avoid permission-related visibility issues. - If no checker is available, print a warning and skip the pre-check (do not silently assume the port is free).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

qodo-code-review · 2026-07-26T09:43:20Z

Code review by qodo was updated up to the latest commit 0361146

yy782 added 6 commits July 24, 2026 08:58

修复并惰性求size

c5705dd

初步完成MultiConcurrent测试

962b5b7

编译成功

6d63ffe

Merge main into 修复事务队列，正常插入到队尾

513e1fb

重新整理单元测试，使用unit_tests

f782fcd

修复TxQueue::Grow()扩容忘记free_head_赋值的bug

11cb2da

qodo-code-review Bot reviewed Jul 25, 2026

View reviewed changes

Comment thread src/detail/tx_queue.cpp Outdated

Comment thread test/CMakeLists.txt

Comment thread test/tx_queue_test.cpp

yy782 added 11 commits July 25, 2026 13:26

正确设置UNIT_TESTS宏,UNIT编译默认加锁

c848f79

修复，让键正确分发到分片上

57c3e81

添加调试信息，可能是锁管理器不能正常清理释放锁导致的死锁问题

8652541

修复锁遗漏的问题

1385de5

测试运行前检查端口被占用

8292a45

添加tx_queue的调试日志，事务取消调度使用非协程

67515dc

使用保守的std::list

145306b

找到协程意外崩溃的原因了，协程没有写co_return;UB

0a0e13a

添加定时发现新添加的事务，就重试

e3bd8cc

DispatchHop应该在ScheduleInternal后调用,否则事务队列不能正常消费

e017b3a

测试全部通过，格式化

0361146

qodo-code-review Bot reviewed Jul 26, 2026

View reviewed changes

yy782 added 7 commits July 26, 2026 09:43

Merge branch 'main' into 修复事务队列，正常插入到队尾

1ff9b4a

Merge branch 'main' into 修复事务队列，正常插入到队尾

c4d2ff4

修复，Shard()函数遗漏了最后一个分片

3a0aaf4

添加空锁断言和部分锁问注释

f42e3cd

添加TryAdd()风险的注释

b4111c9

格式化

dc3a55f

添加test/tx_queue_test.cpp不稳定测试的注释

f531e24

yy782 added 6 commits July 26, 2026 12:31

使用engine_shard的内存分配器初始化资源

09e4fe1

修复:内联协程意外被恢复的问题,修复FinishHop逻辑

c7229ec

测试成功,但是不稳定

24659a9

格式化

50222c5

移除test/CMake的db_table_perf.cpp构建目标

9d91f5d

merge main

21be791

yy782 force-pushed the 修复事务队列，正常插入到队尾 branch from 968437c to 21be791 Compare July 27, 2026 04:12

yy782 added 5 commits July 27, 2026 04:14

Merge branch 'main' into 修复事务队列，正常插入到队尾

8b5e7de

Merge branch 'main' into 修复事务队列，正常插入到队尾

4bdded5

merge main

3e011e4

添加tx_queue测试

f71fbb8

修复id的宏定义域

aa1dbf2

yy782 merged commit 34491cb into main Jul 29, 2026
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

修复事务队列，正常插入到队尾 - #2

Hidden character warning

修复事务队列，正常插入到队尾#2
yy782 merged 35 commits into
mainfrom
修复事务队列，正常插入到队尾

yy782 commented Jul 25, 2026

Uh oh!

qodo-code-review Bot commented Jul 25, 2026

Uh oh!

qodo-code-review Bot commented Jul 25, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yy782 commented Jul 26, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qodo-code-review Bot Jul 26, 2026

Uh oh!

qodo-code-review Bot commented Jul 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Hidden character warning

Conversation

yy782 commented Jul 25, 2026

Uh oh!

qodo-code-review Bot commented Jul 25, 2026

PR Summary by Qodo

Uh oh!

qodo-code-review Bot commented Jul 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Previous review results

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yy782 commented Jul 26, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qodo-code-review Bot Jul 26, 2026

Choose a reason for hiding this comment

Uh oh!

qodo-code-review Bot commented Jul 26, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

qodo-code-review Bot commented Jul 25, 2026 •

edited

Loading