feat(ffi): Database.Close() guarantees proposals committed or freed

ffi/firewood.go

@@ -30,6 +30,7 @@ import (
 	"errors"
 	"fmt"
 	"runtime"
+	"sync"
 )
 
 // These constants are used to identify errors returned by the Firewood Rust FFI.
@@ -49,7 +50,8 @@ type Database struct {
 	// handle is returned and accepted by cgo functions. It MUST be treated as
 	// an opaque value without special meaning.
 	// https://en.wikipedia.org/wiki/Blinkenlights
-	handle *C.DatabaseHandle
+	handle    *C.DatabaseHandle
+	proposals sync.WaitGroup
 }
 
 // Config configures the opening of a [Database].
@@ -139,6 +141,9 @@ func (db *Database) Update(keys, vals [][]byte) ([]byte, error) {
 	return getHashKeyFromHashResult(C.fwd_batch(db.handle, kvp))
 }
 
+// Propose creates a new proposal with the given keys and values. The proposal
+// is not committed until [Proposal.Commit] is called. See [Database.Close] re
+// freeing proposals.
 func (db *Database) Propose(keys, vals [][]byte) (*Proposal, error) {
 	if db.handle == nil {
 		return nil, errDBClosed
@@ -151,8 +156,7 @@ func (db *Database) Propose(keys, vals [][]byte) (*Proposal, error) {
 	if err != nil {
 		return nil, err
 	}
-
-	return getProposalFromProposalResult(C.fwd_propose_on_db(db.handle, kvp), db)
+	return getProposalFromProposalResult(C.fwd_propose_on_db(db.handle, kvp), &db.proposals)
 }
 
 // Get retrieves the value for the given key. It always returns a nil error.
@@ -233,17 +237,23 @@ func (db *Database) Revision(root []byte) (*Revision, error) {
 
 // Close releases the memory associated with the Database.
 //
-// This is not safe to call while there are any outstanding Proposals. All proposals
-// must be freed or committed before calling this.
+// This blocks until all outstanding Proposals are either unreachable or one of
+// [Proposal.Commit] or [Proposal.Drop] has been called on them. Unreachable
+// proposals will be automatically dropped before Close returns, unless an
+// alternate GC finalizer is set on them.
 //
-// This is safe to call if the pointer is nil, in which case it does nothing. The
-// pointer will be set to nil after freeing to prevent double free. However, it is
-// not safe to call this method concurrently from multiple goroutines.
+// This is safe to call if the handle pointer is nil, in which case it does
+// nothing. The pointer will be set to nil after freeing to prevent double free.
+// However, it is not safe to call this method concurrently from multiple
+// goroutines.
 func (db *Database) Close() error {
 	if db.handle == nil {
 		return nil
 	}
 
+	runtime.GC()
+	db.proposals.Wait()
+
 	if err := getErrorFromVoidResult(C.fwd_close_db(db.handle)); err != nil {
 		return fmt.Errorf("unexpected error when closing database: %w", err)
 	}

ffi/firewood_test.go

@@ -1050,3 +1050,60 @@ func TestGetFromRootParallel(t *testing.T) {
 		r.NoError(err, "Parallel operation failed")
 	}
 }
+
+func TestProposalHandlesFreed(t *testing.T) {
+	t.Parallel()
+
+	db, _, err := newDatabase(filepath.Join(t.TempDir(), "test_GC_drops_proposal.db"))
+	require.NoError(t, err)
+
+	// These MUST NOT be committed nor dropped as they demonstrate that the GC
+	// finalizer does it for us.
+	p0, err := db.Propose(kvForTest(1))
+	require.NoErrorf(t, err, "%T.Propose(...)", db)
+	p1, err := p0.Propose(kvForTest(1))
+	require.NoErrorf(t, err, "%T.Propose(...)", p0)
+
+	// Demonstrates that explicit [Proposal.Commit] and [Proposal.Drop] calls
+	// are sufficient to unblock [Database.Close].
+	var keep []*Proposal //nolint:prealloc
+	for name, free := range map[string](func(*Proposal) error){
+		"Commit": (*Proposal).Commit,
+		"Drop":   (*Proposal).Drop,
+	} {
+		p, err := db.Propose(kvForTest(1))
+		require.NoErrorf(t, err, "%T.Propose(...)", db)
+		require.NoErrorf(t, free(p), "%T.%s()", p, name)
+		keep = append(keep, p)
+	}
+
+	done := make(chan struct{})
+	go func() {
+		require.NoErrorf(t, db.Close(), "%T.Close()", db)
+		close(done)
+	}()
+
+	select {
+	case <-done:
+		t.Errorf("%T.Close() returned with undropped %T", db, p0) //nolint:forbidigo // Use of require is impossible without a hack like require.False(true)
+	case <-time.After(300 * time.Millisecond):
+		// TODO(arr4n) use `synctest` package when at Go 1.25
+	}
+
+	runtime.KeepAlive(p0)
+	runtime.KeepAlive(p1)
+	p0 = nil
+	p1 = nil //nolint:ineffassign // Makes the value unreachable, allowing the finalizer to call Drop()
+
+	// In practice there's no need to call [runtime.GC] if [Database.Close] is
+	// called after all proposals are unreachable, as it does it itself.
+	runtime.GC()
+	// Note that [Database.Close] waits for outstanding proposals, so this would
+	// block permanently if the unreachability of `p0` and `p1` didn't result in
+	// their [Proposal.Drop] methods being called.
+	<-done
+
+	for _, p := range keep {
+		runtime.KeepAlive(p)
+	}
+}

ffi/proposal.go

@@ -14,17 +14,13 @@ import (
 	"errors"
 	"fmt"
 	"runtime"
+	"sync"
 	"unsafe"
 )
 
 var errDroppedProposal = errors.New("proposal already dropped")
 
 type Proposal struct {
-	// The database this proposal is associated with. We hold onto this to ensure
-	// the database handle outlives the proposal handle, which is required for
-	// the proposal to be valid.
-	db *Database
-
 	// handle is an opaque pointer to the proposal within Firewood. It should be
 	// passed to the C FFI functions that operate on proposals
 	//
@@ -34,6 +30,12 @@ type Proposal struct {
 	// this handle, so it should not be used after those calls.
 	handle *C.ProposalHandle
 
+	// [Database.Close] blocks on this WaitGroup, which is incremented by
+	// [getProposalFromProposalResult], and decremented by either
+	// [Proposal.Commit] or [Proposal.Done].
+	openProposals *sync.WaitGroup
+	freeOnce      sync.Once
+
 	// The proposal root hash.
 	root []byte
 }
@@ -58,8 +60,8 @@ func (p *Proposal) Get(key []byte) ([]byte, error) {
 	return getValueFromValueResult(C.fwd_get_from_proposal(p.handle, newBorrowedBytes(key, &pinner)))
 }
 
-// Propose creates a new proposal with the given keys and values.
-// The proposal is not committed until Commit is called.
+// Propose is equivalent to [Database.Propose] except that the new proposal is
+// based on `p`.
 func (p *Proposal) Propose(keys, vals [][]byte) (*Proposal, error) {
 	if p.handle == nil {
 		return nil, errDroppedProposal
@@ -72,8 +74,7 @@ func (p *Proposal) Propose(keys, vals [][]byte) (*Proposal, error) {
 	if err != nil {
 		return nil, err
 	}
-
-	return getProposalFromProposalResult(C.fwd_propose_on_proposal(p.handle, kvp), p.db)
+	return getProposalFromProposalResult(C.fwd_propose_on_proposal(p.handle, kvp), p.openProposals)
 }
 
 // Commit commits the proposal and returns any errors.
@@ -86,8 +87,7 @@ func (p *Proposal) Commit() error {
 	}
 
 	_, err := getHashKeyFromHashResult(C.fwd_commit_proposal(p.handle))
-	p.handle = nil // we no longer own the proposal handle
-
+	p.afterDisowned()
 	return err
 }
 
@@ -104,25 +104,32 @@ func (p *Proposal) Drop() error {
 	if err := getErrorFromVoidResult(C.fwd_free_proposal(p.handle)); err != nil {
 		return fmt.Errorf("%w: %w", errFreeingValue, err)
 	}
-
-	p.handle = nil // Prevent double free
-
+	p.afterDisowned()
 	return nil
 }
 
+func (p *Proposal) afterDisowned() {
+	p.freeOnce.Do(func() {
+		p.handle = nil
+		p.openProposals.Done()
+	})
+}
+
 // getProposalFromProposalResult converts a C.ProposalResult to a Proposal or error.
-func getProposalFromProposalResult(result C.ProposalResult, db *Database) (*Proposal, error) {
+func getProposalFromProposalResult(result C.ProposalResult, openProposals *sync.WaitGroup) (*Proposal, error) {
 	switch result.tag {
 	case C.ProposalResult_NullHandlePointer:
 		return nil, errDBClosed
 	case C.ProposalResult_Ok:
 		body := (*C.ProposalResult_Ok_Body)(unsafe.Pointer(&result.anon0))
 		hashKey := *(*[32]byte)(unsafe.Pointer(&body.root_hash._0))
 		proposal := &Proposal{
-			db:     db,
-			handle: body.handle,
-			root:   hashKey[:],
+			handle:        body.handle,
+			root:          hashKey[:],
+			openProposals: openProposals,
 		}
+		openProposals.Add(1)
+		runtime.SetFinalizer(proposal, (*Proposal).Drop)
 		return proposal, nil
 	case C.ProposalResult_Err:
 		err := newOwnedBytes(*(*C.OwnedBytes)(unsafe.Pointer(&result.anon0))).intoError()