Repository navigation
test(cli): verify daemon readiness and oversized response delivery - #4834
ScriptedAlchemy wants to merge 30 commits into
Conversation
Retention fast probes never join the daemon writer queue. Under sustained store-scoped writers, even a fair RwLock cannot admit the maintenance pass. After two per-store nonqueued refusals, join the fair writer queue with cooperative shutdown cancellation; reset/prune the counters on admission. Exercise bounded deferrals, progress with competing store writers, and cancellation of the queued acquisition in native Rust tests. Refs #4616
Drive the existing registered-store production maintenance tick through two real writer refusals and then a queued admission, with six durable code-index generations and a concurrent unrelated store writer. Verify that the writer release enables the same tick to collect five superseded generations. Refs #4616
A shutdown cancellation during an already-admitted retention operation must not drop journaled work that can still own blocking store clients. Separate the cancellable writer wait from the critical section, preserving cooperative operation completion and the existing writer lease after admission. Add a native Rust test that cancels after admission and requires the critical section to complete and release the gate. Refs #4616
The old finite competitor burst could finish before maintenance acquired the daemon gate. Keep unrelated store writers active through the queued retention admission, asserting churn both before and afterward and only stopping writers after the admission proof. This remains a focused gate regression; sealed-artifact N>3 acceptance requires its own native test. Refs #4616
Refuse already-cancelled retention before either fast or queued admission, without interrupting operations that already own the writer gate. Start ongoing writer churn before queueing exclusive retention. Explicitly poll queue admission and a later store writer instead of waiting for later readers to make progress behind the exclusive waiter. Check that cancelling the waiter admits its follower while the original shared holder is alive. Add cancellation-before-admission coverage and restore the native runtime import required by the real maintenance-tick regression. Refs #4616 Bazel/native execution remains required; no Cargo or CI changes.
Use a fair store-local maintenance lane shared by owner/content writers, while retaining daemon-wide exclusivity for truly profile-wide work. Queue verified project retention only against that project's writers and preserve independent project opens during a long running sync. Retain nonqueued daemon-wide probes for session work that lacks proven per-store identity. Exercise same-store fairness, independent writer progress, cancellation and a real registered-project GC tick. Refs #4616
The store-local writer admission regression borrowed a temporary WriterScope across a pinned asynchronous acquisition. Native Bazel Rust compilation fails with E0716 at writer_gate.rs:401; retain the exact scope in a binding until that future completes. Existing behavioral regressions continue proving fairness and cross-project isolation. Refs #4616
Repair three hosted automation_runner_test failures by supplying the configured executable identity required by the production prefilter. Use the same identity for the prefilter and retained runner, preserving manual-schedule, exact-ledger, live-lock and lock-race assertions. Add a production-prefilter regression for unconfigured refusal, deduplication and same-occurrence recovery after binding, with zero backend calls and no effectful terminal. Do not relax backend admission, change timeouts, or alter the graph publication changes. Refs #4532
|
Keep admission receipts for phase diagnostics without granting another response timeout. Both oversized-result journeys use one absolute eight-second deadline for startup, response delivery and process exit. Add a real subprocess regression proving a queued admission receipt cannot restart an already expired watchdog. Preserve all response, retrieval, permission and cancellation assertions.
…ion-writer-admission
|
Refs #4787 — new prerequisite verification blocker on exact head I read hosted native macOS run 42606227916, Test macOS job 129122828921 (completed 2026-11-09 UTC). Bazel's The test and assertion are unchanged between current |
|
Refs #4787 — native macOS prerequisite retry also failed on unchanged #4834 head I inspected selective retry / attempt 2, Test macOS job 129343281326 rather than treating an Actions summary as a source diagnosis. Bazel The retry's isolated daemon log records This remains an integration blocker for |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
🔍 Devin Review: 1 flag
Not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
…-writer-admission Refs #4616 Merge the passing, non-draft retention writer-admission PR into its existing stack target. Preserve branch history and all prerequisite changes; this is not full issue acceptance.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ScriptedAlchemy
left a comment
ScriptedAlchemy
left a comment
There was a problem hiding this comment.
Refs #4787 — independent retention-scheduler review of current head 2fd6a8f1 after #4813 merged into this prerequisite branch.
Confirmed cross-store progress blocker (not the original CPU root-cause diagnosis): admit_retention_writer switches from try_with_writer_in to with_writer_cancellable_acquisition after two store-local deferrals. The latter is awaited inside the sequential for &index in &window loop in MaintenanceCoordinator::run_tick. With store A's gate held, its third retry waits indefinitely for A (unless global shutdown), and neither free store B, the cursor update, nor profile-wide retention can execute. run_cadence awaits the whole tick, so it cannot schedule a new round-robin pass. Per-store gate fairness is useful, but queueing from inside the serial tick violates the independent-store progress invariant.
Please resolve in the owning writer-admission lane, not in #4798: keep the tick bounded/nonblocking and queue/retry any contended store independently, preserving canonical store identity, cancellation-before-admission and completion-after-admission semantics. Add a nonvacuous behavioral regression with A's writer held beyond two retry ticks, B's maintenance and profile sweep actually completing before A is released, then require A to complete once released, without duplicate jobs or unsafe cancellation. The existing same-store fair-writer test does not prove cross-store tick progress.
The existing automated inline finding on this commit identifies the same failure class; this review independently checked the two actual call sites and awaited cadence rather than treating a bot verdict as a test. No source mutation, test weakening, merge authorization or affected-host CPU claim. #4787 remains open; keep #4776/Kev drafts protected and release #4763 untouched.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Transfer contended fair acquisitions and their work to supervised native jobs so store permits are consumed and released between ticks. Keep CPU admission with running work, retain exact work identities, cancel queued acquisitions and join admitted work during shutdown. Reproduce the ordinary-writer stall on the prior implementation, then verify native between-tick completion, cross-store generation and profile progress, cancellation, replacement gates and cooperative shutdown. Focused maintenance and writer-gate tests, the daemon capacity-retirement journey, Clippy, formatting and generated BUILD checks pass.
There was a problem hiding this comment.
Devin Review found 2 new potential issues.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| MaintenanceStoreWork::Registered { | ||
| project_id, | ||
| store, | ||
| profile_database: profile_database.clone(), | ||
| }, |
There was a problem hiding this comment.
🔴 Queued jobs pin the profile database
When a registered store is busy, RetentionWriterJobs holds its profile_database lease beyond the tick. That counted client prevents profile-store retirement until a later census drains the job, even after its work finishes.
Learn more
A registered profile lease owns a counted database client, not merely a reference to profile authority. RegisteredGlobalDbLeaseV1 shares that client across clones. A queued job captures the cloned work unit and remains in RetentionWriterJobs until a later census consumes its result or shutdown joins it. A busy store can therefore keep the profile database open between ticks; if census fails or CPU admission is unavailable, the client remains even after the task finishes.
Example: Store A has a long content writer when retention enqueues A's registered work. The tick ends, but the queued job still holds its profile lease; a profile-store retirement cannot close its last client even though maintenance has not entered A.
Recommended fix: Keep only the project ID and canonical store identity in queued work. Issue a fresh profile lease after admission, and release it when the job finishes; ensure pending outcomes cannot retain a counted client.
Was this helpful? React with 👍 or 👎 to provide feedback.
| for (_, job) in jobs { | ||
| join_cancelled_maintenance_task( | ||
| Some(job.task), | ||
| "retention_store", | ||
| DAEMON_TASK_ABORT_DEADLINE, | ||
| ) | ||
| .await; |
There was a problem hiding this comment.
🔴 Retention jobs multiply the shutdown deadline
With multiple admitted retention jobs stalled, shutdown gives each job a fresh abort deadline in sequence. Each join can wait twice, so daemon shutdown exceeds its bounded deadline by the number of jobs.
Learn more
The shutdown path joins the main maintenance task before entering this loop. join_cancelled_maintenance_task first waits DAEMON_TASK_ABORT_DEADLINE, then aborts and waits for the same duration again. A separate call for each job multiplies the shutdown bound, even though those jobs can run concurrently.
Example: Four admitted stores each await a blocked storage operation for longer than two seconds. Shutdown can spend roughly eight seconds on their initial joins before it has even attempted the last job's abort, rather than limiting the group to two seconds.
Recommended fix: Cancel all jobs, join them concurrently under a single absolute cooperative deadline, then abort remaining tasks and bound their final join against one shared deadline.
Was this helpful? React with 👍 or 👎 to provide feedback.
macOS sandbox and transport qualification
Once DNS recovered, the macOS run exposed five CLI fixtures writing outside the sandbox and a slow-response timeout. On
05f4a292a6, all 201 native CLI cases passed. An additional run that denied all writes to/private/tmpexecuted the five affected cases and passed all five. The daemon endpoint stayed within the Darwin Unix socket path limit.The real CLI exceeded its eight-second deadline on macOS background QoS when every socket byte added a fresh relative sleep. Pacing all 640 individual writes against one monotonic start preserved the two-millisecond cadence and the original deadline. The previously failing background-QoS case and all ten authenticated CLI transport cases passed. The full native runtime acceptance suite passed all 142 cases in 68.87 seconds. Hosted Linux, macOS and Windows verification.
The hosted macOS run on 05f4a292a6 passed the slow-response regression. Of 343 tests, 341 passed. The orphan-store fixture's default socket path was 106 bytes on the runner. The Cursor journey restarted its daemon before ingest, then started the ingest deadline at socket bind.
At
9cb2657d0b, the orphan fixture uses a one-character home beneath its isolated temporary root. The real CLI reported the expected orphan on a 65-byte Darwin scratch path with writes to /private/tmp denied. The Cursor journey now initializes and ingests through the same daemon after the logged readiness event. The exact journey passed three native runs in 6.43s, 2.26s, and 2.25s. Both full Bazel suites passed locally (201 CLI and 142 runtime acceptance). The original assertions and deadlines remain in force. New hosted native macOS run.Changes
The daemon socket permission test now waits for the isolated daemon to report readiness before reading the published socket mode. It retains the exact owner-only permission assertion. This reuses the readiness correction tested in #4789.
Two oversized JSON journeys share an authenticated request receipt timestamp from the scripted daemon. Startup must reach that receipt within eight seconds. Response delivery then gets eight seconds from the actual receipt, including encoding, socket I/O, retained result storage and CLI exit. This replaces the single combined watchdog in these two tests and gives each failure a phase. The held-stream case retains its complete listing and >64 KiB checks. The retrieval case still pages through the shipped daemon, verifies every offset and the final character count, and compares all 1,024 files with the expected JSON. This shares the held-stream admission approach from #4813.
Evidence
The #4827 Linux test reached the successful scripted-daemon join before reporting that the watchdog killed the CLI. The request had been admitted and its response written. The artifact has no timestamps for those stages, so it cannot assign the delay to startup, response delivery or a production stall. Linux job · Bazel logs
Temporary timings from three native macOS runs of the original retrieval fixture measured 105.7–110.9 ms from spawn to the scripted response worker, 1.71–2.01 ms for fixture encoding, 0.14–0.16 ms for the write, and 42.8–47.7 ms from the completed write until CLI exit. All three wrote 87,705 bytes to stdout and passed real paged retrieval. The timing instrumentation has been removed.
On the final fixture, all nine tool-client transport journeys and the owner-only socket journey passed under Bazel on macOS. Both affected crates passed their Bazel rustfmt checks. On
7264f0d6c2, the Linux fleet passed all 183 targets. Both affected binaries ran uncached: the runtime acceptance suite passed in 102.8s, and core CLI in 157.2s. The targeted native Windows suites passed. The updated shared admission watchdog fromec0b338d8dremains in the current two-file fixture diff. The current verification run includes the Linux fleet and both affected suites on native Windows. The Clippy job could not fetch the pinned Linux hermetic-launcher finalizer because GitHub returned HTTP 504. Clippy qualification was pending on that earlier head.The previous verification run checks
2155039730against #4837 at2f533c434e. This head includes the Linux launcher download prerequisite. The 2-file feature diff has the same paths and stable patch ID after updating the prerequisite.The updated Linux fleet on
2155039730passed all 183 targets, including uncached runtime acceptance (109.3s) and core CLI (141.7s). The exact Clippy and Dashboard checks passed. Targeted native Windows stopped before tests when the pinned rules_python archive returned HTTP 504. None of those results changes the draft hold.The current
252331298dtargets draft #4847. Both CLI fixture files match the prior head byte for byte. The combined Linux and targeted native Windows workflow uses the authenticated release fallback. Keep the prerequisite and this PR held for review and qualification.