Develop#11
Merged
Merged
Conversation
Drop the URL :tenantId path segment and adopt the Lerian database-per-tenant
model. Tenant scope is resolved from context.Context via the lib-commons
tenant-manager, so admin routes carry no tenant identifier and storage rows
hold no tenant_id column. Each tenant has its own database; isolation is
natural by connection routing.
Storage layer
- systemplane_entries (Postgres): PK (namespace, key); trigger payload is
{namespace, key, op}. No tenant_id column.
- MongoDB compound _id is {namespace, key}. No tenantID field.
- New store.Entry/Event types with Op = upsert|delete.
Connection resolution
- WithMultiTenantEnabled() toggle; WithModule(name) defaults to "systemplane".
- Multi-tenant: resolveDB(ctx) / resolveCollection(ctx) call
tmcore.GetPGContext / tmcore.GetMBContext. Lazy CREATE TABLE IF NOT EXISTS
per tenant DB via sync.Map cache.
- Single-tenant: static *sql.DB / *mongo.Client from the constructor
(unchanged behavior).
LISTEN/NOTIFY and in-process cache
- Single-tenant: kept (LISTEN connection for Postgres, change stream for
Mongo, in-process value cache).
- Multi-tenant: disabled. Every Get hits the resolved tenant DB; OnChange
and Subscribe return ErrNotSupportedInMultiTenant.
Client surface (collapsed)
- Set/Get/Delete/List/Register are ctx-aware; *ForTenant variants gone.
- OnChange callback signature: func(ctx, ns, key, newValue).
- IsRegistered replaces KeyStatus.
Admin
- Four routes: GET/PUT/DELETE /:ns/:key and GET /:ns. The :tenants and
:tenantID segments are gone. WithAuthorizer hooks read|write; the
authorizer may inspect c.UserContext() for tenant-aware policy.
Errors
- New: ErrNotSupportedInMultiTenant, ErrTenantConnectionMissing.
- Dropped: ErrMissingTenantContext, ErrInvalidTenantID,
ErrTenantScopeNotRegistered, ErrTenantSchemaNotEnabled.
Tests
- Unit: 38 (-race). Contract suite collapsed and parameterized with
RunOptions{SkipSubscribe}.
- Integration: Postgres + Mongo via testcontainers, two databases inside
one container exercise multi-tenant isolation and schema-once caching.
Docs
- README and CLAUDE.md rewritten for the two operating modes.
- MIGRATION_TENANT_SCOPED.md deleted.
X-Lerian-Ref: 0x1
Apply the 15 items raised by CodeRabbit on PR #9. Real bugs (correctness) - mongodb: key schema cache by stable string "<db.Name()>/<collection>" instead of the *mongo.Collection pointer, which is not stable across resolveCollection calls in mongo-driver v2. - mongodb: use db.CreateCollection in multi-tenant bootstrap so the collection actually exists before the first Get. Single-tenant keeps Indexes().List to preserve the existing change-stream attach semantics (CreateCollection caused a flaky first-insert race against the oplog watcher in single-tenant integration tests). The asymmetry is documented in runSchema. - mongodb polling: emit OpDelete by diffing snapshotKeys() between polls. - mongodb polling: dedupe at watermark boundary via seenAtWatermark set with >= filter, so two writes inside the same millisecond are both observed. - mongodb changestream: Subscribe spawns a goroutine that observes ctx.Done() and runs the shared unsubscribe — canceled callers no longer leak handlers. - mongodb: List passes options.Find().SetSort on (namespace, key) so the docstring's ordering guarantee holds. - client: typed accessors (GetString/Int/Bool/Float64/Duration) return (zero, false, ErrValidation) on type/parse failure instead of silently collapsing to zero values. GetInt rejects fractional float64. - client: Start re-checks closed under startMu; Close acquires startMu inside closeOnce.Do so Start and Close are mutually exclusive. - client: hydration vs changefeed race resolved by tracking hydrationTouched during Start and skipping those keys in hydrate, so a changefeed event that lands during hydration is not overwritten by the stale List snapshot. Medium hardening - mongodb + postgres: empty namespace/key wrap store.ErrValidation (newly added); client.ErrValidation is aliased so the admin layer's 400 mapping continues to work end-to-end. - mongodb + postgres: drop the high-cardinality actor attribute from Delete spans. Actor still flows through UpdatedBy on the row. Refactor - client: Client owns a lifecycleCtx canceled by Close; fireSubscribers threads it through to OnChange callbacks instead of context.Background. - admin tests: setupClient returns (*Client, *fakeStore) so write-path assertions can verify the actor extractor reached Delete. - admin tests: namespace filtering test seeds a second namespace and asserts it is excluded. Nit - mongodb integration tests: the missing-tenant-context case no longer starts a container — the assertion is purely in-process. Tests: 46 unit (with -race, +8 new) and 31 integration pass. X-Lerian-Ref: 0x1
CodeRabbit fixes (7)
- client: hydrationTouched is now set AFTER the refresh succeeds, so a
failing changefeed refresh no longer poisons the cache by skipping the
later hydrate List snapshot.
- client: kept the //nolint:gosec on the lifecycle context.WithCancel
allocation — removing it (as CodeRabbit suggested) re-fires G118 under
this repo's gosec config; the directive is load-bearing because cancel
is owned by Close() rather than deferred locally. Restored with a
clearer rationale comment.
- mongodb Subscribe: nil-ctx guard and a single sync.Once that wraps both
subscriber-map removal AND cancelCh close, so concurrent ctx.Done() and
caller-driven unsubscribe can no longer double-close the channel.
- mongodb ensureSchema: failure no longer pinned. On runSchema error the
cached *sync.Once and the schemaErr entry are both evicted so the next
call retries. Added a schemaRunner test seam to exercise this without
a live MongoDB.
- client Close: store.Close() error is now captured and returned wrapped
("systemplane: close store: ...") instead of being discarded.
- client Get/List: json.Unmarshal failures on stored values log at error
level and return a wrapped error instead of falling back to the
default value with ok=true; corrupted JSON is no longer indistinguishable
from a missing row.
- admin: TestAdmin_Delete now asserts that the actor produced by
WithActorExtractor reaches the store (LastDeleteActor accessor on the
fakeStore).
goleak coverage
- TestMain with goleak.VerifyTestMain added to internal/client,
internal/mongodb, internal/postgres. Ignore lists are conservative and
documented (testcontainers Reaper, mongo-driver topology shutdown,
pgxpool.backgroundHealthCheck owned by the caller's DB, net/http
persistConn keep-alive on the Docker client).
- Targeted assertions covering: Subscribe ctx-observer exit on cancel,
Subscribe explicit unsubscribe, concurrent double-unsubscribe + ctx
cancel race (idempotent under sync.Once), nil-ctx safety, change
stream watcher cleanup on Close, polling ticker cleanup, postgres
LISTEN reader cleanup on Close.
- go.uber.org/goleak promoted from indirect to direct dependency.
Tests: 56 unit (+9) and 20 integration (+4) pass with -race; lint, vet,
gofmt, go mod tidy/verify all clean.
X-Lerian-Ref: 0x1
…a callers The failure branch of the once.Do closure in ensureSchemaByKey stored the bootstrap error and immediately deleted it. Concurrent callers that joined the same once.Do — whose local runErr was nil because they did not execute the closure — would then read schemaErr.Load after the delete and observe nil, masking the bootstrap failure as a successful no-op. Remove the immediate Delete in the failure path. Keep schemaOnce.Delete so future attempts retry, and keep the success branch's schemaErr.Delete so the tombstone is cleared on the next successful bootstrap (the success-branch Delete is now load-bearing rather than defensive). Update the existing TestEnsureSchema_TransientFailureRetries assertion that expected immediate eviction; it now asserts the tombstone remains after a failed run and is cleared after the subsequent success. Add TestEnsureSchema_ConcurrentCallersObserveFailure to pin the race: 64 worker goroutines join the same once.Do with a runner that fails behind a release channel, and every worker MUST observe the masked error. Verified regression-sharp by stashing only the production fix: 63 of 64 workers silently dropped the failure; with the fix all 64 observe it. X-Lerian-Ref: 0x1
The polling-mode boundary dedupe keyed seenAtWatermark by (namespace, key)
only. Two writes to the same row landing in the same BSON millisecond
caused the second write to be silently skipped: the first poll recorded
the key, and the next poll's $gte query returned the row again and matched
the dedupe set without comparing the actual value — so the new value never
reached subscribers and peer caches stayed stale until some later write
advanced the watermark.
Carry a content discriminator in the dedupe set: each entry is now a
seenEntry{valueHash} where valueHash is FNV-64a over the raw entryDoc.Value
bytes (the JSON payload is already stored as a BSON string in the document,
so no marshal round-trip). The dedupe rule becomes "skip only when (key,
hash) match"; idempotent rewrites still dedupe, real rewrites now emit.
Tests:
- TestBoundaryDedupHit_DecisionRule pins all five branches of the rule
- TestHashValue_Stability pins hashValue determinism and discrimination
- TestIntegration_PollOnce_SameMsDifferentValue_EmitsBoth is a regression
guard against the original bug (pre-fix emits one event, post-fix emits
two)
- TestIntegration_PollOnce_SameMsSameValue_EmitsOnce is the negative case
for idempotent rewrites
CodeRabbit also flagged //nolint:gosec on the lifecycle context.WithCancel
allocation (client.go:153) as stale. Verified by removing the directive
and running golangci-lint: gosec G118 fires immediately because cancel is
released from Close() rather than via a deferred call. Directive is
load-bearing; kept with the existing inline rationale.
X-Lerian-Ref: 0x1
feat: database-per-tenant compliance (closes #6)
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (86)
WalkthroughThis PR comprehensively refactors the systemplane library to remove tenant-scoped public APIs, restructure multi-tenant support, and make read accessors context-aware with explicit error returns. It shifts from a complex dual-registry design to a two-mode architecture (single-tenant process-wide cache with changefeed, multi-tenant context-driven per-request resolution) and eliminates tenant-aware HTTP admin routes in favor of a simpler four-route design. ChangesPublic API and implementation refactoring
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.