Skip to content

Commit af06a65

Browse files
TheLastCicadaTheLastCicada
committed
fix: subscribe to governance stores before checking sync status
Bugbot flagged a permanent-skip loop in Governance.sync (both v1 and v2): getDataLayerStoreSyncStatus returns false for a store the DataLayer node has no record of, isDlStoreSynced(undefined) is false, sync() early-exits, and because sync() was the only place that called subscribeToStoreOnDataLayer (via getSubscribedStoreData), the store never got subscribed and the skip repeated forever on every scheduled run. Mirror the subscribe-first pattern already used in OrganizationsV2.reconcileOrganization: subscribe first (idempotent), then check sync status. Applied to both the governance body store and the versioned-governance store in both v1 and v2 models. subscribeToStoreOnDataLayer returns false on the common datalayer-unreachable / RPC-failure paths (it does not throw), so guard on both a falsy return and thrown exceptions. Without the falsy-return check, the dominant failure mode would slip past the try/catch and the subsequent status-check skip would fire with a misleading log. Adds tests/v2/integration/governance-sync-subscribe-first.spec.js covering five branches of GovernanceV2.sync: - subscribeToStoreOnDataLayer runs before getDataLayerStoreSyncStatus on the body store - sync() returns cleanly when the body-store subscribe throws - sync() returns cleanly when the body-store subscribe returns falsy - sync() subscribes to the versioned store before its status check (call order: body-subscribe -> body-status -> body-fetch -> versioned-subscribe -> versioned-status), and does not upsert when the versioned store is unsynced - sync() returns cleanly when the versioned-store subscribe returns falsy The v1 model captures USE_SIMULATOR at module import time so the same test pattern cannot reach its production path; the v1 change is structurally identical to v2 and is reviewed by code reading.
1 parent 6e76235 commit af06a65

3 files changed

Lines changed: 413 additions & 0 deletions

File tree

‎src/models/governance/governance.model.js‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -175,6 +175,28 @@ class Governance extends Model {
175175
return;
176176
}
177177

178+
// Subscribe to the governance body store first so it begins syncing on
179+
// fresh deployments. Without this, an unsubscribed store would cause
180+
// getDataLayerStoreSyncStatus to error (the DL node has no record of
181+
// it), we'd skip, and no subsequent run would ever subscribe — a
182+
// permanent skip loop. subscribeToStoreOnDataLayer returns falsy on
183+
// failure (datalayer unreachable, subscribe RPC failure) and also
184+
// throws on unexpected errors, so we guard against both.
185+
try {
186+
const subscribed = await datalayer.subscribeToStoreOnDataLayer(GOVERNANCE_BODY_ID);
187+
if (!subscribed) {
188+
logger.warn(
189+
`[v1]: could not subscribe to governance body store ${GOVERNANCE_BODY_ID}. Skipping sync, will retry on next task run.`,
190+
);
191+
return;
192+
}
193+
} catch (error) {
194+
logger.warn(
195+
`[v1]: could not subscribe to governance body store ${GOVERNANCE_BODY_ID}: ${error.message}. Skipping sync, will retry on next task run.`,
196+
);
197+
return;
198+
}
199+
178200
// Check governance body store sync status before any blocking fetch.
179201
// If the store is not yet synced, return immediately so cached governance
180202
// data remains usable and the next task run retries.
@@ -224,6 +246,24 @@ class Governance extends Model {
224246
return;
225247
}
226248

249+
// Subscribe to the versioned governance store first (its ID is only
250+
// discovered by reading the body store above), then check sync status.
251+
// Same reasoning as the body-store subscribe above.
252+
try {
253+
const versionedSubscribed = await datalayer.subscribeToStoreOnDataLayer(versionedGovernanceStoreId);
254+
if (!versionedSubscribed) {
255+
logger.warn(
256+
`[v1]: could not subscribe to versioned governance store ${versionedGovernanceStoreId}. Skipping sync, will retry on next task run.`,
257+
);
258+
return;
259+
}
260+
} catch (error) {
261+
logger.warn(
262+
`[v1]: could not subscribe to versioned governance store ${versionedGovernanceStoreId}: ${error.message}. Skipping sync, will retry on next task run.`,
263+
);
264+
return;
265+
}
266+
227267
// Check versioned governance store sync status before fetching
228268
try {
229269
const versionedSyncStatus = await datalayer.getDataLayerStoreSyncStatus(versionedGovernanceStoreId);

‎src/models/v2/governance-v2.model.js‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -454,6 +454,28 @@ class GovernanceV2 extends Model {
454454
return;
455455
}
456456

457+
// Subscribe to the governance body store first so it begins syncing on
458+
// fresh deployments. Without this, an unsubscribed store would cause
459+
// getDataLayerStoreSyncStatus to error (the DL node has no record of
460+
// it), we'd skip, and no subsequent run would ever subscribe — a
461+
// permanent skip loop. subscribeToStoreOnDataLayer returns falsy on
462+
// failure (datalayer unreachable, subscribe RPC failure) and also
463+
// throws on unexpected errors, so we guard against both.
464+
try {
465+
const subscribed = await datalayer.subscribeToStoreOnDataLayer(GOVERNANCE_BODY_ID);
466+
if (!subscribed) {
467+
loggerV2.warn(
468+
`[v2]: could not subscribe to governance body store ${GOVERNANCE_BODY_ID}. Skipping sync, will retry on next task run.`,
469+
);
470+
return;
471+
}
472+
} catch (error) {
473+
loggerV2.warn(
474+
`[v2]: could not subscribe to governance body store ${GOVERNANCE_BODY_ID}: ${error.message}. Skipping sync, will retry on next task run.`,
475+
);
476+
return;
477+
}
478+
457479
// Check governance body store sync status before any blocking fetch.
458480
// If the store is not yet synced, return immediately so cached governance
459481
// data remains usable and the next task run retries.
@@ -505,6 +527,24 @@ class GovernanceV2 extends Model {
505527
return;
506528
}
507529

530+
// Subscribe to the versioned governance store first (its ID is only
531+
// discovered by reading the body store above), then check sync status.
532+
// Same reasoning as the body-store subscribe above.
533+
try {
534+
const versionedSubscribed = await datalayer.subscribeToStoreOnDataLayer(versionedGovernanceStoreId);
535+
if (!versionedSubscribed) {
536+
loggerV2.warn(
537+
`[v2]: could not subscribe to versioned governance store ${versionedGovernanceStoreId}. Skipping sync, will retry on next task run.`,
538+
);
539+
return;
540+
}
541+
} catch (error) {
542+
loggerV2.warn(
543+
`[v2]: could not subscribe to versioned governance store ${versionedGovernanceStoreId}: ${error.message}. Skipping sync, will retry on next task run.`,
544+
);
545+
return;
546+
}
547+
508548
// Check versioned governance store sync status before fetching
509549
try {
510550
const versionedSyncStatus = await datalayer.getDataLayerStoreSyncStatus(versionedGovernanceStoreId);

0 commit comments

Comments
 (0)