run all scenarios - #7
Conversation
jeroenubbink
left a comment
There was a problem hiding this comment.
Reviewed both halves of this — the "Run all" UI/orchestrator work and the mint-429 fallback e2e. I traced the e2e's assumptions against the current fix/mint-rate-limit-and-fallback head (Routstr/routstr-core#597): the mint_url column (migration 11eaab843b49), the _request_mint_with_fallback candidate ordering, and the /v1/mint/quote/bolt11 fault path all line up, and the fault-proxy changes stay backward-compatible with the existing swap test (kind defaults to melt_insufficient, melt fault still armed, stats/reset shapes are additive). Nice — the real-wire value here is the part unit tests can't reach: fallback through a mint returning an actual 429, plus the persisted mint_url surviving in the DB. That earns its keep.
A few things, none blocking:
1. The topup assertion is labeled for a mechanism the code doesn't have. test_429_fallback_persists_mint_and_topup_stays_on_backing_mint asserts the top-up landed on the secondary "instead of staying on the API key's backing mint." But #597 doesn't pin top-ups to a backing mint — its own comment in create_invoice says "Top-ups are not pinned to the key's previous/backing mint", and top-ups walk the same _request_mint_with_fallback candidate list as creates. The reason the top-up skips the primary is that the create's 429 put it in a 60s rate-limit cooldown (_MINT_RATE_LIMIT_BASE_COOLDOWN_SECONDS, which max()es over the Retry-After: 0 you send), and the cooldown persists on the shared per-mint guard across the two requests. So the assertion verifies cooldown-persistence but names it "backing mint" — if someone later tweaks the cooldown (you're actively iterating on it), this fails with a message pointing at logic that was never there.
Worth noting #597 already covers this at the unit level and names it honestly: test_wallet_fallback_skips_mint_during_cooldown does the same thing (arms apply_cooldown(60) on the primary, asserts fallback to secondary + primary_wallet.request_mint.assert_not_awaited()), and test_lightning_mint_fallback_on_429 covers the create-path 429. So the topup half here is mostly re-treading already-unit-covered cooldown logic. Suggest either dropping the topup block (keep the genuinely e2e-only bits — real-wire 429 fallback + persisted mint_url) or relabeling it to match the cooldown mechanism your core tests already use.
2. No scenario wires this up — it's a manual-only discriminator. It only runs via make mint-rate-limit-fallback-test with ROUTSTR_CORE_REF=refs/pull/597/head; on a normal stack the topology check trips unavailable and it skips. That's fine while #597 is in flight, but there's no scenarios/*.yaml for it like swap_foreign_mint_retry has, so once #597 lands it won't run anywhere and will quietly rot. Could you add a services_required: true scenario with the fault-proxy-first topology when #597 merges (or now, gated)?
3. "Run all" flattens each scenario's target_profile/upstream_profile. It applies one chosen profile+token to every scenario in the loop and defaults to remote. Local services_required: true scenarios (e.g. swap_foreign_mint) then run with no bring-up and destructive-skip → vacuous green, and it can also fire the real-upstream paid scenarios (openai_chat_completions, real_inference). The summed cost preview covers the money surprise, but the vacuous-local case is worth a guard or at least a note in the modal.
4. The token export isn't remote-only. The if token: block in orchestrate.py runs for local scenarios too, so NODE_A_API_KEY/X_CASHU_TOKENS get set to a raw Cashu token even locally. The comment frames it as intentional (direct-node tests accept raw Cashu), and it's guarded against clobbering operator env — but only the remote path is unit-covered (test_orchestrate_remote_profile_...). Worth confirming no local test reads NODE_A_API_KEY expecting an sk- key.
5. Empty body + the title covers half the diff. "run all scenarios" doesn't hint at the #597 fault-proxy/e2e work or the unrelated webui/pnpm-workspace.yaml esbuild change. The two commits are cleanly split (good, since these merge unsquashed), so no need to split the PR — but a body describing both concerns would help whoever bisects this later.
No description provided.