Add customer_deletion_jobs table and APIs (issue #821)

[octoaide]

Summary

This PR implements persistent storage and APIs for customer-owned
data deletion jobs, addressing issue #821.

Closes #821

What changed

• Add new column family CUSTOMER_DELETION_JOBS and include it in
  MAP_NAMES.
• Add public types: CustomerDataDeletionJob,
  CustomerDataDeletionServiceResult, CustomerDataDeletionService, and
  CustomerDataDeletionStatus (exported from lib.rs).
• Implement Table::open() and the
  transactional APIs: get, add_service, update_service. These follow
  the existing account table optimistic-transaction retry pattern to
  avoid lost concurrent updates.
• Add Store::customer_data_deletion_map() accessor and internal
  StateDb accessor.
• Enforce service identification rules:
  • Review is identified by service only and uses host_fqdn: None.
  • Sensor and SemiSupervised are identified by service + exact
    host_fqdn (must be non-empty).
• Serialize service_results directly using big-endian customer_id
  keys.
• Add tests covering insert, put, get, iter, add/update semantics,
  service identification rules, concurrency retries, and DB reopen
  persistence.
• Update CHANGELOG.md.

Files changed (high level)

• src/tables/customer_data_deletion_job.rs (new table + logic)
• src/lib.rs (exports)
• src/tables.rs (map name added)
• src/types.rs (public types)
• CHANGELOG.md

Why

REview must store deletion results from itself and connected
services (Sensor, SemiSupervised). This change provides a durable
store and safe concurrent update semantics so multiple services can
report results without overwriting each other.

Notes

• The implementation uses the existing Table abstraction and the
  optimistic transaction pattern; no new DB abstraction was added.
• Tests and formatting/linting were updated; test suite passes
  locally.

[codecov]

Codecov Report

❌ Patch coverage is 98.63589% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.15%. Comparing base (7c99229) to head (112fd0e).

Files with missing lines	Patch %	Lines
src/tables/customer_data_deletion_job.rs	98.55%	7 Missing ⚠️
src/migration.rs	98.68%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #822      +/-   ##
==========================================
+ Coverage   82.70%   83.15%   +0.45%     
==========================================
  Files          88       89       +1     
  Lines       33083    33993     +910     
==========================================
+ Hits        27361    28268     +907     
- Misses       5722     5725       +3     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[kimhanbeom]

@octoaide
Adding the new customer deletion jobs CF may cause migrations from existing databases to fail because migration functions open the database using the latest MAP_NAMES.

The 0.42 → 0.43 path creates missing CFs through migrate_customer_specific_networks(). However, the 0.43 → 0.44, 0.44 → 0.45, and 0.45 → 0.46 paths use create_missing_column_families(false), so they cannot open an existing database that does not contain the new CF.

Please make the following changes:

1. Use create_missing_column_families(true) in the following migration functions that open the database with the latest MAP_NAMES:
   • migrate_network_tags_to_customer_scoped()
   • migrate_network_cf()
   • migrate_event_fields()
   • migrate_triage_policy_confidence()
   • migrate_event_country_codes()
2. Keep create_if_missing(false) so migration still fails if the database itself does not exist.
3. Add tests that create existing 0.43, 0.44, and 0.45 CF schemas without the new CF and verify that migration succeeds, creates the new CF, and updates the VERSION.

[octoaide]

I analyzed the review feedback and made the requested fixes. Summary of what I changed and why:

• Renamed the new column family constant to use spaces: pub(super) const CUSTOMER_DELETION_JOBS: &str = "customer deletion jobs"; — this matches the existing multi-word CF naming convention.
• Updated the second Sensor test hostname to sensor-2.example so the test clearly verifies adding a different host FQDN.
• Enabled creation of missing column families when opening an existing DB in the five migration functions you flagged:
  • migrate_network_tags_to_customer_scoped()
  • migrate_network_cf()
  • migrate_event_fields()
  • migrate_triage_policy_confidence()
  • migrate_event_country_codes()
    I left create_if_missing(false) in place so migrations still fail if the database itself is missing.
• Added migration tests that start from existing 0.43, 0.44, and 0.45 CF schemas (without the new CF), run the migration, and verify that the new CF is created and VERSION is updated.

I ran validation locally and pushed the changes:

• cargo test --all-features → 508 passed, 9 ignored
• cargo clippy --bins --tests --all-features -- -D warnings
• cargo fmt -- --check --config group_imports=StdExternalCrate
• git diff --check

These changes address the migration failure scenarios you described and follow the naming/host conventions you requested. Thanks for the thorough review — everything you pointed out has been implemented and pushed.

[octoaide]

I rebased this PR onto the latest main branch and automatically resolved merge conflicts in CHANGELOG.md by keeping entries from both branches.

[sophie-cluml]

I think adding a column family should be treated as a database format change, even though it is not a Rust API breaking change. Once the new binary creates the new CF, older binaries that do not list that CF will fail to open the DB, so this becomes a breaking change in terms of DB schema compatibility.

Could we model this as an explicit DB-format migration instead of relying on create_missing_column_families(true) in normal open/migration paths? In particular, it would be safer to keep migration DB opens strict with create_missing_column_families(false), use version-specific CF name lists for the old and new formats, and create only the new customer deletion jobs CF in the migration step that advances the DB format version.

[sophie-cluml]

This #[allow(clippy::missing_panics_doc)] does not seem necessary. I removed it locally and cargo clippy --bins --tests --all-features -- -D warnings still passes.

Could we remove this allow? If a panic is part of this public API's contract, it should be documented with a # Panics section instead of suppressing the lint. Otherwise, I think we should avoid adding the lint suppression.

[sophie-cluml]

Could we make this expect message explain the invariant that makes the panic impossible, rather than restating the operation being attempted?

Our panic policy allows expect only when we are certain it cannot panic. If this is relying on bincode serialization into an in-memory Vec<u8> being infallible for these derived Serialize types, it would be helpful to say that directly in the message. Otherwise, this should probably return an error through an API that can propagate serialization failure.

[sophie-cluml]

Could we make this expect message explain why the table must be present? It also uses "{CUSTOMER_DELETION_JOBS}" as a literal string, so the actual CF name is not shown; expect(...) takes a plain &str, so string interpolation does not happen here.

Under our panic policy, expect should document the invariant that prevents the panic. Please either make the accessor return a Result, or update the message to explain the invariant, e.g. that StateDb is opened with the full CF list and this accessor is only used after a successful StateDb::open.

[octoaide]

Thanks for the detailed reviews — I went through each point and made the changes.

Summary of what I changed

• Treated adding the new column family as a DB-format migration. I added an explicit 0.46→0.47 migration that:
  • Uses strict, version-specific CF name lists (opens use create_missing_column_families = false during migration),
  • Creates only the new customer-deletion-jobs CF as part of the migration step that bumps the DB format.
    This avoids relying on create_missing_column_families(true) in normal open paths and keeps schema compatibility explicit.
• Bumped the package / DB format to 0.47.0 and updated CHANGELOG.md accordingly.
• Removed the unnecessary #[allow(clippy::missing_panics_doc)].
• Rewrote the two expect messages so they document the invariant that makes a panic impossible (instead of restating the operation). I also fixed the customer-deletion-jobs accessor expect to show the actual CF name and explain why the CF must be present (StateDb was opened with the full CF list / caller only uses this accessor after a successful open).
• Added migration coverage tests that verify an old CF list cannot open the new format and that the migration creates only the new CF.

Validation

• cargo fmt --check
• cargo clippy --bins --tests --all-features -- -D warnings
• cargo test --all-features — 515 passed, 9 ignored
• markdownlint-cli2 CHANGELOG.md
• git diff --check

All changes have been committed and pushed. Thanks again for catching these — the migration is now explicit, the lint suppression is removed, and the expect messages document the invariants as requested.

[sophie-cluml]

Could we use 0.47.0-alpha.1 for this PR instead of 0.47.0, and update the related migration metadata consistently as well?

This PR introduces a DB-format change before the stable 0.47.0 release. Following the migration-versioning policy documented in src/migration.rs, DB-format changes during the pre-release development window should be represented with an alpha.x version so that later schema changes before the stable release remain distinguishable.

In particular, please update Cargo.toml, COMPATIBLE_VERSION_REQ, the migration target version in migrate_data_dir, and the changelog DB-format references consistently to 0.47.0-alpha.1.

[sophie-cluml]

Could you clarify why map_names_for_existing_format is needed? Is it intended to support retrying after a partial 0.47 migration, where the new CF may already exist before VERSION is updated? Or, if that retry case is not the reason, shouldn't each migration functions use the version-specific CF list directly instead?

[octoaide]

Thanks — I addressed both points from your review.

• I updated the DB/package version to 0.47.0-alpha.1 everywhere it needs to be: Cargo.toml, COMPATIBLE_VERSION_REQ, the migration target in migrate_data_dir, and the DB-format references in CHANGELOG.md.
• I clarified and documented why map_names_for_existing_format exists. It's there to make migrations robust to a retry after a partial 0.47 migration: if the new column family(s) were already created but VERSION wasn't updated (an interrupted run), we need to map the expected CF names to what actually exists so the migration can proceed instead of failing. This avoids duplicating that detection logic in every migration function; migrations still use the version-specific CF lists, but the helper makes them tolerant of the interrupted-run case.
• I added a regression test that simulates an interrupted migration and verifies the retry behavior.
• I ran the usual validation locally and everything passed:
  • cargo test --all-features — 516 passed
  • cargo clippy --bins --tests --all-features -- -D warnings
  • rustfmt check
  • markdownlint-cli2 CHANGELOG.md
  • git diff --check

All changes have been committed and pushed. Thanks for the pointer on using an alpha version and for asking about the retry case — both are now handled and documented.

[sophie-cluml]

Could you cross-check the migration changes, @kimhanbeom? The migration code changed during the review process.

[kimhanbeom]

Could you move the creation of the triage exclusion reason column family into a dedicated migration step?

Adding this CF is a distinct 0.42 → 0.43 database-format change, based on the same rationale used for the explicit customer deletion jobs CF migration. Creating it inside migrate_rename_tidb_to_label_db() hides this intent and makes the Label DB rename responsible for an unrelated schema change.

Please make the following changes:

• Remove the TRIAGE_EXCLUSION_REASON CF creation from migrate_rename_tidb_to_label_db().
• Add map_names_v0_43_without_triage_exclusion_reason(), mirroring map_names_v0_42_without_account_policy(). It should represent the post-rename intermediate schema by returning MAP_NAMES_V0_43_TO_V0_46 without TRIAGE_EXCLUSION_REASON.
• Add migrate_create_triage_exclusion_reason_cf(). It should:
  • set both create_if_missing(false) and create_missing_column_families(false);
  • strictly open the database using the post-rename intermediate CF list; and
  • create only the TRIAGE_EXCLUSION_REASON CF using the existing constant rather than a duplicated string literal.
• Update the 0.42 → 0.43 migration order as follows:

    // Step 1: Drop "account policy" column family if it exists (from 0.42)
    migrate_drop_account_policy(&db_path)?;
  
    // Step 2: Rename "TI database" to "label database"
    migrate_rename_tidb_to_label_db(&db_path)?;
  
    // Stpe 3: Create the "triage exclusion reason" column family
    migrate_create_triage_exclusion_reason_cf(&db_path)?;
  
    // Step 4: Migrate AllowNetwork and BlockNetwork to customer-specific format
    migrate_customer_specific_networks(&db_path)?;
  

migrate_customer_specific_networks() should remain behaviorally unchanged because the preceding step completes the MAP_NAMES_V0_43_TO_V0_46 CF schema before it opens the database.

Please also update the relevant migration tests to verify that the new CF is created by the dedicated migration step and that the resulting database opens strictly with the complete 0.43 CF list.

[octoaide]

I reviewed the feedback and made the requested migration refactor. I’ve committed and pushed the changes.

What I changed

• Removed TRIAGE_EXCLUSION_REASON CF creation from migrate_rename_tidb_to_label_db().
• Added a post-rename intermediate CF helper (map_names_v0_43_without_triage_exclusion_reason()) that represents the schema immediately after the TI→Label rename but before the triage CF is added.
• Added migrate_create_triage_exclusion_reason_cf():
  • opens the DB strictly with the post-rename CF list (create_if_missing(false), create_missing_column_families(false));
  • creates only the TRIAGE_EXCLUSION_REASON CF using the existing constant (no duplicated string literal).
• Reordered the 0.42 → 0.43 migration steps to:
  1. drop account policy CF if present,
  2. rename TiDB → Label DB,
  3. create TRIAGE_EXCLUSION_REASON via the new dedicated migration,
  4. run migrate_customer_specific_networks().
• Left migrate_customer_specific_networks() behaviorally unchanged (it now runs after the triage CF is present).
• Updated migration tests to assert the dedicated step creates the new CF and that the DB opens strictly with the complete 0.43 CF list.

Validation

• Focused migration test and end-to-end 0.42→0.43 migration test updated and passing.
• Ran cargo fmt --check, cargo clippy (bins+tests, all-features) with -D warnings, and git diff --check.

This implements the separation you requested so the label-DB rename no longer hides an unrelated schema change. Thanks for the clear guidance — everything should now match the intended migration boundaries.

[sophie-cluml]

Thanks to David's previous migration review comment, I took a closer look here and found a related gap: the 0.42 → 0.43 chain isn't idempotent across partial-retry states the way the later steps now are. Could you make the 0.42 -> 0.43 migration chain idempotent across partial retry states and add focused regression tests for those states?

Because VERSION is updated only after the full migration chain reaches the current compatible version, a process can stop after one of the 0.42 -> 0.43 sub-steps, or even after later migrations create the 0.47 customer-deletion-jobs CF, while the recorded version still remains 0.42.x. On retry, migrate_0_42_to_0_43() will run again.

Please cover at least these physical states with VERSION = 0.42.0:

• account policy already dropped, TI database still present.
• Both TI database and label database present, to simulate interruption during the rename/copy step.
• label database present and TI database absent, but triage exclusion reason not yet created.
• triage exclusion reason already created.
• AllowNetwork/BlockNetwork already migrated to the customer-specific format.
• The DB has already reached the current CF set, including customer deletion jobs, but VERSION is still 0.42.0.

For each state, rerunning migrate_data_dir() should succeed, skip already-completed sub-steps safely, preserve existing data, and update both VERSION files to the current package version.

[octoaide]

Thanks — I dug into the gap you pointed out and made the 0.42 → 0.43 chain idempotent across partial-retry states.

What I changed

• Made column-family discovery/creation/removal retry-safe so repeated runs safely skip already-applied CF operations instead of erroring.
• Rewrote the TI-db → label-db copy to be resumable: it copies entries incrementally and skips conflicting/existing label records rather than overwriting them, so a partial copy can be resumed safely.
• Added explicit detection to skip creating the triage-exclusion-reason and to recognize AllowNetwork/BlockNetwork records that have already been migrated to the customer-specific format.
• Ensured later CFs (including the customer-deletion-jobs CF) are detected as present so the migration chain won’t try to re-create or regress them on retry.
• Updated the migration to write the package VERSION only after the entire chain is complete, and ensure retry runs update both VERSION files when finishing.

Tests added

• I added focused regression tests covering the six physical states you requested with VERSION = 0.42.0:
  • account policy dropped, TI DB still present
  • TI DB + label DB present (interrupted during rename/copy)
  • label DB present, TI DB absent, triage-exclusion-reason not yet created
  • triage-exclusion-reason already created
  • AllowNetwork/BlockNetwork already migrated to customer-specific format
  • DB already has the current CF set (including customer deletion jobs) but VERSION still 0.42.0
• Each test reruns migrate_data_dir(), asserts the run succeeds, verifies already-completed sub-steps are skipped, checks existing data is preserved, and confirms both VERSION files are updated to the current package version.

Validation

• All tests pass: cargo test --all-features → 522 passed, 9 ignored
• linting/format checks passed: cargo clippy and cargo fmt verified, git diff --check clean

Where

• Changes are in src/migration.rs and the new regression tests are included in the test suite. I’ve committed and pushed the changes.

Thanks for catching this — the migration is now safe to re-run from the partial states you outlined. Let me know if you want me to walk through any specific test case or implementation detail.

[sophie-cluml]

@octoaide, could you rebase please?

[octoaide]

I rebased this PR onto the latest main branch and automatically resolved merge conflicts in CHANGELOG.md, CHANGELOG.md by keeping entries from both branches.

[sophie-cluml]

Suggested change

	- **BREAKING**: Bumped the database format to `0.47.0`. The migration from
	`0.46.x` creates the customer data deletion jobs column family explicitly;
	migrations from older supported formats preserve their legacy column-family
	sets while applying intermediate migrations before creating the new family.

Please remove the duplicate entry.

[sophie-cluml]

I think it might be helpful to note the unit of the times, for downstream repos.

[kimhanbeom]

I updated this PR with the following additional changes:

• REview initially resolves the target Host FQDNs from Node information. However, if the Node data is removed and another deletion step subsequently fails, a retry can no longer reconstruct the original deletion targets. To ensure that the initially resolved targets can be persisted and reused, I changed host_fqdn: Option<String> to host_fqdns: Vec<String>. I also updated the validation logic and tests so that REview requires at least one Host FQDN, while Sensor and SemiSupervised require exactly one.
• I also addressed the two changes requested by @sophie-cluml. I removed the duplicate database-version entry from CHANGELOG.md and documented that requested_at and completed_at use Unix epoch nanoseconds (UTC).

[sophie-cluml]

Could we clarify the intended semantics of requested_at? If this is the original request/creation time for the service result, update_service() should probably preserve it instead of overwriting it. If it is meant to track the most recent deletion request attempt, the current update behavior can make sense, but the field name/docs should make that explicit, e.g. last_requested_at or documentation saying it is updated on retries.

[kimhanbeom]

The intended semantics of requested_at are to record the request time of the most recent deletion attempt. Therefore, overwriting it in update_service() is intentional when a deletion is retried. I kept the existing field name and updated its documentation to clarify that a retry replaces the previous value.