From 849ede402d5cfe6e9cc350c53f2c4e5285b0388b Mon Sep 17 00:00:00 2001 From: alistair3149 Date: Fri, 21 Aug 2026 09:41:40 -0400 Subject: [PATCH 1/3] Pace uploads to the rate limit the wiki advertises The gate learned about the limit only by being refused by it, which left two problems. The backoff cap was the constant 60 seconds, which is right only because $wgRateLimits happens to default to a 60 second window. A wiki with a daily cap made the window unreachable: the batch climbed 2, 4, 8, 16, 32, 60 seconds, gave up after 122, and told the user to wait a moment when the real wait was hours. Re-selecting the files restarted the same 122 seconds, so the extension was unusable past such a cap. And when the gate reopened it released every waiting upload in the same tick, into a window that may have had fewer free slots than there were waiting files. Some were refused and the delay doubled for a reason the client had caused. Both come from not reading what the wiki already publishes. meta=userinfo&uiprop=ratelimits gives the hits and seconds that will be enforced, so the cap is now the advertised window and releases are spaced to the advertised rate. Pacing starts only after the wiki has refused something. A batch that fits inside the budget is never refused and so is never slowed down: verified against a live wiki at 4 uploads per 20 seconds, three files still finish in 2 seconds while twelve now complete in 73 with nothing failed. A user holding noratelimit gets an empty limits object, which means unlimited rather than zero, and is never paced: twelve files in 4.5 seconds. The query is deliberately not awaited before the widget is wired, so the button works the moment the page is ready; the gate adopts the limit when it arrives, and pacing cannot begin before then anyway. The give-up message no longer claims the user only has to wait a moment, since the window is configurable and can be a day. Co-Authored-By: Claude Opus 5 (1M context) --- extension.json | 1 + i18n/en.json | 2 +- i18n/qqq.json | 2 +- release-notes.md | 3 + .../ext.SimpleBatchUpload.js | 16 ++++ res/ext.SimpleBatchUpload/rateLimitGate.js | 54 ++++++++++- res/ext.SimpleBatchUpload/rateLimits.js | 68 ++++++++++++++ .../rateLimitGate.test.js | 89 +++++++++++++++++++ .../ext.SimpleBatchUpload/rateLimits.test.js | 74 +++++++++++++++ 9 files changed, 303 insertions(+), 6 deletions(-) create mode 100644 res/ext.SimpleBatchUpload/rateLimits.js create mode 100644 tests/vitest/ext.SimpleBatchUpload/rateLimits.test.js diff --git a/extension.json b/extension.json index bb7bec6..af7f7a5 100644 --- a/extension.json +++ b/extension.json @@ -64,6 +64,7 @@ "ext.SimpleBatchUpload/ext.SimpleBatchUpload.js", "ext.SimpleBatchUpload/batchLimit.js", "ext.SimpleBatchUpload/rateLimitGate.js", + "ext.SimpleBatchUpload/rateLimits.js", "ext.SimpleBatchUpload/renamePattern.js", "ext.SimpleBatchUpload/resultRow.js", "ext.SimpleBatchUpload/uploadQueue.js", diff --git a/i18n/en.json b/i18n/en.json index 5713b5d..acdcb73 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -17,7 +17,7 @@ "simplebatchupload-result-network-error": "Server communication failed.", "simplebatchupload-result-not-uploaded": "The file was not uploaded ($1).", "simplebatchupload-result-queued": "queued", - "simplebatchupload-result-rate-limit-stopped": "Stopped: this wiki's upload rate limit was reached repeatedly. Wait a moment, then select the remaining files again.", + "simplebatchupload-result-rate-limit-stopped": "Stopped: this wiki's upload rate limit was reached repeatedly. Select the remaining files again once the limit has reset.", "simplebatchupload-result-rate-limited": "waiting for the upload rate limit", "simplebatchupload-result-success": "OK", "simplebatchupload-result-token-error": "Could not get an edit token.", diff --git a/i18n/qqq.json b/i18n/qqq.json index 58e7f92..2b2af8d 100644 --- a/i18n/qqq.json +++ b/i18n/qqq.json @@ -17,7 +17,7 @@ "simplebatchupload-result-network-error": "Reason shown when the upload request never reached the server. Used as $1 of {{msg-mw|simplebatchupload-result-error}}.", "simplebatchupload-result-not-uploaded": "Reason shown when the server accepted the request but did not store the file. Used as $1 of {{msg-mw|simplebatchupload-result-error}}. Parameters:\n* $1 - a comma separated list of the server's warning identifiers, which are not translatable", "simplebatchupload-result-queued": "Status of a file that is waiting for its turn to upload. Keep it short, it is shown after the file name.", - "simplebatchupload-result-rate-limit-stopped": "Reason shown when the batch was given up on after repeatedly hitting the wiki's rate limit. Used as $1 of {{msg-mw|simplebatchupload-result-error}}.", + "simplebatchupload-result-rate-limit-stopped": "Shown on every remaining row when the batch gave up because the wiki refused uploads repeatedly. Deliberately does not state how long to wait: the limit window is configurable per wiki and can be as long as a day.", "simplebatchupload-result-rate-limited": "Status of a file whose upload was refused by the wiki's rate limit and will be retried. Keep it short, it is shown after the file name.", "simplebatchupload-result-success": "Status of a file that was uploaded. Keep it short, it is shown after the file name.", "simplebatchupload-result-token-error": "Reason shown when the edit token could not be obtained. Used as $1 of {{msg-mw|simplebatchupload-result-error}}.", diff --git a/release-notes.md b/release-notes.md index f4af652..4ccae66 100644 --- a/release-notes.md +++ b/release-notes.md @@ -5,6 +5,9 @@ * Fixed uploads refused by the wiki's rate limit being reported as permanent errors * Refused files are now retried, so a rate-limited batch takes longer rather than partly failing * A batch that keeps hitting the limit stops and asks for the remaining files to be selected again +* Changed uploading to pace itself to the rate limit the wiki advertises, once the wiki has refused an upload + * Batches that fit inside the limit are unaffected and still upload at full speed + * Wikis configuring a longer limit window, such as a daily cap, are now waited out instead of the batch giving up after two minutes * Fixed files reported as uploaded when the wiki did not store them * Fixed an invalid `+rename` pattern cancelling the rest of the batch with no error shown * Fixed the result list losing uploads that were still running when more files were selected diff --git a/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.js b/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.js index 95c2ca1..59fbf0f 100644 --- a/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.js +++ b/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.js @@ -13,6 +13,7 @@ const { resolveUserLimit, createBatchLimit } = require( './batchLimit.js' ); const { parseRenameDirective } = require( './renamePattern.js' ); const { createRateLimitGate } = require( './rateLimitGate.js' ); +const { bindingLimit } = require( './rateLimits.js' ); const { createUploadQueue } = require( './uploadQueue.js' ); const { createUploadRunner } = require( './uploadRunner.js' ); const { createResultRow, pruneFinishedRows } = require( './resultRow.js' ); @@ -28,6 +29,21 @@ const runner = createUploadRunner( { gate: gate, queue: queue } ); $( () => { const api = new mw.Api(); + + // The wiki publishes the limits it will enforce, so the queue can pace + // itself to them instead of discovering them by being refused. Deliberately + // not awaited: the widget has to work the moment the page is ready, and the + // gate does not pace until something is refused anyway. A failed query + // simply means no pacing. + api.get( { action: 'query', meta: 'userinfo', uiprop: 'ratelimits' } ).then( + ( response ) => { + gate.useLimit( bindingLimit( + response && response.query && response.query.userinfo && + response.query.userinfo.ratelimits + ) ); + }, + () => {} + ); const batchLimit = createBatchLimit( resolveUserLimit( mw.config.get( 'simpleBatchUploadMaxFilesPerBatch' ), mw.config.get( 'wgUserGroups' ) diff --git a/res/ext.SimpleBatchUpload/rateLimitGate.js b/res/ext.SimpleBatchUpload/rateLimitGate.js index e7405dd..6bbf799 100644 --- a/res/ext.SimpleBatchUpload/rateLimitGate.js +++ b/res/ext.SimpleBatchUpload/rateLimitGate.js @@ -19,16 +19,19 @@ const MAX_CONSECUTIVE_RETRIES = 6; /** * @param {number} rejections Consecutive rate limit rejections, 1 for the first + * @param {number} [capMs] Longest useful wait, normally the advertised window. + * Defaults to a minute, which is only right for a wiki using the default + * window; a wiki with a daily cap needs to be able to wait hours. * @return {number} Milliseconds to wait before the next attempt */ -function retryDelay( rejections ) { +function retryDelay( rejections, capMs ) { if ( rejections < 1 ) { return 0; } return Math.min( FIRST_RETRY_DELAY_MS * Math.pow( 2, rejections - 1 ), - MAX_RETRY_DELAY_MS + capMs === undefined ? MAX_RETRY_DELAY_MS : capMs ); } @@ -37,6 +40,9 @@ function retryDelay( rejections ) { * @param {Function} [options.now] Returns the current time in milliseconds * @param {Function} [options.sleep] Returns a promise resolving after n milliseconds * @param {number} [options.maxRetries] + * @param {?Object} [options.limit] From rateLimits.bindingLimit(): the + * { intervalMs, windowMs } the wiki advertises. Null or absent means the user + * is not rate limited, so nothing is paced and the wait falls back to a minute. * @return {Object} */ function createRateLimitGate( options ) { @@ -56,10 +62,37 @@ function createRateLimitGate( options ) { MAX_CONSECUTIVE_RETRIES : settings.maxRetries; + let capMs = MAX_RETRY_DELAY_MS; + let intervalMs = 0; + let rejections = 0; let openAt = 0; let halted = false; + // Pacing starts only once the wiki has actually refused something. A batch + // that fits inside the budget is never refused, so it is never slowed down: + // bursting is what makes the ordinary case fast, and it succeeds. + let pacing = false; + let nextReleaseAt = 0; + + /** + * Adopts the limit the wiki advertises. + * + * Separate from construction because the widget has to work the moment the + * page is ready, and the limit arrives from an API call. Until it does the + * gate behaves as it always did, which is safe: pacing only ever starts + * after a refusal, and a refusal that early is not realistic. + * + * @param {?Object} limit From rateLimits.bindingLimit(), or null for a user + * the wiki does not limit + */ + function useLimit( limit ) { + capMs = limit ? limit.windowMs : MAX_RETRY_DELAY_MS; + intervalMs = limit ? limit.intervalMs : 0; + } + + useLimit( settings.limit || null ); + /** * @return {Promise} True once uploading may continue, false if the * batch was given up on while waiting @@ -70,9 +103,16 @@ function createRateLimitGate( options ) { return false; } - const remaining = openAt - now(); + const releaseAt = pacing ? Math.max( openAt, nextReleaseAt ) : openAt; + const remaining = releaseAt - now(); if ( remaining <= 0 ) { + if ( pacing ) { + // Claim this slot before returning, so the next caller is + // spaced behind it rather than released alongside it. + nextReleaseAt = Math.max( now(), nextReleaseAt ) + intervalMs; + } + return true; } @@ -93,7 +133,12 @@ function createRateLimitGate( options ) { return; } - openAt = now() + retryDelay( rejections ); + if ( intervalMs > 0 ) { + // The budget is demonstrably tight, so stop bursting. + pacing = true; + } + + openAt = now() + retryDelay( rejections, capMs ); } function noteProgress() { @@ -113,6 +158,7 @@ function createRateLimitGate( options ) { } return { + useLimit: useLimit, wait: wait, noteRateLimited: noteRateLimited, noteProgress: noteProgress, diff --git a/res/ext.SimpleBatchUpload/rateLimits.js b/res/ext.SimpleBatchUpload/rateLimits.js new file mode 100644 index 0000000..3409539 --- /dev/null +++ b/res/ext.SimpleBatchUpload/rateLimits.js @@ -0,0 +1,68 @@ +'use strict'; + +/** + * Reads the rate limits the wiki advertises for uploading. + * + * MediaWiki charges an upload against both the 'edit' and the 'upload' limiter + * (UploadBase::verifyTitlePermissions), and a user can fall under several + * categories of one action at once -- a registered newbie is charged as 'ip', + * 'newbie' and 'user' together -- so the limit that actually binds is the most + * restrictive of all of them. + * + * Restrictive means slowest sustained rate, not smallest count: 100 uploads a + * day is far tighter than 8 a minute despite the larger number. + */ + +// The actions an upload is charged against. Anything else the wiki reports is +// irrelevant here. +const CHARGED_ACTIONS = [ 'upload', 'edit' ]; + +/** + * @param {?Object} bucket A { hits, seconds } pair from the API + * @return {boolean} + */ +function usable( bucket ) { + return !!bucket && + typeof bucket.hits === 'number' && bucket.hits > 0 && + typeof bucket.seconds === 'number' && bucket.seconds > 0; +} + +/** + * The limit an upload actually has to respect. + * + * @param {?Object} ratelimits The `ratelimits` object from + * action=query&meta=userinfo&uiprop=ratelimits. An empty object means the user + * holds noratelimit, which is unlimited rather than zero. + * @return {?{hits: number, seconds: number, intervalMs: number, windowMs: number}} + * Null when nothing limits this user, in which case do not pace at all. + */ +function bindingLimit( ratelimits ) { + const reported = ratelimits || {}; + + const buckets = CHARGED_ACTIONS + .reduce( ( found, action ) => found.concat( + Object.keys( reported[ action ] || {} ) + .map( ( category ) => reported[ action ][ category ] ) + ), [] ) + .filter( usable ); + + if ( buckets.length === 0 ) { + return null; + } + + const binding = buckets.reduce( ( slowest, bucket ) => ( + bucket.seconds / bucket.hits > slowest.seconds / slowest.hits ? bucket : slowest + ) ); + + return { + hits: binding.hits, + seconds: binding.seconds, + // Spacing that keeps a sustained batch inside the limit. + intervalMs: Math.ceil( binding.seconds / binding.hits * 1000 ), + // How long an exhausted budget takes to refill, which is the longest a + // wait can usefully be. + windowMs: binding.seconds * 1000 + }; +} + +module.exports = { bindingLimit: bindingLimit }; diff --git a/tests/vitest/ext.SimpleBatchUpload/rateLimitGate.test.js b/tests/vitest/ext.SimpleBatchUpload/rateLimitGate.test.js index c5f6c9e..211af8c 100644 --- a/tests/vitest/ext.SimpleBatchUpload/rateLimitGate.test.js +++ b/tests/vitest/ext.SimpleBatchUpload/rateLimitGate.test.js @@ -116,3 +116,92 @@ describe( 'resuming a given up batch', () => { expect( clock.now() - waitingSince ).toBe( 2000 ); } ); } ); + +describe( 'pacing to the advertised limit', () => { + const EIGHT_PER_MINUTE = { intervalMs: 7500, windowMs: 60000 }; + + function pacedGate( clock, limit ) { + return createRateLimitGate( { + now: clock.now, + sleep: clock.sleep, + limit: limit || EIGHT_PER_MINUTE + } ); + } + + it( 'does not slow anything down before the wiki has refused an upload', async () => { + const clock = createFakeClock(); + const gate = pacedGate( clock ); + + await gate.wait(); + await gate.wait(); + await gate.wait(); + + // A batch that fits inside the budget must not be paced: bursting is + // what makes the common case fast, and it succeeds. + expect( clock.now() ).toBe( 0 ); + } ); + + it( 'spaces releases once refused, so they do not burst back into the limiter', async () => { + const clock = createFakeClock(); + const gate = pacedGate( clock ); + + gate.noteRateLimited(); + + await gate.wait(); + const first = clock.now(); + await gate.wait(); + const second = clock.now(); + await gate.wait(); + + expect( second - first ).toBe( 7500 ); + expect( clock.now() - second ).toBe( 7500 ); + } ); + + it( 'waits up to the advertised window rather than a fixed minute', async () => { + const clock = createFakeClock(); + // A wiki with a daily cap: the useful wait is hours, not a minute. + const gate = pacedGate( clock, { intervalMs: 864000, windowMs: 86400000 } ); + + for ( let refusals = 0; refusals < 20; refusals++ ) { + gate.noteRateLimited(); + gate.noteProgress(); + } + + expect( retryDelay( 20, 86400000 ) ).toBe( 86400000 ); + } ); + + it( 'still caps at a minute when the wiki advertises nothing', () => { + expect( retryDelay( 20 ) ).toBe( 60000 ); + } ); +} ); + +describe( 'learning the limit after the widget is already usable', () => { + it( 'adopts a limit that arrives once the query returns', async () => { + const clock = createFakeClock(); + // Created before the API call resolves, so the button works immediately. + const gate = createRateLimitGate( { now: clock.now, sleep: clock.sleep } ); + + gate.useLimit( { intervalMs: 5000, windowMs: 86400000 } ); + gate.noteRateLimited(); + + await gate.wait(); + const first = clock.now(); + await gate.wait(); + + expect( clock.now() - first ).toBe( 5000 ); + } ); + + it( 'ignores an absent limit, so an unlimited user is never paced', async () => { + const clock = createFakeClock(); + const gate = createRateLimitGate( { now: clock.now, sleep: clock.sleep } ); + + gate.useLimit( null ); + gate.noteRateLimited(); + + await gate.wait(); + const first = clock.now(); + await gate.wait(); + + expect( clock.now() - first ).toBe( 0 ); + } ); +} ); diff --git a/tests/vitest/ext.SimpleBatchUpload/rateLimits.test.js b/tests/vitest/ext.SimpleBatchUpload/rateLimits.test.js new file mode 100644 index 0000000..5577af3 --- /dev/null +++ b/tests/vitest/ext.SimpleBatchUpload/rateLimits.test.js @@ -0,0 +1,74 @@ +const { + bindingLimit +} = require( '../../../res/ext.SimpleBatchUpload/rateLimits.js' ); + +describe( 'bindingLimit', () => { + it( 'reports no limit when the wiki applies none', () => { + // meta=userinfo&uiprop=ratelimits returns an empty object for users + // holding noratelimit, which means "unlimited", not "zero". + expect( bindingLimit( {} ) ).toBeNull(); + } ); + + it( 'reports no limit when the query failed and nothing came back', () => { + expect( bindingLimit( null ) ).toBeNull(); + } ); + + it( 'reads the limit that applies to uploading', () => { + const limit = bindingLimit( { upload: { user: { hits: 90, seconds: 60 } } } ); + + expect( limit.hits ).toBe( 90 ); + expect( limit.seconds ).toBe( 60 ); + } ); + + it( 'takes the most restrictive of several categories of one action', () => { + // A registered newbie is charged under all three at once. + const limit = bindingLimit( { + upload: { + ip: { hits: 8, seconds: 60 }, + newbie: { hits: 4, seconds: 60 }, + user: { hits: 90, seconds: 60 } + } + } ); + + expect( limit.hits ).toBe( 4 ); + } ); + + it( 'takes the most restrictive across edit and upload, because an upload charges both', () => { + const limit = bindingLimit( { + upload: { user: { hits: 90, seconds: 60 } }, + edit: { user: { hits: 10, seconds: 60 } } + } ); + + expect( limit.hits ).toBe( 10 ); + } ); + + it( 'compares rates rather than counts, so a long window can be the binding one', () => { + // 100/day is far more restrictive than 8/minute despite the larger count. + const limit = bindingLimit( { + upload: { user: { hits: 100, seconds: 86400 } }, + edit: { user: { hits: 8, seconds: 60 } } + } ); + + expect( limit.seconds ).toBe( 86400 ); + } ); + + it( 'ignores actions that an upload does not charge', () => { + const limit = bindingLimit( { + move: { user: { hits: 1, seconds: 86400 } }, + upload: { user: { hits: 90, seconds: 60 } } + } ); + + expect( limit.hits ).toBe( 90 ); + } ); + + it( 'gives the spacing needed to stay inside the limit', () => { + const limit = bindingLimit( { upload: { user: { hits: 8, seconds: 60 } } } ); + + expect( limit.intervalMs ).toBe( 7500 ); + expect( limit.windowMs ).toBe( 60000 ); + } ); + + it( 'ignores a malformed bucket rather than pacing on a NaN', () => { + expect( bindingLimit( { upload: { user: { hits: 0, seconds: 60 } } } ) ).toBeNull(); + } ); +} ); From 8f52454506940fd7dbf0bd7a785e9ef8742a4398 Mon Sep 17 00:00:00 2001 From: alistair3149 Date: Fri, 21 Aug 2026 10:11:15 -0400 Subject: [PATCH 2/3] Stop pacing once the rate limit has demonstrably refilled Pacing was switched on by the first refusal and never switched off, so a single refusal at the tail of one batch slowed every later selection on that page for as long as the tab stayed open -- including batches small enough to fit the budget comfortably. Measured at 8 uploads per 60 seconds, a five file batch that fitted was given 30 seconds of pacing it did not need. That contradicts the rule the pacing was built around: a batch that fits must not be slowed down. Pacing now lapses once a full window has passed with nothing refused, on the grounds that whatever was exhausted has refilled by then. Clearing it on a new selection instead would be wrong, because that is the path where pacing is most likely to be needed. Also moves the reading of the API response into rateLimits.js, where it is covered by a test against a real response body. It was the one part of the feature with no test and it failed silently: a wrong path degraded to "this user is not limited", which is indistinguishable from the real thing. A failed query is now logged rather than swallowed. Corrects an overstatement while here. The derived cap only ever binds below about 64 seconds, because the retry ladder tops out there, so it shortens the wait for a wiki that refills quickly rather than lengthening it for one that refills slowly. Spacing the attempts is what carries a file past a long window. The docblock, the test that claimed to cover it and the release note all said otherwise; the test asserted on a pure function and exercised nothing. Co-Authored-By: Claude Opus 5 (1M context) --- i18n/qqq.json | 2 +- release-notes.md | 2 +- .../ext.SimpleBatchUpload.js | 11 +-- res/ext.SimpleBatchUpload/rateLimitGate.js | 33 +++++++- res/ext.SimpleBatchUpload/rateLimits.js | 21 ++++- .../rateLimitGate.test.js | 80 ++++++++++++++++--- .../ext.SimpleBatchUpload/rateLimits.test.js | 43 +++++++++- 7 files changed, 166 insertions(+), 26 deletions(-) diff --git a/i18n/qqq.json b/i18n/qqq.json index 2b2af8d..dd67552 100644 --- a/i18n/qqq.json +++ b/i18n/qqq.json @@ -17,7 +17,7 @@ "simplebatchupload-result-network-error": "Reason shown when the upload request never reached the server. Used as $1 of {{msg-mw|simplebatchupload-result-error}}.", "simplebatchupload-result-not-uploaded": "Reason shown when the server accepted the request but did not store the file. Used as $1 of {{msg-mw|simplebatchupload-result-error}}. Parameters:\n* $1 - a comma separated list of the server's warning identifiers, which are not translatable", "simplebatchupload-result-queued": "Status of a file that is waiting for its turn to upload. Keep it short, it is shown after the file name.", - "simplebatchupload-result-rate-limit-stopped": "Shown on every remaining row when the batch gave up because the wiki refused uploads repeatedly. Deliberately does not state how long to wait: the limit window is configurable per wiki and can be as long as a day.", + "simplebatchupload-result-rate-limit-stopped": "Shown on every remaining row when the batch gave up because the wiki refused uploads repeatedly. Used as $1 of {{msg-mw|simplebatchupload-result-error}}. Do not name a specific duration: the wait depends on the wiki's configuration.", "simplebatchupload-result-rate-limited": "Status of a file whose upload was refused by the wiki's rate limit and will be retried. Keep it short, it is shown after the file name.", "simplebatchupload-result-success": "Status of a file that was uploaded. Keep it short, it is shown after the file name.", "simplebatchupload-result-token-error": "Reason shown when the edit token could not be obtained. Used as $1 of {{msg-mw|simplebatchupload-result-error}}.", diff --git a/release-notes.md b/release-notes.md index 4ccae66..fd281d6 100644 --- a/release-notes.md +++ b/release-notes.md @@ -7,7 +7,7 @@ * A batch that keeps hitting the limit stops and asks for the remaining files to be selected again * Changed uploading to pace itself to the rate limit the wiki advertises, once the wiki has refused an upload * Batches that fit inside the limit are unaffected and still upload at full speed - * Wikis configuring a longer limit window, such as a daily cap, are now waited out instead of the batch giving up after two minutes + * Wikis configuring a limit window longer than a couple of minutes now keep retrying at the advertised pace for much longer before asking for the remaining files to be selected again * Fixed files reported as uploaded when the wiki did not store them * Fixed an invalid `+rename` pattern cancelling the rest of the batch with no error shown * Fixed the result list losing uploads that were still running when more files were selected diff --git a/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.js b/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.js index 59fbf0f..db0e995 100644 --- a/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.js +++ b/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.js @@ -13,7 +13,7 @@ const { resolveUserLimit, createBatchLimit } = require( './batchLimit.js' ); const { parseRenameDirective } = require( './renamePattern.js' ); const { createRateLimitGate } = require( './rateLimitGate.js' ); -const { bindingLimit } = require( './rateLimits.js' ); +const { limitFromUserInfo } = require( './rateLimits.js' ); const { createUploadQueue } = require( './uploadQueue.js' ); const { createUploadRunner } = require( './uploadRunner.js' ); const { createResultRow, pruneFinishedRows } = require( './resultRow.js' ); @@ -36,13 +36,8 @@ $( () => { // gate does not pace until something is refused anyway. A failed query // simply means no pacing. api.get( { action: 'query', meta: 'userinfo', uiprop: 'ratelimits' } ).then( - ( response ) => { - gate.useLimit( bindingLimit( - response && response.query && response.query.userinfo && - response.query.userinfo.ratelimits - ) ); - }, - () => {} + ( response ) => gate.useLimit( limitFromUserInfo( response ) ), + ( error ) => mw.log.warn( 'SimpleBatchUpload: could not read the rate limits', error ) ); const batchLimit = createBatchLimit( resolveUserLimit( mw.config.get( 'simpleBatchUploadMaxFilesPerBatch' ), diff --git a/res/ext.SimpleBatchUpload/rateLimitGate.js b/res/ext.SimpleBatchUpload/rateLimitGate.js index 6bbf799..35cd288 100644 --- a/res/ext.SimpleBatchUpload/rateLimitGate.js +++ b/res/ext.SimpleBatchUpload/rateLimitGate.js @@ -17,11 +17,16 @@ const FIRST_RETRY_DELAY_MS = 2000; const MAX_RETRY_DELAY_MS = 60000; const MAX_CONSECUTIVE_RETRIES = 6; +// Browsers fire a timer immediately above this, so never sleep for longer. +const MAX_TIMEOUT_MS = 2147483647; + /** * @param {number} rejections Consecutive rate limit rejections, 1 for the first * @param {number} [capMs] Longest useful wait, normally the advertised window. - * Defaults to a minute, which is only right for a wiki using the default - * window; a wiki with a daily cap needs to be able to wait hours. + * Only ever binds below about 64 seconds, where the ladder tops out, so this + * shortens the wait for a wiki that refills quickly rather than lengthening it + * for one that refills slowly. Spacing the attempts is what carries a file + * past a long window. * @return {number} Milliseconds to wait before the next attempt */ function retryDelay( rejections, capMs ) { @@ -74,6 +79,22 @@ function createRateLimitGate( options ) { // bursting is what makes the ordinary case fast, and it succeeds. let pacing = false; let nextReleaseAt = 0; + let lastRefusalAt = 0; + + /** + * Stops pacing once the wiki has gone a full window without refusing + * anything, because by then whatever was exhausted has refilled. + * + * Without this, one refusal at the tail of a batch would slow every later + * selection on the page for as long as the tab stayed open, including + * batches small enough to fit comfortably. + */ + function forgetStaleRefusals() { + if ( pacing && now() - lastRefusalAt >= capMs ) { + pacing = false; + nextReleaseAt = 0; + } + } /** * Adopts the limit the wiki advertises. @@ -103,6 +124,8 @@ function createRateLimitGate( options ) { return false; } + forgetStaleRefusals(); + const releaseAt = pacing ? Math.max( openAt, nextReleaseAt ) : openAt; const remaining = releaseAt - now(); @@ -116,7 +139,9 @@ function createRateLimitGate( options ) { return true; } - await sleep( remaining ); + // Above 2^31-1 ms a browser timer fires immediately, which would turn + // this loop hot. The loop re-checks, so clamping is safe. + await sleep( Math.min( remaining, MAX_TIMEOUT_MS ) ); } } @@ -133,6 +158,8 @@ function createRateLimitGate( options ) { return; } + lastRefusalAt = now(); + if ( intervalMs > 0 ) { // The budget is demonstrably tight, so stop bursting. pacing = true; diff --git a/res/ext.SimpleBatchUpload/rateLimits.js b/res/ext.SimpleBatchUpload/rateLimits.js index 3409539..3d55aa4 100644 --- a/res/ext.SimpleBatchUpload/rateLimits.js +++ b/res/ext.SimpleBatchUpload/rateLimits.js @@ -65,4 +65,23 @@ function bindingLimit( ratelimits ) { }; } -module.exports = { bindingLimit: bindingLimit }; +/** + * The binding limit, read straight out of an API response. + * + * Kept here rather than in the DOM glue so the shape of the response is + * covered by a test: a wrong path would otherwise degrade silently to "this + * user is not limited", which is indistinguishable from the real thing. + * + * @param {?Object} response From action=query&meta=userinfo&uiprop=ratelimits + * @return {?Object} See bindingLimit() + */ +function limitFromUserInfo( response ) { + const userinfo = response && response.query && response.query.userinfo; + + return bindingLimit( userinfo && userinfo.ratelimits ); +} + +module.exports = { + bindingLimit: bindingLimit, + limitFromUserInfo: limitFromUserInfo +}; diff --git a/tests/vitest/ext.SimpleBatchUpload/rateLimitGate.test.js b/tests/vitest/ext.SimpleBatchUpload/rateLimitGate.test.js index 211af8c..d60f9d8 100644 --- a/tests/vitest/ext.SimpleBatchUpload/rateLimitGate.test.js +++ b/tests/vitest/ext.SimpleBatchUpload/rateLimitGate.test.js @@ -157,22 +157,30 @@ describe( 'pacing to the advertised limit', () => { expect( clock.now() - second ).toBe( 7500 ); } ); - it( 'waits up to the advertised window rather than a fixed minute', async () => { - const clock = createFakeClock(); - // A wiki with a daily cap: the useful wait is hours, not a minute. - const gate = pacedGate( clock, { intervalMs: 864000, windowMs: 86400000 } ); - - for ( let refusals = 0; refusals < 20; refusals++ ) { - gate.noteRateLimited(); - gate.noteProgress(); - } - - expect( retryDelay( 20, 86400000 ) ).toBe( 86400000 ); + it( 'shortens the ladder for a wiki whose window is tighter than the ladder', () => { + // The cap only ever binds below ~64s, which is where the ladder tops + // out. A wiki refilling every 10s should not be made to wait a minute. + expect( [ 1, 2, 3, 4, 5, 6 ].map( ( n ) => retryDelay( n, 10000 ) ) ) + .toEqual( [ 2000, 4000, 8000, 10000, 10000, 10000 ] ); } ); it( 'still caps at a minute when the wiki advertises nothing', () => { expect( retryDelay( 20 ) ).toBe( 60000 ); } ); + + it( 'spaces attempts by the advertised rate on a wiki with a long window', async () => { + const clock = createFakeClock(); + // 3 uploads per 150s: pacing, not the cap, is what carries a file past + // a window far longer than the retry ladder. + const gate = pacedGate( clock, { intervalMs: 50000, windowMs: 150000 } ); + + gate.noteRateLimited(); + await gate.wait(); + const first = clock.now(); + await gate.wait(); + + expect( clock.now() - first ).toBe( 50000 ); + } ); } ); describe( 'learning the limit after the widget is already usable', () => { @@ -205,3 +213,53 @@ describe( 'learning the limit after the widget is already usable', () => { expect( clock.now() - first ).toBe( 0 ); } ); } ); + +describe( 'pacing stops once the budget has demonstrably refilled', () => { + const EIGHT_PER_MINUTE = { intervalMs: 7500, windowMs: 60000 }; + + it( 'stops pacing a later batch once a full window has passed without a refusal', async () => { + const clock = createFakeClock(); + const gate = createRateLimitGate( { + now: clock.now, + sleep: clock.sleep, + limit: EIGHT_PER_MINUTE + } ); + + gate.noteRateLimited(); + await gate.wait(); + + // The user goes away for longer than the limit window, so whatever the + // wiki was refusing has long since refilled. + clock.advance( 120000 ); + gate.resume(); + + const before = clock.now(); + await gate.wait(); + await gate.wait(); + await gate.wait(); + + // A batch that fits must not be slowed just because an earlier one was + // refused several minutes ago. + expect( clock.now() - before ).toBe( 0 ); + } ); + + it( 'keeps pacing while refusals are still recent', async () => { + const clock = createFakeClock(); + const gate = createRateLimitGate( { + now: clock.now, + sleep: clock.sleep, + limit: EIGHT_PER_MINUTE + } ); + + gate.noteRateLimited(); + await gate.wait(); + + clock.advance( 5000 ); + gate.resume(); + + const before = clock.now(); + await gate.wait(); + + expect( clock.now() - before ).toBeGreaterThan( 0 ); + } ); +} ); diff --git a/tests/vitest/ext.SimpleBatchUpload/rateLimits.test.js b/tests/vitest/ext.SimpleBatchUpload/rateLimits.test.js index 5577af3..ad8e160 100644 --- a/tests/vitest/ext.SimpleBatchUpload/rateLimits.test.js +++ b/tests/vitest/ext.SimpleBatchUpload/rateLimits.test.js @@ -1,5 +1,6 @@ const { - bindingLimit + bindingLimit, + limitFromUserInfo } = require( '../../../res/ext.SimpleBatchUpload/rateLimits.js' ); describe( 'bindingLimit', () => { @@ -72,3 +73,43 @@ describe( 'bindingLimit', () => { expect( bindingLimit( { upload: { user: { hits: 0, seconds: 60 } } } ) ).toBeNull(); } ); } ); + +describe( 'limitFromUserInfo', () => { + // A real body from action=query&meta=userinfo&uiprop=ratelimits. The + // ratelimits object is identical under formatversion 1 and 2. + const REAL_RESPONSE = { + batchcomplete: '', + query: { + userinfo: { + id: 0, + name: '127.0.0.1', + anon: '', + ratelimits: { + // DevelopmentSettings and some wikis disable a limit by setting + // it to PHP_INT_MAX, which arrives as a very large float. + edit: { ip: { hits: Number.MAX_SAFE_INTEGER, seconds: 60 } }, + upload: { ip: { hits: 8, seconds: 60 } } + } + } + } + }; + + it( 'reads the limit out of a real API response', () => { + const limit = limitFromUserInfo( REAL_RESPONSE ); + + // The edit bucket is effectively unlimited, so upload binds. + expect( limit.hits ).toBe( 8 ); + expect( limit.intervalMs ).toBe( 7500 ); + } ); + + it( 'reports no limit when the query came back empty or malformed', () => { + expect( limitFromUserInfo( undefined ) ).toBeNull(); + expect( limitFromUserInfo( {} ) ).toBeNull(); + expect( limitFromUserInfo( { query: {} } ) ).toBeNull(); + expect( limitFromUserInfo( { query: { userinfo: {} } } ) ).toBeNull(); + } ); + + it( 'reports no limit for a user the wiki does not limit', () => { + expect( limitFromUserInfo( { query: { userinfo: { ratelimits: {} } } } ) ).toBeNull(); + } ); +} ); From 0aa829e6c351d81f749738c180b55ca8ef060cb5 Mon Sep 17 00:00:00 2001 From: alistair3149 Date: Fri, 21 Aug 2026 10:31:18 -0400 Subject: [PATCH 3/3] Tighten the release note for the pacing change The bullet described the mechanism at length where the reader only needs the outcome: batches on wikis with a long limit window survive longer. Co-Authored-By: Claude Opus 5 (1M context) --- release-notes.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/release-notes.md b/release-notes.md index fd281d6..f70fbce 100644 --- a/release-notes.md +++ b/release-notes.md @@ -7,7 +7,7 @@ * A batch that keeps hitting the limit stops and asks for the remaining files to be selected again * Changed uploading to pace itself to the rate limit the wiki advertises, once the wiki has refused an upload * Batches that fit inside the limit are unaffected and still upload at full speed - * Wikis configuring a limit window longer than a couple of minutes now keep retrying at the advertised pace for much longer before asking for the remaining files to be selected again + * Batches on wikis with a long limit window now retry for much longer before giving up * Fixed files reported as uploaded when the wiki did not store them * Fixed an invalid `+rename` pattern cancelling the rest of the batch with no error shown * Fixed the result list losing uploads that were still running when more files were selected