refactor: replace in-memory cache with pg-backed repository - #667
Conversation
There was a problem hiding this comment.
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:
ensureProvidersLoadedcan still trigger a multi-pod thundering herd on cold start. If the DB is empty, every pod will seecountByNetwork() === 0and callloadProviders()(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), butdeletePiece(..., providerId)is typed asbigint | undefinedand should receive9nfrom the repository. Keeping the assertion as a number can let a regression slip through ifproviderIdever 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:
findByAddressesCaseInsensitivelowercases the DB column (LOWER(address)), but it passes the inputaddressesarray through unchanged. If any input address is mixed-case, theIN (...)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:
findByAddressperforms a case-sensitive match onaddress, 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 arebigint(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" }),
};
beck-8
left a comment
There was a problem hiding this comment.
I'm not sure about this, so I'd like someone else to confirm it.
There was a problem hiding this comment.
only two things that I spotted that should get fixed.
overall, i'm worried that we are moving too much back to the backend/api pod. It doesn't feel like too much right now, but it could get that way. We need to be careful about this.
We want to keep the main (api/backend) pod as ligthweight as possible, and don't want to build a habit stuffing stuff there just because it makes worker contention hard.
I think we should look into making it possible to isolate which jobs dealbot workers can run, similar to how taints and tolerations work with k8s, and then we could have a single worker pod always be responsible for global/centralized/shared data. Maybe it's safe and fine for those jobs to all be on api/backend, but we need to be careful about resource usage and chain of responsibilities.
EDIT: we don't need to handle all of the above now, I just wanted to call it out as something to be aware of moving forward.
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
StorageProviderRepository, the single place that reads and writes provider data in postgres.WalletSdkServicenow only talks to the chain from the scheduledproviders_refreshjob; every other lookup reads postgres directly.sampled-retrieval,retreival) onto the same repository.Can we use the subgraph instead of PostgreSQL?
I hadn’t considered this approach initially, but there are a couple of concerns:
sampled_retrievalsjob.