Skip to content

Conversation

@vroldanbet
Copy link
Contributor

@vroldanbet vroldanbet commented Nov 27, 2025

Description

This PR isolates dependency with github.com/odigos-io/go-rtml

The introduction of github.com/odigos-io/go-rtml as a dependency has caused many go toolchain operations to fail because it now requires special flags to resolve a runtime symbol. This is not ideal because it breaks the developer experience for folks using IDEs with native Go support. Something as simple as go test ./... no longer works without flags. But most importantly, it becomes sticky for any go module that imports SpiceDB.

This commit isolates the memory protection usage by:

  • adding a global variable that acts as the registry
  • isolating the MemoryUsageProvider into its own package
  • introducing a new NoopMemoryLimiter, and using it everywhere we explicitly used the go-rtml package
  • moving tests into isolated packages
  • initializing the global default provider in main.go, using a function controlled by build tags. Without the memoryprotection build tag, it defaults to using the noop implementation.
  • moving all tests in the main package to make sure one can run them without the need of passing the special flags.

Consequence:

  • now all tests run without special flags, except for those where we explicitly test for this behavior.
  • go modules importing SpiceDB do won't require the flags.
  • Folks can run SpiceDB in their IDEs quickly without tinkering with toolchain flags

Testing

  • Run main.go without tag should show a warning indicating it fell back to noop provider
  • Run main.go with ldflags and memoryprotection tag, no warn log is printed, and log shows the type used
  • run all mage commands that tounch something that uses ldflags and the tags
  • build docker container and check logs, making sure the rtml limiter is in place

References

@vroldanbet vroldanbet requested a review from a team as a code owner November 27, 2025 13:06
@github-actions github-actions bot added area/cli Affects the command line area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools) labels Nov 27, 2025
@vroldanbet vroldanbet enabled auto-merge November 27, 2025 13:07
@codecov
Copy link

codecov bot commented Nov 27, 2025

Codecov Report

❌ Patch coverage is 72.22222% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.05%. Comparing base (5baadbd) to head (97b8187).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...spicedb/memoryprotection/memory_protection_rtml.go 0.00% 2 Missing ⚠️
internal/middleware/memoryprotection/rtml/rtml.go 50.00% 2 Missing ⚠️
cmd/spicedb/main.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2734      +/-   ##
==========================================
+ Coverage   77.04%   77.05%   +0.01%     
==========================================
  Files         464      466       +2     
  Lines       49188    49199      +11     
==========================================
+ Hits        37891    37903      +12     
  Misses       8507     8507              
+ Partials     2790     2789       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@vroldanbet vroldanbet force-pushed the fix-sticky-memory-provider branch 2 times, most recently from 267aed3 to 34d3866 Compare November 27, 2025 14:08
The introduction of "github.com/odigos-io/go-rtml" as
a dependency has caused many go toolchain operations to fail
because it now requires special flags to resolve a runtime symbol.

This is not ideal because it breaks the developer experience
for folks using IDEs with native Go support. Something as
simple as go test ./... no longer works without flags.

But most importantly: it becomes sticky to any go module importing
SpiceDB.

This commit isolates the memory protection usage by:
- adding a global variable that acts as the registry
- isolating the MemoryUsageProvider into its own package
- introducing a new NoopMemoryLimiter, and using it everywhere
  we explicitly used the go-rtml package
- moving tests into isolated packages
- initializing the global default provider in main.go, using a function
  controlled by build tags. Without memoryprotection build tag, defaults
  to using the noop implementation.
- moving all tests in the main package to make sure one can run them
  without the need of passing the special flags.

Consequence:
- now all tests run without special flags, except for those where
  we explicitly test for this behavior.
- go modules importing SpiceDB do won't require the flags.
- folks can run SpiceDB in their IDEs quickly without tinkering
  with toolchain flags
@vroldanbet vroldanbet force-pushed the fix-sticky-memory-provider branch from 34d3866 to 97b8187 Compare November 27, 2025 21:23
@vroldanbet vroldanbet self-assigned this Nov 27, 2025
@github-actions github-actions bot added the area/dependencies Affects dependencies label Nov 27, 2025
Copy link
Contributor

@tstirrat15 tstirrat15 left a comment

Choose a reason for hiding this comment

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

LGTM, but wondering about the function name.

@vroldanbet vroldanbet added this pull request to the merge queue Dec 1, 2025
Merged via the queue into main with commit ae83447 Dec 1, 2025
76 of 78 checks passed
@vroldanbet vroldanbet deleted the fix-sticky-memory-provider branch December 1, 2025 16:40
@github-actions github-actions bot locked and limited conversation to collaborators Dec 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area/cli Affects the command line area/dependencies Affects dependencies area/tooling Affects the dev or user toolchain (e.g. tests, ci, build tools)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants