Skip to content

fix(state): delete stale per-record cache files on dump_state - #651

Open
riyazsh wants to merge 1 commit into
mainfrom
riyaz.shiraguppi/resource-per-file-orphan-cleanup
Open

fix(state): delete stale per-record cache files on dump_state#651
riyazsh wants to merge 1 commit into
mainfrom
riyaz.shiraguppi/resource-per-file-orphan-cleanup

Conversation

@riyazsh

@riyazsh riyazsh commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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 into state.source on the next load_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 separate prune subcommand, which needs to be invoked manually.

Fix

Plumb the existing compute_stale_files + delete_stale_files primitives (previously reachable only via prune) into dump_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:

  • Authoritative-source gate: only prunes types marked via mark_source_authoritative. Same guard compute_stale_files itself enforces (raises ValueError there; here it's silent-skip because dump_state is also called from apply-only paths).
  • Layout gate: no-op under legacy monolithic layout (which is self-cleaning).
  • Subset-scope gate: import_resources_without_saving now skips marking authoritative for types scoped via id_payload (e.g. --id-file on monitors/authn_mappings/team_memberships). A partial-fetch state.source cannot drive pruning. Regression guard test included.
  • Exception isolation: compute_stale_files and delete_stale_files calls 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.
  • Kill switch: DD_SYNC_CLI_DISABLE_DUMP_PRUNE=1 disables the new behavior at runtime without a code revert. Emergency escape hatch for a destructive-cleanup regression discovered in prod; not for routine use.
  • Discriminated status log: sync-cli-timing phase=dump_state now emits prune_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:

  • Create path (baseline, unchanged).
  • Update path (baseline, unchanged).
  • Source-ID-drop cleanup — the new behavior.
  • Destination-ID-drop cleanup on same dump (source-ID keyed).
  • Silent-skip when type is not marked authoritative.
  • No-op under legacy monolithic layout.
  • Origin.SOURCE-only dump does not touch destination files.
  • Per-type authoritative scoping (only marked types are pruned).
  • Empty-in-memory + authoritative correctly prunes all files.
  • Idempotent no-op when there are no stale files.
  • --id-file subset does NOT trigger prune (C1 regression guard).
  • Backend delete_stale_files exception is swallowed with WARNING (H2 resilience).

Regression check: full unit suite (1079 pre-existing tests) still passes on the branch.

Residual risks

  • Under-prune (never over-prune) on id-file paths that happen to list all live IDs: the gate treats id-file scope as non-authoritative unconditionally. Stale files persist one extra cycle until a full-scope sync. Acceptable trade — under-prune is recoverable, over-prune is data loss.
  • Kill-switch env var is out-of-band vs Click convention: intentional for emergency use where argparse may not be reachable (subprocess wrappers).
  • No end-to-end test through import_resources_without_saving with --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.

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.
@riyazsh
riyazsh requested a review from a team as a code owner July 30, 2026 03:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant