feat: accept traffic before sidecars are ready on cold start#171
Merged
Conversation
Remove the Cloud Run container-dependencies gate so ksef-hub starts serving requests as soon as Phoenix is healthy (~5s), rather than waiting for all three sidecars (~8-14s). Sidecars continue starting in parallel. Graceful degradation at each call site: - Upload LiveView: silently warms extractor + classifier on mount (ServiceHealth.warm_upload_sidecars/0) so idle file-browsing time is used productively before the user submits. - Extractor client: retries up to 3 times on :econnrefused with a configurable delay (0ms in tests, 1s in prod) to cover the narrow window where the upload fires before the sidecar is listening. - Invoice detail: HTML preview generated asynchronously via Task.Supervisor so mount never blocks on pdf-renderer startup; falls back to PDF iframe while the preview loads. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughRemoves Cloud Run container-dependencies annotation; adds retry-on-connection-failure with configurable delay for invoice extraction; introduces a public warm_upload_sidecars/0 used by Upload LiveView to asynchronously warm sidecars; moves LiveView preview generation to asynchronous tasks; tests and test config updated accordingly. Changes
Sequence DiagramssequenceDiagram
participant Client as InvoiceExtractor.Client
participant Retry as RetryLoop
participant API as Remote API
Client->>Retry: extract(invoice, retries=@max_retries)
Retry->>API: Req.post(request)
API-->>Retry: error (:econnrefused)
Retry->>Retry: log warning, sleep(delay_ms)
Retry->>API: Req.post(retry)
API-->>Retry: 200 OK + JSON
Retry-->>Client: {:ok, parsed_result}
sequenceDiagram
participant LiveView as Show LiveView
participant Supervisor as TaskSupervisor
participant Generator as PreviewGenerator
participant Mailbox as LiveViewMailbox
LiveView->>LiveView: mount - init preview_ref=nil, loading=true
LiveView->>Supervisor: start_async(generate_preview(invoice))
Supervisor-->>LiveView: monitor_ref (store preview_ref)
Generator->>Generator: render HTML
Generator-->>Mailbox: {ref, html_preview}
LiveView->>LiveView: handle_info({ref, html}) - assign html_preview, loading=false
sequenceDiagram
participant LiveView as Upload LiveView
participant Supervisor as TaskSupervisor
participant Health as ServiceHealth
participant Sidecars as Extractor/Classifier Sidecars
LiveView->>LiveView: mount (connected)
LiveView->>Supervisor: start_async(warm_upload_sidecars())
Supervisor->>Health: run parallel health checks
Health->>Sidecars: health request
Sidecars-->>Health: {:ok, status}
Health-->>Supervisor: results
Supervisor-->>LiveView: task completes (non-blocking)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cloud-run/service.yaml (1)
12-16:⚠️ Potential issue | 🔴 CriticalCritical: YAML indentation is broken.
Line 16
run.googleapis.com/startup-cpu-boost: "true"is not properly indented underannotations. It should be aligned with the other annotation keys (lines 13-15) to be valid YAML.🐛 Proposed fix
annotations: autoscaling.knative.dev/minScale: "0" autoscaling.knative.dev/maxScale: "2" run.googleapis.com/execution-environment: gen2 -run.googleapis.com/startup-cpu-boost: "true" + run.googleapis.com/startup-cpu-boost: "true"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cloud-run/service.yaml` around lines 12 - 16, The YAML has a mis-indented annotation key: move run.googleapis.com/startup-cpu-boost under the existing annotations mapping so it lines up with autoscaling.knative.dev/minScale, autoscaling.knative.dev/maxScale and run.googleapis.com/execution-environment; specifically edit the annotations block (the mapping containing the keys autoscaling.knative.dev/minScale, autoscaling.knative.dev/maxScale, run.googleapis.com/execution-environment) and add or realign run.googleapis.com/startup-cpu-boost: "true" as another key in that same mapping.
🧹 Nitpick comments (2)
lib/ksef_hub/invoice_extractor/client.ex (1)
55-63: Minor: Type spec could usepos_integer()instead ofnon_neg_integer().Since retries is decremented and the base case is
when retries > 0, the function is never called with 0. However,non_neg_integer()is still technically correct since 0 is handled by falling through to error cases.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub/invoice_extractor/client.ex` around lines 55 - 63, Update the `@spec` for do_extract to use pos_integer() for the retries parameter instead of non_neg_integer(); locate the do_extract/6 spec declaration and replace the sixth argument type from non_neg_integer() to pos_integer() so the contract reflects that retries must be > 0.lib/ksef_hub_web/live/invoice_live/upload.ex (1)
24-26: Consider using a supervised task for crash isolation.
Task.start/1spawns an unsupervised process. If the warmup task crashes, it could produce confusing error logs without clear origin. UsingTask.Supervisor.start_child/2with the existingKsefHub.TaskSupervisorwould provide better crash isolation while still being fire-and-forget.♻️ Proposed refactor
if connected?(socket) do - Task.start(fn -> ServiceHealth.warm_upload_sidecars() end) + Task.Supervisor.start_child(KsefHub.TaskSupervisor, fn -> + ServiceHealth.warm_upload_sidecars() + end) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/invoice_live/upload.ex` around lines 24 - 26, Replace the unsupervised Task.start call in the connected? branch with a supervised fire-and-forget child via the existing Task.Supervisor; specifically, instead of calling Task.start(fn -> ServiceHealth.warm_upload_sidecars() end) when connected?(socket) is true, invoke Task.Supervisor.start_child(KsefHub.TaskSupervisor, fn -> ServiceHealth.warm_upload_sidecars() end) so the warm_upload_sidecars/0 work runs under supervision (use KsefHub.TaskSupervisor and ServiceHealth.warm_upload_sidecars to locate the change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ksef_hub_web/live/invoice_live/show.ex`:
- Around line 109-110: The current assigns html_preview and preview_ref conflate
"no preview" and "preview loading"; add a separate boolean assign (e.g.,
preview_loading) and initialize it (false) alongside html_preview and
preview_ref, update start_preview_task/2 to set preview_loading = true when it
actually starts the async job and to no-op (leave preview_loading false) if
there is no XML to render, and adjust render branches that check html_preview to
instead use preview_loading to show a loading state and treat html_preview ==
nil as truly absent; apply the same change to the other locations noted (the
other assign blocks and render logic around lines ~132-148 and ~863-870) so
tasks aren’t started for XML-less invoices and the UI shows loading vs
unavailable distinctly.
In `@test/ksef_hub_web/live/invoice_live/show_test.exs`:
- Around line 79-80: The test asserts render(view) immediately after calling
live(conn, ...), but the async preview task may not have finished; update the
test to wait for preview completion before asserting: either assert_receive the
specific completion message your LiveView sends (e.g., {:preview_complete,
^invoice.id} or similar) after calling live(conn, ...) and only then run assert
render(view) =~ "preview", or implement a short polling loop that repeatedly
calls render(view) (with a small sleep) until the HTML contains "preview" (fail
after a timeout). Ensure you reference the existing live(conn, ...) and
render(view) usage and wait for the LiveView's preview completion message or
successful render before asserting.
---
Outside diff comments:
In `@cloud-run/service.yaml`:
- Around line 12-16: The YAML has a mis-indented annotation key: move
run.googleapis.com/startup-cpu-boost under the existing annotations mapping so
it lines up with autoscaling.knative.dev/minScale,
autoscaling.knative.dev/maxScale and run.googleapis.com/execution-environment;
specifically edit the annotations block (the mapping containing the keys
autoscaling.knative.dev/minScale, autoscaling.knative.dev/maxScale,
run.googleapis.com/execution-environment) and add or realign
run.googleapis.com/startup-cpu-boost: "true" as another key in that same
mapping.
---
Nitpick comments:
In `@lib/ksef_hub_web/live/invoice_live/upload.ex`:
- Around line 24-26: Replace the unsupervised Task.start call in the connected?
branch with a supervised fire-and-forget child via the existing Task.Supervisor;
specifically, instead of calling Task.start(fn ->
ServiceHealth.warm_upload_sidecars() end) when connected?(socket) is true,
invoke Task.Supervisor.start_child(KsefHub.TaskSupervisor, fn ->
ServiceHealth.warm_upload_sidecars() end) so the warm_upload_sidecars/0 work
runs under supervision (use KsefHub.TaskSupervisor and
ServiceHealth.warm_upload_sidecars to locate the change).
In `@lib/ksef_hub/invoice_extractor/client.ex`:
- Around line 55-63: Update the `@spec` for do_extract to use pos_integer() for
the retries parameter instead of non_neg_integer(); locate the do_extract/6 spec
declaration and replace the sixth argument type from non_neg_integer() to
pos_integer() so the contract reflects that retries must be > 0.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dea5e74e-3295-4426-bb75-1ba7a2d3c875
📒 Files selected for processing (9)
cloud-run/service.yamlconfig/test.exslib/ksef_hub/invoice_extractor/client.exlib/ksef_hub/service_health.exlib/ksef_hub_web/live/invoice_live/show.exlib/ksef_hub_web/live/invoice_live/upload.extest/ksef_hub/invoice_extractor/client_test.exstest/ksef_hub_web/live/invoice_live/show_test.exstest/ksef_hub_web/live/invoice_live/upload_test.exs
Comment on lines
+79
to
+80
| {:ok, view, _html} = live(conn, ~p"/c/#{company.id}/invoices/#{invoice.id}") | ||
| assert render(view) =~ "preview" |
There was a problem hiding this comment.
Synchronize this assertion with the async preview task.
live/2 returns before the {ref, html} message is necessarily handled, so an immediate render(view) can still observe the initial state and fail intermittently. Please wait for preview completion explicitly before asserting on the rendered HTML.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/ksef_hub_web/live/invoice_live/show_test.exs` around lines 79 - 80, The
test asserts render(view) immediately after calling live(conn, ...), but the
async preview task may not have finished; update the test to wait for preview
completion before asserting: either assert_receive the specific completion
message your LiveView sends (e.g., {:preview_complete, ^invoice.id} or similar)
after calling live(conn, ...) and only then run assert render(view) =~
"preview", or implement a short polling loop that repeatedly calls render(view)
(with a small sleep) until the HTML contains "preview" (fail after a timeout).
Ensure you reference the existing live(conn, ...) and render(view) usage and
wait for the LiveView's preview completion message or successful render before
asserting.
… task - Add preview_loading assign to distinguish "loading" from "no preview" in invoice show; skip async task for invoices without XML content - Fix mis-indented startup-cpu-boost annotation in service.yaml - Replace unsupervised Task.start with Task.Supervisor.start_child for sidecar warm-up in upload LiveView - Add timer.sleep in async preview test to avoid race condition Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/ksef_hub_web/live/invoice_live/upload.ex (1)
24-28: Handlestart_child/2failure explicitly to avoid silent warm-up skips.The async approach is good, but ignoring the return tuple hides failures to schedule warm-up.
Suggested patch
def mount(_params, _session, socket) do if connected?(socket) do - Task.Supervisor.start_child(KsefHub.TaskSupervisor, fn -> - ServiceHealth.warm_upload_sidecars() - end) + case Task.Supervisor.start_child(KsefHub.TaskSupervisor, fn -> + ServiceHealth.warm_upload_sidecars() + end) do + {:ok, _pid} -> + :ok + + {:error, reason} -> + Logger.warning("Failed to start sidecar warm-up task: #{inspect(reason)}") + end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/ksef_hub_web/live/invoice_live/upload.ex` around lines 24 - 28, The current code calls Task.Supervisor.start_child(KsefHub.TaskSupervisor, fn -> ServiceHealth.warm_upload_sidecars() end) inside the connected?(socket) check and ignores its return tuple, so failures to schedule the warm-up are silent; change this to capture and handle the result (pattern-match the {:ok, pid} and {:error, reason} cases), and on {:error, reason} log an error (using Logger.error or the project's logger) including the reason and context (e.g., "failed to start warm_upload_sidecars task"), optionally attempting a retry or fallback; ensure references to connected?(socket), Task.Supervisor.start_child/2 and ServiceHealth.warm_upload_sidecars() are used so the change is localized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/ksef_hub_web/live/invoice_live/upload.ex`:
- Around line 24-28: The current code calls
Task.Supervisor.start_child(KsefHub.TaskSupervisor, fn ->
ServiceHealth.warm_upload_sidecars() end) inside the connected?(socket) check
and ignores its return tuple, so failures to schedule the warm-up are silent;
change this to capture and handle the result (pattern-match the {:ok, pid} and
{:error, reason} cases), and on {:error, reason} log an error (using
Logger.error or the project's logger) including the reason and context (e.g.,
"failed to start warm_upload_sidecars task"), optionally attempting a retry or
fallback; ensure references to connected?(socket), Task.Supervisor.start_child/2
and ServiceHealth.warm_upload_sidecars() are used so the change is localized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7dee561-0d01-4abb-bd03-c1978cb69009
📒 Files selected for processing (4)
cloud-run/service.yamllib/ksef_hub_web/live/invoice_live/show.exlib/ksef_hub_web/live/invoice_live/upload.extest/ksef_hub_web/live/invoice_live/show_test.exs
💤 Files with no reviewable changes (1)
- cloud-run/service.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- test/ksef_hub_web/live/invoice_live/show_test.exs
- lib/ksef_hub_web/live/invoice_live/show.ex
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
1 participant
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.
Summary
ServiceHealth.warm_upload_sidecars/0pings extractor + classifier when the upload LiveView connects, using idle file-browsing time to get sidecars ready before the user submits.:econnrefused— up to 3 retries with 1s delay (0ms in tests) covers the narrow window where an upload fires before the sidecar is listening.generate_preview/1now runs in aTask.Supervisortask so invoice detail mount never blocks on pdf-renderer startup; existing PDF iframe fallback shows immediately.Context
Service scales to zero (min=0). Cold starts are 8–14s, dominated by the invoice-classifier loading ML models from GCS FUSE. Previously every user on a cold instance paid the full penalty even for browse/search flows that need no sidecar at all.
Test plan
mix test— 2106 tests, 0 failuresretries on econnrefused and succeeds) confirms recovery on second attemptshows preview when xml_file is available) now usesrender(view)to explicitly test async delivery🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Reliability & Performance
Tests