fix: reconcile state.destination on skip-without-write outcomes - #705
Merged
michael-richey merged 4 commits intoSep 23, 2026
Merged
Conversation
When create_resource/update_resource discovers (via _existing_resources_map) that a resource already exists on the destination and raises SkipResource without writing state.destination, the bucket view diverges from destination truth: no state file is persisted, so downstream consumers that trust the bucket count the id as failed even though the resource is confirmed present via the live API. Add a best-effort _reconcile_destination_if_absent helper invoked by the _create_resource/_update_resource wrappers on SkipResource: look up the resource's mapping key in _existing_resources_map and, if found, write the discovered destination resource into state.destination (insert-if-absent only). The original SkipResource re-raises so the handler's counter/metrics/emit accounting is unchanged. Insert-if-absent preserves delegate-then-update resources that already write state.destination before delegating to update_resource (users, teams, logs_metrics, logs_indexes, metric_tag_configurations, synthetics_global_variables, synthetics_tests) -- the framework is a pure no-op for them. Opt-out resources (skip_resource_mapping=True) have an empty _existing_resources_map, so the lookup never matches and nothing is written. Genuine non-existence skips (key absent from the map) write nothing. This fixes the class of bug at its source (the wrappers) with no per-resource edits, protecting future resources automatically. Covered by 11 new test files: framework contract + boundaries, plus per-resource regression guards, no-false-positive guards, and framework safety-net proofs.
michael-richey
commented
Sep 23, 2026
michael-richey
commented
Sep 23, 2026
michael-richey
left a comment
michael-richey
left a comment
Collaborator
Author
There was a problem hiding this comment.
Thorough pass complete on this PR.
What I checked:
- Framework behavior change in
datadog_sync/utils/base_resource.pyfor skip-path reconciliation. - New per-resource regression tests and boundary tests.
- Skip accounting path in
resources_handlerto confirm the originalSkipResourcestill re-raises and existing skipped metrics/counters remain intact.
Validation run:
tox -e py311 -- tests/unit/test_skip_reconcile_framework.py tests/unit/test_team_memberships_skip_reconcile.py tests/unit/test_roles_skip_reconcile.py tests/unit/test_security_monitoring_skip_reconcile.py tests/unit/test_users_skip_reconcile.py tests/unit/test_teams_skip_reconcile.py tests/unit/test_synthetics_global_variables_skip_reconcile.py tests/unit/test_synthetics_tests_skip_reconcile.py tests/unit/test_logs_indexes_skip_reconcile.py tests/unit/test_logs_metrics_skip_reconcile.py tests/unit/test_metric_tag_configurations_skip_reconcile.py
Result: all 47 tests passed.
I left one non-blocking inline suggestion on the reconcile check to use key is not None (instead of truthiness) so it stays fully consistent with map_existing_resources() behavior for empty-string keys.
map_existing_resources() keeps entries when key is not None, so an empty-string
key is mappable during discovery. The reconcile helper gated on truthiness
('if key and ...'), which would never reconcile an empty-string key that IS in
the map -- inconsistent with the discovery path. Switch to 'key is not None'
to match map_existing_resources() exactly.
Addresses review comment on PR #705.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Pre-action skips can still leave destination state unreconciled, preserving the false-negative behavior.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Adds framework-level reconciliation so skipped resources already present at the destination are persisted in destination state.
Changes:
- Reconciles destination state in create/update skip paths.
- Preserves existing state and skip accounting.
- Adds framework and resource-specific regression tests.
| File | Review |
|---|---|
tests/unit/test_users_skip_reconcile.py |
Covers user paths and import-skip boundaries. |
tests/unit/test_teams_skip_reconcile.py |
Covers team reconciliation behavior. |
tests/unit/test_team_memberships_skip_reconcile.py |
Covers membership skip reconciliation. |
tests/unit/test_synthetics_tests_skip_reconcile.py |
Covers synthetic test state handling. |
tests/unit/test_synthetics_global_variables_skip_reconcile.py |
Covers global-variable state handling. |
tests/unit/test_skip_reconcile_framework.py |
Tests framework contracts; contains an unused mock setup that should be removed. |
tests/unit/test_security_monitoring_skip_reconcile.py |
Covers security-rule skip cases and current boundaries. |
tests/unit/test_roles_skip_reconcile.py |
Covers role skip paths. |
tests/unit/test_metric_tag_configurations_skip_reconcile.py |
Covers metric configuration skip cases. |
tests/unit/test_logs_metrics_skip_reconcile.py |
Covers logs metric behavior. |
tests/unit/test_logs_indexes_skip_reconcile.py |
Covers logs index behavior. |
datadog_sync/utils/base_resource.py |
Adds reconciliation, but misses pre-action SkipResource paths; debug formatting also needs correction. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Address two review comments on PR #705: 1. Pre-hook SkipResource (e.g. security_monitoring_rules immutable/deprecated rules whose matching destination rule is in _existing_resources_map) was raised in _pre_resource_action_hook before the _create_resource/ _update_resource wrappers, so the framework never reconciled it -- the same bucket-view false negative. Wrap the pre-hook call in _apply_resource_cb with try/except SkipResource that calls _reconcile_destination_if_absent before re-raising. Insert-if-absent makes this idempotent with the wrapper reconcile. 2. The reconcile debug diagnostic passed positional %s args to logger.debug, but the NDJSON log backend (utils/log.py Log.debug) does not interpolate positional args in JSON mode, so %s placeholders would be emitted literally. Pre-format with an f-string so the message is readable in both modes. Tests: add pre-hook immutable/deprecated matching-map regression cases (RED without the handler wrap, GREEN with it) and a pre-formatted-log assertion. Full unit suite: 1363 passed.
The test_handler_accounting_unchanged_after_framework_reconcile case built up a bare MagicMock 'r_class' with stubs (resource_config, connect_resources, _pre_resource_action_hook, _send_action_metrics, _create_resource, _existing_resources_map) but never used it -- the test drives a real instance through _make_instance so the framework wrapper actually runs. Drop the dead block (flagged by Copilot review) and the now-unused AsyncMock import.
michael-richey
force-pushed
the
michael.richey/skip-reconcile-destination-state
branch
from
September 23, 2026 19:00
21cf7e6 to
4bfa586
Compare
riyazsh
approved these changes
Sep 23, 2026
michael-richey
deleted the
michael.richey/skip-reconcile-destination-state
branch
September 23, 2026 20:14
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
state.destination(loaded from the storage bucket at run start) is the view downstream consumers trust to know "what exists on the destination org." A second, live-API view —_existing_resources_map— is used by somecreate_resource/update_resourcemethods to skip a write when a resource already exists on the destination.When such a method raises
SkipResourcewithout writingstate.destination, the two views diverge: no destination state file is persisted, so a later bucket-presence check counts the id as failed even though the resource is confirmed present via the live API. The reported sync percentage is lower than the true sync state.Solution
In
base_resource.py, the_create_resource/_update_resourcewrappers now catchSkipResourceand, if the resource's mapping key is present in_existing_resources_map, write the discovered destination resource intostate.destination(insert-if-absent only) before re-raising. The originalSkipResourcere-raises, so handler accounting (counter, metrics, emit) is unchanged.One framework change, no per-resource edits — the bug is fixed generically and future resources are protected automatically.
users,teams,logs_metrics,logs_indexes,metric_tag_configurations,synthetics_global_variables,synthetics_tests) never raiseSkipResourcein the exists-path, so the framework is a pure no-op for them._existing_resources_map, so nothing is ever written.state.destinationentry.Tests
11 new test files (~48 cases): framework contract and boundaries, the primary
team_membershipsbug fix, per-resource regression guards, no-false-positive guards, and framework safety-net proofs for all 10 mapping resources. Full unit suite: 1360 passed.