Skip to content

fix(array): check the audit_retain_ms conversion instead of casting - #234

Merged
farhan-syah merged 1 commit into
mainfrom
fix/array-retention-checked-conversion
Aug 1, 2026
Merged

fix(array): check the audit_retain_ms conversion instead of casting#234
farhan-syah merged 1 commit into
mainfrom
fix/array-retention-checked-conversion

Conversation

@farhan-syah

Copy link
Copy Markdown
Member

Summary

CREATE ARRAY carried audit_retain_ms from the parser as u64 and stored it signed with a raw cast:

audit_retain_ms: audit_retain_ms.map(|ms| ms as i64),

It is stored signed because downstream it is subtracted from a wall-clock millisecond to place the purge horizon (now_ms - audit_retain_ms, in engine/array/retention.rs). A value above i64::MAX wraps negative there, which puts the horizon in the future — and a retention pass with a future horizon purges every tile version the array has. That is the exact inverse of what a retention setting asks for, and it would happen silently.

This is not reachable today. Both parsers that produce the value reject a negative (WITH (audit_retain_ms = ...): must be >= 0, and the same on ALTER ... SET) and expect_int yields i64, so every value arrives inside 0..=i64::MAX. The cast cannot currently wrap.

What it removes is the dependence on that. The invariant is established in two parsers and then re-laundered by as at each hop; an entry point added later — a sync path, a catalog restore, a programmatic DDL API — that does not repeat the range check would produce a negative retention with nothing between it and a full purge. The conversion is now checked where the value is stored, so such a path fails with a plan error instead.

The companion change in nodedb-lite (789122b, on main) does the same thing on the embedded side, where the DDL value was being dropped entirely before it reached the engine.

Validation

  • cargo nextest run -p nodedb --all-features -E 'test(array)' — 243/243
  • cargo nextest run -p nodedb --all-features — 7940/7942
  • cargo clippy --all-targets -p nodedb -- -D warnings — clean
  • cargo fmt --all — clean

The two failures are crash_wal_truncation::kv_row_survives_wal_segment_truncation and shutdown_abort_offender::offender_task_aborted_at_500ms_budget. Both are timing-bounded and both pass standalone — 20 s → 6.1 s and 60 s → 1.6 s — so they are timeouts under full-suite parallel load. Neither test creates an array, so neither can reach this code path. The same suite ran 7942/7942 on the immediately preceding commit.

Replace the `as i64` cast on CREATE ARRAY's audit_retain_ms with a
checked try_from that errors out on overflow, since a silently
wrapped negative value would place the purge horizon in the future
and purge every tile version.
@farhan-syah
farhan-syah merged commit f683e32 into main Aug 1, 2026
4 checks passed
@farhan-syah
farhan-syah deleted the fix/array-retention-checked-conversion branch August 1, 2026 23:12
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.

1 participant