Skip to content

Fix issue #867: Implement signed-chronological iteration and strict-before removal for EventDb so th - #869

Open
octoaide[bot] wants to merge 2 commits into
mainfrom
octoaide/issue-867-1788466126666
Open

Fix issue #867: Implement signed-chronological iteration and strict-before removal for EventDb so th#869
octoaide[bot] wants to merge 2 commits into
mainfrom
octoaide/issue-867-1788466126666

Conversation

@octoaide

@octoaide octoaide Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Implement signed-chronological iteration and strict-before removal for EventDb so that public EventDb iterators and remove_before follow ascending/descending signed i128 ordering across the full i64 epoch-nanosecond range without changing stored key bytes or DB comparator. Add tests and CHANGELOG/docs updates as required by the issue.

Changed files

  • CHANGELOG.md
  • src/event.rs

Closes #867


This pull request was automatically created by octoaide.

@octoaide
octoaide Bot requested a review from sophie-cluml September 3, 2026 20:17
@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.67742% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 84.70%. Comparing base (8a29d66) to head (d008b19).

Files with missing lines Patch % Lines
src/event.rs 99.67% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #869      +/-   ##
==========================================
+ Coverage   84.58%   84.70%   +0.12%     
==========================================
  Files          92       92              
  Lines       37105    37364     +259     
==========================================
+ Hits        31385    31650     +265     
+ Misses       5720     5714       -6     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread src/event.rs Outdated
Direction::Forward => IteratorMode::Start,
Direction::Reverse => IteratorMode::End,
};
self.inner = physical_event_iterator(self.db, region, mode);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This creates the second RocksDB iterator only after the first region is exhausted. Without an explicit snapshot in ReadOptions, each physical iterator acquires an implicit snapshot at its own creation time, so one logical EventIterator can observe two database states. Inserts or deletes performed while consuming the first region can unexpectedly appear in or disappear from the second region, unlike the previous single-iterator implementation. Please retain one explicit snapshot for the lifetime of EventIterator, use it for both physical regions, and add a mutation-between-regions regression test.

Comment thread src/event.rs Outdated
for i in 0..total {
let time =
base_time + chrono::Duration::seconds(i64::try_from(i).expect("small value"));
let nanos = i64::try_from(i).expect("small value") - 750;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This produces 750 negative and 750 non-negative timestamps. Since delete_event_region is invoked separately for each physical region, neither invocation reaches EVENT_DELETION_BATCH_SIZE (1,000), so the mid-scan write/reset path is never exercised; this accounts for five of Codecov's six uncovered lines. Please store more than 1,000 events on each side of the epoch and verify both the full-batch flush and the remainder batch.

Comment thread src/event.rs Outdated
fn iterator_preserves_key_order_with_equal_timestamps() {
let (_permit, store) = setup_store();
let db = store.events();
let message = message_at_nanos(-1);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Every inserted message has the same DnsCovertChannel kind, so this test only proves ordering of the collision bits. The linked issue explicitly requires preserving both kind and collision ordering for equal timestamps. Please insert at least two valid event kinds at the same timestamp, including a duplicate of one kind, and assert the complete key order in both directions.

Comment thread src/event.rs
};
let key = i128::from_be_bytes(key_bytes);
let ts = (key >> 64) as i64;
if cutoff_nanos > i128::from(i64::MAX) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add a regression test that stores an event whose timestamp is exactly i64::MAX before exercising this branch. The current far-future test stores only an ordinary in-range timestamp, so the previous implementation that clamped the cutoff to i64::MAX would also pass even though it retained the i64::MAX event.

@octoaide

octoaide Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Pushed follow-up changes in d008b19.

  • Added coverage for full-batch and remainder flushes on both sides of the epoch.
  • Expanded equal-timestamp ordering checks across event kinds and collision bits.
  • Added a regression case for the exact i64::MAX timestamp.
  • Kept a single explicit snapshot across both physical iterator regions and added mutation-between-regions coverage.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix chronological iteration for signed event keys

1 participant