fix(storage): persist deletions and clean up detail files in LargeDataStorage

[Duansg]

Related issue

Closes #2049

Summary

LargeDataStorage.delete() did not persist deletions completely: the index rewrite was skipped when the map became empty (deleted records resurrected on restart, reproducible on a fresh desktop install), and neither delete() nor capacity eviction removed the record's detail JSON, leaking the user's SQL to disk on every delete/eviction.

Minimal fix, no change to storage layout, public API, or callers:

• saveDataList() now writes an empty index when the map is empty.
• delete() removes the record's detail file via a new protected deleteDetailData(Long id) helper (best-effort, logged on failure, matching saveDataList error handling).
• The eviction branch in save() deletes the evicted record's detail file and drops a redundant dataMap.remove() (pollFirstEntry() already removes the entry).

Affected surfaces

• Frontend / Web
• Backend / API / Storage
• Database plugin / Driver
• JCEF / Desktop packaging
• CI / Build / Release
• Documentation only

Verification

• Commands and results:
  • mvn clean test -pl chat2db-community-storage -am -Dtest=LargeDataStorageTest -Dmaven.test.skip=false -DfailIfNoTests=false -Dsurefire.failIfNoSpecifiedTests=false
  • Same command against the previous implementation (fix stashed) → BUILD FAILURE: all three tests fail, confirming they capture the defects.
  • Manual verification: against a running dev backend, created a saved query via POST /api/operation/saved/create, deleted it via DELETE /api/operation/saved — index rewritten (including to an empty file), detail JSON removed from ~/.chat2db-community/dev/storage/console/, and the record no longer resurrects after backend restart. Before the fix, a fresh desktop install reproduced the resurrection (save one query, delete, restart).
  • UI evidence: N/A

Risk and compatibility

• Public API or stored data: file format unchanged; the only new writes are an empty index (previously left stale) and deletion of detail files that were previously orphaned. Startup already tolerates blank index lines.
• Database or driver compatibility: N/A (local JSON storage only).
• Network, privacy, or security: improves data hygiene — deleted SQL no longer remains on disk.
• Community / Local / Pro boundary: change is confined to chat2db-community-storage.
• Backward compatibility: pre-existing orphan files from old versions are inert (loading is index-driven) and need no migration.

Reviewer map

• Start here: LargeDataStorage#saveDataList (new else branch) and LargeDataStorage#delete; then the eviction branch in #save.
• Failure condition: if FileUtil.del fails, deleteDetailData logs and continues — worst case equals the previous behavior (file left behind); in-memory state and index stay consistent.
• Rollback or disable path: revert this single class; no data migration in either direction.

Contributor declaration

• I linked the Issue that defines this change.
• I tested the affected behavior and reported the actual results above.
• I did not include credentials, private data, or generated build output.
• I disclosed substantial AI assistance below, or this PR contains no substantial AI-generated code.

AI assistance: Diagnosis, fix, and tests were developed with substantial assistance from Claude Code (Anthropic); all changes were reviewed and verified locally by the author.

[openai0229]

Thanks for the focused fix and regression coverage. The last-record index rewrite, explicit delete cleanup, and eviction cleanup are moving in the right direction, but there is one blocking consistency issue and one test gap to address before merge.

1. LargeDataStorage.update still writes detail data when the id no longer exists. getAfterSave(null, update) returns the update object, so save -> delete -> delayed update recreates .json without restoring the id in the index. I reproduced this against the current head: the detail-file assertion fails because the file exists again. Since update and delete are separate HTTP operations and the frontend has asynchronous update calls, this can also occur as a real race. Please make update return or reject when dataMap.get(id) is missing, and serialize save, update, and delete on the same storage instance with one shared lock or synchronized methods. The missing-id check alone does not close the interleaving where update reads the old record, delete completes, and update then writes the file.

2. deletedLastRecordMustNotResurrectAfterReload does not independently verify the empty-index rewrite. If the empty-map branch in saveDataList is removed while detail deletion remains, all three current tests still pass; reload only logs that the stale indexed detail file is missing. Please directly assert that the index file is empty after deleting the final record, and that the eviction index contains only the surviving ids.

Please also add a delayed-update-after-delete regression test and a deterministic update/delete concurrency test. Using a temporary injectable storage root instead of writing test data under the real user home would be a useful cleanup as part of this test work.

After these changes, please update the branch with the latest main and run the required checks, including Java CodeQL because this PR changes Java storage code.

[openai0229]

Thank you for the original deletion fix and regression tests. I pushed a maintainer follow-up (fix(storage): serialize mutations and ignore stale updates) to address the remaining consistency issue from the review.

The follow-up serializes save, update, and delete on each storage instance, ignores updates whose ID is no longer indexed, and adds direct assertions for the empty final index and the surviving eviction index. It also adds delayed-update-after-delete and deterministic update/delete interleaving tests.

Verification completed locally:

LargeDataStorageTest: 5 tests, 0 failures, 0 errors
Storage reactor: BUILD SUCCESS
git diff --check: clean

Because this changes Java storage code, I will wait for the refreshed backend CI and Java CodeQL before merging.

[Duansg]

Hi, thanks for the detailed review and for reproducing the update-after-delete case — much appreciated.

Follow-up on the remaining items from our side:

• Injectable storage root (f964818): added a constructor overload taking the storage base path (production behavior unchanged); LargeDataStorageTest now runs entirely under a JUnit @TempDir instead of the real user home.
• Id collision fix (1e2b330): while adding the regression tests, we found that back-to-back saves could collide on the same id — IdUtil.generateId() is millisecond-based, so two calls within the same millisecond returned identical ids and the later record silently overwrote the earlier one. The timestamp part is now strictly increasing via a lock-free CAS, keeping the existing id format so values stay within JS Number.MAX_SAFE_INTEGER. Covered by IdUtilTest (tight-loop, monotonicity, 8-thread concurrency).

The branch is up to date with main, and the storage-module tests pass locally (run with -Dmaven.test.skip=false). CodeQL will run automatically on this PR.

Please take another look when you have a chance — happy to address anything else. Thanks again!

[openai0229]

Thanks for adding the injectable storage root and for catching the same-millisecond ID collision. The new @TempDir coverage is a clear improvement.

During review of the global IdUtil change, we found one additional ordering bug. The atomic timestamp was strictly increasing, but the final ID still concatenated a variable-width user suffix. A call for user suffix 999 followed by suffix 0 reproduced a full-ID regression from 1784981293678999 to 17849812936790. This matters because LargeDataStorage uses the numeric ID order to evict the oldest entry.

We pushed maintainer commit f74858b4 to reserve a fixed three-digit user suffix numerically as timestamp * 1000 + floorMod(userId, 1000), plus a cross-user regression test. Existing IDs remain readable Long values, new IDs remain below JavaScript Number.MAX_SAFE_INTEGER, and new IDs preserve timestamp ordering across user contexts.

Verification:

• storage reactor: 9 tests, 0 failures/errors
• LargeDataStorageTest: 5 passed
• IdUtilTest: 4 passed, including tight-loop, concurrency, monotonicity, and cross-user suffix width
• git diff --check: passed

Because this is Java storage code, we will wait for the refreshed Backend CI and Java CodeQL before approving and merging.

[openai0229]

Reviewed the latest maintainer-assisted fixes at f74858b. The storage reactor tests cover serialized save/update/delete behavior, deletion persistence, detail-file cleanup, and ID monotonicity; all 9 focused tests pass, the required checks pass, and Java CodeQL passes. The code issues from the earlier review are resolved.