Switch to jiff::Span parsing for human-friendly duration input/output

[pcholakov]

This commit replaces humantime with jiff::fmt::friendly to avoid ambiguous unit conversions. The result of this is that we no longer accept inputs such as P1M, 1 month, P1Y or 1 year. It makes sense to me to accept this tradeoff for the sake of more consistent conversions - alternatively, we could define our own conversion ratios for the number of days in a month/year as outlined here https://github.com/restatedev/restate/issues/2259#issuecomment-2726754029.

See jiff's Comparison with the humantime crate for a more detailed discussion about the problems with its implementation.

There's an opportunity for eliminating DurationString in favor of using jiff's serde feature directly, as well as broader adoption - I've kept this small in scope intentionally so that we can have a desired behavior discussion first.

This also opportunistically updates humantime to the latest (final?) available version.

Please note that jiff formats units of days as d, e.g. 30d rather than humantime's 30days. If we choose the verbose printer, it changes the behavior for all units, so h becomes hour/hours etc. I prefer the denser output format but this is a change in observable behavior. Input parsing is unaffected.

Fixes: #2259

---

Testing

❯ curl --no-progress-meter 'http://localhost:9070/services/usersignup' \
  -X 'PATCH' -H 'content-type: application/json' \
  --data-raw '{"idempotency_retention":"90m"}' | jq .idempotency_retention
"1h 30m"

❯ curl --no-progress-meter 'http://localhost:9070/services/usersignup' \
  -X 'PATCH' -H 'content-type: application/json' \
  --data-raw '{"idempotency_retention":"23h"}' | jq .idempotency_retention
"23h"

❯ curl --no-progress-meter 'http://localhost:9070/services/usersignup' \
  -X 'PATCH' -H 'content-type: application/json' \
  --data-raw '{"idempotency_retention":"23h 60m"}' | jq .idempotency_retention
"1d"

❯ curl --no-progress-meter 'http://localhost:9070/services/usersignup' \
  -X 'PATCH' -H 'content-type: application/json' \
  --data-raw '{"idempotency_retention":"25h"}' | jq .idempotency_retention
"1d 1h"

❯ curl --no-progress-meter 'http://localhost:9070/services/usersignup' \
  -X 'PATCH' -H 'content-type: application/json' \
  --data-raw '{"idempotency_retention":"720h"}' | jq .idempotency_retention
"30d"

❯ curl --no-progress-meter 'http://localhost:9070/services/usersignup' \
  -X 'PATCH' -H 'content-type: application/json' \
  --data-raw '{"idempotency_retention":"1month"}'
Failed to deserialize the JSON body into the target type: idempotency_retention: Please use units of days or smaller at line 1 column 34%

❯ restate services config view usersignup
 Name:          usersignup
 Service type:  Workflow

...

 Idempotent requests retention:  30d
  💡   The retention duration of idempotent requests for this service.
       The retention period starts once the invocation completes (with either success or failure).
       After the retention period, the invocation response and the idempotency key will be forgotten.

...

❯ restate services config patch --idempotency-retention P28DT54H usersignup
 Idempotent requests retention:  30d 6h
✔ Are you sure you want to apply these changes? · yes
❯ restate services config view usersignup
 Name:          usersignup
 Service type:  Workflow

...

 Idempotent requests retention:  30d 6h
  💡   The retention duration of idempotent requests for this service.
       The retention period starts once the invocation completes (with either success or failure).
       After the retention period, the invocation response and the idempotency key will be forgotten.

...

❯ curl --no-progress-meter 'http://localhost:9070/services/usersignup' | jq
{
  "name": "usersignup",
  "handlers": [
    {
      "name": "click",
      "ty": "Shared",
      "input_description": "application/json",
      "output_description": "application/json"
    },
    {
      "name": "run",
      "ty": "Workflow",
      "input_description": "application/json",
      "output_description": "application/json"
    }
  ],
  "ty": "Workflow",
  "deployment_id": "dp_16la0zp8reGBItqmP7dyxbP",
  "revision": 1,
  "public": true,
  "idempotency_retention": "30d 6h",
  "workflow_completion_retention": "1d",
  "inactivity_timeout": "30s",
  "abort_timeout": "2m"
}

[github-actions[bot]]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[github-actions[bot]]

Test Results

  7 files  ± 0    7 suites  ±0   3m 8s ⏱️ - 1m 5s  52 tests  -  2   51 ✅  -  2  1 💤 ±0  0 ❌ ±0  213 runs   - 10  210 ✅  - 10  3 💤 ±0  0 ❌ ±0 

Results for commit e5f85cd9. ± Comparison against base commit 461952c7.

This pull request removes 2 tests.

dev.restate.sdktesting.tests.Combinators ‑ awakeableOrTimeoutUsingAwaitAny(Client)
dev.restate.sdktesting.tests.Combinators ‑ firstSuccessfulCompletedAwakeable(Client)

:recycle: This comment has been updated with latest results.

[pcholakov]

I'm a bit conflicted about supporting larger units. If we do, I'd say let's pick a larger value as the more conservative choice - so 30.5 * 24h as a month, 366 * 24h for year. We will document our conversion of course, but I think it would lead to fewer surprises. Say, if I set a retention duration to P1Y and it's pruned 365 * 24 hours later - this could be a surprise if there had been a leap day during the period. Conversely, for most things our users are likely to set to such high durations, the consequences for sticking around for a few extra hours are low.

I'd still rather err on the side of precision and reject these ambiguous units. Expressing a duration as P365D / P365D6H / P366D isn't that much more typing, and is more explicit both for the user configuring the value, and for all the readers of the configuration to follow. And we still have the opportunity to get fancy in a UI layer above the API where we can provide layer tips about the conversion that's occurring. Just my 2c :-) I would love to hear what @nikrooz thinks about this?

Should we also update apply_service_configuration_patch to not use humantime::Duration::from(duration) for rendering the changes? If not, then we might render different numbers than what the user's input was.

Should we also update ServiceMetadata to not use humantime::Duration? Otherwise after patching a service and then listing it, the idempotency and completion retention look off again.

Yes, definitely, I think so for all such occurrences. I stopped a bit short to get feedback early but may as well do those under the same PR.

As a follow-up do you think that we should replace all humantime::Duration usages with jiff? Or is there only little value because most of the humantime::Duration usages usually don't hold very large values?

I would favor moving in that direction, if for no other reason other than I really like the API and the internals I've looked around inspire a lot of confidence.

[nikrooz]

I'd still rather err on the side of precision and reject these ambiguous units. Expressing a duration as P365D / P365D6H / P366D isn't that much more typing, and is more explicit both for the user configuring the value, and for all the readers of the configuration to follow. And we still have the opportunity to get fancy in a UI layer above the API where we can provide layer tips about the conversion that's occurring. Just my 2c :-) I would love to hear what @nikrooz thinks about this?

I totally agree, just accepting a subset of duration format would be less confusing, without losing any functionality. (apprently it's also a common practice like in Java Duration ) My only comment would be to show the same value in the UI, without any smart conversion, as it can be confusing.

[pcholakov]

@tillrohrmann updated the service configuration API and CLIs to use the new code path, too - see updated testing notes.

As a follow-up, we can also remove humantime parsing from our various config option types.

[pcholakov]

As a follow-up do you think that we should replace all humantime::Duration usages with jiff? Or is there only little value because most of the humantime::Duration usages usually don't hold very large values?

I tried this briefly but didn't see much value - the one nice win is that jiff has a serde feature but we lose the finer control over formatting that we have in DurationString at the moment.

[pcholakov]

This is how the UI renders the properties with these latest changes:

If we merge this, we should update the presets and the format tip link in the web UI, too.

[BurntSushi]

I tried this briefly but didn't see much value - the one nice win is that jiff has a serde feature but we lose the finer control over formatting that we have in DurationString at the moment.

Can you say more about what specifically is blocking this transition? I don't think humantime is more flexible than Jiff in this regard, so I'd be curious to hear more!

[pcholakov]

Can you say more about what specifically is blocking this transition? I don't think humantime is more flexible than Jiff in this regard, so I'd be curious to hear more!

To be clear, all I'm saying is I don't see much value in replacing our internal use of std::time::Duration with jiff::SignedDuration. There are no obstacles to replacing all of our usage of humantime with Jiff's friendly parser/printer! 😊

I briefly tried using jiff::SignedDuration with the serde feature to directly map it to/from user-facing strings, but I still needed the flexibility to customize the serialized representation units which negated the advantage. It's entirely possible I'm missing some tricks there though.

PS. I fat-fingered the GH close PR button, and only belatedly realized that I could unlock the conversation. Gotta love an eventually-consistent UI... I realize that might have looked more than a little rude - sincere apologies, I was just a klutz!