Skip to content

Commit

Permalink
fix(gui): Smoothening startup Part I (#355)
Browse files Browse the repository at this point in the history
I think this renders best if split in two pull requests: one dealing
with the lower level code (this one) and another dealing with the
visible parts of the user interface. This way it's easier for reviewers
and also unblocks @EduardGomezEscandell's work more quickly.

So in this PR we reduce the duration of a timeout in case no changes in
the startup state machine happen and we watch for the creation of the
`addr` file. The watch only happens in the code path that causes the GUI
to start the agent. For the watch operation to happen we need the
directory to exist, so I added a precaution step to create it. It
doesn't fail in case the directory already exists. It's important that
we start watching the directory before launching the agent, otherwise
race conditions could happen. Thus we start watching first, then launch
the agent, and finally wait for the watching operation to complete
(which may have completed by the time we start waiting, which is not a
problem at all). This prevents the GUI from spawning more than one agent
process.

The last commit increases the timeout of one specific end-to-end test
because I could see it failing before in CI killing the context,
although I could never reproduce it locally not even with VMs.
  • Loading branch information
CarlosNihelton authored Oct 31, 2023
2 parents 7150b63 + 94b21f7 commit 6699249
Show file tree
Hide file tree
Showing 5 changed files with 36 additions and 4 deletions.
2 changes: 1 addition & 1 deletion end-to-end/organization_token_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ func TestOrganizationProvidedToken(t *testing.T) {
require.NoErrorf(t, err, "Setup: could not wake distro up: %v. %s", err, out)
}

const maxTimeout = 15 * time.Second
const maxTimeout = 30 * time.Second

if !tc.wantAttached {
time.Sleep(maxTimeout)
Expand Down
8 changes: 7 additions & 1 deletion gui/packages/ubuntupro/lib/pages/startup/agent_monitor.dart
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ class AgentStartupMonitor {
/// The loop stops if a terminal condition is found or [timeout] expires.
Stream<AgentState> start({
Duration interval = const Duration(seconds: 1),
Duration timeout = const Duration(seconds: 30),
Duration timeout = const Duration(seconds: 5),
}) async* {
if (_addrFilePath == null) {
// Terminal state, cannot recover nor retry.
Expand Down Expand Up @@ -127,10 +127,16 @@ class AgentStartupMonitor {
// Terminal state, cannot recover nor retry.
return AgentState.unknownEnv;
case AgentAddrFileError.nonexistent:
// The directory must exist so we can watch for changes. This won't fail if the directory already exists.
final agentDir = await File(_addrFilePath!).parent.create();
final watch = agentDir
.watch(events: FileSystemEvent.create)
.firstWhere((event) => event.path.contains(_addrFilePath!));
if (!await agentLauncher()) {
// Terminal state, cannot recover nor retry.
return AgentState.cannotStart;
}
await watch;
return AgentState.starting;
// maybe a race condition allowed us to read the file before write completed? Retry.
// ignore: switch_case_completes_normally
Expand Down
26 changes: 26 additions & 0 deletions gui/packages/ubuntupro/test/startup/agent_monitor_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,32 @@ void main() {
verify(mockClient.ping()).called(1);
});

test('timeout if never addr', () async {
final mockClient = MockAgentApiClient();
// Fakes a successful ping.
when(mockClient.ping()).thenAnswer((_) async => true);
final monitor = AgentStartupMonitor(
/// A launch request will always succeed, but never writes the addr file.
agentLauncher: () async {
return true;
},
clientFactory: (port) => mockClient,
appName: kAppName,
addrFileName: kAddrFileName,
onClient: (_) {},
);

await expectLater(
monitor.start(interval: kInterval),
emitsInOrder([
AgentState.querying,
// instead of waiting the addr file to show up forever, we are caught by our internal timeout.
AgentState.unreachable,
]),
);
verifyNever(mockClient.ping());
});

test('await async onClient callback', () async {
final completeMe = Completer<void>();
final mockClient = MockAgentApiClient();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ class MockAgentStartupMonitor extends _i1.Mock
@override
_i4.Stream<_i3.AgentState> start({
Duration? interval = const Duration(seconds: 1),
Duration? timeout = const Duration(seconds: 30),
Duration? timeout = const Duration(seconds: 5),
}) =>
(super.noSuchMethod(
Invocation.method(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ class MockAgentStartupMonitor extends _i1.Mock
@override
_i5.Stream<_i4.AgentState> start({
Duration? interval = const Duration(seconds: 1),
Duration? timeout = const Duration(seconds: 30),
Duration? timeout = const Duration(seconds: 5),
}) =>
(super.noSuchMethod(
Invocation.method(
Expand Down

0 comments on commit 6699249

Please sign in to comment.