Skip to content

[ENH](frontend): block functions on topology dbs#6836

Open
rescrv wants to merge 23 commits intorescrv/mcmr-testsfrom
rescrv/mcmr-deny-features
Open

[ENH](frontend): block functions on topology dbs#6836
rescrv wants to merge 23 commits intorescrv/mcmr-testsfrom
rescrv/mcmr-deny-features

Conversation

@rescrv
Copy link
Copy Markdown
Contributor

@rescrv rescrv commented Apr 6, 2026

Description of changes

Reject attached function operations when the database name carries a
topology prefix, reusing the same multi-region validation path as
collection forking.

Add targeted frontend endpoint tests covering attach, get, and
detach against topology-prefixed databases.

Test plan

Integration tests that hit the endpoint and confirm error are included.

Migration plan

N/A

Observability plan

N/A

Documentation Changes

N/A

Co-authored-by: AI

rescrv added 5 commits April 6, 2026 14:09
Add backend overrides for manifest bounds and bounded fragment scans.

Use the Spanner-backed manifest manager to:
- derive manifest bounds without loading the full manifest
- scan fragments from the compaction offset with a position_limit index
- let log-service scout and pull logs through the partial-read path
- expose a phase-0 garbage reset helper for GC recovery

Essentially, what we can do now because of this patch is do a bounded
manifest fetch.  As spanner manifests grow in size, the cost of fetching
the manifest begins to dominate.  This adds a
proportional-to-the-fragments fetch path.

Co-authored-by: AI
Introduce dedicated multi-cluster multi-region (MCMR) property tests
that write records across two regions and verify cross-region
replication invariants.  Parameterize existing test_add tests by
database name so they run against both classic and MCMR databases
when multi-region is enabled.

Fix dirty log detection in the repl manifest manager:
- Treat intrinsic_cursor=0 as unset so newly initialized logs are
  not spuriously reported as dirty
- Initialize manifest_regions rows with intrinsic_cursor=0 instead
  of omitting the column, preventing GC from misinterpreting the
  starting offset as an active reader position
- Change the SQL threshold comparison from > to >= so logs at the
  exact threshold boundary are correctly included
- Add decode_intrinsic_cursor helper that filters out zero values

Retry Spanner-aborted flush compaction transactions:
- Add is_spanner_aborted() to SysDbError to detect gRPC Aborted
  status
- Wrap the flush_collection_compaction read-write transaction with
  backon retry (100ms delay, 4 attempts) on Spanner abort
- Propagate gRPC status codes through SysDbError and
  FlushCompactionError instead of mapping all Spanner errors to
  Internal

Improve dirty log test diagnostics by dumping in-memory and Spanner
state on assertion failure.  Add regression tests for single-add and
three-add dirty log scenarios.

Co-authored-by: AI
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 6, 2026

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)

@propel-code-bot
Copy link
Copy Markdown
Contributor

propel-code-bot bot commented Apr 6, 2026

Block attached function operations on topology-prefixed databases in frontend

This PR adds a shared validation helper in rust/frontend/src/server.rs to reject unsupported operations when DatabaseName has a topology prefix (database_name.topology().is_some()). The existing fork rejection logic is refactored to use this helper, and the same validation is now applied to attached function endpoints (attach, get, and detach).

The PR also expands endpoint tests in the same file to verify 400 Bad Request + InvalidArgumentError behavior for topology-prefixed databases across these function operations. Test code is cleaned up with shared helpers (assert_invalid_argument, multi_region_database) and consolidated reqwest imports.

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

Copy link
Copy Markdown
Contributor

@propel-code-bot propel-code-bot bot left a comment

Choose a reason for hiding this comment

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

No issues were identified; the validation centralization and expanded tests appear correct and consistent.

Status: No Issues Found | Risk: Low

Review Details

📁 1 files reviewed | 💬 0 comments

@rescrv rescrv requested a review from tanujnay112 April 6, 2026 23:03
@blacksmith-sh

This comment has been minimized.

@rescrv rescrv force-pushed the rescrv/mcmr-deny-features branch from e48877b to 9c34b92 Compare April 7, 2026 23:57
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