fix(core): Invalidate stock location channel id cache correctly - #5087
Conversation
The MultiChannelStockLocationStrategy caches each StockLocation's channel ids for 7 days, but the cache was never actually invalidated: - The StockLocationEvent subscription passed an already-prefixed key to Cache.delete(), which applies the configured getKey() again, so the delete targeted a double-prefixed key that never exists. - Assigning a StockLocation to a Channel (or removing it) emits a ChangeChannelEvent, not a StockLocationEvent, so the operation that actually changes channel membership never triggered invalidation at all. A newly-added channel could see all products as out of stock until the entry expired or the server restarted. Fixes vendurehq#3324
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe stock-location strategy now invalidates cached channel IDs for stock-location updates and channel-assignment changes. Unit tests cover cache reuse and event filtering. End-to-end tests cover cross-channel update protection and stock visibility after assigning a stock location to a channel. Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/e2e/stock-location.e2e-spec.ts`:
- Around line 500-501: Replace the fixed 100 ms delay in the stock-location test
with bounded polling of the shop query, repeatedly checking until the result is
IN_STOCK. Add a finite timeout or retry limit, and fail clearly if the expected
state is not observed within that bound.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 98249f36-a5a9-4f8a-85ff-6f50187a88e2
📒 Files selected for processing (3)
packages/core/e2e/stock-location.e2e-spec.tspackages/core/src/config/catalog/multi-channel-stock-location-strategy.spec.tspackages/core/src/config/catalog/multi-channel-stock-location-strategy.ts
| // Wait a bit for the ChangeChannelEvent subscriber to invalidate the cache | ||
| await new Promise(resolve => setTimeout(resolve, 100)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Replace the fixed delay with bounded polling.
Line 501 assumes that cache invalidation completes within 100 ms. Under CI load, the event handler can complete later and cause an intermittent failure. Poll the shop query until it returns IN_STOCK, with a bounded timeout.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/e2e/stock-location.e2e-spec.ts` around lines 500 - 501, Replace
the fixed 100 ms delay in the stock-location test with bounded polling of the
shop query, repeatedly checking until the result is IN_STOCK. Add a finite
timeout or retry limit, and fail clearly if the expected state is not observed
within that bound.
michaelbromley
added
the
T2: Elevated risk
2 participants
Description
Fixes #3324.
MultiChannelStockLocationStrategycaches each StockLocation's channel ids for 7 days, but the cache was never actually invalidated, for two independent reasons:1. The delete used a double-prefixed key (the defect reported in the issue). The
StockLocationEventsubscription passedthis.getCacheKey(entity.id)intoCache.delete(), butCache.delete()applies the configuredgetKeyfunction itself — the same wayCache.get()does — so the delete targetedMultiChannelStockLocationStrategy:StockLocationChannelIds:MultiChannelStockLocationStrategy:StockLocationChannelIds:<id>, which never exists, and the real entry lived out its full TTL. The fix is the one-liner proposed in the issue: pass the raw id. (I checked the rest of core for the same pattern — the otherCacheconsumers, e.g.customer-group-condition.tsandfacet-value-checker.ts, already pass raw ids; this was the only occurrence.)2. The operation that changes channel membership never emitted an invalidating event at all — surfaced by the follow-up investigation in this comment.
assignStockLocationsToChannel()/removeStockLocationsFromChannel()delegate toChannelService.assignToChannels()/removeFromChannels(), which emitChangeChannelEvent, notStockLocationEvent. So even with the key fix, assigning a StockLocation to a new Channel leaves that channel reading the stale pre-assignment channel list: in a multi-channel setup, every newly added channel can show all products asOUT_OF_STOCKfor up to 7 days (or until a server restart clears the in-memory cache).The fix adds a second subscription in the strategy's
init(), onChangeChannelEventfiltered toentityType === StockLocation. Subscribing to the event — rather than patching the two service methods — keeps the invalidation inside the strategy that owns the cache, and covers every path that changes a StockLocation's channels, including plugins callingChannelServicedirectly.Both changes live in the same
init()block and are only meaningful together (the first makes deletes actually work, the second makes the right operation trigger them), so they ship as one PR.Breaking changes
None. The only behavioural change is that saleable-stock reads now see channel-membership changes immediately, instead of after cache expiry or a server restart.
Testing
Unit (
multi-channel-stock-location-strategy.spec.ts, new file, 6 cases, no DB): uses the realCachewrapper over an in-memory Map — so the key-prefixing interaction under test behaves exactly as in production — plus a Subject-backed EventBus stub. On master, the three invalidation cases fail (update / assign / remove: the first because the delete misses its key, the other two because no subscription exists); with the fix all 6 pass. The remaining three pin existing behaviour: channel ids are cached after the first lookup,createdevents don't invalidate, andChangeChannelEvents for other entity types don't invalidate.E2E (
stock-location.e2e-spec.ts, new describe): reproduces the real-world sequence from the issue thread — query a variant's saleable stock via the Shop API in a new channel's context (which caches the location's pre-assignment channel list), thenassignStockLocationsToChannel, then query again. On master's dist this fails withOUT_OF_STOCKwhereIN_STOCKis expected; with the fix it passes. The 16 existing tests in the file are unaffected in both runs.What I ran:
stock-location.e2e-spec.tsagainst master diststock-location.e2e-spec.tsagainst rebuilt diststock-control.e2e-spec.ts+stock-control-multi-location.e2e-spec.tsThe pre-existing type error in
order.service.ts:1460that stopsnpm run buildlocally is unrelated (file untouched, reproduces on clean master).🤖 AI assistance
I used Claude Code while working on this. I have reviewed every line of the change myself, and the testing table above reflects test runs I actually performed and observed — including verifying that each new regression test fails without its fix and passes with it.
Checklist
📌 Always:
👍 Most of the time:
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.