Repository navigation
Start a stopped database on login failure, then connect - #217
Merged
Merged
Conversation
A login to a Jasper-managed database that isn't running failed with a raw GCI error, leaving the user to go start the stone and NetLDI by hand.
A jetsam kill reports a null exit code and no output, so the start failure message told the user nothing at all.
Revealing it mid-connect yanked focus off the editor; the connect's own progress notification and row spinner already report what is happening.
srbaker
marked this pull request as ready for review
July 23, 2026 19:12
The auto-start recovery bailed entirely the moment either the stone or the NetLDI was running-but-unresponsive -- even when the other process was simply stopped and could be started on its own. The two are started independently, so a wedge on one side is no reason to leave a genuinely-down process on the other side down: it just dead-ends a recovery that could have worked. Offer to start whatever is stopped first, and only fall back to reporting an unresponsive process when nothing can be started. Adds a test for the down-plus-wedged case. Two fixes from the same review, each on its own rationale: - Inline versionsMatch back into processManager. It was already exported from there, so a dedicated module for one small function bought nothing but an extra hop; the reviewer rightly asked why it existed. The two pure consumers import it from processManager again (their tests mock vscode to stay headless -- the one cost of un-splitting). - Correct a stale comment on the auto-start preference command. It claimed the prompt "has no button for Never", but the prompt offers Yes/No/Always/Never; the palette command is for changing the choice later, not for reaching Never. Left as-is it misleads the next reader about what the prompt actually does.
srbaker
force-pushed
the
srbaker/quick-connect-login
branch
from
July 23, 2026 21:16
b9d5b0c to
4a59e73
Compare
…-login # Conflicts: # client/src/extension.ts
maybeStartDatabaseAndRetry documents itself as responsible for reporting the outcome on every path, but it only guarded the failures it expected: a start, and the retry. Remembering the preference and reading the database list were left bare, so an unwritable settings.json or an unreadable database root rejected straight out of the flow. That used to be caught by the login command's own catch, which this branch replaced with a bare finally. Without it VS Code reports "command failed", naming neither the login nor the reason -- strictly less than the error the recovery flow was standing in for. So make the contract true at the source, and keep the command's net as well since a command handler should never reject.
The prompt's Always/Never buttons write ConfigurationTarget.Global, on the reasoning that whether to start a stopped database is a property of how someone likes to work rather than of whichever folder is open. But the setting declared no scope, so it took VS Code's default of "window" -- which a workspace is free to override, and a workspace value outranks the global one. Anyone with such an override would click "Never", have it written where it stays outranked, and be asked again on the next connect, with no sign of why.
srbaker
enabled auto-merge
August 5, 2026 20:52
MatiasFernandez
approved these changes
Aug 6, 2026
MatiasFernandez
left a comment
MatiasFernandez
left a comment
Collaborator
There was a problem hiding this comment.
I left some comments worth checking
Four things Matias raised on #217. The connecting spinner was held by the login's position in the list, which outlives the render it came from: starting a stone and its NetLDI takes seconds, the tree stays interactive, and a login deleted or reordered meanwhile left the `finally`'s clear landing on whichever login had inherited that slot — the row actually connecting kept spinning with nothing able to stop it. It is keyed by the login's identity now, so both the spinner and the clearing follow the login rather than the slot. A signal-killed process claimed memory pressure whatever the signal was. Only SIGKILL earns that: it is what macOS jetsam does. A SIGSEGV from a broken install or a SIGTERM from someone running kill now say so plainly, instead of sending the reader off to free memory that was never the problem. The test that covered this accepted any of three words, so it passed either way; it is two tests now, one that requires the memory advice and one that forbids it. `isLocalHost` answers "is this login on this machine" in one place, and Serve Seaside's inline copy of the same predicate now asks it. It also accepts `::1` and `[::1]`: an IPv6-loopback login is the same machine, but read as remote it would quietly lose the auto-start offer, with the user seeing only the original login failure again and nothing explaining why. And `resolvedGciPath` says why it exists — `gciPath` is a `let`, which TypeScript widens back to `string | undefined` inside the async closure — so the next reader does not tidy it away and hit a type error they cannot place.
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.
Why
Logging in to a database that is not running just fails with an unhelpful error. You then have to leave the login, find the database, start it by hand, and try again.
What changed
not-respondingis reported only when both are up, where starting cannot help.maybeStartDatabaseAndRetrydocuments itself as reporting the outcome on every path, but only guarded the failures it expected -- remembering the preference and reading the database list were bare, so an unwritablesettings.jsonrejected out of the flow. With this branch having replaced the commandscatchwith a barefinally`, that surfaced as "command failed", naming neither the login nor the reason. The contract is now true at the source, and the command keeps a net as well.ConfigurationTarget.Global, but the setting declared no scope and so took VS Codes default ofwindow` -- which a workspace outranks. Anyone with such an override would click Never, have it written where it stays outranked, and be asked again with no sign of why.Verification
vscepackaging.compile,lint,format:checkclean; 4509 client unit tests pass. The remaining local failures are the integration suites that need a live stone, which CI covers above.