Add content-existence flag to AgentPlan (avoid object-storage round-trip)

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

Problem

WorkItems::AgentPlan#content is stored externally via ExternallyStoredField / a CarrierWave uploader (WorkItems::AgentPlanContentUploader). The DB row (work_item_agent_plans) holds no content column - only file_store, namespace_id, work_item_id. Determining whether a plan actually has content therefore requires a round-trip to object storage (loading the field, or uploader.file.exists?).

This was acceptable while a row only existed once content was set. But !249644 (merged) changes that: we now create an agent_plan row when AI planning is enabled for a work item, before any plan content exists. So "a row exists" no longer implies "content exists" - and the only way to tell them apart today is a costly object-storage fetch.

Callers that only need to know whether a plan exists (e.g. the AI context block in !245983) should not pay that cost.

Why we can't derive this from existing state

Before reaching for a new column, we checked whether something already in the DB answers "does content exist?". Nothing does:

file_store doesn't work

file_store is set unconditionally on every save (Gitlab::ExternallyStoredField#set_file_store_from_uploader_config) and is null: false, default: 1. It records where the blob would live (1 = LOCAL, 2 = REMOTE), not whether one exists. A content-less row (planning enabled, no plan yet) still gets a file_store value, so file_store.present? is always true. Useless as an existence signal.

CarrierWave's filename column doesn't exist here (by design)

Normally CarrierWave's mount_uploader persists the uploaded filename in a DB column (e.g. content_file). A NULL filename means "no file" - that column is the DB-native existence check, no fetch required.

This model deliberately opts out of that. From lib/gitlab/externally_stored_field.rb:

Because we avoid using CarrierWave's mount_uploader -- it wants to store a filename in our ActiveRecord record, but our filenames are deterministic so that's wasteful -- we have to manually instantiate the uploader.

The filename is derived (AgentPlanContentUploader#filename -> "#{work_item_id}.json"), so the filename column was optimized away. That is exactly the column that would have answered this question for free - by dropping it, the design removed the only DB-native existence signal CarrierWave offers. What's left is read_external_payload, which calls external_storage_uploader.file&.exists? - a network stat against the Fog/remote backend. That's the round-trip we want to avoid.

"Row exists ⇒ content exists" no longer holds

We could have avoided a new column if a row only ever existed when content existed - then has_content? = persisted?. But !249644 (merged) breaks exactly that invariant by creating rows when planning is enabled, before content. That's what makes a dedicated flag necessary.

Conclusion: a new persisted column is the minimal correct fix. Re-introducing CarrierWave's filename column would cost the same and be less idiomatic for this hand-rolled setup, so we add a purpose-built boolean instead.

Proposal

Add a persisted boolean to work_item_agent_plans (has_content) that tracks content existence, and expose AgentPlan#has_content? so callers can decide without touching object storage.

  • Set the flag true when content is written, false/reset when content is removed. Keep it in sync with every mutation path (create, update, "remove plan", widget-excluded-in-new-type destroy).
  • Maintain it in the same AR save as file_store so it commits atomically with the row (see Gitlab::ExternallyStoredField#set_file_store_from_uploader_config for the existing hook and its documented after-commit crash window).
  • Expose it via GraphQL on the agent_plan widget so external/HTTP consumers can check existence cheaply.

Implementation details

The key insight: derive the flag from the in-memory content attribute during save

content is a virtual attribute (ExternallyStoredField), but its value is available in memory during the AR save cycle - callers assign agent_plan.content = params[:content] before saving (see WorkItems::Callbacks::AgentPlan#handle_agent_plan_change). So we do not need to read object storage to know whether content is present at write time; we already hold the value. We persist that presence as a boolean column, then reads never touch storage.

This is the same pattern file_store already uses: computed in a before_save hook from in-memory state, committed atomically with the row, and the blob upload happens later in after_commit.

1. Migration

Add a boolean to work_item_agent_plans, defaulting to false (a freshly created row = planning enabled, no content yet):

add_column :work_item_agent_plans, :has_content, :boolean, null: false, default: false

Table is new/small, so a plain add_column with a default is fine. Update db/docs/work_item_agent_plans.yml if needed.

2. Model: maintain the column + expose has_content?

In ee/app/models/work_items/agent_plan.rb, set the column from the in-memory content on every save, and let the boolean column back the predicate:

before_save :sync_has_content

def has_content?
  has_content
end

private

def sync_has_content
  self.has_content = content.present?
end

Notes:

  • content.present? here reads the in-memory attribute (already assigned by the caller), not object storage. On a brand-new row where planning was enabled but no content set, content is nil/blank -> has_content = false. Correct.
  • Because content is externally stored, be careful the before_save does not trigger a lazy load that hits storage. If content was never assigned on a persisted record, accessing it lazily loads from storage. Guard against that: only recompute when the field was actually assigned this cycle, e.g. return unless attribute_changed?(:content) || new_record?. Otherwise leave the existing column value untouched. Verify the exact dirty-tracking behaviour against ExternallyStoredField (the concern overrides content= to force a lazy load so writes register as changes - confirm attribute_changed?(:content) behaves as expected here).

3. Mutation paths that must stay in sync

All content changes flow through the model save, so a single before_save covers them - but confirm each:

  • Create / update content - WorkItems::Callbacks::AgentPlan#handle_agent_plan_change assigns content then saves. sync_has_content sets true.
  • Remove plan - the callbacks destroy! the whole agent_plan record (before_update when excluded_in_new_type?, and DataSync::Widgets::AgentPlan#post_move_cleanup). The row is gone, so there is no stale flag. Good - but note: with !249644 (merged), if "remove content" ever becomes "keep the row, clear content" rather than destroying the record, sync_has_content must flip it back to false. Add a spec for the "set content then clear content on an existing row" path to lock this in.
  • Data sync / move - DataSync::Widgets::AgentPlan#after_save_commit calls AgentPlan.create!(content: rewritten), which runs the same before_save. Fine.

4. GraphQL exposure

On the agent_plan widget type, add a boolean field resolving off the model predicate - never off content:

field :has_content, GraphQL::Types::Boolean, null: false,
  description: 'Whether the agent plan has content.'

def has_content
  object.has_content?
end

Make sure the widget preload (ee/lib/ee/api/helpers/work_items/preloads.rb maps agent_plan: [:agent_plan]) keeps this N+1-free; the predicate reads a column on the already-loaded association.

5. Backfill consideration

Existing rows predate the column. Since a row today only exists because content was set (pre-!249644 behaviour), a post-deploy backfill setting has_content = true for all existing rows is the safe default. Confirm ordering against !249644 (merged)'s rollout - if that MR ships first and starts creating content-less rows, the backfill logic must distinguish them (in that case, backfill from actual content existence, or accept that only rows created before its deploy get true).

Because of the new ai_planning_enabled field... Once could maybe even say we can set the has_content flag to true when ai_planning_enabled = false or rows older than the deploy date of the other MR?

Acceptance criteria

  • work_item_agent_plans has a boolean column tracking content existence (migration + db/docs update as needed).
  • The flag is set/reset correctly across all content mutation paths, verified by specs.
  • A model method returns content existence with no object-storage round-trip.
  • The value is exposed through the agent_plan GraphQL widget.
  • !245983 can switch its check from widget.content.present? to the new method.

Context

  • Model: ee/app/models/work_items/agent_plan.rb
  • Uploader: ee/app/uploaders/work_items/agent_plan_content_uploader.rb
  • External storage concern: lib/gitlab/externally_stored_field.rb
  • Table: work_item_agent_plans (see db/migrate/20260421071247_create_work_item_agent_plans.rb)
  • Related MRs: !245983, !249644 (merged)
Edited by 🤖 GitLab Bot 🤖