Skip to content

Fix : RequestManager ignoring explicit 0 config values#5795

Closed
Chaitu7032 wants to merge 1 commit intosugarlabs:masterfrom
Chaitu7032:RequestMan
Closed

Fix : RequestManager ignoring explicit 0 config values#5795
Chaitu7032 wants to merge 1 commit intosugarlabs:masterfrom
Chaitu7032:RequestMan

Conversation

@Chaitu7032
Copy link
Contributor

Summary

This PR fixes a silent misconfiguration bug in RequestManager where explicit numeric 0 configuration values were ignored and replaced with defaults.

It also adds a focused unit test to prevent regressions and to prove that explicit 0 values are preserved for all relevant options.

What happened (bug / impact)

In the RequestManager constructor, defaults were applied using: options.foo || default

Because 0 is falsy in JavaScript, passing any of the following valid and intentional configurations:

  • { minDelay: 0 } → disable throttling delay
  • { maxRetries: 0 } → disable retries
  • { baseRetryDelay: 0 } → disable backoff delay (common in tests)
  • { maxConcurrent: 0 } → edge case: caller expects “no concurrent execution” semantics

Impact

  • Callers attempting to disable throttling/retries (often in tests or special flows) still experienced delays and retries
  • This caused unexpected latency, extra load, and confusing debugging (“why is it still retrying?”)
  • The issue occurred silently in a reliability-critical component (rate limiting / retry logic)

Fix given (what changed / why it’s correct)

The constructor now uses nullish coalescing (??) instead of logical OR (||) for defaulting:

this.minDelay = options.minDelay ?? 500; this.maxRetries = options.maxRetries ?? 3; this.baseRetryDelay = options.baseRetryDelay ?? 1000; this.maxConcurrent = options.maxConcurrent ?? 3;

Why this is correct

  • ?? only falls back when the value is null or undefined
  • Explicit 0 is preserved (as intended)
  • “Not provided” still correctly receives defaults

Changed files

  • planet/js/RequestManager.js
  • planet/js/__tests__/RequestManager.test.js

How I tested (what I ran)

Targeted Jest run (clean, no coverage collection):

npm test -- --runTestsByPath "planet/js/__tests__/RequestManager.test.js" --coverage=false

New test added

A unit test that constructs:
new RequestManager({ minDelay: 0, maxRetries: 0, baseRetryDelay: 0, maxConcurrent: 0 });

and asserts that all instance fields remain 0.

Before fix - failing test

The new test fails when the constructor uses || , proving the bug existed.

How it was produced (local only, not committed):

Screenshot 2026-02-18 231912

Before fix: explicit 0 values ignored (test fails)

After fix - passing test (proof of resolution)

With ?? in place, the same test now passes, confirming the fix.

Screenshot 2026-02-18 231958

Scope / non-goals

Scope

Constructor defaulting logic for four numeric options only

Non-goals

  • No changes to retry/backoff algorithms
  • No changes to queue processing or request deduplication
  • No new configuration options or features
  • No logging or behavior refactors

Edge note

{ maxConcurrent: 0 } now remains 0 (previously defaulted to 3)

If any code relied on the old buggy behavior, it will now behave as configured, which matches the intended contract .

testing

image image

@github-actions
Copy link
Contributor

✅ All Jest tests passed! This PR is ready to merge.

@Chaitu7032
Copy link
Contributor Author

Chaitu7032 commented Feb 18, 2026

@walterbender sir , @omsuneri This fixes a bug where RequestManager treated explicit 0 config values as unset due to ||, and adds a test to ensure 0 is preserved using ??. thanking you ...

@Chaitu7032
Copy link
Contributor Author

Chaitu7032 commented Feb 25, 2026

@walterbender sir , if you have some time could you please have a look at this .

@Chaitu7032 Chaitu7032 closed this by deleting the head repository Feb 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant