Skip to content

fix(finance): resolve bulk MB dependency ordering, null scan crash, and job status contract - #178

Merged
ilramdhan merged 3 commits into
mutugading:mainfrom
ilramdhan:fix/mb-recipe-31-aug-be
Sep 2, 2026
Merged

fix(finance): resolve bulk MB dependency ordering, null scan crash, and job status contract#178
ilramdhan merged 3 commits into
mutugading:mainfrom
ilramdhan:fix/mb-recipe-31-aug-be

Conversation

@ilramdhan

@ilramdhan ilramdhan commented Sep 2, 2026

Copy link
Copy Markdown
Member

Description

This pull request addresses two critical reliability and correctness issues in bulk Material Balance (MB) head processing within the Finance Service:

  1. Contract Alignment for Job Status: Maps the internal parent job status SUCCESS to either PARTIAL (if FailedChildren() > 0) or DONE, matching the documented GetBulkMBHeadJobStatusResponse.status contract so frontend polling properly recognizes terminal partial-success states.
  2. Dependency Ordering & Crash Prevention: Reorders bulk MB transition batch execution using Kahn's algorithm topological sorting based on composition dependencies (ListMBRefEdgesForBatch), ensuring upstream referenced heads finish prior to dependents. Additionally hardens mbResolveRefProductSysID to scan mbh_cost_product_id into sql.NullInt64, returning a clean domain error instead of panicking on ungenerated cost products.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 💥 Breaking change (fix or feature that changes existing API)
  • ♻️ Refactor (code change without new feature or bug fix)
  • 📚 Documentation update
  • 🧪 Test update
  • 🔧 Chore (dependencies, config, etc.)

Service(s) Affected

  • Finance Service
  • IAM Service
  • Shared Proto (gen/)
  • Root/Common

Changes Made

  • Status Mapping: Updated GetBulkMBHeadJobStatusResponse translation to map job.Status == SUCCESS to PARTIAL when FailedChildren() > 0 and DONE otherwise, utilizing the existing counter from ListBulkMBHeadJobFailures.
  • Topological Sorting: Implemented Kahn's algorithm in dependency_order.go using ListMBRefEdgesForBatch (mbcomposition.Repository) to sequence published transitions in dependency order, with a graceful fallback to original batch order upon cyclic dependencies or lookup errors.
  • Scan Hardening: Updated mbResolveRefProductSysID to scan mbh_cost_product_id into sql.NullInt64, safely returning a domain error when cost product reference has not yet been populated.

Related Issues

Fixes #
Related to #

API Changes (if applicable)

Proto Changes

# No protobuf changes required; fixes contract adherence to existing proto specification

Breaking Changes

None.

Testing Performed

Unit Tests

  • New unit tests added
  • Existing unit tests pass
  • Coverage maintained/improved

Integration Tests

  • New integration tests added
  • Existing integration tests pass

Manual Testing

# Verify bulk MB head status polling response returns PARTIAL / DONE
grpcurl -plaintext localhost:50051 finance.v1.MBService/GetBulkMBHeadJobStatus

# Trigger bulk transitions with interdependent MB heads to verify topological sequence
grpcurl -plaintext -d '{"batch_id": "test-batch-1"}' localhost:50051 finance.v1.MBService/ExecuteBulkTransition

Lint & Build

  • golangci-lint run ./... passes
  • go build ./... succeeds
  • go test -race ./... passes

Database (if applicable)

  • Migration added
  • Migration tested (up and down)
  • No breaking schema changes (or documented)

Documentation

  • README.md updated (if needed)
  • RULES.md updated (if needed)
  • Proto comments updated
  • OpenAPI regenerated

Rollback Plan

Revert this PR commit and redeploy finance-service.

Screenshots/Logs (if applicable)

N/A

Pre-merge Checklist

  • I have read and followed RULES.md
  • I have read and followed CONTRIBUTING.md
  • Clean Architecture principles followed
  • All errors are properly handled
  • Context is passed appropriately
  • Structured logging is used
  • No hardcoded secrets
  • PR description is complete and clear
  • CI checks are passing

Reviewer Notes

  • Please verify the cycle detection fallback in dependency_order.go.
  • Ensure domain errors returned from mbResolveRefProductSysID are mapped properly upstream to job failure records.

ilramdhan and others added 2 commits September 1, 2026 23:50
@ilramdhan @claude
fix(finance): translate internal job status to documented bulk MB hea…
5136e44 
…d contract

The parent job's internal job.Status only knows SUCCESS/FAILED, but the
GetBulkMBHeadJobStatusResponse.status field is documented as
QUEUED/PROCESSING/DONE/FAILED/PARTIAL. A batch with at least one successful
child was reported as raw "SUCCESS" even when some children failed, so
frontend polling never recognized the batch as a terminal partial-success
state. This maps SUCCESS with FailedChildren() > 0 to PARTIAL and otherwise
to DONE, using the same FailedChildren() counter ListBulkMBHeadJobFailures
already relies on.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ilramdhan @claude
fix(finance): order bulk MB transitions by composition dependency, fi…
d41e868 
…x NULL scan crash

Two guards against the same problem class: cross-head ordering within a bulk
transition batch. The mb_bulk_transition worker is a single sequential
consumer, so publish order determines processing order — if a batch contains
both an MB head and another head it references as a nested RM input, the
dependent could be validated before its dependency finished, hitting
mbResolveRefProductSysID with mbh_cost_product_id still NULL and crashing on
a bare int64 scan. This adds a Kahn's-algorithm topological sort
(dependency_order.go) that reorders each batch using a new
ListMBRefEdgesForBatch query (mbcomposition.Repository /
MBCompositionRepository) before publishing children, falling back to
original order on any lookup failure or cycle, and separately hardens
mbResolveRefProductSysID to scan mbh_cost_product_id into sql.NullInt64 and
return a clear domain error instead of panicking when the referenced head
hasn't generated its cost product yet.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ilramdhan ilramdhan added this to the Costing Release Milestone milestone Sep 2, 2026
@ilramdhan ilramdhan self-assigned this Sep 2, 2026
Copilot AI lite review requested due to automatic review settings September 2, 2026 14:01
@ilramdhan ilramdhan added bug Something isn't working fix labels Sep 2, 2026
Copilot AI reviewed Sep 2, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@ilramdhan @claude
refactor(finance): reduce cognitive complexity of kahnTopoSort
1191731 
golangci-lint (gocognit) flagged kahnTopoSort at complexity 32 (>20) in CI.
Split it into four small helpers (buildDependsOn, isReady, appendReadyNodes,
appendStalledInOriginalOrder) with identical logic and iteration order —
behavior and tests are unchanged.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@ilramdhan ilramdhan closed this Sep 2, 2026
@github-project-automation github-project-automation Bot moved this from Todo to Done in Goapps Roadmap [Backend] Sep 2, 2026
@ilramdhan ilramdhan reopened this Sep 2, 2026
@ilramdhan
ilramdhan merged commit 3e99f06 into mutugading:main Sep 2, 2026
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort, defaults to Lite
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments

Assignees

@ilramdhan ilramdhan

Labels

bug Something isn't working fix

Projects

Goapps Roadmap [Backend]
Status: Done

Milestone

Costing Release Milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ilramdhan