Skip to content

Consolidate timestamp parsing and support docker-style options - #41403

Open
ggarzia-MSFT wants to merge 5 commits into
masterfrom
user/ggarzia/network-output-parity-followup
Open

Consolidate timestamp parsing and support docker-style options#41403
ggarzia-MSFT wants to merge 5 commits into
masterfrom
user/ggarzia/network-output-parity-followup

Conversation

@ggarzia-MSFT

Copy link
Copy Markdown
Contributor

Summary of the Pull Request

Follow-up to #41377. Consolidates all timestamp and duration parsing used by wslc into wsl::windows::common::string, fixes a parser that silently swallowed errors, and broadens the --since / --until grammar to match what users expect from an OCI container CLI.

PR Checklist

  • Closes: Link to issue #xxx
  • Communication: I've discussed this with core contributors already. If work hasn't been agreed, this work might be rejected
  • Tests: Added/updated if needed and all pass
  • Localization: All end user facing strings can be localized
  • Dev docs: Added/updated if needed
  • Documentation updated: If checked, please file a pull request on our docs repo and link it here: #xxx

Detailed Description of the Pull Request / Additional comments

Silent parse failures (review feedback from #41377)

Rfc3339ToUtcDisplayTime used a TryParse-style helper and fell back to echoing the raw input when parsing failed, so a malformed daemon timestamp surfaced as a garbled display string instead of an error. It now delegates to Rfc3339ToEpoch, which throws. The TryParse variant is deleted.

Parser consolidation

SpecParsing.cpp had grown its own copies of RFC 3339 and Go-duration parsers, one of which duplicated logic already present a few hundred lines away in the same file. Everything now lives in src/windows/common/string.{hpp,cpp}:

Helper Purpose
Rfc3339ToEpoch Strict RFC 3339 → epoch seconds. Single source of validation.
ExpandToRfc3339 Canonicalizes partial/zone-less user input into strict RFC 3339.
TryParseDuration Go-style duration (10m, 1h30m, 1.5h) → std::chrono::nanoseconds.
EpochToLocalDisplayTime Epoch → local display string.
Rfc3339ToUtcDisplayTime RFC 3339 → UTC display string.

The loose CLI grammar is implemented by canonicalizing input and then handing it to the strict parser, so validation and error messages exist in exactly one place. ServiceMain.cpp also picks up Rfc3339ToEpoch, which fixes a latent bug where it used mktime (local time) on a UTC timestamp.

Broader --since / --until input

GetTimestampFromString (used only by --since / --until) now tries, in order: raw integer epoch → Go duration → Rfc3339ToEpoch(ExpandToRfc3339(...)). Newly accepted:

  • Zone-less local times — 2024-01-02T15:04:05 (interpreted in the local zone)
  • Date-only and partial times — 2024-01-02, 2024-01-02T15, 2024-01-02T15:04
  • Go durations — 10m, 1h30m, 1.5h (relative to now)
  • Pre-1970 timestamps

Still rejected: 10x, 1h30, 2024-13-01, 2024-02-31, and other malformed values. Note that std::chrono::parse silently wraps invalid calendar dates (Feb 31 → Mar 2), so Rfc3339ToEpoch pre-validates via std::chrono::year_month_day::ok().

Signed epoch plumbing

Supporting pre-1970 values required Since / Until to change from ULONGLONG to LONGLONG through wslc.idlWSLCContainerDockerHTTPClientContainerServiceContainerTasksArgumentDefinitions.h. Tail stays ULONGLONG. This touches wslc.idl, which is internal and explicitly non-ABI-stable (see the comment at the top of that file); the SDK-facing WSLCCompat.idl and the public WslPluginApi.h are untouched.

Sentinel handling

0001-01-01T00:00:00Z means "unset" in daemon output but "from the beginning of time" as CLI input. A c_unsetTimestamp constant in docker_schema.h guards the daemon-side call site; the parser itself simply returns -62135596800.

Validation Steps Performed

  • Full cmake --build . from a clean tree — 0 errors across all targets.
  • FormatSource.ps1 / clang-format 19.1.5 run over all changed files.
  • Unit tests in test/windows/wslc/WSLCCLIArgumentUnitTests.cpp extended from 3 to 7 ValidateTimestamp_* cases, covering the newly accepted forms (_ValidPreEpoch, _ValidZoneLessLocalTime, _ValidPartialAndDateOnly, _ValidGoDuration) and additional rejections (10x, 1h30) in _InvalidRfc3339_Rejected. Existing epoch literals updated from ULL to LL.

… accept docker-style --since/--until values

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings August 20, 2026 21:00
@ggarzia-MSFT
ggarzia-MSFT marked this pull request as ready for review August 20, 2026 21:04
@ggarzia-MSFT
ggarzia-MSFT requested review from a team as code owners August 20, 2026 21:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR consolidates timestamp/duration parsing used by wslc into wsl::windows::common::string, expands the --since/--until grammar to accept Docker/OCI-style inputs (partial timestamps, zone-less local times, Go durations, and pre-1970 epochs), and fixes a service-side UTC parsing bug by switching to strict RFC 3339 parsing.

Changes:

  • Centralized strict RFC 3339 parsing (Rfc3339ToEpoch) plus canonicalization (ExpandToRfc3339) and shared Go-duration parsing (TryParseDuration) in src/windows/common/string.*.
  • Switched Since/Until plumbing end-to-end from ULONGLONG to LONGLONG (IDL → session → HTTP client → tasks/services/args) to support pre-epoch timestamps.
  • Updated/expanded unit test coverage for the newly accepted/rejected timestamp forms.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
test/windows/wslc/WSLCCLIArgumentUnitTests.cpp Expands timestamp argument tests (pre-epoch, zone-less local, partial/date-only, Go durations).
src/windows/wslcsession/WSLCContainer.h Updates container logs API to use signed epochs for Since/Until.
src/windows/wslcsession/WSLCContainer.cpp Updates timestamp handling and introduces unset-timestamp guarding for daemon output.
src/windows/wslcsession/DockerHTTPClient.h Changes ContainerLogs signature to accept signed Since/Until.
src/windows/wslcsession/DockerHTTPClient.cpp Sends signed since/until query parameters when non-zero.
src/windows/wslc/tasks/ContainerTasks.cpp Uses signed since/until types for CLI logs task plumbing.
src/windows/wslc/services/ContainerService.h Updates service Logs API to take signed epochs.
src/windows/wslc/services/ContainerService.cpp Implements signed-epoch Logs forwarding to session container.
src/windows/wslc/arguments/SpecParsing.h Changes GetTimestampFromString return type to LONGLONG.
src/windows/wslc/arguments/SpecParsing.cpp Implements broader timestamp grammar via integer → duration → ExpandToRfc3339+Rfc3339ToEpoch.
src/windows/wslc/arguments/ArgumentDefinitions.h Changes Since/Until argument types to LONGLONG.
src/windows/service/inc/wslc.idl Updates IWSLCContainer::Logs to accept signed Since/Until.
src/windows/service/exe/ServiceMain.cpp Fixes update-check release date parsing by using Rfc3339ToEpoch (UTC-correct).
src/windows/inc/docker_schema.h Adds c_unsetTimestamp sentinel for daemon “unset” timestamps.
src/windows/common/string.hpp Adds new shared helpers for RFC 3339 expansion/parsing and Go-duration parsing.
src/windows/common/string.cpp Implements ExpandToRfc3339, signed Rfc3339ToEpoch, and shared TryParseDuration; tightens UTC display-time behavior.
Suppressed comments (1)

src/windows/wslcsession/WSLCContainer.cpp:2537

  • m_stateChangedAt is stored as an unsigned epoch, but Rfc3339ToEpoch() can return negative seconds. The current static_caststd::uint64_t(...) will wrap negative values and produce an incorrect (very large) timestamp instead of treating it as an invalid/unset daemon value.
            if (!timestamp.empty() && timestamp != c_unsetTimestamp)
            {
                container->m_stateChangedAt = static_cast<std::uint64_t>(wsl::windows::common::string::Rfc3339ToEpoch(timestamp));
            }

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/windows/wslcsession/WSLCContainer.cpp
Comment thread test/windows/wslc/WSLCCLIArgumentUnitTests.cpp
Copilot AI review requested due to automatic review settings August 20, 2026 21:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Suppressed comments (3)

test/windows/wslc/WSLCCLIArgumentUnitTests.cpp:782

  • This test calls std::chrono::current_zone() without handling the case where the time zone database is unavailable (current_zone() can throw). The production code falls back to UTC in that case; the test should mirror that fallback to avoid spurious failures.
        const auto offset = std::chrono::duration_cast<std::chrono::seconds>(
                                std::chrono::current_zone()->get_info(std::chrono::system_clock::now()).offset)
                                .count();
        const auto expected = 1705314600LL - offset;

src/windows/common/string.hpp:75

  • The ExpandToRfc3339 comment says unrecognized input is returned unchanged, but the implementation always constructs an expanded string (e.g. "abc" becomes "abcT00:00:00+00:00"). Update the comment (or the function) so the contract matches reality.
// Expands a partial timestamp into a full RFC 3339 one. Hour-only, minute-only and date-only values
// are padded out to a complete time, and a value with no zone designator is resolved against the
// offset currently in effect locally. Input that is not recognized is returned for the parser to
// reject.

test/windows/wslc/WSLCCLIArgumentUnitTests.cpp:811

  • ValidateTimestamp_ValidGoDuration captures "now" once and then allows only a fixed +30s window for all parses. If the test is paused (debugger) or runs slowly under load, it can fail even when parsing is correct. Capture time before/after each parse and assert the parsed value falls within that interval.
        const auto now = std::chrono::floor<std::chrono::seconds>(std::chrono::system_clock::now().time_since_epoch()).count();

        // Durations are measured back from the current time, so allow a small window for the clock
        // to advance while the test runs.
        auto verifyDuration = [&](LPCWSTR value, LONGLONG expectedOffset) {
            const auto parsed = validation::GetTimestampFromString(value);
            VERIFY_IS_GREATER_THAN_OR_EQUAL(parsed, now - expectedOffset);
            VERIFY_IS_LESS_THAN_OR_EQUAL(parsed, now - expectedOffset + 30);
        };

Comment thread src/windows/common/string.hpp Outdated
…iner timestamps signed

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 20, 2026 21:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 26 out of 26 changed files in this pull request and generated no new comments.

Suppressed comments (2)

test/windows/wslc/WSLCCLIArgumentUnitTests.cpp:781

  • This test assumes std::chrono::current_zone() is always available. Production code (LocalUtcOffset/EpochToLocalDisplayTime) explicitly catches time-zone database failures and falls back to UTC, so this test can become flaky in environments where the time-zone DB is unavailable. Consider mirroring the production fallback by treating the offset as 0 on exception.
        const auto offset = std::chrono::duration_cast<std::chrono::seconds>(
                                std::chrono::current_zone()->get_info(std::chrono::system_clock::now()).offset)
                                .count();

src/windows/common/string.cpp:449

  • LocalUtcOffset() uses the current offset (current_zone()->get_info(system_clock::now())). When ExpandToRfc3339() canonicalizes a zone-less timestamp, this can produce incorrect epoch values for timestamps that fall under a different DST offset than “now” (e.g., parsing a winter date during summer time). Consider resolving zone-less inputs using the offset rules at the parsed local timestamp (and handling ambiguous/nonexistent local times) rather than the current offset.
static std::string LocalUtcOffset()
{
    try
    {
        const auto* zone = std::chrono::current_zone();
        const auto offset = zone->get_info(std::chrono::system_clock::now()).offset;
        const auto minutes = std::chrono::duration_cast<std::chrono::minutes>(offset).count();
        const auto magnitude = std::abs(minutes);

        return std::format("{}{:02}:{:02}", minutes < 0 ? '-' : '+', magnitude / 60, magnitude % 60);
    }
    catch (...)
    {
        // The time zone database is unavailable, so fall back to UTC rather than failing the caller.
        LOG_CAUGHT_EXCEPTION();
        return "+00:00";
    }

… module

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 20, 2026 22:17

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated no new comments.

Suppressed comments (4)

test/windows/wslc/WSLCCLIArgumentUnitTests.cpp:781

  • This test calls std::chrono::current_zone() directly, which can throw when the time zone database is unavailable. The production code explicitly falls back to UTC in that case, so the test should handle the same fallback to avoid spurious failures in constrained environments.
        const auto offset = std::chrono::duration_cast<std::chrono::seconds>(
                                std::chrono::current_zone()->get_info(std::chrono::system_clock::now()).offset)
                                .count();

src/windows/common/timestamp.cpp:149

  • Rfc3339ToEpoch converts the parsed time_point to seconds with duration_cast, which truncates toward zero for negative durations. For pre-1970 timestamps with fractional seconds (e.g. 1969-12-31T23:59:59.9Z), this can return 0 instead of -1. Use std::chrono::floor<std::chrono::seconds> to ensure correct flooring for negative values.
    return std::chrono::duration_cast<std::chrono::seconds>(parsed.time_since_epoch()).count();

src/windows/wslc/arguments/SpecParsing.cpp:625

  • Catching ... here will also convert unrelated failures (e.g. allocation failures) into an ArgumentException, which can hide real errors. Limit the catch to the parse failure type / error code (e.g. wil::ResultException with E_INVALIDARG) and rethrow anything else.
    // Name the offending argument rather than surfacing the raw parse failure.
    catch (...)
    {
        throw ArgumentException(Localization::WSLCCLI_InvalidTimestampArgumentError(argName, value));
    }

test/windows/wslc/WSLCCLIArgumentUnitTests.cpp:773

  • Pre-epoch support is newly introduced, but this test set doesn't cover the pre-1970 + fractional-seconds case. Adding one assertion around the epoch boundary helps prevent regressions in the seconds-rounding behavior for negative timestamps.
    TEST_METHOD(ValidateTimestamp_ValidPreEpoch)
    {
        // No lower bound is applied, so pre-1970 values convert to a negative epoch.
        VERIFY_ARE_EQUAL(validation::GetTimestampFromString(L"1960-01-15T10:30:00Z"), -315610200LL);
        VERIFY_ARE_EQUAL(validation::GetTimestampFromString(L"0001-01-01T00:00:00Z"), -62135596800LL);
        VERIFY_ARE_EQUAL(validation::GetTimestampFromString(L"-315610200"), -315610200LL);
    }

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 20, 2026 23:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 32 out of 32 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/windows/common/timestamp.hpp:19

  • timestamp.hpp uses std::string_view and std::int64_t but does not include <string_view> / <cstdint>. This makes the header non-self-contained and can break compilation when it’s included without relying on precompiled-header include order.
#include <chrono>
#include <optional>
#include <string>

src/windows/common/timestamp.cpp:34

  • Zone-less timestamps are expanded using the current local UTC offset (current_zone()->get_info(system_clock::now())). This will produce incorrect epoch values for inputs whose local offset differs from “now” (e.g., timestamps across DST transitions). Consider computing the offset for the parsed local date/time (or parsing as local_time and converting via zoned_time) instead of appending a fixed offset.
        const auto* zone = std::chrono::current_zone();
        const auto offset = zone->get_info(std::chrono::system_clock::now()).offset;
        const auto minutes = std::chrono::duration_cast<std::chrono::minutes>(offset).count();
        const auto magnitude = std::abs(minutes);

        return std::format("{}{:02}:{:02}", minutes < 0 ? '-' : '+', magnitude / 60, magnitude % 60);

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.

3 participants