fix: DeleteNode cascade, go.mod build, DSN pragmas, chunking, backup visibility, key policy - #60
Merged
Merged
Conversation
…swallowing delete errors deleteNodeQ only deleted edges + nodes. The schema declares child tables (embeddings, file_watch, node_signatures) without ON DELETE CASCADE, so with foreign_keys(ON) the nodes delete failed whenever child rows existed, and node_versions/node_metadata rows leaked as orphans. deleteNodeQ now deletes every child table explicitly (leaf tables first) in the same transaction. Engine passes that call DeleteNode (GarbageCollect, Sparsifier passes, consolidateDuplicates, LLM consolidator) silently discarded errors and mis-counted removed nodes; they now log failures via log/slog and count only successful deletions.
…tandalone go.mod required hawk-mcpkit v0.0.0, a version that does not exist on any proxy and was only satisfiable through the monorepo replace directive, so consuming yaad as a dependency module failed to resolve. Require v0.1.5 (the newest tag that ships ServeHTTPWithShutdown, which yaad uses), record its go.sum hashes, and keep the replace for local development. Also drop the ServeSSE/ServeSSEWithShutdown wrappers in internal/server: they called mcpkit APIs added after v0.1.5 and had no callers in yaad, which would have kept the pinned version from compiling standalone.
…is configured synchronous, wal_autocheckpoint, temp_store, mmap_size, cache_size, and recursive_triggers are per-connection settings, but they were issued once through db.ExecContext, so only the pooled connection that served that call was configured; the other four connections ran on SQLite defaults. Move them into the _pragma= DSN list, which the modernc.org/sqlite driver applies on every new connection. Drop page_size (a no-op once the database file exists) and move PRAGMA optimize from startup to Close(), where SQLite recommends running it. Regression test pins all five pooled connections with open transactions and asserts each PRAGMA value.
…iables LoadNodeMetadata (and FillNodeMetadata on top of it) and CountEdgesBatch built a single IN (...) with every requested ID, ignoring the maxSQLVariables (900) host-parameter budget that GetNodesBatch, GetAllEdgesFor, and GetEdgesBetween already chunk at, so large ID sets could exceed SQLite's per-statement parameter limit. Both helpers now query in maxSQLVariables-sized chunks and merge per-chunk results; the misleading chunkedArgs helper is gone. Regression tests cover 950 IDs (two chunks) for both paths, with edges spanning the chunk boundary.
The backup scheduler discarded every RunNow error (_ = b.RunNow), so a failing backup pipeline was indistinguishable from a healthy one. The run loop now logs failures via log/slog and every RunNow outcome is recorded in the new BackupScheduler.Status() accessor (last success / error timestamps plus snapshot and failure counters). RotateBackups ignored all os.Remove errors; unexpected ones are now logged while a vanished file stays a non-event. Store.Backup now fsyncs the snapshot temp file before renaming it into place and best-effort fsyncs the backup directory afterwards, so a crash cannot leave a visible but not-yet-durable backup.
EnvKeyProvider.KeyBytes used the raw environment value as master key material no matter its length, so a short passphrase became an AES-256 key whose entropy was only that of the passphrase. Accept a raw string of at least 32 bytes, or a base64/hex value decoding to exactly 32 bytes (raw wins when a value qualifies both ways); anything else is rejected with an error naming the env var. Upgrade note: existing deployments with a shorter key are now rejected (hawk disables the yaad memory bridge with a logged warning rather than falling back to plaintext), and rows encrypted under a rejected short key cannot be decrypted by EnvKeyProvider anymore.
…one lock AcquireProcessLock keyed its in-process lock registry by the raw dbPath + ".lock" string, so lexically equivalent paths (dir/db, dir/./db, dir/x/../db) each opened the same lock file separately; flock treats separate open file descriptions as separate holders, making the second store open fail with a bogus 'database locked by another yaad process'. Normalize the lock path with filepath.Abs before the registry lookup.
- go.sum: drop hawk-mcpkit v0.1.5 hash lines — matches go mod tidy output in CI (the local replace makes them unnecessary) - storage/crypto.go: gofumpt reformat (CI fmt gate) - storage/backup.go: explicitly discard syncDir error (errcheck; the best-effort rationale is documented above the call)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Deep code-quality audit fixes for yaad:
deleteNodeQnow removesnode_signatures,file_watch,embeddings,node_versions,node_metadatachild rows (the FK-without-CASCADE path causedFOREIGN KEY constraint failedsilently — regression test included). Error-swallowing callers in decay/sparsify/improve now log failures instead of reportingremoved: 0hawk-mcpkit v0.1.5(wasv0.0.0— tag didn't exist, breakinggo get github.com/GrayCodeAI/yaad)synchronous/cache_size/temp_store/mmap_size/recursive_triggersinto the DSN_pragma=list so all 5 pooled connections enforce them (not just the first)LoadNodeMetadataandCountEdgesBatchnow chunk atmaxSQLVariables=900BackupScheduler.Status(); fsync on temp file before renameEnvKeyProviderrequires ≥32 bytes raw or exact 32 bytes decoded from base64/hexUpgrade note: users with short
YAAD_ENCRYPTION_KEYvalues (<32 bytes) will need to set a compliant key; existing rows encrypted under short keys can be recovered via a custom provider with the old material.Test plan
go build ./...(GOWORK=off)go test ./...— 33 packages, 0 failgo build ./...cleanTestDeleteNodeRemovesChildren— fails on old code, passes now