fix: assorted arithmetic and indexing robustness fixes#2451
Conversation
When a cached GCP access token was already expired (expires_at_ms < now), the remaining lifetime calculation used plain subtraction: (expires_at_ms - now) / 1000. This underflows in debug builds and wraps in release builds. Use saturating_sub so an expired token simply yields a non-positive remaining lifetime and is skipped. Add a regression test. Signed-off-by: Andrew White <andrewh@cdw.com>
The lease steal path computed age as now_ms() - record.updated_at_ms. If the stored updated_at timestamp is in the future (e.g. clock skew), the subtraction underflows. Use saturating_sub to treat a future timestamp as age 0, keeping the lease considered fresh. Signed-off-by: Andrew White <andrewh@cdw.com>
max_lifetime_seconds is only validated as >= 0. The code multiplied the raw i64 value by 1000 to compute max_expires, which overflows for values above i64::MAX / 1000. The capped i32 value used for the STS request was already computed but not reused. Compute max_lifetime_ms from the capped value with saturating_mul and use it for both expires_at fallback and max_expires. Signed-off-by: Andrew White <andrewh@cdw.com>
resolve_vertex_ai_route assumed every inference provider profile has at least two base URL config keys by indexing [0] and [1]. Only the Vertex profile satisfies that today; other profiles would panic here. Iterate base_url_config_keys with find_map instead, and add a test with an empty key list to ensure no panic. Signed-off-by: Andrew White <andrewh@cdw.com>
|
I have read the DCO document and I hereby sign the DCO. |
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This is small, concentrated robustness work in existing credential, lease, refresh, and inference paths, with clear motivation and no duplicate candidate found.
Head SHA: 9fa1cda4cd5f87bcbcd3feb01d725efd4a26a614
Review findings:
- Two warning-level edge cases still require author changes: timestamp addition can still overflow, and a blank preferred Vertex base URL prevents a valid fallback alias from being considered.
- One regression-test gap remains for future-timestamp lease clock skew.
Docs: No Fern docs update is needed because these are internal robustness fixes without a new user-facing contract.
Next state: gator:in-review
johntmyers
added
gator:in-review
|
/ok to test 9fa1cda |
|
Label |
Address review feedback: add regression coverage for future-timestamp lease clock skew. Extract lease_is_expired() and clamp the age at zero — i64::saturating_sub saturates at i64::MIN, not zero, so a future updated_at_ms must be clamped explicitly to be treated as age zero and remain unstealable with a positive TTL. Signed-off-by: Andrew White <andrewh@cdw.com>
Address review feedback: find_map stopped at the first present key even when its value was blank, and the outer filter discarded it without considering lower-priority aliases. Filter blank values inside the closure so a blank preferred key falls back to a valid alias, matching the documented priority order. Add a regression test with a blank preferred key and a valid fallback. Signed-off-by: Andrew White <andrewh@cdw.com>
Address review feedback: now_ms + max_lifetime_ms could still overflow when current_time_ms() saturates near i64::MAX. Compute max_expires = now_ms.saturating_add(max_lifetime_ms) once and use it for both the conversion fallback and the expiry cap. Add a boundary unit test covering a saturated clock. Signed-off-by: Andrew White <andrewh@cdw.com>
|
Follow-up addressing all three review findings (296e1b5):
|
johntmyers
left a comment
johntmyers
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
Validation: This remains valid as small, concentrated robustness work in existing credential, lease, refresh, and inference paths.
Head SHA: 296e1b5ddcc05ba22d97647eccfd01ab990cecdc
Thanks @andrewwhitecdw. I checked your follow-up covering the AWS expiry-cap addition, blank preferred Vertex URL fallback, and future-timestamp lease handling. The current head resolves all three prior findings, including the added boundary coverage. No blocking findings remain. The independent review noted only optional test-quality improvements that do not require author action.
Docs: No Fern docs update is needed because these are internal correctness fixes without a new user-facing contract.
E2E: test:e2e remains required because the change touches provider credential flow.
Next state: gator:watch-pipeline
johntmyers
added
gator:watch-pipeline
|
/ok to test 296e1b5 |
johntmyers
added
gator:approval-needed
johntmyers
added
gator:merge-ready
and removed
gator:approval-needed
2 participants
Summary
Four small, independent robustness fixes, one commit each:
expires_at_ms - now) when a cached token was already expired.updated_at_mswas in the future (clock skew).max_lifetime_secondsvalues overflowed when multiplied by 1000; the code already computed a safe, cappedi32value for the STS request but didn't reuse it.resolve_vertex_ai_routehardcoded indices[0]and[1]intobase_url_config_keys, which would panic for any profile with fewer than two keys.Related Issue
N/A — small fixes found during code review.
Changes
provider_credentials.rs:saturating_subfor expired GCP token remaining lifetime; regression testlease.rs:saturating_subfor lease ageprovider_refresh.rs: computemax_lifetime_msfrom the cappedmax_lifetimeusingsaturating_mulinference.rs: iteratebase_url_config_keyswithfind_map; regression test for empty key listTesting
mise run pre-commitpasses (mise unavailable in this environment; ran equivalentcargo fmt+cargo clippyon all touched crates — clean)cargo test -p openshell-core gcp_token_response,cargo test -p openshell-server resolve_vertex_ai_route_handles_empty— passed)Checklist