feat: add informative REST results for TAKE failures (SOFIE-295)

[anteeek]

About the Contributor

This PR is made on behalf of the BBC

Type of Contribution

This is a:

Feature

Current Behavior

The TAKE endpoint returns either 200 or 500, without any additional information

New Behavior

HTTP Rest codes 400, 425 and 429 are added as possible failure reasons, and a UTC timestamp for next possible take is added

Testing

• I have added one or more unit tests for this PR
• I have updated the relevant unit tests
• No unit test changes are needed for this PR

Affected areas

the TAKE endpoint

Time Frame

Not urgent, but we would like to get this merged into the in-development release.

Other Information

Status

• PR is ready to be reviewed.
• The functionality has been tested by the author.
• Relevant unit tests has been added / updated.
• Relevant documentation (code comments, system documentation) has been added / updated.

This PR adds informative REST results for TAKE failures by extending the TAKE endpoint to return additional HTTP status codes, include timing information in responses, and propagate additional error details to clients.

Changes

New Data Types

• Introduced TakeNextPartResult in packages/corelib/src/worker/studio.ts with nextTakeTime: number to communicate the next available TAKE time.

Enhanced Error Handling & Return Values

• handleTakeNextPart in packages/job-worker/src/playout/take.ts now returns Promise<TakeNextPartResult> and supplies nextTakeTime on success.
• Error handling augmented to include additional contextual information and HTTP-like status semantics:
  • HTTP 425 used for blocked TAKE conditions (transition, adlib, too-close-to-autonext); error payloads include nextAllowedTakeTime.
  • HTTP 429 used for rate-limited TAKE requests; error payloads include nextAllowedTakeTime.
• Several Take-related error branches (TakeBlockedDuration, TakeDuringTransition, TakeCloseToAutonext, rate-limit) now attach nextAllowedTakeTime in their error objects.

API Surface Updates

• Return types updated to expose TakeNextPartResult:
  • PlaylistsServerAPI.take in meteor/server/api/rest/v1/playlists.ts: ClientAPI.ClientResponse<TakeNextPartResult>.
  • PlaylistsRestAPI.take in meteor/server/lib/rest/v1/playlists.ts: Promise<ClientAPI.ClientResponse<TakeNextPartResult>>.
  • NewUserActionAPI.take in packages/meteor-lib/src/api/userActions.ts: Promise<ClientAPI.ClientResponse<TakeNextPartResult>>.
• ClientAPI.ClientResponseError in packages/meteor-lib/src/api/client.ts now includes an optional additionalInfo?: Record<string, unknown>, and responseError populates this from user error args when present.
• Server-side REST error propagation (meteor/server/api/rest/v1/index.ts) now accepts and forwards additionalInfo from client errors into HTTP responses.

OpenAPI Documentation

• packages/openapi/api/definitions/playlists.yaml updated:
  • Explicit 200 response schema for TAKE including nextTakeTime.
  • New 425 and 429 response definitions including additionalInfo (e.g. nextAllowedTakeTime).

Tests

• Added tests in packages/meteor-lib/src/api/__tests__/client.test.ts to verify extraction and propagation of additionalInfo (including nextAllowedTakeTime) when present, and absence when not provided.

Impact

Clients calling the TAKE endpoint now receive:

• Clearer failure reasons (blocked vs. rate-limited) via distinct status codes.
• A UTC timestamp (nextTakeTime / nextAllowedTakeTime in additionalInfo) indicating when the next TAKE is allowed, enabling smarter client retry behavior.

[coderabbitai]

Walkthrough

Adds a TakeNextPartResult (with nextTakeTime) returned by take operations, propagates it through job-worker, API and OpenAPI layers, and surfaces optional additionalInfo (including nextAllowedTakeTime) on error responses; tests updated accordingly.

Changes

Cohort / File(s)	Summary
Core types packages/corelib/src/worker/studio.ts	Adds TakeNextPartResult { nextTakeTime: number } and updates StudioJobs.TakeNextPart signature to return it.
Job worker (take logic) packages/job-worker/src/playout/take.ts	handleTakeNextPart now returns TakeNextPartResult with nextTakeTime; error paths include nextAllowedTakeTime in payloads and use 429/425 status codes.
Server REST & API plumbing meteor/server/api/rest/v1/playlists.ts, meteor/server/lib/rest/v1/playlists.ts, meteor/server/api/rest/v1/index.ts	Propagates return type change for take to TakeNextPartResult; APIRequestError now has optional additionalInfo and server propagates response.additionalInfo into error responses.
Client API / User actions packages/meteor-lib/src/api/client.ts, packages/meteor-lib/src/api/userActions.ts	ClientResponseError gains optional additionalInfo?: Record<string, unknown>; take action return type updated to ClientResponse<TakeNextPartResult>.
OpenAPI packages/openapi/api/definitions/playlists.yaml	Updates take operation: explicit 200 response with nextTakeTime; adds 425 and 429 error responses with additionalInfo fields.
Tests packages/meteor-lib/src/api/__tests__/client.test.ts	Adds tests asserting additionalInfo extraction (including nextAllowedTakeTime) and absence when not provided.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant REST as REST API (Playlists)
  participant Server as PlaylistsServerAPI
  participant Jobs as StudioJobs
  participant Worker as JobWorker (handleTakeNextPart)

  Client->>REST: POST /playlists/:id/take
  REST->>Server: invoke take(...)
  Server->>Jobs: StudioJobs.TakeNextPart(...)
  Jobs->>Worker: schedule/execute take handling
  Worker-->>Jobs: TakeNextPartResult { nextTakeTime }
  Jobs-->>Server: return TakeNextPartResult
  Server-->>REST: respond 200 with TakeNextPartResult
  REST-->>Client: 200 { nextTakeTime } / or error with additionalInfo

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested labels

Contribution

Suggested reviewers

• Julusian
• jstarpl

Poem

🐰 I hopped in code to add a clue,
A tiny time for next to do,
When takes are close or limits near,
I whisper info, crisp and clear,
Next hop is timed — a rabbit cheer! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: adding informative REST results for TAKE failures, with a reference to the ticket number SOFIE-295.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments

packages/openapi/api/definitions/playlists.yaml (1)

591-607: Add required properties to the 200 response schema.

The 200 response schema for take defines status and result (with nested nextTakeTime) but doesn't declare any of them as required. This weakens the API contract for code generators and consumers. Other endpoints in this file (e.g., moveNextPart line 260-266) similarly omit required, but since this is a new schema, it's a good opportunity to be explicit.

Suggested fix

              schema:
                type: object
                properties:
                  status:
                    type: number
                    example: 200
                  result:
                    type: object
                    properties:
                      nextTakeTime:
                        type: number
                        description: Unix timestamp (ms) of when the next take will be allowed.
                        example: 1707024000000
+                    required:
+                      - nextTakeTime
+                required:
+                  - status
+                  - result

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[anteeek]

Note: this PR was first created with wrong target in bbc#64

[codecov]

Codecov Report

❌ Patch coverage is 35.84906% with 34 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
packages/job-worker/src/playout/take.ts	31.42%	24 Missing ⚠️
meteor/server/api/rest/v1/index.ts	0.00%	5 Missing ⚠️
meteor/server/api/rest/v1/playlists.ts	0.00%	3 Missing ⚠️
meteor/server/lib/rest/v1/playlists.ts	0.00%	2 Missing ⚠️

📢 Thoughts on this report? Let us know!

[Julusian]

I feel like the openapi definitions should be updated to reflect this addition?

I don't see how the time is being reported for non-200 status codes. Unless the nextAllowedTakeTime inside the UserError is being reported somehow?

[anteeek]

@Julusian addressed the comment, thanks! Please take a look when you can

[Julusian]

I'm not keen on this.

If its going to be a concept that we have defined in the client api, then it should be a proper concept in the UserError too.

But I'm not sure if it should be a dedicated value in ClientResponseError either.
I don't have any suggestions on how to do this better though, it just feels smelly to have this value specific to one UserAction be present in what is otherwise very general code

[Julusian]

I suppose I wouldnt mind if on UserError has an explicit property for nextAllowedOperationTime, and that was on ClientResponseError too. (or something named more around being a rate limit)

Then at least it is more generic and could be used by other things too (even though it wont for now, but maybe there should be more cases where we 'rate limit')

In the api it can continue to call it nextAllowedTakeTime, I am just worried about the specific naming of something for a single UserAction on internal types (if every action did that, these types would become a confusing mess

[anteeek]

@Julusian made it more generic, take a look please

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@packages/openapi/api/definitions/playlists.yaml`:
- Around line 623-656: Replace the non-standard 425 response object with a 409
response: rename the top-level key 425 to 409, change its description to
indicate a Conflict (take blocked due to transition/adlib), and update the
example status value to 409; for both the 409 (formerly 425) and 429 response
schemas add a required: [status, message, nextAllowedTakeTime] array and ensure
the properties status, message and nextAllowedTakeTime remain defined (and keep
their examples/descriptions).

🧹 Nitpick comments (2)

packages/meteor-lib/src/api/client.ts (1)

53-54: Extracting a domain-specific field from untyped error args is pragmatic but fragile.

The nextAllowedTakeTime is extracted from userError.userMessage.args via a cast to number | undefined. Since args is essentially Record<string, any>, there's no compile-time guarantee that the value is actually a number. This works because the callers in take.ts always pass a number, but it could silently propagate incorrect types if a caller passes something else.

Consider adding a runtime guard (e.g., typeof ... === 'number') to be safe:

Suggested defensive check

-		const nextAllowedTakeTime = userError.userMessage.args?.nextAllowedTakeTime as number | undefined
+		const rawNextAllowedTakeTime = userError.userMessage.args?.nextAllowedTakeTime
+		const nextAllowedTakeTime = typeof rawNextAllowedTakeTime === 'number' ? rawNextAllowedTakeTime : undefined

Also applies to: 64-69

packages/job-worker/src/playout/take.ts (1)

29-29: Duplicate import from the same module.

TakeNextPartResult (line 43) is imported separately from TakeNextPartProps (line 29), both from @sofie-automation/corelib/dist/worker/studio. Consolidate into a single import statement.

Suggested fix

At line 29, merge the imports:

-import { TakeNextPartProps } from '@sofie-automation/corelib/dist/worker/studio'
+import { TakeNextPartProps, TakeNextPartResult } from '@sofie-automation/corelib/dist/worker/studio'

Then remove lines 43-44.

Also applies to: 43-44