refactor(sdk): introduce NonZeroIggyDuration for retries and heartbeats

[ethanlin01x]

Which issue does this PR address?

Relates to #3776 (Found while reviewing)

Rationale

Zero-valued durations reach loops that spin or panic. A zero retry interval panics tokio::time::interval, and a zero heartbeat interval turns the heartbeat task into a continuous ping loop. IggyDuration::from_str maps 0, none, disabled and unlimited to the same zero, so a user asking to turn something off arrives there.

A duration that paces a loop is a different thing from a duration that measures a delay, so it gets its own type. NonZeroIggyDuration rejects the zero where the value is built, which means no call site has to remember the rule and every binding that funnels through the Rust SDK gets the guarantee.

What changed?

New type NonZeroIggyDuration (core/common/src/utils/non_zero_duration.rs): TryFrom<IggyDuration>, TryFrom<u64>, FromStr, Display and serde, all rejecting zero and the three aliases that parse as zero. The error is a typed NonZeroDurationError.

Fields that now hold it:

• heartbeat_interval on the TCP, QUIC, WebSocket and HTTP client configs and connection string options, plus BinaryTransport::get_heartbeat_interval and SystemClient::heartbeat_interval
• reconnection.interval on all three transports
• IggyConsumer init_retry_interval and polling_retry_interval
• ProducerCore send_retries_interval

Zero stays legal where it means something, and this is now pinned by tests: the auto-commit interval (zero stores the offsets in a busy loop), reestablish_after (zero reconnects immediately), poll_interval and the background producer linger_time.

The Python and PHP bindings follow the same types.

Breaking changes

• The config fields above change type. Construction becomes NonZeroIggyDuration::from_str("5s")? instead of IggyDuration::from_str("5s")?.
• A connection string with heartbeat_interval=none|disabled|unlimited|0 or reconnection_interval=0 now fails with InvalidConnectionString instead of silently spinning. This includes the HTTP connection string, where the option was parsed and discarded.
• Python: TcpReconnectionConfig(interval=timedelta(0)) is now rejected whatever the retry policy is, where feat(python): expose TCP client configuration #3776 allowed it with a bounded retry count or with reconnection disabled. A zero AutoCommit interval is now accepted, where feat(python): expose TCP client configuration #3776 rejected it.

Local Execution

• Passed
• Pre-commit hooks ran

AI Usage

1. Which tools? Claude
2. Scope of usage? help implement and review this PR
3. How did you verify the generated code works correctly? Unit tests pin each rejected value and each still-legal zero
4. Can you explain every line of the code if asked? Yes, all the changes are checked by the human.

[codecov]

Codecov Report

❌ Patch coverage is 84.89011% with 55 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.80%. Comparing base (2b52fe0) to head (176e7a1).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
core/common/src/utils/duration.rs	89.04%	15 Missing and 1 partial ⚠️
core/sdk/src/client_provider.rs	27.77%	7 Missing and 6 partials ⚠️
core/sdk/src/clients/client_builder.rs	33.33%	8 Missing ⚠️
core/sdk/src/tcp/tcp_client.rs	83.33%	2 Missing and 2 partials ⚠️
...guration/quic_config/quic_client_config_builder.rs	0.00%	2 Missing ⚠️
...ebsocket_config/websocket_client_config_builder.rs	0.00%	2 Missing ⚠️
core/sdk/src/quic/quic_client.rs	88.23%	0 Missing and 2 partials ⚠️
core/sdk/src/websocket/websocket_client.rs	85.71%	0 Missing and 2 partials ⚠️
...tion/http_config/http_connection_string_options.rs	92.85%	0 Missing and 1 partial ⚠️
...ion/quic_config/quic_client_reconnection_config.rs	50.00%	1 Missing ⚠️
... and 4 more

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3891      +/-   ##
============================================
- Coverage     83.84%   83.80%   -0.04%     
  Complexity     1358     1358              
============================================
  Files          1212     1212              
  Lines        166831   167046     +215     
  Branches     134305   134658     +353     
============================================
+ Hits         139876   140000     +124     
+ Misses        23314    23258      -56     
- Partials       3641     3788     +147     

Components	Coverage Δ
Rust Core	84.66% <84.10%> (+0.05%)	⬆️
Java SDK	66.67% <ø> (ø)
C# SDK	74.99% <ø> (-1.56%)	⬇️
Python SDK	90.30% <100.00%> (+0.30%)	⬆️
PHP SDK	84.39% <100.00%> (-0.09%)	⬇️
Node SDK	95.84% <ø> (ø)
Go SDK	68.32% <ø> (ø)

Files with missing lines	Coverage Δ
core/common/src/traits/binary_impls/system.rs	100.00% <100.00%> (ø)
core/common/src/traits/system_client.rs	0.00% <ø> (ø)
...pes/configuration/auth_config/connection_string.rs	95.95% <100.00%> (ø)
...es/configuration/quic_config/quic_client_config.rs	100.00% <100.00%> (ø)
...tion/quic_config/quic_connection_string_options.rs	70.16% <100.00%> (+1.10%)	⬆️
...ypes/configuration/tcp_config/tcp_client_config.rs	100.00% <100.00%> (ø)
...figuration/tcp_config/tcp_client_config_builder.rs	68.04% <100.00%> (ø)
...ation/tcp_config/tcp_client_reconnection_config.rs	100.00% <100.00%> (ø)
...ration/tcp_config/tcp_connection_string_options.rs	86.36% <100.00%> (+7.27%)	⬆️
...ration/websocket_config/websocket_client_config.rs	61.44% <100.00%> (ø)
... and 27 more

... and 69 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[ethanlin01x]

/request-review @hubcio

[ethanlin01x]

/ready

[haubur]

The problem is real for consumers:

• panic on a 0 IggyDuration:

  iggy/core/sdk/src/clients/consumer.rs

  Line 328 in ec456d2

  let mut timer = time::interval(interval.get_duration());

• and (not) sleeping with 0 on the background store offset at interval task loop. :

  iggy/core/sdk/src/clients/consumer.rs

  Line 496 in ec456d2

  _ = sleep(interval.get_duration()) => {}

producers:

• panic on a 0 IggyDuration:

  iggy/core/sdk/src/clients/producer.rs

  Line 265 in ec456d2

  timer.get_or_insert_with(|| tokio::time::interval(interval.get_duration()));

and connection lifecycle:

• continuous heartbeat spins:

  iggy/core/sdk/src/clients/client.rs

  Line 298 in f42f5ab

  sleep(heartbeat_interval.get_duration()).await

Clarification:

• The problem occurs if manually setting a IggyDuration equal to zero.
• No default path actually sets an IggyDuration with zero.
• panics occur for IggyDuration in retry_intervals not for heartbeat_intervals

Solution:

• Having IggyDuration falling to 0 for "unlimited", "disabled" and "none" is a little misleading.
• Since it's a common type there might be unobserved behavior in other places should we change it now.
• To avoid panics, I would prefer not checking in methods calls as done in the PR but enforcing the proposed guards on a type level e.g. by introducing a NonZeroIggyDuration.

[ethanlin01x]

@haubur

Thanks for the review.

I agree that using NonZeroIggyDuration is cleaner than checking in each method. My concern is that some of these are cross-field rules, so I am not sure a type can enforce them. For example, interval = 0 is still valid when max_retries is bounded. Changing the field type would also be a breaking change for the SDK API.

So I would prefer to keep the current approach. What do you think?

[haubur]

@ethanlin01x fair points.

• A NonZeroIggyDuration assumes that 0 intervals are actually undesirable, which I would argue makes sense. I think its reasonable to enforce waiting a bit before retrying (instead of rapid fire). You can still set the duration to some very low time period if you want that, but that's intended behavior than not implicit as it is now.
• It would be a breaking API change, true. If thats something we want at this point is a question to the maintainers I guess, because there is a release coming up this week AFAIK.

[spetz]

@haubur breaking changes are totally fine at this stage, as we're very close to the next release, with lots of breaking changes anyway :)
And yes, I do agree, there should be at least like a 1 ms wait before the retries (while on the other hand, no wait at all for data producer/consumer running as a busy loop is totally fine for specific use cases).

[haubur]

Thanks @spetz! So @ethanlin01x lets have a NonZeroIggyDuration for retries and heartbeats, while keeping the busy loop for AutoCommit on an interval = 0. Would you want to rewrite? Otherwise, I can also take this.

[ethanlin01x]

@haubur Thanks, I just rewrote the whole PR to introduce NonZeroIggyDuration

[haubur]

Thanks! I add some minor comments, basically just location and making sure that the IggyDuration and NonZeroIggyDuration API will align.

[ethanlin01x]

@haubur thanks, all four comments are addressed in 9f07b53 and 71f3484.

[ethanlin01x]

/ready