Repository create() functions perform non-atomic two-step Redis writes, risking orphaned or dangling records on crash

[prodbycorne]

Overview

Every repository's create() (and several remove()/update()) implementations perform two or more separate, non-transactional Redis round trips to write a record and its index entry:

// src/repositories/webhookRepository.js
async function create({ url, events, secret, description }) {
  ...
  const redis = cache.getClient();
  await cache.set(key(id), record);   // write #1: the record itself
  await redis.sadd(IDS_KEY, id);      // write #2: the index entry
  return normalize(record);
}

// src/services/airdrops.js
async function create(data) {
  ...
  const redis = cache.getClient();
  await cache.set(airdropKey(id), airdrop); // write #1
  await redis.sadd(IDS_KEY, id);            // write #2
  if (recipients.length > 0) {
    await redis.lpush(recipientsKey(id), ...recipients.map((r) => JSON.stringify(r))); // write #3
  }
  return airdrop;
}

// src/services/apiKeys.js
async function createKey({ label, scopes = ['default'] }) {
  ...
  const redis = cache.getClient();
  await cache.set(keyPath(record.id), record);      // write #1
  await cache.set(hashPath(hashed), record.id);      // write #2
  await redis.sadd(IDS_KEY, record.id);              // write #3
  ...
}

If the process crashes, the connection drops, or any individual command times out between these calls (a realistic scenario — this is exactly the kind of partial-failure window the rest of the codebase is otherwise careful about, e.g. the deliberate try/catch resilience in priceOracle.js), the result is inconsistent state that nothing ever repairs:

• webhookRepository.create: a record exists at webhook:{id} but is never added to webhooks:ids, so it is permanently invisible to list()/listActiveForEvent() (an orphaned, unreachable webhook that still occupies storage forever, since nothing ever cleans up unreferenced keys).
• apiKeys.createKey: the worst case — if cache.set(hashPath(hashed), record.id) succeeds but cache.set(keyPath(record.id), record) failed (or vice versa), you can end up with a hash pointing to a non-existent key record, or a key record whose hash lookup entry doesn't exist, either of which breaks validateApiKey()'s ability to authenticate a key that a client believes was successfully created (since POST /api/v1/keys may have appeared to fail with a 500, leaving the client with no valid key, yet a valid-looking record could exist depending on exactly which write completed).
• airdrops.create: an airdrop record can exist without ever being indexed in airdrops:ids, making it permanently unlistable/inaccessible via GET /api/v1/airdrops despite GET /api/v1/airdrops/:id still being able to fetch it directly by id if the caller somehow retains the id from the crashed request's (potentially failed) response.

Requirements

• Use Redis MULTI/EXEC (ioredis's pipeline/multi API) to make each repository's multi-key writes atomic, so either all of a record's writes land or none do.
• Where atomicity via MULTI isn't sufficient (e.g. because one of the writes depends on reading the result of another within the same transaction), document why and use an alternative safe pattern (e.g. Lua scripting) instead.
• Add a startup or on-demand reconciliation check (does not need to be automatic/scheduled for this issue, but at least an operator-runnable script) that can detect orphaned records (e.g. a webhook:{id} key with no corresponding entry in webhooks:ids, or an api_key_hash:* value pointing to a non-existent api_key:{id}) for manual cleanup, since some inconsistency from before this fix may already exist in long-running deployments.
• Apply consistently to webhookRepository.create/remove, deliveryRepository.create, apiKeys.createKey/revokeKey, airdropsService.create/remove, and alertsService.create/remove — all of which share this exact pattern.

Acceptance Criteria

• Each affected create()/remove() function performs its multi-key writes as a single atomic Redis transaction.
• A test that forces the second write in a sequence to fail (via a mocked Redis client) verifies that the first write is not left behind as an orphan (either it's rolled back, or the whole operation is retried/reported as failed consistently).
• apiKeys.createKey's hash-pointer and key-record writes are verified to be atomic with a dedicated test for the partial-failure scenario described above.
• A short standalone reconciliation script (documented in the README or docs/) can identify orphaned webhook/airdrop/api-key records in an existing Redis instance.
• All existing repository tests continue to pass.

Additional Notes

More precise references

• src/services/apiKeys.js:60-63 (createKey): three sequential writes — cache.set(keyPath(record.id), record), cache.set(hashPath(hashed), record.id), redis.sadd(IDS_KEY, record.id).
• src/services/apiKeys.js:75-78 (revokeKey): cache.del(keyPath(id)), cache.del(hashPath(record.key_hash)), redis.srem(IDS_KEY, id) — same non-atomicity on the delete path, so a crash mid-revoke can leave a hash pointer alive after the key record is gone, letting validateApiKey (apiKeys.js:105-110) resolve a hash to a since-deleted id and then correctly return null via the !record check — that particular case is self-healing, but the reverse order (key record deleted, hash pointer survives) is not, since nothing ever purges stale hash pointers.
• src/services/airdrops.js:45-51 (create) and :96-105 (remove) — same pattern, three ops in remove (cache.del(airdropKey), cache.del(recipientsKey), redis.srem(IDS_KEY, id)).
• src/repositories/webhookRepository.js create/remove — not shown above but referenced in the issue; same shape as airdrops.js.

Additional edge case

cache.set() (in src/services/cache.js) itself wraps a Redis SET with a TTL option in some call sites and none in others — worth confirming during implementation whether cache.set already pipelines internally (it does not, per a quick look — it issues a single SET/SETEX per call), since if a MULTI is built manually with raw redis.multi(), the existing cache.set/cache.get wrapper functions won't compose directly into a transaction without a parallel low-level path that bypasses the wrapper (e.g. multi.set(key, JSON.stringify(value)) instead of calling cache.set).

Implementation sketch

1. Add a low-level helper in cache.js, e.g. cache.multi() returning redis.multi(), plus cache.serialize(value)/cache.deserialize(raw) exported so callers can build multi-command pipelines without duplicating JSON.stringify/parse logic.
2. In each affected function, replace the sequential await calls with:

   const redis = cache.getClient();
   const multi = redis.multi();
   multi.set(keyPath(record.id), JSON.stringify(record));
   multi.set(hashPath(hashed), record.id);
   multi.sadd(IDS_KEY, record.id);
   await multi.exec();

3. For apiKeys.createKey, since none of the three writes depend on reading a prior write's result within the same transaction, MULTI/EXEC is sufficient — Lua scripting is not required here, contrary to a first guess; the "read-then-write" caveat in the issue mainly matters for a hypothetical future case (none currently exists) where, e.g., an index needs to check-and-set atomically.
4. Reconciliation script: a small one-off Node script (scripts/reconcile-redis.js) that: SMEMBERS webhooks:ids and diffs against KEYS webhook:* (or better, SCAN with pattern, since KEYS blocks Redis); flags any webhook:{id} with no matching id in the set, and vice versa; same for api_key:*/api_keys and api_key_hash:* pointing at a missing api_key:{id}; same for airdrop:*/airdrops:ids. Print a report; do not auto-delete (manual cleanup per the issue's own requirement).

Test/reproduction plan

• Unit test: mock the ioredis client so multi().exec() resolves with a mixed success/failure array (ioredis's MULTI semantics: EXEC can partially fail per-command even though the whole thing is still atomic in terms of visibility — worth explicitly testing that a command-level error inside the transaction is surfaced and handled, not swallowed).
• Reproduction of the current bug (pre-fix): mock redis.sadd to reject after cache.set resolves in apiKeys.createKey; assert that today a key:{id} record exists in cache with no corresponding IDS_KEY membership — this demonstrates the orphan before asserting the fix prevents it.
• Post-fix: same mock setup but asserting multi.exec() is called once and that a rejected exec() leaves neither write visible (verify via cache.get(keyPath(...)) returning null after a forced transaction failure in a real Redis instance, e.g. via redis-server in CI with a forced WATCH conflict or a DISCARD).

Cross-references

• Missing pagination limit clamp allows unbounded limit query params on airdrops and alerts endpoints #86 (pagination clamp on airdrops) and Background jobs (price refresh, webhook retry worker) have no leader election — every horizontally-scaled replica runs them independently #98 (leader election) both read from the same IDS_KEY sets this issue writes to — an orphaned or dangling index entry here would surface as a phantom/missing row in those listing paths, so the reconciliation script doubles as a regression check for both.
• Airdrops never auto-expire against expiry_ledger — no reconciliation job checks current chain state #88 (airdrop expiry reconciliation job) will scan airdrops by status; if this issue's fix isn't in place, that job could itself trip over orphaned airdrop:{id} records with no index entry (invisible to any SMEMBERS-based scan) — worth sequencing this fix before or alongside Airdrops never auto-expire against expiry_ledger — no reconciliation job checks current chain state #88.

[OluRemiFour]

I'd like to work on this issue. I'll carefully follow the proposed design, keep the implementation aligned with the existing architecture, include comprehensive tests, and submit a complete PR covering all acceptance criteria.