feat: recover expired commits via /v1/events fallback; honor Retry-After (v0.3.0)#74
Merged
Conversation
…v0.3.0) A commit records spend that ALREADY happened, so a RESERVATION_EXPIRED rejection (commit landed after the grace period; the server already returned the reserved budget to the pool) must not silently drop the spend. guard.commit() now recovers it as a post-hoc direct-debit event (POST /v1/events): - reuses the commit's idempotency key — exactly-once across the separate event namespace; - subject/action from the reservation (the guard now retains them, threaded through from reserve(); subject()/action() accessors added); - commit metadata carried over plus recovered_reservation_id and recovery_reason = "commit_after_reservation_expired" markers; - no overage_policy (server default ALLOW_IF_AVAILABLE never rejects); - transient event failures retried with the existing bounded backoff policy (CommitRetryEngine::retry_event), same request/key. Recovery is surfaced additively: CommitStatus::RecoveredViaEvent and CommitResponse::recovered_via_event: Option<EventId> (both on #[non_exhaustive] types, so existing callers keep compiling and a plain `commit(...).await?` still means "spend recorded"). A failed fallback returns the new Error::CommitRecoveryFailed carrying BOTH the original expired-commit error and the event error — spend is NOT recorded and the caller must compensate. RESERVATION_FINALIZED deliberately does not trigger the fallback (the reservation was already committed/released — nothing is lost). Retry-After: error responses now parse the Retry-After header (delta-seconds; previously always dropped) into Error::retry_after(), and the retry loop waits at least the server's advertised delay after a 429 LIMIT_EXCEEDED — even past retry_max_delay — consumed once per response. 401/403 stay non-retryable by design; Error::is_auth_error() makes them programmatically distinct. Tests: tests/recovery_test.rs (expired→event success incl. key reuse + metadata markers, non-object metadata preservation, fallback failure surfaces both errors, bounded event retry, FINALIZED non-recovery, 429 Retry-After wall-clock honor, header parsing) plus unit tests for the delay policy, retry_event branches, the new error variant/helpers, and the RECOVERED_VIA_EVENT serde roundtrip. Full suite, clippy -D warnings, and fmt clean; coverage 95.10%. Version 0.2.7 -> 0.3.0 (minor: new behavior + new Error variant, which is source-breaking for exhaustive matches). AUDIT.md, CHANGELOG.md, and README updated.
Review fixes on the v0.3.0 expired-commit event fallback (PR #74): - P2: bodyless 429 is retryable - Error::is_retryable() treats HTTP 429 as retryable by status alone (absent/unparseable body included), honoring the Retry-After header. Cross-SDK parity. - F1: HTTP 410 triggers event recovery even when the body cannot be parsed into RESERVATION_EXPIRED; new Error::status() accessor. - F2: honored Retry-After clamped to 3600s (fleet decision D2); commit() docs state the cancellation contract (recovery is not cancellation-transparent; re-POSTing the event with the commit's idempotency key is safe - exactly-once server-side). - F3: heartbeat cancelled before recovery runs - the reservation is expired for good, extends are doomed traffic. - F5: fallback event with non-APPLIED status (incl. Unknown) is recovery failure -> Error::CommitRecoveryFailed, unexpected status conveyed in event_error. - F6: wire string "RECOVERED_VIA_EVENT" deserializes to CommitStatus::Unknown - client-synthesized only, so a non-conformant server cannot violate the Some-iff-status invariant. - F7: BudgetExceeded retryable only with retry_after AND status 429 (new status: Option<u16> field; None when derived from a DENY decision). A 409 BUDGET_EXCEEDED with Retry-After is not retried. Tests: 4 new wiremock e2e tests (recovery_test.rs), retry-cap unit test, wire-guard serde test, tightened error_test.rs expectations. 171 tests pass; clippy -D warnings and fmt clean; tarpaulin 95.28%. CHANGELOG [0.3.0] and AUDIT.md updated.
Contributor
Author
|
Adversarial self-review fixes landed in 553ed32:
171 tests; tarpaulin 95.28%; clippy -D warnings + fmt clean. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a commit lands after the reservation's grace period, the server answers
RESERVATION_EXPIREDand has already returned the reserved budget to the pool — the spend that actually happened is permanently under-counted. This SDK's inline/awaited retry design (unlike the Python/TS SDKs' old fire-and-forget engines) correctly surfaces the failure to the caller, but the SDK offered no recovery:create_event(POST /v1/events, the spec's post-hoc direct-debit endpoint) existed and was never called, and compensation was punted entirely to the user. Additionally, 429 rate-limit responses were retried on plain backoff, ignoring the server'sRetry-After.Part of the fleet-wide durability rollout: cycles-client-python v0.5.0 (#89), cycles-client-typescript v0.4.0 (#172). Per the Rust SDK's inline design, this PR is deliberately smaller in scope — no disk journal — because
commit().awaitalready blocks until a truthful terminal outcome.Fix
ReservationGuard::commit()catches a finalRESERVATION_EXPIREDand recovers the spend viacreate_event: same idempotency key as the commit (exactly-once across the separate event namespace), the reservation's subject/action, the commit's actual/metrics, and metadata taggedrecovered_reservation_id/recovery_reason: "commit_after_reservation_expired"for reconciliation. Nooverage_policy(spec defaultALLOW_IF_AVAILABLEnever rejects). Transient event failures get the same bounded retry policy as commits.RESERVATION_FINALIZEDdeliberately does not trigger the fallback (spend isn't lost) — pinned by a regression test.commit(...).await?: success returnsCommitResponsewith newCommitStatus::RecoveredViaEvent+recovered_via_event: Option<EventId>(both types#[non_exhaustive]); failure returns newError::CommitRecoveryFailed { reservation_id, commit_error, event_error }so the caller sees both that the commit expired and that recovery failed — spend is explicitly not recorded.Retry-Afterhonored:parse_error_responsenow captures the header (delta-seconds form, per runtime spec v0.1.25.12) intoError::Api/Error::BudgetExceeded, and the retry loop sleepsmax(backoff, retry_after), consumed once per 429 and allowed to exceedretry_max_delay.Error::is_auth_error()helper (401/403) for caller-side handling — auth failures remain truthfulErrs per the inline design.Version
0.2.7 → 0.3.0:
Erroris not#[non_exhaustive], so the newCommitRecoveryFailedvariant is source-breaking for exhaustive matches (documented in CHANGELOG Notes).Verification
cargo test: 161 passed, 0 failed (12 live-server tests ignored by design), including newtests/recovery_test.rs(7 wiremock end-to-end cases)cargo clippy --all-targets --all-features -- -D warningsclean;cargo fmt --checkcleancargo tarpaulincoverage 95.10%, above the 95% gate