fix: implement session locking to prevent race conditions during session management#2581
fix: implement session locking to prevent race conditions during session management#2581jlucaso1 wants to merge 1 commit into
Conversation
|
Thanks for opening this pull request and contributing to the project! The next step is for the maintainers to review your changes. If everything looks good, it will be approved and merged into the main branch. In the meantime, anyone in the community is encouraged to test this pull request and provide feedback. ✅ How to confirm it worksIf you’ve tested this PR, please comment below with: This helps us speed up the review and merge process. 📦 To test this PR locally:If you encounter any issues or have feedback, feel free to comment as well. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR adds keyed per-session mutex locking and isolated transaction support, exposes ChangesPer-session concurrency control and session deletion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 docstrings
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/Utils/auth-utils.ts (1)
364-366: ⚡ Quick winUse
errkey for error logging per coding guidelines.The structured logging convention specifies errors should go under
err, noterror.Proposed fix
} catch (error) { - logger.error({ error }, 'isolated transaction failed') + logger.error({ err: error }, 'isolated transaction failed') throw errorAs per coding guidelines: "Errors should go under
err, noterrorore."🤖 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 `@src/Utils/auth-utils.ts` around lines 364 - 366, In the catch block that logs failed isolated transactions, change the structured payload to use the `err` key instead of `error` so it follows logging guidelines; specifically update the call to `logger.error` that currently passes `{ error }` (the catch variable named `error`) alongside the message `'isolated transaction failed'` to instead pass `{ err: error }` while leaving the message and rethrow unchanged.
🤖 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 `@src/__tests__/Signal/lid-mapping.test.ts`:
- Around line 12-14: The mock introduces a new any via the callback type "(work:
() => any)"; change the callback's type to match the real interface by using
Parameters<SignalKeyStoreWithTransaction['isolatedTransaction']>[0] for the work
parameter (i.e., (work:
Parameters<SignalKeyStoreWithTransaction['isolatedTransaction']>[0]) =>
ReturnType<Parameters<SignalKeyStoreWithTransaction['isolatedTransaction']>[0]>
or simply (work:
Parameters<SignalKeyStoreWithTransaction['isolatedTransaction']>[0]) => any to
preserve behavior) so the jest.fn typing for isolatedTransaction aligns with the
SignalKeyStoreWithTransaction signature and remove the new any; keep the
jest.fn<SignalKeyStoreWithTransaction['isolatedTransaction']>(...) wrapper and
adjust or remove the trailing "as any" cast if no longer needed.
In `@src/__tests__/Utils/tc-token.test.ts`:
- Around line 33-35: Replace the explicit any callback in the
isolatedTransaction mock by letting TypeScript infer the callback parameter:
change the mock to use
jest.fn<SignalKeyStoreWithTransaction['isolatedTransaction']>(async (work) =>
await work()) and remove the explicit "(work: () => any)" (and the unnecessary
"as any" cast) so the work parameter type is inferred from
SignalKeyStoreWithTransaction['isolatedTransaction'].
---
Nitpick comments:
In `@src/Utils/auth-utils.ts`:
- Around line 364-366: In the catch block that logs failed isolated
transactions, change the structured payload to use the `err` key instead of
`error` so it follows logging guidelines; specifically update the call to
`logger.error` that currently passes `{ error }` (the catch variable named
`error`) alongside the message `'isolated transaction failed'` to instead pass
`{ err: error }` while leaving the message and rethrow unchanged.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6c63366a-f1f9-4bd1-a1cf-8305e5988066
📒 Files selected for processing (7)
src/Signal/libsignal.tssrc/Socket/messages-recv.tssrc/Types/Auth.tssrc/Types/Signal.tssrc/Utils/auth-utils.tssrc/__tests__/Signal/lid-mapping.test.tssrc/__tests__/Utils/tc-token.test.ts
There was a problem hiding this comment.
2 issues found across 7 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
f0d33e5 to
9fcf7d5
Compare
|
This PR is stale because it has been open for 14 days with no activity. Remove the stale label or comment or this will be closed in 14 days |
Summary by CodeRabbit
Bug Fixes
Chores