fix(git-sync): address review — configurable poll, always-on loop-guard, cleanup
Comprehensive-review follow-ups (APPROVE WITH SUGGESTIONS; no critical issues):
- poll interval is now actually configurable: replaced the hardcoded
@Interval('git-sync-poll', 15000) with a dynamic SchedulerRegistry interval
registered in onModuleInit from getGitSyncPollIntervalMs() (cleared in
onModuleDestroy); /status and the real cadence now share one config source.
Boots logging 'poll interval registered (Nms)'.
- loop-guard now ALWAYS applies: the lastUpdatedSource==='git-sync' skip was
nested inside the !spaceId/!workspaceId branch, so structural self-writes
(CREATE/MOVE/RESTORE/SOFT_DELETE, which carry spaceId+workspaceId) bypassed it
and re-triggered cycles. Fetch the page row once, guard unconditionally, then
resolve space/workspace.
- remove the dead PAGE_CONTENT_UPDATED subscription (it's a BullMQ job, never an
EventEmitter event; body edits arrive via PAGE_UPDATED).
- fix the stale datasource comment (PageService DOES stamp 'git-sync' now).
- env getters: parseInt radix 10 + NaN/<=0 fallback for poll/debounce (+ max
deletes), with 6 new environment.service.spec tests.
tsc clean; jest 723 pass; live cycle re-verified post-refactor (ran, push
applied, unflagged 92-page space untouched).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -1,5 +1,10 @@
|
||||
import { Injectable, Logger } from '@nestjs/common';
|
||||
import { Interval } from '@nestjs/schedule';
|
||||
import {
|
||||
Injectable,
|
||||
Logger,
|
||||
OnModuleDestroy,
|
||||
OnModuleInit,
|
||||
} from '@nestjs/common';
|
||||
import { SchedulerRegistry } from '@nestjs/schedule';
|
||||
import { RedisService } from '@nestjs-labs/nestjs-ioredis';
|
||||
import type { Redis } from 'ioredis';
|
||||
import { randomUUID } from 'node:crypto';
|
||||
@@ -60,18 +65,21 @@ export interface GitSyncRunStatus {
|
||||
* first; per-space opt-in is now REQUIRED on top of it.
|
||||
*/
|
||||
@Injectable()
|
||||
export class GitSyncOrchestrator {
|
||||
export class GitSyncOrchestrator implements OnModuleInit, OnModuleDestroy {
|
||||
private readonly logger = new Logger(GitSyncOrchestrator.name);
|
||||
private readonly redis: Redis;
|
||||
/** Unique per process instance — the leader-lock value (CAS on release). */
|
||||
private readonly instanceId = randomUUID();
|
||||
/** In-process per-space mutex: spaceIds with a cycle currently running. */
|
||||
private readonly running = new Set<string>();
|
||||
/** The registered poll-interval name, or null when none is registered. */
|
||||
private pollIntervalName: string | null = null;
|
||||
|
||||
constructor(
|
||||
private readonly environmentService: EnvironmentService,
|
||||
private readonly dataSource: GitmostDataSourceService,
|
||||
private readonly vaultRegistry: VaultRegistryService,
|
||||
private readonly schedulerRegistry: SchedulerRegistry,
|
||||
redisService: RedisService,
|
||||
@InjectKysely() private readonly db: KyselyDB,
|
||||
) {
|
||||
@@ -346,18 +354,60 @@ export class GitSyncOrchestrator {
|
||||
|
||||
// --- poll-safety interval (plan §10) -------------------------------------
|
||||
|
||||
/** Registered interval name (shared by registration + teardown). */
|
||||
private static readonly POLL_INTERVAL_NAME = 'git-sync-poll';
|
||||
|
||||
/**
|
||||
* Poll-safety loop: catches events missed by the listener and reconciles after
|
||||
* downtime. Gated on GIT_SYNC_ENABLED. The interval is a fixed value because
|
||||
* `@Interval` cannot read config at class-eval time — the body short-circuits
|
||||
* when disabled. Each enabled space runs under its own lock (overlaps skipped).
|
||||
* Register the poll-safety interval DYNAMICALLY so it honors the configured
|
||||
* GIT_SYNC_POLL_INTERVAL_MS (a static `@Interval` decorator could only hardcode
|
||||
* a value at class-eval time, before config is readable — diverging from what
|
||||
* `/status` reports). When git-sync is disabled we register nothing.
|
||||
*
|
||||
* ScheduleModule: registered ONCE globally by TelemetryModule
|
||||
* (ScheduleModule.forRoot()); GitSyncModule imports the plain ScheduleModule so
|
||||
* @Interval is discovered without a duplicate forRoot (plan §6 note).
|
||||
* ScheduleModule: forRoot() is registered ONCE globally by TelemetryModule;
|
||||
* GitSyncModule imports the plain ScheduleModule so SchedulerRegistry is
|
||||
* injectable without a duplicate forRoot (plan §6 note).
|
||||
*/
|
||||
@Interval('git-sync-poll', 15000)
|
||||
async poll(): Promise<void> {
|
||||
onModuleInit(): void {
|
||||
if (!this.environmentService.isGitSyncEnabled()) return;
|
||||
|
||||
const ms = this.environmentService.getGitSyncPollIntervalMs();
|
||||
const handle = setInterval(() => {
|
||||
void this.pollTick();
|
||||
}, ms);
|
||||
// Do not keep the event loop alive solely for the poll timer.
|
||||
handle.unref?.();
|
||||
this.schedulerRegistry.addInterval(
|
||||
GitSyncOrchestrator.POLL_INTERVAL_NAME,
|
||||
handle,
|
||||
);
|
||||
this.pollIntervalName = GitSyncOrchestrator.POLL_INTERVAL_NAME;
|
||||
this.logger.log(`git-sync: poll interval registered (${ms}ms).`);
|
||||
}
|
||||
|
||||
/** Tear down the dynamic interval on shutdown (guard against double-delete). */
|
||||
onModuleDestroy(): void {
|
||||
if (!this.pollIntervalName) return;
|
||||
try {
|
||||
// deleteInterval clears the timer and removes it from the registry.
|
||||
this.schedulerRegistry.deleteInterval(this.pollIntervalName);
|
||||
} catch (err) {
|
||||
this.logger.warn(
|
||||
`git-sync: failed to delete poll interval: ${
|
||||
err instanceof Error ? err.message : String(err)
|
||||
}`,
|
||||
);
|
||||
} finally {
|
||||
this.pollIntervalName = null;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* One poll tick: catches events missed by the listener and reconciles after
|
||||
* downtime. Gated on GIT_SYNC_ENABLED (defensive — the interval is only
|
||||
* registered when enabled). Each enabled space runs under its own lock
|
||||
* (overlaps skipped). Never throws (runOnce swallows per-space errors).
|
||||
*/
|
||||
private async pollTick(): Promise<void> {
|
||||
if (!this.environmentService.isGitSyncEnabled()) return;
|
||||
let spaces: EnabledSpace[];
|
||||
try {
|
||||
|
||||
Reference in New Issue
Block a user