Syncer: Reduce warning threshold for list resources. - #1126
Conversation
c00d2ec to
e792c69
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| func tooManyWarnings(warningCount uint64, completedActionsCount uint64, threshold float64) bool { | ||
| return warningCount > 10 && | ||
| completedActionsCount > 0 && | ||
| float64(warningCount)/float64(completedActionsCount) > 0.1 | ||
| float64(warningCount)/float64(completedActionsCount) > threshold | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: the warningCount > 10 floor stays hardcoded while the ratio became a parameter, so the 5% list-resource check actually needs 11+ list-resource warnings, not 5% of them. The call-site comment ("If > 5% of list resource actions ended in a warning, exit the sync") and the error message both omit the floor, which will read as a bug when an operator sees 8/20 list-resource warnings not abort. Either parameterize the floor alongside the threshold or state it in the comment.
| FinishAction(ctx context.Context, action *Action) | ||
| FinishActionWithWarning(ctx context.Context, action *Action) |
There was a problem hiding this comment.
🟡 Suggestion: State is exported and gains two methods here (FinishActionWithWarning, GetActionCount), which breaks any downstream implementation of the interface; pkg/sdk/version.go stays at v0.29.0. Only *state implements it in-repo and the interface already has 40+ methods, so external implementers are unlikely — but per the repo criteria this plus the tightened default abort behavior wants a minor bump or an explicit note that no compatibility signal is needed (confidence: medium).
General PR Review: Syncer: Reduce warning threshold for list resources.Blocking Issues: 0 | Suggestions: 6 | Threads Resolved: 0 Review SummaryScanned the full PR diff ( Security IssuesNone found. Correctness IssuesNone found. SuggestionsNew in this round:
Carried over and still open in the current code:
Prompt for AI agents |
e792c69 to
ab2b13c
Compare
| // If > 5% of list resource actions ended in a warning, exit the sync. | ||
| listResourceActionsCount := s.state.GetActionCount(SyncResourcesOp) | ||
| if tooManyWarnings(listResourceActionsCount.WarningCount, listResourceActionsCount.CompletedCount, 0.05) { | ||
| return warnings, fmt.Errorf( | ||
| "%w: warnings: %v list resource warning count: %d completed list resource actions: %d", | ||
| ErrTooManyWarnings, | ||
| warnings, | ||
| listResourceActionsCount.WarningCount, | ||
| listResourceActionsCount.CompletedCount, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🟠 Bug: this check reads durable counts, so once it trips the sync token can never make progress again. ErrTooManyWarnings is in the IsSyncPreservable allowlist (pkg/sync/syncer.go:98), so the artifact and token are preserved and the next run resumes from them. On resume, GetActionCount(SyncResourcesOp) returns the checkpointed WarningCount/CompletedCount unchanged, this check trips on the first loop iteration before any action runs, and the ratio can never improve because no new list-resource action ever completes — every resume returns immediately with zero progress. Consider gating the check on progress made in this process (e.g. require some list-resource action to have completed since resume), or classifying this exit as non-preservable so the next run starts a fresh sync. (confidence: high on the mechanism, medium on intent)
| ShouldSkipEntitlementsAndGrants bool `json:"should_skip_entitlements_and_grants,omitempty"` | ||
| ShouldSkipGrants bool `json:"should_skip_grants,omitempty"` | ||
| CompletedActionsCount uint64 `json:"completed_actions_count,omitempty"` | ||
| ActionCountsMap map[string]ActionCount `json:"action_counts,omitempty"` |
There was a problem hiding this comment.
🟡 Suggestion: action_counts is added to the token without a version bump (StateTokenVersion stays 1), so an older SDK parses the token, silently drops the unknown field, and its next Marshal writes the token back without it. In a mixed-version fleet that resets list-resource warning counts on any downgrade hop, which defeats the durability this change is adding. Worth an explicit note in the PR that downgrade loses the counts, or a test pinning the new-writer → old-reader → new-reader round trip. (confidence: high on the drop, medium on whether it matters in practice)
| require.False(t, tooManyWarnings(10, 1, 0.1), "requires more than ten warnings") | ||
| require.False(t, tooManyWarnings(11, 0, 0.1), "requires completed actions") | ||
| require.False(t, tooManyWarnings(11, 110, 0.1), "exactly ten percent is allowed") | ||
| require.True(t, tooManyWarnings(11, 109, 0.1), "more than ten percent must stop the sync") | ||
|
|
||
| require.False(t, tooManyWarnings(11, 220, 0.05), "exactly five percent is allowed") | ||
| require.True(t, tooManyWarnings(11, 219, 0.05), "more than five percent must stop the sync") | ||
| require.False(t, tooManyWarnings(11, 0, 0.05), "empty list-resource counts must not trip the five percent check") |
There was a problem hiding this comment.
🟡 Suggestion: these cases pin tooManyWarnings arithmetic, and TestResumedActionWarningCountsTripThreshold pins the state round trip, but nothing exercises the new exit through parallelSync itself. There is no test that a sync with >10 warned SyncResourcesOp actions out of >5% actually returns ErrTooManyWarnings from the loop, nor that the resumed run behaves as intended. A syncer-level test would cover the wiring and the resume behavior the unit test cannot reach.
4c96f6c to
7f3f98a
Compare
| // If > 5% of list resource actions ended in a warning, exit the sync. | ||
| listResourceActionsCount := s.state.GetActionCount(SyncResourcesOp) | ||
| if tooManyListResourceWarnings(listResourceActionsCount, s.listResourceActionsCompletedThisRun.Load()) { | ||
| return warnings, fmt.Errorf( | ||
| "%w: warnings: %v list resource warning count: %d completed list resource actions: %d", | ||
| ErrTooManyWarnings, | ||
| warnings, | ||
| listResourceActionsCount.WarningCount, | ||
| listResourceActionsCount.CompletedCount, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: the completedThisRun > 0 gate fixes the wedge, but it also makes this guard self-disabling. The check only runs at the outer loop top, and PeekMatchingActions drains up to maxPeekActionsCount (100) SyncResourcesOp actions per iteration, so a run trips only after a batch has completed. ErrTooManyWarnings is preservable, so the next run resumes; once no SyncResourcesOp actions remain, listResourceActionsCompletedThisRun stays 0 for the whole run and the durable ratio never gates anything — the sync completes and ingests the same deficient resource data. With N pending resource actions the net effect is ceil(N/100) failed attempts followed by a success, so whether the sync is actually stopped depends entirely on the host's retry budget rather than on this check. If the intent is to stop the sync, consider making the tripped state terminal (a durable "already tripped" marker plus a non-preservable error) instead of a per-run gate; if the intent is the one-attempt-fail-then-continue behavior, say so in the comment and add a syncer-level test that drives parallelSync through the trip and the resume after it. (confidence: high on the mechanism, medium on intent)
| // listResourceActionsCompletedThisRun is process-local: it is not in the | ||
| // sync token. tooManyListResourceWarnings uses it so a resumed token | ||
| // whose durable list-resource ratio already trips cannot abort before | ||
| // this run finishes a list-resource action. | ||
| listResourceActionsCompletedThisRun atomic.Uint64 |
There was a problem hiding this comment.
🟡 Suggestion: listResourceActionsCompletedThisRun is documented as "this run" but is never reset. Sync rebuilds s.state on every call (syncer.go:1026-1031), so a syncer is re-runnable in process — the comment at syncer.go:1011 even names "a reused syncer" as a supported case. On a second Sync the counter is already non-zero from the first, so a resumed token whose durable list-resource ratio already trips aborts at the first loop-top iteration with zero progress: exactly the wedge this field exists to prevent. Store 0 at the top of parallelSync (or in Sync alongside the s.state rebuild) so the value really means "this run". No in-repo caller calls Sync twice today, so this is latent rather than active. (confidence: high on the mechanism, medium on reachability)
| func tooManyWarnings(warningCount uint64, completedActionsCount uint64, threshold float64) bool { | ||
| return warningCount > 10 && | ||
| completedActionsCount > 0 && | ||
| float64(warningCount)/float64(completedActionsCount) > 0.1 | ||
| float64(warningCount)/float64(completedActionsCount) > threshold | ||
| } | ||
|
|
||
| // tooManyListResourceWarnings judges the checkpointed list-resource warning | ||
| // ratio, but only after this run has completed a list-resource action. | ||
| // ErrTooManyWarnings is preservable (IsSyncPreservable), so the next run | ||
| // resumes the token that tripped it. Judging the resumed counts before any new | ||
| // list-resource work completes would abort on the first loop iteration with | ||
| // zero progress, and no new completion could ever move the ratio. Gating on | ||
| // this run's completions means each resume drains at least one list-resource | ||
| // action, and once none remain the ratio stops gating the rest of the sync. | ||
| func tooManyListResourceWarnings(counts ActionCount, completedThisRun uint64) bool { | ||
| return completedThisRun > 0 && | ||
| tooManyWarnings(counts.WarningCount, counts.CompletedCount, 0.05) |
There was a problem hiding this comment.
🟡 Suggestion: tooManyWarnings now takes a threshold parameter, but both thresholds are hardcoded literals at the call sites (0.1, 0.05) with no SyncOpt to tune or disable them. This is a fleet-wide default-behavior change: every connector whose list-resource actions produce >10 warnings at >5% will start failing sync attempts with no opt-out, and pkg/sdk/version.go is still v0.29.0 in this PR, so downstreams get no version signal for it. Since the threshold is already a parameter, plumbing it through s.cfg (with 0 meaning "disabled") is nearly free and gives connectors with legitimately 404-heavy resource listings an escape hatch.
| FinishAction(ctx context.Context, action *Action) | ||
| FinishActionWithWarning(ctx context.Context, action *Action) |
There was a problem hiding this comment.
🟡 Suggestion: State is exported, and this adds two methods to it (FinishActionWithWarning here, GetActionCount at line 59), which breaks any downstream implementation that does not define them. Practical impact is low — s.state is only ever assigned newState() (syncer.go:1026) and there is no exported way to inject a custom State, so an external implementer would be inert anyway. Worth either adding the methods behind an optional interface that parallelSync type-asserts (the same shape as the existing if st, ok := s.state.(*state); ok fallback), or unexporting State if nothing outside the package is meant to implement it.
If > 5% of list resource actions result in a warning (such as 404 not found), exit the sync with an error.
7f3f98a to
c88a3c8
Compare
| return warnings, fmt.Errorf( | ||
| "%w: warnings: %v list resource warning count: %d completed list resource actions: %d", | ||
| ErrTooManyWarnings, | ||
| warnings, | ||
| listResourceActionsCount.WarningCount, | ||
| listResourceActionsCount.CompletedCount, | ||
| ) |
There was a problem hiding this comment.
🟡 Suggestion: this message mixes two different scopes. warnings is every warning this process saw across all ops, while WarningCount/CompletedCount are the checkpointed list-resource counts accumulated across runs. On a resume that trips, an operator can see warnings: [] next to list resource warning count: 11, and the printed slice may be mostly grant/entitlement warnings that had nothing to do with the trip. Consider dropping warnings here (or filtering to list-resource warnings) and adding the threshold so the message is self-explaining, e.g. ... ratio %.3f exceeds 0.05. (confidence: high)
| // run resumes the token that tripped it. Judging the resumed counts before any | ||
| // new list-resource work completes would abort on the first loop iteration | ||
| // with zero progress, and no new completion could ever move the ratio. The | ||
| // this-run floor means each resume drains at least eleven list-resource | ||
| // actions, and once none remain the ratio stops gating the rest of the sync. |
There was a problem hiding this comment.
🟡 Suggestion: the stated justification does not match what the floor counts. recordListResourceCompletedThisRun fires from finishActionWithWarning too, so completedThisRun counts warned finishes. A resume whose first 100-action batch warns on all 100 satisfies > 10 and trips — the ratio got no "chance to move," it only got worse. That is probably the behavior you want, but then the floor is "this run drained eleven list-resource actions," not "new listing work had a chance to move the ratio." Either reword, or count only non-warning completions if the intent is to require clean progress before re-arming. (confidence: high on the mechanism, low on intent)
state.go was 1,165 lines holding four unrelated things behind one
33-method exported State interface and one RWMutex: the resumable
action stack, five sync-level facts, best-effort run accounting, and
the grant-expansion entitlement graph. They have different lifetimes
and different writers, so one mutex over all of them said nothing
about which invariant it protected.
run_state.go the action stack: pushAction, finishAction,
nextPage, current, getAction, peekMatchingActions,
transitionAction, the I10 spawn-drain evidence
set, and the spawn-dedup guard. Own mutex.
sync_facts.go the five facts behind set(name)/has(name), keyed
by the token's JSON field names. Replaces ten
getter/setters. No mutex; it lives inside
runState, whose mutex guards it.
run_stats.go step durations, connector-call stats,
session-store stats, ingest quality, compaction
provenance. Own mutex.
expansion_graph.go the entitlement graph, with
get/peek/clear/clearTransientState. No mutex, as
before: callers mutate the graph get() returns
outside any lock, so a mutex here would not guard
it.
token.go serializedTokenV0/V1, the version constants,
unmarshalTokenV0, and marshalToken/unmarshalToken
in place of (*state).Marshal/Unmarshal. The JSON
structs and field order are byte-identical.
syncer's one state field becomes run, stats, and graph: 31 fields to
33. The exported State interface is deleted; it had one implementation,
one constructor, no mocks, and no caller outside pkg/sync. Typing the
fields concretely removes three type assertions that always succeeded
and with them transitionActionState's unreachable fallback.
Wire format unchanged. The golden fixtures and round-trip test landed
in 3659f92, before this work, and are untouched by it.
Locking: marshalToken takes runState's read lock and then runStats', so
a checkpoint is one consistent view of both. Nothing else takes two.
Two defects found by review, each with a regression test:
runIngestionInvariants bound s.run.undrainedSpawnedCursors
unconditionally. A bound method value on a nil *runState is itself
non-nil, so it sailed past checkSpawnedCursorDrain's nil test and
dereferenced the receiver at r.mu.RLock(). Leaving the predicate nil
is how the policy says "no scheduler evidence, skip I10", so the
bind is guarded. TestRunIngestionInvariantsI10EvidenceWiring pins
both directions, so the guard cannot become a way to switch I10 off.
The five fact names are used only as map keys, and the token's wire
names come from the struct tags, so the two spellings matched by
convention with nothing checking it: misspelling factNeedsExpansion
passed the whole suite including all 26 token fixtures. Inert today,
durable in CXE-1358, which stores facts under these names in the
page ledger's append-only fact keyspace.
TestFactNamesMatchTokenJSONNames reflects the constants against the
token struct tags; TestFactNamesAreDistinct catches two facts keyed
alike.
Docs: REVIEW_CHECKLIST.md gains the rule this split satisfies, and
says which holders carry a mutex rather than claiming all three do.
Four comments outside pkg/sync named state.Marshal/state.Unmarshal and
now name marshalToken/unmarshalToken. The dated round notes under
formal/reviews/ still say state.go; they record what a reviewer read
at the time, so they stay.
Rebased onto 1cc6156, which added the per-op action tally (#1126) to
the file this commit deletes. That feature is carried into the split
rather than re-derived: ActionCount and the actionCounts map go on
runState next to completedActions, since the tally is action-stack
accounting the token carries; serializedTokenV1 gains
action_counts in the same position with the same tag;
unmarshalTokenV0, loadRunState (with the nil-map guard),
seedInitAction and marshalToken each handle it where they handle
completedActions. FinishAction/FinishActionWithWarning become
finishAction/finishActionWithWarning over a finishActionLocked
helper, matching the branch's existing recordSpawnedAdmissionLocked
naming, and transitionAction's finish branch calls that helper
instead of repeating its body. GetActionCount becomes getActionCount.
After the carry, parallel_syncer.go differs from main by two lines,
both the holder rename.
The two tests #1126 added to state_test.go rode the rename into
token_test.go and are ported to the new API; they pass, as does the
golden token corpus with the new field in place.
Comments and registry prose that named the deleted methods now name
what runs: sideEffectAnnotationCoverage's I1/I2 entries and
ingestInvariantExclusions say setFact(factNeedsExpansion) and
setFact(factHasExternalResourceGrants), two parallel_syncer.go comments
say runState.transitionAction, Sync's doc comment names runState and
runStats instead of "the state object", and
TestSpawnedCursorDrainEvidence's comment and subtest name use
pushAction/finishAction. The registry values are free-form after the
leading "Ix:" that TestIngestInvariantVerdictTable cuts on, so the
meta-tests are unaffected.
CXE-1356
Co-authored-by: Cursor <cursoragent@cursor.com>
action_counts was the only V1 token field with no recorded bytes. #1126 added it without a fixture, and nothing went red: the golden corpus has no completeness guard over the token structs, so a missing field is invisible there. Its two Go tests marshal and unmarshal through the same code, which catches a dropped field but not a renamed one — renaming the tag on both sides leaves them green. Verified: with the tag changed to action_tallies, both pass and TestGoldenTokenRoundTrip fails. v1_action_counts.json is recorded from marshalToken per testdata/tokens/README.md, not hand-authored. It carries the tally beside a live list-resources action mid-pagination, which is the shape a resumed sync reads it in, and covers both warning_count states: set on one op, dropped by omitempty on the other. CXE-1356 Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
If > 5% of list resource actions result in a warning (such as 404 not found), exit the sync with an error.