Copilot commented on code in PR #8586:
URL: https://github.com/apache/texera/pull/8586#discussion_r4049809160
##########
amber/src/main/scala/org/apache/texera/web/resource/SyncExecutionResource.scala:
##########
@@ -169,7 +174,7 @@ class SyncExecutionResource extends LazyLogging {
),
emailNotificationEnabled = false,
computingUnitId = computingUnitId,
- warehouseId = None
+ warehouseId = request.warehouseId
Review Comment:
This value is validated only inside `initExecutionService`, but this
endpoint calls `shutdownPreviousExecution` at line 160 first. A missing or
unowned warehouse therefore shuts down the currently running execution before
the new request is refused, despite the new validate-before-touch ordering in
`WorkflowService`. Validate/resolve the warehouse before that shutdown (and
avoid a second lookup by passing the resolved result into initialization).
##########
amber/src/main/scala/org/apache/texera/web/service/WorkflowService.scala:
##########
@@ -219,21 +230,24 @@ class WorkflowService(
sessionUri: URI
): Unit = {
- if (executionService.hasValue) {
- executionService.getValue.unsubscribeAll()
- }
-
val (uidOpt, userEmailOpt) = userOpt.map(user => (user.getUid,
user.getEmail)).unzip
+ // Validate before touching the execution already in flight: a request
that is
+ // going to be refused must not take the running one's subscriptions with
it.
// uid is NOT NULL in the DB; fail early here rather than letting the
insert fail downstream.
val uid = uidOpt.getOrElse(
throw new IllegalArgumentException(
"Cannot start execution: a user id (uid) is required but none was
provided."
)
)
+ val warehouseName =
WorkflowService.resolveLakekeeperWarehouseName(req.warehouseId, uid)
+
+ if (executionService.hasValue) {
+ executionService.getValue.unsubscribeAll()
+ }
Review Comment:
This intentional ordering change leaves `WorkflowServiceSpec.scala:598-623`
stale: that test still asserts that a missing user ID detaches the previous
execution, so its post-failure state update now emits an event and `events
shouldBe empty` fails. Update the regression test to assert the new contract
that rejected requests preserve the existing subscriptions.
##########
agent-service/src/server.ts:
##########
@@ -492,6 +496,13 @@ export function buildApp() {
wsLog.info({ agentId, preview: msg.content.substring(0, 50) },
"received command");
+ // The prompt carries the workspace's current warehouse pick, so a
run
+ // uses what the user has selected now rather than whatever was
+ // selected when the agent was created (#7751).
+ if (typeof msg.warehouseId === "number") {
+ agent.setDelegateWarehouse(msg.warehouseId);
+ }
Review Comment:
An omitted `warehouseId` never clears the agent's previous value. If the
picker is cleared (for example after the feature is disabled or the selected
warehouse disappears), the frontend omits this optional field, this guard
leaves the old ID in `delegateConfig`, and the next sync request still sends
it; flag-off requests are then rejected as explicit picks instead of using
shared storage. Treat an absent field as the current `undefined` selection as
well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
