Hourly cronjob #2 is not deleting filedata

Collapse
X
 
  • Time
  • Show
Clear All
new posts
  • Zambfd
    New Member
    • Nov 2012
    • 16
    • 4.1.x

    Hourly cronjob #2 is not deleting filedata

    Hi,

    VB: 4.2.2 Patch Level 4
    File: /includes/cron/cleanup2.php

    Code:
    // Unused filedata is removed after one hour
    $attachdata =& datamanager_init('Filedata', $vbulletin, ERRTYPE_SILENT, 'attachment');
    $attachdata->set_condition("fd.refcount = 0 AND fd.dateline < " . (TIMENOW - 3600), 1000);
    $attachdata->delete();
    This is not working because the following if clause in /includes/class_dm_attachment.php on line 909 is always "false".
    Code:
    if (!$this->pre_delete($doquery) OR empty($this->lists['filedataids']))
    The problem is located in the pre_delete method in vB_DataManager_AttachData. It returns false in the foreach loop at line 409 because there are many no more existing contenttypeids left in the older filedata entries.

    Because of this issue we have possibly over 280.000 orphaned uploaded user files in our database and on the file system.

    We could fix it by changing the mentioned class_dm_attachment.php from ...
    Code:
    if (!$this->pre_delete($doquery) OR empty($this->lists['filedataids']))
    {
         return $false;
    }
    (shouldn't it be "return false;" not "return $false;"? ;P)

    ... to ...

    Code:
    $this->pre_delete($doquery);
    ... but i don't know if this causes any other issues like the deletion of active files with correct refcounts or content id assignments.
    Last edited by Zambfd; Wed 18 Feb '15, 5:51am.
  • kh99
    Senior Member
    • Aug 2009
    • 533

    #2
    Hmm...it also looks like vB_DataManager_AttachData pre_delete() doesn't have the same parameter list, and the first parameter is actually the type, so that by calling $this->pre_delete($doquery), it's defaulting to type == 'attachment' instead of type=='filedata'. Although I don't know what the effect of that is. It looks like it switches which table is the 'left' table in the query, but I haven't studied it to see what that would mean.

    Anyway, I've noticed that my unused filedata never seems to go away, but I never thought too much about it.

    Comment

    • Zambfd
      New Member
      • Nov 2012
      • 16
      • 4.1.x

      #3
      Originally posted by kh99
      Hmm...it also looks like vB_DataManager_AttachData pre_delete() doesn't have the same parameter list, and the first parameter is actually the type, so that by calling $this->pre_delete($doquery), it's defaulting to type == 'attachment' instead of type=='filedata'. Although I don't know what the effect of that is. It looks like it switches which table is the 'left' table in the query, but I haven't studied it to see what that would mean.
      I tested it and set some debug output points, it's working .
      If you try my code changes the variable $this->lists['filedataids'] is filled correctly, but with 280k+ entries it is a little bit to much for the following IN() statement.
      Also the $limit parameter in set_condition() of the datamanger is ignored.

      Comment

      • kh99
        Senior Member
        • Aug 2009
        • 533

        #4
        You should enter the bug in the Tracker.

        Comment

        • Sqυιd
          Member
          • Mar 2009
          • 39
          • 4.2.X

          #5
          Originally posted by kh99
          Hmm...it also looks like vB_DataManager_AttachData pre_delete() doesn't have the same parameter list, and the first parameter is actually the type, so that by calling $this->pre_delete($doquery), it's defaulting to type == 'attachment' instead of type=='filedata'. Although I don't know what the effect of that is. It looks like it switches which table is the 'left' table in the query, but I haven't studied it to see what that would mean.

          Anyway, I've noticed that my unused filedata never seems to go away, but I never thought too much about it.
          In your analysis you're missing one level of function call. In vB_DataManager_Filedata when you have the function call on line 909 to pre_delete($doquery) it is calling the pre_delete function on line 902 and not the one in vB_DataManager_AttachData at line 357 directly. The pre-delete function in the vB_DataManager_Filedata correctly calls the one in the vB_DataManager_AttachData class with the proper type.

          The real question, aside from the $false to false change. Is why is pre_delete call to the vB_Attachment_Dm_Library returning false when it shouldn't be? There are only two other places that could return false, one is if there are no ids read from the db that need deleting, which isn't triggering given that Zambfd verified that the lists['filedataids'] had values in it, which could only happen if you had a non-zero number of results from the query or the call to the vB_DataManager pre_delete, but it always returns true.

          Comment

          • kh99
            Senior Member
            • Aug 2009
            • 533

            #6
            Originally posted by Sqυιd

            In your analysis you're missing one level of function call. In vB_DataManager_Filedata when you have the function call on line 909 to pre_delete($doquery) it is calling the pre_delete function on line 902 and not the one in vB_DataManager_AttachData at line 357 directly. The pre-delete function in the vB_DataManager_Filedata correctly calls the one in the vB_DataManager_AttachData class with the proper type.
            Lol, you're right, and it's right above it too. I shouldn't try to look at these things first thing in the morning.

            Comment

            • Sqυιd
              Member
              • Mar 2009
              • 39
              • 4.2.X

              #7
              Actually I'm wondering if its because permissions are being checked for filedata but not for attachment data.

              You'll notice that in cleanup2.php when deleting attachments on line 67 the call to delete specifies that permissions should not be checked (i.e. delete(true, false) on line 67), which get propagated up the pre_delete call stack to the vb_Attachment_Dm_Library sub class pre_delete calls, but when doing filedata the delete does not specifiy that permissions should not be checked (i.e. delete() on line 72).

              To force permissions not to be checked for filedata you'd need to make the following changes:

              includes/cron/cleanup2.php:

              Change line 72 from:
              Code:
              attachdata->delete();
              to:
              Code:
              attachdata->delete([B][COLOR=#FF0000]true, false[/COLOR][/B]);
              includes/class_dm_attachment.php:
              Change line 902-912 from:
              Code:
                  public function pre_delete($doquery = true, $checkperms = true, $type = 'filedata')
                  {
                      return parent::pre_delete('filedata', $doquery);
                  }
              
                  public function delete($doquery = true)
                  {
                      if (!$this->pre_delete($doquery) OR empty($this->lists['filedataids']))
                      {
                          return $false;
                      }
              to:
              Code:
                  public function pre_delete($doquery = true, $checkperms = true, $type = 'filedata')
                  {
                      return parent::pre_delete('filedata', $doquery[B][COLOR=#FF0000], $checkperms[/COLOR][/B]);
                  }
              
                  public function delete($doquery = true[B][COLOR=#FF0000], $checkperms = true[/COLOR][/B])
                  {
                      if (!$this->pre_delete($doquery[B][COLOR=#FF0000], $checkperms[/COLOR][/B]) OR empty($this->lists['filedataids']))
                      {
                          return [B][COLOR=#FF0000]false[/COLOR][/B]; //removed the $ from false
                      }
              I haven't had a chance to check if this fixes the issue or not, but it is one difference between the two deletes.

              Comment

              • Sqυιd
                Member
                • Mar 2009
                • 39
                • 4.2.X

                #8
                The above seems to have worked, but my backlog of undeleted filedata was so big (210K+) it wouldn't complete the pre_delete check, so I added:
                Code:
                 . " LIMIT 500"
                to make the line 70 like:
                Code:
                $attachdata->set_condition("fd.refcount = 0 AND fd.dateline < " . (TIMENOW - 3600) . " LIMIT 500");
                After the above change the cleanup2.php script went through.

                Comment

                • Sqυιd
                  Member
                  • Mar 2009
                  • 39
                  • 4.2.X

                  #9
                  I have confirmed, after clearing the long backlog of undeleted filedata items that if $checkperms is not sent through as false, the entries do not delete. I checked this by changing the function call (in my new version of the code) from attachdata->delete(true, false) to attachdata(true, true) and no filedata items were deleted. I'll make a bug report for the bug.

                  Comment

                  • Sqυιd
                    Member
                    • Mar 2009
                    • 39
                    • 4.2.X

                    #10
                    Bug Report: http://tracker.vbulletin.com/browse/VBIV-16070

                    Comment

                    • Zambfd
                      New Member
                      • Nov 2012
                      • 16
                      • 4.1.x

                      #11
                      I already fixed it in our boards. Anyway, thank you Squid for the bug tracker entry! I wasn't thinking it.

                      Comment

                      widgetinstance 262 (Related Topics) skipped due to lack of content & hide_module_if_empty option.
                      Working...