Visitar URL original
fix(logs): close the remaining double logs found in review · simstudioai/sim@1c503e3 · GitHub
Skip to content

Commit 1c503e3

Browse files
waleedlatif1waleedlatif1
committed
fix(logs): close the remaining double logs found in review
Carries failure marks through the scrubbed Pi error and normalizes a non-Error node failure before logging so the engine sees its mark; drops the condition handler's and response-size handler's own error lines in favor of the owning boundary; logs MCP failures once and marks their output; passes the block id to the API and Function tool calls so the tool line carries it; attributes custom tool parameter validation and condition expression errors to the author; checks the whole cause chain for a retryable setup failure before honoring a mark; and restores the sandbox failure logs, whose adapters cannot yet tell a provider exception from a non-zero exit.
1 parent 2e58162 commit 1c503e3

9 files changed

Lines changed: 131 additions & 110 deletions

File tree

‎apps/sim/executor/execution/engine.ts‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -482,15 +482,17 @@ export class ExecutionEngine {
482482
/**
483483
* Block failures were logged by the block executor. This catches a completion-handling
484484
* fault, which only this frame sees when a concurrent failure already won `executionError`.
485+
* Normalized first, as `trackExecution` would, so `run()` sees the mark on the same object.
485486
*/
486-
logFailureOnce(this.execLogger, 'Node execution failed', error, {
487+
const failure = toError(error)
488+
logFailureOnce(this.execLogger, 'Node execution failed', failure, {
487489
metadata: () =>
488-
projectResolvedSecretDiagnosticError(error, this.context.resolvedSecretTraceRegistry, {
490+
projectResolvedSecretDiagnosticError(failure, this.context.resolvedSecretTraceRegistry, {
489491
nodeId,
490492
}),
491493
executionId: this.context.executionId,
492494
})
493-
throw error
495+
throw failure
494496
}
495497
}
496498

‎apps/sim/executor/handlers/api/api-handler.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,6 +72,7 @@ export class ApiBlockHandler implements BlockHandler {
7272
isDeployedContext: ctx.isDeployedContext,
7373
enforceCredentialAccess: ctx.enforceCredentialAccess,
7474
callChain: ctx.callChain,
75+
blockId: block.id,
7576
},
7677
},
7778
{ executionContext: ctx }

‎apps/sim/executor/handlers/condition/condition-handler.ts‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { createLogger } from '@sim/logger'
2-
import { getErrorMessage, toError } from '@sim/utils/errors'
3-
import { adoptToolFailure } from '@/lib/core/errors/failure-log'
2+
import { getErrorMessage } from '@sim/utils/errors'
3+
import { adoptToolFailure, markFailureKind } from '@/lib/core/errors/failure-log'
44
import { normalizeStringRecord, normalizeWorkflowVariables } from '@/lib/core/utils/records'
55
import {
66
isNonRetryableExecutionError,
@@ -508,8 +508,11 @@ export class ConditionBlockHandler implements BlockHandler {
508508
case 'no-match':
509509
return null
510510
case 'expression-threw':
511-
logger.error('Failed to evaluate condition', { conditionCount: conditions.length })
512-
throw conditionError(conditions[evaluation.index], evaluation.message)
511+
/** The author's expression threw; the block executor logs it once. */
512+
throw markFailureKind(
513+
conditionError(conditions[evaluation.index], evaluation.message),
514+
'user'
515+
)
513516
case 'no-verdict':
514517
if (!evaluation.retryable) {
515518
throw new NonRetryableExecutionError(
@@ -523,7 +526,6 @@ export class ConditionBlockHandler implements BlockHandler {
523526
// failure as it stands. The whole list was one call, so no single
524527
// branch owns that failure; name the first, where evaluation started.
525528
if (evaluation.timedOut || ctx.abortSignal?.aborted) {
526-
logger.error('Failed to evaluate conditions', { conditionCount: conditions.length })
527529
throw conditionError(conditions[0], evaluation.message)
528530
}
529531
logger.warn('Batched condition evaluation produced no verdict, retrying one at a time', {
@@ -549,7 +551,6 @@ export class ConditionBlockHandler implements BlockHandler {
549551
)
550552
if (conditionMet) return condition
551553
} catch (error) {
552-
logger.error('Failed to evaluate condition', { errorName: toError(error).name })
553554
throw conditionError(
554555
condition,
555556
getErrorMessage(error, 'Condition evaluation failed'),

‎apps/sim/executor/handlers/function/function-handler.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,7 @@ export class FunctionBlockHandler implements BlockHandler {
107107
userId: ctx.userId,
108108
isDeployedContext: ctx.isDeployedContext,
109109
enforceCredentialAccess: ctx.enforceCredentialAccess,
110+
blockId: block.id,
110111
},
111112
}
112113

‎apps/sim/executor/handlers/pi/core/redaction.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { getErrorMessage } from '@sim/utils/errors'
2+
import { inheritFailureMarks } from '@/lib/core/errors/failure-log'
23
import type { PiEvent } from '@/executor/handlers/pi/core/events'
34

45
/**
@@ -36,11 +37,14 @@ export function getScrubbedPiErrorMessage(
3637
return scrubPiSecrets(getErrorMessage(error, fallback), secrets)
3738
}
3839

39-
/** Creates a boundary-safe error without retaining a potentially secret-bearing cause. */
40+
/**
41+
* Creates a boundary-safe error without retaining a potentially secret-bearing cause. The failure
42+
* marks still cross, so a GitHub tool failure the tool layer logged is not logged again.
43+
*/
4044
export function createScrubbedPiError(
4145
error: unknown,
4246
secrets: readonly string[],
4347
fallback?: string
4448
): Error {
45-
return new Error(getScrubbedPiErrorMessage(error, secrets, fallback))
49+
return inheritFailureMarks(new Error(getScrubbedPiErrorMessage(error, secrets, fallback)), error)
4650
}

‎apps/sim/lib/core/errors/failure-log.test.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,13 @@ describe('classifyFailure', () => {
2626
expect(classifyFailure(wrapped)).toBe('internal')
2727
})
2828

29+
it('keeps a retryable setup failure internal even beneath a marked wrapper', () => {
30+
const setup = new RetryableSetupError('setup')
31+
expect(classifyFailure(markFailureKind(new Error('wrapped', { cause: setup }), 'user'))).toBe(
32+
'internal'
33+
)
34+
})
35+
2936
it('keeps a retryable setup failure internal even when its cause was the author’s', () => {
3037
const cause = markFailureKind(new Error('missing field'), 'user')
3138
expect(classifyFailure(new RetryableSetupError('setup', { cause }))).toBe('internal')

‎apps/sim/lib/core/errors/failure-log.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -63,10 +63,10 @@ export function markFailureKind<T>(error: T, kind: FailureKind): T {
6363
*/
6464
export function classifyFailure(error: unknown): FailureKind {
6565
if (findDatabaseQueryError(error)) return 'internal'
66+
const chain = causeChain(error)
67+
if (chain.some(isRetryableSetupError)) return 'internal'
6668

67-
for (const link of causeChain(error)) {
68-
if (isRetryableSetupError(link)) return 'internal'
69-
69+
for (const link of chain) {
7070
const marked = failureKinds.get(link)
7171
if (marked) return marked
7272
if (link instanceof UserFailure) return 'user'

‎apps/sim/lib/execution/remote-sandbox/index.ts‎

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -958,14 +958,10 @@ async function executeInSandboxWithinBudget(
958958

959959
if (execution.error) {
960960
const errorMessage = `${execution.error.name}: ${execution.error.value}`
961-
/** The author's code raising is logged once by the tool boundary; a provider failure is ours. */
962-
if (execution.providerFailure) {
963-
logger.error('Sandbox execution failed', {
964-
sandboxId,
965-
hasTraceback: Boolean(execution.error.traceback),
966-
providerFailure: execution.providerFailure,
967-
})
968-
}
961+
logger.error('Sandbox execution failed', {
962+
sandboxId,
963+
hasTraceback: Boolean(execution.error.traceback),
964+
})
969965
const executionResult = {
970966
result: null,
971967
stdout: execution.error.traceback || errorMessage,
@@ -1157,14 +1153,10 @@ async function executeShellInSandboxWithinBudget(
11571153
// back to stdout for the real command output before the generic message.
11581154
const errorMessage =
11591155
result.stderr || result.stdout || `Process exited with code ${result.exitCode}`
1160-
/** A non-zero exit is logged once by the tool boundary; a provider failure is ours. */
1161-
if (result.providerFailure) {
1162-
logger.error('Sandbox shell execution error', {
1163-
sandboxId,
1164-
exitCode: result.exitCode,
1165-
providerFailure: result.providerFailure,
1166-
})
1167-
}
1156+
logger.error('Sandbox shell execution error', {
1157+
sandboxId,
1158+
exitCode: result.exitCode,
1159+
})
11681160
const executionResult = {
11691161
result: null,
11701162
stdout,

0 commit comments

Comments
 (0)