Skip to content

Commit 4750483

Browse files
rustyconoverclaude
andcommitted
test_launcher_e2e: make the launch: idle-respawn case real
"ResolveAndConnect respawns an idle-shutdown launch: worker" had no checks: a comment, `(void)location; // SKIPPED`, and so a pass on every run. It was written in beafc4c, when ResolveAndConnect could only launch with LaunchConfig's 300 s idle timeout, far too long to wait out in a test. LaunchOverrides landed the same day (c3143fa) and now carries idle_timeout and state_dir through ResolveAndConnect, but the placeholder was never revisited. Nothing else covered the path it names: the unix:// case asserts an IOException (nothing respawns those), and "retries once when the cached path goes stale" clears the cache and reconnects to a live worker, so its retry never fires. The case now drives real processes. It resolves a `launch:` location with a 1 s idle timeout in an isolated state dir, records the worker's PID from the kernel (SO_PEERCRED on Linux, LOCAL_PEERPID on macOS), disconnects, waits for the worker to idle-shut-down and unlink its socket, asserts the cache still hands out that dead path, and calls ResolveAndConnect again. It must connect, to a different PID at the same per-hash path. The cache entry is dropped on every exit so its override pin cannot leak into later cases as a BinderException. With ResolveAndConnect's invalidate-and-relaunch retry temporarily replaced by a rethrow, the case fails with the IOException for the dead socket; restored, it passes (first worker 4019193, respawned 4019201). All 50 [launcher] cases pass. The macOS branch was syntax-checked only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 045e453 commit 4750483

1 file changed

Lines changed: 70 additions & 21 deletions

File tree

‎test/cpp/test_launcher_e2e.cpp‎

Lines changed: 70 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,25 @@ static bool WaitForPathGone(const std::string &path, std::chrono::milliseconds t
111111
return false;
112112
}
113113

114+
// PID of the worker listening on the far end of a connected AF_UNIX socket, as
115+
// the kernel recorded it. Tells one worker process from another without taking
116+
// the worker's word for anything.
117+
static pid_t PeerPid(const UnixSocket &sock) {
118+
#if defined(__linux__)
119+
struct ucred cred {};
120+
socklen_t len = sizeof(cred);
121+
REQUIRE(::getsockopt(sock.GetFd(), SOL_SOCKET, SO_PEERCRED, &cred, &len) == 0);
122+
return cred.pid;
123+
#elif defined(__APPLE__)
124+
pid_t pid = 0;
125+
socklen_t len = sizeof(pid);
126+
REQUIRE(::getsockopt(sock.GetFd(), SOL_LOCAL, LOCAL_PEERPID, &pid, &len) == 0);
127+
return pid;
128+
#else
129+
#error "PeerPid: no AF_UNIX peer-PID query for this platform"
130+
#endif
131+
}
132+
114133
// ---------------------------------------------------------------------------
115134

116135
TEST_CASE("Launch spawns a worker and connects to its socket", "[launcher][e2e]") {
@@ -400,28 +419,58 @@ TEST_CASE("ResolveAndConnect transparently respawns an idle-shutdown worker",
400419
}
401420

402421
TEST_CASE("ResolveAndConnect respawns an idle-shutdown launch: worker", "[launcher][e2e][slow]") {
403-
// Same scenario as above but with the ``launch:`` scheme, where the
404-
// cache invalidation triggers a fresh Launch() that brings up a new
405-
// worker. This is the path that actually self-heals. Sleeps past the
406-
// idle timeout to force the cached worker to disappear, then asserts
407-
// the next ResolveAndConnect call succeeds without manual intervention.
408-
IsolatedStateDir dir;
409-
auto worker_argv = TestWorkerPath();
410-
std::string location = "launch:" + worker_argv;
411-
412-
// Set up a tight idle timeout via the launch worker's argv pass-through.
413-
// The launcher pipeline appends --idle-timeout SEC to whatever argv we
414-
// give it; we can't change the SEC value through ResolveAndConnect
415-
// (it's hard-coded to LaunchConfig defaults). Instead, use Launch()
416-
// directly with a 1-s idle to set up the worker, then exercise the
417-
// cache path by passing the resolved-back location.
422+
// The self-healing path a long-lived session depends on. The cached
423+
// `launch:` worker idle-shuts down while the cache still names its socket,
424+
// so the next ResolveAndConnect is refused on the dead path and must
425+
// invalidate the entry and bring up a fresh worker, with nothing flushed
426+
// by hand. (The unix:// case above ends in an IOException instead: nothing
427+
// respawns an operator-managed worker.)
418428
//
419-
// The simpler shape: Launch via the cache (which uses default 300s
420-
// idle). Wait briefly, then verify a second ResolveAndConnect works.
421-
// This doesn't exercise the actual stale-after-idle path, so we use
422-
// the more targeted unit test above for that. Here we just verify
423-
// that ResolveAndConnect doesn't break the happy-path round-trip.
424-
(void)location; // SKIPPED — covered by the unix:// variant + unit-level retry test below.
429+
// This case was an empty placeholder until LaunchOverrides existed:
430+
// ResolveAndConnect could only launch with LaunchConfig's 300 s idle
431+
// timeout, far too long to wait out in a test. The overrides now carry a
432+
// short idle timeout (and an isolated state dir) through it.
433+
IsolatedStateDir dir;
434+
const std::string location = "launch:" + TestWorkerPath();
435+
duckdb::vgi::LaunchOverrides overrides;
436+
overrides.idle_timeout = 1s;
437+
overrides.state_dir = dir.path();
438+
439+
// The cache is process-wide and pins overrides per location. Start clean,
440+
// and drop this entry on every exit, a failed REQUIRE included, so a later
441+
// case resolving the same location with other overrides is not refused
442+
// with a BinderException.
443+
duckdb::vgi::ClearLauncherSocketCache();
444+
struct ForgetLocation {
445+
std::string location;
446+
~ForgetLocation() {
447+
duckdb::vgi::InvalidateLauncherSocketCache(location);
448+
}
449+
} forget {location};
450+
451+
std::string path;
452+
pid_t first_pid = 0;
453+
{
454+
auto sock = duckdb::vgi::ResolveAndConnect(location, 10s, overrides);
455+
REQUIRE(sock.IsOpen());
456+
first_pid = PeerPid(sock);
457+
path = duckdb::vgi::ResolveLauncherSocketPath(location, overrides); // cache hit
458+
} // the only client disconnects; the worker's idle clock starts
459+
REQUIRE(first_pid > 0);
460+
461+
// The worker exits and unlinks its socket...
462+
REQUIRE(WaitForPathGone(path, 3500ms));
463+
// ...but the cache still hands out that path, so the connect below is
464+
// refused first and only the invalidate-and-relaunch retry can recover.
465+
REQUIRE(duckdb::vgi::ResolveLauncherSocketPath(location, overrides) == path);
466+
467+
auto sock = duckdb::vgi::ResolveAndConnect(location, 10s, overrides);
468+
REQUIRE(sock.IsOpen());
469+
// A new worker process, at the same per-hash socket path.
470+
const pid_t second_pid = PeerPid(sock);
471+
CHECK(second_pid > 0);
472+
CHECK(second_pid != first_pid);
473+
CHECK(duckdb::vgi::ResolveLauncherSocketPath(location, overrides) == path);
425474
}
426475

427476
TEST_CASE("ResolveAndConnect retries once when the cached path goes stale",

0 commit comments

Comments
 (0)