docs: pull our concurrent database guidance into STYLE_GUIDE.md - #4641
docs: pull our concurrent database guidance into STYLE_GUIDE.md#4641chet wants to merge 1 commit into
Conversation
|
✅ Action performedFull review finished. |
|
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:
Summary by CodeRabbit
Walkthrough
ChangesDatabase concurrency guidance
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 404-407: Update the `txn_held_across_await` guidance in
STYLE_GUIDE.md to replace the ambiguous phrase “a nested transaction derived
from it” with wording that explicitly says “a nested transaction derived from
that transaction,” while preserving the surrounding exception and responsibility
guidance.
- Around line 431-446: Expand the “Long-running work locks” documentation to
describe all WorkLock failure transitions: the keepalive loop retries Database,
WorkLockManagerSend, and WorkLockManagerReply errors but exits on LockLost;
release() can return dispatch, database, and reply errors; and Drop queues
release without waiting, so the lease may expire if cleanup fails. State that
successful release does not permit continuing protected work, while retaining
the existing fence_transaction and external fencing, idempotency, or
reconciliation requirements.
🪄 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: 85fbbf23-5fd6-4c2f-9d5f-4c6ff80637c2
📒 Files selected for processing (1)
STYLE_GUIDE.md
chet
changed the title
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
ᕱᕱ ✅ 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 421-426: Expand the version-checked update guidance to require one
atomic statement that writes the requested values and advances or replaces the
version token, preventing reuse of the expected version. Define zero affected
rows explicitly: map them to ConcurrentModificationError only when the statement
or transaction distinguishes stale versions from missing or soft-deleted
targets; otherwise return NotFoundError or document the chosen conflict policy.
🪄 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: 2da8202d-6295-422d-921f-a6bb3631c7b4
📒 Files selected for processing (1)
STYLE_GUIDE.md
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
ᕱᕱ ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 427-432: Clarify the no-match contract in the version-checked
predicate guidance by defining the outcome for targets that still exist but are
soft-deleted or otherwise ineligible. Assign these cases an explicit error or
not-applied result, or explicitly include them in the documented combined
missing/ineligible policy, while preserving the existing handling for proven
staleness and absence.
- Around line 444-445: Update the STYLE_GUIDE.md guidance for work-lock database
updates to explicitly require WorkLock::fence_transaction and all protected
writes to execute within the same short transaction, with the fence called
before the writes; document this ordering and transaction-scope contract
clearly.
🪄 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: 976ff8bb-bd07-4104-ba16-92e356bb4dc5
📒 Files selected for processing (1)
STYLE_GUIDE.md
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai full_review, thanks! |
|
ᕱᕱ ✅ Action performedFull review finished. |
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai full_review, thanks! |
|
ᕱᕱ ✅ Action performedFull review finished. |
|
🌿 Preview your docs: https://nvidia-preview-pull-request-4641.docs.buildwithfern.com/infra-controller |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 449-453: Update the keepalive-loop documentation to define the
caller transition after the manager reports ownership loss: stop issuing new
protected writes or external side effects, perform the documented cleanup or
reconciliation path, and reacquire the work key before resuming protected work.
Preserve the existing distinction between Drop and release() cleanup behavior
and document that cleanup errors do not preserve ownership.
- Around line 421-425: Update the concurrency guidance around `SELECT ... FOR
UPDATE` to require that lock acquisition, the dependent write, and commit occur
within the same transaction, in that order. State that the write must happen
before committing the transaction so the row lock remains held through the
update.
🪄 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: 42c78b37-9f18-4a95-b9f1-0f6f6241a8e2
📒 Files selected for processing (1)
STYLE_GUIDE.md
This is an attempt to capture the general NICo maintainer design principles and guidance around concurrent database updates for the codebase. This change is derived from the pre-OSS review corpus as a whole -- years of MRs and tens of thousands of comments and discussions. As such, it leans into the guiding design decisions and principles used to define and grow the project into the product we have today. The idea is to ensure we capture our core principles in `STYLE_GUIDE.md`. If any of those principles have changed, we should capture that too, ensuring we don't lose sight of why decisions were made as the codebase evolves with new contributors, human and agentic alike. For this change specifically, I focused on concurrent database work. The search surfaced related full-row-update, version-check, row-lock, atomic-operation, transaction-lifetime, and long-running work-lock conversations throughout the corpus and across multiple participants. The current `WorkLockManager` contract also makes the lease and fencing boundaries explicit. This pulls out the recurring parts: - Assume database updates can run concurrently. - Protect read-modify-write behavior with the narrowest invariant that proves it safe. - Keep transactions short, and treat `txn_held_across_await` as design feedback rather than something to silence. - Use `WorkLockManager` when long-running work needs cross-process lease admission, and fence the database writes performed under that lease. - Remember that a `WorkLock` is an expiring lease, not a fencing token; repeated or overlapping external effects need their own protection. - Make a zero-row version-checked update explicit instead of reporting the mutation as applied. - Test concurrent updates when the contract promises stale-write rejection. It also keeps the important exceptions that a row lock is not always the right answer and that a deliberately conditional API may return a clear not-applied result. A narrowly reviewed infrastructure boundary or test may have a real reason to allow `txn_held_across_await`, but that reason and its connection-pool cost need to remain visible next to the allowance. Task-local exclusion should use an in-process owner or mutex, and a work lock is appropriate only when old and new workers overlapping after lease expiry cannot make the operation unsafe. Atomic SQL expressions, narrow updates, constraints, and version predicates may already exclude the invalid interleaving, and a work lock does not replace them for writers that do not participate in the same work key. Again, we can always adjust this now or later. The hope is that we don't lose the reasoning behind why we made certain decisions to get us where we are now, and can continue using that reasoning to help drive future decisions. This supports NVIDIA#4624 Signed-off-by: Chet Nichols III <chetn@nvidia.com>
|
@coderabbitai full_review, thanks! |
|
ᕱᕱ
|
|
@coderabbitai full_review, thanks! |
|
ᕱᕱ
|
|
@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 443-460: Extend the WorkLockManager guidance to document all
caller-visible acquisition outcomes, including retrying WorkAlreadyLocked,
exiting on cancellation, releasing after post-acquisition cancellation,
successful completion, releasing after marker-check errors, and propagating
other acquisition errors. Also specify the required caller action for every
fence_transaction error that does not prove ownership loss, including when to
abort, retry, reconcile external work, release, or reacquire, while preserving
the existing ownership-loss behavior.
🪄 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: c30d208c-a19c-4ac9-8b51-b3d670da9cb3
📒 Files selected for processing (1)
STYLE_GUIDE.md
5 participants
This continues the series from #4522 (and related to #4522), with more to come.
This is an attempt to capture the general NICo maintainer design principles and guidance around concurrent database updates for the codebase. This PR is derived from the pre-OSS review corpus as a whole -- years of MRs and tens of thousands of comments and discussions. As such, it leans into the guiding design decisions and principles used to define and grow the project into the product we have today.
The idea is to ensure we capture our core principles in
STYLE_GUIDE.md. If any of those principles have changed, we should capture that too, ensuring we don't lose sight of why decisions were made as the codebase evolves with new contributors, human and agentic alike.For this PR specifically, I focused on concurrent database work. The corpus search surfaced related conversations across multiple people related to:
This pulls out the recurring parts:
txn_held_across_awaitas design feedback rather than something to silence.WorkLockManagerwhen long-running work needs cross-process lease admission, and fence the database writes performed under that lease.WorkLockis an expiring lease, not a fencing token; repeated or overlapping external effects need their own protection.The current
WorkLockManagercontract also makes the lease and fencing boundaries explicit.It also keeps the important exceptions that a row lock is not always the right answer and that a deliberately conditional API may return a clear not-applied result. A narrowly reviewed infrastructure boundary or test may have a real reason to allow
txn_held_across_await, but that reason and its connection-pool cost need to remain visible next to the allowance. Task-local exclusion should use an in-process owner or mutex, and a work lock is appropriate only when old and new workers overlapping after lease expiry cannot make the operation unsafe. Atomic SQL expressions, narrow updates, constraints, and version predicates may already exclude the invalid interleaving, and a work lock does not replace them for writers that do not participate in the same work key.Again, we can always adjust this now or later. The hope is that we don't lose the reasoning behind why we made certain decisions to get us where we are now, and can continue using that reasoning to help drive future decisions.
Related issues
This supports #4624
Type of Change
Breaking Changes
Testing