CB-1-10 — مراجعة كود · C++ · عالٍ

Summary Your staging crash has a concrete cause in this file: the stale-key loop erases a vector element and then increments the dead iterator, and your own main() reaches that path on the second run(). The frozen-price incident cannot come from this file alone. The leak plus the zero-booking gap explains it once the API's miss behaviour is known. Fix ownership and atomic publication before adding competitor data, because competitor fetching widens every race listed here. This is a hand reading of the code you pasted; nothing was compiled or run. Line numbers count from the first #include of the snippet as pasted. Severity Lines Failure scenario Fix direction Blocker 87–89 erase(it) invalidates it, and then ++it runs on it. That is undefined behaviour. In main(), markStale("H1","STD") is followed by run(). The lookup uses lastRunDate\_ "2026-10-09", which no booking carries, so the only element is erased and the loop steps past end(). Whether it crashes depends on heap layout, which fits a crash you could not reproduce. Write it = staleKeys\_.erase(it) with no increment on that path, or use erase–remove\_if. Blocker 25–29, 53, 83 The API thread calls get() while run() calls clear() and put() on the same unordered\_map, with no lock. That is a data race and undefined behaviour; a rehash inside put() can leave find() walking freed buckets. Even when nothing crashes, every lookup between clear() and the last put() misses. Build the new map locally. Publish it with std::atomic\_store on a std::shared\_ptr<const Map>, and have readers atomic\_load once per request. High 24, 29, 53–54, 81 Records are created with new and never deleted, and clear() on both containers drops the pointers. Memory grows every night for the life of the process. Ownership is also undefined: the day someone adds delete to fix the leak, any pointer the API holds becomes use-after-free. Make the snapshot own shared\_ptr<const PriceRecord>, and have get() return that or a copy, never a raw pointer. High 46, 77 When a (property, type) has no capacity row, total is 0 and line 77 divides by zero. Integer division by zero is undefined behaviour and typically kills the process. This is your second crash candidate: a new room type appearing in bookings before its capacity row exists. Skip and report rows with missing or zero capacity. High 57–60, 69, 86–91 Prices are produced only from booking rows. A property, type or date with zero bookings gets no record at all. The "GC pass" is dead logic: it never touches cache\_, compares against a hard-coded lastRunDate\_ that never updates, and deletes from the stale list itself. Iterate the inventory (capacity × date horizon) and treat zero bookings as zero occupancy. Delete the GC pass or give it an explicit eviction rule. Medium 76–78 Two truncating integer divisions turn the multiplier into a staircase: below half occupancy prices at base, and 95 of 120 rooms earns the same multiplier as a half-full hotel. Overbooking produces a higher step, and free goes negative and is never used. Compute occupancy as a double, clamp it, decide what overbooking means, and remove free. Medium 65–67, 79, 81 base\[{property,type}] uses operator\[], which inserts 0.0 for an unknown key. A type with capacity but no base row publishes a zero price silently. Use find() and treat a missing base as a data error. Low 83, 100–101 The key built from "|" concatenation collides if an id contains |. PriceRecord lacks propertyId, so report() cannot tell H1 STD from H2 STD on the same date. Use a struct key with a hash, and add propertyId to the record. Low 1–6, 70 <tuple>/<utility> are used but not included and compile only transitively. auto key3 = entry.first copies three strings per row. Include what you use and bind const auto\&. Your hint about zero bookings is right, but only half the story. Line 53 empties the cache, so a property missing from the night's feed produces a nullptr, not last week's price. A frozen value therefore needs a consumer that keeps its last non-null pointer or falls back to a previous record on a miss. The leak is what makes that fallback look healthy. Old records are never freed, so the retained pointer stays readable indefinitely and serves the old value without any error. One quiet feed night for one property gives you exactly "stuck for one property for a day." Suggestions Order the tickets so that each fix does not open a new hazard. Fixing the leak before fixing publication converts a slow memory growth into a use-after-free. Ticket order 1 Fix the iterator loop and the zero-capacity guard Both are local changes and both remove a known crash path. 2 Introduce an immutable snapshot with atomic publish Readers see either the old complete map or the new complete map, never a partial one. 3 Move ownership into the snapshot Only now replace raw pointers with shared\_ptr to const; this removes the leak without creating a dangling reader. 4 Price from inventory, not from bookings Zero-booking dates and properties get explicit base prices. 5 Replace the integer multiplier Define the curve as floating-point occupancy, with overbooking handled deliberately. 6 Define staleness explicitly Stamp each snapshot with a generation and an as-of time, and remove the dead GC pass. Competitor pricing turns several of these from latent defects into certain failures. Longer exposure window. Competitor data means network I/O inside run(), so the gap between clear() and the last put() stretches from a tight loop to the length of a remote fetch. Today's race window becomes a long one. Undefined partial failure. The contract has no answer for a competitor feed that fails halfway. It could publish own-demand prices, keep yesterday's snapshot, or publish a mix. Without a generation number or as-of timestamp on the snapshot, the API cannot tell which one it is serving. Mutable shared records. get() hands out a mutable pointer. If any caller adjusts a price in place, for example to apply a competitor offset, it is writing to state shared across all readers without synchronisation. No source dimension. The cache key and PriceRecord have no field saying where a price came from. Once two inputs shape a price, an incident like your frozen one becomes impossible to diagnose after the fact. The contract should be: the API receives an immutable snapshot with a generation and an as-of time, plus a defined "feed missing" outcome rather than a silent fallback. When get() returns nullptr today, what does the pricing API do: fail the request, serve a default, or reuse a previously fetched record? That answer confirms or rules out the frozen-price mechanism above. What I do not know: Deterministic inspection of your code — code review, security findings, performance, concurrency patterns, algorithmic complexity, allocation, canon compliance, structure

This is a review of the snippet as you pasted it. Line numbers count from the first #include as line 1, so (a) is L76, (b) L77, (c) L78 and (d) L89. I've called the file nightly\_price\_job.cpp; adjust to your real file names. How this maps to your two incidents Prices stuck for one property: most likely Finding 5, made worse by Findings 4 and 7. A property or date with no booking rows never gets a record. The leaked records from earlier nights stay valid in memory, so whatever the API was already holding keeps serving last week's price. Confirm by checking what the pricing API does when get() returns nullptr. Unreproduced staging crash: three candidates, any of which fits. Integer divide-by-zero (Finding 2): data-dependent, so staging feed differences would explain it. Iterator invalidation in the GC pass (Finding 3): the demo main() itself triggers it. Data race with the API thread (Finding 1): timing-dependent, so hard to reproduce. Blockers 1\. Cache is rebuilt in place while the API thread reads it (data race, UB) Where: L53, L83, L24–27, L31 Scenario: run() calls clear() and then operator\[] inserts on the unordered\_map. Inserts can rehash. Meanwhile the API thread is calling find() on the same map with no synchronization. That is a data race, which is undefined behavior. In practice it shows up as crashes inside find, or as torn or missing lookups. Even with no race, every request between L53 and the last L83 sees an empty or partial cache. If run() throws midway, the cache stays partial until the next night. Fix direction: Build the new cache completely off to the side, then publish it in one step. For example, use std::shared\_ptr<const Snapshot> swapped with std::atomic\_load/std::atomic\_store, or a mutex around the pointer swap. Readers grab the snapshot once per request, and the old snapshot is freed only when the last reader releases it. 2\. Integer divide-by-zero when capacity is unknown, and missing base price defaults to 0 Where: L77 (and L74 / L46), L79, L81 Scenario: totalRoomsFor returns 0 for any (property, type) not in capacity. That happens with a new property, a new room type, or a casing or ID mismatch in the feed. occupied \* 100 / 0 is integer division by zero, which is UB and usually a SIGFPE crash on x86. Separately, base\[{property, type}] uses operator\[], which silently inserts 0.0 for a missing key. So a missing base price publishes a final price of 0 with no error. Fix direction: Use find()/at() for both lookups. Treat a missing capacity or base as a data error: skip that key, log it, and alert. Never default to 0. Validate that total > 0 before any division. 3\. erase inside a range loop, then ++it, which is UB Where: L87–89 Scenario: staleKeys\_.erase(it) invalidates it, and the loop then increments it. If the erased element was the last one, it moves past end() and the loop never terminates correctly. The demo hits this exactly. markStale("H1","STD") followed by run() makes booked.count({"H1","STD","2026-10-09"}) equal 0, because no feed row has that date. The only element is erased and the next ++it is UB. Even when it doesn't crash, every element after an erased one is skipped. Fix direction: Use it = staleKeys\_.erase(it); and only ++it in the else branch, or use the erase–remove\_if idiom. Also see Finding 7, because the logic itself is wrong. High 4\. Unclear ownership of PriceRecord: every night leaks every record Where: L24, L29, L54, L81–83, L106 Scenario: Records are allocated with raw new and stored in two containers. Neither one owns them: records\_.clear() and cache\_.clear() both just drop the pointers. Rough size: 40 properties × a few room types × a 365-day horizon is roughly 50–100k records per night, which over 8 months without a restart is gigabytes. The tempting quick fix, adding delete before clear(), would immediately turn this into a use-after-free. The API thread holds raw pointers from get() all night. Today the leak is what keeps those pointers valid. NightlyPriceJob is also copyable while holding raw owning pointers. Fix direction: Store PriceRecord by value inside the snapshot from Finding 1, and have get() return a copy or a shared\_ptr to the snapshot. Remove raw new entirely. Fix this together with Finding 1, not on its own. 5\. Only (property, type, date) keys with at least one booking get a price, which explains the stuck prices Where: L57–60, L69 Scenario: The pricing loop iterates booked, which only contains keys that appear in the feed. A property with zero bookings for a night has no rows, so no PriceRecord is produced, and the same applies to individual dates or room types with no bookings. In the sample data, H1 DLX on 2026-10-11 and H2 STD on 2026-10-11 get no price. After cache\_.clear() the API gets nullptr for those keys. If the API falls back to last-known or holds an old pointer, which Finding 4 keeps alive, it serves last week's price. The logic is also backwards for the business: zero bookings means low demand, which should give the base price, not no price. Fix direction: Drive the loop from inventory, meaning every (property, type) in capacity across the date horizon, and look up occupied in booked, defaulting to 0. Check the API's nullptr handling, and make "no price" an explicit, alerting state rather than a silent fallback. 6\. The multiplier is a step function because of integer division Where: L77–78 Scenario: occPct is truncated to an integer, and then occPct / 50 is integer division again. So the result is ×1.0 for 0–49% occupancy, ×1.1 for 50–99%, and ×1.2 from 100%. Demand only changes the price at exactly two thresholds. With the sample data, 95/120 (79%) gives 330, while 120/120 gives 360. Overbooking (occupied > total) pushes the result above 100% with no cap. If the intent was a linear "+10% per 50% occupancy", this is a silent pricing bug that has been live for 8 months. Fix direction: Confirm the intended curve with revenue management. Compute in double (occupied / static\_cast<double>(total)), clamp occupancy to \[0, 1], and apply explicit floor and ceiling bounds to the multiplier and final price. Medium 7\. The "GC pass" does nothing useful and its intent is inverted Where: L86–90, L94–96, L109 Scenario: Separate from the UB in Finding 3, the logic is broken in four ways: It never touches cache\_, so stale cache entries are never dropped. It removes keys from the stale list when they're absent from today's feed, which is the opposite of the comment. lastRunDate\_ is hard-coded and never updated, so it compares against a fixed date that will almost never be in the feed. Anything not erased accumulates in staleKeys\_ forever. The comment shows the author knew about the zero-bookings problem, but this code doesn't address it. Fix direction: Delete it once Finding 5 is fixed, since driving from inventory removes the need for it. If a stale concept is still needed, define it explicitly with a ticket describing the intended behavior. 8\. PriceRecord has no property ID, run ID or timestamp Where: L15–20, L98–102 Scenario: The record doesn't say which property it belongs to, so report() prints identical-looking lines for H1 STD and H2 STD. Nothing on the record tells the API or an on-call engineer which night produced it. That is exactly why "stuck at last week's value" was hard to diagnose. Fix direction: Add propertyId, computedAt and a run or generation ID to the record or snapshot. Expose the snapshot's age in health checks and alert if it is older than about 26 hours. 9\. The cache key is an ad-hoc concatenated string, an implicit contract with the API Where: L83 Scenario: The writer builds property|type|date inline, and the API must build the same string independently. Any format drift, such as casing, date format or an ID containing |, produces silent misses. The risk grows when competitor data adds key dimensions. Fix direction: Define a shared PriceKey struct with a hash and equality, used by both writer and reader. Low 10\. Unused free hides the overbooking case. L76 is never used and goes negative when overbooked, and it should produce a compiler warning. Remove it, or use it deliberately when handling overbooking (see Finding 6). 11\. Includes. std::tuple, std::get and std::make\_tuple are used without #include <tuple>; it compiles only through transitive includes. <memory> is included but unused. Add <tuple> and <utility>. 12\. Unnecessary copies. L70 copies a tuple of three strings every iteration, and L69 binds a non-const auto\&. Use const auto\& \[key, occupied] with structured bindings. 13\. Capacity and base prices are hard-coded inside run() (L62–67). This may just be the simplification. If production also keeps these in code or config separate from the bookings feed, onboarding a property without updating them triggers Finding 2. Confirm where they come from. Contract risks for adding competitor pricing Concurrency model: Findings 1 and 4 must be fixed first. If competitor data updates intraday, run() stops being the only writer. Decide now whether competitor updates produce a new full snapshot or patch the current one, and keep the publish step a single atomic swap either way. Reader lifetime: The API must never hold a raw pointer past one request. Make the API return values, or a snapshot handle, so a rebuild can't invalidate what a request is using. Missing inputs: Competitor data will be missing for some properties and dates. Define the fallback explicitly, such as base × demand only and marked "no competitor signal". Don't repeat Finding 5's "no row means silently stale". Bounds and sanity checks: Competitor feeds bring outliers like 0, scraped errors or currency mix-ups. Enforce a per-room-type floor and ceiling, plus a maximum change per run, before publishing, and log any rejected price. Provenance: Store each component (base, demand multiplier, competitor adjustment) on the record so a disputed price can be explained after the fact. Key schema: Settle the shared PriceKey (Finding 9) before adding dimensions. Suggested ticket order: Findings 2 and 3 first, as quick crash fixes. Then 1 and 4 together, then 5, then 6 after product sign-off on the curve. Finding 7 can be done alongside 5.

Review findings I found several production-significant issues. The two most serious are unsafe cache publication/concurrency and invalid iterator use; the zero-booking behavior is also a credible explanation for the “stuck price” symptom, although the exact symptom requires behavior outside this stripped snippet. 1\. Blocker — Cache rebuild is not thread-safe and can cause undefined behavior Lines: 53, 83, 24–27 Area: PriceCache / NightlyPriceJob::run() The pricing API is explicitly reading the cache while the nightly job calls clear() and subsequently performs put(). std::unordered\_map cannot be read concurrently with mutation without synchronization. A reader executing find() while clear() or insertion/rehashing is occurring has a data race and therefore undefined behavior. The problem is worse than merely seeing a temporarily incomplete cache: get() returns a raw pointer whose lifetime is not represented by the API contract. Failure scenario: At 02:00 an API request calls get() while run() executes cache\_.clear() or inserts records. This can produce intermittent crashes, corrupted reads, or apparently missing prices—the sort of staging-only failure that can be extremely difficult to reproduce. Suggested fix direction: Build a complete new immutable snapshot off to the side, then publish/swap the snapshot atomically under a well-defined synchronization mechanism. The API should read from a snapshot whose records remain alive for the entire lifetime of that snapshot rather than from a mutable container of raw pointers. 2\. Blocker — staleKeys\_.erase(it) invalidates the loop iterator Lines: 87–90 std::vector::erase() invalidates the erased iterator and all iterators after it. The for loop then executes ++it on the invalidated iterator. Failure scenario: As soon as an entry satisfies the condition at line 88, line 89 invalidates it; line 87's increment subsequently operates on an invalid iterator. This is undefined behavior and is a strong candidate for the unreproducible staging crash. Suggested fix direction: Use an iterator-safe erase pattern, or use an algorithm/container operation designed for removal while iterating. Add a regression test containing multiple stale entries, not just one. 3\. High — PriceRecord ownership is leaked Lines: 81–83, 29, 106–107 Every new PriceRecord allocates dynamically, but neither records\_.clear() nor PriceCache::clear() deletes the objects. NightlyPriceJob also has no destructor that releases them. Consequently, every nightly run permanently leaks all generated PriceRecords. Failure scenario: At 40 properties × many room types × many dates, repeated nightly executions continually increase process memory. The service may appear healthy for months and then gradually suffer from memory pressure. Suggested fix direction: Establish one explicit owner for records using RAII, preferably a container of smart pointers or immutable snapshot ownership. The cache should reference an owning snapshot rather than independently holding unmanaged raw pointers. 4\. High — Fixing the leak naively can create dangling cache pointers Lines: 24–31, 53–54, 81–83 The ownership model is ambiguous. records\_ contains the same raw pointers stored in cache\_, but neither component owns them explicitly. If somebody changes records\_.clear() to delete the records, the cache can immediately contain dangling pointers. Conversely, making PriceCache delete them would leave records\_ dangling. This is particularly dangerous because the API thread is expected to retain/read cache entries throughout the night. Failure scenario: A future “memory leak fix” deletes records during the next run(), while another API request still has a pointer obtained from get(). That request dereferences freed memory. Suggested fix direction: Define a single ownership/lifetime model. Cache entries should own or share immutable records, and an API lookup should never outlive the snapshot that contains its record. 5\. High — Zero-booking property/room/date combinations are never priced Lines: 57–60, 69–84 The pricing loop iterates over booked, not over the configured inventory/calendar. That means a property/room type/date with zero bookings has no entry in booked and therefore receives no PriceRecord at all. This directly matches the hint in the incident description. Failure scenario: Suppose H7/STD has 0 bookings for a future date. No new price is generated for that date. If the API treats a missing cache value as “keep/use the previous price,” it can expose the previous day's or previous successful run's price indefinitely. If the cache is rebuilt atomically, the result will instead be a cache miss—but an API fallback can turn that miss into the observed “stuck at last week's value” behavior. Suggested fix direction: Generate pricing for the complete set of property × room type × sellable date combinations, including zero-booking combinations. Separately define an explicit policy for a missing warehouse record rather than implicitly treating missing data as “retain the old price.” Important distinction: In this exact snippet, line 53 clears the cache first, so zero bookings alone cannot leave an old entry in PriceCache; it produces a missing entry. The reported “last week's value” therefore strongly suggests a fallback/cache layer outside this stripped code, or a different publication mechanism in production. 6\. High — total == 0 causes division by zero Lines: 74–78 totalRoomsFor() explicitly returns 0 when the property/room type isn't present in capacity: return it == capacity.end() ? 0 : it->second; Line 77 then performs: occupied \* 100 / total with total == 0. Integer division by zero is undefined behavior. Failure scenario: Warehouse data contains a new room type, typo, deactivated room type, or property not represented in the capacity map. A booking for it reaches the pricing loop and the process executes division by zero. Suggested fix direction: Treat missing/zero capacity as invalid input and reject/quarantine that combination before calculating occupancy. Add an explicit invariant that every priced room type has positive capacity. 7\. High — Multiplier calculation uses unintended integer division Line: 78 double mult = 1.0 + (occPct / 50) \* 0.10; Both occPct and 50 are integers, so occPct / 50 is integer division. Therefore: Occupancy occPct / 50 Multiplier 0–49% 0 1.0 50–99% 1 1.1 100–149% 2 1.2 This is a step function, not a continuous demand multiplier. Failure scenario: A room type moving from 49% to 50% occupancy suddenly gets a 10% price increase, while 50% through 99% all receive exactly the same multiplier. Conversely, 75% occupancy does not produce a 15% multiplier if that was the intended formula. Suggested fix direction: Clarify the intended demand curve and perform floating-point arithmetic explicitly if occupancy is meant to be continuous. Add boundary tests around 0%, 49%, 50%, 99%, and 100%. 8\. Medium — Occupancy can exceed 100% without validation Lines: 75–78 There is no validation that: 0 <= occupied <= total free can become negative, and occPct can exceed 100. This could happen from duplicate/incorrect warehouse data, overbooking, cancellations not reflected in the feed, or simply bad capacity data. Failure scenario: 150 booked rooms against capacity 120 produces 125% occupancy and a multiplier of 1.2 under the current integer formula. Whether that is desirable or an input error is undefined. Suggested fix direction: Define an explicit overbooking policy. Either clamp occupancy according to the pricing specification or reject/alert on impossible source data. 9\. Medium — Signed integer arithmetic can overflow Lines: 59, 77 Both booking aggregation and occupied \* 100 use int. booked\[...] += b.roomsBooked can overflow if bad/large warehouse data is supplied, and occupied \* 100 can overflow a signed int. Signed integer overflow is undefined behavior in C++. Failure scenario: An unexpectedly large booking count or aggregation across many records produces an overflow and potentially nonsensical occupancy/pricing. Suggested fix direction: Use an appropriately sized numeric type for booking quantities and perform percentage calculations in a wider type before converting to floating point. 10\. Medium — The “GC pass” does not actually remove stale cache entries Lines: 86–91 The comment says: “drop stale cache entries” but the code only removes entries from staleKeys\_. It never calls cache\_.erase() because PriceCache doesn't even expose such an operation. Failure scenario: If the cache is ever changed so that cache\_.clear() is no longer performed wholesale, stale prices can remain indefinitely. More importantly, the code gives maintainers the false impression that stale cache entries are being cleaned up. Suggested fix direction: Make cache lifecycle explicit: either rebuild/publish a complete snapshot and discard the old snapshot, or implement precise stale-key removal. Don't maintain a GC mechanism whose operation doesn't match its stated purpose. 11\. Medium — staleKeys\_ doesn't contain enough information to identify a cache entry Lines: 83, 94–95 Cache keys contain: property | type | date but staleKeys\_ contains only: property, type So even if the GC pass actually removed cache entries, it wouldn't know which date(s) to remove. Failure scenario: H1/STD has prices for October 10–20, and only October 10 becomes stale. A (H1, STD) stale marker cannot distinguish that date from the others. Suggested fix direction: Make stale state use the same identity dimensions as the cache key, or eliminate this secondary bookkeeping in favor of generation/snapshot-based cache replacement. 12\. Medium — lastRunDate\_ is inconsistent with the generated booking dates Lines: 88, 109 lastRunDate\_ is hard-coded to "2026-10-09", while the supplied bookings are for "2026-10-10" and "2026-10-11". Therefore: booked.count(property, type, "2026-10-09") will be zero for the sample data. The stale marker created in main() will consequently be removed from staleKeys\_—but the actual cache has already been globally cleared and rebuilt. Failure scenario: Production date handling drifts from the feed's business date. The stale mechanism silently makes the wrong decision. Suggested fix direction: Establish one authoritative run/business date and derive it from the job's execution context or warehouse data rather than maintaining a manually initialized string. 13\. Medium — base\[{property, type}] silently manufactures a zero base price Lines: 79, 81 std::map::operator\[] inserts a missing key with a default double value of 0.0. So an unknown property/room type does not fail validation—it produces a price of zero. Failure scenario: Booking data contains a room type absent from the base-price configuration. The service generates a seemingly valid PriceRecord with basePrice = 0 and finalPrice = 0. That could become a particularly serious pricing incident if zero is interpreted as a legitimate sell price. Suggested fix direction: Look up pricing configuration without insertion and treat a missing base price as configuration/data failure. Add validation that every generated combination has a positive, valid base price. 14\. Low — free is calculated but never used Line: 76 int free = total - occupied; The value has no effect on the result. This isn't itself a correctness bug, but it suggests the occupancy calculation may have evolved from a different pricing formula and that important business logic may be missing. Suggested fix direction: Remove it if unnecessary, or incorporate it if availability was intended to influence pricing. Add tests around capacity/availability if it is part of the pricing specification. 15\. Low/Medium — Raw-pointer ownership makes copy/move semantics unsafe for future maintenance Lines: 106–108 NightlyPriceJob contains raw pointers with implicit compiler-generated copy/move operations. The current class happens not to define a destructor, so copying it primarily compounds leaks and duplicated pointer state. If a future developer adds a destructor to correct the memory leak, ordinary copying can become a double-delete problem. Suggested fix direction: Make ownership explicit with RAII and define/disable copy/move semantics according to the intended ownership model. This should be settled before the class gains more state for competitor pricing. What most likely explains the two incidents? “One property stuck at last week's value” The strongest code-level issue is the absence of a price when there are zero bookings. The current algorithm effectively says: “If this combination appears in the booking feed, calculate a price; otherwise, don't produce a price.” That is usually wrong for hotel pricing because 0 bookings is valid demand information and should normally produce a price, not absence of a price. However, there is an important nuance: this exact snippet clears the cache at line 53. Therefore the snippet itself would yield a cache miss, not literally retain last week's record. The likely production chain is: zero bookings → no new record → cache miss → API/fallback retains previous successful price → customer sees stale price. That should be investigated alongside the actual API's missing-price behavior. Staging crash The most obvious deterministic UB is: line 89 staleKeys\_.erase(it) → line 87 increments invalid iterator. The second major crash candidate is: API thread reads unordered\_map while nightly thread clears/inserts it. And the third is: capacity missing/zero → line 77 division by zero. Those three should be treated as independent crash paths rather than assuming the staging incident had one root cause. Priority ticket order I would turn the review into tickets approximately in this order: BLOCKER: Replace mutable shared cache rebuild with an atomic/locked immutable snapshot model. BLOCKER: Fix staleKeys\_ iterator invalidation. HIGH: Establish record ownership/lifetime with RAII; eliminate raw owning pointers. HIGH: Generate prices for zero-booking combinations. HIGH: Validate positive capacity before occupancy calculation. HIGH: Correct/confirm the intended multiplier arithmetic and add boundary tests. MEDIUM: Reject missing base-price configuration instead of silently generating zero. MEDIUM: Validate occupancy/booking quantities and prevent numeric overflow. MEDIUM: Remove/redesign the ineffective stale-cache mechanism. MEDIUM: Define the business-date contract instead of hard-coding lastRunDate\_. LOW: Clean up unused calculations and formalize copy/move semantics. One architectural requirement before competitor pricing I would not add competitor pricing to the current cache design. Competitor data will increase both the amount of computation and the likelihood that multiple producers/readers interact with the cache, while the current design has no defined consistency boundary. The desired contract should be something like: A pricing API request observes one complete pricing snapshot—never a partially rebuilt snapshot—and every record returned from that snapshot remains valid for the duration of the read. Once that contract exists, competitor pricing can be another input to the snapshot-building phase without exposing the API to half-old/half-new prices.

Review Notes: NightlyPriceJob Below are findings ordered by severity, each with file:line references (line numbers based on the posted snippet), failure scenario, and fix direction. BLOCKER B1. PriceCache::clear() leaks every PriceRecord on every run Location: PriceCache::clear (\~line 30) and NightlyPriceJob::run (\~line 64 cache\_.clear(); records\_.clear();) Scenario: PriceCache stores raw PriceRecord\* and clear() only clears the unordered\_map, not the pointed-to objects. records\_ is a vector<PriceRecord\*> that is also just cleared. On every nightly run, all PriceRecords from the previous run are leaked. Over 8 months of nightly runs (\~240 runs), this is a steady leak in a long-lived process. records\_ and cache\_ both hold non-owning raw pointers to the same heap allocations, and neither frees them. If the process is long-lived (it is — the API thread reads the cache "during the whole night", implying a persistent service), this is unbounded growth. Fix direction: Make ownership explicit: either store std::unique\_ptr<PriceRecord> in one owner container and raw pointers/weak\_ptr in the cache, or use std::shared\_ptr<PriceRecord> in both. Never use a raw pointer as the sole reference to a heap allocation held in two containers. B2. cache\_.clear() publishes an empty cache while the API thread reads it — data race / torn read Location: run() line \~64 (cache\_.clear();) and PriceCache::put/get Scenario: The prompt states the cache is read by the API thread throughout the night. run() mutates the same unordered\_map (clear(), then put() for each record) with no synchronization. Concurrent get() from the API thread during a rebuild is UB: iterator invalidation, rehash, and reading a half-populated map. This is the most likely cause of the "prices stuck at last week's value for one property for a day" symptom if the API thread happened to read during the window, or if a crashed/half-completed run left the cache in a partial state. Fix direction: Build the new cache off to the side (a local unordered\_map), then swap it in atomically under a mutex or std::atomic<std::shared\_ptr<...>> (double-buffering / RCU-style publish). Never clear-then-refill a live cache. B3. Iterator invalidation / use-after-erase in the "GC pass" (staleKeys\_.erase(it) inside a range-for over the same vector) Location: run() lines \~82–88, specifically staleKeys\_.erase(it); inside for (auto it = staleKeys\_.begin(); ...). Scenario: std::vector::erase invalidates it and all iterators at or after the erase point. The loop then does ++it on an invalidated iterator — undefined behavior. In practice this often "works" for a while and then corrupts memory or skips elements, which matches the "one crash in staging we couldn't reproduce" symptom. Also note the loop is a for over staleKeys\_ but the erase is on staleKeys\_ itself, so the container is being mutated during iteration. Fix direction: Use the erase-remove idiom (std::remove\_if) or iterate with an index and rebuild into a new vector. Do not erase from a container while iterating it with the iterator being advanced. B4. base\[{property, type}] uses operator\[] on a std::map — silently inserts default 0.0 for unknown (property, type) Location: run() lines \~79 and \~81 (double price = base\[{property, type}] \* mult; and the new PriceRecord{... base\[{property, type}] ...}). Scenario: If the warehouse feed ever contains a (property, type) pair not in the hard-coded base map (which will happen as soon as you add competitor data or a new property/room type), operator\[] default-constructs a 0.0 entry and the code prices the room at 0 \* mult = 0. Worse, the map is mutated, so the "unknown" entry persists for the rest of the run. There is no validation that base and capacity cover every (property, type) in the feed. Fix direction: Use base.at({property, type}) (or .find() with an explicit error/skip path) so an unknown pair fails loudly instead of silently pricing at zero. Validate feed coverage against the base/capacity maps at the top of run(). HIGH H1. Integer division truncates occPct, biasing the multiplier downward — and total == 0 divides by zero Location: run() lines \~75–77: cpp int free = total - occupied; // (a) int occPct = occupied \* 100 / total; // (b) double mult = 1.0 + (occPct / 50) \* 0.10; // (c) Scenario (a): free is computed but never used — dead code, and it will trigger -Wunused-variable; more importantly it signals the intended occupancy logic was never finished (likely meant to feed the multiplier). Scenario (b): occupied \* 100 / total is integer division, so occPct is truncated (e.g. 95/120 → 79, not 79.17). For total == 0 (unknown property/type, or a room type with zero capacity) this is integer division by zero — UB, not a catchable exception. This is the crash-class bug. Scenario (c): occPct / 50 is also integer division. So occPct in \[0,49] → 0, \[50,99] → 1, \[100,149] → 2, etc. The multiplier is a step function, not the intended 1.0 + (occPct/50.0)\*0.10. At 95% occupancy the multiplier is 1.20 instead of 1.19; at 49% it's 1.00 instead of 1.098. This is a silent pricing-accuracy bug across the whole fleet. Fix direction: Use double occPct = total > 0 ? (static\_cast<double>(occupied) \* 100.0 / total) : 0.0; and double mult = 1.0 + (occPct / 50.0) \* 0.10;. Guard total == 0 explicitly and log/skip. H2. records\_ and cache\_ hold dangling pointers after clear() — the API thread can read freed/garbage memory Location: PriceCache::clear (\~line 30, comment "does NOT delete records") and run() lines \~64–65. Scenario: cache\_.clear() empties the map, but records\_ still holds pointers to the same PriceRecords until records\_.clear() runs on the next line. If the API thread calls cache\_.get(...) between those two lines (or during the whole rebuild), it can observe a half-torn state. Even after the rebuild, records\_ is a second, independent reference to the same heap objects with no ownership — if anyone ever frees via records\_, the cache dangles, and vice versa. This is the classic double-ownership bug and is the likely mechanism behind "prices stuck at last week's value" (stale pointer still reachable, or a torn map state). Fix direction: Single owner (std::vector<std::unique\_ptr<PriceRecord>> or shared\_ptr), cache holds non-owning PriceRecord\*/weak\_ptr only, and publish the new cache atomically (see B2). H3. staleKeys\_ logic is broken: wrong key tuple and never actually removes the stale cache entry Location: run() lines \~82–88 and markStale (\~line 91). Scenario: Three problems: The GC checks booked.count(std::make\_tuple(it->first, it->second, lastRunDate\_)) — it matches on lastRunDate\_ ("2026-10-09"), a hard-coded date that is not the date of the bookings being processed (the feed has 2026-10-10/2026-10-11). So the check almost never matches, and stale keys are effectively never erased. lastRunDate\_ is never updated by run(), so it's stale forever. Even if it matched, the code erases from staleKeys\_ (the stale-tracking list) but never calls cache\_.clear() for that key or removes the corresponding PriceRecord. The comment says "drop stale cache entries", but no cache entry is dropped — only the bookkeeping entry. Scenario that matches the incident: A property has zero bookings in a night's feed. Its (property, type, date) tuples are absent from booked, so no new PriceRecord is created and no cache\_.put happens. But cache\_.clear() at the top of run() wipes the map, so the property's old price is gone — unless the API holds a pointer from a previous get(). Either the property disappears from the cache (and the API serves last-known / default), or — if the GC logic had been "fixed" to keep it — it would serve last week's value. This is the direct explanation of the "stuck prices for one property for a day" symptom: zero-booking properties are not re-priced and not explicitly carried forward or invalidated. There is no defined behavior for "no bookings today." Fix direction: Define explicit semantics for zero-booking (property, type, date): either carry forward the last price or emit a default/floor price, and do it in run() — don't rely on staleKeys\_. Fix the GC to remove the cache entry and the PriceRecord, and key it on the actual run date, not a hard-coded string. H4. API contract is undefined for concurrent reads during rebuild; no snapshot/versioning Location: Class-level: PriceCache has no mutex, no version, no snapshot; run() rebuilds in place. Scenario: Adding competitor data next quarter means more fields per record, more frequent rebuilds, and likely a longer rebuild window. The current contract ("cache is read by the API thread during the whole night") is violated by clear() + refill. Even single-threaded, a reader that obtained a PriceRecord\* before a rebuild holds a pointer to an object that may be freed (once B1/H2 are fixed) or overwritten. Fix direction: Publish an immutable snapshot: build a new shared\_ptr<const CacheSnapshot>, then atomically swap the shared\_ptr the API reads. Readers take a shared\_ptr copy for the duration of their request, guaranteeing the record outlives their use. MEDIUM M1. PriceRecord has no propertyId — key is reconstructed by string concatenation, risking collisions Location: struct PriceRecord (\~lines 19–24) and cache\_.put(property + "|" + type + "|" + date, rec) (\~line 80). Scenario: The record stores only roomType, date, basePrice, finalPrice. The property is only in the concatenated cache key. If any field ever contains "|" (or if the delimiter choice changes), keys collide and one property overwrites another's price — a plausible cause of cross-property price confusion. Also, the API must reconstruct the same key string, duplicating the format in two places. Fix direction: Add propertyId to PriceRecord and use a structured key (e.g. std::tuple<std::string,std::string,std::string> in a std::map, or a small struct with operator==/hash). Centralize key construction in one function. M2. lastRunDate\_ is a hard-coded member, never set from the feed or clock Location: line \~97 (std::string lastRunDate\_ = "2026-10-09";) and its use in the GC (\~line 85). Scenario: The job "runs nightly" but has no notion of today. The GC compares against a fixed date. Any date-based logic (stale detection, carry-forward, idempotency) is therefore wrong. This is also why the GC never fires. Fix direction: Pass the run date into run(const std::string\& runDate) or derive it from std::chrono::system\_clock (UTC), and use it consistently for GC and for "no bookings today" handling. M3. staleKeys\_ grows unbounded; markStale can add duplicates Location: markStale (\~line 91) and the GC that only erases on a match. Scenario: Because the GC's match condition is almost never true (H3), staleKeys\_ only grows. markStale does no dedup, so repeated calls add duplicates. Over months this is another slow leak. Fix direction: Store stale keys in a std::set/unordered\_set, dedup on insert, and clear entries only when the GC actually removes the corresponding cache entry. M4. report() dereferences records\_ which may contain pointers to objects also in cache\_; no null checks, no ownership Location: report() (\~lines 93–99). Scenario: If any code path ever inserts a null into records\_ (none currently, but the pattern invites it) or frees via the cache, report() dereferences freed memory. It also prints only roomType and date, not propertyId, so it can't disambiguate H1/H2 records — a diagnostics gap. Fix direction: Use unique\_ptr/shared\_ptr and iterate by reference; add propertyId to the output. LOW L1. free is computed and never used Location: line \~75 (int free = total - occupied;). Scenario: Dead code; compiler warning; suggests the occupancy/multiplier logic is incomplete (likely free was meant to drive a "scarcity" multiplier). Fix direction: Remove it or use it explicitly in the multiplier; if it's intended for future use, comment why. L2. capacity and base are hard-coded locals inside run() Location: lines \~71–76. Scenario: For a 40-property chain, this must come from the warehouse/config. Hard-coding means the nightly job silently prices only the 3 known pairs and (via B4) zero-prices anything else. It also makes the "unknown property" path untestable. Fix direction: Load capacity and base from the same warehouse/config source as bookings; validate coverage. L3. occPct naming and int arithmetic invite future mistakes Location: line \~76. Scenario: Naming an int occPct (a percentage) and then dividing it again by 50 is easy to misread. A future editor may "fix" it by changing 50 to 50.0 in one place and not the other. Fix direction: Compute occupancy as a double ratio (0.0–1.0 or 0–100) once, name it clearly (occupancyRatio), and derive the multiplier from it in one expression. L4. main() calls run() twice and markStale in between — test harness semantics leak into the design Location: main() (\~lines 101–107). Scenario: run() is not idempotent and markStale is only meaningful between runs. The public API doesn't make the lifecycle explicit; a caller can markStale after the last run() and it silently does nothing useful. Fix direction: Separate "ingest" (build snapshot) from "publish" (swap cache), and make markStale operate on the next run's input, not on a side vector. Summary of incident mapping Symptom Most likely findings Prices stuck at last week's value for one property for a day H3 (zero-booking property has no re-price path; GC keyed on hard-coded date never fires), B2/H2 (torn cache during rebuild; stale pointer still reachable), M2 (no real run date) Unreproducible crash in staging B3 (iterator invalidation in GC erase), H1 (integer divide-by-zero when total == 0), B4 (silent map mutation via operator\[]) Top priorities before adding competitor data B1 + B2 + H2: single ownership + atomic snapshot publish — this is the API-contract fix you'll need for concurrent reads. B3: fix the GC erase; it's UB today. B4 + H1: fail loudly on unknown (property, type) and use floating-point occupancy with a total == 0 guard. H3 + M2: define and implement "no bookings today" semantics with a real run date. I'd turn B1–B4 and H1–H3 into tickets first; the rest can ride along with the competitor-pricing work.

Here are the findings and code review notes for the NightlyPriceJob service, categorized by severity.Blocker Findings1. Pointer Ownership \& Memory Leak Across Job RunsSeverity: BlockerFile / Line Reference: Lines 22, 63–65Failure Scenario: cache\_.clear() empties the std::unordered\_map without deleting the heap-allocated PriceRecord pointers created via new. The records\_ vector is also cleared without deleting its contents, permanently leaking memory every night. In production, this causes continuous memory growth until the process encounters an out-of-memory crash (likely explaining the staging crash). Furthermore, raw non-owning pointers are exposed to concurrent readers, risking use-after-free or dangling pointers if the cache is cleared while a reader accesses a record.Suggested Fix: Replace raw pointers (PriceRecord\*) with smart pointers (e.g., std::shared\_ptr<const PriceRecord> or std::unique\_ptr). Alternatively, manage ownership in records\_ using std::vector<PriceRecord> or std::vector<std::unique\_ptr<PriceRecord>> and store raw non-owning pointers/weak pointers in PriceCache with clear lifecycle bounds.2. Undefined Behavior via Iterator Invalidation During Container EraseSeverity: BlockerFile / Line Reference: Lines 68–72Failure Scenario: Calling staleKeys\_.erase(it) inside the loop invalidates it. The loop then attempts ++it on an invalidated iterator, resulting in undefined behavior (UB), heap corruption, or an immediate crash. This directly explains the intermittent crash observed in staging whenever staleKeys\_ was non-empty and matching condition criteria were met.Suggested Fix: Use the C++20 std::erase\_if idiom or the classic C++11 remove-erase idiom:staleKeys\_.erase(std::remove\_if(staleKeys\_.begin(), staleKeys\_.end(), ...), staleKeys\_.end());3. Data Race \& Concurrent Read/Write HazardsSeverity: BlockerFile / Line Reference: Lines 20, 38–39, 65Failure Scenario: run() directly mutates cache\_ (clear() and put()) while the API thread is concurrently reading from it via cache\_.get(). Modifying a std::unordered\_map without synchronization while other threads read from it causes data races and UB, leading to reads returning corrupted pointers, null dereferences, or infinite loops inside unordered\_map::find().Suggested Fix: Implement atomic double-buffering or pointer-swapping: compute the new cache entirely off-line into a new instance, then swap or assign a std::shared\_ptr<const PriceCache> atomically under a mutex or using std::atomic\_load/std::atomic\_store (or std::atomic<std::shared\_ptr>).High Severity Findings4. Properties with Zero Bookings Are Omitted (Explains "Stuck Prices")Severity: HighFile / Line Reference: Lines 41–44, 53–66Failure Scenario: booked is aggregated solely from active entries in loadBookings(). If a property has zero bookings on a given night, no entries exist in booked for it, so the loop in line 53 completely skips generating new price records for that property. Since the GC logic in lines 68–72 only removes keys from staleKeys\_ rather than purging stale records out of cache\_, the previously cached prices remain in memory indefinitely. This directly explains why prices remained stuck at last week's values for properties with zero bookings.Suggested Fix: Iterate over all known property/room-type combinations from capacity or base rather than iterating strictly over active booked entries, ensuring a zero-booking record is generated and price records are explicitly updated or evicted for every valid property.5. Integer Division Truncation in Multiplier CalculationSeverity: HighFile / Line Reference: Line 60Failure Scenario: In (occPct / 50), both occPct and 50 are integers. Integer division truncates towards zero. As a result, any occupancy percentage below 50% yields 0 (giving mult = 1.0), occupancy from 50% to 99% yields 1 (giving mult = 1.10), and 100% yields 2 (giving mult = 1.20). Floating-point granularity is entirely lost, leading to step-function pricing leaps rather than continuous scaling.Suggested Fix: Cast to floating-point before division, for example: double mult = 1.0 + (static\_cast<double>(occPct) / 50.0) \* 0.10;.6. Potential Division by ZeroSeverity: HighFile / Line Reference: Lines 47–49, 59Failure Scenario: If a property or room type combination is absent from capacity, totalRoomsFor returns 0. Line 59 then executes occupied \* 100 / total, performing a division by zero, which results in undefined behavior (typically an immediate process crash on integer division by zero).Suggested Fix: Check if total == 0 before calculating occPct, and explicitly handle missing capacity (e.g., log an error, skip pricing calculation, or raise a fall-back metric).Medium Severity Findings7. Implicit Insertion in std::map Modifies Pricing Source DataSeverity: MediumFile / Line Reference: Line 61Failure Scenario: base\[{property, type}] uses std::map::operator\[], which is non-const and inserts a default-constructed entry (0.0) if the key {property, type} does not exist in base. If a booking references an unknown room type, it gets priced at $0.00 and mutates the base map.Suggested Fix: Use base.at({property, type}) or base.find(...) to safely retrieve base prices without mutating the map, throwing or falling back safely if the key is absent.8. Date Field Mismatch in GC/Stale Purge LogicSeverity: MediumFile / Line Reference: Line 69Failure Scenario: staleKeys\_ checks against lastRunDate\_ ("2026-10-09"), whereas incoming bookings in loadBookings() feature future target dates (e.g., "2026-10-10"). Comparing target stay dates against lastRunDate\_ causes stale checks to fail consistently, leaving stale tracking ineffective.Suggested Fix: Align the date parameters in staleKeys\_ with the actual target stay dates being processed by the job rather than hardcoding or using an un-updated lastRunDate\_.Low Severity Findings9. Missing Rule-of-Five / Class Resource CleanupsSeverity: LowFile / Line Reference: Lines 19–24, 76Failure Scenario: PriceCache and NightlyPriceJob manage heap resources (PriceRecord\*) but do not define custom destructors, copy operations, or move operations. If instances are copied, multiple containers will hold duplicate raw pointers without clear ownership.Suggested Fix: Adhere to the Rule of Zero by replacing all raw pointers with smart pointers (std::shared\_ptr / std::unique\_ptr), which automates proper cleanup and copy/move semantics.Summary Table for Ticket CreationIDTitleSeverityLocation1Heap Memory Leak and Raw Pointer Lifecycle in PriceCacheBlockerLines 22, 63–652Undefined Behavior: Vector Iterator Invalidation in staleKeys\_ PurgeBlockerLines 68–723Concurrent Read/Write Race Condition on Cache During Job ExecutionBlockerLines 20, 38–39, 654Missing Pricing Updates / Stale Cache Retention for Properties with 0 BookingsHighLines 41–44, 53–665Integer Truncation in Demand Multiplier CalculationHighLine 606Division by Zero Risk in occPct CalculationHighLines 47–49, 597Side-Effect Map Insertion and Zero Base Price on Missing KeyMediumLine 618Date Logic Inconsistency in Stale Key PurgingMediumLine 699Missing Copy/Move Constructors and Destructors (Rule of Zero Violation)LowLines 19–24, 76