feat: Explicit DB handle threading for complex _context() wrappers (Phase 7)

[jsavin]

Summary

• Convert the 5 remaining save/swap/restore _context() wrappers in db_format.c to pass the database handle explicitly through new _hdb functions
• All databasedata global mutation is now eliminated from the _context() wrapper layer
• Bottom-up conversion: Layer 2 (header I/O _fnum), Layer 3 (allocator _hdb), Layer 4 (high-level _hdb), Layer 5 (wrapper conversion)

Key Design Decisions

• _hdb functions derive fnum and v6/v7 format from (**hdb).headerLength — no dependency on db_format_mode_current()
• dbcopy_hdb reads source data from dbsaveas_source during Save As (the source block lives in the original DB, not the destination)
• dbassign_hdb checks fldatabasesaveas to force fresh allocation in destination
• ~15 internal allocator helpers converted to _hdb variants (all static to db.c)

Test plan

• 295 unit tests pass (4 new Phase 7 tests: allocate+read, assign, savehandle, copy roundtrips)
• 1704 integration tests pass, 0 failures
• Frontier.root v6→v7 migration verified
• Existing callee-saves tests verify databasedata is never mutated by wrappers
• All new _hdb tests prove explicit handle threading works without touching globals

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[claude]

Code Review: Phase 7 — Explicit DB Handle Threading

Overall this is a solid, well-structured continuation of the _context() wrapper de-globalization work. The layering is clean and the test coverage demonstrates correct behavior explicitly. A few items need attention before merge:

---

Bugs

1. dbclearshadowavaillist_hdb declared static boolean but has no return statement

The function returns boolean but falls off the end without a return value in all code paths. Callers ignore the return value, which suggests the intent is void. Either change the return type to void, or add return (true); before the closing brace.

This compiles due to C's implicit-return allowance but is undefined behavior when the caller reads the return value (even if no current caller does). The legacy dbclearshadowavaillist was likely void — worth checking and matching that signature.

---

2. Ignored return value in dbmergeleft_hdb

At the bottom of dbmergeleft_hdb:

dbsetavailshadow_hdb (ixshadow, newadr, newsize, hdb);  /* return value silently dropped */

Every other call to dbsetavailshadow_hdb in this file checks its return value. This one silently discards it. If the shadow write fails, dbmergeleft_hdb returns true with corrupted shadow state. Should be:

if (!dbsetavailshadow_hdb (ixshadow, newadr, newsize, hdb))
    return (false);

---

Design Concerns

3. dbclearshadowavaillist_hdb still temporarily swaps databasedata

The comment acknowledges this: "a rare path that will be fully converted in a future phase." That's fine, but the PR description claims "all databasedata global mutation is now eliminated from the _context() wrapper layer" which is inaccurate for this path. The PR description should note this remaining exception so reviewers aren't misled.

4. dbreadheader_fnum is a trivial pass-through with no added value

This exported function is a one-liner that calls dbreadheader_core with identical arguments. Adding a public API symbol for no behavioral difference creates extra maintenance surface. Consider exporting dbreadheader_core directly under the stable name, or document why the indirection is intentional.

---

Minor

5. Repeated test setup — four identical open/new/save/restore blocks

Each of the four new tests contains identical ~15-line setup and cleanup sequences. A static boolean open_scratch_db(...) / close_scratch_db(...) helper would reduce duplication and let each test body express only what it actually tests.

---

What's working well

• The db_hdb_use64 / db_hdb_header_size / db_hdb_trailer_size / db_hdb_fnum static inline helpers are clean and prevent v6/v7 format detection from being scattered across every function body.
• dbcopy_hdb correctly handles the Save As source/dest split — the dbsaveas_source logic is clearly documented and mirrors the original dbcopy_internal intent.
• The new tests explicitly assert databasedata == saved_db after every operation, which is exactly the right verification for this change.
• 295/295 unit tests + 1704 integration tests passing is good confidence.

---

Summary

Two bugs need fixing before merge: the missing return in dbclearshadowavaillist_hdb and the ignored return from dbsetavailshadow_hdb in dbmergeleft_hdb. The PR description should also be corrected to note that one rare code path in the shadow allocator still temporarily swaps the global.

🤖 Generated with Claude Code

[claude]

Code Review — PR #453: Explicit DB Handle Threading (Phase 7)

Overall assessment: Strong, well-structured work. The layered architecture is clear, the _context() simplifications are a genuine improvement, and test coverage is solid. A few items worth addressing before merge.

---

What is Working Well

• Layered architecture (L2 _fnum -> L3 _hdb -> L4 _hdb -> L5 wrapper conversion) is easy to follow and correctly separates concerns
• _context() simplifications are a major quality improvement — eliminating the save/restore/apply boilerplate makes each wrapper's intent immediately legible
• Format detection from headerLength is the right approach; avoids depending on the global db_format_mode_current() for the critical _hdb paths
• static inline helper quartet (db_hdb_use64, db_hdb_header_size, db_hdb_trailer_size, db_hdb_fnum) prevents format logic from scattering across the file
• 4 new roundtrip tests exercise the full allocate-read, assign, savehandle, and copy paths against real temp databases; the pattern of saving/restoring databasedata before the test exercise and asserting it was unchanged afterward is exactly right
• 295 unit tests + 1704 integration tests all pass

---

Issues

Medium: "No global access" comments overstated for dbcopy_hdb and dbassign_hdb

dballocate_hdb and dbrelease_hdb doc comments say "no global databasedata access", which is true. But dbassign_hdb and dbcopy_hdb both silently read non-databasedata globals:

// dbassign_hdb:
if (fldatabasesaveas || (adr == nildbaddress)) { ...

// dbcopy_hdb:
hdldatabaserecord source_hdb = (fldatabasesaveas && dbsaveas_source != nil) ? dbsaveas_source : hdb;

The distinction between "no databasedata access" and "no global access" is meaningful. A future caller who takes the comment at face value could misuse these functions in a context where fldatabasesaveas/dbsaveas_source are not in the expected state. Suggest updating the doc comments to say something like:

Reads the global fldatabasesaveas (and dbsaveas_source in dbcopy_hdb) to handle Save As context. Does not read or write databasedata.

Medium: dbclearshadowavaillist_hdb still swaps databasedata (acknowledged, but needs a tracking comment)

The comment explains the two temporary swaps and notes this will be "fully converted in a future phase." Given the GIL model this is safe today — but the swap happens twice in sequence (once for dbflushheader, once for dbrelease_internal), and the correctness guarantee depends on no yield points occurring between save and restore. The comment should say this explicitly so future maintainers know why the pattern is safe. A TODO tag would help track it:

/* TODO(Phase 8): Convert dbflushheader and dbrelease_internal to _hdb
   so this function no longer needs to touch databasedata.
   Safe under GIL: no yield points between save and restore. */

Low: dbreadheader_fnum is a trivial forwarding wrapper

boolean dbreadheader_fnum (...) {
    return dbreadheader_core (...);
}

This adds a public API symbol for zero logic. Either rename dbreadheader_core to dbreadheader_fnum directly, or have internal callers call dbreadheader_core themselves. One less exported symbol.

Low: ixshadow + 1 array access in dbmergeright_hdb / dbmergeleft_hdb

assert ((*(hdlavaillistshadow)(**hdb).u.extensions.availlistshadow.data) [ixshadow + 1].adr == nextavail);

assert is stripped in release builds. If the avail list is corrupt, ixshadow + 1 could be an out-of-bounds read in production. This mirrors the legacy pattern, but since this is new code it is worth noting — a bounds check before the assert would make this safe in production too.

Low: hsize computed then gethandlesize(h) called again in dbassignhandle_hdb

long hsize = (h != nil) ? gethandlesize(h) : 0;

if (*adr == nildbaddress) {
    ...
    fl = dballocate_hdb ((long) gethandlesize (h), *h, adr, hdb);  /* redundant call */

hsize is never used in the nildbaddress branch. Either use hsize consistently or drop the early computation. Minor redundancy.

Low: Test temp files leak on assertion failure

All four new tests use assert() for setup steps before the cleanup block:

assert(dbnew(fnum, true));
/* ...many asserts... */
remove(scratch_path);  /* never reached if any assert fires */

If any assert() fires mid-test, the /tmp/hdb_*.db file is left on disk. A goto cleanup pattern would be more robust for CI environments.

---

Nits

• dbwriteheader_fnum and dbwritetrailer_fnum both accept hdldatabaserecord hdb solely for v6/v7 format detection. The parameter name makes it look like it might route I/O, but only fnum does that. A brief inline comment (/* format detection only */) would prevent confusion for future callers.
• The Phase 7 block header comment in db.c is well-written — the layered design explanation is exactly the kind of context that pays dividends in later phases.

---

Summary

Severity	Count
Medium	2
Low	4
Nit	1

The medium items are documentation/comment accuracy rather than behavioral bugs — the code appears correct given the GIL model and existing Save As semantics. None of the issues are blocking, but the "no global access" comment precision is worth fixing before this lands so future phases do not inherit a misleading invariant claim.

Generated with Claude Code

[claude]

Code Review: Phase 7 — Explicit DB Handle Threading

This is a well-structured refactor with clear layering, good documentation of design decisions, and solid test coverage. The goal—eliminating databasedata mutation from the _context() wrapper layer—is achieved cleanly.

---

What's Working Well

• Clean bottom-up layering: Layers 2→3→4→5 build on each other logically, and the naming convention (_hdb suffix) is consistent and clear.
• Format detection via headerLength: Deriving v6/v7 format from (**hdb).headerLength rather than the mutable global db_format_mode_current() is the right design—format is intrinsic to the database file, not a mode flag.
• Good helper factoring: The static inline helpers (db_hdb_use64, db_hdb_header_size, db_hdb_fnum, etc.) eliminate repetition and make the format-dispatch readable.
• Test design: open_scratch_v7_db explicitly restores globals before handing off to the test body, so the assertions databasedata == ctx.saved_db are meaningful—they prove the _hdb path never touches the global.

---

Issues and Concerns

1. Ignored return value in dballocate_hdb (shadow path) — db.c

In the #ifdef dbshadow fast-path branch of dballocate_hdb, when a block is found and used in full (no split), the shadow deletion is not checked:

dbdeleteavailshadow_hdb (i, hdb);     // return ignored

if (!dbsetavaillink_hdb (prevnomad, nextnomad, fnum, hdb))
    goto failure;

The non-shadow path in dbrelease_hdb correctly checks the insert return. While dbdeleteavailshadow_hdb is unlikely to fail (it's a handle stream operation), silent failure here could lead to a corrupted shadow list—leaving a stale entry that points to an allocated block. Should be:

if (!dbdeleteavailshadow_hdb (i, hdb))
    goto failure;

2. Remaining global mutation in dbclearshadowavaillist_hdb — acknowledged but worth flagging

The TODO(Phase 8) comment is noted, and the "safe under GIL" claim is reasonable. However, this function is called from both dballocate_hdb and dbrelease_hdb, so it's on the hot path for all allocations under SMART_DB_OPENING. The double global-swap pattern:

{ hdldatabaserecord saved = databasedata; databasedata = hdb; dbflushheader(); databasedata = saved; }
{ hdldatabaserecord saved = databasedata; databasedata = hdb; dbrelease_internal(adrblock); databasedata = saved; }

...means that during dballocate_hdb, if dbflushheader() or dbrelease_internal() hits a yield point (despite the claim), another thread could observe an unexpected databasedata. The claim of "no yield points" should be verified/documented with a reference to the specific yield-point list (GIL yield points are at langbackgroundtask() / thread.sleepTicks()—does dbflushheader call either?).

3. db_format_mode no longer applied in _context() wrappers — implicit design invariant

The old wrappers called db_format_mode_apply(&context->mode) before the operation. The new wrappers delegate to _hdb which derives format from (**hdb).headerLength. This is correct if context->mode is always consistent with (**hdb).headerLength. Is there any code path where a db_context is constructed with a mismatched mode and database handle? If so, that would be a silent regression. Worth a doc comment (or assert) in dbassign_context / dbcopy_context etc. noting the invariant:

// Invariant: context->mode must match (**context->database).headerLength
// Phase 7+ derives format from headerLength, not from the mode field.

4. Hardcoded /tmp paths in tests — potential parallelism / leftover-file issues

assert(open_scratch_v7_db(&ctx, "/tmp/hdb_alloc_test.db"));
assert(open_scratch_v7_db(&ctx, "/tmp/hdb_assign_test.db"));

If the test runner ever parallelizes test executables or if a test crashes leaving the file behind, subsequent runs could fail or operate on stale data. Using paths that include the PID (e.g., snprintf(path, sizeof(path), "/tmp/hdb_alloc_%d.db", getpid())) would be more robust.

5. Minor: dbwritetrailer_fnum parameter comment is in function signature

boolean dbwritetrailer_fnum (..., hdldatabaserecord hdb /* format detection only */) {

This is fine, but for the public header declaration in db.h, the comment is absent. Consider mirroring the comment there, or dropping it from the implementation signature and putting it in the function body comment—it's slightly confusing in the signature.

6. dbfindpreviousavail_hdb has an unused parameter in the shadow path

In the #ifdef dbshadow branch:

static boolean dbfindpreviousavail_hdb (dbaddress adr, dbaddress *prev, long *ixshadow, hdlfilenum fnum, hdldatabaserecord hdb) {
#ifdef dbshadow
    ...
    // fnum is never used in this branch

fnum is unused in the shadow branch. Under dbshadow, all lookups go through havailshadow, not through file I/O. This will generate a compiler warning on some build configs. The __unused annotation or a (void)fnum; cast would suppress it.

---

Summary

Category	Status
Correctness	✅ Good — the core conversion is correct
Global elimination	✅ Achieved in all 5 wrappers
Remaining globals	⚠️ dbclearshadowavaillist_hdb still swaps (acknowledged)
Test coverage	✅ 4 new roundtrip tests, all passing
Return value checking	⚠️ dbdeleteavailshadow_hdb unchecked in shadow allocator path
Design invariants	⚠️ context->mode vs headerLength invariant undocumented
Test robustness	⚠️ Hardcoded /tmp paths

The unchecked dbdeleteavailshadow_hdb return value (#1) is the most actionable item before merge. The rest are documentation/robustness concerns that could be addressed in follow-on work.

🤖 Generated with Claude Code

[claude]

Code Review: Phase 7 — Explicit DB Handle Threading for _context() Wrappers

Overall this is a well-designed, high-quality PR. The architectural goal (eliminating all databasedata global mutation from the _context() wrapper layer) is clearly achieved, the layering is clean, and the test strategy of explicitly asserting databasedata == ctx.saved_db after each _hdb call is exactly the right invariant to verify.

---

Positives

• Clean elimination of save/swap/restore pattern in all 5 remaining _context() wrappers. The diff from 20+ lines of boilerplate to 3–5 lines is a readability win and eliminates a whole class of re-entrancy bugs.
• Layering is well-documented — Phase 7 section headers, Layer 2/3/4 comments in db.h, and the block comment before the _hdb section in db.c all make the structure easy to follow for future maintainers.
• Test strategy is correct: using open_scratch_v7_db to create a real temp database, then restoring globals, then asserting databasedata == ctx.saved_db after each _hdb call proves the explicit-handle threading is actually working. This is better than mocking.
• Fallthrough paths are safe: all _context() wrappers correctly fall through to the legacy function when context == NULL or context->database == nil, preserving existing behavior for callers not yet converted.
• 295/295 unit tests pass, 1704/1704 integration tests pass — solid signal.

---

Issues

1. File descriptor leak in open_scratch_v7_db if dbnew fails (tests/db_format_tests.c:808–831)

if (!openfile(&fs, &ctx->fnum, false)) return false;
ctx->saved_db = databasedata;
ctx->saved_mode = db_format_mode_current();
if (!dbnew(ctx->fnum, true)) return false;   // ← fnum is open but never closed

If dbnew fails, ctx->fnum is already set and the temp file exists, but the function returns false without closing the file or deleting the path. The callers use assert(open_scratch_v7_db(...)) so this only matters if dbnew fails in CI (which would abort the process anyway), but it's worth cleaning up for correctness:

if (!dbnew(ctx->fnum, true)) {
    closefile(ctx->fnum);
    remove(ctx->path);
    return false;
}

2. Format-detection logic duplicated in dbwriteheader_fnum / dbwritetrailer_fnum (db.c)

Both functions inline:

boolean use64 = (hdb != nil && (**hdb).headerLength == (long) sizeof (tydatabaserecord_64));

This is identical to db_hdb_use64(), but that helper is defined later in the file as static inline, so it can't be called from these earlier functions. The duplication is understandable, but a forward-declaration comment or moving db_hdb_use64 earlier would prevent these from diverging silently. Consider:

/* (db_hdb_use64 defined below — see Phase 7 Layer 3+ helpers) */

or simply reorder the static inline helpers to appear before the Layer 2 _fnum functions.

3. assert() for internal invariant in dbwriteavailnode_hdb and dbsetavaillink_hdb (db.c)

assert ((**hdb).u.extensions.availlistblock == nildbaddress);

In a release build with asserts disabled, violating this invariant would silently proceed and corrupt the database. The original codebase uses assert for this too, so this is a pre-existing pattern — but since this is new code being added, this is a good opportunity to convert to a checked error return:

if ((**hdb).u.extensions.availlistblock != nildbaddress) {
    dberror(dbinternalerror);
    return (false);
}

4. flfree read but unused in dbmergeleft_hdb (carried over from original)

In dbmergeleft_hdb, dbreadavailnode_hdb(adr, &flfree, &ctbytes, &nextavail, ...) reads flfree, but the value is never subsequently checked. The original dbmergeleft has the same behavior — only ctbytes and the avail-list pointer matter for the merge logic. This is a pre-existing dead read, not introduced by this PR, but worth noting as technical debt. A suppression comment or removing the variable declaration (if dbreadavailnode_hdb can accept NULL for unused outputs) would clean this up.

5. Bounds check returns false silently on avail-list corruption (db.c)

if (ixshadow + 1 >= ctavail)
    return (false); /* corrupt avail list — OOB access */

This is a good bounds check, but returning false silently on what is effectively database corruption makes the failure hard to diagnose post-hoc. Consider dblogerror(dbmergeinvalidblockerror) (or a new specific error code) before returning, consistent with the pattern used elsewhere in the merge functions:

if (ixshadow + 1 >= ctavail) {
    dblogerror(dbmergeinvalidblockerror);
    return (false);
}

---

Minor / Nits

• register keyword on Handle h in dbcopy_hdb is a no-op in modern C. This is consistent with the existing codebase style, so no action needed — just confirming it's intentional.
• Remaining global reads (fldatabasesaveas, dbsaveas_source) in dbassign_hdb and dbcopy_hdb are clearly documented in both the PR description and inline comments. This is an appropriate documented dependency and doesn't undermine the goal of the PR.
• dbcopy_hdb's register boolean flreturned is declared at the top but the pattern could be simplified slightly, though it's consistent with existing style.

---

Summary

Severity	Count
Low (test fix)	1
Low (duplication/ordering)	1
Low (silent failure on corruption)	2
Nit/informational	2

None of the issues are blockers. The most actionable items are the file descriptor cleanup in open_scratch_v7_db (#1) and adding error logging before the OOB avail-list returns (#5). The architectural work itself is correct and well-executed — this is exactly the kind of incremental global-state elimination that makes the codebase safer for concurrent and multi-database use.

🤖 Generated with Claude Code

[claude]

Code Review: Phase 7 — Explicit DB Handle Threading

This is a well-structured, bottom-up refactor that makes real progress toward eliminating databasedata global mutation from the context-wrapper layer. The layered approach (Layer 2 → 3 → 4 → wrapper conversion) is clean and the test strategy is solid. Below are observations from most to least significant.

---

Architecture / Correctness

1. dbclearshadowavaillist_hdb still temporarily mutates databasedata

The two brief databasedata swaps in this function are the only remaining global mutation on the hot path for dballocate_hdb and dbrelease_hdb. The GIL-safety argument is correct (no yield points inside dbflushheader/dbrelease_internal), and the TODO(Phase 8) comment is clear. However, because this is called from both public entry points of the new layer, it means dballocate_hdb and dbrelease_hdb are not yet fully global-free under SMART_DB_OPENING builds.

This is acceptable as a staged approach, but consider adding an assertion or compile-time note that the SMART_DB_OPENING path still holds the GIL assumption. If that invariant is ever broken (e.g., a future yield point added to dbflushheader), the temporary swap will become a race window.

2. Shadow list bounds checks outside #ifdef dbshadow in dbmergeright_hdb / dbmergeleft_hdb

In dbmergeright_hdb and dbmergeleft_hdb, the bounds checks on ixshadow and the assert on the shadow list entry are outside any #ifdef dbshadow guard. The original dbmergeright/dbmergeleft keep their shadow-list manipulation inside #ifdef dbshadow. Since dbshadow is unconditionally defined in dbinternal.h, this works in practice, but it's a style inconsistency that would silently break in a hypothetical non-shadow build.

Wrapping these blocks in #ifdef dbshadow would match the pattern of the original functions and make the code defensively correct:

// In dbmergeright_hdb, after dbfindpreviousavail_hdb():
#ifdef dbshadow
    {
        long ctavail = (**hdb).u.extensions.availlistshadow.eof / sizeof (tyavailnodeshadow);
        if (ixshadow + 1 >= ctavail) { ... }
        assert (...);
    }
#endif

3. dbassign_hdb silently ignores dbrelease_hdb failure

if (newsize > cttotal) {
    dbrelease_hdb (adr, hdb); /*ignore return — don't abort saving*/
    return dballocate_hdb (newsize, pdata, padr, hdb);
}

The comment acknowledges this matches the legacy dbassign_internal behavior. Worth keeping as-is for compatibility, but this means a failed release leaves a dangling block in the avail list. Consider at minimum a dblogerror on failure so the condition is observable in logs.

---

Code Quality

4. Format detection logic duplicated before db_hdb_use64() is defined

Both dbwriteheader_fnum and dbwritetrailer_fnum contain:

/* Same logic as db_hdb_use64() defined below in Layer 3+ helpers section */
boolean use64 = (hdb != nil && (**hdb).headerLength == (long) sizeof (tydatabaserecord_64));

Since db_hdb_use64 is a static inline defined later in the file, one option is to forward-declare it (or move the helper block earlier in the file) to eliminate the duplication. Not a bug, but the comment calling out the duplication will confuse future readers.

5. Public API exposure of internal-only functions

dbwriteheader_fnum and dbwritetrailer_fnum are declared extern in db.h, but the diff shows no callers outside db.c. If they're only needed internally, they should be static (or at least noted in the header as "exposed for testing only"). Unneeded public symbols increase API surface area.

6. Minor comment inconsistency

dbassign_context has a different comment style from the other four converted wrappers — it says "Context-aware dbassign. For non-NULL contexts..." rather than the "Phase 7: Explicit context — no global mutation." pattern used by the others. Not a defect, but inconsistent with the Phase 7 narrative.

---

Test Coverage

7. assert() used for test setup — resource leak risk

assert(open_scratch_v7_db(&ctx, "alloc"));

If open_scratch_v7_db fails, the assert terminates the process before close_scratch_v7_db can clean up. More importantly, if an assert inside the test body fires (e.g., assert(adr != nildbaddress)), any handles allocated before that point leak. Since these are temp files on disk, the leak is minor, but it's cleaner to use explicit failure handling:

if (!open_scratch_v7_db(&ctx, "alloc")) {
    log_error(..., "setup failed");
    goto cleanup;
}

8. test_hdb_allocate_and_read assertion after potential NULL dereference

if (!dbrefhandle_hdb(adr, &href, ctx.hdb)) goto cleanup;
assert(href != nil);              // reachable only if dbrefhandle_hdb returned true
assert(GetHandleSize(href) == sizeof(payload));
lockhandle(href);
assert(memcmp(*href, payload, sizeof(payload)) == 0);
unlockhandle(href);
disposehandle(href);

If dbrefhandle_hdb returns true but sets href = nil (shouldn't happen per contract, but defensive programming), the lockhandle/memcmp would crash. A single early check covers this:

assert(href != nil && GetHandleSize(href) == sizeof(payload));

Minor point — the existing assert(href != nil) already guards this before the dereference.

---

Positive Highlights

• Clean layer boundaries: Layer 2 (_fnum) → Layer 3 (_hdb allocator) → Layer 4 (_hdb high-level) → wrapper conversion is easy to follow and auditable.
• GIL safety analysis is thorough: The inline comments in dbclearshadowavaillist_hdb correctly apply the ADR-014 reasoning and identify exactly which functions can be swapped safely.
• dbcopy_hdb Save As handling is correct: The source_hdb/src_fnum split correctly reads from the original DB and allocates in the destination, matching the dbsaveas_source contract.
• Global non-mutation is test-verified: Each Phase 7 test asserts databasedata == ctx.saved_db after calling _hdb functions, making the global-freedom guarantee machine-checkable.
• Test count increase: +4 unit tests (291→295), all passing. Integration test baseline (1704) held. The test names clearly identify the coverage.

---

Summary

The implementation is correct for the intended scope. The two items worth addressing before merge are (2) the shadow list guard consistency and (4) the format detection duplication — both are style/defensive issues rather than bugs given the current always-defined dbshadow. The dbclearshadowavaillist_hdb global mutation is acceptable as a documented Phase 8 item.

🤖 Generated with Claude Code

[claude]

Phase 7 Code Review

This is a well-executed continuation of the global-state elimination project. The layered architecture (L2 _fnum → L3/L4 _hdb → L5 _context() wrappers) is clean and the direction is correct. 295 unit tests + 1704 integration tests passing is solid evidence of correctness.

---

Issues

1. db_hdb_use64() duplicated in dbwriteheader_fnum and dbwritetrailer_fnum (maintainability)

db.c:970 and db.c:1010 both inline:

/* Same logic as db_hdb_use64() defined below in Layer 3+ helpers section */
boolean use64 = (hdb != nil && (**hdb).headerLength == (long) sizeof (tydatabaserecord_64));

The comment correctly explains why it's duplicated (forward reference — db_hdb_use64() is static inline defined ~2100 lines later). But this is a maintenance risk: if the use64 detection criterion ever changes, dbwriteheader_fnum and dbwritetrailer_fnum won't automatically inherit the change.

Suggested fix: Move the four static inline helpers (db_hdb_use64, db_hdb_header_size, db_hdb_trailer_size, db_hdb_fnum) to just before dbwriteheader_fnum (around line 959). They have no dependencies on Layer 3+ code and can live anywhere above their first use site. This eliminates the duplication and the explanatory comment.

---

2. dbclearshadowavaillist_hdb — temporary databasedata swap is a known limitation (tracked, acceptable)

The Phase 8 TODO for dbflushheader and dbrelease_internal is clearly documented and safe under the GIL. No action needed for this PR — just noting it for visibility.

---

Minor / Test Code

3. close_scratch_v7_db — spurious NULL check on array member (tests/db_format_tests.c)

if (ctx->path != NULL) remove(ctx->path);

ctx->path is a char[128] array, not a pointer — this condition is always true. It's harmless but misleading. The correct guard would be ctx->path[0] != '\0', or simply call remove(ctx->path) unconditionally (an empty string passed to remove fails gracefully anyway).

---

4. Handle leak on partial failure in tests (minor)

In e.g. test_hdb_assign_roundtrip, if dbrefhandle_hdb returns false after setting href to a non-nil value, the handle is leaked before hitting goto cleanup. This is test-only and the happy path is what's being tested, so it's low priority — noting for future test hardening.

---

Confirmed-Correct Observations

• dbinsertavailshadow_hdb called without #ifdef dbshadow guard in dbrelease_hdb: Mirrors the original dbrelease_internal() at db.c:2669 exactly — confirmed correct.
• dballocate_hdb split-path omits dbsetavaillink_hdb: Also matches the original — the left piece after a split inherits the same predecessor link, so no update is needed. ✓
• dbassign_hdb and dbcopy_hdb reading fldatabasesaveas / dbsaveas_source globals: These are coordinator flags (read-only from _hdb's perspective) and are well-documented in the function comments. Correct.
• Test pattern: open_scratch_v7_db clearing globals then close_scratch_v7_db restoring them cleanly proves the _hdb functions are independent of databasedata. Good idiom to preserve in Phase 8.
• All new public symbols follow the _hdb / _fnum naming conventions consistently.

---

Summary

The one substantive fix worth applying before merge is moving the four static inline helpers earlier in db.c to eliminate the duplicated use64 detection. The test NULL-check is a minor cosmetic nit. Everything else looks correct. This is mergeable with the helper-reorder applied.

🤖 Generated with Claude Code