[DO NOT MERGE] hack duplicate welcomes #543
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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 |
How to use the Graphite Merge QueueAdd the label mergequeue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| if strings.Contains(err.Error(), "duplicate key value violates unique constraint") { | ||
| return nil, NewAlreadyExistsError(err) | ||
|
|
||
| var firstMessage *queries.WelcomeMessage |
There was a problem hiding this comment.
firstMessage = &message takes the address of the loop variable, so after later iterations it points to the last value, not the first. Consider storing the first result in a separate variable outside the loop and take its address instead.
- var firstMessage *queries.WelcomeMessage
+ var firstMessage *queries.WelcomeMessage
+ var first queries.WelcomeMessage
@@
- if i == 0 {
- firstMessage = &message
- }
+ if i == 0 {
+ first = message
+ firstMessage = &first
+ }🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
| numInserts = 3 | ||
| } | ||
|
|
||
| var firstMessage *queries.WelcomeMessage |
There was a problem hiding this comment.
The loop aborts on the first duplicate-key error, so later attempts never run when XMTP_ENABLE_DUPLICATE_WELCOMES is true. Consider continuing on duplicate-key errors and only return an error after all attempts fail; return the first successful insert.
🚀 Reply to ask Macroscope to explain or update this suggestion.
👍 Helpful? React to give us feedback.
Force pushing Docker images in build-node-go.yml and add an env-gated triple-insert path to
store.Store.InsertWelcomeMessageandstore.Store.InsertWelcomePointerMessagefor a hack to duplicate welcomesThe workflow always pushes Docker images. The store layer adds an
XMTP_ENABLE_DUPLICATE_WELCOMES='true'path that loops 3 times, appending a 4-byte big-endian index to the hash input to produce distinctInstallationKeyDataHashvalues, returning the first insert and erroring on any subsequent insert failure.📍Where to Start
Start with
InsertWelcomeMessageandInsertWelcomePointerMessagein store.go.📊 Macroscope summarized 6cd1a15. 1 file reviewed, 7 issues evaluated, 5 issues filtered, 1 comment posted
🗂️ Filtered Issues
pkg/mls/store/store.go — 1 comment posted, 7 evaluated, 5 filtered
InsertWelcomeMessagesucceeded, the second failed, and the function returns an error). This violates atomicity and can surprise callers who may interpret a non-nil error as "nothing inserted". Consider wrapping the loop in a transaction with rollback on failure, or altering the contract to tolerate partial inserts and return the successfully inserted IDs along with errors. [ Low confidence ]strings.Contains(err.Error(), "duplicate key value violates unique constraint"). This is brittle and dependent on the exact wording from the database driver/server. Different databases, locales, or driver versions may not match this substring, causing duplicate-key errors to be misclassified as generic errors. Prefer checking driver-specific error codes (e.g., PostgreSQLpq.Errorcode23505) or a structured error type. [ Low confidence ]XMTP_ENABLE_DUPLICATE_WELCOMESis enabled. If any iteration after a successful insert fails (e.g., due to a unique constraint or other DB error), the function returns an error but prior inserts remain committed, violating atomicity/invariants and potentially creating inconsistent duplicates. There is no rollback or grouped transaction around the loop. This occurs in thefor i := 0; i < numInserts; i++ { ... }loop and the error handling that immediately returns on any error. [ Low confidence ]wrapperAlgorithmtoint16(WrapperAlgorithm: int16(wrapperAlgorithm)) risks silent truncation iftypes.WrapperAlgorithmcan hold values outside theint16range. This is a data-conversion boundary with potential silent data loss and incorrect persistence of the algorithm identifier. [ Low confidence ]strings.Contains(err.Error(), "duplicate key value violates unique constraint")). This is brittle across drivers/locales and may misclassify errors or miss duplicates, leading to incorrect error wrapping. Use driver-specific error codes or typed errors. [ Low confidence ]