fix: avoid collisions in auto-generated Job IDs causing dropped jobs

[hughsw]

Alternate fix for #78 and #189
Builds on #193 , returning to its original intent

Uses random hex string for Job.id, generated and registered in Queue prior to adding job to Redis.

[hughsw]

[ This issue is resolved ]

This approach breaks the semantic that custom job id's don't affect checkHealth().newestJob, so the Queue Health Check should not report the latest job for custom job ids test fails.

The semantic could be implemented, but:

• it's additional complexity to support an obscure semantic
• I don't know how much the existing semantic is relied upon, it takes some hoop-jumping (as per the test) to observe -- what is the justification for custom ids being treated differently in terms of newestJob ?

[hughsw]

Actually, it's not hard to preserve the existing semantic, but I'd like some discussion about the necessity of this semantic and the test.

[hughsw]

[ This issue is resolved for Job.fromId]

Wrapping Job.fromId in order to test removal from 'active' prior to processing is problematic: This approach replaces a global resource with a one-shot modification. In a parallel testing framework, there's no guarantee that the test that replaces fromId is the one that uses the one-shot replacement... I'm not sure if this problem exists in the current master where it's a Queue instance that gets fiddled...

[hughsw]

So the one remaining test failure is baffling to me. The deepEqual of two Set object fails because the items in the sets are in different orders (even though the set of items is the same). It is reasonable that the code changes for uuid Job.id would cause the ordering to have changed, but my understanding is that Sets should be deepEqual regardless of the order. Any insight into the failure?

If I sort the error arrays (and deepEqual the arrays rather than going to the trouble of making Sets), the test passes, so that's likely what I'm going to add to this PR.

Here's a typical failure I get locally (similar to Travis failures):

  1 test failed

  queue-test › Queue Processing jobs should capture error data

  /bee-queue/test/queue-test.js:1238

   1237:                      
   1238:       t.deepEqual(   
   1239:         failedErrors,

  Difference:

    Set {
      `Error: has stack␊
          at Queue.queue.process.job (/bee-queue/test/queue-test.js:1212:23)␊
          at /bee-queue/node_modules/promise-callbacks/dist/index.js:300:18␊
          at new Promise (<anonymous>)␊
          at Queue.asyncWrapper [as handler] (/bee-queue/node_modules/promise-callbacks/dist/index.js:293:19)␊
          at Queue._runJob (/bee-queue/lib/queue.js:531:24)␊
          at finally_._getNextJob.then (/bee-queue/lib/queue.js:690:23)␊
          at process._tickCallback (internal/process/next_tick.js:68:7)`,
  +   'has message',
  +   'is string',
      true,
  -   'is string',
  -   'has message',
    }

[skeggse]

I'll try and take a look later tomorrow. I'm also curious what you think of #232.

[coveralls]

Coverage remained the same at 100.0% when pulling d018be3 on hughsw:fix-dropped-jobs-uuid into 3430506 on bee-queue:master.

[coveralls]

Coverage remained the same at 100.0% when pulling f6a1d1a on hughsw:fix-dropped-jobs-uuid into 3430506 on bee-queue:master.

[compwright]

Approved pending rebase

[bradvogel]

What is stored in bq:name:id? My understanding from this PR is that it's the number of jobs that don't have a custom id? What's that used for?

[compwright]

I believe that is correct, it's poorly named, and appears to come from the fact that auto-generated IDs are currently sequential. So job ID 1000 would be the 1000th job. This PR of course changes that but does not rename to try to maximize compatibility, replacing the stat with the actual count instead of the actual last job ID from which you would infer the count.

[bradvogel]

Where is that information ("the number of jobs that don't have a custom id") used?