Devana fixes - #6
Conversation
option() flattened custom fields into JobCreateRequest.extra, where serde flatten lets colliding keys (tasks/tag/webhook_url/redirect) overwrite the canonical struct fields at serialize time. Route both JobBuilder::option and JobGraphBuilder::option through insert_option, which drops reserved keys so the built task graph and core properties cannot be corrupted. Fixes devana extra-overwrites-job-core-fields (P1).
Each typed task struct flattened its option() extras map AFTER the canonical
fields. With serde emitting fields in declaration order and serde_json keeping
the last write per key, an option("input", ...) etc. could overwrite the
SDK-set task wiring at serialize time.
Move #[serde(flatten)] extra to the first field of every task struct (16 typed
tasks + the pdf_task! macro) so canonical fields are written last and win.
Add invariant comments at each flatten site and a regression test.
Fixes devana extra-overwrites-task-core-fields (P1).
send_api_with_retry replayed any failed request, including POST creates. A transient 5xx or timeout after the server accepted a create could re-send it and produce duplicate jobs/tasks/webhooks. Inspect the built request method and only allow multiple attempts for idempotent methods (GET/HEAD/OPTIONS/TRACE/ PUT/DELETE); POST/PATCH are sent exactly once. Add a test asserting a flaky POST create is attempted once and surfaces the error rather than being retried. Fixes devana retry-replays-non-idempotent-posts (P1).
CloudConvertSocket emitted subscribe payloads only once at connect. After a transport drop, rust_socketio restored the transport but the server never received another subscribe, so managed waits (wait_socket/create_and_wait_socket) stopped receiving terminal job/task events and failed spuriously. Register an Event::Connect handler (fired on every (re)connection) that re-emits the stored subscribe payloads. Pre-serialize payloads up front so serialization errors still surface before connecting; keep the explicit initial subscribe loop for emit-error propagation. Fixes devana socket-reconnect-loses-subscriptions (P1).
When the socket channel closed without a terminal event, wait_socket returned Error::Socket immediately. A job/task that finished during a subscribe race, reconnect gap, or dropped buffer event was misreported as a failure. On next_event() == None, do a final get(): return Ok if the resource is already terminal, otherwise return the socket-closed error. Mirrors the existing None-payload fallback in the terminal-event arm. Applied to both Jobs and Tasks. Fixes devana wait-socket-no-close-reconciliation (P1).
api_url resolves endpoints with base.join(path). A custom base without a trailing slash (https://api.example.com/v2) joined with "jobs" drops the /v2 segment per RFC 3986, causing 404s/misrouting for custom-base integrations. Add ensure_trailing_slash and apply it to both resolved base URLs in ClientBuilder::build. Defaults already end with / and are unchanged. Fixes devana custom-base-url-join-strips-path (P2).
JobBuilder::task and add_named_task did a bare BTreeMap::insert, so a second task with an existing explicit name silently replaced the first, dropping a pipeline step while input references still pointed at the name. Extract deduplicated_task_name from generated_task_name and route explicit-name inserts through it, so duplicates get a -2/-3 suffix (matching add_task). add_named_task returns the actual stored handle. Graph add_named_task is fixed by delegation. Fixes devana duplicate-task-name-silent-overwrite (P2).
The callback uses tokio mpsc send().await, which applies backpressure on a full buffer (waits for capacity) and only errs when the receiver is dropped. It does not discard events. rust_socketio awaits the callback inline without timeout, so a blocked send stalls until the wait loop drains rather than dropping the terminal event. No silent-drop bug exists. Closes devana socket-buffer-drops-terminal-events (P2, invalid).
upload_part sent every parameter through form_value, which mapped Value::Null to an empty string and submitted it via multipart.text. A null-valued optional parameter became an extra empty form field outside the signed parameter set, which can break strict presigned-POST signature validation. Skip null parameters (treat them as absent). Extend the upload test to assert the null fixture field is omitted while real fields remain. Fixes devana upload-form-null-becomes-empty (P2).
validate_task_strict flagged any payload key not in operation.options and not in is_common_task_field, which listed only 8 shared fields. Structural fields like url/headers, provider credentials, and command/metadata payloads were rejected as unknown options, so non-convert tasks could not use strict mode. Expand is_common_task_field with the SDK's structural fields (I/O references, object-storage location/credential fields, command/metadata payloads). Tunable options (width, fit, font_*, opacity, profile, ...) are intentionally excluded so strict mode still validates them against the options map. Fixes devana validate-strict-rejects-operation-fields (P2).
validate_task only checked the operation name and option-level constraints, never the Operation record's input_format/output_format/engine/engine_version. A task with the wrong output_format for that record passed validation. Add a FormatMismatch error kind and compare the task payload's canonical format/engine fields to the record (when both specify them), in both lenient and strict modes. Fixes devana validate-task-ignores-format-identity (P2).
wait_socket returned the embedded job/task on a terminal-named socket event without checking is_terminal() on the deserialized status. An event named job.finished carrying a non-terminal or unrecognized (Unknown) status would return a non-terminal resource. Only accept the embedded resource when it is actually terminal; otherwise reconcile via get() and, if still non-terminal, keep waiting. Applied to Jobs and Tasks. Fixes devana wait-socket-skips-terminal-check (P2).
validate_value_for_operation compared the whole submitted value against each possible_values entry. For an array-typed option, possible_values enumerate allowed array elements, so any submitted array was compared against scalar strings and always rejected as InvalidOptionValue. When kind == Array and the value is an array, validate each element is a member of possible_values instead of the whole array. Add a regression test against the recorded metadata-write remove option. Fixes devana validate-array-option-rejects-valid-arrays (P2).
The retry loop parsed Retry-After only as u64 seconds, so an RFC 7231 HTTP-date value (sent by some proxies/gateways) failed parsing and silently fell back to the small exponential delay, retrying far inside the server's stated window. Add parse_retry_after: try delta-seconds, then httpdate::parse_http_date (delay from now; past dates yield zero). httpdate is an optional dep gated by the retry feature (already present transitively). The max_delay cap on explicit directives is kept by design (tested safety bound). Fixes devana retry-after-http-date-dropped (P3).
backoff_factor only clamped the lower bound, leaving f64::INFINITY intact, and next_retry_delay used Duration::mul_f64, which panics on a non-finite or overflowing product before the max_delay clamp. Reject non-finite factors in the setter, and compute the next delay with checked float math (try_from_secs_f64) that saturates to max_delay instead of panicking. Add unit tests for both. Fixes devana retry-backoff-factor-mulf64-panic (P3).
|
Warning Review limit reached
More reviews will be available in 48 minutes and 15 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (18)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@codex review |
![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: 41c285b649
ℹ️ 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".
| | "engine_version" | ||
| | "filename" | ||
| | "timeout" | ||
| | "url" |
There was a problem hiding this comment.
With strict validation enabled, every name in this global allowlist bypasses the !self.options.contains_key(name) check for every operation. That means a strict convert validation now accepts payload keys such as url, bucket, or other import/export-only fields even when the operation metadata does not document them, so validate_task_strict misses the unknown-option errors it is meant to catch for those inputs.
Useful? React with 👍 / 👎.
1 participant
No description provided.