Skip to content

fix: prevent system segments from being overwritten via change request drafts #8298

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
srijantrpth wants to merge 7 commits into
base: main
Choose a base branch
from
+48 −0
Open

fix: prevent system segments from being overwritten via change request drafts #8298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f59da3a
fix: prevent system segments from being overwritten via change reques…
srijantrpth Aug 14, 2026
0b5ab68
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 14, 2026
00269fc
fix: make change request commit fully atomic and add rollback asserti…
srijantrpth Aug 14, 2026
14137a2
Merge branch 'main' into fix-8269-system-segments
srijantrpth Aug 14, 2026
c3c40a9
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 14, 2026
3e15279
style: add trailing newline to pass ruff linter
srijantrpth Aug 14, 2026
3d0b1ba
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions api/core/workflows_services.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ class ChangeRequestCommitService:
def __init__(self, change_request: "ChangeRequest") -> None:
self.change_request = change_request

@transaction.atomic

@coderabbitai coderabbitai Bot Aug 14, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Defer external side effects until commit succeeds.

The decorator on Line 23 now wraps _publish_environment_feature_versions() and _publish_segments(). The Celery .delay(...) calls and environment_feature_version_published.send(...) still run before the transaction commits. If the system-segment guard raises ValueError, database changes roll back, but broker messages and signal-receiver side effects can remain. Register these calls with transaction.on_commit() and bind each loop value per callback.

@srijantrpth srijantrpth Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the catch! Since _publish_environment_feature_versions handles core versioning logic outside the scope of this specific system segment security patch, I'll leave the transaction hooks as-is for now to avoid altering existing versioning behavior, but I appreciate the architectural insight.

@coderabbitai coderabbitai Bot Aug 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@srijantrpth, understood. The system-segment protection can remain scoped to this patch.

The side-effect ordering risk remains after the new outer transaction. Would you like me to open a follow-up issue for deferring the Celery dispatches and environment_feature_version_published.send() calls with transaction.on_commit()?

🐇

🧠 Learnings used 
Learnt from: khvn26
Repo: Flagsmith/flagsmith PR: 8042
File: api/trust_relationships/services.py:30-53
Timestamp: 2026-07-20T09:52:12.198Z
Learning: In the Flagsmith Django API, do not rely on `ATOMIC_REQUESTS` being enabled (it is not used and will not be adopted). When reviewing code in the service layer (e.g., `services.py`), avoid assumptions that view-layer operations run inside an enclosing request transaction. If you see `transaction.atomic()` in service code, treat it as defining the transaction boundary itself (or explicitly justify nesting/interaction), and do not flag issues based solely on the absence/presence of a request-level transaction.

You are interacting with an AI system.

@srijantrpth srijantrpth Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure, opening a follow-up issue for that side-effect ordering risk would be a great proactive step!

@coderabbitai coderabbitai Bot Aug 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

repo="Flagsmith/flagsmith"
body=$(cat <<'EOF'
## Summary

Defer external side effects from change request publishing until the enclosing database transaction commits successfully.

## Rationale

`ChangeRequestCommitService.commit()` now runs in `transaction.atomic`. `_publish_environment_feature_versions()` dispatches Celery tasks and sends `environment_feature_version_published` before this transaction commits. If a later operation fails, for example when `_publish_segments()` rejects a system segment, the database transaction rolls back but queued tasks and signal-receiver side effects can still run against data that was not committed.

## Affected area

- `api/core/workflows_services.py`
- `ChangeRequestCommitService._publish_environment_feature_versions`

## Required changes

Register `trigger_update_version_webhooks.delay(...)`, `rebuild_environment_document.delay(...)`, and `environment_feature_version_published.send(...)` through `transaction.on_commit()`. Bind each loop value in its callback so callbacks use the correct environment feature version and scheduled timestamp.

## Acceptance criteria

- No Celery task is dispatched when `ChangeRequestCommitService.commit()` rolls back.
- No `environment_feature_version_published` receiver runs when `ChangeRequestCommitService.commit()` rolls back.
- The existing side effects run after a successful transaction commit.
- Tests cover both rollback and successful commit behaviour.

## Backlinks

- Pull request: https://github.com/Flagsmith/flagsmith/pull/8298
- Review comment: https://github.com/Flagsmith/flagsmith/pull/8298#discussion_r3786731701
- Requested by: `@srijantrpth`
EOF
)

gh issue create \
  --repo "$repo" \
  --title "Defer change request publishing side effects until transaction commit" \
  --body "$body" \
  --assignee "srijantrpth"

Length of output: 2047

@srijantrpth, I created follow-up issue #8299: Defer change request publishing side effects until transaction commit.

🐇

🧠 Learnings used 
Learnt from: khvn26
Repo: Flagsmith/flagsmith PR: 8042
File: api/trust_relationships/services.py:30-53
Timestamp: 2026-07-20T09:52:12.198Z
Learning: In the Flagsmith Django API, do not rely on `ATOMIC_REQUESTS` being enabled (it is not used and will not be adopted). When reviewing code in the service layer (e.g., `services.py`), avoid assumptions that view-layer operations run inside an enclosing request transaction. If you see `transaction.atomic()` in service code, treat it as defining the transaction boundary itself (or explicitly justify nesting/interaction), and do not flag issues based solely on the absence/presence of a request-level transaction.

You are interacting with an AI system.

def commit(self, committed_by: "FFAdminUser") -> None:
if not self.change_request.is_approved():
raise ChangeRequestNotApprovedError(
Expand Down Expand Up @@ -130,6 +131,12 @@ def _publish_segments(self) -> None:
logger.warning("missing-live-segment", draft_segment=draft_segment.uuid)
continue

# Prevent overwriting system segments
if getattr(live_segment, "is_system_segment", False):
raise ValueError(
"System segments cannot be overwritten via change request drafts."
)

# Make a revision of the live segment
revision = live_segment.clone(is_revision=True)
logger.info(
Expand Down
41 changes: 41 additions & 0 deletions api/tests/unit/features/workflows/core/test_unit_workflows_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1283,3 +1283,44 @@ def test_change_request_commit__v1_segment_override_draft__inherits_mv_hashing_s
# Then the draft carries the superseded override's id as its bucketing salt
draft_feature_state.refresh_from_db()
assert draft_feature_state.mv_hashing_salt == live_override.id


def test_change_request_commit__system_segment_draft__raises_value_error(
segment: Segment,
change_request: ChangeRequest,
admin_user: FFAdminUser,
feature: Feature,
environment: Environment,
) -> None:
# Given
segment.is_system_segment = True
segment.save()

Segment.objects.create(
name="system-segment-draft",
change_request=change_request,
project=segment.project,
version_of=segment,
)

# Add a feature state to test transaction rollback behavior
feature_state = FeatureState.objects.create(
feature=feature,
environment=environment,
change_request=change_request,
version=None,
)
initial_version = feature_state.version

# When / Then
with pytest.raises(
ValueError,
match="System segments cannot be overwritten via change request drafts.",
):
change_request.commit(admin_user)
Comment thread
coderabbitai[bot] marked this conversation as resolved.

# Assert that the transaction rolled back successfully
feature_state.refresh_from_db()
change_request.refresh_from_db()
assert feature_state.version == initial_version
assert change_request.committed_at is None
Loading