sync: omit the entitlement graph from checkpoint tokens#1019
Conversation
General PR Review: sync: omit the entitlement graph from checkpoint tokensBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryScanned the full PR diff (only Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
state.Marshal serialized the entire entitlement graph into every checkpoint. For large tenants that JSON encoding multiplied the checkpoint's memory footprint several times over — the json map encoding, the string copy, and the store's record/compression/batch copies are each O(graph) and live at once. In production this OOM-killed sync workers mid-checkpoint (be-temporal-sync, twice: 2026-07-17 at a 24G limit and 2026-07-18 at 30G; the final pre-OOM heap profile shows state.Marshal at 6.4GB cum of a 13.5GB heap, with a live graph of only ~0.9GB). The graph is a projection of data already in the store: loadEntitlementGraph rebuilds it from PendingExpansionPage with no connector calls, and PrepareExpansionReplayToken already relies on whole expansions being re-runnable from the store. So checkpoints now omit it, and a crash mid-expansion re-runs the load and expansion phases on resume instead of resuming them from the token. Because a resumed reader gets no graph, an in-flight SyncGrantExpansionOp action's PageToken is blanked in the serialized copy — resuming that pagination against a fresh graph would silently drop the edges from earlier pages. Blanking at marshal time keeps graph-less tokens safe for older readers too (rollback mid-sync); Unmarshal additionally normalizes orphan page tokens from writers that didn't. Tokens written by older SDKs still carry the graph inline and decode + resume exactly as before. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review follow-ups: exercise both blanking paths with a mixed action map (non-expansion pagination must survive, no entries dropped), pin the upgrade chain (legacy inline-graph token in -> next checkpoint out must be graph-less with the expansion page token blanked), and cover orphan blanking through the V0 fallback decode path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
994cdaf to
4afa50c
Compare
1 participant
Problem
state.Marshalserializes the entire entitlement graph into every checkpoint token (every ≥10s during a sync, including once per page while the graph is being loaded). For large tenants this is what has been OOM-killingbe-temporal-syncin prod — twice in the last day, first at a 24 G limit (2026-07-17 23:06Z) and again at 30 G after the limit was raised (2026-07-18 02:04Z, image 121645).The last pre-OOM heap profile (22s before the second kill) shows the marshal path owning roughly 11 GB of a 13.5 GB heap for a live graph of only ~0.9 GB:
Every copy is O(graph) and several are live simultaneously, so one sufficiently large tenant checkpointing is enough to kill a worker — and since the OOM lands mid-checkpoint, the sync crash-loops: the checkpoint meant to preserve progress is the thing preventing completion.
Change
Checkpoints no longer carry the graph. It is a projection of data already in the store —
loadEntitlementGraphrebuilds it fromPendingExpansionPagewith no connector calls, andPrepareExpansionReplayTokenalready relies on whole expansions being re-runnable from the store on finished syncs. This extends that same replay property to interim checkpoints: a crash mid-expansion re-runs the load + expansion phases on resume instead of resuming them from the token.Checkpoint memory becomes O(actions + stats) — effectively KBs — independent of tenant size.
Resume-safety detail
A resumed reader gets no graph, so an in-flight
SyncGrantExpansionOpaction'sPageTokenis blanked in the serialized copy (live state is untouched): resuming that pagination against a fresh graph would silently drop the edges from earlier pages. Doing this at marshal time keeps graph-less tokens safe for older readers too (version rollback mid-sync ⇒ the old SDK sees a fresh graph + empty page token ⇒ clean full reload, not silent corruption).Unmarshaladditionally normalizes orphan page tokens defensively.Note the load phase leaves the final page token on the action even after
Loaded=true, so the blanking applies uniformly across phases.Compatibility
Nothing outside
pkg/syncconsumes the token's graph:ClearEntitlementGraphalready stripped it from every finished sync's token, and c1 passesSyncTokenthrough opaquely.Trade-off
Crash/restart mid-expansion now redoes the graph load + expansion (store-local reads and idempotent
StoreExpandedGrantsupserts, no connector traffic) — minutes of local recompute in the rare-restart case, versus a guaranteed OOM-loop for large tenants today. Small tenants redo little; large tenants are exactly the ones that couldn't get through a checkpoint at all.Stated plainly: cross-restart expansion progress is now zero. A tenant whose entire expansion exceeds one worker/activity lifetime would livelock on repeated restarts, where before it could inch forward page by page (though for exactly those tenants the checkpoint itself was what OOM'd the worker, so the old "progress" was moot in practice). If that livelock is ever observed, the follow-up is a durable expansion progress marker persisted in the store — not re-inflating the token.
Testing
TestSyncerTokenOmitsEntitlementGraph— token carries neither graph nor in-flight expansion pagination; live state unmutated; resumed state restarts the load; non-expansion actions keep their pagination and no map entries are droppedTestSyncerTokenLegacyInlineGraphStillDecodes— old-style tokens restore the graph and keep their pagination; the next checkpoint after a legacy resume writes the new format (graph omitted, expansion pagination blanked) while the live state is unaffectedTestSyncerTokenUnmarshalBlanksOrphanExpansionPageToken— defensive normalization (mixed action map)TestSyncerTokenV0OrphanExpansionPageTokenBlanked— the defensive blanking also covers tokens decoded through the V0 fallback./pkg/sync/...suite green (incl. expand's topological-merge interrupt/resume tests, pebble fastpath, sqlite parity); token tests pass with-raceA syncer-level "interrupt exactly mid-expansion, resume, assert grant parity" e2e would need a new test hook (the existing interruption harness is connector-failure-based, and expansion makes no connector calls) — happy to add one as a follow-up if you want the hook.
🤖 Generated with Claude Code