Skip to content

Fix concurrent namespace load error attribution - #9677

Merged
jason-crow merged 8 commits into
masterfrom
jason-crow-fix-register-namespace-errors
Sep 10, 2026
Merged

Fix concurrent namespace load error attribution#9677
jason-crow merged 8 commits into
masterfrom
jason-crow-fix-register-namespace-errors

Conversation

@jason-crow

@jason-crow jason-crow commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Summary

  • determine namespace load success from i18next's resource store for active languages
  • avoid attributing queue-wide errors to a requested namespace that loaded successfully
  • add deterministic coverage for concurrent successful and failed loads when the failure completes first

Reviewer Q&A

Is removing the try/catch completely safe?

It is safe under ITwinLocalization's existing initialized-use contract. The removed try/catch only handled err being non-iterable. The replacement does not read or iterate err; it uses i18next's stable hasResourceBundle() API. Also, getLanguageList().map(...) was already outside the old try, so uninitialized usage was never protected by it.

An unexpected exception inside i18next would indicate corrupted or uninitialized state rather than a normal namespace-load error. Removing this catch therefore does not hide or newly expose an expected error path.

What was thisLocale for, and why is it safe to remove its role?

thisLocale represented each active or fallback language, such as fr-CA, fr, or en. The old implementation searched error strings for /<locale>/ and inferred whether all locale attempts failed. That inference is unsafe during concurrent loads because the callback can contain an error for another namespace in the same locale.

The replacement does not remove locales from the decision. It checks the requested namespace together with every active or fallback language:

const wasLoaded = this.getLanguageList().some((language) =>
  this.i18next.hasResourceBundle(language, name),
);

i18next adds successful resources to its store before invoking the load callback, so this directly answers the intended question: did this namespace load for any applicable locale? It is more precise than parsing error-message text because both the locale and requested namespace participate in the check.

Validation

  • git diff --check — passed
  • rushx build — blocked because the incomplete Rush dependency install left TypeScript files unavailable
  • rushx lint — blocked because the incomplete Rush dependency install left ESLint unavailable
  • rushx test — blocked because the incomplete Rush dependency install left a test-runner dependency unavailable
  • rush install — retried, but the local pnpm store failed with ERR_PNPM_ENOENT while materializing workspace packages

Fixes #9676

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request fixes a race in ITwinLocalization.registerNamespace where i18next can report queue-wide errors from other concurrent namespace loads, causing a successfully loaded namespace to be incorrectly logged as unavailable. The fix determines load success by checking i18next’s resource store for the requested namespace across the active language resolution list, and adds a deterministic unit test covering the “failure completes first” concurrency case.

Changes:

  • Replace error-attribution logic in registerNamespace with a resource-store check via i18next.hasResourceBundle(...) for active/fallback languages.
  • Add a unit test that concurrently registers a successful and failing namespace and asserts only the failing namespace is logged.
  • Add a Rush change file documenting the behavior fix for @itwin/core-i18n.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
core/i18n/src/ITwinLocalization.ts Updates namespace-load failure attribution to rely on resource-store presence instead of parsing callback error contents.
core/i18n/src/test/ITwinLocalization.test.ts Adds deterministic coverage for concurrent namespace registration where the failing load completes first.
common/changes/@itwin/core-i18n/jason-crow-fix-concurrent-namespace-errors_2026-08-31-17-20.json Records the fix as a no-version-change Rush change entry for @itwin/core-i18n.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread core/i18n/src/ITwinLocalization.ts

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new regression test deadlocks by awaiting the failed load before completing the successful pending load.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread core/i18n/src/test/ITwinLocalization.test.ts Outdated
@MichaelSwigerAtBentley

Copy link
Copy Markdown
Contributor

I fixed up the lint errors that were blocking the pipeline. Other than the issues commented on, LGTM

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@jason-crow
jason-crow merged commit b59c8bd into master Sep 10, 2026
22 checks passed
@jason-crow
jason-crow deleted the jason-crow-fix-register-namespace-errors branch September 10, 2026 19:17
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.

ITwinLocalization.registerNamespace misattributes concurrent namespace load failures

4 participants