fix(daemon): start open publication bound at claim - #1807
Conversation
The 500 ms wait answered how long the connection had been here, so route enrollment could spend the budget before the request joined the open. A restart then reported warming for a store the open had already refused with reset_required. The bound now starts at the claim, and the waiter reads a recorded refusal before the cache probe a deadline can cancel. Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
|
The paused enrollment-delay check needs an explicit output type so the publication wait can see the reset_required refusal. Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
|
Complements #1794 ( |
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. |
![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: bf0eeb4604
ℹ️ 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 let ProjectOpenTaskState::Failed(failure) = | ||
| state.borrow().clone() | ||
| { | ||
| return Err(failure.to_error()); |
There was a problem hiding this comment.
When this waiter is descheduled after its claimed task fails, ProjectOpenTaskRegistry::prune can remove the completed failure (or discard a stale reset refusal), allowing another request to open and publish the same route before this waiter resumes. This new pre-cache return then reports the old receiver's failure even though a valid owner is available, contradicting the existing a_recorded_open_failure_replaces_only_the_warming_hint invariant that a published owner outranks a recorded failure; probe the cache before returning the failure, as the previous ordering did. The portable loop contains the same regression.
Useful? React with 👍 / 👎.
The pre-cache `Failed` read made a waiter answer with its own claim's recorded failure even when another open had since published a server for the same route, contradicting the invariant `a_recorded_open_failure_replaces_only_the_warming_hint` states. The read was also redundant. With the bound measured from the claim, the publication wait always starts with a full budget, so an elapsed deadline can only preempt a later iteration, and `prefer_recorded_open_failure` already substitutes the recorded failure for that warming hint. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`enrollment_delay_does_not_hide_a_quick_reset_required` computed its own deadline with `project_open_publication_deadline(now)`, so it never observed where the request loop arms the bound. Reverting the production change to master's connection-arrival placement left the test passing, which is the detection it claimed to provide. `reset_required_survives_http_mcp_and_rust_sdk_across_restart` remains the detector for this defect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Review from the merge sweep. The fix is right and nothing on master subsumes it. Master's Two changes pushed on top of the author's commits, plus a merge of
Verified in a clean lane: |
2 participants
Summary
reset_requiredrefusal is read before the cache probe an elapsed deadline can cancel, so a restart's first HTTP observation is the refusal instead of retryableunavailable.Motivation
Master tip
3f71bccb2dbastill has the connection-bound race that failedreset_required_survives_http_mcp_and_rust_sdk_across_restarton run 35411306897: the first HTTP observation after restart wasapplication.surface.unavailable/after_delayfor a store whose shape isreset_required.The shared premise of the later test polls (
e2c8b592cd, #1794) is that the client must ride out warming. That premise is wrong for this refusal. The 500 ms bound answered how long the connection had been here, so route enrollment spent it before the request joined the open. The wait then returned immediately and never read the refusal already on the watch. CLI clients retried; a one-shot HTTP or SDK call reported warming for a terminalreset.#1794's test poll and #1797's unrelated live-daemon fixes are not absorbed.
.github/workflows/ci.ymlpull_requesttriggers are unchanged.HEAD:
bf0eeb460405e5c1a63db4bab5aeae0b61f4a145Changes
project_open_publication_deadlinemeasures the bound from the claim. Both the Unix and portable request loops use it.Failedbefore the cache-probe await.enrollment_delay_does_not_hide_a_quick_reset_requiredfails if enrollment time is charged to the bound: the publication future is pending on its first poll, which is when an already-elapsed bound used to win.Test plan
daemon::tests::bootstrap::enrollment_delay_does_not_hide_a_quick_reset_required— 1 passedforeground_project_open_wait_is_bounded_and_accepts_quick_publicationanda_recorded_open_failure_replaces_only_the_warming_hint— 2 passedexplicit_init_retries_after_joining_an_ordinary_missing_database_open— 1 passedreset_required_survives_http_mcp_and_rust_sdk_across_restart— 1 passed in 3.57s, then 5/5 reruns in 2.72–2.81sChecklist
.envfiles includedCHANGELOG.mdupdated (daemon admission timing; no user-facing contract change)