Fix queued snapshot status handler #1963
Conversation
🦋 Changeset detectedLatest commit: 1c8f8a8 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExampleTask
participant ResourceMonitor
User->>ExampleTask: Trigger resourceMonitorTest
ExampleTask->>ResourceMonitor: Create instance (dirName, processName)
ExampleTask->>ResourceMonitor: startMonitoring(1000ms)
ExampleTask->>ResourceMonitor: logResourceSnapshot("initial")
Note right of ResourceMonitor: Collects disk, memory, process metrics
ExampleTask->>ExampleTask: Wait 5 seconds
ExampleTask->>ResourceMonitor: logResourceSnapshot("after 5s")
ExampleTask->>ResourceMonitor: stopMonitoring()
ExampleTask-->>User: Return message
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
references/hello-world/src/resourceMonitor.ts (2)
99-110: Possible overlapping calls when the snapshot takes longer than the interval.
setIntervalkeeps firing even iflogResourceSnapshotis still running, causing race conditions & CPU spikes.
ConsidersetTimeoutrecursion or a running‑flag.-this.logInterval = setInterval(this.logResources.bind(this), intervalMs); +const tick = async () => { + await this.logResources(); + this.logInterval = setTimeout(tick, intervalMs); +}; +tick();
266-269:continueinside thecatchis redundant.Control already flows to the next iteration; removing it silences the linter warning.
-} catch { - // Ignore errors reading individual process info - continue; -} +} catch { + // Ignore errors reading individual process info +}🧰 Tools
🪛 Biome (1.9.4)
[error] 268-268: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/cli-v3/src/entryPoints/managed/execution.ts(2 hunks)packages/core/src/v3/runtime/managedRuntimeManager.ts(3 hunks)references/hello-world/src/resourceMonitor.ts(1 hunks)references/hello-world/src/trigger/example.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
references/hello-world/src/resourceMonitor.ts
[error] 268-268: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
packages/core/src/v3/runtime/managedRuntimeManager.ts (4)
30-33: Added regular status logging to help debug stuck executions.This is a valuable improvement for observability. Periodic logging of the runtime status will help identify and debug stuck executions by providing insights into the internal state every 5 minutes.
179-179: Enhanced error logging with detailed context.Including the full runtime status in the error log when a waitId is missing provides significantly more context for debugging, making troubleshooting easier.
186-186: Improved error logging with runtime state context.Enriching the log with the current status when a resolver is missing provides better diagnostic information for tracking down issues with waitpoints and resolvers.
229-234: Well-structured status getter implementation.The new private getter efficiently encapsulates the internal state of the runtime manager, providing a consistent view of active waitIds and waitpoints that's used throughout the logging improvements.
packages/cli-v3/src/entryPoints/managed/execution.ts (2)
272-278: Fixed handling of requeued runs.This change properly handles the "QUEUED" status by suspending the run instead of failing it. The implementation correctly follows the pattern established for the "FINISHED" case by killing the process without marking the run as failed, allowing the system to naturally wait for another run.
412-412: Removed QUEUED from invalid status cases.This change is a necessary companion to the new QUEUED status handler above. Since QUEUED is now properly handled with its own case, it's correctly removed from the invalid status changes section.
references/hello-world/src/resourceMonitor.ts (1)
284-292:machine.memorymay be0, leading to divide‑by‑zero innodeMemoryPercent.Add a minimum value or fallback to
totalMemory.-const machineMemoryBytes = this.ctx.machine - ? this.ctx.machine.memory * 1024 * 1024 * 1024 - : totalMemory; +const machineMemoryBytes = + this.ctx.machine?.memory && this.ctx.machine.memory > 0 + ? this.ctx.machine.memory * 1024 * 1024 * 1024 + : totalMemory;references/hello-world/src/trigger/example.ts (1)
3-3: Import uses a.jsextension in a TypeScript file.If the project compiles with
moduleResolution: node(classic), this may break. Prefer path without extension (or.ts) and let the bundler resolve it.-import { ResourceMonitor } from "../resourceMonitor.js"; +import { ResourceMonitor } from "../resourceMonitor";
Runs can be requeued, e.g. when runners fail to come up fast enough. We didn't handle this very well previously - the runs would fail. We now handle this more gracefully by exiting the current execution without failing the run, and waiting for another run as usual.
This PR also adds a resource monitor helper to the references and improves runtime manager debug logs in case of resume failures.
Summary by CodeRabbit
New Features
Improvements