fix(syncdeck): stop Learn entry mappings from expiring under live sessions - #332
Merged
Conversation
…sions Root-caused a class of session splits where an instructor's editor showed an active session but a student's launch landed in a waiting room, and a later reopen created a separate session: the active-entry TTL fallback (`sessions.ttlMs ?? WAITING_TTL_MS`) silently resolved to the 10-minute waiting TTL in production, since the Valkey-backed store never exposes a top-level `ttlMs`. On top of that, the entry's expiry was refreshed only by Learn REST polling, never by the live session's own websocket activity, so a genuinely busy class could still orphan its mapping. - Add resolveActiveEntryTtlMs() to check sessions.valkeyStore?.ttlMs before falling back, matching the existing statusRoute.ts idiom. - Add a generic linkedSessionId keepalive convention to the session store (mirrors embeddedParentSessionId): touch() now propagates to a linked record, so stamping the live SyncDeck session with its Learn entry's mapping id keeps the entry alive for as long as anyone stays connected, independent of Learn's polling cadence. - Add non-reversible identity-fingerprint logging (learn-identity-resolved) across status/student-entry/start/stop so a future split is traceable without logging raw provider/resourceLinkId/session identity, and document the exact algorithm plus a worked test vector in the shared Learn/ActiveBits integration plan so Learn can implement a matching scheme for cross-system correlation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughLearn SyncDeck now records HMAC-based identity fingerprints, resolves active-entry TTLs through session stores, and links live sessions to Learn entries. Session stores propagate touches to directly linked sessions. Tests cover lifecycle logging, privacy, expiry, and linkage. ChangesLearn SyncDeck integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LearnIntegration
participant SessionStore
participant LearnEntry
LearnIntegration->>LearnEntry: resolve mapping and log identity fingerprints
LearnIntegration->>SessionStore: create live session with linkedSessionId
SessionStore->>LearnEntry: refresh linked entry mapping
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@activities/syncdeck/server/learnIntegration.ts`:
- Line 623: Remove the raw resourceLinkId field from the
learn-instructor-session-start-pending log in the pending-start logging flow,
while retaining requestId, state, and the identityFingerprints output. Add or
update a test verifying that this event payload does not contain the raw
resource identifier.
In `@server/core/sessions.ts`:
- Around line 155-158: Make linked-session refresh propagation exactly one hop:
in server/core/sessions.ts lines 155-158, replace the recursive touch call with
a direct refresh of linkedSessionId; apply the same non-propagating direct-touch
behavior to cached and Valkey-backed records in server/core/sessions.ts lines
263-284. In server/sessionStore.test.ts lines 151-196, add chain and
two-record-cycle tests verifying only the direct linked record is refreshed and
touch() completes.
In `@server/sessionStore.test.ts`:
- Around line 172-195: Increase the timing margin in the test around
createSessionStore and the wait calls: use a substantially larger TTL and
proportionally longer delays so event-loop scheduling cannot expire the linked
session before the first cleanup assertion, while preserving the final cleanup
assertion that verifies expiration after the touched session’s TTL.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0141b127-3b5b-4c11-bae7-27e6e17ac4ff
📒 Files selected for processing (7)
.agent/knowledge/data-contracts.md.agent/knowledge/security-notes.md.agent/plans/learn-syncdeck-session-integration.mdactivities/syncdeck/server/learnIntegration.test.tsactivities/syncdeck/server/learnIntegration.tsserver/core/sessions.tsserver/sessionStore.test.ts
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes SyncDeck Learn entry expiration and adds privacy-preserving identity correlation.
Changes:
- Adds linked-session keepalive and production TTL resolution.
- Adds fingerprint-based integration logging and tests.
- Documents lifecycle and logging contracts.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
server/core/sessions.ts |
Adds single-hop linked-session touches. |
server/sessionStore.test.ts |
Tests linked-session behavior. |
activities/syncdeck/server/learnIntegration.ts |
Adds TTL resolution, linkage, and fingerprints. |
activities/syncdeck/server/learnIntegration.test.ts |
Tests integration behavior and logging. |
README.md |
Documents mapping keepalive. |
ARCHITECTURE.md |
Documents linked-session lifecycle. |
DEPLOYMENT.md |
Adds deployment guidance. |
.agent/plans/learn-syncdeck-session-integration.md |
Defines cross-system fingerprint contract. |
.agent/knowledge/security-notes.md |
Records security and expiry decisions. |
.agent/knowledge/data-contracts.md |
Records persistent integration contracts. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
server/core/sessions.ts:276
- The production Valkey implementation of linked-session propagation is not exercised by the added tests: every new
sessionStore.test.tscase usescreateSessionStore(null, ...), while the Learn test's mocktouch()only returnstrue. A regression in this cache/Valkey branch would therefore leave the original production failure undetected. Please add coverage that drives the wrapped-store path and verifies that touching a cached and uncached live record refreshes the linked record in Valkey while remaining single-hop.
const touchDirect = async (id: string): Promise<{ touched: boolean; session: SessionRecord | null }> => {
const cached = cache.getFresh(id)
if (cached) {
cache.touch(id)
return { touched: true, session: cached }
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/core/sessions.ts`:
- Around line 211-218: Update createSessionStore to replace the two plain
console.log calls for in-memory and Valkey store selection with structured JSON
logs, using stable event names and consistent fields that identify the selected
session-store type and relevant configuration.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6aa2437c-862c-4608-8366-651931f0a41f
📒 Files selected for processing (11)
.agent/knowledge/data-contracts.md.agent/knowledge/security-notes.md.agent/knowledge/testing-patterns.md.agent/plans/learn-syncdeck-session-integration.mdARCHITECTURE.mdDEPLOYMENT.mdREADME.mdactivities/syncdeck/server/learnIntegration.test.tsactivities/syncdeck/server/learnIntegration.tsserver/core/sessions.tsserver/sessionStore.test.ts
…erage for both modes.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (1)
activities/syncdeck/server/learnIntegration.ts:641
- The documented cross-system log contract says the pending-start variant includes
operation,reused, andsessionFingerprint, but this event omits all three. Consumers normalizinglearn-identity-resolvedand pending-start records therefore cannot rely on the schema described in the integration plan and data-contract notes. Emit the start operation,reused: false, and a null session fingerprint here (there is no session yet).
console.info(JSON.stringify({ activity: ACTIVITY_ID, event: 'learn-instructor-session-start-pending', requestId, ...identityFingerprints(auth.key.secret, provider, resourceLinkId, id), state: 'starting' }))
The event now includes: operation: "start" reused: false sessionFingerprint: null Added explicit regression assertions and updated the log contract note.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (1)
activities/syncdeck/server/learnIntegration.ts:350
- This back-reference is never cleared when Learn stops the session or when activation rolls back after this write.
session-endedonly sends a message; it does not close the WebSocket, so an old client can continue touching the stopped session. If the same mapping ID is recreated, that stale session then refreshes the new entry (and can retain its provider/resource data beyond the intended waiting TTL). ClearlinkedSessionIdon stop and on every failed activation path, or otherwise make the linkage lifecycle-bound to the active entry.
liveSession.data = { ...liveSession.data, linkedSessionId: entryMappingId }
await sessions.set(sessionId, liveSession)
linkedSessionId is cleared on Learn stop only when it still matches that entry mapping. Both normal and substitute instructor activation rollbacks clear a link created before the failure. Added stop-path coverage proving the matching link is removed. Documented the lifecycle contract.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
activities/syncdeck/server/learnIntegration.test.ts:699
- This no-raw-ID check only scans
infoLogs. The failure case above emitslearn-instructor-session-start-failedtoconsole.errorwith the rawfailedInstructorResourceId, so the test passes while a Learn lifecycle event still violates the stated fingerprint-only logging contract. IncludeerrorLogsin this assertion and fingerprint/scrub those failure events too.
console.info('[TEST] Verifying Learn lifecycle logs never leak raw resourceLinkId, sessionId, or handoff material.')
const learnLifecycleLogs = infoLogs.filter((message) => message.includes('"event":"learn-identity-resolved"') || message.includes('"event":"learn-instructor-session-') || message.includes('"event":"learn-integration-stop-noop"'))
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (2)
activities/syncdeck/server/learnIntegration.test.ts:699
- This filter does not verify the stated no-raw-identifiers guarantee. It excludes
learn-integration-request-failed, while the start/student-entry failure paths still pass rawresourceLinkIdand sometimessessionIdinto that event (for example, presentation mismatch and recovery-unavailable). Those incident logs can therefore still expose exactly the identifiers this test says are absent. Convert those contexts to fingerprints and include all Learn integration lifecycle/failure events in this assertion.
const learnLifecycleLogs = [...infoLogs, ...errorLogs].filter((message) => message.includes('"event":"learn-identity-resolved"') || message.includes('"event":"learn-instructor-session-') || message.includes('"event":"learn-integration-stop-noop"'))
server/core/sessions.ts:309
- This bypasses the cache optimization for every cached
touch():wsRoutercallstouch()on every WebSocket message and pong, so even sessions with no link now perform a ValkeyGETper event. High-frequency SyncDeck messages can therefore create many concurrent reads, while the cache was introduced specifically to batch keepalives. Preserve a bounded authoritative-link refresh (for example, track source-data freshness separately from touch freshness or invalidate links cross-instance) rather than re-reading Valkey on every touch.
const authoritativeSession = fromCache ? await loadSessionRecord(id) : session
…t fingerprints when authenticated identity is available; privacy coverage includes those logs. Cached Valkey touches revalidate source link data at most once per five seconds, then update the cache—preventing stale cross-instance links without a Valkey read on every websocket event.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agent/knowledge/data-contracts.md:
- Around line 755-758: Correct the future-dated knowledge entries in
.agent/knowledge/data-contracts.md:755-758 and
.agent/knowledge/data-contracts.md:760-764 by changing both August 9, 2026 dates
to August 8, 2026 or their actual record dates; no other content needs
modification.
In `@activities/syncdeck/server/learnIntegration.ts`:
- Around line 333-348: Update logLearnLifecycleError in
activities/syncdeck/server/learnIntegration.ts (lines 333-348) to accept and
serialize only an allowlisted lifecycle-error schema, using a stable error class
or code and excluding Error.message and arbitrary context values. In
activities/syncdeck/server/learnIntegration.test.ts (lines 699-704), add a
[TEST] marker before the intentional failure and assert captured logs exclude
sentinel resource, session, URL, and token values.
In `@server/core/sessions.ts`:
- Around line 229-230: Bound linkedSessionRevalidatedAt so it cannot retain
entries indefinitely as sessions are evicted or deleted. Integrate pruning with
the relevant session/cache lifecycle, or enforce a bounded-size eviction policy,
while preserving the existing revalidation behavior governed by
LINKED_SESSION_REVALIDATION_MS.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d68982bc-cef8-4166-986b-e553d32d8cc7
📒 Files selected for processing (9)
.agent/knowledge/data-contracts.md.agent/knowledge/testing-patterns.mdARCHITECTURE.mdDEPLOYMENT.mdactivities/syncdeck/server/learnIntegration.test.tsactivities/syncdeck/server/learnIntegration.tsactivities/syncdeck/server/routes.tsserver/core/sessions.tsserver/sessionStore.test.ts
Restricted lifecycle error logs to an allowlisted schema with stable error codes; sentinel resource, session, URL, and token values are verified absent. Bounded linkedSessionRevalidatedAt to 1,000 entries, pruned it with cache cleanup/deletes, and cleared it on close.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (3)
server/sessionStore.test.ts:12
- The test imports
../activities/syncdeck/server/routes.jsto accessnormalizeSyncDeckSessionData, but that module has top-level side effects (it callsregisterSessionNormalizer('syncdeck', ...)on import). This means unrelated tests earlier in this file run with the SyncDeck normalizer pre-registered, which can make the suite order-dependent and harder to reason about.
Prefer importing a side-effect-free normalizer (e.g. move/export normalizeSyncDeckSessionData from a dedicated helper module that does not self-register), or use a scoped dynamic import within the specific test and immediately reset/re-register the normalizer to avoid polluting global state.
import { registerSessionNormalizer, resetSessionNormalizersForTests } from './core/sessionNormalization.js'
import { listenForTest } from './testPortBinding.js'
import { normalizeSyncDeckSessionData } from '../activities/syncdeck/server/routes.js'
activities/syncdeck/server/learnIntegration.ts:904
- This unlink-failure log includes the raw error message. Even though this route avoids logging raw identifiers directly, upstream/store errors can embed session IDs, mapping keys, or URLs in their messages, which would violate the fingerprint-only logging goal of this PR. Prefer logging a stable errorCode + identity fingerprints (like the other Learn lifecycle errors) and omit the raw error text.
try {
if (sessionId) await unlinkLiveSessionFromEntry(sessions, sessionId, id)
} catch (unlinkError) {
console.error(JSON.stringify({ activity: ACTIVITY_ID, event: 'learn-substitute-link-unlink-failed', jti: link.jti, error: unlinkError instanceof Error ? unlinkError.message : String(unlinkError) }))
}
server/sessionStore.test.ts:71
- This test monkey-patches
console.infofor the duration of the whole test viat.after(). If the Node test runner executes files concurrently (common when--test-concurrencyis used), this global override can capture/affect logs from other tests and become order-dependent.
Prefer scoping the override with a local try/finally around just the createSessionStore() calls you’re asserting, so the patch window is as small as possible.
const previousInfo = console.info
const logs: string[] = []
console.info = (...args: unknown[]) => { logs.push(args.map(String).join(' ')) }
t.after(() => {
console.info = previousInfo
})
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (1)
activities/syncdeck/server/learnIntegration.ts:280
identityFingerprintis a cross-system contract (Learn must implement it byte-for-byte). The current variadic...partssignature makes it easy for future call sites to accidentally hash additional segments (or change separator semantics) while still type-checking. Consider locking the API to exactly(secret, domainTag, value)and hashing${domainTag}|${value}to better encode the contract in the function signature.
export function identityFingerprint(secret: string, ...parts: string[]): string {
return createHmac('sha256', secret).update(parts.join('|'), 'utf8').digest('hex').slice(0, 16)
}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Root-caused a class of session splits where an instructor's editor showed an active session but a student's launch landed in a waiting room, and a later reopen created a separate session: the active-entry TTL fallback (
sessions.ttlMs ?? WAITING_TTL_MS) silently resolved to the 10-minute waiting TTL in production, since the Valkey-backed store never exposes a top-levelttlMs. On top of that, the entry's expiry was refreshed only by Learn REST polling, never by the live session's own websocket activity, so a genuinely busy class could still orphan its mapping.Summary by CodeRabbit
New Features
Bug Fixes
Documentation