Skip to content

[ENH]: add validation during garbage collection for empty file paths #4586

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 27, 2025

Conversation

codetheweb
Copy link
Contributor

@codetheweb codetheweb commented May 20, 2025

Description of changes

Adds a defensive check that validates that the current version is v0 if there are no file paths.

Test plan

How are these changes tested?

  • Tests pass locally with pytest for python, yarn test for js, cargo test for rust

Added a test.

Documentation Changes

Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs section?

n/a

Copy link

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

@codetheweb codetheweb force-pushed the feat-validate-file-paths-during-gc branch from f57dc9d to b8ab357 Compare May 20, 2025 21:20
@codetheweb codetheweb marked this pull request as ready for review May 20, 2025 21:21
@codetheweb codetheweb requested a review from sanketkedia May 20, 2025 21:21
Copy link
Contributor

propel-code-bot bot commented May 20, 2025

Add Defensive Validation for Empty File Paths During Garbage Collection

This PR introduces a safeguard in the garbage collector, ensuring that if a collection version has no file paths, it is only permitted if the version is 0 and all its ancestors are also at version 0. It enforces this invariant with explicit validation in the orchestrator, adds a comprehensive test, and cleans up internal sysdb structures to align version file path management with implementation requirements.

Key Changes:
• Added validation logic in garbage_collector_orchestrator_v2.rs to ensure empty file paths are only allowed for v0 and v0 ancestor versions.
• Introduced a new test (errors_on_empty_file_paths) to verify that an error is thrown if a non-v0 version lacks file paths.
• Refactored sysdb test helpers: removed collection_to_version_file_name in favor of using the version_file_path field within collection structs.
• Enhanced test setup for construct_version_graph_orchestrator to ensure collections are properly created before attaching version files.
• Updated proptest_helpers reference to ensure generated transitions for IncrementCollectionVersion only allow new versions with at least one file path.
• Minor test logic fixes in worker orchestration tests to handle updated behavior for version_file_path increments.

Affected Areas:
• Garbage collector orchestrator (validation logic)
• TestSysDb mock and associated test helpers
• Construct version graph orchestrator test setup
• Proptest reference model for garbage collection
• Worker orchestration compaction tests

This summary was automatically generated by @propel-code-bot

@codetheweb codetheweb force-pushed the feat-gc-orchestrator-v2 branch from a72bc81 to 5c86e1c Compare May 20, 2025 21:27
@codetheweb codetheweb force-pushed the feat-validate-file-paths-during-gc branch from b8ab357 to 76c115d Compare May 20, 2025 21:27
@codetheweb codetheweb force-pushed the feat-gc-orchestrator-v2 branch from 5c86e1c to 2191cf0 Compare May 20, 2025 22:02
@codetheweb codetheweb force-pushed the feat-validate-file-paths-during-gc branch from 76c115d to 02546b5 Compare May 20, 2025 22:02
@codetheweb codetheweb force-pushed the feat-gc-orchestrator-v2 branch from 2191cf0 to 1451c71 Compare May 21, 2025 16:46
@codetheweb codetheweb force-pushed the feat-validate-file-paths-during-gc branch from 02546b5 to c570721 Compare May 21, 2025 16:46
@codetheweb codetheweb force-pushed the feat-validate-file-paths-during-gc branch from c570721 to bcbf758 Compare May 22, 2025 22:23
@codetheweb codetheweb force-pushed the feat-gc-orchestrator-v2 branch from 1451c71 to 53e1a29 Compare May 22, 2025 23:24
@codetheweb codetheweb force-pushed the feat-validate-file-paths-during-gc branch from bcbf758 to f72bd63 Compare May 22, 2025 23:24
@codetheweb codetheweb force-pushed the feat-gc-orchestrator-v2 branch from 53e1a29 to 8e08e2f Compare May 23, 2025 00:00
@codetheweb codetheweb force-pushed the feat-validate-file-paths-during-gc branch from f72bd63 to c62bf3f Compare May 23, 2025 00:00
@codetheweb codetheweb force-pushed the feat-gc-orchestrator-v2 branch from 8e08e2f to de511ea Compare May 23, 2025 00:48
@codetheweb codetheweb force-pushed the feat-validate-file-paths-during-gc branch from c62bf3f to d6bf939 Compare May 23, 2025 00:48
@codetheweb codetheweb force-pushed the feat-gc-orchestrator-v2 branch from de511ea to c73fd49 Compare May 23, 2025 01:26
@codetheweb codetheweb force-pushed the feat-validate-file-paths-during-gc branch from d6bf939 to e0a0f88 Compare May 23, 2025 01:27
@codetheweb codetheweb force-pushed the feat-gc-orchestrator-v2 branch from c73fd49 to 3a3adda Compare May 23, 2025 02:07
@codetheweb codetheweb force-pushed the feat-validate-file-paths-during-gc branch from e0a0f88 to 7f275bb Compare May 23, 2025 02:08
@codetheweb codetheweb force-pushed the feat-gc-orchestrator-v2 branch 2 times, most recently from 7969af3 to 2ce9ae0 Compare May 23, 2025 17:11
@codetheweb codetheweb force-pushed the feat-validate-file-paths-during-gc branch from 7f275bb to a22cc8a Compare May 23, 2025 17:11
@codetheweb codetheweb changed the base branch from feat-gc-orchestrator-v2 to graphite-base/4586 May 23, 2025 22:54
@codetheweb codetheweb force-pushed the graphite-base/4586 branch from 2ce9ae0 to 9dec43e Compare May 23, 2025 22:54
@codetheweb codetheweb force-pushed the feat-validate-file-paths-during-gc branch from a22cc8a to 3cd7274 Compare May 23, 2025 22:54
@graphite-app graphite-app bot changed the base branch from graphite-base/4586 to main May 23, 2025 22:55
@codetheweb codetheweb force-pushed the feat-validate-file-paths-during-gc branch 2 times, most recently from 8f20043 to 4a3b406 Compare May 23, 2025 23:36
@codetheweb codetheweb force-pushed the feat-validate-file-paths-during-gc branch from 4a3b406 to e533b0c Compare May 27, 2025 17:22
@codetheweb codetheweb merged commit 3baca7c into main May 27, 2025
72 checks passed
Copy link
Contributor Author

Merge activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants