Skip to content

Migrate Network/Node creation_time to a primitive i64 epoch-nanoseconds storage (chrono to Jiff) #810

Description

@sophie-cluml

Background

Network and Node::Inner store creation_time via chrono's default serde, which renders an RFC 3339 string, and review-database encodes table values with bincode::DefaultOptions (the private serialize helper in src/tables.rs). In other words, the stored contract is a library-dependent (chrono) timestamp string.

These surfaces are part of the chrono → Jiff migration in discussion #733, and were previously handled through #786 → PR #808. However, #808 introduced a serde adapter that routes jiff::Timestamp through chrono in order to keep the existing byte contract, which conflicts with:

"no migration = byte-identical" does not hold (measured)

#733 classified default DateTime<Utc> surfaces as "no data migration needed if stored bytes are preserved (= use the matching Jiff default serde)", but this only holds coincidentally for full-precision values without trailing zeros. Reproduced with a POC test that exercises the production encoder (bincode::DefaultOptions) — see the <details> below, runnable via cargo test:

  • chrono pads the fractional part to 0/3/6/9 digits (SecondsFormat::AutoSi, keeping trailing zeros: .120, .500, .100).
  • jiff::Timestamp also serializes to a string, but trims trailing zeros (.12, .5, .1).
  • So the bytes differ for common values such as millisecond precision.
  • Reads work both ways (old chrono strings are still readable by jiff).
Reproduce: tests/poc_timestamp_bytes.rs (POC branch poc/network-node-timestamp-bytes)

After enabling jiff's serde feature in Cargo.toml, serialize with the same bincode::DefaultOptions used in production and compare. Verified passing with cargo test --test poc_timestamp_bytes --all-features.

use bincode::Options;
use chrono::{DateTime, Utc};
use jiff::Timestamp;

fn table_value_bytes<T: serde::Serialize>(value: &T) -> Vec<u8> {
    // Same as the private `serialize` in src/tables.rs: bincode::DefaultOptions
    bincode::DefaultOptions::new().serialize(value).expect("serializes")
}

#[test]
fn chrono_and_jiff_table_value_bytes_are_not_interchangeable() {
    // Trailing-zero values: chrono pads vs jiff trims -> different bytes
    for s in ["2024-01-15T10:30:00.120Z", "2024-01-15T10:30:00.500Z"] {
        let c: DateTime<Utc> = DateTime::parse_from_rfc3339(s).unwrap().with_timezone(&Utc);
        let j: Timestamp = s.parse().unwrap();
        assert_ne!(table_value_bytes(&c), table_value_bytes(&j));
    }
    // Full precision, no trailing zeros: coincidentally identical
    // (which is why fixture-only checks look byte-stable)
    for s in ["2000-02-29T12:34:56.123456789Z", "2024-01-15T10:30:00.123456Z"] {
        let c: DateTime<Utc> = DateTime::parse_from_rfc3339(s).unwrap().with_timezone(&Utc);
        let j: Timestamp = s.parse().unwrap();
        assert_eq!(table_value_bytes(&c), table_value_bytes(&j));
    }
}

Observed bytes, e.g.: .120Z → chrono 2024-01-15T10:30:00.120Z (25B) vs jiff ...00.12Z (24B). .500Z → chrono .500 (25B) vs jiff .5 (23B).

Conclusion: to keep the string contract you must either (a) keep chrono in the production path forever (violates policy #4; #808's choice), or (b) switch to jiff-native serde but then silently mix two formats on disk after upgrade. Neither is desirable.

Direction: normalize to primitive

Per the principle established in the #733 discussion ("surfaces that already require migration should be normalized to a primitive storage contract instead of a library-dependent string/serde representation"), migrate these surfaces to i64 epoch-nanoseconds primitive storage. This unifies with the i64-nanos contract already used by the event boundary and removes chrono from the production path entirely.

Scope

  • Target structs/fields (each struct has exactly one datetime field, creation_time):
    • Stored (on-disk, value only, not key):
      • Value.creation_time (private projected struct in src/tables/network.rs)
      • Inner.creation_time in src/tables/node.rs
    • Public API:
      • Network.creation_time, Node.creation_time
  • In-memory / public API type: chrono::DateTime<Utc>jiff::Timestamp.
  • Stored contract: RFC 3339 string → i64 epoch nanoseconds primitive.
  • Explicit data migration converting old chrono-string values to i64 nanos (read old bytes → write new bytes).
  • Reuse/share the i64-nanos Jiff serde adapter pattern established (or being established) at the event boundary. chrono is allowed only inside old-schema migration structs (parsing old strings) and is removed from the production read/write path.

Approach

  • Apply an i64-nanos Jiff serde adapter to the stored creation_time field so the value is stored as an 8-byte integer.
  • Add a data migration in src/migration.rs:
    • Read existing chrono-string values via old-schema structs, parse into jiff::Timestamp, and rewrite as i64 nanos.
    • Bump the DB format version and follow the existing migration version/range conventions.
    • Verify the actual migration path with the review-migrate binary.
  • Handle the i64-nanos representable range (~1677-09-21 to 2262-04-11) and boundary/overflow behavior consistently with the event key contract. creation_time values fall well within this range, but the adapter should keep the same range constraint explicitly.

Acceptance criteria

  • Stored creation_time for Network/Node is stored as i64 epoch-nanoseconds, and existing chrono-string bytes are converted without value loss (including nanosecond precision) by the migration.
  • Migration tests cover: empty DB migration, old DB migration (using old-string fixtures), no-op on re-run, and resulting state consistency including the version file. Run with review-migrate where practical.
  • Existing old-byte fixtures (network_projected_private_value.bin, node_inner_literal.bin, the public Node serde baseline, etc.) are used as "old byte" inputs, and new fixtures for the post-migration i64-nanos form are added.
  • The public API change (DateTime<Utc>jiff::Timestamp) is visible through compile/tests, and downstream impact is checked via e.g. review-web compile checks where feasible.
  • chrono usage is removed from production code. Any remaining chrono is limited to old-schema migration structs (parsing old values) and is explicitly justified.
  • creation_time semantics (creation instant) and precision are preserved, and existing Network/Node table behavior tests pass.

Out of scope

Notes

Activity

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

Metadata

Metadata

Assignees

Labels

blockedPending another issue

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions