recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Fix an issue where Drydock followup tasks would not queue if the main task failed

Summary:
Ref T9994. This fixes the first issue discussed on that task, which is that when a merge fails after "arc land", we would not clean up all the leases properly.

Specifically, when a merge fails, we use `queueTask()` to schedule a followup task. This followup destroys the lease and frees the underlying resource.

However, the default behavior of `queueTask()` is to //not queue tasks// if the parent task fails. This is a reasonable, safe behavior that was originally introduced in D8774, where it kept us from sending too much mail if a task did "send some mail" and then failed a little later on and got retried.

Since I think the default behavior is correct, I just special cased the behavior for Drydock to make it queue even on failure. These are the only types of followup tasks we currently want to queue on main task failure.

(It's possible that future Blueprints might want some kind of more specialized behavior, where some tasks queue only on success, but we can cross that bridge when we come to it.)

Test Plan:
- See T9994#149878 for test case setup.
- I ran that test case again with this patch, and saw the followup task queue properly in the `--trace` log, a correspoinding update task show up in `/daemon/`, and the lease get destroyed when I ran it a moment later.

{F1029915}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9994

Differential Revision: https://secure.phabricator.com/D14818

epriestley e9af4f89 5bbf0ba1

+59 -16
+1
src/applications/drydock/worker/DrydockLeaseUpdateWorker.php
··· 26 26 $this->handleUpdate($lease); 27 27 } catch (Exception $ex) { 28 28 $lock->unlock(); 29 + $this->flushDrydockTaskQueue(); 29 30 throw $ex; 30 31 } 31 32
+1
src/applications/drydock/worker/DrydockResourceUpdateWorker.php
··· 24 24 $this->handleUpdate($resource); 25 25 } catch (Exception $ex) { 26 26 $lock->unlock(); 27 + $this->flushDrydockTaskQueue(); 27 28 throw $ex; 28 29 } 29 30
+26
src/applications/drydock/worker/DrydockWorker.php
··· 170 170 return 15; 171 171 } 172 172 173 + protected function flushDrydockTaskQueue() { 174 + // NOTE: By default, queued tasks are not scheduled if the current task 175 + // fails. This is a good, safe default behavior. For example, it can 176 + // protect us from executing side effect tasks too many times, like 177 + // sending extra email. 178 + 179 + // However, it is not the behavior we want in Drydock, because we queue 180 + // followup tasks after lease and resource failures and want them to 181 + // execute in order to clean things up. 182 + 183 + // At least for now, we just explicitly flush the queue before exiting 184 + // with a failure to make sure tasks get queued up properly. 185 + try { 186 + $this->flushTaskQueue(); 187 + } catch (Exception $ex) { 188 + // If this fails, we want to swallow the exception so the caller throws 189 + // the original error, since we're more likely to be able to understand 190 + // and fix the problem if we have the original error than if we replace 191 + // it with this one. 192 + phlog($ex); 193 + } 194 + 195 + return $this; 196 + } 197 + 198 + 173 199 }
+26 -6
src/infrastructure/daemon/workers/PhabricatorWorker.php
··· 158 158 159 159 while (true) { 160 160 try { 161 - $worker->doWork(); 162 - foreach ($worker->getQueuedTasks() as $queued_task) { 163 - list($queued_class, $queued_data, $queued_options) = $queued_task; 164 - self::scheduleTask($queued_class, $queued_data, $queued_options); 165 - } 161 + $worker->executeTask(); 162 + $worker->flushTaskQueue(); 166 163 break; 167 164 } catch (PhabricatorWorkerYieldException $ex) { 168 165 phlog( ··· 236 233 * 237 234 * @return list<tuple<string, wild, int|null>> Queued task specifications. 238 235 */ 239 - final public function getQueuedTasks() { 236 + final protected function getQueuedTasks() { 240 237 return $this->queuedTasks; 238 + } 239 + 240 + 241 + /** 242 + * Schedule any queued tasks, then empty the task queue. 243 + * 244 + * By default, the queue is flushed only if a task succeeds. You can call 245 + * this method to force the queue to flush before failing (for example, if 246 + * you are using queues to improve locking behavior). 247 + * 248 + * @param map<string, wild> Optional default options. 249 + * @return this 250 + */ 251 + final public function flushTaskQueue($defaults = array()) { 252 + foreach ($this->getQueuedTasks() as $task) { 253 + list($class, $data, $options) = $task; 254 + 255 + $options = $options + $defaults; 256 + 257 + self::scheduleTask($class, $data, $options); 258 + } 259 + 260 + $this->queuedTasks = array(); 241 261 } 242 262 243 263
+5 -10
src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php
··· 216 216 // NOTE: If this throws, we don't want it to cause the task to fail again, 217 217 // so execute it out here and just let the exception escape. 218 218 if ($did_succeed) { 219 - foreach ($worker->getQueuedTasks() as $task) { 220 - list($class, $data, $options) = $task; 221 - 222 - // Default the new task priority to our own priority. 223 - $options = $options + array( 224 - 'priority' => (int)$this->getPriority(), 225 - ); 226 - 227 - PhabricatorWorker::scheduleTask($class, $data, $options); 228 - } 219 + // Default the new task priority to our own priority. 220 + $defaults = array( 221 + 'priority' => (int)$this->getPriority(), 222 + ); 223 + $worker->flushTaskQueue($defaults); 229 224 } 230 225 231 226 return $result;