restructure DI#2178
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
3 Skipped Deployments
|
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR restructures the DI bootstrapping in
Confidence Score: 5/5Safe to merge — the restructuring is well-scoped and the bootstrap shim correctly cleans up in a finally block, so there is no risk of leaked state at runtime. The change replaces fire-and-forget cache warming with an awaited Promise.all, which is strictly safer. The circular-dependency workaround in di-bootstrap.ts is temporary, clearly documented, and provably correct given JavaScript's single-threaded event loop — by the time any background revalidation timer fires, di._context is already assigned. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant index as index.ts
participant di as EnsApiDiContainer
participant bctx as buildEnsApiDiContext
participant boot as di-bootstrap
participant caches as SWR Caches
participant diboot as resolveDiDeps
index->>di: await di.init()
di->>bctx: await buildEnsApiDiContext(process.env)
bctx->>bctx: build ensApiConfig, ensDbConfig, ensDbClient
bctx->>boot: "setBootstrapDeps({ ensDbClient, ensApiConfig })"
bctx->>caches: await Promise.all([stackInfoCache.read(), ...])
caches->>diboot: resolveDiDeps()
diboot->>boot: getBootstrapDeps() returns deps
diboot-->>caches: "{ ensDbClient, ensApiConfig }"
caches-->>bctx: "cache results (T | Error)"
bctx->>bctx: peek stackInfoCache, extract namespace, build rootChain deps
bctx->>boot: clearBootstrapDeps() [finally]
bctx-->>di: EnsApiDiContext
di->>di: "this._context = Object.freeze(context)"
di-->>index: init complete
Reviews (2): Last reviewed commit: "dont use di.init() in config tests, dont..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| async init(): Promise<void> { | ||
| if (this._context) { | ||
| throw new Error( | ||
| "DI context has already been initialized. If you want to re-initialize, call `di.destroy()` first to clean up resources.", | ||
| ); | ||
| } | ||
|
|
||
| // Load the DI context | ||
| this.loadContext(); | ||
|
|
||
| logger.info("Initializing caches"); | ||
| void Promise.all([ | ||
| this.context.indexingStatusCache.read(), | ||
| this.context.stackInfoCache.read(), | ||
| this.context.referralProgramEditionConfigSetCache.read(), | ||
| ]).then(() => logger.info("Caches initialized")); | ||
| await this.loadContext(); | ||
| } |
There was a problem hiding this comment.
init() calls not guarded atomically
The guard if (this._context) in both init() and loadContext() is checked before the async buildEnsApiDiContext call completes, so two concurrent await di.init() calls (e.g., from tests or an unusual startup path) can both pass the guard simultaneously. Both would then call buildEnsApiDiContext concurrently, resulting in two sequential setBootstrapDeps / clearBootstrapDeps calls that interleave mid-Promise.all, where the second clearBootstrapDeps runs in a finally block during the first call's active cache reads. Setting a _pending: Promise<void> | undefined flag and short-circuiting on it would prevent this without changing the public API.
There was a problem hiding this comment.
just dont call init() concurrenty lol
2 participants
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
Why
Testing
Notes for Reviewer (Optional)
Pre-Review Checklist (Blocking)