fix: dump progress output and stricter rate limit overrides

src/api/rateLimit.test.ts

@@ -107,18 +107,27 @@ describe('RateLimiter', () => {
       ).toEqual({ burst: 10, refillTokens: 5, refillIntervalMillis: 30000 });
     });
 
-    it.each(['0', '-1', 'abc', 'NaN'])('should fall back and complain about %s', value => {
-      const complaints: string[] = [];
-      const original = console.error;
-      console.error = (m: string) => complaints.push(m);
-      try {
-        expect(bucketFromEnvironment({ STEADYBIT_RATE_LIMIT_BURST: value }).burst).toEqual(defaultBucket.burst);
-      } finally {
-        console.error = original;
-      }
-      expect(complaints).toHaveLength(1);
-      expect(complaints[0]).toContain('STEADYBIT_RATE_LIMIT_BURST');
+    it('should take a value with surrounding whitespace', () => {
+      expect(bucketFromEnvironment({ STEADYBIT_RATE_LIMIT_BURST: ' 10 ' }).burst).toEqual(10);
     });
+
+    // `Number` would have read these as 1000, 16 and 2.5. None is how a request count
+    // gets written on purpose, and accepting them changes the pacing silently.
+    it.each(['0', '-1', 'abc', 'NaN', '1e3', '0x10', '2.5', '10,5', '+5'])(
+      'should fall back and complain about %s',
+      value => {
+        const complaints: string[] = [];
+        const original = console.error;
+        console.error = (m: string) => complaints.push(m);
+        try {
+          expect(bucketFromEnvironment({ STEADYBIT_RATE_LIMIT_BURST: value }).burst).toEqual(defaultBucket.burst);
+        } finally {
+          console.error = original;
+        }
+        expect(complaints).toHaveLength(1);
+        expect(complaints[0]).toContain('STEADYBIT_RATE_LIMIT_BURST');
+      }
+    );
   });
 
   it('should default to the allowance the platform documents', () => {

src/api/rateLimit.ts

@@ -37,33 +37,38 @@ const systemClock: Clock = {
 // 429 itself, by which point remaining is zero.
 export function bucketFromEnvironment(env: NodeJS.ProcessEnv = process.env): BucketOptions {
   return {
-    burst: positiveNumber(env.STEADYBIT_RATE_LIMIT_BURST, 'STEADYBIT_RATE_LIMIT_BURST', defaultBucket.burst),
-    refillTokens: positiveNumber(
+    burst: positiveInteger(env.STEADYBIT_RATE_LIMIT_BURST, 'STEADYBIT_RATE_LIMIT_BURST', defaultBucket.burst),
+    refillTokens: positiveInteger(
       env.STEADYBIT_RATE_LIMIT_REFILL,
       'STEADYBIT_RATE_LIMIT_REFILL',
       defaultBucket.refillTokens
     ),
     refillIntervalMillis:
-      positiveNumber(
+      positiveInteger(
         env.STEADYBIT_RATE_LIMIT_INTERVAL,
         'STEADYBIT_RATE_LIMIT_INTERVAL',
         defaultBucket.refillIntervalMillis / 1000
       ) * 1000,
   };
 }
 
-function positiveNumber(value: string | undefined, name: string, fallback: number): number {
+// Plain decimal digits only. `Number` would also have taken '1e3', '0x10' and '2.5',
+// which are not how anyone means to write a request count, and reading '0x10' as 16
+// would quietly change how hard the CLI hits the platform.
+const POSITIVE_INTEGER = /^\d+$/;
+
+function positiveInteger(value: string | undefined, name: string, fallback: number): number {
   if (value === undefined || value.trim() === '') {
     return fallback;
   }
-  const parsed = Number(value);
-  if (!Number.isFinite(parsed) || parsed <= 0) {
+  const trimmed = value.trim();
+  if (!POSITIVE_INTEGER.test(trimmed) || Number(trimmed) <= 0) {
     // Warned about rather than ignored: a typo here silently changes how hard the CLI
     // hits the platform, which is the last thing that should fail quietly.
-    console.error(`Ignoring ${name}: '${value}' is not a positive number. Using ${fallback}.`);
+    console.error(`Ignoring ${name}: '${value}' is not a positive whole number. Using ${fallback}.`);
     return fallback;
   }
-  return parsed;
+  return Number(trimmed);
 }
 
 export class RateLimiter {

src/experiment/dump.test.ts

@@ -53,6 +53,24 @@ describe('experiment dump', () => {
     process.exitCode = undefined;
   });
 
+  // The list-gathering phase is paced like everything else, so without this the CLI can
+  // sit silent for minutes before its first line of output.
+  it('should report progress while gathering the experiment lists', async () => {
+    const written: string[] = [];
+    const stdout = vi.spyOn(process.stdout, 'write').mockImplementation(chunk => {
+      written.push(String(chunk));
+      return true;
+    });
+
+    await dump({ directory: freshDirectory() });
+
+    stdout.mockRestore();
+    const output = written.join('');
+    expect(output).toContain('Listing experiments for 1 team');
+    // The dot lands before the per-team walk starts, not after it.
+    expect(output.indexOf('.\n')).toBeLessThan(output.indexOf('Fetching experiments for team'));
+  });
+
   it('should write the experiment and its executions', async () => {
     givenExecutions('TST-1', [1, 2, 3]);
     const directory = freshDirectory();

src/experiment/dump.ts

@@ -36,11 +36,16 @@ export async function dump(options: Options) {
 
   // The experiment lists are fetched up front, which costs nothing extra because each
   // team needs one anyway, so that the size of the walk is known before it starts.
+  // These requests are paced like any other, so on a tenant with many teams, or a
+  // reduced allowance, they take long enough that silence here reads as a hang.
   const teams = selectTeams(await getAllTeams(false), options.team);
+  process.stdout.write(`Listing experiments for ${teams.length} ${teams.length === 1 ? 'team' : 'teams'}`);
   const listPerTeam = new Map<string, ExperimentList>();
   for (const team of teams) {
     listPerTeam.set(team.key, await fetchExperiments(team.key));
+    process.stdout.write('.');
   }
+  process.stdout.write('\n');
   warnAboutLargeDump([...listPerTeam.values()].reduce((total, list) => total + list.experiments.length, 0));
 
   for (const team of teams) {