fix(daemon): close socket-group and pointer-rename races - #1819
Merged
Merged
Conversation
`map_execution_error` mapped `SessionTemporalExecutionError::Empty` to
`CompleteZero` whatever freshness rode with it. After a hook ingest
commits rows, the project store publishes the temporal generation but
its relation receipt is applied later by the LCM summary convergence
page, so `root_readiness` answers `Partial { generation_lag: 1 }` and
the candidate cohort is empty. The first `tracedecay_message_search`
then reported `outcome: complete_zero` — "nothing exists, and that is
final" — for a transcript the store had already committed.
`map_report` already refuses that for an empty ranked page: a `Partial`
generation returns the typed partial outcome instead. The
execution-error path owed the same refusal. It now answers
`Partial { items: [], omitted: generation_lag }`, which the retained
message-search surface already renders as `outcome: "partial"`, so a
caller re-reads rather than believing the zero.
`production_codex_hook_ingest_survives_message_search_reopen` loops on
that typed partial the way the refresh assertions loop on `running`,
and still fails on an empty `complete_zero` or any other outcome.
Measured on `taskset -c 0,1`, 6 concurrent copies, 42 runs:
25 failures before, 0 after.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`tracedecay update` installed v0.1.0-beta.47, restarted the daemon, then refused the binary it had just installed: protocol identity mismatch (name=tracedecay, version=0.1.0-beta.47+84598a0b..., expected version=0.1.0-beta.47) and the daemon logged `daemon_version_skew` on every readiness poll, with the same two strings. Both sides were one binary. One value drove both symptoms. The GitHub-release upgrade path reports the release it installed as the bare tag (`upgrade.rs` `run_versioned_upgrade`), while the package-manager path reports the installed binary's own `--version`. That value becomes the maintenance window's `expected_version` (`service.rs` `adopt_maintenance_outcome`), which readiness compared to the daemon's advertised `build_version()` with a raw string `==`, and which `query_daemon_identity_stream` also sends as the probe's own `client_version`, where `client_version_skew` compared it with a second raw `==`. Build metadata never changes SemVer precedence, so a side reporting only the release is less specific, not different. `versions_name_same_build` is now the one comparison every identity check runs: equal release precedence, and the same commit whenever both sides name one. That keeps the skew 367a44a added this comparison to catch, two checkout builds of one release differing only by commit, while the release tag and the binary it ships resolve to one identity. Tests cover both halves and fail without the fix: readiness classifies 0.1.0-beta.47+<sha> as Ready against a bare 0.1.0-beta.47, a stale release and a second commit of the same release stay mismatches, and unparseable versions still compare literally. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A daemon spent eleven hours emitting 70,985 identical warnings:
session temporal refresh pass will retry class=Storage
retry_attempt=25107 error="Storage { operation: \"persist session;
refresh progress\", source: Sqlite { code: Some;
( 19 );
, extended_code:;
Some;
( 1811 );
, message: \"invalid session refresh progress\" } }"
Three workers each resubmitted the same progress row about once a
second for the whole process lifetime, pinning two tokio workers.
Two independent defects combined:
1. The worker used `SessionStoreError::is_storage` as its retryability
predicate. That only says the failure came from the storage adapter.
A `SQLITE_CONSTRAINT` abort is a schema-contract trigger refusing
this exact row, and an exact-SQL materialization ceiling (6,627 more
warnings on the same loop, reported as an untyped `Runtime` message)
refuses this exact statement. Replaying either unchanged can only
spin at the backoff cap, which for `Storage` is 800ms forever.
2. Even classified terminal, the projection arm only counted the error
and left the operation `running`, so the next pass rediscovered it
and rebuilt the same row. Only the projector's terminal errors
durably failed a refresh.
`Error::is_deterministic_refusal` now names the two engine failures
that cannot be replayed, `QueryLimitExceeded` maps to the typed
`InvalidOperation` its sibling limit refusal already used, and the
worker retires a refused refresh through the existing terminal-attempt
and discovery-suppression machinery.
`validate_successor` also admitted an equal committed frontier where
the durable guard requires a strict advance, so a stalled successor
reached the trigger as a constraint abort instead of typed state. It
now matches the guard.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A daemon mounted /fast/projects/tracedecay at 15:25:35, decided the
sealed generation predated the build and had to be rebuilt, and then
recorded no further scheduler progress for eleven hours. A full thread
dump taken while it was stuck shows two blocking-pool threads in
registry::mount_worktree_inner
-> LatestCodeTextGenerationV1::advance_text_serving_inner
-> CodeLexicalCloneSuccessorV1::verify_resumed_page
-> verify_clone_fingerprint_page_rows
-> sqlite3_step / sqlite3BtreeNext / readDbPage
so the mount was not blocked on a seat, a permit or a store lock: it was
running SQL, which is why the process also sat at 100% on two workers.
`clone_exact_postings` and `clone_fingerprint_postings` are WITHOUT
ROWID tables whose primary keys start at `class` and `language`, so
`WHERE symbol_occurrence_id = ?` has no index and scans the whole table.
Resume verification issued two of those per clone body, making the pass
quadratic in a repository's postings.
Both tables are now read once per page and bucketed by occurrence. Each
primary key is unique within one occurrence, so a sorted bucket
reproduces the `ORDER BY` the per-body queries used and the comparisons
stay byte-identical; `clone_body_payloads` and `clone_occurrences` are
already keyed by their lookup column and are untouched. Resume
verification is covered by
`v16_clone_payloads_are_content_addressed_and_postings_page`, which
drops and reopens a successor mid-corpus and then compares finished
section digests.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
cc10394 made every non-retryable failure of the projection batch persist retire the running refresh. A cancelled worker control also fails that persist, and it is not a refusal: the row was never submitted, and the next pass may hold a live control. Retiring it through `claim_terminal_attempt` under a cancelled state claimed nothing and counted nothing, so `cancelled_worker_control_prevents_projection_batch_persistence` observed zero terminal errors on CI. Only a deterministic engine refusal (constraint abort, exact-SQL ceiling) retires the refresh; every other terminal error counts as before and leaves the operation running. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
8b75523 cut resume verification from one postings scan per body to one per page, but a restart still replays every committed page, so a daemon resumed against a large repository sat inside `verify_clone_page_rows` for over thirty minutes with no scheduler progress: N pages times a full scan of both postings tables. The clone successor now installs `symbol_occurrence_id` indexes on both postings tables when it opens (idempotent, so a copied prior built without them gains them), and page verification reads postings per occurrence through them. Nothing digests or enumerates the index schema; the receipt records the file size after the indexes exist. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`single_analytics_append_commits_in_one_writer_dispatch` asserted the append future was still `Poll::Pending` after its first poll. That poll enqueues the INSERT on the writer OS thread via a synchronous `try_send` and then awaits a `bounded(1)` reply channel, so the writer can execute, autocommit and answer before the caller first polls the receiver. The assertion was racing that thread, not checking a contract: 9/60 failures under `taskset -c 0,1` with 6 concurrent copies, matching the CI flake in run 35422336760 (TRY 1 fail, TRY 2 pass). The contract commit 4fc1439 added is that a *detached* append still commits: the writer runs the request and only then replies, ignoring a dropped receiver. Make the test enforce that with a gate it owns -- scope the future so it is dropped right after the single poll, before anyone reads the reply -- and keep the existing commit and no-retained-transaction checks. Restoring the transaction-framed append fails the restructured test 30/30 with "single append did not autocommit", so the regression guard is intact and now deterministic. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit ea8113159372c2f4ab1f7c1e8138ce4c5d846494) (cherry picked from commit 80341240b4e94c5fbe075d024337c87b256cc3f3)
`mounted_code_generation_retention_continues_capped_segment_reclamation`
failed both nextest tries at ~3.9 s on master run 35422072661 with `code
generation retention plan: Storage("No such file or directory (os error
2)")`; the identical tree passed the same partition minutes earlier on run
35420910183, so it is a race. Under `taskset -c 0,1` with 6 concurrent
copies it reproduced 4/78.
Instrumenting every enumerate-then-open loop in the planner with the path
and a re-stat named the vanishing file on both ENOENT reproductions: the
text-artifact inventory's `read_dir` + `symlink_metadata` loop, on
`.text-artifact-<digest>.staging` and on a `.staging-journal` sidecar,
both `present_now=false` while their siblings were still listed.
`prepare_next_code_generation_retention_cancellable` scans without the
generation-store lock on purpose - the full-digest read routinely covers
several GiB and must not pin the daemon writer gate - and the text-artifact
builder retires a whole `.staging` family (`discard_incompatible_staging`
-> `retire_text_artifact_staging_family`) under that lock in the background
pass tail, the same tail fenced in 656b532 and 760e221dda. So an entry
the listing just named can be gone before the scan stats it, and the whole
plan failed. Production reads that as `retention_plan_failed` and fails the
pass with a loud degraded log, so every publish that raced a maintenance
tick lost the tick.
An entry that vanished mid-scan is already reclaimed, which is what the
inventory would have planned for it anyway: skip it, the way this loop
already skips an absent active-staging path fifteen lines above. Completed
artifacts the durable index references are verified before this scan and
stay fail-closed. The unit test retires the staging family from inside the
scan's own cancellation probe and asserts the plan survives without naming
the vanished files; it fails with the ENOENT before this change.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
(cherry picked from commit 012965c52514790e56d5531e51b48acb808cd1c5)
The same 6-copy `taskset -c 0,1` reproduction of `mounted_code_generation_retention_continues_capped_segment_reclamation` also failed with `code generation retention plan: GenerationStoreBusy` (1 of 4 failures in 78 runs). That is not a failure. The planner's recovery probes the generation-store lock with `try_acquire_code_generation_store_lock` and answers `GenerationStoreBusy` when a writer owns the store; production maintenance consumes it with `defer_generation_store_busy` and comes back on the next tick. The route under test stays mounted, and `publish_code_edit` returns as soon as the serving generation id changes, so the pass tail that sealed that generation can still own the store when the test plans - the same pass-tail exposure as 760e221dda. Consume the typed busy answer under a bounded wait instead of reading it as fatal. Every assertion on the resulting plan is unchanged, and any other error still panics. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit 833a3f30a2c77c426864123c0065bc4d8b664586)
`tracedecay_unsafe_patterns` picked `enclosing` as the smallest-line-span
symbol covering the match's line. When several declarations share one line
every candidate has span 1, so the winner was whichever symbol the page
happened to yield first, and pages arrive in occurrence order over
per-project digests. `#[test] fn a() { x.unwrap(); } pub fn b() { panic!(); }`
therefore reported `@test` (the annotation-usage node the Rust extractor
names `::@name`), `a`, or `b` at random: 7 of 12 identical local runs
disagreed, which is what broke the CI job deterministically on its runner.
Resolve the enclosing declaration by the byte range the graph already
publishes in `CodeGraphSymbolBindingV1::source_span`, containing the offset
of the matched construct. That is stable, names the innermost declaration
that really contains the site, and drops attributes for free: `#[test]`
spans only its own bytes, so it can never contain a call.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
(cherry picked from commit 178afe414b370165fd8910620085e922d7e9fe27)
`adapter_leaves_repository_byte_identical` snapshots `.git` before and after the read-only adapter calls. The fixture's `git commit` spawns a detached `git maintenance run --auto`, whose `.git/objects/maintenance.lock` was still present for the first walk and gone for the second, so the byte-identical assertion failed on both nextest tries with the lock as the only difference. Disable auto maintenance and gc for every fixture git call, as the scheduler and daemon fixtures already do. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
An explicitly published branch worktree is never a project-open route, so nothing mounted a core query authority on it. An exact branch read could only borrow one already mounted on a peer checkout of the same repository (`mount_query_authority_from_project_peer`), and that peer's own mount is deferred until it seats a text generation. A read taken right after the publication sealed its provenance therefore failed closed with a non-retryable `authority_unavailable`, even though the branch generation was committed and servable. That is the CI flake in the tracedecay-cli `core_cli_suite` linked-worktree branch-add journey, where a freshly added peer worktree searches the just-published branch before it has seated anything of its own. Mount the published branch's own authority from the project's durable cursor-key authority, reusing the existing project-open mount rather than adding a second mount path. It runs inside the daemon-owned publication task after the generation is committed, so the admitting `branch add` caller — which returns at admission, before activation even starts — never waits for it and never pays for the profile session-registry lock. An earlier attempt (51402cd) mounted on the admitting caller's path and correlated with mount and publish failures elsewhere; this one stays off that critical section. Best effort by design: the generation is already committed, so a missing session mount or cursor key must not retract it, and the exact read still falls back to borrowing a peer authority when the mount could not run. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit 6d887a0c3ce25c2838f2943200e133a610cdf2e8)
Harness stop signaled only the detached group leader, so descendants kept the listen socket after wait. Publication renamed the active pointer without checking the bytes it had observed. Signal the whole group before reap, unlink the published socket, and refuse a pointer rename whose file is no longer that observation. Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
|
`tracedecay_field_sites` picked each site's `enclosing` declaration by filtering the file's symbols to those whose line range covers the site's line and taking the smallest `line_span`. When several declarations share one line every candidate has span 1, so `min_by_key` broke the tie on the order the graph page yielded symbols, and pages arrive in occurrence order over per-project digests: a coin flip per run, the same defect 63c0784 fixed in `tracedecay_unsafe_patterns`. Resolve `enclosing` by the byte range the graph already publishes in `CodeGraphSymbolBindingV1::source_span` containing the site's own byte offset, innermost containing span winning. `FieldSite` already carried that offset (`byte`, the end of the field name, keyed the same way as the receiver-type map), and masking preserves byte layout, so no matcher change was needed. `enclosing_declaration` moves from `unsafe_patterns` up to the shared `analysis` module and now returns the symbol rather than its name, since the field-site qualifier check needs the occurrence id. Both handlers then apply one attribution rule instead of two copies that can drift. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> (cherry picked from commit 71522df546dae3b7cff729d60988b2f7bb13b750)
`elapsed_freshness_window_alone_does_not_make_dashboard_state_stale` read the dashboard right after `wait_for_dashboard_ready`, which joins only the running pass. The mount leaves clone backfill behind, and the wakes that drain it leave a banked permit whose no-op pass projects `Verifying` instead of `Fresh` at the sample (CI run 35425541839, both tries). Use the single-permit registry, settle the mount-era chain, hold the admission so no pass can start, and prove the pending-wake slot stays empty, as the text-progress test already does. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Under `--all-features` the binary binds the hotpath metrics port on start. When a sibling test's daemon already holds it, the bind error lands on stderr and `shipped_binary_stops_quietly_when_a_pipeline_reader_exits` fails on its empty-stderr assertion. Turn the metrics server off for this command, as `hotpath_command` already does. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
ScriptedAlchemy
marked this pull request as ready for review
September 19, 2026 06:45
Owner
Author
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
5 tasks
…or/daemon-socket-pointer-lock-8c78
![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: 6099dda2c4
ℹ️ 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".
Comment on lines
+2237
to
+2239
| let prior_bytes = if prior_pointer.is_some() { | ||
| Some(std::fs::read(&self.active_path).map_err(Self::unavailable)?) | ||
| } else { |
There was a problem hiding this comment.
When a fault injector or non-cooperating writer changes the active file between read_publication_pointer() and this separate read, prior_pointer describes the old file while prior_bytes contains the new one. The later commit therefore accepts the new bytes as its observation but constructs its replacement from stale state, erasing the intervening update or corruption; return the parsed pointer and its exact source bytes from one read, or verify that these bytes decode to the same pointer before using them as the token.
AGENTS.md reference: AGENTS.md:L177-L178
Useful? React with 👍 / 👎.
| std::fs::remove_file(&temporary).map_err(Self::unavailable)?; | ||
| } | ||
| Self::write_durable(&temporary, bytes)?; | ||
| if let Err(error) = std::fs::rename(&temporary, &self.active_path) { |
There was a problem hiding this comment.
When a fault injector or writer bypassing the store lock changes the pointer after the read at line 1288 but before this rename, the equality decision is already stale and this rename still overwrites the changed file. Consequently commit_observed_pointer is not a compare-and-swap and can lose exactly the corruption or replacement it promises to preserve; use the existing conditional-publication primitive that atomically retains and verifies the displaced object rather than a check-then-rename sequence.
AGENTS.md reference: AGENTS.md:L124-L131
Useful? React with 👍 / 👎.
Comment on lines
210
to
212
| atomic_write( | ||
| &store_root.join(ACTIVE_POINTER_FILE), | ||
| "code-generation-text-artifact-mutation", |
There was a problem hiding this comment.
When the active pointer is replaced or truncated after the re-read at line 204 but before atomic_write performs its rename, this write still replaces that intervening state. Re-reading immediately before publication only narrows the race and does not provide the claimed refusal semantics; route this through the repository's conditional atomic-write authority and verify the displaced bytes against the original observation.
AGENTS.md reference: AGENTS.md:L124-L131
Useful? React with 👍 / 👎.
`group_stop_releases_an_inherited_listen_socket` failed 2 of 40 local runs of the freshly built `daemon_suite` binary with `connect returned Ok(UnixStream ...)`. The proof spawns a holder that binds, listens and forks, then asserts the path is refused the instant `kill_and_wait` returns. That assertion is stronger than the kernel guarantees. The descendant holding the inherited listen descriptor is reparented, not a child, so the harness cannot `wait` on it: the group `SIGKILL` is delivered asynchronously and its descriptors close when the kernel finishes the teardown, not when the leader's `wait` returns. The harness does not rely on that instant either - `release_socket_on_stop` unlinks the published path, which is what makes `spawn_tracedecay_daemon_process` deterministic. Poll for the refusal on a bounded deadline. A descendant that was never signaled keeps accepting past it, so the regression this proof exists to catch still fails the test. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`cargo clippy --workspace --all-targets --locked -- -D warnings` failed on `map_unwrap_or` in the pointer memo installed by `remember_publication_pointer`, which is CI's exact lens. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`terminate_and_reap` signals `kill(-pid, SIGKILL)` before its own `wait`, and both `kill_and_wait` and `Drop` call it. Every child that a test already reaped - `wait_with_output`, `wait_for_exit`, `is_running` all reap through `try_wait` - therefore got a second group signal from `Drop` on a pid the kernel had already freed, which addresses whatever process group later claims it. Record the reap on `TestChildProcess` and signal the group only while the pid is still ours. A leader that exited but has not been waited on is still an unreaped zombie owning its pid, so the descendant kill the socket proof relies on is unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The restart journeys reassign their handle - `daemon = spawn_tracedecay_ daemon_with(..)` - so the predecessor is dropped *after* the successor bound a new socket at the same path. Releasing by path alone unlinked the live daemon's endpoint and the next request failed with `daemon_connect_ down ... daemon.sock: No such file or directory`; twelve daemon_suite journeys failed that way, `ignored_dependency_admission_survives_physical_ daemon_restart_without_widening` on every run. Comparing the file's `(dev, ino)` does not separate them: the successor's socket lands on the inode the predecessor's shutdown just freed, which reproduced 5 of 5. Record the publisher instead - claiming a path evicts the previous claim - and unlink only while this child still holds it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Owner
Author
|
Review lane verdict: MERGE (after fixes pushed here: bf444b5, ed6d8f9, d59b7eb, 8889736 publisher registry for socket unlink). Overlaps #1833 (already in batch #1848) in tests/common/mod.rs; the integration will keep one harness (yours, which subsumes #1833's and #1794's group-kill hunks) plus #1833's regression test. Merged via the next batch run once green. |
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
2 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.
Summary
Motivation
#1792-class waits assumed the races were timing. They are not. The harness calls
process_group(0)and thenSIGKILLs only the leader, so any descendant that inherited the listen socket stays connectable afterwait. Publicationrenames over whatever occupies the pointer path, including a fault written after the writer read a valid pointer. #1794 and #1797 do not close that assignment (#1794 adds more waits and conflicts with master; #1797 is the retry, clone-scan, and version-identity lane), so this is a separate PR..github/workflows/ci.ymlis unchanged.pull_requestwas not added.Changes
crates/tracedecay/tests/common/mod.rs: group signal before reap, unlink the recorded socket, refuse a still-accepting path instead of polling it out.crates/tracedecay-code-index-runtime/.../publication_store.rs:commit_observed_pointeris the rename, and the memo is installed only when the file still matches those bytes.crates/tracedecay-code-index-retention/.../text_artifacts.rs: the artifact mutation compares the durable pointer again immediately beforeatomic_write.Test plan
cargo test -p tracedecay-code-index-runtime --lib code_index_scheduler::tests::publication_store::stale_pointer_commit_does_not_replace_a_changed_active_pointer -- --exact— 1 passedcargo test -p tracedecay --features test-helpers --test daemon_suite socket_lifecycle_test— 2 passed (group_stop_releases_an_inherited_listen_socket,stop_unlinks_the_socket_path_the_child_published)code_index_ignored_dependencies_test::flight_tests::coalesced_publication_failure_preserves_the_scheduler_error_family— 1 passed; truncated pointer stays{Dispatch SHA:
6099dda2c4acc73a09b4e2f8516a399455acbe0b