Skip to content

Don't collect rows a save is still in the middle of writing - #1993

Merged
jcschaff merged 3 commits into
masterfrom
fix/cleanup-age-guard
Aug 19, 2026
Merged

Don't collect rows a save is still in the middle of writing#1993
jcschaff merged 3 commits into
masterfrom
fix/cleanup-age-guard

Conversation

@jcschaff

Copy link
Copy Markdown
Member

Fixes #1992. Should also close #1961, which is the same race seen from the sweep's side.

The race

DBBackupAndClean.cleanupDatabase — run every 15 minutes by DatabaseServer.DatabaseCleanupThread — collects rows that no document references. But "unreferenced" does not mean "garbage", because ServerDocumentManager.saveBioModel is not one transaction: each child (geometry, math description, model, simulation context, simulation) is committed by its own DBTopLevel.insertVersionable call, and the vc_biomodelsimcontext / vc_biomodelsim link rows are only written in a later transaction.

For the few hundred milliseconds in between, the child is committed and referenced by nothing — indistinguishable, to these queries, from an orphan left behind by a delete. Collecting it there breaks the save in progress with ORA-02291: parent key not found; when the race goes the other way the sweep aborts instead with ORA-02292: child record found (#1961).

Prod lost that race twice in the last 14 days of Loki retention, and a cleanup sweep was in flight for both:

2026-08-14 20:06:12  INSERT INTO vc_biomodelsimcontext (id,biomodelRef,simContextRef)
                     VALUES (322090139,322090135,322090117)
                     ORA-02291 (VCELL.SYS_C008211)   <- vc_simcontext 322090117 was
                                                        committed by this same save ~150 ms earlier
2026-08-07 03:06:28  INSERT INTO vc_simcontext ...
                     ORA-02291 (VCELL.SYS_C008289)   <- mathRef/geometryRef, swept a step earlier

The 2026-08-14 one raised the Better Stack VCell Release Health - Sim alert. Both predate or are unrelated to any recent change here — 1a27cd646a (8.0.24.01) removed the cross-site amplification by giving prod sole ownership of the sweep, but not the race itself.

The change

Require a row to have been unreferenced for CLEANUP_MIN_AGE_HOURS (1) before collecting it:

DELETE FROM vc_simcontext
 WHERE vc_simcontext.id NOT IN (SELECT vc_biomodelsimcontext.simContextRef FROM vc_biomodelsimcontext)
   AND vc_simcontext.versionDate < SYSDATE - INTERVAL '1' HOUR

Applied to all five unreferenced-row deletes and to the report queries that must stay consistent with them — including the vc_simdelfromdisk insert-select, which would otherwise queue simulations for disk deletion that the DELETE no longer removes.

Two deliberate boundaries:

  • The simulations guard goes in cleanRemoveUnreferencedSimulations, not in the shared getSelectUnreferencedSimKeySQL. SimulationDispatcher:636 uses that query to abort active jobs; its behaviour is unchanged.
  • cleanRemoveUnReferencedSotwareVersions is left alone. vc_softwareversion rows are written in the same transaction as their versionable (insertVersionableInit), so there is no window, and the table has no versionDate.

Garbage an hour old is still garbage; a row a few hundred milliseconds old is a save in flight.

Verification

  • mvn compile -pl vcell-server -am clean.
  • Drove the patched cleanupDatabase through a java.lang.reflect.Proxy stub Connection to capture every statement it emits in both dialects. The guard is present on all five deletes and their matching selects, with SYSDATE for Oracle and CURRENT_TIMESTAMP for Postgres.
  • Ran all 15 generated Postgres statements against a real postgres:15-alpine on a stub schema — all accepted.
  • Behavioural check on that Postgres: an unreferenced row 2 h old is collected; an unreferenced row written "just now" survives. Negative control — the pre-patch statement deletes both.

Not verified: execution against Oracle. SYSDATE - INTERVAL '1' HOUR is checked by construction only.

🤖 Generated with Claude Code

https://claude.ai/code/session_01HYdenYzMw35USHDQEATGVq

jcschaff and others added 2 commits August 18, 2026 14:55
ServerDocumentManager.saveBioModel is not one transaction. Each child --
geometry, math description, model, simulation context, simulation -- is
committed by its own DBTopLevel.insertVersionable call, and the
vc_biomodelsimcontext / vc_biomodelsim link rows are written in a later
transaction. For the few hundred milliseconds in between, the child is
committed and referenced by nothing, which is exactly the definition the
cleanup sweep uses to decide a row is garbage.

Prod has lost that race twice in the last two weeks. Both times the
health check's own save was the victim, and both times a cleanup sweep on
a *different* site sharing the same Oracle instance was mid-run:

  2026-08-14 20:06:12  INSERT INTO vc_biomodelsimcontext ... 322090117
                       ORA-02291 (VCELL.SYS_C008211) parent key not found
  2026-08-07 03:06:28  INSERT INTO vc_simcontext ...
                       ORA-02291 (VCELL.SYS_C008289) parent key not found

Issue #1961 is the same race seen from the sweep's side, failing with
ORA-02292 (child record found) when the save wins instead.

Require a row to have been unreferenced for CLEANUP_MIN_AGE_HOURS before
collecting it, on all five deletes and on the report queries that must
stay consistent with them (including the vc_simdelfromdisk insert-select,
which feeds disk cleanup). Garbage an hour old is still garbage.

The guard goes in cleanRemoveUnreferencedSimulations rather than in
getSelectUnreferencedSimKeySQL, which SimulationDispatcher also uses to
abort active jobs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HYdenYzMw35USHDQEATGVq
The sweep is assembled by string concatenation and only ever runs in the db
service against production Oracle, so a dialect slip in it is invisible until
it aborts a whole cleanup run in production. Nothing exercised it.

DatabaseCleanupSqlDialectTest (Fast, no database) drives cleanupDatabase()
through a Connection that records statements instead of executing them, and
holds the invariant that no unreferenced-row delete -- nor the report query or
vc_simdelfromdisk hand-off that has to match it -- goes out without the age
guard, in either dialect. vc_softwareversion is the documented exception.

DatabaseCleanupSqlTest (@QuarkusTest) runs the real sweep against the
testcontainers PostgreSQL with the real VCell schema, and asserts the guard's
actual behaviour: an orphan two hours old is collected, a row a save is still
writing survives. With the guard removed it fails on exactly that second
assertion.

Oracle was verified by hand against gvenzl/oracle-free 23ai:
SELECT (SYSDATE - INTERVAL '1' HOUR) FROM DUAL returns one hour earlier, all
15 generated statements execute, and the old/fresh orphan pair behaves as it
does on PostgreSQL. Left out of CI rather than adding a ~2 GB image pull to
every run.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HYdenYzMw35USHDQEATGVq
@jcschaff

Copy link
Copy Markdown
Member Author

Added dialect coverage — the sweep had none.

DatabaseCleanupSqlDialectTest (vcell-server, Fast, no database) drives cleanupDatabase() through a Connection that records statements instead of executing them, and asserts the invariant that no unreferenced-row delete goes out without the age guard, in either dialect — along with the report query and the vc_simdelfromdisk hand-off that have to match it. vc_softwareversion is the documented exception (its rows are written in the same transaction as their versionable, and the table has no versionDate). This is what would catch a sixth delete being added later without a guard.

DatabaseCleanupSqlTest (vcell-rest @QuarkusTest) runs the real sweep against the testcontainers PostgreSQL with the real VCell schema from scripts/init.sql, and asserts behaviour rather than text: an orphan two hours old is collected, a row a save is still writing survives. Negative control — with the guard replaced by 1=1 it fails on exactly that second assertion (a row a save is still writing must not be collected). It also can't disturb other tests' fixtures, precisely because everything they create is under an hour old.

Oracle was verified by hand against gvenzl/oracle-free:23-slim-faststart (23ai):

SQL> SELECT SYSDATE AS now_dt, (SYSDATE - INTERVAL '1' HOUR) AS date1hr FROM DUAL;
NOW_DT               DATE1HR
2026-08-18 20:09:43  2026-08-18 19:09:43

plus all 15 generated Oracle statements executed against a stub schema (no ORA-, exit 0), and the same old/fresh orphan pair behaving as it does on PostgreSQL (1 row deleted, save-in-flight remaining).

I left the Oracle container out of CI rather than add a ~2 GB image pull to every run — happy to wire it in as a regression.yml group (merge queue + nightly) instead if you'd rather have it gated there. Your call.

The sweep only ever runs against production Oracle, so PostgreSQL coverage in
the fast lane leaves the dialect that matters unchecked. Oracle needs its own
~2 GB image, which does not belong on every push -- so it goes where the heavy
suites already live: a regression group, run by the merge queue before anything
lands and again nightly.

DatabaseCleanupOracleSqlTest boots gvenzl/oracle-free (23ai) on a stub schema
and asserts three things: every statement cleanupDatabase() issues is accepted
by Oracle, SYSDATE - INTERVAL '1' HOUR evaluates to exactly one hour back, and
an orphan two hours old is collected while a row a save is still writing is
spared. With the guard removed it fails on that last assertion.

A stub schema rather than the real one because what is under test is Oracle's
acceptance of these statements, not the schema.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HYdenYzMw35USHDQEATGVq
@jcschaff

Copy link
Copy Markdown
Member Author

Oracle is now gated rather than spot-checked.

Oracle_IT regression groupDatabaseCleanupOracleSqlTest boots gvenzl/oracle-free:23-slim-faststart (23ai) on a stub schema and asserts three things: every statement cleanupDatabase() issues is accepted by Oracle, SYSDATE - INTERVAL '1' HOUR evaluates to exactly one hour back, and an orphan two hours old is collected while a row a save is still writing is spared. Negative control: with the guard replaced by 1=1 it fails on that last assertion, on Oracle as on PostgreSQL.

Verified end to end on this branch — gh workflow run regression.yml --ref fix/cleanup-age-guard -f test-group=Oracle_IT, run 32183505458:

✓ prepare regression matrix    3s
✓ CI-Test-group-Oracle_IT      2m35s   Tests run: 3, Failures: 0, Errors: 0
✓ regression-gate              4s

55s of that is the image pull plus Oracle boot on the runner — the cheapest group in the suite by some margin, and it costs the fast lane nothing. It runs in the merge queue before anything lands, nightly, and is selectable from the "Run workflow" dropdown.

Fast lane still green on the same commit: build, Fast-core, Fast-other, Quarkus, CodeQL.

Coverage now stands as: PostgreSQL on every push (real schema, real sweep), Oracle on the merge queue and nightly (real Oracle), and a database-free invariant test in Fast that fails if a sixth unreferenced-row delete is ever added without a guard.

@jcschaff

Copy link
Copy Markdown
Member Author

Filed #1994 for the broader version of this: the database layer branches on DatabaseSyntax in 55 places across 33 files, and CI only ever runs the PostgreSQL side while production only ever runs the Oracle side. Oracle_IT here closes that for the cleanup sweep; #1994 proposes closing it for the rest by running the existing Quarkus suite under Oracle as a second regression group.

Two findings from this PR that make it cheaper than it sounds: SQLCreateAllTables.getVCellTables() is public and Table.getCreateSQL(DatabaseSyntax) already emits per-dialect DDL, so no hand-written Oracle init.sql is needed; and quarkus-jdbc-oracle/ojdbc11 are already vcell-rest dependencies. The blocker is AgroalConnectionFactory.usePostgresql() switching on the profile name — a new profile would hit its default: throw, so the datasource choice wants to be its own config property instead.

@jcschaff
jcschaff merged commit 0d0705c into master Aug 19, 2026
12 checks passed
@jcschaff
jcschaff deleted the fix/cleanup-age-guard branch August 19, 2026 02:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant