fix: make order-compared TEXT timestamp columns sort by time, not by bytes - #153
Merged
Merged
Conversation
…bytes
A post-merge scripts/check.sh on main failed at
failure_surfacing::tests::notification_failure_is_truthful_and_retryable's
.expect("due"). Root cause: several tables store an instant as an RFC 3339
TEXT column and compare it with an inequality in SQL, but jiff's Timestamp
Display is not an order-preserving encoding. It trims trailing fractional
zeros and omits the fraction entirely on a whole second, and 'Z' (0x5A) sorts
after '.' (0x2E), so time order inverts inside every second:
"2026-08-07T04:22:57Z" > "2026-08-07T04:22:57.000123456Z"
Two live defects followed, each now covered by a deterministic regression:
- a dead letter whose next_attempt_at landed on an exact second was not
claimable until the next second
(a_whole_second_retry_deadline_is_claimable_one_nanosecond_later);
- fail-open: a skill-context selection token expiring on an exact second read
as still live for up to a second under expires_at > ?
(a_token_expiring_on_an_exact_second_is_refused_one_nanosecond_later).
store::sql_time::sql_timestamp renders a fixed nine-digit fraction, so byte
order and instant order coincide. Every write to, and comparison against, the
seven order-compared columns now goes through it. Versioned migration v4
normalizes existing rows; it is idempotent and skips NULLs and already-
canonical values.
consume_skill_context_selection_and_append_audit now takes now: Timestamp
rather than reading the clock internally, matching claim_due_dead_letter(now);
that is what makes the expiry boundary testable. Its one caller already had
the gate()'s own now in scope.
Recorded as D-154.
Mutation-verified: replacing sql_timestamp's body with timestamp.to_string()
fails both behavioural regressions on their intended assertions while the
...is_live_one_nanosecond_earlier control still passes.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (47)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds canonical fixed-width RFC 3339 timestamp serialization for SQLite ChangesTimestamp normalization
Estimated code review effort: 3 (Moderate) | ~25 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The failure
The post-merge
./scripts/check.shonmainafter #152 failed:claim_due_dead_letter(Timestamp::now())returnedNonefor a dead letter enqueued microseconds earlier.Root cause
Several tables store an instant as an RFC 3339
TEXTcolumn and then compare it with an inequality in SQL —next_attempt_at <= ?1,expires_at > ?5,created_at < ?1,occurred_at > ?2. SQLite comparesTEXTbytewise, so those queries are only correct if the rendering is order-preserving.jiff's
TimestampDisplayis not. It prints the smallest faithful fraction: trailing zeros trimmed, and no fractional part at all on a whole second (jiff-0.2.24/src/fmt/temporal/printer.rs:247,src/fmt/buffer.rs:527-538). Since'Z'(0x5A) sorts after'.'(0x2E), time order inverts inside every second:The left instant is earlier, yet compares greater. The suite failure is the ~1-in-10⁶ case where the clock reads an exact whole second.
Two real defects, not just a flaky test
next_attempt_atlanded on an exact second was not claimable until the next second.expires_at > ?5.Each now has a deterministic regression:
a_whole_second_retry_deadline_is_claimable_one_nanosecond_lateranda_token_expiring_on_an_exact_second_is_refused_one_nanosecond_later.Fix
store::sql_time::sql_timestamprenders RFC 3339 with a fixed nine-digit fraction, so byte order and instant order coincide. Every write to, and comparison against, the order-compared columns goes through it:notify_dead_letters.{enqueued_at, next_attempt_at, claimed_until}·task_grants.expires_at·skill_context_selections.expires_at·connector_restart_ledger.occurred_at·worker_dispatch.created_atVersioned migration v4 normalizes existing rows: idempotent, skips NULLs, and only touches the legacy 20–29-character shape, so it can neither corrupt a value nor invent an instant.
Store::consume_skill_context_selection_and_append_auditnow takesnow: Timestampinstead of reading the clock internally, matching the existingclaim_due_dead_letter(now). That is what makes the expiry boundary testable at all; its one caller already hadgate()'s ownnowin scope, so the gate decision and the expiry check use the same instant.Columns deliberately not converted —
worker_dispatch.{updated_at, failed_at, recovery_claimed_at},conversation_in_flight.claimed_at,digest_items.ts(ordered by integerseq) — are never inequality-compared.spend.rs'sutc_daysplits onTand is unaffected. Adding such a comparison later requires moving the column to the canonical renderer first.Fixed-width rendering was chosen over migrating to
INTEGERepoch nanoseconds because it preserves the stored shape, round-trips through the existingstr::parse::<Timestamp>()readers unchanged, rewrites no column, and keeps the downgrade path trivial.Recorded as D-154.
Verification
sql_timestamp's body withtimestamp.to_string()makes both behavioural regressions fail on their intended assertions, while the…is_live_one_nanosecond_earliercontrol still passes — so the tests bite on the real defect and are not trivially failing../scripts/check.sh→ exit 0 (fmt, clippy-D warnings, nextest 963 tests, file sizes, claims gate, omp-ceremony,openspec validate --all --strict45/0).sql_time.rs, 3 instore/gate_support_tests.rs, 1 infailure_surfacing/tests.rs, 1 migration test covering all three legacy renderings plus idempotency.worker_dispatch.rsandworker_result_relay.rseach had a thirdnotify_dead_letterswriter still onDisplay, which would have re-introduced the exact stall against migrated rows. Both fixed, plus its SHOULD-FIX (document the four-digit-year precondition on the migration macro) and its nit. An earlier advisory also caught three missedoccurred_atcutoffs inworker_supervision.rs. The scope/test-quality lens returned APPROVE.CI is expected to fail instantly with the GitHub Actions billing/spending-limit error; that is not a code failure.
Summary by cubic
Make TEXT timestamp comparisons order-correct by rendering RFC 3339 with a fixed 9-digit fraction. This prevents stalled retries and a fail-open expiry at whole-second boundaries.
Bug Fixes
store::sql_time::sql_timestampand used it for all inequality-compared columns (dead letters, task grants, skill selections, worker ledger, worker dispatch).next_attempt_athit an exact second.consume_skill_context_selection_and_append_auditnow takesnowso checks use a consistent time.Migration
Written for commit 377f471. Summary will update on new commits.