Fix for heartbeat race condition (acking) #2380
Conversation
… IF the deduplication key matches
|
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
packages/core/src/logger.ts (1)
145-152: Fix TS type error when accessingerror.metadata; use a proper type guard
erroris typed asError. The"metadata" in error ? error.metadatapattern doesn’t refine the type, soerror.metadata(and the same usage forstructuredError.error) will fail type-checking. Replace with a type guard to safely access the field.Apply this diff:
name: error.name, - metadata: "metadata" in error ? error.metadata : undefined, + metadata: hasMetadata(error) ? error.metadata : undefined, }; } const structuredError = args.find((arg) => arg?.error); if (structuredError && structuredError.error instanceof Error) { return { message: structuredError.error.message, stack: structuredError.error.stack, name: structuredError.error.name, - metadata: - "metadata" in structuredError.error ? structuredError.error.metadata : undefined, + metadata: hasMetadata(structuredError.error) ? structuredError.error.metadata : undefined, }; }Add this helper (anywhere in this module, e.g., above
extractStructuredErrorFromArgs):type WithMetadata = { metadata?: Record<string, unknown> }; function hasMetadata(val: unknown): val is WithMetadata { return !!val && typeof val === "object" && "metadata" in val; }Optional hardening:
- Consider sanitizing/size-limiting
error.metadatabefore logging to avoid PII/oversized logs.- Consider try/catch around
JSON.stringify(structuredLog, ...)ifmetadatacould contain non-serializable structures.Also applies to: 156-163
🧹 Nitpick comments (1)
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (1)
321-326: Nit: make the log message match the owning class/methodThe message references
RunEngine.createRunAttempt()but this isRunAttemptSystem.startRunAttempt(). Adjust for clarity and log searchability.- "RunEngine.createRunAttempt(): snapshot has changed since the attempt was created, ignoring.", + "RunAttemptSystem.startRunAttempt(): snapshot has changed since the attempt was created, ignoring.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal-packages/run-engine/src/engine/errors.ts(1 hunks)internal-packages/run-engine/src/engine/index.ts(0 hunks)internal-packages/run-engine/src/engine/systems/checkpointSystem.ts(1 hunks)internal-packages/run-engine/src/engine/systems/executionSnapshotSystem.ts(0 hunks)internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts(1 hunks)packages/core/src/logger.ts(2 hunks)
💤 Files with no reviewable changes (2)
- internal-packages/run-engine/src/engine/systems/executionSnapshotSystem.ts
- internal-packages/run-engine/src/engine/index.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
packages/core/src/logger.tsinternal-packages/run-engine/src/engine/systems/runAttemptSystem.tsinternal-packages/run-engine/src/engine/errors.tsinternal-packages/run-engine/src/engine/systems/checkpointSystem.ts
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
packages/core/src/logger.ts
🧬 Code Graph Analysis (2)
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (1)
internal-packages/run-engine/src/engine/errors.ts (1)
ServiceValidationError(59-68)
internal-packages/run-engine/src/engine/systems/checkpointSystem.ts (2)
internal-packages/run-engine/src/engine/errors.ts (1)
ServiceValidationError(59-68)internal-packages/run-engine/src/engine/statuses.ts (1)
isPendingExecuting(16-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
internal-packages/run-engine/src/engine/systems/runAttemptSystem.ts (1)
327-330: Good addition: include snapshot mismatch metadata on ServiceValidationErrorAttaching
{ snapshotId, latestSnapshotId }asmetadataaligns well with the logger’s structured error handling and will aid incident triage.internal-packages/run-engine/src/engine/systems/checkpointSystem.ts (1)
273-280: Good: include diagnostic metadata on validation errorsAdding
{ snapshotId, latestSnapshotId }and{ snapshotId, snapshotStatus }toServiceValidationErrorgreatly improves observability without altering control flow.Also applies to: 285-291
| public status?: number, | ||
| public metadata?: Record<string, unknown> | ||
| ) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Preserve Error prototype chain for reliable instanceof checks
When extending Error, set the prototype explicitly to avoid issues in some transpilation/runtime scenarios.
Apply this diff:
constructor(
message: string,
public status?: number,
public metadata?: Record<string, unknown>
) {
super(message);
this.name = "ServiceValidationError";
+ Object.setPrototypeOf(this, new.target.prototype);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public status?: number, | |
| public metadata?: Record<string, unknown> | |
| ) { | |
| constructor( | |
| message: string, | |
| public status?: number, | |
| public metadata?: Record<string, unknown> | |
| ) { | |
| super(message); | |
| this.name = "ServiceValidationError"; | |
| Object.setPrototypeOf(this, new.target.prototype); | |
| } |
🤖 Prompt for AI Agents
In internal-packages/run-engine/src/engine/errors.ts around lines 62 to 64, the
custom Error subclass does not preserve the Error prototype chain which can
break instanceof checks in some transpilation/runtime environments; after
calling super(...) in the constructor, explicitly set the prototype (e.g.
Object.setPrototypeOf(this, EngineError.prototype)) and set this.name
appropriately (and optionally use Error.captureStackTrace if available) so the
instance properly inherits from Error and instanceof checks work reliably.
We were acking the heartbeat in a couple of places which could cause problems if the run had transitioned execution status between the stall being dequeued and the code running.
We don’t need to ack it because the Redis worker deals with this exact situation if you just return – it acks ONLY if the deduplication key matches, otherwise it leaves the item there.
Also added some metadata logging to some of the ServiceValidationErrors because there are a few of these and I want to confirm they’re not real issues.