[cuebot] Broken bindings and invalid SQL in by-layer dispatch frame queries (FIND_DISPATCH_FRAME_BY_LAYER_AND_PROC / FIND_DISPATCH_FRAME_BY_LAYER_AND_HOST)

[DiegoTavares]

Summary

The two non-local by-layer dispatch queries in DispatchQuery.java are broken, and have been since 2019:

1. FIND_DISPATCH_FRAME_BY_LAYER_AND_PROC references a column that does not exist (job.pk_layer), so it throws BadSqlGrammarException on any execution.
2. Both FIND_DISPATCH_FRAME_BY_LAYER_AND_PROC and FIND_DISPATCH_FRAME_BY_LAYER_AND_HOST are called from DispatcherDaoJdbc with their last positional parameters in the wrong order: the layer id is bound to the h.str_name = ? placeholder and the hostname to the l.pk_layer = ? placeholder.

Neither bug currently fires in production because the only caller (LocalDispatcher) always sets isLocalDispatch = true, which routes to the FIND_LOCAL_* variants. The non-local by-layer branch is effectively dead code. Any future caller that books a proc or host against a specific layer in remote mode will either fail immediately with a SQL error (by-proc variant) or silently receive zero frames (by-host variant, whose SQL is valid but whose empty inner join can never match).

Affected code

• cuebot/src/main/java/com/imageworks/spcue/dao/postgres/DispatchQuery.java
  • FIND_DISPATCH_FRAME_BY_LAYER_AND_PROC: invalid column reference job.pk_layer=? (should be layer.pk_layer=?)
  • Both by-layer queries: placeholder order in the limit subquery is h.str_name = ? (inside the JOIN host h ON (...)) followed by l.pk_layer = ? in the WHERE clause
• cuebot/src/main/java/com/imageworks/spcue/dao/postgres/DispatcherDaoJdbc.java
  • findNextDispatchFrames(LayerInterface, VirtualProc, int), non-local branch: binds ..., layer.getLayerId(), layer.getLayerId(), proc.hostName, limit against a query expecting ..., layerId, hostName, layerId, limit
  • findNextDispatchFrames(LayerInterface, DispatchHost, int), non-local branch: binds ..., layer.getLayerId(), layer.getLayerId(), host.getName(), limit against the same expected order

Details

Placeholder order of FIND_DISPATCH_FRAME_BY_LAYER_AND_PROC on current master, versus what the DAO binds:

#	Placeholder	Bound value
1	layer.int_cores_min <= ?	proc.coresReserved (ok)
2	layer.int_mem_min <= ?	proc.memoryReserved (ok)
3	layer.int_gpus_min <= ?	proc.gpusReserved (ok)
4	layer.int_gpu_mem_min <= ?	proc.gpuMemoryReserved (ok)
5	job.pk_layer=? (invalid column)	layer.getLayerId()
6	h.str_name = ?	layer.getLayerId() (wrong: layer id bound to hostname)
7	l.pk_layer = ?	proc.hostName (wrong: hostname bound to layer id)
8	LINENUM <= ?	limit (ok)

FIND_DISPATCH_FRAME_BY_LAYER_AND_HOST has the same tail swap (positions 7 and 8 of its 9 parameters). Its SQL is valid (layer.pk_layer=?), so it does not throw; instead the subquery's JOIN host h ON (... AND h.str_name = <layer uuid>) matches no host, the join yields zero rows, the IN clause is empty, and the query silently returns no frames.

Compare with the correctly bound by-job call sites in the same file, which pass jobId, hostName, jobId.

History

• The job.pk_layer typo dates back to the introduction of the Postgres DispatchQuery.java (commit 1c6c8ca1, "OpenCue rename for cuebot", Jan 2019, inherited from the original code drop).
• The binding swap was introduced by commit 9e4fb1cb "Add basic limits functionality" (Add basic limits functionality #414, Oct 2019). That commit restructured the tag-matching subquery in all four dispatch frame queries, moving h.str_name = ? from the end of the subquery into the JOIN host h ON (...) clause, which reversed the relative order of the hostname and id placeholders. The same commit updated the two by-job call sites in DispatcherDaoJdbc to the new order but did not update the two by-layer call sites.
• Commit c22fe127 "Add multiple GPU support" (Add multiple GPU support #760 #924, Jun 2021) re-aligned the leading GPU parameters of these call sites but carried the swapped tail over verbatim.
• All later commits touching these queries were unrelated or cosmetic (regex word boundary fix, reformats, loki column), so the swap from Add basic limits functionality #414 survives unchanged on master.

Impact

• No production impact today: LocalDispatcher.dispatchProcToJob sets proc.isLocalDispatch = true and LocalDispatcher.prepHost sets host.isLocalDispatch = true before these DAO methods run, so only the FIND_LOCAL_* queries (valid SQL, correct bindings) execute. CoreUnitDispatcher only uses the by-job variants.
• The DispatcherDao interface advertises findNextDispatchFrames(LayerInterface, ...) as generally usable. Any new caller using it in remote mode will hit the broken branch: a hard BadSqlGrammarException for the proc variant, or silent empty results for the host variant.

Test gap

All existing tests in DispatcherDaoTests that touch the by-layer variants (testFindNextDispatchFramesByHostAndLayerLocal, testFindNextDispatchFramesByProcAndLayerLocal) set isLocalDispatch = true, so the non-local queries have zero coverage. This is why a query that throws on first execution has gone unnoticed for years.

Suggested fix

1. In DispatchQuery.java, change job.pk_layer=? to layer.pk_layer=? in FIND_DISPATCH_FRAME_BY_LAYER_AND_PROC.
2. In DispatcherDaoJdbc.java, reorder the non-local by-layer bindings to (..., layerId, hostName, layerId, limit) in both findNextDispatchFrames(LayerInterface, VirtualProc, int) and findNextDispatchFrames(LayerInterface, DispatchHost, int).
3. Add non-local variants of the by-layer DAO tests to pin the placeholder order.
4. Optionally, review the trailing AND ... OR sum_running IS NULL predicate in the limit subqueries: without parentheses the OR bypasses the preceding conditions for layers without limits, which weakens the layer id filter in all of these subqueries.

Credits:

This issue was uncovered by @akhaffache