fix(mcp): preserve trigger_at offset across schedule repeat advancement#959
Conversation
next_trigger_at parsed trigger_at via parse::<DateTime<Utc>>, normalizing to UTC and discarding the original offset; advancement then re-serialized with to_rfc3339 on the Utc value, rendering +00:00 regardless of the caller's offset. Capture the original offset and re-apply it when rendering the advanced timestamp, on both the normal-fire advance path and the missed-event re-arm path. Fixes #792 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rain The repeat drain parsed trigger_at with strict DateTime::parse_from_rfc3339, but the schedule write boundary (khive-pack-schedule) validates with the relaxed at.parse::<DateTime<Utc>>() grammar and stores the original string verbatim. Already-persisted relaxed timestamps (space instead of T, offset without a colon) were silently skipped in the drain and never fired. Parse with trigger_at_str.parse::<DateTime<FixedOffset>>() instead — same grammar family as the write boundary — keeping the existing with_timezone(&Utc) and offset() handling. Adds a drain-level regression test using a relaxed legacy timestamp with a non-UTC offset. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ky tx-age sweep run_writer_task registers a writer_task_tx handle in the process-wide tx_registry singleton; the checkpoint tx_age_sweep_* tests read that singleton under #[serial(tx_registry)]. The writer_task.rs tests that spawn a writer task were missing from the group, leaking a longer-lived writer_task_tx into the sweep's oldest() read and intermittently failing the assertion on main CI. Join them to the serial group. No prod change.
|
Codex static review: APPROVE at ecfaf11 (0 blocking findings). |
oceanwaves630
left a comment
oceanwaves630
left a comment
There was a problem hiding this comment.
Independent full-diff read at head ecfaf11. Approving.
The offset-preservation fix (pending_events.rs) is correct and complete:
-
It now parses
trigger_atasDateTime<FixedOffset>via the relaxedFromStrgrammar — the same grammar the write boundary validates with — rather than the strictDateTime::parse_from_rfc3339. That choice matters: a legacy stored timestamp using a space separator and a colonless offset (... 09:00:00+0400) still parses and advances instead of being silently skipped forever as unparseable. The UTC instant used for every due-ness comparison and repeat computation is then derived viawith_timezone(&Utc), so the comparison/arithmetic semantics are identical to the previousparse::<DateTime<Utc>>(); only the serialization of the advanced occurrence changes. -
It captures the original offset once and renders both advancement sites at that offset. I grepped the full production body to confirm those two sites (the missed-path re-arm and the normal fire-and-advance) are the complete set of advanced-
trigger_atserialization points — there is no third site left rendering bare UTC.fired_atcorrectly stays UTC (it is an event timestamp, not a schedule offset to preserve), and the SQL due-ness predicates normalize throughdatetime(), so they remain offset-agnostic. -
Three targeted tests assert the fix across the paths that matter: a daily advance preserves the +04:00 offset and the same local wall-clock hour while adding exactly one day of instant; a relaxed legacy-grammar (
space++0400) timestamp is recognized as due and advanced (not skipped) with its offset preserved; and the missed-repeat re-arm path preserves the offset too. The unparseable fall-through stillcontinues exactly as before, so the fail-safe is unchanged.
The remaining files are test-hardening (serial-tagging the tx_registry race, a --no-embed backend-routing fixture, and a behavior-preserving runtime-setup test seam that delegates via a no-op closure) with no new production behavior. CI is green.
Surgical, complete, well-tested. Approving.
2 participants
Fixes #792. Root cause in the commit message. Regression test with a non-UTC offset included. Local gates deferred to CI due to build-lane contention; codex review follows.