Chrono to Jiff migration plan and compatibility checks

[danbi2990]

Background

As part of discussion-repo issue#344, I would like to share a plan for reducing the remaining chrono dependency and exposure in review-database, and to discuss the time-type contract that this repository should provide to downstream consumers.

Based on the latest discussion-repo issue#344 summary, manager and unsupervised-engine have already moved much of their internal code to Jiff, but chrono still remains at the boundaries that touch review-database.

Therefore, I think the starting point for this work should be the contract defined by review-database. However, changing the review-database contract can directly affect downstream compile/API compatibility or serialized payload compatibility, so the goal is to make that impact visible early through fixtures and compile checks.

Before opening implementation PRs, I reviewed previous chrono -> Jiff work in other repositories and relevant precedents inside review-database. My conclusion is that this should not be treated as a simple replacement of chrono types with Jiff types. A safer approach is to first define and verify the review-database boundaries for public API, serialized payloads, and RocksDB stored data, and then let downstream consumers follow that contract.

Proposed issue and PR structure

Implementation PRs may be generated by Octoaide. So I think the important thing for this discussion is not to prescribe exact patches, but to agree on the contract and review gates that our human-written issues should require.

Umbrella issue: review-database chrono to Jiff tracking

Purpose:

• Track the whole review-database side of discussion-repo issue#344.
• Keep the contract decisions, sub-issues, Octoaide PRs, and review gate status in one place.
• Avoid losing the data migration and downstream impact decisions across multiple generated PRs.

Acceptance criteria:

• Links every human-written Octoaide work issue and generated PR.
• Records the current decision for each stored-data surface: no data migration, custom adapter, compatibility parser/writer, or data migration.
• Tracks whether the compatibility baseline, implementation PRs, downstream checks, and final chrono cleanup are complete.
• Records maintainer confirmations for private manager / unsupervised-engine boundaries when a sub-issue touches those boundaries.

Issue 1: Compatibility baseline

Purpose:

• Capture the current chrono behavior before changing production types.
• Prove the current stored/key/payload contracts with fixtures or equivalent tests.
• Group baseline work by shared contract where useful, such as event keys,
  ts_nanoseconds stored fields, default bincode table values, string
  timestamp values, and column_stats.
• Make the first Octoaide PR easy to review because it should not change public API signatures or remove chrono.

Acceptance criteria:

• Existing event key timestamp semantics are fixed by tests.
• Existing event stored field byte contracts are covered enough to catch a serde-shape regression.
• Existing column_stats key/value timestamp behavior is covered.
• Representative default chrono bincode table values are covered.
• The table-value vs event-field bincode encoder difference is reflected in the tests.
• The baseline includes nanosecond precision, pre-1970 timestamps, and i64 nanos boundary/overflow behavior.
• The PR description explains how the author checked that the tests are not merely round-trip tests, for example by showing that representative temporary mutations would fail the baseline.
• Invalid or out-of-range timestamp behavior is either asserted directly or explicitly documented as out of scope for the baseline.

Issue 2+: Jiff implementation with stored-contract preservation

Purpose:

• Replace the selected chrono API/types with Jiff or primitive timestamp types.
• Preserve existing RocksDB key/value byte shape unless a data migration is explicitly required.
• Let Octoaide split the implementation into multiple PRs if the diff becomes too broad, for example by event boundary, column_stats, or stored table surface.
• After the baseline has fixed the relevant contracts, prefer implementation
  issues that close over one struct or a closely related struct group when that
  makes migration, production code, fixtures, and review more coherent.

Acceptance criteria:

• review-database::EventMessage.time is Jiff-based, while event key timestamp bytes continue to use the existing i64 epoch nanoseconds contract.
• chrono::serde::ts_nanoseconds stored fields use a custom Jiff adapter that reads and writes the existing i64 nanos bytes.
• Default chrono bincode table values remain compatible through matching Jiff default serde types where the touched surface proves the same byte contract.
• TorExitNode.updated_at and TrustedUserAgent.updated_at are handled as explicit data migration surfaces.
• If any stored byte shape changes, the issue/PR must include the corresponding data migration plan, migration structures for the old schema, old-data fixtures, and version/range behavior.
• If a surface is kept no-migration by preserving byte shape, the PR should show that old bytes are still readable by the new code.
• Downstream-facing API impact is visible through review-database tests and, if feasible, accessible downstream compile checks such as review-web.

Issue 3: Chrono dependency cleanup

Purpose:

• Remove remaining chrono usage only after compatibility evidence and Jiff implementation PRs have landed.
• Keep this as a small cleanup issue if possible.

Acceptance criteria:

• Production code no longer needs chrono.
• Any remaining chrono usage is limited to compatibility fixtures or old data migration structures, or is explicitly justified.
• A compile-surface check confirms no unexpected direct chrono surface remains.

PR review expectations

For Octoaide-generated PRs, I would review against at least these expectations.

• Stored byte shape must stay stable unless the issue explicitly asks for data migration.
• Event key and column_stats key must preserve the existing i64 epoch nanoseconds contract, including the 1677-09-21 to 2262-04-11 range behavior.
• Event key being a 16-byte big-endian i128 key must not be confused with storing timestamps as Jiff i128 nanoseconds.
• chrono::serde::ts_nanoseconds replacements must use a custom i64 nanos Jiff adapter, not Jiff default serde.
• Default chrono bincode values should remain readable with matching Jiff default serde types only for the touched surfaces where tests prove the same encoder settings and byte contract.
• Explicit DateTime<Utc>::to_string() timestamp values must be handled through data migration, not silently switched to another format.
• Fixture generation must reflect the production encoder path. Table values and event fields use different bincode paths, so a generic round-trip test is not enough.
• Tests should include nanosecond precision, pre-1970 timestamps, and boundary or overflow behavior for i64 nanos where relevant.
• Data migration PRs should include explicit scenarios for empty DB migration,
  old DB migration, successful no-op behavior when re-run, and resulting state
  consistency including version files. The review-migrate binary should be
  used where practical.
• Downstream impact should be checked against available repositories, while private manager / unsupervised-engine details should be confirmed with maintainers when a PR touches those boundaries.

Supporting analysis

Surface inventory summary

The currently identified chrono/Jiff migration surfaces can be grouped into the following categories. The baseline issue/PR does not need to change all of them at once. Instead, I propose using this inventory as the review frame: surfaces actually changed by a PR should be covered by fixtures or data migration tests, and surfaces not changed by that PR should be explicitly left out of scope.

One important classification detail is that a stored table surface and a public API surface can share the same Rust struct. When creating actual issues, each chrono-bearing struct should be checked for both roles: whether it defines stored table value/key bytes, and whether the same struct or a related DTO is publicly exported or returned by public Store/Table methods.

• Event ingress / storage
  • review-database::EventMessage.time
  • Event RocksDB key timestamp bytes
  • Event fields stored bytes
  • DateTime-bearing *FieldsStored and EventDb / iterator returned event time semantics
  • EventDb::remove_before
• column_stats and related query surfaces
  • column_stats public methods that accept/return chrono::NaiveDateTime
  • internal i64 nanoseconds key layout
  • Statistics.batch_ts
  • load_rounds_by_cluster seek/pagination behavior
  • structured::Element::DateTime values inside column statistics
• Stored bincode table values with chrono fields
  • Account, Customer, Network, Node::Inner
  • SamplingPolicy, TrafficFilter, TriagePolicy
  • tables::Cluster, TriageResponse, Filter.period
  • metadata tables such as ModelIndicator, TorExitNode, TrustedUserAgent
• Public/API and compile surfaces
  • event graph structs exposed by review-database
  • table structs that are also public API types, such as Account
  • types::Cluster, tables::Cluster, private ClusterDbSchema
  • TriagePolicyInput and other public DTO/input structs, if any
  • TriageResponse public methods and timestamp-bearing key contract
  • backup::BackupInfo.timestamp
  • deprecated top_n/time_series.rs
  • review-web GraphQL/API wrappers around Store, events, column_stats, and time filters
• Non-timestamp time/config surfaces, expected to stay unchanged unless explicitly touched
  • BackupConfig.backup_time
  • AccountPolicy.expiry_period_in_secs
  • AccountPolicy.lockout_duration_in_secs
  • BackupConfig.backup_duration
  • event_retention_period_days
  • RetentionConfig.period_in_days and purge_old_column_stats
  • primitive timestamp call sites such as Host.creation_time and classifier temp-path timestamps
• Migration compatibility
  • old migration structures using chrono serde helpers
  • migration version/range behavior if any stored bytes or key layouts change
  • bincode encoder split: table values use DefaultOptions, event fields use plain bincode::serialize

Inventory validation result

I also performed one mechanical validation pass before implementation.

Method:

1. Temporarily removed the direct chrono dependency from review-database.
2. Ran cargo check --message-format=json.
3. Classified the resulting direct chrono errors into the following categories: public/API surface, stored-data migration surface, internal final-cleanup surface, and old-schema migration compatibility surface.
4. Immediately reverted Cargo.toml, and only carried the classification results into the inventory and PR checklist.

The check produced 68 direct chrono errors and did not reveal any new top-level surface category. It did, however, clarify several final-cleanup call sites such as the lockout calculation in tables/accounts.rs, the default range calculation in tables/time_series.rs, the purge cutoff calculation in lib.rs, and classifier temp-path timestamps in classifier_fs.rs.

What this check means:

• Strength: it quickly exposes direct chrono imports, chrono::serde helpers, Utc::now(), and chrono duration helpers.
• Limitation: it does not prove bincode byte compatibility, old DB migration correctness, or downstream compatibility.
• Therefore, this step should not replace stored-format decisions. It is only an auxiliary check to reduce the chance of missing a surface in the inventory.
• Repeating the same check during implementation PRs can also show whether the direct chrono surface is actually shrinking.

Why not remove chrono immediately?

In previous PRs from other repositories, reviewers repeatedly asked for the same safeguards:

• Freeze existing chrono behavior with tests first.
• Write tests that exercise the real production path.
• Confirm serialization/deserialization compatibility.
• Do not lose subsecond or nanosecond precision.
• Do not mix UTC and local timezone semantics.
• Split the work into baseline tests, production implementation, and dependency cleanup.

In particular, one semi-supervised-engine migration review pointed out that EventMessage encoded with Jiff by semi-supervised-engine had to be decoded/deserialized correctly by chrono-based review-database. I think the same review perspective applies to this review-database work.

Stored data migration scope: decision

Data migration should not be treated as globally required. I think the rule for this work should be:

• If stored bytes are already i64 epoch nanos or i64 epoch seconds, and the Jiff change can keep the same primitive bytes, do not perform data migration.
  However, i64 epoch nanoseconds have a representable range of roughly 1677-09-21 through 2262-04-11. Current chrono timestamp_nanos_opt() also returns None outside this range, so the Jiff adapter should explicitly preserve the same range constraint.
• If stored bytes use default chrono DateTime<Utc> / NaiveDateTime serde, then changing to the matching Jiff default serde type may proceed without data migration only for surfaces where the PR proves the same encoder settings and byte contract. A direct reproduction showed identical bincode bytes and successful old-byte decode for the checked primitive chrono/Jiff type pairs, but each touched stored surface should still be covered by tests or fixtures.
• If stored bytes are ad hoc strings based on chrono to_string(), then changing them to a Jiff-native format or primitive format should be treated as data migration.
• Do not mechanically apply Jiff default serde or Jiff nanosecond serde helpers at stored boundaries. Jiff default Timestamp serde matches default chrono DateTime<Utc> bincode bytes in the checked case, but it does not match the i64 bytes from chrono::serde::ts_nanoseconds. Jiff nanosecond serde helpers are also i128-based, so they are not the existing i64 contract. Therefore, when a stored field currently using chrono::serde::ts_nanoseconds is changed to a Jiff type, it needs a custom serde/adapter that reads and writes the existing i64 bytes. Jiff conversion APIs such as as_nanosecond() / from_nanosecond() should only be used inside that adapter implementation.

Direct reproduction results:

• Chrono DateTime<Utc> bincode bytes using default serde were identical to Jiff default Timestamp bincode bytes in the checked reproduction, and decoded as Jiff Timestamp.
• Chrono NaiveDateTime bincode bytes using default serde were identical to Jiff civil::DateTime bincode bytes in the checked reproduction, and decoded as Jiff civil::DateTime.
• Chrono ts_nanoseconds bytes did not decode as Jiff default Timestamp.
• Chrono DateTime<Utc>::to_string() values did not parse as Jiff Timestamp.

Data migration not required if we preserve stored bytes

The following should not require data migration if designed to preserve the existing stored contract:

• Stored bincode table values with default chrono fields:
  Account, Customer, Network, Node::Inner, SamplingPolicy,
  TrafficFilter, TriagePolicy, tables::Cluster,
  TriageResponse stored value, Filter.period
• Event RocksDB key:
  Even if EventMessage.time is changed to Jiff, the existing key byte layout can be preserved by converting it to the same i64 epoch nanos before key construction.
  The event key itself is a 16-byte big-endian i128 key composed from timestamp nanos i64, event kind, and random/counter bits. This does not mean the timestamp is stored as Jiff i128 nanos.
  Therefore, we should not introduce Jiff's wider nanosecond range into the event key. Instead, the existing i64 nanos range and overflow behavior should be fixed by fixtures.
• column_stats key/value:
  The public API currently uses NaiveDateTime, but the stored key is already i64 nanos, and the value is structured::Description / NLargestCount.
• Event stored fields with primitive time:
  Stored fields that are already i64 nanos, such as start_time / end_time, should stay as they are.
• Event stored fields with chrono::serde::ts_nanoseconds:
  Even if these are changed to Jiff, the existing bincode bytes can be preserved by using a custom i64 nanos serde adapter. In that case, data migration should not be performed.
• ModelIndicator.last_modification_time:
  This currently uses chrono::serde::ts_seconds, so the stored contract is i64 seconds. If the same seconds representation is kept after the Jiff change, data migration should not be performed.

Data migration required

The following should be treated as requiring data migration code that reads old DB value bytes and writes them back using the new schema:

• Explicit timestamp string tables using DateTime<Utc>::to_string():
  TorExitNode.updated_at, TrustedUserAgent.updated_at
  (values stored with updated_at.to_string().into_bytes() and read by string parsing)
  Because these surfaces already require data migration, the new stored contract
  should consider a primitive representation such as i64 epoch
  seconds/nanoseconds instead of another library-dependent string or serde
  representation.

Special case: EventMessage.time

review-database::EventMessage.time should be changed to Jiff. However, the event table does not store the whole EventMessage as a value. EventDb::put converts EventMessage.time to the same i64 epoch nanoseconds value as today and stores that value in the timestamp portion of the event RocksDB key. EventMessage.fields is converted separately into storage-specific *FieldsStored bytes and stored as the value.

Therefore, changing the Rust type of EventMessage.time is not old RocksDB value data migration. The required checks are:

• An event key stability fixture proving that Jiff EventMessage.time enters the key as the same i64 epoch nanoseconds.
• A protocol fixture proving that review-protocol::EventMessage.time with a Jiff timestamp is ingested with the same timestamp semantics.
• If a downstream producer directly sends a review-database::EventMessage bincode payload, that should be handled as a payload compatibility issue, not DB data migration.

Data migration policy

1. Old stored value schemas that are actually data-migrated should remain as migration structures, following the existing convention.
2. A data-migrated surface should add old-byte fixtures and verify that the new code can read the new stored bytes after data migration.
3. Data migration tests should cover empty DB, old DB, re-run/no-op behavior, and version file consistency.
4. The goal is not to permanently dual-read old chrono bytes in the production read path.
5. Backward-write compatibility, where an old binary reads a DB written by the new code, is out of scope for this work.
6. Detailed surface-by-surface decisions are captured in a separate internal analysis note, and I can summarize them in a discussion follow-up if needed.

References

• discussion-repo issue#344
• discussion-repo issue#514
• review-database#382 timestamp precision comment
• review-database#382 post-#466 column_stats note

[danbi2990]

@sophie-cluml Could you review this plan when you have time?

I would especially appreciate your feedback on the proposed issue/PR structure, the stored-data compatibility and data migration policy, and whether the expectations for Octoaide-generated PRs are clear enough before we start writing the actual work issues.

[sophie-cluml]

Thank you for the detailed review and proposal. I generally agree with the direction. Before turning this into actual issues, I would like to suggest considering the following points.

1. Regarding the overall issue structure, I think the following could work well (as you suggested):

• Umbrella tracking issue
• Baseline / fixture issues
• Implementation issues (which may involve DB migration)
• Final cleanup issue

For the baseline phase, I think it may be better to group together surfaces that share the same contract, so that the relevant tests can be established and reviewed more clearly. Where appropriate, these baseline issues may cover not only the storage layer but also the public API layer. For example, the split could look like this:

• ts_nanoseconds storage contract baseline
• default bincode chrono value baseline
• string timestamp baseline
• event key baseline
• column_stats baseline

The storage layer and API layer may be handled within the same issue or in separate issues, depending on how tightly the changes are coupled and how reviewable the resulting scope is.

For the implementation phase, my preference would be to proceed with struct-by-struct issues, or at least issues organized around closely related struct groups (for example, TriagePolicy + TriageResponse). The reason is that the baseline phase is meant to pin down the current contract, so a cross-cutting split is acceptable there. By contrast, in the implementation phase, it may be more appropriate from the standpoint of completeness and reviewability for migration, production code, fixtures, and review to close over a single unit.

2. I think the “Surface inventory summary” is generally well organized, but it does seem possible that some surfaces are still missing. For example, on the Public/API side, it would be good to explicitly check surfaces such as Account and TriagePolicyInput, and on the DB side, surfaces such as TriageResponse, where the key contract itself involves a timestamp value. I do not think this discussion needs to enumerate every surface perfectly at this stage, but I would appreciate a careful pass on this when the actual issues are created.

3. For some surfaces, we may eventually conclude that explicit migration is required. If so, I think it is worth considering whether those surfaces should be normalized to a primitive-based storage contract, instead of continuing to depend on string formats or library-dependent time representations. I do not mean that all time information in review-database should be stored as primitive types. However, if a surface already requires migration anyway, especially one currently stored using DateTime<Utc>::to_string(), then it may be beneficial to migrate it toward primitive DB values.

4. I think it could be helpful to make migration test scenarios explicit in the acceptance criteria or PR review expectations. For example, this could include tests for successful execution on an empty DB, successful execution on an old DB, successful no-op behavior when re-run after migration has already completed, and consistency of the resulting state including the version file. As a practical note, I think some of the above testing could be exercised by utilizing the review-migrate binary in this repository.

[danbi2990]

Thank you for the detailed review. I updated the discussion body to reflect your comments.

Changes I made:

• Added the baseline/implementation split you suggested: baseline work can be grouped by shared contract, while implementation issues should preferably close over one struct or a closely related struct group when that makes migration, production code, fixtures, and review more coherent.
• Added a note that stored table surfaces and public API surfaces may share the same Rust struct, so actual issues should check both roles explicitly.
• Added TriagePolicyInput to the public/API surface list, and also called out TriageResponse as a timestamp-bearing key-contract surface.
• Added explicit migration test expectations for empty DB migration, old DB migration, re-run/no-op behavior, version file consistency, and using review-migrate where practical.
• Added a note that surfaces already requiring data migration, especially DateTime<Utc>::to_string()-based timestamp values, should consider a primitive storage contract such as i64 epoch seconds/nanoseconds instead of moving to another library-dependent string or serde representation.

I’ll use these points when writing the actual work issues.

[sophie-cluml]

Looks good to me. Could you proceed to create the epic issue and sub-issues please?