Skip to content

fix(server): notify watchers after atomic policy commits - #2556

Merged
drew merged 1 commit into
NVIDIA:mainfrom
NaveCohenMonday:2517-notify-atomic-policy-watchers/nc
Jul 30, 2026
Merged

fix(server): notify watchers after atomic policy commits#2556
drew merged 1 commit into
NVIDIA:mainfrom
NaveCohenMonday:2517-notify-atomic-policy-watchers/nc

Conversation

@NaveCohenMonday

Copy link
Copy Markdown
Contributor

Summary

Notify sandbox watchers after an atomic UpdateConfig policy revision commits.
Without this notification, live WatchSandbox(follow_status=true) consumers can
continue showing stale sandbox state until another event occurs. Sandbox
supervisors poll GetSandboxConfig independently and are not affected by this
notification gap.

Related Issue

Fixes #2517

Changes

  • Notify the sandbox watch bus only after put_policy_revision_atomic succeeds.
  • Extend the existing same-policy/new-provenance regression test to assert that
    one committed revision emits exactly one watcher notification.
  • Preserve conflict retry and failure behavior so unsuccessful writes do not
    notify observers.

Testing

  • CARGO_INCREMENTAL=0 mise run pre-commit passes
  • Unit tests added/updated
  • E2E tests added/updated (not applicable)

Focused test:

CARGO_INCREMENTAL=0 mise exec -- cargo test -p openshell-server \
  update_config_same_policy_hash_with_new_provenance_creates_revision --lib

test result: ok. 1 passed; 0 failed

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (not applicable)

Signed-off-by: Nave Cohen <nave@monday.com>
@copy-pr-bot

copy-pr-bot Bot commented Jul 30, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions

github-actions Bot commented Jul 30, 2026

Copy link
Copy Markdown

All contributors have signed the DCO ✍️ ✅
Posted by the DCO Assistant Lite bot.

@NaveCohenMonday

Copy link
Copy Markdown
Contributor Author

I have read the DCO document and I hereby sign the DCO.

@drew drew left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gator-agent

PR Review Status

Validation: This concentrated server fix implements the confirmed watcher-notification defect in linked issue #2517 and stays within that issue's scope.
Head SHA: 28702360e622a60670c967098519e6ebb0185332

Review findings:

  • No blocking findings remain. The notification is reached only after the atomic policy revision transaction commits, while failed writes, exhausted retries, and idempotent no-op requests return before it.
  • The regression test covers delivery and exactly-once behavior for the affected same-policy/new-provenance path.

Docs: Not needed; this restores an existing internal watch notification invariant without changing a public API, CLI, configuration field, or documented workflow.

E2E: No test:* label is needed; the focused server unit test directly covers this in-memory notification hook.

Next state: gator:watch-pipeline

@drew drew added the gator:watch-pipeline Gator is monitoring PR CI/CD status label Jul 30, 2026
@drew

drew commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

/ok to test 2870236

@drew
drew added this pull request to the merge queue Jul 30, 2026
Merged via the queue into NVIDIA:main with commit 28f3bee Jul 30, 2026
33 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gator:watch-pipeline Gator is monitoring PR CI/CD status

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] UpdateConfig does not wake sandbox watchers after an atomic policy revision commit

2 participants