Skip to content

Follow-ups from the review of #118 (concurrent database starts) #134

Closed

Description

@MatiasFernandez

These came out of reviewing #118. Each one was checked against main. None of them is a regression that blocks merge: they are either already on main or new code that works but could be better. Listed most important first.

Should fix

1. gemdb file.py starts the stone without assertSafeToStart's guards

src/cli.ts (file mode, startstone) checks nothing before it starts the stone: not the engine version against the extent, NFS, space limits or room for the extent. #118 added these guards to the GemDB Shell. File mode is the one way to start the stone that still skips them. This was already the case on main.

Impact: after an engine pin moves, every existing database is orphaned (see CLAUDE.md). A terminal user then gets a bare GemStone 4045 login failure instead of the "delete this directory" message. A stone is also left running that the window would have refused to start. Every pin move will cause this for anyone who runs files from a terminal.

2. Stone start/wait logic is implemented twice: TypeScript and generated bash

The stone state parsing, Startup/Shutdown handling, the lock wait and the timeouts live both in processes.ts/lock.ts and in the wrapper cli.ts generates. #118 added about 65 lines of bash to that. The copies already differ: the lock wait is 10 s in bash and 60–150 s with progress tracking in TypeScript, bash has no guards (item 1), and they handle an external database differently (item 3).

Suggestion: have file mode run ensureProcesses through the Node bundle that is already staged (for example gemdb-shell.js --ensure-stone). Keep a bash fast path for "stone at OK" so the common case doesn't pay for Node startup. This would also fix items 1 and 3.

3. External database: the wrapper waits at Startup, the window and Shell don't

The wrapper's external branch now rejects Shutdown and waits on Startup. lifecycle.ts (ensureRunning's external branch) and cliMain.ts still accept any listed stone. The [Unreleased] CHANGELOG entry says GemDB waits, which is only true for GemDB's own database. While fixing this, also pull the two copies of the external-is-down check into one assertExternalDatabaseUp().

Impact: when the administrator's stone is starting or recovering, the window or Shell login fails with a GemStone error while gemdb file.py waits. This is rare.

4. The no-lock overlap integration test can pass without an overlap

rivalStartsNetldi (src/__integration__/fixtures/processContender.ts) gives up silently after 20 s. The test in processes.test.ts never asserts that the two startnetldi runs overlapped, which side lost, or the raw side's exit code. If it doesn't overlap, the test still passes, so a return of #89 could pass CI. It should fail on timeout, and assert startedNetldi and the exit codes.

5. A stone lock carrying this process's own pid is never treated as stale

stoneLockIsStale returns !isAlive(owner), and isAlive(process.pid) is always true. A lock left by a crash, whose pid this process happens to get again (devcontainers, deterministic pids), blocks every start for a minute. #118 (ea64653) put the lock path back in that error message. The real fix is to track whether this process actually holds the stone lock, as the setup lock already does with heldHere. An own-pid lock we don't hold is then stale. The wording "This window…" is also wrong when the process is the GemDB Shell. This was already on main.

6. A stone that vanishes while we wait is reported as "stopped while it was starting"

waitForStoneReady (and the wrapper's wait_for_stone) say this even when this call didn't start the stone. For example, a stopping stone's last moments read Startup (measured). It would be better to loop back and start the stone, or at least word it as a stop. The outcome matches main (no automatic start), only the message differs, and running it again works.

Nits

  • Wrapper and a killed stone: stone_status ignores gslist's exit code, so a killed row is treated as up for one invocation (-c cleans it, and the next run starts the stone). Treat killed as not listed. stoneState could also map killed to absent. The extension is unaffected, because listProcesses discards non-zero gslist output.
  • Wrong log line after a declined force-stop: with the stone at Shutdown, runStop logs "GemDB is still running.". It should check stoneState for stopping. The log line is the same on main, and the status bar is correct.
  • $STATUS in the generated wrapper: it holds the stone's gslist status, then topaz's exit-status file. Rename the first to STONE_STATUS.
  • CHANGELOG: [Unreleased] has three ### Fixed headings. Merge them when the release is cut.
  • ensureListener and ensureProcesses: their locked flows overlap. Consider merging them only if they change again, since their wait semantics differ.

🤖 Generated with Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions