mirror of
https://github.com/logos-co/logos-logoscore-cli.git
synced 2026-08-27 19:01:07 +00:00
Dario Gabriel LipicarandClaude Opus 5
162dbc9fff
`logosctl daemon stop` printed {"code":"RPC_FAILED","message":"shutdown RPC
call failed."} and exited 3 for shutdowns that had already succeeded. It cost
the "Stop the daemon" step of doctests/logosctl-daemon.test.yaml one failure
out of nine identical shutdowns in the same macOS CI job; the daemon really
had stopped, and `daemon status` two seconds later said so.
Two independent defects, one on each side of the call.
DAEMON. CoreServiceImpl::shutdown() returned {"status":"ok"} and left the
event loop from a detached std::thread that slept 200ms and called
QCoreApplication::quit(). The reply is not on the wire at that point: the
transport serialises it after the handler returns and hands it to the socket,
which only pushes it out when the event loop services that socket's write
notifier. quit() is not a queued event -- QCoreApplication::exit() interrupts
the dispatcher directly -- so if the main thread was descheduled for longer
than the sleep, the loop came back, exited, and the buffered reply died with
the process. QtRO surfaces no transport error for this; the client just waited
out its 20s deadline and saw nothing.
The quit now runs on the main thread, from a timer, and drains the event loop
before ending it. The 200ms is now a courtesy margin rather than the
correctness mechanism, and $LOGOSCTL_SHUTDOWN_GRACE_MS makes it settable --
including to 0, which the new regression test uses because it is the setting
that used to lose the reply outright.
QtRO offers nothing better: QRemoteObjectHostBase has no per-reply
write-completion signal and no client-disconnect signal, so "quit when the
response has actually been flushed" is not reachable without forking Qt, and
the daemon also serves plain TCP/TLS through a different transport.
CLIENT. RpcClient::shutdown() reported RPC_FAILED whenever the reply was not
an object -- including when there was no reply. But a missing reply is the
expected outcome of asking a process to die, and both docs said so already:
docs/spec.md promised "the client treats the connection loss as a successful
shutdown" and docs/project.md promised exit 0 for it. Neither was implemented.
It now answers the question the reply was standing in for, from evidence: the
pid recorded in daemon/state.json (snapshotted before the call, since a clean
shutdown deletes that file) is watched for up to 15s, or for a remote daemon
the endpoint is re-probed. Gone means success, with `confirmed_by` naming the
evidence; still running means a real error, with a message that says which.
Blindly treating silence as success would have been the more dangerous
mistake -- a wedged daemon is also silent -- so it is not what this does.
That inference is only sound about a pid that was alive to begin with, so
`stop` now refuses a stale session up front the way `daemon status` already
does: a state.json naming this client's instance and a dead pid means there is
no daemon to stop (NO_DAEMON, exit 2). Without it, a session left behind by
last week's daemon would "connect" to nothing, time out, observe that the pid
is gone, and call that a successful shutdown.
TESTS.
* ShutdownReplyTest.StopSucceedsWithNoGracePeriod (integration): 60
start/stop cycles at LOGOSCTL_SHUTDOWN_GRACE_MS=0, asserting the command
succeeds, the daemon is actually gone, and the reply arrived rather than
being reconstructed from the process exiting. Measured through this
fixture on macOS: 6 losses in 100 cycles before the daemon fix, 0 in 120
after.
* CommandTest.Stop_StaleSession_* : the stale-session guard, its live-pid
control, and the remote-client case it must not block. CommandTest now
isolates HOME and the config dir, so the suite no longer reads whichever
~/.logosctl the developer happens to have.
* ProcessUtil.WaitForProcessExit* : the primitive the confirmation rests on.
A/B over the shipped binaries, 30 stop cycles per arm at zero grace, macOS:
pre-fix 8 failures; daemon fix only 0 (no reply lost); client fix only 0
(20 replies lost, every command still correct); both 0. At the default 200ms
grace both arms are clean, which is why this presented as a rare CI flake.
Independent of PR #99: that PR does not touch either function, and the two
diffs do not overlap.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>