feat(finance): exclude MB products from generic calc engine and enforce MB batch isolation - #166
Merged
Merged
Conversation
MB costs are owned by MB Batch (MB Push to Head). Letting a calc job also compute them gave two writers for the same (product, period, calc_type): a calc job run after a push superseded the APPROVED cst_product_cost row, leaving cst_mb_cost pointing at a SUPERSEDED source with a stale value. Exclusion is layered: - Orchestrator (authoritative): allActiveProducts and productsByType filter MB out of the seed set; resolveInitialSet rejects SINGLE_PRODUCT on an MB product and FILTERED by the MB type. Under ALL the drop is silent — MB is simply not part of what the calc engine owns. loadProductRMEdges is deliberately left unfiltered: the rule constrains what a job targets, not how dependencies resolve, so yarn products consuming MB still price correctly. - Finance trigger handler: an optional MBTypeChecker guard turns what would be an async FAILED job into a synchronous 400 before any cal_job row is written. Wired via WithMBGuard so existing call sites are unchanged. Verified against dev: scope ALL seeds 9457 products vs 13625 before, a difference of exactly 4168 — the MB active-route count — and writes zero MB rows. Forced MB paths (FILTERED type 81, SINGLE_PRODUCT on an MB product) both return 400. Non-MB types are untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MB products are computed exclusively by MB Batch and approved via push-to-head. Three gaps let that invariant erode silently. 1. needs-repush visibility (mbpush) Re-running MB Batch after a push supersedes the approved source row and inserts a fresh CALCULATED one, leaving cst_mb_cost pointing at a superseded row. The value is not wrong -- it is the last approved one -- but nothing told the operator a newer result was waiting. Preview now flags such heads and reports needs_repush_count. They remain pushable; re-pushing is the remedy. Classification is read-only: no bucket moves, no auto-push, no write-path change. 2. manual verify/approve accepted MB rows (costcalc) VerifyCost/ApproveCost took any cost_id. Flipping an MB row to VERIFIED made push-to-head skip it, since push requires status exactly CALCULATED. Both now reject MB-owned rows with a 400. MarkApprovedFromCalculatedTx -- the legitimate push path -- is a distinct method and stays unblocked. 3. calc engine could persist an MB dependency node (costcalc) loadProductRMEdges is deliberately unfiltered so a parent consuming an MB still resolves it; that left ProcessChunk able to supersede an MB row pulled in as a dependency. Closed at the write site: such products are marked BLOCKED with reason MB_OWNED_BY_MB_BATCH instead of persisted. Parents read committed state via LoadUpstreamCosts, so skipping the persist changes no parent's number. Non-MB products are unaffected: every guard keys on a single cpt_type_code = 'MB' equality and degrades to a no-op when unwired. Verified against live data -- all 350 PRODUCT edges to an MB come from another MB, zero from non-MB. Tests: finance and finance-cost-orchestrator go test -race pass, including TestDagBuilder_All_KeepsEveryNonMBProduct, TestProcessChunk_AllNonMB_EveryProductPersisted, and the non-MB pass-through cases for both guards. golangci-lint v2.3.0: 0 issues. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ilramdhan
added
documentation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR enforces that MB products are calculated exclusively by MB Batch and approved via push-to-head. Previously, generic calculation jobs could also calculate MB products, resulting in multiple writers for the same
(product, period, calc_type)tuple and causingcst_mb_costto point to superseded source rows with stale values.To resolve this and prevent state erosion, changes were made across multiple layers:
ALL,FILTERED,SINGLE_PRODUCT) and trigger handlers return a synchronous400Bad Request prior to job creation.ProcessChunkmarks MB dependency nodes asBLOCKEDwith reasonMB_OWNED_BY_MB_BATCHinstead of persisting superseded rows.mbpushpreview (needs_repush_count).VerifyCostandApproveCostfrom accepting MB-owned cost rows.Type of Change
Service(s) Affected
Changes Made
allActiveProducts,productsByType) infinance-cost-orchestrator.resolveInitialSetto rejectSINGLE_PRODUCTon MB products andFILTEREDjobs targeting MB type.WithMBGuard(MBTypeChecker) in finance trigger handler to return synchronous400 Bad Requestbefore creatingcal_jobrows.loadProductRMEdgesunfiltered so parent products consuming MB still resolve and price correctly.ProcessChunkto mark MB dependency nodes asBLOCKEDwith reasonMB_OWNED_BY_MB_BATCHinstead of persisting superseded rows.needs_repush_counttracking inmbpushpreview to inform operators when newer calculated results are pending re-push.VerifyCostandApproveCostfrom accepting MB-owned cost rows (returns400 Bad Request).Related Issues
Fixes #
Related to #
API Changes (if applicable)
Proto Changes
Breaking Changes
SINGLE_PRODUCTorFILTEREDtype 81) now returns a synchronous400 Bad Requestinstead of failing asynchronously or generating invalid cost rows.Testing Performed
Unit Tests
Integration Tests
Manual Testing
Lint & Build
golangci-lint run ./...passesgo build ./...succeedsgo test -race ./...passesDatabase (if applicable)
Documentation
Rollback Plan
Revert code changes in
financeandfinance-cost-orchestrator. All guards key strictly oncpt_type_code = 'MB'and degrade to no-ops when unwired.Screenshots/Logs (if applicable)
Pre-merge Checklist
Reviewer Notes
loadProductRMEdgesremains unfiltered so parent products consuming MB items continue to resolve and calculate costs correctly using committed state viaLoadUpstreamCosts.cpt_type_code = 'MB'.TestDagBuilder_All_KeepsEveryNonMBProduct,TestProcessChunk_AllNonMB_EveryProductPersisted, and non-MB pass-through guard test cases.golangci-lint v2.3.0reported 0 issues.