Skip to content

fix(cp): make pdbFetch throw instead of returning null - #299

Merged
netravnen merged 1 commit into
dev-nextfrom
fix/cp-pdbfetch-throws
Aug 19, 2026
Merged

netravnen merged 1 commit into
dev-nextfrom
fix/cp-pdbfetch-throws

Conversation

@netravnen

Copy link
Copy Markdown
Contributor

pdbFetch resolved to null for every failure mode -- HTTP error, JSON parse
failure, transport error and timeout alike. Because null is also a
plausible "nothing here" value, that silently turned the error handling
written around it into dead code.

fetchRecentNetixlanChanges shows the cost. Its first try block always
reached its return, so both its catch and the entire client-side filtering
fallback beneath it were unreachable. A rejected updated__gte filter
therefore reported "0 rows, no error" -- an admin verifying an IXLAN
renumber would read that as "nothing changed" rather than "the check did
not run". The code was written expecting pdbFetch to throw; the null
return was the deviation, so this restores the surrounding design rather
than imposing a new one.

Changes:

  • Added makePdbFetchError(url, detail), producing a PdbFetchError carrying
    name/status/url/detail so callers can branch on the failure rather than
    guess at it.
  • All seven null exits (three same-origin, four cross-origin) now throw or
    reject. recordFetchFailure diagnostics are unchanged; the same-origin
    catch rethrows an existing PdbFetchError untouched so status and type
    survive.
  • Reviewed all 21 call sites. Eighteen already had an enclosing catch that
    was dead code and is now live; each was read to confirm it does something
    sensible (they return error-carrying result objects).
  • Three callers deliberately want a sentinel, so they catch at their own
    boundary and say why: requestJson (getBootstrap carries an explicit "@ai
    Preserve ... null-on-failure" contract for graceful RDAP degradation),
    and the GraphQL/REST network-name batch fetchers, whose null and []
    returns are fallback signals their paging loop depends on.
  • Guarded the networkixlan toolbar's fire-and-forget void (async ...)(),
    which had no handler at all. A throw there would have become an unhandled
    rejection; the org/ix links are decorative, so a failed lookup now logs
    via dbg and omits them, matching prior behavior.
  • Documented both rules in docs/CONVENTIONS.md.

Security:

  • N/A directly, but it removes a class of silent failure in the flows that
    verify destructive work. The renumber verification report can no longer
    claim zero changes when it never successfully queried.

Testing:

  • node --test from user.js/: 500 tests, 499 pass, 1 skipped (live tests are
    opt-in), up from 493/492.
  • Added tests/cp-pdbfetch-errors.test.js covering the typed rejection and
    its status/url/detail, that success still resolves to a parsed payload,
    and that the revived fallback both runs and reports source
    "client-filter" -- a path that could not previously execute.
  • Confirmed not vacuous: restoring the null return on HTTP error fails 6 of
    the 7 new cases.
  • Re-audited every call site afterwards; none is left without a handler.

Backwards Compatibility:

  • Behavior on success is identical.
  • Behavior on failure changes by design: callers that previously saw null
    now see a rejection. Every site was inspected and either already handled
    it or was updated, so no caller observes a different outcome than before
    -- except fetchRecentNetixlanChanges, which now actually falls back.

Assisted-by: Claude:claude-opus-5


Stack created with GitHub Stacks CLI • Give Feedback 💬

@netravnen
netravnen force-pushed the fix/cp-pdbfetch-throws branch from b462723 to 728af2b Compare August 19, 2026 22:27
@netravnen
netravnen force-pushed the fix/cp-pdbfetch-throws branch from 728af2b to a03774d Compare August 19, 2026 22:28
@netravnen
netravnen force-pushed the fix/cp-pdbfetch-throws branch from a03774d to 3f44b1c Compare August 19, 2026 22:29
Base automatically changed from fix/retry-safe-methods-only to dev-next August 19, 2026 22:31
pdbFetch resolved to null for every failure mode -- HTTP error, JSON parse
failure, transport error and timeout alike. Because null is also a
plausible "nothing here" value, that silently turned the error handling
written around it into dead code.

fetchRecentNetixlanChanges shows the cost. Its first try block always
reached its return, so both its catch and the entire client-side filtering
fallback beneath it were unreachable. A rejected updated__gte filter
therefore reported "0 rows, no error" -- an admin verifying an IXLAN
renumber would read that as "nothing changed" rather than "the check did
not run". The code was written expecting pdbFetch to throw; the null
return was the deviation, so this restores the surrounding design rather
than imposing a new one.

Changes:
- Added makePdbFetchError(url, detail), producing a PdbFetchError carrying
  name/status/url/detail so callers can branch on the failure rather than
  guess at it.
- All seven null exits (three same-origin, four cross-origin) now throw or
  reject. recordFetchFailure diagnostics are unchanged; the same-origin
  catch rethrows an existing PdbFetchError untouched so status and type
  survive.
- Reviewed all 21 call sites. Eighteen already had an enclosing catch that
  was dead code and is now live; each was read to confirm it does something
  sensible (they return error-carrying result objects).
- Three callers deliberately want a sentinel, so they catch at their own
  boundary and say why: requestJson (getBootstrap carries an explicit "@ai
  Preserve ... null-on-failure" contract for graceful RDAP degradation),
  and the GraphQL/REST network-name batch fetchers, whose null and []
  returns are fallback signals their paging loop depends on.
- Guarded the networkixlan toolbar's fire-and-forget `void (async ...)()`,
  which had no handler at all. A throw there would have become an unhandled
  rejection; the org/ix links are decorative, so a failed lookup now logs
  via dbg and omits them, matching prior behavior.
- Documented both rules in docs/CONVENTIONS.md.

Security:
- N/A directly, but it removes a class of silent failure in the flows that
  verify destructive work. The renumber verification report can no longer
  claim zero changes when it never successfully queried.

Testing:
- node --test from user.js/: 500 tests, 499 pass, 1 skipped (live tests are
  opt-in), up from 493/492.
- Added tests/cp-pdbfetch-errors.test.js covering the typed rejection and
  its status/url/detail, that success still resolves to a parsed payload,
  and that the revived fallback both runs and reports source
  "client-filter" -- a path that could not previously execute.
- Confirmed not vacuous: restoring the null return on HTTP error fails 6 of
  the 7 new cases.
- Re-audited every call site afterwards; none is left without a handler.

Backwards Compatibility:
- Behavior on success is identical.
- Behavior on failure changes by design: callers that previously saw null
  now see a rejection. Every site was inspected and either already handled
  it or was updated, so no caller observes a different outcome than before
  -- except fetchRecentNetixlanChanges, which now actually falls back.

Assisted-by: Claude:claude-opus-5
@netravnen
netravnen force-pushed the fix/cp-pdbfetch-throws branch from 3f44b1c to 12fa9de Compare August 19, 2026 22:31
@netravnen
netravnen marked this pull request as ready for review August 19, 2026 23:05
@netravnen
netravnen merged commit b5c0300 into dev-next Aug 19, 2026
3 checks passed
@netravnen
netravnen deleted the fix/cp-pdbfetch-throws branch August 20, 2026 09:39
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