fix(storage): prevent history wipe on transient dataStore errors

.gitignore

@@ -45,6 +45,9 @@ cypress/fixtures/
 .idea/*
 .vscode/*
 
+# Claude Code
+.claude/
+
 # Configuration
 /**/app-config.json
 

openspec/changes/fix-history-wipe-on-storage-error/.openspec.yaml

@@ -0,0 +1,2 @@
+schema: spec-driven
+created: 2026-03-04

openspec/changes/fix-history-wipe-on-storage-error/design.md

@@ -0,0 +1,77 @@
+## Context
+
+The `StorageDataStoreClient.getObject()` method wraps DHIS2 dataStore GET calls. Currently it catches all errors and returns `undefined`:
+
+```typescript
+public async getObject<T extends object>(key: string): Promise<T | undefined> {
+    try {
+        const value = await this.dataStore.get<T>(key).getData();
+        return value;
+    } catch (error: any) {
+        console.error(error);
+        return undefined;  // 404 AND network errors both return undefined
+    }
+}
+```
+
+All collection mutation methods in the abstract `StorageClient` base class use the pattern `(await this.getObject(key)) ?? []` to read existing data before writing. When `getObject` returns `undefined` due to a transient error, the collection is treated as empty and overwritten.
+
+This affects history directly: every `reports.save()` call during a sync and every `useDeleteHistory` run on app load go through `saveObjectInCollection("history", ...)`.
+
+## Goals / Non-Goals
+
+**Goals:**
+- Prevent history wipe caused by transient dataStore read errors
+- Distinguish "key not found" (404) from "request failed" (network/5xx) in `getObject`
+- Ensure collection mutation methods propagate read errors instead of treating them as empty collections
+
+**Non-Goals:**
+- Adding retry logic to dataStore operations (can be a follow-up)
+- Fixing the scheduler race condition where concurrent syncs lose each other's reports (separate concern, requires locking or append-only writes)
+- Migrating all callers from `getObject` to `getObjectFuture` (ongoing refactor, out of scope)
+
+## Decisions
+
+### 1. Make `getObject` only return `undefined` for 404, re-throw other errors
+
+**Change**: In `StorageDataStoreClient.getObject()`, inspect the error response status. Return `undefined` only when the status is 404 (key not found). For all other errors (network, 5xx, timeout), re-throw.
+
+```typescript
+public async getObject<T extends object>(key: string): Promise<T | undefined> {
+    try {
+        const value = await this.dataStore.get<T>(key).getData();
+        return value;
+    } catch (error: any) {
+        if (error?.response?.status === 404) {
+            return undefined;
+        }
+        throw error;
+    }
+}
+```
+
+**Rationale**: This is the minimal change that fixes the bug. Callers that handle `undefined` for missing keys continue working. Callers that assumed errors were silently swallowed will now see exceptions — which is the correct behavior since they were unknowingly losing data.
+
+**Alternatives considered:**
+- Adding a separate `getObjectOrThrow` method — adds API surface without fixing the root cause; all existing callers would still silently swallow.
+- Returning a `Result<T | undefined, Error>` — good long-term but too invasive for a bug fix; the codebase is already migrating to Futures.
+
+### 2. Apply the same fix to `StorageConstantClient`
+
+**Change**: Check if `StorageConstantClient.getObject()` has the same silent-catch pattern and fix it consistently.
+
+**Rationale**: Both storage backends must behave the same way to prevent the bug regardless of which backend is configured.
+
+### 3. No change to collection mutation methods
+
+**Rationale**: Once `getObject` properly throws on transient errors, the `?? []` fallback in `saveObjectInCollection` etc. will only trigger for genuinely missing keys (404 → `undefined`). The error will naturally propagate up through the `await this.getObject(key)` call. No changes needed in `StorageClient.ts`.
+
+### 4. Apply same fix to `getOrCreateObject`
+
+**Change**: `getOrCreateObject` calls `getObject` and creates a default if it returns `undefined`. With the fix, a transient error will throw instead of creating a spurious default. This is correct behavior — we should not create a default object because the network was flaky.
+
+## Risks / Trade-offs
+
+- **[Medium] Callers that relied on silent error swallowing** → Some callers may have implicitly depended on `getObject` never throwing. These will now see unhandled exceptions. This is intentional — it surfaces bugs rather than silently losing data. Each caller should handle errors appropriately (show user message, retry, etc.).
+- **[Low] 404 detection heuristic** → The d2-api library wraps errors with a `response` object. If the error shape changes in future d2-api versions, the 404 check may need updating. This risk is low since d2-api follows standard axios error conventions.
+- **[Low] `getMetadataByKey` in StorageDataStoreClient** → This private method also silently catches errors and returns `undefined`. It should get the same treatment for consistency but is lower priority since it's only used for sharing operations, not collection mutations.

openspec/changes/fix-history-wipe-on-storage-error/proposal.md

@@ -0,0 +1,24 @@
+## Why
+
+Sync history in the DEV environment is occasionally getting completely wiped. The root cause is that `StorageDataStoreClient.getObject()` catches **all** errors (including network failures, 5xx, timeouts) and silently returns `undefined`. All collection mutation methods (`saveObjectInCollection`, `removeObjectsInCollection`, etc.) treat `undefined` as "empty collection" via `(await this.getObject(key)) ?? []`, then overwrite the dataStore key with the result. A single transient network error during any sync report save or history cleanup wipes the entire history.
+
+## What Changes
+
+- **Distinguish 404 from transient errors in `StorageDataStoreClient.getObject()`**: Return `undefined` only for 404 (key genuinely does not exist). Re-throw on network errors, 5xx, timeouts, etc.
+- **Add error propagation in collection mutation methods**: `saveObjectInCollection`, `saveObjectsInCollection`, `removeObjectInCollection`, and `removeObjectsInCollection` in `StorageClient` must not silently treat a failed read as an empty collection. If `getObject` throws, the error must propagate up rather than being swallowed into `?? []`.
+
+## Capabilities
+
+### New Capabilities
+
+_(none — this is a bug fix to existing storage behavior)_
+
+### Modified Capabilities
+
+_(no spec-level requirement changes — the storage client was always supposed to preserve data on transient errors)_
+
+## Impact
+
+- **Code**: `src/data/storage/StorageDataStoreClient.ts` (error handling in `getObject`), `src/domain/storage/repositories/StorageClient.ts` (collection mutation methods)
+- **User impact**: Sync history will no longer be wiped by transient network errors. Users will see error messages instead of silent data loss.
+- **Risk**: Medium — `getObject` is used widely. The change must carefully distinguish 404 from other errors to avoid breaking callers that legitimately expect `undefined` for missing keys. Other `StorageClient` implementations (`StorageConstantClient`) need the same treatment.

openspec/changes/fix-history-wipe-on-storage-error/specs/storage-error-handling/spec.md

@@ -0,0 +1,39 @@
+## ADDED Requirements
+
+### Requirement: StorageClient.getObject SHALL distinguish missing keys from transient errors
+
+When `getObject` is called and the dataStore key does not exist (HTTP 404), the method MUST return `undefined`. When `getObject` is called and the request fails for any other reason (network error, HTTP 5xx, timeout), the method MUST throw an error rather than returning `undefined`.
+
+#### Scenario: Missing key returns undefined
+- **WHEN** `getObject("nonexistent-key")` is called and the dataStore returns HTTP 404
+- **THEN** the method SHALL return `undefined`
+
+#### Scenario: Network error throws instead of returning undefined
+- **WHEN** `getObject("history")` is called and the dataStore request fails with a network error
+- **THEN** the method SHALL throw an error
+
+#### Scenario: Server error throws instead of returning undefined
+- **WHEN** `getObject("history")` is called and the dataStore returns HTTP 500
+- **THEN** the method SHALL throw an error
+
+### Requirement: Collection mutation methods SHALL NOT overwrite data on read failure
+
+When `saveObjectInCollection`, `saveObjectsInCollection`, `removeObjectInCollection`, or `removeObjectsInCollection` fail to read the existing collection due to a transient error, the method MUST propagate the error rather than treating the collection as empty and overwriting it.
+
+#### Scenario: saveObjectInCollection propagates read error
+- **WHEN** `saveObjectInCollection("history", newEntry)` is called
+- **AND** the read of existing history fails with a transient error
+- **THEN** the method SHALL throw an error without modifying the dataStore
+
+#### Scenario: removeObjectsInCollection propagates read error
+- **WHEN** `removeObjectsInCollection("history", ids)` is called
+- **AND** the read of existing history fails with a transient error
+- **THEN** the method SHALL throw an error without modifying the dataStore
+
+### Requirement: Error handling SHALL be consistent across storage backends
+
+Both `StorageDataStoreClient` and `StorageConstantClient` MUST implement the same error-handling semantics for `getObject`: return `undefined` only for missing keys, throw for transient errors.
+
+#### Scenario: StorageConstantClient follows same error contract
+- **WHEN** `StorageConstantClient.getObject()` encounters a non-404 error
+- **THEN** the method SHALL throw an error rather than returning `undefined`

openspec/changes/fix-history-wipe-on-storage-error/tasks.md

@@ -0,0 +1,18 @@
+## 1. Fix error handling in StorageDataStoreClient.getObject
+
+- [ ] 1.1 [BE] In `src/data/storage/StorageDataStoreClient.ts`, modify `getObject()` to only return `undefined` for 404 errors and re-throw all other errors (network, 5xx, timeout)
+- [ ] 1.2 [BE] Apply the same fix to `getMetadataByKey()` in the same file for consistency
+
+## 2. Fix error handling in StorageConstantClient
+
+- [ ] 2.1 [BE] Check `StorageConstantClient.getObject()` for the same silent-catch pattern and apply the same 404-only-undefined fix
+
+## 3. Unit Tests
+
+- [ ] 3.1 [BE] Add unit tests for `StorageDataStoreClient.getObject()` verifying: returns undefined on 404, throws on 500, throws on network error
+- [ ] 3.2 [BE] Add unit test verifying `saveObjectInCollection` propagates the error when `getObject` throws (does not overwrite with empty data)
+- [ ] 3.3 [BE] Run `yarn test` to confirm all existing tests pass
+
+## 4. Validation
+
+- [ ] 4.1 [BE] Run `yarn test` and `yarn build core-app` to confirm everything compiles and passes

src/data/storage/StorageDataStoreClient.ts

@@ -36,8 +36,10 @@ export class StorageDataStoreClient extends StorageClient {
             const value = await this.dataStore.get<T>(key).getData();
             return value;
         } catch (error: any) {
-            console.error(error);
-            return undefined;
+            if (error?.response?.status === 404) {
+                return undefined;
+            }
+            throw error;
         }
     }
 
@@ -137,8 +139,10 @@ export class StorageDataStoreClient extends StorageClient {
 
             return data;
         } catch (error: any) {
-            console.error(error);
-            return undefined;
+            if (error?.response?.status === 404) {
+                return undefined;
+            }
+            throw error;
         }
     }
 }

src/data/storage/__tests__/StorageDataStoreClient.spec.ts

@@ -0,0 +1,110 @@
+import { CancelableResponse } from "@eyeseetea/d2-api";
+import { Instance } from "../../../domain/instance/entities/Instance";
+import { StorageDataStoreClient } from "../StorageDataStoreClient";
+
+function buildClient() {
+    const instance = Instance.build({
+        url: "http://test.example.com",
+        name: "Test Instance",
+        version: "2.36",
+    });
+
+    const client = new StorageDataStoreClient(instance);
+
+    // Replace the private dataStore with a mock to avoid real HTTP calls
+    const mockDataStore = {
+        get: vi.fn(),
+        getMetadata: vi.fn(),
+        save: vi.fn(),
+        delete: vi.fn(),
+        getKeys: vi.fn(),
+    };
+
+    (client as any).dataStore = mockDataStore;
+
+    return { client, mockDataStore };
+}
+
+function buildCancelableResponse<T>(promise: () => Promise<T>): CancelableResponse<T> {
+    return CancelableResponse.build({
+        cancel: () => {},
+        response: () => promise().then(data => ({ status: 200, data, headers: {} })),
+    });
+}
+
+function buildCancelableRejection<T>(error: unknown): CancelableResponse<T> {
+    return CancelableResponse.build({
+        cancel: () => {},
+        response: () => Promise.reject(error),
+    });
+}
+
+describe("StorageDataStoreClient", () => {
+    describe("getObject", () => {
+        it("returns the value when the dataStore key exists", async () => {
+            const { client, mockDataStore } = buildClient();
+            const expected = { foo: "bar" };
+
+            mockDataStore.get.mockReturnValue(buildCancelableResponse(() => Promise.resolve(expected)));
+
+            const result = await client.getObject("test-key");
+
+            expect(result).toEqual(expected);
+            expect(mockDataStore.get).toHaveBeenCalledWith("test-key");
+        });
+
+        it("returns undefined when the dataStore responds with 404", async () => {
+            const { client, mockDataStore } = buildClient();
+
+            const notFoundError = { response: { status: 404 }, message: "Not Found" };
+            mockDataStore.get.mockReturnValue(buildCancelableRejection(notFoundError));
+
+            const result = await client.getObject("missing-key");
+
+            expect(result).toBeUndefined();
+        });
+
+        it("throws when the dataStore responds with 500", async () => {
+            const { client, mockDataStore } = buildClient();
+
+            const serverError = { response: { status: 500 }, message: "Internal Server Error" };
+            mockDataStore.get.mockReturnValue(buildCancelableRejection(serverError));
+
+            await expect(client.getObject("some-key")).rejects.toEqual(serverError);
+        });
+
+        it("throws when a network error occurs", async () => {
+            const { client, mockDataStore } = buildClient();
+
+            const networkError = new Error("Network Error");
+            mockDataStore.get.mockReturnValue(buildCancelableRejection(networkError));
+
+            await expect(client.getObject("some-key")).rejects.toThrow("Network Error");
+        });
+
+        it("throws when a timeout error occurs", async () => {
+            const { client, mockDataStore } = buildClient();
+
+            const timeoutError = new Error("timeout of 30000ms exceeded");
+            mockDataStore.get.mockReturnValue(buildCancelableRejection(timeoutError));
+
+            await expect(client.getObject("some-key")).rejects.toThrow("timeout of 30000ms exceeded");
+        });
+    });
+
+    describe("saveObjectInCollection", () => {
+        it("propagates the error when getObject throws and does NOT overwrite data", async () => {
+            const { client, mockDataStore } = buildClient();
+
+            const serverError = { response: { status: 500 }, message: "Internal Server Error" };
+            mockDataStore.get.mockReturnValue(buildCancelableRejection(serverError));
+
+            await expect(client.saveObjectInCollection("rules", { id: "rule-1" })).rejects.toEqual(serverError);
+
+            // Verify save was never called -- data was NOT overwritten
+            expect(mockDataStore.save).not.toHaveBeenCalled();
+        });
+    });
+});
+
+export {};