lore: Support service start stop and automatic use, and a lightweight runtime when using the service#139
Open
mjansson wants to merge 20 commits into
Open
lore: Support service start stop and automatic use, and a lightweight runtime when using the service#139mjansson wants to merge 20 commits into
mjansson wants to merge 20 commits into
Conversation
`lore service start` and `lore service stop` were stubs returning failure on
every platform. Their original TODOs described a per-repository model: a
sentinel file in `.lore/` marking a repository as service-controlled, and a
registry of serviced repositories inside the service process.
This instead makes using the service a global setting, in the same shape as
`use_shared_store_automatically`. When it is set, the library routes every call
through the service and starts one if none is running, so there is nothing to
register and no per-repository state to track. `start` and `stop` become
utilities to control that process explicitly. No sentinel file is written, and
`repository::SERVICE` stays unused.
There is still no system service integration; the process is an ordinary
detached child, not a systemd unit or a Windows Service.
- `lore-revision/src/global.rs`: new `use_service_automatically` field on
`GlobalConfig`, with an accessor next to the shared-store one.
- `lore/src/call_delegation.rs`: `dispatch_call` consults that setting rather
than only the `LORE_USE_SERVICE` environment variable. Because every public
entry point calls it and reading the setting parses the global TOML config,
the value is cached for the life of the process and invalidated when written.
`LORE_USE_SERVICE` remains as a per-invocation override; it now also
understands `0` and `false` as "off", where previously any non-empty value
meant "on". A call already executing inside the service runs locally, so it
cannot dispatch back into the service it is running in.
- `lore/src/remote/process.rs` (new): liveness check, detached spawn of
`lore service run` (`setsid` on unix, `DETACHED_PROCESS` on Windows), and
waits for the service to start and stop. Automatic start-up refuses to run
anything not named `lore`, so an embedder does not have its own executable
relaunched with `service run` arguments.
It also holds the service's shutdown flag. Publishing the flag is what marks
a process as the service, which is how `stop` distinguishes delivering a
request from receiving one.
- `lore/src/remote/call.rs`: `service_call` starts the service when none is
listening, so a failure to start is reported through the existing event path
rather than surfacing as a bare connection error. Adds
`service_send_no_reply` for requests whose effect is that the service exits,
where waiting for a reply would race the service's own shutdown.
- `lore/src/service.rs`: implements all three handlers. `start` ensures a
service is running; `stop` sends the request and waits for the socket to be
released, or trips the accept loop directly when it is the service receiving
that request; `set_use_automatically` writes the global config. None of the
three route through `dispatch_call`, which would otherwise start the service
in order to ask it to stop, or to be told it should no longer be used.
Starting when a service is already running, and stopping when none is, are
no-ops rather than errors.
- `lore-client/src/cli/commands/service/run.rs`: the service publishes its
shutdown flag at start-up, and now waits for the accept loop however it was
stopped. Waiting on the termination signal alone left the process alive after
a stop delivered over IPC: the accept loop ended and the socket was unlinked,
but nothing woke `service_main`.
- `LoreServiceStopArgs::all` is removed. It meant "stop servicing all
repositories", which has no meaning without per-repository servicing. The
`stop` subcommand takes no arguments, and `set-use-automatically` is added
alongside it.
- FFI: `lore_service_set_use_automatically` and its async form, with the
cbindgen rename and the regenerated header.
- Docs: the global `config.toml` was not documented at all, so
`lore-cli-config.md` gains a section for it covering all three of its fields,
not just the new one. `lore-cli-commands.md` is regenerated; that picks up
pre-existing drift in the global flags as well as the service subcommands.
- `cargo clippy --all-targets -- -D warnings --no-deps`, `cargo +nightly fmt
--all --check`, and `cargo test -p lore --lib` (90 passed): all clean.
- Verified end to end on macOS against an isolated `LORE_GLOBAL_PATH`:
- With the setting unset, a command starts no service.
- With it set, a command starts one and is served by it.
- `start` when a service is running, and `stop` when none is, both exit 0.
- `stop` terminates the process and releases the socket; SIGTERM still does
the same.
- `LORE_USE_SERVICE=0` overrides the setting off.
- Disabling the setting does not start a service.
- Routing was confirmed rather than assumed: with the setting on and automatic
start-up refused (the executable renamed so it is not `lore`), a command
fails at the service boundary, while the same binary with the override off
succeeds locally. Comparing output alone does not distinguish the two paths.
Not verified on Windows: no `x86_64-pc-windows-msvc` target is installed
locally, and the detached-spawn flags there have not been exercised. The Python
tests are likewise unverified, since they need a running server; they also
assume sequential execution, as the service socket is per user rather than per
test.
Signed-off-by: Mattias Jansson <[email protected]>
# Summary A client whose calls all execute in the service process still built the full runtime: on a ten-core machine that is ten tokio worker threads and a nine-thread rayon compute pool, both spawned eagerly, for a process that only writes one message and reads the reply. Measured at 25 threads for a single `lore status`. Such a process now sizes its runtime for relaying instead, which measures at 5 threads for the same command in the same repository, with no rayon threads at all. A client that is not relaying is unaffected and still gets the full runtime. ## Changes - `lore-base/src/runtime.rs`: `TokioSettings` gains `eager_compute_pool`, and `TokioSettings::relay_only()` for a process that performs no work of its own. Turning the compute pool off only stops it being paid for up front; `compute_pool()` still builds it on first use, so a caller that unexpectedly needs it gets it rather than failing. Both pools keep `MIN_THREADS_PER_POOL`, so neither can starve the other. - `lore/src/call_delegation.rs`: `will_use_service` answers the routing question without a runtime, by reading the global config directly rather than through its async loader. It fills the same cache `use_service` uses, so the decision taken at start-up and the one taken per call cannot disagree. - `lore/src/lib.rs`: exposes `will_use_service` and `size_threads_for_relaying`. - `lore-client/src/cli/client_main.rs`: sizes for relaying when the command will be routed to the service. `service run` is excluded: it is the process doing the work, so it keeps the full complement however the calling user has configured routing. The choice has to be made before the first Lore operation, because the runtime is built on first use and later settings are ignored. `log::initialize` and `setup_config` do not touch it, so the existing `set_thread_limit` call site is early enough. # Test Plan Peak thread counts, measured in a real repository so that the command does enough work to be sampled, three runs each and stable across them: | Client path | Threads | rayon (`lore-compute`) | | --- | --- | --- | | Executing locally | 25 | full pool | | Relaying over IPC | 5 | 0 | - That rayon is absent when relaying was confirmed by thread name rather than inferred from the count: `sample` on a client frozen mid-call reports zero `lore-compute` threads. The only lazy callers of `compute_pool` are in `lore-storage`, which a relaying client never reaches, because `dispatch_call` diverts at the API entry before any store is opened. - `service run` is unaffected at 23 threads, with all nine `lore-compute` threads present. - `lore status` output is byte-identical whether relayed or executed locally, and `service stop` works through the relaying runtime. - `cargo clippy --all-targets -- -D warnings --no-deps`, `cargo +nightly fmt --all --check`, `cargo test -p lore --lib` (90), `cargo test -p lore-base --lib` (79): all clean. Note on method: sampling `ps -M` against a bare `lore status` is unreliable, because the process is too short-lived to catch and reports whatever count happens to exist at that instant. The figures above come from freezing the client with SIGSTOP, and from running in a repository where the work lasts long enough to sample. Not verified on Windows. Signed-off-by: Mattias Jansson <[email protected]>
The suite runs under pytest-xdist with `--dist loadgroup`, so tests are spread
across workers unless grouped. The service tests share one per-user socket, and
two of them start or stop the service, so running them on different workers at
once would let one bind while another unlinks. Group them onto a single worker,
matching how the other stateful suites (topology, replicated store, forwarded
requests) already do it.
- `scripts/test/test_service.py`: add `@pytest.mark.xdist_group("lore_service")`
to all four service tests.
- `uv run pytest scripts/test/test_service.py -v`: 4 passed. Alongside pinning
them, this is the first end-to-end run of the start/stop feature against a
real build, covering automatic start-up, explicit start and stop, and the
use-automatically setting.
Signed-off-by: Mattias Jansson <[email protected]>
# Summary
The choice of runtime size — lean when a client only relays to the service,
full otherwise — had no automated coverage. Add unit tests for the sizing, and
refactor the runtime builder so the sizing can be asserted without the shared
runtime singleton.
## Changes
- `lore-base/src/runtime.rs`: extract `resolve_worker_threads` and
`build_tokio_runtime` from `runtime_with_settings`, so a runtime can be built
from a `TokioSettings` in isolation. Behaviour is unchanged. Tests:
- `relay_only_settings_are_minimal`: the relay settings use the per-pool
minimum and do not build the compute pool eagerly, while the default does.
- `relay_runtime_is_smaller_than_full`: a runtime built from `relay_only()`
has the minimum worker count, and the full runtime is never smaller.
- `built_runtime_honors_resolved_worker_count`: the built runtime's worker
count matches what `resolve_worker_threads` resolves, independent of the
`LORE_WORKER_THREADS` override.
- `lore-client/src/cli/client_main.rs`: test `runs_work_in_this_process` — only
`service run` keeps the full runtime; `service start`/`stop`/
`set-use-automatically` and a regular command all relay and are sized lean.
This pins the exclusion that keeps the service process itself full.
# Test Plan
- `cargo test -p lore-base --lib` (82) and `cargo test -p lore-client --lib`
(5): all pass.
- `cargo clippy --all-targets -- -D warnings --no-deps` and `cargo +nightly fmt
--all --check`: clean.
Not covered by these unit tests, and noted for follow-up: the end-to-end wiring
(that `client_main` actually applies `relay_only` when routing to the service)
and the absence of rayon threads in a relaying client were verified by hand
(worker threads 25 -> 5, zero compute threads), not in an automated test, and
no CI currently runs any of this.
Signed-off-by: Mattias Jansson <[email protected]>
# Summary The `background_lore_service` fixture started the service without setting `LORE_GLOBAL_PATH`, so the service ran against the developer's real global config. A test routed through it — creating a shared store while serving a command — wrote the store entry into the real `config.toml`. Isolate the service the same way `lore_service_in_directory` already does, and add a test for the config-driven routing path, which had no coverage. ## Changes - `scripts/test/conftest.py`: `background_lore_service` now starts the service with the test's `LORE_GLOBAL_PATH`, and the readiness probe uses it too, so nothing the service writes lands in the real global config. - `scripts/test/test_service.py`: `test_config_setting_routes_to_service` enables `use_service_automatically` in the isolated config and shows a plain command then routes to the service without the `LORE_USE_SERVICE` override. Routing is proven without a real daemon: a binary not named `lore` refuses to auto-start the service, so a routed command fails with that refusal while the same binary forced local does not. This is the first coverage of the config-driven routing path — the feature's primary production trigger. # Test Plan - `uv run pytest scripts/test/test_service.py`: 6 passed. - Snapshotted the real global config before the run and confirmed it was byte-identical after (same mtime, same entries), where before this change a run added `default_shared_stores` entries to it. - `uv run ruff check`: clean. Signed-off-by: Mattias Jansson <[email protected]>
# Summary `LORE_USE_SERVICE` only treated an empty value and `0`/`false` as off, so a user exporting `off` or `no` to disable service routing silently enabled it, and values were not trimmed. Recognise the common off tokens and ignore surrounding whitespace. ## Changes - `lore/src/call_delegation.rs`: extract `use_service_from_value`, which treats an empty value and `0`, `false`, `f`, `no`, `n`, and `off` (case-insensitive, trimmed) as off and any other value as on, and unit-test it directly without touching the process environment. # Test Plan - `cargo test -p lore --lib call_delegation`: passes, including the new value-parsing test. - `cargo clippy -p lore --all-targets -- -D warnings --no-deps` and `cargo +nightly fmt --all --check`: clean. Signed-off-by: Mattias Jansson <[email protected]>
# Summary `service stop` was check-then-act: `is_running()`, then a send that connects to the socket, with an await between. If the service exited in that window — a concurrent stop or a termination signal — the send's connect failed and `stop` returned failure, even though the service was in fact stopped, which is the outcome asked for. ## Changes - `lore/src/service.rs`: in `stop_local`, a failed send is only an error if the service is still running afterwards; a service that has since stopped returns success. # Test Plan - Two `service stop` invocations racing against one live service, three trials: both return exit 0 every time and the service ends stopped. A normal stop and a stop against an already-stopped service both still return 0. - `cargo clippy -p lore --all-targets -- -D warnings --no-deps` and `cargo +nightly fmt --all --check`: clean. Signed-off-by: Mattias Jansson <[email protected]>
# Summary The doc comment on the cache invalidation claimed a long-lived embedder never keeps the old value, which is only true for writes made by the same process. Document the actual limits: the cache is process-lifetime and does not see a change made by another process until restart, and the save-then-invalidate ordering races a concurrent reader. Both are acceptable for the CLI (one command per process) and only expose a long-lived multi-threaded embedder. ## Changes - `lore/src/call_delegation.rs`: correct the doc comments on the `USE_SERVICE` cache and `invalidate_use_service_cache`. # Test Plan - `cargo build -p lore` and `cargo +nightly fmt --all --check`: clean. Signed-off-by: Mattias Jansson <[email protected]>
# Summary The runtime is sized for relaying once, when it is built, and later settings are ignored. A long-lived process that turns the service off after sizing for relay then runs its work locally on the small runtime — correct but slow. Document this on `size_threads_for_relaying` so the constraint is explicit; the CLI is unaffected since it runs one command per process. ## Changes - `lore/src/lib.rs`: note the one-shot sizing and the flip-mid-process hazard on `size_threads_for_relaying`. # Test Plan - `cargo build -p lore`, `cargo doc -p lore --no-deps` (the new intra-doc link resolves), and `cargo +nightly fmt --all --check`: clean. Signed-off-by: Mattias Jansson <[email protected]>
# Summary On shutdown the service awaits only its accept loop, not the connection handlers it spawned, so in-flight requests are aborted rather than drained: a client mid-operation sees the connection close, and a request that was writing is truncated (recovery left to the store's crash-consistency). Document this on service_main; a bounded drain is left as a future improvement. ## Changes - `lore-client/src/cli/commands/service/run.rs`: note the no-drain behaviour on `service_main`. # Test Plan - `cargo build -p lore-client` and `cargo +nightly fmt --all --check`: clean. Signed-off-by: Mattias Jansson <[email protected]>
# Summary When the service closes a connection mid-request — which happens when it is stopped or terminated while a request is in flight — the client reported only that the connection closed without a result. Say that the service may have been stopped or terminated, so the cause is clear. ## Changes - `lore/src/remote/call.rs`: extend the closed-without-result error message. # Test Plan - `cargo build -p lore` and `cargo +nightly fmt --all --check`: clean. Signed-off-by: Mattias Jansson <[email protected]>
# Summary A user cannot query whether the service is running or whether automatic routing is enabled. Record the missing `status` command as a follow-up next to the service subcommands. ## Changes - `lore-client/src/cli/commands/service.rs`: TODO for a `status` command mirroring `shared-store info`. # Test Plan - `cargo build -p lore-client` and `cargo +nightly fmt --all --check`: clean. Signed-off-by: Mattias Jansson <[email protected]>
# Summary `start_local` guarded on `running_as_service()`, but that branch was unreachable: `start` runs locally and `ServiceStart` is never sent over IPC, so `start_local` never executes inside the service. It read as a live path and implied a false symmetry with `stop_local`, whose guard is genuinely reached because `ServiceStop` is delivered over IPC. `ensure_running` already no-ops when a service is already running, so removing the guard changes nothing. ## Changes - `lore/src/service.rs`: drop the dead `running_as_service()` branch in `start_local`. # Test Plan - `cargo build -p lore`, `cargo clippy -p lore --all-targets -- -D warnings --no-deps`, `cargo +nightly fmt --all --check`: clean. Signed-off-by: Mattias Jansson <[email protected]>
# Summary Stopping the service depends on the wake-up connection in `request_shutdown` succeeding. Spell out the consequence of a failure: the polling client path self-corrects, but the termination-signal path discards the error to a detached process's null stderr, so exit is delayed until another connection arrives. The failure is rare — connecting to one's own listening socket essentially only fails under backlog exhaustion or a removed socket file. ## Changes - `lore/src/remote/process.rs`: expand the `request_shutdown` doc comment. # Test Plan - `cargo build -p lore` and `cargo +nightly fmt --all --check`: clean. Signed-off-by: Mattias Jansson <[email protected]>
# Summary The shutdown flag is stored in a OnceLock, so only the first service_main in a process registers a flag that request_shutdown can trip. A second in-process service would run an accept loop watching a flag that is never tripped and could not be stopped. Document this on register_shutdown_flag; the CLI runs the service once per process, so only a future embedder or in-process test is constrained. ## Changes - `lore/src/remote/process.rs`: note the single-service-per-process constraint. # Test Plan - `cargo build -p lore` and `cargo +nightly fmt --all --check`: clean. Signed-off-by: Mattias Jansson <[email protected]>
# Summary Every service-routed call first called `ensure_running`, whose liveness check opens and drops a connection, then opened a second connection for the real message — two connects, accepts, and handler-task spawns per call, the first a phantom the service reads as an immediate EOF. Send to an existing service first and only start one if that fails, so the common path opens a single connection. ## Changes - `lore/src/remote/call.rs`: serialize the message once, then `connect_and_send` it; on failure, `ensure_running` and retry. Auto-start still happens when no service is listening, and a start refusal still surfaces through the retry. # Test Plan - `uv run pytest scripts/test/test_service.py`: 6 passed, covering the three paths — service already up (single connection), service down (start then retry), and a non-`lore` binary refusing to auto-start. - `cargo clippy -p lore --all-targets -- -D warnings --no-deps` and `cargo +nightly fmt --all --check`: clean. Signed-off-by: Mattias Jansson <[email protected]>
# Summary
Add the two tractable coverage gaps from the review: that a termination signal
stops the service cleanly, and that two stops racing one live service both
succeed (the loser's send finds the service gone but must still report success).
## Changes
- `scripts/test/test_service.py`:
- `test_service_shuts_down_gracefully_on_signal` (SIGTERM and SIGINT,
parametrized, POSIX-only): the service exits with code 0.
- `test_concurrent_service_stop_all_succeed`: two `service stop` processes
race a started service, three rounds, and both exit 0 each round.
# Test Plan
- `uv run pytest scripts/test/test_service.py`: 9 passed. The two new tests were
run three further times and were stable.
- `uv run ruff check`: clean.
Signed-off-by: Mattias Jansson <[email protected]>
# Summary The CLI sizes its runtime for relaying before running a command, but an embedder calling the FFI has no such step: the first call built the full runtime even when the service is enabled and every call just relays over IPC, so the process kept a full runtime it never uses for its whole lifetime. ## Changes - `lore/src/call_delegation.rs`: the synchronous and asynchronous FFI runners size the runtime for relaying, when the service is enabled, before building it. A no-op once the runtime exists and when the service is off. This covers every FFI entry point uniformly — routed operations relay, and the local ones (`service start`/`stop`/`set-use-automatically`) are lightweight, so the lean runtime suits both. The one case it does not cover: an embedder whose very first call is `set-use-automatically(true)` in the same process, since the setting is not yet on when that call is sized. # Test Plan - `lore/tests/dispatch_runtime.rs` (own test binary, fresh runtime): with the service enabled, the first FFI call builds a two-worker runtime. - `cargo test -p lore --lib` (93), clippy (`-D warnings`), nightly fmt: clean. Signed-off-by: Mattias Jansson <[email protected]>
# Summary The pedantic lints enabled in .cargo/config.toml flagged two constructs, and the regenerated CLI command reference had a double trailing newline the end-of-file-fixer objects to. ## Changes - `lore/src/call_delegation.rs`: `Result::map(..).unwrap_or(false)` becomes `is_ok_and(..)` (`clippy::map_unwrap_or`). - `lore/src/remote/call.rs`: the single-arm `match` becomes `if let ... else` (`clippy::single_match_else`). - `docs/reference/lore-cli-commands.md`: collapse the trailing newline to one. # Test Plan - `cargo clippy --all-targets -- -D warnings --no-deps`: clean across the workspace. - `cargo +nightly fmt --all --check` and `cargo test -p lore --lib` (93): clean. Signed-off-by: Mattias Jansson <[email protected]>
# Summary The service handlers passed a string literal as the `caller` argument to `no_repository_call`, and the command logging derives the command name from `type_name` of that argument — so the debug log read `Executing command: &str` instead of a command path. Pass the handler function, as the other handlers do, so the name resolves properly. ## Changes - `lore/src/service.rs`: pass `start` / `stop` / `set_use_automatically` rather than string literals to `no_repository_call`. # Test Plan - `lore --debug service set-use-automatically true` now logs `Executing command: lore::service::set_use_automatically`, matching the form other commands use (e.g. `lore::shared_store::info`). - `cargo clippy --all-targets -- -D warnings --no-deps`, `cargo +nightly fmt --all --check`, `cargo test -p lore --lib` (93): clean. Signed-off-by: Mattias Jansson <[email protected]>
ragnarula
reviewed
Jul 23, 2026
| * `--search-limit <SEARCH_LIMIT>` — Set maximum number of revisions to search when matching or finding revisions | ||
| * `--search-nearest` — Set to search for nearest match when matching revisions | ||
| * `--gc` — Set to run automatic garbage collection on local store in background | ||
| * `--no-gc` — Prevent automatic incremental garbage collection for this command; it otherwise runs in the background on writes. `lore repository gc` always runs a full pass regardless |
Collaborator
There was a problem hiding this comment.
Are we making gc on by default for all clients?
Collaborator
Author
There was a problem hiding this comment.
That is already the case. This was just auxiliary cleanup of stale docs while adding the docs on service process. I can move this line to a separate PR if need be.
ragnarula
reviewed
Jul 23, 2026
| With `use_service_automatically` enabled, Lore sends each command to a background service process over a local socket instead of executing it in the CLI process, and starts that service automatically if it is not already running. Set it with: | ||
|
|
||
| ```bash | ||
| lore service set-use-automatically true |
Collaborator
There was a problem hiding this comment.
Should we have configs interspersed in each subcommand or would be be better to have something like lore config set/get?
Collaborator
Author
There was a problem hiding this comment.
Yes, but we don't have that yet and this matches the shared store way of configuring.
ragnarula
reviewed
Jul 23, 2026
| /// connection close and reports it, and a request that was writing is truncated, | ||
| /// with recovery left to the store's crash-consistency (the same guarantee a | ||
| /// locally interrupted command relies on). A bounded drain of outstanding | ||
| /// handlers before exit is a possible future improvement. |
Collaborator
There was a problem hiding this comment.
This comment looks like a big context leak. Review against the commenting guide in developer docs.
Collaborator
Author
There was a problem hiding this comment.
It is not - it's for anything using this public entry point to know the limitations, which is that commands are not drained but aborted when the service loop ends on the documented termination conditions.
Collaborator
Author
There was a problem hiding this comment.
But I can clean it up a bit to make it more clear
ragnarula
reviewed
Jul 23, 2026
ragnarula
left a comment
ragnarula
left a comment
Collaborator
There was a problem hiding this comment.
Nice work but it's primitive as is. Maybe thats fine for a start. Some thought though.
-
We have a number of ENV vars which are read at the point of use, so from the environment of the daemon now rather than the caller. Any that would cause an issue? I saw you added working directory to the caller context, I do wonder what more might be missing? Anything you're aware of?
- What happens if I ctrl+c the calling process in the middle of a large operation? Does the daemon keep running it? So I cancel a big clone or sync but it doesn't really stop? Is my only option to kill the daemon? What happens if I cancel a big operation and then try to run a smaller one the same repo straight after? Will the caller be stalled until the big one finishes, but without any visibility of it?
-
What happens if I upgrade my lore version without killing the daemon? Do I need to remember to do that first? There's no versioning in the IPC protocol right?
-
When the service does shutdown what happens to inflight work?
| /// Whether this command does its own work rather than relaying it to the Lore | ||
| /// service. Only `service run` does: it *is* the service, so it needs the full | ||
| /// complement of threads however the calling user has configured routing. | ||
| fn runs_work_in_this_process(command: &LoreCommands) -> bool { |
Collaborator
There was a problem hiding this comment.
Does this small private function really warrant this much comment? It's quite confusingly written.
| fn runs_work_in_this_process(command: &LoreCommands) -> bool { | ||
| matches!( | ||
| command, | ||
| LoreCommands::Service(args) if matches!(args.command, ServiceCommands::Run(_)) |
Collaborator
There was a problem hiding this comment.
Are you sure these will be the only commands that run in this process? What if we add more elsewhere, this seems like an easy thing to go stale.
|
|
||
| /// Sends one command to a service that is already running and returns as soon | ||
| /// as it is written, without waiting for a result. Used for requests whose | ||
| /// effect is that the service exits, where waiting for a reply would race the |
Collaborator
There was a problem hiding this comment.
I don't think it matters that its used for exit, why couldn't it be used for other future one shot commands?
Collaborator
There was a problem hiding this comment.
Or should the service shutting down not race and deterministically return that it finished?
| if let Err(error) = UdsStream::connect() { | ||
| eprintln!("Failed to wake the accept loop: {error}"); | ||
| } | ||
| if tokio::time::timeout(SHUTDOWN_TIMEOUT, accept_task) |
Collaborator
There was a problem hiding this comment.
Why remove the timeout?
| cwd=str(tmp_path), | ||
| env=env, | ||
| ) | ||
| assert refusal in (routed.stdout + routed.stderr), ( |
Collaborator
There was a problem hiding this comment.
Do we have to match a string? Can't there be a return code?
|
|
||
| /// Sends one command to a service that is already running and returns as soon | ||
| /// as it is written, without waiting for a result. Used for requests whose | ||
| /// effect is that the service exits, where waiting for a reply would race the |
Collaborator
There was a problem hiding this comment.
Or should the service shutting down not race and deterministically return that it finished?
|
|
||
| /// Whether calls should be routed through the service process. | ||
| /// | ||
| /// Test builds never read the stored setting, so that the suite runs against a |
Collaborator
There was a problem hiding this comment.
Tests (at least through pytest, should probably be set up for rust tests somehow) are supposed to give each test its own global config. I think that setup needs to be hardened, but IMO it's better than each setting derived from the global config needing a set of overrides like this.
Collaborator
Author
There was a problem hiding this comment.
This is to prevent the Rust unit tests from accidentally trying to use the service process if the user happens to have env vars or other things setup when running cargo test
Collaborator
Author
Valid comments, but unrelated to this change. This ONLY automates the start of the background process, rather than running it explicitly with |
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
3 participants
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.
Motivation
Lore's service process start/stop were stubs that returned failure, and the only way to route through the service was an undocumented env var, with no persistent opt-in.
A client relaying all its work to the service also still built the full runtime it never uses, since all processing is routed over IPC and executed in the service process.
Summary
Implement lore service start / stop (both idempotent).
Add a
use_service_automaticallyglobal setting (lore service set-use-automatically <bool>): when on, every command routes through the service, auto-starting it on demand. Off by default;LORE_USE_SERVICEstill overrides per-invocation.Manage the daemon lifecycle: detached start, working directory parked away from the caller's, clean shutdown on signal or on a stop over the socket.
Size a relaying client's runtime lean (no compute pool, fewer threads); local work and the service process itself keep the full runtime.
Document the setting and commands.
Testing
Rust unit tests added and verified:
LORE_USE_SERVICEparsing; runtime sizing (relay vs full); the service run-only full-runtime classification.Python smoke tests added: start/stop idempotency, config-driven routing, auto-start on demand, working-directory resolution, SIGTERM/SIGINT shutdown, concurrent stop — all against an isolated global config (verified a run leaves the real system config byte-identical).