Repository navigation
fix(music): bound the poll loop by the payment authorization, not just the poll budget - #113
Merged
Merged
Conversation
…t the poll budget blockrun_video got this in #111; music has the same shape and the same latent bug, one budget bump away from firing. music measures its 240s poll budget from AFTER submit, but validBefore is counted from signing — and submit is allowed 95s. So the real worst case from the authorization's own origin is 95 + 240 + 5 + 90 = 430s. That fits inside 600s today, which is exactly why it looks fine and why the next person to raise MUSIC_POLL_BUDGET_MS the way #110 raised video's would walk straight into it: at 420s the sum is 610s and settlement starts failing for tracks that rendered. Naively copying video's fix (stamp startedAt before signing) would have been worse than the bug: the 240s budget would then absorb up to 95s of submit, leaving as little as 145s of polling for MiniMax tracks the code documents as taking 60-180s. So the loop takes the EARLIER of two deadlines instead — the poll budget, and the authorization minus settle-side slack. A fast submit keeps the full 240s window; a slow one shortens it rather than silently pushing polls past validBefore. - share pollTimeoutFor from src/utils/poll.ts, used by both tools - deliberately not in http.ts: four test files replace that module with a network sentinel, so a pure function there forces them to stub arithmetic - video.ts: repair the indentation break at the spend-booking site and drop the redundant inner !spendBooked guard already implied by the enclosing condition - cover the clamp in test/poll.test.ts and the music invariant in music-cost
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the three loose ends from the #110/#111 review. No user-visible behaviour changes on the happy path.
1.
blockrun_musichas video's bug, latentmusic.tsmeasures its 240s poll budget from after submit, butvalidBeforeis counted from signing, and submit is allowed 95s. Worst case from the authorization's own origin:Fits today. That's precisely why it reads as fine — and why the next person to raise
MUSIC_POLL_BUDGET_MSthe way #110 raised video's walks into it. At 420s the sum is 610s, and settlement starts failing for tracks that actually rendered.Copying video's fix would have been worse than the bug. Video stamps
startedAtbefore signing; do that here and the 240s budget absorbs up to 95s of submit, leaving as little as 145s of polling for MiniMax tracks the code itself documents as taking 60-180s. That breaks real jobs to fix a hypothetical one.So the loop takes the earlier of two deadlines: the poll budget, and the authorization minus settle-side slack. Fast submit keeps the full 240s window; slow submit shortens it instead of silently pushing polls past
validBefore.2. Shared clamp, in a module nobody mocks
pollTimeoutFormoves tosrc/utils/poll.tsand serves both tools.It is deliberately not in
http.ts. I put it there first and four test files broke: they replacehttp.jswholesale with a network sentinel, so a pure arithmetic function living besidefetchWithTimeoutforces every one of them to stub arithmetic just to import the tool under test. The mocks were right and the module boundary was wrong.3. The
video.tsindentation defectFlagged as pre-existing during the #110 review. The comment and statement at the spend-booking site were indented out to the enclosing scope, and the inner
if (!spendBooked)was already implied byif (lastStatus === "completed" && !spendBooked). Both cleaned up; behaviour identical.Deliberately not changed
The review also flagged that spend is booked the moment
"completed"is observed, without confirming settlement succeeded. Leaving it alone on purpose: that booking errs toward over-recording, which makes the agent's cap conservative and is the safe direction. Gating it on a receipt would introduce under-recording — real USDC spent that the ledger never sees, silently raising the cap — which is the exact bug the current ordering was written to fix. With the clamps in place the expired-authorization tail is unreachable in both tools anyway.Validation
npm test— 345 passed (was 343; +2 new)npm run typecheck,npm run build— cleanMath.min(maxTimeoutMs, remainingMs)to a baremaxTimeoutMsand confirmed both guards fail (poll.test.tsand the video clamp test), so neither is decorative