-
Notifications
You must be signed in to change notification settings - Fork 1
[WIP] Improve error output for task failures #98
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,10 +2,12 @@ import * as core from "@actions/core"; | |||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||
| getServiceLogs, | ||||||||||||||||||||||||||
| listServices, | ||||||||||||||||||||||||||
| listServiceTasks, | ||||||||||||||||||||||||||
| type Service, | ||||||||||||||||||||||||||
| type ServiceWithMetadata, | ||||||||||||||||||||||||||
| } from "./engine.js"; | ||||||||||||||||||||||||||
| import type { Settings } from "./settings.js"; | ||||||||||||||||||||||||||
| import type { TaskInfo } from "./types.js"; | ||||||||||||||||||||||||||
| import { sleep } from "./utils.js"; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||
|
|
@@ -63,20 +65,48 @@ export async function monitorDeployment(settings: Readonly<Settings>) { | |||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||
| complete = isServiceUpdateComplete(service); | ||||||||||||||||||||||||||
| } catch (error) { | ||||||||||||||||||||||||||
| const logs = await getServiceLogs(service.ID, { since: startTime }); | ||||||||||||||||||||||||||
| const message = error instanceof Error ? error.message : String(error); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| core.error( | ||||||||||||||||||||||||||
| new Error( | ||||||||||||||||||||||||||
| `Service "${serviceIdentifier}" failed to update: ${message}`, | ||||||||||||||||||||||||||
| { cause: error }, | ||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||
| core.error(`Service Details:\n${JSON.stringify(service, null, 2)}`); | ||||||||||||||||||||||||||
| // Fetch task details to get actionable error information | ||||||||||||||||||||||||||
| let taskFailureDetails: string | undefined; | ||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||
| const tasks = await listServiceTasks(service.ID); | ||||||||||||||||||||||||||
| taskFailureDetails = await getTaskFailureDetails(tasks); | ||||||||||||||||||||||||||
| } catch (taskError) { | ||||||||||||||||||||||||||
| core.debug( | ||||||||||||||||||||||||||
| `Failed to fetch task details: ${taskError instanceof Error ? taskError.message : String(taskError)}`, | ||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Build comprehensive error message with task details | ||||||||||||||||||||||||||
| const errorMessage = taskFailureDetails | ||||||||||||||||||||||||||
| ? `Service "${serviceIdentifier}" failed to update: ${message}. ${taskFailureDetails}` | ||||||||||||||||||||||||||
| : `Service "${serviceIdentifier}" failed to update: ${message}`; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Single error annotation with actionable information | ||||||||||||||||||||||||||
| core.error(new Error(errorMessage, { cause: error })); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Fetch logs for summary | ||||||||||||||||||||||||||
| const logs = await getServiceLogs(service.ID, { since: startTime }); | ||||||||||||||||||||||||||
| core.setOutput("service-logs", logs.toString()); | ||||||||||||||||||||||||||
| core.summary.addHeading("Service Logs", 2); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Add detailed information to job summary (not as error annotation) | ||||||||||||||||||||||||||
| core.summary.addHeading("Service Update Failure Details", 2); | ||||||||||||||||||||||||||
| core.summary.addRaw( | ||||||||||||||||||||||||||
| `Before the "${serviceIdentifier}" service update failed, the following logs were generated:`, | ||||||||||||||||||||||||||
| `Service "${serviceIdentifier}" failed to update.`, | ||||||||||||||||||||||||||
| true, | ||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Add task failure details to summary if available | ||||||||||||||||||||||||||
| if (taskFailureDetails) { | ||||||||||||||||||||||||||
| core.summary.addHeading("Task Failure Reason", 3); | ||||||||||||||||||||||||||
| core.summary.addRaw(taskFailureDetails, true); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Add service logs to summary | ||||||||||||||||||||||||||
| core.summary.addHeading("Service Logs", 3); | ||||||||||||||||||||||||||
| core.summary.addRaw( | ||||||||||||||||||||||||||
| `Logs generated before the service update failed:`, | ||||||||||||||||||||||||||
| true, | ||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||
| core.summary.addTable([ | ||||||||||||||||||||||||||
|
|
@@ -99,6 +129,10 @@ export async function monitorDeployment(settings: Readonly<Settings>) { | |||||||||||||||||||||||||
| ]), | ||||||||||||||||||||||||||
| ]); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| // Add service details to summary for debugging | ||||||||||||||||||||||||||
| core.summary.addHeading("Service Details (Debug)", 3); | ||||||||||||||||||||||||||
| core.summary.addCodeBlock(JSON.stringify(service, null, 2), "json"); | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| throw error; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
@@ -240,12 +274,66 @@ function resolveFailureReason( | |||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||
| const reason = | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| paused: "Service is paused", | ||||||||||||||||||||||||||
| paused: | ||||||||||||||||||||||||||
| "Service update paused due to task failure (check task logs for details)", | ||||||||||||||||||||||||||
| rollback_started: "Service failed to update and is being rolled back", | ||||||||||||||||||||||||||
| rollback_completed: "Service failed to update and was rolled back", | ||||||||||||||||||||||||||
| rollback_paused: "Service is paused and is being rolled back", | ||||||||||||||||||||||||||
| rollback_paused: | ||||||||||||||||||||||||||
| "Service rollback paused due to task failure (check task logs for details)", | ||||||||||||||||||||||||||
| unknown: `Service update status '${state}' is unknown`, | ||||||||||||||||||||||||||
| }[state] ?? "Unknown failure reason"; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| return reason + (message ? `: ${message}` : ""); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||
| * Extract actionable error information from failed tasks | ||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||
| * This function analyzes failed tasks to provide meaningful error messages | ||||||||||||||||||||||||||
| * that help diagnose why a service update failed. It examines task states, | ||||||||||||||||||||||||||
| * error messages, and failure patterns to give actionable feedback. | ||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||
| * @param tasks Array of tasks from docker service ps | ||||||||||||||||||||||||||
| * @returns A string describing the task failure reason, or undefined if no clear failure | ||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||
| async function getTaskFailureDetails( | ||||||||||||||||||||||||||
| tasks: TaskInfo[], | ||||||||||||||||||||||||||
| ): Promise<string | undefined> { | ||||||||||||||||||||||||||
|
Comment on lines
+299
to
+301
|
||||||||||||||||||||||||||
| async function getTaskFailureDetails( | |
| tasks: TaskInfo[], | |
| ): Promise<string | undefined> { | |
| function getTaskFailureDetails( | |
| tasks: TaskInfo[], | |
| ): string | undefined { |
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filter logic may incorrectly identify non-failed tasks as failures. The condition task.DesiredState === "shutdown" will match any task being shut down, including successfully completed old tasks during a normal rolling update. This could result in misleading error messages that reference successfully completed tasks rather than the actual failed tasks.
Consider refining the filter to only include tasks where Status.State indicates an actual failure (failed, rejected) or where DesiredState is shutdown AND Status.Err is present. For example:
const failedTasks = tasks.filter(
(task) =>
task.Status.State === "failed" ||
task.Status.State === "rejected" ||
(task.DesiredState === "shutdown" && task.Status.Err)
);This ensures that shutdown tasks are only considered failures if they have an associated error message.
| // Filter to only failed or rejected tasks | |
| const failedTasks = tasks.filter( | |
| (task) => | |
| task.Status.State === "failed" || | |
| task.Status.State === "rejected" || | |
| task.DesiredState === "shutdown", | |
| // Filter to only failed or rejected tasks, or shutdown tasks with an error | |
| const failedTasks = tasks.filter( | |
| (task) => | |
| task.Status.State === "failed" || | |
| task.Status.State === "rejected" || | |
| (task.DesiredState === "shutdown" && task.Status.Err), |
Copilot
AI
Feb 13, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sort operation mutates the failedTasks array in place. While this works correctly, it's better practice to avoid mutating the filtered array. Consider using toSorted() instead of sort() to create a new sorted array without mutating the original, or make a copy before sorting.
For example:
const recentFailedTask = [...failedTasks].sort(
(a, b) =>
new Date(b.UpdatedAt).getTime() - new Date(a.UpdatedAt).getTime(),
)[0];or:
const recentFailedTask = failedTasks.toSorted(
(a, b) =>
new Date(b.UpdatedAt).getTime() - new Date(a.UpdatedAt).getTime(),
)[0];Note: toSorted() requires Node.js 20+ and TypeScript 5.2+.
| const recentFailedTask = failedTasks.sort( | |
| const recentFailedTask = failedTasks.toSorted( |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -438,6 +438,81 @@ describe("Monitoring", () => { | |
| ); | ||
| }); | ||
|
|
||
| it("should include task failure details in error message", async () => { | ||
| vi.useFakeTimers(); | ||
|
|
||
| const mockTasks = [ | ||
| { | ||
| ID: "task1abcdefghijklmnop", | ||
| ServiceID: "web_service", | ||
| NodeID: "node1", | ||
| DesiredState: "shutdown", | ||
| Labels: {}, | ||
| Status: { | ||
| State: "failed", | ||
| Err: "task: non-zero exit (1)", | ||
| Message: "started", | ||
| }, | ||
| Spec: {}, | ||
| CreatedAt: "2024-01-01T00:00:00Z", | ||
| UpdatedAt: "2024-01-01T00:00:02Z", | ||
| }, | ||
| ]; | ||
|
|
||
| const serviceHistory = [ | ||
| [ | ||
| { | ||
| ID: "web_service", | ||
| Spec: { Name: "test" }, | ||
| UpdateStatus: { State: "paused", Message: "update paused" }, | ||
| } as ServiceWithMetadata, | ||
| ], | ||
| ]; | ||
|
|
||
| vi.spyOn(engine, "listServices") | ||
| .mockResolvedValueOnce(serviceHistory[0]); | ||
| vi.spyOn(engine, "listServiceTasks").mockResolvedValueOnce(mockTasks); | ||
| vi.spyOn(engine, "getServiceLogs").mockResolvedValueOnce([]); | ||
|
|
||
| // Should include task details in error | ||
| // noinspection JSVoidFunctionReturnValueUsed | ||
| const promise = expect(monitorDeployment(settings)).rejects.toThrow(); | ||
| await vi.runAllTimersAsync(); | ||
| await promise; | ||
|
|
||
| // Verify listServiceTasks was called to fetch task details | ||
| expect(engine.listServiceTasks).toHaveBeenCalledWith("web_service"); | ||
| }); | ||
|
Comment on lines
+441
to
+485
|
||
|
|
||
| it("should handle task fetch errors gracefully", async () => { | ||
| vi.useFakeTimers(); | ||
|
|
||
| const serviceHistory = [ | ||
| [ | ||
| { | ||
| ID: "web_service", | ||
| Spec: { Name: "test" }, | ||
| UpdateStatus: { State: "paused" }, | ||
| } as ServiceWithMetadata, | ||
| ], | ||
| ]; | ||
|
|
||
| vi.spyOn(engine, "listServices") | ||
| .mockResolvedValueOnce(serviceHistory[0]); | ||
| vi.spyOn(engine, "listServiceTasks").mockRejectedValueOnce( | ||
| new Error("Failed to fetch tasks"), | ||
| ); | ||
| vi.spyOn(engine, "getServiceLogs").mockResolvedValueOnce([]); | ||
|
|
||
| // Should still throw error even if task fetch fails | ||
| // noinspection JSVoidFunctionReturnValueUsed | ||
| const promise = expect(monitorDeployment(settings)).rejects.toThrowError(); | ||
| await vi.runAllTimersAsync(); | ||
| await promise; | ||
|
|
||
| expect(engine.listServiceTasks).toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("should not monitor deployment if `monitor` is false", async () => { | ||
| vi.spyOn(engine, "listServices"); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message construction could result in double periods if the original error message already ends with a period. While this is a minor formatting issue, consider normalizing the message format.
For example:
Alternatively, you could simply use a space without a period between the messages: