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..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": "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. 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 f4af652..f70fbce 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 + * 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 diff --git a/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.js b/res/ext.SimpleBatchUpload/ext.SimpleBatchUpload.js index 95c2ca1..db0e995 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 { limitFromUserInfo } = require( './rateLimits.js' ); const { createUploadQueue } = require( './uploadQueue.js' ); const { createUploadRunner } = require( './uploadRunner.js' ); const { createResultRow, pruneFinishedRows } = require( './resultRow.js' ); @@ -28,6 +29,16 @@ 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( limitFromUserInfo( response ) ), + ( error ) => mw.log.warn( 'SimpleBatchUpload: could not read the rate limits', error ) + ); 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..35cd288 100644 --- a/res/ext.SimpleBatchUpload/rateLimitGate.js +++ b/res/ext.SimpleBatchUpload/rateLimitGate.js @@ -17,18 +17,26 @@ 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. + * 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 ) { +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 +45,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 +67,53 @@ 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; + 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. + * + * 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,13 +124,24 @@ function createRateLimitGate( options ) { return false; } - const remaining = openAt - now(); + forgetStaleRefusals(); + + 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; } - 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 ) ); } } @@ -93,7 +158,14 @@ function createRateLimitGate( options ) { return; } - openAt = now() + retryDelay( rejections ); + lastRefusalAt = now(); + + if ( intervalMs > 0 ) { + // The budget is demonstrably tight, so stop bursting. + pacing = true; + } + + openAt = now() + retryDelay( rejections, capMs ); } function noteProgress() { @@ -113,6 +185,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..3d55aa4 --- /dev/null +++ b/res/ext.SimpleBatchUpload/rateLimits.js @@ -0,0 +1,87 @@ +'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 + }; +} + +/** + * 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 c5f6c9e..d60f9d8 100644 --- a/tests/vitest/ext.SimpleBatchUpload/rateLimitGate.test.js +++ b/tests/vitest/ext.SimpleBatchUpload/rateLimitGate.test.js @@ -116,3 +116,150 @@ 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( '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', () => { + 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 ); + } ); +} ); + +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 new file mode 100644 index 0000000..ad8e160 --- /dev/null +++ b/tests/vitest/ext.SimpleBatchUpload/rateLimits.test.js @@ -0,0 +1,115 @@ +const { + bindingLimit, + limitFromUserInfo +} = 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(); + } ); +} ); + +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(); + } ); +} );