dnshelper/specs/001-safety-reliability-refactor/research.md

# Research: Safety, Reliability & Idiomatic Refactor

**Feature Branch**: `001-safety-reliability-refactor`  
**Date**: 2026-03-03  

## R-001: Replacing `github.com/miekg/dns` with `golang.org/x/net/dns/dnsmessage`

### Decision
Use `golang.org/x/net/dns/dnsmessage` (part of the approved `golang.org/x` source) to replace all DNS query/response functionality currently provided by `github.com/miekg/dns`.

### Rationale
- `dnsmessage` provides low-level DNS message building and parsing (A, CNAME, and all standard record types via `Message.Pack()`/`Message.Unpack()` or the streaming `Builder`/`Parser` API).
- The current usage surface of `miekg/dns` is narrow: build a query, send over UDP, parse A/CNAME responses. `dnsmessage` handles all of this.
- Eliminates 4 transitive dependencies (`x/mod`, `x/sys`, `x/tools` pulled in by `miekg/dns`) from the dependency tree.
- `golang.org/x/net` is already in the project's dependency list and is an approved source per the constitution.

### Go 1.20 Compatibility
- **`golang.org/x/net v0.35.0`** is the latest version with `go 1.18` directive (compatible with Go 1.20). Starting at `v0.36.0`, the module requires `go 1.23.0`.
- The `dnsmessage` sub-package API is stable and has not changed materially across these versions.
- **Pin to `golang.org/x/net v0.35.0`** in `go.mod`.

### Implementation Pattern
- Use `Message.Pack()` to build queries (simpler API, adequate for single-query CLI tool).
- Use `Message.Unpack()` to parse responses.
- Send raw DNS packets over UDP using `net.DialTimeout("udp", server+":53", timeout)`. No length prefix needed for UDP (only TCP requires a 2-byte length prefix).
- DNS names must be FQDN with trailing dot: `dnsmessage.MustNewName("example.com.")`.
- `AResource.A` is `[4]byte`, convert to string via `net.IP(r.A[:]).String()`.
- Response buffer of 1232 bytes is sufficient for standard UDP DNS responses.
- Validate response `Header.ID` matches query ID to prevent spoofing.
- Default 5-second timeout via `conn.SetDeadline()`.

### Key Type Mappings (miekg/dns → dnsmessage)

| miekg/dns | dnsmessage |
|-----------|------------|
| `dns.Client{}` + `c.Exchange()` | `net.DialTimeout("udp", ...)` + `conn.Write()`/`conn.Read()` |
| `dns.Msg{}.SetQuestion()` | `dnsmessage.Message{}.Pack()` |
| `dns.TypeA` | `dnsmessage.TypeA` |
| `dns.TypeCNAME` | `dnsmessage.TypeCNAME` |
| `*dns.A` → `rec.A.String()` | `*dnsmessage.AResource` → `net.IP(r.A[:]).String()` |
| `*dns.CNAME` → `rec.Target` | `*dnsmessage.CNAMEResource` → `r.CNAME.String()` |
| `dns.RcodeSuccess` | `dnsmessage.RCodeSuccess` |
| `dns.RcodeToString[code]` | `rcode.String()` (method on `RCode` type) |

### Alternatives Considered
1. **`net.Resolver` with custom `Dialer`**: The Go stdlib `net.Resolver` does not support querying a specific DNS server by address without hacky workarounds. The tool's core purpose requires directing queries to a user-specified resolver.
2. **Keep `miekg/dns`**: Violates Constitution Principle II (Standard Library First). The constitution explicitly calls this a "grandfather clause" dependency with preferred migration.
3. **Raw UDP + manual DNS wire format**: Would work but requires implementing DNS message serialization from scratch. `dnsmessage` already does this correctly and is an approved package.

---

## R-002: Atomic File Write Strategy (temp-file-and-rename)

### Decision
Write new hosts file content to a temporary file in the same directory, then `os.Rename()` over the original. Combined with a pre-write backup in the executable's directory.

### Rationale
- `os.Rename()` on same-volume is atomic (or effectively atomic) on all three target platforms:
  - **Linux**: POSIX `rename(2)` is atomic. Guaranteed by the kernel.
  - **macOS**: Darwin `rename(2)` follows POSIX semantics. Atomic on both APFS and HFS+.
  - **Windows**: Go's `os.Rename` calls `MoveFileEx` with `MOVEFILE_REPLACE_EXISTING`. Effectively atomic on NTFS (MFT metadata update), though Microsoft does not formally guarantee atomicity.
- The temp file **must** be in the same directory as the hosts file to avoid cross-filesystem/cross-volume failures (`EXDEV` on Unix, copy+delete on Windows).
- The original hosts file is never opened for writing or truncated.

### Go 1.20 Compatibility
- `os.CreateTemp(dir, pattern)` available since Go 1.16.
- `os.Rename()` stable across all Go versions.
- No known bugs in Go 1.20 affecting this pattern.

### Implementation Details

1. **Temp file creation**: `os.CreateTemp(filepath.Dir(hostsPath), ".dns-helper-tmp-*")` — creates file in same directory with recognizable prefix.
2. **Permissions**: The renamed file retains the temp file's permissions, NOT the original's. Must `os.Stat(hostsPath)` to get original mode, then `os.Chmod(tempFile, originalMode)` before renaming.
3. **Close before rename**: Required on Windows — cannot rename a file that's still open by the writing process.
4. **Sync before close**: Call `file.Sync()` for durability before closing.
5. **Cleanup on failure**: Use a `defer` with a success flag to remove the temp file if any step fails.
6. **Windows retry**: `os.Rename` on the hosts file may fail with `ERROR_SHARING_VIOLATION` if Windows Defender or the DNS Client service has a transient read lock. Implement 2-3 retry attempts with 100-200ms delay.
7. **Orphan cleanup**: On startup, optionally clean stale `.dns-helper-tmp-*` files in the hosts directory.

### Backup Strategy (FR-002)
- Before any write operation, copy the current hosts file to `<exe-dir>/hosts.bak.<date>-<random>` (e.g., `hosts.bak.20260303-a7f3`).
- The backup is a regular file copy (not rename), since it's in a different directory (exe dir vs hosts file dir).
- On successful rename of temp over hosts file: delete the backup.
- On rename failure: the original hosts file was never modified. Clean up temp file and backup.
- The date+random naming handles concurrent instances.

### Alternatives Considered
1. **Write directly to hosts file (current approach)**: Uses `O_WRONLY|O_TRUNC`, which immediately truncates the file. A crash during write leaves it empty/partial. This is the bug being fixed.
2. **Backup-only (no atomic rename)**: Would still truncate the hosts file during write. The backup only helps after a failure, not during.
3. **Write to temp, then copy content over original**: The copy step is not atomic — a crash during copy is the same as a crash during direct write.

---

## R-003: Lock File Implementation

### Decision
Use `os.OpenFile` with `O_CREATE|O_EXCL` to create a lock file in the executable's directory. Use timeout-based staleness detection (file modification time).

### Rationale
- `O_CREATE|O_EXCL` is atomic on all three platforms (Windows maps to `CREATE_NEW`, Unix maps to `O_CREAT|O_EXCL`).
- Single code path — no platform-specific build files needed for the lock mechanism.
- Timeout-based staleness avoids the cross-platform pitfalls of PID-based detection (Windows doesn't support `Signal(0)` for process existence checks without `x/sys`).

### Implementation Details

| Parameter | Value | Rationale |
|-----------|-------|-----------|
| Lock file name | `.dns-helper.lock` | Hidden file, recognizable prefix |
| Lock location | Executable's directory | Same as backup files; writable by admin/root (required anyway) |
| Stale timeout | 2 minutes | Lock held only during read-modify-write (<1s normally). 2min is very conservative. |
| Retry interval | 200ms | Short enough to be responsive, long enough to avoid busy-wait |
| Max wait | 2 seconds | 10 retries. Enough for another instance's write cycle. |

**Lock protocol per FR-016:**
1. Parse args and validate input.
2. Perform DNS resolution (before lock acquisition to minimize lock hold time).
3. Acquire lock (retry up to 2s, break stale locks >2min).
4. Read hosts file → modify content → write temp file → rename over hosts file.
5. Release lock (delete lock file).
6. Lock release in `defer` to handle error paths and panics.

**Staleness detection:**
- `os.Stat(lockFile).ModTime()` — if older than 2 minutes, `os.Remove()` and retry.
- Cross-platform via Go stdlib. No platform-specific code needed.

**Signal handling:**
- `defer lock.Release()` handles normal exit and errors.
- For SIGKILL/power loss, the stale lock persists. Next invocation detects and breaks it after 2 minutes.
- Write PID to lock file for debugging (human inspection), but don't use it for staleness logic.

### Alternatives Considered
1. **Advisory file locking (`flock`/`LockFileEx`)**: Auto-releases on crash (no stale lock problem), but requires 3 platform-specific build files. `syscall.Flock` doesn't exist on Windows. Adds complexity for marginal benefit.
2. **PID-based staleness**: More precise than timeout, but `os.FindProcess` + `Signal(0)` doesn't work on Windows without `golang.org/x/sys/windows`. Defeats the single-code-path advantage.
3. **No locking**: Risk of two instances writing the hosts file simultaneously. The spec explicitly requires serialization (FR-016).

---

## R-004: Multi-Host Deletion Bug Analysis

### Decision
Fix the nested loop logic in `removeHostsFromExistingContent` by inverting the loop order (iterate lines on the outside, check all hosts on the inside).

### Rationale — Bug Analysis

Current code:
```go
func removeHostsFromExistingContent(data *workingData) {
    var updatedContent []string
    for _, host := range data.Hosts {
        for _, line := range data.ExistingContent {
            if !strings.Contains(line, host) {
                updatedContent = append(updatedContent, line)
            }
        }
    }
    data.NewContent = updatedContent
}
```

**Bug**: The inner loop always iterates over `data.ExistingContent` (the original, unfiltered content). `updatedContent` is never reset between outer iterations. Result:
- Lines removed by one host are re-added by another host's pass (re-scans original).
- Surviving lines are duplicated once per host in `data.Hosts`.

**Trace** with `Hosts = ["host1", "host2"]`, `ExistingContent = ["1.1.1.1\thost1", "2.2.2.2\thost2", "3.3.3.3\thost3"]`:
- Pass 1 (host1): keeps `host2`, `host3` → `updatedContent = [host2, host3]`
- Pass 2 (host2): re-scans original, keeps `host1`, `host3` → `updatedContent = [host2, host3, host1, host3]`
- **Result**: Both hosts survive, `host3` is duplicated. Every invariant violated.

### Correct Algorithm
Iterate lines on the outside; for each line, check if it matches ANY host:
```go
func removeHostsFromExistingContent(data *workingData) {
    var updatedContent []string
    for _, line := range data.ExistingContent {
        shouldRemove := false
        for _, host := range data.Hosts {
            if strings.Contains(line, host) {
                shouldRemove = true
                break
            }
        }
        if !shouldRemove {
            updatedContent = append(updatedContent, line)
        }
    }
    data.NewContent = updatedContent
}
```

---

## R-005: CNAME Resolution Duplicate IP Bug Analysis

### Decision
Fix the self-appending loop in the CNAME resolution path of `lookupIP()` and add a max-depth limit for CNAME chain following.

### Rationale — Bug Analysis

Current code:
```go
for _, ans := range r.Answer {
    if rec, ok := ans.(*dns.CNAME); ok {
        ips = lookupIP(rec.Target, resolver)
        for _, ip := range ips {
            ips = append(ips, ip)
        }
    }
}
```

**Bug**: `ips` is the result of the recursive `lookupIP` call. The `for range` then iterates over `ips` and appends each element back to itself. In Go, `for range` evaluates the slice length once at the start, so this is not infinite — it exactly doubles every element. With `ips = ["10.0.0.1", "10.0.0.2"]`, the result is `["10.0.0.1", "10.0.0.2", "10.0.0.1", "10.0.0.2"]`.

**Additional issue**: No CNAME chain depth limit. Circular CNAMEs would cause infinite recursion.

### Correct Algorithm
```go
for _, ans := range r.Answer {
    if rec, ok := ans.(*dns.CNAME); ok {
        cnameIPs := lookupIP(rec.Target, resolver)
        ips = append(ips, cnameIPs...)
    }
}
```
Plus a max-depth parameter (10 levels) to prevent infinite CNAME recursion.

---

## R-006: Go 1.20 Dependency Cleanup

### Decision
Update `go.mod` to `go 1.20`. Pin `golang.org/x/net` to `v0.35.0`. Remove `github.com/miekg/dns` and all its transitive dependencies.

### Rationale

Current `go.mod` dependencies:
```
github.com/miekg/dns v1.1.59     ← removing (FR-014)
golang.org/x/mod v0.16.0         ← transitive dep of miekg/dns, will be removed
golang.org/x/net v0.22.0         ← keeping, updating to v0.35.0
golang.org/x/sys v0.18.0         ← transitive dep of x/net, will be updated
golang.org/x/tools v0.19.0       ← transitive dep of miekg/dns, will be removed
```

After cleanup:
```
golang.org/x/net v0.35.0         ← for dns/dnsmessage
golang.org/x/sys (transitive)    ← pulled by x/net
```

`golang.org/x/mod` and `golang.org/x/tools` are only needed by `miekg/dns` and will be removed by `go mod tidy` after removing the `miekg/dns` import.

### Version Verification
- `golang.org/x/net v0.35.0`: `go 1.18` directive — compatible with Go 1.20.
- `golang.org/x/net v0.36.0`: `go 1.23.0` directive — **NOT compatible**. Must pin at v0.35.0.

---

## R-007: Testability Design for TDD (Constitution Principle VI)

### Decision
Define interfaces for external dependencies (filesystem, DNS, lock) to enable testing with fakes/stubs. All business logic operates on interfaces, not concrete OS calls.

### Rationale
Constitution Principle VI mandates TDD with testable interfaces. The current codebase has zero tests and directly calls OS-level I/O from business logic.

### Interface Design

**Filesystem interface** (for `hostfile` package):
```go
type FileSystem interface {
    ReadFile(path string) ([]byte, error)
    WriteFile(path string, data []byte, perm os.FileMode) error
    Stat(path string) (os.FileInfo, error)
    CreateTemp(dir, pattern string) (*os.File, error)
    Rename(oldpath, newpath string) error
    Remove(path string) error
}
```

**Resolver interface** (for `resolver` package):
```go
type Resolver interface {
    LookupIP(hostname, server string) ([]string, error)
}
```

**Lock interface** (for `lockfile` package):
```go
type Lock interface {
    Release() error
}
type Locker interface {
    Acquire(dir string) (Lock, error)
}
```

Production code uses real implementations. Tests inject fakes that simulate errors, crashes, and edge cases without touching the real filesystem or DNS.

### Test Categories
1. **Unit tests**: Managed block parsing, line removal, deduplication, input validation — pure logic, no I/O.
2. **Integration tests with fakes**: Atomic write sequence, backup creation/cleanup, lock acquisition/release — use in-memory filesystem fakes.
3. **Platform tests**: `getHostsFileLocation()` on each OS — verify return value and error handling against build tags.

---

## R-008: Managed Block Corruption Detection (FR-011)

### Decision
Detect and report four corruption scenarios for managed block markers. Refuse to modify the file when corruption is detected.

### Rationale
The current `extractLinesToEdit` function detects "start without end" via a `fileCorrupted` flag, but the error message ("content has been corrupted") is vague and doesn't cover all cases.

### Corruption Scenarios

| Scenario | Detection | Error Message |
|----------|-----------|---------------|
| Start marker without end marker | `startIndex > 0 && endIndex == 0` | "managed block is corrupt: start marker found at line N but no end marker" |
| End marker without start marker | `startIndex == 0 && endIndex > 0` | "managed block is corrupt: end marker found at line N but no start marker" |
| End marker before start marker | `endIndex < startIndex` | "managed block is corrupt: end marker (line N) appears before start marker (line M)" |
| Duplicate start or end markers | Multiple occurrences detected during scan | "managed block is corrupt: duplicate start/end markers found" |

All corruption scenarios must result in an error returned to the caller. The hosts file must not be modified.

### Current Code Bug
The current code uses `startIndex == 0` to mean "not found," but index 0 is a valid line position (the first line). Should use a sentinel value (-1) or a boolean flag for "found/not found."