Commit Graph

8 Commits

Author SHA1 Message Date
beads/crew/dave
ec1a32b9a8 fix(storage): add reconnectMu RLock protection to prevent race condition (#1054)
Add missing reconnectMu.RLock() protection to storage methods that were
vulnerable to the same race condition fixed in GH#607. The FreshnessChecker
can trigger reconnect() which closes s.db while queries are in flight,
causing "database is closed" errors during daemon export operations.

Protected methods:
- labels.go: GetLabelsForIssues (GetLabels intentionally unprotected - called from GetIssue which holds lock)
- comments.go: GetIssueComments, GetCommentsForIssues
- dependencies.go: GetDependencyCounts, GetDependencyRecords, GetAllDependencyRecords, GetDependencyTree, loadDependencyGraph
- config.go: SetConfig, GetConfig, GetAllConfig, DeleteConfig, SetMetadata, GetMetadata
- dirty.go: MarkIssueDirty, GetDirtyIssues, GetDirtyIssueHash, GetDirtyIssueCount
- events.go: GetEvents, GetStatistics, GetMoleculeProgress
- hash.go: All hash methods
- hash_ids.go: GetNextChildID, ensureChildCounterUpdated (getNextChildNumber unprotected - called internally)

Internal helpers called from already-locked contexts intentionally omit
RLock to avoid deadlock (Go's RWMutex doesn't support recursive locking).

Fixes: bd-vx7fp

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Executed-By: beads/crew/dave
Rig: beads
Role: crew
2026-01-12 18:29:42 -08:00
Erick Matsen
3f2b693bea fix: respect hierarchy.max-depth config setting (GH#995) (#997)
* fix: respect hierarchy.max-depth config setting (GH#995)

The hierarchy.max-depth config setting was being ignored because storage
implementations had the depth limit hardcoded to 3. This fix:

- Registers hierarchy.max-depth default (3) in config initialization
- Adds hierarchy.max-depth to yaml-only keys for config.yaml storage
- Updates SQLite and Memory storage to read max depth from config
- Adds validation to reject hierarchy.max-depth values < 1
- Adds tests for configurable hierarchy depth

Users can now set deeper hierarchies:
  bd config set hierarchy.max-depth 10

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

* refactor: extract shared CheckHierarchyDepth function (GH#995)

- Extract duplicated depth-checking logic to types.CheckHierarchyDepth()
- Update sqlite and memory storage backends to use shared function
- Add t.Cleanup() for proper test isolation in sqlite test
- Add equivalent test coverage for memory storage backend
- Add comprehensive unit tests for CheckHierarchyDepth function

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
2026-01-10 13:36:52 -08:00
Steve Yegge
1e7c7f5e54 fix(sqlite): update child_counters when explicit child IDs are created (GH#728)
When creating issues with explicit hierarchical IDs (e.g., bd-test.1, bd-test.2
via --id flag or import), the child_counters table was not being updated.
This caused GetNextChildID to return colliding IDs when later called with
--parent.

Changes:
- Add ensureChildCounterUpdatedWithConn() to update counter on explicit child creation
- Add ParseHierarchicalID() to extract parent and child number from IDs
- Update CreateIssue to call counter update after hierarchical ID validation
- Update EnsureIDs to call counter update when parent exists
- Add post-insert phase in batch operations to update counters after FK
  constraint can be satisfied
- Update tests to reflect new behavior where counter is properly initialized

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2025-12-24 13:22:55 -08:00
Steve Yegge
e816e91ecb Complete bd-ar2 P2 tasks: metadata, resurrection, and testing improvements
This commit addresses the remaining P2 tasks from bd-ar2 code review follow-up:

## Completed Tasks

### bd-ar2.4: Improve parent chain resurrection
- Modified `tryResurrectParentWithConn()` to recursively resurrect ancestor chain
- When resurrecting bd-root.1.2, now also resurrects bd-root.1 if missing
- Handles deeply nested hierarchies where intermediate parents are deleted
- All resurrection tests pass including new edge cases

### bd-ar2.5: Add error handling guidance
- Documented metadata update failure strategy in `updateExportMetadata()`
- Explained trade-off: warnings vs errors (safe, prevents data loss)
- Added user-facing message: "Next export may require running 'bd import' first"
- Clarifies that worst case is requiring import before next export

### bd-ar2.6: Document transaction boundaries
- Added comprehensive documentation for atomicity trade-offs
- Explained crash scenarios and recovery (bd import)
- Documented decision to defer defensive checks (Option 4) until needed
- No code changes - current approach is acceptable for now

### bd-ar2.12: Add metadata key validation
- Added keySuffix validation in `updateExportMetadata()` and `hasJSONLChanged()`
- Prevents ':' separator in keySuffix to avoid malformed metadata keys
- Documented metadata key format in function comments
- Single-repo: "last_import_hash", multi-repo: "last_import_hash:<repo_key>"

### bd-ar2.7: Add edge case tests for GetNextChildID resurrection
- TestGetNextChildID_ResurrectParent_NotInJSONL: parent not in history
- TestGetNextChildID_ResurrectParent_NoJSONL: missing JSONL file
- TestGetNextChildID_ResurrectParent_MalformedJSONL: invalid JSON lines
- TestGetNextChildID_ResurrectParentChain: deeply nested missing parents
- All tests pass, resurrection is robust against edge cases

## Files Changed
- cmd/bd/daemon_sync.go: Metadata validation, error handling docs
- cmd/bd/integrity.go: Added strings import, keySuffix validation
- internal/storage/sqlite/hash_ids.go: Improved resurrection comments
- internal/storage/sqlite/resurrection.go: Recursive ancestor resurrection
- internal/storage/sqlite/child_id_test.go: Added 4 new edge case tests

## Testing
All export, sync, metadata, and resurrection tests pass.
Edge cases properly handled: missing JSONL, malformed JSON, deep nesting.

## Remaining Tasks
- bd-ar2.8 (P3): Additional export metadata edge case tests (deferred)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
2025-11-21 11:40:37 -05:00
Steve Yegge
cd8cb8b86a Fix bd-ar2 code review issues: metadata tracking and multi-repo support
This commit addresses critical code review findings from bd-dvd and bd-ymj fixes:

## Completed Tasks

### bd-ar2.1: Extract duplicated metadata update code
- Created `updateExportMetadata()` helper function
- Eliminated 22-line duplication between createExportFunc and createSyncFunc
- Single source of truth for metadata updates

### bd-ar2.2: Add multi-repo support to export metadata updates
- Added per-repo metadata key tracking with keySuffix parameter
- Both export and sync functions now update metadata for all repos

### bd-ar2.3: Fix tests to use actual daemon functions
- TestExportUpdatesMetadata now calls updateExportMetadata() directly
- Added TestUpdateExportMetadataMultiRepo() for multi-repo testing
- Fixed export_mtime_test.go tests to call updateExportMetadata()

### bd-ar2.9: Fix variable shadowing in GetNextChildID
- Changed `err` to `resurrectErr` to avoid shadowing
- Improves code clarity and passes linter checks

### bd-ar2.10: Fix hasJSONLChanged to support per-repo keys
- Updated hasJSONLChanged() to accept keySuffix parameter
- Reads metadata with correct per-repo keys
- All callers updated (validatePreExport, daemon import, sync command)

### bd-ar2.11: Use stable repo identifiers instead of paths
- Added getRepoKeyForPath() helper function
- Uses stable identifiers like ".", "../frontend" instead of absolute paths
- Metadata keys now portable across machines and clones
- Prevents orphaned metadata when repos are moved

## Files Changed
- cmd/bd/daemon_sync.go: Helper functions, metadata updates
- cmd/bd/integrity.go: hasJSONLChanged() with keySuffix support
- cmd/bd/sync.go: Updated to use getRepoKeyForPath()
- cmd/bd/*_test.go: Tests updated for new signatures
- internal/storage/sqlite/hash_ids.go: Fixed variable shadowing

## Testing
All export, sync, and integrity tests pass.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
2025-11-21 11:28:25 -05:00
Steve Yegge
4c5f99c5bd Fix bd-dvd and bd-ymj: Parent resurrection and export metadata
Bug 1 (bd-dvd): GetNextChildID now attempts parent resurrection from JSONL
before failing. Added TryResurrectParent call to match CreateIssue behavior.

Bug 2 (bd-ymj): Export now updates last_import_hash metadata to prevent
'JSONL content has changed' errors on subsequent exports.

Files changed:
- internal/storage/sqlite/hash_ids.go: Add resurrection attempt
- cmd/bd/daemon_sync.go: Add metadata updates after export
- Tests added for both fixes
- Fixed pre-existing bug in integrity_content_test.go

Follow-up work tracked in epic bd-ar2 (9 issues for improvements).

Fixes GH #334
2025-11-21 10:29:30 -05:00
Steve Yegge
2da7487b69 Refactor: consolidate ID generation and shared helpers (bd-0702, bd-1445)
- Created ids.go with ValidateIssueIDPrefix, GenerateIssueID, EnsureIDs
- Created issues.go with insertIssue/insertIssues helpers
- Created events_helpers.go with recordCreatedEvent/recordCreatedEvents
- Created dirty_helpers.go with markDirty/markDirtyBatch
- Refactored sqlite.go and batch_ops.go to use new helpers
- Removed duplicate code from hash_ids.go

Amp-Thread-ID: https://ampcode.com/threads/T-b1ab5a16-96de-4e4d-b255-3617055a89eb
Co-authored-by: Amp <amp@ampcode.com>
2025-11-02 14:49:27 -08:00
Steve Yegge
b5db80c412 Refactor sqlite.go: Extract hash IDs, batch ops, validators (bd-90a5, bd-c796, bd-d9e0)
- Extract hash ID generation to hash_ids.go (bd-90a5)
  - generateHashID, getNextChildNumber, GetNextChildID
  - Reduced sqlite.go from 1880 to 1799 lines

- Extract batch operations to batch_ops.go (bd-c796)
  - validateBatchIssues, generateBatchIDs, bulkInsertIssues
  - bulkRecordEvents, bulkMarkDirty, CreateIssues
  - Reduced sqlite.go from 1799 to 1511 lines

- Extract validation functions to validators.go (bd-d9e0)
  - validatePriority, validateStatus, validateIssueType
  - validateTitle, validateEstimatedMinutes, validateFieldUpdate
  - Reduced sqlite.go from 1511 to 1447 lines

- Add comprehensive validator tests (bd-3b7f)
  - validators_test.go with full coverage

Total reduction: 2298 → 1447 lines (851 lines extracted, 37% reduction)

Part of epic bd-fc2d to modularize sqlite.go
All tests pass

Amp-Thread-ID: https://ampcode.com/threads/T-09c4383b-bc5c-455e-be24-02b4f9df7d78
Co-authored-by: Amp <amp@ampcode.com>
2025-11-01 19:55:48 -07:00