Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
anteeek wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions meteor/server/api/rest/v1/playlists.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import {
} from '../../../collections'
import { DBRundownPlaylist } from '@sofie-automation/corelib/dist/dataModel/RundownPlaylist'
import { ServerClientAPI } from '../../client'
import { QueueNextSegmentResult, StudioJobs } from '@sofie-automation/corelib/dist/worker/studio'
import { QueueNextSegmentResult, StudioJobs, TakeNextPartResult } from '@sofie-automation/corelib/dist/worker/studio'
import { getCurrentTime } from '../../../lib/lib'
import { TriggerReloadDataResponse } from '@sofie-automation/meteor-lib/dist/api/userActions'
import { ServerRundownAPI } from '../../rundown'
Expand Down Expand Up @@ -457,7 +457,7 @@ class PlaylistsServerAPI implements PlaylistsRestAPI {
event: string,
rundownPlaylistId: RundownPlaylistId,
fromPartInstanceId: PartInstanceId | undefined
): Promise<ClientAPI.ClientResponse<void>> {
): Promise<ClientAPI.ClientResponse<TakeNextPartResult>> {
triggerWriteAccess()
const rundownPlaylist = await RundownPlaylists.findOneAsync(rundownPlaylistId)
if (!rundownPlaylist) throw new Error(`Rundown playlist ${rundownPlaylistId} does not exist`)
Expand Down Expand Up @@ -801,7 +801,7 @@ export function registerRoutes(registerRoute: APIRegisterHook<PlaylistsRestAPI>)
}
)

registerRoute<{ playlistId: string }, { fromPartInstanceId?: string }, void>(
registerRoute<{ playlistId: string }, { fromPartInstanceId?: string }, TakeNextPartResult>(
'post',
'/playlists/:playlistId/take',
new Map([
Expand Down
4 changes: 2 additions & 2 deletions meteor/server/lib/rest/v1/playlists.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
RundownPlaylistId,
SegmentId,
} from '@sofie-automation/corelib/dist/dataModel/Ids'
import { QueueNextSegmentResult } from '@sofie-automation/corelib/dist/worker/studio'
import { QueueNextSegmentResult, TakeNextPartResult } from '@sofie-automation/corelib/dist/worker/studio'
import { Meteor } from 'meteor/meteor'

/* *************************************************************************
Expand Down Expand Up @@ -228,7 +228,7 @@ export interface PlaylistsRestAPI {
event: string,
rundownPlaylistId: RundownPlaylistId,
fromPartInstanceId: PartInstanceId | undefined
): Promise<ClientAPI.ClientResponse<void>>
): Promise<ClientAPI.ClientResponse<TakeNextPartResult>>
/**
* Clears the specified SourceLayers.
*
Expand Down
6 changes: 5 additions & 1 deletion packages/corelib/src/worker/studio.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,10 @@ export interface CleanupOrphanedExpectedPackageReferencesProps {
rundownId: RundownId
}

export interface TakeNextPartResult {
nextTakeTime: number
}

/**
* Set of valid functions, of form:
* `id: (data) => return`
Expand All @@ -404,7 +408,7 @@ export type StudioJobFunc = {
[StudioJobs.QueueNextSegment]: (data: QueueNextSegmentProps) => QueueNextSegmentResult
[StudioJobs.ExecuteAction]: (data: ExecuteActionProps) => ExecuteActionResult
[StudioJobs.ExecuteBucketAdLibOrAction]: (data: ExecuteBucketAdLibOrActionProps) => ExecuteActionResult
[StudioJobs.TakeNextPart]: (data: TakeNextPartProps) => void
[StudioJobs.TakeNextPart]: (data: TakeNextPartProps) => TakeNextPartResult
[StudioJobs.DisableNextPiece]: (data: DisableNextPieceProps) => void
[StudioJobs.RemovePlaylist]: (data: RemovePlaylistProps) => void
[StudioJobs.RegeneratePlaylist]: (data: RegeneratePlaylistProps) => void
Expand Down
43 changes: 35 additions & 8 deletions packages/job-worker/src/playout/take.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,12 @@ import { PlayoutRundownModel } from './model/PlayoutRundownModel.js'
import { convertNoteToNotification } from '../notifications/util.js'
import { PersistentPlayoutStateStore } from '../blueprints/context/services/PersistantStateStore.js'

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

/**
* Take the currently Next:ed Part (start playing it)
*/
export async function handleTakeNextPart(context: JobContext, data: TakeNextPartProps): Promise<void> {
export async function handleTakeNextPart(context: JobContext, data: TakeNextPartProps): Promise<TakeNextPartResult> {
const now = getCurrentTime()

return runJobWithPlayoutModel(
Expand Down Expand Up @@ -77,17 +79,29 @@ export async function handleTakeNextPart(context: JobContext, data: TakeNextPart
}
}
if (lastTakeTime && now - lastTakeTime < context.studio.settings.minimumTakeSpan) {
const nextTakeTime = lastTakeTime + context.studio.settings.minimumTakeSpan
logger.debug(
`Time since last take is shorter than ${context.studio.settings.minimumTakeSpan} for ${
playlist.currentPartInfo?.partInstanceId
}: ${now - lastTakeTime}`
)
throw UserError.create(UserErrorMessage.TakeRateLimit, {
duration: context.studio.settings.minimumTakeSpan,
})
throw UserError.create(
UserErrorMessage.TakeRateLimit,
{
duration: context.studio.settings.minimumTakeSpan,
nextAllowedTakeTime: nextTakeTime,
},
429
)
}

return performTakeToNextedPart(context, playoutModel, now, undefined)
const nextTakeTime = now + context.studio.settings.minimumTakeSpan

await performTakeToNextedPart(context, playoutModel, now, undefined)

return {
nextTakeTime,
}
}
)
}
Expand Down Expand Up @@ -159,7 +173,14 @@ export async function performTakeToNextedPart(
logger.debug(
`Take is blocked until ${currentPartInstance.partInstance.blockTakeUntil}. Which is in: ${remainingTime}`
)
throw UserError.create(UserErrorMessage.TakeBlockedDuration, { duration: remainingTime })
throw UserError.create(
UserErrorMessage.TakeBlockedDuration,
{
duration: remainingTime,
nextAllowedTakeTime: currentPartInstance.partInstance.blockTakeUntil,
},
425
)
}

// If there was a transition from the previous Part, then ensure that has finished before another take is permitted
Expand All @@ -171,11 +192,17 @@ export async function performTakeToNextedPart(
start &&
now < start + currentPartInstance.partInstance.part.inTransition.blockTakeDuration
) {
throw UserError.create(UserErrorMessage.TakeDuringTransition)
throw UserError.create(
UserErrorMessage.TakeDuringTransition,
{
nextAllowedTakeTime: start + currentPartInstance.partInstance.part.inTransition.blockTakeDuration,
},
425
)
}

if (currentPartInstance.isTooCloseToAutonext(true)) {
throw UserError.create(UserErrorMessage.TakeCloseToAutonext)
throw UserError.create(UserErrorMessage.TakeCloseToAutonext, undefined, 425)
}
}

Expand Down
18 changes: 18 additions & 0 deletions packages/meteor-lib/src/api/__tests__/client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,24 @@ describe('ClientAPI', () => {
})
}
})
it('Extracts nextAllowedTakeTime from error args', () => {
const error = ClientAPI.responseError(
UserError.create(
UserErrorMessage.TakeRateLimit,
{
duration: 1000,
nextAllowedTakeTime: 1234567890,
},
429
)
)
expect(error.nextAllowedTakeTime).toBe(1234567890)
expect(error.errorCode).toBe(429)
})
it('Does not include nextAllowedTakeTime when not in args', () => {
const error = ClientAPI.responseError(UserError.create(UserErrorMessage.InactiveRundown))
expect(error.nextAllowedTakeTime).toBeUndefined()
})
describe('isClientResponseSuccess', () => {
it('Correctly recognizes a responseSuccess object', () => {
const response = ClientAPI.responseSuccess(undefined)
Expand Down
9 changes: 8 additions & 1 deletion packages/meteor-lib/src/api/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ export namespace ClientAPI {
errorCode: number
/** On error, provide a human-readable error message */
error: SerializedUserError
/** For blocked TAKE operations, the next allowed take time (Unix timestamp ms) */
nextAllowedTakeTime?: number
}

/**
Expand All @@ -59,7 +61,12 @@ export namespace ClientAPI {
* @returns A `ClientResponseError` object containing the error and the resolved error code.
*/
export function responseError(userError: UserError): ClientResponseError {
return { error: UserError.serialize(userError), errorCode: userError.errorCode }
const nextAllowedTakeTime = userError.userMessage.args?.nextAllowedTakeTime as number | undefined
Copy link
Member

@Julusian Julusian Feb 4, 2026

Choose a reason for hiding this comment

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

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

Copy link
Member

@Julusian Julusian Feb 4, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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

return {
error: UserError.serialize(userError),
errorCode: userError.errorCode,
...(nextAllowedTakeTime !== undefined && { nextAllowedTakeTime }),
}
}
export interface ClientResponseSuccess<Result> {
/** On success, return success code (by default, use 200) */
Expand Down
8 changes: 6 additions & 2 deletions packages/meteor-lib/src/api/userActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@ import { BucketAdLib } from '@sofie-automation/corelib/dist/dataModel/BucketAdLi
import { AdLibActionCommon } from '@sofie-automation/corelib/dist/dataModel/AdlibAction'
import { BucketAdLibAction } from '@sofie-automation/corelib/dist/dataModel/BucketAdLibAction'
import { Time } from '@sofie-automation/blueprints-integration'
import { ExecuteActionResult, QueueNextSegmentResult } from '@sofie-automation/corelib/dist/worker/studio'
import {
ExecuteActionResult,
QueueNextSegmentResult,
TakeNextPartResult,
} from '@sofie-automation/corelib/dist/worker/studio'
import {
AdLibActionId,
BucketAdLibActionId,
Expand Down Expand Up @@ -34,7 +38,7 @@ export interface NewUserActionAPI {
eventTime: Time,
rundownPlaylistId: RundownPlaylistId,
fromPartInstanceId: PartInstanceId | null
): Promise<ClientAPI.ClientResponse<void>>
): Promise<ClientAPI.ClientResponse<TakeNextPartResult>>
setNext(
userEvent: string,
eventTime: Time,
Expand Down
51 changes: 50 additions & 1 deletion packages/openapi/api/definitions/playlists.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,22 @@ resources:
description: May be specified to ensure that multiple take requests from the same Part do not result in multiple takes.
responses:
200:
$ref: '#/components/responses/putSuccess'
description: Take was successful - returns the next allowed take time.
content:
application/json:
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
404:
$ref: '#/components/responses/playlistNotFound'
412:
Expand All @@ -579,6 +594,40 @@ resources:
message:
type: string
example: No Next point found, please set a part as Next before doing a TAKE.
425:
description: Take is blocked due to a transition or adlib action.
content:
application/json:
schema:
type: object
properties:
status:
type: number
example: 425
message:
type: string
example: Cannot take during a transition
nextAllowedTakeTime:
type: number
description: Unix timestamp (ms) of when the next take will be allowed.
example: 1707024000000
429:
description: Take rate limit exceeded - takes are happening too quickly.
content:
application/json:
schema:
type: object
properties:
status:
type: number
example: 429
message:
type: string
example: Ignoring TAKES that are too quick after eachother (1000 ms)
nextAllowedTakeTime:
type: number
description: Unix timestamp (ms) of when the next take will be allowed.
example: 1707024000000
500:
$ref: '#/components/responses/internalServerError'

Expand Down
Loading