docs: capture state-controller durability in STYLE_GUIDE.md - #4857
Conversation
|
@coderabbitai full_review, thanks! |
Summary by CodeRabbit
WalkthroughThe change adds style-guide guidance for state-controller operations that cross database and external-system boundaries. It defines recovery points, transaction boundaries, and controls for repeated or overlapping external work. ChangesState Controller Guidance
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🐇 ✅ Action performedFull review finished. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4857.docs.buildwithfern.com/infra-controller |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/architecture/state_handling.md (2)
40-53: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the scheduling contract executable for operators.
This section states a 30-second default, immediate
Transitionrescheduling, and a stale-claim interval longer than the handling deadline. It does not identify the configuration sources, units, deadline semantics, or takeover condition. Verify these claims against the scheduler implementation and document the exact operational contract. Also verify the state-history link and anchor.As per path instructions: “Review documentation for technical correctness, operator usability, broken links, stale command/API references, spelling, grammar, and consistency with current deployment and CLI behavior.”
#!/bin/sh set -eu rg -n -C 8 \ 'Transition|Enqueuer|enqueue|stale|claim|deadline|Duration|30[ _-]?seconds' \ crates target="docs/playbooks/stuck_objects/diagnostic_tools.md" test -f "$target" rg -n -i -C 2 'query-state-history|query state history' "$target"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture/state_handling.md` around lines 40 - 53, Update the scheduling and claim paragraphs using the scheduler and configuration symbols as the source of truth: document the actual configuration keys, duration units, default interval, handling-deadline semantics, immediate Transition behavior, stale-claim takeover condition, and processor ownership rules. Verify and correct the “Query state history” link and anchor against the current diagnostic tools document and CLI behavior, removing unsupported claims.Source: Path instructions
17-23: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winComplete the
StateHandlerandStateControllerIOcontracts.This section names two interfaces but defines only their broad roles. Add a concise contract table from
crates/state-controller/src/state_handler.rsandcrates/state-controller/src/io.rs. Include inputs, outputs, missing or ineligible resource behavior, stale-write results, error behavior, transaction boundaries, and side effects. The supplied implementations show thatload_object_statecan returnNoneandpersist_controller_statereturnsbool; this page does not define how callers interpret those outcomes.As per coding guidelines: “Document interface contracts completely, including spelling, requiredness, defaults, accepted values, units, bounds, interactions, ordering, fallback behavior, outputs, side effects, errors, and unsupported paths.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/architecture/state_handling.md` around lines 17 - 23, Expand the StateHandler and StateControllerIO section with a concise contract table based on the symbols in state_handler.rs and io.rs. Document inputs and outputs, including load_object_state returning None and persist_controller_state returning bool, plus missing or ineligible resources, stale-write outcomes, errors, transaction boundaries, ordering, and side effects. Define how callers interpret each outcome and cover unsupported paths without changing implementation behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@docs/architecture/state_handling.md`:
- Around line 40-53: Update the scheduling and claim paragraphs using the
scheduler and configuration symbols as the source of truth: document the actual
configuration keys, duration units, default interval, handling-deadline
semantics, immediate Transition behavior, stale-claim takeover condition, and
processor ownership rules. Verify and correct the “Query state history” link and
anchor against the current diagnostic tools document and CLI behavior, removing
unsupported claims.
- Around line 17-23: Expand the StateHandler and StateControllerIO section with
a concise contract table based on the symbols in state_handler.rs and io.rs.
Document inputs and outputs, including load_object_state returning None and
persist_controller_state returning bool, plus missing or ineligible resources,
stale-write outcomes, errors, transaction boundaries, ordering, and side
effects. Define how callers interpret each outcome and cover unsupported paths
without changing implementation behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 779a95ea-dbc7-447e-b2bf-11ecbaa1d342
📒 Files selected for processing (2)
STYLE_GUIDE.mddocs/architecture/state_handling.md
|
@coderabbitai full_review, thanks! |
|
🐇 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/architecture/state_handling.md`:
- Around line 107-110: Clarify the external-effects recovery guidance around
WorkLock::fence_transaction: lock-free reconciliation must remain read-only,
while any mutating reconciliation requires independent external fencing or
idempotency. Before retrying protected database writes, reacquire the WorkLock
and call fence_transaction again in the same short transaction.
- Around line 41-43: Expand the StateControllerConfig documentation to cover
processor_dispatch_interval (default 2s) and max_concurrency (default 10),
alongside the existing duration settings. Document duration syntax and accepted
bounds, require max_concurrency > 0, and state that processor_dispatch_interval
should remain below iteration_time; clarify that zero removes dispatch delay but
increases database polling at the same rate.
- Around line 17-23: Expand the architecture documentation around StateHandler
and StateControllerIO to describe their observable contracts: Wait, DoNothing,
and Deleted semantics; object listing/loading; version-checked state writes;
conditional history writes; outcome persistence; metric naming;
manual-intervention reporting; SLA resolution; and error handling. Explicitly
state that Deleted does not persist an outcome, and anchor each behavior to the
relevant interface or method symbols from the referenced state-controller
modules.
- Around line 83-90: Expand the crash matrix in the architecture state-handling
documentation to distinguish in-call stops, known external failures, and unknown
responses, specifying persisted retry/error state for known failures and
rediscovery or verification for unknown outcomes. Add a recovery test checklist
covering poll, skip, maintenance, error transitions, and persisted resume state,
then run rumdl and inspect the rendered table and checklist.
- Around line 64-68: Define an explicit, persisted ownership handoff marker or
version for lifecycle state, and enforce it on every state write, including Site
Explorer’s ManagedHostState::DpuDiscoveringState updates and
MachineStateControllerIO. Make the handoff atomic, reject stale or non-owner
writes after ownership transfers, and document deterministic conflict resolution
across concurrent and restarted discovery.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5ed5185d-80b6-423e-a336-5ffb1f9fe281
📒 Files selected for processing (1)
docs/architecture/state_handling.md
chet
changed the title
This keeps the state-controller half of the durability guidance close to the existing work-lock contract while leaving the reader-facing architecture update in the documentation review path. The rule calls out the durable recovery point, transaction boundary, and fencing, idempotency, or reconciliation required when external work can repeat or overlap. It links to the existing state-handling page without depending on the companion docs PR landing first. This supports NVIDIA#4623 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
🐇 ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@STYLE_GUIDE.md`:
- Around line 542-544: Update the transaction/external-I/O guidance in the
“controller-model background” section to define ordering: persist the recovery
point in a short database transaction before any external I/O, then mark the
work complete only after external convergence or reconciliation confirms the
desired state. Preserve the existing fencing, idempotency, and reconciliation
guidance.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 54cb91c8-ecb4-4a10-8984-99f890f1be8e
📒 Files selected for processing (1)
STYLE_GUIDE.md
5 participants
This continues the style-guide series from #4608, #4648, #4739, #4641, and #4783.
This is the
STYLE_GUIDE.mdhalf of the state-controller durability guidance from #4623.Transactions cannot make external side effects atomic. State-controller work that crosses database and external-system boundaries needs a durable recovery point, must avoid holding database transactions open across external I/O, and needs fencing, idempotency, or reconciliation when work can repeat or overlap.
The short rule has its own scan point beside the existing long-running work-lock contract and links to the repository's current state-handling page for controller-model background. The detailed
docs/architecture/state_handling.mdrefresh is in #4867 so it can move through the technical-writer queue separately.Related issues
This supports #4623
Documentation companion: #4867
Type of Change
Breaking Changes
Testing
Additional Notes
Validated with
cargo make format-nightly,cargo make clippy, the cached Carbide-lints workflow,rumdl,git diff --check, and a Pandoc render. No tests were run because this is a documentation-only change.