cli: address Bugbot/Copilot review on the cell fan-out layer

299b273→main· Soph·1w ago·4 files·+169 added/-29 removed

• Bound resolveCellBaseURLs's catalog lookup with cellResolveTimeout, like resolveRepoCellTarget: a hung core must not stall the command before the fan-out even starts.
• Refuse a concrete baseURL without a jurisdiction: minting a home-jurisdiction token and dialing a foreign cell with it is exactly the mismatch resolveRepoCellTarget refuses; such a group stays on home routing.
• Key groupReposByCell on (cell, jurisdiction): blank-cell index rows in different jurisdictions no longer collapse into one group routed by whichever repo came first.
• Single-flight token mints per jurisdiction (mintSlot) instead of one factory-wide mutex held across the exchange: a slow region's mint no longer burns other cells' per-cell deadlines. Failed mints cache nothing, so the next caller retries.

Co-Authored-By: Claude Fable 5 noreply@anthropic.com

Sessions

ab23b05bf985View transcript

Changes

4

• cmd/entire/cli

  • auth

    • Mcell_data_api.go+29/-11
    • Mcell_data_api_test.go+68

  • Mcell_fanout.go+37/-11

  • Mcell_fanout_test.go+35/-7

type CellClientFactory struct {
    subject cellSubject

mu     sync.Mutex
    tokens map[string]string // jurisdiction -> minted identity token
    mu    sync.Mutex           // guards slots (map access only, never held across I/O)
    slots map[string]*mintSlot // jurisdiction -> its token slot
}

// mintSlot single-flights one jurisdiction's token: the slot mutex is held
// across the mint, so concurrent callers for the same jurisdiction wait for one
// exchange instead of duplicating it, while other jurisdictions mint in
// parallel on their own slots. A failed mint caches nothing — the next caller
// retries.
type mintSlot struct {
    mu    sync.Mutex
    token string
}

// NewEntireAPICellClientFactory resolves the exchange subject (active stored
if err != nil {
    return nil, err
}
return &CellClientFactory{subject: subject, tokens: make(map[string]string)}, nil
return &CellClientFactory{subject: subject, slots: make(map[string]*mintSlot)}, nil
}

// ClientFor returns an authenticated client for the given cell target (nil
}

// tokenFor returns the cached identity token for jurisdiction, minting it on
// first use. The mutex is held across the mint: concurrent callers for the
// same jurisdiction wait for one exchange instead of duplicating it, at the
// cost of serializing cross-jurisdiction mints (fine for the handful of
// jurisdictions a fan-out touches).
// first use. Locking is per jurisdiction (see mintSlot): a slow exchange for
// one jurisdiction never blocks another jurisdiction's mint — in a fan-out,
// healthy cells must not burn their per-cell deadline waiting on an unrelated
// region's exchange.
func (f *CellClientFactory) tokenFor(ctx context.Context, jurisdiction, coreURL string) (string, error) {
    f.mu.Lock()
    defer f.mu.Unlock()
    if token, ok := f.tokens[jurisdiction]; ok {
        return token, nil
    }
    slot, ok := f.slots[jurisdiction]
    if !ok {
        slot = &mintSlot{}
        f.slots[jurisdiction] = slot
    }
    f.mu.Unlock()

slot.mu.Lock()
    defer slot.mu.Unlock()
    if slot.token != "" {
        return slot.token, nil
    }
    audience := jurisdictionAudience(jurisdiction, f.subject.dataOrigin, f.subject.discoveredCore)
token, err := exchangeJurisdictionToken(ctx, coreURL, f.subject.loginJWT, audience, f.subject.httpClient)
if err != nil {
    return "", fmt.Errorf("exchange jurisdictional identity token: %w", err)
}
f.tokens[jurisdiction] = token
slot.token = token
return token, nil
}

// TestResolveCellBaseURLs_RefusesBaseURLWithoutJurisdiction pins the guard: a
// concrete baseURL is only usable together with the jurisdiction its token
// must be minted for; a catalog row with no jurisdiction leaves the group on
// home routing instead of dialing a foreign cell with a home token.
func TestResolveCellBaseURLs_RefusesBaseURLWithoutJurisdiction(t *testing.T) {
    t.Parallel()
    cells := []cellGroup{{cell: "aws-eu-west-1", clusterSlug: "eu-prod"}} // no jurisdiction anywhere
    fake := &fakeCellCore{clusters: []coreapi.Cluster{
        {Slug: "eu-prod", ApiUrl: coreapi.NewOptString(euCellAPIURL)}, // row has no jurisdiction either
    }}
    resolveCellBaseURLs(context.Background(), fake, cells)
    if cells[0].baseURL != "" || cells[0].jurisdiction != "" {
        t.Fatalf("group = %+v, want untouched (home routing)", cells[0])
    }
}