fix(session): keep active channels reusable - #416
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryUpdates MPP session handling for the newer PayKit contract, including voucher-signer configuration, negotiated idle timeouts, safer payer-proxy session credential caching, stablecoin-only session settlement, and catalog-cache recovery. The focused lifecycle check exercised a channel with a negotiated timeout shorter than the server close delay. It passed, disproving the concern that lifecycle touches ignore the negotiated timeout. No defects were demonstrated in the exercised behavior. T-Rex validation blocked
Confidence Score: 5/5
 T-Rex LogsWhat T-Rex did
Reviews (8): Last reviewed commit: "style(types): rustfmt the deny_unknown_f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
OpenapiSource::Path became a spec-dir-anchored local path and endpoint fields moved under ResolvedEndpoint::spec (#398); the network-gated smoke test still used the pre-#398 shapes, so it neither compiled nor ran under --all-features. Exercise the Url source it actually fetches. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Pin a569ac52, the head of pay-kit#259: reworked MPP session server with the idle-close root fix (process_commit re-arms last_activity_at) and splits-bound open verification. Cascade in pay: - session server/client moved to the reworked open/challenge API; the challenge build is now fallible and fails the challenge instead of silently degrading (tracked follow-up in pay-kit#259) - with_blockhash_cache pass-through so session challenges reuse the server's recent-blockhash observation (second #259 follow-up) - end-to-end queued-touch-discard regression test mirroring the pay#416 review harness: a heartbeat Touch dequeued after its lease is released must be discarded, not persisted The pin lands on an unmerged pay-kit branch head on purpose (re-assess now, re-point to the squash commit once #259 merges). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Greptile's remaining 4/5 finding on this PR, verified against the live catalog: discovery intersected advertised model lists with the named pricing families only, hiding models the runtime accepts and prices via the 'default' variant sentinel (the Gemini entry describes it as 'Fallback for unlisted Gemini models'). Discovery and metering now share one support policy: - providers declaring a default variant keep their full advertised list - non-chat entries are dropped by capability instead, via Gemini's supportedGenerationMethods (embeddings/imagen/aqa never reach the agent picker); OpenAI-shape lists are unchanged - default-sentinel matching is case-insensitive, matching metering's resolve_variant Providers without a default sentinel (e.g. Model Studio's named Qwen families) intersect exactly as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed three commits (
Verification at |
Follow-up to the picker/metering desync fix, from archy's verification pass: resolve_price also falls back to top-level endpoint dimensions (e.g. Model Studio's conservative list-price backstop), so the gateway runtime-prices every model for those providers too. Spell out that this fallback deliberately does not admit models to the picker — only the 'default' sentinel is the catalog author's promise that unlisted models are first-class; the dimensions backstop is quote safety at a punitive rate, and hiding those models steers users toward correctly-priced families instead of silently billing at the backstop. Doc comment only; no behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…he pre-binding split Pin PayKit 3524a11b: persisted ChannelState now round-trips unknown fields and stamps schema_version, so a draining revision or a stale-pinned worker sweeping the shared Redis can no longer strip newer fields (2026-08-01 modelstudio incident). Mirror PayKit's verify split here: a row whose binding fields are absent fails with "session channel predates proof binding; open a new session" instead of the generic mismatch. Also drop the extra echoed-challenge-id comparison PayKit's canonical check does not have — per draft-solana-session-00 the bearer proof is bound to the opening challenge only, while the outer challenge rotates freely.
…pstreams Oauth2 auth gains an optional audience and the special token_url gcp_metadata_identity, which fetches an OIDC identity token from the metadata server /identity endpoint (raw JWT; cache TTL from its exp claim). Needed to invoke IAM-protected Cloud Run micro-agents where an access token from gcp_metadata does not work. Validation is fail-closed: identity mode requires audience, audience is rejected with any other token_url, and scopes are rejected in identity mode. The token cache key now includes audience so distinct upstreams cannot evict each other. No ADC fallback: user credentials cannot mint audience-bound identity tokens.
Session challenges and operator-signed metering convert USD prices to token base units by decimal scaling alone (price_unit_base_amount), so a SOL-currency session would advertise and settle a $0.01 price as 0.01 SOL. Fail closed at boot when a session currency is not a recognized USD-pegged stablecoin instead of mispricing every voucher. x402/charge currencies are unchanged; only the session scheme gains the guard.
clean_stale_caches deleted every skills-*.json except the file its own process just wrote. Concurrent pay processes (CLI, gateways, one MCP server per agent session) refresh the cache independently, so two racing refreshes could delete each other's write and leave zero catalog files — downstream consumers then read "catalog unavailable" indefinitely. Prune only files whose filename timestamp is strictly older than the kept write; racing refreshes now converge on the newest file instead of an empty directory.
The picker's catalog discovery only ever read the cached skills index; when no cache file existed (fresh machine, or wiped by the refresh race fixed in the previous commit) it silently fell back to the built-in gateway providers, which deliberately carry no pricing metadata — so every Gemini model rendered "unpriced" in the goose/claude provider picker. On a cache miss, fetch via load_skills_for (which also rewrites the cache) before resorting to the priceless fallbacks.
Re-pins solana-pay-kit from the pre-merge branch head 3524a11b to the pay-kit#259 squash commit on main (0.4.0 -> 0.5.0) and adapts to the spec-shape wire changes: - VoucherPayload now carries the spec's REQUIRED top-level channelId; the delegated-voucher path sets it to the signed voucher's channel. - ChannelState gained processed_topup_signatures (top-up exactly-once); test fixture seeds it empty. - pdb correlation reads the renamed inner voucher key (data -> voucher, mpp-specs e702dd8), keeping the legacy key as a fallback so captures recorded before the rename still parse.
EfeDurmaz16
left a comment
EfeDurmaz16
left a comment
There was a problem hiding this comment.
The heartbeat and cancel core is solid and the earlier Greptile P1s look addressed, but five verified issues block this round: the negotiated idle timeout is never honored by the gateway touch path, the payer proxy's new preserve-on-unknown-402 semantics wedge it permanently in reachable cases including a plain gateway restart, the cached use credential can outlive a failed open, the session cap check no longer bounds the signed deposit, and legacy session YAML silently downgrades to client voucher signing. Details inline with five follow-up notes. Small body nit: the PR text still references PayKit pin 3524a11b while Cargo pins dc3a36ee.
| } | ||
| None => self.persist_touch(&channel_id, touched_at_ms).await, | ||
| } | ||
| .map_err(|error| error.to_string()); | ||
| if let Err(error) = &result { | ||
| tracing::warn!( |
There was a problem hiding this comment.
persist_touch (just below, line 703) computes the idle-close deadline from the global close_delay only and never reads the channel's negotiated idle_timeout_seconds. PayKit re-arms close_after to now plus the negotiated timeout on every accepted action, but SessionMpp::process calls touch_channel right after and the store's monotonic guard lets the longer close_delay deadline win, so a client-selected shorter option is advertised, persisted, and never honored, holding deposits up to the ten minute default. Clamping the touch deadline per channel, for example min(close_delay, state.idle_timeout_seconds * 1000) using the state touch_channel_lifecycle already returns, restores the negotiated contract; the opposite direction is already safe.
There was a problem hiding this comment.
Fixed in bbb2ff0. persist_touch now reads the channel's negotiated idle_timeout_seconds before computing the deadline and clamps to min(close_delay_ms, idle_timeout_seconds * 1000). Red test: touch_honors_negotiated_idle_timeout_shorter_than_close_delay (session.rs), confirmed failing pre-fix. Also exposed idle_timeout_options_seconds in playground-api.yaml as a client-facing example now that it's actually honored.
| @@ -587,6 +649,41 @@ async fn proxy(State(state): State<Arc<PayerState>>, req: Request) -> Response { | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The reusable use credential is cached (line 624) before the retry that carries the open credential, so a transport error or 5xx on that paid call leaves a use credential cached for a channel the gateway may never have opened; the next request then gets a challenge-less session_failed 402 that the preserve branch keeps forever. Pre-PR code cached the idempotent open credential and dropped the cache on unrecognized 402s, so it recovered. Cache the use credential only after the paid retry returns non-402, or make unrecognized session_failed rejections terminal.
There was a problem hiding this comment.
Fixed in b86e3ca. The retry's credential is now only adopted into the cache once send_upstream proves non-402 — on a transport error the cache is left exactly as it was before the attempt, never adopting the unconfirmed credential. Updated the existing test that encoded the old (buggy) assumption: renamed session_open_credential_survives_a_paid_retry_transport_error to does_not_survive and flipped its assertion to None, confirmed it fails against the old code.
| || text.contains("session_capacity_reserved") | ||
| } | ||
|
|
||
| fn terminal_cached_session_error(body: &[u8]) -> bool { |
There was a problem hiding this comment.
terminal_cached_session_error misses at least five terminal gateway messages that arrive as challenge-less 402s: unknown session channel, session credential echoes a challenge this server did not issue, session channel predates proof binding, use authentication does not match the proof bound at open, and use is only valid for operator-signed sessions. With the default in-memory store and per-process challenge binding secret, a plain gateway restart triggers the first two, and the preserve branch then replays the dead credential on every request until the payer proxy itself is restarted. Adding these to the terminal list cannot open a replacement over a live channel and restores the pre-PR self-healing.
There was a problem hiding this comment.
Fixed in b86e3ca. Pinned the five terminal error texts as pub const in pay_core::server::session::terminal_errors (used at all five error-construction sites in session.rs) and added them to terminal_cached_session_error's checks, so the two sides can't drift apart again. Test grid added: terminal_cached_session_error_recognizes_every_server_rejection (one case per message) and a negative control (terminal_cached_session_error_does_not_misclassify_a_transient_store_error) for a plain store-read failure, which must stay retryable.
| )); | ||
| }; | ||
| let required_micro = amount_as_stablecoin_micro(&request.cap, &request.currency)?; | ||
| let required = session_deposit_amount(request).unwrap_or("0"); |
There was a problem hiding this comment.
enforce_session_cap validates session_deposit_amount(request).unwrap_or("0") while pay_session_and_retry signs suggested.unwrap_or(1_000_000).max(minimum).max(1), so the enforced and signed amounts diverge: a challenge omitting both deposit fields passes a 0.10 cap yet locks 1 USDC, and a minimum above suggested enforces the smaller value but deposits the larger. Computing the deposit once with the signing expression and enforcing that value restores the pre-PR invariant where the cap bounded what was actually signed.
There was a problem hiding this comment.
Fixed in 1004c4b. Extracted resolve_session_deposit as the single source of truth for the deposit amount (suggested, floored by minimum, falling back to 1 USDC) and used it in both enforce_session_cap and pay_session_and_retry. Red test: enforce_session_cap_bounds_the_exact_amount_that_gets_signed — a challenge with neither field set now correctly rejects a $0.10 cap instead of passing it.
| Operator, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] |
There was a problem hiding this comment.
SessionSpec has no deny_unknown_fields and no aliases for the removed keys, so an existing config with settlement_authority: delegated boots without error as voucher_signer: client, silently changing who signs settlement vouchers (verified with serde_yml against head). The sibling config structs in this file already use deny_unknown_fields; adding it here, or erroring explicitly on the removed legacy keys, keeps the upgrade fail-closed.
There was a problem hiding this comment.
Fixed in 6c9c1ea. Added deny_unknown_fields to SessionSpec. This also surfaced a live instance of exactly this bug: web-ui/proxy/alibaba-qwen.yml and google-gemini.yml were still on settlement_authority: delegated (pre-9440d18), silently booting as voucher_signer: client instead of operator. Migrated both to voucher_signer: operator in the same commit and verified all three shipped session configs (these two + playground-api.yaml) parse cleanly under the new attribute.
| (None, Some(settlement)) => Some(settlement), | ||
| (None, None) => None, | ||
| } | ||
| next_lifecycle_wakeup(close, settlement) | ||
| } | ||
|
|
There was a problem hiding this comment.
Pre-existing rather than introduced here, noting for a follow-up: when the settle in operator_close_channel fails after close_requested_at is set, no automatic path retries it, since close_due_channels filters on close_requested_at.is_none() (line 845) and the watermark and reconcile scans skip close-pending channels, so the retrying-after-delay log is wrong and only an authenticated wire close recovers the channel. Dropping the is_none clause from the close scan filter would let the existing Close-already-requested tolerance finish the settle.
There was a problem hiding this comment.
Agreed, tracked as a follow-up rather than fixed in this round. Flagging the pattern match: this is the Rust twin of pay-kit#259's close re-drive blocker (close-pending channels invisible to every retry scan because of the same is_none()-style filter) — that fix's pattern (drop the filter / re-drive stranded settles) should port directly. Noted in the PR body.
| pub fn challenge(&self, amount: Option<u64>) -> Result<PaymentChallenge> { | ||
| let mut request = self | ||
| .server | ||
| .build_challenge_request() |
There was a problem hiding this comment.
Follow-up: during an RPC outage past the 45s blockhash cache window every session 402 fails as 500 challenge_generation_failed, which also blocks resume actions that never consume the hints and suppresses the endpoint's other schemes. The pay-kit migration note wanted fail-the-challenge for new channels only; issuing a hint-less session challenge when build_challenge_request fails keeps resume alive while new opens still fail loudly.
There was a problem hiding this comment.
Legitimate, tracked as a follow-up, not addressed in this round. Noted in the PR body's review-response section.
| .close_action(Some(increment)) | ||
| .await | ||
| .map_err(|e| Error::Mpp(format!("Failed to build close action: {e}")))?, | ||
| _ => SessionAction::Close(ClosePayload { |
There was a problem hiding this comment.
Follow-up: close_header(None) on a client-signed channel that never sent a voucher signs a cumulative-0 voucher the pinned process_close always rejects, since the equal-watermark replay check needs a byte match with a stored signature that does not exist, and the nonzero case passes only through deterministic re-signing of the identical message. Accepting a freshly signed voucher at the current watermark server-side, or guarding the zero case here, plus an end-to-end test for client-mode close_header(None), would close it.
There was a problem hiding this comment.
Legitimate, tracked as a follow-up, not addressed in this round. Noted in the PR body's review-response section.
| "status": { "Ok": null } | ||
| }] | ||
| }), | ||
| "getAccountInfo" => serde_json::json!({ |
There was a problem hiding this comment.
Follow-up: the canned RPC answers getAccountInfo with the pre-built channel regardless of the requested address and confirms any signature, and this test is now the only integration coverage of the server-broadcast open path, so a wrong-account or wrong-signature regression would still pass. One-line hardening per arm: assert the requested address equals the channel id, and the queried signature equals the one echoed from sendTransaction.
There was a problem hiding this comment.
Agreed — the canned RPC's blanket getAccountInfo/signature matching is a real coverage gap now that this is the only integration coverage of the server-broadcast open path. Tracked as a follow-up, not addressed in this round.
| if configured_models.is_empty() { | ||
| return advertised_models; | ||
| } | ||
| advertised_models |
There was a problem hiding this comment.
Follow-up, minor: if a provider has no default pricing variant and none of its configured families match the advertised list, this filter returns an empty Vec with no fallback, unlike the configured_models.is_empty() guard above. Unreachable in shipped config and the picker degrades to a no-models-reported row, so low stakes; falling back to configured_models would be safer.
There was a problem hiding this comment.
Agreed, minor and unreachable in shipped config today, but worth the guard for defense-in-depth. Tracked as a follow-up, not addressed in this round.
SessionSpec was the only session-related struct missing deny_unknown_fields, so a stale/renamed key silently drops instead of failing to parse. Concretely: the settlement_authority -> voucher_signer rename in 9440d18 left web-ui/proxy/alibaba-qwen.yml and google-gemini.yml still keyed on settlement_authority: delegated, which has been silently defaulting voucher_signer to client (the operator was never signing settlement) since that rename. Migrate both to voucher_signer: operator and drop the dead modes/pull_voucher_strategy keys (removed from the struct in the same rework) from those two files and playground-api.yaml.
persist_touch computed close_after from the operator's global close_delay_ms only, ignoring the channel's negotiated idle_timeout_seconds. Since the store's touch is monotonic (close_after only moves forward), a shorter negotiated timeout could never win against the longer default once any touch used it — holding deposits open up to close_delay_ms past what the client selected. Clamp the effective delay to min(close_delay, idle_timeout_seconds) by reading the channel's negotiated timeout before computing the deadline.
Follow-up to earlier question: the field already existed on SessionSpec and was wired end-to-end, just undocumented by example. Add it now that the idle-timeout clamp (bbb2ff0) actually honors it.
…signed enforce_session_cap and pay_session_and_retry independently derived the deposit from the same SessionRequest but with different fallbacks: the cap check treated a challenge with no suggested/minimum deposit as requiring 0 (always passing), while signing defaulted that same case to 1 USDC. A challenge omitting both fields passed a $0.10 cap and then locked 1 USDC. Extract resolve_session_deposit as the single source of truth for the deposit amount and use it on both the enforcement and signing side, so the two call sites cannot drift again.
Two bugs compounded into a wedge that only cleared on a manual proxy restart: 1. terminal_cached_session_error only recognized 6 of the server's session-failure texts. Five real, permanently-fatal rejections (unknown channel, forged challenge echo, wrong voucher signer, pre-binding channel, proof mismatch) were treated as transient, so the proxy kept preserving and retrying a dead cached credential forever instead of renegotiating. 2. The paid retry's credential was cached before its outcome was known. A transport error on that retry (e.g. a gateway restart) left a credential cached for a channel that may never have opened, which then surfaced as one of the five unrecognized terminal errors above on the next request. Pin the five terminal error texts as shared constants in pay_core::server::session::terminal_errors, used by both the error sites and the proxy's classifier so they can't drift apart again. Only adopt the retry's credential once the retry proves non-402; on a transport error, leave the cache exactly as it was so the next request renegotiates instead of trusting an unconfirmed credential. Updates session_open_credential_survives_a_paid_retry_transport_error (renamed to _does_not_survive_): the old assertion encoded exactly the bug being fixed here.
EfeDurmaz16
left a comment
EfeDurmaz16
left a comment
There was a problem hiding this comment.
All five blockers are properly fixed at adc8e4d, traced in code: persist_touch now clamps the deadline to the negotiated idle timeout, the retry credential is only adopted after a non-402 upstream response, the five terminal rejection texts are pinned as shared consts with a classifier test covering every server rejection, resolve_session_deposit is the single source for both the cap check and the signed amount, and SessionSpec rejects unknown fields (nice catch on the two live web-ui configs). Tracked follow-ups in the threads are fine as scoped, the close re-drive twin especially. CI is green.
2 participants
Summary
session.close_delay_msvalues to ten minutesdc3a36ee(merged squash of feat(mpp/session)!: final session wire contract + open context — Rust, TypeScript, Python (Go/Kotlin/Swift follow-up) pay-kit#259: reworked MPP session API + idle-close root fix + old-writer-safe channel records) and cascade: fallible challenge build that fails the challenge instead of silently degrading,with_blockhash_cachepass-through, end-to-end queued-touch-discard regression testdefaultpricing-variant sentinel keeps the full advertised model list (runtime prices unlisted models via that sentinel); non-chat entries are dropped by capability (supportedGenerationMethods) instead of by pricing; providers without a sentinel intersect with named families as beforePicker/metering policy note
resolve_pricealso falls back to top-level endpointdimensions(e.g. Model Studio's conservative list-price backstop), so the runtime technically prices every model for those providers too. That fallback deliberately does not admit models to the picker: only thedefaultsentinel is the catalog author's promise that unlisted models are first-class; the dimensions backstop is quote safety at a punitive rate, and hiding those models steers users toward correctly-priced families instead of silently billing at the backstop. Documented onmodels_matching_configured_families.pay-kit#259has merged anddc3a36ee(d829fa0) is the final pin — no further re-point pending.2026-08-01 incident follow-up (
367dd9f): mirrors PayKit's new pre-binding row split ("session channel predates proof binding" instead of the generic mismatch for rows that predate — or were stripped of — the proof binding), and drops the extra echoed-challenge-id comparison PayKit's canonical check doesn't have. Deploy alongside agent-gateway#14 (settlement worker pin) and the four sibling gateways.GCP identity tokens for IAM-locked upstreams (
bc95dca)Folded in per pay-cli thread:
oauth2auth gains an optionalaudienceand the specialtoken_url: gcp_metadata_identity, minting an audience-bound OIDC identity token from the metadata server's/identityendpoint (raw JWT; cache TTL from its ownexpclaim). This is what invoking IAM-protected Cloud Run services (scale-to-zero micro-agents) needs — agcp_metadataaccess token doesn't work there. Fail-closed validation: identity mode requiresaudience;audienceorscopeswith the wrongtoken_urlare spec errors with actionable messages. Token cache key now includes audience. No ADC fallback — user credentials cannot mint audience-bound identity tokens, and the error says so.Picker "unpriced" root cause: skills cache wiped by racing refreshes (
6f773a7,7d3e568)The recurring all-models-unpriced picker state had a second cause beyond the family-intersect bug fixed in
2a6b14f:clean_stale_cachesdeleted everyskills-*.jsonexcept the file its own process just wrote, so two concurrent catalog refreshes (there is onepay mcpper agent session) could delete each other's write and leave zero cache files. With no cached catalog, picker discovery silently fell back to the built-ingoogle_gemini_fallback/alibaba_modelstudio_fallbackproviders, which deliberately carry no pricing — every model rendered "unpriced". Reproduced live (cache absent → 11 Gemini models, all unpriced) and verified fixed (same state → catalog fetched, 42 models, all priced).6f773a7(core): prune only cache files whose filename timestamp is strictly older than the kept write — racing refreshes converge on the newest file instead of an empty directory.7d3e568(cli): on cache miss, picker discovery fetches viaload_skills_for(which rewrites the cache) before resorting to the priceless fallbacks — self-heals even against older installed binaries that still race.Review response (
6c9c1ea..adc8e4d)Fixes for @EfeDurmaz16's five blockers (inline replies posted on each comment):
6c9c1ea—SessionSpecnow hasdeny_unknown_fields; migrates the two live gateway configs (alibaba-qwen.yml,google-gemini.yml) off the removedsettlement_authority: delegatedkey, which had been silently defaultingvoucher_signertoclientsince this PR's own9440d18renamebbb2ff0—persist_touchnow clamps the idle-close deadline to the channel's negotiatedidle_timeout_seconds, not just the operator'sclose_delay_ms1004c4b—enforce_session_capand the signing path now share oneresolve_session_deposit, so the enforced cap can no longer diverge from what actually gets signedb86e3ca— payer proxy: five previously-unrecognized terminal session errors (pinned as shared constants inpay_core::server::session::terminal_errors) plus a cache-order fix so an unconfirmed credential is never adopted before its retry proves non-402The five follow-ups are tracked as-is, not fixed in this round; each has an inline acknowledgment on the PR. Of note, the
operator_close_channelsettle-retry gap is the Rust twin ofpay-kit#259's close re-drive blocker and should port the same fix pattern.Test plan
At
adc8e4d:cargo test --workspace --all-features— 26 targets, 1768 passed, 0 failed (includes the repaired live-network openapi smoke test)cargo fmt --all --check— cleancargo clippy --workspace --all-features --all-targets— cleancargo check -p pay(default features) — clean