Skip to content

[core] Fix FormatTimeSys rendering the same timestamp inconsistently (#3225) - #3342

Open
steps-re wants to merge 3 commits into
Haivision:masterfrom
steps-re:fix/formattimesys-3225
Open

[core] Fix FormatTimeSys rendering the same timestamp inconsistently (#3225)#3342
steps-re wants to merge 3 commits into
Haivision:masterfrom
steps-re:fix/formattimesys-3225

Conversation

@steps-re

@steps-re steps-re commented Jul 7, 2026

Copy link
Copy Markdown

Fixes #3225.

Bug

FormatTimeSys() renders a steady_clock timestamp as wall-clock time, but it combined whole-second wall time from ::time() with the sub-second part of steady_clock::now(). Those clocks have unrelated sub-second phases (steady counts from an arbitrary epoch), so near a second boundary the same timestamp rendered one second apart between calls — the ±1s discrepancy in the issue. Per-call re-sampling of two clocks non-atomically also adds ~1µs jitter.

Fix

Map the timestamp through a single steady↔wall reference pair, captured once at load time, in one consistent microsecond domain. A given timestamp now always formats to exactly the same string. The arithmetic is factored into a pure, testable overload FormatTimeSys(target_us, steady_now_us, wall_now_us). This touches only the [SYST] logging/trace formatter — not the transport path.

Tradeoff (for maintainer review)

This caches the steady↔wall offset for stability. If the wall clock is NTP-stepped mid-run, [SYST] times drift by the step amount but stay internally consistent. The alternative — live re-tracking with only the flicker removed — is also reasonable; happy to switch to that if you prefer. Flagging since it's your issue and touches a utility.

Verification

  • New Sync.FormatTimeSysStable sweeps the reference across misaligned steady/wall second boundaries and asserts a fixed timestamp renders identically — fails on the old arithmetic, passes with the fix.
  • Full unit suite: 271/271 pass. --gtest_repeat=200 on Sync.FormatTime* → 200/200 (also removes a latent flake in the existing back-to-back-equality test).
  • Built with -DENABLE_UNITTESTS=ON, no new warnings. DCO sign-off included.

FormatTimeSys mapped a steady-clock timestamp onto wall-clock time by
combining whole-second wall time from ::time() with the sub-second part
of steady_clock::now(). The two clocks have unrelated sub-second phases
(steady counts from an arbitrary epoch such as boot), so near a second
boundary the very same timestamp could render one second apart between
calls (issue Haivision#3225).

Map the timestamp using a single steady<->wall reference pair captured
once at load time, in a consistent microsecond domain. This removes the
+/-1 s flicker and also the ~1 us jitter that per-call re-sampling would
cause, so a given timestamp always formats to exactly the same string.

The mapping arithmetic is factored into a pure overload
FormatTimeSys(target_us, steady_now_us, wall_now_us) so it can be unit
tested deterministically. Adds a regression test (Sync.FormatTimeSysStable)
that sweeps the "now" reference across steady/wall second boundaries with
misaligned phases and asserts the output is stable.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Mike German <mike@stepsventures.com>
@ethouris

Copy link
Copy Markdown
Collaborator

That's a very good solution of the problem, but I don't like the way how the code is shaped.

If the goal of providing a second FormatTimeSys function was to allow exposure of the raw values for the sake of the UT, this should be done more direct way. Sure, there's no other (reasonable) way to expose this for the UT than to place it in the header file, but this should be definitely presented as an internal version, not intended for any other use. So, in short, the sync.h file should contain

std::string FormatTimeSys(const steady_clock::time_point& time);

// Exposed for testing purposes only
struct SysClockReference;
std::string FormatTimeSysInternal(const steady_clock::time_point& time, const SysClockReference& rf);

Then the normal FormatTimeSys function should then pass the parameters directly, while the value of SysClockReference should be created as a static local variable - this is the only acceptable form of a C++ singleton (safe against threads and global variable initialization ordering).

It might be also worth a shot to split the implementation of FormatTimeSysInternal into the part that does all these time value conversion in order to produce target_wall_us, and the other that formats this value into text.

Replace the second FormatTimeSys overload with an explicitly internal
API, as requested in review:

- sync.h now declares SysClockReference (a steady<->wall clock sample
  pair) and FormatTimeSysInternal(time_point, SysClockReference), both
  marked as exposed for testing purposes only.
- The public FormatTimeSys() keeps its single signature and obtains the
  reference as a function-local static (thread-safe, initialized on
  first use), then delegates to FormatTimeSysInternal.
- The implementation is split into ToSysTimeMicroseconds (steady->wall
  mapping) and FormatSysTimeMicroseconds (text formatting).
- The regression test now drives FormatTimeSysInternal with explicit
  SysClockReference values instead of raw int64_t triples.

Signed-off-by: Mike German <mike@stepsventures.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@steps-re

Copy link
Copy Markdown
Author

reshaped as you suggested. SysClockReference and FormatTimeSysInternal now sit in the header under an "exposed for testing purposes only" banner, and the public FormatTimeSys keeps its single signature with the reference as a static local. also took your optional suggestion and split the internal impl into the steady-to-wall conversion and the text formatting. full suite passes locally, 276/276, and the regression test now goes through FormatTimeSysInternal.

@steps-re

steps-re commented Aug 6, 2026

Copy link
Copy Markdown
Author

@ethouris bump when you get a chance. the reshape from your last review went in on the 28th and CI is green. happy to keep iterating if the shape still isn't what you had in mind.

@ethouris

Copy link
Copy Markdown
Collaborator

Sorry, we are a bit in a hot water now, hence the delay.

Looks very good; if you can merge the latest master to it I'll be appreciated. I need to see @cl-ment to finally look into it, but I think it can be assigned now to the next release.

@ethouris ethouris added this to the v1.5.7 milestone Aug 11, 2026
@ethouris ethouris added Type: Bug Indicates an unexpected problem or unintended behavior [core] Area: Changes in SRT library core [tests] Area: Unit tests [dev] Changes are development-oriented only (no functionality changes). labels Aug 11, 2026
@steps-re

Copy link
Copy Markdown
Author

merged latest master in, no conflicts, and it does not touch anything our diff does.

built locally with unit tests on, full suite is 210/210 green including Sync.FormatTimeSysStable. no rush on the review, whenever your side settles down.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[core] Area: Changes in SRT library core [dev] Changes are development-oriented only (no functionality changes). [tests] Area: Unit tests Type: Bug Indicates an unexpected problem or unintended behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] FormatTimeSys happens to work incorrectly.

2 participants