refactor(kbhttp): one reconciling goroutine for both local servers - #29652
Draft
chrisnojima wants to merge 4 commits into
Draft
chrisnojima wants to merge 4 commits into
chrisnojima wants to merge 4 commits into
Conversation
chrisnojima
added this pull request to stack #29658
September 21, 2026 18:56
This was referenced Sep 21, 2026
StartWithHandlers populates the ServeMux under the same lock that binds the listener, so no request can reach a freshly started server before its handlers exist.
manager.Srv now owns its kbhttp.Srv from a single goroutine: run reads the app state it started in, then reconciles on every app state change, handler registration and shutdown. Handlers are registered into the mux as the listener binds, so a restart can't answer a 404, and the last bound address is kept while the server is stopped so URLs built then point at where it comes back. kbfs's libhttpserver drops its own restart/monitorAppState/serverLock copy of this logic and takes a manager.Srv, passing its env.AppStateUpdater methods as the app state functions. The server now stays up in INACTIVE, so chat's getURL no longer refuses to build a URL while it is down, and GetBootstrapStatus reads the address once instead of polling Addr for two seconds. The service builds its Srv in SetupCriticalSubServices rather than NewService: the new Srv reads NotifyRouter once at construction, so the router has to exist first.
A retry of a 127.0.0.1 src rewrote nothing but a cache-buster, so an attachment url baked with an old port or an old per-process token kept failing. localhost-src rewrites both from the httpSrv info the service announced. The server now stays up while the app is inactive, so only BACKGROUND holds off the retry.
kbfs runs a second local server on 127.0.0.1 with its own port and its own token, and its /files/ urls reach the same native Image. Matching on the 127.0.0.1 prefix alone rewrote those to the service's address and token, so a transient failure became a permanent wrong-port 403. Repoint only the service's own endpoints; everything else still gets the cache-buster.
chrisnojima
force-pushed
the
nojima/HOTPOT-as-04-kbhttp
branch
from
September 22, 2026 23:08
bb3a6e2 to
8d1d7d0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stack 4/9, splitting #29637. Parent: #29651.
Why
The local http server serves chat attachments, avatars and maps to the app's own webviews and image components. When it is down, images silently fail. On master there are two separate implementations of "keep a local server alive across app-state changes" — the service's and KBFS's — each with its own monitor, lock and restart path, and neither recovers from the server dying on its own.
Master bugs this fixes
startHTTPSrvstarts the server and returns; nothing watches forServeexiting. If it exits unexpectedly, the server stays down until the next app-state transition happens to restart it. Until then every attachment, avatar and map fails to load.libhttpserverhas its ownrestart(),monitorAppState(), lock and cancel func, so a fix has to be made twice and the two have already drifted — KBFS's monitor only ever restarted, never stopped.config.gopollsAddr()for up to 2 seconds to learn the address instead of asking once.What this changes
One reconciling goroutine owns both servers.
manager.NewtakesappState/nextAppStateas plain function parameters rather than an interface — that is what lets KBFS pass its ownAppStateUpdaterand the service passMobileAppStatewithout a shared type.reconciletears the server down only in BACKGROUND, and only wherestopInBackground. The server restarts after an unexpectedServeexit, keeps its last bound address while stopped, and rebinds when leaving the background.libhttpserverdeletes its own machinery and delegates.config.gomakes oneInfo()call.httpSrvmoves fromNewServicetoSetupCriticalSubServicesbecause the newSrvreads the notify router once at construction, and the router does not exist yet inNewService.Also includes the localhost-src slice: a retried image src is repointed at the current server address and token, so an image baked with an old address recovers instead of failing forever.
One fix beyond a straight port of #29637
#29637 repoints any
http://127.0.0.1:PORTsrc. KBFS serves/files/from a second local server on its own port with its own independently-rotated token, so that rewrote KBFS URLs to the chat server's address and token — turning a transient image failure into a permanent wrong-port/403. Before this PR the retry only appended a cache-buster, so the regression would have been introduced here. Repointing is now gated on the service's own endpoints (/at,/av,/map); everything else still gets the cache-buster. Reachable from the KBFS file preview, which passes these URLs toZoomableImage. Regression test added.Verification
go build,go vet,gofmt,golangci-lint --new-from-rev master0 issues. kbhttp, libhttpserver and attachment tests pass, including under-raceand repeated runs.yarn lint:allclean, unit suite green.Worth a decision during review:
stopInBackgroundmeans KBFS's server is now torn down in BACKGROUND and no longer binds atNew()on mobile, where the initial state is BACKGROUND — soServer.Address()andGetGUIFileContextfail until the first foreground. That is a real behaviour change from master, where it was never stopped.