fix(antigravity): re-discover the effort matrix when a turn names an unknown model - #370
fix(antigravity): re-discover the effort matrix when a turn names an unknown model#370r-uben wants to merge 4 commits into
Conversation
…unknown model The matrix was discovered once per process and never invalidated, so a model `agy` gained after boot stayed unknown for the lifetime of the gateway. Every turn on it hit the non-authoritative arm of resolve_effort and omitted --effort, which ids that require one reject outright — an outage lasting until restart, not the documented single first turn. A miss against a warm cache now schedules a background discovery, rate-limited to one subprocess a minute so an id that genuinely does not exist cannot spawn one per turn. The refresh stays off the request path, matching the rest of the module: the turn that first names the model still passes through unvalidated, so the model becomes unknown once rather than forever. The rate-limit policy is split into a pure `refresh_is_due` so it is testable without a process-wide clock, including the backwards-clock case where a wrapping subtraction would have allowed every turn through. Closes pleaseai#366
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces rate-limited background discovery for unrecognized models in the Antigravity adapter. When a request queries a model not present in the cached effort matrix, a background refresh is scheduled, restricted to at most once every 60 seconds to prevent excessive subprocess spawning. The changes include documentation updates explaining this behavior, implementation of the rate-limiting logic using atomic variables, and comprehensive unit tests. I have no feedback to provide as the implementation is robust and well-tested.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Architecture diagram
sequenceDiagram
participant Client
participant GW as Gateway (Antigravity Adapter)
participant M as models module
participant Cache as Process-global matrix cache
participant Sub as agy CLI subprocess
Note over Client,Sub: Model Effort Discovery and Resolution Flow
Client->>GW: Request with model
GW->>M: effort_matrix(agy_bin, model)
M->>Cache: cached() lookup
alt Cache miss (empty matrix)
Cache-->>M: None
M->>Sub: spawn agy models (background)
Sub-->>Cache: Update matrix
M-->>GW: Empty matrix (pass-through)
else Cache hit
Cache-->>M: Matrix
alt Model not in matrix
M->>M: claim_unknown_model_refresh()
alt Refresh slot claimed
M->>Sub: spawn agy models (background)
Sub-->>Cache: Update matrix
else Throttled (within 60s window)
Note over M: No refresh spawned
end
M-->>GW: Stale matrix (this turn passes through)
else Model in matrix
M-->>GW: Matrix with model
end
end
GW->>M: resolve_effort(matrix, model)
alt Model found in matrix
M-->>GW: Effort level
else Model not found
M-->>GW: EffortChoice::Unspecified
end
GW->>Sub: Execute turn with --effort flag (if specified)
Sub-->>GW: Response
GW-->>Client: Turn result
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Follow-up to the cubic P2 on this PR. The finding was real: while the cache is empty, `refresh` clears `DISCOVERING` on failure so the next turn may spawn `agy models` again, and nothing throttled that path — a persistently failing binary could be re-forked once per turn. The suggested fix was to share `LAST_UNKNOWN_MODEL_REFRESH` between the warm and cold paths. That is declined and this is the alternative offered in the review thread, because sharing the claim burns it at boot: a turn arriving while `warm`'s discovery is in flight would take the slot, then no-op inside `spawn_refresh` because `DISCOVERING` is already held. A failing boot discovery would leave the process cold *and* throttled for a full interval on a claim that never ran a discovery — and boot is exactly when `warm` and the first turns race. Stamp the failure instead of the attempt. `LAST_FAILED_DISCOVERY` is written only on the `None` branch of `refresh`, and the cold-cache gate is a plain read that takes no claim, so an attempt that never fails never throttles the turn after it. The backoff is 10s, not the unknown-model minute. A cold matrix is not a steady state: every effort-taking model requires the flag, so routes pinning no effort hard-fail until discovery lands, and a minute of that would turn a blip into an outage. A few multiples of the measured ~1.3-1.9s discovery cost bounds the forking without meaningfully extending the cold window. No reset on success: `MATRIX` is only ever assigned, never cleared, so a success makes the cold path unreachable and the stamp unreadable thereafter. The three global-state assertions live in one test rather than three, since `LAST_FAILED_DISCOVERY` is process-wide and the harness runs tests in parallel. Refs pleaseai#366
|
Implemented the alternative I flagged above, so this no longer waits on a decision from you.
The backoff is 10s rather than the unknown-model minute. A cold matrix is not a steady No reset on success, deliberately: The three global-state assertions are one test, not three. Also updated the one sentence in the provider guide that described this exact behavior Local gate: fmt and clippy clean, full workspace suite passes, 0 failures. Happy to drop the commit if you would rather keep this PR to the #366 fix and take the |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f1258c693
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !matrix.contains_key(model) && claim_unknown_model_refresh() { | ||
| spawn_refresh(agy_bin); |
There was a problem hiding this comment.
When the first request for a newly released model races with completion of startup discovery, it can read the just-published stale matrix while DISCOVERING is still true. This condition then claims the 60-second throttle slot, but spawn_refresh immediately returns without spawning because discovery is still marked in flight; subsequent requests are throttled, so the new model can continue failing for a minute instead of being available on the next turn. Make claiming and scheduling atomic, or stamp the throttle only after a refresh subprocess is actually scheduled.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR refreshes Antigravity’s cached model/effort catalogue after an unknown-model lookup while keeping discovery off the request path.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/adapters/antigravity/models.rs | Adds throttled background catalogue refresh and failed-discovery backoff. |
| src/adapters/antigravity/mod.rs | Supplies the routed upstream model to the effort-matrix lookup. |
| site/src/content/docs/guides/providers.mdx | Documents runtime catalogue refresh and cold-cache retry behavior. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Antigravity turn] --> B{Effort matrix cached?}
B -- No --> C{Cold discovery due?}
C -- Yes --> D[Schedule background agy models]
C -- No --> E[Use empty matrix]
B -- Yes --> F{Model present?}
F -- Yes --> G[Resolve effort from cached matrix]
F -- No --> H{Unknown-model refresh due?}
H -- Yes --> D
H -- No --> I[Use stale matrix]
D --> J[Current turn continues without waiting]
E --> J
I --> J
Reviews (3): Last reviewed commit: "Merge branch 'main' into fix/366-antigra..." | Re-trigger Greptile
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/adapters/antigravity/models.rs">
<violation number="1" location="src/adapters/antigravity/models.rs:134">
P2: With concurrent cold requests, one can read the gate as due just before another refresh records failure, then spawn after `DISCOVERING` is cleared. Claim the discovery flag before rechecking the failure stamp, and release it without spawning when the backoff is closed.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| /// model becomes unknown *once* rather than forever. | ||
| pub async fn effort_matrix(agy_bin: &Path, model: &str) -> EffortMatrix { | ||
| let Some(matrix) = cached() else { | ||
| if cold_discovery_is_due() { |
There was a problem hiding this comment.
P2: With concurrent cold requests, one can read the gate as due just before another refresh records failure, then spawn after DISCOVERING is cleared. Claim the discovery flag before rechecking the failure stamp, and release it without spawning when the backoff is closed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/adapters/antigravity/models.rs, line 134:
<comment>With concurrent cold requests, one can read the gate as due just before another refresh records failure, then spawn after `DISCOVERING` is cleared. Claim the discovery flag before rechecking the failure stamp, and release it without spawning when the backoff is closed.</comment>
<file context>
@@ -103,7 +131,9 @@ static LAST_UNKNOWN_MODEL_REFRESH: AtomicU64 = AtomicU64::new(0);
pub async fn effort_matrix(agy_bin: &Path, model: &str) -> EffortMatrix {
let Some(matrix) = cached() else {
- spawn_refresh(agy_bin);
+ if cold_discovery_is_due() {
+ spawn_refresh(agy_bin);
+ }
</file context>
Two assertions in the new tests were written wider than rustfmt's target and CI's format check rejected them. No behavior change.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e19dd0dfc3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| discovery in the background, at most once a minute. That request still passes through to | ||
| `agy` unvalidated; the one after it resolves normally. Without this, a model released |
There was a problem hiding this comment.
When a second request arrives before the background agy models subprocess completes, effort_matrix still returns the stale cached matrix immediately, so that request can also pass through unvalidated and fail. The documentation should promise normal resolution only for requests made after discovery has finished, rather than guaranteeing that the very next turn succeeds.
AGENTS.md reference: AGENTS.md:L44-L46
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ee7df7d0d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| and `agy` validates them itself — a failed discovery is retried rather than cached, after | ||
| a short backoff so a persistently failing `agy` is not re-forked once per turn while the | ||
| matrix is still empty. |
There was a problem hiding this comment.
Because this English provider-guide change documents new user-visible Antigravity refresh/backoff behavior, the maintained locale copies of the same page need the same update. I checked site/src/content/docs/{ja,ko,zh-cn}/guides/providers.mdx; those pages still have no Antigravity section, so non-English docs now omit this behavior even though the repo requires localized site copies to stay current with English.
AGENTS.md reference: AGENTS.md:L57-L64
Useful? React with 👍 / 👎.
2 participants
Summary
Addresses #366 on the subprocess transport — deliberately not
Fixes, since themaintainer is keeping #366 open as the cross-transport tracking issue.
effort_matrixreturned the process-global cache unconditionally whenever itwas
Some, so the one-shotagy modelsdiscovery was frozen for the lifetime of thegateway: a model that appeared upstream after boot resolved to "no
--effortflag" andkept failing until restart. Reproduced with
gemini-3.7-flash, which — unlike 3.6 and 3.5flash — requires an effort value, so every turn 502'd on a gateway that had discovered
its matrix before 3.7 shipped.
The fix keeps the cache as the fast path and adds exactly one trigger: when a turn names a
model the cached matrix does not contain, spawn a background re-discovery. The turn itself
is not blocked — it proceeds on the current matrix, and the next one sees the refreshed
catalogue. A 60s floor (
UNKNOWN_MODEL_REFRESH_INTERVAL) keeps a route naming an idagygenuinely does not have from spawning one subprocess per turn forever;
DISCOVERINGalready collapses a concurrent burst, but not a steady stream.
The throttle stamp is a
u64of milliseconds since aOnceLock<Instant>epoch, comparedwith
saturating_sub, so a non-monotonic reading denies the refresh rather than wrappinginto a permanent one.
Scope note re: #368
This is the subprocess transport, which #368 deprecates. Filing it anyway on the maintainer's
stated rule that deprecation is not removal — this is a live failure on what ships today.
The
agy-specific mechanism does not carry forward, but per the #366 comment the requirementdoes: whatever supplies per-account capability hints on the native HTTP path (
POST /v1internal:fetchAvailableModels) needs invalidation designed in, or it reproduces thisexact bug. Nothing here should be ported verbatim.
Milestone / spec
None — antigravity has no frozen milestone spec in
docs/(the provider was added in#233/#234 and hardened in #325 without one). Tracking issue: #366; superseded-by
relationship recorded on #368.
Checklist
cargo buildpassescargo testpasses (new behavior is covered; tests run without network/loopback where possible)cargo clippy --all-targets -- -D warningscleancargo fmt --all --checkcleandocs/updated if this change deviates from it — n/a, no antigravity spec existssite/src/content/docs/guides/providers.mdxTest plan
cargo test --all-features --workspace: 1362 passed, 0 failed in the lib suite, plus 17integration suites all
0 failed.cargo fmt --all --checkclean,cargo clippy --all-targets --all-features -- -D warningsclean.Five new unit tests cover the throttle in isolation (no subprocess, no clock dependency):
a_never_refreshed_matrix_is_immediately_due— the zero sentinela_second_unknown_model_within_the_interval_is_not_duethe_interval_boundary_is_due—>=, not>a_backwards_clock_denies_rather_than_wrappingthe_first_claim_wins_and_the_next_is_throttled— the compare-and-swap racePre-existing flake, unrelated to this diff (which touches no code it exercises):
tests/antigravity_process.rs::streaming_turn_translates_stub_events_to_ssehits the 20sREQUEST_GUARDtimeout under parallel load. Isolated: 1 failure in 3 runs (20.01s vs 0.15son the passes).
Notes for reviewers
misconfigured route becomes an unbounded subprocess spawner. Worth checking the
compare-exchange in
claim_unknown_model_refresh— a lost race must mean "someone elseis refreshing", never "refresh twice".
guides/providers.mdxchanged. Theja/,ko/, andzh-cn/copies have no antigravity section at all (0 mentions), so there is nothing to translate.
Flagging explicitly given feat(xai): add grok-4.6 and refresh the Grok model surface #343.
~20sfigure in themodels.rsrustdoc did not reproduce, though one shell run didhang >120s on
Fetching available models..., so the fast numbers are likely warm-cache.Summary by cubic
Re-discovers the antigravity effort matrix when a turn names an unknown model so models added after startup resolve on the next turn. Previously the matrix never invalidated; new models omitted
--effortand failed until restart.agy models; do not block; the next turn uses the updated matrix.UNKNOWN_MODEL_REFRESH_INTERVAL) via compare-and-exchange; a non‑monotonic clock denies rather than wraps.FAILED_DISCOVERY_BACKOFF) to avoid re-forking per turn while the matrix is empty; boot-time races do not consume the slot.effort_matrixtoeffort_matrix(&Path, &str)and passroute.upstream_modelat the antigravity adapter call site; update internal callers accordingly.Written for commit 7ee7df7. Summary will update on new commits.