Kans/provenance semirings 20x++ grant expansion speed by kans · Pull Request #965 · ConductorOne/baton-sdk

kans · 2026-06-16T06:50:06Z

Summary

This PR adds a projection-backed topological grant expansion path for Pebble c1z files

The new path changes expansion from repeated source-page fanout / destination probing into a destination-first topological reduce. It builds a temporary Pebble projection DB for planned source entitlement streams, ingests the projection via SST, and then merges parent contribution streams by principal before writing dirty grants once per destination entitlement. This is probably at least 25x faster than main, possibly double that or more.

Key pieces:

Adds EntitlementGraphPlan, stored on EntitlementGraph, with per-node strategy metadata and selective ProjectionSources.
Adds RunTopologicalMergeProjection, plus supporting topological/streaming merge implementations.
Adds benchmark-only dense graph harnesses for current, topological, projection, and SST shuffle variants.
Adds projection-specific benchmark metrics:
- projection rows built
- nodes reduced
- destination entitlements reduced
- dirty grants written

Why
Large high-fan-in graphs make the existing expansion loop repeatedly probe and rewrite the same destination grant sets.

The main improvement is algorithmic: build compact source projections once, then reuse sorted source streams across destination reductions instead of repeatedly hydrating source grants and probing destination principals.

github-actions · 2026-06-16T06:54:38Z

+		if err := priBatch.Commit(pebble.NoSync); err != nil {
+			return err
+		}
+		return idxBatch.Commit(pebble.NoSync)


🟡 Suggestion: This path always commits NoSync, including on resumed (non-fresh) syncs. The doc above cites EndFreshSync as the hardening Flush, but EndFreshSync returns early without flushing when the sync isn't fresh (engine.go checks wasFresh and calls clearCurrentSync() then returns). So on a resumed sync, durability of these expanded grants depends solely on Close() → Quiesce(). That's acceptable since expanded grants are regenerable, but worth confirming a resumed sync always reaches Quiesce — otherwise these writes can be silently lost on a clean EndSync that isn't followed by Close.

github-actions · 2026-06-16T07:03:02Z

General PR Review: Kans/provenance semirings 20x++ grant expansion speed

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Review mode: full
View review run

Review Summary

Reviewed the new projection-backed topological grant-expansion path (pkg/sync/expand/topological_merge*.go, expansion_plan.go) and its Pebble engine support (PutExpandedGrantRecords, scratch-buffer key encoders, DecodeTupleStringAlias, KeyUpperBound). No security or confident correctness issues found. The topological path's cycle error is defensive — SyncGrantExpansion collapses cycles via FixCyclesFromComponents before expansion runs. The new NoSync expansion writes are hardened by the unconditional Flush in Engine.Close, and the scratch/proto aliasing in PutExpandedGrantRecords is safe because each record is marshaled within its loop iteration before prior is reset. The change is well covered by parity, differential, resume, and SQLite-parity tests. Note for reviewers: this makes the topological projection evaluator the default expansion path for all Pebble-backed syncs.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

pkg/sync/expand/topological_merge_streaming.go:326 — the per-destination k-way merge loop has no in-loop ctx.Err() check; a single whale destination can run a long time ignoring a cancelled context between flush boundaries.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/sync/expand/topological_merge_streaming.go`:
- Around line 326: The k-way merge loop in mergeContributionGroupStreams has no
  context cancellation check. Cancellation only propagates at destination
  boundaries (driveTopological checkBudget) and at flush boundaries inside the
  sink (every expansionDirtyFlushChunk dirty grants). A destination that reads
  millions of input principals but emits few dirty grants can run for a long
  time after the context is cancelled. Add a periodic ctx.Err() check near the
  top of the loop body so cancellation is honored promptly.

github-actions

No blocking issues found.

github-actions · 2026-06-16T15:56:07Z

+// entitlement_id|principal_rt|principal_id|external_id, and pagination walks it
+// in key order, so consumers can group by principal without buffering and
+// sorting the whole entitlement.
+func (a *Adapter) GrantsForEntitlementPrincipalSorted() bool { return true }


🟡 Suggestion: This method is inserted between ListGrantsForEntitlement's doc comment (lines 169-172) and the function itself, so that "ListGrantsForEntitlement paginates grants..." comment now attaches to GrantsForEntitlementPrincipalSorted, and ListGrantsForEntitlement (line 181) is left with no doc. Consider moving this method above the ListGrantsForEntitlement doc block, or inserting a blank line so each comment sits with its function.

github-actions

No blocking issues found.

github-actions

No blocking issues found.

github-actions

No blocking issues found.

btipling

Deep-arbiter review auto-submitted to make draft comments live for editing. Inlines had empty bodies due to extraction bug in the review tool; being PATCHed to full text in the next steps of this run. Verdict: NEEDS-WORK (WIP). Full details in the (updated) comment bodies and review body.

btipling · 2026-06-16T17:01:52Z

+	streams []contributionGroupStream,
+) ([]*v2.Grant, error) {
+	for _, stream := range streams {
+		defer stream.close()


[nit] defer stream.close() is written inside a range loop over the streams slice inside mergeContributionGroupStreams. (Classic defer-in-range pattern, even if 1.22+ iteration semantics make the var capture safe here.)

Suggestion: Hoist with explicit capture (for i := range streams { s := streams[i]; defer s.close() }) or a helper that closes the slice at the end of the func (after the early error returns). Same pattern noted on ancestor.

btipling · 2026-06-16T17:01:52Z

+func (c *topoContribution) add(sourceEntitlementID string, isDirect bool, principal *v2.Resource) {
+	c.addSource(sourceEntitlementID, isDirect)
+	if c.principal == nil && principal != nil {
+		c.principal = proto.Clone(principal).(*v2.Resource)


[nit] Unchecked proto.Clone casts remain in the new topo code: c.principal = proto.Clone(principal).(*v2.Resource), byID[id] = proto.Clone(grant).(*v2.Grant) (404), grant := proto.Clone(baseGrant).(*v2.Grant) (446). A store returning an unexpected concrete type (or future proto change) will panic rather than a typed error. Same sites existed on ancestor; now on hot topo paths.

Suggestion: Introduce a small safeClone helper in the expand package (or reuse existing) that does the , ok := ... check and returns error. Apply at these three sites (and audit pre-existing clones in the same file if they are now on expansion hot paths).

btipling · 2026-06-16T17:01:52Z

+	}
+	defer os.RemoveAll(tempDir)
+
+	projDB, err := pebble.Open(filepath.Join(tempDir, "db"), &pebble.Options{})


[suggestion] projDB is opened with &pebble.Options{} (engine defaults, no memtable/WAL/cache tuning) inside a plain os.MkdirTemp("baton-expand-projection-*"). buildProjectionDB does full materialization of projectionSources into kvs then one Ingest; later addProjectionRows use NoSync batches. No explicit Flush between phases beyond what Ingest provides, no size accounting or budget, and defer projDB.Close() + RemoveAll is the only cleanup (correct on happy path but the temp pebble is a whole second engine instance with its own invariants).

Suggestion: Document the memory envelope (same order as the chosen projectionSources grants). Consider explicit Options (DisableWAL: true, etc.) or a bounded cache. Add a comment confirming Ingest + later batch visibility to iters on the same handle. Ensure iterator closes on all error paths inside build/add (current defers help). Consider whether the temp DB abstraction could be narrower than a full *pebble.DB.

github-actions

No blocking issues found.

github-actions

No blocking issues found.

github-actions · 2026-06-16T21:07:24Z

+	sink destinationSink,
+) error {
+	for _, stream := range streams {
+		defer stream.close()


🟡 Suggestion: defer stream.close() discards the error from projectionContributionStream.close(), which returns the Pebble iterator's Close() error. A read error surfaced only at iterator close would be silently dropped, so a partial/corrupt projection read could go unreported. Consider capturing it into a named return error when the function isn't already returning one.

github-actions

No blocking issues found.

github-actions · 2026-06-16T22:34:39Z

+	// for a given entitlement. The topological evaluators require it. Pebble's
+	// by_entitlement index satisfies it; SQLite (orders by grant id) and the
+	// in-memory test doubles do not and report false.
+	GrantsForEntitlementPrincipalSorted() bool


🟡 Suggestion: Adding GrantsForEntitlementPrincipalSorted() bool to the exported ExpanderStore interface is a compile-time breaking change for any external code that implements ExpanderStore and calls the exported NewExpander. If that surface is intended to be public, consider documenting this as a breaking change or providing a default via a type assertion (as expanderStoreAdapter already does) rather than a required method.

github-actions

No blocking issues found.

…rant expansion evaluator... Replace the source-batched expander with a destination-first topological evaluator that reduces each destination entitlement exactly once by sort-merging its base grants with every parent source stream on the principal key. This removes the per-principal descendant existence probes and the repeated destination rewrites that dominated high fan-in graphs. Includes the streaming (memory-bounded k-way merge) and projection (temporary sorted Pebble scratch DB read via range scans, with a feedback loop so finalized grants feed descendants) variants, plus a graph-shape expansion planner. Pebble hot-path allocation cuts: - StoreExpandedGrants arena-allocates the v3 record/entitlement/principal structs in contiguous backing arrays instead of 3 heap allocs per grant. - codec.DecodeTupleStringAlias decodes index-key components without the intermediate []byte alloc on the common no-escape path. - Share key upper-bound logic via codec.KeyUpperBound. Validated with differential, SQLite-parity, resume, and benchmark tests.

github-actions · 2026-06-16T23:16:53Z

+	}
+
+	flusher := newDirtyFlusher(sink)
+	for h.Len() > 0 {


🟡 Suggestion: This k-way merge loop has no ctx.Err() check. driveTopological polls checkBudget between destinations, and cancellation reaches StoreExpandedGrants at each flush boundary (every expansionDirtyFlushChunk=10000 dirty grants), but a single whale destination that reads millions of input principals while producing few dirty grants can run for a long time ignoring a cancelled context. Consider a periodic if err := ctx.Err(); err != nil { return err } inside the loop.

github-actions

No blocking issues found.

kans requested a review from a team June 16, 2026 06:50

kans changed the title ~~[wip] Kans/provenance semirings~~ [wip] Kans/provenance semirings (algo improvements for expansion) Jun 16, 2026