Skip to content

fix(storage): let a damaged record cost only that record - #782

Merged
Ishaan Gangwani (ishaan1124) merged 2 commits into
synthetic-sciences:mainfrom
aniruddhaadak80:fix/storage-migration-damaged-record
Sep 29, 2026
Merged

Ishaan Gangwani (ishaan1124) merged 2 commits into
synthetic-sciences:mainfrom
aniruddhaadak80:fix/storage-migration-damaged-record

Conversation

@aniruddhaadak80

Copy link
Copy Markdown
Contributor

What does this PR do, and why?

The storage migration chain advances a marker only past a migration that
completes:

const ok = await migration(dir).then(
  () => true,
  (error) => { log.error("failed to run migration", { index, error }); return false },
)
if (!ok) break
await Bun.write(path.join(dir, "migration"), (index + 1).toString())

and both migrations read their input with an unguarded await Bun.file(item).json(),
which throws on a zero-length or malformed record. So one damaged file fails the
migration, the loop breaks, and the marker never advances
— and since
MIGRATIONS is append-only, every migration added in every future release then
silently never runs for that user, for good.

Nothing surfaces it. state() still resolves and returns { dir }, so reads,
writes and list keep working and the CLI looks completely healthy. The only
evidence is one log.error line in a rotating log.

A zero-length record is exactly what an interrupted write leaves behind, and
Storage.publish is the one record writer in the tree that never fsyncs (every
sibling does), so this is reachable rather than theoretical.

The fix is that a damaged record costs that record:

const session = await Bun.file(item)
  .json()
  .catch(() => undefined)
if (!session?.projectID) continue

Applied to all five unguarded parses in the two migrations, so the chain always
makes progress.

The same lesson is already applied elsewhere in the tree and was worth following:
data-dir.ts carries the note "A corrupt legacy store must cost that store, not
the import. Left unguarded this threw past the marker write, so every later boot
re-ran the whole import and failed at the same byte, forever."

Linked issue

Self-identified. My issue budget this turn went to the two apply_patch defects
and the two localStorage copies; happy to file one for this if you would rather
track it separately.

How did you verify it?

test/storage/migration-damaged-record.test.ts runs the migration against a
throwaway directory containing a zero-length record and a healthy one, and
asserts both that it resolves and that the healthy record is still migrated. It
calls the migration directly rather than through state(), because the marker is
process-global and a marker-based test would depend on file order in the run.
That is also why Storage.MIGRATIONS is now exported — the alternative was a
test that only passes when it happens to run first.

I could not run the new test against unmodified main, because the export does
not exist there. Instead I proved the two mechanisms it depends on, each in
isolation:

---- zero-length record throws: true ----
---- marker after run: 1 (3 would mean all migrations ran) ----

The first is Bun.file(<empty>).json() — main's exact expression. The second
runs main's loop shape with a throwing migration at index 1: the marker is still
1 afterwards, so index 2 never runs. Together with reading the loop, that is the
whole causal chain, and it is why the test's resolves.toBeUndefined() is the
assertion that matters.

Commands run:

  • bun test --timeout 60000 ./test/storage/migration-damaged-record.test.ts → 1 pass
  • bun test --timeout 120000 ./test/storage → 17 pass, 3 fail
  • Those 3 failures (competing reclaimers cannot remove a newly acquired storage lock, a stale storage observer revalidates before replacing a new live owner,
    storage mutations and authority signals cross real process boundaries) are
    pre-existing: with my change stashed the same directory reports them, plus
    my own test failing to import. They are interprocess file-lock tests, which is
    the area most sensitive to Windows semantics.
  • bun run --cwd backend/cli typecheck → exit 0

One thing I deliberately did not do

There is a related durability gap: Storage.publish renames a record into place
without ever fsyncing the file or its directory, while JsonStore.replace,
Config, DataRelocation and CredentialLifecycle all do both. Fixing it
properly needs a crash-injection test, and the only assertion I could construct
was one that reads the source and looks for sync() — which AGENTS.md rules
out. I would rather raise it than smuggle in a test the conventions forbid.

Checklist

  • bun run check is green (format, typecheck, backend + frontend/ui + SDK tests) — blocked on this Windows checkout by the CRLF and symlink artifacts in my other pull requests; backend typecheck is clean and the storage suite adds no new failure
  • bun run --cwd frontend/workspace build succeeds if I touched frontend/workspace or frontend/ui — not touched
  • ./tooling/repo/generate.ts was run and the tooling/sdk output committed if I changed backend/cli/src/server — not touched
  • CHANGELOG.md has an Unreleased entry if the change is user-visible
  • The matching docs page under frontend/docs/src/content/openscience/ is updated if behavior changed — no doc change needed: this is invisible either way except that migrations now complete
  • Screenshots or a short video are attached for UI changes — not a UI change
  • No version bumps (package.json versions and tags are written by the release workflow)
  • install and frontend/landing/public/install are still byte-identical if I touched either — not touched

@vercel

vercel Bot commented Sep 28, 2026

Copy link
Copy Markdown

ANIRUDDHA ADAK (@aniruddhaadak80) is attempting to deploy a commit to the InkVell Team on Vercel.

A member of the Team first needs to authorize it.

@aniruddhaadak80
ANIRUDDHA ADAK (aniruddhaadak80) force-pushed the fix/storage-migration-damaged-record branch from bf9a56d to 54f3aa5 Compare September 29, 2026 13:32
The clash was CHANGELOG.md alone: main added several bullets at the anchor this branch also inserted at. The changelog is rebuilt from main's copy with this branch's entry kept, and the code files are carried over unchanged. Rebased as a single parent on main so a later rebase carries it.
@aniruddhaadak80
ANIRUDDHA ADAK (aniruddhaadak80) force-pushed the fix/storage-migration-damaged-record branch from 54f3aa5 to 3fa18fd Compare September 29, 2026 14:04
@ishaan1124
Ishaan Gangwani (ishaan1124) merged commit cd73014 into synthetic-sciences:main Sep 29, 2026
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants