fix: assorted arithmetic and indexing robustness fixes#2451
fix: assorted arithmetic and indexing robustness fixes#2451andrewwhitecdw wants to merge 7 commits into
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
| .to_millis() | ||
| .unwrap_or_else(|_| now_ms + max_lifetime_i64 * 1000); | ||
| let max_expires = now_ms + max_lifetime_i64 * 1000; | ||
| .unwrap_or_else(|_| now_ms + max_lifetime_ms); |
There was a problem hiding this comment.
gator-agent
Warning: The multiplication is now bounded, but now_ms + max_lifetime_ms can still overflow because current_time_ms() may saturate to i64::MAX (CWE-190). Compute let max_expires = now_ms.saturating_add(max_lifetime_ms); once, then use it for both the conversion fallback and expiry cap. Add a boundary unit test covering a saturated clock.
| if let Some(base_url) = profile | ||
| .base_url_config_keys | ||
| .iter() | ||
| .find_map(|key| config.get(*key)) |
There was a problem hiding this comment.
gator-agent
Warning: find_map stops at the first present key even when its value is blank; the outer filter then discards it without checking lower-priority aliases. This conflicts with the documented priority-order behavior and find_provider_config_value. Filter inside the closure, for example .find_map(|key| config.get(*key).filter(|v| !v.trim().is_empty())), and test a blank preferred key with a valid fallback key.
| let record = self.read().await?.ok_or(LeaseError::NotFound)?; | ||
|
|
||
| let age_ms = now_ms() - record.updated_at_ms; | ||
| let age_ms = now_ms().saturating_sub(record.updated_at_ms); |
There was a problem hiding this comment.
gator-agent
Suggestion: Add regression coverage asserting that a future updated_at_ms is treated as age zero and cannot be stolen. Existing lease tests cover active and zero-TTL leases but not the clock-skew behavior this change intends to preserve.
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
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