Repository navigation
fix: reject cross-segment ExclusiveStartKey in parallel Scan #203
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
Changes from all commits
43200aa
5170d9b
966c553
6d5d55f
90f5172
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 |
|---|---|---|
|
|
@@ -376,6 +376,35 @@ pub trait DataEngine: Send + Sync { | |
| index_name: Option<&str>, | ||
| ) -> BoxFuture<'_, QueryResult>; | ||
|
|
||
| /// Whether `key` belongs to `segment` of a `total_segments`-way parallel | ||
| /// scan, under this backend's own segment-assignment function. | ||
| /// | ||
| /// Segment assignment is backend-private (Postgres hashes the partition | ||
| /// key with `hashtext`, MongoDB with CRC32, SQLite partitions by rowid), | ||
| /// so only the backend can answer this. The engine owns the rule and the | ||
| /// error: a `Scan` whose `ExclusiveStartKey` reports `false` here is | ||
| /// refused before storage is asked to scan, because resuming a segment | ||
| /// with another segment's key would otherwise return a silently wrong | ||
| /// (truncated or fabricated) page. | ||
| /// | ||
| /// `Ok(true)` means "cannot be proven foreign" rather than "proven local": | ||
|
Collaborator
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. Worth one more sentence here: the lenient rule is justified by "DynamoDB permits an ExclusiveStartKey that no longer exists" — true, but the service hashes key content, so it still rejects a deleted foreign key, while SQLite admits it and can return exactly the silently-wrong page this PR prevents. That residual divergence is inherent to rowid assignment and fine to accept, but the doc should state it rather than leaving the impression the lenient path is service-identical. |
||
| /// a backend whose assignment depends on storage identity rather than key | ||
| /// content (SQLite's rowid) MUST return `true` for a key it cannot resolve, | ||
| /// because DynamoDB permits an `ExclusiveStartKey` that no longer exists | ||
| /// and refusing it would reject legitimate resumptions after a delete. | ||
| /// | ||
| /// Required, not defaulted: a new backend must decide its answer | ||
| /// explicitly, since returning a constant `true` silently disables the | ||
| /// engine's cross-segment refusal for that backend. | ||
| fn scan_key_in_segment( | ||
| &self, | ||
| key_info: &TableKeyInfo, | ||
| key: &Item, | ||
| segment: i64, | ||
| total_segments: i64, | ||
| index_name: Option<&str>, | ||
| ) -> BoxFuture<'_, Result<bool, StorageError>>; | ||
|
|
||
| /// The vector-search implementation, if this backend has one. | ||
| /// | ||
| /// Defaults to `None`, so a backend that has never heard of vector search is | ||
|
|
@@ -816,6 +845,17 @@ mod vector_opt_out_tests { | |
| struct MinimalDataEngine; | ||
|
|
||
| impl DataEngine for MinimalDataEngine { | ||
| fn scan_key_in_segment( | ||
| &self, | ||
| _key_info: &TableKeyInfo, | ||
| _key: &Item, | ||
| _segment: i64, | ||
| _total_segments: i64, | ||
| _index_name: Option<&str>, | ||
| ) -> BoxFuture<'_, Result<bool, StorageError>> { | ||
| Box::pin(async { Ok(true) }) | ||
| } | ||
|
|
||
| fn put_item( | ||
| &self, | ||
| _key_info: &TableKeyInfo, | ||
|
|
||
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.
This defensive arm contradicts the trait's documented semantics (same in the SQLite impl):
falsemeans "proven foreign", so if this is ever reached the client gets a misleading "wrong Segment" ValidationException for a malformed key. Upstream validation makes it unreachable today, but eitherOk(true)per the documented rule or anInternalerror would keep the defensive path from inventing a segment verdict.