injective keys - 75%+ faster expansion, 50% reduction in file size, massive allocation reduction && Soundness by kans · Pull Request #993 · ConductorOne/baton-sdk

kans · 2026-07-01T04:48:12Z

Summary

This branch replaces the Pebble engine's record addressing — previously keyed by the lossy :-joined external-id string — with structural identity keys derived only from the structured fields records already carry, and layers on the write-path, expansion, and lifecycle work that the new keyspace enables. External ids are unchanged as an external contract: baton stores and emits them byte-identical to what connectors produce, and never parses them to decide identity.

Structural (injective) identity keys

Identity from parts, not strings. Entitlement identity is (resource_type, resource_id, flag, tail) where the flag records pure byte-prefix compression (an id literally starting with rt:rid: stores only the tail); grant identity is the entitlement identity plus principal refs. The mapping is bijective given (rt, rid), which fixes the injectivity hole where distinct records could fold to one key or silently overwrite each other. One derivation function is shared by writes, reads, migration, and bulk import, so primary/index divergence is impossible by construction. Design doc: docs/tasks/raw-component-identity.md.
Bare-id lookups become query planning. Readers that only have an id string (GetGrant, GetEntitlement, delete-by-id) resolve it by combinatorial candidate probing with an exactly-one rule — one hit wins, zero is NotFound, multiple is an ambiguity error — with a lazy in-memory id map as the fallback for opaque ids. A miss is a miss, never a mis-keyed write. Probing is confined to interactive edges (CLI, explorer); sync and expansion always carry refs.
Duplicate-identity merge. Legacy rows with distinct external ids but one structural identity are field-merged during migration and bulk import with a commutative, fold-order-independent winner rule (earliest discovered_at, tie-break smallest external id), warned about, and reflected in computed stats.
Migration + format stamping. An in-place id-index migration re-keys main-format pebble files from value fields. The engine-meta stamp is authoritative; a new advisory pebble_id_index_format manifest field lets tooling inspect legacy-vs-structured state without extracting the payload.

Write-path and index restructuring

The by_principal index family is no longer maintained inline: it is excised and rebuilt from the primary keyspace at EndSync via a spill-sorted deferred build (with a durable pending marker so a crash+resume still runs it). The rebuild permutes primary-key bytes directly rather than decode/re-encode, eliminating ~7GB of allocations per whale build.
Bulk import gains an entitlement spill sorter and duplicate-resolving k-way merge; a sources synth-encode fast path avoids full proto round-trips.

Grant expansion pipeline

Topological spill sort for grant expansion with streaming merge and projection — roughly 50% fewer allocations on the whale benchmark.
Expansion writes commit NoSync and are hardened by the single EndSync flush; an explicit compaction pass runs at the end of expansion.

Sync lifecycle: sealing and compaction scheduling

Explicit sealed phase after EndSync. Fixes races where writes could interleave with finalize steps and index creation could be dropped: a sealed engine rejects normal writes, and only designated finalize-path writes (sync-run metadata, deferred-index cleanup) use allow-sealed variants.
Pausable compaction scheduler. Background compactions are paused during bulk ingestion (data volume alone spuriously tripped thresholds hundreds of times) and in the EndSync-to-close window, where a fresh-sync store is saved and closed moments later — on the whale benchmark that window ran 200+ compactions (~2 minutes of CPU/IO) whose output never survived to the saved artifact. In-flight compactions finish; flushes are unaffected.

Testing

End-to-end fuzz tests over the sync/expansion pipeline.
Property tests for identity ↔ external-id bijectivity against adversarial ids (ARN-style colon-bearing resource ids, rt:rid:-prefixed opaque ids, empty tails).
Migration-semantics tests (duplicate folds, missing-ref skips), lookup-tier tests, sealed-lifecycle and deferred-index resume tests, and SQLite-parity/differential suites.

github-actions · 2026-07-01T05:03:20Z

General PR Review: injective keys - 75%+ faster expansion, 50% reduction in file size, massive allocation reduction && Soundness

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 380d00189619.
Review mode: incremental since a59f234
View review run

Review Summary

The new commit ("adds logging during the migration") adds a 15s-cadence progress log to the grant id-index migration loop plus two migration test files (export_test.go test-only key/marshal exports, and store_migration_roundtrip_test.go exercising the legacy-envelope -> writable-open -> re-fence -> read-only-reopen path). The full PR diff was scanned for security and correctness: the proto change (new PebbleIdIndexFormat field 42) is additive with matching generated code and no field renumbering, and the pebble2 -> pebble3 manifest fence is read-compatible (old name accepted, never written). No new security or correctness issues were introduced by the new commit; the three prior suggestion-level findings are on code untouched by this commit and are not re-raised.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

pkg/dotc1z/engine/pebble/id_index_migration.go:204 -- the new progress/cancellation gate keys on rows (added-row count), which freezes across ref-missing skips, so a long skip run can suppress both ctx.Err() checks and progress logging; gate on the raw loop count instead.

Prompt for AI agents

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

## Suggestions

In `pkg/dotc1z/engine/pebble/id_index_migration.go`:
- Around line 204: The periodic-work gate `if rows&0xFFFF == 0` uses `rows`, which
  is only incremented for successfully re-keyed grants (line 232) and NOT for rows
  skipped due to missing refs (line 221). If a large contiguous run of grant rows is
  skipped, `rows` stays fixed and the gate stops firing, suppressing both the
  `ctx.Err()` cancellation check and the 15s progress log for the whole run -- the
  exact silent-open situation this commit is meant to prevent. Introduce a separate
  raw loop counter (incremented on every iteration, before the skip check) and gate
  the periodic block on that counter instead of on `rows`.

github-actions

No blocking issues found.

github-actions

No blocking issues found.

github-actions · 2026-07-01T15:09:19Z

 message Grant {
  c1.connector.v2.Entitlement entitlement = 1 [(validate.rules).message = {required: true}];
  c1.connector.v2.Resource principal = 2 [(validate.rules).message = {required: true}];
+  // These ids may not map one to one with the grant itself.


🟡 Suggestion: The generated pb/c1/connector/v2/grant.pb.go rawDesc in this PR marks field id = 3 as deprecated=true (the added \x18\x01 option bytes), but this .proto source only adds a comment — it does not declare deprecated = true on the field. This is generated-artifact drift: running make protogen would drop the deprecated flag. If the intent is to deprecate id, add deprecated = true to the field options here and regenerate; otherwise the generated file shouldn't carry it. Confidence: high (mismatch), low blast radius.

github-actions

No blocking issues found.

github-actions · 2026-07-02T02:18:56Z

+		// isGranting serializes granting from Done and the periodic granter,
+		// and lets Unregister wait out an in-flight grant loop.
+		isGranting                   bool
+		isGrantingCond               *sync.Cond


🟡 Suggestion: pausableCompactionScheduler reimplements pebble's ConcurrencyLimitScheduler with non-trivial concurrency (the isGranting handoff, Unregister waiting out an in-flight grant loop, Done/periodic-granter/TrySchedule all mutating runningCompactions). It has no direct unit test — only exercised indirectly via the env-gated whale benchmark. A focused test covering pause during an in-flight grant, resume-poke pickup, and Register/Unregister ordering would guard this against future pebble-version bumps. Same applies to the new Begin/Add/Finish/Abort layer session (segment cut, worker-error propagation, abort mid-wave), only reached with BATON_PEBBLE_SYNTH_LAYER_SST set.

github-actions

No blocking issues found.

github-actions

No blocking issues found.

github-actions · 2026-07-02T19:32:09Z

+	for k := 0; k < len(entitlementID); k++ {
+		if entitlementID[k] != ':' {
+			continue
+		}
+		for l := k + 1; l < len(entitlementID); l++ {
+			if entitlementID[l] != ':' {
+				continue
+			}
+			cand := entitlementIdentity{
+				resourceTypeID: entitlementID[:k],
+				resourceID:     entitlementID[k+1 : l],
+				stripped:       true,
+				tail:           entitlementID[l+1:],
+			}
+			nonEmpty, err := e.grantPrimaryPrefixNonEmpty(encodeGrantPrimaryEntitlementPrefix(cand))
+			if err != nil {
+				return entitlementIdentity{}, err


🟡 Suggestion: This direct-split fallback opens a fresh Pebble iterator (grantPrimaryPrefixNonEmpty) for every O(colons²) candidate with no upper bound, unlike resolveGrantIdentityByExternalID which caps enumeration at maxGrantIDCandidates. An entitlement id with many colons and no matching entitlement record would open a large number of iterators on this bare-id edge. Consider applying a similar cap here for consistency and robustness. (low confidence)

github-actions

No blocking issues found.

github-actions

No blocking issues found.

github-actions · 2026-07-06T21:30:49Z

+		if s.sorter == nil {
+			if err := e.initSynthLayerSession(ctx, s); err != nil {
+				return err
+			}
+			// Segment SSTs carry primary rows only; make sure EndSync builds
+			// the deferred by_principal index even if no batch write set the
+			// flag.
+			if err := e.markDeferredIdxPending(); err != nil {
+				return err
+			}


🟡 Suggestion: initSynthLayerSession sets s.sorter and starts the worker goroutine before returning, so if markDeferredIdxPending() fails here (after init succeeded), the session is left initialized (s.sorter != nil) with a live worker, but the durable deferred-index marker is unarmed and its CAS rolled back. A subsequent Add skips the if s.sorter == nil block and never re-attempts the marker, so EndSync could silently skip building the synth by_principal index. Consider arming the marker before initSynthLayerSession (or tearing the session down on marker failure) so the failure is fully retryable. (confidence: low)

github-actions

No blocking issues found.

github-actions · 2026-07-06T22:17:04Z

+	err := e.IterateAllSyncRuns(ctx, func(r *v3.SyncRunRecord) bool {
+		syncID = r.GetSyncId()
+		return false
+	})


🟡 Suggestion: This yield returns false on the first record, and IterateAllSyncRuns stops iteration on false, so syncID binds to the first (earliest KSUID) sync run, not the latest. If a legacy c1z ever carries multiple sync-run records, post-migration stats are recomputed against an arbitrary/earliest sync rather than the current one. Low impact today since the Pebble primary keyspace holds a single generation, but the control flow reads as "some sync" when the intent is "the current sync." (confidence: medium)

…-keys

github-actions

No blocking issues found.

github-actions

No blocking issues found.

ggreer

Approved so that this can be merged at your leisure (once the tests pass).

github-actions · 2026-07-07T15:41:57Z

+// combinatorial prober above answers SDK-shaped ids without ever getting
+// here), never from sync or expansion. Exactly-one rule: zero matches is
+// pebble.ErrNotFound, several is ErrAmbiguousExternalID.
+func (e *Engine) scanGrantIdentityByStoredExternalID(ctx context.Context, grantID string) (grantIdentity, error) {


🟡 Suggestion: This fallback changes grant-id resolution behavior — a connector-custom id that previously returned NotFound in O(1) now triggers an O(all grants) full-keyspace scan on every miss. The code comment correctly bounds this to the CLI/reader-API edge, but worth confirming no reader-API caller resolves unknown/custom ids in a loop (each miss is now a whale-sized scan). The change itself improves SQLite parity and is well-guarded (context checks, exactly-one rule). (low confidence)

github-actions

No blocking issues found.

github-actions

No blocking issues found.

github-actions

No blocking issues found.

github-actions

No blocking issues found.

github-actions · 2026-07-07T22:38:44Z

+	lastLog := start
+	var rows, skippedMissingRefs int64
+	for iter.First(); iter.Valid(); iter.Next() {
+		if rows&0xFFFF == 0 {


🟡 Suggestion: The rows&0xFFFF == 0 gate uses rows, which only increments for successfully re-keyed grants (line 232), not for skipped rows (line 221). A long contiguous run of ref-missing skips leaves rows frozen at a value where the gate is false, suppressing both the ctx.Err() cancellation check and the progress log for the duration of that run — the exact silent-open scenario this commit targets. Gating on the raw loop count (or checking iter-level progress) instead of the added-row count would keep both behaviors firing. Low severity: skipped grants are pathological, so in practice the gate fires steadily. (confidence: medium)

github-actions

No blocking issues found.

...

wip

e1b70d4

kans requested a review from a team July 1, 2026 04:48