fix: deadline-aware update admission and TimestampWindow capacity safety

[zhanglei1949]

Summary

Fixes the two VersionManager prerequisites tracked by #821 without adding a second writer-admission mechanism:

1. update acquisition can honor an absolute monotonic deadline;
2. TimestampWindow cannot confuse two timestamps that alias the same ring slot or overrun its safe capacity.

This PR intentionally does not provide FIFO scheduling. Update waiters directly contend the existing admission phase, and the successful phase CAS linearizes ownership. Acquisition order is unspecified; this is sufficient for MVCC and transaction correctness while preserving the auto-commit update fast path.

Changes

Deadline-aware update admission

• Add an optional steady_clock deadline to acquire_update_timestamp and UpdateTimestampLease.
• Preserve the legacy no-deadline path: no clock read and no extra queue or mutex.
• On the deadline path, contenders directly CAS kOpen -> kInsertsBlocked with runtime-aware backoff.
• Check expiry while competing for the phase, draining admitted inserts, and immediately before timestamp reservation.
• Restore any phase acquired by the timed-out attempt before throwing TransactionTimeoutException (ERR_TX_TIMEOUT); do not leak a timestamp or inserter count.

TimestampWindow capacity and identity

• Replace boolean completion slots with exact uint32_t timestamp tags (0 means empty).
• Reserve a timestamp only when candidate - read_ts <= window_size using 64-bit arithmetic.
• Stop before the reserved UINT32_MAX sentinel instead of wrapping the timeline.
• Release insert admission before backing off on a full window, so update/compact drain cannot deadlock on a timestamp-less inserter.
• Remove the old range-based window cleanup; advancing read_ts now clears only the exact completed timestamp.

Scope

This is readiness work for explicit transactions planned for NeuG v0.3. It does not add TransactionContext, multi-statement execution, a public explicit-transaction API, WAL framing, COW-core extraction, checkpoint changes, a waiter queue, or a general clock abstraction.

Fixes #821

[Copilot]

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

This PR closes correctness gaps in transaction admission by adding deadline-aware FIFO update admission and making TimestampWindow capacity reservation safe (no wrap-around and no leaked admission counts on backpressure/timeouts).

Changes:

• Add optional monotonic deadlines to update admission, with FIFO queuing and a new TransactionTimeoutException.
• Make timestamp window completion slots carry exact timestamp identity and reserve write timestamps via CAS with a capacity check.
• Add deterministic concurrency tests covering FIFO ordering, timeout cleanup, no-clock-read fast paths, window backpressure, and exhaustion behavior.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/transaction/test_transaction_release_order.cc	Adds a regression test ensuring deadline failure does not create lease ownership.
tests/transaction/test_runtime_wait.cc	Adds deterministic tests for FIFO update admission, deadline behavior, backpressure, and exhaustion.
tests/transaction/test_read_view_publication.cc	Updates mocked interface to new deadline-aware update acquisition signature.
src/utils/exception/exception.cc	Implements TransactionTimeoutException mapped to ERR_TX_TIMEOUT.
src/transaction/version_manager.cc	Implements FIFO update admission with optional deadlines; adds timestamp reservation logic and backpressure-safe insert admission release.
src/transaction/timestamp_window.cc	Switches completion slots from bools to exact timestamp identity and removes sliding cleanup.
src/transaction/timestamp_lease.cc	Adds deadline-bearing UpdateTimestampLease constructor.
include/neug/utils/exception/exception.h	Declares TransactionTimeoutException and adds THROW_TRANSACTION_TIMEOUT.
include/neug/transaction/version_manager.h	Extends acquire_update_timestamp with optional deadline and adds internal queue/reservation helpers.
include/neug/transaction/timestamp_window.h	Updates completion-slot representation and exposes kWindowSize as a public constant.
include/neug/transaction/timestamp_lease.h	Adds deadline-bearing lease constructor declaration.

---

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

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/transaction/version_manager.cc:232

• In release builds, if candidate <= current_read_ts ever occurs, the subtraction will underflow (after the casts) and outstanding will become huge, causing the code to mis-report a full window and potentially spin/backpressure incorrectly. Since this is an invariant violation, consider turning the DCHECK_GT into a CHECK_GT (fail-fast) or adding an explicit runtime branch that handles candidate <= current_read_ts by aborting with an internal error before doing the subtraction.

    const uint32_t current_read_ts = read_ts_.load(std::memory_order_acquire);
    DCHECK_GT(candidate, current_read_ts);
    const uint64_t outstanding = static_cast<uint64_t>(candidate) -
                                 static_cast<uint64_t>(current_read_ts);
    if (outstanding > TimestampWindow::kWindowSize) {
      return {TimestampReservationState::kWindowFull};
    }

src/transaction/version_manager.cc:388

• Removing an update waiter is currently O(n) due to std::find on the deque. Under heavy update contention, repeated removals/timeouts can make total work quadratic in the number of waiters. A more scalable approach is to store an iterator/node handle in UpdateWaiter (e.g., switch to std::list and store std::list<...>::iterator, or use an intrusive list) so removal is O(1) without searching.

void VersionManager::remove_update_waiter(UpdateWaiter* waiter) {
  std::lock_guard lock(update_waiters_lock_);
  const auto it =
      std::find(update_waiters_.begin(), update_waiters_.end(), waiter);
  // A missing waiter means admission bookkeeping is already corrupt. Aborting
  // is safer than throwing here, where the caller may hold an admission phase
  // that only a successful removal would ever release.
  CHECK(it != update_waiters_.end()) << "Update waiter is not queued";
  update_waiters_.erase(it);
}

tests/transaction/test_runtime_wait.cc:89

• g_blocked_waiters is incremented before the thread actually observes g_block_waiters under the mutex and blocks. This can make WaitForBlockedWaiters() succeed even if a call didn’t truly block (especially if future tests reuse this helper in different ordering). To make the barrier deterministic, consider moving the increment to after acquiring the lock and confirming g_block_waiters is true (or increment immediately before calling wait() while holding the lock), and reset the waiter count while holding the same mutex to avoid miscounting if these helpers are ever used concurrently.

void BlockingRuntimeWait(RuntimeWaitAction) noexcept {
  g_blocked_waiters.fetch_add(1, std::memory_order_release);
  std::unique_lock lock(g_blocking_wait_lock);
  g_blocking_wait_cv.wait(lock, [] { return !g_block_waiters; });
}

void BlockRuntimeWaiters() {
  g_blocked_waiters.store(0, std::memory_order_relaxed);
  std::lock_guard lock(g_blocking_wait_lock);
  g_block_waiters = true;
}

include/neug/transaction/timestamp_lease.h:29

• The deadline type alias is duplicated here and in version_manager.h. Even though identical redeclarations are currently OK, it creates a maintenance hazard if one side ever changes. A more robust pattern is to centralize MonotonicTimePoint (and related MonotonicNowFn) in a small shared header (e.g., a transaction/time_types.h) that both headers include, eliminating the need for duplication while keeping dependency weight low.

// Same deadline time type as IVersionManager::acquire_update_timestamp.
// Redeclared here so this RAII header does not need to include the full
// version manager header; diverging from the manager's alias is a compile
// error wherever both are visible.
using MonotonicTimePoint = std::chrono::steady_clock::time_point;

[Copilot]

Pull request overview

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

Suppressed comments (1)

src/transaction/version_manager.cc:388

• remove_update_waiter() does a linear scan over update_waiters_ on every timeout/success path. Under high update contention (large queue) this can become a measurable hot spot, especially with deadlines causing frequent removals. A more scalable approach is to store a stable handle in UpdateWaiter (e.g., switch the container to std::list and store an iterator in the waiter, or use an intrusive list/node) so removal is O(1) without searching.

void VersionManager::remove_update_waiter(UpdateWaiter* waiter) {
  std::lock_guard lock(update_waiters_lock_);
  const auto it =
      std::find(update_waiters_.begin(), update_waiters_.end(), waiter);
  // A missing waiter means admission bookkeeping is already corrupt. Aborting
  // is safer than throwing here, where the caller may hold an admission phase
  // that only a successful removal would ever release.
  CHECK(it != update_waiters_.end()) << "Update waiter is not queued";
  update_waiters_.erase(it);
}

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (6)

tests/transaction/test_runtime_wait.cc:457

• Using a fatal ASSERT after starting std::thread is unsafe: if this assertion fails, the test returns early and the joinable thread's destructor will call std::terminate. Prefer a non-fatal EXPECT here so the test can still release waiters and join the thread cleanly.

  ASSERT_TRUE(WaitForBlockedWaiters(1));

tests/transaction/test_runtime_wait.cc:424

• Using a fatal ASSERT after starting std::thread is unsafe: if this assertion fails, the test returns early and the joinable threads' destructors will call std::terminate. Prefer a non-fatal EXPECT here so the test can still release waiters and join threads cleanly.

  ASSERT_TRUE(WaitForBlockedWaiters(2));

tests/transaction/test_runtime_wait.cc:492

• Using a fatal ASSERT after starting std::thread is unsafe: if this assertion fails, the test returns early and the joinable thread's destructor will call std::terminate. Prefer a non-fatal EXPECT here so the test can still release waiters and join the thread cleanly.

  ASSERT_TRUE(WaitForBlockedWaiters(1));

tests/transaction/test_runtime_wait.cc:503

• Using a fatal ASSERT after starting std::thread is unsafe: if this assertion fails, the test returns early and the joinable threads' destructors will call std::terminate. Prefer a non-fatal EXPECT here so the test can still release waiters and join threads cleanly.

  ASSERT_TRUE(WaitForBlockedWaiters(2));

src/transaction/version_manager.cc:190

• The internal-error message in release_insert_admission still says "release_insert_timestamp without admission", which is misleading for callers and makes crashes harder to triage. It should refer to the actual helper being used.

  const uint64_t observed =
      operation_gate_state_.load(std::memory_order_relaxed);
  if (NEUG_UNLIKELY(OperationGateWord::inserters(observed) == 0)) {
    THROW_INTERNAL_EXCEPTION("release_insert_timestamp without admission");
  }

tests/transaction/test_runtime_wait.cc:418

• Using a fatal ASSERT after starting std::thread is unsafe: if this assertion fails, the test returns early and the joinable threads' destructors will call std::terminate. Prefer a non-fatal EXPECT here so the test can still release waiters and join threads cleanly.

This issue also appears in the following locations of the same file:

• line 424
• line 457
• line 492
• line 503

  ASSERT_TRUE(WaitForBlockedWaiters(1));

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/transaction/version_manager.cc:198

• The internal error message here references release_insert_timestamp, but the check is in release_insert_admission() (also called from the window-full backoff path). This makes failures harder to diagnose because the message can point to the wrong call site.

void VersionManager::release_insert_admission() {
  const uint64_t observed =
      operation_gate_state_.load(std::memory_order_relaxed);
  if (NEUG_UNLIKELY(OperationGateWord::inserters(observed) == 0)) {
    THROW_INTERNAL_EXCEPTION("release_insert_timestamp without admission");
  }

src/transaction/version_manager.cc:227

• __builtin_trap() is a compiler-specific builtin and is not used elsewhere in the codebase. Using the existing glog CHECK_* macros keeps the fail-fast behavior while remaining portable across toolchains that may not support this builtin.

          DCHECK_GT(candidate, current_read_ts);
          __builtin_trap();

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.

Suppressed comments (3)

tests/transaction/test_runtime_wait.cc:485

• Like the other blocking-waiter tests, this one can hang the whole suite if an ASSERT fails after BlockRuntimeWaiters() (leaving g_block_waiters=true and threads stuck in BlockingRuntimeWait()). Add an RAII guard after blocking so ReleaseRuntimeWaiters() is guaranteed to run even on early test exit.

  BlockRuntimeWaiters();
  std::promise<uint32_t> waiting_insert;

tests/transaction/test_runtime_wait.cc:445

• This test blocks the runtime waiters and then uses ASSERT_TRUE() before unblocking them. If the assertion fails (or any early return happens), the update thread can remain stuck in BlockingRuntimeWait() with g_block_waiters still true, hanging the test run. Introduce a local RAII guard right after BlockRuntimeWaiters() to always call ReleaseRuntimeWaiters() on scope exit.

  BlockRuntimeWaiters();
  std::promise<bool> timed_out;

tests/transaction/test_runtime_wait.cc:404

• After BlockRuntimeWaiters(), this test uses ASSERT_TRUE() while other threads are potentially blocked inside BlockingRuntimeWait(). If an ASSERT aborts the test early, g_block_waiters remains true and the blocked threads never wake, which can hang the entire test binary and subsequent tests. Add a small RAII guard immediately after blocking to guarantee ReleaseRuntimeWaiters() runs on all exit paths.

This issue also appears in the following locations of the same file:

• line 444
• line 484

  BlockRuntimeWaiters();
  std::promise<bool> waiter_timed_out;

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

tests/transaction/test_transaction_release_order.cc:229

• This test references exception::TransactionTimeoutException but does not include neug/utils/exception/exception.h directly, relying on transitive includes (currently via other headers). That can break with include cleanups or different include ordering; add the explicit include for robustness.

  const auto holder = version_manager.acquire_update_timestamp();
  EXPECT_THROW(
      UpdateTimestampLease(version_manager, std::chrono::steady_clock::now()),
      exception::TransactionTimeoutException);

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Copilot]

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.