fix(state): delete stale per-record cache files on dump_state - #651
Open
riyazsh wants to merge 1 commit into
Open
fix(state): delete stale per-record cache files on dump_state#651riyazsh wants to merge 1 commit into
riyazsh wants to merge 1 commit into
Conversation
Under --resource-per-file, put() only writes files for currently-live IDs, so cache files for dropped source IDs survived on disk and got re-read into state.source on the next load — resurrecting deleted records and re-pushing them to destination each sync. Legacy monolithic layout did not have this problem incidentally because put() rewrote a single per-type file each cycle. Plumb existing compute_stale_files + delete_stale_files into dump_state so per-record layout gets the same self-cleaning behavior. Same gate compute_stale_files itself enforces (_authoritative_source_types), plus: skip id-file-scoped types (subset scope != authoritative), no-op under legacy layout, kill-switch env var, swallow backend delete errors.
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.
What
Under
--resource-per-file,storage.put()only writes files for currently-live IDs. It does not enumerate the cache directory and delete files for IDs that dropped since the last dump. Stale files survive on disk and get re-read intostate.sourceon the nextload_state(), resurrecting records that no longer exist on source and re-applying them to destination on every subsequent sync.Legacy monolithic layout did not have this problem incidentally —
put()rewrote a single per-type file each cycle, so stale entries were replaced. Per-record layout lost that side effect and no explicit cleanup step replaced it. Until now, the only way to reconcile was the separateprunesubcommand, which needs to be invoked manually.Fix
Plumb the existing
compute_stale_files+delete_stale_filesprimitives (previously reachable only viaprune) intodump_state. Per-record layout now self-cleans on the same lifecycle as legacy.Destination-side files are keyed by source ID (see
state.py:275-277), so the same authoritative-source guard cleans both sides on the same dump — a source ID dropped after a successful destination API DELETE + in-memory pop is removed from both source and destination on the same cycle.Safety gates
All preserving
prune's own discipline:mark_source_authoritative. Same guardcompute_stale_filesitself enforces (raises ValueError there; here it's silent-skip becausedump_stateis also called from apply-only paths).import_resources_without_savingnow skips marking authoritative for types scoped viaid_payload(e.g.--id-fileonmonitors/authn_mappings/team_memberships). A partial-fetch state.source cannot drive pruning. Regression guard test included.compute_stale_filesanddelete_stale_filescalls are try-wrapped inside_prune_stale_files_after_put. Backend transients log at WARNING; stale files remain and retry on the next dump. The put() itself has already succeeded, so callers' happy path is never aborted by a prune-side failure.DD_SYNC_CLI_DISABLE_DUMP_PRUNE=1disables the new behavior at runtime without a code revert. Emergency escape hatch for a destructive-cleanup regression discovered in prod; not for routine use.sync-cli-timing phase=dump_statenow emitsprune_status={ok,gated_kill_switch,gated_legacy_layout,gated_no_authoritative,no_stale,compute_error,delete_error}so operators can distinguish zero-because-clean from zero-because-gated.Tests
New file
tests/unit/test_state_dump_cleanup.py, 12 cases:Origin.SOURCE-only dump does not touch destination files.--id-filesubset does NOT trigger prune (C1 regression guard).delete_stale_filesexception is swallowed with WARNING (H2 resilience).Regression check: full unit suite (1079 pre-existing tests) still passes on the branch.
Residual risks
import_resources_without_savingwith--id-file: the C1 gate is verified at the State layer, not through the caller. The caller fix is a 5-line filter with a debug log; end-to-end coverage is a reasonable follow-up.Same shape as
PR #650 (
fix(spans_metrics): fall back to update on 409 during create). Both extend an existing primitive to close a per-file-mode gap left by an earlier performance-driven layout change.