Skip to content

refactor: replace in-memory cache with pg-backed repository - #667

Open
silent-cipher wants to merge 7 commits into
mainfrom
refactor/no-in-memory-provider-cache
Open

refactor: replace in-memory cache with pg-backed repository#667
silent-cipher wants to merge 7 commits into
mainfrom
refactor/no-in-memory-provider-cache

Conversation

@silent-cipher

@silent-cipher silent-cipher commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Why

Each pod kept its own in-memory copy of the on-chain storage provider registry. Only whichever pod happened to run the scheduled refresh job got fresh data. Every other pod could keep serving stale info (wrong active/inactive/approved status) until it restarted. On top of that, three different code path independently called the chain to fill this cache, and those uncoordinated bursts were the root cause of the erpc rate-limit failures behind the "providers_refresh jobs failing" alert.

What changed

  • Added StorageProviderRepository, the single place that reads and writes provider data in postgres.
  • WalletSdkService now only talks to the chain from the scheduled providers_refresh job; every other lookup reads postgres directly.
  • Consolidated a couple of duplicate ad-hoc provider lookups (sampled-retrieval, retreival) onto the same repository.
  • Updated call sites across jobs, retrieval, deal, and pull-check services to await the now-async provider lookups.

Can we use the subgraph instead of PostgreSQL?

I hadn’t considered this approach initially, but there are a couple of concerns:

  • Goldsky endpoints are rate-limited, and we are already approaching those limits with the sampled_retrievals job.
  • A subgraph outage could halt Dealbot operations entirely. If the subgraph needs to be redeployed, we would also need to wait for it to reindex before Dealbot could resume. Since the indexing duration is outside our control, this would introduce an unpredictable recovery delay.

@FilOzzy FilOzzy added this to FOC Jul 29, 2026
@github-project-automation github-project-automation Bot moved this to 📌 Triage in FOC Jul 29, 2026
@silent-cipher silent-cipher self-assigned this Jul 29, 2026
@silent-cipher
silent-cipher marked this pull request as ready for review July 30, 2026 16:02
Copilot AI review requested due to automatic review settings July 30, 2026 16:02

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

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

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

Suppressed comments (5)

apps/backend/src/wallet-sdk/wallet-sdk.service.ts:112

  • Important: ensureProvidersLoaded can still trigger a multi-pod thundering herd on cold start. If the DB is empty, every pod will see countByNetwork() === 0 and call loadProviders() (chain RPC) concurrently, which can reintroduce the rate-limit bursts this PR is trying to eliminate. Consider adding a Postgres-backed distributed lock (e.g. advisory lock per network) around the "count==0 → load" path and re-checking the count after acquiring the lock.
    apps/backend/src/piece-cleanup/piece-cleanup.service.spec.ts:457
  • This assertion expects a number (9), but deletePiece(..., providerId) is typed as bigint | undefined and should receive 9n from the repository. Keeping the assertion as a number can let a regression slip through if providerId ever gets coerced incorrectly.
      for (const call of deletePieceSpy.mock.calls) {
        expect(call[5]).toBe(9);
      }

apps/backend/src/providers/repositories/storage-provider.repository.ts:116

  • Blocker: findByAddressesCaseInsensitive lowercases the DB column (LOWER(address)), but it passes the input addresses array through unchanged. If any input address is mixed-case, the IN (...) comparison will not match and stale-provider cleanup will silently miss rows.
      where: {
        network,
        address: Raw((alias) => `LOWER(${alias}) IN (:...addresses)`, { addresses }),
      },

apps/backend/src/providers/repositories/storage-provider.repository.ts:30

  • Blocker: findByAddress performs a case-sensitive match on address, which can miss rows when callers provide checksummed/mixed-case addresses (common for EVM-style addresses). This can cause providers to appear “not found” even though they exist in Postgres, leading to skipped jobs or failed checks.
  async findByAddress(address: string, network: Network): Promise<PDPProviderEx | undefined> {
    const row = await this.repo.findOne({ where: { address, network } });
    return row ? this.hydrateProvider(row) : undefined;

apps/backend/src/piece-cleanup/piece-cleanup.service.spec.ts:89

  • The repository mock returns id: 9 (number), but production provider ids are bigint (PDPProviderEx.id). Using a number here can mask type/serialization issues and makes the new providerId-passing optimization less faithful to runtime behavior.

This issue also appears on line 455 of the same file.

  function createStorageProviderRepositoryMock() {
    return {
      findByAddress: vi.fn().mockResolvedValue({ id: 9, name: "Test SP" }),
    };

@silent-cipher
silent-cipher requested a review from beck-8 as a code owner August 3, 2026 06:09

@beck-8 beck-8 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not sure about this, so I'd like someone else to confirm it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 📌 Triage

Development

Successfully merging this pull request may close these issues.

4 participants