diff --git a/.gitignore b/.gitignore index f861804cf..674eed813 100644 --- a/.gitignore +++ b/.gitignore @@ -45,6 +45,9 @@ cypress/fixtures/ .idea/* .vscode/* +# Claude Code +.claude/ + # Configuration /**/app-config.json diff --git a/openspec/changes/fix-history-wipe-on-storage-error/.openspec.yaml b/openspec/changes/fix-history-wipe-on-storage-error/.openspec.yaml new file mode 100644 index 000000000..5aae5cfa4 --- /dev/null +++ b/openspec/changes/fix-history-wipe-on-storage-error/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-03-04 diff --git a/openspec/changes/fix-history-wipe-on-storage-error/design.md b/openspec/changes/fix-history-wipe-on-storage-error/design.md new file mode 100644 index 000000000..4715709e5 --- /dev/null +++ b/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(key: string): Promise { + try { + const value = await this.dataStore.get(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(key: string): Promise { + try { + const value = await this.dataStore.get(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` — 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. diff --git a/openspec/changes/fix-history-wipe-on-storage-error/proposal.md b/openspec/changes/fix-history-wipe-on-storage-error/proposal.md new file mode 100644 index 000000000..4f523e12a --- /dev/null +++ b/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. diff --git a/openspec/changes/fix-history-wipe-on-storage-error/specs/storage-error-handling/spec.md b/openspec/changes/fix-history-wipe-on-storage-error/specs/storage-error-handling/spec.md new file mode 100644 index 000000000..2650842c0 --- /dev/null +++ b/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` diff --git a/openspec/changes/fix-history-wipe-on-storage-error/tasks.md b/openspec/changes/fix-history-wipe-on-storage-error/tasks.md new file mode 100644 index 000000000..b37ec716a --- /dev/null +++ b/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 diff --git a/src/data/storage/StorageDataStoreClient.ts b/src/data/storage/StorageDataStoreClient.ts index 0e57b5a4f..ca41f1255 100644 --- a/src/data/storage/StorageDataStoreClient.ts +++ b/src/data/storage/StorageDataStoreClient.ts @@ -36,8 +36,10 @@ export class StorageDataStoreClient extends StorageClient { const value = await this.dataStore.get(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; } } } diff --git a/src/data/storage/__tests__/StorageDataStoreClient.spec.ts b/src/data/storage/__tests__/StorageDataStoreClient.spec.ts new file mode 100644 index 000000000..5b9b2dac2 --- /dev/null +++ b/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(promise: () => Promise): CancelableResponse { + return CancelableResponse.build({ + cancel: () => {}, + response: () => promise().then(data => ({ status: 200, data, headers: {} })), + }); +} + +function buildCancelableRejection(error: unknown): CancelableResponse { + 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 {};