Skip to content

Commit 60d6ee9

Browse files
authored
Merge pull request #771 from nitrobass24/fix/ssh-picker-followups
fix: SSH test-connection / directory-picker follow-ups (stacked on #726)
2 parents 1e3f2c4 + 13e8f07 commit 60d6ee9

12 files changed

Lines changed: 162 additions & 34 deletions

File tree

‎src/angular/src/app/pages/settings/directory-picker.component.spec.ts‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ function result(overrides: Partial<BrowseResult> = {}): BrowseResult {
1313
parent: '/',
1414
directories: ['sub1', 'sub2'],
1515
errorMessage: null,
16+
badPath: false,
1617
...overrides,
1718
};
1819
}
@@ -112,8 +113,8 @@ describe('DirectoryPickerComponent', () => {
112113
expect(cancelled).toBe(true);
113114
});
114115

115-
function failure(errorMessage: string): BrowseResult {
116-
return { success: false, path: null, parent: null, directories: [], errorMessage };
116+
function failure(errorMessage: string, badPath = true): BrowseResult {
117+
return { success: false, path: null, parent: null, directories: [], errorMessage, badPath };
117118
}
118119

119120
it('falls back to the nearest listable ancestor when the initial path fails, keeping the error', () => {
@@ -138,17 +139,26 @@ describe('DirectoryPickerComponent', () => {
138139
});
139140

140141
it('stops at "/" and disables Select when nothing lists', () => {
141-
mockBrowserService.browseRemote.mockReturnValue(of(failure('Connection refused')));
142+
mockBrowserService.browseRemote.mockReturnValue(of(failure('No such file or directory')));
142143
const fixture = createFixture('remote', '~/gone');
143144

144145
expect(mockBrowserService.browseRemote.mock.calls.map((c) => c[0])).toEqual(['~/gone', '~', '/']);
145-
expect(fixture.componentInstance.errorMessage).toBe('Connection refused');
146+
expect(fixture.componentInstance.errorMessage).toBe('No such file or directory');
146147
const select = Array.from(fixture.nativeElement.querySelectorAll('button') as NodeListOf<HTMLButtonElement>).find(
147148
(b) => b.textContent?.includes('Select This Folder'),
148149
);
149150
expect(select?.disabled).toBe(true);
150151
});
151152

153+
it('does not walk up on a connection failure, which would be one more SSH login per ancestor', () => {
154+
mockBrowserService.browseRemote.mockReturnValue(of(failure('Connection refused', false)));
155+
const component = createComponent('remote', '/downloads/a/b/c');
156+
157+
expect(mockBrowserService.browseRemote).toHaveBeenCalledTimes(1);
158+
expect(component.errorMessage).toBe('Connection refused');
159+
expect(component.loaded).toBe(false);
160+
});
161+
152162
it('does not walk up when a later navigation fails', () => {
153163
const component = createComponent('local', '/downloads');
154164
mockBrowserService.browseLocal.mockClear();
@@ -165,7 +175,7 @@ describe('DirectoryPickerComponent', () => {
165175
of(result({ success: true, errorMessage: null })).pipe(),
166176
);
167177
mockBrowserService.browseLocal.mockReturnValue(
168-
of({ success: false, path: null, parent: null, directories: [], errorMessage: 'Permission denied' }),
178+
of(failure('Permission denied')),
169179
);
170180

171181
component.onEnterDirectory('locked');

‎src/angular/src/app/pages/settings/directory-picker.component.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,10 @@ export class DirectoryPickerComponent implements OnInit {
7272
}
7373

7474
/** @param fallBackToParent Initial load only: if the configured path no longer
75-
* lists (renamed, now a file), walk up its ancestors to the first one that
76-
* does, keeping the original error visible, so the picker isn't a dead end. */
75+
* exists (renamed, now a file), walk up its ancestors to the first one that
76+
* lists, keeping the original error visible, so the picker isn't a dead end.
77+
* Connection-level failures stop immediately: each remote step is a fresh
78+
* SSH login, and retrying one that just failed is what the lockout avoids. */
7779
private load(path: string, options?: { fallBackToParent?: boolean; keepError?: string }): void {
7880
this.loading = true;
7981
this.errorMessage = options?.keepError ?? null;
@@ -88,7 +90,7 @@ export class DirectoryPickerComponent implements OnInit {
8890
this.directories = result.directories;
8991
} else {
9092
const error = options?.keepError ?? result.errorMessage;
91-
const parent = options?.fallBackToParent ? parentPathOf(path) : null;
93+
const parent = options?.fallBackToParent && result.badPath ? parentPathOf(path) : null;
9294
if (parent !== null) {
9395
this.load(parent, { fallBackToParent: true, keepError: error ?? undefined });
9496
return;

‎src/angular/src/app/pages/settings/option.component.spec.ts‎

Lines changed: 28 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,16 @@
11
import '@angular/compiler';
22
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
33
import { Subject, Subscription } from 'rxjs';
4-
import { debounceTime, distinctUntilChanged } from 'rxjs/operators';
4+
import { debounceTime } from 'rxjs/operators';
55
import { TestBed } from '@angular/core/testing';
66
import { ComponentFixture } from '@angular/core/testing';
77

88
import { OptionComponent, OptionType, OptionValue, DEBOUNCE_TIME_MS } from './option.component';
99
import { REDACTED_SENTINEL } from '../../models/config';
1010

1111
/**
12-
* Test the debounce/distinctUntilChanged pipe logic directly using the same
13-
* operators as OptionComponent, avoiding Angular component lifecycle timing issues.
12+
* Test the debounce pipe logic directly using the same operator as
13+
* OptionComponent, avoiding Angular component lifecycle timing issues.
1414
*/
1515
describe('OptionComponent — debounce pipe logic', () => {
1616
let newValue: Subject<OptionValue>;
@@ -22,7 +22,7 @@ describe('OptionComponent — debounce pipe logic', () => {
2222
newValue = new Subject<OptionValue>();
2323
emitted = [];
2424
subscription = newValue
25-
.pipe(debounceTime(DEBOUNCE_TIME_MS), distinctUntilChanged())
25+
.pipe(debounceTime(DEBOUNCE_TIME_MS))
2626
.subscribe((val) => emitted.push(val));
2727
});
2828

@@ -52,16 +52,6 @@ describe('OptionComponent — debounce pipe logic', () => {
5252
expect(emitted).toEqual(['c']);
5353
});
5454

55-
it('should not re-emit same value after full debounce period (distinctUntilChanged)', () => {
56-
newValue.next('same');
57-
vi.advanceTimersByTime(DEBOUNCE_TIME_MS);
58-
expect(emitted).toEqual(['same']);
59-
60-
newValue.next('same');
61-
vi.advanceTimersByTime(DEBOUNCE_TIME_MS);
62-
expect(emitted).toEqual(['same']);
63-
});
64-
6555
it('should emit different values after each debounce period', () => {
6656
newValue.next('first');
6757
vi.advanceTimersByTime(DEBOUNCE_TIME_MS);
@@ -92,6 +82,30 @@ describe('OptionComponent — onChange and effectiveChoices', () => {
9282
vi.useRealTimers();
9383
});
9484

85+
it('emits only when the typed value differs from the bound value, even after an external change', () => {
86+
vi.useFakeTimers();
87+
fixture.componentRef.setInput('value', '/a');
88+
fixture.detectChanges();
89+
const emitted: OptionValue[] = [];
90+
component.changeEvent.subscribe((v) => emitted.push(v));
91+
92+
component.onChange('/a');
93+
vi.advanceTimersByTime(DEBOUNCE_TIME_MS);
94+
expect(emitted).toEqual([]);
95+
96+
component.onChange('/b');
97+
vi.advanceTimersByTime(DEBOUNCE_TIME_MS);
98+
expect(emitted).toEqual(['/b']);
99+
100+
// The directory picker saves '/x' through a different path; typing '/b'
101+
// again is a real change now and must not be swallowed.
102+
fixture.componentRef.setInput('value', '/x');
103+
fixture.detectChanges();
104+
component.onChange('/b');
105+
vi.advanceTimersByTime(DEBOUNCE_TIME_MS);
106+
expect(emitted).toEqual(['/b', '/b']);
107+
});
108+
95109
it('should suppress REDACTED_SENTINEL for password type', () => {
96110
fixture.componentRef.setInput('type', OptionType.Password);
97111
fixture.detectChanges();

‎src/angular/src/app/pages/settings/option.component.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { Component, ChangeDetectionStrategy, OnInit, OnDestroy, input, output, computed } from '@angular/core';
22
import { FormsModule } from '@angular/forms';
3-
import { Subject, Subscription, debounceTime, distinctUntilChanged } from 'rxjs';
3+
import { Subject, Subscription, debounceTime, filter } from 'rxjs';
44
import { REDACTED_SENTINEL } from '../../models/config';
55

66
export enum OptionType {
@@ -63,8 +63,11 @@ export class OptionComponent implements OnInit, OnDestroy {
6363
private subscription?: Subscription;
6464

6565
ngOnInit(): void {
66+
// Compare against the bound value, not the last emission: the value can
67+
// also change from outside (e.g. the directory picker), after which
68+
// re-typing the previously emitted value is a real change again.
6669
this.subscription = this.newValue
67-
.pipe(debounceTime(DEBOUNCE_TIME_MS), distinctUntilChanged())
70+
.pipe(debounceTime(DEBOUNCE_TIME_MS), filter((val) => val !== this.value()))
6871
.subscribe({ next: (val) => this.changeEvent.emit(val) });
6972
}
7073

‎src/angular/src/app/pages/settings/settings-page.component.spec.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -185,6 +185,25 @@ describe('SettingsPageComponent connection-field changes invalidate a verified c
185185

186186
expect(component.connectionVerified).toBe(true);
187187
});
188+
189+
it('discards a Test Connection result that was in flight when a connection field changed', () => {
190+
const resultSubject = new Subject<ConnectionTestResult>();
191+
const testConnection = TestBed.inject(ConnectionTestService).testConnection as ReturnType<typeof vi.fn>;
192+
testConnection.mockReturnValue(resultSubject.asObservable());
193+
component.connectionVerified = false;
194+
component.connectionLockedOut = false;
195+
196+
component.onTestConnection();
197+
component.onSetConfig('lftp', 'remote_password', 'new-password');
198+
setSubject.next({ success: true, data: 'lftp.remote_password updated', errorMessage: null });
199+
resultSubject.next({ success: false, message: 'Incorrect password', credentialError: true });
200+
201+
expect(component.connectionLockedOut).toBe(false);
202+
expect(component.connectionVerified).toBe(false);
203+
expect(component.connectionResult).toBeNull();
204+
expect(component.testingConnection).toBe(false);
205+
expect(component.connectionTestInFlight).toBe(false);
206+
});
188207
});
189208

190209
describe('SettingsPageComponent directory picker', () => {

‎src/angular/src/app/pages/settings/settings-page.component.ts‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,9 @@ export class SettingsPageComponent implements OnInit {
109109
/** Set for both manual tests and the silent page-load probe, so a click
110110
* can't start a second SSH login while one is still running. */
111111
connectionTestInFlight = false;
112+
/** Bumped when a connection field changes so a test started against
113+
* the old credentials can't mark the new ones verified (or locked out). */
114+
private connectionTestSeq = 0;
112115
connectionResult: TestResult | null = null;
113116
/** True once a Test Connection call has succeeded against the currently
114117
* saved lftp connection fields. Gates the Server Directory picker. */
@@ -137,6 +140,15 @@ export class SettingsPageComponent implements OnInit {
137140
private badValueNotifs = new Map<string, Notification>();
138141

139142
ngOnInit(): void {
143+
// Seed from the shared flag so a revisit doesn't re-lock the Server
144+
// Directory picker (and re-probe) when the connection is already verified.
145+
this.connectionStatusService.verified$.pipe(
146+
takeUntilDestroyed(this.destroyRef),
147+
).subscribe((verified) => {
148+
this.connectionVerified = verified;
149+
this.cdr.markForCheck();
150+
});
151+
140152
this.connectedService.connected$.pipe(
141153
takeUntilDestroyed(this.destroyRef),
142154
).subscribe({
@@ -185,7 +197,8 @@ export class SettingsPageComponent implements OnInit {
185197
take(1),
186198
takeUntilDestroyed(this.destroyRef),
187199
).subscribe(([config]) => {
188-
if (config.lftp?.remote_address && config.lftp?.remote_username && config.lftp?.remote_port) {
200+
const configured = config.lftp?.remote_address && config.lftp?.remote_username && config.lftp?.remote_port;
201+
if (configured && !this.connectionVerified) {
189202
this.onTestConnection({ silent: true });
190203
}
191204
});
@@ -231,6 +244,9 @@ export class SettingsPageComponent implements OnInit {
231244
// and clears a credential-error lockout since the credentials are
232245
// no longer what caused it.
233246
if (section === 'lftp' && CONNECTION_FIELDS.has(option)) {
247+
this.connectionTestSeq++;
248+
this.connectionTestInFlight = false;
249+
this.testingConnection = false;
234250
this.setConnectionVerified(false);
235251
this.connectionResult = null;
236252
this.connectionLockedOut = false;
@@ -249,6 +265,7 @@ export class SettingsPageComponent implements OnInit {
249265

250266
this.logger.error(reaction.errorMessage);
251267
}
268+
this.cdr.markForCheck();
252269
},
253270
});
254271
}
@@ -302,9 +319,14 @@ export class SettingsPageComponent implements OnInit {
302319
if (!silent) {
303320
this.connectionResult = null;
304321
}
322+
this.cdr.markForCheck();
323+
const seq = ++this.connectionTestSeq;
305324
this.connectionTestService.testConnection().pipe(
306325
takeUntilDestroyed(this.destroyRef),
307326
).subscribe((result) => {
327+
if (seq !== this.connectionTestSeq) {
328+
return; // credentials changed mid-flight; result is for the old ones
329+
}
308330
this.connectionTestInFlight = false;
309331
this.testingConnection = false;
310332
if (!silent) {

‎src/angular/src/app/services/settings/directory-browser.service.spec.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ describe('DirectoryBrowserService', () => {
4040
parent: '/',
4141
directories: ['a', 'b'],
4242
errorMessage: null,
43+
badPath: false,
4344
});
4445
});
4546

@@ -69,6 +70,19 @@ describe('DirectoryBrowserService', () => {
6970
expect(result!.success).toBe(false);
7071
expect(result!.errorMessage).toBe('Connection refused');
7172
expect(result!.directories).toEqual([]);
73+
expect(result!.badPath).toBe(false);
74+
});
75+
76+
it('flags a 400 as a bad path so the picker can walk up to a parent', () => {
77+
let result: BrowseResult | undefined;
78+
service.browseRemote('/gone').subscribe((r) => (result = r));
79+
80+
httpMock.expectOne((r) => r.url === '/server/browse/remote').flush(
81+
{ error: 'find: /gone: No such file or directory' },
82+
{ status: 400, statusText: 'Bad Request' },
83+
);
84+
85+
expect(result!.badPath).toBe(true);
7286
});
7387

7488
it('falls back to a generic message when the error body is unparsable', () => {

‎src/angular/src/app/services/settings/directory-browser.service.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@ export interface BrowseResult {
1010
readonly parent: string | null;
1111
readonly directories: string[];
1212
readonly errorMessage: string | null;
13+
/** True when the server rejected the path itself (400) rather than failing
14+
* to reach the filesystem (502) — the picker only walks up on the former. */
15+
readonly badPath: boolean;
1316
}
1417

1518
interface BrowseResponse {
@@ -42,6 +45,7 @@ export class DirectoryBrowserService {
4245
parent: body.parent,
4346
directories: body.directories,
4447
errorMessage: null,
48+
badPath: false,
4549
})),
4650
catchError((err: HttpErrorResponse) =>
4751
of({
@@ -50,6 +54,7 @@ export class DirectoryBrowserService {
5054
parent: null,
5155
directories: [],
5256
errorMessage: failureFromHttpError(err, 'Failed to browse directory').message,
57+
badPath: err.status === 400,
5358
}),
5459
),
5560
);

‎src/python/seedsync.py‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -300,18 +300,18 @@ def _handle_controller_failure(self, controller_job: ControllerJob, e: Exception
300300
set up before the loop starts: status.server.up/error_msg surface the
301301
error in the UI, and the controller only restarts via an explicit
302302
restart request (same as any other change requiring one), not
303-
automatically. controller_job's own thread has already run its
304-
cleanup (see Job.run()/ControllerJob.cleanup(), which calls
305-
Controller.exit()) by the time propagate_exception() has something to
306-
raise, so terminate()/join() here just formally reap it.
303+
automatically. controller_job's thread records the exception before
304+
running its cleanup (see Job.run()/ControllerJob.cleanup(), which
305+
calls Controller.exit()), so join() here waits for that teardown.
307306
"""
308307
if isinstance(e, (ServiceExit, ServiceRestart)) or self.context.args.exit or not is_credential_error(str(e)):
309308
raise e
310309
self.context.logger.exception("Controller stopped due to a credential error; web app remains available")
311-
controller_job.terminate()
312-
controller_job.join()
310+
# Publish the error first: join() waits for Controller.exit() to finish.
313311
self.context.status.server.up = False
314312
self.context.status.server.error_msg = str(e)
313+
controller_job.terminate()
314+
controller_job.join()
315315

316316
def persist(self):
317317
# Save the persists

‎src/python/tests/integration/test_web/test_handler/test_browse.py‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,12 @@ def test_nonexistent_path_is_rejected(self):
6767
)
6868
self.assertEqual(400, resp.status_int)
6969

70+
def test_tilde_expands_to_home(self):
71+
with patch.dict(os.environ, {"HOME": self.browse_root}):
72+
resp = self.test_app.get("/server/browse/local", {"path": "~"})
73+
self.assertEqual(200, resp.status_int)
74+
self.assertEqual(self.browse_root, json.loads(resp.text)["path"])
75+
7076

7177
class TestBrowseHandlerRemote(BaseTestWebApp):
7278
def _configure_remote(self):
@@ -123,6 +129,20 @@ def test_ssh_failure_surfaces_error(self):
123129
body = json.loads(resp.text)
124130
self.assertEqual("Connection refused", body["error"])
125131

132+
def test_missing_remote_directory_is_a_bad_request_not_a_gateway_error(self):
133+
self._configure_remote()
134+
error = SshcpError("find: /remote/gone: No such file or directory")
135+
with patch("ssh.lftp_ssh.Sshcp.shell", side_effect=error):
136+
resp = self.test_app.get("/server/browse/remote", {"path": "/remote/gone"}, expect_errors=True)
137+
self.assertEqual(400, resp.status_int)
138+
self.assertIn("No such file or directory", json.loads(resp.text)["error"])
139+
140+
def test_directory_names_are_split_on_newline_only(self):
141+
self._configure_remote()
142+
with patch("ssh.lftp_ssh.Sshcp.shell", return_value=b"/remote/dir/Foo\x0bBar\n"):
143+
resp = self.test_app.get("/server/browse/remote", {"path": "/remote/dir"})
144+
self.assertEqual(["Foo\x0bBar"], json.loads(resp.text)["directories"])
145+
126146
def test_path_with_spaces_is_shell_quoted(self):
127147
self._configure_remote()
128148
with patch("ssh.lftp_ssh.Sshcp.shell", return_value=b"") as mock_shell:

0 commit comments

Comments
 (0)