Skip to content

fix(configManager): rethrow errors instead of masking them as a TypeError - #1052

Open
DeepDiver1975 wants to merge 1 commit into
github-community-projects:main-enterprisefrom
DeepDiver1975:deepdiver/configmanager-rethrow
Open

fix(configManager): rethrow errors instead of masking them as a TypeError#1052
DeepDiver1975 wants to merge 1 commit into
github-community-projects:main-enterprisefrom
DeepDiver1975:deepdiver/configmanager-rethrow

Conversation

@DeepDiver1975

Copy link
Copy Markdown

Problem

ConfigManager.loadYaml handles a failed repos.getContent in a .catch() that logs the error but neither rethrows nor returns a value:

https://github.com/github-community-projects/safe-settings/blob/main-enterprise/lib/configManager.js#L22-L30

So the awaited expression resolves to undefined, and the very next statement dereferences response.data. Every failure to read the config file therefore surfaces as:

ERROR (probot): Error getting settings HttpError: Not Found
Unexpected error during full sync: TypeError: Cannot read properties of undefined (reading 'data')

Two consequences:

  1. The real error is lost. The TypeError has no .status, so the if (e.status === 404) return null in the enclosing catch never matches and rethrows it instead. A missing settings file aborts the run rather than returning null as the code plainly intends.
  2. Every cause looks identical. 404 (no admin repo / no settings file / wrong CONFIG_PATH / wrong account), 403 (permissions, rate limit) and 5xx all produce the same reading 'data' TypeError, pointing at a line that is not where anything went wrong.

That error string shows up in issue reports where it obscured the actual cause — it is the error output in #782, and the same masking pattern is visible in #586. We lost about two days to it: the underlying failure was a plain 404 on the config repo, and nothing in the logs said so.

Change

Add throw e to the .catch(). lib/settings.js already does exactly this in its equivalent .catch():

https://github.com/github-community-projects/safe-settings/blob/main-enterprise/lib/settings.js#L865-L874

so this brings configManager in line with the working version in the same codebase. With the fix, a 404 returns null through the existing catch, and any other error propagates unchanged with its status intact.

Behaviour change worth calling out: a non-404 config read error now fails the sync with the real HTTP error instead of a TypeError. Both abort, so nothing that used to succeed starts failing — only the reported error changes, and a 404 now correctly returns null instead of aborting.

Tests

lib/configManager.js had no test file. Added test/unit/lib/configManager.test.js (7 tests) covering the success path, folder and symlink responses, loadGlobalSettingsYaml path composition, and the two error paths: 404 resolves to null, and a non-404 rejects with the original error object so its status survives.

3 of the 7 fail on main-enterprise without the change, with Received message: "Cannot read properties of undefined (reading 'data')" — i.e. the suite reproduces the reported failure and then pins the fix.

npx jest test/unit/lib/configManager.test.js  -> 7 passed
npx jest --roots=lib --roots=test/unit        -> 144 passed, 14 skipped (137 passed before, no regressions)
npx standard lib/configManager.js test/unit/lib/configManager.test.js  -> clean
npx eslint   lib/configManager.js test/unit/lib/configManager.test.js  -> clean

Node 22.12.0.

…rror

`loadYaml` handled failures from `repos.getContent` in a `.catch()` that
logged the error but neither rethrew nor returned a value. The awaited
expression therefore resolved to `undefined`, and the next statement
dereferenced `response.data`, so every config-read failure surfaced as:

    TypeError: Cannot read properties of undefined (reading 'data')

That TypeError carries no `.status`, so the `if (e.status === 404) return
null` in the enclosing catch never matched and the real HTTP error was
lost. A missing settings file aborted the run instead of returning null,
and a 403/500 was indistinguishable from a 404.

`lib/settings.js` already rethrows from the equivalent `.catch()`; this
brings `configManager` in line with it.

Adds test/unit/lib/configManager.test.js, which had no coverage: a 404
now returns null, and a non-404 rejects with the original error object so
its status survives. Three of the seven tests fail without this change.

Signed-off-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>
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