-
Notifications
You must be signed in to change notification settings - Fork 56
Recover WIP login stashes #687
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
src/core/login/login-selectors.ts
Outdated
| const { stashes } = ai.props.state.login | ||
| for (const stash of stashes) { | ||
| for (const rootStash of stashes) { | ||
| const stash = rootStash.wipChange ?? rootStash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong, but it was there to figure out why it wasn't working during sanity test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@swansontec Should I keep this or should I go with another approach to solve the problem where pin-login will no longer work on the light-account when there is a wipChange and that the login server has?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you are asking. How is this line wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have said "I don't know if this is right". I don't know if it's wrong, but it solved the issue with pin-login, but I wasn't sure if this is the correct solution. I felt uncomfortable always assuming wipChange is the correct stash to use.
Now that I think about why I felt uncomfortable, there is a case where pin-login will break if the login-server doesn't have the changes that wipChange has. In this case, pin-login will break because wipChange is the wrong stash according to the login server. So perhaps the correct solution is to pick wipChange depending on if the login-server has the changes or not. This would require that we query the login-server to determine this and then drill this information down to the selector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or perhaps heal just the stash that has the issue, so
const stashTree = lookupStashForLogin(...);
if (stashTree.wipChange != null)
await healStash(stashTree);
restOfLogin(...)Obviously pseudocode, but you get the idea. If we can't hit the server for healing, then we aren't going to be PIN logging in anyhow. If it's a login method like touch, we can just ignore failures to heal and just use the old data.
swansontec
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks generally correct. I would say, "debug it yourself" now, except that the AI may have noticed something worth double-checking.
12ccda1 to
64ee466
Compare
|
This was a pretty challenging thing to test and debug to ensure it does what we expect under the poor network conditions we expect. To simulate the poor network conditions, I added the following patch originally: --- a/src/core/login/login.ts
+++ b/src/core/login/login.ts
@@ -450,6 +450,7 @@ export async function applyKit(
request.data = kit.server
try {
await loginFetch(ai, serverMethod, serverPath, request)
+ throw new Error('Simulated Network Timeout')
} catch (error) {
// If we fail, try to sync to see if the server got it:
await syncLogin(ai, sessionKey).catch(() => {})Here's the testing steps:
Expected: At step 4, username chosen in the backup flow is shown. At step 6, login with PIN should work as normal. So far testing commit a3705a1 which deviates from the original algorithm, the test passes. When adding the fixup commit 64ee466 to match the original algorithm from the task, the tests break. 🤷♂️ |
swansontec
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two more comments worth addressing, but OK, it's close enough we can live with it.
src/core/storage/storage-actions.ts
Outdated
| import { StorageWalletStatus } from './storage-reducer' | ||
|
|
||
| export const SYNC_INTERVAL = 30 * 1000 | ||
| export const EXPEDITED_SYNC_INTERVAL = 5000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has nothing to do with storage (it deals with the login server), and should go in the account pixie.
src/core/login/login-selectors.ts
Outdated
| const { stashes } = ai.props.state.login | ||
| for (const stashTree of stashes) { | ||
| for (const rootStash of stashes) { | ||
| const stashTree = rootStash.wipChange ?? rootStash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that the healing loop is more aggressive, can we skip this line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be here. Now I after removing it, I must test thoroughly again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test fails. This const stashTree = rootStash.wipChange ?? rootStash line was what was causing the tests to pass before. Had this not slipped into the rebase, the test would have failed. I now am not comfortable with this PR.
I'm not sure why the healing is not applying. I'll need more time with this task.
|
Here's a detailed summary of what has been fixed during a long debugging session with Cursor. So far it makes sense to me what the issue was: the login-server payload was destroying the wipChange which has local-only username data which is valuable to the stash. So we need to make sure we don't destroy this when applying login-server payloads nor when applying the Summary of Fixes1. src/core/login/login.ts - Preserve wipChange in applyLoginPayloadInner// Added line 122: if (stash.wipChange != null) out.wipChange = stash.wipChange Why: When applyLoginPayload rebuilds a stash from server data, it creates a new object and copies over "client-only" fields like lastLogin, username. But wipChange wasn't being preserved, so background syncLogin calls would lose pending changes. 2. src/core/login/login.ts - Remove wipChange: undefined from clone callbackChanged the updateTree clone callback from explicitly setting wipChange: undefined to just spreading the existing stash. Why: The clone callback applies to ancestor nodes when updating a child node. Setting wipChange: undefined here would clear pending changes on parent stashes when syncing child logins. 3. src/core/login/login.ts - healStash preserves client-only fields when discardingconst newStash = { ...stashTree, username: stashTree.wipChange?.username ?? stashTree.username, lastLogin: stashTree.wipChange?.lastLogin ?? stashTree.lastLogin, wipChange: undefined } Why: When healStash discards wipChange (because server confirmed no changes), it was losing the username stored in wipChange. Since username is a client-only field (not on server), it must be preserved from wipChange even when discarding. 4. src/core/context/context-api.ts - Pre-login healing in loginWithPINAdded logic to call healStash before PIN login if the stash has a wipChange with a matching username. Why: Ensures stashes with pending wipChange are healed before attempting to look up by username, so the username is properly resolved before login proceeds. The Core Problem: After backup flow with a simulated network error, the username was stored in wipChange but never made it to the main stash. Various code paths were discarding or overwriting wipChange without preserving the client-only username field, causing "User does not exist" errors on PIN login. |
samholmes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're going with a different approach after a lengthy review-bar. The gist is that we'll be removing healLogin/healStash because it optimistic but also introduces an edge condition which is a broken state. Instead, we'll be only healing by a successful login and continued background sync (which requires that the user is currently logged in). This means we only use the wipChange for attempting loginWithPIN which needs it, and we will use the login-server's userTextBox to decrypt the username that is from the login server (no client-side only state needed to be preserved).
src/core/context/context-api.ts
Outdated
| // Heal any stash with wipChange before looking up by username: | ||
| if (!useLoginId) { | ||
| const username = fixUsername(usernameOrLoginId) | ||
| for (const stash of ai.props.state.login.stashes) { | ||
| if ( | ||
| stash.wipChange != null && | ||
| (stash.username === username || | ||
| stash.wipChange.username === username) | ||
| ) { | ||
| await healStash(ai, stash) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this, just heal stash after stashTree definition if wipChange is not null and then return the healed version to redefine the stashTree. Fail gracefully if the internet is down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or delete this code block because login with key doesn't really require the wipChanges to be healed in order to complete successfully. Only loginWithPIN does.
src/core/context/context-api.ts
Outdated
| // Heal any stash with wipChange before looking up by username: | ||
| if (!useLoginId) { | ||
| const username = fixUsername(usernameOrLoginId) | ||
| for (const stash of ai.props.state.login.stashes) { | ||
| if ( | ||
| stash.wipChange != null && | ||
| (stash.username === username || | ||
| stash.wipChange.username === username) | ||
| ) { | ||
| await healStash(ai, stash) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apply heal after stashTree is gotten like mentioned above.
Tests PassedNetwork Timeout
Expected: Routine immediately catches the timeout error and syncs with the login server. This results in the username being shown and wipChange being removed due to a successful login sync, fully recovering the correct state. Change Fails
Expected: No changes to the account are applied (no username), and PIN login still works with the guest account. Network Disconnect
Expected: No changes to the account are applied until roughly 5 seconds later, while logged in, when the login sync routine recovers the correct state. Username suddenly appears for the account while logged in. Network Disconnect (with Failed Routine Login Sync)
Expected: No changes to the account are applied until after PIN login. |
13cd569 to
1472e50
Compare
src/core/login/login.ts
Outdated
| if (stash.lastLogin != null) out.lastLogin = stash.lastLogin | ||
| if (stash.username != null) out.username = stash.username | ||
| if (stash.userId != null && out.userId == null) out.userId = stash.userId | ||
| if (stash.wipChange != null) out.wipChange = stash.wipChange |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wipChange not cleared on root login after sync
The applyLoginPayloadInner function preserves wipChange on line 123, but the clone function in applyLoginPayload (lines 192-198) that clears wipChange is only called for non-matching nodes. Since updateTree returns immediately when the predicate matches (line 55), clone is never invoked for the matching node. For root logins where the root stash matches, wipChange is incorrectly preserved after a successful server sync, contradicting the comment stating it should be discarded. This causes the expedited sync interval to continue indefinitely and PIN login to unnecessarily attempt the wipChange path.
Additional Locations (1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I, too, am baffled by this line.
swansontec
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small changes and one question.
src/core/account/account-pixie.ts
Outdated
| const { sessionKey } = input.props.state.accounts[accountId] | ||
| const { stashTree } = getStashById( | ||
| toApiInput(input), | ||
| sessionKey.loginId | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead do const { stashTree, sessionKey } = input.props.state.accounts[accountId]. It was sitting there the whole time for free.
src/core/context/context-api.ts
Outdated
| } catch (error) { | ||
| // Fail gracefully with the original stash tree... | ||
| sessionKey = await loginPin2(ai, stashTree, mainStash, pin, opts) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional: This might read cleaner as sessionKey = await loginPin2(...wip...).catch(() => loginPin2(...mainStash...), but your way works too.
src/core/login/login.ts
Outdated
| if (stash.lastLogin != null) out.lastLogin = stash.lastLogin | ||
| if (stash.username != null) out.username = stash.username | ||
| if (stash.userId != null && out.userId == null) out.userId = stash.userId | ||
| if (stash.wipChange != null) out.wipChange = stash.wipChange |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I, too, am baffled by this line.
46abadb to
99f24d0
Compare
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
noneNote
Improves resilience of login-stash updates and sync behavior.
LoginStash.wipChangeand persists WIP changes inapplyKit; clears it on successful server payload (applyLoginPayload) or local stash updatesapplyKit, triggers backgroundsyncLoginand rethrows; PIN login now triesstashTree.wipChangefirst and falls back gracefullyEXPEDITED_SYNC_INTERVALinaccount-pixieusernamefromuserTextBoxwhen presentWritten by Cursor Bugbot for commit 99f24d0. This will update automatically on new commits. Configure here.