Skip to content

fix(purchases): FOLD-1037 robustness findings (fanout ctx guard, slice aliases, commitmentopts, audit markers) #1069

Description

@cristim

Problem

A cluster of Medium/Low/Nit robustness and correctness findings from code-review report 05, routed to PR #1037 (fix/money-path-idempotency) because they touch the same files.

05-H3 — fanout semaphore ignores context cancellation

internal/execution/fanout.go:70-72

sem <- struct{}{} in the goroutine-launch loop blocks unconditionally even when ctx is already cancelled. A large account-count fan-out after deadline expiry can block indefinitely on the semaphore instead of draining quickly. Fix: select{case sem<-struct{}{}: case <-ctx.Done():} with the remaining slots recorded as ctx.Err().

05-M1 — recs[:0] slice aliasing in suppression and override filters

internal/scheduler/scheduler.go:1144 and scheduler_overrides.go:55

applySuppressionIndex and filterRecsByResolvedConfigs both write out := recs[:0], which aliases the caller's backing array. Any call site that holds a reference to the original slice (or iterates with range before calling these) will see mutations. Fix: allocate a fresh make([]config.RecommendationRecord, 0, len(recs)).

05-M4 — commitmentopts: permissive fallback when provider known but service absent

internal/commitmentopts/service.go:148-176

Validate returns true (permissive) when the provider is in the probe data but the service is not. Since the probe data is provider-scoped, an absent service means it is not commitment-capable per the probe — a warn+metric on this path would surface misconfigured or novel service combos rather than silently permitting them.

05-L1 — free-text audit-gap marker is unqueryable

internal/purchase/execution.go:477-486,497-504

recordHistoryAuditGap appends a free-text note to exec.Error. Adding a structured prefix (e.g. history_write_failed: + commitment ID) makes the gap visible to programmatic log searches and alerting without changing the UI-facing description.

05-L2 — getAWSAccountID returns sentinel string, caller should decide

internal/purchase/execution.go:885-902

getAWSAccountID returns the string "unknown" on all failure paths. The caller decides how to use the account ID (tagging, logging); returning ("", error) lets the caller choose between logging with a sentinel vs failing loudly vs skipping.

05-L3 — normalizePurchaseSource proceeds untagged on invalid source

internal/purchase/execution.go:526-537

When NormalizeSource rejects the source, the purchase proceeds with an empty tag. Consider failing the rec rather than silently untagging, or at minimum logging at Error level (currently Warn). Decision: proceed with warn+untag (safe/reversible default) with an explanatory comment.

05-N1 — dead _ = now placeholder in reaper

internal/purchase/reaper.go:222,227

Two _ = now lines suppress the unused-variable warning but now is never used in the function body. Either wire up an injected clock or remove the parameter; the latter is the minimal fix.

05-N2 — missing contract comment on processPurchaseRecommendations

internal/purchase/execution.go:374

The aggregation loop in processPurchaseRecommendations is intentionally single-threaded (concurrent writes to exec.Recommendations and totals would race). Add one-line comment documenting this contract on the aggregation function.

05-N3 — two stuck-execution sweep thresholds (15m approved vs 10m approved+running)

internal/purchase/manager.go:93 vs internal/purchase/reaper.go:39

staleApprovedThreshold (15m, manager) covers only "approved" status in the legacy recovery sweep; DefaultReapAfter (10m, reaper) covers "approved"+"running" with CAS. They overlap on "approved" with different thresholds. Document the intent, and consider consolidating both under DefaultReapAfter so one env var controls the behavior.

Fix

Implement all of the above as atomic commits on PR #1037 (fix/money-path-idempotency). Each fix includes a regression test that fails on pre-fix code.

Files touched

  • internal/execution/fanout.go + fanout_test.go
  • internal/scheduler/scheduler.go + scheduler_overrides.go + tests
  • internal/commitmentopts/service.go
  • internal/purchase/execution.go
  • internal/purchase/reaper.go

Metadata

Metadata

Assignees

No one assigned

    Labels

    effort/mDaysimpact/manyAffects most userspr-createdA PR has been opened for this issue (dedup guard for the auto-PR loop)pr-mergedThe PR for this issue has been mergedpriority/p1Next up; this sprintseverity/highSignificant harmtriagedItem has been triagedtype/bugDefecturgency/this-sprintWithin the current sprint

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions