fix: parse day and fractional-second components in FromTimeSpan (changes parsed values)

[Scott-Emberson]

Fixes #434.

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 parsed to zero, fractional seconds were dropped, and an empty string panicked on a slice bound. Only the plain hh:mm:ss form 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

FromTimeSpan("00:00:00")       = 0s          correct
FromTimeSpan("01:00:00")       = 1h0m0s      correct
FromTimeSpan("1.00:00:00")     = 0s          want 24h
FromTimeSpan("02.00:00:00")    = 0s          want 48h
FromTimeSpan("7.12:30:00")     = 12h30m0s    want 180h30m
FromTimeSpan("37500.00:00:00") = 50h0m0s     want 900000h
FromTimeSpan("00:00:00.50000") = 0s          want 500ms
FromTimeSpan("")               = panic: slice bounds out of range [:4] with length 0

"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:ss against hh:mm:ss.fffffff), so a leading segment is only treated as the day component when what follows still holds a complete hh: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/ and pkg/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.go called FromTimeSpan seven times and logged each result without asserting anything. All seven returned 0s and the test passed, which is why this went unnoticed. It now asserts, along with a round trip check over ToTimeSpan, and the same file is added to pkg/machinepolicies/.

Verified against a 2026.x server: a policy with a seven day interval now reads back as 168h0m0s instead of 0s.

ToTimeSpan is unchanged. The server accepts its zero-padded day output and normalises it on read.

Impact

Any time.Duration read back through FromTimeSpan was affected. In MachinePolicy that covers ConnectionConnectTimeout, ConnectionRetrySleepInterval, ConnectionRetryTimeLimit, PollingRequestQueueTimeout and PollingRequestMaximumMessageProcessingTimeout, plus MachineHealthCheckPolicy.HealthCheckInterval and MachineCleanupPolicy.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.

Upgrade note (please carry into the release notes)

This changes values consumers already read. Any day-scale duration on the fields above that previously came back as 0s will start coming back as its true value. In particular, tools that diff read state against stored state (such as the Terraform provider) may show a one-off diff on machine policy timeouts after upgrading. That diff is the correction, not drift, but it will generate questions if the release does not call it out.

For whoever tags the release: goreleaser builds notes from commit subjects only, so this needs a manual note on the GitHub Release. A ready-to-paste version is in #435 (comment). The PR title already carries the (changes parsed values) marker so the default squash subject signals it.

Out of scope, tracked separately: negative time spans are mishandled in both FromTimeSpan and ToTimeSpan on both sides of this change. See #447 for details.

[NickJosevski]

Thanks for this, and for splitting it out of #436 so it can land on its own.

I checked the behaviour against main at 1b925bc rather than reading the diff, by running the current parser and yours over the same inputs:

input	correct	main today	this PR
00:05:00	5m0s	5m0s	5m0s
01:30:00	1h30m0s	1h30m0s	1h30m0s
1.00:00:00	24h0m0s	0s	24h0m0s
7.12:30:00	180h30m0s	12h30m0s	180h30m0s
07.12:30:00	180h30m0s	0s	180h30m0s
10.00:00:00	240h0m0s	0s	240h0m0s
00:00:00.5000000	500ms	0s	500ms
1.02:03:04.5000000	26h3m4.5s	2h3m4s	26h3m4.5s
""	0s	panic	0s

Every day-bearing value was silently wrong rather than failing loudly, which is the worst shape for a bug like this — a 10-day health check interval reads back as 0s, and 7.12:30:00 is out by a factor of 14. The plain hh:mm:ss cases are identical before and after, so nothing that works today changes.

Worth noting for anyone reviewing the blast radius: ToTimeSpan is untouched, and all 14 in-repo callers of FromTimeSpan are inside UnmarshalJSON on machine policy, cleanup policy, and health check policy. So this only affects what the SDK reads back, never what it sends. It also closes a quiet data-loss path — today you can read a policy, write it back unchanged, and send a day-scale interval out as zero.

Two notes, neither blocking:

1. Negative time spans are still wrong, in both directions. Not a regression and I don't think it belongs in this PR, but recording it so it isn't rediscovered later:

FromTimeSpan("-1.02:03:04") = -21h56m56s   // .NET means -26h3m4s; the sign only reaches the days field
FromTimeSpan("-00:05:00")   = 5m0s         // sign dropped entirely
ToTimeSpan(-26h3m4s)        = "-1.-2:-3:-4"

That last line is existing code this PR doesn't touch, which is a decent argument that negative spans were never supported on either side. Happy for it to stay out of scope — a follow-up issue seems right if we care.

2. This changes a value consumers already read. Anyone who has been getting 0s for these fields starts getting the true value, so a Terraform plan may show a one-off diff on machine policy timeouts after upgrading. That diff is the correction, but it'll generate questions if it ships unannounced, so I'd like the release to call it out.

One process thing before merge: this is a fork PR, so the Go test workflow has never actually run — the only green checks are CLA and GitGuardian. I'll approve the run so we have CI on the record. Locally it builds clean, vets clean, and the formatter tests pass in both pkg/machines and pkg/machinepolicies.

[Scott-Emberson]

On the CI run you approved: the Integration Tests job failed, but no Go tests executed. It died in the "Initialize containers" step, before the test job started. The Octopus Server container could not log into SQL Server (Login failed for user 'sa', retried 60 times over a minute), which looks like the usual fork-PR shape where repository secrets are not exposed to workflow runs from forks. CodeQL, Skip Tests, CLA, and GitGuardian are all green. Happy to have it re-run if you think it was transient, or to lean on the local evidence you already recorded (build, vet, and formatter tests in both packages).

On your two notes:

1. Negative time spans: filed as Negative TimeSpan values are mishandled by FromTimeSpan and ToTimeSpan #447 with your repro and the ToTimeSpan output preserved, so it does not get rediscovered. Agreed it stays out of scope here.

2. Release callout: added an "Upgrade note" section to the PR description covering the read-back change and the one-off Terraform diff, worded so it can be lifted straight into the release notes.

[NickJosevski]

Two follow-ups: one for @Scott-Emberson, one for whoever merges and tags this.

The red test check is not your change

I approved the workflow run as promised, and it failed. It isn't yours, and there is nothing you can do about it from a fork.

integration-tests.yml stands up SQL Server and an Octopus Server container using secrets.DB_IMAGE_SA_PASSWORD, OD_IMAGE_ADMIN_API_KEY and OCTOPUS_SERVER_BASE64_LICENSE. GitHub does not pass repository secrets to pull_request runs originating from a fork, so the SA password arrives empty, SQL Server rejects the login, and the job dies before any Go runs:

Testing connection to the 'master' database...
Login failed for user 'sa'.
##[error]Failed to initialize container octopusdeploy/octopusdeploy
##[error]One or more containers failed to start.

Every fork PR on this repo hits this. Build and vet, which run before the containers are needed, both passed.

(Related: the green test check next to it comes from skip-test.yml, whose body is echo "No build required". It filters on path: rather than paths:, which is not a valid key, so it runs on every PR and is always green. Two checks named test, one meaningless. Raising that separately.)

To get a real integration run, a maintainer can push this branch into the main repo — maintainerCanModify is true — and let CI execute with secrets.

For whoever merges and tags: this release needs a manual note

There is no CHANGELOG.md here and no release automation beyond goreleaser, which builds release notes purely from squashed commit subjects. That is not enough for this change, because it alters values consumers already read: anyone who has been getting 0s for these fields starts getting the true duration, which can surface as an unexpected Terraform plan on machine policy timeouts.

1. Squash commit subject — keep the fix: prefix so it groups under Bug Fixes, but signal the behaviour change:

fix: parse day and fractional-second components in FromTimeSpan (changes parsed values)

2. After tagging, paste this under the generated changelog on the GitHub Release:

Note on this release — FromTimeSpan returns different values

machines.FromTimeSpan and machinepolicies.FromTimeSpan previously read the day component from an always-empty slice, so any time span carrying days parsed to zero and fractional seconds were dropped. 10.00:00:00 returned 0s; 7.12:30:00 returned 12h30m0s instead of 180h30m0s.

These functions now return the correct duration. Values in plain hh:mm:ss form are unaffected.

This is a read-path change only — ToTimeSpan, which produces the values sent to the server, is unchanged. Fields affected are on machine policy, machine cleanup policy, and machine health check policy: connection timeouts, retry intervals, polling timeouts, DeleteMachinesElapsedTimeSpan, and HealthCheckInterval.

If you upgrade and use these fields, expect a one-off diff where a value that read as zero now reads as its real duration. In Terraform this may surface as an unexpected plan on machine policy timeouts. The new value is the correct one.

Negative time spans remain unsupported in both directions and are unchanged by this release.

goreleaser will not generate this, so it has to be pasted by hand on whichever tag carries this PR. I'm raising a separate PR to adopt release-please so the next change like this can be documented in a CHANGELOG.md at review time instead of remembered at tag time.

[Scott-Emberson]

Understood on the fork CI limitation, and agreed there is nothing more I can do about it from this side. If a maintainer wants a real integration run before merge, pushing the branch in-repo works for me (maintainerCanModify is true).

To make the release handling harder to miss at merge and tag time:

• PR title now matches your suggested squash subject, so the default squash commit will carry the (changes parsed values) marker without anyone having to remember it.
• The PR description's upgrade note now links your ready-to-paste release note directly, so whoever tags has it one click away rather than buried in the comment thread.

release-please adoption sounds right for exactly this reason; happy to review that PR when it exists.