fix: lazy persistence #30

gullmar · 2025-12-07T23:22:48Z

Make storage persistence lazy, instead of aggressive: values will be persisted periodically, instead of immediately on update.

This will make the stored values possibly outdated in case of crash or abrupt abort.
On the other hand, this aligns the behavior with the Apify SDK (useAutoSavedValue method), and allows having synchronous tracking updates, making it easier to extract and isolate tracking logic in a future refactor. It should, presumably, also reduce the number of API calls and improve the performance, with an extra improvement when using encryption, because data is encrypted only when persisted.

Refactor:

The Tracker was split into multiple files.
Fixed spelling: RunsTracker → RunTracker.
Encryption logic was isolated.
Storage logic was extracted from trackers.
OrchestratorContext was replaced by GlobalContext, but the former will be removed in a future refactor.
Apply the same conventions used for tracking and storage to logging: use "build" instead of "generate", and pass the whole OrchestratorOptions for simplicity.

Test runs:

Test run without persistence: https://console.apify.com/view/runs/0WKm6de1rOtoeh9BV
Test run with persistence and sensitive information visible: https://console.apify.com/view/runs/0iqvhyD4ANUyx1duw
Test run with persistence and encryption: https://console.apify.com/view/runs/MxOXlxHXgvotnAcbk
Resurrected run: https://console.apify.com/view/runs/RCSe6XeWMW3CUISXN
Resurrected run with encryption: https://console.apify.com/view/runs/qcO7x9uxPv8x5mR36

Copilot

Pull request overview

This PR refactors the orchestrator's persistence layer to use lazy/periodic persistence instead of immediate persistence, aligning with Apify SDK's useAutoSavedValue behavior. This change enables synchronous tracking updates and improves performance at the cost of potentially outdated values in case of crashes.

Key changes:

Refactored tracking logic from a monolithic RunsTracker to separate CurrentRunTracker, FailedRunHistoryTracker, and RunTracker classes
Extracted encryption logic into a dedicated utils/encryption.ts module
Implemented lazy persistence via Actor.on('persistState') event in EncryptedKeyValueStore

Reviewed changes

Copilot reviewed 28 out of 28 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`src/utils/encryption.ts`	New module isolating encryption/decryption functions with `EncryptionKey` interface
`src/utils/key-value-store.ts`	Refactored to `EncryptedKeyValueStore` class with lazy persistence via persistState event
`src/utils/persist.ts`	Removed - persistence logic absorbed by new key-value-store implementation
`src/tracking/run-tracker.ts`	Main tracker orchestrating current and failed run tracking
`src/tracking/current-run-tracker.ts`	Manages active run state with in-memory updates
`src/tracking/failed-run-history-tracker.ts`	Tracks history of failed runs
`src/tracking/builder.ts`	Factory for constructing tracker instances with appropriate storage backends
`src/utils/context.ts`	Added `GlobalContext` interface, renamed `runsTracker` to `runTracker`
`src/tracker.ts`	Removed - split into multiple tracking modules
`src/index.ts`	Updated to use `buildRunTrackerForOrchestrator` and new context structure
`src/clients/*.ts`	Updated to use `runTracker` instead of `runsTracker` with synchronous updates
`test/utils/encryption.test.ts`	New tests for encryption utilities
`test/utils/key-value-store.test.ts`	Rewritten tests for `EncryptedKeyValueStore` class
`test/tracking/*.test.ts`	New test files for split tracker components
`test/_helpers/*.ts`	New test helpers for consistent test setup
`test/clients/*.test.ts`	Updated to use new test helpers and `runTracker`

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/utils/key-value-store.ts

test/utils/key-value-store.test.ts

gullmar · 2025-12-08T09:44:35Z

src/utils/context.ts

+/**
+ * @deprecated `runTracker` should not be in the global context, because there is one tracker per Apify client.
+ * TODO: Remove or replace.
+ */
 export interface OrchestratorContext {
    logger: Logger;
-    runsTracker: RunsTracker;
+    runTracker: RunTracker;
+}


I plan to get rid of this in the next refactor, when I will change the scheduling logic, and I'll need to touch all the clients that use the OrchestratorContext.

Copilot

Pull request overview

Copilot reviewed 35 out of 35 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/tracking/current-run-tracker.ts

src/utils/storage.ts

test/utils/key-value-store.test.ts

src/utils/key-value-store.ts

halvko

I may be missing something, but to me it seems like the storage is needlessly complicated - we've agreed to talk about it tomorrow 😌

halvko · 2025-12-08T16:05:20Z

src/utils/key-value-store.ts

+        const value = await this.getValue<T>(key, defaultValue);

-    return JSON.parse(Buffer.from(decrypted).toString()) as T;
-}
+        // Check the cache again to avoid creating multiple states for the same key in concurrent scenarios.
+        cachedValue = this.cache.get(key) as T;
+        if (cachedValue) return cachedValue;

-class EncryptedKeyValueStore extends KeyValueStore {
-    private cryptSecret: string;
-    protected kvStore: KeyValueStore;
+        this.cache.set(key, value);


I would probably prefer to do a lock/signal here

I replaced the double cache check with pending promises in this commit: 8ac9fad

src/utils/key-value-store.ts

src/tracking/builder.ts

halvko

I must admit I went over the test code a little bit quickly, but all looks good to me

halvko · 2025-12-16T14:58:02Z

src/utils/key-value-store.ts

+        private readonly logger: Logger,
+        private readonly encryptionKey: EncryptionKey,
+    ) {
+        Actor.on('persistState', this.persistCache.bind(this));


I'm surprised this works 😅 especially since as far as I can see persistCache doesn't take any arguments?

halvko · 2025-12-16T14:59:37Z

src/utils/key-value-store.ts

+        if (cachedValue) return cachedValue;
+
+        const pendingOperation = this.pendingOperations.get(key) as Promise<T> | undefined;
+        if (pendingOperation) return await pendingOperation;


Is it ok to await the same promise multiple times like this?

halvko · 2025-12-16T15:03:12Z

src/utils/key-value-store.ts

+        const pendingOperation = this.pendingOperations.get(key) as Promise<T> | undefined;
+        if (pendingOperation) return await pendingOperation;
+
+        const operation = this.getValue<T>(key, defaultValue)
+            .then((value) => {
+                this.cache.set(key, value);
+                this.pendingOperations.delete(key);
+                return value;
+            })
+            .catch((error) => {
+                this.pendingOperations.delete(key);
+                throw error;
+            });
+
+        this.pendingOperations.set(key, operation);


I would want to denote that there is a critical section here - any await point will lead to a race condition

halvko · 2025-12-16T15:12:12Z

src/run-tracker.ts

+        const defaultTrackedRuns = getDefaultTrackedRuns();
+        const storageKey = `${options?.storagePrefix ?? ''}${TRACKED_RUNS_KEY}`;
+        const trackedRuns =
+            (await options?.storage?.useState<TrackedRuns>(storageKey, defaultTrackedRuns)) ?? defaultTrackedRuns;


And we are guaranteed that it is the same underlying object which is returned, so even if we create two of these they will always be synchronized...

halvko · 2025-12-16T15:17:28Z

src/run-tracker.ts

+        return runInfo;
+    }
+
+    declareLostRun(runName: string, reason?: string) {


there is some naming here - why do does the function declaring end up deleting the run? But also, are we using the findAndDeleteRun anywhere else, cause otherwise it looks like exactly the same code except the two last lines of this function. I think it's better to merge them (unless I missed a call)

gullmar added 3 commits December 6, 2025 21:02

refactor(tracker): init signature

2850a27

fix(tracker): spelling

c133a2f

refactor: lazy persistence + split tracker

9171213

gullmar requested a review from Copilot December 7, 2025 23:22

gullmar self-assigned this Dec 7, 2025

Copilot started reviewing on behalf of gullmar December 7, 2025 23:23 View session

Copilot AI reviewed Dec 7, 2025

View reviewed changes

src/utils/key-value-store.ts Outdated Show resolved Hide resolved

test/utils/key-value-store.test.ts Outdated Show resolved Hide resolved

gullmar added 8 commits December 8, 2025 09:05

fix: apply suggestions

8fc828b

refactor: extract apify utils

e0443cd

refactor(run-tracker): do not expose currentRuns

ad1720c

fix(apify-client): improve run abort logs

a7da6b7

doc(tracker): explain failed run tracker building

f69ea99

refactor: extract storage logic and put it in global context

28b27b5

doc(context): mark OrchestratorContext as deprecated

0ba5259

refactor: align logging conventions to storage

b2a288c

gullmar commented Dec 8, 2025

View reviewed changes

gullmar requested a review from Copilot December 8, 2025 09:48

Copilot started reviewing on behalf of gullmar December 8, 2025 09:48 View session

Copilot AI reviewed Dec 8, 2025

View reviewed changes

gullmar added 6 commits December 8, 2025 11:44

fix(current-run-tracker): remove unused method

62b8bc9

doc: add reference to original kvs implementation

2518d7c

test(storage): dedicated test suite

dc59171

fix: key-value-store test mocks

788d332

fix(key-value-store): race condition

f716158

fix: relative imports

bfe45c2

gullmar marked this pull request as ready for review December 8, 2025 14:11

halvko reviewed Dec 8, 2025

View reviewed changes

gullmar added 3 commits December 9, 2025 11:58

refactor(run-tracker): simplify implementation

8cd12fa

refactor(run-tracker): simplify implementation

6e4d048

fix: store tracking data with prefix

728707c

gullmar added 3 commits December 11, 2025 17:04

chore: EncryptedKeyValueStore private attributes

dc871f9

feat: EncryptedKeyValueStore pending promises

77debe5

fix: relative import

503f05d

gullmar force-pushed the fix/lazy-persistence branch from a564749 to 503f05d Compare December 11, 2025 16:04

halvko approved these changes Dec 16, 2025

View reviewed changes

fix: lazy persistence #30

Are you sure you want to change the base?

fix: lazy persistence #30

Uh oh!

Conversation

gullmar commented Dec 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

gullmar Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

halvko left a comment

Choose a reason for hiding this comment

Uh oh!

halvko Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

gullmar Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

halvko left a comment

Choose a reason for hiding this comment

Uh oh!

halvko Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

halvko Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

halvko Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

halvko Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

halvko Dec 16, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gullmar commented Dec 7, 2025 •

edited

Loading

gullmar Dec 8, 2025 •

edited

Loading