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:
claude code agent 227
2026-06-21 16:01:37 +03:00
parent b2f13aea93
commit bf23c3c82d
7 changed files with 155 additions and 46 deletions

View File

@@ -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 {