Skip to content

fix: reject cross-segment ExclusiveStartKey in parallel Scan - #203

Merged
LeeroyHannigan merged 5 commits into
mainfrom
fix/parallel-scan-exclusive-start-key
Oct 13, 2026
Merged

LeeroyHannigan merged 5 commits into
mainfrom
fix/parallel-scan-exclusive-start-key

Conversation

@LeeroyHannigan

@LeeroyHannigan LeeroyHannigan commented Jul 7, 2026 •

Copy link
Copy Markdown
Collaborator

What

Reject a cross-segment ExclusiveStartKey in a parallel Scan.

A parallel Scan (TotalSegments/Segment) accepted an ExclusiveStartKey belonging to a different segment, silently returning a truncated or empty page. This validates the start key against the same segment-assignment function used
for the scan and rejects a mismatch with a ValidationException naming the correct Segment, so a key returned as a LastEvaluatedKey for one segment is only valid when the same segment is re-scanned.

Why

DynamoDB validates that an ExclusiveStartKey supplied to a parallel scan belongs to the requested segment, and rejects it otherwise; the engine instead accepted any key and combined it with the segment predicate, silently yielding a truncated or empty page rather than an error. This is a correctness/parity gap for parallel-scan pagination. The behavior was verified against the real DynamoDB service.

Closes #

Testing done

  • Scan integration test: a key that belongs to a segment is accepted as that segment's ExclusiveStartKey, and rejected with a ValidationException when supplied for a different segment.
  • Verified verbatim against the real DynamoDB service (us-east-1): a same-segment ExclusiveStartKey paginates within the segment; a cross-segment key returns ValidationException: The provided starting key is invalid: Invalid ExclusiveStartKey. Please use ExclusiveStartKey with correct Segment. TotalSegments: 4 Segment: N.
  • cargo test -p extenddb-storage-postgres unit pass; scan integration test passes; cargo clippy --workspace --all-targets and cargo fmt --all -- --check clean.

Checklist

  • I have read CONTRIBUTING.md
  • All tests pass (cargo test --workspace)
  • Code is formatted (cargo fmt --check)
  • Clippy is clean (cargo clippy -- -W clippy::pedantic)
  • I have added or updated tests for new functionality
  • I have updated documentation if behavior changed
  • Breaking changes are noted below (if any)
  • If this changes the wire protocol, Storage trait, auth model, on-disk
    format, or public CLI surface, an RFC has been accepted or is linked
    below. Otherwise, an ADR captures the decision (link below).

ADR / RFC: n/a -- no change to wire protocol, Storage trait, auth model,
on-disk format, or public CLI surface. Request-validation correctness only.

Breaking changes

None. A request that was previously mis-accepted (a cross-segment ExclusiveStartKey) now returns the DynamoDB-correct ValidationException; valid same-segment pagination is unchanged.


By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache License 2.0 and I agree to the Developer Certificate of
Origin (DCO). See CONTRIBUTING.md for details.

jcshepherd
jcshepherd previously approved these changes Jul 7, 2026

@jcshepherd jcshepherd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little bizarre that we're executing a SQL query to do a mathematical calculation. I get why but in a perfect world I wonder if we'd be taking a dependency on a Postgres internal hash function for assigning segments.

@LeeroyHannigan
LeeroyHannigan added this pull request to the merge queue Jul 8, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Jul 8, 2026
A parallel Scan (TotalSegments/Segment) accepted an ExclusiveStartKey
that belongs to a different segment, silently returning a truncated or
empty page. Validate the start key against the same segment-assignment
function used for the scan and reject a mismatch with a ValidationException
naming the correct Segment, so a key returned as a LastEvaluatedKey for one
segment is only valid when the same segment is re-scanned. Verified against
DynamoDB.

Adds a scan integration test.

Signed-off-by: Lee Hannigan <lhnng@amazon.com>
LeeroyHannigan and others added 3 commits August 16, 2026 12:24
A parallel `Scan` resumed with a `LastEvaluatedKey` produced by a
different segment returned a silently wrong page instead of an error.
The refusal existed only in the Postgres scan path, so MongoDB and
SQLite fabricated a page from whichever rows their own segment
predicate happened to match. The MongoDB integration test
`parallel_scan_rejects_cross_segment_exclusive_start_key` proved this:
it received three items where it expected a ValidationException.

The check cannot live in shared engine code as pure logic, because each
backend assigns segments with its own private function:

  Postgres  (hashtext(pk)::bigint & 2147483647) % total = segment
  MongoDB   crc32(pk_text) % total = segment
  SQLite    rowid % total = segment, not a function of the key at all

So the split follows the ADR-0005 pattern: the engine owns the rule and
the measured service message, and the storage trait gains one primitive,
`scan_key_in_segment`, that each backend answers for itself. Postgres
keeps its existing `hashtext` query (moved out of `query_scan` behind the
trait method), MongoDB answers with a pure CRC32 computation and no round
trip, and SQLite resolves the key to its rowid.

Two decisions worth recording:

`Ok(true)` means "cannot be proven foreign", not "proven local". SQLite's
assignment depends on storage identity rather than key content, so a key
it cannot resolve must be admitted: DynamoDB permits an
`ExclusiveStartKey` for an item that no longer exists, and refusing it
would reject a legitimate resumption after a delete.

The trait method is required rather than defaulted. A default returning
`true` would silently disable the refusal for any future backend, which
is the exact failure this commit fixes.

Verification, per backend, live against a running server:

  sqlite    13/13 scan integration tests
  postgres  13/13 scan integration tests
  mongodb   13/13 scan integration tests

Each backend also has a negative control: sabotaging its
`scan_key_in_segment` to return `Ok(true)` unconditionally makes the
discriminating test fail with the fabricated page, confirming the test
distinguishes the fix from its absence rather than passing incidentally.

fmt clean; clippy `--all-targets -D warnings` clean on the sqlite and
postgres feature sets; 943 lib tests passed, 0 failed, 0 filtered on
both. The mongodb clippy failure (`install_backend` dead code in
`crates/bin/src/main.rs`) is pre-existing on this branch's base and
reproduces identically with these changes stashed.
@LeeroyHannigan

Copy link
Copy Markdown
Collaborator Author

Pushed 6d5d55f, which fixes the MongoDB integration failure by moving the refusal to where it can be uniform.

The failing test was correct. parallel_scan_rejects_cross_segment_exclusive_start_key got three items back on MongoDB where it expected a ValidationException, because the check lived inside the Postgres scan path only. MongoDB and SQLite fabricated a page from whatever their own segment predicate matched.

It can't be shared logic in the engine, because each backend assigns segments differently and privately:

backend assignment
Postgres (hashtext(pk)::bigint & 2147483647) % total
MongoDB crc32(pk_text) % total
SQLite rowid % total, not a function of the key at all

So it follows the ADR-0005 split: the engine owns the rule and the measured service message, and the storage trait gains one primitive, scan_key_in_segment, that each backend answers for itself. Postgres keeps its existing hashtext query (moved out of query_scan behind the trait method, no behaviour change), MongoDB is a pure CRC32 computation with no round trip, and SQLite resolves the key to its rowid.

@jcshepherd this also speaks to your point about the SQL query doing a mathematical calculation. It's still a query on Postgres, but the dependency on hashtext is now contained behind a trait method that documents it as backend-private, rather than being the shape the shared path assumes. If we ever want to move Postgres off hashtext, that's now a one-backend change with no engine edit.

Two decisions worth flagging for review:

Ok(true) means "cannot be proven foreign", not "proven local". SQLite's assignment depends on storage identity rather than key content, so a key it can't resolve has to be admitted: DynamoDB permits an ExclusiveStartKey for an item that no longer exists, and refusing it would reject a legitimate resumption after a delete.

The trait method is required, not defaulted. A default returning true would silently disable the refusal for any future backend, which is the failure mode this PR exists to fix.

Verification, live against a running server for each backend:

  • sqlite 13/13 scan integration tests
  • postgres 13/13
  • mongodb 13/13

Each backend also has a negative control: sabotaging its scan_key_in_segment to return Ok(true) unconditionally makes the discriminating test fail with the fabricated page, so the test distinguishes the fix from its absence rather than passing incidentally.

fmt clean; clippy --all-targets -D warnings clean on the sqlite and postgres feature sets; 943 lib tests passed, 0 failed, 0 filtered on both. The mongodb clippy failure (install_backend dead code in crates/bin/src/main.rs) is pre-existing on this branch's base and reproduces identically with these changes stashed, so it's not from this work, but it will need addressing separately for the mongodb job to go green.

@robinnsc robinnsc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving — the rework is the right structure: the engine owns the rule and the measured message, the trait answers backend-private membership, and the negative controls show the test discriminates the fix from its absence. Keeping hashtext in SQL rather than reimplementing the Postgres-internal hash in Rust is the safer choice.

One request that doesn't anchor to a line: the PR description contradicts the final code. The ADR/RFC section still says "no change to wire protocol, Storage trait, ..." and Testing Done still describes the old Postgres-only iteration — but 6d5d55f adds scan_key_in_segment to the DataEngine trait, which is exactly the surface the checklist says needs an RFC or ADR. The commit message's ADR-0005 argument may well satisfy that, but the description should say so rather than denying the trait change, since it's what reviewers and the merge record will read. Please update it before merging.

Two smaller comments inline.

Box::pin(async move {
let Some(pk_value) = key.get(&pk_name) else {
// Schema validation upstream already rejects this shape.
return Ok(false);

Copy link
Copy Markdown
Collaborator

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): false means "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 either Ok(true) per the documented rule or an Internal error would keep the defensive path from inventing a segment verdict.

Comment thread crates/storage/src/lib.rs
/// 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":

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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.

@LeeroyHannigan
LeeroyHannigan added this pull request to the merge queue Oct 13, 2026
Merged via the queue into main with commit 1f9a59d Oct 13, 2026
29 checks passed
@LeeroyHannigan
LeeroyHannigan deleted the fix/parallel-scan-exclusive-start-key branch October 13, 2026 11:02
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.

3 participants