Skip to content

fix(identityconfig): bound the installed identity-config snapshot across refreshes - #598

Open
sujanchalla0510 wants to merge 1 commit into
adcontextprotocol:mainfrom
sujanchalla0510:fix-570-snapshot-entry-cap
Open

sujanchalla0510 wants to merge 1 commit into
adcontextprotocol:mainfrom
sujanchalla0510:fix-570-snapshot-entry-cap

Conversation

@sujanchalla0510

Copy link
Copy Markdown
Collaborator

Fixes #570.

The per-response cap (scope3.DefaultMaxConfigEntries, 250k) bounds one config-source response, but applyDeltaWithLogger merges every delta into a copy of the current snapshot, so the installed snapshot is cumulative and unbounded over the process's life (measured in #570: 250k new keys per refresh, linear memory growth, RefreshOutcomeSuccess throughout).

What changed:

  • New Service option WithMaxSnapshotEntries(n): any initial load or delta refresh whose merged snapshot would exceed n entries is rejected and the prior snapshot is kept. A rejected delta reports RefreshOutcomeError through the refresh observer and leaves the watermark unadvanced, so a later under-cap delta can still apply; a rejected initial load fails Start under StartModeFailFast (retried under StartModeRetry). The rejection is loud by design: a silently frozen snapshot is its own failure mode, so this deliberately does not fall back to a quiet success.
  • The decision left open in Bound the installed identity-config snapshot across refreshes #570 is answered as a separate knob, not a changed default: zero or negative disables the cap (the default), preserving the previous unbounded behavior for deployments that have not opted in. This keeps the change non-breaking; operators who want the bound set it explicitly.
  • Wired through the identity agent: IdentityConfigSourceConfig.MaxSnapshotEntries, loaded from CONFIG_MAX_SNAPSHOT_ENTRIES, passed to the service in setup.

How it was tested:

  • go test -count=1 ./identityconfig/ ./identityagent/ (targeting module): all pass, including 3 new tests — over-cap delta rejected with prior snapshot kept and a later under-cap delta still applying, exactly-at-cap delta installing, over-cap initial load failing fail-fast with an error outcome and no snapshot installed.
  • go vet clean on both packages; touched files gofmt-clean (config.go has two pre-existing gofmt deviations elsewhere in the file, left untouched).

if err != nil {
errs = append(errs, err)
}
configMaxSnapshotEntries, err := lookupInt("CONFIG_MAX_SNAPSHOT_ENTRIES", 0)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

(non-blocking) Env var prefix diverges from its sibling cap. MaxSnapshotEntries lands in the same IdentityConfigSourceConfig struct as MaxConfigEntries (env CONFIG_SOURCE_MAX_CONFIG_ENTRIES, line 522), but reads from CONFIG_MAX_SNAPSHOT_ENTRIES — dropping the SOURCE_ segment. Operators setting the natural-by-analogy CONFIG_SOURCE_MAX_SNAPSHOT_ENTRIES get the default silently, i.e. the safety cap stays off. This is fresh wire; aligning to CONFIG_SOURCE_MAX_SNAPSHOT_ENTRIES now is free and avoids a permanent inconsistency.

@aao-secretariat aao-secretariat Bot 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.

Ladon verdict: Approve

Approve — clean additive change.

Adds an opt-in cumulative snapshot-entry cap (CONFIG_MAX_SNAPSHOT_ENTRIES) to identityconfig.Service, default-disabled so prior unbounded behavior is preserved. Cap logic in loadAllOnce/refreshDelta is fail-closed (no partial snapshot stored, watermark left unadvanced so a later under-cap delta applies), rejection is observable via RefreshOutcomeError + warn log, and all three branches are tested.

Checked against repo-specific gates: not a schema/types_gen/wire-field change (no regen or pathway-back obligation), no TMP signing/verification impact, no identity-agent TEE boundary widening, no protocol-managed skill edits, and no exported-symbol removal/rename requiring a breaking marker.

High-risk flag is set only because files match targeting/** and are all (modified) — no deletions and no medium-or-higher findings, so the modifications are presumed safe. gated_paths is false and the author is not on a no-auto-approve team.

Decision table: no critical/high/medium findings (only 1 low nit), no gated-path or deletion trigger — falls through to row 9.

Nit (non-blocking): targeting/identityagent/config.go:537 — the new env var drops the SOURCE_ prefix used by its sibling cap; worth aligning before it ships to avoid a silent footgun.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bound the installed identity-config snapshot across refreshes

1 participant