refactor!: Extract service management from Configuration into ServiceLocator class

[janbuchar]

• closes Introduce a service locator for Crawlee component lifecycle management #3073

Review action items (comment)

• (1) bindMethodsToServiceLocator is fragile — The super call concern is incorrect — AsyncLocalStorage context propagates through the entire sync/async call tree, so prototype methods reached via super execute within the context established by the instance-level wrapper. Identity loss is theoretical — binding happens before any references are captured. We did fix Symbol-keyed methods (now iterates getOwnPropertySymbols too).
• (2) Per-crawler scoping inconsistent during construction — Fixed. After bindMethodsToServiceLocator, we now call enterScope() to enter the AsyncLocalStorage context for the rest of the constructor body, wrapped in a try/finally that calls exitScope(). exitScope restores the previous store value, so nested scoping (e.g. constructing a crawler inside another crawler's scope) works correctly.
• (3) Circular dependency in EventManager.init() — fixed by accepting persistStateIntervalMillis as a constructor param, added LocalEventManager.fromConfig() factory (aligned with crawlee-python)
• (4) Exported serviceLocator is a plain object, not a ServiceLocator instance — I don't perceive this as a problem
• (5) Configuration.set() removal — restored the method, removed from upgrading guide's "removed" list
• (6) StorageManager.getManager cache keying ignores config — fixed by removing the config parameter, first step of a process to be finished in Rework the storage client system #3075
• (7) Inconsistent log levels for implicit service creation — decided warning is appropriate for side-effect warnings, debug for default creation messages
• (8) clearStorageManagerCache uses fragile string matching — since this is going to be redone in Rework the storage client system #3075, we are leaving it unresolved
• (9) Cache serviceLocator.getConfiguration() in local variables — dismissed, AsyncLocalStorage.getStore() is trivial
• (10) Test bug: new LocalEventManager(crawlerConfig) passes unused arg — fixed
• (11) RequestQueue constructor requires config as mandatory — kept the change, documented in upgrading guide
• (12) FileDownload fix is unrelated to this PR — split into separate PR
• (13) Vitest setup comment about dynamic import — no longer needed after circular dep fix, switched to static import
• (14) JSDoc example on Configuration shows old BasicCrawler constructor signature — fixed
• EventManager/LocalEventManager constructor options and fromConfig factory documented in upgrading guide

[janbuchar]

@B4nan @barjin @l2ysho there is one failing test that I'm trying to fix, but feel free to review this in the meantime.

[l2ysho]

I left some nit (or rather question) related to cyclic dependency, otherwise it looks good. 🚀

[B4nan]

This is what claude things about it. As usual, take it with a grain of salt, but some of those things do sound valid to me. I am sure it will make more sense to you than me 🙃 I am particularly surprised by the removal of Configuration.set, I think we want to keep that, I wouldn't call the configuration immutable. If we wanna drop it, we should first get some feedback on how people use it. And if we really end up dropping it, this deserves a separate section in the upgrading guide, with an alternative approach.

  PR #3325 Review: Extract service management from Configuration into ServiceLocator

  Overall assessment: The direction is right — aligning with the Python Crawlee service locator pattern and separating concerns between Configuration (immutable settings) and ServiceLocator (service lifecycle). However, the implementation has several
  significant issues around the AsyncLocalStorage + method monkey-patching approach, inconsistent scoping semantics, and subtle correctness bugs.

  ---
  Critical Issues

  1. bindMethodsToServiceLocator is fragile and dangerous (service_locator.ts:291-319)

  This function walks the entire prototype chain and replaces every method on the crawler instance with an arrow-function wrapper that runs inside AsyncLocalStorage.run(). Problems:

  - Breaks super calls: Subclass methods that call super.methodName() will invoke the original unwrapped method on the prototype, not the wrapped one. This means any code reached via super escapes the AsyncLocalStorage context and falls back to the global
   service locator silently.
  - Doesn't handle Symbol-keyed methods: Object.getOwnPropertyNames skips Symbols.
  - Identity loss: Code that captures a method reference before bindMethodsToServiceLocator runs (e.g., const fn = this.someMethod in the constructor before the binding call) will hold a stale reference.
  - Extremely implicit: The entire per-crawler scoping mechanism relies on invisible runtime patching. A developer reading serviceLocator.getEventManager() in Snapshotter has no way to know which locator they'll get without understanding this binding
  machinery.

  Suggestion: Consider making the service locator an explicit instance property on crawlers (like this.services: ServiceLocator) and threading it through, rather than relying on AsyncLocalStorage ambient context. This is more verbose but explicit and
  debuggable. If you must use AsyncLocalStorage, scope it only to the run() call rather than every method.

  2. Per-crawler scoping is inconsistent

  The scoped ServiceLocator is created in BasicCrawler's constructor and bound to methods:

  // basic-crawler.ts:720-729
  if (storageClient || eventManager || (configuration !== undefined && ...)) {
      const scopedServiceLocator = new ServiceLocator(configuration, eventManager, storageClient);
      bindMethodsToServiceLocator(scopedServiceLocator, this);
  }

  But then in the same constructor body, serviceLocator.getEventManager() is still called bare (not through any scoped mechanism), because bindMethodsToServiceLocator runs on this — meaning the current constructor execution is NOT inside the
  AsyncLocalStorage context. The binding only applies to future method calls on the instance. So everything that happens during construction (e.g., SessionPool created in _init, Statistics constructor) will use the global locator, not the scoped one. This
   means the "per-crawler isolation" story has holes.

  3. Circular dependency workaround (event_manager.ts:40-42)

  // Lazy import to break circular dependency
  const { serviceLocator } = await import('../service_locator.js');

  Dynamic import() in a hot path (init()) is a code smell suggesting the dependency graph is wrong. The service locator depends on LocalEventManager, which extends EventManager, which now depends on the service locator. Consider passing the persist-state
  interval as a constructor parameter or having the service locator call eventManager.init(config) instead of having EventManager reach back to the service locator.

  ---
  Design Concerns

  4. The exported serviceLocator is not a ServiceLocator

  The exported singleton is a plain object conforming to ServiceLocatorInterface, not an instance of ServiceLocator. This means serviceLocator instanceof ServiceLocator is false, which may surprise users. The Python implementation exports an actual
  instance. Consider exporting a Proxy or an actual instance with the AsyncLocalStorage delegation.

  5. Configuration.set() removal leaves a gap

  The PR removes Configuration.set() (making Configuration immutable), and the migration guide says "make an updated copy." But there's no Configuration.clone() or copy constructor. And once a Configuration has been retrieved from the service locator, you
   can't replace it due to ServiceConflictError. Users who were doing Configuration.getGlobalConfig().set('headless', true) mid-program have no migration path.

  6. StorageManager.getManager lost config-based cache keying

  Previously, config.storageManagers meant each Configuration had its own set of storage managers. Now, serviceLocator.getStorageManager(constructor) is a single flat map. If a user creates two crawlers with different configurations + storage clients, the
   second crawler's StorageManager.getManager() call will return the first crawler's cached manager (since the config parameter is only used when creating a new manager, not for cache lookup). This breaks the per-crawler isolation that the PR claims to
  support.

  // storage_manager.ts:42-47
  static getManager<T extends IStorage>(storageClass: Constructor<T>, config?: Configuration): StorageManager<T> {
      let storageManager = serviceLocator.getStorageManager(storageClass);
      if (storageManager === undefined) {
          storageManager = new StorageManager(storageClass, config ?? serviceLocator.getConfiguration());
          serviceLocator.setStorageManager(storageClass, storageManager);
      }
      return serviceLocator.getStorageManager(storageClass) as StorageManager<T>;  // ignores `config`
  }

  ---
  Medium Issues

  7. Inconsistent log levels for implicit service creation

  - getConfiguration(): log.debug(...)
  - getEventManager(): log.debug(...)
  - getStorageClient(): log.warning(...) for the config side-effect

  Pick one. I'd suggest log.debug for all three since implicit creation is the expected default path.

  8. clearStorageManagerCache uses fragile string matching (service_locator.ts:257-265)

  if ((manager as any).name === 'KeyValueStore') {
      (manager as any).cache?.forEach((item: any) => {

  This casts to any and matches on a string name. If someone renames the class or the name property changes, this silently stops clearing KVS caches. Use instanceof or a method on the StorageManager interface.

  9. Repeated serviceLocator.getConfiguration() calls in hot paths

  Snapshotter.start() calls serviceLocator.getConfiguration() three times. Each call goes through AsyncLocalStorage.getStore(). Cache it in a local variable:

  const config = serviceLocator.getConfiguration();
  const memoryMbytes = config.get('memoryMbytes', 0);
  // ... use config ...

  Same pattern appears in many files.

  10. Test bug: new LocalEventManager(crawlerConfig) (service_locator.test.ts:108)

  const crawlerEvents = new LocalEventManager(crawlerConfig);

  EventManager's constructor was changed to constructor() (no params). This extra argument is silently ignored — the event manager is NOT associated with crawlerConfig. The test passes but doesn't test what it claims.

  11. RequestQueue constructor now requires config as mandatory second arg

  Every direct new RequestQueue(...) call in tests had to add , serviceLocator.getConfiguration(). This is a public API regression — if external users construct RequestQueue directly, their code breaks with no default.

  ---
  Minor / Nits

  12. The FileDownload fix (file-download.ts:167-176) is unrelated to the service locator refactor — should be a separate commit/PR.

  13. The vitest setup file (test/vitest.setup.ts) uses dynamic import to avoid circular deps, but this means the reset() happens asynchronously. Since beforeEach awaits the async function this works, but the comment should note why dynamic import is
  needed.

  14. The upgrading guide is well-written and comprehensive — nice work there.

  ---
  Summary of Recommendations
  ┌──────────┬──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┐
  │ Priority │                                                                Action                                                                │
  ├──────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ P0       │ Rethink bindMethodsToServiceLocator — the prototype-patching approach has correctness issues with super calls and constructor timing │
  ├──────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ P0       │ Fix StorageManager.getManager cache keying to actually support per-crawler isolation                                                 │
  ├──────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ P1       │ Address circular dependency properly instead of dynamic import workaround                                                            │
  ├──────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ P1       │ Provide a migration path for Configuration.set() users (copy constructor or builder)                                                 │
  ├──────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ P1       │ Make RequestQueue/RequestQueueV2 config param optional with a default                                                                │
  ├──────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ P2       │ Fix test for new LocalEventManager(crawlerConfig) — arg is silently dropped                                                          │
  ├──────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ P2       │ Split the FileDownload fix into a separate PR                                                                                        │
  ├──────────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┤
  │ P2       │ Cache serviceLocator.getConfiguration() in local variables instead of calling repeatedly                                             │
  └──────────┴──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────┘

[janbuchar]

This is what claude things about it. As usual, take it with a grain of salt, but some of those things do sound valid to me. I am sure it will make more sense to you than me 🙃

Right, I tried to knock out the easy ones first.

I am particularly surprised by the removal of Configuration.set, I think we want to keep that, I wouldn't call the configuration immutable. If we wanna drop it, we should first get some feedback on how people use it. And if we really end up dropping it, this deserves a separate section in the upgrading guide, with an alternative approach.

Yeah, let's discuss that some other time, other place.

5. Configuration.set() removal leaves a gap

returned

7. Inconsistent log levels for implicit service creation

• getConfiguration(): log.debug(...)
• getEventManager(): log.debug(...)
• getStorageClient(): log.warning(...) for the config side-effect

Pick one. I'd suggest log.debug for all three since implicit creation is the expected default path.

Warning it is.

9. Repeated serviceLocator.getConfiguration() calls in hot paths

Snapshotter.start() calls serviceLocator.getConfiguration() three times. Each call goes through AsyncLocalStorage.getStore(). Cache it in a local variable:

const config = serviceLocator.getConfiguration();
const memoryMbytes = config.get('memoryMbytes', 0);
// ... use config ...

Same pattern appears in many files.

Most of those only execute once during the entire crawler lifetime, hardly a hot path. A local variable would just introduce noise.

10. Test bug: new LocalEventManager(crawlerConfig) (service_locator.test.ts:108)

const crawlerEvents = new LocalEventManager(crawlerConfig);

EventManager's constructor was changed to constructor() (no params). This extra argument is silently ignored — the event manager is NOT associated with crawlerConfig. The test passes but doesn't test what it claims.

Fair, removed the argument.

12. The FileDownload fix (file-download.ts:167-176) is unrelated to the service locator refactor — should be a separate commit/PR.

#3411

13. The vitest setup file (test/vitest.setup.ts) uses dynamic import to avoid circular deps, but this means the reset() happens asynchronously. Since beforeEach awaits the async function this works, but the comment should note why dynamic import is
    needed.

Added a comment.

...other half coming soon.

[janbuchar]

@B4nan I resolved the comments, feel free to check again

[barjin]

The changes are mostly fine by me, below there are some questions. Please forgive me if some questions are random (and feel free to skip those), it took me some time to get my head wrapped around this.

[barjin]

I see this has been discussed in the Claude comment above (and RequestQueue.open() being more popular is a valid argument by me), but what exactly is the reason for this change? Why cannot RequestQueue pull the Configuration instance from the serviceLocator on its own?

[janbuchar]

Apparently it's not a problem anymore - I made it so in a previous attempt to resolve an import cycle. Thanks!

[barjin]

Why we get to overwrite individual storage managers, but overriding the other services fails with an error?

[janbuchar]

Good point, updated

[barjin]

Why doesn't this use serviceLocator.getStorageManager?

crawlee/packages/core/src/service_locator.ts

Lines 213 to 215 in 0383a31

	getStorageManager(constructor: Constructor<IStorage>): StorageManager | undefined {
	return this.storageManagers.get(constructor);
	}

[janbuchar]

I know this is confusing, but StorageManager still has the functionality it had before, only the "cache" got moved to ServiceLocator. This will be fully resolved in #3075.

[barjin]

this is no longer necessarily the "global" config, right?

const globalConfig = new Configuration();
const localConfig = new Configuration();

serviceLocator.setConfiguration(globalConfig);

const crawler = new CheerioCrawler({
    requestHandler: async () => {
        const config = Configuration.getGlobalConfig();

        /// !!! this will fail
        assert(config === globalConfig, "Configuration.getGlobalConfig is not returning the global config");
    },
    configuration: localConfig
});

Isn't this an (unexpected) BC for users? Is this intentional?

[janbuchar]

Yes. I added a note about this to the upgrading guide. The situation is the same on the Python side, but I agree that the name is not really... fitting anymore.

[barjin]

lgtm, thank you for the quick changes (and explanation) 👍

---

the conflicts with #3388 will be fun 🙃

[janbuchar]

the conflicts with #3388 will be fun 🙃

sad to hear it, let me know if I can help