cylc-flow icon indicating copy to clipboard operation
cylc-flow copied to clipboard

Fix.6701.scheduler time zone change bug

Open wxtim opened this issue 7 months ago • 9 comments

Closes #6701

The first time the scheduler loads cylc.flow.wallclock the constants TIME_ZONE_STRING_LOCAL_BASIC and TIME_ZONE_STRING_LOCAL_EXTENDED are populated for the lifetime of the scheduler. This means that the time zone returned by get_time_string will remain the same, even if the time it adds to the time zone label changes time zone.

For example a scheduler started in UK winter will always return date_time.strftime(date_time_format_string) + 'Z'. If the clocks subsequently go forward, the time recorded for local time 09:00+01:00 will be 09:00Z which is equivelent to 10:00+01:00. The consequence is that the database table will record the time_submit field as nearly an hour after the time_submit_exit field.

This bug is particularly noticable in Cylc Review where the following was observed:

image

i.e. the task has submitted 22 minutes in the future!

Check List

  • [x] I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • [x] Contains logically grouped changes (else tidy your branch by rebase).
  • [x] Does not contain off-topic changes (use other PRs for other changes).
  • [x] Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • [x] Tests are included (or explain why tests are not needed).
  • [x] Changelog entry included if this is a change that can affect users
  • [x] Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • [x] If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

wxtim avatar Apr 08 '25 13:04 wxtim

N.B. will conflict with https://github.com/cylc/cylc-flow/pull/6640

MetRonnie avatar Apr 09 '25 10:04 MetRonnie

@wxtim Can you please explain the bug in the PR description

MetRonnie avatar Apr 09 '25 14:04 MetRonnie

@oliver-sanders - this is working on Linux, but on Mac seems to be failing as if I hadn't fixed it - as a Mac user can you point out any assumptions about timezones that I may have missed?

wxtim avatar Apr 16 '25 10:04 wxtim

I suspect it's an issue with the test? Maybe the setting of the time zone isn't quite right?

oliver-sanders avatar Apr 16 '25 10:04 oliver-sanders

I suspect it's an issue with the test? Maybe the setting of the time zone isn't quite right?

The timezone change in the test looks right to me. The problem does indeed seem to be the bug still happens on MacOS despite the attempt at fixing

MetRonnie avatar Apr 16 '25 13:04 MetRonnie

I suspect it's an issue with the test? Maybe the setting of the time zone isn't quite right?

The timezone change in the test looks right to me. The problem does indeed seem to be the bug still happens on MacOS despite the attempt at fixing

It is, I've somehow pushed the bug to time_submit_exit, but only on Mac. 🤮

From the downloaded db:

sqlite> SELECT * FROM task_jobs;
1|one|1|[1]|0|1|2025-04-16T14:01:56Z|2025-04-16T14:01:56+19:17|0|||||localhost|background|

wxtim avatar Apr 16 '25 14:04 wxtim

Credit to @MetRonnie for working out why Mac was behaving differently: Python on Mac doesn't appear to require time.tzset() to update the value of TZ in Python. This means that when the orginal fixture using monkeypatch.context exited from the patched context, Python returned to the original TZ, rather than remaining in the alternate.

wxtim avatar Apr 16 '25 15:04 wxtim

@hjoliver - Review Poke

wxtim avatar Apr 17 '25 13:04 wxtim

@hjoliver Review poke

wxtim avatar May 08 '25 12:05 wxtim

@hjoliver Review poke

wxtim avatar Aug 14 '25 13:08 wxtim