Forward the SSO-Check node's session and checkpoint reads to the Session node - #4401
Merged
madurangasiriwardena merged 1 commit intoJul 30, 2026
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
💤 Files with no reviewable changes (5)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe SSO checkpoint flow now uses direct checkpoint-context lookup, forwards resolved session data between paired executors, and accepts structured checkpoint-load inputs that reuse matching forwarded rows. ChangesSSO checkpoint forwarding
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SSOCheckExecutor
participant SessionExecutor
participant SessionService
participant SessionStore
SSOCheckExecutor->>SessionService: FindCheckpoint(sessionID, checkpoint)
SessionService->>SessionStore: GetByCheckpoint(sessionID, checkpoint)
SessionStore-->>SessionService: SessionContext
SessionService-->>SSOCheckExecutor: checkpoint context
SSOCheckExecutor->>SessionExecutor: Forward Session and SessionContext
SessionExecutor->>SessionService: LoadCheckpoint(LoadCheckpointInput)
SessionService-->>SessionExecutor: Session and SessionContext
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
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 `@backend/internal/flow/session/transport.go`:
- Around line 65-71: Update WithInbound to preserve the existing non-nil
InboundHandle.memo when wrapping an inbound context, allocating a new
ssoReadMemo only when no memo is present; continue storing the resulting handle
with context.WithValue.
🪄 Autofix (Beta)
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: CHILL
Plan: Pro Plus
Run ID: 70c2533b-6039-49d2-b3eb-a826a5eb970b
📒 Files selected for processing (9)
backend/internal/flow/session/interface.gobackend/internal/flow/session/service.gobackend/internal/flow/session/service_test.gobackend/internal/flow/session/sessionStore_mock_test.gobackend/internal/flow/session/session_context_store_test.gobackend/internal/flow/session/store.gobackend/internal/flow/session/store_constants.gobackend/internal/flow/session/transport.gobackend/internal/flow/session/transport_test.go
💤 Files with no reviewable changes (5)
- backend/internal/flow/session/store_constants.go
- backend/internal/flow/session/sessionStore_mock_test.go
- backend/internal/flow/session/interface.go
- backend/internal/flow/session/store.go
- backend/internal/flow/session/session_context_store_test.go
madurangasiriwardena
force-pushed
the
fix/sso-reuse-duplicate-reads
branch
3 times, most recently
from
July 28, 2026 13:57
845d923 to
d6079bd
Compare
madurangasiriwardena
changed the title
madurangasiriwardena
added
the
trigger-pr-builder
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
12 tasks
ThaminduDilshan
previously approved these changes
Jul 30, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 30, 2026
…ion node An SSO reuse issued six SSO_SESSION/SSO_SESSION_CONTEXT statements for four rows' worth of work. The SSO-Check node resolved the session and answered "does this checkpoint exist" by listing every checkpoint id, then the paired Session node re-read the same session by the same handle and fetched the checkpoint context, both microseconds later in the same request. Neither repeat read validated anything the resolver had not already checked. Hand both rows to the Session node through ForwardedData, the engine's existing node-to-node channel, so the load path uses what the check node already read. LoadCheckpoint takes them on its input struct and validates each before use: the session is rejected unless its handle matches the one being loaded, the context unless its session id and checkpoint id both match. Anything rejected or absent is read from the store, so a Session node reached without the handover still works and a partial handover costs a query rather than correctness. ForwardedData reaches the immediate next node only, so the rows are forwarded on the Skip outcome alone. The Authenticate outcome prompts and suspends the flow, and ForwardedData is a serialized field, so data left there could be persisted with the flow context. HasCheckpoint becomes FindCheckpoint, returning the checkpoint context instead of a bool. Fetching the row by its full primary key answers the availability question by itself and gives the check node something to forward, so ListCheckpointIDs and SSO-SESS-08 have no callers left and are removed. Measured on PostgreSQL with pg_stat_statements: the reuse leg drops from six statements to four (one SSO_SESSION read instead of two, one SSO_SESSION_CONTEXT read instead of two), removing two round trips and the 0.121 ms the checkpoint listing cost, which was a prefix range scan against the 0.024 ms full-key probe that replaces it. Verified end to end: the whole reuse leg falls from 20 statements to 18 with SSO-SESS-08 absent from pg_stat_statements, and 250 concurrent reuses across 25 workers complete with no failures. No SQL was added, so nothing is engine-specific.
madurangasiriwardena
force-pushed
the
fix/sso-reuse-duplicate-reads
branch
from
July 30, 2026 15:34
d6079bd to
7c81869
Compare
madurangasiriwardena
enabled auto-merge
July 30, 2026 15:49
ThaminduDilshan
approved these changes
Jul 30, 2026
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
2 participants
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.
Purpose
An SSO session reuse issued six statements against
SSO_SESSION/SSO_SESSION_CONTEXTfor four rows' worth of work, because the two nodes that make up a reuse each read the same rows independently:SSO-SESS-02).SSO-SESS-08).SSO-SESS-02again), microseconds later in the same request.SSO-SESS-06).Neither repeat read validated anything: liveness, state, both deadlines, flow id and flow version are all decided by the resolver in step 1, and the Session node's re-read only detected a vanished row. The listing in step 2 is a prefix range scan that answers a question the full-key fetch in step 4 answers by itself, and its cost grows with the number of checkpoints a flow defines.
This PR hands both rows from the check node to the Session node, so a reuse issues four statements instead of six: one
SSO_SESSIONread instead of two, oneSSO_SESSION_CONTEXTread instead of two.Measured on PostgreSQL with
pg_stat_statements(track_planning=on, counters reset per run) and the correlation-id query log:SSO_SESSIONreads per reuseSSO_SESSION_CONTEXTreads per reuseThe removed statements measured 0.121 ms (0.096 planning + 0.025 execution) for the checkpoint listing and roughly 0.086 ms for the duplicate session read, against 0.433 ms of total server-side time for the whole reuse path. Note that planning dominates every read on this path, at 2x to 4x its execution time, because each statement is sent as an unnamed prepared statement and re-planned per call. The 0.024 ms full-key probe that replaces the 0.121 ms listing is the same query the load path already used.
No user-facing behaviour changes. No SQL syntax was added, so nothing is engine-specific.
Approach
The rows travel on
ForwardedData, the engine's existing node-to-node channel. The SSO-Check node puts the session it resolved and the context it fetched on itsExecutorResponse; the engine promotes them onto the next node's context; the Session node reads them and passes them to the service. No new plumbing, and the handover is visible in the graph rather than hidden in a side channel.Forwarded only on the Skip outcome.
ForwardedDatareaches the immediate next node and is then cleared, and it is a serialized field. On the Authenticate outcome the flow prompts and suspends, so anything left there could be persisted with the flow context. The Skip outcome is the only path where the Session node runs next, so it is the only path that forwards.assertAbsentassertsForwardedDatais empty across all three Authenticate cases.Validated on arrival, not trusted. The service rejects a forwarded session unless its
HANDLE_IDmatches the handle being loaded, and a forwarded context unless both its session id and checkpoint id match what is being restored. Anything rejected or absent is read from the store, so a Session node reached without the handover still works and a partial handover costs a query rather than correctness.Interface changes (both internal to
internal/flow, nothing product-facing):Service.HasCheckpoint(ctx, sessionID, checkpoint) (bool, error)becomesFindCheckpoint(...) (*SessionContext, error). Fetching the row by its full primary key answers the availability question by itself (!= nil) and gives the check node something to forward.Service.LoadCheckpointtakes aLoadCheckpointInputstruct rather than growing to seven positional parameters, mirroring the existingSaveCheckpointInput. The two forwarded rows are optional fields on it.These regenerate
internal/flow/session/Service_mock_test.goand, becauseServiceis consumed frominternal/flow/executor, the sharedtests/mocks/flow/sessionmock/Service_mock.go. That accounts for the mock diff.Dead code removed.
sessionStore.ListCheckpointIDs, its store implementation andqueryListCheckpointsBySessionID(SSO-SESS-08) have no callers left and are deleted rather than left as unused SQL.Known limitation, deliberate.
ForwardedDatareaches the immediate next node only, so the handover applies when the Session node follows its SSO-Check node directly. The shipped BASIC_SSO template wiresonSuccessto the same node ascheckpointRef, and the console always emits that shape, so this holds in practice. It is not enforced:validateSSOCheckExecutorchecks thatcheckpointRefnames an existingSessionExecutornode, but not that the success edge points at it. A flow authored through the management API with a node in between simply falls back to two reads, so correctness is unaffected and only the saving is lost. Worth noting separately that the same unenforced invariant has a pre-existing consequence beyond this PR: such a flow would take the Skip outcome and never reach the Session node, so the snapshot would never be restored. That validation gap is a follow-up, not part of this change.Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
Summary by CodeRabbit
Performance Improvements
Bug Fixes
Tests