Repository navigation
feat(npmjs): re-check laggy packuments through a queue - #1942
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
47c9919 to
fa412e7
Compare
| event.Records.map(async (record, i) => { | ||
| if (!outcomes[i].done) { | ||
| failures.push(record.messageId); | ||
| await scheduleNextAttempt(record, queueUrl); |
There was a problem hiding this comment.
Feels like a bit of overkill to do this, but I guess we're trying to go easy on NPM?
What is the base delay and visibility timeout? Couple minutes?
| const index = Math.min( | ||
| Math.max(attempt, 1) - 1, | ||
| PACKUMENT_RETRY_BACKOFF_SECONDS.length - 1 | ||
| ); | ||
| return PACKUMENT_RETRY_BACKOFF_SECONDS[index]; |
There was a problem hiding this comment.
Don't forget to add jitter
| problem = `could not be checked (${error})`; | ||
| } | ||
|
|
||
| if (attempt < PACKUMENT_MAX_ATTEMPTS) { |
There was a problem hiding this comment.
attempt is "approximate" so feels dangerous to me to base logic around it.
We can figure out that we've given up by the fact that the message lands in the DLQ. The processor on the DLQ could do this logging, and it could go to the same log group as this Lambda. Could even be the same Lambda instance that processes both queues; we don't need to retain the DLQ, it's not like we're going to redrive it or anything. Lost is lost.
| }); | ||
| this.queue = new Queue(this, 'Queue', { | ||
| encryption: QueueEncryption.KMS_MANAGED, | ||
| retentionPeriod: Duration.days(4), |
There was a problem hiding this comment.
Why not the full 2 weeks? We never intend to hit this anyway.
| * after the request was made. A request that still finds an old revision | ||
| * then is given up on, and moves to the dead-letter queue. | ||
| */ | ||
| export const PACKUMENT_RETRY_BACKOFF_SECONDS: readonly number[] = [ |
There was a problem hiding this comment.
Why a fixed list and not a calculation with a couple more touch-in points?
Together with receive count being "approximate" this feels somewhat dangerous to me.
fa412e7 to
8a466da
Compare
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
When the npm registry serves an older revision of a package than the changes feed announced, the follower processes what it got and checks the package again later. Until now it kept these "laggy packuments" in its state file and re-checked the 50 oldest on every run. Packages that never caught up held those slots for a day, so newer ones were not checked at all.
Laggy packuments now go to an SQS queue, and a new packument processor function works through it. It checks each package again with growing waits, from 5 minutes up to 11 hours. After about 27 hours it gives up, and the message moves to a dead-letter queue. The processor can run next to the follower now that known versions live in DynamoDB (#1941).
A 404 for a change entry used to be treated as a deleted package and ignored. Most of the time it is a new package that has not replicated to the registry yet, and prod sees a thousand or more of these a day. They now go through the same queue, but get their own metrics. Change entries marked as deleted are skipped completely.
Alarms
The dead-letter queue is graphed but doesn't get an alarm: with this many 404s, it seems useless. Instead we have low severity alarms aiming to detect npm outages and processing failures. Thresholds are based on a week of prod logs.
What's Next
This change deliberately only addresses laggy and missing packuments. I want to make sure the processing works as intended and observe for a bit. Once we are happy, the plan is to change the follower to do nothing more than reading the change stream and putting entries on the queue for the packument processor to pick up. This final step will make the architecture more resilient to outages: We have a single follower function that does the absolute minimum of work (read change stream, add to queue). Everything else happens detached in a queue processor and work can be fanned out. In theory this should also speed up ingestion from zero.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license