From c96cbf0de4ad0c13aa50f7040afbae5e7b7df495 Mon Sep 17 00:00:00 2001 From: LYJW131 <153256373+LYJW131@users.noreply.github.com> Date: Tue, 3 Nov 2026 17:07:58 +0800 Subject: [PATCH 1/2] fix(manager): a login ends only once its service started or its dir is gone Review of #18 found two windows where a retry could lose its files: - A login whose child had already logged in ended (exited) before the account's service started, which ran on its own goroutine; an abort then returned at once and a retry wiped the starting account's dir. The service now starts within the login, an abort waits for it (up to the service's ready timeout), and a replacing login finds the account logged in instead. - A failed login released the account before removing its dir, so a retry could reserve it and write there first. The dir now goes under the registry's lock while the failed login still holds the account. A reaped login child is forgotten, so an abort never signals a reused pid. Co-Authored-By: Claude Opus 5.5 Signed-off-by: LYJW131 <153256373+LYJW131@users.noreply.github.com> --- manager/login.go | 71 ++++++++++++++++++++++++++++++------------------ 1 file changed, 44 insertions(+), 27 deletions(-) diff --git a/manager/login.go b/manager/login.go index f5c72661..65af1bc8 100644 --- a/manager/login.go +++ b/manager/login.go @@ -24,9 +24,10 @@ const loginWait = 90 * time.Second // server's WriteTimeout, which is shorter than a login. const loginWriteTimeout = loginWait + 15*time.Second -// abortWait bounds how long an abort waits for the killed login child to be -// reaped and its half-made instance dir removed. -const abortWait = 10 * time.Second +// abortWait bounds how long an abort waits for the login to end: the killed +// child reaped and its half-made instance dir removed, or, for a child that +// had already logged in, the account's service started. +const abortWait = liteReadyTimeout + 15*time.Second // errLoginCanceled ends a login whose client went away or that a new login of // the same account replaced. @@ -68,8 +69,9 @@ type LoginState struct { aborted bool done chan error // nil => success; error => failure - // exited is closed once runLoginChild has returned: the child is reaped - // and a failed login's instance dir is gone. + // exited is closed once the login has ended: the child is reaped, and a + // failed login's instance dir is gone or a successful one's service has + // started. exited chan struct{} } @@ -124,7 +126,8 @@ func releaseLogin(id string, st *LoginState) bool { // abortLogin kills an in-flight login's child and waits, up to abortWait, // until runLoginChild has reaped it and removed its instance dir. A child -// that has already finished the login is left to finish. +// that has already logged in is not undone: the wait then lasts until the +// account's service has started. func abortLogin(st *LoginState) { st.mu.Lock() st.aborted = true @@ -136,7 +139,7 @@ func abortLogin(st *LoginState) { select { case <-st.exited: case <-time.After(abortWait): - log.Warnf("[wrapper %s] the canceled login's child did not exit within %s", shortID(st.Id), abortWait) + log.Warnf("[wrapper %s] the canceled login did not end within %s", shortID(st.Id), abortWait) } } @@ -233,6 +236,10 @@ func startLogin(ctx context.Context, req LoginRequest) LoginResult { if old := getLogin(id); old != nil { log.Infof("[wrapper %s] a new login replaces the one in flight", shortID(id)) abortLogin(old) + // The old one may have logged in before it was stopped. + if inst := GetInstance(id); inst != nil && inst.Region != "" { + return LoginResult{Code: -1, Msg: "already login", LoginId: id} + } } st, ok := reserveLogin(id) if !ok { @@ -273,8 +280,13 @@ func runLoginChild(id, username, password string, st *LoginState) { for { select { case werr := <-waitCh: - // Child exited. Token files present => success; else classify. - if st.isAborted() { + // Child exited and reaped: its pid may be reused, so an abort + // from now on kills nothing. + st.mu.Lock() + st.cmd = nil + st.mu.Unlock() + // Token files present => success; else classify. + if st.isAborted() && !hasTokenFiles(id) { loginFailed(id, st, errLoginCanceled) return } @@ -390,24 +402,23 @@ func provide2FACode(id string, code string) error { // finishLoginSuccess is called when tokens are on disk: it starts the service // process and, once the instance reports ready, resolves the login. username -// is recorded as the instance's Name. +// is recorded as the instance's Name. It runs within runLoginChild, so the +// login has not ended (exited) until the service has started. func finishLoginSuccess(id, username string, st *LoginState) { - go func() { - syscall.Sync() // the tokens the child wrote, before anything counts on them - instance := getInstanceOrNew(id) - instance.NoRestart = false - instance.Name = username - if err := startLiteService(instance); err != nil { - log.Errorf("[wrapper %s] failed to start after login: %v", shortID(id), err) - loginFailed(id, st, fmt.Errorf("failed to start service: %w", err)) - return - } - if countReady() >= ShouldStartInstances { - setReady(true) - } - SaveInstances() - resolveLogin(id, st, nil) - }() + syscall.Sync() // the tokens the child wrote, before anything counts on them + instance := getInstanceOrNew(id) + instance.NoRestart = false + instance.Name = username + if err := startLiteService(instance); err != nil { + log.Errorf("[wrapper %s] failed to start after login: %v", shortID(id), err) + loginFailed(id, st, fmt.Errorf("failed to start service: %w", err)) + return + } + if countReady() >= ShouldStartInstances { + setReady(true) + } + SaveInstances() + resolveLogin(id, st, nil) } // resolveLogin completes the pending login. @@ -423,9 +434,15 @@ func resolveLogin(id string, st *LoginState, cause error) { // an error. func loginFailed(id string, st *LoginState, cause error) { log.Warnf("[wrapper %s] login failed: %v", shortID(id), cause) - if releaseLogin(id, st) { + // The dir goes while st still holds the account, under the registry's + // lock: a retry cannot reserve it and start writing there in between, + // and a login that replaced st keeps its dir. + loginRegistry.Lock() + if loginRegistry.m[id] == st { _ = RemoveWrapperDataQuiet(id) + delete(loginRegistry.m, id) } + loginRegistry.Unlock() resolveLogin(id, st, cause) } From d30be6f2a0448425f07290635493a403cae35a9a Mon Sep 17 00:00:00 2001 From: LYJW131 <153256373+LYJW131@users.noreply.github.com> Date: Tue, 3 Nov 2026 17:11:39 +0800 Subject: [PATCH 2/2] fix(manager): a timed-out login answers before its abort ends The abort can wait for a service that is starting, past the request's write deadline; it now runs after the answer, and a retry waits for it. The README says that a cancel after the child logged in does not undo the login. Co-Authored-By: Claude Opus 5.5 Signed-off-by: LYJW131 <153256373+LYJW131@users.noreply.github.com> --- manager/README.md | 9 ++++++--- manager/login.go | 12 ++++++++---- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/manager/README.md b/manager/README.md index bbbe1ab1..10bbe77e 100644 --- a/manager/README.md +++ b/manager/README.md @@ -173,11 +173,14 @@ Notes: telling you to restart the login without a code. - Failed logins clean up the instance data directory before reporting. - A login request waits up to 90 seconds (its write deadline is 105). When - that passes, or the client closes the connection, the login child is killed - and its instance dir removed before the answer (`"login timeout"`, or - `"login canceled"` to a client that is gone), so a retry starts clean. + the client closes the connection, the login child is killed and its + instance dir removed before the answer (`"login canceled"`); when the 90 + seconds pass, it answers `"login timeout"` and the same cleanup follows, + which a retry waits for. - A login without a code replaces one of the same account still in flight, such as one waiting for a code: the old child is killed and reaped first. +- A cancel that comes after the login child has logged in does not undo the + login: the account's service starts, and a retry answers `"already login"`. - The instance id is a deterministic UUIDv5 derived from the username, so the `loginId` is stable across attempts. diff --git a/manager/login.go b/manager/login.go index 65af1bc8..aea87015 100644 --- a/manager/login.go +++ b/manager/login.go @@ -328,9 +328,10 @@ func classifyLoginFailure(werr error) error { // waitForLoginFinish blocks until the in-flight login resolves: the child // exits (success/failure), or 2FA becomes required. When 2FA is required it // is reported once (code 2); a second call (the client resubmits with a code) -// blocks until the child exits with the final outcome. When ctx ends or -// loginWait passes first, the login is aborted: its child is killed and its -// instance dir removed before this returns, so a retry starts clean. +// blocks until the child exits with the final outcome. When ctx ends first, +// the login is aborted: its child is killed and its instance dir removed +// before this returns, so a retry starts clean. When loginWait passes first, +// the abort runs on after the answer, and a retry waits for it. func waitForLoginFinish(ctx context.Context, id string, st *LoginState) LoginResult { timeout := time.NewTimer(loginWait) defer timeout.Stop() @@ -349,7 +350,10 @@ func waitForLoginFinish(ctx context.Context, id string, st *LoginState) LoginRes abortLogin(st) return LoginResult{Code: -1, Msg: errLoginCanceled.Error(), LoginId: id} case <-timeout.C: - abortLogin(st) + // The answer must beat the write deadline, and the abort may + // wait for a service that is starting: it ends on its own, and a + // retry waits for it (startLogin). + go abortLogin(st) return LoginResult{Code: -1, Msg: "login timeout", LoginId: id} case <-tick.C: if st.claimPending2FA() {