End a feed read's operation the way the caller sees it, not the way HEY answered - #204
Merged
Merged
Conversation
…EY answered Postings::changes and Calendars::recording_changes turn HEY's 409 — the cursor is too far behind for an increment — into a full_sync_required answer. They did it after Client::execute had returned, and execute runs the whole operation lifecycle: the gate, the tracing span, on_operation_start, and on_operation_end from the Running drop guard. So the hooks heard GetBoxPostingChanges and GetCalendarRecordingChanges end with a Conflict while the caller was handed an Ok. A trace showed a failed span, a failure metric counted a success, and a hook that keys anything off the end outcome was told the wrong thing. The circuit breaker was not among them — should_trip_circuit counts network errors and 5xx alone, so a 409 never opened it — but ResilienceHooks records the outcome all the same, and any Hooks a caller wrote heard a failure. Go reports success on both paths: postings.go defers OnOperationEnd on the named err, which is nil on the 409, and calendar_changes.go returns nil from the instrumented closure; the Kotlin client does the same. The client gains a crate-private Client::as_operation that runs some work as one operation: the gate, the span, the start, and the end with the work's outcome, from the same Running guard execute uses, so a dropped future still ends as cancelled. instrument's lifecycle moves into an announced helper the two share. The work is held to the one deadline execute would hold it to and scoped so every send inside inherits it rather than starting a limit of its own; a read that runs out of time ends with the same timed_out error the caller gets. A quiet send made inside the bracket records its status and request id on the bracket's span, through a task-local carrying it, so the trace still shows the 409 the full-sync answer came from — outside a bracket a quiet send runs in its caller's span, as before. The two feeds mark their send quiet and run the send, the 409 conversion and the body decoding inside as_operation under the operation's own info, so the service, operation and resource type the hooks hear are unchanged. The same treatment goes to the two other conveniences that catch an error after execute and reword it: TimeTracks::start_tracking, whose 409 becomes a Conflict carrying HEY's own message, and the contact writes, whose 409 and 422 become a clash and a rejection read out of the body. Their hooks now end with the error the caller gets rather than the one the client first read. Nothing the caller sees changes, and the public API is untouched. Tests on a 409 from each feed assert an Ok with full_sync_required, one operation start, one end with Ok, and a request hook that saw the 409; a 404 on either feed still ends with that error; a reworded refusal ends with the rewording; a refusing gate sends nothing; a read cut off at the operation limit ends with the timeout; and the tracing test sees the 409 on the feed's own span.
There was a problem hiding this comment.
🟢 Approved
The lifecycle refactor is focused, preserves request reporting, and has comprehensive tests for success, failure, gating, timeout, and tracing behavior.
Pull request overview
Aligns Rust SDK operation hooks and traces with the final result callers receive.
Changes:
- Adds
Client::as_operationfor shared lifecycle, deadlines, and tracing. - Applies it to feeds, contact writes, and time-track conflicts.
- Expands observability and tracing tests and updates documentation.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
AGENTS.md |
Documents the new lifecycle pattern. |
rust/hey-sdk/README.md |
Updates quiet-send observability behavior. |
rust/hey-sdk/src/client.rs |
Adds shared operation lifecycle and span propagation. |
rust/hey-sdk/src/operation.rs |
Clarifies quiet-operation semantics. |
rust/hey-sdk/src/trace.rs |
Supports spans announced from operation metadata. |
rust/hey-sdk/src/services/calendar_changes.rs |
Reports converted 409 responses as successful operations. |
rust/hey-sdk/src/services/contacts.rs |
Reports reworded contact errors to operation hooks. |
rust/hey-sdk/src/services/mod.rs |
Documents service convenience conventions. |
rust/hey-sdk/src/services/postings.rs |
Aligns posting-feed lifecycle with caller results. |
rust/hey-sdk/src/services/publications.rs |
Updates lifecycle documentation. |
rust/hey-sdk/src/services/time_tracks.rs |
Reports reworded start conflicts consistently. |
rust/hey-sdk/tests/observability.rs |
Tests lifecycle, gates, errors, and deadlines. |
rust/hey-sdk/tests/services_calendar_changes.rs |
Updates expected recording-feed outcomes. |
rust/hey-sdk/tests/services_postings.rs |
Updates expected posting-feed outcomes. |
rust/hey-sdk/tests/support/mod.rs |
Updates outcome helper documentation. |
rust/hey-sdk/tests/tracing.rs |
Verifies response metadata on enclosing spans. |
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
The defect
Postings::changesandCalendars::recording_changesturn HEY's 409 — the cursor is too far behind for an increment to carry the difference — into afull_sync_requiredanswer. They did it afterClient::executehad returned, andexecuteruns the whole operation lifecycle: the gate, the tracing span,on_operation_start, andon_operation_endfrom theRunningdrop guard. So the hooks heardGetBoxPostingChangesandGetCalendarRecordingChangesend with a Conflict while the caller was handedOk. A trace showed a failed span, a failure metric counted a success, and anyHooksa caller wrote that keys off the end outcome was told the wrong thing.Whether the 409 opened the circuit breaker: it did not.
should_trip_circuitcounts network errors and 5xx alone, soResilienceHooksrecorded the 409 as neither a success nor a failure. It did pass the failure on to the hooks it wraps, though, and the breaker's own accounting now sees the success the caller got.Go already reports success on both paths —
postings.godefersOnOperationEndon the namederr, which is nil on the 409, andcalendar_changes.goreturns nil from the instrumented closure — and the Kotlin client (#198) ends the operation as the success the caller sees.The fix
The client gains a crate-private
Client::as_operation(info, work)that runs some work as one operation: the gate, the span, the start, and the end with the work's outcome, from the sameRunningguardexecuteuses, so a dropped future still ends ascancelled.instrument's lifecycle moves into anannouncedhelper the two share. The work is held to the one deadlineexecutewould hold it to and scoped so every send inside inherits it rather than starting a limit of its own; a read cut off at the operation limit ends with the sametimed_outerror the caller gets. A quiet send made inside the bracket records its status and request id on the bracket's span, through a task-local carrying it, so the trace still shows the 409 the full-sync answer came from; outside a bracket a quiet send runs in its caller's span, as before. The public API is untouched.Conveniences changed
Each marks its send
quiet()and runs the send, the conversion and the body decoding insideas_operationunder the operation's owninfo, so the service, operation and resource type the hooks hear are unchanged. The request hooks still hear what HEY answered.Postings::changes— 409 ends the operation withOk(full_sync_required).Calendars::recording_changes— the same.calendar_changesnever answers 409 and is untouched.TimeTracks::start_tracking— the 409 reworded as a Conflict carrying HEY's own message now ends the operation with that rewording rather than the error the client first read.Contacts::write, behindcreate_contact,update_contactand the note writes) — the 409 read as aContactConflictand the 422 read as a validation error end the operation with the reworded error.Publications::publishandWorkflows::stage_topickeep their quiet read-back as it was; the doc line claiming the SDK had no seam for bracketing a block of sends is dropped since it now has one.Tests
On a 409 from each feed: the result is
Okwithfull_sync_required, exactly one operation start and one end were reported, the end outcome isOk, and the request hook saw the 409. A 404 on either feed still ends the operation with that error. A reworded refusal fromstart_trackingand a contact create ends with the message the caller got. A refusing gate sends nothing. A feed read that runs past the operation limit ends with the timeout the caller gets. The tracing test sees the 409 and request id on the feed operation's own span. The two service tests that pinned the old ending are updated.GOWORK=off make rs-check rs-check-drift conformance-rsis green.Summary by cubic
Changes the operation lifecycle for conveniences that convert or reword what HEY returns, so hooks and traces now report the operation ending the way the caller sees it.
Previously
Postings::changesandCalendars::recording_changesturned a 409 into afull_sync_requiredsuccess afterClient::executehad already reported the operation as failed. Now these feeds, along withTimeTracks::start_trackingand the contact writes, run their send and conversion inside a new crate-privateClient::as_operationthat keeps the gate, span, start, and end hooks aligned with the final result. Request hooks still see the raw HEY status; operation hooks now end with the caller's outcome. Tracing spans for these operations also record the underlying response. Public API is unchanged.Written for commit ebbaa9d. Summary will update on new commits.