Skip to content

fix: parse multi-unit duration strings in convert_duration_to_interval#3895

Open
juanuicich wants to merge 1 commit into
hatchet-dev:mainfrom
juanuicich:fix/multi-unit-duration-interval
Open

fix: parse multi-unit duration strings in convert_duration_to_interval#3895
juanuicich wants to merge 1 commit into
hatchet-dev:mainfrom
juanuicich:fix/multi-unit-duration-interval

Conversation

@juanuicich
Copy link
Copy Markdown

Summary

convert_duration_to_interval(duration text) only inspects the leading integer and the final unit suffix. Multi-unit Go duration strings such as 42m30s therefore parse as roughly 42s. Registration accepts the value through time.ParseDuration and the engine stores the original string, so timeouts silently shrink at enforcement time.

This PR replaces the helper with one that walks the whole string and sums every component into a single fixed interval.

Fixes #3894

What changed

  • New migration 20260512135156_v1_0_106.sql does CREATE OR REPLACE FUNCTION convert_duration_to_interval(duration text).
  • The replacement parser supports the units the previous helper accepted (ms, s, m, h), decimal components, and an optional leading sign.
  • Legacy single-unit suffixes d, w, y keep their prior calendar semantics. They are only accepted as the entire input ('1d', '1y'), so mixed forms such as '30s1d' fall through to the standard fallback.
  • Invalid input returns the existing fallback interval, 5 minutes, matching the live function in 20240321215205_v0_17_1.sql.
  • The -- +goose Down block restores the body of that prior migration verbatim.
  • frontend/docs/pages/v1/timeouts.mdx and frontend/docs/pages/v1/error-handling/timeouts.mdx describe the multi-unit, decimal format that the engine now enforces.

Behavior changes

  • Decimal single-unit values are now parsed correctly. The previous helper truncated 1.5h to 1h because num_value was INT. The new helper returns 1h30m, which matches what registration validation already accepted.

The fix is engine-side because stored values and other clients can already send multi-unit strings. An SDK-only change would not cover them.

Regression coverage

pkg/repository/duration_test.go adds a database-facing test that runs the migration through setupPostgresWithMigration(t).

Multi-unit and fractional cases compare seconds via:

SELECT EXTRACT(EPOCH FROM convert_duration_to_interval($1))::double precision

Cases:

  • 42s to 42
  • 11m to 660
  • 1h to 3600
  • 42m30s to 2550
  • 1h30m to 5400
  • 1h30m5s to 5405
  • 1500ms to 1.5
  • 1.5h to 5400
  • -1.5h to -5400
  • 0s to 0
  • bad, 42, 30s1d to 300 (the 5 minutes fallback)

Legacy single-unit cases compare interval values directly to confirm calendar semantics are preserved:

  • 1d equals 1 day
  • 1w equals 7 days
  • 1y equals 12 mons
  • 10d equals 10 days

Generated files

task generate-sqlc produces no diff. convert_duration_to_interval is not declared in the sqlc schema files, so no query signatures change.

Test plan

  • go test -count=1 ./pkg/repository -run TestConvertDurationToInterval -v: passing
  • go test -count=1 ./pkg/validator -run TestValidatorDuration -v: passing
  • task generate-sqlc produces no diff
  • gofmt -s -l on changed files reports no diff (the task fmt-go body)
  • task test: full unit suite, 33 packages with tests, all pass
  • task lint-go: golangci-lint run ./... --config .golangci.yml reports 170 pre-existing issues, none in files this PR touches
  • task test-integration: go test -count=1 -tags integration $(go list ./... | grep -v quickstart) -failfast against docker compose Postgres + RabbitMQ, all pass

@vercel
Copy link
Copy Markdown

vercel Bot commented May 12, 2026

@juanuicich is attempting to deploy a commit to the Hatchet Team on Vercel.

A member of the Team first needs to authorize it.

The previous helper inspected only the leading integer and the final unit
suffix, so multi-unit Go duration strings such as 42m30s were enforced as
roughly 42s. Registration accepts these strings through time.ParseDuration
and the engine stores them verbatim, so timeouts silently shrank at
enforcement time.

The replacement walks the whole string and sums every component. It
accepts the units the previous helper supported (ms, s, m, h) plus
decimal components and an optional leading sign. Legacy single-unit
suffixes d, w, y keep their calendar semantics and are only accepted as
the entire input. Invalid input still falls back to 5 minutes.

Fixes hatchet-dev#3894
@juanuicich juanuicich force-pushed the fix/multi-unit-duration-interval branch from d8dae02 to 7359cbf Compare May 12, 2026 15:15
@gregfurman gregfurman requested a review from abelanger5 May 12, 2026 15:40
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.

[BUG] Multi-unit duration strings are enforced with the wrong timeout

1 participant