Absorb fleet soak harness ancestry into the live stack - #1154
Conversation
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
|
Warning Your free Security trial is over. An organization admin can activate billing to continue. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Warning Your free Security trial is over. An organization admin can activate billing to continue. Comment |
|
@codex review Please review exact head |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
…mmit The retained-tool-content ceiling changes what the page states as implemented behavior, so the page's verified-against reference advances to the commit that implements it, per the pre-push self-review rule. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jz817TssAaL2SzfthXwAP6
Re-verify the container start-site inventory the previous wave invalidated: the convergence sweep runtime added a marked testcontainers start, so the tooling count moves from 40 to 41. That count is what turned CI red. 202608210400 through 202608210404 all land in this unmerged slice, so no database has held an inactivity park without the parked-dispatch columns. Drop the backfill UPDATE and the NOT VALID arm: both were data-upgrade scaffolding for a schema that was never deployed, and the schema now asserts unconditionally that every inactivity park carries a dispatch identity. Revalidate the complete paginated connections after the final details fence rather than before it. The fence compares only the initial pages, refs, and page information, so a thread or check on a later page could previously change after its own reread but before the fence and be accepted from a census the provider had already invalidated. Unroll the parked-target and capped-backoff test setups onto named helpers so each transition is inspected at its own call site. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jz817TssAaL2SzfthXwAP6
…spatch-start-lease # Conflicts: # crates/persistence/tests/repo_watch_dispatch_postgres.rs # docs/spec/repo-watch.md
`cargo fmt --all --check` reported a diff in `model_execution.rs`: the `projected_tool_content_bytes` fold and its test-side counterpart were written with the receiver on its own line, which rustfmt collapses onto the `fold` call. Formatting only -- no behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jz817TssAaL2SzfthXwAP6
`mdformat --check *.md docs/` reported `docs/spec/persistence-protocol.md` unformatted: the paragraph describing the recovery lap's page bound was wrapped by hand, leaving an em dash starting a line and a short final line that mdformat's wrap=80 rejoins. Reflowed by the pinned toolchain in `tooling/requirements-mdformat.txt`; prose unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jz817TssAaL2SzfthXwAP6
… wf-pr-1084 # Conflicts: # clients/web/e2e/product-shell.spec.ts # clients/web/src/ProductApp.tsx # clients/web/src/commands.ts # clients/web/src/preferences.test.ts # clients/web/src/preferences.ts # clients/web/src/product.test.ts # clients/web/src/product.ts # clients/web/src/session-timeline/model.ts # clients/web/src/state.ts
`mdformat --check *.md docs/` reported `docs/spec/tool-loop.md` unformatted: the verification reference this branch extended and the retained-tool-content paragraph it added were hand-wrapped past the 80-column bound `.mdformat.toml` sets. Reflowed by the pinned toolchain; prose unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jz817TssAaL2SzfthXwAP6
`Bound each supersession cursor lap` rewrote `AUTOMATIC_MODEL_CALL_RECONCILIATION_SUPERSESSION` in `crates/persistence/src/lock_inventory.rs` -- adding the materialized bounds CTE and its high-water mark -- but left the pinned checksum at the value an earlier commit in this branch recorded, so the gate that makes changes to the reviewed inventory explicit failed on its own branch. Pin the checksum to the inventory this branch actually carries. The scan for strongest-mode row locks outside the inventory is unchanged and still finds none. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jz817TssAaL2SzfthXwAP6
`reconcile` opens a transaction that takes the delegated child endpoint locks and then the inventoried strongest-mode `session_scheduler` row lock, both unqualified, without a server-side `lock_timeout`. Its two siblings both set one: `claim_due` binds `CLAIM_LOCK_WAIT`, and `recover_running_candidate` binds `CANDIDATE_RECOVERY_LOCK_WAIT` through `bound_candidate_recovery_lock_wait`. The attempt runs under a client-side timeout in the daemon watchdog, but dropping that future queues a `ROLLBACK` rather than sending a `CancelRequest`: the backend keeps waiting for the lock and the pooled connection stays checked out for the full real wait while the caller has already given up and will retry. That is exactly the mechanism the `bound_candidate_recovery_lock_wait` doc comment records — contention turned into connection exhaustion — and this transaction now runs under live traffic where it once contended with nothing. Set the transaction-local `lock_timeout` before the first lock is taken, at the siblings' budget. The wait then ends as `55P03`, which this repository already records as an ordinary infrastructure failure spent against the attempt budget, with nothing read or written; the statement can only interrupt a lock wait, never a commit. The new integration test holds the scheduler row and asserts the attempt gives it up as `55P03`, leaves the turn parked and the recovery row `attempting`, and reconciles on retry once the contention clears. Without the fix it blocks past a 30s client bound. The inventoried SQL is unchanged, so the explicit-lock inventory checksum stands. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jz817TssAaL2SzfthXwAP6
The dismissal path could not run. fetch_convergence_evidence reports gating_check_inventory_quiesced as a conservative default, and the polling path replaces it with the verdict computed against the freshness the last committed cursor published. The clearance revalidation never did, so its assessment was always unsettled, the candidate lookup short-circuited before its provider request, and dismiss_review_node was unreachable. The in-code comment asserting "Clearance candidacy does not consult it" was wrong in both halves: candidacy does consult settlement, and refuses every unsettled head. The revalidation now applies the same rule through the same helper. The two observations it compares are the committed poll that raised the candidate and this pre-dismissal re-read, so a gating check that appeared in between leaves the head unsettled and the review undismissed until a later poll sees the inventory hold still. PullResponse gained the updated_at the listing already carried, since the comparison is against what a committed poll recorded from that listing. Freshness is now published immediately after the cursor commit rather than at the end of the sweep: the cursor is durable at that point, and a publication the clearance sweep never reached would leave every candidate unsettled. A failed attempt still invalidates every entry on its way out. Fixing that activates a second hole, so both land together. The candidate rule never checked gating_check_count, so a head that ran no gating check at all — whose empty non-green list is indistinguishable from a fully green head's, and which the reference convergence rule therefore counts as blocked — was admissible. With the revalidation repaired, the daemon would have dismissed blocking reviews on pull requests with zero green checks. The candidate predicate, the poller's mirror of it, and the durable eligibility query now all require a positive count, matching RepoWatchConvergenceAssessment::try_new. Tests: a quiesced inventory carries a planned clearance through to its dismissal, an inventory that grew since the committed poll refuses it, a zero-check assessment yields no candidate, and the durable query plans nothing for a recorded assessment without a gating check while still planning for one with it. RepoWatchPlannedStaleReviewClearance gained a test-support constructor so the revalidation is testable without a store.
`projected_tool_content_bytes` walked only the resolved tool evidence, so the 256 MiB ceiling counted request arguments, result text, error detail, and denial reasons while `render_frontier_messages` also cloned assistant text, context summaries, delegation task, message and outcome content, origin and steering user content, and attested imported text. `AssistantText` carries no length bound of its own -- `NonEmptyUnicodeText` bounds nothing and no migration adds an `octet_length` CHECK -- so only the transport cap bounds a single response, and the round ceiling multiplies that uncounted content by 256 rounds against a ceiling that never sees it. The accounting is now `projected_frontier_content_bytes`, which sums both content sources a render draws from -- the projected payloads and the evidence they name -- with one term per shape the renderer clones and zero for every shape it skips or refuses. The constant keeps its 256 MiB value and the refusal keeps closing through the `ToolRoundLimitReached` pre-send contract; both are renamed from "tool content" to "frontier content" so the bound's name matches what it counts. `render_within` also deep-cloned every semantic entry into a `Vec` before the guard ran, which made the documented "checked before any content is cloned" ordering true only for tool evidence: the payload content was already duplicated. The projection now borrows the durable frontier through `PreparedModelCallRequest::frontier_entry_slice`, and origin content is measured by reference, so nothing is cloned before the ceiling decides. Tests: the accounting-fidelity test now compares the counted bytes against the bytes the rendered messages actually hold across tool evidence, assistant text, and origin content, and a second test does the same for the payload kinds the fixture cannot reach -- imported text, delegation content and outcomes, context summaries -- against both the render and an explicit per-term sum. A new case puts a frontier over a ceiling set to its own text-free byte count, proving the assistant text is what refuses it, and that the refusal still precedes a rendering failure the same inputs would otherwise raise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jz817TssAaL2SzfthXwAP6
…mmit The spec's verification reference still named the tool-evidence-only accounting and the commit that introduced it. Both now name the commit where the ceiling counts every kind of content a render clones. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jz817TssAaL2SzfthXwAP6
The migration carried prefix 202608140102, below main's highest (202608200003), so on merge sqlx would apply it out of order against the _sqlx_migrations ledger. Renumber to 202608200004 and update the migration range in the persistence protocol spec. The file is unmerged and unrecorded, so the pre-merge renumber is the sanctioned correction. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jz817TssAaL2SzfthXwAP6
The renumber moved the highest migration prefix to 202608200004; the spec's stated range still ended at 202608200003. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jz817TssAaL2SzfthXwAP6
Rust coverage (report only)Report only. This measurement has no threshold, gates no merge, and
What this number does not measure
Per crate, least-covered first
25 files with the most uncovered lines
Measured at |
Native client coverage (report only)Report only. This measurement has no threshold, gates no merge, and 49.22% of product lines covered (14561/29584). The
30 files with the most uncovered lines
Measured at |
Summary
Meaningfully changed lines excluding lockfiles: 0. The merge is ancestry-only and its tree is byte-identical to the immediate base.
Numeric-bound ceilings added by this slice: none.
Absorption ledger
4cdc5dfbc: fleet soak harness; its scenarios are superseded by the current live-line recovery assertionsValidation
The immediate base passed both affected PostgreSQL restore-reachability tests, workspace check, warning-denied clippy, doc tests, warning-denied rustdoc, Cargo metadata, formatting, and contract checks. This merge has an empty tree diff, and the release daemon artifact remains byte-identical, so advancing the deployment ref requires no process restart.