fix: key session store by sync_id to prevent concurrent sync clobbering

[c1-dev-bot]

Summary

GRPCSessionServer previously held a single mutable store pointer shared across all concurrent sync tasks. In service mode with task-concurrency > 1, when a second sync_full task (full or targeted) started mid-flight, SetSessionStore overwrote the pointer. The first sync's reads were then silently served from the wrong c1z file, returning (nil, false, nil) — indistinguishable from "never written." This caused dropped grants that the platform reconciled as revocations.

Changes

• pkg/types/sessions/sessions.go: Extended SetSessionStore interface to accept syncID and added RemoveSessionStore for cleanup.
• pkg/session/session_server.go: Replaced single store pointer with a sync.RWMutex-guarded map[string]SessionStore keyed by syncID. Each RPC resolves its backing store via req.GetSyncId() (already threaded through the wire protocol). Added RemoveSessionStore for teardown.
• internal/connector/connector.go: Updated connectorClient to forward syncID through SetSessionStore/RemoveSessionStore.
• pkg/sync/syncer.go: Moved session store registration from loadStore (where syncID is not yet known) to Sync() after startOrResumeSync returns the syncID. Added RemoveSessionStore call in Close() to prevent memory leaks.
• pkg/session/session_server_test.go: Added regression tests verifying concurrent syncs each read their own data, removal prevents stale access, and unregistered syncIDs fail loudly instead of silently returning empty.

Trigger sequence (before this fix)

A: full sync starts  -> SetSessionStore(storeA)         [global ptr = storeA]
A: writes under sync_id=A
B: targeted sync starts -> SetSessionStore(storeB)      [SWAP — storeA gone]
A: Get(sync_id=A, ...) -> served from storeB -> (nil, false, nil)
A: emits zero grants (silent) -> task SUCCESS
C1: previously-synced grants now absent -> reconciled as revoked

After this fix

Each sync registers its store keyed by syncID. RPCs resolve the correct backing store via the request's sync_id. Concurrent syncs never interfere with each other.

Fixes: CE-822

---

Automated PR Notice

This PR was automatically created by c1-dev-bot as a potential implementation.

This code requires:

• Human review of the implementation approach
• Manual testing to verify correctness
• Approval from the appropriate team before merging

[linear-code]

CE-822

[github-actions]

🟡 Suggestion: This changes the exported SetSessionStore interface signature (adds syncID) and adds a new RemoveSessionStore method, which breaks any external implementer of this interface. In practice the only implementers/consumers are SDK-internal (connectorClient, GRPCSessionServer, the syncer), so impact is likely low, but per the repo's compatibility criteria a breaking SDK contract change should be signaled with a 0.x minor bump in pkg/sdk/version.go (still v0.15.5) and noted in the PR. (confidence: medium)

[github-actions]

General PR Review: fix: key session store by sync_id to prevent concurrent sync clobbering

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base d50c4ba06e17.
Review mode: full
View review run

Review Summary

Scanned the full PR diff for security and correctness. The change replaces the single shared store pointer in GRPCSessionServer with a map[syncID]SessionStore guarded by an RWMutex, moves session-store registration from loadStore to Sync (after the syncID is known), and adds RemoveSessionStore wired into syncer.Close. The keying-by-syncID and mutex correctly resolve the concurrent-sync clobbering and the data race; the nil-map risk is avoided since GRPCSessionServer is only built via NewGRPCSessionServer, Close is invoked on both success and error paths so registrations are cleaned up, and SkipSync (the other loadStore caller) never used the session store. New tests cover concurrency, isolation, removal, and unregistered/clear paths. No blocking issues found.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• pkg/types/sessions/sessions.go:75-78: Exported SetSessionStore interface signature change + new RemoveSessionStore method is a breaking SDK contract change; signal with a 0.x minor bump in pkg/sdk/version.go (still v0.15.5) and a PR note.

Prompt for AI agents

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

## Suggestions

In `pkg/types/sessions/sessions.go`:
- Around line 75-78: The exported `SetSessionStore` interface changed signature
  (added `syncID string` to `SetSessionStore`) and gained a new `RemoveSessionStore`
  method. The same applies to the parallel `session.SetSessionStore` interface in
  `pkg/session/session_server.go`. This breaks any external implementer of these
  interfaces. Since this is a breaking SDK contract change in a pre-1.0 repo, bump
  `pkg/sdk/version.go` (currently `v0.15.5`) by a 0.x minor version to signal the
  break, and describe the contract change in the PR description.

[c1-dev-bot]

Checking lint failure - will push a fix if needed.

[github-actions]

No blocking issues found.

[kans]

This is the wrong solution to the problem. The backend tries really hard to not schedule concurrent syncs of any type because we dont' support it in the sdk. If the SDK gets parallel sync tasks, it should error one of them, or reject it, or hold on to it until it finishes another one (or something). This possibly requires some changes in c1.