Add upstream error classification and optional persistent cache for finance connectors - #17
Merged
Merged
Conversation
Co-authored-by: charles2ke <6725706+charles2ke@users.noreply.github.com>
Copilot created this pull request from a session on behalf of
charles2ke
September 5, 2026 02:54
View session
charles2ke
approved these changes
Sep 5, 2026
There was a problem hiding this comment.
🟡 Changes recommended
The new file-backed cache performs synchronous disk writes in resolver hot paths, which can block the Node event loop and materially impact latency when enabled.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR extends the finance connector layer with (1) stable upstream error classification surfaced through the GraphQL schema and observability metrics/logs, and (2) an optional persistent (file-backed) cache store selected via environment configuration.
Changes:
- Adds upstream error classification (
category,status,retryable) and exposes it viaFinanceUpstreamErrorin the schema, plus per-category error metrics. - Enhances the HTTP client to tag transport failures as
timeoutvsnetwork, enabling classification without message parsing and addingkindto retry-failure metrics. - Introduces pluggable resolver caching (
memorydefault, optionalfile) and adds tests/docs for resilience, classification, and cache behavior.
File summaries
| File | Description |
|---|---|
| test/resilience.test.js | New tests covering error classification, metrics counting, and memory/file cache behavior. |
| test/graphql.test.js | Updates GraphQL selections/assertions to include the new FinanceUpstreamError fields. |
| src/services/financeService.js | Replaces ad-hoc upstream error shaping with classifyUpstreamError, adds error counter, and switches caching to a pluggable store with store-labelled cache metrics. |
| src/schema.js | Extends FinanceUpstreamError with category, status, and retryable. |
| src/observability/errors.js | New module implementing stable upstream error categorization and retryability inference. |
| src/connectors/httpClient.js | Tags transport failures with kind and emits metrics including the kind label. |
| src/config/finance.js | Adds FINANCE_CACHE_STORE and FINANCE_CACHE_FILE configuration plumbing. |
| src/cache/index.js | New cache store implementations (memory + persistent file) and a selector helper. |
| README.md | Documents new env vars, caching strategy, and error classification/metrics. |
| .gitignore | Ignores .cache/ (default location for persistent cache file). |
Review details
- Files reviewed: 9/10 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: charles2ke <6725706+charles2ke@users.noreply.github.com>
Co-authored-by: charles2ke <6725706+charles2ke@users.noreply.github.com>
charles2ke
approved these changes
Sep 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the remaining gaps in the finance connector cluster: upstream error classification and an optional persistent cache strategy. Live authenticated connectors, query pagination, and date filters were already in place and verified.
Changes
src/observability/errors.js): maps every upstream failure to a stablecategory(AUTH,RATE_LIMIT,TIMEOUT,NETWORK,UPSTREAM_CLIENT_ERROR,UPSTREAM_SERVER_ERROR,UNKNOWN) withstatusandretryable.FinanceUpstreamErrornow exposescategory,status, andretryablealongsidesource,code, andmessage.timeoutvsnetworkso they can be classified without message parsing; retry-failure metrics carry thekindlabel.finance_upstream_errors_total{source,operation,category,retryable}counter; cache counters labelled with the active store; classified fields included in error logs.src/cache/index.js):FINANCE_CACHE_STORE=memory|filewithFINANCE_CACHE_FILE. The file store survives restarts and ignores expired or corrupt data. Payloads containing upstream errors are no longer cached, so a transient outage is not pinned for the whole TTL..cache/added to.gitignore.Testing
npm test— 42/42 passing, including newtest/resilience.test.jscovering classification, error metrics, and both cache stores.Note: this repository has no Playwright setup or browser UI to exercise, so no Playwright screenshots are attached.