Commit ec8712bd authored by Stan Hu's avatar Stan Hu

Fix audit and Geo project deletion events not being logged under certain conditions

This commit fixes two issues:

1. Due to DNS errors, one of the after_commit hooks were causing an
Excon::SocketError error, which then got caught by the exception handler in
Projects::DestroyService. However, since the model was already deleted in the
database, attempting to update it would fail with a "Can't modify frozen hash"
error.

2. The Projects::DestroyService would then return `false`, which would cause
the prepended EE::Projects::DestroyService to skip logging of the project
deletion. Now we log an event if we see that the project was destroyed.

Closes gitlab-org/gitlab-ce#44378
parent 0c8c4636
......@@ -89,7 +89,10 @@ module Projects
def attempt_rollback(project, message)
return unless project
project.update_attributes(delete_error: message, pending_delete: false)
# It's possible that the project was destroyed, but some after_commit
# hook failed and caused us to end up here. A destroyed model will be a frozen hash,
# which cannot be altered.
project.update_attributes(delete_error: message, pending_delete: false) unless project.destroyed?
log_error("Deletion failed on #{project.full_path} with the following message: #{message}")
end
......
......@@ -7,7 +7,10 @@ module EE
def execute
succeeded = super
if succeeded
# It's possible that some error occurred, but at the end of the day
# if the project is destroyed from the database, we should log events
# and clean up where we can.
if project&.destroyed?
mirror_cleanup(project)
log_geo_event(project)
log_audit_event(project)
......
---
title: Fix audit and Geo project deletion events not being logged under certain conditions
merge_request:
author:
type: fixed
......@@ -74,4 +74,14 @@ describe Projects::DestroyService do
end
end
end
context 'system hooks exception' do
before do
allow_any_instance_of(SystemHooksService).to receive(:execute_hooks_for).and_raise('something went wrong')
end
it 'logs an audit event' do
expect { subject.execute }.to change(AuditEvent, :count)
end
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment