Skip to content

fix(gcp): wire the recent-commitment duplicate checker into the platform #682

New issue
New issue
Open
Open
fix(gcp): wire the recent-commitment duplicate checker into the platform#682
Labels
effort/lWeeksimpact/fewLimited audiencepriority/p2Backlog-worthyseverity/mediumModerate harmtriagedItem has been triagedtype/bugDefecturgency/this-sprintWithin the current sprint

Description

@cristim
cristim
opened on Oct 8, 2026

Split out of #452 / PR #671. The platform pins cloud-commitments-go with the GCP recent-commitment dedupe and the ElastiCache engine dedupe, but never calls them.

Problem

computeengine.(*Client).FilterRecommendationsForRecentCommitments (library providers/gcp/services/computeengine/dedupe.go:49) is only reached through pkg/recfilter.(*DuplicateChecker).AdjustRecommendationsForExisting (pkg/recfilter/dedupe.go:43,58). git grep for DuplicateChecker, recfilter, AdjustRecommendationsForExisting, FilterRecommendationsForRecentCommitments and GetExistingCommitments finds no non-test call site in the platform (checked at main 72843b5). So recent matching GCP CUDs do not suppress repeat purchases, and the ElastiCache engine dedupe is also unreachable. The AWS reservation-state guard is different: it runs inside each service's PurchaseCommitment and is already active.

Open design question

GetExistingCommitments on the current GCP pin returns an error for the whole list when any commitment has no name, no parseable start timestamp, an unknown type or no VCPU resource (convertGCPCommitmentToCommon / commitmentVCPUCount). Once wired, one odd commitment in a project would affect every recommendation for it. Decide: fail closed (block purchases for that account/region until fixed) or skip (log, treat as no existing commitments, accepting the duplicate-purchase risk).

To decide / do

  1. Where it runs: recommendation collection, or the purchase path (execution), or both.
  2. Per-account GCP credentials and the extra Compute API calls per collection.
  3. The fail-closed vs skip decision above.
  4. A test through the real path: a recent matching CUD suppresses a repeat purchase (fails without the wiring).

Plan plus two review passes before code; this is a money path.

Activity

  1. cristim
    added
    priority/p2Backlog-worthy
    severity/mediumModerate harm
    urgency/this-sprintWithin the current sprint
    effort/lWeeks
    triagedItem has been triaged
    impact/fewLimited audience
    type/bugDefect
    on Oct 8, 2026

  2. IAmJSD commented on Oct 20, 2026

    @IAmJSD
    IAmJSD
    on Oct 20, 2026
    Collaborator

    Claiming: working on this

  3. IAmJSD
    self-assigned this
    on Oct 20, 2026

  4. IAmJSD commented on Oct 20, 2026

    @IAmJSD
    IAmJSD
    on Oct 20, 2026
    Collaborator

    Released: the issue asks for a plan and two review passes before any code (money path), and it has open owner decisions: where the check runs (collection, execution or both), fail-closed vs skip when GetExistingCommitments errors on one odd commitment, and the per-account API cost. Happy to write the plan as a comment for review if that would help. @cristim

  5. IAmJSD
    removed their assignment
    on Oct 20, 2026

  6. cristim commented on Nov 4, 2026

    @cristim
    cristim
    on Nov 4, 2026
    MemberAuthor

    Claiming: claude-cc-platform-2 (fleet), next unclaimed p2 this-sprint by priority.

    The open design question is settled by the owner decision recorded on #452 (2026-11-03, option A): the check runs at purchase execution and fails closed when GetExistingCommitments errors. This is a money path, so a written plan with two adversarial review passes comes before any code. I'll post the plan here.

  7. cristim commented on Nov 5, 2026

    @cristim
    cristim
    on Nov 5, 2026
    MemberAuthor

    Plan (reviewed): GCP recent-commitment check at purchase execution

    The plan had two adversarial review passes plus closure checks by the same cold reviewer, and is now closed. It follows the owner decision recorded on #452: check at execution, and fail closed when listing errors.

    G1 (cloud-commitments-go, lands first). GetExistingCommitments for GCP lists only live commitments, using the filter (status != EXPIRED) (status != CANCELLED).

    • Why: today the list includes expired and cancelled CUDs, and the converter caps it at 50 items. A region with a long history would always error, and fail closed would then block it permanently.
    • The test asserts the exact filter string. Real-API filtering is marked unverified until the first live run.
    • Before the change, every consumer of the method is checked for one that expects expired CUDs.

    P0 (platform). Today a run in which every rec was refused before anything was sent still ends as an uncertain failure. Under P0 it is recorded as no submission, so the ramp doesn't count it as holding capacity and a ladder tranche isn't stranded.

    • Single-account runs: the error passes through the existing terminal save.
    • Multi-account runs: the per-account row is flagged before it is saved.
    • Runs with a purchased or uncertain rec are unchanged.
    • Existing before-submission refusals are unaffected, because they stop the whole execution before any rec runs.

    P1 (platform). The check runs right before each GCP compute purchase.

    • It lists existing commitments through a wrapper that forwards the GCP filter explicitly.
    • Commitments named after this execution's own idempotency tokens are dropped, so a lost-response retry and two same-family recs in one execution aren't refused by their own purchases. The name rule mirrors the library's, with a parity test against the real client.
    • The project comes from the listed commitments, which also works with ambient credentials.
    • A refusal records a fixed note: either the list could not be read, or a commitment for that project, region and machine family started in the last 24h.
    • A Retry reuses the same lineage, so inside 24h it is refused again.

    Known limits.

    • A region with more than 50 live CUDs still trips the library cap, and fail closed blocks it.
    • Two different purchases that both list before either buys can still race, as today.
    • On multi-account runs, a refused account's suppressions stay kept, the conservative side.

    The ElastiCache engine dedupe mentioned above is out of scope here; it is a separate follow-up.

  8. cristim
    added a commit that references this issue on Nov 5, 2026

    fix(gcp): build the live-commitment filter from the SDK enum and allo…

    e8879d3

  9. cristim commented on Nov 5, 2026

    @cristim
    cristim
    on Nov 5, 2026
    MemberAuthor

    Claimed by claude-cc-platform-2. The go slice G1 is LeanerCloud/cloud-commitments-go#685, in delta review. The platform slices P0 and P1 come next, after #685 merges and the pin bump.

  10. cristim
    added 2 commits that reference this issue on Nov 5, 2026

    fix(gcp): list only live commitments so the item cap counts live CUDs (…

    cd0bd04

    chore(deps): bump cloud-commitments-go to 790f369f and forward the RI…

    04f3515

  11. cristim commented on Nov 7, 2026

    @cristim
    cristim
    on Nov 7, 2026
    MemberAuthor

    P0 is #1842. Scope note from its review: on a multi-account (fan-out) run, only a refused account's own row gets NoSubmission. The root row keeps today's outcome, so the ramp, suppression and ladder effects of P0 apply to single-account runs only. Carrying the proof to an all-refused fan-out root is #1844 (p3, safe direction). P1, the GCP recent-commitment check, follows after #1842 merges.

  12. cristim
    added a commit that references this issue on Nov 7, 2026

    test(purchase): pin that a panic after submission is not counted as r…

    67ba0c6

  13. cristim commented on Nov 7, 2026

    @cristim
    cristim
    on Nov 7, 2026
    MemberAuthor

    P1, the GCP recent-commitment check at purchase execution, is in progress by claude-cc-platform-2. It's held as local commits stacked on #1842 (P0) while platform is at its PR cap, and the PR opens after #1842 merges and a slot frees.

  14. cristim
    added 4 commits that reference this issue on Nov 7, 2026

    test(purchase): update the Azure Redis routing test for the four-valu…

    eb72026

    fix(purchase): record no_submission when every failed rec was refused…

    6fd9a2e

    test(purchase): pin that recent commitments from two projects fail cl…

    a96345d

    test(purchase): pin that another lineage's recent CUD is refused, not…

    903695f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    effort/lWeeksimpact/fewLimited audiencepriority/p2Backlog-worthyseverity/mediumModerate 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