Skip to content

Queue parity: (app_id, request_id) signatures, cancel, webhooks#4

Closed
mculp wants to merge 9 commits into
mainfrom
feat/queue-parity
Closed

Queue parity: (app_id, request_id) signatures, cancel, webhooks#4
mculp wants to merge 9 commits into
mainfrom
feat/queue-parity

Conversation

@mculp

@mculp mculp commented Jun 7, 2026

Copy link
Copy Markdown
Owner

Second PR in the parity stack (stacked on #3). Brings the queue API in line with the official fal clients (JS / Python / PHP).

Review #3 first — this branch targets chore/foundation, so the diff here is only the queue work.

What changed

  • EndpointId — a value object that parses fal-ai/flux/schnell into the run path (full id) and the queue application path (fal-ai/flux). Namespaced ids (workflows/, comfy/) keep three segments. This is the one place that knows the parsing rule.
  • Endpoints rebuilt around itRun/Submit address the full id; Status/Result/Cancel are built from the app root, so nested ids resolve to the correct /{app}/requests/{id}/... path. Submit appends ?fal_webhook= when given a URL.
  • Queue#status / #result now take (app_id, request_id) instead of a raw URL — matching the other clients and the README. ⚠️ Breaking, but pre-1.0 and the previous form was undocumented.
  • Queue#cancel(app_id, request_id)true once cancellation is requested, false on HTTP 400 (already finishing), raises otherwise.
  • logs: on subscribe/status; webhook_url: on subscribe/submit.
  • SubmitResponse gains app_id and cancel_url.
  • Connection#put added; post/get/put collapsed onto one verb-dispatching path.

Why the integration tests are a correctness proof

The queue is per-app: fal-ai/flux/schnell submits under the full path but is polled under fal-ai/flux. The existing nested-model integration tests stub the server's app-rooted URLs end to end, so a green suite means EndpointId constructs exactly the URLs the gateway returns — the same behaviour the prior "trust the returned URL" code had, now with the ergonomic (app_id, request_id) API.

Verification

  • 158 examples, 0 failures on Ruby 3.4.2 and 4.0.0 (up from 136).
  • RuboCop: 34 files inspected, no offenses detected.

🤖 Generated with Claude Code

mculp and others added 4 commits June 6, 2026 21:27
`gem "fal-ai"` makes Bundler auto-require "fal-ai", and the README tells users
to `require "fal-ai"`, but the gem only shipped lib/fal.rb — so the documented
and Bundler-default entrypoint raised LoadError. Add lib/fal-ai.rb (matching the
gem name) delegating to the implementation under "fal".

Remove lib/fal/ai.rb: it was added on the mistaken premise that Bundler requires
"fal/ai", and implies a Fal::AI namespace that does not exist.

Add a loading spec that exercises each public require entrypoint in a pristine
subprocess (spec_helper already loads "fal", so an in-process require proves
nothing).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- Replace ruby.yml with ci.yml: matrix 3.2/3.3/3.4/4.0, run on every pull
  request (not just PRs targeting main, so stacked PRs get CI), add
  fail-fast: false and in-progress cancellation.
- Bump required_ruby_version to ">= 3.2" and .ruby-version to 3.4.2.
- Gitignore + untrack Gemfile.lock (standard for libraries; CI resolves
  against current dependency versions).
- RuboCop: TargetRubyVersion 3.2; keep named block forwarding (self-
  documenting public API) and exclude the dash-named lib/fal-ai.rb from
  Naming/FileName. Extract Queue#submit validation to satisfy MethodLength
  and DRY the integration spec JSON into helpers.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Issue forms (bug/feature) + config, pull request template, Dependabot for
bundler and github-actions, CODEOWNERS, and a release workflow that publishes
to RubyGems via trusted publishing (OIDC) on version tags.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CI surfaced two issues with the broad matrix:

- Ruby 4.0: the loading spec shelled out with `2>&1`, and Ruby 4.0 + bundler
  2.6.2 prints harmless Gem::Platform constant warnings to stderr, polluting
  the captured output. Capture stdout/stderr separately with Open3 and assert
  on stdout only.
- Ruby 3.2: RuboCop now pulls parallel 2.1.0, which requires Ruby >= 3.3, so a
  no-lockfile install fails on 3.2. Drop 3.2 from the matrix and set the floor
  to >= 3.3 (still covers 3.3, 3.4, and 4.0). Only claim what we test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@greptile-apps greptile-apps Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

@coderabbitai

coderabbitai Bot commented Jun 7, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ee34e708-11d3-44a3-b479-c4a9e67da296

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch feat/queue-parity

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Per CodeRabbit/zizmor security review:
- Pin actions/checkout, ruby/setup-ruby, and rubygems/release-gem to commit
  SHAs (Dependabot is configured to keep them current).
- Set persist-credentials: false on checkout so the job leaves no git auth
  token behind.
- Disable bundler-cache on the privileged release job (cache-poisoning blast
  radius) and bundle install explicitly there instead.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@mculp mculp force-pushed the feat/queue-parity branch from cc72acb to f8dc95d Compare June 7, 2026 04:03
@greptile-apps

greptile-apps Bot commented Jun 7, 2026

Copy link
Copy Markdown

Greptile Summary

This PR brings the Ruby fal client's queue API to parity with the official JS/Python/PHP clients by introducing EndpointId — a value object that encapsulates the owner/alias vs. owner/alias/variant split — and rebuilding the queue endpoints around it.

  • EndpointId centralises the rule that fal-ai/flux/schnell submits on the full path but is polled under fal-ai/flux, eliminating all manual string splitting elsewhere; namespaced ids (comfy/, workflows/) are handled as a special case keeping three segments.
  • Queue#status/#result signatures changed from a raw URL to (app_id, request_id), and a new Queue#cancel method is added (PUT, returns true/false/raises); Connection gains a #put verb alongside a refactored single-dispatch perform private method.
  • SubmitResponse is extended with app_id and cancel_url; subscribe and status gain logs: and submit/subscribe gain webhook_url: keyword options.

Confidence Score: 5/5

Safe to merge — the changes are well-scoped, the URL construction logic is correct end-to-end as proven by the WebMock integration tests, and no data-loss or correctness regressions were found.

The EndpointId parsing logic, the per-verb Connection dispatch, and the cancel true/false/raise semantics are all exercised by the test suite at both the unit and integration level. The breaking API changes are documented in the CHANGELOG and are pre-1.0. No issues were found in the changed code beyond the points already raised in previous review threads.

spec/fal/integration_spec.rb — the multi-step polling integration test was removed (flagged in a prior thread); everything else is straightforward.

Important Files Changed

Filename Overview
lib/fal/endpoint_id.rb New value object cleanly encapsulates the submit-path vs. app-path distinction; NAMESPACES constant handles comfy/workflows correctly; coerce, equality, and hash are all implemented.
lib/fal/endpoints.rb QueueRequest base class + Status/Result/Cancel subclasses replace the old Url wrapper; CGI.escape used correctly for webhook URL encoding; Status's keyword-forwarding initializer is valid Ruby 3+.
lib/fal/queue.rb Public API migrated to (app_id, request_id) signatures; cancel correctly maps HTTP 400 → false and re-raises anything else; require_request_id guards the only truly required field.
lib/fal/connection.rb post/get/put now share a single perform+handle_response path; body_options correctly omits the body keyword when nil, avoiding spurious empty-body PUT/GET calls.
lib/fal/subscriber.rb Updated to use (app_id, request_id) from SubmitResponse; logs: keyword threaded through correctly.
spec/fal/endpoint_id_spec.rb New spec covers to_s, app derivation (no variant, one variant, deeply nested, comfy namespace, workflows+variant), coerce, and value equality.
spec/fal/integration_spec.rb cancel and webhook tests added; subscribe multi-step polling test removed (replaced by single-COMPLETED stub); response_url assertion dropped from submit test.
spec/fal/queue_spec.rb All queue methods updated to new signatures; cancel covered for success, 400, and other errors; webhook and missing-request-id paths tested.
spec/fal/endpoints_spec.rb All endpoint classes covered including webhook URL encoding and logs query parameter; reorganised as nested describes.

Sequence Diagram

sequenceDiagram
    participant C as Client
    participant Q as Queue
    participant EI as EndpointId
    participant Conn as Connection
    participant API as fal API

    Note over C,API: submit
    C->>Q: submit("fal-ai/flux/schnell", input, webhook_url:)
    Q->>EI: coerce("fal-ai/flux/schnell")
    Q->>Conn: post(Submit endpoint, body: input)
    Note right of Conn: POST queue.fal.run/fal-ai/flux/schnell[?fal_webhook=...]
    Conn->>API: HTTP POST
    API-->>Conn: "{request_id, status_url, cancel_url, ...}"
    Conn-->>Q: Response
    Q-->>C: SubmitResponse(app_id, request_id, ...)

    Note over C,API: Subscriber polling loop
    loop until completed or timeout
        C->>Q: status("fal-ai/flux/schnell", request_id, logs:)
        Q->>EI: coerce → .app → "fal-ai/flux"
        Q->>Conn: get(Status endpoint)
        Note right of Conn: GET queue.fal.run/fal-ai/flux/requests/{id}/status[?logs=1]
        Conn->>API: HTTP GET
        API-->>Conn: "{status: IN_QUEUE|IN_PROGRESS|COMPLETED}"
        Conn-->>Q: Response
        Q-->>C: Status object (yield to block)
    end

    C->>Q: result("fal-ai/flux/schnell", request_id)
    Q->>EI: coerce → .app → "fal-ai/flux"
    Q->>Conn: get(Result endpoint)
    Note right of Conn: GET queue.fal.run/fal-ai/flux/requests/{id}
    Conn->>API: HTTP GET
    API-->>C: result data

    Note over C,API: cancel (optional)
    C->>Q: cancel("fal-ai/flux/schnell", request_id)
    Q->>EI: coerce → .app → "fal-ai/flux"
    Q->>Conn: put(Cancel endpoint)
    Note right of Conn: PUT queue.fal.run/fal-ai/flux/requests/{id}/cancel
    Conn->>API: HTTP PUT
    alt 2xx
        API-->>C: true
    else HTTP 400
        API-->>C: false
    else other error
        API-->>C: raises ApiError
    end
Loading

Reviews (2): Last reviewed commit: "Guard require_request_id with an explici..." | Re-trigger Greptile

Comment thread spec/fal/integration_spec.rb
Comment thread lib/fal/queue.rb
Comment thread lib/fal/endpoint_id.rb
mculp and others added 4 commits June 6, 2026 23:10
Adding /Gemfile.lock to .gitignore does not untrack an already-committed file.
git rm --cached it so the lockfile is no longer version-controlled, as the
gitignore entry and this PR intend.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Match the queue ergonomics of the official fal clients (JS/Python/PHP):

- Add EndpointId, a value object that parses "fal-ai/flux/schnell" into the run
  path (the full id) and the queue application path (owner/alias, "fal-ai/flux").
  Namespaced ids (workflows/, comfy/) keep three segments.
- Rebuild Endpoints around it: Run/Submit address the full id; Status/Result/
  Cancel are built from the app root so nested ids resolve to the correct
  /{app}/requests/{id}/... path. Submit appends ?fal_webhook when given a URL.
- Queue#status and #result now take (app_id, request_id) instead of a raw URL;
  add Queue#cancel (true on accept, false on HTTP 400, raise otherwise) and a
  logs: option on #status.
- SubmitResponse now carries app_id and cancel_url; Subscriber polls via
  (app_id, request_id) and forwards logs:.
- Add Connection#put and collapse post/get/put onto one verb-dispatching path.

The existing nested-model integration tests double as a correctness proof: they
stub the server's app-rooted URLs, so a green suite means EndpointId builds the
same URLs the gateway returns.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`|| raise` also fires on a false value; request_id is always a string, so this
is behavior-preserving, but nil? states the intent.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@mculp mculp force-pushed the feat/queue-parity branch from f8dc95d to d7ec2fd Compare June 7, 2026 04:11
Base automatically changed from chore/foundation to main June 7, 2026 10:03
mculp added a commit that referenced this pull request Jun 7, 2026
Review feedback (PR #4):
- Re-add a WebMock-backed test that subscribe yields IN_QUEUE -> IN_PROGRESS ->
  COMPLETED in order, restoring end-to-end coverage of the polling loop.
- Note that SubmitResponse status/response/cancel URLs are informational (the
  gem builds its own request URLs) and may be nil.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@mculp

mculp commented Jun 7, 2026

Copy link
Copy Markdown
Owner Author

Consolidating the remaining stack into #5 (now targeting main).

After #3 merged, this PR can no longer stand alone correctly:

So the queue-parity work + its required fixes ship together in #5 as one clean, CI-green PR with the final code (no transient broken state on main). All three review threads here are addressed in #5. Branches are intact, so happy to re-split if you prefer separate reviews.

@mculp mculp closed this Jun 7, 2026
mculp added a commit that referenced this pull request Jun 8, 2026
Review feedback (PR #4):
- Re-add a WebMock-backed test that subscribe yields IN_QUEUE -> IN_PROGRESS ->
  COMPLETED in order, restoring end-to-end coverage of the polling loop.
- Note that SubmitResponse status/response/cancel URLs are informational (the
  gem builds its own request URLs) and may be nil.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant