mirror of
https://github.com/AsamK/signal-cli.git
synced 2026-05-17 13:11:00 +00:00
Surface server Retry-After for rate-limit send failures (#2016)
* Surface retry-after seconds for plain rate-limit failures libsignal-service's RateLimitException exposes retryAfterMilliseconds for HTTP 413 responses, but signal-cli only forwarded retry-after for ProofRequired (428) failures. Clients had no signal for when it was safe to retry plain rate-limited sends, so every failed retry potentially extended the server-side window. SendMessageResult now carries an optional rateLimitRetryAfterSeconds, populated from the upstream Optional<Long>. JsonSendMessageResult exposes it for RATE_LIMIT_FAILURE type. Text output includes the window when known. Aggregate RateLimitErrorException now carries the real nextAttemptTimestamp (was hardcoded to 0). Closes #1996. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Address review: include proof-required retry-after and ceiling-round millis Codex adversarial review flagged two issues in the phase 1 retry-after plumbing: * Aggregate retry-after ignored proof-required failures. Because isRateLimitFailure is true for proof-required cases but rateLimitRetryAfterSeconds was only populated from plain 413s, an all-proof-required batch (or a mixed batch where the proof-required delay was longer) could flow into outputResult() and produce a RateLimitException(0), telling callers to retry immediately. * Millisecond Retry-After values were truncated by integer division, so 1..999ms became 0 and non-second-aligned values lost up to 999ms. A retry suggested from the floored value can land before the server's real deadline and re-trigger the limit. SendMessageResult.from(...) now populates rateLimitRetryAfterSeconds from either the proof-required seconds or the plain rate-limit ms (converted via ceiling division), giving maxRateLimitRetryAfterSeconds a single source of truth. JsonSendMessageResult.from(...) reads the unified field. New millisToCeilingSeconds helper plus boundary test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Preserve source compat and document retry-after field change Add a non-canonical 8-arg SendMessageResult constructor that delegates to the canonical form with null retry-after. This keeps source compatibility for any downstream code that constructs the record directly (tests, mocks) without changing the canonical shape. Records permit additional constructors alongside the canonical one. Document the retryAfterSeconds meaning change in the CHANGELOG. The field was previously populated only for proof-required failures; it is now populated whenever the server sends a Retry-After header. The canonical proof-required discriminator is still token != null. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
aafb40fd94
commit
e1b17bf863
@ -2,6 +2,10 @@
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Changed
|
||||
|
||||
- Send message results now surface server-advised retry time for plain rate-limit (HTTP 413) failures, not only for proof-required challenges. The `retryAfterSeconds` field in JSON-RPC `SendMessageResult` is populated whenever the server sends a `Retry-After` header. The canonical way to distinguish proof-required failures remains `token != null`. Text output includes "retry after N seconds" when known.
|
||||
|
||||
## [0.14.2] - 2026-04-04
|
||||
|
||||
### Added
|
||||
|
||||
@ -11,11 +11,38 @@ public record SendMessageResult(
|
||||
boolean isIdentityFailure,
|
||||
boolean isRateLimitFailure,
|
||||
ProofRequiredException proofRequiredFailure,
|
||||
boolean isInvalidPreKeyFailure
|
||||
boolean isInvalidPreKeyFailure,
|
||||
Long rateLimitRetryAfterSeconds
|
||||
) {
|
||||
|
||||
/**
|
||||
* Source-compatible constructor for callers built against the pre-retry-after record shape.
|
||||
* Delegates to the canonical constructor with a null retry-after, which is the correct value
|
||||
* for any result not produced by {@link #from}.
|
||||
*/
|
||||
public SendMessageResult(
|
||||
RecipientAddress address,
|
||||
boolean isSuccess,
|
||||
boolean isNetworkFailure,
|
||||
boolean isUnregisteredFailure,
|
||||
boolean isIdentityFailure,
|
||||
boolean isRateLimitFailure,
|
||||
ProofRequiredException proofRequiredFailure,
|
||||
boolean isInvalidPreKeyFailure
|
||||
) {
|
||||
this(address,
|
||||
isSuccess,
|
||||
isNetworkFailure,
|
||||
isUnregisteredFailure,
|
||||
isIdentityFailure,
|
||||
isRateLimitFailure,
|
||||
proofRequiredFailure,
|
||||
isInvalidPreKeyFailure,
|
||||
null);
|
||||
}
|
||||
|
||||
public static SendMessageResult unregisteredFailure(RecipientAddress address) {
|
||||
return new SendMessageResult(address, false, false, true, false, false, null, false);
|
||||
return new SendMessageResult(address, false, false, true, false, false, null, false, null);
|
||||
}
|
||||
|
||||
public static SendMessageResult from(
|
||||
@ -23,16 +50,32 @@ public record SendMessageResult(
|
||||
RecipientResolver recipientResolver,
|
||||
RecipientAddressResolver addressResolver
|
||||
) {
|
||||
final var rateLimitFailure = sendMessageResult.getRateLimitFailure();
|
||||
final var proofRequiredFailure = sendMessageResult.getProofRequiredFailure();
|
||||
final Long retryAfterSeconds;
|
||||
if (proofRequiredFailure != null) {
|
||||
retryAfterSeconds = proofRequiredFailure.getRetryAfterSeconds();
|
||||
} else if (rateLimitFailure != null) {
|
||||
retryAfterSeconds = rateLimitFailure.getRetryAfterMilliseconds()
|
||||
.map(SendMessageResult::millisToCeilingSeconds)
|
||||
.orElse(null);
|
||||
} else {
|
||||
retryAfterSeconds = null;
|
||||
}
|
||||
return new SendMessageResult(addressResolver.resolveRecipientAddress(recipientResolver.resolveRecipient(
|
||||
sendMessageResult.getAddress())).toApiRecipientAddress(),
|
||||
sendMessageResult.isSuccess(),
|
||||
sendMessageResult.isNetworkFailure(),
|
||||
sendMessageResult.isUnregisteredFailure(),
|
||||
sendMessageResult.getIdentityFailure() != null,
|
||||
sendMessageResult.getRateLimitFailure() != null || sendMessageResult.getProofRequiredFailure() != null,
|
||||
sendMessageResult.getProofRequiredFailure() == null
|
||||
? null
|
||||
: new ProofRequiredException(sendMessageResult.getProofRequiredFailure()),
|
||||
sendMessageResult.isInvalidPreKeyFailure());
|
||||
rateLimitFailure != null || proofRequiredFailure != null,
|
||||
proofRequiredFailure == null ? null : new ProofRequiredException(proofRequiredFailure),
|
||||
sendMessageResult.isInvalidPreKeyFailure(),
|
||||
retryAfterSeconds);
|
||||
}
|
||||
|
||||
static long millisToCeilingSeconds(long millis) {
|
||||
// Round up so we never advise a retry before the server's deadline.
|
||||
return (millis + 999L) / 1000L;
|
||||
}
|
||||
}
|
||||
|
||||
@ -26,4 +26,18 @@ public record SendMessageResults(long timestamp, Map<RecipientIdentifier, List<S
|
||||
.flatMap(res -> res.stream().map(SendMessageResult::isRateLimitFailure))
|
||||
.allMatch(r -> r) && results.values().stream().mapToInt(List::size).sum() > 0;
|
||||
}
|
||||
|
||||
/**
|
||||
* Longest rate-limit retry-after window across all rate-limited recipients, in seconds.
|
||||
* Null when no recipient reported one (server omitted Retry-After, or no rate-limit failures).
|
||||
*/
|
||||
public Long maxRateLimitRetryAfterSeconds() {
|
||||
return results.values()
|
||||
.stream()
|
||||
.flatMap(List::stream)
|
||||
.map(SendMessageResult::rateLimitRetryAfterSeconds)
|
||||
.filter(r -> r != null)
|
||||
.max(Long::compareTo)
|
||||
.orElse(null);
|
||||
}
|
||||
}
|
||||
|
||||
@ -0,0 +1,38 @@
|
||||
package org.asamk.signal.manager.api;
|
||||
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
||||
import static org.junit.jupiter.api.Assertions.assertEquals;
|
||||
import static org.junit.jupiter.api.Assertions.assertNull;
|
||||
|
||||
class SendMessageResultTest {
|
||||
|
||||
/**
|
||||
* Ceiling division — we must never advise a retry before the server's deadline,
|
||||
* so sub-second values round up rather than truncate toward zero.
|
||||
*/
|
||||
@Test
|
||||
void millisToCeilingSecondsRoundsUp() {
|
||||
assertEquals(0L, SendMessageResult.millisToCeilingSeconds(0L));
|
||||
assertEquals(1L, SendMessageResult.millisToCeilingSeconds(1L));
|
||||
assertEquals(1L, SendMessageResult.millisToCeilingSeconds(500L));
|
||||
assertEquals(1L, SendMessageResult.millisToCeilingSeconds(999L));
|
||||
assertEquals(1L, SendMessageResult.millisToCeilingSeconds(1000L));
|
||||
assertEquals(2L, SendMessageResult.millisToCeilingSeconds(1001L));
|
||||
assertEquals(2L, SendMessageResult.millisToCeilingSeconds(1500L));
|
||||
assertEquals(60L, SendMessageResult.millisToCeilingSeconds(60_000L));
|
||||
}
|
||||
|
||||
/**
|
||||
* Source-compat: callers built against the pre-retry-after record shape use the 8-arg
|
||||
* constructor. It must continue to compile and produce a record with a null retry-after.
|
||||
*/
|
||||
@Test
|
||||
@SuppressWarnings("deprecation")
|
||||
void legacyEightArgConstructorPreservesSourceCompat() {
|
||||
var result = new SendMessageResult(new RecipientAddress(null, null, "+15551234567", null),
|
||||
true, false, false, false, false, null, false);
|
||||
|
||||
assertNull(result.rateLimitRetryAfterSeconds());
|
||||
}
|
||||
}
|
||||
@ -32,7 +32,7 @@ public record JsonSendMessageResult(
|
||||
? Type.INVALID_PRE_KEY_FAILURE
|
||||
: Type.IDENTITY_FAILURE,
|
||||
result.proofRequiredFailure() != null ? result.proofRequiredFailure().getToken() : null,
|
||||
result.proofRequiredFailure() != null ? result.proofRequiredFailure().getRetryAfterSeconds() : null);
|
||||
result.rateLimitRetryAfterSeconds());
|
||||
}
|
||||
|
||||
public enum Type {
|
||||
|
||||
@ -59,8 +59,10 @@ public class SendMessageResultUtils {
|
||||
if (sendMessageResults.hasOnlyUntrustedIdentity()) {
|
||||
throw new UntrustedKeyErrorException("Failed to send message due to untrusted identities");
|
||||
} else if (sendMessageResults.hasOnlyRateLimitFailure()) {
|
||||
final var retryAfter = sendMessageResults.maxRateLimitRetryAfterSeconds();
|
||||
final var nextAttempt = retryAfter == null ? 0L : System.currentTimeMillis() + retryAfter * 1000L;
|
||||
throw new RateLimitErrorException("Failed to send message due to rate limiting",
|
||||
new RateLimitException(0));
|
||||
new RateLimitException(nextAttempt));
|
||||
} else {
|
||||
throw new UserErrorException("Failed to send message");
|
||||
}
|
||||
@ -110,7 +112,10 @@ public class SendMessageResultUtils {
|
||||
} else if (result.isNetworkFailure()) {
|
||||
return String.format("Network failure for \"%s\"", identifier);
|
||||
} else if (result.isRateLimitFailure()) {
|
||||
return String.format("Rate limit failure for \"%s\"", identifier);
|
||||
final var retryAfter = result.rateLimitRetryAfterSeconds();
|
||||
return retryAfter != null
|
||||
? String.format("Rate limit failure for \"%s\", retry after %d seconds", identifier, retryAfter)
|
||||
: String.format("Rate limit failure for \"%s\"", identifier);
|
||||
} else if (result.isUnregisteredFailure()) {
|
||||
return String.format("Unregistered user \"%s\"", identifier);
|
||||
} else if (result.isIdentityFailure()) {
|
||||
|
||||
@ -0,0 +1,113 @@
|
||||
package org.asamk.signal.json;
|
||||
|
||||
import org.asamk.signal.manager.api.RecipientAddress;
|
||||
import org.asamk.signal.manager.api.RecipientIdentifier;
|
||||
import org.asamk.signal.manager.api.SendMessageResult;
|
||||
import org.asamk.signal.manager.api.SendMessageResults;
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
||||
import java.util.List;
|
||||
import java.util.Map;
|
||||
import java.util.UUID;
|
||||
|
||||
import static org.junit.jupiter.api.Assertions.assertEquals;
|
||||
import static org.junit.jupiter.api.Assertions.assertNull;
|
||||
|
||||
class JsonSendMessageResultTest {
|
||||
|
||||
private static final RecipientAddress ADDRESS = new RecipientAddress(null, null, "+15551234567", null);
|
||||
|
||||
@Test
|
||||
void rateLimitFailureSurfacesRetryAfterSeconds() {
|
||||
var result = new SendMessageResult(ADDRESS,
|
||||
false, false, false, false,
|
||||
true,
|
||||
null,
|
||||
false,
|
||||
3600L);
|
||||
|
||||
var json = JsonSendMessageResult.from(result);
|
||||
|
||||
assertEquals(JsonSendMessageResult.Type.RATE_LIMIT_FAILURE, json.type());
|
||||
assertEquals(3600L, json.retryAfterSeconds());
|
||||
assertNull(json.token());
|
||||
}
|
||||
|
||||
@Test
|
||||
void rateLimitFailureWithoutRetryAfterLeavesFieldNull() {
|
||||
var result = new SendMessageResult(ADDRESS,
|
||||
false, false, false, false,
|
||||
true,
|
||||
null,
|
||||
false,
|
||||
null);
|
||||
|
||||
var json = JsonSendMessageResult.from(result);
|
||||
|
||||
assertEquals(JsonSendMessageResult.Type.RATE_LIMIT_FAILURE, json.type());
|
||||
assertNull(json.retryAfterSeconds());
|
||||
}
|
||||
|
||||
@Test
|
||||
void successLeavesRetryAfterNull() {
|
||||
var result = new SendMessageResult(ADDRESS,
|
||||
true, false, false, false,
|
||||
false,
|
||||
null,
|
||||
false,
|
||||
null);
|
||||
|
||||
var json = JsonSendMessageResult.from(result);
|
||||
|
||||
assertEquals(JsonSendMessageResult.Type.SUCCESS, json.type());
|
||||
assertNull(json.retryAfterSeconds());
|
||||
}
|
||||
|
||||
@Test
|
||||
void aggregateReturnsLongestRetryAfter() {
|
||||
var small = rateLimited("+15551234567", 60L);
|
||||
var big = rateLimited("+15559876543", 3600L);
|
||||
var unknown = rateLimited("+15550000000", null);
|
||||
|
||||
var aggregate = new SendMessageResults(1L,
|
||||
Map.of(new RecipientIdentifier.Uuid(UUID.randomUUID()), List.of(small, big, unknown)));
|
||||
|
||||
assertEquals(3600L, aggregate.maxRateLimitRetryAfterSeconds());
|
||||
}
|
||||
|
||||
@Test
|
||||
void aggregateReturnsNullWhenNoRetryAfter() {
|
||||
var aggregate = new SendMessageResults(1L,
|
||||
Map.of(new RecipientIdentifier.Uuid(UUID.randomUUID()),
|
||||
List.of(rateLimited("+15551234567", null))));
|
||||
|
||||
assertNull(aggregate.maxRateLimitRetryAfterSeconds());
|
||||
}
|
||||
|
||||
/**
|
||||
* Regression for a bug where the aggregate helper could overlook the longest
|
||||
* wait if only some recipients reported a value. Ensures the max is picked
|
||||
* across any mix — which is what downstream captcha/rate-limit clients rely on.
|
||||
*/
|
||||
@Test
|
||||
void aggregatePicksMaxEvenWhenSomeValuesAreNull() {
|
||||
var withValue = rateLimited("+15551111111", 7200L);
|
||||
var withoutValue = rateLimited("+15552222222", null);
|
||||
var alsoWithValue = rateLimited("+15553333333", 120L);
|
||||
|
||||
var aggregate = new SendMessageResults(1L,
|
||||
Map.of(new RecipientIdentifier.Uuid(UUID.randomUUID()),
|
||||
List.of(withoutValue, withValue, alsoWithValue)));
|
||||
|
||||
assertEquals(7200L, aggregate.maxRateLimitRetryAfterSeconds());
|
||||
}
|
||||
|
||||
private static SendMessageResult rateLimited(String number, Long retryAfterSeconds) {
|
||||
return new SendMessageResult(new RecipientAddress(null, null, number, null),
|
||||
false, false, false, false,
|
||||
true,
|
||||
null,
|
||||
false,
|
||||
retryAfterSeconds);
|
||||
}
|
||||
}
|
||||
Loading…
x
Reference in New Issue
Block a user