drop inline graph writer - #1130
Conversation
No code path can now produce a sync token containing "entitlement_graph", and every code path can still consume one. The reader is untouched: tokens written before #1019 carry an inline graph and must still resume. Removed WithEntitlementGraphInCheckpoints, cfg.checkpointEntitlementGraph, the withCheckpointEntitlementGraph stateOpt, and state.checkpointEntitlementGraph. Marshal's graph drop and expansion page-token blanking are now unconditional; the normalization itself is unchanged, including that only the serialized copy is normalized while the live state keeps its page token. withCheckpointEntitlementGraph was the only stateOpt, so newState takes no arguments and the option type goes with it rather than staying as a constructor mechanism with zero options. The option had no callers: none in this repo, none in the c1 monorepo, none across the baton-* connector repos. It has never been enabled by anything. Its tradeoff was also asymmetric. #1019 stopped serializing the graph because doing so OOM-killed sync workers twice in production, with state.Marshal at 6.4GB cumulative of a 13.5GB heap against a live graph of ~0.9GB. The option was then added for a tenant whose expansion "could fail to converge", which was never observed; loadEntitlementGraph rebuilds the graph from PendingExpansionPage with no connector calls, so what it preserved cost seconds of rework even on very large c1zs. Tests: TestSyncerTokenIncludesEntitlementGraphWhenEnabled and TestWithEntitlementGraphInCheckpointsReachesState are deleted with the option they cover. The reader assertions in the former's tail (graph restored with Depth and Nodes intact, expansion page token preserved) are already made by TestSyncerTokenLegacyInlineGraphStillDecodes, so no coverage moves. Three reader tests built their legacy token by turning the writer on — the compatibility matrix, TestGraphFromToken, and TestPrepareExpansionReplayToken_ClearsPreservedGraph — and now encode serializedTokenV1 directly through marshalLegacyInlineGraphToken. goldenTokenCases sheds the v1_inline_graph.json self round-trip, which was the writer; the v1_inline_graph.json -> v1_inline_graph.dropped.json case and the dropped self case stay, so no fixture is orphaned. Validated against three planted mutants. Restoring the graph into the serialized struct and dropping the page-token blanking each fail TestGoldenTokenRoundTrip on the v1_inline_graph.json -> .dropped.json case, the second on the action map. Making Unmarshal drop the inline graph fails all four reader tests plus TestGoldenTokenHelpers, so the rewritten reader tests are not tautologies through the deleted writer. Ride-along: two comments claimed preserveEntitlementGraph keeps the graph in the final token when the store lacks the sidecar capability. That branch cannot run. Its only caller gates on c1zstore.EnginePebble (synccompactor/compactor.go:1327) and pebbleStore is the only implementer of either EntitlementGraphStore or c1zstore.GrantGenerationDigestReader, so the capabilities are always present. Both comments now describe the sidecar as the only path; no fallback added. No big-O change: Marshal loses one branch. pkg/sync/checkpoint_cost_bench_test.go needs no new ratchet. Fixture README: v1_inline_graph.json moves back to hand-authored input, because this commit deletes the writer that produced it, and the two table rows that named the "opt-in" and "default" writers no longer describe anything — there is one writer now. The companion commit classified it as recorded output, which was correct only while the option existed. CXE-1376 Co-authored-by: Cursor <cursoragent@cursor.com>
29576f5 to
d124cfa
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
General PR Review: drop inline graph writerBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe new commit Risk triage (per Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
Both described a fallback the inline-graph writer used to provide. With that writer gone, a preserved graph without a sidecar has nowhere to go rather than landing in the final sync token as bloat, and state.Marshal drops the graph from every token it writes rather than by default. Co-authored-by: Cursor <cursoragent@cursor.com>
Both said an in-token graph came from WithPreserveEntitlementGraph. That option writes to the c1z sidecar, and with the inline writer gone only a pre-omission SDK puts a graph in a token. Also record what actually enforces the replay contract. Marshal drops the graph from every token it writes, so ClearEntitlementGraph in PrepareExpansionReplayToken cannot change the emitted bytes: removing that call leaves the whole pkg/sync suite green. The call stays to keep the contract stated where the token is rewritten, and the test comment now says no assertion pins it. Renames the test from ClearsPreservedGraph to DropsInlineGraph: the graph is not preserved, and the test does not pin the clear. Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Deletes
WithEntitlementGraphInCheckpointsand the inline-graph checkpointwriter behind it.
state.Marshalnow drops the entitlement graph and blanks theexpansion page token unconditionally, instead of doing so only when the option
was unset.
Why this is safe
The option had no callers. A sweep of 65 ConductorOne repos under
~/workplusthe
c1monorepo found zero call sites in first-party code, in the current treesor in
git log -Shistory — every text hit is vendored SDK source. It was alsonever reachable except by a direct Go call: nothing in
pkg/cli,pkg/config,pkg/field,pkg/connectorrunner, orpkg/tasksever plumbed it to a flag orconfig field, so no connector could have enabled it through config. Inside the
SDK the only invocation was
TestWithEntitlementGraphInCheckpointsReachesState,which this change removes.
It is a public API in a v0.x module, released in
v0.23.0throughv0.29.0(2026-08-11 to 2026-09-04), so a third-party caller cannot be ruled out. It is
deleted rather than kept as a deprecated no-op deliberately: the behavior is
intentionally unavailable rather than relocated, so there is no replacement call
to migrate to, and a no-op would leave a caller believing they still have an OOM
remedy. A compile error is the honest signal and the fix is deleting one line.
Reader compatibility is unchanged
Only the writer is gone. Tokens written by pre-omission SDKs still decode with
the graph restored and pagination preserved. The reader tests that used to build
those bytes by enabling the option now hand-craft them through a
marshalLegacyInlineGraphTokenhelper, soTestSyncerTokenLegacyInlineGraphStillDecodes,TestGraphFromToken, and the graph compatibility matrix keep their coverage. Thegolden token corpus added in #1129 pins the wire format on bytes across this
change.
Accepted risk
The option was introduced as a break-glass knob for a tenant whose grant
expansion cannot finish within one worker or activity lifetime, on the theory
that checkpointing the graph would let expansion resume mid-run. On Pebble it
never did.
RunSingleSteproutes toRunTopologicalMergeProjectionwhenever thestore yields principal-sorted grants, which is the Pebble adapter;
driveTopologicalLayernever consultsedge.IsExpanded, andMarkExpansionCompleteflips every edge at once only after the full projectionreturns clean. A Pebble worker that dies at 90% of the projection comes back with
every edge unexpanded whether or not the graph was in the token. That has been
true since #965 landed the projection on 2026-06-16, six weeks before the option
was written.
Mid-expansion resume only ever existed on the source-batched expander that
SQLite and the in-memory doubles use, where
RunSingleStepretires one queuedgraph.Actionsentry per call and marks that action's edges expanded as it goes.So the exposure is writable syncs over not-yet-converted v1 files, which
selectStoreDriverdeliberately keeps on SQLite when opened without an explicitengine. That exposure already exists on
main: #1019 made graph omission thedefault on 2026-08-10 and nothing has set the option since, so no sync has been
getting inline-graph checkpoints for a month. This change deletes an unused
remedy rather than changing behavior.
The intended path for such a tenant is conversion to Pebble via an explicit
WithEngine(EnginePebble), which makes expansion fast enough that resume stopsmattering. Persisting the whole graph into every checkpoint is what caused the
OOM that #1019 fixed.
Ride-alongs
pkg/sync/syncer.gothat claimedpreserveEntitlementGraphfalls back to inline token storage when the storelacks the sidecar capabilities. There was never such a fallback in code — the
claim was only reachable through the option this change deletes.
pkg/synccompactorfor the same reason: a preservedgraph without a sidecar has nowhere to go rather than bloating the final token,
and
state.Marshaldrops the graph from every token rather than by default.stateOptandnewState's variadic parameter, which had no remainingimplementations.