Add app-server current-time impl (3/n)#28835
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d473fdc80a
ℹ️ 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".
| thread_id: ThreadId, | ||
| ) -> Result<DateTime<Utc>> { | ||
| let connection_ids = thread_state_manager | ||
| .current_time_capable_connections_for_thread(thread_id) |
There was a problem hiding this comment.
Wait for subscriber attachment before reading app-server time
For threads created outside the synchronous thread/start path, such as subagents, client subscription is attached later by the thread_created_rx background branch, but the first model request can call this provider immediately. With clock_source = "app_server_client" and a reminder due on the first request, this lookup can see zero current-time-capable subscribers and fail the turn even though an initialized client would be attached moments later; make creation/attachment ordered for these threads or have the provider wait/retry for the initial subscriber.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed cleanly for the supported single-client case in 5617874.
There was a problem hiding this comment.
is this a real problem? how can a thread be started without a client?
e8c21f1 to
3e4d0c9
Compare
2c669b3 to
57c2e3a
Compare
3e4d0c9 to
38a6d16
Compare
57c2e3a to
6dbca65
Compare
38a6d16 to
d45f58b
Compare
6dbca65 to
c5f8cf9
Compare
d45f58b to
4d678ec
Compare
c5f8cf9 to
92e446b
Compare
4d678ec to
f09e32e
Compare
cc336fc to
8bb2652
Compare
14e4299 to
0539ddc
Compare
8bb2652 to
272f0ae
Compare
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn wait_for_thread_subscriber_unblocks_after_connection_attaches() -> Result<()> { |
There was a problem hiding this comment.
is see how it is related
pakrym-oai
left a comment
There was a problem hiding this comment.
should we simplify and drop capability
0539ddc to
671d4f5
Compare
1e18383 to
a24c554
Compare
| }, | ||
|
|
||
| /// Read the current time from an external clock owned by the client. | ||
| CurrentTimeRead => "currentTime/read" { |
There was a problem hiding this comment.
can we mark experimental?
671d4f5 to
933b6ab
Compare
9b945b9 to
55871a2
Compare
933b6ab to
5396ac2
Compare
5396ac2 to
ba927fa
Compare
4b6b95c to
deff2c0
Compare
deff2c0 to
9280920
Compare
What
Server should request:
Client should respond with something like:
Why
Sessions configured with
clock_source = "external"need a thread-specific external time source before inference. The system clock remains the default production provider.Validation
cargo test -p codex-app-server-protocolcargo test -p codex-app-server --test all current_time_read_round_trip_adds_reminder_to_model_inputcargo test -p codex-app-server first_attestation_capable_connection_for_thread_only_uses_thread_subscriberscargo test -p codex-analyticsjust fix -p codex-app-server-protocoljust fix -p codex-app-serverStacked on #28824.