The stop is legible from the node's own log: serve marker, numbered drain, named origin (#568) - #569
Conversation
…top that skipped the door, the drain reports its numbers, and every stop request says who asked (#568) CIRISServer#568 read as "the node re-composes after announce and drops the response". It was the agent's setup-complete hand-off 130 ms later, and the node log could not say so: a serve that is replaced or killed leaves no line, and the stop that IS clean did not say what it drained. - `serve_marker`: `<data_dir>/serving.json` (pid, instance_id, started_at, listen_addr, key_id) written the instant the read API is bound, cleared only after it has drained. The next boot inspects it first: a present marker is WARN "the previous serve did not stop through the shutdown door — replaced, killed or crashed; any response in flight was lost to its caller", ERROR if that pid is still alive (the bind will fail), recorded as a compose_status mark, then cleared. - lens-core read API: an in-flight request counter over the whole router, host routes included; `shutdown()` logs "draining — in_flight=N" and "stopped — drained N, still_in_flight 0, took_ms" so a dropped connection can be placed on the right side of the door from the log alone. - `node_control::request_shutdown_from(origin)`: every stop request logs its origin; `shutdown_node()` names the embedding host. Gates: the marker brackets the listener (inspect < listener_bound < write < drain < clear); every stop request says who asked; unit tests for the marker (clean, own pid, dead pid, damaged) and the counter. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b6714dc37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let _inside = Inside; | ||
| next.run(request).await |
There was a problem hiding this comment.
When a handler returns a response whose body is still being sent, next.run(request).await completes as soon as the Response is constructed, so _inside drops before Hyper polls or writes that body. This always undercounts the merged SSE route in src/federation_surface.rs:634-644, and can report in_flight=0 while an ordinary large response is still on the wire, defeating the shutdown diagnostic this counter provides. Attach the guard to a response-body wrapper and decrement it on end-of-stream or body drop.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 20f5c72: the guard now rides the response body — track_in_flight wraps the response in CountedBody (an http_body::Body wrapper holding the Inside guard), so the count falls only when the body ends or is dropped. The counter test asserts the request is still inside after the handler has returned while the body is unread, and falls back once the body is consumed.
| // means it did not stop through the door — replaced, killed or crashed — | ||
| // and this is where that becomes a line in THIS log rather than a guess | ||
| // from a caller's dropped connection (CIRISServer#568). | ||
| let _previous_serve = crate::serve_marker::inspect_at_boot(&cfg.data_dir); |
There was a problem hiding this comment.
When an unclean marker is found, this inspection calls compose_status::mark("previous_serve_unclean") before the first compose_status::phase(...) at line 119. Even with diagnostics enabled, mark returns without recording anything while State.current is None (src/compose_status.rs:150-153), so the promised in-process status mark is never observable. Start a phase before inspection or store this condition independently of sub-phase marks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 20f5c72: inspect_at_boot now runs immediately after compose_status::phase("halt_gate") opens the first phase, so the previous_serve_unclean mark is recorded; the gate asserts first_phase < inspect.
| if !matches!(prev, Previous::Clean) { | ||
| let _ = std::fs::remove_file(path(data_dir)); |
There was a problem hiding this comment.
When a second process starts against the same home while the first server is still running, this unconditionally deletes the first process's marker even though pid_alive is Some(true). The second process will normally fail its later bind, but if the original server is then killed or crashes, the next boot finds no marker and incorrectly reports a clean previous stop. Do not remove a marker owned by a live process before the new serve has successfully claimed the listener.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 20f5c72: a marker whose owner is still alive is left in place (previous_still_running), and this serve withholds its own marker rather than overwriting it — the bind is about to fail against the survivor, and if the survivor is later killed its marker is still there for the next boot. Unit test a_live_previous_serve_keeps_its_marker (pid 1).
| // SAFETY: `kill(pid, 0)` sends no signal; it only checks existence | ||
| // and permission. EPERM means the process exists but is not ours — | ||
| // still alive for this question. | ||
| let rc = unsafe { libc::kill(pid as libc::pid_t, 0) }; |
There was a problem hiding this comment.
After a container restart using a persistent data directory, the new server commonly has the same namespace-local PID as the killed server (often PID 1). In that case kill(pid, 0) succeeds because it targets the new process itself, so a kill-and-restart is incorrectly logged as the previous serve still running and about to cause AddrInUse, rather than the intended unclean-stop warning. At minimum, treat a marker PID equal to std::process::id() as reused/replaced; robust detection for other reused PIDs requires recording and comparing a process-start identity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 20f5c72: the marker records a process start identity (Linux /proc/<pid>/stat starttime) and previous_is_alive answers false when the marker's pid is our own (the container PID-1 case) or when the recorded start identity differs from the live pid's; both cases are unit-tested (a_marker_with_our_own_pid_is_a_replaced_serve_not_a_survivor, a_reused_pid_with_a_different_start_identity_is_not_alive). Elsewhere than Linux it falls back to the pid-equals-ours rule and otherwise reports None.
| // marker, cleared only by a stop that drains the read API | ||
| // (CIRISServer#568). A write failure is logged, not fatal — the marker | ||
| // is legibility, not a lock. | ||
| if let Err(e) = crate::serve_marker::write(&cfg.data_dir, read.listen_addr(), &cfg.key_id) { |
There was a problem hiding this comment.
read_api_with_extra_at_fidelity binds and spawns the Axum accept task before returning (crates/ciris-lens-core/src/role/node.rs:1112-1132), so on the normal multi-threaded runtime another worker can accept and serve requests before execution reaches this write. If the process is killed in that interval, a client connection or response can be cut but the next boot finds no marker and reports a clean previous stop. Persist the marker after the bind succeeds but before the accept task is spawned or otherwise exposed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 20f5c72: the marker is written just BEFORE read_api_with_extra_at_fidelity (which binds and spawns the accept loop in one call), and cleared again on the bind-failure arm so a failed bind never reads as an unclean stop. The gate now encodes first_phase < inspect < write < listener_bound < drain < clear, taking the post-drain clear rather than the bind-failure one.
…knows a re-used pid, and goes down before the bind (Codex on #569) - The in-flight guard now travels with the response BODY (`CountedBody`, an http_body::Body wrapper): `next.run` returns when the handler has built its response, and a streaming or large body is still on the wire after that. The counter test asserts the request is still inside while the body is unread and falls back when it ends. - The marker is inspected after the first compose phase opens, so its compose_status mark is recorded rather than dropped. - A marker owned by a LIVE previous serve is kept, not cleared; this serve withholds its own marker (the bind is about to fail against the survivor), so a later kill of the survivor still leaves its marker for the next boot. - Process identity: the marker records the Linux /proc starttime; a marker naming our own pid (a container's PID 1 after restart) or a pid whose start identity differs is a REPLACED serve, never a survivor. - The marker is written just BEFORE lens-core binds and exposes the accept loop, and cleared again if the bind fails, so no window exists with a live listener and no marker. The gate encodes the order: first_phase < inspect < write < listener_bound < drain < clear. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
The end-to-end proof found a second, larger fact, now in this PR too. The stop did not end the process. Kept harness stack, What this PR does about it (commit after this comment): every teardown step after the drain goes through Also from the proof: the harness mounts |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20f5c72b2d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if marker_written { | ||
| crate::serve_marker::clear(&cfg.data_dir); |
There was a problem hiding this comment.
When two processes start against the same home concurrently, both can inspect before either marker exists and both can subsequently write one; after one process wins the bind, the loser receives AddrInUse and this cleanup unconditionally removes whichever marker is currently at the path, potentially the winning server's marker. If that server later crashes, the next boot incorrectly reports a clean stop. The new bind-failure arm is fresh evidence beyond the prior live-marker fix: marker_written proves only that this process wrote at some point, not that it still owns the file, so removal needs to compare an ownership token first.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 2b815cb: serve_marker::clear now reads the marker and removes it only when its pid and instance_id are this serve's; another serve's marker is logged and left. Both clear sites (post-drain and bind-failure) go through it. Unit test clear_leaves_another_serves_marker_alone.
| /// HTTP requests currently inside the read API: accepted, response not yet | ||
| /// fully written. Counted by [`track_in_flight`] on every route, including the | ||
| /// routes a host merges in. Read at shutdown so the drain is a number. | ||
| pub static IN_FLIGHT: std::sync::atomic::AtomicUsize = std::sync::atomic::AtomicUsize::new(0); |
There was a problem hiding this comment.
For users of the public LensCore::read_api* constructors that run more than one listener in a process, every listener updates this same static counter while each ReadApiHandle::shutdown reports it as that listener's drain count. A long-lived response on listener B therefore makes shutting down listener A log that A drained B's request and can leave still_in_flight nonzero after A has fully stopped, defeating the per-listener shutdown diagnostic. Store a counter with each router/handle pair instead of sharing one process-wide atomic.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 2b815cb: the counter is InFlight (an Arc<AtomicUsize>) created per read_api_with_extra_at_fidelity call, installed on that listener's router via from_fn_with_state, and carried by its ReadApiHandle, whose shutdown reads only its own. The process-wide static and the free in_flight() are gone; the counter test builds its own InFlight.
…r, and every teardown line names its step in the message (Codex round 2 on #569, plus the probe) - `serve_marker::clear` reads the file first and removes it only when its pid and instance_id are this serve's — two boots racing for one home can both write before one loses the bind, and the loser's clean-up must not take the winner's marker (unit test: another serve's marker survives our clear). - lens-core: `InFlight` is one counter per read-API listener, carried by the `ReadApiHandle` and installed on that listener's router with `from_fn_with_state`; the process-wide static is gone, so a host with two listeners sees each drain its own requests. - `stop_step` puts the step name in the MESSAGE: the log dedup collapses lines whose message normalises the same, and on the probe it hid three of the six "teardown step done" lines behind the first. Probe on the rebuilt binary (local, persistent home): SIGTERM → drained in <1 ms, `edge run loop` named at ERROR after its 10 s budget, `node stopped took_ms=10001`, exit 143, marker cleared. kill -9 then restart → WARN "the previous serve on this home did NOT stop through the shutdown door" with the dead pid, then a fresh marker. Edge's side is CIRISEdge#578. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Proof on the rebuilt binary (local, persistent home — the harness mounts its home on tmpfs, which is why the container proof could not show the WARN): SIGTERM SIGTERM → exit in 10.07 s, exit status 143, marker gone. Before this PR the same binary was still alive eight minutes after SIGTERM. kill -9, then start again on the same home The straggler is edge's |
|
The ubuntu |
…ine; the one-scan test reads its own engine's ordinal; the ladder runs for the config plane (#570) Main 55b8c7a failed on macOS and Windows: "150 keyed reads + one list must cost ONE scan; got 2", and the same test then failed on ubuntu in #569's run. Two causes, both fixed at the root: - the cache was ONE process-wide slot and SCANS a process-wide counter, so a sibling test's engine reading or writing its own config plane evicted this test's snapshot and counted a scan against it. graph_config now keeps a small map (engine identity → snapshot, CACHE_SLOTS = 8, oldest evicted), and the writers — set_config, attest::put — invalidate per engine; invalidate() still clears all for compose's re-serve. - on a cold runner the loop itself outlived the 2 s TTL. The test compares ITS engine's snapshot ordinals before and after, tolerating one rescan per TTL elapsed; the process counter is no longer part of the assertion. Also: src/graph_config.rs, src/attest.rs and src/node_key.rs join the mesh-harness ladder's path filter — the plane every node writes at boot was not running the ladders on a PR. Ladders green on this head. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0132ELwj5JU6t4jEKpwH9bJ7
… reads are index-served (#572) Adopts CIRISServer#571 (a minor on #559). Pins ×4 persist, ×2 edge, the lens-core member, TARGET_* in the substrate gate, README, evidence rows; one copy each. verify v15.0.0 and leviculum unchanged. persist v42.1.0: #817 both dimension axes of list_attestations are index-served (V137 indexes V106's generated `dimension` column; the prefix filter compiles to a range on it) — the substrate fix for our #557, so graph_config's `config:` prefix scan is no longer O(rows this node authored), and its comment says so. #818 a dimension-prefix filter compares bytes on every backend (sqlite's LIKE was case-insensitive). edge v21.1.0: #579 the cohab lane injects verify beside persist; our lane injects no pins. Ships what main carries since 0.5.203: #569 the legible, bounded stop; #570 the per-engine config snapshot cache; #566 the gating Windows installer; #565 the process-global test log capture. Verified: 22 gates, lib 494, test-anchor suites (the anchor block verifies under the new pair), every admission and config suite, lens-core, a sequential sweep of all 119 integration binaries, both harness ladders on the release wheel, and the full OS matrix on the PR. Closes #571. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0132ELwj5JU6t4jEKpwH9bJ7
1 participant
CIRISServer#568 read as "the node re-composes after announce and drops the
response". It was the agent's setup-complete hand-off 130 ms later, and the
node log could not say so: a serve that is replaced or killed leaves no
line, and the stop that IS clean did not say what it drained.
serve_marker:<data_dir>/serving.json(pid, instance_id, started_at,listen_addr, key_id) written the instant the read API is bound, cleared
only after it has drained. The next boot inspects it first: a present
marker is WARN "the previous serve did not stop through the shutdown door
— replaced, killed or crashed; any response in flight was lost to its
caller", ERROR if that pid is still alive (the bind will fail), recorded as
a compose_status mark, then cleared.
host routes included;
shutdown()logs "draining — in_flight=N" and"stopped — drained N, still_in_flight 0, took_ms" so a dropped connection
can be placed on the right side of the door from the log alone.
node_control::request_shutdown_from(origin): every stop request logs itsorigin;
shutdown_node()names the embedding host.Gates: the marker brackets the listener (inspect < listener_bound < write <
drain < clear); every stop request says who asked; unit tests for the
marker (clean, own pid, dead pid, damaged) and the counter.
What a reader now sees, without the host's log
A clean stop (
shutdown_node(), SIGTERM, SIGINT):A stop that skipped the door (exec, kill -9, crash), on the NEXT boot:
and ERROR with
pid_alive=Some(true)when the old serve is still running (the bind is about to fail with AddrInUse).End-to-end proof on a kept harness stack (docker stop → drain lines; kill -9 → restart → the WARN) follows in the PR conversation.
🤖 Generated with Claude Code