Give the secrets read billing emit its own idempotency marker

Vijay Hawoldarrequested to merge
18551-billing-emit-idempotency into master

What does this MR do and why?

Gives the secrets_read billing emit in SecretsManagement::AuditLogWorker its own idempotency marker, so a job redelivered after a successful emit does not count the same read twice in billable_usage_daily_aggregates.

Found while building the air-gapped export on top of !249324 (merged). That MR does not touch this worker, and the worker calling the emitter is not the problem: a billable read is an OpenBao audit entry, and the worker has emitted one billing event per entry since audit processing moved to Sidekiq in July. What !249324 (merged) adds is a second downstream for those events, and that is where the worker's existing at-least-once semantics stop being safe.

The bug

perform wraps write_audit_log in an IdempotencyCache marker, then calls SecretsReadEmitter.emit! outside it. The comment on write_audit_log explains the reasoning, which was correct when written:

Billing needs no marker here: the emitter sends its own request-id-based idempotency key downstream, so re-running it is safe.

True for the streaming path, where CustomersDot dedupes on event_id. Not true for local persistence, where RecordAggregateService upserts quantity = quantity + EXCLUDED.quantity and never sees the per-event id.

Failing sequence: the worker performs, the audit write commits and sets its marker, the emitter runs (+1), the process dies before Sidekiq acks. On redelivery the audit write is skipped by its marker and the emitter runs again (+1). One read, quantity: 2, and that is the number the customer exports and is billed on.

The existing spec pins the old behaviour as intended: "still invokes the billable event emitter, which deduplicates downstream", asserting emit! is called twice on redelivery. This MR flips that expectation.

Why two markers rather than one

Moving the emit inside the existing marker looks smaller and does not work, because of the rescue around the emit:

  • Rescue inside the block: an emit failure still lets the block return, so the marker is set and no redelivery ever retries the emit. The billing event is lost.
  • Rescue outside the block: a failed emit leaves the marker unset, so redelivery re-runs the audit write too, and the audit tables have no natural-key guard. A duplicate audit row.

A separate marker for the emit, set only when the block returns, avoids both. Redelivery after a successful emit skips it. If the block raises, the marker stays unset and the audit write is untouched.

One limit, raised in review and worth stating plainly: SecretsReadEmitter#emit! and Client#track_billing_event both rescue StandardError internally, so a genuine billing failure returns normally and the marker is set. That is unchanged from today, where the job acked normally on the same failure, so nothing is lost here, but this MR does not make a failed emit retryable either. Doing that needs an explicit success result from the emitter and client and belongs with the local persistence rollout. The worker's own rescue is still live: IdempotencyCache makes Redis calls outside the emitter's rescue.

A second limit, raised by Duo review. The marker check and the marker write are two Redis round trips, so a process killed between a successful emit and the marker write will re-emit on redelivery and double count. This is not introduced here: IdempotencyCache.ensure_idempotency is check-then-act too, so the audit write has had the same window since the original change. What this MR does is narrow it from the whole audit-write-plus-emit sequence to a single emit call.

An atomic SET NX would not close that window, it would invert it: claiming the marker atomically means claiming it before the emit, so a crash in the same place loses the read rather than double counting it, which is the opposite of the trade-off above. Closing it properly means giving the local write the per-event dedupe that streaming gets from CustomersDot, and doing it atomically with the increment, which a Redis marker and a Postgres upsert cannot be. In practice that means the marker moving into Postgres and committing in the same transaction as RecordAggregateService's upsert, at the cost of a row per billable event until it is pruned. Whether that is worth it against a window this small is a rollout decision, so it belongs with #628679.

The audit marker's Redis key is byte-identical to before (the stage argument is nil for it), so markers already in Redis stay valid across the deploy. The worker's idempotent! declaration becomes true again for the local path.

References

How to set up and validate locally

Stage 1 reproduces the redelivery on master and shows this branch emitting once. Stage 2 shows the count in the aggregate table, and needs the local write path from !249324 (merged).

Stage 1: the emit runs once per OpenBao request

  1. Enable the two flags the emit is gated on, and pick a project and a user who can read it:

    Feature.enable(:secrets_manager_emit_secret_read_events)
Feature.enable(:billing_event_tracking)
project = Project.first
user = project.first_owner

  2. Build one OpenBao audit line. The request id is what both markers key on, so keep it fixed across the two runs. policy_results.allowed is what makes the read billable:

    request_id = SecureRandom.uuid
payload = {
  time: Time.current.utc.iso8601(5),
  type: 'response',
  auth: {
    policies: ['default', "project_#{project.id}/users/direct/user_#{user.id}"],
    entity_id: SecureRandom.uuid,
    policy_results: { allowed: true },
    metadata: { correlation_id: SecureRandom.hex(13), namespace_id: project.namespace_id.to_s,
                project_id: project.id.to_s, user_id: user.id.to_s }
  },
  request: {
    id: request_id, operation: 'read', path: 'secrets/kv/data/explicit/my_test_secret',
    namespace: { id: 'M0zDLE', path: "user_#{user.id}/project_#{project.id}/" },
    remote_address: '172.16.123.1'
  },
  response: { mount_type: 'kv' }
}.to_json

  3. Deliver it twice. Calling perform directly on a new worker instance is the same thing Sidekiq does when it redelivers a job that never acked:

    audits_before = AuditEvents::ProjectAuditEvent.where(project_id: project.id).count
2.times { SecretsManagement::AuditLogWorker.new.perform(payload) }

  4. Check the outcomes:

    AuditEvents::ProjectAuditEvent.where(project_id: project.id).count - audits_before
# => 1 on both master and this branch: the audit marker already existed

Gitlab::Redis::SharedState.with { |r| r.keys("secrets_management:audit_log_worker:*#{request_id}") }
# this branch => two keys, one plain and one with the `billing` stage
# master       => one key
    grep -c "billing event tracked" log/application_json.log

    Run that grep before and after step 3. The client logs one BillingEvents: billing event tracked line per emit that reached it, so the difference is the number of emits. This branch: 1. Master: 2. Nothing else in the payload changes between the two runs, so the second line on master is the double count.

    The Entitlement.for! call inside the emitter may log a CustomersDot connection error in development. It is fail-open and does not stop the emit.

  5. Rerun steps 2 to 4 with a fresh request_id to confirm a new request still emits once. The markers expire after PROCESSED_MARKER_TTL (6 hours), or clear them with Gitlab::Redis::SharedState.with { |r| r.del(*r.keys('secrets_management:audit_log_worker:*')) }.

Stage 2: the aggregate row counts the read once

The local write path from !249324 (merged) is on master, so this branch has it. Follow that MR's setup for an offline licence and local_billing_persistence, then repeat stage 1 steps 2 and 3 and check the row:

Utilization::BillableUsage::DailyAggregate.where(event_type: 'secrets_read').pick(:quantity, :events_count).map(&:to_i)
# => [1, 1] with this branch; [2, 2] without it

Spec

bundle exec rspec ee/spec/workers/secrets_management/audit_log_worker_spec.rb

The redelivery context asserts one emit across two deliveries. The emitter-raises context asserts the marker contract: no billing marker while the block raises, and no second audit row.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist.

Edited by Vijay Hawoldar

Merge request reports