[Bug] `RetryPolicy.maximumAttempt === 0` throws, while it is documented as a valid value for "unlimited"

[jsamr]

What are you really trying to do?

Setting retry policy to "unlimited".

Describe the bug

In a workflow, call proxyActivities with the below retry policy object:

const retryPolicy = {
  maximumAttempts: 0
}

Minimal Reproduction

Not needed. The code below is at odds with the documentation:

sdk-typescript/packages/common/src/retry-policy.ts

Lines 53 to 63 in 9b3ec22

	if (retryPolicy.maximumAttempts != null) {
	if (retryPolicy.maximumAttempts === Number.POSITIVE_INFINITY) {
	// drop field (Infinity is the default)
	const { maximumAttempts: _, ...without } = retryPolicy;
	retryPolicy = without;
	} else if (retryPolicy.maximumAttempts <= 0) {
	throw new ValueError('RetryPolicy.maximumAttempts must be a positive integer');
	} else if (!Number.isInteger(retryPolicy.maximumAttempts)) {
	throw new ValueError('RetryPolicy.maximumAttempts must be an integer');
	}
	}

It throws when maximumAttempts is 0.

The documentation states the following:

Setting the [Maximum Attempts] value to 0 also means unlimited.

Environment/Versions

Not needed.

Additional context

The workaround is to set the value to Number.POSITIVE_INFINITY or undefined.

[mjameswh]

Thanks for pointing this out. It indeed looks like the code and docs are contradicting each others.

That specific doc is not TS specific, so the error probably resulted from incorrect generalization of doc originally written for another SDK. I'll have to double-check what we do in other SDKs, and from there, we'll decide internally if we should either align all SDKs on the docs, or to fix the docs.

In any case, I'd strongly recommend using undefined or Number.POSITIVE_INFINITY. Both better express the meaning of "no limit to the number of attempts" than giving a special meaning to the value 0.

[jsamr]

In any case, I'd strongly recommend using undefined or Number.POSITIVE_INFINITY. Both better express the meaning of "no limit to the number of attempts" than giving a special meaning to the value 0.

I totally agree! We have an internal temporal SDK wrapper where we use the "unlimited" string literal to mean unlimited. But we need to map these wrapper types to vanilla temporal types at some point, and since we needed JSON-serializable values, we opted for 0 in this case. Replaced with undefined and it works fine!