perf(ui): parallelize session decryption on cold start#124
Open
lucharo wants to merge 2 commits intohappier-dev:devfrom
Open
perf(ui): parallelize session decryption on cold start#124lucharo wants to merge 2 commits intohappier-dev:devfrom
lucharo wants to merge 2 commits intohappier-dev:devfrom
Conversation
Replace sequential for-await loops with Promise.all for both encryption key decryption and session metadata/agentState decryption, significantly reducing cold start time when loading many sessions. Closes happier-dev#123 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Greptile SummaryThis PR optimises cold-start performance in Key observations:
Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| apps/ui/sources/sync/engine/sessions/sessionSnapshot.ts | Parallelises key and session decryption with Promise.all; logic is preserved for null-return failure paths, but any thrown rejection in either Promise.all block will discard all sessions — Promise.allSettled would be more resilient. |
Sequence Diagram
sequenceDiagram
participant C as fetchAndApplySessions
participant E as encryption
participant S as sessions[]
Note over C,S: Phase 1 — Key decryption (parallelised)
C->>+E: Promise.all: decryptEncryptionKey(session[0..n].dataEncryptionKey)
E-->>-C: keyResults[0..n] { id, decrypted, hasKey }
C->>C: Build sessionKeys Map
C->>E: initializeSessions(sessionKeys)
Note over C,S: Phase 2 — Session decryption (parallelised)
loop For each session in parallel
C->>E: getSessionEncryption(session.id)
alt encryptionMode === 'e2ee'
C->>E: decryptMetadata(version, metadata)
C->>E: decryptAgentState(version, agentState)
else encryptionMode === 'plain'
C->>C: parsePlainMetadata(metadata)
C->>C: parsePlainAgentState(agentState)
end
end
C->>C: filter(s !== null) → decryptedSessions
Note over C,S: Phase 3 — Apply & repair
C->>S: applySessions(decryptedSessions)
C-->>C: fire-and-forget repairInvalidReadStateV1 loop
Last reviewed commit: 8ef72d3
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+121
to
+158
| const decryptedSessionResults = await Promise.all( | ||
| sessions.map(async (session) => { | ||
| const encryptionMode: 'e2ee' | 'plain' = session.encryptionMode === 'plain' ? 'plain' : 'e2ee'; | ||
|
|
||
| const sessionEncryption = encryption.getSessionEncryption(session.id); | ||
| if (encryptionMode === 'e2ee' && !sessionEncryption) { | ||
| console.error(`Session encryption not found for ${session.id} - this should never happen`); | ||
| return null; | ||
| } | ||
| }; | ||
|
|
||
| const parsePlainAgentState = (value: string | null): unknown => { | ||
| if (!value) return {}; | ||
| try { | ||
| const parsedJson = JSON.parse(value); | ||
| const parsed = AgentStateSchema.safeParse(parsedJson); | ||
| return parsed.success ? parsed.data : {}; | ||
| } catch { | ||
| return {}; | ||
| } | ||
| }; | ||
|
|
||
| const metadata = | ||
| encryptionMode === 'plain' | ||
| ? parsePlainMetadata(session.metadata) | ||
| : await sessionEncryption!.decryptMetadata(session.metadataVersion, session.metadata); | ||
|
|
||
| const agentState = | ||
| encryptionMode === 'plain' | ||
| ? parsePlainAgentState(session.agentState) | ||
| : await sessionEncryption!.decryptAgentState(session.agentStateVersion, session.agentState); | ||
|
|
||
| // Put it all together | ||
| const accessLevel = session.share?.accessLevel; | ||
| const normalizedAccessLevel = | ||
| accessLevel === 'view' || accessLevel === 'edit' || accessLevel === 'admin' ? accessLevel : undefined; | ||
| decryptedSessions.push({ | ||
| ...session, | ||
| encryptionMode, | ||
| thinking: false, | ||
| thinkingAt: 0, | ||
| metadata, | ||
| agentState, | ||
| accessLevel: normalizedAccessLevel, | ||
| canApprovePermissions: session.share?.canApprovePermissions ?? undefined, | ||
| }); | ||
| } | ||
| const metadata = | ||
| encryptionMode === 'plain' | ||
| ? parsePlainMetadata(session.metadata) | ||
| : await sessionEncryption!.decryptMetadata(session.metadataVersion, session.metadata); | ||
|
|
||
| const agentState = | ||
| encryptionMode === 'plain' | ||
| ? parsePlainAgentState(session.agentState) | ||
| : await sessionEncryption!.decryptAgentState(session.agentStateVersion, session.agentState); | ||
|
|
||
| const accessLevel = session.share?.accessLevel; | ||
| const normalizedAccessLevel = | ||
| accessLevel === 'view' || accessLevel === 'edit' || accessLevel === 'admin' ? accessLevel : undefined; | ||
| return { | ||
| ...session, | ||
| encryptionMode, | ||
| thinking: false, | ||
| thinkingAt: 0, | ||
| metadata, | ||
| agentState, | ||
| accessLevel: normalizedAccessLevel, | ||
| canApprovePermissions: session.share?.canApprovePermissions ?? undefined, | ||
| }; | ||
| }), | ||
| ); | ||
| const decryptedSessions = decryptedSessionResults.filter( | ||
| (s): s is NonNullable<typeof s> => s !== null, | ||
| ); |
Contributor
There was a problem hiding this comment.
Promise.all fail-fast drops all sessions on a single decryption error
Promise.all rejects immediately if any of the async callbacks throws. This means that if sessionEncryption!.decryptMetadata(...) or sessionEncryption!.decryptAgentState(...) rejects for even one session, the entire decryptedSessionResults promise rejects — no sessions will be applied, and applySessions is never called.
In the original sequential for-await loop the behaviour was the same (a throw would propagate), but migrating to parallel execution is the natural moment to also make error handling more resilient. Switching to Promise.allSettled and filtering out rejected outcomes would let successfully-decrypted sessions still load even when one session's crypto call fails:
const decryptedSessionResults = await Promise.allSettled(
sessions.map(async (session) => {
// ... same body ...
}),
);
const decryptedSessions = decryptedSessionResults
.filter((r): r is PromiseFulfilledResult<NonNullable<...>> =>
r.status === 'fulfilled' && r.value !== null,
)
.map((r) => r.value);
The same concern applies to the first Promise.all at lines 75–83: if encryption.decryptEncryptionKey rejects for any session, the for loop on line 84 never runs, initializeSessions is never called, and the function throws — potentially leaving sessionDataKeys in whatever state it was before this call.
WalkthroughParallelizes session decryption by converting sequential per-session key and metadata/agentState decryptions to parallel Promise.all flows, adds per-session error handling that preserves nulls for failed decryptions, introduces parsing helpers for plain sessions, and produces normalized decrypted session objects before applying them. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ui/sources/sync/engine/sessions/sessionSnapshot.ts`:
- Around line 121-155: The current sessions.map used inside
decryptedSessionResults can throw if sessionEncryption.decryptMetadata or
decryptAgentState throws, causing Promise.all to reject; wrap the async callback
body (the logic that computes metadata and agentState for each session) in a
try-catch so individual session failures are caught, log the error with
identifying info (e.g., session.id and which step failed), and return null for
that session so Promise.all resolves with other sessions; keep existing checks
(encryption.getSessionEncryption, handling of 'plain' mode, normalization of
accessLevel) and only catch errors around decryptMetadata/decryptAgentState
within the async function that builds the returned session object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ae5139a1-77ad-4b91-991b-dea505e17055
📒 Files selected for processing (1)
apps/ui/sources/sync/engine/sessions/sessionSnapshot.ts
Wrap both Promise.all callbacks in try-catch so a single session's decryption failure (e.g. malformed base64 from server) doesn't prevent all other sessions from loading. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/ui/sources/sync/engine/sessions/sessionSnapshot.ts (3)
89-101: Redundant error logging for decryption failures.When
decryptEncryptionKeythrows, the error is logged at line 84. Then, the post-processing loop at line 91 logs another error for the same session. Consider removing one of the duplicate logs for cleaner output.♻️ Proposed fix to remove redundant logging
for (const { id, decrypted, hasKey } of keyResults) { if (hasKey && !decrypted) { - console.error(`Failed to decrypt data encryption key for session ${id}`); sessionKeys.set(id, null); sessionDataKeys.delete(id); } else if (hasKey && decrypted) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/sources/sync/engine/sessions/sessionSnapshot.ts` around lines 89 - 101, The loop over keyResults in sessionSnapshot.ts currently logs decryption failures again even though decryptEncryptionKey already logs the error; remove the duplicate console.error(`Failed to decrypt data encryption key for session ${id}`) inside the for-loop and leave the rest of the handling (setting sessionKeys.set(id, null) and sessionDataKeys.delete(id)) intact so decryption failures are only logged once by decryptEncryptionKey.
115-124: Consider using explicit return type for type consistency.The
parsePlainAgentStatefunction returnsunknown, but it would be more precise to returnAgentState(or the inferred type fromAgentStateSchema) for consistency with schema-driven parsing. This would improve type safety downstream.♻️ Proposed type improvement
- const parsePlainAgentState = (value: string | null): unknown => { + const parsePlainAgentState = (value: string | null): Record<string, unknown> => {Or import and use the
AgentStatetype if available:import { AgentState } from '@/sync/domains/state/storageTypes'; // ... const parsePlainAgentState = (value: string | null): AgentState => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/sources/sync/engine/sessions/sessionSnapshot.ts` around lines 115 - 124, The parsePlainAgentState function currently has an imprecise return type of unknown; change its signature to return the concrete AgentState (or the type exported alongside AgentStateSchema) by importing AgentState from its module and updating the function signature (parsePlainAgentState(value: string | null): AgentState). Ensure all early-return paths (falsy value, schema failure, and catch) return a valid AgentState value (use a typed default or casted empty/default object consistent with AgentState) so callers get correct typing from AgentStateSchema.safeParse usage.
174-187: Consider parallelizing the repair loop for consistency.The repair loop still runs sequentially with
for...ofandawait. Given the PR's goal of improving cold-start performance through parallelization, this loop could also benefit fromPromise.allwith per-session error handling. However, reviewing therepairInvalidReadStateV1function shows it already tracksinFlightsessions to prevent duplicate repairs, so concurrent calls would safely no-op for the same session.♻️ Optional: Parallelize repair loop
void (async () => { - for (const session of decryptedSessions) { - try { + await Promise.all(decryptedSessions.map(async (session) => { + try { const readState = (session.metadata as Metadata | null)?.readStateV1; if (!readState) continue; - if (readState.sessionSeq <= (session.seq ?? 0)) continue; + if (!readState) return; + if (readState.sessionSeq <= (session.seq ?? 0)) return; await repairInvalidReadStateV1({ sessionId: session.id, sessionSeqUpperBound: session.seq ?? 0 }); } catch (err) { console.error('[sessionsSnapshot] Failed to repair invalid readStateV1', { sessionId: session.id, err }); } - } + })); })().catch((err) => { console.error('[sessionsSnapshot] Invalid readStateV1 repair loop failed', { err }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ui/sources/sync/engine/sessions/sessionSnapshot.ts` around lines 174 - 187, The sequential repair loop over decryptedSessions should be converted to run repairs in parallel: replace the for...of/await loop with Promise.all over decryptedSessions.map(...) where each mapped async function reads (session.metadata as Metadata | null)?.readStateV1 and skips when absent or when readState.sessionSeq <= (session.seq ?? 0), then calls repairInvalidReadStateV1({ sessionId: session.id, sessionSeqUpperBound: session.seq ?? 0 }) and wraps that call in a try/catch to log failures per-session (preserving the current console.error usage and message), relying on repairInvalidReadStateV1's inFlight tracking to prevent duplicate repairs; keep the outer IIFE and its .catch to log any top-level errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/ui/sources/sync/engine/sessions/sessionSnapshot.ts`:
- Around line 89-101: The loop over keyResults in sessionSnapshot.ts currently
logs decryption failures again even though decryptEncryptionKey already logs the
error; remove the duplicate console.error(`Failed to decrypt data encryption key
for session ${id}`) inside the for-loop and leave the rest of the handling
(setting sessionKeys.set(id, null) and sessionDataKeys.delete(id)) intact so
decryption failures are only logged once by decryptEncryptionKey.
- Around line 115-124: The parsePlainAgentState function currently has an
imprecise return type of unknown; change its signature to return the concrete
AgentState (or the type exported alongside AgentStateSchema) by importing
AgentState from its module and updating the function signature
(parsePlainAgentState(value: string | null): AgentState). Ensure all
early-return paths (falsy value, schema failure, and catch) return a valid
AgentState value (use a typed default or casted empty/default object consistent
with AgentState) so callers get correct typing from AgentStateSchema.safeParse
usage.
- Around line 174-187: The sequential repair loop over decryptedSessions should
be converted to run repairs in parallel: replace the for...of/await loop with
Promise.all over decryptedSessions.map(...) where each mapped async function
reads (session.metadata as Metadata | null)?.readStateV1 and skips when absent
or when readState.sessionSeq <= (session.seq ?? 0), then calls
repairInvalidReadStateV1({ sessionId: session.id, sessionSeqUpperBound:
session.seq ?? 0 }) and wraps that call in a try/catch to log failures
per-session (preserving the current console.error usage and message), relying on
repairInvalidReadStateV1's inFlight tracking to prevent duplicate repairs; keep
the outer IIFE and its .catch to log any top-level errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f81dd55-c9dd-480b-8d7b-bba52ddb2b6e
📒 Files selected for processing (1)
apps/ui/sources/sync/engine/sessions/sessionSnapshot.ts
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
1 participant
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.
Summary
for-awaitloops withPromise.allfor both encryption key decryption and session metadata/agentState decryption insessionSnapshot.tsChanges
for + await→Promise.all(sessions.map(...))for + await→Promise.all(sessions.map(...))parsePlainMetadataandparsePlainAgentStatehelpers out of the loop since they're pure functionsTest plan
Closes #123
🤖 Generated with Claude Code
Summary by CodeRabbit