Can applyNode be split into two APIs?

[sehkone]

@sehkone,

Do you think atomicity is necessary here?

My short answer is that I don't believe the entire applyNode mutation needs to be atomic, as that could introduce unnecessary complexity and potential performance bottlenecks.

However, I do think we need to be very deliberate about which parts of the process have potential race conditions and where we should enforce atomicity. As I see it, the current applyNode flow involves a few distinct steps:

1. It updates the node's state in the database.
2. It then reads the updated state back from the database.
3. Finally, it notifies the relevant agents about the change.

There's a potential race condition between steps 1 and 2. Another request could modify the data between the write and the read, leading the function to proceed with stale data. The best place to solve this is not in the review-web application logic, but within review-database. The database-level function that updates the node should atomically perform the update and return the final state of the node in a single operation. This would eliminate the race condition and simplify the application code.

The race condition with step 3 (notifying agents) is a different kind of problem. If two applyNode calls happen in quick succession, the agents might receive notifications out of order. The most resilient way to solve this is not by locking the applyNode API, but by making the agents themselves robust to out-of-order (or even duplicate) notifications. If an agent's logic is idempotent, it won't matter what order the messages arrive in; the end state will be correct. This is generally a more scalable and robust approach for distributed systems.

Stepping back, this line of thinking brings up a more fundamental, and perhaps naïve, architectural question for me. I know the team has invested a lot of effort into designing the current "node" concept, and since I wasn't part of those early discussions, I might be missing key context.

My question is: Why are we coupling changes to an agent's configuration with changes to the node's data model in a single API call?

It seems to me that these are two distinct concerns:

1. Managing the metadata and properties of a node (e.g., its hostname, profile).
2. Managing the configuration and state of the agent running on that node.

Would it be simpler, both conceptually and in implementation, if these were two separate operations? For example, we could have an applyNode mutation that only handles the database record, and a separate applyAgentConfig mutation that handles pushing updates to the agent.

Separating these concerns might make each API call's purpose clearer and reduce the complexity of functions like applyNode. I'd be very interested to hear the team's thoughts and the original rationale behind the current design.

The above is what @msk pointed out. I appreciate his perspective, as addressing this would lead to a more robust solution in terms of data integrity and avoiding race conditions.

One thing we need to evaluate is how this change would impact the user experience. If we can strike a balance between the two—robustness and usability—that would be the best outcome.

Since it may take some time to define the right approach, I’m documenting this here while closing the previous issue, which is no longer applicable.

[sophie-cluml]

Following up on the direction agreed in this discussion, I've drafted two issues:

• review-web: aicers/review-web#835 — Split applyNode into applyNode (DB) and applyAgentConfig (agent notify)
• review-database: aicers/review-database#721 — Make node Table::update fully atomic and return the post-update Node

Both issues are marked blocked, pending feedback. I'd appreciate your comments and corrections.

What changes for a GraphQL client

The existing single-shot applyNode(id, node) becomes a two-call sequence: applyNode(id, node) promotes the drafts in the database, then applyAgentConfig(nodeId=id) sends agent-config notifications based on the current DB state. No data handoff is needed between the two calls — each is self-contained.

What changes in review-database

The node Table::update path becomes a single atomic operation covering Node, Agent, and ExternalService writes together, eliminating the current partial atomicity. Its return type changes from Result<()> to Result<Node> so the caller gets the post-update state without a separate read-back.

[sehkone]

After further discussion, we've decided to proceed with the split direction but keep the existing applyNode mutation untouched for backwards compatibility. Both follow-up issues have been updated accordingly:

• review-web: aicers/review-web#835 — now adds applyNodeDraft and applyAgentConfig as new, additive mutations. The existing applyNode is preserved unchanged in its public GraphQL contract; its internal implementation will be updated to call the new atomic Table::update, but the response shape stays as ID!.
• review-database: aicers/review-database#721 — atomic Table::update returning Node (direction unchanged).

Deprecation and eventual removal of applyNode is intentionally out of scope here and will be handled in a separate issue once downstream migration to the new pair is complete.