Findings from the final review of #4496 that were classified as not merge-blocking and deliberately left for a follow-up. Each was verified against the code; file and line refer to the state at review time.
1. The wait exemption only covers the first call
CONTRIBUTING.md (long-polling section) and auth.controller.ts — the exemption for GET /v1/auth/mail/confirm is argued from the first call, which enqueues the job, kicks the dispatcher and then waits for work it started itself. A repeat call finds an existing job and does nothing but wait for another process, which is exactly what the rule means by passive waiting. It does not change behaviour, but it does skew how these calls are classified in latency monitoring.
Suggestion: narrow the exemption text to the first call, or move repeat calls onto their own .../wait path.
2. nullable: false not stated explicitly
job.entity.ts, job-attempt.entity.ts — the contributing guide asks for it to be explicit where it applies. The migrations produce correct NOT NULL columns either way, so this is consistency only.
3. Relative imports inside the job module, and one unsorted import
job.service.ts and neighbours use relative paths where the repo generally uses absolute src/... ones; user.module.ts has the JobModule import out of alphabetical order.
4. CRLF line endings in dfx-cron.service.ts
The file carries CRLF, which is why its diff appeared as a full-file rewrite at one point and why git diff --check reports trailing whitespace. Normalising it to LF would make future diffs on that file readable — worth doing in a change that touches it anyway, not as a standalone churn commit.
5. uid serves as both public identifier and sole authorisation
job.controller.ts, job-dispatcher.service.ts — for a job without userData, knowing the uid is enough to read its status, and the same uid is exported as a span attribute. Not exploitable today: the only shipped group is AccountMerge, which always sets userData and has exposeResult: false. It is a foot-gun for future groups.
Suggestion: separate the public identifier from the authorisation capability, and keep the uid out of trace attributes.
6. dfx_job_finished misses jobs that recoverStale ends terminally
job-dispatcher.service.ts, job.service.ts — when a stale job has exhausted its attempts, recoverStale sets it to Failed, but the counter is only incremented on the dispatcher's own completion path. Those jobs are missing from the outcome metric.
Suggestion: return the terminally ended jobs from recoverStale, or increment at the point of the winning conditional update.
7. The metric snapshot has no age signal
job-metric.service.ts — the in-memory snapshot is served to the gauges regardless of how old it is. After a hung or failing snapshot cron, stale values are indistinguishable from genuine zeros. dfx_cron_seconds_since_last_run does not close this, because it is also updated on failed runs.
Suggestion: export a dedicated dfx_job_snapshot_age_seconds (or a validity flag) so the dashboards can tell fresh zeros from stale ones.
Findings from the final review of #4496 that were classified as not merge-blocking and deliberately left for a follow-up. Each was verified against the code; file and line refer to the state at review time.
1. The
waitexemption only covers the first callCONTRIBUTING.md(long-polling section) andauth.controller.ts— the exemption forGET /v1/auth/mail/confirmis argued from the first call, which enqueues the job, kicks the dispatcher and then waits for work it started itself. A repeat call finds an existing job and does nothing but wait for another process, which is exactly what the rule means by passive waiting. It does not change behaviour, but it does skew how these calls are classified in latency monitoring.Suggestion: narrow the exemption text to the first call, or move repeat calls onto their own
.../waitpath.2.
nullable: falsenot stated explicitlyjob.entity.ts,job-attempt.entity.ts— the contributing guide asks for it to be explicit where it applies. The migrations produce correctNOT NULLcolumns either way, so this is consistency only.3. Relative imports inside the job module, and one unsorted import
job.service.tsand neighbours use relative paths where the repo generally uses absolutesrc/...ones;user.module.tshas theJobModuleimport out of alphabetical order.4. CRLF line endings in
dfx-cron.service.tsThe file carries CRLF, which is why its diff appeared as a full-file rewrite at one point and why
git diff --checkreports trailing whitespace. Normalising it to LF would make future diffs on that file readable — worth doing in a change that touches it anyway, not as a standalone churn commit.5.
uidserves as both public identifier and sole authorisationjob.controller.ts,job-dispatcher.service.ts— for a job withoutuserData, knowing theuidis enough to read its status, and the sameuidis exported as a span attribute. Not exploitable today: the only shipped group isAccountMerge, which always setsuserDataand hasexposeResult: false. It is a foot-gun for future groups.Suggestion: separate the public identifier from the authorisation capability, and keep the uid out of trace attributes.
6.
dfx_job_finishedmisses jobs thatrecoverStaleends terminallyjob-dispatcher.service.ts,job.service.ts— when a stale job has exhausted its attempts,recoverStalesets it toFailed, but the counter is only incremented on the dispatcher's own completion path. Those jobs are missing from the outcome metric.Suggestion: return the terminally ended jobs from
recoverStale, or increment at the point of the winning conditional update.7. The metric snapshot has no age signal
job-metric.service.ts— the in-memory snapshot is served to the gauges regardless of how old it is. After a hung or failing snapshot cron, stale values are indistinguishable from genuine zeros.dfx_cron_seconds_since_last_rundoes not close this, because it is also updated on failed runs.Suggestion: export a dedicated
dfx_job_snapshot_age_seconds(or a validity flag) so the dashboards can tell fresh zeros from stale ones.