feat-cleanup: protocol split, build-tag gating, lifecycle hardening, CI matrix#6
Merged
feat-cleanup: protocol split, build-tag gating, lifecycle hardening, CI matrix#6
Conversation
Rename module from `github.com/pgodw/omnitalk/go` to `github.com/pgodw/omnitalk`. The repository root is the module root; the `/go` suffix was redundant and confusing for tooling and newcomers. All import sites rewritten across the codebase. Go doc comments in CLAUDE.md updated to match. Step 0 of the architectural refactor plan (.claude/plans/review-the-architectural-layout-splendid-squirrel.md). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The appletalk/ package was a thin re-export of protocol/ddp/ and encoding/ types, adding no value beyond a layer of aliases. Every consumer now imports the underlying package directly. - appletalk.Datagram → ddp.Datagram - appletalk.DDPChecksum → ddp.Checksum - appletalk.MaxDataLength → ddp.MaxDataLength - appletalk.DatagramFromLongHeaderBytes → ddp.DatagramFromLongHeaderBytes - appletalk.DatagramFromShortHeaderBytes → ddp.DatagramFromShortHeaderBytes - appletalk.MacRomanToUpper → encoding.MacRomanToUpper - appletalk.MacRomanToUTF8 → encoding.MacRomanToUTF8 - appletalk.UTF8ToMacRoman → encoding.UTF8ToMacRoman - appletalk.Packet → new protocol.Packet (in new protocol/ package) The Packet interface moves to the new top-level protocol/ package, which will host cross-protocol contracts as the wire-format code is lifted out of service/ in later steps. Step 1 of the architectural refactor plan. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Introduces pkg/hwaddr/ with Ethernet, LocalTalk, and AppleTalk types plus parsing, formatting, generation, and conversion helpers. Consolidates duplicated MAC parsing previously in cmd/omnitalk's parseMAC and service/macip's fabricateMACForAT. - cmd/omnitalk/main.go: parseMAC replaced with hwaddr.ParseEthernet; helper deleted. - service/macip/dhcp_client.go: fabricateMACForAT delegates to hwaddr.MacIPEthernetFromAppleTalk, preserving the exact on-wire layout used by existing DHCP leases (0x02 | net | node | 'M' | 'I'). - Round-trip and validation tests in pkg/hwaddr/. Further call-site migrations (ethertalk.New accepting hwaddr.Ethernet, port/localtalk GenerateLocalTalk adoption) will land alongside Step 10 (BridgeConfigurable / ethertalk.Options) where the surrounding signatures change anyway. Step 2 of the architectural refactor plan. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Creates internal/testutil/ with exported MockPort and MockRouter plus NewMockPort/NewMockRouter constructors. Replaces the two duplicated mock files that previously lived in service/ and service/zip/. - service/mock_router_port_test.go: deleted. The service package had no test consumers; this file was dead code. - service/zip/mock_test.go: reduced to a pair of type aliases so the existing zip tests keep compiling under their familiar names. - service/zip/name_information_test.go: mock factories now delegate to testutil.NewMockPort/NewMockRouter; field-mutation sites updated to exported casing. NewAFPHarness is intentionally deferred to Step 15 when AFP decomposition lands; building it now would freeze an API that step will reshape. Step 4 of the architectural refactor plan. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Eight stdlib log.Printf calls replaced with netlog.Info / netlog.Warn so every port/ethertalk log line flows through the project's shared logger. The "log" stdlib import is removed from the package. netlog remains the interim transport; Step 6 replaces it with pkg/logging (slog-based) and Step 7 migrates all call sites. Step 5 of the architectural refactor plan. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
pkg/logging is a thin slog wrapper providing per-component source tags ([AFP], [ASP], ...) rendered as a prefix in console output and emitted as "source":"AFP" in JSON, with dual-mode sink fan-out so console and JSON can run simultaneously. pkg/logging/protolog is an opt-in channel for raw wire traffic (per-source, per-direction filtering; console, JSON sinks). netlog stays functional as a shim writing through stdlib log so every existing call site keeps its current behaviour until step 7 migrates them. The netlog package body is otherwise untouched; only the header comment changes to mark it deprecated. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Rather than thread a *slog.Logger through every service constructor (a
30-file, 349-call-site mechanical rewrite that would stall the rest of
the refactor), wire the netlog shim to forward to a slog.Logger that
main.go installs at startup. A pkg/logging root logger is built with a
console sink at the configured level, set as slog.Default, and handed
to netlog.SetLogger.
The shim keeps dual-mode behaviour: when no logger is installed (e.g.
in tests that haven't initialised slog), Debug/Info/Warn still emit
through stdlib log for backward-compat with tests that capture
log.SetOutput. Once SetLogger runs, every netlog call flows through
slog with the "source":"OmniTalk" attribute; sub-components get their
own source tags in a follow-up pass that replaces netlog call sites
with logger.With("source", "AFP")-style constructors.
This delivers the plan's behavioural outcome (structured slog output
with handler fan-out available) while deferring the ergonomic
constructor threading to a later commit.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
INI parsing and defaulting move out of cmd/omnitalk into a new top-level config/ package. config.Root, config.LoadINI, and config.Defaults are the entry points; cmd/omnitalk keeps a thin compat shim aliasing config.Root as iniConfig so main.go and its tests continue to compile without a mass rename pass. This is the lift-and-shift cut. Later commits split Root into per- service subtrees (RouterConfig, AFPConfig, etc.) and add Validate methods; the move here is what unblocks those later cuts, since services may now import config without depending on cmd/omnitalk. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The AFP-agnostic pieces of service/afp/cnid.go and sqlite_store.go move to pkg/cnid: the Store interface, MemoryStore, SQLiteStore, and the SQLite helper (OpenSQLiteDB, SQLitePath, SQLiteFilename). Constants are renamed to drop the CNID prefix (cnid.Root, cnid.Invalid, cnid.ParentOfRoot) since the package name already provides that context. service/afp keeps the AFP-coupled backend adapters (MemoryCNIDBackend, SQLiteCNIDBackend) and aliases the pkg/cnid types so the ~16 existing callers inside AFP (volume, fork, directory, path_codec, tests) keep compiling unchanged. The SQLite Desktop DB helper now calls cnid.OpenSQLiteDB directly so the same per-volume database can back both CNID and Desktop storage without duplicating the pragma setup. Follow-ups: tighten the backend registry (plan step 21) so third- party filesystems can plug their own Store implementation; move the CNID backend selection out of AFPOptions into the volume config subtree. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… PcapPort in main Step 10 of the architectural refactor. Introduces port.BridgeConfigurable as an optional interface for ports that need operator control over Ethernet bridge mode and host-MAC synthesis. cmd/omnitalk/main.go now obtains these knobs via interface assertion instead of depending on the concrete *ethertalk.PcapPort type. This keeps transports that don't need bridge configuration (LocalTalk, LToUDP, future virtual ports) from being forced to implement stubs, and sets up the eventual move to construction-time PortOptions. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ink via constructor Step 11 of the architectural refactor. The plan called for MacIP to accept an externally-constructed rawlink.RawLink rather than opening its own. That shape is already present: - service/macip/macip.go: Service.New takes ipLink rawlink.RawLink. - service/macip/etherlink.go: newEtherIPLink takes rawlink.RawLink. - cmd/omnitalk/main.go: opens the pcap RawLink and injects it. No macip.Transport interface is warranted: etherlink.go only needs Read/Write/Close, which RawLink already provides. Verified with go test ./service/macip/... and go test -tags integration ./service/macip/... Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ContinuePacket Step 12 of the architectural refactor. Adds pkg/binutil with allocation- conscious Put/Get helpers for fixed-width big-endian integers and Pascal strings, plus the canonical Wire interface shape documented in the package doc comment. Pilot migration: ASP WriteContinuePacket now implements MarshalWire/UnmarshalWire/WireSize via binutil. The legacy MarshalData method is retained as a thin adapter so existing call sites keep working; follow-up steps migrate remaining models. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…itial counters Step 13 of the architectural refactor. Adds pkg/telemetry with Counter, Gauge, and Histogram types backed by expvar (stdlib, zero deps). A future //go:build otel file can replace the expvar publishers with an OpenTelemetry backend without touching call sites. Initial counters wired: - omnitalk_router_frames_in_total: incremented on every Router.Inbound - omnitalk_afp_commands_total: incremented on every AFP HandleCommand - omnitalk_aarp_probe_retries_total: incremented on every AARP probe retry Metric declarations live in per-package metrics.go files to avoid churning existing imports. The Prometheus-style naming convention is documented in the package doc comment. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
First slice of Step 14. ATPHeader now implements MarshalWire/ UnmarshalWire/WireSize via pkg/binutil. The legacy Marshal/Unmarshal methods become thin adapters to keep existing protocol.Packet callers working unchanged. Adds a golden hex round-trip test plus short-buffer rejection so any future regression in the wire layout fails loudly. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Second slice of Step 14. DSI Header now implements MarshalWire/ UnmarshalWire/WireSize via pkg/binutil. Legacy Marshal/Unmarshal methods are retained as adapters so existing call sites in dsi.go continue to work. Adds golden hex round-trip + short-buffer rejection tests. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Step 16 of the architectural refactor. The AppleDouble v2 sidecar format (parse/build, magic/version/entry-id constants, SidecarPath) is generic to any AFP-style server and is no longer buried in service/afp. service/afp/appledouble.go shrinks to a thin alias layer that keeps the historical lowercase identifiers compiling. AFP I/O code (appledouble_backend.go, desktop_rebuild.go) updates to the exported field names on appledouble.Parsed. The AFP-specific ForkMetadataBackend stays in service/afp/ — it depends on Volume config and disk I/O. A future step may relocate it to service/afp/fork/ once the package is decomposed (Step 15). Adds round-trip tests for the new pkg/appledouble package. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Step 17 of the architectural refactor. The pre-existing FrameSender interface in port/localtalk/localtalk.go was unused; backends plugged into Port via a ConfigureSendFrame(closure) call. Adds SetFrameSender as the interface-shaped counterpart and migrates the LToUDP and TashTalk backends to it. Each backend now exposes an exported SendFrame method that satisfies FrameSender, with the legacy unexported sendFrame retained as the implementation. ConfigureSendFrame is kept for the virtual/test backends that pass ad-hoc closures. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Step 20 of the architectural refactor. ARCHITECTURE.md is the entry
point for new contributors: module map, layering rules, core
interfaces, configuration model, logging/telemetry shape, wire codec
convention, timer patterns, AFP architecture, and a glossary.
Reflects the package layout as of Steps 1–17: protocol/ddp lifted
out of appletalk/, pkg/{binutil,appledouble,cnid,logging,telemetry}
in place, MacIP consuming rawlink, BridgeConfigurable extracted, and
LocalTalk backends wired through FrameSender.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add FileSystemFactory + RegisterFS/NewFS in fs.go. local_fs.go and macgarden_fs.go self-register in init(). newBackendForVolumeConfig dispatches via the registry instead of switching on FSType, so future backends register themselves without modifying core AFP.
macgarden_fs.go and its tests now require -tags macgarden. A stub file under !macgarden registers a factory that returns ErrMacGardenDisabled so config validation still surfaces a clear message. NormalizeFSType now consults the registry instead of hardcoding the supported types, which keeps validation honest across build-tag combinations. Default builds shed the macgarden HTTP client dependency surface.
The default build now skips modernc.org/sqlite (verified via
'go list -deps ./cmd/omnitalk' before/after — modernc.org/sqlite drops
out without the tag). pkg/cnid/sqlite_stub.go provides no-op
SQLiteStore methods plus error-returning constructors so service/afp's
existing fallback paths ('sqlite init failed; falling back to memory')
route through the in-memory CNID + memory desktop DB stores
transparently.
AFP and macip tagging deferred — those are multi-day refactors
requiring 60+ files tagged plus stub packages for the public API
surface main.go imports. Tracking separately.
Four high-priority sites flagged in the plan now honour service shutdown instead of blocking on hardcoded timeouts: * service/asp: Service.Start spins up a lifeCtx; Stop cancels it. The three pending.Wait(context.Background()) sites — completeWrite, sendTickle, SendAttention — now use s.drainCtx() so background ATP transaction releases stop promptly when the server quits. * service/llap: the CTS-wait select grew <-st.stop and <-s.stop arms alongside the timer; time.After replaced with time.NewTimer so the timer is properly stopped on early return. * service/macip dhcp_client: dhcpClient now carries the service stop channel; RequestIP returns nil immediately on shutdown rather than waiting up to dhcpTimeout. * service/macip etherlink: resolveMAC's ARP-reply wait selects on l.stop alongside the timer; the waiter cleanup logic moved into dropARPWaiter so timeout and shutdown paths share it. Step 24 deferred work: full ctx-as-first-arg propagation through service.Service / port.Port / atp.Endpoint is a multi-day refactor with cascading signature changes — out of scope for this pass.
AFPService gains a stop channel and WaitGroup; the desktop-DB rebuild goroutine kicked off by NewAFPService is now tracked and Stop() waits for it. Stop also closes any volume FileSystem that exposes Close(). MacGardenFileSystem gains the same lifecycle pair plus a Close() method. The fire-and-forget HeadContentLength probe loop checks the stop channel between asset fetches so service shutdown drains cleanly instead of leaking long-running HTTP requests. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds doc.go to packages that previously had no // Package X ... block: router, port/ethertalk, port/localtalk, service/llap, service/rtmp, service/zip, service/dsi, service/aep, service/macgarden, and encoding. Each one-paragraph stub points at the relevant spec/ doc so go doc / pkg.go.dev surface the right context. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Renames inside the afp package and its sole external caller: AFPService -> Service AFPOptions -> Options NewAFPService -> NewService DefaultAFPOptions -> DefaultOptions Mechanical gofmt -r rewrites; no behaviour change. Both default and -tags macgarden builds pass; afp + cmd test suites green. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replaces the single %v formatting of the AFPService.Stop error slice with errors.Join + %w so callers can errors.Is/As against any of the underlying transport or backend Close errors. Wider audit: the codebase already wraps with %w everywhere a chained error is reported, and the existing reliance on stdlib sentinels (fs.ErrPermission, fs.ErrNotExist) covers the categories the plan suggested adding bespoke AFP sentinels for. No new sentinels needed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Fuzz both DatagramFromLongHeaderBytes (with and without checksum verification) and DatagramFromShortHeaderBytes. The harness only asserts non-panic — any malformed inbound frame must surface as an error, never as a runtime crash on the router fast path. Smoke run at -fuzztime=2s exercises ~320k inputs cleanly. Future protocol/ packages (atp, asp, dsi, afp wire models) get the same treatment when their wire code lands behind the protocol/* split. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Adds two jobs to .github/workflows/pr-ci.yml: - quality: go vet, go test -race, golangci-lint, govulncheck - build-tags: matrix builds with "" and "macgarden" tags so the optional macgarden VFS keeps compiling alongside the default Adds .golangci.yml enabling errcheck, errorlint, govet, ineffassign, misspell, staticcheck, unused (errcheck off in tests). Fixes the one go vet finding the new gate would surface: ASP SessionManager.Close() was copying a Session containing sync.Mutex into a local snap before invoking the onClose callback. Callers only need a pointer (the session is already removed from the map and its writers are cancelled), so the snap is gone and onClose receives sess directly. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Embedded routing builds don't need the IP-over-AppleTalk gateway. Tag every file in service/macip behind //go:build macip and split the cmd-layer wiring into a tag-neutral hook interface (macip_hook.go) plus enabled/disabled implementations. main.go now calls wireMacIP via the interface and never imports service/macip directly. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- golangci-lint: add gocritic + revive; build under -tags=all so afp/macgarden/macip files lint too - race tests, govulncheck, build matrix all use -tags all (or include sqlite_cnid row) - new gosec job over the network-input-parsing surface (macip, macgarden) - Makefile with build/test/test-race/lint/vuln/gosec/fuzz targets for local dev Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- protocol/asp: FuzzParseCommandPacket/Write/OpenSess to round out the
per-protocol fuzz set (ddp/atp/llap/nbp already had them).
- protocol/atp: BenchmarkHeader{Marshal,Unmarshal,RoundTrip}Wire as a
zero-alloc baseline so the binutil migration regressions surface.
- encoding/macroman: t.Parallel() on the three pure-function tests.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
NewClient previously fired a GET against macintoshgarden.org during construction, so even tests that swap in mock RoundTrippers triggered real network hits before reaching the assertion. Move the prime into a new Client.Prime() method that production callers invoke explicitly, and let tests skip it. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The plan's netlog->slog migration was always aspirational: only one production caller (cmd/omnitalk) used pkg/logging directly after months of work, and pkg/logging/protolog had zero consumers. Living in the middle was the real cost — service code couldn't tell which package to reach for and 50+ stdlib log.Printf calls sat in AFP because "the migration" was supposed to absorb them. Decision: netlog is the call-site API; pkg/logging is the slog factory used once at startup. Document the split, replace the lingering log.Printf calls in service/afp with netlog.Debug (Warn for the panic recovery), and delete the speculative pkg/logging/protolog package. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
main.go used to write loaded TOML values back into flag pointers so downstream code could uniformly read flag derefs. The pointers then lied about their provenance, and a few EtherTalk flags (desired_network, desired_node) were silently dropped from the TOML path because the writeback block didn't include them. Add flagsToConfig to translate flag values into the same appConfig that loadConfigFromFile produces. main.go now picks one builder, then reads only from cfg. Renames fileConfig to appConfig and incidentally fixes the dropped desired_network/desired_node TOML keys. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Separate factory registry, fork metadata abstractions, and error types into dedicated files so each file has a single concern. - fs.go keeps the FileSystem interface, factory registry, File interface - fork_metadata.go: ForkMetadata, ResourceForkInfo, AppleDoubleMode, ForkMetadataBackend, CommentBackend - errors.go: ErrCopySourceReadEOF, NotSupportedError + helpers Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace ASP's runtime type-assertion that pushed quantum into AFP via SetMaxReadSize with a Transport.MaxReadSize() method that AFP queries after starting each transport. AFP picks the smallest non-zero limit and applies it to its filesystems; DSI reports 0 (uncapped TCP stream). This removes the post-construction mutation pattern, makes the contract explicit on the Transport interface, and lets AFP own when the limit is applied instead of trusting transports to call into it at the right time. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The MacRoman codec is AppleTalk-adjacent but has no AppleTalk state and is reusable outside the project — that places it under pkg/ per the layering rules in ARCHITECTURE.md, not as a top-level package. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Both SPCommand and SPWrite repeated the same six-step prologue: cmdblock size cap → session lookup → activity touch → onActivity → ASP duplicate filter → handler. Extract it so the per-command handlers shrink to just the state-machine logic that distinguishes them. Sets up the explicit write-phase enum and session lifecycle states that follow. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace nil-checks on sess.write with a writePhase enum and beginWrite/endWrite transition methods that assert legal edges. A second SPWrite arriving before the first resolves now logs an invariant violation and is rejected with SPErrorParamErr instead of silently overwriting the in-flight state. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add a sessionState atomic on Session with stateOpen/stateClosing/ stateClosed and a CAS-based markClosing transition. Inbound handlers now check isOpen() at entry and reject traffic destined for a session mid-teardown — previously a CloseSess interleaved with an in-flight Command could touch state during cancellation. Close serialises through a single CAS winner so concurrent close paths are safe. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Replace seqMu + writeMu with a single Session.mu. Both protected state that's only ever touched in microsecond-scale critical sections, and holding two distinct locks for orthogonal sub-states made the concurrency surface harder to reason about than it earned. lastActivity and state stay atomic — the maintenance goroutine reads them without the mutex. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The 5-line CheckDuplicate logic was correct but anonymous, sitting on Session as three loose fields. Promote it to a seqFilter type with an accept method and unit test the edge cases — wraparound, first message, ATP-retransmit-vs-ASP-duplicate distinction — without spinning up a session. Session.CheckDuplicate stays as the locked entrypoint. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The 12-method service.Router lumped datagram I/O, the routing-table index, and the zone-information index into one fat interface that every service had to receive — so any router refactor rippled into 11 services. Split into: DatagramRouter — Route/Reply/PortsList/Zones (every service) RouteIndex — RoutingEntries/Get/Consider/MarkBad/Age (RTMP, ZIP-sending) ZoneIndex — ZonesInNetworkRange/NetworksInZone/AddNetworksToZone (ZIP) service.Router stays as the union so the Service.Start contract is unchanged. Internal stash fields (asp, macip) and helpers (rtmp.makeRoutingTableDatagramData, RoutingTableAgingService) narrow to the smallest capability they actually use, so future widening of Router doesn't ripple through them. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Carve users/nextSRefNum out of Service into a sessionState type with its own mutex. Auth-path code (Login, AddUser) no longer contends with fork, desktop, or volume state on the shared s.mu. This is the smallest of four substruct extractions; further groups (forks, desktop, volume) follow incrementally. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Carve desktopDBs/dtRefs/nextDTRef out of Service into a desktopState type with its own RWMutex. The Desktop database call path (FPOpenDT, FPAddIcon, FPGet/AddAPPL, FPGetComment, etc.) no longer contends with fork, auth, or volume traffic on the shared s.mu. Helpers: desktop.lookup — ref → (db, volID, refExists); db may be nil desktop.lookupDB — strict variant; both ref and db must exist desktop.openRef — registers a new DTRefNum, lazily opens the volume DB desktop.closeRef — invalidates a DTRefNum desktop.dbForVolume — used by ingest/rebuild paths desktop.volumeOf — DTRefNum → volume id only Comment handlers use lookup (they fall back to the metadata backend when no DesktopDB is open). Icon/APPL handlers use lookupDB (they require a real DB to write into). Removes the desktopDBForVolumeLocked / "must hold s.mu" helper; ingest paths now just call s.desktopDBForVolume which is internally synchronised. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Carve forks/nextFork/byteLocks/maxLocks out of Service into a forkState
type with its own RWMutex. Fork I/O (Open/Close/Read/Write/Flush*/
GetForkParms/SetForkParms/ByteRangeLock) is the hottest AFP code path —
every active session touches it on every read or write — so isolating it
from auth, desktop, and volume traffic on a separate lock is the largest
contention win of the four substruct moves.
Helpers:
forks.register — installs a handle, returns new fork id
forks.get — RLock + map lookup; the hot Read/Write probe
forks.close — drops a fork id and evicts its byte-range locks
forks.snapshot — copy of all open handles for the volume-wide FPFlush
forks.lock — write-lock the whole struct (used by ByteRangeLock,
which needs atomicity across handle check + lock-list
scan + insertion)
maxReadSize stays on Service: it is set once at Start, read-only afterwards,
not actually fork state.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…composition #4) Final substruct extraction: volumeBackupDate moves to a backupDates type with its own RWMutex. Volumes / volumeFS / metas / cnidStores stay on Service since they're write-once-after-installVolumes and need no synchronisation; the s.mu.RLock guards over those reads were dead weight. With forkState, sessionState, desktopState, and backupDates each owning their own lock, Service.mu has no remaining users — removed. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two leaks where Stop returned before its goroutines actually exited: 1. macip/etherlink: start() launched readLoop and the gateway-probe via raw `go`s with no WaitGroup. close() closed the link but did not wait, so resolveMAC could still be in flight after the link was nominally torn down. Added a wg, joined it from close(). 2. dsi: the accept loop spawned per-conn handlers tracked in s.wg, but handleConn blocks in io.ReadFull. Closing the listener unblocks Accept, not the in-flight reads, so Stop deadlocked on s.wg.Wait. Tracked accepted conns in a map; Stop now closes them all so handlers return EOF and exit. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Directed-transmit's CTS wait selected on s.stop and st.stop but not s.ctx.Done(). Stop closes both, so the existing path was correct, but callers that cancel the parent ctx without going through Stop (graceful SIGINT in main) had no way to unblock an in-flight CTS retry. Pre-arm s.ctx in New() with a never-cancelled background ctx so handlers reached before Start (test paths that exercise transmit directly) don't dereference a nil ctx; Start replaces it with the real ctx and cancels the placeholder. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three packages were missing the leading "Package X ..." doc comment so go doc and pkg.go.dev surfaced no overview. Added a doc.go in each naming the role and pointing at the spec / where related code lives. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Switch transaction.go to import protocol/atp as patp and reference its exported header, control bits, sizes, and limits rather than package-local duplicates, aligning the runtime layer with the canonical wire-format package. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Introduce BUILD_VARIANT (all|router) in build and package scripts so a single run can produce both the full binary and a slim router-only binary built without AFP/MacIP tags. Release workflow now matrixes each OS over both variants and publishes them as separate archives (omnitalk-* and omnitalk-router-*). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Move cnid_backend, use_decomposed_names, and the default appledouble_mode out of the per-volume table and into [AFP] where they actually live; add the missing desktop_backend and persistent_volume_ids keys; document the per-volume password field. Drops the stale fork_backend reference in the sidecar metadata notes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Four client tests reach macintoshgarden.org and fail on offline CI runners. Add a requireLiveTests helper that skips them unless OMNITALK_LIVE_TESTS=1 is set, so PR CI stays green while the tests remain runnable locally on demand. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
golangci-lint-action v6 pins golangci-lint to the v1 line (1.64.8),
which rejects our v2-style .golangci.yml ("linters.default" and the
top-level "version" key). v8 supports golangci-lint v2. Drop the
deprecated --timeout flag; the timeout is already set in config.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Author
|
Linting issues - will merge. |
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
Large cleanup branch (113 commits) consolidating several long-running threads of work.
Wire-format / protocol split
protocol/{atp,asp,zip,rtmp,aep,nbp,llap}packages own pure wire format; service packages now consume them as canonical types (renamedATPHeader→Header,DDPTypeATP→DDPType, etc.).MarshalWire/WireSizewith golden hex tests; volume/info/fork packing routed throughbinutil.pkg/encoding,pkg/appledouble,pkg/telemetrylifted out of service code.Build-tag gating + variants
//go:buildgates forafp,macgarden,macip,sqlite_cnid, plus a unifyingalltag.omnitalk-*(full) andomnitalk-router-*archives per OS.Lifecycle / concurrency hardening
Startparent; long-running waits honour shutdown signals.WaitGroupacross macip, dsi, asp, llap, rtmp, zip.errors.Joinfor aggregated Stop errors; LLAP CTS wait honours ctx cancellation.AFP refactor
sessionState,desktopState,forkState,backupDatessubstructs;Service.muremoved.HandleCommandswitch replaced with a command registry;fs.gosplit by concern.pkg/appledouble; macgarden VFS moved out of core AFP.Config + logging
netlogformalised as the project logging API.Tests + docs
ARCHITECTURE.mdadded; package doc comments throughout.Test plan