-
-
Notifications
You must be signed in to change notification settings - Fork 171
Add tombstone/deleting safety net for stream deletion (#1763) #1768
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1217,8 +1217,12 @@ impl Stream { | |
| } | ||
|
|
||
| /// Stores the provided stream metadata in memory mapping | ||
| pub async fn set_metadata(&self, updated_metadata: LogStreamMetadata) { | ||
| *self.metadata.write().expect(LOCK_EXPECT) = updated_metadata; | ||
| pub async fn set_metadata(&self, mut updated_metadata: LogStreamMetadata) { | ||
| let mut metadata = self.metadata.write().expect(LOCK_EXPECT); | ||
| // mark_deleting() is documented as monotonic -- a reload racing a | ||
| // delete must not silently clear it back to false. | ||
| updated_metadata.deleting |= metadata.deleting; | ||
| *metadata = updated_metadata; | ||
| } | ||
|
|
||
| pub fn get_first_event(&self) -> Option<String> { | ||
|
|
@@ -1352,6 +1356,17 @@ impl Stream { | |
| self.metadata.read().expect(LOCK_EXPECT).hot_tier_enabled | ||
| } | ||
|
|
||
| /// Marks this stream as being deleted. Once set, this flag is never | ||
| /// cleared for this in-memory entry — a deletion in progress runs to | ||
| /// completion (or is resumed on restart), it is never cancelled. | ||
| pub fn mark_deleting(&self) { | ||
| self.metadata.write().expect(LOCK_EXPECT).deleting = true; | ||
| } | ||
|
|
||
| pub fn is_deleting(&self) -> bool { | ||
| self.metadata.read().expect(LOCK_EXPECT).deleting | ||
| } | ||
|
Comment on lines
+1359
to
+1368
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift Enforce deletion at the write boundary.
🤖 Prompt for AI Agents🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Preserve
Proposed fix pub async fn set_metadata(&self, updated_metadata: LogStreamMetadata) {
- *self.metadata.write().expect(LOCK_EXPECT) = updated_metadata;
+ let mut metadata = self.metadata.write().expect(LOCK_EXPECT);
+ let deleting = metadata.deleting || updated_metadata.deleting;
+ *metadata = updated_metadata;
+ metadata.deleting = deleting;
}🤖 Prompt for AI Agents |
||
|
|
||
| pub fn get_stream_type(&self) -> StreamType { | ||
| self.metadata.read().expect(LOCK_EXPECT).stream_type | ||
| } | ||
|
|
@@ -1744,6 +1759,22 @@ mod tests { | |
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_mark_deleting_sets_is_deleting() { | ||
| let options = Arc::new(Options::default()); | ||
| let stream = Stream::new( | ||
| options, | ||
| "test_stream", | ||
| LogStreamMetadata::default(), | ||
| None, | ||
| &None, | ||
| ); | ||
|
|
||
| assert!(!stream.is_deleting()); | ||
| stream.mark_deleting(); | ||
| assert!(stream.is_deleting()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_staging_with_special_characters() { | ||
| let stream_name = "test_stream_!@#$%^&*()"; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
Repository: parseablehq/parseable
Length of output: 12503
🏁 Script executed:
Repository: parseablehq/parseable
Length of output: 50377
🏁 Script executed:
Repository: parseablehq/parseable
Length of output: 37643
🏁 Script executed:
Repository: parseablehq/parseable
Length of output: 50377
Make the tombstone check cover resident streams and the load race.
check_or_load_streamreturnstruefor a resident stream without callingis_tombstoned. A tombstone alone can therefore leave the resident stream accessible whendeletingisfalse. The check increate_stream_and_schema_from_storagealso occurs before listing, loading, and registration, so a concurrent tombstone can be missed. Coordinate tombstone admission with stream registration, or re-check it under deletion coordination. Add a test that keeps a stream resident, writes its tombstone, and expects rejection without settingdeleting.🤖 Prompt for AI Agents