Session reversal#179
Conversation
📝 WalkthroughWalkthroughAdds a ChangesRemove Participant Study Session
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
Greptile SummaryThis PR adds a
Confidence Score: 3/5The endpoint is functional under normal conditions but can leave the participant record and its responses in diverged state if the second DB write fails. The two MongoDB writes in removeStudySession are independent with no transaction or rollback: a transient error on the response bulk-update after the participant was already patched produces an irrecoverable mismatch between CurrentStudySession and the session labels stored on historical responses. A retry would skip the participant update entirely (the if-branch won't match the already-cleared value), so idempotent retry doesn't heal the inconsistency either. participant-management.go — the handler's two-step write sequence needs attention before merging to production data.
|
| Filename | Overview |
|---|---|
| services/management-api/apihandlers/participant-management.go | Adds removeStudySession handler and route; the two sequential DB writes lack atomicity — a failure after the participant update but before the response update leaves data in an inconsistent state with no recovery path. |
| pkg/db/study/responses.go | Adds UpdateResponseSessionContext helper using UpdateMany; when newSession is empty the field is set to "" rather than unset, which may leave ghost entries for downstream code checking key existence. |
Sequence Diagram
sequenceDiagram
participant Client
participant Handler as removeStudySession
participant ParticipantsDB
participant ResponsesDB
Client->>Handler: POST /:participantID/remove-session
Handler->>ParticipantsDB: GetParticipantByID
ParticipantsDB-->>Handler: participant p
alt "p.CurrentStudySession == sessionToRemove"
Handler->>ParticipantsDB: UpdateParticipantIfNotModified
ParticipantsDB-->>Handler: updated p
Note over Handler,ParticipantsDB: If next step fails state is inconsistent
end
Handler->>ResponsesDB: UpdateResponseSessionContext
ResponsesDB-->>Handler: count
Handler-->>Client: 200 OK
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
services/management-api/apihandlers/participant-management.go:247-263
**Partial-failure window leaves data in inconsistent state**
The two writes — updating the participant's `CurrentStudySession` (step 1) and relabelling every response via `UpdateResponseSessionContext` (step 2) — are independent MongoDB operations with no transaction or rollback. If step 1 succeeds but step 2 fails (network hiccup, timeout, etc.), the participant's current session has been cleared but all historical responses still carry the old session label, silently introducing a mismatch that cannot be detected or recovered automatically. Retrying the request after this failure would skip step 1 entirely (the `if` branch won't match any more) while step 2 would still attempt to migrate responses, so idempotent retry does not fix the inconsistency either.
### Issue 2 of 3
pkg/db/study/responses.go:304-309
The two branches of the `if/else` produce the same `$set` expression — only the value differs. The whole block can be collapsed to a single assignment, reducing the surface area for future divergence.
```suggestion
update := bson.M{"$set": bson.M{"context.session": newSession}}
```
### Issue 3 of 3
pkg/db/study/responses.go:304-309
When `newSession` is empty the intent is to "remove the session association" (per the API handler comment), but the update stores an explicit empty string (`""`) instead of unsetting the key. Downstream code that checks for the key's existence (e.g. `if _, ok := ctx["session"]; ok`) would still see a session entry. Using `$unset` when the replacement is empty matches the stated semantic more precisely.
```suggestion
var update bson.M
if newSession == "" {
update = bson.M{"$unset": bson.M{"context.session": ""}}
} else {
update = bson.M{"$set": bson.M{"context.session": newSession}}
}
```
Reviews (1): Last reviewed commit: "More accurate doc wording." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if p.CurrentStudySession == req.SessionToRemove { | ||
| p.CurrentStudySession = req.ReplacementSession | ||
| p, err = h.studyDBConn.UpdateParticipantIfNotModified(token.InstanceID, studyKey, p) | ||
| if err != nil { | ||
| slog.Error("failed to update participant session", slog.String("error", err.Error())) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to update participant"}) | ||
| return | ||
| } | ||
| } | ||
|
|
||
| // Migrate session label on all responses recorded under the removed session | ||
| count, err := h.studyDBConn.UpdateResponseSessionContext(token.InstanceID, studyKey, participantID, req.SessionToRemove, req.ReplacementSession) | ||
| if err != nil { | ||
| slog.Error("failed to update response sessions", slog.String("error", err.Error())) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "failed to update response sessions"}) | ||
| return | ||
| } |
There was a problem hiding this comment.
The two writes — updating the participant's CurrentStudySession (step 1) and relabelling every response via UpdateResponseSessionContext (step 2) — are independent MongoDB operations with no transaction or rollback. If step 1 succeeds but step 2 fails (network hiccup, timeout, etc.), the participant's current session has been cleared but all historical responses still carry the old session label, silently introducing a mismatch that cannot be detected or recovered automatically. Retrying the request after this failure would skip step 1 entirely (the if branch won't match any more) while step 2 would still attempt to migrate responses, so idempotent retry does not fix the inconsistency either.
Prompt To Fix With AI
This is a comment left during a code review.
Path: services/management-api/apihandlers/participant-management.go
Line: 247-263
Comment:
**Partial-failure window leaves data in inconsistent state**
The two writes — updating the participant's `CurrentStudySession` (step 1) and relabelling every response via `UpdateResponseSessionContext` (step 2) — are independent MongoDB operations with no transaction or rollback. If step 1 succeeds but step 2 fails (network hiccup, timeout, etc.), the participant's current session has been cleared but all historical responses still carry the old session label, silently introducing a mismatch that cannot be detected or recovered automatically. Retrying the request after this failure would skip step 1 entirely (the `if` branch won't match any more) while step 2 would still attempt to migrate responses, so idempotent retry does not fix the inconsistency either.
How can I resolve this? If you propose a fix, please make it concise.
| var update bson.M | ||
| if newSession == "" { | ||
| update = bson.M{"$set": bson.M{"context.session": ""}} | ||
| } else { | ||
| update = bson.M{"$set": bson.M{"context.session": newSession}} | ||
| } |
There was a problem hiding this comment.
if/else produce the same $set expression — only the value differs. The whole block can be collapsed to a single assignment, reducing the surface area for future divergence.
| var update bson.M | |
| if newSession == "" { | |
| update = bson.M{"$set": bson.M{"context.session": ""}} | |
| } else { | |
| update = bson.M{"$set": bson.M{"context.session": newSession}} | |
| } | |
| update := bson.M{"$set": bson.M{"context.session": newSession}} |
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/db/study/responses.go
Line: 304-309
Comment:
The two branches of the `if/else` produce the same `$set` expression — only the value differs. The whole block can be collapsed to a single assignment, reducing the surface area for future divergence.
```suggestion
update := bson.M{"$set": bson.M{"context.session": newSession}}
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| var update bson.M | ||
| if newSession == "" { | ||
| update = bson.M{"$set": bson.M{"context.session": ""}} | ||
| } else { | ||
| update = bson.M{"$set": bson.M{"context.session": newSession}} | ||
| } |
There was a problem hiding this comment.
newSession is empty the intent is to "remove the session association" (per the API handler comment), but the update stores an explicit empty string ("") instead of unsetting the key. Downstream code that checks for the key's existence (e.g. if _, ok := ctx["session"]; ok) would still see a session entry. Using $unset when the replacement is empty matches the stated semantic more precisely.
| var update bson.M | |
| if newSession == "" { | |
| update = bson.M{"$set": bson.M{"context.session": ""}} | |
| } else { | |
| update = bson.M{"$set": bson.M{"context.session": newSession}} | |
| } | |
| var update bson.M | |
| if newSession == "" { | |
| update = bson.M{"$unset": bson.M{"context.session": ""}} | |
| } else { | |
| update = bson.M{"$set": bson.M{"context.session": newSession}} | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/db/study/responses.go
Line: 304-309
Comment:
When `newSession` is empty the intent is to "remove the session association" (per the API handler comment), but the update stores an explicit empty string (`""`) instead of unsetting the key. Downstream code that checks for the key's existence (e.g. `if _, ok := ctx["session"]; ok`) would still see a session entry. Using `$unset` when the replacement is empty matches the stated semantic more precisely.
```suggestion
var update bson.M
if newSession == "" {
update = bson.M{"$unset": bson.M{"context.session": ""}}
} else {
update = bson.M{"$set": bson.M{"context.session": newSession}}
}
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
services/management-api/apihandlers/participant-management.go (1)
247-263: 💤 Low valueNon-atomic two-phase update may leave partial state on failure.
If
UpdateParticipantIfNotModifiedsucceeds (line 249) butUpdateResponseSessionContextfails (line 258), the participant's session is already changed while responses retain the old session label. The client receives an error but the participant record is modified.The operation is retry-safe since on retry the participant update is skipped (session no longer matches) and response migration is re-attempted. For a management API this is likely acceptable, but consider whether the error response should indicate that participant was partially updated, or whether a compensating rollback is warranted.
🤖 Prompt for 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. In `@services/management-api/apihandlers/participant-management.go` around lines 247 - 263, The update operation in the session removal logic is non-atomic and can leave partial state if UpdateParticipantIfNotModified succeeds but UpdateResponseSessionContext fails afterward. The participant record will be modified while responses retain the old session label, creating inconsistency. To fix this, implement either a compensating rollback (reverting the participant session change if the response update fails) or modify the error response to clearly indicate that the participant was partially updated so the caller knows the system state is inconsistent. Choose the approach appropriate for your system's consistency requirements and implement the necessary error handling between the UpdateParticipantIfNotModified and UpdateResponseSessionContext calls.
🤖 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.
Nitpick comments:
In `@services/management-api/apihandlers/participant-management.go`:
- Around line 247-263: The update operation in the session removal logic is
non-atomic and can leave partial state if UpdateParticipantIfNotModified
succeeds but UpdateResponseSessionContext fails afterward. The participant
record will be modified while responses retain the old session label, creating
inconsistency. To fix this, implement either a compensating rollback (reverting
the participant session change if the response update fails) or modify the error
response to clearly indicate that the participant was partially updated so the
caller knows the system state is inconsistent. Choose the approach appropriate
for your system's consistency requirements and implement the necessary error
handling between the UpdateParticipantIfNotModified and
UpdateResponseSessionContext calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 984105b6-c848-4324-b988-12ed7eee7707
📒 Files selected for processing (2)
pkg/db/study/responses.goservices/management-api/apihandlers/participant-management.go
2 participants
Summary by CodeRabbit