fix: parse day and fractional-second components in FromTimeSpan - #435
Open
Scott-Emberson wants to merge 1 commit into
Open
fix: parse day and fractional-second components in FromTimeSpan#435Scott-Emberson wants to merge 1 commit into
Scott-Emberson wants to merge 1 commit into
Conversation
FromTimeSpan read the time span fields at fixed offsets and took the day component from timeSpan[0:0], which is always the empty string. Every value carrying a day component therefore parsed to zero, including "1.00:00:00" — the interval on the default machine policy — and any fractional seconds were dropped. An empty string panicked on a slice bound. Parse the components by separator instead. The day and fractional-second parts are both optional, and the server does not pad the day component to a fixed width, so offsets cannot be assumed. Malformed input now yields a zero duration rather than a panic. The existing tests only logged their results and asserted nothing, which is why this went unnoticed; they now assert, and every case they already covered was returning zero. Closes OctopusDeploy#434 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 3, 2026
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
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.
Fixes #434.
FromTimeSpanread the time span fields at fixed offsets and took the day component fromtimeSpan[0:0], which is always the empty string. Every value carrying a day component parsed to zero, fractional seconds were dropped, and an empty string panicked on a slice bound. Only the plainhh:mm:ssform worked.The fixed offsets also assumed a single-digit day, and the server does not pad the day component, so
"7.12:30:00"and"07.12:30:00"could not both be read correctly even with the days segment fixed.What it returned before
"1.00:00:00"is the health check interval on the default machine policy, so this sits on a common path.The change
Split on the separators rather than slicing at fixed offsets. Both uses of
.are ambiguous (d.hh:mm:ssagainsthh:mm:ss.fffffff), so a leading segment is only treated as the day component when what follows still holds a completehh:mm:ss. The fractional part is read as a decimal fraction of a second, which handles both the five-digit form this package writes and the seven-digit form .NET produces. Malformed input returns a zero duration instead of panicking.Applied to
pkg/machinepolicies/andpkg/machines/, which each carry their own copy of the function. Patching only one would leave consumers of the two packages parsing the same payload differently.Tests
pkg/machines/duration_formatter_test.gocalledFromTimeSpanseven times and logged each result without asserting anything. All seven returned0sand the test passed, which is why this went unnoticed. It now asserts, along with a round trip check overToTimeSpan, and the same file is added topkg/machinepolicies/.Verified against a 2026.x server: a policy with a seven day interval now reads back as
168h0m0sinstead of0s.ToTimeSpanis unchanged. The server accepts its zero-padded day output and normalises it on read.Impact
Any
time.Durationread back throughFromTimeSpanwas affected. InMachinePolicythat coversConnectionConnectTimeout,ConnectionRetrySleepInterval,ConnectionRetryTimeLimit,PollingRequestQueueTimeoutandPollingRequestMaximumMessageProcessingTimeout, plusMachineHealthCheckPolicy.HealthCheckIntervalandMachineCleanupPolicy.DeleteMachinesElapsedTimeSpan.Callers who were compensating for the zero values will see real durations after this. I could not find any such workaround in this repository.