fix(models): reject non-finite and out-of-range durations instead of saturating

[ErikBjare]

Follow-up to #750, addressing this review comment: #750 (comment)

seconds_to_nanos (the canonical seconds-to-Duration conversion since #750) cast with as i64. That cast silently saturates to i64::MAX/MIN beyond about ±292 years, and turns NaN into 0. So a duration of 1e12 s (about 31 700 years) was stored as about 292 years without any error. This isn't a regression: the truncating cast before #750 did the same.

Changes

• seconds_to_nanos returns Option<i64>, with None for NaN, ±inf, and results outside the i64 range. The range check runs on the rounded f64 before the cast. i64::MIN (-2^63) is exactly representable and allowed. i64::MAX is not representable: its nearest f64 is 2^63, which is exactly what a duration of i64::MAX ns serializes as. So 2^63 is accepted and maps to i64::MAX, and every representable duration round-trips through its JSON (found by Greptile and Codex in review). Anything above 2^63 is rejected.
• Deserialization: an out-of-range event duration is now a serde error (duration ... s is not finite or out of range) instead of corrupted data. The #[serde(remote)] derive can only convert through an infallible From, so DurationSerialization now provides serialize/deserialize functions for #[serde(with = ...)]. They go through a private newtype with the same name, so the wire format is unchanged (a plain number in JSON), and the default for a missing duration still applies.
• Other callers:
  • Legacy importer: skips such events with a warning, like it does for events with unparseable data.
  • flood() query function: rejects such a pulsetime, like a negative one. It had its own saturating cast since fix(transform): port aw-core's flood() so both servers agree #748. The sign is checked on the input, so tiny negative values that round to 0 ns are still rejected.
  • sum_durations: uses checked_add and returns a query error on overflow instead of panicking. The overflow limit is chrono::Duration's range (about 292 million years), so it's theoretical, but it's no longer a panic.
• aw-transform's heartbeat still casts its pulsetime with as, which saturates. I left it alone: its signature returns Option<Event>, and a saturated pulsetime there just means "always merge", not corrupted data.

Tests

• seconds_to_nanos: normal values are unchanged (including 515.968 from Durations lose a nanosecond on parse, so sum_durations comes out 1 ms low and touching periods don't merge #745, -2.25, -0.0 and 1.5e-9). Also 1e12, -1e12, NaN and ±inf. At the exact boundaries, -2^63 ns converts to i64::MIN, 2^63 ns to i64::MAX, and the next f64s beyond either limit are rejected. 9e9 s (about 285 years) is still accepted.
• Round-trip: 4000 durations near i64::MAX and i64::MIN serialize and deserialize again (within a few f64 ulps).
• Deserializing: 1e12, -1e12 and 1e400 durations are errors. Integer and missing durations still work, and 515.968 still round-trips exactly.
• Query: flood(events, 1000000000000) and flood(events, 0 - 0.0000000001) are rejected like a negative pulsetime.

cargo test, cargo clippy -- -D warnings and cargo fmt --check pass locally. master currently doesn't build (toml missing in aw-client-rust since #740), so CI here stays red until #752 is merged. On the pre-#740 base, the whole workspace passes.

[ErikBjare]

@greptileai review

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-26T19:31:45.756916Z	90fa054	New commits

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[greptile-apps]

Confidence Score: 5/5

[Medium risk] Tightens validation for duration values in queries and imports.

The PR appears safe to merge; both previous findings are fixed and no new actionable issue was identified in the PR changes.

Summary

The PR rejects non-finite and out-of-range event durations, skips invalid durations during legacy import, and returns query errors for invalid flood pulsetimes or overflowing duration sums. The latest revision preserves round-tripping at the maximum duration and restores rejection of tiny negative pulsetimes.

_{Reviews (2) · Last reviewed commit: "fix(models): keep maximum durations roun..."}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c190b66e7e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[ErikBjare]

@greptileai review

[TimeToBuildBob]

🤖 AI code review

This PR changes duration handling in aw-models, aw-query, and aw-datastore. It makes seconds_to_nanos return Option<i64> instead of i64, rejecting non-finite and out-of-range values. It replaces the serde remote derive with manual serialize/deserialize functions for DurationSerialization, so deserializing an out-of-range duration becomes a serde error. It updates the legacy importer to skip events with invalid durations, the flood query function to reject invalid pulsetimes, and sum_durations to use checked_add to avoid overflow panics. Tests are added for the new behavior.

Safe to merge — no P0/P1 findings

Confidence 5/5

✅ No thread-worthy findings. Advisory notes follow; they are retained without opening review threads.

3 advisory findings (summary-only, not scored)

These P2 guard, heuristic, trade-off, or documentation claims are retained for judgment without opening review threads.

⚠️ P2 medium — aw-models/src/duration.rs:34

The new deserialize function for DurationSerialization uses Seconds::deserialize(deserializer)? which deserializes an f64. However, the Seconds struct is a newtype with #[serde(rename = "DurationSerialization")]. This rename attribute is only relevant for struct serialization, not for the inner f64. The wire format is a plain number, so this is fine. But there is a subtle issue: the Seconds struct is private and only used internally. The DurationSerialization struct is public but has no fields. The serialize and deserialize functions are associated functions, not trait implementations. The #[serde(with = "DurationSerialization")] attribute expects a module or a type with serialize and deserialize functions. This is satisfied. However, the Seconds struct derives Deserialize and Serialize, and the rename attribute is unnecessary but harmless. The real issue is that the deserialize function does not handle the case where the JSON value is an integer, but serde_json will deserialize an integer as f64, so that's fine. The test test_deserialize_duration_integer_and_default passes. I don't see a bug here.

_{How this was verified: Checked the serde with attribute usage and the test cases. The deserialize function correctly parses f64 from JSON numbers.}

⚠️ P2 medium — aw-models/src/duration.rs:162

The test test_duration_round_trip_near_limits in aw-models/src/duration.rs uses a tolerance of 4096 nanoseconds for the round-trip difference. However, the comment says that near the limits, one ulp is 1024-2048 ns, and converting to seconds and back rounds a couple of times. The tolerance of 4096 is generous, but the test might pass even if the round-trip is off by more than that? Actually, the test asserts that the diff is <= 4096, so it would fail if the round-trip is off by more. But is the tolerance correct? For i64::MAX, the f64 representation is 2^63, and converting to seconds and back gives 2^63 ns, which is i64::MAX after saturation. The diff is 1 ns. For i64::MIN, it's exact. For values near the limit, the f64 precision is 2048 ns, so the round-trip error could be up to 2048 ns. The tolerance of 4096 is safe. I don't see a bug.

_{How this was verified: Checked the f64 precision and the test values. The tolerance is appropriate.}

⚠️ P2 medium — aw-datastore/src/legacy_import.rs:134

In aw-datastore/src/legacy_import.rs, the change skips events with invalid durations. However, the warning message uses duration_float which is an f64. For NaN or infinity, the format will print 'NaN' or 'inf', which is fine. But the continue statement skips the event, which means the event is silently dropped from the import. This is a behavior change: previously, such events would have been imported with a saturated duration. Now they are dropped. This could lead to data loss if the legacy database contains events with out-of-range durations. However, the PR description says this is intentional, to avoid corrupting data. The alternative would be to clamp or error out. But the change is consistent with the handling of unparseable data. I don't see a bug, but it's a trade-off.

_{How this was verified: Checked the surrounding code and the PR description. The skip is intentional.}

Files changed (4) — the diff as I read it

• aw-datastore/src/legacy_import.rs — Skips events with invalid durations during legacy import, logging a warning instead of saturating.
• aw-models/src/duration.rs — Changes seconds_to_nanos to return Option<i64>, adds manual serialize/deserialize for DurationSerialization, and adds tests for range and round-trip behavior.
• aw-query/src/functions.rs — Updates flood to reject invalid pulsetimes via seconds_to_nanos, and sum_durations to use checked_add and return an error on overflow.
• aw-query/tests/query.rs — Adds test cases for flood rejecting negative and out-of-range pulsetimes.

Previous review passes

commit	score	findings	engine	when
8e4f541c5879	3/5	1	llm	2026-09-26 19:03 UTC

_{Reviewed 90fa05443a80 · openrouter/deepseek/deepseek-v4-flash-0731 · llm engine · 64s · about this reviewer}

_{Maintainer commands}

_{@TimeToBuildBob review (own line) — fresh review · @TimeToBuildBob fix — a worker acts on the findings. Once per comment; 👀 = received.}

[TimeToBuildBob]

❌ P1 — The range check in seconds_to_nanos accepts 2^63 ns (the f64 nearest to i64::MAX) and casts it to i64::MAX, but the comment and tests claim that 'the next f64 above it is out of range'. However, the check nanos <= i64::MAX as f64 uses i64::MAX as f64 which is 2^63 (since i64::MAX is not representable). This means any f64 in the range (2^63 - 1024, 2^63] is accepted and cast to i64::MAX, but f64 values just below 2^63 that are not exactly representable as i64 (e.g., 2^63 - 1) will be cast to i64::MAX as well, losing precision. More importantly, the test let below = f64::from_bits((i64::MAX as f64).to_bits() - 1); assert_eq!(seconds_to_nanos(below / 1e9), Some(below as i64)); expects below as i64 to be the result, but below is an f64 that is one ulp below 2^63, and casting it to i64 saturates to i64::MAX, so the assertion passes vacuously. The actual bug is that durations between 2^63 - 1024 and 2^63 ns (which are not representable as i64) are silently converted to i64::MAX, which is a different duration. This violates the PR's stated goal of rejecting out-of-range durations instead of saturating. The fix is to reject any value > i64::MAX as f64 (i.e., > 2^63 - 1024) or to use a more precise check that ensures the rounded value is exactly representable as i64.