Paths

Table of Contentst

Differential D25246

Work around exception in Herald transcript of old tasks: Call to a member function getAppliedTransactionPHIDs() on bool
ClosedPublic
Actions

Authored by aklapper on May 26 2023, 14:27.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15343: Herald Transcript of old Tasks: Unhandled Exception: Call to a member function getAppliedTransactionPHIDs() on bool

Commits

rP54ee51d7abd0: Work around exception in Herald transcript of old tasks: Call to a member…

Summary

In some cases, older Herald transcripts which got garbage collected display an exception.
For unknown reasons, calling getObjectTranscript() on the HeraldTranscript object $xscript can return a bool with the value false.
Afterwards, calling getAppliedTransactionPHIDs() on that bool throws an exception.
Thus as a workaround, return an empty array instead in this case, like the code already does when $xscript->getObjectTranscript()->getAppliedTransactionPHIDs() === null.

Note that it remains unclear whether/when the bool may have the value true hence this situation is not covered by this code change (the code would still throw an exception in this situation).

Closes T15343

Test Plan

Change system time to one year back (or optionally, use ./phorge/bin/garbage set-policy --collector herald.transcripts --days 1, drop steps 9 and 10, and wait a day before performing step 11)
As an admin, go to http://phorge.localhost/herald/create/
Select "Maniphest Tasks"
On http://phorge.localhost/herald/create/?adapter=HeraldManiphestTaskAdapter , select "Global Rule"
Under "Conditions", select any of and set the three conditions Description | contains | Internet Archive, Description | contains | archive.org, Description | contains | Wayback Machine.
Under "Action", select Add projects | someProject
Select "Save Rule"
Trigger the rule by creating a new task.
Change system time back to today
On the shell, run ./phorge/bin/phd restart
Visit created task and select "View Herald Transcript"
Transcript page displays "Old Transcript - Details of this transcript have been garbage collected." and shows no exception anymore

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

aklapper created this revision.May 26 2023, 14:27

Owners added a reviewer: O1: Blessed Committers.May 26 2023, 14:28

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptMay 26 2023, 14:28

aklapper requested review of this revision.May 26 2023, 14:28

Harbormaster completed remote builds in B460: Diff 814.May 26 2023, 14:28

aklapper edited the test plan for this revision. (Show Details)May 26 2023, 14:29

Thanks! nice

You can also remove the is_bool() thing and just keep the strict check with triple equals, since it already verifies the type

This programming langauge clearly outsmarts me

Harbormaster completed remote builds in B465: Diff 819.May 26 2023, 22:08

aklapper mentioned this in Z5: Phorge (flame).Jun 12 2023, 01:59

Thank you so much for this very lovely deep journey.

It seems the garbage collector is here:

https://we.phorge.it/source/phorge/browse/master/src/applications/herald/garbagecollector/HeraldTranscriptGarbageCollector.php

And it just sets the objectTranscript as an empty string in MySQL.

The empty string can be verified inspecting the phabricator_herald.herald_transcript table checking for garbage-collected items:

SELECT * FROM herald_transcript WHERE garbageCollected = 1;

Now, what happens, is that the field objectTranscript is a PHP serialization:

https://we.phorge.it/source/phorge/browse/master/src/applications/herald/storage/transcript/HeraldTranscript.php$99

And, the problem is, the empty string is not a valid de-serialization.

Proof:

$ php
$ var_dump(unserialize(''));
bool(false)

And here the origin of that false!

Note that an empty string is never a valid de-serializable thing. For example, an empty string, when serialzied, it is not empty:

$ php
$ var_dump(serialize(""));
string(7) "s:0:"";"

So, indeed if there we receives false, that means the garbage collector has done its work since false means a serialization error very probably caused by the garbage collector.

Example context where false is assumed as "garbage collector passed":

src/applications/herald/controller/HeraldTranscriptController.php

$object_xscript = $xscript->getObjectTranscript();
if (!$object_xscript) {
  $notice = id(new PHUIInfoView())
    ->setSeverity(PHUIInfoView::SEVERITY_NOTICE)
    ->setTitle(pht('Old Transcript'))
    ->appendChild(phutil_tag(
      'p',
      array(),
      pht('Details of this transcript have been garbage collected.')));

So, this seems good to me, since after this change this page finally renders:

Screenshot_2023_06_12_085025.png (313×822 px, 21 KB)

But

I noticed that probably we can avoid to call the method getTranscriptTransactionPHIDs(), in this way:

diff --git a/src/applications/herald/controller/HeraldTranscriptController.php b/src/applications/herald/controller/HeraldTranscriptController.php
index dfc0dfc0b1..4bc569538f 100644
--- a/src/applications/herald/controller/HeraldTranscriptController.php
+++ b/src/applications/herald/controller/HeraldTranscriptController.php
@@ -717,8 +717,8 @@ final class HeraldTranscriptController extends HeraldController {
       ->setName(pht('Field Values'))
       ->setIcon('fa-file-text-o');

-    $xaction_phids = $this->getTranscriptTransactionPHIDs($xscript);
-    $has_xactions = (bool)$xaction_phids;
+    $has_xactions = $xscript->getObjectTranscript()
+      && $this->getTranscriptTransactionPHIDs($xscript);

     $nav->newLink('xactions')
       ->setName(pht('Transactions'))

So, I accept the current solution as behalf of myself, but probably it would be even better to do something like above, since we avoid to read nonsense data, if the object itself is nonsense.