Repository navigation
feat: record requests that return no invocation result - #424
Open
mnajafian-nv wants to merge 94 commits into
Open
mnajafian-nv wants to merge 94 commits into
mnajafian-nv wants to merge 94 commits into
Conversation
Core already writes adapter-invocation.json before each request, so a killed run shows which invocation started. Nothing on disk says how long it ran, how it ended, or why it stopped, and RunResult exists only in memory. Add an opt-in, append-only run-record.jsonl per invocation under <artifact root>/.fabric/<runtime_id>/<invocation_id>/. runtime.record.enabled turns it on; an artifact root alone does not. Planning rejects an enabled record without an artifact root, with a non-local environment provider, or with an adapter that is not a process or python adapter. Core writes a start line right before it sends the request to the host and an end line after the invocation returns or raises, then syncs the file once after the host lock is released. Both lines are built from typed, allowlisted fields: other context keys appear by name only, override_keys lists override names without values, and the end line keeps the error code, stage and retryable flag but never the message or adapter metadata. A failed write adds a record_write_failed event and never fails the run. The record artifact gets a unique name, as adapter and Relay artifacts do. RecordConfig derives Default (recording off), so a caller can turn recording on without spelling out every record field. The record module is crate-private; the line format is published as the v1alpha1 run-record-line schema. The RFC 3339 formatter lives in a crate-private time module so other core features can share it. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
The recorder promises that a failed record write never replaces the invocation result, but only a failed create was tested. Add per-thread test hooks to the record writer that delay record creation or fail the start line, the end line or the sync, and use them to check that each failure keeps the result's status, output, error, usage and identity, adds one record_write_failed event, and leaves a raised invocation error unchanged. The hooks compile only in tests. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Core set the adapter's deadline and then created the record and wrote its start line, while the host timeout starts only when the request is sent. Slow record storage therefore shortened the adapter's budget without shortening the host timeout, so recording could turn a successful invocation into a timeout. Create the record and write the start line before core sets the deadline and writes adapter-invocation.json. The start line records the timeout, not the deadline, so the deadline the adapter receives and the one in adapter-invocation.json stay the same value. A failed adapter-invocation.json write now happens after the record starts, so its end line reads artifact_write_failed. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
The start line cut model role names and request context key names to 256 characters, and both are map keys. Two names that shared their first 255 characters became the same key, so one entry silently replaced the other. Config validation does not bound either name, so keep both names whole and cut only values. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
The record docs said recording never changes the run and that the file is never created through a symbolic link. Both overstated the contract: record I/O runs on the invocation path, so slow storage delays the request and the return, and create_new refuses only a file or symbolic link at the record path itself. State that record I/O errors do not replace the invocation result, that recording is synchronous and best-effort with no time limit, that the record is created before the adapter deadline is set, and that creation refuses an existing file or symbolic link at run-record.jsonl. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
A run record's end line keeps the error code but no cause. A reader has to know core's and each adapter's codes to separate infrastructure failures from the agent's own, and an out-of-memory kill leaves no trace. The end line gains `stop`: a cause from Fabric's own closed list (fabric_failed, sandbox_failed, agent_exhausted_budget, unclassified, agent_failed), `reason: timeout` with the invoke timeout that applied, the time used and whether Fabric force-stopped the host, and the host's exit code or signal when it exited. Following the adapter contract, only a failed result reads agent_failed; every raised error other than a timeout or a host exit reads fabric_failed, except that an error from the exchange with the host during an out-of-memory rise reads sandbox_failed, because a kill can make the adapter raise. Both lines record the container's cgroup v2 oom_kill count when Fabric owns its cgroup. A rise sets a cause only with runtime.record.exclusive_container, which the start line records; set it only when nothing else runs in the container during an invocation, including other runtimes in the same process. The host's exit status is read only when a record is on. An ignored Linux test drives a real out-of-memory kill of the adapter host; it runs only in a memory-limited cgroup v2 container. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
InvocationRecorder::stop never sets fabric_failed together with host_exited, and a timeout and a host exit come from one error code, so they are never both true either. Two rule tests used those states, which taught the rule from cases that cannot happen. The second test now checks the real boundary: an out-of-memory rise outranks a timeout. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
terminate_local_host waits 2 seconds for the host to exit on its own before it kills it, so a host that exits in that window records its own status. On Windows the Job Object stop gives exit code 1 and no signal. State both instead of saying exit_signal is always the signal Fabric sent. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
finish read the container's oom_kill count after core had released the host lock and processed the response. In that window another caller on the same runtime can start the next exchange, and its out-of-memory kill would enter this record's rise and could set sandbox_failed on the wrong invocation. Read the count under the host lock as soon as exchange_lifecycle_message returns, before a timed-out host is stopped, and carry it to finish in an ExchangeEnd value together with the exchange's elapsed time and, after a failure, the host's wait status. ExchangeEnd replaces exchange_failed and is set on success too. After a host_crashed error the count is read again once the exit is observed and the larger value is kept, because the kernel counts a kill after it sends SIGKILL. File writes and the sync stay outside the lock. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
The adapter deadline code counts as a timeout only in a failed result or a raised error, so a cancelled result that carries error code timeout has no stop cause: cancellation takes precedence. Add a contract test from a real local host for that case and state the exception in the stop rules. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
The Windows exit code 1 and the Unix signal 9 appear only when Fabric terminates the host; a host that exits by itself during the 2-second grace period records its own exit status while forced stays true. The fabric_failed and agent_failed mapping is guidance for a Harbor consumer of the record; nothing in Fabric reads the record for Harbor. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📜 Recent review details
WalkthroughThis pull request adds shared usage accounting, configurable model pricing, opt-in run records, request evidence, and SDK reporting. It adds adapter accounting support and extends Relay gateway startup readiness to a 30-second budget. ChangesUsage accounting and pricing
Run records and reporting
Supporting updates
Priority: ➖ Normal Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Adapter
participant UsageReducer
participant FabricCore
participant RecordWriter
participant PythonReporting
Adapter->>UsageReducer: provide invocation usage observations
UsageReducer->>FabricCore: return accounting extension
FabricCore->>FabricCore: normalize usage and apply eligible catalog rates
FabricCore->>RecordWriter: write invocation and request evidence
FabricCore->>RecordWriter: persist finalized result
PythonReporting->>RecordWriter: read saved evidence
PythonReporting->>PythonReporting: validate and summarize accounting
Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 40.79% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 505 functions across 56 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title "feat: record requests that return no invocation result" uses an allowed Conventional Commits type (feat), is lowercase, has no trailing period, and is 54 characters. It describes the main c… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Description check The description covers the main intent: it saves a separate request record around one-shot execution, explains the start and end records, and states the failure behavior and reader limits. It also inc… Full details: Title check
-
Explanation
The title "feat: record requests that return no invocation result" uses an allowed Conventional Commits type (feat), is lowercase, has no trailing period, and is 54 characters. It describes the main change: request-level records for startup or invocation failures that return no result. The PR objectives confirm this focus, though the summary also covers writes before startup, which the title does not mention.
Full details: Description check
-
Explanation
The description covers the main intent: it saves a separate request record around one-shot execution, explains the start and end records, and states the failure behavior and reader limits. It also includes Overview, Details, Validation, and Where should the reviewer start sections. It is missing a 'Related Issues' value: the author wrote 'Relates to: none', which does not match the template's required 'Closes / Fixes / Resolves / Relates to' with an issue number. The template's 'Closes #' item is not filled in, and the author did not fill the 'Where should the reviewer start' section with a clear reviewer path beyond file names. The description also mentions a dependency on
#423, which is outside the template but useful context. Overall the description is mostly complete, so it passes, with a note on the related-issue field.
-
✨ Finishing Touches: 💡 1
-
🧪 Generate unit tests (beta)
-
- Create a new PR
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
2 tasks done
When the invocation raises, the run record cannot add an event to a result, and the raised error passes through unchanged. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
|
Fern docs preview: https://nvidia-preview-pull-request-424.docs.buildwithfern.com/nemo/fabric |
A returned result can still gain the run_record artifact, so list the fields that stay the same instead of saying nothing else changes. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
…sult The result extension check raises InvalidAdapterExtension for a value that breaks the declared schema; the end line recorded it as fabric_error, so one fault had two codes. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
1 task done
Contributor
Author
|
/nvskills-ci |
Bring in the explicit environment lifecycle SDK, the OpenShell environment provider, and multi-runtime sandboxes. The config and runtime test modules keep both the run record tests and main's environment tests, and SchemaName::ALL grows to 23 entries for EnvironmentReference and RunRecordLine. The run record contract is unchanged. Main resolves a local environment's artifact root as before, falling back to runtime.artifacts, and a non-local provider, OpenShell included, still fails planning when the record is on. start_runtime_in() starts a local runtime through the same local adapter host, so its invocations write the record the same way. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Main now ships OpenShell as a real environment provider whose task paths pass config validation. Check that an OpenShell environment with valid task paths still fails planning when the run record is on, next to the existing docker case. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Regenerate the Rust library reference with Rust 1.94.0 so it lists main's environment lifecycle items next to the run record items, with updated page positions. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Bring in the latest run record branch, which now includes upstream main with the explicit environment lifecycle SDK, the OpenShell environment provider, and multi-runtime sandboxes. The test that maps rejected result extensions to invalid_agent_run_result now passes the container out-of-memory count and the stop cause that error_end_line takes on this branch. The OpenShell provider test sets exclusive_container to false like the other record config tests. The runtime test module keeps the stop cause tests and main's environment tests. The stop cause contract is unchanged: the record still requires the local provider, and the cgroup count and host exit status still come from the local adapter host. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Bring in Relay OpenTelemetry file sinks, OpenShell SDK runtimes in task sandboxes, the turnkey OpenShell echo example, and the OpenShell sandbox name limit fix. The crate root declares both main's task_command module and this branch's time module. An enabled record still requires the local provider: config validation rejects it on the OpenShell provider, and start_runtime_in() checks that the environment handle's provider matches the plan, so main's OpenShell runtime path never runs with a record enabled. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Bring in the latest run record branch, which now includes upstream main with Relay OpenTelemetry file sinks, OpenShell SDK runtimes in task sandboxes, the turnkey OpenShell echo example, and the OpenShell sandbox name limit fix. The merge applied without conflicts. Stop causes are still written only by the local adapter host path. Main's OpenShell runtime path never runs with a record enabled, because config validation rejects a record on the OpenShell provider. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Bring in the Claude adapter's per-invocation cost and cache token usage. No conflicts. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Use the system Bash path so macOS tests exercise Bash 3.2 even when PATH contains a newer Bash. This keeps the empty-array regression meaningful in CI. Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
Signed-off-by: mnajafian-nv <mnajafian@nvidia.com>
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
An invocation record cannot explain every request that returns no result. Runtime startup can fail before an invocation exists, leaving the caller without runtime or invocation IDs.
Save a separate request record around one-shot execution. It shows that a request was registered and whether execution returned a result or raised at startup or invocation.
Dependencies: #423 supplies invocation recording, full-result publication and inspection. The diff against
mainincludes unmerged prerequisites.Details
Write
request-start.jsonbefore one-shot runtime startup andrequest-end.jsonafterward under.fabric/requests/<dispatch_id>/. Preserve the caller's request ID without inventing inner runtime or invocation IDs. Configuration resolution happens before this recording boundary and is not covered.The end record distinguishes a returned result reference from no result. Plan validation and environment preparation failures are recorded at the startup boundary; SDK errors keep their more specific
planorpreparestage. Existing result status and error fields remain the invocation's stop evidence.python -m nemo_fabric.reporting <request-record.json>explains what the record establishes. A start record alone does not prove execution finished. A result reference does not prove the result file was saved, and the reader does not open that reference automatically. No result never means zero usage or spend. An outer timeout does not establish that inner or remote work stopped.Request files use exclusive publication and a 64 KiB limit. Recording failures preserve the original execution error or become diagnostics on a returned result. SDK exceptions retain the recording root for investigation. The full result remains authoritative for the overall outcome, including shutdown.
Validation
Regression tests cover startup and preparation failures without invented invocation IDs, request-file size limits, independent recording failures, and result references that do not prove a result was saved. Reader tests keep a start-only request distinct from completed execution.
Where should the reviewer start?
Start with
RequestRecorderincrates/fabric-core/src/record/results.rsand its use inrun_plan. Then review the no-result and request-reference cases intests/python/test_invocation_reporting.py.Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)
Relates to: none
I confirm this contribution is my own work, or I have the right to submit it under this project's license.
I searched existing issues and open pull requests, and this does not duplicate existing work.